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 labels
Poem
✨ Finishing Touches
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores