fix(desktop): keep preview picking active across subframe navigation - #9741
fix(desktop): keep preview picking active across subframe navigation#9741Lucenx9 wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe element-picking session now handles every navigation start. Tests model Electron one-shot listeners and verify that subframe navigation does not end the pick session. ChangesElement picking navigation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Desktop preview element picking now remains active through subframe navigation and ends on main-frame navigation, with regression coverage for that sequence. No current merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c45a5dc. Configure here.
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a focused preview-picking bug fix that keeps the listener active through subframe navigation and still cancels on main-frame navigation, with regression coverage for both cases. The runtime change is isolated to the existing preview manager and does not alter defaults, schemas, security-sensitive code, or deployment behavior. You can add or adjust custom eligibility rules. Learn more. |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |

What Changed
isMainFramefrom the current Electron navigation event details instead of deprecated positional arguments.oncesemantics and cover a subframe navigation followed by a main-frame navigation.Why
Electron removes a
oncelistener after the first matching event, even when that event comes from a subframe. A subframe navigation could therefore consume the listener and leave the picker promise and listeners active when the main frame navigated afterward. Electron 43 exposes frame details on the event object, so the handler and regression test use that current shape.sequenceDiagram participant S as Subframe participant P as Picker session participant M as Main frame S->>P: did-start-navigation Note over P: Ignore event and keep listener M->>P: did-start-navigation P->>P: Settle and remove listenersVerification
pnpm exec vp test run src/preview/Manager.test.ts(80 tests)pnpm --filter @t3tools/desktop typecheckmainfor@t3tools/desktop(pass; zero introduced findings)Checklist
Made with GPT-5.6-sol using the Codex harness in T3 Code.
Note
Low Risk
Localized preview pick-session behavior with listener removal already handled in
pickElementcleanup; regression test added for the subframe-then-main-frame case.Overview
Fixes preview element picking ending too early when a subframe navigates before the main document does.
pickElementnow registers a persistentdid-start-navigationlistener (removed in the existing pick cleanup) instead ofonce, because Electron drops one-shot listeners on the first event—including non–main-frame navigations that should not cancel the pick. The handler readsevent.isMainFramefrom the navigation event object.Tests update the webview
oncemock to match real Electron behavior and assert the pick stays in flight after a subframe navigation and only settles when the main frame navigates.Reviewed by Cursor Bugbot for commit c482cbd. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix
PreviewManagerelement picking losing navigation listener on subframe navChanges the
did-start-navigationsubscription inPreviewManager.makeNativeOperationsfrom a one-time listener to a regular listener so it keeps receiving navigation events until the pick settles. The callback now consumes the typed navigation event and only settles the pick withnullwhenmain-frameis true; non-main-frame events leave the pick pending. Tests in Manager.test.ts were updated to model one-time listener semantics, pass structured navigation events, and assert the pick settles after a main-frame navigation.Macroscope summarized c482cbd.
Summary by CodeRabbit