feat(share): improved shared review — name capture, inline marks, annotation UX - #39
Merged
Conversation
Adds --author-url flag to `arc share show` that reads the registered share from ~/.arc/shares.json and prints the full author URL (with edit_token fragment) to stdout, so it can be piped into pbcopy/xclip by authors who lost their original CLI scrollback.
Update paste-server runbook to reflect the frictionless auth UX: two-URL output from arc share create, token-based author detection, lazy reviewer identity, and the new Frictionless auth scenarios walkthrough section.
Adds a top-right "Share link" button that copies the bare share URL (no &t=) to the reviewer's clipboard. Visible only when isAuthor is true (token present in fragment). Visual treatment: monospace uppercase stamp tied to the existing ink-on-paper aesthetic. On click, briefly switches to the warm-amber --ink-comment palette already established as the "noted" signal. A thin vertical hairline separates the stamp from the identity chip to read as one "header instruments" cluster.
…dal) Reviewers without a name no longer get a separate "What's your name?" modal mid-flow — a real footgun where users typed their comment text INTO the name field and got permanently identified by it. Now: when the floating toolbar opens for a no-name reviewer, it grows two extra rows. A monospace name input on top with an editorial proof-sheet baseline rule. The action icons in the middle stay visible but render at 0.35 opacity (cursor: not-allowed). Below the icons, a mono micro-caption: "ENTER YOUR NAME TO BEGIN". Click an icon while the field is empty: the input snaps to a 2px red outline (--ink-delete-edge), the row shakes laterally for ~250ms, focus moves to the input, and the caption text-swaps to "NAME REQUIRED". After ~700ms the row resets. Type a name + Enter (or click the commit arrow): name persists to localStorage, the two extra rows collapse, the toolbar reflows to its normal single-row icon strip and actions become live. ensureName() and pendingAfterName are now dead code — toolbar actions are guaranteed to have a name by the time they reach the page handler. NamePromptModal is still used for the chip-click rename flow (which is intentional and prefilled, no risk of confusion).
Reviewers can now delete their own annotations, with the asymmetry
discussed in design: only the original commenter retracts; plan author
keeps Reject (with reply) for unwanted feedback so rationale survives
in the audit trail.
Mechanism: new RetractionEvent kind. Replay marks the target comment
status='retracted' iff the event's author_name matches the comment's.
Forged retractions (third-party or plan-author-targeting-someone-else)
are silently dropped at replay, mirroring the EditEvent authorization.
UI: trash icon next to ✎ Edit on the action bar, gated on canRetract
(commenter matches reviewerName + status open/reopened). Click expands
an inline confirm row mirroring the reject-reply pattern's footprint —
prose explanation + Cancel / Yes-delete buttons in --ink-delete palette.
No textarea (retraction has no rationale field).
Retracted comments are filtered from:
- Right-rail annotations panel
- Inline document marks
- `arc share comments` default output
- JSON bundle for LLM consumers
The encrypted retraction event remains in the log so the action is
auditable; downstream consumers don't see (and don't act on) revoked
material.
Tests:
- 5 new replay cases in events.test.ts (happy path + 3 forge cases +
one chronological override edge case)
- TestRunShareCommentsHidesRetracted in cmd/arc covering CLI default
output filtering plus a forged retraction that must NOT take effect
selection.toString() inserts newlines between block-level elements, so a multi-block annotation's quotedText never matched any single block's textContent — the lineStart-only querySelector returned a block, the needle wasn't found in it, and the wrap silently failed. The card still showed in the rail; only the inline yellow mark was missing. Fix: when lineStart !== lineEnd, split quotedText on \n+ and wrap each segment in its corresponding [data-source-line] block, walked in source order. Single-block annotations keep the existing fast path. If segment count and block count diverge (rare — would mean the markdown renderer collapsed/dropped a block since the comment was posted), wrap as many as align rather than nothing — partial highlight beats a missing one.
Previous fix split needle by \n+ and paired one segment per
[data-source-line] block. That works for paragraph + paragraph but
breaks for any block that has multi-line internal structure:
- <ul> with several <li>s (one source line, many \n-separated items
in selection.toString())
- <pre><code> with many lines (one source line, many newlines in
selection)
When segment count and block count diverge, fall back to wrapping every
non-whitespace text node across the blocks in range. Per-text-node
wrap (rather than a Range spanning sibling block elements) keeps the
list/code structure intact — ranges that cross <li> boundaries either
fail surroundContents or, on extractContents fallback, flatten the
list into one <mark>.
Trade-off: in the rare partial-mid-block multi-block selection that
hits the fallback, this over-highlights the partial blocks. Strictly
better than the no-highlight failure mode it replaces.
…cture
Two-tier match + two-tier wrap, replacing the special-cased single vs
multi-block paths.
Search (in this order):
1. Exact substring of the flat concatenated text-node string.
2. Whitespace-normalized match. Collapses runs of whitespace
(including the \n that selection.toString() inserts between
block-level elements), with a position map back to raw offsets so
the wrap step can act on real text-node positions.
Wrap (in this order):
1. Range surroundContents from start text node to end text node.
Works when the range doesn't cross sibling block elements.
2. Per-text-node wrap. Each text node's slice within [start, end) is
wrapped in its own <mark>. Text-node ranges are always safe to
surroundContents regardless of ancestor structure, so this
preserves <li>/<p>/<code> boundaries that the Range path can't
surround.
Replaces the prior wrapAllTextNodesInBlocks fallback (which over-
highlighted entire blocks). The new path computes the actual matched
region and only wraps that — partial selections inside lists and code
blocks now show the correct extent of yellow markup.
The previous extractContents fallback (in the catch of surroundContents)
is removed: extracting across <li> boundaries flattened the list into
one <mark>, breaking structure. Per-text-node wrap is always safe.
selection.toString() inserts \n at <br> elements and at block boundaries (between <li>s, between <p>s, after a heading), but a SHOW_TEXT TreeWalker over the same DOM yields zero whitespace between adjacent text nodes. The whitespace-normalization fallback could only collapse existing runs, so multi-block selections with no inter-block whitespace (e.g. heading + bulleted list) and selections spanning a markdown hard line break (`<br>`) silently failed to highlight. Walk SHOW_TEXT | SHOW_ELEMENT and build a parallel `searchSpace` string that mirrors selection.toString()'s output by inserting synthetic '\n' at every block-level boundary (closest-block ancestor differs) and at every <br>/<hr> element. Matches map back to acc positions through searchToAcc[], skipping the synthetic chars before the wrap step. Adds inline-annotations.test.ts with regression coverage for the heading + bulleted list, two-paragraph, hard-break, and code-block cases — all 9 pass under jsdom.
Dev workflow against a non-default arc-paste port (e.g. :7436) needs the Vite proxy to talk to that port. Falls back to :7432 when the env var is unset so the existing default still works.
Remove the always-on caption block at the bottom of the FloatingToolbar
when no reviewer name is set. Three feedback channels now form an
escalation gradient: dimmed icons (passive), hover tooltip "Enter your
name first" (intentional), shake + red outline + auto-refocus on click
(committed). The caption was redundant with the placeholder in the
default state and unnecessary in the error state since the input itself
already shouts.
- Auto-focus the name field on toolbar mount so the user can type
immediately without an extra click.
- Quieter italic placeholder ("your name…") replacing the slightly
verbose "your name to begin".
- Locked action buttons swap their hover title to "Enter your name
first" via a small actionTitle() helper.
- Drop the now-dead .name-caption CSS rules.
Chromium's triple-click frequently lands Range endpoints at element
boundaries (e.g. startContainer = <p>, startOffset = 0) rather than
inside a text node. The old `range.startContainer.parentElement?.closest
('[data-source-line]')` walked from the PARENT of <p> upward — past
the actual block — and returned null when the parent was <article>
(which has no data-source-line). That tripped the empty-selection
dismiss branch in handleMouseUp, hiding the toolbar 50% of the time
on triple-click.
Extract a resolveBlock() helper that uses the node itself as the
closest() search root when it's already an element, and falls back to
parentElement for text nodes. Both endpoint shapes now resolve to the
correct containing block.
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.
Summary
Polishes the shared-review (arc-paste) flow end-to-end: the author/reviewer URL split, an inline name-capture toolbar that replaces the legacy modal, retraction support for commenters, a much more robust inline-highlight implementation, and a few smaller UX nits.
Author + reviewer flow
arc share createprints two URLs (Share + Author);arc share show --author-urlreprints just the author URL.isAuthor(was string-match),postAuthorEventfunnel for forward compat.NamePromptModalacceptsinitialNameso the rename flow seeds the existing name.Toolbar UX
Inline annotation highlights
Four progressive fixes (1862c7e → aeaf15f → f79a578 → 242bcae) culminating in the current implementation:
SHOW_TEXT | SHOW_ELEMENTacross the rendered DOM.searchSpacestring with synthetic\nat every block-level boundary AND every<br>/<hr>element, so it mirrorsSelection.toString()output.Annotation retraction
Triple-click reliability
PlanRenderer.handleMouseUpnow resolves Range endpoints whether they're text nodes or elements. Triple-click in Chromium often lands endpoints at element boundaries (startContainer = <p>, startOffset = 0); the oldparentElement.closest()walked past the containing block to<article>and returned null, dismissing the toolbar 50% of the time. NewresolveBlock()helper handles both endpoint shapes.Dev tooling
vite.config.tsproxy target overridable viaARC_PASTE_BACKEND(so the dev server can talk to a non-default arc-paste port).Test plan
bun run check— clean (svelte-check 0 errors)bun run vitest run— 64/64 passing across 9 filesbun run lint— clean (biome + eslint)<li>and a heading — same behavior.<h2>+ bulleted list → inline highlight covers all bullets.