fix: prevent page reload on run trigger to open remote browser #389
Conversation
WalkthroughThe pull request introduces enhancements to browser management and workflow interpretation across multiple files. The changes focus on adding a new Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/pages/MainPage.tsx (2)
93-98: Consider adding metadata when storing run info in local storage.
Right now, only thebrowserId,runId, andrecordingNameare stored, which is fine for basic use. However, storing timestamps or versioning could help identify stale or concurrent runs.
109-118: Review additional run statuses beyond'success'.
Currently, the code only notifies on'success'or defaults to'error'. You might consider special handling for other statuses (e.g.,'aborted','partial','cancelled') if they exist in the backend logic.server/src/workflow-management/classes/Interpreter.ts (1)
335-336: Add error handling for interpretation failures.
Currently,'run-completed'is hardcoded to"success"only. In cases of interpretation errors or exceptions, consider emitting a different status (e.g.,"error") so the frontend can properly display a failure notification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/browser-management/classes/BrowserPool.ts(4 hunks)server/src/browser-management/controller.ts(2 hunks)server/src/routes/record.ts(3 hunks)server/src/workflow-management/classes/Interpreter.ts(2 hunks)src/pages/MainPage.tsx(5 hunks)
🔇 Additional comments (8)
server/src/browser-management/classes/BrowserPool.ts (5)
18-19: Clarify the optionalisRobotRunbehavior.Adding
isRobotRun?: boolean;provides an easy flag for identifying robot-run browsers. However, ensure that any logic that relies on this property's truthiness also accounts for when it'sundefined.
51-57: Validate the newisRobotRunparameter inaddRemoteBrowser.Passing
isRobotRunas a distinct boolean argument helps control the browser’s run state more explicitly. Just make sure callers consistently provide the correct value based on their flow.
63-65: Confirm performance sufficiency ofhasActiveRobotRun.Using
Object.values(this.pool).some(...)to check for an active run is fine for smaller pools. If usage scales, you might consider an alternative approach to avoid scanning the entire pool each time.
67-73: Ensure no concurrency conflicts inclearRobotRunState.Resetting
isRobotRuntofalseis a simple way to clear the run state. Confirm that there’s no parallel code path that might set it back totrueafter this method is called.
84-85: Good cleanup of the run state before deletion.Calling
clearRobotRunStateprior to removing the browser ensures consistent and predictable state in the pool. Nice approach!server/src/routes/record.ts (1)
19-19: EnsurebrowserPoolis a valid singleton.Importing
browserPoolallows you to share the same pool references across modules. Double-check that it’s indeed a singleton or properly scoped to avoid initialization issues.server/src/browser-management/controller.ts (1)
62-62: Explicitly marking the browser run as a robot run.Changing
browserPool.addRemoteBrowser(id, browserSession, true, true);clarifies that these instances are indeed active and designated for robot runs. Make sure all relevant teardown/notification hooks also respect this setting.src/pages/MainPage.tsx (1)
71-78: Clarify or consolidate success logic withinreadyForRunHandler.
By commenting out the success/failure notifications and only stopping the browser on failure, the success path now resides solely in the'run-completed'event. This separation could introduce confusion or duplication. Consider either removing the remaining failure handling here, or reintroducing success-handling to keep logic consistent.Use this script to check if any other places also rely on
interpretationreturn values besides'run-completed':
| router.use('/', requireSignIn, (req: AuthenticatedRequest, res: Response, next) => { | ||
| if (browserPool.hasActiveRobotRun()) { | ||
| logger.log('debug', 'Preventing browser initialization - robot run in progress'); | ||
| return res.status(403).json({ | ||
| error: 'Cannot initialize recording browser while a robot run is in progress' | ||
| }); | ||
| } | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Race condition risk confirmed in browser pool management
The current implementation of BrowserPool lacks proper synchronization mechanisms. Multiple concurrent requests could potentially pass the hasActiveRobotRun check before any of them sets the robot run state, as the operations are not atomic. While Node.js's event loop provides some protection, it's not guaranteed to be safe in high-concurrency scenarios.
Suggested improvements:
- Implement proper locking mechanism or semaphore for robot run state changes
- Consider using atomic operations for state transitions
- Add request queuing to serialize the robot run initialization
🔗 Analysis chain
Preventing parallel sessions on robot runs.
Blocking new browser sessions when hasActiveRobotRun is true is a solid approach. For high-traffic scenarios, consider if multiple requests might pass this check simultaneously (race condition) before the run is registered. If needed, use a lock or queue to serialize these events and avoid edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Gather usage references to see how `hasActiveRobotRun` is being checked.
rg -A 4 "hasActiveRobotRun"
Length of output: 878
Script:
#!/bin/bash
# Find where robot runs are initiated and how isRobotRun is set
ast-grep --pattern 'isRobotRun = true'
rg -B 2 -A 4 "isRobotRun.*=.*true"
# Look for any synchronization mechanisms
rg -i "lock|mutex|semaphore|atomic"
Length of output: 3944
Script:
#!/bin/bash
# Get the complete BrowserPool implementation
cat server/src/browser-management/classes/BrowserPool.ts
# Find robot run related code
rg -B 2 -A 4 "robotRun|robot.*run"
Length of output: 14625
src/pages/MainPage.tsx
Outdated
| useEffect(() => { | ||
| const storedRunInfo = localStorage.getItem('runInfo'); | ||
| console.log('storedRunInfo', storedRunInfo); | ||
|
|
||
| if (storedRunInfo) { | ||
| // Parse the stored info | ||
| const { browserId, runId, recordingName } = JSON.parse(storedRunInfo); | ||
|
|
||
| // Reconnect to the specific browser's namespace | ||
| setIds({ browserId, runId }); | ||
| const socket = io(`${apiUrl}/${browserId}`, { | ||
| transports: ["websocket"], | ||
| rejectUnauthorized: false | ||
| }); | ||
|
|
||
| // Update component state with stored info | ||
| setRunningRecordingName(recordingName); | ||
| setSockets(sockets => [...sockets, socket]); | ||
|
|
||
| // Set up event listeners | ||
| socket.on('ready-for-run', () => readyForRunHandler(browserId, runId)); | ||
| socket.on('debugMessage', debugMessageHandler); | ||
| socket.on('run-completed', (status) => { | ||
| if (status === 'success') { | ||
| notify('success', t('main_page.notifications.interpretation_success', { name: recordingName })); | ||
| } else { | ||
| notify('error', t('main_page.notifications.interpretation_failed', { name: recordingName })); | ||
| } | ||
| setRunningRecordingName(''); | ||
| setCurrentInterpretationLog(''); | ||
| setRerenderRuns(true); | ||
| localStorage.removeItem('runInfo'); // Clean up stored info | ||
| }); | ||
|
|
||
| // Cleanup function | ||
| return () => { | ||
| socket.off('ready-for-run', () => readyForRunHandler(browserId, runId)); | ||
| socket.off('debugMessage', debugMessageHandler); | ||
| socket.off('run-completed'); | ||
| }; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cleanup functions use the same listener references.
Inside the useEffect, the cleanup relies on arrow callbacks that differ from those in socket.on(...). Reacting to new runs or re-renders may result in unremoved listeners. As with line 130, store references to your event handlers, then pass them consistently to on and off.
| @@ -108,6 +127,7 @@ export const MainPage = ({ handleEditRecording, initialContent }: MainPageProps) | |||
| return (socket: Socket, browserId: string, runId: string) => { | |||
| socket.off('ready-for-run', () => readyForRunHandler(browserId, runId)); | |||
| socket.off('debugMessage', debugMessageHandler); | |||
| socket.off('run-completed'); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use identical function references to remove socket listeners.
Calling socket.off('run-completed', () => ...) with a new inline arrow function won’t remove the original listener. This can lead to memory leaks or multiple event fires. Store the listener in a variable and pass the same reference to both on and off.
- socket.on('run-completed', (status) => { ... });
- socket.off('run-completed', () => { ... });
+ const handleRunCompleted = (status) => { ... };
+ socket.on('run-completed', handleRunCompleted);
+ socket.off('run-completed', handleRunCompleted);Committable suggestion skipped: line range outside the PR's diff.
amhsirak
added
the
Status: 🚫🚫 DO NOT MERGE 🚫🚫
feat: execute run as a separate job
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/routes/storage.ts (1)
480-480: Remove commented-out code if unneeded.
Leaving this line in the codebase may cause confusion about whether it should be enabled. If you no longer need to callcreateRemoteBrowserForRun, consider removing it:- // const id = createRemoteBrowserForRun(req.user.id);server/src/worker.ts (1)
40-59: Use optional chaining for safer schedule checks.
You can simplify this condition by applying optional chaining onrobot.schedule. For instance:- if (robot.schedule && robot.schedule.cronExpression && robot.schedule.timezone) { + if (robot.schedule?.cronExpression && robot.schedule?.timezone) {🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/routes/storage.ts(3 hunks)server/src/worker.ts(3 hunks)src/pages/MainPage.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/MainPage.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/worker.ts
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
server/src/routes/storage.ts (3)
491-491: VerifybrowserIdalignment with actual browser creation.
This line implies that theidof the recording inreq.params.idis reused for the browser ID. Confirm that the front end or the rest of the system does not rely on a different ID for browsers, which could lead to mismatches.
500-504: Ensure consistent handling of theisScheduledflag.
The job is added withisScheduled: false, which triggers the unscheduled workflow path in the worker. Double-check that other parts of the code correctly handle this flag and that no re-scheduling logic is executed inadvertently.
508-508: Duplicate assignment ofbrowserIdasreq.params.id.
This is the same assignment as on line 491, which may cause confusion between robot/recording IDs and browser IDs if the meaning differs. Ensure both references are intentional.server/src/worker.ts (3)
4-5: Confirm distinction between scheduled vs. direct run recordings.
Using different function names for scheduled vs. direct runs can improve clarity. Ensure that both implementations stay synchronized if they share logic.
[approve]
26-26: Evaluate defaultingisScheduledto true.
This fallback is helpful, but confirm that manual runs always passfalseto avoid erroneously calling the scheduled-run handler.
28-30: Conditional call effectively separates scheduled vs. on-demand runs.
This is a clean approach. Ensure that both functions handle unexpected job data states gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/routes/storage.ts (1)
500-504: LGTM! Consider adding error handling for the queue operation.The job addition to the workflow queue is well-placed and contains all necessary identifiers. However, if the queue operation fails, the run record would still be created but the workflow wouldn't be queued.
Consider wrapping the queue operation in a try-catch block and updating the run status if queueing fails:
}); + try { const job = await workflowQueue.add( 'run workflow', { id, runId, userId: req.user.id, isScheduled: false }, ); + } catch (error) { + await run.update({ status: 'failed', finishedAt: new Date().toLocaleString() }); + throw new Error(`Failed to queue workflow: ${error.message}`); + } const plainRun = run.toJSON();
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/src/worker.ts (4)
26-30: Add JSDoc to document the job data structure.The job processing logic looks good, but it would benefit from documentation explaining the purpose of the
isScheduledflag and when it should be set to false.+/** + * Job data structure for workflow processing + * @typedef {Object} WorkflowJobData + * @property {string} runId - The unique identifier for the run + * @property {string} userId - The user initiating the run + * @property {string} id - The workflow identifier + * @property {boolean} [isScheduled=true] - Whether this is a scheduled run (true) or manual run (false) + */ const { runId, userId, id, isScheduled = true } = job.data;
48-49: Use optional chaining for cleaner code.The condition can be simplified using optional chaining.
-if (robot.schedule && robot.schedule.cronExpression && robot.schedule.timezone) { - const nextRunAt = computeNextRun(robot.schedule.cronExpression, robot.schedule.timezone) || undefined; +if (robot.schedule?.cronExpression && robot.schedule?.timezone) { + const nextRunAt = computeNextRun(robot.schedule.cronExpression, robot.schedule.timezone) ?? undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
58-58: Improve error message specificity.The error message could be more helpful by indicating which specific property is missing.
-logger.error('Robot schedule, cronExpression, or timezone is missing.'); +logger.error(`Robot ${robot.id} missing required scheduling properties: ${[ + !robot.schedule && 'schedule', + robot.schedule && !robot.schedule.cronExpression && 'cronExpression', + robot.schedule && !robot.schedule.timezone && 'timezone' +].filter(Boolean).join(', ')}`);
50-56: Be explicit about schedule property updates.Using the spread operator with the schedule object could inadvertently include unwanted properties. Consider explicitly listing the properties that should be updated.
await robot.update({ schedule: { - ...robot.schedule, + cronExpression: robot.schedule.cronExpression, + timezone: robot.schedule.timezone, lastRunAt, nextRunAt, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/worker.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/worker.ts
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
server/src/worker.ts (2)
4-5: LGTM! Clear separation of concerns.The imports effectively distinguish between scheduled and unscheduled run recording handlers.
26-30: Verify integration with related components.The changes look good but let's verify the integration points:
Also applies to: 40-59
✅ Verification successful
Integration verification completed successfully
The changes are properly integrated:
- Queue jobs correctly set the
isScheduledflag- Handlers are properly imported and used
- Scheduling mechanism is consistently implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration points with related components # Check if handleRunRecording is properly exported from both modules echo "Checking exports from workflow management modules..." rg -A 1 "export.*handleRunRecording" "server/src/workflow-management/" # Check if isScheduled flag is properly passed from all callers echo "Checking job creation with isScheduled flag..." rg -A 3 "workflowQueue.add" # Check for any direct Robot model updates that might need the new conditional logic echo "Checking Robot model updates..." rg "Robot.update.*schedule" --type tsLength of output: 1096
Script:
#!/bin/bash # Get more context about the run recording handlers and their usage echo "Checking handleScheduledRunRecording export and implementation..." ast-grep --pattern 'export async function handleScheduledRunRecording($_) { $$$ }' echo "\nChecking complete context of queue additions..." rg -B 5 -A 10 'workflowQueue.add' echo "\nChecking for any other related scheduling functions..." rg "RunRecording.*schedule" --type tsLength of output: 2303
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
server/src/workflow-management/record.ts (5)
52-55: Clarify or remove the function if not essential.
resetRecordingStatesimply reassignsbrowserIdandidto empty strings. This has no side effects beyond modifying local variables. Unless there's a future plan to persist or propagate these changes, consider removing the function or making its purpose explicit.
105-105: Avoid assignment within an expression.
Passing(newPage: Page) => currentPage = newPagecan be confusing. Consider moving the assignment into a separate statement or function for clarity.Example refactor:
- (newPage: Page) => currentPage = newPage, + function handleNewPage(newPage: Page) { + currentPage = newPage; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
131-135: Consider optional chaining for safer checks.
Instead of performing repeated logical AND checks, optional chaining can enhance readability, e.g.,updatedRun.dataValues.binaryOutput?.["item-0"]. However, this is purely stylistic and not critical.🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
120-120: Use a consistent date format for storing timestamps.
Currently usingnew Date().toLocaleString()can produce time zone–dependent variations. Storing timestamps in an ISO format (new Date().toISOString()) is often more predictable and consistent across different systems.
198-224:userIdis not used.
The function signature requiresuserId, but there's no usage withinhandleRunRecording. If it's not intended for future expansions, removing or handling it might improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/workflow-management/record.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/workflow-management/record.ts
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 105-105: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
server/src/workflow-management/record.ts (3)
23-50: Robust error handling and clear flow.
ThereadyForRunHandlerfunction effectively captures errors and logs them, then cleans up the browser in case of failure. It emits the run status to the client, ensuring feedback loops are well-managed. Nice job!🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
57-66: Declarative approach looks good.
AddGeneratedFlagsfunction effectively clones the workflow and prepends the "generated" action. This is a straightforward approach, though it might involve extra memory usage for large workflows. Overall, it appears well-structured and clear.
68-142: Overall, this function is well-organized.
executeRunthoughtfully fetches run and recording data, handles browser interactions, and updates run status as needed. The callbacks for interpretation, logging, and storing binary outputs are cohesive. Good use ofdestroyRemoteBrowseron success or error.🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| function cleanupSocketListeners(socket: Socket, browserId: string, id: string) { | ||
| socket.off('ready-for-run', () => readyForRunHandler(browserId, id)); | ||
| logger.info(`Cleaned up listeners for browserId: ${browserId}, runId: ${id}`); | ||
| } |
There was a problem hiding this comment.
Ensure event listeners are removed properly.
The callback passed to socket.off is a newly created arrow function, which doesn’t match the one registered with socket.on. This might prevent proper cleanup of the original listener. Consider storing a reference to the original callback.
Possible fix:
- socket.off('ready-for-run', () => readyForRunHandler(browserId, id));
+ const readyForRunCallback = () => readyForRunHandler(browserId, id);
// ...
+ socket.on('ready-for-run', readyForRunCallback);
// ...
+ socket.off('ready-for-run', readyForRunCallback);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/src/workflow-management/record.ts (1)
228-231:⚠️ Potential issueEnsure event listeners are removed properly.
The callback passed to
socket.offis a newly created arrow function, which doesn't match the one registered withsocket.on. This might prevent proper cleanup of the original listener.+const readyForRunCallback = (browserId: string, id: string) => () => readyForRunHandler(browserId, id); + export async function handleRunRecording(id: string, userId: string, runId: string) { try { // ... - socket.on('ready-for-run', () => readyForRunHandler(id, runId)); + const callback = readyForRunCallback(id, runId); + socket.on('ready-for-run', callback); // ... socket.on('disconnect', () => { - cleanupSocketListeners(socket, id, runId); + socket.off('ready-for-run', callback); + logger.info(`Cleaned up listeners for browserId: ${id}, runId: ${runId}`); }); } } - -function cleanupSocketListeners(socket: Socket, browserId: string, id: string) { - socket.off('ready-for-run', () => readyForRunHandler(browserId, id)); - logger.info(`Cleaned up listeners for browserId: ${browserId}, runId: ${id}`); -}
🧹 Nitpick comments (4)
server/src/workflow-management/record.ts (4)
24-44: Consider using optional chaining and DRY up the code.
- Use optional chaining to safely access the success property
- Consider extracting the common cleanup code to avoid duplication
- if (result && result.success) { + if (result?.success) { logger.info(`Interpretation of ${id} succeeded`); - resetRecordingState(browserId, id); - return result.interpretationInfo; + const interpretationInfo = result.interpretationInfo; + await cleanup(); + return interpretationInfo; } else { logger.error(`Interpretation of ${id} failed`); - await destroyRemoteBrowser(browserId); - resetRecordingState(browserId, id); - return null; + await cleanup(); + return null; } + async function cleanup() { + await destroyRemoteBrowser(browserId); + resetRecordingState(browserId, id); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
96-101: Avoid assignment in callback function.The assignment to
currentPagein the callback makes the code harder to understand and maintain.- (newPage: Page) => currentPage = newPage, + async (newPage: Page) => { + currentPage = newPage; + return currentPage; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 99-99: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
126-136: Use optional chaining for safer property access.The nested property access could throw if any intermediate value is undefined.
- if (updatedRun.dataValues.binaryOutput && updatedRun.dataValues.binaryOutput["item-0"]) { + if (updatedRun.dataValues?.binaryOutput?.["item-0"]) { extractedScreenshotsCount = 1; } - if (updatedRun.dataValues.serializableOutput && updatedRun.dataValues.serializableOutput["item-0"]) { + if (updatedRun.dataValues?.serializableOutput?.["item-0"]) { const itemsArray = updatedRun.dataValues.serializableOutput["item-0"];🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
209-212: Consider using URL constructor for better URL handling.The current URL construction using template literals could be improved using the URL constructor for better validation and handling.
- const socket = io(`${process.env.BACKEND_URL ? process.env.BACKEND_URL : 'http://localhost:8080'}/${id}`, { + const baseUrl = process.env.BACKEND_URL || 'http://localhost:8080'; + const url = new URL(id, baseUrl).toString(); + const socket = io(url, { transports: ['websocket'], rejectUnauthorized: false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/workflow-management/record.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/workflow-management/record.ts
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
server/src/workflow-management/record.ts (1)
1-22: LGTM! Well-organized imports and setup.The imports are logically grouped and the stealth plugin setup is properly configured.
| (newPage: Page) => currentPage = newPage, | ||
| plainRun.interpreterSettings | ||
| ); | ||
|
|
||
| const binaryOutputService = new BinaryOutputService('maxun-run-screenshots'); | ||
| const uploadedBinaryOutput = await binaryOutputService.uploadAndStoreBinaryOutput( | ||
| run, | ||
| interpretationInfo.binaryOutput | ||
| ); | ||
|
|
||
| await destroyRemoteBrowser(plainRun.browserId); | ||
|
|
||
| const updatedRun = await run.update({ | ||
| ...run, | ||
| status: 'success', | ||
| finishedAt: new Date().toLocaleString(), | ||
| browserId: plainRun.browserId, | ||
| log: interpretationInfo.log.join('\n'), | ||
| serializableOutput: interpretationInfo.serializableOutput, | ||
| binaryOutput: uploadedBinaryOutput, | ||
| }); | ||
|
|
||
|
|
||
| let totalRowsExtracted = 0; | ||
| let extractedScreenshotsCount = 0; | ||
| let extractedItemsCount = 0; | ||
|
|
||
| if (updatedRun.dataValues.binaryOutput && updatedRun.dataValues.binaryOutput["item-0"]) { | ||
| extractedScreenshotsCount = 1; | ||
| } | ||
|
|
||
| if (updatedRun.dataValues.serializableOutput && updatedRun.dataValues.serializableOutput["item-0"]) { | ||
| const itemsArray = updatedRun.dataValues.serializableOutput["item-0"]; | ||
| extractedItemsCount = itemsArray.length; | ||
| totalRowsExtracted = itemsArray.reduce((total: number, item: any) => { | ||
| return total + Object.keys(item).length; | ||
| }, 0); | ||
| } | ||
|
|
||
| logger.info(`Extracted Items Count: ${extractedItemsCount}`); | ||
| logger.info(`Extracted Screenshots Count: ${extractedScreenshotsCount}`); | ||
| logger.info(`Total Rows Extracted: ${totalRowsExtracted}`); | ||
|
|
||
|
|
||
| capture('maxun-oss-run-created-manual', { | ||
| runId: id, | ||
| created_at: new Date().toISOString(), | ||
| status: 'success', | ||
| extractedItemsCount, | ||
| totalRowsExtracted, | ||
| extractedScreenshotsCount, | ||
| }); | ||
|
|
||
| // Handle Google Sheets integration | ||
| try { | ||
| googleSheetUpdateTasks[plainRun.runId] = { | ||
| robotId: plainRun.robotMetaId, | ||
| runId: plainRun.runId, | ||
| status: 'pending', | ||
| retries: 5, | ||
| }; | ||
| await processGoogleSheetUpdates(); | ||
| } catch (err: any) { | ||
| logger.error(`Failed to update Google Sheet for run: ${plainRun.runId}: ${err.message}`); | ||
| } | ||
|
|
||
| serverIo.of(plainRun.browserId).emit('run-completed', 'success'); | ||
|
|
||
| return { | ||
| success: true, | ||
| interpretationInfo: updatedRun.toJSON() | ||
| }; | ||
|
|
||
| } catch (error: any) { | ||
| logger.error(`Error running robot: ${error.message}`); | ||
| const run = await Run.findOne({ where: { runId: id } }); | ||
| if (run) { | ||
| await run.update({ | ||
| status: 'failed', | ||
| finishedAt: new Date().toLocaleString(), | ||
| }); | ||
|
|
||
| const plainRun = run.toJSON(); | ||
| serverIo.of(plainRun.browserId).emit('run-completed', 'failed'); | ||
| } | ||
|
|
||
| capture('maxun-oss-run-created-manual', { | ||
| runId: id, | ||
| created_at: new Date().toISOString(), | ||
| status: 'failed', | ||
| error_message: error.message, | ||
| }); | ||
|
|
||
|
|
||
| return { | ||
| success: false, | ||
| error: error.message, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down this large function into smaller, focused functions.
The function handles multiple responsibilities including database operations, browser management, workflow interpretation, and analytics. Consider extracting these into separate functions for better maintainability.
Example structure:
async function executeRun(id: string) {
try {
const { run, recording } = await fetchRunData(id);
const browser = await setupBrowser(run);
const interpretationResult = await interpretWorkflow(recording, browser);
await handleBinaryOutput(run, interpretationResult);
await updateRunStatus(run, interpretationResult);
await handleGoogleSheetsIntegration(run);
await cleanup(run);
return formatSuccess(interpretationResult);
} catch (error) {
await handleError(error, id);
return formatError(error);
}
}🧰 Tools
🪛 Biome (1.9.4)
[error] 99-99: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 130-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| function resetRecordingState(browserId: string, id: string) { | ||
| browserId = ''; | ||
| id = ''; | ||
| } |
There was a problem hiding this comment.
Fix ineffective state reset logic.
The current implementation of resetRecordingState is ineffective as it only modifies the local parameter variables. These changes don't affect the original values passed to the function.
Consider one of these approaches:
- Return the reset values and update them in the caller:
-function resetRecordingState(browserId: string, id: string) {
- browserId = '';
- id = '';
+function resetRecordingState(): { browserId: string, id: string } {
+ return { browserId: '', id: '' };
}- Use a state object that can be modified:
+interface RecordingState {
+ browserId: string;
+ id: string;
+}
+
+const recordingState: RecordingState = {
+ browserId: '',
+ id: ''
+};
+
+function resetRecordingState() {
+ recordingState.browserId = '';
+ recordingState.id = '';
+}Committable suggestion skipped: line range outside the PR's diff.
RohitR311
added
Type: Bug
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
server/src/routes/storage.ts (2)
698-703: Fix duplicate cron expressions for minutes and hours frequency.The cron expressions for
MINUTESandHOURSfrequencies are identical, which could lead to incorrect scheduling behavior.switch (runEveryUnit) { case 'MINUTES': - cronExpression = `${startMinutes} */${runEvery} * * *`; + cronExpression = `*/${runEvery} * * * *`; break; case 'HOURS': - cronExpression = `${startMinutes} */${runEvery} * * *`; + cronExpression = `${startMinutes} */${runEvery} * * *`; break;
730-740: Clean up existing jobs before creating new schedule.The endpoint should remove any existing scheduled jobs for this robot before creating a new schedule to prevent duplicate executions.
+ // Remove existing jobs before creating new schedule + const existingJobs = await workflowQueue.getJobs(['delayed', 'waiting']); + for (const job of existingJobs) { + if (job.data.id === id) { + await job.remove(); + } + } + // Create the job in the queue with the cron expression const job = await workflowQueue.add( 'run workflow',
🧹 Nitpick comments (2)
server/src/worker.ts (1)
40-61: Add defensive programming and use optional chaining.While the logic is correct, consider these improvements for better robustness:
- Use optional chaining for safer property access
- Add defensive checks for robot and schedule properties
Apply this diff to improve the code:
- if (job.data.isScheduled) { - const robot = await Robot.findOne({ where: { 'recording_meta.id': job.data.id } }); - if (robot) { - // Update `lastRunAt` to the current time - const lastRunAt = new Date(); - - // Compute the next run date - if (robot.schedule && robot.schedule.cronExpression && robot.schedule.timezone) { - const nextRunAt = computeNextRun(robot.schedule.cronExpression, robot.schedule.timezone) || undefined; - await robot.update({ - schedule: { - ...robot.schedule, - lastRunAt, - nextRunAt, - }, - }); - } else { - logger.error('Robot schedule, cronExpression, or timezone is missing.'); - } + if (job.data?.isScheduled) { + const robot = await Robot.findOne({ where: { 'recording_meta.id': job.data?.id } }); + if (!robot) { + logger.error('Robot not found.'); + return; + } + + // Update `lastRunAt` to the current time + const lastRunAt = new Date(); + + // Compute the next run date + if (!robot.schedule?.cronExpression || !robot.schedule?.timezone) { + logger.error('Robot schedule, cronExpression, or timezone is missing.'); + return; + } + + const nextRunAt = computeNextRun(robot.schedule.cronExpression, robot.schedule.timezone) || undefined; + await robot.update({ + schedule: { + ...robot.schedule, + lastRunAt, + nextRunAt, + }, + });🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
server/src/routes/storage.ts (1)
499-502: Add error handling for the workflow queue operation.While adding the job to the workflow queue is a good addition, consider:
- Adding specific error handling for queue operations
- Including the job ID in the response for better tracking
const job = await workflowQueue.add( 'run workflow', { id, runId, userId: req.user.id, isScheduled: false }, ); + + const plainRun = run.toJSON(); + + return res.send({ + browserId: id, + runId: plainRun.runId, + robotMetaId: recording.recording_meta.id, + jobId: job.id, // Add job ID to response }); } catch (e) { const { message } = e as Error; + // Handle queue-specific errors + if (e instanceof Error && e.message.includes('queue')) { + logger.log('error', `Queue error while creating run: ${message}`); + return res.status(503).send({ error: 'Service temporarily unavailable' }); + } logger.log('info', `Error while creating a run with robot id: ${req.params.id} - ${message}`); return res.send(''); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/routes/storage.ts(1 hunks)server/src/worker.ts(3 hunks)src/pages/MainPage.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/MainPage.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/worker.ts
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
server/src/worker.ts (3)
4-5: LGTM! Clear distinction between scheduled and unscheduled run handlers.The import aliasing effectively differentiates between the two types of run handlers.
26-26: LGTM! Backward compatible addition ofisScheduledproperty.The default value of
truemaintains backward compatibility with existing job data.
28-30: LGTM! Clear conditional handling of scheduled vs unscheduled runs.The logic correctly routes to the appropriate handler and passes the required parameters.
Summary by CodeRabbit
New Features
Bug Fixes
Chores