fix: prevent page reload on run trigger to open remote browser #386
Conversation
WalkthroughThis pull request introduces enhancements to the browser management system, focusing on tracking robot run states across multiple components. The changes include adding an Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (9)
server/src/browser-management/classes/BrowserPool.ts (3)
18-19: Add documentation for the newisRobotRunproperty.
This helps future readers understand its purpose.interface BrowserPoolInfo { browser: RemoteBrowser, active: boolean, + /** + * Indicates whether the browser is part of a robot run. + */ isRobotRun?: boolean; }
67-72: Consider logging a warning if the browser does not exist.
Currently,this.pool[id]is checked, but if the ID is invalid, no message is logged. This may be acceptable depending on your design, but if missing IDs are unexpected, a warning or error log could help identify issues.public clearRobotRunState(id: string): void { if (this.pool[id]) { this.pool[id].isRobotRun = false; logger.log('debug', `Robot run state cleared for browser ${id}`); + } else { + logger.log('warn', `No browser with id: ${id} found in the pool to clear robot run state.`); } }
84-85: Remove or justify the extra robot-run state clearing before deletion.
ClearingisRobotRunimmediately before deletion may be unnecessary overhead if you have no side effects. If logging or side effects are intended, consider explaining it in a comment.src/pages/MainPage.tsx (3)
71-78: Restore success notifications or confirm removal is intentional.
You’ve commented out the success/failure notifications, which might reduce user feedback. Consider reintroducing them or clarifying the user experience strategy.
93-98: Watch for potential sensitive data inlocalStorage.
Storing identifiers likebrowserIdorrunIdis typically acceptable, but ensure no sensitive information is placed here without encryption.
108-118: Consider handling additional statuses beyond “success.”
The code checks only'success', defaulting all other cases to an “error.” If more nuanced statuses exist (e.g., “partialSuccess”), handle them here for clarity.server/src/routes/record.ts (3)
19-19: Consider restructuring the import to avoid potential circular dependencies.Importing
browserPoolfrom '../server' could lead to circular dependencies. Consider moving thebrowserPoolinstance to a separate service file that both the server and routes can import from.
37-45: LGTM! Well-structured middleware implementation.The middleware effectively prevents concurrent operations between recording and robot runs. The status code and error message are appropriate.
Consider adding error handling and request logging:
router.use('/', requireSignIn, (req: AuthenticatedRequest, res: Response, next) => { - if (browserPool.hasActiveRobotRun()) { + try { + if (browserPool.hasActiveRobotRun()) { + logger.log('debug', `Denied browser initialization request from user ${req.user?.id} - robot run in progress`); + return res.status(403).json({ + error: 'Cannot initialize recording browser while a robot run is in progress' + }); + } + next(); + } catch (error) { + logger.log('error', `Error checking robot run status: ${error}`); + return res.status(500).json({ + error: 'Internal server error while checking robot run status' + }); + } - } - next(); });
46-47: Remove unnecessary empty line.Consider removing one of the consecutive empty lines to maintain consistent spacing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/browser-management/classes/BrowserPool.ts(3 hunks)server/src/browser-management/controller.ts(1 hunks)server/src/routes/record.ts(2 hunks)server/src/workflow-management/classes/Interpreter.ts(1 hunks)src/pages/MainPage.tsx(5 hunks)
🔇 Additional comments (6)
server/src/browser-management/classes/BrowserPool.ts (2)
51-57: InitializeisRobotRunconsistently with the new parameter.
The addition of theisRobotRunparameter aligns well with your approach to manage state in the browser pool.
63-65: Efficiently check for a running robot.
Using.some()to detect any active robot run is a simple, concise solution.server/src/browser-management/controller.ts (1)
62-62: Verify usage of the newisRobotRunparameter.
Passingtruefor bothactiveandisRobotRunis logical. Ensure that downstream components handle theisRobotRunflag appropriately (e.g., preventing multiple runs, raising relevant events, etc.).✅ Verification successful
isRobotRunparameter is properly implemented and handled
The BrowserPool implementation includes comprehensive robot run management with proper state tracking, prevention of multiple runs viahasActiveRobotRun(), and clean state management throughclearRobotRunState(). The implementation aligns with the expected behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that references to `addRemoteBrowser` handle the new isRobotRun parameter correctly. rg -A 5 'addRemoteBrowser'Length of output: 1381
Script:
#!/bin/bash # Search for robot-related code and multiple run prevention in BrowserPool ast-grep --pattern 'class BrowserPool { $$$ isRobotRun $$$ }' # Search for robot-related event handlers and prevention logic rg -i "robot.*run|prevent.*run|multiple.*run" server/src/browser-management/ # Look for usage of pool entries rg "this\.pool\[.*\]" server/src/browser-management/Length of output: 1859
src/pages/MainPage.tsx (2)
130-130: Unsubscribe from ‘run-completed’ event properly.
This call is correct. The removal helps prevent memory leaks when the component unmounts or re-renders.
145-186: Handle possible JSON parse errors and remove console logs in production.
UsingJSON.parseonrunInfomight fail if the data is invalid. Also, consider removing or guarding theconsole.logstatement for production environments.useEffect(() => { const storedRunInfo = localStorage.getItem('runInfo'); - console.log('storedRunInfo', storedRunInfo); + if (process.env.NODE_ENV !== 'production') { + console.log('storedRunInfo', storedRunInfo); + } if (storedRunInfo) { let runInfo; try { + runInfo = JSON.parse(storedRunInfo); } catch (e) { + notify('error', t('main_page.notifications.invalid_run_info')); + return; } ... } }, []);✅ Verification successful
The review comment is valid and should be implemented as suggested.
The suggested changes improve error handling for JSON parsing and introduce proper production environment checks for logging. Consider applying similar patterns to other unguardedJSON.parsecalls and console logs throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for JSON.parse usage echo "=== JSON.parse patterns ===" ast-grep --pattern 'JSON.parse($_)' echo -e "\n=== Console.log patterns ===" rg "console\.log\(" -A 1 echo -e "\n=== Environment checks ===" rg "process\.env\.NODE_ENV"Length of output: 19406
server/src/routes/record.ts (1)
37-45: Verify alignment with PR objective.The changes prevent browser initialization during robot runs, but it's not clear how this addresses the PR objective of fixing browser reload functionality. Could you clarify how this middleware helps solve the issue where "Browser reload does not trigger remote browser to open"?
Let's verify the browser reload functionality:
| this.socket.emit('run-completed', "success"); | ||
|
|
There was a problem hiding this comment.
Ensure failure cases also emit an appropriate run-completed status.
Currently, the code always emits "success". It may confuse the client if an error occurs. Consider handling failing or partial outcomes too.
this.socket.emit('run-completed', "success");
+// Proposed example:
+// if (status === 'success') {
+// this.socket.emit('run-completed', 'success');
+// } else {
+// this.socket.emit('run-completed', 'error');
+// }Committable suggestion skipped: line range outside the PR's diff.
amhsirak
changed the title
2 participants
Closes: #374
Summary by CodeRabbit
New Features
Bug Fixes
Refactor