Stop the browser guest outliving the overlays drawn over it - #99
Conversation
Three faults in the task browser panel, all pre-existing and all from the guest being a `<webview>` on `document.body` rather than an element in the page. Nothing occludes it. An app-wide dialog -- settings, sign-in, the shortcut guide, the file viewer's expand modal, the chat's image preview -- draws its dim layer over the page and the guest keeps painting straight over the top of it. Opening a dialog is also not a tab switch, which is the only park signal the slot otherwise gets, so the panel now reports coverage itself. Cmd+F stops working after any of those closes. The find opener is a single slot; an overlay's own host claims it, clears it on unmount, and the panel -- whose inputs never changed -- never re-registers. Both hooks now read one coverage value, so the panel gives up the slot and takes it back. A load failure survives a session switch. `targetId` changes in place when the selected session changes, with no remount, so the next session's guest is parked behind an error notice naming the previous session's URL. The failure is stamped with the guest it happened on and filtered on read, which costs no extra render and never briefly shows a failed page as fine.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change detects when overlays cover browser guests. Covered guests are parked and excluded from Cmd+F registration. Browser load errors remain associated with their originating target. ChangesBrowser guest coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TaskBrowserPanel
participant useIsGuestCovered
participant useBrowserSlot
participant useBrowserFind
TaskBrowserPanel->>useIsGuestCovered: Read overlay coverage
TaskBrowserPanel->>useBrowserSlot: Pass covered state
useBrowserSlot->>useBrowserSlot: Park guest and clear emulation
TaskBrowserPanel->>useBrowserFind: Pass covered state
useBrowserFind->>useBrowserFind: Skip Cmd+F registration
TaskBrowserPanel->>TaskBrowserPanel: Match load errors to targetId
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/studio/src/client/components/task/browser-panel.tsx (1)
185-200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore event handlers after the webview target changes.
targetIdis changed only via the prop, while the listener cleanup runs only onactive/targetIdeffect teardown (apps/studio/src/client/components/task/browser-panel.tsx:163-226). Between a rerender and cleanup, the oldwebview.addEventListener(...)callbacks still run with their capturedtargetId, anddid-navigate/did-start-loadingclearfailurewhiledid-fail-loadreplaces it withtargetId: detail.targetId, affecting whichever target becomes current next. Keep a current rendered target ref, update it each render, and return from each callback whencurrentRef !== targetId. Add a regression test that rerenders the panel to another target, dispatches an old-target loading event before cleanup, and leaves the new target failure visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/studio/src/client/components/task/browser-panel.tsx` around lines 185 - 200, Update the webview event handlers in the panel component to ignore callbacks from stale targets by maintaining a ref to the currently rendered target, updating it on each render, and returning early from each handler when the ref differs from its captured targetId. Ensure did-navigate, did-start-loading, and did-fail-load cannot modify the new target’s failure state after a target switch, and add a regression test covering an old-target loading event dispatched before listener cleanup while preserving the new target’s visible failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/studio/src/client/components/task/browser-panel.tsx`:
- Around line 185-200: Update the webview event handlers in the panel component
to ignore callbacks from stale targets by maintaining a ref to the currently
rendered target, updating it on each render, and returning early from each
handler when the ref differs from its captured targetId. Ensure did-navigate,
did-start-loading, and did-fail-load cannot modify the new target’s failure
state after a target switch, and add a regression test covering an old-target
loading event dispatched before listener cleanup while preserving the new
target’s visible failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92f2e620-5d21-47c9-b2de-3a8b5afa2de3
📒 Files selected for processing (5)
apps/studio/src/client/atoms/studio-modal.tsapps/studio/src/client/components/task/browser-panel.tsxapps/studio/src/client/hooks/use-browser-find.tsapps/studio/src/client/hooks/use-browser-slot.tsapps/studio/src/client/hooks/use-guest-covered.ts
The palette is the same shape as the dialogs the coverage hook already knows about -- a Radix dialog with a dim layer, mounted app-wide -- but its open state is its own atom rather than the studio-modal slot, so nothing was reporting it. Cmd+K over a browser panel left the page painting over the palette.
The panel's webview listeners outlive `targetId` changing in place, by the frame between the render and the effect teardown. A did-navigate or did-start-loading arriving from the previous guest in that window cleared the failure whatever it was stamped with, so returning to a failed session while the agent drove the one you left dropped its error notice and unparked its guest over the slot. The clear is now guarded on the listener's own guest, matching the write side.
1 participant
Three pre-existing faults in the task browser panel, all traceable to one fact: the guest is a
<webview>ondocument.body, not an element in the page. 48 lines.Split out of #97 so they aren't hostage to the browser-abstraction redesign.
1. Guests paint through app-wide dialogs
Open Settings, sign-in, the shortcut guide, the file viewer's expand modal, or the chat's image preview while a browser panel is showing, and the page keeps drawing on top of the dialog's dim layer. Nothing occludes a body-mounted guest, and opening a dialog isn't a tab switch — which is the only park signal
useBrowserSlototherwise receives. The panel now reports coverage itself.2. Cmd+F dies after any of those closes
setBrowserFindOpeneris a single slot. The overlay's own host claims it, clears it on unmount, and the panel — whose inputs never changed — never re-registers, so Cmd+F silently stops working until the panel is reopened. Both hooks now read one coverage value, so the panel gives up the slot and takes it back.3. A load failure survives a session switch
targetIdchanges in place when the selected session changes —TaskViewpasses it as a prop with no key — so a failure from session A parks session B's guest behind an error notice naming A's URL. The failure is now stamped with the guest it happened on and filtered on read, which costs no extra render and never briefly shows a failed page as fine.Verification
Driven in a running Studio: with the browser panel open and its guest showing, opening a studio dialog parks the guest (opacity 0.001, paint-host z-index) while the panel stays mounted, and closing it restores the guest over the slot at z-0.
pnpm check-and-test:cigreen, 22/22.Summary by CodeRabbit