Repository navigation
feat(browser): annotation v2 S3 — browser comments as composer attachments, Cmd+Enter send - #454
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 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| const pending = browserComments.latest(); | ||
| const sentCommentIds = pending.map((comment) => comment.id); |
There was a problem hiding this comment.
🔴 Fresh comments missed by composer send
If Send runs while a new comment's crop is pending, browserComments.latest() omits that comment. The store publishes comments-changed only after capture, so the message goes out without the comment and leaves it pending.
Learn more
A newly saved browser comment enters the main-process store before its screenshot is captured. The store does not publish the comment to the composer until saveComment finishes the capture. The page shortcut waits for pending crops through requestSubmit, but clicking the composer's Send button does not. As a result, a send during a crop snapshots the previous comment list and succeeds without the newly saved comment.
Example: Save comment c2 while c1 is already visible in the drawer, then click Send before c2's screenshot resolves. The prompt contains only c1; c2 appears in the drawer after the message is sent.
Recommended fix: Give the composer's submit path a main-process barrier that awaits pending saves before reading the authoritative comment list and images. Ensure the snapshot and consume IDs come from the same settled store state.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 3b49109. New main command browser_settle_comments waits for in-flight save commits (crop → images → comments-changed), then returns the store's list. submitDraft sends that settled list (button and page Cmd+Enter alike) and consumes exactly those ids; if the settle fails, nothing is sent or consumed. Host and composer regression tests added.
| if (event.type === "submit-requested") { | ||
| // Cmd/Ctrl+Enter in the page. A composer mounted for this Session | ||
| // answers by submitting; with none, the comments stay on the page and | ||
| // the user hears that nothing was sent. | ||
| if (event.sessionId !== sessionId || !alive.current) return; | ||
| if (!requestComposerSubmit(sessionId)) { |
There was a problem hiding this comment.
🟡 Page send lost after dock switch
If the Browser panel unmounts while a crop runs, submit-requested has no listener when the crop completes. The composer remains mounted, but the page's send request disappears and its comments stay pending.
Learn more
The Browser panel subscribes to browser events only while it is mounted. SessionSurfaceContent renders only the active surface, so switching away from Browser unmounts this listener. requestSubmit waits for a pending crop before emitting its request. The request can therefore arrive after the user switches surfaces, even though the Session composer remains available.
Example: Save a comment using Cmd+Enter, then switch to Changes before its screenshot finishes. Main emits the request after capture, but no Browser panel forwards it; no message is sent.
Recommended fix: Subscribe to Session-scoped submit events from a component that remains mounted alongside the composer, rather than from the active Browser surface. Keep a suitable notice path when the composer cannot accept a request.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 3b49109. The composer now listens for the browser submit-requested event itself, so the send lands even if the Browser panel is unmounted. The panel only keeps the "no composer" notice, which it shows when a ref-counted presence registry reports no composer mounted for the Session.
| if (!alive || sawEvent || !fetched) { | ||
| return; | ||
| } | ||
| latestRef.current = fetched; | ||
| setComments(fetched); |
There was a problem hiding this comment.
🟡 Late comment list discarded despite newer contents
If browser_list_comments reads after a comments-changed event but replies later, sawEvent discards the newer list. The drawer and send snapshot retain outdated comments until another store change.
Learn more
The initial comment list is a separate IPC request from the event subscription. The sawEvent flag records only arrival order in the renderer, not when the main-process list was read. A list response that was computed after an event can arrive later, but this condition drops it and leaves the earlier event's comments in both UI state and latestRef.
Example: The subscription receives event E with [c1]; main then reads [c1,c2] for the list command, and that command's reply arrives after E. The drawer and submit still see only [c1].
Recommended fix: Attach monotonically increasing store revisions to events and list replies, and apply the result with the highest revision. Alternatively serialize the initial read and subscription through a single main-process snapshot/event handshake.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 3b49109. The store keeps a commentRevision bumped on every change; comments-changed, browser_list_comments and browser_settle_comments all carry it, and useBrowserComments applies only answers newer than the last one applied, replacing the arrival-order flag. Tests cover both orderings.
Saved comments now show as chips in the composer drawer and go out with the next message as a formatted block plus one crop per comment, then leave the store exactly once. Cmd/Ctrl+Enter in the page saves and sends; Enter keeps collecting. The v1 To composer button, its full-viewport capture path and the draft injection are removed. Closes #448
d75b8fd to
3b49109
Compare
Summary
Stacked on #452. PRD:
.scratch/browser-annotation-v2/PRD.mddecision 4 (S3 revisions noted inline).Tokenchips (#n first line, removable; stale ones yellow with an explanation). Removing a chip deletes the comment in main, so the page marker disappears too. An empty draft with comments can be sent.submitDraftsnapshots the pending comments, fetches their crops, and appendsformatBrowserCommentsafter the draft. Comment images go last, in comment order. On success only the snapshot ids are consumed: comments saved during the send stay, and a failed send consumes nothing. Covers send / queue / steer-from-queue.@pace/core): replacesformatBrowserAnnotationPrompt. Comments stay in Session order with aPage:header whenever the URL changes; multiline comments are kept (main's clamp no longer folds newlines incomment);[screenshot]/rect/stalemarkers. Template is pinned by full-string tests.submit-requestedafter the save report; main waits for in-flight crop commits and emits asubmit-requestedbrowser event that the composer handles itself, so it works even with the Browser panel unmounted. The panel only shows a notice when no composer is mounted (ref-counted presence registry). The composer ignores the request while busy or when there is nothing to send. Enter still saves and keeps collecting.browser_settle_comments, which waits for pending crops and returns the store's revision-stamped list, so a comment saved just before Send is included with its screenshot. The drawer only applies list replies and events newer than the last revision it applied.To composerbutton,browser_capture_annotation,prepare-capture/capture-ready,BrowserAnnotationCapture, andinjectIntoComposer. feat(browser): annotation v2 S1 — edit-first comment overlay #451's palette-only command, badge clipping and rebind behavior are kept./designdrawer entry plusdocs/design/{chat,workspace}.mdanddocs/self-built-ui.mdupdated.Closes #448
Test plan
bun run typecheck,bun run lint,bun run buildbun run test: all pass except the 2 pre-existingastryx-text-ref.test.tsxfailures (also onmain)bun run test:e2e e2e/smoke/browser-surface.spec.ts: 4/4. A real end-to-end send: save 2 comments → chips → Cmd/Ctrl+Enter in the page → transcript shows the comment block and bothbrowser-comment-N.pngcrops; chips and count go to 0