Repository navigation
feat(browser): annotation v2 S2 — Session comment store, crops and restore - #452
Conversation
bubbleptr
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 7 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (group.comments.length >= maxSessionComments) { | ||
| return; |
There was a problem hiding this comment.
🔴 Comment limit leaves phantom page marks
At 200 Session comments, saveComment rejects another save without answering the overlay. The unsaved mark stays on the page, and its overlay remains hidden until the one-second fallback expires.
Learn more
The page creates its mark before it sends the save report; saveEditor also hides the overlay awaiting a crop. A full Session causes this early return before either captureCommentCrop or commitCommentChange runs. The store has no record, while the page continues showing the locally inserted mark, and the hidden host only recovers by timer.
Example: A Session contains 200 comments. Save a 201st on a live tab: its badge appears locally, but browser_list_comments still contains only 200 and no store sync removes the badge.
Recommended fix: On a rejected save, send a store sync for the tab and finish the pending crop handshake. Surface the limit to the user rather than silently accepting a local mark.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f6d8669. A save rejected at the 200-comment cap now releases the overlay (capture-done), resyncs the tab so the unsaved local mark disappears, and emits comment-rejected; the Browser panel shows a notice that the Session is full. Host and panel regression tests added.
| if (entry) { | ||
| entry.annotation = copy; | ||
| entry.badge.textContent = String(copy.index); | ||
| } else { | ||
| restoreAnnotation(copy); |
There was a problem hiding this comment.
🔴 SPA replacements permanently hide restored marks
When an SPA replaces an annotated element at the same URL, applySync keeps the disconnected entry. positionEntries hides it, and later syncs never resolve its selector again.
Learn more
A restored mark holds both its selector and the element that was resolved on the first sync. positionEntries hides marks whose element is disconnected. On subsequent syncs, applySync updates existing entries without checking whether their element remains connected, so the replacement node is never found.
Example: A React page renders <button id="cta">, receives a mark for #cta, then replaces that button during a rerender without changing the URL. The old badge disappears and cannot return on any later store sync despite the new #cta.
Recommended fix: Detect disconnected entries when applying a sync, remove their old badge and outline, and resolve/watch their selector again. Also consider DOM mutations for existing entries so the mark can recover even without a new sync.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f6d8669. applySync sends a disconnected entry back through the restore path (rebind by unique selector, else watch → stale after 5s). positionEntries also tries the same unique, unowned selector rebind before hiding, so the mark comes back without waiting for a sync. Tests cover rebind on sync, rebind on the next position pass, and an ambiguous selector that stays unbound.
| function restoreAnnotation(annotation: BrowserAnnotationElement) { | ||
| if (pendingRestores.has(annotation.id)) { | ||
| return; |
There was a problem hiding this comment.
🟡 Delayed restores show outdated comment data
When a pending restore receives updated text or numbering, restoreAnnotation retains its first snapshot. The eventual badge and editor show old values because the observer renders that snapshot rather than the latest sync.
Learn more
The first sync starts a MutationObserver for a selector that does not yet resolve. A later sync updates the authoritative annotations array, but the existing pending observer closes over the original object, and this guard prevents replacing it. When the element arrives, renderEntry renders the old index and stores the old comment for its editor.
Example: #late is absent during a sync for comment c1 numbered 2. Deleting the first Session comment changes c1 to index 1; #late appears afterwards, but its badge shows 2.
Recommended fix: Keep pending restores keyed to the latest annotation data, or look up the id in the current annotations array inside the observer before rendering.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f6d8669. When the element appears, the restore observer looks up the current annotation by id in annotations; if that id was dropped it renders nothing. The stale timer uses the same lookup. Regression test: renumbered and edited while pending, it renders the latest index and comment.
| const { url, title } = controller.pageInfo(); | ||
| const comment: StoredComment = { |
There was a problem hiding this comment.
🟡 Navigation can misfile a newly saved comment
If navigation starts after a new save but before reportSaved runs, saveComment reads the destination URL. The old page's comment is stored under the destination and its crop can photograph the wrong page.
Learn more
The overlay waits two animation frames before sending a newly saved annotation via reportSaved. The store stamps the comment with whatever URL the tab is showing when main handles that later IPC, rather than the document in which the annotation was created. A navigation during this interval can put the old annotation under the destination URL and use the destination page for its crop.
Example: Save a note on /home, then immediately navigate to /settings. If the navigation commits before main handles the deferred save, pageInfo() reports /settings, although the note's selector and rect describe /home.
Recommended fix: Include the saving document's URL or navigation identity with the annotation-saved report and reject saves after that document is replaced. Bind the crop to the same navigation identity.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f6d8669. annotation-saved now carries documentUrl (location.href of the document that saved, validated ≤2048). saveComment drops the save and still releases the overlay when its urlKey differs from the tab's current URL. The stored URL still comes from the tab controller.
| case "browser_consume_comments": { | ||
| // Sends to the composer take their comments out of the store exactly | ||
| // once — unknown ids are simply already gone. | ||
| const ids = new Set(readStringList(args?.ids)); |
There was a problem hiding this comment.
🟡 Repeated sends duplicate browser comments
After Send to composer succeeds, sendToComposer never calls browser_consume_comments. The Session store retains those comments, so another Send injects the same annotations again.
Learn more
The new store provides a consume command to remove sent comments. The existing sendToComposer still assembles a full-page prompt and injects it, but it never calls consumeBrowserComments. Thus a successful send does not remove comments from the store or the page, and another click sends them again. This behavior was already possible for page marks before this PR; the new Session-level store makes the repeated comments persist across reloads and tabs.
Example: Save one comment, click Send twice. The composer receives two copies of the same comment while browser_list_comments still lists it.
Recommended fix: Consume the exact ids included in the successful injection after delivery is confirmed, or explicitly defer the send-path migration alongside S3 if this remains an intentional interim limitation.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Addressed by the next layer, #454: the v1 To composer path is removed. The composer sends the Session's comments as structured attachments and consumes exactly the sent ids on success, so they can't be sent twice.
| case "annotation-deleted": | ||
| getBrowserHost().deleteComment(target, message.id); | ||
| break; |
There was a problem hiding this comment.
🟥 Child frames can alter top-level comments
If a child frame sends annotation-deleted, the listener accepts its parent WebContents identity. The child can delete comments belonging to the top-level page without owning that page's overlay.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not reachable today: the annotation preload runs only in the main frame (no nodeIntegrationInSubFrames), so a child frame has no ipcRenderer to send on. Hardened anyway in f6d8669: the handler now ignores any message whose event.senderFrame is not event.sender.mainFrame.
| const images: Record<string, string | null> = {}; | ||
|
|
||
| for (const id of ids) { | ||
| images[id] = group?.images.get(id) ?? null; |
There was a problem hiding this comment.
🟨 Comment image IDs collide with object prototype
If ids includes __proto__, the plain images object returns inherited data instead of a null entry. Callers expecting a map receive a malformed response for that id.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f6d8669. browser_comment_images builds its reply with Object.fromEntries, so every id (including __proto__) is an own data property. Test added.
Comments now outlive navigation and reloads: main owns them per Session, restores same-URL markers on new documents (flagging ones whose element is gone as stale), and saves a clean cropped screenshot with each new comment. Read, delete and consume commands prepare the composer slice. Closes #447
efd04ea to
f6d8669
Compare
Summary
Stacked on #451. PRD:
.scratch/browser-annotation-v2/PRD.md(decisions 4–5).browser-host.ts): annotations are owned per Session instead of per page overlay. Comments survive navigation, reloads and tab close; capped at 200 per Session.indexis the Session-wide 1..N order, recomputed on every read.capturePage(rect), downsampled to CSS pixels, then releases the page withcapture-done. Edits update text only.readyand main-framedid-navigate-in-page, main syncs the tab's comments whose URL matches (origin + pathname + search, hash ignored). The overlay re-resolves selectors (MutationObserver retry up to 5s) and reports stale comments.annotationsmessage replaced byannotation-saved/annotation-deleted/annotation-presence;clear-annotationsreplaced bysync-annotations. Page-side edits/deletes are scoped to the sender's own tab.browser_list_comments(side-effect-free read),browser_comment_images,browser_delete_comment,browser_consume_comments; newcomments-changedbrowser event.Clearnow removes the tab's comments.To composerkeeps working.Closes #447
Test plan
bun run typecheck,bun run lint,bun run buildbun run test— all pass except the pre-existingastryx-text-ref.test.tsxfailure (also fails onmain)bun run test:e2e e2e/smoke/browser-surface.spec.ts— 4/4, including save → reload → marker restored, navigate away → back → count restored