fix: recorder crashes #742
Conversation
WalkthroughAdd defensive guards that check Playwright Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Action
participant G as Generator.onClick
participant P as Playwright Page
U->>G: onClick(INPUT/TEXTAREA, selector)
G->>P: Check page.isClosed()
alt Page is closed
note right of G: Debug log and safe return
G-->>U: Return early (skip page.evaluate)
else Page open
G->>P: page.evaluate(...) to compute cursor position
P-->>G: Cursor position
G-->>U: Continue onClick flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/workflow-management/selector.ts (1)
1987-1991: Boolean bug: overlay check is effectively always trueThe condition uses negation with != null in a way that always evaluates to true when hoveredElement is non-null, ignoring overlay-controls. This can produce incorrect selector generation.
- if ( - hoveredElement != null && - !hoveredElement.closest('#overlay-controls') != null - ) { + if ( + hoveredElement != null && + hoveredElement.closest('#overlay-controls') == null + ) {
🧹 Nitpick comments (7)
server/src/workflow-management/selector.ts (7)
24-27: Good defensive guard before page.evaluate; confirm return-type consistencyThis prevents the crash on navigations. Minor: this branch returns null, while the JSDoc above says undefined when not found. Consider standardizing on a single sentinel (null) across codepaths and updating the doc to match.
309-312: Unify debug message wording (drop “else branch”)Message suffix “(else branch)” is noisy and leaks control-flow details. Prefer a consistent message across branches.
- logger.debug('Page is closed, cannot get element information (else branch)'); + logger.debug('Page is closed, cannot get element information');
849-852: Mirror guard reads well; keep behavior symmetrical across branchesLGTM. Same note as above about consistent logging copy if you decide to deduplicate messages.
8-15: Docs say “undefined” but code now returns “null” in several pathsUpdate the return annotation to include null (and ideally remove undefined to keep one sentinel), or adapt code to consistently return undefined. I recommend null for explicitness.
Proposed doc tweak:
/** * Checks the basic info about an element and returns a BaseActionInfo object. * If the element is not found, returns null. * ... * @returns {Promise<BaseActionInfo|null>} */
613-616: Standardize error logging (console.error vs logger)Elsewhere you use logger.log('error', ...). Consider switching these console.error calls to logger for uniformity and better transport/filtering.
- console.error('Error while retrieving selector:', message); - console.error('Stack:', stack); + logger.log('error', `Error while retrieving selector: ${message}`); + logger.log('error', `Stack: ${stack}`);
24-27: Consider a tiny “guardPageOpen” to DRY up repeated checksYou’ve added six identical guards; factoring them reduces duplication and mis-message drift.
Example utility (can live near top of this module):
function guardPageOpen(page: Page, debugMsg: string): boolean { if (page.isClosed()) { logger.debug(debugMsg); return true; } return false; }Usage:
if (guardPageOpen(page, 'Page is closed, cannot get element information')) return null; // ... if (guardPageOpen(page, 'Page is closed, cannot get element rect')) return null;Also applies to: 309-312, 622-625, 849-852, 1095-1098, 2033-2036, 2391-2394, 2756-2759
1095-1098: Action: IntegratesafeEvaluateand Replace Directpage.evaluateCallsWe verified that there are no strict
undefinedcomparisons nor unsafe property accesses on potentiallynullselector results. However, direct calls topage.evaluateremain throughout the codebase and should be refactored to use the newsafeEvaluatehelper to gracefully handle “Execution context was destroyed” and related errors.Key locations to update:
- server/src/workflow-management/classes/Generator.ts
• Line 548:const positionAndCursor = await page.evaluate(...)- server/src/workflow-management/selector.ts
• Line 28, 313, 626, 853, 1098: initialelementInfo,rect, andselectorsevaluations
• Line 2037, 2395, 2760: additional selector and child-selectors evaluations- server/src/browser-management/classes/RemoteBrowser.ts
• Line 487: injectable script injection viapage.evaluate(getInjectableScript())
• Line 712: list-selector/fields evaluation withpage.evaluate(...)For each of these, wrap the existing
page.evaluatecall insafeEvaluate(page, fn, arg, debugMsg)so that transient navigation or context-teardown errors are downgraded to a debug log and returnnullinstead of bubbling up. This centralizes error handling, reduces noise, and preserves existing control flow (null checks already in place).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/src/workflow-management/selector.ts(8 hunks)
🔇 Additional comments (4)
server/src/workflow-management/selector.ts (4)
622-625: Solid guard to avoid rect-evaluate on closed pageThis is aligned with the PR objective and should eliminate a common crash path.
2033-2036: Non-unique selectors: safe fallback on closed page is appropriateReturning a stable object shape with empty generalSelector is a good choice. Ensure downstream code treats both null (evaluate returning null) and { generalSelector: '' } equivalently as “not found.”
2391-2394: List branch guard mirrors non-list branch; LGTMConsistent behavior across both branches. Same downstream handling note as above.
2756-2759: Child selectors: early bail-out is correctReturning [] on a closed page aligns with callers expecting an array. Good choice.
closes #740 #739 #
Summary by CodeRabbit
Bug Fixes
Chores