feat: support for sdk #920
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a comprehensive SDK API system with robot CRUD operations, execution, and run management alongside workflow enrichment utilities. Adds browser-side page analysis for selector generation and pagination detection, LLM-based workflow generation, selector validation, and updates scheduling signature by removing userId parameter. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SDK API
participant RemoteBrowser
participant PageAnalyzer
participant LLM
participant SelectorValidator
participant Storage
Client->>SDK API: POST /sdk/robots/:id/execute
SDK API->>RemoteBrowser: createRemoteBrowserForValidation(userId)
RemoteBrowser-->>SDK API: page, browserId
alt Workflow Generation from Prompt
SDK API->>PageAnalyzer: analyzeElementGroups()
PageAnalyzer-->>SDK API: element groups
SDK API->>LLM: getLLMDecisionWithVision(groups, prompt)
LLM-->>SDK API: decision (actionType, selectedGroup)
SDK API->>SelectorValidator: auto-detect fields & pagination
SelectorValidator-->>SDK API: enriched selectors
else Workflow Validation
SDK API->>SelectorValidator: validateSelectors(workflow)
SelectorValidator->>PageAnalyzer: evaluateSelector()
PageAnalyzer-->>SelectorValidator: matching elements
SelectorValidator-->>SDK API: enriched workflow
end
SDK API->>Storage: scheduleWorkflow(id, cronExpression, timezone)
Storage-->>SDK API: scheduled
SDK API->>RemoteBrowser: cleanup & close
SDK API-->>Client: run ID & status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RohitR311
added
Type: Feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/storage/schedule.ts (1)
9-16: JSDoc parameter mismatch with function signature.The
@param userIddocumentation at line 12 still references the removeduserIdparameter that no longer exists in the function signature./** * Utility function to schedule a cron job using PgBoss * @param id The robot ID - * @param userId The user ID * @param cronExpression The cron expression for scheduling * @param timezone The timezone for the cron expression */
🟡 Minor comments (8)
server/src/browser-management/controller.ts-487-494 (1)
487-494: Cleanup may fail if browser not yet in pool.If initialization fails before
addRemoteBrowser(line 472), callingdestroyRemoteBrowserwon't find the browser in the pool. Consider usingbrowserSession.switchOff()directly for cleanup, similar to error handling ininitializeBrowserAsync(lines 387-392).} catch (error: any) { logger.log('error', `Failed to create validation browser ${id}: ${error.message}`); try { - await destroyRemoteBrowser(id, userId); + // Browser may not be in pool yet, so try direct cleanup + if (browserPool.getRemoteBrowser(id)) { + await destroyRemoteBrowser(id, userId); + } } catch (cleanupError) { logger.log('warn', `Failed to cleanup browser ${id}: ${cleanupError}`); } throw error; }Committable suggestion skipped: line range outside the PR's diff.
server/src/sdk/browserSide/pageAnalyzer.js-1506-1508 (1)
1506-1508: Regex patterns contain potentially unintended control characters.The static analysis flagged line 1552, but the issue starts with these regex definitions. The character
\x20is an explicit space which is fine, but\x0B(vertical tab) at line 1552 is unusual and may be unintended.Verify that
\x0B(vertical tab) is intentionally included in the whitespace check at line 1552:} else if (/[\t\n\f\r\x0B]/.test(character)) {If intentional, add a comment explaining why vertical tab is included. If not, consider using a simpler pattern:
- } else if (/[\t\n\f\r\x0B]/.test(character)) { + } else if (/\s/.test(character) && !/[ ]/.test(character)) {server/src/sdk/browserSide/pageAnalyzer.js-209-209 (1)
209-209: Missing semicolon after array declaration.- const childSelectors = Array.from(new Set(allChildSelectors)).sort() + const childSelectors = Array.from(new Set(allChildSelectors)).sort();server/src/sdk/workflowEnricher.ts-173-190 (1)
173-190: Silently continuing on auto-detect failure may lead to incomplete workflows.When
autoDetectListFieldsfails or returns empty fields, the action is skipped but the workflow continues. This could result in a "successful" enrichment that's actually incomplete.Consider tracking skipped actions or providing more detailed feedback:
if (!autoDetectResult.success || !autoDetectResult.fields || Object.keys(autoDetectResult.fields).length === 0) { errors.push(autoDetectResult.error || 'Failed to auto-detect fields from list selector'); + logger.warn(`Skipping scrapeList action due to auto-detect failure for selector: ${config.itemSelector}`); continue; }server/src/sdk/browserSide/pageAnalyzer.js-1747-1751 (1)
1747-1751:generateMandatoryCSSFallbackcreates random data-mx-id which may conflict.Using
Math.floor(Math.random() * 10000)could generate duplicate IDs on pages with many elements. Consider using a counter or UUID approach.+ let mxIdCounter = 0; function generateMandatoryCSSFallback(element) { - const mxId = Math.floor(Math.random() * 10000).toString(); + const mxId = (++mxIdCounter).toString(); element.setAttribute('data-mx-id', mxId); return element.tagName.toLowerCase() + '[data-mx-id="' + mxId + '"]'; }server/src/sdk/browserSide/pageAnalyzer.js-97-108 (1)
97-108: Attribute selector parsing doesn't handle all quote styles.The regex
/([^=]+)="([^"]+)"/only matches double-quoted attribute values. Single-quoted values like[attr='value']won't be parsed correctly.- const eqMatch = content.match(/([^=]+)="([^"]+)"/); + const eqMatch = content.match(/([^=]+)=["']([^"']+)["']/);server/src/sdk/browserSide/pageAnalyzer.js-962-993 (1)
962-993: Parent-child duplicate removal has a logic issue.When a candidate contains an existing element,
shouldIncludeis set tofalse, but then lines 983-985 unconditionally set it back totruefor<a>or<img>tags. This could lead to including both parent and child anchor/image elements.if (tagName === 'a' || tagName === 'img') { - shouldInclude = true; + // Only force include if we haven't found a containment relationship + if (shouldInclude === false && !candidates.some(c => + candidate.element.contains(c.element) || c.element.contains(candidate.element) + )) { + shouldInclude = true; + } }Committable suggestion skipped: line range outside the PR's diff.
server/src/sdk/workflowEnricher.ts-45-56 (1)
45-56: Potential issue with regex URL extraction.When the URL is a regex object (
{ $regex: string }), extracting just the$regexvalue may not yield a valid navigable URL. Regex patterns often contain special characters that won't work as URLs.Consider validating that the extracted URL is actually navigable:
if (rawUrl && rawUrl !== 'about:blank') { - url = typeof rawUrl === 'string' ? rawUrl : rawUrl.$regex; + url = typeof rawUrl === 'string' ? rawUrl : rawUrl.$regex; + // Validate URL is navigable (regex patterns may not be valid URLs) + try { + new URL(url); + } catch { + url = undefined; + continue; + } break; }
🧹 Nitpick comments (11)
server/src/sdk/selectorValidator.ts (2)
230-233: Use async file read to avoid blocking the event loop.
fs.readFileSyncblocks the event loop. In an async method, preferfs.promises.readFileor cache the script content at module load time.+import { promises as fsPromises } from 'fs'; + +// Option 1: Async read - const fs = require('fs'); - const path = require('path'); - const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); - const scriptContent = fs.readFileSync(scriptPath, 'utf8'); + const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); + const scriptContent = await fsPromises.readFile(scriptPath, 'utf8');Or better, cache the script content at module initialization since it doesn't change at runtime.
569-575: Method nameclose()is misleading - only clears reference.The
close()method only setsthis.page = nullbut doesn't actually close the page or browser. This could mislead callers into thinking resources are released. Consider renaming toclearPage()ordetach(), or document that the caller is responsible for browser cleanup./** - * Clear page reference + * Clear page reference. Note: Does NOT close the browser/page. + * Caller is responsible for browser lifecycle management via destroyRemoteBrowser(). */ - async close(): Promise<void> { + async detach(): Promise<void> { this.page = null; - logger.info('Page reference cleared'); + logger.info('Page reference detached from validator'); }server/src/api/sdk.ts (1)
470-492: Unusedintervalparameter in function call.The
waitForRunCompletionfunction accepts anintervalparameter, but the call at line 427 only passesrunId. The default value works, but consider if the interval should be configurable per-call or if the parameter should be removed.server/src/sdk/workflowEnricher.ts (3)
13-18: Consider using a discriminated union instead ofstring | typeof Symbol.asyncDispose.The
actionfield typestring | typeof Symbol.asyncDisposeis unusual. IfSymbol.asyncDisposeis a valid action type, consider using a more explicit union type or documenting why this combination is needed. The code at line 77-79 already filters non-string actions, suggestingSymbol.asyncDisposemay not be intentionally handled.
352-355: Synchronous file read on potential hot path.Using
fs.readFileSyncin an async method blocks the event loop. Consider caching the script content or using async file reading.+ private static cachedPageAnalyzerScript: string | null = null; + + private static async getPageAnalyzerScript(): Promise<string> { + if (!this.cachedPageAnalyzerScript) { + const fs = require('fs').promises; + const path = require('path'); + const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); + this.cachedPageAnalyzerScript = await fs.readFile(scriptPath, 'utf8'); + } + return this.cachedPageAnalyzerScript; + }
683-684: Consider making the default limit configurable.The default limit of 100 is hardcoded. Users may want different defaults based on their use case.
- const limit = llmDecision.limit || 100; + const DEFAULT_LIST_LIMIT = 100; + const limit = llmDecision.limit || DEFAULT_LIST_LIMIT;Or accept it as a parameter to allow customization.
server/src/sdk/browserSide/pageAnalyzer.js (5)
44-73: CSS to XPath conversion may not handle all CSS selector features.The conversion handles tags, IDs, classes, and attribute selectors but doesn't support pseudo-classes (
:first-child,:nth-child), pseudo-elements, or combinators beyond>and descendant. This is acceptable for the use case but worth documenting.Consider adding a comment:
+ /** + * Convert CSS selector to XPath + * Note: Supports basic selectors (tag, #id, .class, [attr], descendant, child) + * Does not support pseudo-classes, pseudo-elements, or sibling combinators + */ function cssToXPath(cssSelector) {
139-154: XPath modification logic has redundant conditions.Lines 140-143 check for selector ending with
]twice with nearly identical logic. The condition at line 143 (xpathSelector.replace(/\]$/, ...)) assumes the selector ends with]but the check at line 142 already handles that case.Simplify the XPath modification:
if (uniqueCounts.length > 1 && childCounts.filter(c => c === 1).length > childCounts.length / 2) { - if (xpathSelector.includes('[') && xpathSelector.endsWith(']')) { - xpathSelector = xpathSelector.slice(0, -1) + ' and count(*)=1]'; - } else if (xpathSelector.includes('[')) { - xpathSelector = xpathSelector.replace(/\]$/, ' and count(*)=1]'); - } else { + if (xpathSelector.endsWith(']')) { + // Has existing predicate - append to it + xpathSelector = xpathSelector.slice(0, -1) + ' and count(*)=1]'; + } else { + // No existing predicate - add new one const lastSlash = xpathSelector.lastIndexOf('/'); if (lastSlash !== -1) { const beforeTag = xpathSelector.substring(0, lastSlash + 1); const tag = xpathSelector.substring(lastSlash + 1); xpathSelector = beforeTag + tag + '[count(*)=1]'; } else { xpathSelector = xpathSelector + '[count(*)=1]'; } } }
1145-1199: Large inline implementation of finder algorithm.The selector generation logic (based on @medv/finder) is comprehensive but adds significant complexity. Consider extracting this into a separate file if it needs maintenance.
The code is functional, but for maintainability, consider:
- Adding a reference comment to the original library version
- Extracting to a separate
finderAlgorithm.jsif changes are anticipated
2217-2231:normalizeClassesis duplicated from line 603.This function is defined twice in the file - once at the module level (line 603) and again inside
analyzeElementGroups. Consider removing the duplicate.Remove the inline definition and use the module-level function:
window.analyzeElementGroups = function() { try { - const normalizeClasses = (classList) => { - return Array.from(classList) - .filter((cls) => { - return ( - !cls.match(/\d{3,}|uuid|hash|id-|_\d+$/i) && - !cls.startsWith('_ngcontent-') && - !cls.startsWith('_nghost-') && - !cls.match(/^ng-tns-c\d+-\d+$/) - ); - }) - .sort() - .join(' '); - }; + // Uses module-level normalizeClasses function
886-888: Silent error swallowing in loop.The empty catch block hides errors that might help debug issues with selector evaluation.
Consider logging at debug level:
} catch (error) { + // Selector evaluation failed - continue with next selector }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/src/api/sdk.ts(1 hunks)server/src/browser-management/controller.ts(2 hunks)server/src/routes/storage.ts(1 hunks)server/src/sdk/browserSide/pageAnalyzer.js(1 hunks)server/src/sdk/selectorValidator.ts(1 hunks)server/src/sdk/workflowEnricher.ts(1 hunks)server/src/storage/schedule.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
server/src/routes/storage.ts (1)
server/src/storage/schedule.ts (1)
scheduleWorkflow(16-37)
server/src/sdk/selectorValidator.ts (2)
server/src/sdk/browserSide/pageAnalyzer.js (29)
isXPath(14-14)selector(2138-2138)selector(2146-2146)selector(2161-2161)selector(2169-2169)element(291-291)element(847-847)element(1812-1812)element(2044-2044)el(2105-2105)result(17-23)result(454-454)result(813-819)result(1123-1123)doc(809-811)elements(25-25)elements(705-705)elements(821-821)elements(843-843)elements(1088-1088)i(26-26)i(54-54)i(196-196)i(234-234)i(315-315)i(328-328)i(822-822)i(2104-2104)i(2471-2471)src/helpers/clientPaginationDetector.ts (1)
evaluateSelector(290-318)
server/src/sdk/workflowEnricher.ts (2)
server/src/sdk/selectorValidator.ts (1)
SelectorValidator(27-576)server/src/browser-management/controller.ts (2)
createRemoteBrowserForValidation(445-496)destroyRemoteBrowser(124-200)
server/src/sdk/browserSide/pageAnalyzer.js (3)
src/helpers/clientPaginationDetector.ts (5)
evaluateSelector(290-318)matchesAnyPattern(354-356)getClickableElements(323-333)isVisible(338-349)isNearList(361-389)src/helpers/clientSelectorGenerator.ts (16)
generateOptimizedChildXPaths(2668-2710)getAllDescendantsIncludingShadow(2597-2666)buildOptimizedAbsoluteXPath(2839-2863)isMeaningfulElement(585-608)getOptimizedStructuralPath(2866-2936)elementContains(3074-3093)getCommonClassesAcrossLists(2996-3071)queryElementsInScope(2796-2807)generateOptimizedStructuralStep(2712-2782)getSiblingPosition(2785-2793)isAttributeCommonAcrossLists(2938-2964)getElementPath(2966-2977)findCorrespondingElement(2979-2994)isInShadowDOM(2810-2812)deepQuerySelectorAll(2815-2837)getDeepestElementFromPoint(3984-4013)maxun-core/src/browserSide/scraper.js (2)
attrValue(927-927)shadowRoot(992-992)
🪛 Biome (2.1.2)
server/src/sdk/selectorValidator.ts
[error] 236-236: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 301-301: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
server/src/sdk/workflowEnricher.ts
[error] 358-358: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
server/src/sdk/browserSide/pageAnalyzer.js
[error] 1552-1552: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (11)
server/src/routes/storage.ts (1)
897-897: LGTM!The call to
scheduleWorkflowcorrectly aligns with the updated function signature that no longer requiresuserId.server/src/api/sdk.ts (1)
31-117: LGTM - robot creation logic is sound.The robot creation endpoint properly validates input, handles both 'scrape' and 'extract' types, enriches workflows with WorkflowEnricher.enrichWorkflow, and persists the robot. Error handling and telemetry capture are appropriate.
server/src/sdk/workflowEnricher.ts (5)
34-43: LGTM on the method signature and initial validation.Good defensive check for empty workflow with clear error message.
93-93: Sensitive data encryption looks correct, but verify encryption is reversible when needed.The value is encrypted before storage. Ensure downstream consumers (workflow execution) have access to the decryption mechanism to use these values.
572-584: Defensive JSON parsing is good but could be simplified.The multi-pattern JSON extraction handles markdown code blocks well. Consider adding a log when fallback parsing is used for debugging.
612-639: LGTM on fallback heuristic.The scoring approach is reasonable: keyword matching plus a bonus for larger groups. The word length filter of 3+ characters helps avoid noise from short common words.
705-707: Good defensive error for unsupported action types.Clear error message helps with debugging when LLM returns unexpected results.
server/src/sdk/browserSide/pageAnalyzer.js (4)
5-6: Good encapsulation using IIFE pattern.The immediately-invoked function expression with 'use strict' properly encapsulates the code and avoids polluting the global scope beyond the intended window exports.
174-177: Good use of WeakMap for element caches.Using WeakMap for
pathCache,descendantsCache, andmeaningfulCacheensures that element references don't prevent garbage collection.
272-341: LGTM on descendant traversal with appropriate limits.The BFS implementation with configurable limits (
MAX_MEANINGFUL_ELEMENTS,MAX_NODES_TO_CHECK,MAX_DEPTH) prevents performance issues on complex DOM structures. Shadow DOM handling is properly included.
2321-2369: Similarity calculation is well-designed with weighted scoring.The
calculateSimilarityfunction uses appropriate weights for different aspects (tag name: 10, classes: 8, children structure: 8, attributes: 5, depth: 2, text: 3) and has a smart early return when tag names don't match.
| router.get("/sdk/robots", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robots = await Robot.findAll(); | ||
|
|
||
| return res.status(200).json({ | ||
| data: robots | ||
| }); | ||
| } catch (error: any) { | ||
| logger.error("[SDK] Error listing robots:", error); | ||
| return res.status(500).json({ | ||
| error: "Failed to list robots", | ||
| message: error.message | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing user filtering - returns all users' robots.
The endpoint returns all robots from the database without filtering by the authenticated user. This is a potential data exposure issue where users could see other users' robots.
router.get("/sdk/robots", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => {
try {
- const robots = await Robot.findAll();
+ const robots = await Robot.findAll({
+ where: {
+ userId: req.user.id
+ }
+ });
return res.status(200).json({
data: robots
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.get("/sdk/robots", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robots = await Robot.findAll(); | |
| return res.status(200).json({ | |
| data: robots | |
| }); | |
| } catch (error: any) { | |
| logger.error("[SDK] Error listing robots:", error); | |
| return res.status(500).json({ | |
| error: "Failed to list robots", | |
| message: error.message | |
| }); | |
| } | |
| router.get("/sdk/robots", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robots = await Robot.findAll({ | |
| where: { | |
| userId: req.user.id | |
| } | |
| }); | |
| return res.status(200).json({ | |
| data: robots | |
| }); | |
| } catch (error: any) { | |
| logger.error("[SDK] Error listing robots:", error); | |
| return res.status(500).json({ | |
| error: "Failed to list robots", | |
| message: error.message | |
| }); | |
| } |
| let cronExpression; | ||
| const dayIndex = days.indexOf(startFrom); | ||
|
|
||
| switch (runEveryUnit) { | ||
| case 'MINUTES': | ||
| cronExpression = `*/${runEvery} * * * *`; | ||
| break; | ||
| case 'HOURS': | ||
| cronExpression = `${startMinutes} */${runEvery} * * *`; | ||
| break; | ||
| case 'DAYS': | ||
| cronExpression = `${startMinutes} ${startHours} */${runEvery} * *`; | ||
| break; | ||
| case 'WEEKS': | ||
| cronExpression = `${startMinutes} ${startHours} * * ${dayIndex}`; | ||
| break; | ||
| case 'MONTHS': | ||
| cronExpression = `${startMinutes} ${startHours} ${dayOfMonth} */${runEvery} *`; | ||
| if (startFrom !== 'SUNDAY') { | ||
| cronExpression += ` ${dayIndex}`; | ||
| } | ||
| break; | ||
| default: | ||
| return res.status(400).json({ | ||
| error: "Invalid runEveryUnit. Must be one of: MINUTES, HOURS, DAYS, WEEKS, MONTHS" | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated cron expression generation logic.
This switch statement for generating cron expressions from schedule parameters is duplicated from server/src/routes/storage.ts (lines 855-880). Extract to a shared utility function in server/src/utils/schedule.ts to ensure consistency and reduce maintenance burden.
// In server/src/utils/schedule.ts
export function buildCronExpression(
runEvery: number,
runEveryUnit: string,
startMinutes: number,
startHours: number,
dayIndex: number,
dayOfMonth: number,
startFrom: string
): string | null {
switch (runEveryUnit) {
case 'MINUTES':
return `*/${runEvery} * * * *`;
case 'HOURS':
return `${startMinutes} */${runEvery} * * *`;
// ... etc
}
}🤖 Prompt for AI Agents
In server/src/api/sdk.ts lines 266-292, the cron-expression switch is duplicated
elsewhere; extract this logic into a new helper in server/src/utils/schedule.ts
(exported function buildCronExpression) that takes the schedule params
(runEvery, runEveryUnit, startMinutes, startHours, dayIndex or startFrom,
dayOfMonth) and returns the cron string or null on invalid unit; replace the
switch in this file with a call to buildCronExpression (import it), compute any
required inputs (e.g. dayIndex) before calling, and if the helper returns null
keep the existing res.status(400).json(...) behavior; also update the duplicate
location (server/src/routes/storage.ts) to call the same helper so both places
share the single implementation.
| if (run.status !== 'running' && run.status !== 'queued') { | ||
| return res.status(400).json({ | ||
| error: "Run is not in a state that can be aborted", | ||
| currentStatus: run.status | ||
| }); | ||
| } | ||
|
|
||
| await run.update({ status: 'aborted' }); | ||
|
|
||
| logger.info(`[SDK] Run ${runId} marked for abortion`); | ||
|
|
||
| return res.status(200).json({ | ||
| message: "Run abortion initiated", | ||
| data: run | ||
| }); |
There was a problem hiding this comment.
Incomplete abort implementation - doesn't stop interpreter.
Unlike the abort endpoint in storage.ts (lines 1042-1067), this SDK abort only updates the status to 'aborted' without actually stopping the interpreter or cleaning up the browser. This may leave orphaned browser instances running.
Consider calling the interpreter stop logic similar to the main route:
// After updating status, also stop the interpreter
const browser = browserPool.getRemoteBrowser(run.browserId);
if (browser && browser.interpreter) {
await browser.interpreter.stopInterpretation();
}
// Also queue cleanup job like storage.ts does| export const createRemoteBrowserForValidation = async ( | ||
| userId: string | ||
| ): Promise<{ browserId: string; page: Page }> => { | ||
| const id = uuid(); | ||
|
|
||
| logger.log('info', `Creating validation browser ${id} for user ${userId}`); | ||
|
|
||
| try { | ||
| const dummySocket = { | ||
| emit: (event: string, data?: any) => { | ||
| logger.log('debug', `Browser ${id} emitted ${event}`); | ||
| }, | ||
| on: () => {}, | ||
| off: () => {}, | ||
| id: `validation-${id}`, | ||
| } as any; | ||
|
|
||
| const browserSession = new RemoteBrowser(dummySocket, userId, id); | ||
|
|
||
| const VALIDATION_INIT_TIMEOUT = 45000; | ||
| const initPromise = browserSession.initialize(userId); | ||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => reject(new Error('Validation browser initialization timeout')), VALIDATION_INIT_TIMEOUT); | ||
| }); | ||
|
|
||
| await Promise.race([initPromise, timeoutPromise]); | ||
|
|
||
| const added = browserPool.addRemoteBrowser(id, browserSession, userId, true, 'run'); | ||
| if (!added) { | ||
| await browserSession.switchOff(); | ||
| throw new Error('Failed to add validation browser to pool'); | ||
| } |
There was a problem hiding this comment.
Consider reserving browser slot atomically before initialization.
Unlike createRemoteBrowserForRun (lines 92-115) which uses browserPool.reserveBrowserSlotAtomic() before initialization to prevent race conditions and enforce limits, this function directly initializes and adds the browser. This could allow exceeding browser limits under concurrent validation requests.
Consider adding slot reservation:
export const createRemoteBrowserForValidation = async (
userId: string
): Promise<{ browserId: string; page: Page }> => {
const id = uuid();
logger.log('info', `Creating validation browser ${id} for user ${userId}`);
+ const slotReserved = browserPool.reserveBrowserSlotAtomic(id, userId, "run");
+ if (!slotReserved) {
+ throw new Error('No available browser slots for validation');
+ }
+
try {
// ... initialization code ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createRemoteBrowserForValidation = async ( | |
| userId: string | |
| ): Promise<{ browserId: string; page: Page }> => { | |
| const id = uuid(); | |
| logger.log('info', `Creating validation browser ${id} for user ${userId}`); | |
| try { | |
| const dummySocket = { | |
| emit: (event: string, data?: any) => { | |
| logger.log('debug', `Browser ${id} emitted ${event}`); | |
| }, | |
| on: () => {}, | |
| off: () => {}, | |
| id: `validation-${id}`, | |
| } as any; | |
| const browserSession = new RemoteBrowser(dummySocket, userId, id); | |
| const VALIDATION_INIT_TIMEOUT = 45000; | |
| const initPromise = browserSession.initialize(userId); | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| setTimeout(() => reject(new Error('Validation browser initialization timeout')), VALIDATION_INIT_TIMEOUT); | |
| }); | |
| await Promise.race([initPromise, timeoutPromise]); | |
| const added = browserPool.addRemoteBrowser(id, browserSession, userId, true, 'run'); | |
| if (!added) { | |
| await browserSession.switchOff(); | |
| throw new Error('Failed to add validation browser to pool'); | |
| } | |
| export const createRemoteBrowserForValidation = async ( | |
| userId: string | |
| ): Promise<{ browserId: string; page: Page }> => { | |
| const id = uuid(); | |
| logger.log('info', `Creating validation browser ${id} for user ${userId}`); | |
| const slotReserved = browserPool.reserveBrowserSlotAtomic(id, userId, "run"); | |
| if (!slotReserved) { | |
| throw new Error('No available browser slots for validation'); | |
| } | |
| try { | |
| const dummySocket = { | |
| emit: (event: string, data?: any) => { | |
| logger.log('debug', `Browser ${id} emitted ${event}`); | |
| }, | |
| on: () => {}, | |
| off: () => {}, | |
| id: `validation-${id}`, | |
| } as any; | |
| const browserSession = new RemoteBrowser(dummySocket, userId, id); | |
| const VALIDATION_INIT_TIMEOUT = 45000; | |
| const initPromise = browserSession.initialize(userId); | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| setTimeout(() => reject(new Error('Validation browser initialization timeout')), VALIDATION_INIT_TIMEOUT); | |
| }); | |
| await Promise.race([initPromise, timeoutPromise]); | |
| const added = browserPool.addRemoteBrowser(id, browserSession, userId, true, 'run'); | |
| if (!added) { | |
| await browserSession.switchOff(); | |
| throw new Error('Failed to add validation browser to pool'); | |
| } |
| await validator.initialize(page as any, url); | ||
|
|
||
| const validatorPage = (validator as any).page; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unnecessary type cast and private property access.
Line 299 casts page to any unnecessarily, and line 301 accesses the private page property via type assertion. The page variable from line 296 should be used directly.
- await validator.initialize(page as any, url);
-
- const validatorPage = (validator as any).page;
- const screenshotBuffer = await page.screenshot({ fullPage: true, type: 'png' });
+ await validator.initialize(page, url);
+
+ const screenshotBuffer = await page.screenshot({ fullPage: true, type: 'png' });And update line 308:
- const pageHTML = await validatorPage.content();
+ const pageHTML = await page.content();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 299–301 (and update line
308), remove the unnecessary type cast and private property access: call
validator.initialize(page, url) without casting page to any, delete the
extraction of (validator as any).page, and replace uses of validatorPage on line
308 with the original page variable; ensure types align by adjusting the
validator.initialize signature or the variable types if needed rather than
resorting to any or private property access.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); | ||
| const scriptContent = fs.readFileSync(scriptPath, 'utf8'); | ||
|
|
||
| await page.evaluate((script: string) => { | ||
| eval(script); | ||
| }, scriptContent); |
There was a problem hiding this comment.
Security risk: Avoid using eval() inside page.evaluate().
The static analysis correctly flags this as a security concern. While the script is read from the server's filesystem, using eval() in the browser context is risky and prevents proper error stack traces.
Use page.addScriptTag() instead:
- await page.evaluate((script: string) => {
- eval(script);
- }, scriptContent);
+ await page.addScriptTag({ content: scriptContent });Alternatively, if you need to ensure the script is executed before continuing:
+ await page.addScriptTag({ content: scriptContent });
+ await page.waitForFunction(() => typeof (window as any).analyzeElementGroups === 'function');
- await page.evaluate((script: string) => {
- eval(script);
- }, scriptContent);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fs = require('fs'); | |
| const path = require('path'); | |
| const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); | |
| const scriptContent = fs.readFileSync(scriptPath, 'utf8'); | |
| await page.evaluate((script: string) => { | |
| eval(script); | |
| }, scriptContent); | |
| const fs = require('fs'); | |
| const path = require('path'); | |
| const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); | |
| const scriptContent = fs.readFileSync(scriptPath, 'utf8'); | |
| await page.addScriptTag({ content: scriptContent }); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 358-358: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 352 to 359, reading a local
JS file and invoking it inside page.evaluate via eval introduces security and
debugging problems; replace this pattern by injecting the script into the page
with Puppeteer’s page.addScriptTag rather than eval: read the file as before but
call await page.addScriptTag({ content: scriptContent }) (or addScriptTag({ url:
'...' }) if serving the file over HTTP), then await any initialization exported
by that script or use page.evaluate to call a known function it defines; also
add try/catch around the read-and-inject steps and surface errors via logger so
failures have proper stack traces.
| const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { | ||
| model: ollamaModel, | ||
| messages: [ | ||
| { | ||
| role: 'system', | ||
| content: systemPrompt | ||
| }, | ||
| { | ||
| role: 'user', | ||
| content: userPrompt, | ||
| images: [screenshotBase64] | ||
| } | ||
| ], | ||
| stream: false, | ||
| format: jsonSchema, | ||
| options: { | ||
| temperature: 0.1 | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing timeout on Ollama API request.
The axios call to Ollama has no timeout configured. If the LLM service is slow or unresponsive, this could hang indefinitely.
const response = await axios.post(`${ollamaBaseUrl}/api/chat`, {
model: ollamaModel,
messages: [
// ...
],
stream: false,
format: jsonSchema,
options: {
temperature: 0.1
}
+ }, {
+ timeout: 120000 // 2 minute timeout for LLM inference
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { | |
| model: ollamaModel, | |
| messages: [ | |
| { | |
| role: 'system', | |
| content: systemPrompt | |
| }, | |
| { | |
| role: 'user', | |
| content: userPrompt, | |
| images: [screenshotBase64] | |
| } | |
| ], | |
| stream: false, | |
| format: jsonSchema, | |
| options: { | |
| temperature: 0.1 | |
| } | |
| }); | |
| const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { | |
| model: ollamaModel, | |
| messages: [ | |
| { | |
| role: 'system', | |
| content: systemPrompt | |
| }, | |
| { | |
| role: 'user', | |
| content: userPrompt, | |
| images: [screenshotBase64] | |
| } | |
| ], | |
| stream: false, | |
| format: jsonSchema, | |
| options: { | |
| temperature: 0.1 | |
| } | |
| }, { | |
| timeout: 120000 // 2 minute timeout for LLM inference | |
| }); |
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 474-492, the axios.post call
to the Ollama API has no timeout configured which can cause indefinite hangs;
add a request timeout by passing an axios config object as the third argument
(e.g., { timeout: 10000 }) to axios.post or create an axios instance with a
default timeout, and ensure the surrounding call remains in the existing
async/try-catch so timeout errors are propagated/handled consistently.
| const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { | ||
| model: openaiModel, | ||
| messages: [ | ||
| { | ||
| role: 'system', | ||
| content: systemPrompt | ||
| }, | ||
| { | ||
| role: 'user', | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: userPrompt | ||
| }, | ||
| { | ||
| type: 'image_url', | ||
| image_url: { | ||
| url: `data:image/png;base64,${screenshotBase64}` | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| max_tokens: 1024, | ||
| temperature: 0.1 | ||
| }, { | ||
| headers: { | ||
| 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing timeout on OpenAI API request.
Similar to the Ollama call, the OpenAI request lacks a timeout.
const response = await axios.post(`${openaiBaseUrl}/chat/completions`, {
// ...
}, {
headers: {
'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`,
'Content-Type': 'application/json'
- }
+ },
+ timeout: 120000 // 2 minute timeout
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { | |
| model: openaiModel, | |
| messages: [ | |
| { | |
| role: 'system', | |
| content: systemPrompt | |
| }, | |
| { | |
| role: 'user', | |
| content: [ | |
| { | |
| type: 'text', | |
| text: userPrompt | |
| }, | |
| { | |
| type: 'image_url', | |
| image_url: { | |
| url: `data:image/png;base64,${screenshotBase64}` | |
| } | |
| } | |
| ] | |
| } | |
| ], | |
| max_tokens: 1024, | |
| temperature: 0.1 | |
| }, { | |
| headers: { | |
| 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, | |
| 'Content-Type': 'application/json' | |
| } | |
| }); | |
| const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { | |
| model: openaiModel, | |
| messages: [ | |
| { | |
| role: 'system', | |
| content: systemPrompt | |
| }, | |
| { | |
| role: 'user', | |
| content: [ | |
| { | |
| type: 'text', | |
| text: userPrompt | |
| }, | |
| { | |
| type: 'image_url', | |
| image_url: { | |
| url: `data:image/png;base64,${screenshotBase64}` | |
| } | |
| } | |
| ] | |
| } | |
| ], | |
| max_tokens: 1024, | |
| temperature: 0.1 | |
| }, { | |
| headers: { | |
| 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, | |
| 'Content-Type': 'application/json' | |
| }, | |
| timeout: 120000 // 2 minute timeout | |
| }); |
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 532 to 562, the axios POST to
the OpenAI chat completions endpoint is missing a request timeout; add a timeout
option to the axios request config (use llmConfig.timeout if available or fall
back to a sensible default like 30_000 ms), e.g. include timeout:
llmConfig?.timeout ?? 30000 in the third argument along with headers, and ensure
any callers or surrounding error handling can handle a timeout error
consistently with the Ollama call.
| await pgBossClient.schedule(queueName, cronExpression, | ||
| { id, runId, userId }, | ||
| { id, runId }, | ||
| { tz: timezone } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for scheduled-workflow handlers and related code
rg -n -C5 'scheduled-workflow' --type tsRepository: getmaxun/maxun
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Search for pgBoss job handlers and their usage of the payload
rg -n 'pgBoss\.subscribe' --type ts -A 10Repository: getmaxun/maxun
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for usages of userId in job handlers or scheduled contexts
rg -n -C3 'userId' server/src/storage/schedule.tsRepository: getmaxun/maxun
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Look for the queue name and any handlers that process it
rg -n 'queueName\|SCHEDULED' --type ts | head -30Repository: getmaxun/maxun
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for pgBoss usage patterns more broadly
rg -n 'pgBoss' --type ts | head -40Repository: getmaxun/maxun
Length of output: 3915
🏁 Script executed:
#!/bin/bash
# Search for files that might contain job handlers or workers
fd -e ts | xargs rg -l 'subscribe\|handler' | head -20Repository: getmaxun/maxun
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look for the actual function signature before and after to understand the change
cat -n server/src/storage/schedule.ts | head -35Repository: getmaxun/maxun
Length of output: 1497
🏁 Script executed:
#!/bin/bash
# Search for where this function is called to understand context
rg -n 'scheduleWorkflow' --type ts -B 2 -A 2Repository: getmaxun/maxun
Length of output: 1923
🏁 Script executed:
#!/bin/bash
# Check the schedule-worker.ts handler implementation
cat -n server/src/schedule-worker.ts | sed -n '90,130p'Repository: getmaxun/maxun
Length of output: 1844
🏁 Script executed:
#!/bin/bash
# Search for ScheduledWorkflowData type definition
rg -n 'ScheduledWorkflowData' --type ts -B 3 -A 5Repository: getmaxun/maxun
Length of output: 1796
🏁 Script executed:
#!/bin/bash
# Check if userId is referenced anywhere in schedule-worker.ts
rg -n 'userId' server/src/schedule-worker.tsRepository: getmaxun/maxun
Length of output: 285
Critical: Missing userId in scheduled job payload causes handler failure.
The ScheduledWorkflowData interface in schedule-worker.ts (lines 24–28) requires userId, and the handler destructures and uses it at line 33-34 to call handleRunRecording(id, userId) at line 38. Removing userId from the payload at line 27 will cause the scheduled workflow jobs to fail at runtime with undefined userId.
Restore userId to the payload or update the interface and handler to remove the userId dependency.
🤖 Prompt for AI Agents
In server/src/storage/schedule.ts around lines 26–29 the scheduled job payload
omits userId, which the ScheduledWorkflowData consumer expects; restore userId
into the payload passed to pgBossClient.schedule (e.g., include { id, runId,
userId } using the available userId value) so the scheduled handler receives a
defined userId, and update any tests or callers to supply userId; alternatively,
if you intend to remove userId entirely, update the ScheduledWorkflowData
interface and the handler to stop depending on userId instead of changing the
schedule call.
What this PR does?
Adds support for maxun sdk
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.