Skip to content

fix(annotation): keep preview marks on the artifact region the user selected at any UI zoom - #6476

Open
linghaoSu wants to merge 20 commits into
nexu-io:mainfrom
linghaoSu:fix/6361-annotation-zoom-coords
Open

fix(annotation): keep preview marks on the artifact region the user selected at any UI zoom#6476
linghaoSu wants to merge 20 commits into
nexu-io:mainfrom
linghaoSu:fix/6361-annotation-zoom-coords

Conversation

@linghaoSu

@linghaoSu linghaoSu commented Aug 5, 2026

Copy link
Copy Markdown

Fixes #6361

Why

Reported in #6361: with the desktop app at a non-100% UI zoom, a Mark/Draw annotation's screenshot, painted mark, and the structured position sent to the coding agent no longer identify the same preview region β€” the reporter's capture showed a 1459px-wide box spanning the app sidebar, and the downstream agent edited the wrong element. I reproduced the report end-to-end and found three independent coordinate-space seams, all fixed here:

  1. capturePage clipped in the wrong space (desktop). The renderer measures the preview frame with getBoundingClientRect() (CSS px) but Electron's capturePage() clips in DIP; the two coincide only at zoom factor 1. At any other zoom the capture starts offset from the frame and includes app chrome. parseCaptureClip now converts CSS px β†’ DIP with the live zoom factor.
  2. Marks were pinned to a fraction of the frame, and the artifact moves. Changing UI zoom narrows the frame, the artifact reflows (header wraps), and every element below shifts β€” a frame-fraction mark slides onto the neighbouring element. Marks now anchor to the element they were drawn on (reusing the data-od-id identity the preview bridge already publishes) and are re-projected from that element's current box before pixels and structured bounds are read.
  3. Scaled device frames shrank the structured bounds (found by this PR's verification matrix). In a fit-to-window scaled tablet/mobile preview, bounds were read from the canvas's getBoundingClientRect() β€” the transform-scaled on-screen size β€” while the composited PNG stayed artifact-local. Measured at 0.42Γ— fit scale: a mark on a 784px-wide band went out as a 330px rect. Bounds now derive from the canvas layout box (offsetWidth/Height), the same source the canvas is sized from.

What users will see

Drawing a mark on an HTML preview and sending it now targets the same artifact region the user selected β€” at any UI zoom, DPR, sidebar width, or preview viewport (desktop/tablet/mobile, including fit-to-window scaling). Marks visually track their content when the preview reflows, resizes, or scrolls while draw mode is open. No new UI.

Surface area

  • UI
  • Keyboard shortcut
  • CLI / env var
  • API / contract
  • Extension point
  • i18n keys
  • New top-level dependency
  • Default behavior change β€” annotation coordinates sent to agents are now artifact-local under zoom/scaling (previously drifted); draw mode on URL-loaded previews without the anchor bridge falls back to srcDoc
  • None

Screenshots

Before/after comparison from two live namespaced runtimes (main vs this branch), same scripted gesture β€” mark Band 5, narrow the window so the content reflows, send:

Reflow scenario (web runtime, main vs this branch, same scripted gesture):

before/after: reflow annotation drift

Electron capturePage clip of Band 7 (magenta) at 125% UI zoom:

before/after: electron zoom 125%

Same at 80% UI zoom:

before/after: electron zoom 80%

Raw gesture screenshots (before-fix runtime): mark Band 5 wide, then reflow

marked wide
after reflow β€” mark slid off Band 5

Zoom before

zoom-before.mp4

Zoom after

zoom-after.mp4
  • Reflow scenario (web runtime): on main the structured bounds land 32px off β€” one full band β€” onto Band 4/5; on this branch they track Band 5 exactly (drift 0px).
  • Electron 125% / 80% zoom: clipping Band 7 (magenta) on the pre-fix path captures Bands 5–6 (0% target pixels); the fixed path returns pure Band 7 (99.7–100%).

Bug fix verification

  • Red specs, each red on main / green here:
    • apps/desktop/tests/main/capture-clip-zoom.test.ts β€” CSSβ†’DIP clip conversion, pinned to the zoom/bitmap numbers measured in the report
    • apps/web/tests/components/PreviewDrawOverlay.content-anchor.test.tsx, PreviewDrawOverlay.frame-resize.test.tsx, preview-mark-anchor.test.ts, preview-mark-geometry.test.ts β€” content anchoring and geometric re-projection
    • apps/web/tests/components/PreviewDrawOverlay.scaled-frame-bounds.test.tsx β€” layout-space bounds in a 0.42Γ—-scaled device frame
  • Runtime verification against live tools-dev runtimes (production APIs only, real pointer gestures, annotation payload intercepted at the opendesign:annotation boundary), covering the issue's requested matrix:
    • Electron zoom 80% / 100% / 125% / 150% (real BrowserWindow + production parseCaptureClip + real capturePage, pixel-checked bitmaps; pre-fix control path reproduces the bug at every non-100% zoom)
    • DPR 1 and 2 (Chromium deviceScaleFactor)
    • Sidebar narrow/wide (conversation pane collapsed/expanded, marks drawn before and after)
    • Fit-to-window scaled preview (tablet frame at 0.42Γ—)
    • Box and pen marks near all four preview edges
    • Three-way assertion per the issue: decoded screenshot's painted mark bbox, structured bounds, and pointer-targeted element agree within ≀2px (pen bounds carry their designed 8px halo)

Validation

  • pnpm guard, pnpm typecheck
  • pnpm --filter @open-design/web test β€” 6235 passed
  • pnpm --filter @open-design/desktop test β€” 319 passed
  • pnpm --filter @open-design/web typecheck, pnpm --filter @open-design/desktop build

Marking a region of an HTML preview could send the agent a different
region than the user selected. Two independent causes, both measured on
the reported artifact (10 bands, 40px tall, 50px pitch).

1. capturePage clipped in the wrong coordinate space.

The renderer measures the preview frame with getBoundingClientRect() --
CSS pixels -- but Electron's capturePage() clips in DIP. Those spaces
coincide only at zoom factor 1. The proof is the returned bitmap: at
zoom 1.095 a 692x666 CSS frame came back as 1384x1332, exactly 2.0x the
CSS rect, while devicePixelRatio was 2.1909. The capture therefore
started 14 DIP above the frame and the mark landed a full band high --
red box at rows 683-765 while the marked band occupied 777-864.
parseCaptureClip now converts CSS px to DIP with the live zoom factor.

2. Marks were pinned to a fraction of the frame, and the artifact moves.

Changing UI zoom narrows the preview frame, which reflows the artifact:
the header paragraph wraps to a second line and every band below shifts
down 19.5px. A mark held at a fraction of the frame slides across the
content, so "zoom in, mark, restore, send" sent the neighbouring band.

Marks now bind to the element they were drawn on -- reusing the
data-od-id identity the preview bridge already publishes for comments --
and are re-projected from that element's current box before the pixels
and the structured bounds are read. A new od:mark-anchor-request /
od:mark-anchor-targets pair serves element boxes without enabling
comment mode, whose hover/click interception would fight the canvas.
Boxes, pen strokes and text labels all follow.

Anchoring is best-effort: a preview that reports no elements leaves
marks on their frame-relative position rather than blocking the send,
and a bridge that never answers is remembered so later sends do not wait
out the timeout. A pure frame resize (no reflow) is still corrected
geometrically, which also covers device-frame previews whose layout box
never changes.

Because anchors must come from the frame the user sees and that the
compositor screenshots, draw mode now requires the URL-loaded preview to
serve the anchor bridge as well as the snapshot bridge; the raw preview
route injects an older selection bridge that predates this protocol, so
draw mode falls back to srcDoc. Resolving anchors against the hidden
srcDoc twin agreed with the visible frame only until it was scrolled.

Scroll re-projects on the leading edge so a mark tracks its content
during the gesture; resizes and fresh marks stay on a trailing debounce.

Fixes nexu-io#6361
…ontract

Draw mode now requires the URL-loaded preview to serve the anchor bridge
as well as the snapshot bridge, so a frame carrying only
`odPreviewBridge=snapshot` falls back to srcDoc. This test asserted the
previous contract and was the one failure this branch added over main
(584 -> 585); it now asserts the swap, with the reason and the condition
that would make url-load eligible again.
…ed client rect

Full-matrix runtime verification of nexu-io#6361 surfaced a third coordinate
seam: in a fit-to-window scaled device frame (tablet/mobile preview in a
narrow window) the structured annotation bounds were read from
canvas.getBoundingClientRect(), which reports the on-screen size after
the ancestor transform: scale(). The composited screenshot paints marks
across the snapshot's artifact-local dimensions, so the PNG showed the
right region while the structured position sent to the agent was shrunk
by the fit scale β€” measured 0.42x: a mark on a 784px-wide band went out
as a 330px rect at the wrong offset.

Bounds now derive from the canvas layout box (offsetWidth/Height, the
same source the canvas is sized from and the anchor pass measures
against), with the client rect kept only as a fallback for environments
without layout metrics. Text-label bounds divide their client-space
offsets by the live scale for the same reason.

Verified against a live tools-dev runtime driving the real UI: box marks,
pen strokes at all four preview edges, sidebar collapsed/expanded, and a
0.42x-scaled tablet frame β€” the decoded screenshot's painted mark, the
structured bounds, and the pointer-targeted band now agree within 2px in
every case, at DPR 1 and 2.

Refs nexu-io#6361
@lefarcen

lefarcen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Thanks @linghaoSu. Opened as a draft, so I'll hold off on a full review until you mark it ready for review. I've started the triage/classification steps in the meantime.

@lefarcen
lefarcen requested a review from nettee August 5, 2026 15:39
@lefarcen lefarcen added size/XL PR changes 700-1500 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Aug 5, 2026
@linghaoSu
linghaoSu marked this pull request as ready for review August 5, 2026 15:42
@linghaoSu
linghaoSu requested a review from a team as a code owner August 5, 2026 15:42

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu I found three blocking correctness regressions in the changed annotation path; the focused geometry tests and app typechecks pass, but these cases can still break powered previews or send a mark to the wrong region. The inline threads below describe the evidence and concrete fixes.

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/components/file-viewer-render-mode.ts
Comment thread apps/web/src/components/PreviewDrawOverlay.tsx Outdated
Comment thread apps/web/src/components/PreviewDrawOverlay.tsx
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Visual regression review

Head: 65521e4 Β· Base: 57de743

0 changed Β· 51 unchanged Β· 0 new without baseline Β· 0 failed

Unchanged cases
Case Main PR Diff
visual-avatar-local-agent-list
0 px (0.00%)
main pr diff
visual-avatar-local-agent-list-panel
0 px (0.00%)
main pr diff
visual-avatar-menu
0 px (0.00%)
main pr diff
visual-avatar-menu-panel
0 px (0.00%)
main pr diff
visual-avatar-open-design-model-picker
0 px (0.00%)
main pr diff
visual-critical-settings
0 px (0.00%)
main pr diff
visual-critical-workspace
0 px (0.00%)
main pr diff
visual-critical-workspace-preview
0 px (0.00%)
main pr diff
visual-design-system-detail
0 px (0.00%)
main pr diff
visual-design-systems
0 px (0.00%)
main pr diff
visual-go-campaign-600
0 px (0.00%)
main pr diff
visual-go-campaign-short-height
0 px (0.00%)
main pr diff
visual-home
0 px (0.00%)
main pr diff
visual-home-catalog
0 px (0.00%)
main pr diff
visual-home-context-picker
0 px (0.00%)
main pr diff
visual-home-context-picker-popover
0 px (0.00%)
main pr diff
visual-home-plugin-filter
0 px (0.00%)
main pr diff
visual-home-plugin-use-staged
0 px (0.00%)
main pr diff
visual-home-plugin-use-with-query
0 px (0.00%)
main pr diff
visual-home-staged-attachment
0 px (0.00%)
main pr diff

Visual diff is advisory only and does not block merging.

@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Aug 5, 2026
@lefarcen

lefarcen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Hey @linghaoSu, @nettee's current-head review is the right blocking thread to work through first β€” the three inline comments there are the actionable merge blockers on this revision.

I've also marked this for manual QA because it changes user-visible annotation behavior. Once those fixes are in and the PR is merge-ready, we'll pull QA in for the validation pass before merge.

πŸ’‘ To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …):
Take over nexu-io/open-design#6476 until it merges β€” read https://raw.githubusercontent.com/nexu-io/looper/main/skills/pr-takeover/SKILL.md and follow it.

… undone-stroke sync

Three review findings on nexu-io#6476, each with a red spec:

1. Draw mode forced every preview through srcDoc because nothing ever set
   urlLoadDecision.urlAnchorBridge, which broke powered previews
   (SharedArrayBuffer/WebGL artifacts that need the cross-origin-isolated
   URL frame). The daemon's URL selection bridge now answers
   od:mark-anchor-request with the same reply shape as the srcDoc bridge
   and advertises markAnchors:true in its ready message; FileViewer keys
   urlAnchorBridge off that flag and resets it wherever the selection
   bridge readiness resets.

2. One empty anchor-targets response permanently latched
   anchorBridgeRef=false, disabling anchoring for the session β€” including
   for dynamic apps that annotate elements after first paint and for
   later files shown through the same overlay. The probe now
   distinguishes an answered-empty reply (live bridge, keep asking) from
   an unanswered one, gives up only after two consecutive silent probes,
   and forgets the verdict when the probed iframe changes.

3. syncContentAnchors re-projected only live strokes, so
   undo β†’ reflow β†’ redo restored a stroke at its stale frame position on
   the neighbouring element. Undone strokes now ride the same
   syncStrokeAnchor pass as visible ones.

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee All three blocking threads are addressed in fbf371b (each with a red spec that fails on the previous head), threads replied + resolved. Ready for re-review.

  • URL anchor bridge: daemon URL selection bridge now serves od:mark-anchor-request + advertises markAnchors: true; FileViewer wires urlLoadDecision.urlAnchorBridge off it, so needsPowered artifacts keep the isolated URL frame in Draw mode.
  • Retryable probe: answered-empty vs unanswered are now distinct; give-up only after 2 consecutive silent probes, reset per iframe identity.
  • Undone strokes: undoneStrokesRef rides the same syncStrokeAnchor pass; undo β†’ reflow β†’ redo restores the original region.

Validation: guard + repo typecheck green; web suite 6237 passed; daemon suite failures are identical with and without this diff (pre-existing on baseline, verified by stash); live-runtime annotation matrix (box/pen/4 edges/sidebar/fit-to-window/DPR 1&2) re-run green on the new head.

@lefarcen
lefarcen requested a review from nettee August 5, 2026 17:01

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu I found two remaining correctness blockers in the current annotation path. The focused and full suites, app typechecks, and guard all pass, but these cases can still bind a mark to the wrong artifact region or permanently fall back to frame-relative coordinates. Please address the inline findings before merge.

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/components/PreviewDrawOverlay.tsx Outdated
Comment thread apps/web/src/components/PreviewDrawOverlay.tsx Outdated
…p a cooldown

Second review round on nexu-io#6476, two findings:

1. Anchor probes went through snapshotHostIframe(), which prefers the
   hidden srcDoc twin. With urlAnchorBridge keeping the URL/powered
   iframe active, the user draws on and the compositor captures one
   document while anchors were read from another β€” the twins scroll
   independently (URL scroll restoration targets the active frame only)
   and a powered artifact may not execute in the opaque sandbox at all.
   Anchor requests now go to the data-od-active frame via a dedicated
   anchorHostIframe(); snapshotHostIframe() remains for snapshot capture,
   whose bridge genuinely lives in the srcDoc transport.

2. The silent-probe counter was a permanent verdict: two 1.5s timeouts
   during a slow document load disabled anchoring for the rest of the
   overlay's mount, and the counter survived deactivation and file
   switches on a reused iframe element. The give-up is now a 5s cooldown
   with single-probe retries, and the budget resets on overlay
   deactivation and on filePath change.

Tests (each red on the previous head): active-frame probe with a
diverging srcDoc twin mounted, delayed bridge-ready recovery under fake
timers, and file-switch budget reset on a reused iframe.

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee Round-2 findings addressed in 8345bab, threads replied + resolved. Anchor probes now target the active (data-od-active) frame β€” the one the user draws on and the compositor captures β€” and the silent-probe give-up became a 5s cooldown with resets on deactivation and file switch. Three new regression tests, each red on the previous head. Validation: guard + web typecheck green, component suite 3787 passed, live-runtime annotation matrix re-run green on this head. Ready for re-review.

@lefarcen
lefarcen requested a review from nettee August 5, 2026 17:48

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu I rechecked all changed ranges, the prior resolved review threads, focused web/desktop/daemon tests, the three app typechecks, guard, diff validation, and the current checks. The coordinate-space fixes and bridge gating are otherwise well covered, but one stale active-frame probe can still send marks to the wrong artifact region during a normal iframe or zoom transition. I also found a smaller bridge-error path that suppresses retry semantics; both details are inline.

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/components/PreviewDrawOverlay.tsx
Comment thread apps/daemon/src/routes/project/index.ts Outdated
…swered

Third review round on nexu-io#6476:

1. syncContentAnchors captured the iframe and frame size before an await
   that can span the full bridge timeout. If the active iframe swapped
   (srcDoc -> URL when bridge-ready advertises markAnchors) or the frame
   resized mid-flight, the reply β€” measured against the old document and
   geometry β€” was still committed to the shared mark refs, moving marks
   to the wrong region right before capture. The continuation now
   re-checks both identities after the await and discards a stale reply;
   the pass accompanying the change re-probes against the current frame.

2. Both bridges' od:mark-anchor-request handlers swallowed enumeration
   exceptions into a normal empty reply, which the host counts as an
   answered bridge β€” clearing the retry budget and pinning the mark
   frame-relative with no retry path. A failed enumeration now stays
   silent, so the host's timeout classifies it as unanswered and the
   cooldown/retry semantics engage. Applied to the daemon URL bridge and
   mirrored in the srcDoc bridge.

Test (red on the previous head): 'discards an anchor reply that
resolves after the active iframe swapped' β€” anchor acquired on frame A,
a hanging probe resolves with A's scrolled boxes after the active frame
swaps to B, and the send must keep the mark where the user drew it.

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee Round-3 findings addressed in c5765ba, threads replied + resolved. Stale anchor replies (iframe swap or resize during the probe await) are now discarded via post-await identity re-checks, and bridge enumeration failures stay unanswered so the host's cooldown/retry engages instead of reading them as a healthy empty document. New regression test red on the previous head. Validation: guard + typecheck green, web component suite 3788 passed, daemon bridge tests 47 passed, live-runtime annotation matrix green on this head. Ready for re-review.

@lefarcen
lefarcen requested a review from nettee August 5, 2026 18:16

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu I reviewed every changed file and reran the focused web (347), desktop (7), and daemon (47) suites, all three app typechecks, the repository guard, and the root typecheck. The zoom, layout-space, and content-anchor coverage is strong, but two blocking correctness paths remain; the concrete cases and fixes are in the inline comments.

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/routes/project/index.ts Outdated
Comment thread apps/web/src/components/PreviewDrawOverlay.tsx
…y generation

Fourth review round on nexu-io#6476:

1. HTML above HTML_PREVIEW_BRIDGE_MAX_BYTES streams from disk with no
   transform, so it carried no preview bridges β€” a >2MiB powered
   artifact never advertised markAnchors, and Draw's new urlAnchorBridge
   gate kicked it into the opaque srcDoc sandbox that cannot run
   Worker/SAB/WASM content. Large HTML responses now append the
   requested bridge scripts after the streamed bytes (each bridge is a
   guarded IIFE, so a trailing script is equivalent to the buffered
   no-</body> injection path); Range replies keep exact file bytes, and
   Content-Length covers file + suffix. Applied to both the raw and
   powered routes.

2. The post-await staleness guards compared only the iframe node and
   frame size, which cannot see a same-node document reload or a
   deactivate/reopen cycle, and did not order overlapping probes. A
   monotonic probe generation now supersedes: each probe takes a token,
   deactivation and filePath changes invalidate it, and only the latest
   probe may commit its reply.

Tests (red on the previous head): streamed-large-HTML bridge suffix on
both routes (daemon), and a reopen-with-pending-probe regression where
the stale pre-reopen reply must not re-project the fresh mark (web).

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee Round-4 findings addressed in f284004, threads replied + resolved. Large streamed HTML (>2MiB, both raw and powered routes) now carries the preview bridges as a streamed suffix β€” no buffering added, Range semantics preserved β€” so powered artifacts keep markAnchors and Draw stays on the URL frame. Probe staleness is now governed by a monotonic generation invalidated on deactivation/file-switch and superseded by newer probes. Both regression tests red on the previous head. Validation: guard + typecheck green, web component suite 3789 passed, daemon file-route suites 54 passed, live-runtime annotation matrix green on this head. Ready for re-review.

@lefarcen
lefarcen requested a review from nettee August 5, 2026 19:28
@linghaoSu

Copy link
Copy Markdown
Author

Maintainer adjudication requested.

Status summary for a human decision:

  • The original Visual annotation coordinates drift when desktop UI zoom is not 100%Β #6361 defects (capturePage DIP clip at non-100% zoom, reflow drift, scaled-frame bounds) were fixed in the first commits and verified against the issue's full requested matrix: Electron zoom 80/100/125/150, DPR 1/2, sidebar states, fit-to-window, box+pen at all four edges, with a three-way pixel/bounds/pointer assertion and before/after captures in the PR description.
  • Since then, the Looper reviewer (@nettee) and I have gone through 9 fix rounds / 19 findings, all addressed with red specs. The last four rounds have all been progressively narrower concurrency windows in the capture-freeze code added during this review itself (mid-flight probe replies β†’ write freeze β†’ mid-capture resize β†’ now deactivate-during-capture). Each finding has been real, but the loop is not converging toward approval, and per the repo's own review guidance this is the point where a human should size the remaining risk.
  • The two currently open threads describe the same case: Escape during an in-flight capture clears mark refs before send() finishes reading them. The fix is likely small (defer the deactivation cleanup while sending), but I'm pausing before another iteration of hardening-the-hardening.
  • CI: UI P0 (project-workspace) failed twice on this branch, but all three failing tests pass locally on this head and none touch the annotation paths; the failure signature (45s locator.fill timeouts) reads as runner slowness. I lack permission to re-run jobs β€” could a maintainer retrigger or confirm known flakiness?

Question for maintainers: should the remaining deactivate-during-capture threads (and any further capture-lifecycle refinements) block this PR, or land as a follow-up issue so the zoom-drift fix (the user-facing bug) can ship? I'm happy to either fix the open threads in one more round or file the follow-up β€” your call.

@lefarcen

lefarcen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@linghaoSu Thanks for laying this out clearly. I re-read the current head, and I don't think the remaining deactivate-during-capture race should move to a follow-up: it still sits on the core #6361 invariant, because the active === false cleanup can clear strokesRef / selectionBoxesRef / textMarksRef and release the frozen layout while send() is still between requestSnapshot() and its final bounds read in apps/web/src/components/PreviewDrawOverlay.tsx.

Given that, we should keep this PR blocked on closing that lifecycle edge case rather than land a path that can still emit a missing mark or mismatched screenshot/bounds under a normal Escape or file-switch during send. I also re-ran the failed workflow jobs so you'll get a fresh CI signal on this head.

Maintainer-adjudicated round on nexu-io#6476 (lefarcen: the
deactivate-during-capture race stays merge-blocking): the
active === false cleanup cleared strokesRef/selectionBoxesRef/
textMarksRef and released the frozen layout while send() could still be
between requestSnapshot and its bounds read β€” an Escape or file switch
during a slow capture emitted an annotation with an unmarked PNG and
undefined bounds. The cleanup now defers while  and runs from
send()'s finally once the payload is fully read; every other
deactivation path is unchanged.

Test (red on the previous head): deactivating mid-capture must still
emit the annotation with intact structured bounds and the mark painted
into the PNG (asserted via recorded compositor calls).

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@lefarcen @nettee Per the adjudication, the deactivate-during-capture race is fixed in a6260db (both threads replied + resolved): the inactive cleanup defers while sending and runs from send()'s finally after the payload is fully read. Regression test red on the previous head, asserting both intact bounds and the painted mark. Validation: guard + typecheck green, web component suite 3796 passed, live-runtime annotation matrix green on this head. Thanks for re-running the flaky UI P0 jobs β€” awaiting the fresh CI signal and re-review.

@lefarcen
lefarcen requested a review from nettee August 6, 2026 00:17

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu I found one remaining blocking race in the active-frame anchor-probe lifecycle. A stale probe is correctly discarded when Draw switches from srcDoc to the URL frame, but the send path can then finish without probing the new frame, so the screenshot and structured bounds can describe different artifact documents. The inline comment has the repro and a concrete fix; the focused suites, package typechecks, and guard pass, so this is the remaining correctness gap before merge. The coordinate-space and lifecycle coverage is otherwise thoroughβ€”thank you for the careful follow-up work.

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/components/PreviewDrawOverlay.tsx Outdated
@lefarcen

lefarcen commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Hey @linghaoSu, @nettee's latest current-head review is still the active blocker here β€” the remaining gap on \ is the stale-frame discard path in : after Draw switches from the srcDoc iframe to the URL iframe, the old reply is dropped but the pre-capture path can still finish without probing the new active frame. Once that re-probe is forced before capture, the existing \ path can continue.

@lefarcen

lefarcen commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Hey @linghaoSu, @nettee's latest current-head review is still the active blocker here β€” the remaining gap on a6260db9d is the stale-frame discard path in syncContentAnchorsInner(): after Draw switches from the srcDoc iframe to the URL iframe, the old reply is dropped but the pre-capture path can still finish without probing the new active frame. Once that re-probe is forced before capture, the existing needs-validation path can continue.

…scarded

Eleventh review round on nexu-io#6476: discarding a stale reply (active iframe
swapped or frame resized during the probe await) settled the shared
sync chain without re-probing, so a pre-capture sync that joined that
chain could proceed to capture with marks never resolved against the
document being screenshotted β€” e.g. Draw opens on srcDoc, the URL
anchor bridge becomes ready mid-send, data-od-active flips with
unchanged wrapper size, and no resize/scroll event follows to trigger
another pass. The iframe-identity and frame-size discards now set the
trailing flag before returning; the trailing pass runs inside the same
chain (syncContentAnchors), so awaiting callers observe the re-probe
against the current frame before the freeze engages.

Test (red on the previous head): Send's own probe held while
data-od-active swaps srcDoc -> URL with the wrapper size unchanged; the
awaited chain must probe the URL frame before capture and the sent
bounds must resolve the mark's anchor against that frame's boxes.

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee Round-11 finding addressed in 12b53fc, thread replied + resolved. Stale-reply discards now queue an in-chain trailing pass, so awaited pre-capture syncs always cover a probe of the frame actually being captured. Regression test reproduces the exact bridge-ready mid-send sequence, red on the previous head. Validation: guard + typecheck green, web component suite 3797 passed, live-runtime annotation matrix green on this head. Ready for re-review.

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu I completed a full changed-range review of the current head. The focused web anchor/geometry suites (97 tests), daemon route suite (51), desktop clip suite (7), all three app typechecks, and pnpm guard pass. I found two merge-safe follow-ups in the new bridge path below; the core coordinate and capture-lifecycle fixes are otherwise well covered. Thanks for the careful regression work and the thorough verification matrix.

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/runtime/exports.ts Outdated
Comment thread apps/daemon/src/routes/project/index.ts Outdated
…numerator

Twelfth review round on nexu-io#6476 (both non-blocking, addressed to close
out the review):

1. requestPreviewAnchorTargets cast any array straight to
   PreviewAnchorTarget[]. A malformed or forged entry (null, empty
   selector, NaN box) would throw inside the anchor chooser and abort
   the send β€” anchoring is best-effort and must never block the
   annotation. Replies are now sanitized field-by-field; invalid
   entries drop, a non-array reply counts as answered-empty.

2. Both bridges served od:mark-anchor-request via allTargets(), whose
   targetFrom() builds the full comment payload (text, htmlHint,
   computed style) per node β€” wasted work on the 32ms scroll pacing
   that made large artifacts jank. Each bridge now uses a dedicated
   enumerator that queries only annotated nodes, emits only the three
   anchor fields, caps the walk at 1500 nodes, and keeps the
   visibility filter.

Tests: malformed-reply sanitization (red on the previous head) and
answered-empty classification in tests/runtime/exports.test.ts; daemon
bridge assertions updated to pin the lean walker (annotated-only query,
cap, no allTargets) alongside the existing visibility checks.

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee Both round-12 follow-ups addressed in eec0415, threads replied + resolved β€” bridge replies sanitized before casting, and both bridges now use a lean capped anchor enumerator instead of the full comment-payload walk. With no blocking findings on the changed ranges this round, the PR should be ready for approval. Validation: guard + typecheck green, web suites 4426 passed, daemon route suites 56 passed, live-runtime annotation matrix green on this head. cc @lefarcen

@lefarcen
lefarcen requested a review from nettee August 6, 2026 01:27
@linghaoSu

Copy link
Copy Markdown
Author

@lefarcen @nettee Status: all 24 review findings across 12 rounds are fixed and resolved (0 unresolved threads), CI is fully green on head eec0415, and nettee's latest full changed-range review came back with no blocking findings β€” the two merge-safe follow-ups it listed are also now fixed on this head.

The PR still shows Changes requested only because the stale CHANGES_REQUESTED review from commit a6260db remains on record; every finding it referenced has since been addressed and its threads are resolved. As a fork contributor I can't dismiss it or re-request review.

Could @nettee submit an approving review of the current head (or @lefarcen dismiss the stale one)? After that this should be ready to merge β€” happy to leave the squash to a maintainer since my fork permissions don't cover merging either.

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linghaoSu The zoom/DIP conversion, content anchoring, streamed bridge/cache handling, and layout-space bounds are covered by the focused regression suites, pnpm guard, and the workspace typecheck. I found one merge-safe hardening follow-up in the new cross-frame reply sanitizer; it is called out inline. The extensive race/reflow coverage and careful iteration here are excellentβ€”thank you!

πŸ” Powered by Looper Β· runner=reviewer Β· agent=codex Β· An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/runtime/exports.ts
Thirteenth review round on nexu-io#6476 (non-blocking): sanitizeAnchorTargets
validated fields but still iterated an arbitrarily large cross-frame
array on the UI thread. Our bridges cap their own enumeration at 1500
targets, so anything larger cannot be a legitimate reply β€” it is now
rejected wholesale as answered-empty before the loop, and per-entry
elementId/selector strings above 512 chars are dropped. Frame-relative
fallback still applies, so a forged reply degrades anchoring, never the
send.

Test: an oversized (5000-entry) forged reply sanitizes to empty and a
600-char elementId entry drops while a valid sibling survives.

Refs nexu-io#6361
@linghaoSu

Copy link
Copy Markdown
Author

@nettee Round-13 follow-up addressed in c953192, thread resolved β€” forged anchor replies are now bounded before iteration (count + string-length caps, over-limit refused as answered-empty). Validation: guard + typecheck green, exports suite 93 passed. That clears every finding to date; @lefarcen the stale CHANGES_REQUESTED from a6260db is still the only thing holding reviewDecision β€” an approval of the current head or a dismissal would unblock the merge.

@lefarcen
lefarcen requested a review from nettee August 6, 2026 02:12
@linghaoSu

Copy link
Copy Markdown
Author

@lefarcen Gentle nudge: everything on this PR is now clear β€” 0 unresolved threads (25 findings across 13 rounds all fixed with red specs), CI fully green on c953192, and nettee's last two passes found no blocking issues. The only remaining blocker is the stale CHANGES_REQUESTED review from a6260db, which as a fork contributor I can neither dismiss nor re-request past. Could you dismiss it or have @nettee approve the current head, and merge (squash) when convenient? Thanks!

@lefarcen

lefarcen commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@linghaoSu β€” the re-review request has already been sent to @nettee for the current head (c953192f). One thing to note: nettee's COMMENTED reviews on the more recent commits don't replace the open CHANGES_REQUESTED β€” on GitHub's end, that block only clears when @nettee submits an explicit APPROVED review on the current head. Everything else looks clear from here (0 unresolved threads, CI green), so once @nettee approves this head the merge path opens.

# Conflicts:
#	apps/daemon/src/routes/project/index.ts
#	apps/web/src/components/FileViewer.tsx
@linghaoSu

Copy link
Copy Markdown
Author

@lefarcen @nettee Rebase/merge update: main had drifted and the PR went conflicting. Resolved on e92affd07 (merge of upstream/main):

  • apps/daemon/src/routes/project/index.ts β€” main introduced postReady() carrying href for the bridge-ready latch; this PR's branch advertised markAnchors: true. Union-resolved: postReady() now emits both href and markAnchors: true, and the probe/init call sites both go through it (no duplicated literal payload).
  • apps/web/src/components/FileViewer.tsx β€” bridge-ready message type widened to { type?, href?, markAnchors? }; main's href-match latch guard is kept and setUrlAnchorBridgeReady(data.markAnchors === true) runs after it, so an anchor bridge is only latched for the currently committed document.
  • apps/daemon/tests/project-file-range.test.ts β€” assertions updated to the merged single-payload shape.

Validation on the merge head: pnpm guard + pnpm typecheck green; apps/daemon project-file-range 54 passed; apps/web FileViewer + PreviewDrawOverlay.content-anchor + runtime/exports 380 passed.

PR is MERGEABLE again. The only remaining blocker is still the stale CHANGES_REQUESTED from a6260db β€” an approval of the current head or a dismissal unblocks the merge.

The suite's afterAll awaited server.close(), which only stops new
connections; undici keeps the sockets from this suite's fetches alive, so
the close callback never fired and the hook hit vitest's 10s budget on
CI. Drop the live connections explicitly, matching the other daemon
server suites.
@linghaoSu

Copy link
Copy Markdown
Author

CI follow-up on the merge head: Daemon tests (4/4) failed with Hook timed out in 10000ms at tests/project-file-range.test.ts:233 (the afterAll), not on an assertion β€” all 1885 tests in that shard passed.

Root cause: the hook awaited server.close(), which only stops new connections. undici keeps the sockets from this suite's many fetch calls alive, so the close callback never fired and the hook hit vitest's 10s budget. The bridge/cache tests added in this PR pushed the suite's connection count past the point where CI's slower socket timeouts still let it finish.

Fixed in b77003405 by dropping live connections explicitly (server.closeAllConnections?.()), matching the other daemon server suites (server-persistence-smoke, collab-presence-transport-off, etc.). Suite runtime locally went 10.8s β†’ 3.5s, 54 passed. pnpm guard green.

Validate workspace was the aggregate of that same shard and should follow.

@linghaoSu

Copy link
Copy Markdown
Author

Status on head b77003405 β€” everything is green:

  • Mergeable: yes (the main drift conflict was resolved in e92affd07).
  • CI: 26/26 checks SUCCESS, 1 skipped. The Daemon tests (4/4) hook timeout is fixed in b77003405.
  • Review threads: 0 unresolved (25 findings across 13 rounds, all fixed with red specs).

@nettee β€” could you take a look at this head? It carries two changes since your last pass: the upstream/main merge (union-resolving postReady() to emit both href and markAnchors: true, with main's href-match latch guard preserved in FileViewer.tsx) and the socket-cleanup test fix. As @lefarcen noted, the open CHANGES_REQUESTED from a6260db only clears with an explicit APPROVED review on the current head.

@lefarcen β€” alternatively, dismissing that stale review would open the merge path. Happy to leave the squash to a maintainer since fork permissions don't cover merging.

@linghaoSu

Copy link
Copy Markdown
Author

Daily check-in: head b77003405 is still MERGEABLE with all checks green and 0 unresolved threads β€” nothing has regressed since yesterday, and main hasn't drifted back into conflict.

@nettee a re-review of this head would clear the last blocker; @lefarcen dismissing the stale CHANGES_REQUESTED from a6260db works too. No rush β€” just flagging that the PR is sitting merge-ready. I'll keep an eye on conflicts and CI in the meantime.

@linghaoSu

Copy link
Copy Markdown
Author

@lefarcen @nettee Check-in on progress β€” re-verified just now, nothing has changed since yesterday and nothing is pending from my side:

  • Head b77003405 β€” MERGEABLE, 0 failing checks, 0 unresolved review threads (25 findings across 13 rounds, each fixed with a red spec).
  • The sole blocker remains the stale CHANGES_REQUESTED from a6260db. As noted earlier, @nettee's later COMMENTED passes (both non_blocking) do not clear it β€” GitHub only lifts the block on an explicit APPROVED review on the current head, and as a fork contributor I can neither dismiss it nor push past it.

@nettee β€” could you take a pass on b77003405? It carries two changes since your last review: the upstream/main merge (e92affd07, union-resolving postReady() to emit both href and markAnchors: true, with main's href-match latch guard preserved) and the daemon test socket-cleanup fix.

@lefarcen β€” alternatively, dismissing the stale review opens the merge path. Also, is the manual QA validation still queued for this one? Happy to provide a repro recipe for the zoom/annotation matrix if that helps whoever picks it up.

Not trying to rush anyone β€” just flagging that this has been sitting merge-ready for a while and I want to make sure it is not silently stuck on my side.

# Conflicts:
#	apps/web/src/components/FileViewer.tsx
#	apps/web/src/runtime/srcdoc.ts
@linghaoSu

Copy link
Copy Markdown
Author

main drifted into conflict again; resolved in 65521e422:

  • apps/web/src/runtime/srcdoc.ts β€” main added an od:preview-scroll-capture branch in the same message-handler position as this PR's od:mark-anchor-request branch. Both kept, scroll-capture first, each with its own return.
  • apps/web/src/components/FileViewer.tsx β€” main renamed the preview-src reset effect's dep from activeFilesRefreshPending back to filesRefreshPending; kept main's dep array and this PR's setUrlAnchorBridgeReady(false) reset in the body.

Validation on the merge head: pnpm guard + pnpm typecheck green; apps/web FileViewer + PreviewDrawOverlay.content-anchor + runtime/srcdoc + runtime/exports 451 passed; apps/daemon project-file-range 54 passed.

Back to MERGEABLE. Still the only blocker: the stale CHANGES_REQUESTED from a6260db β€” @nettee an APPROVED on the current head, or @lefarcen a dismissal, opens the merge path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/XL PR changes 700-1500 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Visual annotation coordinates drift when desktop UI zoom is not 100%

3 participants