feat: support for sdk #920
Conversation
WalkthroughAdds a protected /sdk router implementing robot CRUD, execution, run lifecycle (start/list/get/abort), scheduling orchestration, telemetry, and LLM-driven extraction; introduces browser-side page analysis, a SelectorValidator, a WorkflowEnricher (LLM + optional vision), a remote-browser validation helper, and schedule-worker exports/registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK_API as SDK API (/sdk)
participant Remote as RemoteBrowser
participant PageAnalyzer as In-Page Analyzer
participant LLM
participant Validator as SelectorValidator
participant Storage
participant Scheduler
Client->>SDK_API: POST /sdk/extract/llm or POST /sdk/robots/:id/execute
SDK_API->>Remote: createRemoteBrowserForValidation(userId)
Remote-->>SDK_API: page, browserId
alt LLM-driven generation
SDK_API->>PageAnalyzer: analyzeElementGroups() (in-page)
PageAnalyzer-->>SDK_API: groups/samples
SDK_API->>LLM: send screenshot/HTML + prompt
LLM-->>SDK_API: decision (targets/selectors)
SDK_API->>Validator: validate/enrich selectors on page
Validator-->>SDK_API: enriched workflow/actions
else Direct validation/enrichment or execute
SDK_API->>Validator: validateSelectors(workflow)
Validator-->>SDK_API: enriched workflow
end
SDK_API->>Storage: persist robot/run
Storage-->>SDK_API: persisted id
SDK_API->>Scheduler: scheduleWorkflow(id, cron, tz) -> registerWorkerForQueue(queue)
Scheduler-->>SDK_API: queue registered
SDK_API->>Remote: execute run actions
Remote-->>SDK_API: outputs, screenshots
SDK_API-->>Client: run id, status, outputs
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)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
amhsirak
removed
Type: Feature
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (10)
server/src/sdk/workflowEnricher.ts (4)
299-301: Unnecessary type cast and private property access.Line 299 casts
pagetoanyunnecessarily, and line 301 accesses the privatepageproperty via type assertion. Use thepagevariable directly.- await validator.initialize(page as any, url); - - const validatorPage = (validator as any).page; + await validator.initialize(page, url);And update line 308:
- const pageHTML = await validatorPage.content(); + const pageHTML = await page.content();
357-359: Security: Avoideval()in browser context.Same security concern as in selectorValidator.ts. Use
page.addScriptTag()instead.- await page.evaluate((script: string) => { - eval(script); - }, scriptContent); + await page.addScriptTag({ content: scriptContent }); + await page.waitForFunction(() => typeof (window as any).analyzeElementGroups === 'function');
474-492: Missing timeout on Ollama API request.The axios call has no timeout configured, which could hang indefinitely if the LLM service is unresponsive.
const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { // ...request body... + }, { + timeout: 120000 // 2 minute timeout for LLM inference });
532-562: Missing timeout on OpenAI API request.Same issue as the Ollama call - add a timeout to prevent indefinite hangs.
}, { headers: { 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, 'Content-Type': 'application/json' - } + }, + timeout: 120000 // 2 minute timeout });server/src/sdk/selectorValidator.ts (3)
245-247: Security: Replaceeval()withpage.addScriptTag().Using
eval()to inject scripts is a security risk. Use Playwright's built-in methods instead.- await this.page.evaluate((script) => { - eval(script); - }, scriptContent); + await this.page.addScriptTag({ content: scriptContent }); + await this.page.waitForFunction(() => typeof (window as any).autoDetectListFields === 'function');
310-312: Sameeval()security issue - apply the same fix.This is the same pattern as line 246.
- await this.page.evaluate((script) => { - eval(script); - }, scriptContent); + await this.page.addScriptTag({ content: scriptContent }); + await this.page.waitForFunction(() => typeof (window as any).autoDetectPagination === 'function');
401-414: Extract duplicatedevaluateSelectorhelper.This function is defined identically 4 times in this file (lines 402-414, 449-461, 503-515, 534-546). Extract it as a class method or module-level helper.
Add as a private method:
private getEvaluateSelectorScript(selector: string): string { return ` (function(sel) { const isXPath = sel.startsWith('//') || sel.startsWith('(//'); if (isXPath) { const result = document.evaluate(sel, document, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null); const elements = []; for (let i = 0; i < result.snapshotLength; i++) { elements.push(result.snapshotItem(i)); } return elements; } else { return Array.from(document.querySelectorAll(sel)); } })('${selector.replace(/'/g, "\\'")}') `; }server/src/api/sdk.ts (3)
125-138: Critical: Missing user filtering - returns all users' robots.This endpoint returns all robots without filtering by the authenticated user, potentially exposing other users' data.
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 });
268-294: Extract duplicated cron expression generation logic.This switch statement for generating cron expressions is duplicated from
server/src/routes/storage.ts. Extract to a shared utility function.
615-629: Incomplete abort implementation - doesn't stop the interpreter.Unlike the abort endpoint in
storage.ts, this only updates the status without actually stopping the interpreter or cleaning up browser resources. This may leave orphaned browser instances.
🧹 Nitpick comments (10)
server/src/sdk/selectorValidator.ts (4)
35-48: Navigation fallback logic is good, but consider shorter timeout.The fallback from
networkidletodomcontentloadedis appropriate. However, the 100-second timeout is quite long and may cause poor user experience if navigation is slow.Consider reducing the timeout or making it configurable:
- async initialize(page: Page, url: string): Promise<void> { + async initialize(page: Page, url: string, timeout: number = 60000): Promise<void> { this.page = page; try { await page.goto(url, { waitUntil: "networkidle", - timeout: 100000, + timeout, }); } catch (err) { await page.goto(url, { waitUntil: "domcontentloaded", - timeout: 100000, + timeout, }); }
240-243: Use top-level imports instead of inlinerequire().Using
require()inside methods is a code smell in TypeScript. Importfsandpathat the top of the file.import { Page } from 'playwright-core'; import logger from '../logger'; +import * as fs from 'fs'; +import * as path from 'path';Then update the method:
- const fs = require('fs'); - const path = require('path'); const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js');
441-441: Avoid usingwaitForTimeout- use explicit waits instead.
waitForTimeoutis discouraged in Playwright as it leads to flaky tests. Consider usingwaitForLoadStateorwaitForSelectorto wait for content changes.- await this.page.waitForTimeout(2000); + await this.page.waitForLoadState('networkidle').catch(() => {});
531-531: SamewaitForTimeoutissue - prefer explicit waits.Consider using a more robust wait strategy for scroll-based content loading.
server/src/sdk/workflowEnricher.ts (3)
352-355: Use top-level imports instead of inlinerequire().Same issue as in selectorValidator.ts - import
fsandpathat the top of the file.import { SelectorValidator } from './selectorValidator'; import { createRemoteBrowserForValidation, destroyRemoteBrowser } from '../browser-management/controller'; import logger from '../logger'; import { v4 as uuid } from 'uuid'; import { encrypt } from '../utils/auth'; import Anthropic from '@anthropic-ai/sdk'; +import * as fs from 'fs'; +import * as path from 'path';
391-393: Use top-level import for axios.Importing axios inside the method is inconsistent with the rest of the codebase. Add it to the top-level imports.
import Anthropic from '@anthropic-ai/sdk'; +import axios from 'axios';Then remove line 393:
- const axios = require('axios');
584-584: Potential JSON parsing error not handled gracefully.If the LLM returns malformed JSON,
JSON.parse(jsonStr)will throw. While the outer try-catch handles it, consider providing a more specific error message.- const decision = JSON.parse(jsonStr); + let decision; + try { + decision = JSON.parse(jsonStr); + } catch (parseError) { + logger.error('Failed to parse LLM response as JSON:', jsonStr); + throw new Error('LLM returned invalid JSON response'); + }server/src/sdk/browserSide/pageAnalyzer.js (2)
1027-2229: Consider breaking downautoDetectPaginationfunction.This function spans over 1200 lines and contains multiple nested helper functions. While it's comprehensive, extracting helper functions to the module level would improve readability and testability.
Consider extracting these internal functions to the module level:
matchesAnyPatterngetClickableElementsisVisiblegeneratePaginationSelectorgetSelectorsisNearListdetectInfiniteScrollScore
886-888: Empty catch block silently swallows errors.The empty catch block makes debugging difficult. Consider logging the error or removing the try-catch if errors are expected.
} catch (error) { + // Selector evaluation failed - skip this element }server/src/schedule-worker.ts (1)
88-115: MakeregisterWorkerForQueuerobust against concurrent calls for the same queueThe
registeredQueuesSet prevents duplicate registration only in the simple case:if (registeredQueues.has(queueName)) return; // ... await pgBoss.work(queueName, handler); registeredQueues.add(queueName);If two callers invoke
registerWorkerForQueue('foo')concurrently, both can:
- See
registeredQueues.has('foo') === false- Both call
pgBoss.work('foo', ...)- Only after that, add
'foo'to the SetResult: multiple workers for the same queue in the same process, which may or may not be acceptable depending on how
pg-bosstreats multipleworkregistrations on the same queue.If you need strict idempotency per queue, consider one of:
- Lightweight guard: track in-flight registrations so only the first call actually registers the worker and others await it:
const registeredQueues = new Set<string>(); const registeringQueues = new Map<string, Promise<void>>(); export async function registerWorkerForQueue(queueName: string) { if (registeredQueues.has(queueName)) return; const inFlight = registeringQueues.get(queueName); if (inFlight) return inFlight; const registration = (async () => { try { await pgBoss.work(queueName, async (job: Job<ScheduledWorkflowData> | Job<ScheduledWorkflowData>[]) => { const singleJob = Array.isArray(job) ? job[0] : job; return processScheduledWorkflow(singleJob); }); registeredQueues.add(queueName); logger.log('info', `Registered worker for queue: ${queueName}`); } finally { registeringQueues.delete(queueName); } })(); registeringQueues.set(queueName, registration); return registration; }
- Or, if multiple workers per queue are actually desired, add a brief comment to clarify that duplicate registration is intentional.
Either way, tightening or documenting this behavior will make the API safer for use from concurrent call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(1 hunks)server/src/api/sdk.ts(1 hunks)server/src/routes/storage.ts(1 hunks)server/src/schedule-worker.ts(2 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(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/storage/schedule.ts
🧰 Additional context used
🧬 Code graph analysis (3)
server/src/routes/storage.ts (1)
server/src/storage/schedule.ts (1)
scheduleWorkflow(17-40)
server/src/sdk/selectorValidator.ts (2)
server/src/sdk/browserSide/pageAnalyzer.js (29)
isXPath(14-14)selector(2143-2143)selector(2152-2152)selector(2170-2170)selector(2180-2180)element(291-291)element(847-847)element(1813-1813)element(2045-2045)el(2106-2106)result(17-23)result(454-454)result(813-819)result(1124-1124)doc(809-811)elements(25-25)elements(705-705)elements(821-821)elements(843-843)elements(1089-1089)i(26-26)i(54-54)i(196-196)i(234-234)i(315-315)i(328-328)i(822-822)i(2105-2105)i(2489-2489)src/helpers/clientPaginationDetector.ts (1)
evaluateSelector(290-318)
server/src/api/sdk.ts (5)
server/src/routes/record.ts (1)
AuthenticatedRequest(22-24)server/src/middlewares/api.ts (1)
requireAPIKey(5-16)maxun-core/src/types/workflow.ts (1)
WorkflowFile(57-60)server/src/sdk/workflowEnricher.ts (1)
WorkflowEnricher(30-711)server/src/storage/schedule.ts (2)
cancelScheduledWorkflow(47-71)scheduleWorkflow(17-40)
🪛 Biome (2.1.2)
server/src/sdk/selectorValidator.ts
[error] 246-246: 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] 311-311: 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] 1553-1553: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (4)
server/src/routes/storage.ts (1)
897-897: The AI-generated summary is inaccurate; the code is correct as written.The review comment correctly identifies an inconsistency: the AI summary incorrectly stated that the
userIdparameter was removed from thescheduleWorkflowcall, but line 897 still passesreq.user.idas the second argument. The function signature atserver/src/storage/schedule.ts:17definesscheduleWorkflow(id: string, userId: string, cronExpression: string, timezone: string): Promise<void>, and the call at line 897 matches this signature exactly. The same pattern is used consistently atserver/src/api/sdk.ts:303.No code issues exist; the discrepancy is with the AI summary description, not the implementation.
Likely an incorrect or invalid review comment.
server/src/sdk/browserSide/pageAnalyzer.js (1)
5-6: LGTM - Good use of IIFE and strict mode.Using an IIFE with strict mode is a good practice for browser-side scripts to avoid polluting the global namespace and catch common errors.
package.json (1)
7-7: No action needed on@anthropic-ai/sdkversion.The version
^0.71.2is the current latest stable release with no known security vulnerabilities.server/src/schedule-worker.ts (1)
16-20: This review comment is based on a misunderstanding of the codebase's architectural design. The workers intentionally run in the same server process (not a separate dedicated worker process) for memory sharing, as explicitly documented in server.ts. Both development and production modes deliberately importschedule-workerinto the main server process. The exports ofpgBossandregisterWorkerForQueueare intentional API design—registerWorkerForQueueis used bystorage/schedule.tsfor dynamic queue registration when schedules are created. There is no "extra scheduler" being spun up; the single-process design prevents duplicate workers and job consumption by design. The proposed refactors (splitting into separate files or env-gating startup) would actually break the intended architecture.Likely an incorrect or invalid review comment.
| router.get("/sdk/robots/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robotId = req.params.id; | ||
|
|
||
| const robot = await Robot.findOne({ | ||
| where: { | ||
| 'recording_meta.id': robotId | ||
| } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: "Robot not found" | ||
| }); | ||
| } | ||
|
|
||
| return res.status(200).json({ | ||
| data: robot | ||
| }); | ||
| } catch (error: any) { | ||
| logger.error("[SDK] Error getting robot:", error); | ||
| return res.status(500).json({ | ||
| error: "Failed to get robot", | ||
| message: error.message | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing user ownership check for robot access.
Similar to the list endpoint, the get-by-ID endpoint should verify the robot belongs to the authenticated user to prevent unauthorized access.
const robot = await Robot.findOne({
where: {
- 'recording_meta.id': robotId
+ 'recording_meta.id': robotId,
+ userId: req.user.id
}
});📝 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/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| return res.status(200).json({ | |
| data: robot | |
| }); | |
| } catch (error: any) { | |
| logger.error("[SDK] Error getting robot:", error); | |
| return res.status(500).json({ | |
| error: "Failed to get robot", | |
| message: error.message | |
| }); | |
| } | |
| }); | |
| router.get("/sdk/robots/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId, | |
| userId: req.user.id | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| return res.status(200).json({ | |
| data: robot | |
| }); | |
| } catch (error: any) { | |
| logger.error("[SDK] Error getting robot:", error); | |
| return res.status(500).json({ | |
| error: "Failed to get robot", | |
| message: error.message | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In server/src/api/sdk.ts around lines 145 to 171, the GET /sdk/robots/:id
handler does not verify the authenticated user owns the requested robot; update
the logic to ensure ownership by either adding the authenticated user id to the
DB query (e.g., include userId/ownerId === req.user.id in the where clause) or
by fetching the robot and immediately checking robot.ownerId against
req.user.id, and if it does not match return a 404 (or 403 per project
convention) so users cannot access others' robots; keep error handling the same.
| router.put("/sdk/robots/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robotId = req.params.id; | ||
| const updates = req.body; | ||
|
|
||
| const robot = await Robot.findOne({ | ||
| where: { | ||
| 'recording_meta.id': robotId | ||
| } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: "Robot not found" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing user ownership check for robot update.
The update endpoint should verify the robot belongs to the authenticated user.
const robot = await Robot.findOne({
where: {
- 'recording_meta.id': robotId
+ 'recording_meta.id': robotId,
+ userId: req.user.id
}
});📝 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.put("/sdk/robots/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const updates = req.body; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| router.put("/sdk/robots/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const updates = req.body; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId, | |
| userId: req.user.id | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } |
🤖 Prompt for AI Agents
In server/src/api/sdk.ts around lines 177 to 192, the PUT /sdk/robots/:id
handler fetches a robot by recording_meta.id but does not verify that the robot
belongs to the authenticated user; update the lookup or add an explicit
ownership check using the authenticated user id (e.g. req.user.id) so only the
owner can update: either include the user id in the Robot.findOne where clause
(e.g. where: { 'recording_meta.id': robotId, userId: req.user.id }) or after
finding the robot compare robot.userId to req.user.id and return 403 if they
don’t match, then proceed with the update.
| router.delete("/sdk/robots/:id", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robotId = req.params.id; | ||
|
|
||
| const robot = await Robot.findOne({ | ||
| where: { | ||
| 'recording_meta.id': robotId | ||
| } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: "Robot not found" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing user ownership check for robot deletion.
The delete endpoint should verify the robot belongs to the authenticated user.
const robot = await Robot.findOne({
where: {
- 'recording_meta.id': robotId
+ 'recording_meta.id': robotId,
+ userId: req.user.id
}
});| router.post("/sdk/robots/:id/execute", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const user = req.user; | ||
| const robotId = req.params.id; | ||
|
|
||
| logger.info(`[SDK] Starting execution for robot ${robotId}`); | ||
|
|
||
| const runId = await handleRunRecording(robotId, user.id.toString()); | ||
| if (!runId) { | ||
| throw new Error('Failed to start robot execution'); | ||
| } |
There was a problem hiding this comment.
Missing user ownership check for robot execution.
Add user filtering to prevent executing other users' robots.
Verify robot ownership before execution:
const robot = await Robot.findOne({
where: {
'recording_meta.id': robotId,
userId: user.id
}
});
if (!robot) {
return res.status(404).json({ error: "Robot not found" });
}🤖 Prompt for AI Agents
In server/src/api/sdk.ts around lines 417 to 427, the route starts execution
without verifying that the authenticated user owns the robot; add a DB lookup
that queries Robot with a where clause matching recording_meta.id (robotId) and
userId === user.id before calling handleRunRecording, and return a 404 JSON
error if not found; also ensure the Robot model is imported at the top of the
file.
| async function waitForRunCompletion(runId: string, interval: number = 2000) { | ||
| const MAX_WAIT_TIME = 180 * 60 * 1000; | ||
| const startTime = Date.now(); | ||
|
|
||
| while (true) { | ||
| if (Date.now() - startTime > MAX_WAIT_TIME) { | ||
| throw new Error('Run completion timeout after 3 hours'); | ||
| } | ||
|
|
||
| const run = await Run.findOne({ where: { runId } }); | ||
| if (!run) throw new Error('Run not found'); | ||
|
|
||
| if (run.status === 'success') { | ||
| return run.toJSON(); | ||
| } else if (run.status === 'failed') { | ||
| throw new Error('Run failed'); | ||
| } else if (run.status === 'aborted') { | ||
| throw new Error('Run was aborted'); | ||
| } | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, interval)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Function signature mismatch and missing timeout.
The second parameter is named interval but defaults to 2000 (milliseconds), while the caller passes user.id.toString() at line 429. This suggests a signature error. Also, the infinite loop without proper timeout handling could hang the request.
-async function waitForRunCompletion(runId: string, interval: number = 2000) {
+async function waitForRunCompletion(runId: string, pollInterval: number = 2000): Promise<any> {
const MAX_WAIT_TIME = 180 * 60 * 1000;
const startTime = Date.now();
while (true) {And fix the caller at line 429:
- const run = await waitForRunCompletion(runId, user.id.toString());
+ const run = await waitForRunCompletion(runId);Committable suggestion skipped: line range outside the PR's diff.
| router.get("/sdk/robots/:id/runs", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robotId = req.params.id; | ||
|
|
||
| const robot = await Robot.findOne({ | ||
| where: { | ||
| 'recording_meta.id': robotId | ||
| } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: "Robot not found" | ||
| }); | ||
| } | ||
|
|
||
| const runs = await Run.findAll({ | ||
| where: { | ||
| robotMetaId: robot.recording_meta.id | ||
| }, | ||
| order: [['startedAt', 'DESC']] | ||
| }); |
There was a problem hiding this comment.
Missing user ownership check for listing runs.
Add user filtering to prevent accessing other users' robot runs.
const robot = await Robot.findOne({
where: {
- 'recording_meta.id': robotId
+ 'recording_meta.id': robotId,
+ userId: req.user.id
}
});🤖 Prompt for AI Agents
In server/src/api/sdk.ts around lines 500 to 521, the route returns robot runs
without verifying the requesting user's ownership; update the Robot lookup and
runs query to enforce ownership: when fetching the robot include a condition
that its owner/user id matches req.user.id (or fetch the robot then immediately
check robot.userId/ownerId === req.user.id and return 404/403 if not), and when
querying Run add the same user ownership constraint (e.g., include
userId/ownerId: req.user.id in the where clause) so only runs belonging to that
user’s robot are returned.
| router.get("/sdk/robots/:id/runs/:runId", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robotId = req.params.id; | ||
| const runId = req.params.runId; | ||
|
|
||
| const robot = await Robot.findOne({ | ||
| where: { | ||
| 'recording_meta.id': robotId | ||
| } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: "Robot not found" | ||
| }); | ||
| } | ||
|
|
||
| const run = await Run.findOne({ | ||
| where: { | ||
| runId: runId, | ||
| robotMetaId: robot.recording_meta.id | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing user ownership check for getting run details.
Add user filtering to prevent unauthorized access.
const robot = await Robot.findOne({
where: {
- 'recording_meta.id': robotId
+ 'recording_meta.id': robotId,
+ userId: req.user.id
}
});📝 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/:id/runs/:runId", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const runId = req.params.runId; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| const run = await Run.findOne({ | |
| where: { | |
| runId: runId, | |
| robotMetaId: robot.recording_meta.id | |
| } | |
| }); | |
| router.get("/sdk/robots/:id/runs/:runId", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const runId = req.params.runId; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId, | |
| userId: req.user.id | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| const run = await Run.findOne({ | |
| where: { | |
| runId: runId, | |
| robotMetaId: robot.recording_meta.id | |
| } | |
| }); |
| router.post("/sdk/robots/:id/runs/:runId/abort", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | ||
| try { | ||
| const robotId = req.params.id; | ||
| const runId = req.params.runId; | ||
|
|
||
| const robot = await Robot.findOne({ | ||
| where: { | ||
| 'recording_meta.id': robotId | ||
| } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: "Robot not found" | ||
| }); | ||
| } | ||
|
|
||
| const run = await Run.findOne({ | ||
| where: { | ||
| runId: runId, | ||
| robotMetaId: robot.recording_meta.id | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing user ownership check for aborting runs.
Add user filtering to prevent unauthorized abort operations.
const robot = await Robot.findOne({
where: {
- 'recording_meta.id': robotId
+ 'recording_meta.id': robotId,
+ userId: req.user.id
}
});📝 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.post("/sdk/robots/:id/runs/:runId/abort", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const runId = req.params.runId; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| const run = await Run.findOne({ | |
| where: { | |
| runId: runId, | |
| robotMetaId: robot.recording_meta.id | |
| } | |
| }); | |
| router.post("/sdk/robots/:id/runs/:runId/abort", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { | |
| try { | |
| const robotId = req.params.id; | |
| const runId = req.params.runId; | |
| const robot = await Robot.findOne({ | |
| where: { | |
| 'recording_meta.id': robotId, | |
| userId: req.user.id | |
| } | |
| }); | |
| if (!robot) { | |
| return res.status(404).json({ | |
| error: "Robot not found" | |
| }); | |
| } | |
| const run = await Run.findOne({ | |
| where: { | |
| runId: runId, | |
| robotMetaId: robot.recording_meta.id | |
| } | |
| }); |
🤖 Prompt for AI Agents
In server/src/api/sdk.ts around lines 585 to 607, the current endpoint fetches a
Robot and Run without verifying the requesting user's ownership; add user
filtering so only the robot owner can abort runs. Modify the Robot lookup to
include the authenticated user's id (e.g., where: {'recording_meta.id': robotId,
ownerId: req.user.id} or fetch the robot and immediately compare robot.ownerId
(or equivalent) to req.user.id and return 404/403 if mismatched), and likewise
ensure the Run query includes the robot/owner constraint (e.g., where: { runId,
robotMetaId: robot.recording_meta.id, ownerId: req.user.id } or validate the
run.robotMetaId belongs to the previously verified robot) before proceeding with
abort logic.
| allChildSelectors.push(...selectors); | ||
| } | ||
|
|
||
| const childSelectors = Array.from(new Set(allChildSelectors)).sort() |
There was a problem hiding this comment.
Missing semicolon after array deduplication.
There's a missing semicolon which could cause issues with ASI (Automatic Semicolon Insertion) in some edge cases.
- const childSelectors = Array.from(new Set(allChildSelectors)).sort()
+ const childSelectors = Array.from(new Set(allChildSelectors)).sort();📝 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 childSelectors = Array.from(new Set(allChildSelectors)).sort() | |
| const childSelectors = Array.from(new Set(allChildSelectors)).sort(); |
🤖 Prompt for AI Agents
In server/src/sdk/browserSide/pageAnalyzer.js around line 209, the statement
"const childSelectors = Array.from(new Set(allChildSelectors)).sort()" is
missing a trailing semicolon; add a semicolon at the end of that line (i.e.,
change it to end with ";") to terminate the statement and avoid ASI-related
edge-case issues.
| } else { | ||
| value = '\\' + codePoint.toString(16).toUpperCase() + ' '; | ||
| } | ||
| } else if (/[\t\n\f\r\x0B]/.test(character)) { |
There was a problem hiding this comment.
Regex contains control character - potential bug.
The regex at this line contains a control character (\x0B - vertical tab), which is unusual and may be unintentional. Static analysis flagged this.
The regex /[\t\n\f\r\x0B]/ includes vertical tab (\x0B). If this is intentional, consider using the named escape \v for clarity:
- } else if (/[\t\n\f\r\x0B]/.test(character)) {
+ } else if (/[\t\n\f\r\v]/.test(character)) {📝 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.
| } else if (/[\t\n\f\r\x0B]/.test(character)) { | |
| } else if (/[\t\n\f\r\v]/.test(character)) { |
🧰 Tools
🪛 Biome (2.1.2)
[error] 1553-1553: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
In server/src/sdk/browserSide/pageAnalyzer.js around line 1553 the character
class /[\t\n\f\r\x0B]/ includes the control character \x0B (vertical tab) which
is likely unintentional or unclear; replace \x0B with the named escape \v for
clarity (i.e. use /[\t\n\f\r\v]/) or, if vertical tab was not intended, remove
it from the class; update any related comments/tests to reflect the change.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
server/src/sdk/workflowEnricher.ts (4)
295-303: Remove unnecessaryanycast and privatevalidator.pageaccess.
generateWorkflowFromPromptstill usespage as anyand reads(validator as any).pagejust to callcontent()(Lines 299–301, 308). This bypasses type safety and relies onSelectorValidatorinternals.You already have the
pagereturned fromcreateRemoteBrowserForValidation, andvalidator.initialize(page, url)is used without casting inenrichWorkflow. You can safely reusepageinstead of poking intovalidator:- const { browserId: id, page } = await createRemoteBrowserForValidation(userId); + const { browserId: id, page } = await createRemoteBrowserForValidation(userId); browserId = id; - await validator.initialize(page as any, url); - - const validatorPage = (validator as any).page; + await validator.initialize(page, url); const screenshotBuffer = await page.screenshot({ fullPage: true, type: 'png' }); @@ - const pageHTML = await validatorPage.content(); + const pageHTML = await page.content();This keeps the abstraction around
SelectorValidatorintact and avoids fragileany-based access.Also applies to: 308-308
349-369: ReplaceevalinanalyzePageGroupswithpage.addScriptTag.
analyzePageGroupscurrently readspageAnalyzer.jsand injects it viapage.evaluate+eval(script)(Lines 351–359). This is exactly what the static analysis warns about: it weakens CSP-like guarantees, hurts debuggability, and is unnecessary given Puppeteer’s API.You can inject the script more safely and idiomatically:
const page = (validator as any).page; 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); + await page.addScriptTag({ content: scriptContent });The rest of the logic (calling
win.analyzeElementGroups) can remain unchanged.Confirm that Puppeteer’s `Page` type supports `addScriptTag({ content: string })` for the version in use, and that this is preferred over evaluating a large script string with `eval`.
448-495: Add timeouts to Ollama/OpenAI axios calls ingetLLMDecisionWithVision.Both the Ollama (Lines 474–492) and OpenAI (Lines 532–562) calls use
axios.postwithout any timeout. If an LLM endpoint hangs or becomes very slow, these awaits can stall the entire SDK request indefinitely.Consider introducing a shared timeout constant and passing it via the axios config:
+const LLM_REQUEST_TIMEOUT_MS = 120_000; // 2 minutes; tweak as needed @@ - const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { - model: ollamaModel, - messages: [ - // ... - ], - stream: false, - format: jsonSchema, - options: { - temperature: 0.1 - } - }); + const response = await axios.post( + `${ollamaBaseUrl}/api/chat`, + { + model: ollamaModel, + messages: [ + // ... + ], + stream: false, + format: jsonSchema, + options: { + temperature: 0.1 + } + }, + { + timeout: LLM_REQUEST_TIMEOUT_MS + } + ); @@ - const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { - // ... - }, { - headers: { - 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, - 'Content-Type': 'application/json' - } - }); + const response = await axios.post( + `${openaiBaseUrl}/chat/completions`, + { + // ... + }, + { + headers: { + 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, + 'Content-Type': 'application/json' + }, + timeout: LLM_REQUEST_TIMEOUT_MS + } + );You can also make the timeout configurable via
llmConfigif needed.#!/bin/bash # Quick check of all axios.post usages in this file to ensure they provide a timeout. rg -n "axios\\.post" server/src/sdk/workflowEnricher.ts -n -C2Also applies to: 532-565
724-768: Add timeouts to all LLM axios calls ingenerateFieldLabelsandgetLLMDecisionWithVision.Both the Ollama (line 748) and OpenAI (line 802) calls in
generateFieldLabelslack timeouts and can hang the enrichment flow. Additionally, the same issue exists ingetLLMDecisionWithVision(Ollama at line 474, OpenAI at line 532).Define a shared timeout constant near the top of the file and apply it to all four axios.post calls:
+const LLM_REQUEST_TIMEOUT_MS = 30000; // 30 seconds + export class WorkflowEnricher {Then update each axios.post to include the timeout in the config object (third parameter):
- const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { + const response = await axios.post( + `${ollamaBaseUrl}/api/chat`, + { model: ollamaModel, messages: [ // ... ], stream: false, format: jsonSchema, options: { temperature: 0.1, top_p: 0.9 } - }); + }, + { + timeout: LLM_REQUEST_TIMEOUT_MS + } + );For OpenAI calls with existing headers, add timeout to the config object:
}, { headers: { 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, 'Content-Type': 'application/json' - } + }, + timeout: LLM_REQUEST_TIMEOUT_MS });This ensures consistent timeout behavior across all LLM interactions.
🧹 Nitpick comments (3)
server/src/sdk/workflowEnricher.ts (3)
34-255: Overall enrichment flow is sound; minor edge-case refinements possible.The
enrichWorkflowimplementation looks coherent: URL extraction, single remote browser session, selector validation, and consistent cleanup are all handled well. A couple of small improvements you might consider:
- For
scrapeListactions, you collectenrichedFieldsandlistSelectorbut never push any selectors from them intoselectors(Line 173–188, 211–223). Ifwhere.selectorsis used downstream for diagnostics or targeting, you may want to add the list selector and/or field selectors there as you do forscrapeSchema.config.maxItems || 100(Line 221) will treat0or any falsy numeric as “no value” and force100. If you ever want to distinguish between “no limit” and “default limit”, consider an explicit check such astypeof config.maxItems === 'number' ? config.maxItems : 100.These are non-blocking and can be refined later.
612-629: Make fallback heuristic robust to missing or non-stringsampleTexts.
fallbackHeuristicDecisionassumes each group has asampleTextsarray of strings (Lines 619–626). IfpageAnalyzerever returns a group without this property or with non-string values,for (const sampleText of group.sampleTexts)andsampleText.toLowerCase()will throw.You can harden this without changing behavior for well-formed inputs:
- const scoredGroups = elementGroups.map((group, index) => { - let score = 0; - for (const sampleText of group.sampleTexts) { - const keywords = promptLower.split(' ').filter((w: string) => w.length > 3); - for (const keyword of keywords) { - if (sampleText.toLowerCase().includes(keyword)) score += 2; - } - } + const scoredGroups = elementGroups.map((group, index) => { + let score = 0; + const sampleTexts: string[] = Array.isArray(group.sampleTexts) + ? group.sampleTexts + : []; + for (const raw of sampleTexts) { + const sampleText = String(raw).toLowerCase(); + const keywords = promptLower.split(' ').filter((w: string) => w.length > 3); + for (const keyword of keywords) { + if (sampleText.includes(keyword)) score += 2; + } + }This keeps the heuristic working even if the analyzer output changes slightly.
884-965: Minor robustness improvements forextractFieldSamples.
extractFieldSamplesworks, and errors are contained by the outer try/catch, but a couple of small tweaks would make it safer:
- Before using
fieldInfo.selector, ensure it’s a non-empty string to avoidselector.startsWiththrowing inevaluateSelector.- Within the
forEachoverfieldsData, you currently swallow all errors (catch (e) {}at Lines 944–945). That’s fine for avoiding hard failures, but you might want at least a debug log for unexpected selector issues to aid troubleshooting.Example tweak inside the page function:
Object.entries(args.fieldsData).forEach(([fieldLabel, fieldInfo]: [string, any]) => { const samples: string[] = []; - const selector = fieldInfo.selector; + const selector = typeof fieldInfo.selector === 'string' ? fieldInfo.selector : ''; + if (!selector) { + results[fieldLabel] = []; + return; + } @@ - listItems.forEach((listItem: Element) => { + listItems.forEach((listItem: Element) => { try { const elements = evaluateSelector(selector, document); @@ - } catch (e) { - } + } catch (e) { + // optionally log in the Node context via a flag/result + } });Not critical, but it will make the sampling logic more tolerant of unexpected field definitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/sdk/workflowEnricher.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
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)
| if (decision.selectedGroupIndex === undefined || decision.selectedGroupIndex < 0 || decision.selectedGroupIndex >= elementGroups.length) { | ||
| throw new Error(`Invalid selectedGroupIndex: ${decision.selectedGroupIndex}. Must be between 0 and ${elementGroups.length - 1}`); | ||
| } | ||
|
|
||
| const selectedGroup = elementGroups[decision.selectedGroupIndex]; | ||
| return { | ||
| actionType: 'captureList', | ||
| selectedGroup, | ||
| itemSelector: selectedGroup.xpath, | ||
| reasoning: decision.reasoning, | ||
| limit: decision.limit || null | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Handle limit values explicitly instead of || to avoid misinterpreting 0/NaN.
In two places, limit is derived using ||:
getLLMDecisionWithVisionreturnslimit: decision.limit || null(Lines 595–601).buildWorkflowFromLLMDecisionusesconst limit = llmDecision.limit || 100(Lines 1040–1041).
This conflates “0”, negative numbers, NaN, and null/undefined:
- A valid
limitof0from the LLM would be turned intonulland then into100. - Any falsy numeric value will silently become the default.
Consider treating numeric limits explicitly and falling back otherwise:
- return {
- actionType: 'captureList',
- selectedGroup,
- itemSelector: selectedGroup.xpath,
- reasoning: decision.reasoning,
- limit: decision.limit || null
- };
+ return {
+ actionType: 'captureList',
+ selectedGroup,
+ itemSelector: selectedGroup.xpath,
+ reasoning: decision.reasoning,
+ limit: typeof decision.limit === 'number' && Number.isFinite(decision.limit)
+ ? decision.limit
+ : null
+ };
@@
- const limit = llmDecision.limit || 100;
+ const limit =
+ typeof llmDecision.limit === 'number' && llmDecision.limit > 0
+ ? llmDecision.limit
+ : 100;This keeps your semantics clear and avoids surprising behavior.
Also applies to: 1040-1045
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 590–601 and 1040–1045, the
code uses the || operator to derive numeric limits which treats 0, NaN and
negative numbers as falsy and substitutes defaults; change these to explicit
numeric checks: when building the LLM decision return the limit only if typeof
decision.limit === 'number' and Number.isFinite(decision.limit) (otherwise set
null), and when consuming llmDecision set limit = llmDecision.limit if it's a
finite number (otherwise use the real default 100); apply the same explicit
checks rather than || so 0 and other valid numeric values are preserved and
NaN/undefined are handled safely.
| const openaiBaseUrl = llmConfig?.baseUrl || 'https://api.openai.com/v1'; | ||
| const openaiModel = llmConfig?.model || 'gpt-4o-mini'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenAI Chat Completions models current recommended gpt-4-vision-preview deprecated
💡 Result:
Short answer: Yes — gpt-4-vision-preview was deprecated; OpenAI recommends using the gpt-4o family (and other current Chat Completions models) instead.
Evidence:
- OpenAI deprecations page lists gpt-4-vision-preview (shutdown announced 2024-12-06) and recommends replacing it with gpt-4o. [1]
- Current OpenAI model docs show gpt-4o / gpt-4o-mini and the GPT-3.5/GPT-4x family as the supported Chat Completions models. [2]
Sources:
[1] OpenAI — Deprecations (gpt-4-vision-preview → recommended replacement: gpt-4o).
[2] OpenAI — Models / Chat-completions documentation (lists gpt-4o, gpt-4o-mini, gpt-3.5/gpt-4 variants).
🌐 Web query:
gpt-4o-mini current valid OpenAI model 2024 2025
💡 Result:
Short answer — Yes, but its availability changed in 2025.
- GPT‑4o‑mini was released July 18, 2024 and was offered in OpenAI’s API/Assistants/Chat/Batches (and for fine‑tuning). [1][2]
- OpenAI replaced GPT‑4o‑mini in the ChatGPT model picker with GPT‑4.1 mini on May 14, 2025 (GPT‑4.1 mini became the default/successor in ChatGPT). [3]
- Microsoft/Azure documentation lists the gpt‑4o‑mini (2024‑07‑18) retirement window as “no earlier than Sept 15, 2025” (so Azure planned to deprecate it around or after that date). [4]
If you need current live availability for your account (API, Azure, or ChatGPT), tell me which platform and I’ll check the latest status.
Sources:
[1] OpenAI release (GPT‑4o mini, July 18, 2024).
[2] OpenAI Platform docs — GPT‑4o‑mini model page.
[3] OpenAI Help Center release notes — GPT‑4.1 mini replaces GPT‑4o mini (May 14, 2025).
[4] Microsoft Azure / MS Docs Q&A on gpt‑4o‑mini retirement (no earlier than Sept 15, 2025).
Update OpenAI model identifiers to currently supported versions.
The following models have deprecation or phase-out concerns:
- Line 529–531:
gpt-4-vision-preview— Deprecated (shutdown announced December 2024). Replace withgpt-4o, which supports vision capabilities. - Line 799–801:
gpt-4o-mini— Being phased out. OpenAI replaced it withGPT-4.1 miniin ChatGPT (May 2025) and Azure planned retirement no earlier than September 2025. Consider upgrading togpt-4oor the newer mini variant depending on your cost/capability needs.
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 799 to 801, the default
OpenAI model identifier 'gpt-4o-mini' is being phased out; update the default to
a currently supported model (for example replace with 'gpt-4o' or 'gpt-4.1-mini'
depending on desired cost/capability tradeoffs), ensure the chosen model string
matches the OpenAI SDK/API naming, and make the default configurable via
llmConfig so callers can override it if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/src/sdk/selectorValidator.ts (2)
400-420: Deduplicate the repeatedevaluateSelectorDOM/XPath helper used in scroll tests.The same
evaluateSelector(sel, doc)implementation is inlined four times acrosstestLoadMoreButtonandtestInfiniteScrollByScrolling(once in eachpage.evaluateblock). This makes the DOM/XPath selection semantics easy to accidentally change in one place but not the others.Consider centralizing this logic in a single private helper that wraps
page.evaluate, for example along the lines of:private async getListState( listSelector: string, options: { includeScrollY?: boolean } = {}, ): Promise<{ itemCount: number; scrollHeight: number; scrollY?: number }> { if (!this.page) { return { itemCount: 0, scrollHeight: 0 }; } const { includeScrollY = false } = options; return this.page.evaluate( ({ selector, includeScrollY }) => { function evaluateSelector(sel: string, doc: Document) { const isXPath = sel.startsWith('//') || sel.startsWith('(//'); if (isXPath) { const result = doc.evaluate( sel, doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null, ); const elements: HTMLElement[] = []; for (let i = 0; i < result.snapshotLength; i++) { const node = result.snapshotItem(i); if (node && node.nodeType === Node.ELEMENT_NODE) { elements.push(node as HTMLElement); } } return elements; } return Array.from(doc.querySelectorAll(sel)) as HTMLElement[]; } const listElements = evaluateSelector(selector, document); const base = { itemCount: listElements.length, scrollHeight: document.documentElement.scrollHeight, }; return includeScrollY ? { ...base, scrollY: window.scrollY } : base; }, { selector: listSelector, includeScrollY }, ); }
testLoadMoreButtonandtestInfiniteScrollByScrollingcan then callgetListStateinstead of inlining the same DOM/XPath logic, and you can keep the implementation aligned with the existing helper insrc/helpers/clientPaginationDetector.ts(see the provided snippet) for consistent selector semantics.Also applies to: 447-467, 501-522, 532-553
239-248: Replaceeval-based script injection withpage.addScriptTag(and avoid re-reading the file).Both
autoDetectListFieldsandautoDetectPaginationreadbrowserSide/pageAnalyzer.jswithfs.readFileSyncand then inject it into the page viaevalinsidepage.evaluate. Even though the script source is local, this pattern:
- trips the Biome
noGlobalEvalsecurity rule- is harder to reason about and unnecessary given Playwright’s script injection APIs
- duplicates the same script-loading logic twice
You can simplify and harden this by letting Playwright load the file directly and dropping
evalentirely:@@ - const fs = require('fs'); - const path = require('path'); - const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); - const scriptContent = fs.readFileSync(scriptPath, 'utf8'); - - await this.page.evaluate((script) => { - eval(script); - }, scriptContent); + const path = require('path'); + const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); + await this.page.addScriptTag({ path: scriptPath }); @@ - const fs = require('fs'); - const path = require('path'); - const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); - const scriptContent = fs.readFileSync(scriptPath, 'utf8'); - - await this.page.evaluate((script) => { - eval(script); - }, scriptContent); + const path = require('path'); + const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); + await this.page.addScriptTag({ path: scriptPath });You might also consider extracting this into a small helper (e.g.
private async ensurePageAnalyzerLoaded()) so the script-path resolution and injection live in one place.Check Playwright 1.57.0 docs for the `page.addScriptTag({ path })` API, including any caveats about resolving local file paths and repeated injections.Also applies to: 304-313
🧹 Nitpick comments (1)
server/src/sdk/selectorValidator.ts (1)
35-107: Core selector validation and enrichment look good; parallelization is an optional improvement.The initialization, single-selector validation, schema field validation, list field validation, and input-type detection all look logically sound: XPath vs CSS is handled consistently, missing elements are surfaced with clear errors, and the enriched metadata shape is coherent.
If this ends up being a latency hotspot when validating many fields, you could optionally parallelize
validateSchemaFieldsby collecting promises and usingPromise.allinstead of awaiting eachvalidateSelectorsequentially, but that’s a pure perf/ergonomics tweak, not a blocker.Also applies to: 113-138, 143-182, 187-224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/sdk/selectorValidator.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/sdk/selectorValidator.ts (2)
server/src/sdk/browserSide/pageAnalyzer.js (29)
isXPath(14-14)selector(2143-2143)selector(2152-2152)selector(2170-2170)selector(2180-2180)element(291-291)element(847-847)element(1813-1813)element(2045-2045)el(2106-2106)result(17-23)result(454-454)result(813-819)result(1124-1124)doc(809-811)elements(25-25)elements(705-705)elements(821-821)elements(843-843)elements(1089-1089)i(26-26)i(54-54)i(196-196)i(234-234)i(315-315)i(328-328)i(822-822)i(2105-2105)i(2489-2489)src/helpers/clientPaginationDetector.ts (1)
evaluateSelector(290-318)
🪛 Biome (2.1.2)
server/src/sdk/selectorValidator.ts
[error] 246-246: 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] 311-311: 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)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
server/src/sdk/workflowEnricher.ts (9)
299-301: Unnecessary type cast and private property access.Line 299 casts
pagetoanyunnecessarily, and line 301 accesses the privatepageproperty via type assertion. Thepagevariable from line 296 should be used directly.Apply this diff:
- await validator.initialize(page as any, url); - - const validatorPage = (validator as any).page; + 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();Based on past review comments.
357-359: Security risk: Avoid usingeval()insidepage.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 });Based on past review comments and static analysis hints.
474-492: 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 });Based on past review comments.
528-562: 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 });Based on past review comments.
529-530: Update deprecated OpenAI model identifier.The
gpt-4-vision-previewmodel was deprecated (shutdown announced December 2024). Replace withgpt-4o, which supports vision capabilities.- const openaiModel = llmConfig?.model || 'gpt-4-vision-preview'; + const openaiModel = llmConfig?.model || 'gpt-4o';Based on past review comments.
570-571: Consider reducing or truncating logs that may contain page/LLM content.The log includes the full LLM response which can easily include scraped content or user-provided data that might be sensitive in some deployments.
Consider:
- Truncating long responses, e.g.,
llmResponse.slice(0, 300)- Logging only structural metadata (e.g., response length, presence of actionType)
- Downgrading full raw-content logs to debug-level
- logger.info(`LLM Response: ${llmResponse}`); + logger.debug(`LLM Response (truncated): ${llmResponse.slice(0, 300)}...`); + logger.info(`LLM Response received, length: ${llmResponse.length}`);Based on past review comments. Also applies to lines 830-831, 867-868.
590-601: Handlelimitvalues explicitly instead of||to avoid misinterpreting 0/NaN.The expression
decision.limit || nullwill convert a valid limit of0intonull, and then line 1034 will convert it to100. This conflates falsy numeric values with undefined/null.return { actionType: 'captureList', selectedGroup, itemSelector: selectedGroup.xpath, reasoning: decision.reasoning, - limit: decision.limit || null + limit: typeof decision.limit === 'number' && Number.isFinite(decision.limit) + ? decision.limit + : null };Also update line 1034:
- const limit = llmDecision.limit || 100; + const limit = + typeof llmDecision.limit === 'number' && llmDecision.limit > 0 + ? llmDecision.limit + : 100;Based on past review comments.
799-822: Update OpenAI model and add timeout.Two issues:
- The
gpt-4o-minimodel is being phased out (replaced with GPT-4.1 mini in May 2025, with Azure retirement planned for September 2025).- Missing timeout on the API request.
- const openaiModel = llmConfig?.model || 'gpt-4o-mini'; + const openaiModel = llmConfig?.model || 'gpt-4o'; const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { model: openaiModel, messages: [ { role: 'system', content: systemPrompt }, { role: 'user', content: userPrompt } ], max_tokens: 2048, temperature: 0.1, response_format: { type: 'json_object' } }, { headers: { 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, 'Content-Type': 'application/json' - } + }, + timeout: 120000 // 2 minute timeout });Based on past review comments.
830-831: Consider reducing or truncating logs that may contain field data.Similar to line 570, this log may contain sensitive field data extracted from pages.
- logger.info(`LLM Field Labeling Response: ${llmResponse}`); + logger.debug(`LLM Field Labeling Response (truncated): ${llmResponse.slice(0, 300)}...`); + logger.info(`LLM Field Labeling Response received, length: ${llmResponse.length}`);Based on past review comments.
🧹 Nitpick comments (1)
server/src/sdk/workflowEnricher.ts (1)
95-111: Consider validatingprovidedInputTypevalue.When a
providedInputTypeis provided (line 95), it's used directly without validation. If an invalid input type is passed, it could cause issues downstream.Consider adding validation:
if (!providedInputType) { try { const inputType = await validator.detectInputType(selector); enrichedStep.what.push({ ...action, args: [selector, encryptedValue, inputType] }); } catch (error: any) { errors.push(`type action: ${error.message}`); continue; } } else { + // Validate the provided input type + const validTypes = ['text', 'email', 'password', 'number', 'tel', 'url', 'search', 'date']; + if (!validTypes.includes(providedInputType)) { + errors.push(`Invalid input type: ${providedInputType}`); + continue; + } enrichedStep.what.push({ ...action, args: [selector, encryptedValue, providedInputType] }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/sdk/workflowEnricher.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
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)
🔇 Additional comments (10)
server/src/sdk/workflowEnricher.ts (10)
82-91: LGTM! Type action validation and encryption.The validation ensures required arguments are present, and the value is properly encrypted before storage. The encryption of input values is a good security practice.
176-190: Proper error handling for auto-detection.The auto-detection logic correctly validates results and provides clear error messages when fields cannot be detected. The logging of detected field count is helpful for debugging.
243-270: Robust cleanup logic with proper error handling.The cleanup sequence properly closes the validator and destroys the remote browser, with error handling in the catch block to ensure resources are freed even if cleanup fails. This prevents resource leaks.
321-343: LGTM! Consistent cleanup pattern.The cleanup logic mirrors the pattern in
enrichWorkflow, ensuring resources are properly freed in both success and error cases.
407-423: Well-structured system prompt for LLM.The system prompt provides clear rules for group selection, emphasizing content matching over position, and properly instructs the LLM on limit extraction. The critical group selection rules help avoid common pitfalls.
612-638: LGTM! Reasonable fallback heuristic.The fallback logic uses keyword matching and element count as scoring factors, which provides a sensible default when the LLM is unavailable. The simplicity ensures it won't introduce complex failure modes.
868-876: LGTM! Proper fallback handling.The error handling provides a sensible fallback by preserving generic labels when LLM-based labeling fails, ensuring the workflow can still proceed.
895-950: LGTM! Robust sample extraction logic.The browser-side evaluation correctly handles both XPath and CSS selectors, properly scopes extraction to list items, and includes de-duplication and length limits. The error handling within the evaluation callback prevents individual failures from breaking the entire extraction.
988-1019: LGTM! Comprehensive workflow building.The logic properly:
- Auto-detects fields from the selected group
- Extracts samples for semantic labeling
- Generates user-friendly field names via LLM
- Renames fields with semantic labels
This provides a complete, user-friendly workflow from a simple prompt.
1024-1050: Proper pagination detection and workflow assembly.The pagination auto-detection with fallback to 'none' is appropriate, and the final scrapeList action is correctly structured with all enriched metadata.
| interface SimplifiedAction { | ||
| action: string | typeof Symbol.asyncDispose; | ||
| args?: any[]; | ||
| name?: string; | ||
| actionId?: string; | ||
| } |
There was a problem hiding this comment.
Clarify or remove unusual type in SimplifiedAction.action.
The action field is typed as string | typeof Symbol.asyncDispose, which is unusual. The Symbol.asyncDispose type doesn't appear to be used anywhere in the code and seems out of place for an action name. This may have been added by mistake or during experimentation.
If this type is intentional, please document why; otherwise, simplify to:
interface SimplifiedAction {
- action: string | typeof Symbol.asyncDispose;
+ action: string;
args?: any[];
name?: string;
actionId?: string;
}🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 13 to 18, the
SimplifiedAction.action type includes an unusual union with typeof
Symbol.asyncDispose which appears unused and confusing; remove the
Symbol.asyncDispose union and make action a plain string (or a documented
enum/union of allowed string literals if there are a fixed set of actions),
update any call sites/consumers to use string values or the new enum, add a
short comment explaining the chosen type if intentional, and run TypeScript
typechecking to ensure no remaining references to Symbol.asyncDispose remain.
| private static async analyzePageGroups(validator: SelectorValidator): Promise<any[]> { | ||
| try { | ||
| const page = (validator as any).page; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid accessing private properties via type assertion.
Line 351 accesses the private page property using (validator as any).page. Instead, consider:
- Making the
pageproperty public inSelectorValidatorif it needs to be accessed externally - Adding a public getter method like
getPage()inSelectorValidator - Passing the page as a parameter to this method
Option 2 (recommended):
In SelectorValidator:
public getPage() {
return this.page;
}In WorkflowEnricher:
private static async analyzePageGroups(validator: SelectorValidator): Promise<any[]> {
try {
- const page = (validator as any).page;
+ const page = validator.getPage();🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 349 to 351, the code accesses
a private SelectorValidator property via (validator as any).page; replace this
unsafe access by adding a public getter on SelectorValidator (e.g., public
getPage(): Page) and then call validator.getPage() here (remove the any cast).
Ensure SelectorValidator exports the getter, adjust imports/types if necessary,
and update any calling code or tests to use the getter instead of direct
property access.
| const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { | ||
| model: ollamaModel, | ||
| messages: [ | ||
| { | ||
| role: 'system', | ||
| content: systemPrompt | ||
| }, | ||
| { | ||
| role: 'user', | ||
| content: userPrompt | ||
| } | ||
| ], | ||
| stream: false, | ||
| format: jsonSchema, | ||
| options: { | ||
| temperature: 0.1, | ||
| top_p: 0.9 | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing timeout on Ollama API request.
Similar to the vision LLM call, this Ollama request lacks a timeout configuration.
const response = await axios.post(`${ollamaBaseUrl}/api/chat`, {
model: ollamaModel,
messages: [
{
role: 'system',
content: systemPrompt
},
{
role: 'user',
content: userPrompt
}
],
stream: false,
format: jsonSchema,
options: {
temperature: 0.1,
top_p: 0.9
}
+ }, {
+ timeout: 120000 // 2 minute timeout for LLM inference
});🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 748 to 766, the axios POST to
the Ollama API is missing a timeout configuration; add a timeout to the request
(matching the pattern used for the vision LLM call) by supplying a timeout value
in the axios request config (or using an AbortController) so the call cannot
hang indefinitely, and ensure the timeout value is consistent with other LLM
calls and error handling paths are updated to handle timeout errors.
| const page = (validator as any).page; | ||
| if (!page) { | ||
| throw new Error('Page not available'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid accessing private properties via type assertion.
Similar to lines 351 and 301, this accesses the private page property. Use the same solution as recommended earlier (add a public getter method in SelectorValidator).
- const page = (validator as any).page;
+ const page = validator.getPage();
if (!page) {
throw new Error('Page not available');
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 890 to 893, the code accesses
a private page property via a type assertion ((validator as any).page) which
should be replaced by a public getter on SelectorValidator; add a public get
page(): Page | undefined (or appropriate type) to the SelectorValidator class
(the same change applied at the other occurrences around lines ~301 and ~351),
then replace the type-asserted access with validator.page and keep the existing
null check and error throw if undefined so the code uses the public API rather
than casting to any.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (20)
server/src/sdk/workflowEnricher.ts (10)
13-28: Unusual type inSimplifiedAction.action.The
actionfield includestypeof Symbol.asyncDisposewhich appears unused and confusing. This should be simplified to juststringunless there's a documented reason for this union type.interface SimplifiedAction { - action: string | typeof Symbol.asyncDispose; + action: string; args?: any[]; name?: string; actionId?: string; }
600-606: Handlelimitvalues explicitly to avoid misinterpreting 0/NaN.Using
|| nullwill convert a valid limit of0tonull. Use explicit type checking.return { actionType: 'captureList', selectedGroup, itemSelector: selectedGroup.xpath, reasoning: decision.reasoning, - limit: decision.limit || null + limit: typeof decision.limit === 'number' && Number.isFinite(decision.limit) + ? decision.limit + : null };
803-829: Consider updating default OpenAI model and add timeout.
gpt-4o-mini(line 805) is being phased out. Also, this axios call lacks a timeout.- const openaiModel = llmConfig?.model || 'gpt-4o-mini'; + const openaiModel = llmConfig?.model || 'gpt-4o'; const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { // ... request body ... }, { headers: { 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, 'Content-Type': 'application/json' - } + }, + timeout: 120000 });
1036-1039: Handlelimitvalues explicitly.Same issue as earlier - using
|| 100will convert a valid limit of0to100.- const limit = llmDecision.limit || 100; + const limit = + typeof llmDecision.limit === 'number' && llmDecision.limit > 0 + ? llmDecision.limit + : 100;
296-313: Unnecessary type cast and private property access.Line 298 casts
pagetoanyunnecessarily, and line 300 accesses the privatepageproperty via type assertion. Thepagevariable from line 295 should be used directly. Similarly, line 313 usesvalidatorPagewhenpagewould suffice.- await validator.initialize(page as any, url); - - const validatorPage = (validator as any).page; + await validator.initialize(page, url); // ... screenshot code ... - const pageHTML = await validatorPage.content(); + const pageHTML = await page.content();Consider adding a public
getPage()method toSelectorValidatorif external access is needed.
354-378: Security risk: Avoideval()and prefer async file reading.Two issues here:
- Using
eval()insidepage.evaluate()is a security concern flagged by static analysis, even though the script is from the filesystem.fs.readFileSyncblocks the event loop in an async method.+ import * as fs from 'fs/promises'; + import * as path from 'path'; private static async analyzePageGroups(validator: SelectorValidator): Promise<any[]> { try { - const page = (validator as any).page; - const fs = require('fs'); - const path = require('path'); + const page = validator.getPage(); // Use public getter const scriptPath = path.join(__dirname, 'browserSide/pageAnalyzer.js'); - const scriptContent = fs.readFileSync(scriptPath, 'utf8'); + const scriptContent = await fs.readFile(scriptPath, 'utf8'); - await page.evaluate((script: string) => { - eval(script); - }, scriptContent); + await page.addScriptTag({ content: scriptContent }); + await page.waitForFunction(() => typeof (window as any).analyzeElementGroups === 'function');
479-497: 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`, { // ... request body ... + }, { + timeout: 120000 // 2 minute timeout for LLM inference });
533-567: Update OpenAI model and add timeout.Two issues:
gpt-4-vision-preview(line 535) was deprecated in December 2024. Usegpt-4oinstead.- Missing timeout on the axios request.
- const openaiModel = llmConfig?.model || 'gpt-4-vision-preview'; + const openaiModel = llmConfig?.model || 'gpt-4o'; const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { // ... request body ... }, { headers: { 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, 'Content-Type': 'application/json' - } + }, + timeout: 120000 // 2 minute timeout });
752-771: Missing timeout on Ollama API request in generateFieldLabels.Same issue as the other Ollama call - no timeout configured.
const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { // ... request body ... + }, { + timeout: 120000 // 2 minute timeout for LLM inference });
894-898: Avoid accessing private properties via type assertion.Use a public getter method instead of
(validator as any).page.- const page = (validator as any).page; + const page = validator.getPage(); if (!page) { throw new Error('Page not available'); }server/src/api/sdk.ts (10)
125-139: Critical: Missing user filtering - returns all users' robots.The endpoint returns all robots from the database without filtering by the authenticated user. This is a data exposure issue.
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 + } + });
145-171: Critical: Missing user ownership check for robot access.The get-by-ID endpoint should verify the robot belongs to the authenticated user.
const robot = await Robot.findOne({ where: { - 'recording_meta.id': robotId + 'recording_meta.id': robotId, + userId: req.user.id } });
177-192: Critical: Missing user ownership check for robot update.The update endpoint should verify the robot belongs to the authenticated user.
const robot = await Robot.findOne({ where: { - 'recording_meta.id': robotId + 'recording_meta.id': robotId, + userId: req.user.id } });
366-380: Critical: Missing user ownership check for robot deletion.The delete endpoint should verify the robot belongs to the authenticated user.
const robot = await Robot.findOne({ where: { - 'recording_meta.id': robotId + 'recording_meta.id': robotId, + userId: req.user.id } });
417-427: Critical: Missing user ownership check for robot execution.Add user filtering before execution to prevent executing other users' robots.
router.post("/sdk/robots/:id/execute", requireAPIKey, async (req: AuthenticatedRequest, res: Response) => { try { const user = req.user; const robotId = req.params.id; + const robot = await Robot.findOne({ + where: { + 'recording_meta.id': robotId, + userId: user.id + } + }); + if (!robot) { + return res.status(404).json({ error: "Robot not found" }); + } + logger.info(`[SDK] Starting execution for robot ${robotId}`);
429-429: Function signature mismatch: caller passes string, function expects number.Line 429 calls
waitForRunCompletion(runId, user.id.toString())but the function at line 472 expectsinterval: number = 2000. The second argument is being misinterpreted.// Fix the caller at line 429: - const run = await waitForRunCompletion(runId, user.id.toString()); + const run = await waitForRunCompletion(runId); // Or if you need to pass userId for some reason, fix the function signature: -async function waitForRunCompletion(runId: string, interval: number = 2000) { +async function waitForRunCompletion(runId: string, pollInterval: number = 2000): Promise<any> {Also applies to: 472-494
500-521: Critical: Missing user ownership check for listing runs.Add user filtering to prevent accessing other users' robot runs.
const robot = await Robot.findOne({ where: { - 'recording_meta.id': robotId + 'recording_meta.id': robotId, + userId: req.user.id } });
539-561: Critical: Missing user ownership check for getting run details.Add user filtering to prevent unauthorized access.
const robot = await Robot.findOne({ where: { - 'recording_meta.id': robotId + 'recording_meta.id': robotId, + userId: req.user.id } });
585-607: Critical: Missing user ownership check for aborting runs.Add user filtering to prevent unauthorized abort operations.
const robot = await Robot.findOne({ where: { - 'recording_meta.id': robotId + 'recording_meta.id': robotId, + userId: req.user.id } });
615-629: Incomplete abort implementation - doesn't stop interpreter.Unlike the abort endpoint in
storage.ts, 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
🧹 Nitpick comments (1)
server/src/sdk/workflowEnricher.ts (1)
396-399: Move axios import to top-level.Dynamic
require('axios')inside methods prevents tree-shaking and is inconsistent with ES module style used elsewhere. Import at the top of the file.+import axios from 'axios'; import Anthropic from '@anthropic-ai/sdk'; // Then remove: - const axios = require('axios');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/api/sdk.ts(1 hunks)server/src/sdk/workflowEnricher.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
server/src/sdk/workflowEnricher.ts
[error] 363-363: 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)
🔇 Additional comments (12)
server/src/sdk/workflowEnricher.ts (8)
1-11: LGTM on imports.Imports are well-organized with clear dependencies for browser management, validation, LLM integration, and utilities.
30-75: LGTM on enrichWorkflow initialization and URL extraction.Good validation of empty workflows and URL extraction logic with proper handling of regex patterns.
76-119: LGTM on type action handling.Proper validation, encryption of sensitive values, and input type detection with appropriate error handling.
120-165: LGTM on scrapeSchema action enrichment.Correct field validation, enrichment, and selector collection with proper error aggregation.
166-240: LGTM on scrapeList action enrichment.Good auto-detection of fields and pagination with appropriate fallbacks and error handling.
241-270: LGTM on cleanup and error handling.Proper resource cleanup in both success and error paths with validator.close() and destroyRemoteBrowser().
614-644: LGTM on fallback heuristic decision.Reasonable scoring logic based on keyword matching and group size with proper error handling for empty groups.
968-1065: LGTM on buildWorkflowFromLLMDecision structure.Good parallel execution of field samples and pagination detection, proper error handling, and well-structured workflow output.
server/src/api/sdk.ts (4)
1-26: LGTM on imports and interface definition.Well-organized imports with appropriate dependencies for SDK functionality.
31-119: LGTM on robot creation endpoint.Good validation, workflow enrichment, proper error handling, and analytics capture.
193-360: LGTM on update logic for schedule, webhooks, and proxy settings.Comprehensive handling of schedule parameters with proper validation, timezone checks, cron expression generation, and cleanup of existing schedules.
643-715: LGTM on LLM extraction endpoint.Good validation of required parameters, proper workflow generation with WorkflowEnricher, and correct robot persistence. The previous
user.idbug has been addressed - line 645 now correctly assigns the full user object.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
server/src/sdk/workflowEnricher.ts (8)
13-18: SimplifySimplifiedAction.actiontype (removeSymbol.asyncDisposeunion).
SimplifiedAction.actionis typed asstring | typeof Symbol.asyncDispose, but only string actions are ever used (and non-strings are immediately skipped). TheSymbol.asyncDisposeunion looks accidental and makes the type harder to understand.Recommend simplifying to a plain string:
interface SimplifiedAction { - action: string | typeof Symbol.asyncDispose; + action: string; args?: any[]; name?: string; actionId?: string; }If you intended to support non-string actions, please document and wire that through explicitly instead of using this symbol.
166-176: TightenscrapeListconfig validation and avoid||for numericmaxItems.Two issues in the
scrapeListbranch:
config.itemSelectoris used without validation (Line 177). If it’s missing or not a string,autoDetectListFieldswill receive an invalid selector and fail in less obvious ways.limit: config.maxItems || 100(Line 220) treats0, negative numbers,NaN, and other falsy numbers as “use default 100”. A user-specified0(or other edge numeric) would be silently overridden.Consider:
- const config = action.args[0]; + const config = action.args[0] || {}; @@ - try { - const autoDetectResult = await validator.autoDetectListFields(config.itemSelector); + const itemSelector = config.itemSelector; + if (!itemSelector || typeof itemSelector !== 'string') { + errors.push('scrapeList config.itemSelector is required and must be a non-empty string'); + continue; + } + + try { + const autoDetectResult = await validator.autoDetectListFields(itemSelector); @@ - limit: config.maxItems || 100 + limit: + typeof config.maxItems === 'number' && + Number.isFinite(config.maxItems) && + config.maxItems > 0 + ? config.maxItems + : 100This makes failure modes explicit and preserves valid numeric values instead of collapsing them via
||.Also applies to: 191-221
291-309: Remove unsafeanycasts and privatepageaccess onSelectorValidator.There are several places where
SelectorValidatorinternals are accessed viaas any:
- Line 298:
await validator.initialize(page as any, url);- Line 300:
const validatorPage = (validator as any).page;- Line 356:
const page = (validator as any).page;- Line 895:
const page = (validator as any).page;These bypass TypeScript’s safety, reach into a private field, and make refactors of
SelectorValidatorbrittle.Suggested direction:
- Use the strongly-typed
pageyou already have ingenerateWorkflowFromPrompt:- await validator.initialize(page as any, url); - - const validatorPage = (validator as any).page; + await validator.initialize(page, url); @@ - const screenshotBuffer = await page.screenshot({ + const screenshotBuffer = await page.screenshot({ @@ - const elementGroups = await this.analyzePageGroups(validator); + const elementGroups = await this.analyzePageGroups(validator); @@ - const pageHTML = await validatorPage.content(); + const pageHTML = await page.content();
- Add a public getter to
SelectorValidatorand use it where you truly need the page (e.g.,analyzePageGroups,extractFieldSamples):In
SelectorValidator:export class SelectorValidator { private page: Page | null = null; + public getPage(): Page | null { + return this.page; + }Then here:
- const page = (validator as any).page; + const page = validator.getPage(); + if (!page) { + throw new Error('Page not available'); + }(and similarly in
extractFieldSamples).This keeps the validator’s encapsulation intact and removes the need for
anyassertions.Also applies to: 354-360, 887-899
535-536: Update default OpenAI model identifiers to current, supported models.Defaults:
gpt-4-vision-preview(Line 535) is deprecated.gpt-4o-mini(Line 805) has been or is being phased out in favor of newergpt-4o/gpt-4.1variants.To avoid sudden breakage when OpenAI retires older models, consider:
- Switching the defaults to a current model, e.g.
gpt-4o(vision-capable) andgpt-4.1-mini(or the current “mini” model).- Keeping them overridable via
llmConfig.modelas you already do.Example:
- const openaiModel = llmConfig?.model || 'gpt-4-vision-preview'; + const openaiModel = llmConfig?.model || 'gpt-4o'; @@ - const openaiModel = llmConfig?.model || 'gpt-4o-mini'; + const openaiModel = llmConfig?.model || 'gpt-4o';(Adjust to whatever OpenAI currently recommends for your use case.)
What are the currently recommended OpenAI Chat Completions models (including vision) and which legacy models are deprecated?Also applies to: 805-806
575-576: Reduce or downgrade logging of full LLM responses.These
infologs include entire LLM payloads:
- Line 575:
LLM Response: ${llmResponse}- Line 835:
LLM Field Labeling Response: ${llmResponse}Depending on the scraped page, this can easily include user content or sensitive data and can be very large.
Suggested adjustments:
- Truncate content and/or log at debug level:
- logger.info(`LLM Response: ${llmResponse}`); + const preview = llmResponse.slice(0, 300); + logger.debug(`LLM response preview (first 300 chars): ${preview}`);and similarly for field labeling.
- Optionally add a config flag to enable full logging only in dev.
This keeps observability while being friendlier to privacy/compliance and log volume.
Also applies to: 835-836
579-606: Handlelimitfrom LLM explicitly instead of||defaults.Two places use
||with limit values:
- In
getLLMDecisionWithVision(Line 605):limit: decision.limit || null.- In
buildWorkflowFromLLMDecision(Line 1036):const limit = llmDecision.limit || 100;.This conflates several cases:
- A valid
0(or other falsy number) becomesnull→100.NaN, negative values, andundefinedall collapse the same way.Recommend explicit numeric checks:
- return { - actionType: 'captureList', - selectedGroup, - itemSelector: selectedGroup.xpath, - reasoning: decision.reasoning, - limit: decision.limit || null - }; + return { + actionType: 'captureList', + selectedGroup, + itemSelector: selectedGroup.xpath, + reasoning: decision.reasoning, + limit: + typeof decision.limit === 'number' && Number.isFinite(decision.limit) + ? decision.limit + : null + }; @@ - const limit = llmDecision.limit || 100; + const limit = + typeof llmDecision.limit === 'number' && llmDecision.limit > 0 + ? llmDecision.limit + : 100;This makes behavior for edge cases explicit and avoids surprising defaults.
Also applies to: 1036-1051
887-965: Use validator getter instead of(validator as any).pageinextractFieldSamples.
extractFieldSamplesalso accesses the validator page viaas any:const page = (validator as any).page; if (!page) { throw new Error('Page not available'); }Once you add a
getPage()toSelectorValidator(as suggested earlier), this can become:- const page = (validator as any).page; + const page = validator.getPage(); if (!page) { throw new Error('Page not available'); }which keeps type safety and encapsulation consistent across the file.
455-497: Add timeouts to all axios LLM requests (Ollama/OpenAI).All axios calls to LLM backends currently lack a timeout:
- Ollama vision call (Lines 479–497).
- OpenAI vision call (Lines 537–567).
- Ollama field-labeling call (Lines 752–771).
- OpenAI field-labeling call (Lines 807–827).
If any backend hangs, these requests can block indefinitely.
Recommend adding explicit timeouts (optionally configurable via
llmConfig), e.g.:- const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { + const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { // ... - }); + }, { + timeout: llmConfig?.timeout ?? 120_000 + }); @@ - const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { + const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { // ... - }, { - headers: { - 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, - 'Content-Type': 'application/json' - } - }); + }, { + headers: { + 'Authorization': `Bearer ${llmConfig?.apiKey || process.env.OPENAI_API_KEY}`, + 'Content-Type': 'application/json' + }, + timeout: llmConfig?.timeout ?? 120_000 + });Apply the same pattern in
generateFieldLabelsfor both Ollama and OpenAI calls.Does axios support a `timeout` option in the request config object, and what are the semantics for aborted requests?Also applies to: 537-567, 729-771, 807-827
🧹 Nitpick comments (1)
server/src/sdk/workflowEnricher.ts (1)
384-451: (Optional) Remove unusedpageHTMLparameter or put it to use.
getLLMDecisionWithVisionacceptspageHTML(Line 388), and you passpageHTMLfromgenerateWorkflowFromPrompt(Line 319), but the variable is never used inside the function.Either:
- Remove the argument and parameter to reduce noise, or
- Incorporate
pageHTMLinto the prompt (e.g., for cases where the screenshot alone is ambiguous).Right now it’s dead data flowing through the call.
Also applies to: 577-590
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/sdk/workflowEnricher.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/sdk/workflowEnricher.ts (4)
server/src/sdk/selectorValidator.ts (1)
SelectorValidator(27-585)server/src/browser-management/controller.ts (2)
createRemoteBrowserForValidation(445-496)destroyRemoteBrowser(124-200)server/src/sdk/browserSide/pageAnalyzer.js (35)
selectors(200-204)selectors(228-228)selectors(1086-1086)selector(2143-2143)selector(2152-2152)selector(2170-2170)selector(2180-2180)value(498-498)fields(157-157)fields(211-215)result(17-23)result(454-454)result(813-819)result(1124-1124)fieldName(1011-1011)config(1158-1158)path(644-644)attribute(908-908)i(26-26)i(54-54)i(196-196)i(234-234)i(315-315)i(328-328)i(822-822)i(2105-2105)i(2489-2489)doc(809-811)isXPath(14-14)elements(25-25)elements(705-705)elements(821-821)elements(843-843)elements(1089-1089)el(2106-2106)server/src/routes/index.ts (1)
workflow(10-10)
🪛 Biome (2.1.2)
server/src/sdk/workflowEnricher.ts
[error] 363-363: 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)
🔇 Additional comments (1)
server/src/sdk/workflowEnricher.ts (1)
971-1052: OverallbuildWorkflowFromLLMDecisionstructure looks solid, aside fromlimithandling.The high-level flow here is good:
- Seed workflow with a
goto+waitForLoadState.- Auto-detect list fields + pagination in parallel.
- Generate semantic field labels and rename fields.
- Push a single
scrapeListwith an actionId and a final wait.Once the
limithandling is made explicit (see earlier comment), this should behave predictably for most captureList decisions.
| const screenshotBuffer = await page.screenshot({ | ||
| fullPage: true, | ||
| type: 'jpeg', | ||
| quality: 85 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n server/src/sdk/workflowEnricher.ts | sed -n '300,320p'Repository: getmaxun/maxun
Length of output: 972
🏁 Script executed:
cat -n server/src/sdk/workflowEnricher.ts | sed -n '510,525p'Repository: getmaxun/maxun
Length of output: 576
🏁 Script executed:
cat -n server/src/sdk/workflowEnricher.ts | sed -n '550,560p'Repository: getmaxun/maxun
Length of output: 417
🌐 Web query:
Anthropic vision API media_type requirements base64 image
💡 Result:
Short answer: For Anthropic's Vision (Messages) API, base64 images must be sent as a content block with source.type = "base64", include a media_type set to one of the allowed MIME types, and a base64 string in data (no data: URI prefix). Example fields and allowed media_type values:
-
JSON structure (inside a message.content array):
{
"type": "image",
"source": {
"type": "base64",
"media_type": "image/jpeg", <-- allowed MIME types
"data": "<BASE64_ENCODED_IMAGE_DATA>"
}
} -
Allowed media_type values: "image/jpeg", "image/png", "image/gif", "image/webp". [Anthropic docs show the base64 example and field names; the allowed MIME types are documented and shown in examples and error text]. [1][2][3]
Sources:
- Anthropic Vision (Messages API) base64 example and field names. [1]
- Anthropic Messages / Vision docs (allowed image types listed). [2]
- Reported API validation error listing allowed media_type values (GitHub issue). [3]
References:
[1] Anthropic Vision docs — base64 example. (docs.anthropic.com)
[2] Anthropic Vision guide / examples. (anthropic.mintlify.app)
[3] GitHub issue showing validation error and allowed types. (github.com)
Citations:
- 1: https://docs.anthropic.com/en/docs/build-with-claude/vision?utm_source=openai
- 2: https://anthropic.mintlify.app/en/docs/build-with-claude/vision?utm_source=openai
- 3: [Bug] Anthropic API Error: Invalid Image Media Type for Base64 Upload anthropics/claude-code#7088
🌐 Web query:
OpenAI Chat Completions image_url data URL format requirements image/jpeg image/png
💡 Result:
Short answer: Use a base64 data URI of the form
data:image/;base64,<BASE64_PAYLOAD>
with mime = image/png or image/jpeg (jpg accepted). The payload must be valid base64 (no extra line breaks), under the API size limits, and passed in the message content as the image field (e.g., an input_image / image_url entry). Example: data:image/jpeg;base64,/9j/4AAQ... . [1][2][3]
Key points and recommendations
- Exact format: data:image/;base64,. Supported types include image/png and image/jpeg (also webp, non-animated gif per docs). [1]
- No extra prefixes or wrappers — include only the single data URI string as the image URL value. Keep the base64 string contiguous (no newlines). [1][3]
- Respect request payload limits (total request and per-image size limits — e.g., docs note payload limits such as up to ~50 MB total per request depending on endpoint). If image is large, host it instead of embedding. [1][4]
- Some client SDKs or integrations have had bugs where data: URIs were rejected even though the API supports them; if you see “unsupported image” or “base64 not supported,” try sending the same payload directly to the OpenAI HTTP endpoint or check for SDK/adapter updates. [2][3]
If you want, tell me which model/SDK and I’ll draft an exact JSON example you can paste.
Fix image type mismatch between JPEG screenshot and PNG metadata in API calls.
Line 305–306 captures a JPEG screenshot (type: 'jpeg', quality: 85), but both downstream API calls incorrectly declare PNG:
- Anthropic (line 517):
media_type: 'image/png' - OpenAI (line 554):
data:image/png;base64,...
Anthropic explicitly validates media_type and supports both image/jpeg and image/png. Sending JPEG data with PNG metadata will cause API validation failures or undefined behavior. OpenAI accepts both formats but the mismatch still indicates incorrect metadata.
Update the API calls to match the actual JPEG format:
- media_type: 'image/png',
+ media_type: 'image/jpeg',and
- url: `data:image/png;base64,${screenshotBase64}`
+ url: `data:image/jpeg;base64,${screenshotBase64}`Alternatively, convert the screenshot to PNG by removing the quality parameter and changing type: 'jpeg' to type: 'png', then keep the current metadata.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 303–306, the code captures a
JPEG screenshot (type: 'jpeg', quality: 85) but downstream API calls still claim
PNG; update the metadata to match the actual JPEG format by changing Anthropic's
media_type to 'image/jpeg' and OpenAI's data URI prefix to
'data:image/jpeg;base64,' so the declared media type matches the screenshot, or
alternatively change the screenshot to PNG (set type: 'png' and remove the
quality option) and keep the existing PNG metadata; ensure both the captured
image type and all API metadata are consistent.
| private static async analyzePageGroups(validator: SelectorValidator): Promise<any[]> { | ||
| try { | ||
| const page = (validator as any).page; | ||
| 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.
🧩 Analysis chain
🌐 Web query:
Playwright page.addScriptTag API documentation
💡 Result:
Summary — page.addScriptTag (Playwright)
- Purpose: injects a <script> tag into the page (by url, file path, or raw content) and returns the added element when the script's onload fires or when content is injected. [1]
Options (object; pick one or more)
- content: string — raw JavaScript to inject. [1]
- path: string — path to a local .js file (relative paths resolved against cwd). [1]
- url: string — URL of the script to add. [1]
- type: string — script type (e.g., 'module' for ES modules). [1]
Simple example (Node.js)
- Inject raw content:
await page.addScriptTag({ content: 'window.foo = 42;' }); [1] - Inject from file:
await page.addScriptTag({ path: require('path').resolve(__dirname, 'script.js') }); [1] - Inject as ES module:
await page.addScriptTag({ url: 'https://example.com/lib.mjs', type: 'module' }); [1]
Notes
- For code that must run before page scripts, use addInitScript / browserContext.addInitScript instead. [2]
Sources
[1] Playwright — Page.addScriptTag API docs.
[2] Playwright — Evaluating JavaScript / addInitScript guidance.
🏁 Script executed:
# Search for the SelectorValidator class and the mentioned methods
find . -type f -name "*.ts" -o -name "*.js" | head -20
</br>
rg "class SelectorValidator" -A 5
</br>
# Search for eval usage in the context of page.evaluate
rg "eval\(" --type ts --type js -B 2 -A 2Repository: getmaxun/maxun
Length of output: 923
🏁 Script executed:
# Specifically look for the methods mentioned in the review
rg "autoDetectListFields|autoDetectPagination" --type ts --type js -B 2 -A 10Repository: getmaxun/maxun
Length of output: 12015
🏁 Script executed:
# Check the exact code at lines 354-365 in workflowEnricher.ts
sed -n '350,370p' server/src/sdk/workflowEnricher.tsRepository: getmaxun/maxun
Length of output: 773
🏁 Script executed:
# Search for all eval usage in workflowEnricher.ts
rg "eval" server/src/sdk/workflowEnricher.ts -B 3 -A 3 -nRepository: getmaxun/maxun
Length of output: 1802
🏁 Script executed:
# Also check selectorValidator.ts for similar patterns
rg "eval|page.evaluate" server/src/sdk/selectorValidator.ts -B 2 -A 5 -nRepository: getmaxun/maxun
Length of output: 5583
Replace eval in page.evaluate with page.addScriptTag for safe script injection.
analyzePageGroups (lines 362-364) in workflowEnricher.ts currently uses:
await page.evaluate((script: string) => {
eval(script);
}, scriptContent);Replace with:
await page.addScriptTag({ content: scriptContent });The same pattern appears in SelectorValidator.autoDetectListFields() (lines 245-247) and SelectorValidator.autoDetectPagination() (lines 310-312) and should be updated identically. Using page.addScriptTag() is safer, more maintainable, and follows Playwright's recommended approach for injecting scripts.
🧰 Tools
🪛 Biome (2.1.2)
[error] 363-363: 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 354 to 365, replace the
unsafe page.evaluate + eval(script) injection with Playwright's
page.addScriptTag by reading the script content and calling page.addScriptTag({
content: scriptContent }) instead of page.evaluate; also make the identical
replacements in SelectorValidator.autoDetectListFields() (around lines 245-247)
and SelectorValidator.autoDetectPagination() (around lines 310-312) so all three
sites inject browserSide/pageAnalyzer.js via page.addScriptTag for safer,
recommended script injection.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
server/src/sdk/workflowEnricher.ts (9)
13-18: Unusual type union withSymbol.asyncDisposeremains unaddressed.The
actionfield still includestypeof Symbol.asyncDisposein its union type, which was previously flagged. This appears to be a mistake as Symbol.asyncDispose is not used anywhere in the codebase for action types.
298-313: Multiple unaddressed issues from previous reviews.Three issues remain unaddressed:
- Line 298: Unnecessary
as anycast onpageparameter- Line 300: Unsafe access to private
pageproperty via type assertion- Lines 303-307: JPEG screenshot captured but downstream API calls (lines 517, 554) declare PNG metadata
These were all flagged in previous reviews but remain unfixed.
354-379: Critical security issue and unsafe property access remain unaddressed.Two previously flagged issues are still present:
- Line 356: Unsafe access to private
pageproperty via(validator as any).page- Lines 362-364: Security risk using
eval()insidepage.evaluate()The past review recommended using
page.addScriptTag({ content: scriptContent })instead of eval, which is safer and follows Playwright best practices. The same pattern appears in SelectorValidator methods and should be fixed consistently.
479-497: Missing timeout on Ollama API request.The axios POST to Ollama has no timeout configured, which was flagged in a previous review. If the LLM service is unresponsive, this could hang indefinitely.
537-567: Missing timeout on OpenAI API request.Similar to the Ollama call, this axios POST lacks a timeout configuration. Add a timeout in the request config (e.g.,
timeout: 120000in the third argument).
595-606: Limit handling with||operator treats 0 as falsy.Line 605 uses
decision.limit || null, which will convert a valid limit of 0 to null. Use explicit numeric checks instead:limit: typeof decision.limit === 'number' && Number.isFinite(decision.limit) ? decision.limit : nullThe same issue exists at line 1276 (
llmDecision.limit || 100).
746-764: Missing timeout on Ollama API request.This axios POST to Ollama lacks a timeout, similar to the vision LLM call. Add timeout configuration to prevent indefinite hangs.
796-822: Default OpenAI model may be deprecated and missing timeout.Two issues:
- Line 798:
gpt-4o-minimay be phased out; consider usinggpt-4oor verify current model availability- Lines 800-820: OpenAI API call lacks timeout configuration
What is the current status of gpt-4o-mini model in OpenAI API as of December 2025?
1113-1116: Unsafe private property access.Line 1113 accesses the private
pageproperty via(validator as any).page, which was flagged in previous reviews. The recommended fix is to add a public getter method in SelectorValidator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/sdk/workflowEnricher.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
server/src/sdk/workflowEnricher.ts
[error] 363-363: 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)
| const response = await axios.post(`${ollamaBaseUrl}/api/chat`, { | ||
| model: ollamaModel, | ||
| messages: [ | ||
| { | ||
| role: 'system', | ||
| content: systemPrompt | ||
| }, | ||
| { | ||
| role: 'user', | ||
| content: userPrompt | ||
| } | ||
| ], | ||
| stream: false, | ||
| format: jsonSchema, | ||
| options: { | ||
| temperature: 0.1, | ||
| top_p: 0.9 | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing timeout on Ollama API request.
Consistent with other LLM calls in this file, this axios POST to Ollama needs a timeout configuration.
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 973–991 the axios.post call
to the Ollama API is missing a timeout config; add an axios request config as
the third argument supplying a timeout value consistent with other LLM calls in
this file (use the same timeout constant/variable used elsewhere, or set a
sensible default such as 60000 ms) so the request will fail fast on network
hangs.
| const response = await axios.post(`${openaiBaseUrl}/chat/completions`, { | ||
| model: openaiModel, | ||
| messages: [ | ||
| { | ||
| role: 'system', | ||
| content: systemPrompt | ||
| }, | ||
| { | ||
| role: 'user', | ||
| content: userPrompt | ||
| } | ||
| ], | ||
| max_tokens: 1024, | ||
| temperature: 0.1, | ||
| response_format: { type: 'json_object' } | ||
| }, { | ||
| 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.
Add timeout configuration to this OpenAI API call to match the pattern needed throughout the file.
🤖 Prompt for AI Agents
In server/src/sdk/workflowEnricher.ts around lines 1019 to 1039, the axios POST
to the OpenAI chat completions endpoint is missing a timeout option; add a
timeout (in milliseconds) to the axios request config to match the rest of the
file (e.g., use the existing llmConfig timeout field or a shared DEFAULT/ENV
var). Modify the third argument to include timeout: llmConfig?.timeout ||
Number(process.env.OPENAI_REQUEST_TIMEOUT_MS) || 60000 (or the project's
default) so the request will fail fast on network stalls and follow the
established timeout pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
server/src/sdk/browserSide/pageAnalyzer.js (2)
209-209: Missing semicolon after array deduplication.This was flagged in a previous review. There's a missing semicolon which could cause ASI-related issues.
1552-1552: Regex contains control character\x0B- use\vfor clarity.This was flagged in a previous review and by static analysis. The vertical tab control character should use the named escape for readability.
server/src/sdk/selectorValidator.ts (3)
239-247: Avoideval()for script injection - security risk.This was flagged in a previous review. Using
eval()exposes the application to security risks.
304-312: Sameeval()security issue - refactor script injection.This is the same pattern as line 246. Apply the same fix.
400-413: Extract duplicatedevaluateSelectorhelper function.This was flagged in a previous review. The
evaluateSelectorfunction is defined identically 4 times withinpage.evaluatecallbacks.Also applies to: 448-460, 502-514, 533-545
🧹 Nitpick comments (7)
server/src/sdk/browserSide/pageAnalyzer.js (3)
174-177: Module-level caches are never cleared - potential memory accumulation.The
pathCache,descendantsCache,meaningfulCache(WeakMaps), andclassCache(Map) are declared at module scope. While WeakMaps allow GC of unreferenced keys, theclassCacheMap will accumulate entries indefinitely during long browser sessions.Consider adding a cache-clearing mechanism or limiting cache size:
const pathCache = new WeakMap(); const descendantsCache = new WeakMap(); const meaningfulCache = new WeakMap(); -const classCache = new Map(); +const classCache = new Map(); +const MAX_CLASS_CACHE_SIZE = 1000; + +function addToClassCache(key, value) { + if (classCache.size >= MAX_CLASS_CACHE_SIZE) { + const firstKey = classCache.keys().next().value; + classCache.delete(firstKey); + } + classCache.set(key, value); +}Then replace
classCache.set(cacheKey, ...)calls withaddToClassCache(cacheKey, ...).
36-38: Empty catch blocks silently swallow errors.Multiple catch blocks return empty arrays or null without any logging. This makes debugging production issues difficult when selectors fail unexpectedly.
Consider adding minimal error logging, at least in debug mode:
} catch (err) { + // Optional: console.debug('evaluateSelector failed:', selector, err); return []; }Also applies to: 70-72, 399-402, 886-887
1145-1824: Large inline finder algorithm duplicates external library logic.The ~700-line
getSelectorsfunction embeds a complete CSS selector finder algorithm (attributed to@medv/finder). While functional, this creates maintenance burden.Consider either:
- Extracting this to a separate file (
finder.js) and importing it- Using the actual
@medv/finderpackage as a dependency if licensing permitsThis would improve maintainability and allow independent testing.
server/src/sdk/selectorValidator.ts (4)
35-48: Navigation timeout handling is reasonable but consider shorter initial timeout.The fallback from
networkidletodomcontentloadedis a good pattern. However, 100 seconds is quite long for initial page load - consider a shorter timeout with the same fallback strategy.try { await page.goto(url, { waitUntil: "networkidle", - timeout: 100000, + timeout: 30000, }); } catch (err) { await page.goto(url, { waitUntil: "domcontentloaded", - timeout: 100000, + timeout: 30000, }); }
240-243: Use ES module imports instead of CommonJSrequire()in TypeScript.Using
require()inside an async function is unconventional in TypeScript. Move these to top-level imports for better static analysis and tree-shaking.+import * as fs from 'fs'; +import * as path from 'path'; import { Page } from 'playwright-core'; import logger from '../logger';Then remove the inline
require()calls at lines 240-243 and 305-308.
440-440: AvoidwaitForTimeout- use more reliable waiting strategies.
waitForTimeoutis discouraged in Playwright as it introduces flakiness. Consider waiting for specific conditions instead.- await this.page.waitForTimeout(2000); + // Wait for network to settle or list count to change + await this.page.waitForLoadState('networkidle').catch(() => {}); + // Or use waitForFunction to check for DOM changesFor infinite scroll testing:
- await this.page.waitForTimeout(2000); + await this.page.waitForLoadState('networkidle').catch(() => {});Also applies to: 530-530
578-584:close()method only clears reference - consider documenting lifecycle.The method sets
pageto null but doesn't actually close the browser/page. This is fine if the caller manages the page lifecycle, but document this expectation./** - * Clear page reference + * Clear page reference. Note: This does not close the browser or page. + * The caller (e.g., RemoteBrowser) is responsible for browser lifecycle. */ async close(): Promise<void> { this.page = null; logger.info('Page reference cleared'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/sdk/browserSide/pageAnalyzer.js(1 hunks)server/src/sdk/selectorValidator.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/sdk/selectorValidator.ts (2)
server/src/sdk/browserSide/pageAnalyzer.js (31)
isXPath(14-14)selector(2133-2133)selector(2142-2142)selector(2160-2160)selector(2170-2170)selector(2189-2189)selector(2199-2199)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(2574-2574)src/helpers/clientPaginationDetector.ts (1)
evaluateSelector(290-318)
🪛 Biome (2.1.2)
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)
server/src/sdk/selectorValidator.ts
[error] 246-246: 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] 311-311: 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)
🔇 Additional comments (3)
server/src/sdk/browserSide/pageAnalyzer.js (2)
2254-2647: Well-structured element group analysis with proper shadow DOM handling.The
analyzeElementGroupsfunction implements comprehensive fingerprinting with:
- Structural signature generation
- Similarity scoring with weighted factors
- Table row special-case handling
- Shadow DOM traversal
- Serializable output format with XPath generation
The implementation handles edge cases well and returns useful metadata for workflow enrichment.
1027-1032: Pagination auto-detection has comprehensive pattern matching.The function handles multiple pagination strategies (next/prev buttons, load-more, infinite scroll) with internationalization support (French, Spanish, German patterns). The confidence scoring and fallback logic is well-thought-out.
server/src/sdk/selectorValidator.ts (1)
54-108: Well-implemented selector validation with proper XPath/CSS handling.The
validateSelectormethod correctly:
- Detects XPath vs CSS selectors
- Uses Playwright's locator API appropriately
- Extracts shadow DOM presence via element evaluation
- Returns structured validation results with proper error handling
What this PR does?
Adds support for maxun sdk
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.