Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 seconds. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f32ada7a4015dc4c04bcbd366d3086cff9d7434e and 56180cc. 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 0ad77ea and f32ada7a4015dc4c04bcbd366d3086cff9d7434e. 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWhen a browser webview receives focus, its handler dismisses address-bar suggestions and looks up the browser guest’s owning group using the current app-store state. If a matching group exists, the handler focuses it. The handler is removed during cleanup. Tests cover group lookup, focus behavior, and listener cleanup. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Guest focus now selects its owning split while preserving suggestion dismissal and listener cleanup. No actionable merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, fix, scope, preserved behavior, regression trade-off, linked issue, and one automated test. However, it does not follow the repository template and omits the required ELI5, What Changed, Why, Visual Proof, Testing checklist, AI Disclosure, Review, boundary, Notes, and Checklist sections. The behavior change also lacks the required visual proof or an explicit N/A explanation. Resolution Rewrite the description using the repository template. Add the ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections. Provide before/after visual evidence for the UI behavior change, or write exactly
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. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
One real defect: the new guest-focus listener is never removed because the cleanup still references the old named handler. Details on the line.
Reviewed changes
Read the full diff end-to-end (3 files). Context checked: focusGroup (tabs-group-actions.ts), the overlay's existing onPointerDown/onFocusCapture focus re-sync (BrowserPaneOverlayLayer.tsx), the browser-tab entityId === browserTabId convention across the renderer, and the attach/rebind path (attach-browser-page-webview.ts, use-browser-page-webview-lifecycle.ts).
- New helper
browser-guest-owning-group.ts— resolves the owninggroupIdfor a browser page and callsfocusGroup; no-ops for a tab mid-move or on another worktree. - Guest
focuslistener — replaces the named dismiss handler with an anonymous arrow that also focuses the owning split, reached throughuseAppStore.getState(). - New unit test — covers the owning split, a non-browser tab with the same id, and the same page id on another worktree; exercises only the pure helper, not the listener binding.
The behavior is otherwise correct: focusGroup only mutates split state (no DOM focus pull), so there is no re-entrancy loop, and entityId === browserTabId matches every other browser-tab lookup in the renderer.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Sync update (
|
There was a problem hiding this comment.
Important
The new bind test file fails the casting scan that PR CI runs (pnpm run check:code-quality:changed), so the check stays red until its three assertions carry the required SAFETY: suppressions.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (f32ada7a40, on top of 5f358a1b91). Verified the key semantics against the actual producers: workspaceId is the browser BrowserWorkspace.id (browser-workspace-pane.tsx passes workspaceId={browserTab.id}), which is exactly the value buildBrowserUnifiedTab writes to the unified tab's entityId (runtime/web-session-tabs-sync/tab-builders.ts:51), so the new lookup matches where the previous live-page-id lookup did not.
- Named the guest-focus handler —
handleGuestFocusis now registered and removed by the same reference, so unbind detaches the listener (retires the prior review's leak finding). - Corrected the owning-tab key — the owning-tab match uses
workspaceId(stored in unifiedentityId) instead of the live page id. - Updated and added tests — the helper test follows the rename; a new bind test asserts the handler focuses the owning group and is removed on cleanup.
focusGroup only mutates split state, so a guest focus cannot pull DOM focus or re-enter. No behavior regressions found in the delta.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Guest webview focus does not bubble to the overlay, so Ctrl+Tab kept the previously focused split. Address-bar focus still stays put. Fixes stablyai#22144
Co-authored-by: Cursor <cursoragent@cursor.com>
Guest focus is registered through one helper, and cleanup removes that same function. The test uses a small listener target instead of casting a partial webview.
f32ada7 to
56180cc
Compare
Sync update (
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (f32ada7a40). The only net change is 56180cc263, which addresses that review's finding and tidies the test. Both prior Pullfrog threads are resolved.
- Extracted
bindBrowserGuestFocus— the focus handler and its add/remove now live in one exported function returning its own cleanup, with the webview typed through a narrowBrowserGuestFocusTarget;bindBrowserPageWebviewListenerscalls it and usesremoveGuestFocus()in cleanup. - Simplified the bind test — it targets
bindBrowserGuestFocusdirectly with a hand-typed fake webview, dropping all threeascasts (and the heavy module mocks) that failed the casting scan.
Verified locally: pnpm run check:code-quality:changed reports 0 findings across the 4 changed files, pnpm tc:web passes, and both new tests pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
Description
Clicking inside a browser page did not focus the split that owns that page. The overlay focuses its group on its own pointer and focus events, but a guest webview does not bubble those events. Ctrl+Tab then cycled the previously focused group.
Focused fix
In:
focusevent, resolve the browser page's unified tab and focus that tab's group.Out:
Preserves
The existing address-bar suggestion dismiss still runs on guest focus. Focusing the address bar does not emit webview
focus, so it does not move the split.focusGrouponly updates split state; it does not pull keyboard focus out of the page.Evidence
node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts --cache false src/renderer/src/components/browser-pane/host-guest/browser-guest-owning-group.test.tsThe test covers the owning split, a non-browser tab with the same id, and the same page id on another worktree.
User-regression-tradeoffs
A click in the page now makes that split the Ctrl+Tab target, which is the reported expectation. An orphaned page that has not landed in a group yet leaves the previous split focused.
Fixes #22144