Repository navigation
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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 5 minutes. View limit details
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The changes satisfy the coding requirements in #23988.handleDidStartLoadingno longer tears down annotation state.handleFullDidNavigatetears down the session only for main-frame commits by clea…Out of Scope Changes check The changes remain within #23988. They update annotation-session cancellation and navigation/loading wiring for the localBrowserPagePane, plus focused test fixtures and automated tests. The changes…Title check The title clearly and concisely describes the main behavior change: preserving the annotation session during iframe loads. Description check The description is complete and follows the repository template. It explains the user impact, implementation, rationale, linked issue, testing, scope, and checklist status. Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements in
#23988.handleDidStartLoadingno longer tears down annotation state.handleFullDidNavigatetears down the session only for main-frame commits by clearing annotations, resetting the overlay viewport, and callingcancelGrabSessionRef. In-page and subframe navigation preserve the session.browser-page-annotation-session-teardown.test.tscovers main-frame cleanup, in-page navigation, subframe navigation and loading, anddid-start-loadingalone.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within
#23988. They update annotation-session cancellation and navigation/loading wiring for the localBrowserPagePane, plus focused test fixtures and automated tests. The changes do not modify remote or client-hosted browser panes, workspace previews, or unrelated browsing flows.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/browser-pane/host-guest/browser-page-favicon-retention.test.ts (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for navigation teardown.
The changed harness provides teardown callbacks, but it only invokes loading and navigation-start handlers. It never invokes
handleDidNavigateor asserts the teardown callbacks. Add cases for main-frame teardown and for subframe, in-page, and loading-start-only events that must preserve the session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 116acfe5-1d99-4890-a474-fbb04737176b
📒 Files selected for processing (8)
src/renderer/src/components/browser-pane/annotate/use-browser-page-grab-annotations.tssrc/renderer/src/components/browser-pane/assemble-chrome/browser-page-pane.tsxsrc/renderer/src/components/browser-pane/host-guest/attach-browser-page-webview.tssrc/renderer/src/components/browser-pane/host-guest/bind-browser-page-webview-listeners.tssrc/renderer/src/components/browser-pane/host-guest/browser-page-favicon-retention.test.tssrc/renderer/src/components/browser-pane/host-guest/browser-page-webview-loading-handlers.tssrc/renderer/src/components/browser-pane/host-guest/browser-page-webview-navigation-handlers.tssrc/renderer/src/components/browser-pane/host-guest/use-browser-page-webview-lifecycle.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/browser-pane/host-guest/browser-page-webview-loading-handlers.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The change itself is sound and I found no behavioral bug — but the PR's Testing section describes a new unit test that is not present in the branch. Reconcile that before merge (add the test, or correct the description).
Reviewed changes — the full local BrowserPagePane annotate-teardown fix against main, verified end-to-end (diff, wiring, and the Electron event contract):
- Teardown moved from
did-start-loadingtodid-navigate— the annotate clears now run on the main-frame commit signal instead of every load, so subframe/iframe polling no longer destroys the session (browser-page-webview-navigation-handlers.ts:149). - Full session reset via
cancelGrabSessionRef— new ref owned byuse-browser-page-grab-annotations.ts:104clears the pending card and callsgrab.cancel()when not idle, so no half-torn-down state survives (use-browser-page-grab-annotations.ts:115). - Loading handler de-cluttered —
handleDidStartLoadingnow only reportsloading: true(browser-page-webview-loading-handlers.ts:65); the removed props are unwound through the attach/lifecycle/bind chain. - Test harness args updated —
browser-page-favicon-retention.test.ts:43follows the moved props (passes locally, 6/6).
I verified the load-bearing premise against Electron 43.7.5 (the pinned version) rather than assuming it: did-start-loading does fire for iframe loads (electron#30479), and did-navigate fires only on a main-frame document commit — never for subframes (did-frame-navigate) or same-document changes (did-navigate-in-page). The fix does what it claims.
⚠️ Testing section references a test file that is not in the PR
The Testing section states a new browser-page-annotation-session-teardown.test.ts was added and mutation-checked ("re-adding the old clear inside handleDidStartLoading turns two of the tests red"). That file is not in the branch — it is absent from git ls-files and the working tree is clean. The only test touched is browser-page-favicon-retention.test.ts, whose harness never dispatches handleFullDidNavigate, so the central subframe-vs-main-frame teardown behavior currently has no automated coverage.
Technical details
# Missing test file and coverage gap
## Affected sites
- PR description "## Testing" → "Unit: new `browser-page-annotation-session-teardown.test.ts`" — file does not exist in the branch.
- `src/renderer/src/components/browser-pane/host-guest/browser-page-favicon-retention.test.ts:64-72` — `navigateTo` calls `handleDidStartLoading` + `handleDidStartNavigation` only; `handleFullDidNavigate` (the new teardown path) is never exercised.
## Required outcome
- Either commit the described test (asserting: main-frame `did-navigate` clears annotations, resets the viewport, and fires `cancelGrabSessionRef`; `did-navigate-in-page`, subframe, and `did-start-loading`-only paths clear nothing), or update the Testing section to reflect the actual coverage.
## Open questions for the human
- Was `browser-page-annotation-session-teardown.test.ts` intended to be part of this PR and dropped from the commit?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the incremental delta since the prior pullfrog review (1de6ccf → 90b3f6ff0a, test-only):
- Added the missing teardown regression test —
browser-page-annotation-session-teardown.test.tsnow drives a main-framedid-navigatecommit and asserts the annotation clears, the viewport resets, and the grab session resets; it also proves the in-page, subframe, anddid-start-loading-only paths clear nothing. This closes the prior review's only finding. - Favicon harness re-pointed at the moved props —
browser-page-favicon-retention.test.tspassesclearBrowserPageAnnotationsRef/cancelGrabSessionRefto the navigation factory instead of the loading factory.
I mutation-checked the new test: deleting the cancelGrabSessionRef.current() call from handleFullDidNavigate turns the first test red (expected 1 calls, got 0), so the coverage is real rather than theatre. New file runs 4/4 under the project config.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
90b3f6f to
a85a067
Compare
nwparker
changed the title
did-start-loading fires for subframe loads too, so a page that reloads an iframe on each poll invalidated the annotation document every cycle: the pending card closed, grab mode was cancelled and saved-note markers were retired. Main-frame did-start-navigation already invalidates for every document replacement (including same-URL reloads) and same-document navigation, so drop the did-start-loading handler and move the overlay viewport reset next to that invalidation. Fixes stablyai#23988 Co-authored-by: Brant <562916493@qq.com>
a85a067 to
3182fed
Compare
ELI5
When you annotate a page in Orca's browser and that page quietly reloads an iframe in the background (ads, chat widgets, dashboards that poll), Orca thought the whole page had reloaded. It closed your half-written note, stopped grab mode, and hid your saved note markers. Now only a real page navigation or reload resets the annotate session.
What Changed
did-start-loadinghandler invalidated the annotation document and reset the overlay viewport. Electron firesdid-start-loadingfor subframe loads too, so a page that reloads an iframe on each poll tore down the annotate session every cycle: the pending card closed, grab mode was cancelled, and saved-note markers were retired.did-start-loadinglistener is gone. The main-framedid-start-navigationhandler already invalidates the annotation document for every document replacement (including same-URL reloads) and for same-document navigation. The overlay viewport reset now runs next to that invalidation. Iframe-only loads no longer touch the annotate session.handleDidStartLoadingnow drivehandleDidStartNavigationinstead. The same-URL reload test still checks that saved notes are kept and capture is cancelled.Why
did-start-loadingcannot tell main-frame loads apart from subframe loads.did-start-navigationcarriesisMainFrame, and an event-order probe on Electron 43.8.1 shows it fires on the main frame for every case the old handler was there to cover:location.reload(),webview.reload(),loadURL, hash and pushState changes, and 204/download navigations. Iframe reloads, src swaps, and appended iframes emit onlymain=false. The original PR keptdid-start-loadingand added subframe filtering on top. This version drops the redundant signal and reuses the one that is already correct.Linked Issue
Fixes #23988
Visual Proof
N/A for screenshots: an iframe reloading looks the same on screen before and after this change. What differs is whether the annotate session gets torn down, so the proof is an event trace plus a regression probe.
Electron 43.8.1 webview event order (excerpt):
Listener probe ("iframe reload/src-swap does not invalidate the annotate session"):
Testing
Reproduced on macOS with a listener probe: on
main, iframe reloads, src swaps, and appended iframes each invalidate the annotate session. On this branch they do not, and main-frame reload and navigation still invalidate it.pnpm test src/renderer/src/components/browser-pane/host-guest/: 24 files, 190 tests pass.oxlint, oxfmt, the anti-slop, code-quality, and ast-grep gates, and the changed-code quality gate are all clean on the touched files.
Platforms: renderer-only webview event wiring with no OS-specific paths. It behaves the same on macOS, Linux, and Windows, and for SSH/remote and folder workspaces.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Original fix by @Brant-lzh in #24032.
Agent skill upstream boundary
Notes
Renderer-only change to webview listener wiring. No IPC, protocol, or persisted-state changes, so mixed client/server versions are unaffected. It also does less work: one fewer listener, and no re-render on every subframe load.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)