fix: add page closed condition for highlighter #558
Conversation
WalkthroughThe Changes
Poem
✨ Finishing Touches
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. 🪧 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 (
|
amhsirak
changed the title
amhsirak
added
Type: Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
server/src/browser-management/inputHandlers.ts (1)
225-242: 💡 Verification agent🧩 Analysis chain
Consider applying similar checks to other handler functions
The
handleWheelfunction already has a similar check for closed pages, but other handlers likehandleKeydown,handleKeyup, etc. might benefit from the same pattern to ensure robust handling of browser state.
🏁 Script executed:
#!/bin/bash # Check for other handlers that might benefit from page.isClosed() checks # Look for handler functions that don't have isClosed checks echo "Handler functions without isClosed checks:" rg "const handle\w+ = async \(generator: WorkflowGenerator, page: Page" -A 5 | \ rg -v "if \(page\.isClosed\(\)\)" | \ rg "await page\." -B 5 # Count how many handler functions check for closed pages echo -e "\nCount of handlers that check for page.isClosed():" rg "if \(page\.isClosed\(\)\)" --countLength of output: 3340
Apply page.isClosed() guards to all input handlers
Several other handlers in server/src/browser-management/inputHandlers.ts don’t check whether the target page is closed before performing operations. To prevent unhandled errors and no-ops against a closed page, add the same
if (page.isClosed()) { logger.log('debug', …); return; }guard at the top of each handler:• handleMousedown
• handleKeydown
• handleKeyup
• handleChangeUrl
• handleRefresh
• handleGoBack
• handleGoForwardYou can copy the pattern from handleMousemove (or factor out a small HOC/helper) to keep the code DRY.
🧹 Nitpick comments (1)
server/src/browser-management/inputHandlers.ts (1)
232-236: Consider optimizing the throttle implementationThe throttled function is always created and called, even when the page is closed. While the inner function now has a guard clause, it would be slightly more efficient to check if the page is closed before setting up the throttle at all.
- throttle(async () => { - if (!page.isClosed()) { - await generator.generateDataForHighlighter(page, { x, y }); - } - }, 100)(); + if (!page.isClosed()) { + throttle(async () => { + await generator.generateDataForHighlighter(page, { x, y }); + }, 100)(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/browser-management/inputHandlers.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/src/browser-management/inputHandlers.ts (1)
src/helpers/inputHelpers.ts (1)
throttle(9-20)
🔇 Additional comments (2)
server/src/browser-management/inputHandlers.ts (2)
227-230: Good defensive programming!Adding this check to prevent mouse movement actions on closed pages is an excellent improvement. This will help avoid potential errors when attempting to interact with a page that's no longer available.
233-235: Good enhancement to prevent unnecessary processingWrapping the highlighter data generation in a condition that checks if the page is still open prevents unnecessary work and potential errors when the page has been closed between the mouse move and the throttled callback execution.
closes #559
Summary by CodeRabbit