Comment on shared HTML documents, by element (attn-61t, attn-yqun) - #14
Merged
Conversation
Phases 0-3 of the HTML document annotation epic: give shared HTML documents a comment anchoring substrate and the in-frame runtime that resolves it. Read-only HTML sharing already worked (attn-qgd); this is the commenting half. Design (planning/collab/html-annotation.md, amendments #19/#20): The June-locked "Design B" — serve the shared document from a distinct localhost origin so the frame has a checkable origin — is superseded. It cannot work for the hosted browser reviewer, which has no Rust process: it renders decrypted bytes via srcdoc, so there is no server, port, or origin to serve from. Adopting it would have left every browser-side reviewer unable to comment. Instead the frame keeps its opaque-origin sandbox and the runtime is injected into the HTML source — a pure content transform, so native and hosted run byte-identical code. The channel is a hello -> MessageChannel handshake bound on event.source; origin checking is meaningless on an opaque frame and unnecessary once traffic moves to a private port. Retaining the opaque origin also denies the untrusted document the storage and same-origin capability a real origin would have granted it. The document frame is untrusted: it may propose anchors and report geometry, never create or mutate review state. Comment bodies and the submit action stay in shell-owned UI, so the worst a hostile page can do is misdescribe a proposal the user sees before committing. Landed: - Rust: HtmlAnchor W3C selector layer (CssSelector + ranked fallbacks, TextQuote, TextPosition, RangeSelector) plus an agent-context block, so a comment is actionable to a coding agent that never saw the document. Anchor.html is serde-optional; markdown wire bytes are unchanged. HtmlAnchor::validate bounds every field at the trust boundary. - Rust: SnapshotAnnotation::HtmlSelectorsV1 capability; bootstrap now publishes HTML with it rather than (DocType::Html, None). HTML still never carries a Rust-built anchorIndex — that would need a headless HTML parser in the binary and the size gate forbids it. - Rust: the markdown resolver refuses HTML anchors outright rather than landing them somewhere plausible and wrong; manager validates HTML anchors before persisting or syncing them. - Web: doc-runtime/ (selector generation + resolution, CSS Custom Highlight API text layer, element overlays with inert fills, persistent comment pins, scope breadcrumb), bundled to an injectable IIFE. - Web: doc-protocol.ts with a validating parser for every inbound frame message, and HtmlAnnotationBridge for the handshake and coordinates. - Web: HtmlViewer gains annotate/onBridge. Annotating needs allow-scripts, so hosted reviewers get page scripts enabled while annotating; the frame stays opaque-origin, and read-only viewing is unchanged. Tests: 557 Rust, 97 web test files, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n-ges) The runtime cannot be tested without a browser engine: it depends on live Range geometry, getClientRects, document.getSelection, the CSS Custom Highlight API, and on postMessage/MessageChannel behaving as they do across a genuinely opaque-origin sandboxed iframe — which is the whole premise of the design. The spec stands up a shell harness that injects the runtime into a srcdoc frame with sandbox="allow-scripts" and no allow-same-origin, performs the real hello -> MessageChannel handshake, and asserts against what the frame reports back. Covers the handshake, a text proposal carrying every selector layer, exact resolution, re-anchoring by quote after the document shifts (the reason all layers are written at creation time), stale detection when the content is gone, the cell/row/table scope chain, element overlay + persistent pin with a pointer-events:none fill, and resilience to malformed shell messages. 8 passing. Run with `npm run test:e2e:html-annotation`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…attn-08r) Phases 5 and 6. The annotation runtime now reaches the user: a shared HTML snapshot that declares the client-side annotation capability renders with the runtime injected, mounts the comment margin beside it, and round-trips comments through the same encrypted event path markdown uses. Phase 6 — IPC contract: ReportHtmlAnchorResolution carries the document frame's verdict to the daemon, which finds the event's fileId and emits AnchorResolutionChanged so the rail can show position and confidence. Deliberately local-only: unlike ResolveAnchor it mints no durable event and reaches no peer, because an HTML anchor resolves against *this* client's rendered DOM. Two peers can legitimately disagree, and propagating one peer's view would overwrite the other's correct one. ExactReason and RemappedReason gain ClientResolved rather than reusing a markdown reason. The existing variants each name a step this resolver ran, and for HTML none of them did — labelling a frame-reported match as base_hash_match would assert something the daemon cannot know. Phase 5 — shell wiring: ReviewMargin gains an optional anchorTops record, consulted only when there is no ProseMirror view. An HTML document renders in a cross-origin frame whose DOM the shell cannot touch, so the frame reports its own geometry and the shell forwards it. The markdown path is untouched: with a view present, every code path is exactly what it was. HtmlCommentComposer is a sibling of CommentComposer rather than a variant. That component is built around a ProseMirror view — quote via textBetween, anchor tracking via coordsAtPos, anchor construction from the live selection — none of which exists here. Keeping them apart leaves the markdown authoring path, which everything else in review depends on, completely unmodified. Also carries `annotation` through the two snapshot assembly sites that were silently dropping it; without that the reviewer always saw a read-only document no matter what the owner published. Tests: 557 Rust, 97 web test files, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The existing spec drove the protocol with an inline harness. That proved the document side works but said nothing about the code the app ships, so a regression in HtmlAnnotationBridge or injectDocRuntime would have gone unnoticed. These four cases bundle the real modules into the page. Covers the handshake through the shipped injector, viewport→shell coordinate conversion (with the frame deliberately offset so the two spaces cannot coincide by accident), rejection of a forged hello that did not come from this exact frame — the event.source binding that stands in for an origin check an opaque frame cannot provide — and the queue-before-handshake path, since the shell renders threads well before the frame finishes booting. 12 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hosted reviewer got the full treatment first; this brings the native window to parity so the feature exists on both surfaces rather than only in the browser build. Same shape as BrowserReviewApp: the bridge state mirrors what the document frame reports, threads become renderable anchors keyed by thread id, frame geometry becomes ReviewMargin card tops, and proposals open HtmlCommentComposer. The native margin already mounted unconditionally via rightRailPlaceholder, so it only needed the geometry source and a null view for HTML — the markdown path keeps its ProseMirror view and every code path it had. Also adds an envelope round-trip test for an HTML-anchored comment. That is the convergence-critical path: the anchor is opaque to Rust, so nothing downstream would notice a dropped or reordered selector — the comment would simply land somewhere else on the peer's screen, or nowhere. Tests: 558 Rust, 12 Playwright, 0 svelte-check errors. Note: web/src/lib/review/review-drift-check.test.ts is flaky (~1 in 3) on a 60ms timing race in the test itself. Reproduced independently of these changes; untouched here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sibling to test-html-share-e2e.sh, which proves an HTML document reaches a reviewer read-only. This proves the half that makes it a review surface: the annotation capability survives publish → encrypt → relay → decrypt, the runtime is spliced into the reviewer's frame, and the comment margin mounts. Two assertions are worth calling out. The capability check exists because losing it in transit is invisible — the reviewer silently falls back to the read-only viewer, which looks identical until you try to comment. The sandbox check asserts the frame gained allow-scripts but NOT allow-same-origin, since the whole isolation argument in amendments #19 rests on the document staying on an opaque origin. The assertions are deliberately shell-observable rather than reaching into the frame: the automation bridge evaluates in the shell's context and cannot cross into a sandboxed cross-origin document. The frame's own behavior is covered in a real browser by web/e2e/html-annotation-runtime.spec.ts. Adds task test:html-annotation and task test:html-annotation:runtime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… calibration follow-ups Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Post-hoc review of attn-61t (3 independent passes over the full diff) surfaced these; all verified by test before fixing. Frame runtime and injection: - positionAt counted astral chars as 6 bytes (per-code-unit byteLength on lone surrogates); every offset right of an emoji skewed. Walk code points. - Producers sliced by CHARS against wire caps in BYTES, so selections in CJK/emoji documents blew the parser caps and the whole message was silently dropped (commenting dead on non-ASCII docs). New clampText enforces both bounds without splitting surrogate pairs; applied to quote/scopePreview/preview/title; prefix/suffix now cut by code points. - Normalized-quote tier probed RAW text with 24 normalized chars, failing exactly when the cosmetic edit sat in the probe, and sized the range with the collapsed-whitespace length. normalizeTextWithMap maps the normalized match back to exact raw offsets. - injectDocRuntime spliced at the FIRST </body> — including one inside a comment, script string, or attribute (runtime dead or document corrupted). Now splices before the LAST, skips documents already carrying the runtime, and concatenates instead of String.replace (whose $' patterns could splice document text into the script). Runtime also guards itself with a window global against double boot. build-doc-runtime escapes </script and <!-- in the bundle and syntax-checks the result. Trust boundary (Rust): - Inbound events never ran HtmlAnchor::validate() — a modified client with valid room keys could sync an unbounded selector blob to every peer's events.jsonl and doc frame. authorize_event now validates CommentCreated and SuggestionCreated anchors (InboundError::InvalidAnchor, test). - CreateSuggestion accepted html-anchored drafts that the apply pipeline can never resolve (v1 non-goal, §8); now refused at creation. Hosted shell: - validateSnapshotPlaintext's html arm required EXACT keys, so a snapshot carrying the annotation capability failed validation outright — a hosted joiner could not hydrate a hosted-published HTML share at all. The arm now accepts and PRESERVES annotation (known value only, html only). - hydrateSnapshot dropped annotation even once parsed — live-joining reviewers landed read-only until a refresh replayed the log. Now copied; regression-tested through the real validator + session. - Hosted resolution reports called a native-only IPC that no-ops in the browser; verdicts now apply straight to the review store (local-only by design), with client_resolved added to the TS ResolvedAnchor unions to match the Rust wire. - Both composers bound LIVE snapshot identity to an anchor whose offsets were measured at selection time; identity is now captured at open and the composer closes on republish/file switch. Verified: cargo test green (incl. new inbound rejection test), fmt+check clean; web 98/98 test files (10 new text-bounds + 4 new inject cases + annotation validator cases), svelte-check 0 errors; Playwright 12/12; the dual-instance E2E (task test:html-annotation) run for the first time and passing 9/9 against the rebuilt binary. Markdown review regression-checked: scripts/test-review-e2e.sh matches the merge-base failure profile exactly (34 PASS / 9 pre-existing FAIL; the scroll-tracking delta reproduced as an environmental rAF-throttling flake, tracks correctly in live probes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sample.html is a centered card — no table, no scroll, one heading. It proves the viewer renders, but it cannot exercise the parts of annotation that only a human can judge. This fixture is shaped for that pass: long enough to scroll (margin cards tracking their anchors), a real table with a header row (the cell < row < table scope breadcrumb), a list/code block/blockquote (each its own gutter scope), and one sentence repeated verbatim in two places so prefix/suffix disambiguation can be watched keeping two comments on their own paragraphs. Self-contained — no remote fonts — so the smoke test never depends on the network. Inline script retained deliberately: annotating runs with scripts enabled (html-annotation.md §4). Verified over the real relay: shares, reaches the reviewer with the annotation capability, and the runtime injects with the table intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-mz25.2) The shared dialog body was `grid gap-4 p-6`. A bare grid has ONE implicit column sized `auto` = `minmax(auto, max-content)`, and inside the ScrollArea that resolves to the widest child's max-content width rather than the dialog's own. One unbreakable string — the font-mono project path in the share modal — therefore widened the whole column past the frame, where `overflow-hidden` silently clipped it: 'Select visible' cut mid-word, the file-type labels gone entirely, the create button sliced in half. Measured in the running app: a 542px column inside a 446px content box, 32 elements up to 70px past the right edge. `grid-cols-[minmax(0,1fr)]` resolves the track against the dialog and lets children shrink below min-content, which is precisely what gives the `truncate`/`min-w-0` already on those children a definite width to work against. Same measurement after: 447.5px column, zero overflow, the path ellipsising as intended. The share dialog's project-root row also gains `min-w-0`, since a flex item defaults to `min-width:auto` and refuses to shrink below its nowrap min-content no matter what `truncate` says. Fixes all four dialogs on the primitive (Settings, Share, NamePrompt, ReviewExitConfirm). Vertical scrolling was already correct and stays so: verified the body scrolls its full 301px when the frame is squashed to 240px, with no horizontal overflow at that size either. The mirror-image VERTICAL bug was fixed and guarded in attn-11g4.1.1, but that guard asserted nothing about the horizontal axis, which is how this one reached a user. dialog-scroll.spec.ts now covers both: a width sweep as a blast-radius check, plus an unbreakable-string case that reproduces the real trigger. Verified honest — reverting the fix fails that test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
`wait_for_dual 'h1'` is a fine proxy for "the window rendered" only while the fixture is markdown, which renders into the SHELL's own DOM. An HTML document renders inside a sandboxed, opaque-origin iframe (src=attn://… for a local file, srcdoc for a received snapshot), so its <h1> lives in the frame's document and the automation bridge — which evaluates in the shell's context — can never see it. So `FIXTURE_PATH=….html scripts/dev-collab.sh` timed out after 20s on a window that had rendered perfectly, and tore the whole harness down before the user could click Share. Probed to confirm rather than assume: with the smoke fixture open the shell reports 0 `h1` elements and 1 `[data-slot="html-viewer"]`. The library gains `dual_ready_selector` (fixture path → the selector that proves it rendered) and `wait_for_dual_fixtures`, which waits for each window against ITS OWN fixture's selector — the owner may be on .html while the reviewer is still on .md, so one selector for both cannot work. `start_dual` stashes the reviewer fixture it actually booted so the two calls cannot disagree. `wait_for_dual` is untouched for existing callers. Verified: the failing command now reaches "Daemons running" and stays up; the default markdown path still boots; test:dual passes 10/0/0; the html-annotation E2E passes 9/9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… document subscribeIconPack deliberately emits its current value synchronously on subscribe (icon-pack.ts:71). nativeFileIconResolver forwarded that first emission straight to the subscriber, so FileTree's `iconRevision += 1` ran while the very effect that registered it was still executing. The render reads iconRevision (FileTree.svelte:145,150), so Svelte re-ran the subscription effect until it tripped effect_update_depth_exceeded — which kills the reactive graph. The editor then mounts a single empty paragraph and NO document renders at all, markdown included. Bisected to 2aa663d. The same commit had already found and fixed this exact hazard in the HOSTED registry, whose comment describes the native failure precisely — 'Notifying here would write every recursive FileTree's revision from inside its own subscription effect'. The native resolver was simply left behind, so this mirrors that fix: track the selected pack, keep kicking off the lazy load on the initial emission, and only notify on a genuine change. Later loads still notify through the async path, so icons still repaint (verified: 13 icons across 8 tree items after the change). Not the HTML commenting epic: 5bb122b measures healthy, 2aa663d does not. Verified against the pre-epic baseline exactly — ProseMirror back to 17 children, 1 h1, zero effect errors (was 1 child, 2 errors). The HTML annotation E2E goes from 6 failures to 12/12 including the owner-side assertions; svelte-check clean over 1592 files; 125 web test files pass. The review-surface suite's 13 remaining failures are the known rail/gutter cluster that predates this branch, untouched by icons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
angusbezzina
marked this pull request as ready for review
August 14, 2026 14:13
Smoke testing found the in-frame annotation surface both mis-designed and
broken: no element-by-element inspection, the comment affordance vanished
when the cursor reached for it, and pressing Comment did nothing. All of it
shipped green because the automation bridge evaluates in the shell's DOM and
cannot cross into the sandboxed frame, so the entire interaction layer had
zero coverage.
Two defects made a saved comment look lost, both pre-existing and only
reachable once commenting worked well enough to make one:
- RenderableAnchor objects are built from the Svelte 5 review store, where
every tracked object is a Proxy. postMessage serialises with structured
clone, which throws DataCloneError on a proxy — so renderAnchors threw on
every call, the frame never received an anchor, and no pin, resolution or
geometry ever came back.
- ReviewMargin's HTML branch skipped any thread with no reported geometry,
so those comments rendered nowhere at all. It now falls back to y=0, as
the resolved-chip branch beside it already did: a card stacked at the top
is a position problem, a missing card is a lost comment.
The interaction model is now element-first, as agentation.dev does it: hover
outlines the element under the cursor and names it on a breadcrumb chip
(table > row 2 > cell), clicking it opens the composer, and the breadcrumb
drills to any ancestor scope. Hover chrome and click-to-comment are one
switch, gated on the document actually being under review — the chip is
opaque and painted over the page, so raising it on a document that is merely
being read would occlude and swallow clicks on that page's own links.
Also fixed: the path-mode iframe was never bound to the bridge, so an owner's
local file never completed the handshake; annotatability and the composer
gate keyed off the focused room, which an owner routinely lacks since they
stay on their local file; a click could commit to an element the chip never
named; the runtime's own chrome hid itself under the cursor; and
snapshotPublishesPath could match a same-named file from another folder,
which decides where a comment lands.
Closes the coverage gap with 26 real-browser cases driving a mouse inside an
opaque-origin frame, including one that renders PROXIED anchors — every
earlier bridge test passed plain literals, which is exactly why none caught
the clone failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed inspect chrome Two review findings on the HTML commenting surface. A local HTML file published through overlapping owner rooms (a folder share plus a separate share of the same file) had its annotation snapshot chosen by recency alone, while the rail, margin, and threadsForCurrentFile stayed scoped to the room the path resolves to. A comment could post into a review the person was not looking at and never appear beside the text. The resolved room now wins; recency only breaks ties within it. Turning inspection off (room stopped or revoked, snapshot gone) only stopped new hover chrome from being raised. A chip already on screen handles its own clicks, bypassing the onDocumentClick guard, so it kept swallowing page clicks and emitting proposals for a document that was no longer reviewable. The inspect handler now hides the hover chrome and clears the current scope on the true->false transition. Covered by a new runtime E2E case; verified to fail without the teardown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…scription Adds one-command HMR entry points for the marketing site and the hosted browser app, documents both in AGENTS.md, and adds `prepare: svelte-kit sync` so a fresh site/ checkout has its generated types before first run. Also retitles the html-annotation E2E task to match what it now covers (both sides receive an annotatable frame). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cargo clippy --all-targets -- -D warnings` (the CI quality gate) failed on collapsible_if in inject_html_annotation_runtime. Folded the `>` check into the `if let` as a let-chain; edition 2024 already allows it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
At 50% the scrim barely separated a dialog from the INK theme, where the app surface is already near-black — the modal read as one more panel rather than the thing being asked about. 70% pushes the document and sidebar back without reaching the shadcn default of 80%, which crushes the paper theme. Sheets move with dialogs: they make the same promise about attention, and two scrim weights would look like the app disagreed with itself about which one mattered more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
angusbezzina
changed the title
'Rust Quality (Linux)' failed on resident_away_owner_loop_imports_notifies_focuses_and_survives_restart: by_room[&first_room].body did not start with "2 new comments". The debounce is real wall-clock in a worker thread, and the two first_room events had a durable store append -- a file write plus an fsync -- between them. When that append outlasted the 15ms window on a loaded runner, the trailing-edge batch fired early and the room posted "1 new comment" twice instead of "2 new comments" once. The count assertion above it still passed, because two notifications from two distinct rooms is exactly what a split burst produces; only the body showed it. Every append now happens before the burst, so the window covers the forwarding alone, and the window is 500ms rather than 15ms. The negative waits shrink from 4x to 1-2x the debounce -- a post that is due arrives immediately, so they were only ever paying for the old window being tiny. Net suite time is unchanged (669 tests, 1.9s -> 2.3s, the sleeps overlap). Also names the burst-fold assertion, so a future split reports what it got instead of failing bare. Verified with 6 consecutive runs under 8 spinning CPU hogs. The original could not be reproduced locally -- macOS fsync is too fast to close a 15ms window -- which is consistent with it only ever failing on the Ubuntu runner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t a hang The two anti-enumeration cases had been failing 'Relay Tests (Ubuntu)' on almost every run since 2026-07-27. attn-i38g recorded them as hanging only under GitHub Actions, on the grounds that they finish in ~350ms locally, and told us not to touch the budgets until the hang was explained: if the limiter can stall a request path, a longer timeout would hide a production bug. There is no hang, and nothing is Actions-specific. The ~350ms figure came from running the file on its own. Run the whole integration suite and the same two cases take 19,660ms and 9,216ms on macOS. The cost is the SELF.fetch round trip, not anything under test. An unknown-room GET costs 1.6ms when this file runs alone and ~250ms when the suite shares the workerd isolate. A GET /health -- no Durable Object, no quota, no rate limiter on that path -- costs ~217ms under the same load, the same price. So it scales with sequential calls times suite activity, and these four cases make the most sequential calls in the repo: the cap (antiEnumPerFiveMin=30) can only be exercised by 30+ distinct probes. They were simply first over the line, which is also why they were intermittent rather than uniformly broken. That clears the concern the bead raised. The limiter does not stall anything; a no-op route is equally slow under the same conditions. Ruled out by measurement: file parallelism (--no-file-parallelism changed nothing), accumulated Durable Objects (500 preloaded DOs left the tests fast), a single bad neighbour file (only the full integration set reproduces), and concurrency as a fix (Promise.all over 20 probes measured 1.0x against the sequential loop, loaded and idle -- the pool serializes them regardless). So the only lever is fewer sequential calls. The 'existing rooms' known-room loop drops from 50 hits to 5: it was ~11s of that case's ~19s, and the property under test is a per-request branch that holds on the first hit or not at all, so five guards against an accumulator as well as fifty did. Worst case under full-integration load falls 19,660ms -> 8,967ms. All four cases then get an explicit 30s budget. Every one of them is structurally 30+ sequential calls and was marginal at the 15s default, so the two that still passed were next. Raising the budget here is the measured response to harness overhead, documented in the block comment, not a way to hide a suspected stall. Making antiEnumPerFiveMin configurable would be the real speed fix -- 30 probes becomes ~6 -- but the limiter is a module-level singleton shared across the isolate, and files other than this one still share the "unknown" IP bucket, so a smaller global cap would make unrelated cases 429. Left alone deliberately. Refs: attn-i38g Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comment on shared HTML documents by pointing at things. Hover any element to outline it and see it named on a breadcrumb chip (
table › row 2 › cell), click to comment on it, or select text and use the Comment pill. Works on the hosted reviewer and the native window, and comments ride the same end-to-end-encrypted event path markdown already uses — so the rail, replies, and resolve work unchanged.Closes the
attn-61tepic (9/9) and theattn-yqunepic (4/4). Follow-ups live underattn-mz25andattn-yqun.Two decisions worth reviewing first
Design B is superseded (
amendments.md#19). The June plan called for serving the shared document from a distinct localhost origin. That cannot work for the hosted browser reviewer, which has no Rust process — it renders decrypted bytes viasrcdoc, so there is no server, port, or origin to serve from. Building it would have shipped commenting to the native daemon and left every browser-side reviewer out.Instead the frame keeps its opaque-origin sandbox and the runtime is injected into the HTML source: one code path for both surfaces, and the untrusted document is denied the storage and same-origin reach a real origin would have granted it. The channel is a
hello→MessageChannelhandshake bound onevent.source, since origin checking is meaningless on an opaque frame (event.originis the string"null").The document frame is untrusted (
amendments.md#20). It may propose anchors and report geometry; it never creates or mutates review state. Comment bodies and the submit action stay in shell-owned UI, so the worst a hostile page can do is misdescribe a proposal you see before committing.Rust never parses HTML — anchors are opaque blobs, resolved client-side in the frame — which keeps a headless HTML parser out of the 40 MiB binary budget. HTML anchors are therefore "unverified-by-authority", and client resolution reports are local-only: they mint no durable event and reach no peer, because an HTML anchor resolves against this client's DOM and peers can legitimately disagree.
The interaction model (
attn-yqun)The first human smoke test found the in-frame surface both mis-designed and broken: no element-by-element inspection, the comment affordance vanished when the cursor reached for it, and pressing Comment did nothing. All of it had shipped green, because the automation bridge evaluates in the shell's DOM and cannot cross into the sandboxed frame — the entire interaction layer had zero coverage.
What changed:
Two bugs that made a saved comment look lost
Both pre-existing, both only reachable once commenting worked well enough to make a comment. Found by reproducing the smoke test against a live daemon.
RenderableAnchorobjects are assembled from the Svelte 5 review store, where every tracked object is a Proxy — andpostMessageserialises with structured clone, which throwsDataCloneErroron a proxy.renderAnchorsthrew on every call, so the frame never received an anchor: no pin, no resolution, no geometry. Every prior bridge test passed plain object literals, which is exactly why none caught it.ReviewMargin's HTML branch skipped any thread with no reported geometry, so those comments rendered nowhere at all — saved, threaded, correctly scoped, and invisible. It now falls back toy=0, as the resolved-chip branch beside it already did: a card stacked at the top is a position problem, a missing card is a lost comment.Also fixed: the path-mode iframe was never bound to the bridge, so an owner's local file never completed the handshake; annotatability and the composer gate keyed off the focused room, which an owner routinely lacks since they stay on their local file after sharing; the runtime's own chrome hid itself under the cursor; and
snapshotPublishesPathcould match a same-named file from an unrelated folder — which decides where a comment lands.Hardening after adversarial review
Reviewers swept the full diff across two rounds. The architecture held; the defects were real:
HtmlAnchor::validate()— a modified client with valid room keys could sync unbounded selector blobs into every peer'sevents.jsonland doc frame. Now enforced atauthorize_event. HTML-anchored suggestions (unresolvable by design) are refused at creation.</body>— including one inside a comment or script string — usedString.replace(whose$'patterns could splice document text into the script), and had no double-injection guard.Closing the coverage gap
The reason all of this shipped green was that nothing could see inside the frame. Playwright can — it drives a real mouse into an opaque-origin sandboxed iframe — so that is where the coverage went, rather than adding a debug channel to an untrusted runtime.
26 real-browser cases, including hover→outline→chip, the reach for the chip, click-to-comment, breadcrumb drilling, clicks left alone on a document that cannot take a comment, and anchors delivered as proxies. Six were verified to fail when their fix is reverted. The daemon-level E2E gained a shell-observable assertion that the owner's frame is both annotatable and connected — also verified to fail when its fix is reverted.
Verification
Full Rust suite, 125 web test files, svelte-check clean across 1592 files, clippy and
cargo fmtclean, 26/26 Playwright in real Chromium, and the dual-instance E2E (scripts/test-html-annotation-e2e.sh) passing 13/13 over a real relay. The complete surface — numbered pin, overlay, and a rail card aligned to its element — confirmed visually against a running daemon.Markdown review was regression-checked empirically rather than argued: the review-surface suite produces a failure profile identical to the merge-base, so nothing here changed it.
🤖 Generated with Claude Code