Redesign review comment cards: author border, hover linking, unresolve - #15
Open
angusbezzina wants to merge 1 commit into
Open
Redesign review comment cards: author border, hover linking, unresolve#15angusbezzina wants to merge 1 commit into
angusbezzina wants to merge 1 commit into
Conversation
Implements the attn-bb6t epic across the native and hosted surfaces. Card border (attn-bb6t.1): the 3px left accent strip becomes a full 2px border in the comment author's identity colour. The strip was a positioned pseudo-element because neither border-left nor an inset shadow survives the 6px corner radius — one mitres, the other tapers. A uniform border has neither failure mode, so the pseudo-element, its stacking context and the -1px offsets that kept it as tall as the card all go with it. Active no longer recolours the border (that would erase author identity); it is an outer ring instead. Excerpts and hover linking (attn-bb6t.2): the quoted excerpt is gone from anchored cards, replaced by hover in both directions. Stale cards keep their quote — a stale anchor paints no inline mark, so the quote is the only thing left that says what the comment was about. Card-to-document hover toggles a class straight on the mark DOM rather than rebuilding the decoration set, which is what the existing perf note in that file asks for. The is-focused class, emitted by the plugin since it was written but never styled anywhere, is now implemented too. HTML annotations (attn-bb6t.3): hovered becomes its own AnchorRenderState, distinct from active, with a third CSS Custom Highlight bucket. Restoring an anchor's base state on un-hover is owned by the bridge, which retains the rendered specs — a shell that got this wrong would leave a resolved anchor painted as unresolved. The frame-to-shell direction needs a new additive anchorHover message: text ranges are highlights, not DOM nodes, so they receive no events and are hit-tested against getClientRects(). Unresolve (attn-bb6t.4/.5): resolve was one-way by design, so reopening needs a new CommentReopened event — authorised exactly like resolve (non-agent, self-attributed), threaded through the session facades, and pinned by a new signing-corpus vector. The projection now folds resolve and reopen last-writer-wins by event comparison: the log is not delivery-ordered, so a Set of closed ids would flip a thread open or closed depending on arrival. Resolved cards gain an Unresolve action, the ticks become Lucide check icons, and the resolved badge spans the rail like the cards it sits among. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
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.
Implements the attn-bb6t epic (all five children) across the native app, the hosted owner shell, and the browser reviewer. Closes attn-bb6t.
What changed
Card border — the 3px left accent strip becomes a full 2px border in the comment author's identity colour, radius unchanged. The strip existed as a positioned pseudo-element because neither
border-leftnor an inset shadow survives the 6px corner radius: one mitres into the curve, the other tapers at both ends. A uniform border has neither failure mode, so the pseudo-element, its stacking context, and the-1pxoffsets that kept it as tall as the card are all gone with it. Active/selected no longer recolours the border — that would erase author identity — and is an outer ring instead.Excerpts and hover linking — the quoted excerpt is removed from anchored cards and replaced by hover in both directions. Stale cards keep their quote: a stale anchor paints no inline mark, so the quote is the only surviving trace of what the comment was about.
Card→document hover toggles a class directly on the mark DOM rather than rebuilding the decoration set, which is what the existing perf note in
App.svelteasks for (buildDecorationsrebuilds every mark in the document; hovering a card shouldn't cost that). While wiring this I found the plugin has always emitted anis-focusedclass with no CSS anywhere — clicking a card gave zero document feedback. That's implemented too.HTML annotations —
hoveredbecomes its ownAnchorRenderState, deliberately distinct fromactive(which means "focused"), with a third CSS Custom Highlight bucket. Restoring an anchor's base state on un-hover is owned by the bridge rather than the three shells, because the bridge is the only thing retaining the rendered specs — a shell that got this wrong would leave a resolved anchor painted as unresolved.The frame→shell direction needed a new additive
anchorHovermessage (DOC_PROTOCOL_VERSIONstays 1; both sides already ignore unknown types). Text ranges are CSS Custom Highlights, not DOM nodes, so they receive no events at all and are hit-tested geometrically againstgetClientRects().Unresolve — resolve was one-way by design (there's a comment in
manager.rssaying reopening would need a futureCommentReopenedevent), so this adds that event: authorised exactly like resolve (non-agent, self-attributed), threaded through the full session facade chain, and pinned by a new shared signing-corpus vector.The subtle part is the projection. Resolved state was an add-only
Setof thread ids, and the event array is not in log order — replay and live delivery interleave peers. A naive add/delete pass would flip a thread open or closed depending on arrival order.reconstructThreadsnow folds resolve/reopen last-writer-wins by the same comparator that orders the log.Resolved cards gain an Unresolve action (the resolved footer branch was deliberately empty under attn-42y, which was a statement about the protocol rather than a design choice), the ticks become Lucide check icons, and the resolved badge spans the rail like the cards it sits among.
Verification
Verified against an isolated checkout of this commit alone, not the tree it was developed in — the commit deliberately excludes unrelated in-flight work from a parallel session, so "it works locally" wouldn't have proven much.
svelte-checkclean, doc-runtime artifact in sync, production web build OKNotes for review
CommentReopenedis a new externally-tagged event variant, so receivers on 0.9.0 and earlier reject it outright and will keep showing a reopened thread as resolved — silently. Same family as the snapshot-field problem in attn-mz25; recorded on attn-mz25.1 so the rollout answer covers new event variants too.runtime.generated.js,doc-runtime.generated.ts) are rebuilt and committed, as the build gate requires.🤖 Generated with Claude Code