diff --git a/cmd/arc/share.go b/cmd/arc/share.go index 9432c4c..6ebb91f 100644 --- a/cmd/arc/share.go +++ b/cmd/arc/share.go @@ -98,6 +98,21 @@ type editEvent struct { CreatedAt string `json:"created_at"` } +// retractionEvent is the original commenter taking their annotation back. +// Replay marks the target comment with status='retracted'; the printers +// and JSON bundle filter retracted entries out of the default output so +// downstream LLM consumers don't act on revoked material. The encrypted +// event remains in the log as audit. Only an event whose author_name +// matches the target's author_name takes effect — plan author canNOT +// retract someone else's comment (they have Reject for that). +type retractionEvent struct { + Kind string `json:"kind"` // always "retraction" + ID string `json:"id"` + CommentID string `json:"comment_id"` + AuthorName string `json:"author_name"` + CreatedAt string `json:"created_at"` +} + // --- commands --- var shareCmd = &cobra.Command{ @@ -180,6 +195,7 @@ var ( shareCreateTitle string shareCommentsAccepted bool shareCommentsJSON bool + shareShowAuthorURL bool ) func init() { @@ -191,11 +207,15 @@ func init() { shareCreateCmd.Flags().StringVar(&shareCreateAuthor, "author", "", "Author name embedded in the plan (precedence: flag > share_author in "+ "cli-config.json > $ARC_SHARE_AUTHOR > `git config user.name`). "+ - "Reviewers entering this exact name gain Accept/Resolve/Reject controls.") + "Auto-populates the name chip when the Author URL is opened and is "+ + "used for display attribution. Author privileges are gated by the "+ + "&t= in the Author URL, not by this name.") shareCreateCmd.Flags().StringVar(&shareCreateTitle, "title", "", "Optional plan title shown in the share UI header (defaults to the filename)") shareCommentsCmd.Flags().BoolVar(&shareCommentsAccepted, "accepted-only", false, "Only print accepted comments") shareCommentsCmd.Flags().BoolVar(&shareCommentsJSON, "json", false, "Output as JSON") + shareShowCmd.Flags().BoolVar(&shareShowAuthorURL, "author-url", false, + "Print the author URL (includes edit_token) instead of the plan content") shareCmd.AddCommand(shareCreateCmd, shareListCmd, shareShowCmd, shareCommentsCmd, sharePullCmd, shareApproveCmd, shareUpdateCmd, shareDeleteCmd) @@ -254,8 +274,12 @@ func runShareCreate(cmd *cobra.Command, args []string) error { }); err != nil { return err } - fmt.Printf("Share URL: %s/share/%s#k=%s\n", strings.TrimRight(server, "/"), resp.ID, keyB64) - fmt.Printf("Edit token: %s (saved in ~/.arc/shares.json — keep safe)\n", resp.EditToken) + trimmedServer := strings.TrimRight(server, "/") + fmt.Printf("Share URL (send to reviewers):\n %s/share/%s#k=%s\n\n", + trimmedServer, resp.ID, keyB64) + fmt.Printf("Author URL (keep private — gives you Accept/Resolve):\n %s/share/%s#k=%s&t=%s\n\n", + trimmedServer, resp.ID, keyB64, resp.EditToken) + fmt.Println("Edit token saved to ~/.arc/shares.json") return nil } @@ -275,6 +299,9 @@ func runShareList(cmd *cobra.Command, args []string) error { } func runShareShow(cmd *cobra.Command, args []string) error { + if shareShowAuthorURL { + return printAuthorURL(args[0]) + } id, server, key, err := resolveShareRef(args[0]) if err != nil { return err @@ -287,6 +314,22 @@ func runShareShow(cmd *cobra.Command, args []string) error { return nil } +func printAuthorURL(ref string) error { + // Resolve to id; accept either bare id or full share URL. + id, _, _, err := resolveShareRef(ref) + if err != nil { + return err + } + s, _ := sharesconfig.Find(id) + if s == nil || s.EditToken == "" || s.KeyB64Url == "" { + return fmt.Errorf("no edit_token for share %s in ~/.arc/shares.json "+ + "(--author-url requires a share registered on this machine)", id) + } + fmt.Printf("%s/share/%s#k=%s&t=%s\n", + strings.TrimRight(s.URL, "/"), id, s.KeyB64Url, s.EditToken) + return nil +} + func runShareComments(cmd *cobra.Command, args []string) error { id, server, key, err := resolveShareRef(args[0]) if err != nil { @@ -377,10 +420,11 @@ func runShareDelete(cmd *cobra.Command, args []string) error { // // Returns "" if none of these produce a value. // -// The author name is the only thing that lets the share UI distinguish the -// plan owner from reviewers — when a visitor enters this exact name in the -// SPA's name prompt, they get Accept/Resolve/Reject controls. Without it, -// nobody is recognized as the author and the controls stay hidden for all. +// The author name is used for: (a) auto-populating the name chip when the +// Author URL is opened in the SPA, (b) display attribution on resolution and +// edit events, and (c) the replay-time guard in events.ts that drops forged +// edits/resolutions. Author UI privileges (Accept/Resolve/Reject) are gated +// by the &t= in the Author URL fragment, not by this name. func resolveAuthor(flag string) string { if s := strings.TrimSpace(flag); s != "" { return s @@ -563,8 +607,8 @@ func printComments(server, id string, key []byte, acceptedOnly, asJSON bool) err return err } decoded := decodeAndSortEvents(events, key) - comments, resolutions := replayEvents(decoded, plan.AuthorName) - entries := buildCommentEntries(comments, resolutions, acceptedOnly) + comments, resolutions, retracted := replayEvents(decoded, plan.AuthorName) + entries := buildCommentEntries(comments, resolutions, retracted, acceptedOnly) if asJSON { return emitBundle(id, plan, entries) } @@ -609,9 +653,13 @@ func decodeAndSortEvents(events []paste.Event, key []byte) []decodedEvent { return out } -func replayEvents(events []decodedEvent, planAuthor string) (map[string]commentEvent, map[string]resolutionEvent) { +func replayEvents( + events []decodedEvent, + planAuthor string, +) (map[string]commentEvent, map[string]resolutionEvent, map[string]bool) { comments := map[string]commentEvent{} resolutions := map[string]resolutionEvent{} + retracted := map[string]bool{} for _, d := range events { switch d.kind { case "comment": @@ -620,9 +668,31 @@ func replayEvents(events []decodedEvent, planAuthor string) (map[string]commentE applyResolutionEvent(d.raw, planAuthor, resolutions) case "edit": applyEditEvent(d.raw, planAuthor, comments) + case "retraction": + applyRetractionEvent(d.raw, comments, retracted) } } - return comments, resolutions + return comments, resolutions, retracted +} + +// applyRetractionEvent marks the target comment as retracted iff the event's +// author_name matches the comment's original author_name. Plan author canNOT +// retract someone else's comment — they have Reject (with reply) for that +// purpose. Forged retractions are silently dropped, matching the edit-event +// authorization model. +func applyRetractionEvent(raw json.RawMessage, comments map[string]commentEvent, retracted map[string]bool) { + var r retractionEvent + if err := json.Unmarshal(raw, &r); err != nil { + return + } + target, ok := comments[r.CommentID] + if !ok { + return + } + if r.AuthorName != target.AuthorName { + return + } + retracted[r.CommentID] = true } func applyCommentEvent(raw json.RawMessage, comments map[string]commentEvent) { @@ -680,12 +750,19 @@ func applyEditEvent(raw json.RawMessage, planAuthor string, comments map[string] func buildCommentEntries( comments map[string]commentEvent, resolutions map[string]resolutionEvent, + retracted map[string]bool, acceptedOnly bool, ) []commentEntry { // Build a deterministic-order list of comment entries so multiple runs // produce identical output (Go map iteration is randomized). entries := make([]commentEntry, 0, len(comments)) for cid, c := range comments { + // Retracted comments are filtered from default output; the encrypted + // event still lives in the log for audit but downstream LLM consumers + // shouldn't see (and act on) revoked material. + if retracted[cid] { + continue + } status := "open" reply := "" if res, ok := resolutions[cid]; ok { diff --git a/cmd/arc/share_test.go b/cmd/arc/share_test.go index 83baedf..ffc3a8f 100644 --- a/cmd/arc/share_test.go +++ b/cmd/arc/share_test.go @@ -108,6 +108,77 @@ func TestRunShareCommentsRoundTrip(t *testing.T) { } } +// TestRunShareCommentsHidesRetracted verifies that a retraction event from the +// comment's original author removes the comment from `arc share comments` +// default output, while a forged retraction (mismatched author_name) is +// silently dropped at replay time. The encrypted retraction event remains in +// the log for audit. +func TestRunShareCommentsHidesRetracted(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + srv := startTestPasteServer(t) + defer srv.Close() + + plan := filepath.Join(t.TempDir(), "p.md") + _ = os.WriteFile(plan, []byte("# P"), 0o600) + shareCreateServer = srv.URL + if err := runShareCreate(shareCreateCmd, []string{plan}); err != nil { + t.Fatal(err) + } + f, _ := sharesconfig.Load() + s := f.Shares[0] + keyBytes := mustDecodeKey(t, s.KeyB64Url) + + postEv := func(t *testing.T, payload map[string]any) { + t.Helper() + blob, iv, err := paste.EncryptJSON(payload, keyBytes) + if err != nil { + t.Fatalf("encrypt: %v", err) + } + body, _ := json.Marshal(paste.AppendEventRequest{Blob: blob, IV: iv}) + if err := postRaw(t, srv.URL+"/api/paste/"+s.ID+"/blobs", body); err != nil { + t.Fatal(err) + } + } + + // Two comments: Alice's (which she'll retract) and Bob's (which she can't). + postEv(t, map[string]any{ + "kind": "comment", "id": "c1", "author_name": "Alice", "comment_type": "comment", + "body": "regret posting this", + "anchor": map[string]any{"line_start": 1, "line_end": 1, "quoted_text": "P"}, + "created_at": "2026-04-29T00:00:00Z", + }) + postEv(t, map[string]any{ + "kind": "comment", "id": "c2", "author_name": "Bob", "comment_type": "comment", + "body": "stays visible", + "anchor": map[string]any{"line_start": 1, "line_end": 1, "quoted_text": "P"}, + "created_at": "2026-04-29T00:01:00Z", + }) + + // Alice retracts her own — must take effect. + postEv(t, map[string]any{ + "kind": "retraction", "id": "x1", "comment_id": "c1", "author_name": "Alice", + "created_at": "2026-04-29T00:02:00Z", + }) + // Mallory tries to retract Bob's — must be silently dropped. + postEv(t, map[string]any{ + "kind": "retraction", "id": "x2", "comment_id": "c2", "author_name": "Mallory", + "created_at": "2026-04-29T00:03:00Z", + }) + + out := captureStdout(t, func() { _ = runShareComments(shareCommentsCmd, []string{s.ID}) }) + + if strings.Contains(out, "regret posting this") { + t.Errorf("retracted comment must be hidden; got:\n%s", out) + } + if !strings.Contains(out, "stays visible") { + t.Errorf("forged retraction should not have removed Bob's comment; got:\n%s", out) + } + if !strings.Contains(out, "Bob") { + t.Errorf("expected Bob's comment to still be present; got:\n%s", out) + } +} + // TestRunShareCommentsAppliesEdits verifies that an `edit` event from the // comment's original author rewrites the body shown by `arc share comments`. // This locks in CLI parity with the SPA's replay logic. @@ -516,6 +587,57 @@ func captureStdout(t *testing.T, fn func()) string { // global `configPath` at a temp file, and from the user's git identity by // running each subtest with $PATH cleared so `git` is unavailable (the // helper falls back silently to "" on git failure). +func TestShareCreatePrintsBothURLs(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + srv := startTestPasteServer(t) + defer srv.Close() + + plan := filepath.Join(t.TempDir(), "plan.md") + _ = os.WriteFile(plan, []byte("# Hello\n\nBody."), 0o600) + + // Capture stdout. + r, w, _ := os.Pipe() + stdout := os.Stdout + os.Stdout = w + defer func() { os.Stdout = stdout }() + + shareCreateServer = srv.URL + if err := runShareCreate(shareCreateCmd, []string{plan}); err != nil { + t.Fatalf("runShareCreate: %v", err) + } + _ = w.Close() + out, _ := io.ReadAll(r) + output := string(out) + + if !strings.Contains(output, "Share URL") { + t.Errorf("expected 'Share URL' label, got: %s", output) + } + if !strings.Contains(output, "Author URL") { + t.Errorf("expected 'Author URL' label, got: %s", output) + } + if !strings.Contains(output, "send to reviewers") { + t.Errorf("expected reviewer guidance, got: %s", output) + } + if !strings.Contains(output, "keep private") { + t.Errorf("expected privacy guidance for author URL, got: %s", output) + } + if strings.Contains(output, "Edit token: ") { + t.Errorf("raw 'Edit token: ' line should not appear in output: %s", output) + } + if !strings.Contains(output, "Edit token saved to") { + t.Errorf("expected pointer to shares.json, got: %s", output) + } + + // Author URL must contain &t=. + for line := range strings.SplitSeq(output, "\n") { + if strings.Contains(line, "/share/") && strings.Contains(line, "&t=") { + return + } + } + t.Errorf("expected an author URL line with &t=, got: %s", output) +} + func TestResolveAuthor(t *testing.T) { // Save & restore globals touched by the helper. Env vars use t.Setenv // which auto-restores; configPath is package-global so we restore manually. @@ -660,6 +782,60 @@ func TestResolveServer(t *testing.T) { } } +func TestShareShowAuthorURL(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + srv := startTestPasteServer(t) + defer srv.Close() + + // Create a share so shares.json has an entry. + plan := filepath.Join(t.TempDir(), "plan.md") + _ = os.WriteFile(plan, []byte("# Hi"), 0o600) + shareCreateServer = srv.URL + if err := runShareCreate(shareCreateCmd, []string{plan}); err != nil { + t.Fatal(err) + } + f, _ := sharesconfig.Load() + id := f.Shares[0].ID + editToken := f.Shares[0].EditToken + keyB64 := f.Shares[0].KeyB64Url + + // Capture stdout for `arc share show --author-url`. + r, w, _ := os.Pipe() + stdout := os.Stdout + os.Stdout = w + defer func() { os.Stdout = stdout }() + + shareShowAuthorURL = true + defer func() { shareShowAuthorURL = false }() + if err := runShareShow(shareShowCmd, []string{id}); err != nil { + t.Fatalf("runShareShow: %v", err) + } + _ = w.Close() + out, _ := io.ReadAll(r) + got := strings.TrimSpace(string(out)) + + want := srv.URL + "/share/" + id + "#k=" + keyB64 + "&t=" + editToken + if got != want { + t.Errorf("author URL mismatch:\n got: %q\nwant: %q", got, want) + } +} + +func TestShareShowAuthorURLMissingShare(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + shareShowAuthorURL = true + defer func() { shareShowAuthorURL = false }() + err := runShareShow(shareShowCmd, []string{"abc12345"}) + if err == nil { + t.Fatal("expected error for missing share, got nil") + } + if !strings.Contains(err.Error(), "abc12345") { + t.Errorf("error should reference share id, got: %v", err) + } +} + // writeShareServerConfig points the global configPath at a temp cli-config.json // containing the given share_server (omitted entirely when empty). func writeShareServerConfig(t *testing.T, server string) { diff --git a/docs/review_howto.md b/docs/review_howto.md index 9c7f847..d6fec26 100644 --- a/docs/review_howto.md +++ b/docs/review_howto.md @@ -19,6 +19,21 @@ Mental shortcut: The discriminator is whether the comment should *cause an edit downstream*. Accept is the only path that does. Resolve and Reject both close the thread; the difference is whether the disagreement is worth recording — Reject preserves a reply, Resolve doesn't. +## URLs you'll receive + +`arc share create` prints two URLs: + +| URL | What it is | Who gets it | +|---|---|---| +| **Share URL** (`#k=…`) | Read + comment | Reviewers | +| **Author URL** (`#k=…&t=…`) | Adds Accept / Resolve / Reject | Plan author only | + +The author URL is a strict superset of the share URL — anyone given the author URL can also read and comment. Treat it like a write password: don't paste it into tickets, screenshots, or shared chat threads. The first time you open the author URL, the page detects your role from the URL itself; you don't sign in or pick a name. + +Lost the URL? Run `arc share show --author-url` to reprint it (uses the `edit_token` saved to `~/.arc/shares.json`). + +Reviewers see no sign-in screen. The first time a reviewer leaves a comment, a small modal asks for their name. The name is stored in this browser only. + ## Concrete examples ### Accept diff --git a/docs/runbooks/paste-server.md b/docs/runbooks/paste-server.md index 5530c81..2c86abb 100644 --- a/docs/runbooks/paste-server.md +++ b/docs/runbooks/paste-server.md @@ -59,11 +59,18 @@ Validate the shared review feature. EOF ./bin/arc share create /tmp/test-plan.md --local -# → Share URL: http://localhost:7432/share/#k= -# → Edit token: (saved in ~/.arc/shares.json) - -./bin/arc share list # see all known shares -ls -la ~/.arc/shares.json # verify file mode 0600 +# Output: +# Share URL (send to reviewers): +# http://localhost:7432/share/#k= +# +# Author URL (keep private — gives you Accept/Resolve): +# http://localhost:7432/share/#k=&t= +# +# Edit token saved to ~/.arc/shares.json + +./bin/arc share list # see all known shares +ls -la ~/.arc/shares.json # verify file mode 0600 +./bin/arc share show --author-url # reprint the author URL on demand ``` The author identity embedded in the plan is resolved in this order (highest to lowest priority): @@ -73,7 +80,7 @@ The author identity embedded in the plan is resolved in this order (highest to l 3. `$ARC_SHARE_AUTHOR` env var 4. `git config user.name` -**Note the name that gets used** — you'll need to type it back in the UI to claim the author role. If none of these produce a value, `arc share create` prints a warning and Accept/Resolve/Reject controls won't appear for anyone. +**Note the name that gets used** — it is embedded in the share bundle and displayed automatically when the Author URL is opened. If none of these produce a value, `arc share create` prints a warning and Accept/Resolve/Reject controls won't appear for anyone. To set a persistent default once: @@ -87,16 +94,17 @@ echo '{"share_author":"Ben Firestone"}' | jq -s '.[0] * .[1]' ~/.arc/cli-config. ### Exercise the UI -Open the printed URL in Chrome/Firefox. +Open the **Author URL** (the one with `&t=`) in Chrome/Firefox. -1. **Name prompt** appears on first comment — type the **same name** that was embedded as the author at create time (i.e. your `git config user.name` output, or whatever you passed to `--author`). The reviewer-name chip in the header will read ` · author` once the names match. -2. **Highlight a paragraph** in the rendered plan → floating annotation toolbar appears -3. **Pick a label** (`praise` / `issue` / `suggestion` / `question` / `nit`) -4. **Type a comment**, optionally toggle "Suggest replacement text" -5. **Post** — the comment appears in the sidebar -6. As the author (your localStorage name matches `plan.author_name`), you'll see **Accept / Resolve / Reject** controls. Try Accept on one comment, Reject (with reply) on another, and Resolve on a third. If the controls don't appear, double-check that the name in the header chip matches the value in `git config user.name` (or whatever you passed to `--author`) — the comparison is case- and whitespace-sensitive. -7. As a reviewer (your localStorage name does NOT match the plan's author), find your own annotation in the sidebar. You should see an **✎ Edit** button on it. Click it; the body becomes a textarea pre-filled with your existing comment. Refine the wording — e.g. expand "expand this more" into a fully-formed suggestion — and **⌘/Ctrl-⏎** (or click Save). The card re-renders with `· edited Nm` next to the timestamp. Confirm `arc share comments ` prints the new body, not the original. -8. Switch back to the author window. The **✎ Edit** button should also appear on every reviewer's annotation (not just your own). Use it to sharpen a thin reviewer comment — the displayed `author_name` stays as the reviewer, only the body changes. Run `arc share comments ` again and confirm the refined body shows up; this is the path that lets the author shape feedback for downstream LLM consumption without round-trips with the reviewer. +1. **Open the Author URL.** The header chip immediately reads ` · author` (no modal). Author detection is now driven by the `&t=` fragment param, not a name-string match — so even if your localStorage holds a different name, the page knows you're the author and renders Accept/Resolve/Reject controls. (Remember the URL printed by `arc share create`; if you've lost it, run `arc share show --author-url`.) +2. **Highlight a paragraph** in the rendered plan → floating annotation toolbar appears. +3. **Pick a label** (`praise` / `issue` / `suggestion` / `question` / `nit`). +4. **Type a comment**, optionally toggle "Suggest replacement text". +5. **Post** — the first time you do this, a small modal asks for your name. (As the author you should already have a name from step 1; the modal won't fire.) The comment appears in the sidebar. +6. As the author, you'll see **Accept / Resolve / Reject** controls on every comment. Try Accept on one, Reject (with reply) on another, and Resolve on a third. If the controls don't appear, you opened the Share URL (no `&t=`) instead of the Author URL — close the tab and reopen via the Author URL. +7. As a reviewer (a fresh browser profile or incognito window with NO `&t=` in the URL), find your own annotation in the sidebar. You should see an **✎ Edit** button on it. Click it; the body becomes a textarea pre-filled with your existing comment. Refine the wording — e.g. expand "expand this more" into a fully-formed suggestion — and **⌘/Ctrl-⏎** (or click Save). The card re-renders with `· edited Nm` next to the timestamp. Confirm `arc share comments ` prints the new body, not the original. +8. Switch back to the author window. The **✎ Edit** button should also appear on every reviewer's annotation (not just your own). Use it to sharpen a thin reviewer comment — the displayed `author_name` stays as the reviewer, only the body changes. Run `arc share comments ` again and confirm the refined body shows up. +9. **Click the chip** in the header — the name prompt opens prefilled with your current name (text selected). Edit and save → chip updates. This is the rename affordance. ### Pull comments back to the CLI @@ -108,13 +116,25 @@ Open the printed URL in Chrome/Firefox. ### Simulate a second reviewer -Without spinning up another machine, open the same URL in an **incognito window** (or a different browser entirely). Incognito gets a fresh `localStorage`, so: +Without spinning up another machine, open the **Share URL (no `&t=`)** in an **incognito window** (or a different browser entirely). Incognito gets a fresh `localStorage`, so: + +1. **No chip** appears in the header. There's no "Sign in" button anywhere — identity is captured lazily. +2. Highlight a paragraph and click Comment in the toolbar. A name prompt fires; type any name (e.g. "Reviewer-2"). The comment posts; the chip now reads `Reviewer-2` (no `· author`, because the URL has no `&t=`). +3. Post a few more comments (the prompt won't fire again — name is sticky in localStorage). +4. Refresh the author's window → new comments replay in via `replayEvents()`. + +The author's resolution events still apply: their UI is gated on `authorToken` (in-memory, parsed from `&t=` in the fragment), not a name match. Reviewer-2 cannot resolve comments because Reviewer-2 doesn't have the token — the Accept/Resolve/Reject buttons are never rendered for that profile. + +### Frictionless auth scenarios -1. The name prompt fires again — type any name **other than** the embedded author name (e.g. "Reviewer-2"). The chip in the header should NOT show `· author`. -2. Post a few comments -3. Refresh the author's window → new comments replay in via `replayEvents()` +Walk these in order to confirm the new auth UX end-to-end: -The author's resolution events still apply (their `author_name` matches the plan's). Reviewer-2's comments cannot be marked as `accepted` by Reviewer-2 itself, even if they tried — the client filters out resolution events whose `author_name` doesn't match `plan.author_name`. +1. **Create a share.** `arc share create --local` — confirm output prints both Share URL and Author URL, no raw `Edit token: ` line, and a "saved to ~/.arc/shares.json" pointer. +2. **Reprint the author URL.** `arc share show --author-url` — single line, contains `&t=`. +3. **Author URL flow.** Open the Author URL in a fresh browser profile. Header chip shows ` · author` immediately, no modal. Accept / Resolve buttons visible on existing comments. +4. **Reviewer URL flow.** Open the Share URL (without `&t=`) in a different fresh profile. No chip in the header, no "Sign in" button. Select text → click Comment in the toolbar → name modal opens → save a name → comment posts → chip now shows the name. +5. **Rename via chip.** Click the chip on the reviewer side. Modal opens prefilled with the saved name (text selected). Edit, save → chip updates. +6. **Author opens the bare share URL.** Open the share URL (without `&t=`) on the author's browser. They are now in reviewer mode (no Accept buttons). Reopen via the Author URL → author mode restored. ## 3. Shared / remote review @@ -198,7 +218,7 @@ Step 7 (review loop) uses `arc share approve` and `arc share pull` instead of th | Symptom | Cause | Fix | |---|---|---| -| Accept / Resolve / Reject controls never appear, even when typing the "right" name | Plan was created without an author name (`arc share create` was run pre-fix, or `git config user.name` was empty and no `--author` flag passed). `plan.author_name` is empty, so `isAuthor` is `false` for every reviewer | Recreate the share with `--author "Your Name"` (or set `git config user.name` first), then enter that exact name in the SPA prompt | +| Accept / Resolve / Reject controls never appear | You opened the Share URL (no `&t=`) instead of the Author URL — author detection is token-based, not name-based. Or the share was created without an author name (`git config user.name` was empty and no `--author` flag passed), so `plan.author_name` is empty and the chip never shows `· author` | Close the tab and reopen via the Author URL (run `arc share show --author-url` to retrieve it). If the share has no author name, recreate it with `--author "Your Name"` | | `/share/` returns `{"message":"Not Found"}` (Echo's default 404 JSON) | Binary built without the `webui` build tag — `web.RegisterSPA` is the no-op stub | Rebuild with `make build` (not `make build-quick`); for arc-paste use `go build -tags webui -o ./bin/arc-paste ./arc-paste` | | `/share/` returns blank HTML / cannot find static assets | `web/build/` not present at compile time, even with the `webui` tag | Re-run `bun run build` in `web/`, then rebuild the binary | | SPA console says `missing #k= in URL` | URL was pasted without its fragment | Use the full URL printed by `arc share create` — fragments are dropped by some chat apps; copy carefully | diff --git a/web/src/app.css b/web/src/app.css index 316be49..1c5e33c 100644 --- a/web/src/app.css +++ b/web/src/app.css @@ -993,6 +993,106 @@ color 120ms ease-out; } +/* ── Inline name capture (when reviewer has no name yet) ───────────────── + * The toolbar grows two extra rows: a name input on top, a mono caption + * on the bottom. The icons in the middle are visually inert until the + * input has content. Editorial proof-sheet treatment — a baseline rule + * rather than a form border — keeps the aesthetic of writing on paper. */ + +.share-page .floating-toolbar.needs-name { + min-width: 220px; + padding: 0.5rem 0.625rem 0.4rem; +} + +.share-page .floating-toolbar .name-row { + display: flex; + align-items: center; + gap: 0.4rem; + padding-bottom: 0.45rem; + border-bottom: 1px dashed var(--ink-rule); + transition: + border-color 160ms ease, + transform 80ms ease; +} +/* Required-field treatment: thin red baseline (proof mark) + 2px outline + * pulse on the input itself. The shake fires once via the .is-error class + * being applied for ~700ms by the component. */ +.share-page .floating-toolbar .name-row.is-error { + border-bottom-color: var(--ink-delete-edge); + animation: name-row-shake 240ms cubic-bezier(0.36, 0.07, 0.19, 0.97) both; +} +@keyframes name-row-shake { + 0%, 100% { transform: translateX(0); } + 25% { transform: translateX(-5px); } + 75% { transform: translateX(5px); } +} + +.share-page .floating-toolbar .name-field { + flex: 1; + min-width: 0; + font-family: "JetBrains Mono", ui-monospace, monospace; + font-size: 0.75rem; + letter-spacing: 0.01em; + color: var(--ink-text); + background: transparent; + border: 0; + padding: 0.2rem 0.1rem; + outline: 0; + caret-color: var(--ink-comment); +} +.share-page .floating-toolbar .name-field::placeholder { + color: var(--ink-text-faint); + font-style: italic; +} +.share-page .floating-toolbar .name-row.is-error .name-field { + outline: 2px solid var(--ink-delete-edge); + outline-offset: 2px; + border-radius: 2px; + background: var(--ink-delete-bg); +} + +.share-page .floating-toolbar .name-commit { + display: inline-flex; + align-items: center; + justify-content: center; + width: 22px; + height: 22px; + padding: 0; + border: 1px solid var(--ink-rule); + border-radius: 4px; + background: var(--ink-paper); + color: var(--ink-text-faint); + cursor: pointer; + flex: none; + transition: + color 140ms ease, + border-color 140ms ease, + background-color 140ms ease; +} +.share-page .floating-toolbar .name-commit:hover { + color: var(--ink-text-muted); + border-color: oklch(from var(--ink-rule) calc(l - 0.05) c h); +} +/* When the field has substantive content, the commit arrow "wakes up" to + * the warm amber comment color — the same idiom we use elsewhere for + * "noted / acknowledged" — signaling the action is now available. */ +.share-page .floating-toolbar .name-commit.is-ready { + color: var(--ink-comment); + border-color: var(--ink-comment-edge); + background: var(--ink-comment-bg); +} + +/* Action row: when no name is set, dim the icons so they read as + * unavailable. Click handlers still fire (they trigger the name-row flash); + * the visual signal is intent only. */ +.share-page .floating-toolbar .action-row.is-locked { + opacity: 0.35; + pointer-events: auto; /* keep clickable so we can flash the name field */ +} +.share-page .floating-toolbar .action-row.is-locked button { + cursor: not-allowed; +} + /* Annotation card type chips */ .share-page .chip-comment { color: var(--ink-comment); @@ -1088,3 +1188,76 @@ padding: 0.25rem 0.625rem; border-radius: 9999px; } + +/* Author-only "Share link" stamp. + * + * Visual sibling of the mono "Plan · " label on the left of the header: + * monospace, uppercase, tight tracking. Distinguishes ACTION (verb, mono) from + * IDENTITY (noun, sans pill). Confirmation reuses the established amber + * --ink-comment palette — already the language of "noted / acknowledged" on + * this page — instead of inventing a new accent. */ +.share-page .share-stamp { + display: inline-flex; + align-items: center; + gap: 0.4rem; + min-width: 7.25rem; /* keeps width stable across "Share link" / "Copied" */ + justify-content: center; + font-family: "JetBrains Mono", ui-monospace, monospace; + font-size: 0.6875rem; + font-weight: 500; + letter-spacing: 0.14em; + text-transform: uppercase; + color: var(--ink-text-muted); + background: var(--ink-paper-raised); + border: 1px solid var(--ink-rule); + padding: 0.3rem 0.7rem 0.28rem; + border-radius: 4px; /* squarer than the chip — reads as a stamp, not a tag */ + cursor: pointer; + transition: + color 140ms ease, + border-color 140ms ease, + background-color 140ms ease, + transform 80ms ease, + opacity 80ms ease; +} +.share-page .share-stamp:hover { + color: var(--ink-text); + border-color: oklch(from var(--ink-rule) calc(l - 0.05) c h); +} +.share-page .share-stamp:active { + /* The "stamp press" — a 1px tap on click before the copied state takes + * over. Subliminal; you feel it more than see it. */ + transform: translateY(1px); + opacity: 0.85; +} +.share-page .share-stamp.is-copied { + color: var(--ink-comment); + background: var(--ink-comment-bg); + border-color: var(--ink-comment-edge); +} +.share-page .share-stamp-icon { + display: inline-flex; + width: 12px; + height: 12px; + flex: none; +} +.share-page .share-stamp-label { + display: inline-block; +} + +/* Header instruments group: a thin vertical hairline ties the stamp and the + * identity chip into one cluster on the right side of the header. The rule + * matches --ink-rule from the bottom border of the header itself. */ +.share-page .header-instruments > * + * { + position: relative; +} +.share-page .header-instruments > * + *::before { + content: ""; + position: absolute; + left: -0.625rem; /* halfway into the parent's gap-3 */ + top: 50%; + width: 1px; + height: 0.875rem; + background: var(--ink-rule); + transform: translateY(-50%); +} diff --git a/web/src/lib/paste/events.test.ts b/web/src/lib/paste/events.test.ts index 614409d..7bf5217 100644 --- a/web/src/lib/paste/events.test.ts +++ b/web/src/lib/paste/events.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; import { replayEvents, acceptedOnly } from './events'; -import type { CommentEvent, EditEvent, ResolutionEvent } from './types'; +import type { CommentEvent, EditEvent, ResolutionEvent, RetractionEvent } from './types'; const c: CommentEvent = { kind: 'comment', @@ -192,4 +192,41 @@ describe('replayEvents', () => { }); }); }); + + describe('retraction events', () => { + const retract: RetractionEvent = { + kind: 'retraction', + id: 'x1', + comment_id: 'c1', + author_name: 'Alice', + created_at: '2026-04-29T00:08:00Z' + }; + + it('marks comment retracted when original author retracts', () => { + const states = replayEvents('Ben', [c, retract]); + expect(states.get('c1')?.status).toBe('retracted'); + }); + + it('ignores retraction by someone other than the original commenter', () => { + // Plan author Ben tries to retract Alice's comment — must be silently dropped. + const states = replayEvents('Ben', [c, { ...retract, author_name: 'Ben' }]); + expect(states.get('c1')?.status).toBe('open'); + }); + + it('ignores forged retraction by a third party', () => { + const states = replayEvents('Ben', [c, { ...retract, author_name: 'Mallory' }]); + expect(states.get('c1')?.status).toBe('open'); + }); + + it('ignores retraction targeting a non-existent comment', () => { + const states = replayEvents('Ben', [c, { ...retract, comment_id: 'nope' }]); + expect(states.get('c1')?.status).toBe('open'); + }); + + it('retraction overrides a prior accept (chronological replay)', () => { + // Edge case: author accepts, reviewer then retracts. Last write wins. + const states = replayEvents('Ben', [c, accept, retract]); + expect(states.get('c1')?.status).toBe('retracted'); + }); + }); }); diff --git a/web/src/lib/paste/events.ts b/web/src/lib/paste/events.ts index 205e12f..4a0df3f 100644 --- a/web/src/lib/paste/events.ts +++ b/web/src/lib/paste/events.ts @@ -1,8 +1,16 @@ import type { CommentEvent, EventPlaintext, ResolutionStatus } from './types'; +/** + * 'retracted' is added to the status union for original-commenter retractions. + * It is intentionally distinct from author-driven 'rejected': retraction means + * "I take this back, never mind"; reject means "the author considered this + * and explicitly disagreed." Different semantics, different consumers. + */ +export type CommentStatus = 'open' | ResolutionStatus | 'retracted'; + export type CommentState = { event: CommentEvent; - status: 'open' | ResolutionStatus; + status: CommentStatus; reply?: string; replyAt?: string; /** @@ -58,6 +66,14 @@ export function replayEvents( comment_type: e.comment_type !== undefined ? e.comment_type : target.event.comment_type }; target.editedAt = e.created_at; + } else if (e.kind === 'retraction') { + const target = states.get(e.comment_id); + if (!target) continue; + // Only the original commenter can retract. Plan author canNOT — + // they have Reject (with reply) for that purpose, which preserves + // rationale in the audit trail. + if (e.author_name !== target.event.author_name) continue; + target.status = 'retracted'; } } return states; diff --git a/web/src/lib/paste/identity.test.ts b/web/src/lib/paste/identity.test.ts index 7e5f83b..de1cbc1 100644 --- a/web/src/lib/paste/identity.test.ts +++ b/web/src/lib/paste/identity.test.ts @@ -1,6 +1,6 @@ // @vitest-environment jsdom import { describe, it, expect, beforeEach } from 'vitest'; -import { getReviewerName, setReviewerName, clearReviewerName } from './identity'; +import { getReviewerName, setReviewerName, clearReviewerName, parseShareFragment } from './identity'; describe('identity', () => { beforeEach(() => localStorage.clear()); @@ -21,3 +21,31 @@ describe('identity', () => { expect(getReviewerName()).toBe('Carol'); }); }); + +describe('parseShareFragment', () => { + it('parses k only', () => { + expect(parseShareFragment('#k=abc')).toEqual({ k: 'abc', t: null }); + }); + + it('parses k and t', () => { + expect(parseShareFragment('#k=abc&t=xyz')).toEqual({ k: 'abc', t: 'xyz' }); + }); + + it('order-independent', () => { + expect(parseShareFragment('#t=xyz&k=abc')).toEqual({ k: 'abc', t: 'xyz' }); + }); + + it('handles missing leading #', () => { + expect(parseShareFragment('k=abc&t=xyz')).toEqual({ k: 'abc', t: 'xyz' }); + }); + + it('returns null for missing keys', () => { + expect(parseShareFragment('')).toEqual({ k: null, t: null }); + expect(parseShareFragment('#')).toEqual({ k: null, t: null }); + expect(parseShareFragment('#other=foo')).toEqual({ k: null, t: null }); + }); + + it('treats empty values as null', () => { + expect(parseShareFragment('#k=&t=')).toEqual({ k: null, t: null }); + }); +}); diff --git a/web/src/lib/paste/identity.ts b/web/src/lib/paste/identity.ts index 420b672..1796b59 100644 --- a/web/src/lib/paste/identity.ts +++ b/web/src/lib/paste/identity.ts @@ -14,3 +14,12 @@ export function clearReviewerName(): void { if (typeof localStorage === 'undefined') return; localStorage.removeItem(KEY); } + +export function parseShareFragment(hash: string): { k: string | null; t: string | null } { + const raw = hash.startsWith('#') ? hash.slice(1) : hash; + const params = new URLSearchParams(raw); + return { + k: params.get('k') || null, + t: params.get('t') || null + }; +} diff --git a/web/src/lib/paste/types.ts b/web/src/lib/paste/types.ts index 2ebc9a6..88ccea1 100644 --- a/web/src/lib/paste/types.ts +++ b/web/src/lib/paste/types.ts @@ -121,4 +121,29 @@ export type PlanEditEvent = { created_at: string; }; -export type EventPlaintext = CommentEvent | ResolutionEvent | EditEvent | PlanEditEvent; +/** + * The original commenter retracting their own annotation. Replay marks the + * target comment with status='retracted'; UI hides retracted comments and + * their inline marks. The encrypted event stays in the log so the action is + * auditable, but `arc share comments` filters retracted entries out of the + * default output (LLM consumers shouldn't act on retracted material). + * + * Authorization is replay-time: only an event whose `author_name` matches + * the target comment's `author_name` takes effect. The plan author canNOT + * retract someone else's comment — they must use Reject (with a reply that + * preserves rationale in the audit trail). + */ +export type RetractionEvent = { + kind: 'retraction'; + id: string; + comment_id: string; + author_name: string; + created_at: string; +}; + +export type EventPlaintext = + | CommentEvent + | ResolutionEvent + | EditEvent + | RetractionEvent + | PlanEditEvent; diff --git a/web/src/routes/share/[id]/+page.svelte b/web/src/routes/share/[id]/+page.svelte index fefc35e..d8f1c76 100644 --- a/web/src/routes/share/[id]/+page.svelte +++ b/web/src/routes/share/[id]/+page.svelte @@ -3,7 +3,7 @@ import { PasteClient, b64ToBytes, eventBytes } from '$lib/paste/client'; import { importKey, encryptJSON, decryptJSON } from '$lib/paste/crypto'; import { replayEvents, type CommentState } from '$lib/paste/events'; - import { getReviewerName } from '$lib/paste/identity'; + import { getReviewerName, parseShareFragment, setReviewerName } from '$lib/paste/identity'; import type { PlanPlaintext, EventPlaintext, @@ -12,6 +12,7 @@ EditEvent, ResolutionEvent, ResolutionStatus, + RetractionEvent, Anchor } from '$lib/paste/types'; import PlanRenderer from './components/PlanRenderer.svelte'; @@ -32,8 +33,12 @@ // --- Reviewer identity --- let reviewerName = $state(null); + let authorToken = $state(null); let showNamePrompt = $state(false); - let pendingAfterName: (() => void) | null = null; + + // --- Share-link copy (author-only) --- + let copiedShareLink = $state(false); + let copyResetTimer: ReturnType | null = null; // --- UI state for selection-driven actions --- type SelectionInfo = { @@ -52,23 +57,23 @@ let client: PasteClient | undefined; - const isAuthor = $derived( - reviewerName !== null && - plan !== null && - !!plan.author_name && - plan.author_name === reviewerName - ); + const isAuthor = $derived(authorToken !== null); const orderedStates = $derived.by(() => { - return [...comments.values()].sort((a, b) => - b.event.created_at.localeCompare(a.event.created_at) - ); + return [...comments.values()] + .filter((s) => s.status !== 'retracted') + .sort((a, b) => b.event.created_at.localeCompare(a.event.created_at)); }); const marks = $derived.by((): InlineMark[] => { const out: InlineMark[] = []; for (const state of comments.values()) { - if (state.status === 'resolved' || state.status === 'rejected') continue; + if ( + state.status === 'resolved' || + state.status === 'rejected' || + state.status === 'retracted' + ) + continue; out.push({ id: state.event.id, kind: state.event.action === 'delete' ? 'delete' : 'comment', @@ -85,14 +90,13 @@ client = new PasteClient(window.location.origin); try { - const fragment = window.location.hash.replace(/^#/, ''); - const params = new URLSearchParams(fragment); - const k = params.get('k'); + const { k, t } = parseShareFragment(window.location.hash); if (!k) { loadError = 'Missing #k= in URL — share link is incomplete.'; return; } key = await importKey(k); + authorToken = t; const resp = await client.get(data.id); plan = await decryptJSON( @@ -101,6 +105,16 @@ key ); + // Author URL flow: token + plan author name → auto-populate reviewer identity. + // If the share was created without --author, fall through to the + // reviewerName already loaded from localStorage at the top of onMount — + // the lazy NamePromptModal will fire on first action. isAuthor still + // stays true via authorToken in that case. + if (authorToken && plan?.author_name) { + reviewerName = plan.author_name; + setReviewerName(plan.author_name); + } + const events: EventPlaintext[] = []; for (const ev of resp.events) { const { blob, iv } = eventBytes(ev); @@ -116,21 +130,9 @@ } }); - function ensureName(after: () => void) { - if (reviewerName) { - after(); - return; - } - pendingAfterName = after; - showNamePrompt = true; - } - function handleNameSaved(name: string) { reviewerName = name; showNamePrompt = false; - const cb = pendingAfterName; - pendingAfterName = null; - cb?.(); } function clearSelection() { @@ -141,12 +143,37 @@ sel?.removeAllRanges(); } + // Build the bare share URL (no &t=) from the current location and copy it. + // Author URL fragment carries both k and t; reviewers must only ever see k. + async function copyShareLink() { + const { k } = parseShareFragment(window.location.hash); + if (!k) return; + const url = `${window.location.origin}${window.location.pathname}#k=${k}`; + try { + await navigator.clipboard.writeText(url); + } catch { + return; + } + copiedShareLink = true; + if (copyResetTimer) clearTimeout(copyResetTimer); + copyResetTimer = setTimeout(() => { + copiedShareLink = false; + copyResetTimer = null; + }, 1500); + } + async function postEvent(event: EventPlaintext) { if (!key || !client) return; const { blob, iv } = await encryptJSON(event, key); await client.appendEvent(data.id, blob, iv); } + async function postAuthorEvent(event: EventPlaintext) { + // Phase B: identical to postEvent. Phase C will add an auth header here + // and route the call through a server endpoint that verifies authorToken. + return postEvent(event); + } + function buildAnchor(sel: SelectionInfo): Anchor { return { line_start: sel.lineStart, @@ -183,51 +210,50 @@ comments = next; } + // Name capture moved into FloatingToolbar.svelte: when reviewerName is + // null, the toolbar renders an inline name field and gates its action + // icons on it. By the time we receive an action here, we know the name + // is set — so these handlers no longer need ensureName(). async function handleToolbarAction(action: ToolbarAction) { if (!activeSelection) return; const sel = activeSelection; switch (action) { case 'praise': - ensureName(async () => { - await createComment({ - body: 'Looks good', - comment_type: 'praise', - action: 'comment', - anchor: buildAnchor(sel) - }); - clearSelection(); + await createComment({ + body: 'Looks good', + comment_type: 'praise', + action: 'comment', + anchor: buildAnchor(sel) }); + clearSelection(); return; case 'comment': - ensureName(() => { - popoverMode = 'comment'; - }); + popoverMode = 'comment'; return; case 'delete': - ensureName(async () => { - await createComment({ - body: '', - comment_type: 'comment', - action: 'delete', - anchor: buildAnchor(sel) - }); - clearSelection(); + await createComment({ + body: '', + comment_type: 'comment', + action: 'delete', + anchor: buildAnchor(sel) }); + clearSelection(); return; case 'suggest': - ensureName(() => { - popoverMode = 'suggest'; - }); + popoverMode = 'suggest'; return; case 'quick-label': - ensureName(() => { - showQuickLabel = true; - }); + showQuickLabel = true; return; } } + function handleSetName(name: string) { + reviewerName = name; + setReviewerName(name); + } + async function handlePopoverSave(body: string, suggestedText?: string) { if (!activeSelection) return; await createComment({ @@ -282,7 +308,11 @@ suggested_text: suggestedText, created_at: new Date().toISOString() }; - await postEvent(event); + if (isAuthor && !isMyComment) { + await postAuthorEvent(event); + } else { + await postEvent(event); + } // Apply locally so the card updates without a round-trip refetch. const next = new Map(comments); @@ -309,15 +339,39 @@ author_name: reviewerName, created_at: new Date().toISOString() }; - await postEvent(event); + await postAuthorEvent(event); const next = new Map(comments); const target = next.get(commentId); - if (target && plan?.author_name === reviewerName) { + if (target && isAuthor) { next.set(commentId, { ...target, status, reply, replyAt: event.created_at }); } comments = next; } + // The original commenter retracting their own annotation. Goes through the + // regular postEvent (not postAuthorEvent) — retraction is a reviewer-side + // authority, not an author privilege, so a future Phase C server-enforced + // auth would NOT gate retraction behind the author token. + async function handleRetract(commentId: string) { + if (!reviewerName) return; + const target = comments.get(commentId); + if (!target) return; + // Replay also enforces this; failing fast here avoids posting events + // the replay would silently drop. + if (target.event.author_name !== reviewerName) return; + const event: RetractionEvent = { + kind: 'retraction', + id: `x-${crypto.randomUUID()}`, + comment_id: commentId, + author_name: reviewerName, + created_at: new Date().toISOString() + }; + await postEvent(event); + const next = new Map(comments); + next.set(commentId, { ...target, status: 'retracted' }); + comments = next; + } + function handleSelection(sel: SelectionInfo | null) { if (!sel) { if (!popoverMode && !showQuickLabel) activeSelection = null; @@ -363,19 +417,59 @@ {plan?.title ?? 'Untitled plan'} - {#if reviewerName} - - {reviewerName}{isAuthor ? ' · author' : ''} - - {:else} - - {/if} +
+ {#if isAuthor} + + {/if} + {#if reviewerName} + + {/if} +
{#if loadError} @@ -412,6 +506,7 @@ onCardClick={handleCardClick} onResolve={handleResolve} onEdit={handleEdit} + onRetract={handleRetract} /> @@ -426,6 +521,8 @@ anchorRect={activeSelection.rect} onAction={handleToolbarAction} onDismiss={clearSelection} + {reviewerName} + onSetName={handleSetName} /> {/if} @@ -448,6 +545,6 @@ {/if} {#if showNamePrompt} - + {/if} diff --git a/web/src/routes/share/[id]/components/AnnotationCard.svelte b/web/src/routes/share/[id]/components/AnnotationCard.svelte index d96ad1d..f4d9369 100644 --- a/web/src/routes/share/[id]/components/AnnotationCard.svelte +++ b/web/src/routes/share/[id]/components/AnnotationCard.svelte @@ -11,7 +11,8 @@ isActive = false, onClick, onResolve, - onEdit + onEdit, + onRetract }: { entry: CommentState; isAuthor: boolean; @@ -20,6 +21,7 @@ onClick: () => void; onResolve: (status: ResolutionStatus, reply?: string) => Promise; onEdit: (body: string, suggestedText: string | undefined) => Promise; + onRetract: () => Promise; } = $props(); const modKey = modifierGlyph(); @@ -101,6 +103,30 @@ (entry.status === 'open' || entry.status === 'reopened') ); + // Retraction is strictly the original commenter's authority — author + // keeps Reject (with reply) for unwanted feedback so rationale survives + // in the audit trail. Same status window as canEdit: only meaningful + // while the comment is still under discussion. + const canRetract = $derived( + reviewerName !== null && + e.author_name === reviewerName && + (entry.status === 'open' || entry.status === 'reopened') + ); + + let showRetractConfirm = $state(false); + let retracting = $state(false); + + async function confirmRetract() { + if (retracting) return; + retracting = true; + try { + await onRetract(); + } finally { + retracting = false; + showRetractConfirm = false; + } + } + async function startEdit() { editBody = e.body ?? ''; editSuggested = e.suggested_text ?? ''; @@ -302,7 +328,7 @@ - Reject: `--ink-delete` (editorial red — load-bearing no) - Resolve / Reopen: muted (closing actions, no decision weight) --> - {#if (canEdit || isAuthor) && !isEditing && !showRejectReply} + {#if (canEdit || canRetract || isAuthor) && !isEditing && !showRejectReply && !showRetractConfirm}
{#if canEdit} + {/if} + + {#if (canEdit || canRetract) && isAuthor}
{/if} + + {#if showRetractConfirm} + +
+

+ Take this annotation back? It disappears from the rail and is excluded from + arc share comments output. +

+
+ + +
+
+ {/if} diff --git a/web/src/routes/share/[id]/components/AnnotationsPanel.svelte b/web/src/routes/share/[id]/components/AnnotationsPanel.svelte index f31d282..9c170ea 100644 --- a/web/src/routes/share/[id]/components/AnnotationsPanel.svelte +++ b/web/src/routes/share/[id]/components/AnnotationsPanel.svelte @@ -9,7 +9,8 @@ activeId, onCardClick, onResolve, - onEdit + onEdit, + onRetract }: { states: CommentState[]; isAuthor: boolean; @@ -22,6 +23,7 @@ reply?: string ) => Promise; onEdit: (commentId: string, body: string, suggestedText: string | undefined) => Promise; + onRetract: (commentId: string) => Promise; } = $props(); const visibleStates = $derived(states); // could filter by status later @@ -72,6 +74,7 @@ onClick={() => onCardClick(entry.event.id)} onResolve={(status, reply) => onResolve(entry.event.id, status, reply)} onEdit={(body, suggestedText) => onEdit(entry.event.id, body, suggestedText)} + onRetract={() => onRetract(entry.event.id)} /> {/each} diff --git a/web/src/routes/share/[id]/components/FloatingToolbar.svelte b/web/src/routes/share/[id]/components/FloatingToolbar.svelte index f136502..696d3ad 100644 --- a/web/src/routes/share/[id]/components/FloatingToolbar.svelte +++ b/web/src/routes/share/[id]/components/FloatingToolbar.svelte @@ -7,13 +7,69 @@ const { anchorRect, onAction, - onDismiss + onDismiss, + reviewerName, + onSetName }: { anchorRect: DOMRect; onAction: (a: ToolbarAction) => void; onDismiss: () => void; + reviewerName: string | null; + onSetName: (name: string) => void; } = $props(); + // Inline name capture — only rendered when the reviewer has no name yet. + // Replaces the legacy modal flow where typing a comment first then being + // asked for a name led to people answering with their comment text. + // Here the name input lives in the toolbar itself, sibling to the action + // icons; actions are visibly inert until the field has content. + let nameDraft = $state(''); + let nameError = $state(false); + let nameInput: HTMLInputElement | undefined = $state(); + const needsName = $derived(!reviewerName); + const nameReady = $derived(nameDraft.trim().length > 0); + + function commitName() { + const trimmed = nameDraft.trim(); + if (!trimmed) { + flashRequired(); + return; + } + onSetName(trimmed); + } + + function flashRequired() { + nameError = true; + nameInput?.focus(); + setTimeout(() => { + nameError = false; + }, 700); + } + + function handleNameKey(e: KeyboardEvent) { + if (e.key === 'Enter') { + e.preventDefault(); + commitName(); + } + } + + function tryAction(a: ToolbarAction) { + if (needsName) { + // Persist what they've typed if it's substantive — they may have + // just typed a name and reached for an action. Otherwise flash. + if (nameReady) { + commitName(); + // Don't auto-perform: let them deliberately click again now + // that the toolbar has woken up. Avoids accidentally posting + // "Praise" the moment the name input clears. + return; + } + flashRequired(); + return; + } + onAction(a); + } + let toolbar: HTMLDivElement | undefined = $state(); // Measured after the toolbar mounts (its width is content-driven, so we // can't know it ahead of time). We seed with a conservative upper bound @@ -55,6 +111,9 @@ // width — typically ~200px depending on the icon set. await tick(); if (toolbar) measuredWidth = toolbar.offsetWidth; + // Focus the name field on first render so the user can start typing + // without an extra click. Skipped once a name is already on file. + if (needsName) nameInput?.focus(); }); onDestroy(() => { @@ -69,20 +128,75 @@ delete: 'text-[var(--ink-delete)] hover:bg-[var(--ink-delete-bg)]', muted: 'text-[var(--ink-text-muted)] hover:bg-[var(--ink-paper)]' }; + + // While the field is empty, action buttons are functionally locked. Their + // hover tooltip swaps from the action's normal label to a single directive + // — the gentle middle tier between "passively dimmed" and "shake on click". + function actionTitle(active: string): string { + return needsName ? 'Enter your name first' : active; + } diff --git a/web/src/routes/share/[id]/components/NamePromptModal.svelte b/web/src/routes/share/[id]/components/NamePromptModal.svelte index e922a18..8e4e1f5 100644 --- a/web/src/routes/share/[id]/components/NamePromptModal.svelte +++ b/web/src/routes/share/[id]/components/NamePromptModal.svelte @@ -2,7 +2,14 @@ import { onMount, tick } from 'svelte'; import { setReviewerName } from '$lib/paste/identity'; - const { onSave }: { onSave: (name: string) => void } = $props(); + const { + onSave, + initialName = '' + }: { onSave: (name: string) => void; initialName?: string } = $props(); + // Seed inside onMount so we don't snapshot the prop at the + // reactive-graph top level (svelte/state_referenced_locally). The + // modal is destroyed/recreated by `{#if showNamePrompt}`, so reading + // `initialName` once on mount captures the correct value each open. let name = $state(''); let input: HTMLInputElement | undefined = $state(); @@ -21,8 +28,10 @@ } onMount(async () => { + name = initialName; await tick(); input?.focus(); + input?.select(); }); diff --git a/web/src/routes/share/[id]/components/PlanRenderer.svelte b/web/src/routes/share/[id]/components/PlanRenderer.svelte index 97e5ca1..5c7206a 100644 --- a/web/src/routes/share/[id]/components/PlanRenderer.svelte +++ b/web/src/routes/share/[id]/components/PlanRenderer.svelte @@ -132,6 +132,20 @@ }); }); + // Resolve a Range endpoint (text node OR element) to its containing + // [data-source-line] block. Triple-click in Chromium frequently puts + // the range endpoints at element boundaries (e.g. startContainer =

, + // startOffset = 0) rather than inside a text node; in that case + // `node.parentElement.closest(...)` walks PAST the containing block to + //

, which has no data-source-line, and returns null — which + // would trip the dismiss path below. Treating the node itself as the + // closest() search root when it's already an element fixes that. + function resolveBlock(node: Node): HTMLElement | null { + const el = + node.nodeType === Node.ELEMENT_NODE ? (node as Element) : node.parentElement; + return (el?.closest('[data-source-line]') ?? null) as HTMLElement | null; + } + function handleMouseUp(e: MouseEvent) { const target = e.target as Element | null; const existingMark = target?.closest('mark[data-anno-id]') as HTMLElement | null; @@ -146,12 +160,8 @@ return; } const range = sel.getRangeAt(0); - const startEl = (range.startContainer.parentElement as Element | null)?.closest( - '[data-source-line]' - ) as HTMLElement | null; - const endEl = (range.endContainer.parentElement as Element | null)?.closest( - '[data-source-line]' - ) as HTMLElement | null; + const startEl = resolveBlock(range.startContainer); + const endEl = resolveBlock(range.endContainer); if (!startEl || !endEl) { onSelection?.(null); return; diff --git a/web/src/routes/share/[id]/components/inline-annotations.test.ts b/web/src/routes/share/[id]/components/inline-annotations.test.ts new file mode 100644 index 0000000..bd4700c --- /dev/null +++ b/web/src/routes/share/[id]/components/inline-annotations.test.ts @@ -0,0 +1,222 @@ +// @vitest-environment jsdom +import { describe, it, expect, beforeEach } from 'vitest'; +import { applyInlineAnnotations, type InlineMark } from './inline-annotations'; + +function el(tag: string, opts: { dataSourceLine?: number; text?: string } = {}): HTMLElement { + const e = document.createElement(tag); + if (opts.dataSourceLine !== undefined) { + e.setAttribute('data-source-line', String(opts.dataSourceLine)); + } + if (opts.text !== undefined) e.appendChild(document.createTextNode(opts.text)); + return e; +} + +function makeContainer(): HTMLElement { + return document.createElement('article'); +} + +function mark(opts: Partial & Pick): InlineMark { + return { + id: opts.id ?? 'm1', + kind: opts.kind ?? 'comment', + lineStart: opts.lineStart, + lineEnd: opts.lineEnd, + quotedText: opts.quotedText + }; +} + +describe('applyInlineAnnotations', () => { + let container: HTMLElement; + beforeEach(() => { + container = makeContainer(); + }); + + it('wraps a single contiguous selection inside one paragraph', () => { + const p = el('p', { dataSourceLine: 1, text: 'The quick brown fox' }); + container.appendChild(p); + + applyInlineAnnotations(container, [ + mark({ quotedText: 'quick brown', lineStart: 1, lineEnd: 1 }) + ]); + + const marks = container.querySelectorAll('mark.anno-comment'); + expect(marks.length).toBe(1); + expect(marks[0].textContent).toBe('quick brown'); + expect(marks[0].parentElement?.tagName).toBe('P'); + }); + + it('wraps text inside an inline element (e.g. ) without splitting structure', () => { + const p = el('p', { dataSourceLine: 1 }); + p.appendChild(document.createTextNode('use ')); + const code = el('code', { text: 'foo()' }); + p.appendChild(code); + p.appendChild(document.createTextNode(' here')); + container.appendChild(p); + + applyInlineAnnotations(container, [ + mark({ quotedText: 'use foo() here', lineStart: 1, lineEnd: 1 }) + ]); + + // The whole phrase should be wrapped — possibly as a single Range surround + // (one ) or as per-text-node wraps (three s). Either is OK + // as long as the visible covered text is exactly the needle. + const marks = Array.from(container.querySelectorAll('mark.anno-comment')); + expect(marks.length).toBeGreaterThanOrEqual(1); + const covered = marks.map((m) => m.textContent ?? '').join(''); + expect(covered).toBe('use foo() here'); + // Crucially, is still present and still contains its text. + expect(container.querySelector('code')?.textContent).toBe('foo()'); + }); + + it('wraps a multi-paragraph selection where the needle has \\n separators', () => { + const p1 = el('p', { dataSourceLine: 1, text: 'First paragraph.' }); + const p2 = el('p', { dataSourceLine: 3, text: 'Second paragraph.' }); + container.appendChild(p1); + container.appendChild(p2); + + applyInlineAnnotations(container, [ + mark({ quotedText: 'First paragraph.\nSecond paragraph.', lineStart: 1, lineEnd: 3 }) + ]); + + const marks = Array.from(container.querySelectorAll('mark.anno-comment')); + expect(marks.length).toBeGreaterThanOrEqual(2); + expect(marks.some((m) => m.textContent === 'First paragraph.')).toBe(true); + expect(marks.some((m) => m.textContent === 'Second paragraph.')).toBe(true); + // Paragraph elements remain intact. + expect(container.querySelectorAll('p').length).toBe(2); + }); + + it('wraps a heading + bulleted list selection (the regression case)', () => { + // Reproduces the "Non-goals" failure from the handoff:

followed by + // a
    with several
  • s. The TreeWalker yields text nodes with no + // whitespace between them, but selection.toString() inserts \n between + // the heading and each
  • — so the search needs synthetic block + // separators to match. + const h = el('h2', { dataSourceLine: 1, text: 'Non-goals' }); + const ul = el('ul', { dataSourceLine: 3 }); + const li1 = el('li', { text: 'Real authentication, OAuth, or accounts' }); + const li2 = el('li', { text: 'Server-side comment aggregation' }); + const li3 = el('li', { text: 'Live multi-user co-editing' }); + ul.appendChild(li1); + ul.appendChild(li2); + ul.appendChild(li3); + container.appendChild(h); + container.appendChild(ul); + + const needle = [ + 'Non-goals', + 'Real authentication, OAuth, or accounts', + 'Server-side comment aggregation', + 'Live multi-user co-editing' + ].join('\n'); + + applyInlineAnnotations(container, [mark({ quotedText: needle, lineStart: 1, lineEnd: 3 })]); + + const marks = Array.from(container.querySelectorAll('mark.anno-comment')); + // One mark per text node (heading + 3 list items). + expect(marks.length).toBe(4); + expect(marks.map((m) => m.textContent ?? '')).toEqual([ + 'Non-goals', + 'Real authentication, OAuth, or accounts', + 'Server-side comment aggregation', + 'Live multi-user co-editing' + ]); + // List structure is preserved — three
  • children of one
      . + expect(container.querySelectorAll('ul > li').length).toBe(3); + }); + + it('wraps a code-block selection containing literal newlines', () => { + // 
      ...
      — text contains real \n chars, not block + // boundaries. The needle from selection.toString() also has real \n. + const pre = el('pre', { dataSourceLine: 1 }); + const code = el('code'); + code.appendChild(document.createTextNode('line one\nline two\nline three')); + pre.appendChild(code); + container.appendChild(pre); + + applyInlineAnnotations(container, [ + mark({ quotedText: 'line one\nline two\nline three', lineStart: 1, lineEnd: 1 }) + ]); + + const marks = Array.from(container.querySelectorAll('mark.anno-comment')); + expect(marks.length).toBeGreaterThanOrEqual(1); + const covered = marks.map((m) => m.textContent ?? '').join(''); + expect(covered).toBe('line one\nline two\nline three'); + // 
       structure preserved.
+		expect(container.querySelector('pre > code')).toBeTruthy();
+	});
+
+	it('treats 
       as a line break (the markdown hard-break case)', () => {
+		// Markdown's two-trailing-spaces hard break renders as 
       inside a
+		// single 
    •  or 
      . Selection.toString() emits '\n' at the 
      ; the
+		// TreeWalker SHOW_TEXT path doesn't see it. Without explicit handling
+		// the search whiffs because needle has '\n' but searchSpace doesn't.
+		const ul = el('ul', { dataSourceLine: 1 });
+		const li = el('li');
+		li.appendChild(document.createTextNode('they remain as'));
+		li.appendChild(document.createElement('br'));
+		li.appendChild(document.createTextNode('legacy storage'));
+		ul.appendChild(li);
+		container.appendChild(ul);
+
+		applyInlineAnnotations(container, [
+			mark({ quotedText: 'they remain as\nlegacy storage', lineStart: 1, lineEnd: 1 })
+		]);
+
+		const marks = Array.from(container.querySelectorAll('mark.anno-comment'));
+		// Either a single Range wrap (1 mark covering text+
      +text) or per-
+		// text-node fallback (2 marks). Both are correct outcomes; what matters
+		// is that *something* wrapped, the structural 
       survived, and both
+		// halves of the text are inside a mark.
+		expect(marks.length).toBeGreaterThanOrEqual(1);
+		const covered = marks.map((m) => m.textContent ?? '').join('|');
+		expect(covered).toContain('they remain as');
+		expect(covered).toContain('legacy storage');
+		expect(container.querySelectorAll('br').length).toBe(1);
+	});
+
+	it('handles a partial selection inside a single 
    • ', () => {
+		const ul = el('ul', { dataSourceLine: 1 });
+		ul.appendChild(el('li', { text: 'one apple' }));
+		ul.appendChild(el('li', { text: 'two oranges' }));
+		container.appendChild(ul);
+
+		applyInlineAnnotations(container, [
+			mark({ quotedText: 'two oranges', lineStart: 1, lineEnd: 1 })
+		]);
+
+		const marks = Array.from(container.querySelectorAll('mark.anno-comment'));
+		expect(marks.length).toBe(1);
+		expect(marks[0].textContent).toBe('two oranges');
+		expect(container.querySelectorAll('ul > li').length).toBe(2);
+	});
+
+	it('clears prior marks on re-application', () => {
+		const p = el('p', { dataSourceLine: 1, text: 'Hello world' });
+		container.appendChild(p);
+
+		const m1: InlineMark = mark({ quotedText: 'Hello', lineStart: 1, lineEnd: 1, id: 'a' });
+		const m2: InlineMark = mark({ quotedText: 'world', lineStart: 1, lineEnd: 1, id: 'b' });
+
+		applyInlineAnnotations(container, [m1]);
+		expect(container.querySelectorAll('mark[data-anno-id="a"]').length).toBe(1);
+
+		applyInlineAnnotations(container, [m2]);
+		// First mark torn down, second applied.
+		expect(container.querySelectorAll('mark[data-anno-id="a"]').length).toBe(0);
+		expect(container.querySelectorAll('mark[data-anno-id="b"]').length).toBe(1);
+	});
+
+	it('returns silently when the anchor is missing', () => {
+		const p = el('p', { dataSourceLine: 1, text: 'Hello' });
+		container.appendChild(p);
+
+		applyInlineAnnotations(container, [
+			mark({ quotedText: 'goodbye', lineStart: 1, lineEnd: 1 })
+		]);
+
+		expect(container.querySelectorAll('mark.anno-comment').length).toBe(0);
+		// Original text untouched.
+		expect(p.textContent).toBe('Hello');
+	});
+});
diff --git a/web/src/routes/share/[id]/components/inline-annotations.ts b/web/src/routes/share/[id]/components/inline-annotations.ts
index 6a91ea5..8ca0302 100644
--- a/web/src/routes/share/[id]/components/inline-annotations.ts
+++ b/web/src/routes/share/[id]/components/inline-annotations.ts
@@ -1,13 +1,43 @@
 /**
  * Apply inline annotation marks to already-rendered markdown.
  *
- * Strategy: walk text nodes inside each block (data-source-line) referenced by
- * the anchor, find the first occurrence of the anchor's quoted_text, and wrap
- * it in a  with the right CSS class. This is similar to plannotator's
- * web-highlighter but tailored to our line-anchored model.
+ * The hard problem: selection.toString() inserts \n separators at block
+ * boundaries (between 
    • s, between 
      s, after a heading) and at 
      
+ * elements, but a TreeWalker over the same DOM yields just the text-node
+ * data with no separators. Naively searching the concatenated text-node
+ * string for the needle, or even a whitespace-normalized version of it,
+ * fails when the gap between blocks is zero whitespace — normalization can
+ * only collapse existing runs.
  *
- * The implementation rebuilds the marks every render rather than diffing; this
- * is fine for the volume we expect (tens of annotations per plan).
+ * Strategy: for each annotation, gather the [data-source-line] blocks in
+ * [lineStart, lineEnd], walk all their text nodes plus 
      /
       elements
+ * in document order, and build TWO parallel strings:
+ *
+ *   - `acc`: raw concatenation of text-node data. Cumulative offsets into
+ *     this string map directly into textNodes via length math — used by the
+ *     wrap step.
+ *   - `searchSpace`: same content but with a synthetic '\n' inserted at
+ *     every block-level boundary AND every 
      /
       element. This mirrors
+ *     what selection.toString() emits, so the needle can match.
+ *     `searchToAcc[i]` maps every searchSpace index to its acc index, with
+ *     -1 marking a synthetic boundary char.
+ *
+ * Search is two-tier on `searchSpace`: exact indexOf, then whitespace-
+ * normalized fallback for cases like extra trailing whitespace. The hit's
+ * range gets mapped back through `searchToAcc` (skipping synthetic chars)
+ * before reaching the wrap step.
+ *
+ * Wrapping is also two-tier:
+ *
+ *   1. Range surroundContents on a single Range from start text node to end
+ *      text node — clean for ranges within one inline-friendly element.
+ *   2. Per-text-node wrap when (1) fails because the range crosses sibling
+ *      block elements like 
    • s. surroundContents throws in that case;
+ *      falling back to extractContents would rip the list structure apart,
+ *      so we instead wrap the matched slice of each individual text node
+ *      (which is always inline-safe).
+ *
+ * The implementation rebuilds the marks every render rather than diffing.
  */
 
 export type InlineMark = {
@@ -32,9 +62,14 @@ export function applyInlineAnnotations(
 	clearMarks(container);
 
 	for (const mark of marks) {
-		const block = container.querySelector(`[data-source-line="${mark.lineStart}"]`);
-		if (!block) continue;
-		wrapFirstOccurrence(block, mark, mark.id === activeId);
+		const isActive = mark.id === activeId;
+		const blocks: HTMLElement[] = [];
+		for (let line = mark.lineStart; line <= mark.lineEnd; line++) {
+			const b = container.querySelector(`[data-source-line="${line}"]`);
+			if (b) blocks.push(b);
+		}
+		if (blocks.length === 0) continue;
+		wrapNeedleAcrossBlocks(blocks, mark.quotedText, mark, isActive);
 	}
 }
 
@@ -50,28 +85,180 @@ function clearMarks(container: HTMLElement): void {
 	}
 }
 
-function wrapFirstOccurrence(block: HTMLElement, mark: InlineMark, isActive: boolean): void {
-	const needle = mark.quotedText;
+function wrapNeedleAcrossBlocks(
+	blocks: HTMLElement[],
+	needle: string,
+	mark: InlineMark,
+	isActive: boolean
+): void {
 	if (!needle) return;
 
-	// Walk text nodes in document order; track running offset so we can find the
-	// first occurrence even when the needle spans multiple text nodes.
-	const walker = document.createTreeWalker(block, NodeFilter.SHOW_TEXT);
+	// Walk text nodes AND inline structural elements (
      /
      ) across all
+	// blocks in document order. We build:
+	//
+	//   - `acc`: raw text-node concatenation (textNode position math basis).
+	//   - `searchSpace`: acc with synthetic '\n' at:
+	//       * block-level boundaries (different closest-block ancestor), and
+	//       * 
      /
       elements (invisible to SHOW_TEXT but they produce a
+	//         '\n' in Selection.toString()).
+	//     Mirrors what selection.toString() emits so the needle can match.
 	const textNodes: Text[] = [];
 	let acc = '';
-	while (walker.nextNode()) {
-		const t = walker.currentNode as Text;
-		textNodes.push(t);
-		acc += t.data;
+	let searchSpace = '';
+	const searchToAcc: number[] = [];
+	let prevBlock: Element | null = null;
+	const filter: NodeFilter = {
+		acceptNode(node) {
+			if (node.nodeType === Node.TEXT_NODE) return NodeFilter.FILTER_ACCEPT;
+			if (
+				node.nodeType === Node.ELEMENT_NODE &&
+				LINE_BREAK_TAGS.has((node as Element).tagName)
+			) {
+				return NodeFilter.FILTER_ACCEPT;
+			}
+			return NodeFilter.FILTER_SKIP;
+		}
+	};
+	for (const block of blocks) {
+		const walker = document.createTreeWalker(
+			block,
+			NodeFilter.SHOW_TEXT | NodeFilter.SHOW_ELEMENT,
+			filter
+		);
+		while (walker.nextNode()) {
+			const node = walker.currentNode;
+			if (node.nodeType === Node.ELEMENT_NODE) {
+				if (acc.length > 0) {
+					searchSpace += '\n';
+					searchToAcc.push(-1);
+				}
+				continue;
+			}
+			const t = node as Text;
+			const curBlock = closestBlockAncestor(t);
+			if (prevBlock && curBlock !== prevBlock) {
+				searchSpace += '\n';
+				searchToAcc.push(-1);
+			}
+			prevBlock = curBlock;
+			textNodes.push(t);
+			const baseAcc = acc.length;
+			for (let i = 0; i < t.data.length; i++) {
+				searchSpace += t.data[i];
+				searchToAcc.push(baseAcc + i);
+			}
+			acc += t.data;
+		}
 	}
+	if (textNodes.length === 0) return;
 
-	const offset = acc.indexOf(needle);
-	if (offset < 0) return; // anchor lost; caller already showed a drift badge
+	// Two-tier search against searchSpace (which mirrors selection.toString()).
+	let searchStart = -1;
+	let searchEnd = -1;
+	const exactOffset = searchSpace.indexOf(needle);
+	if (exactOffset >= 0) {
+		searchStart = exactOffset;
+		searchEnd = exactOffset + needle.length;
+	} else {
+		const norm = normalizeWithMap(searchSpace);
+		const needleNorm = normalizeWS(needle);
+		if (!needleNorm) return;
+		const normOffset = norm.normalized.indexOf(needleNorm);
+		if (normOffset < 0) return;
+		searchStart = norm.rawPositions[normOffset];
+		const lastNormIdx = normOffset + needleNorm.length - 1;
+		if (lastNormIdx >= norm.rawPositions.length) return;
+		searchEnd = norm.rawPositions[lastNormIdx] + 1;
+	}
 
-	const start = offset;
-	const end = offset + needle.length;
+	// Map searchSpace [start, end) back to acc, skipping synthetic boundary
+	// chars. The first real char at-or-after searchStart anchors `start`;
+	// the last real char before searchEnd anchors `end`.
+	let start = -1;
+	for (let i = searchStart; i < searchToAcc.length; i++) {
+		if (searchToAcc[i] >= 0) {
+			start = searchToAcc[i];
+			break;
+		}
+	}
+	let end = -1;
+	for (let i = searchEnd - 1; i >= 0; i--) {
+		if (searchToAcc[i] >= 0) {
+			end = searchToAcc[i] + 1;
+			break;
+		}
+	}
+	if (start < 0 || end <= start) return;
+
+	// Wrap. Try the contiguous Range first; on failure (range crosses sibling
+	// block elements like 
    • s), fall back to per-text-node wrap.
+	if (!tryRangeWrap(textNodes, start, end, mark, isActive)) {
+		wrapPerTextNode(textNodes, start, end, mark, isActive);
+	}
+}
 
-	// Find the start text node + offset within it.
+// Tags that produce a literal '\n' in Selection.toString() despite having no
+// text-node children. We have to walk SHOW_ELEMENT to see them and translate
+// each into a synthetic separator.
+const LINE_BREAK_TAGS = new Set(['BR', 'HR']);
+
+// Tags treated as inline for the purpose of "did selection.toString() insert
+// a \n between these two text nodes?". Anything not in this set is treated
+// as a block boundary (paragraphs, list items, headings, code blocks, table
+// cells, etc.). Matches the HTML spec's "phrasing content" tags that the
+// markdown renderer can plausibly emit.
+const INLINE_TAGS = new Set([
+	'A',
+	'ABBR',
+	'B',
+	'BDI',
+	'BDO',
+	'BR',
+	'CITE',
+	'CODE',
+	'DATA',
+	'DEL',
+	'DFN',
+	'EM',
+	'I',
+	'INS',
+	'KBD',
+	'MARK',
+	'Q',
+	'S',
+	'SAMP',
+	'SMALL',
+	'SPAN',
+	'STRONG',
+	'SUB',
+	'SUP',
+	'TIME',
+	'U',
+	'VAR',
+	'WBR'
+]);
+
+function closestBlockAncestor(node: Node): Element | null {
+	let cur: Node | null = node.parentNode;
+	while (cur && cur.nodeType === 1) {
+		if (!INLINE_TAGS.has((cur as Element).tagName)) return cur as Element;
+		cur = cur.parentNode;
+	}
+	return null;
+}
+
+/**
+ * Attempts to wrap [start, end) of the concatenated text-node string in a
+ * single Range surroundContents. Works for ranges that don't cross sibling
+ * block elements. Returns false on any failure so the caller can fall back.
+ */
+function tryRangeWrap(
+	textNodes: Text[],
+	start: number,
+	end: number,
+	mark: InlineMark,
+	isActive: boolean
+): boolean {
 	let cum = 0;
 	let startNode: Text | null = null;
 	let startInner = 0;
@@ -90,30 +277,109 @@ function wrapFirstOccurrence(block: HTMLElement, mark: InlineMark, isActive: boo
 		}
 		cum = next;
 	}
-	if (!startNode || !endNode) return;
+	if (!startNode || !endNode) return false;
 
 	try {
 		const range = document.createRange();
 		range.setStart(startNode, startInner);
 		range.setEnd(endNode, endInner);
+		const wrapper = createMarkWrapper(mark, isActive);
+		range.surroundContents(wrapper);
+		return true;
+	} catch {
+		// surroundContents throws when the range crosses element boundaries
+		// it can't surround (e.g., from inside one 
    •  to inside another).
+		// We deliberately do NOT call extractContents here — that would rip
+		// the structure apart and put the list contents into one flat .
+		return false;
+	}
+}
 
-		const wrapper = document.createElement('mark');
-		wrapper.className = CLASS_BY_KIND[mark.kind] + (isActive ? ' is-active' : '');
-		wrapper.dataset.annoId = mark.id;
-
-		// `surroundContents` works for ranges that don't cross element boundaries.
-		// For multi-element ranges we extract+wrap+reinsert which works for inline
-		// content (text + simple inline elements like , ).
+/**
+ * Wraps the [start, end) slice of each text node that overlaps that range.
+ * Each text-node range is its own atom: surroundContents on a single text
+ * node is always safe regardless of the surrounding element structure, so
+ * this preserves 
    • /
      / boundaries when the contiguous Range path
+ * couldn't wrap across them.
+ */
+function wrapPerTextNode(
+	textNodes: Text[],
+	start: number,
+	end: number,
+	mark: InlineMark,
+	isActive: boolean
+): void {
+	let cum = 0;
+	for (const t of textNodes) {
+		const tStart = cum;
+		const tEnd = cum + t.data.length;
+		cum = tEnd;
+		const overlapStart = Math.max(start, tStart);
+		const overlapEnd = Math.min(end, tEnd);
+		if (overlapStart >= overlapEnd) continue;
+		const localStart = overlapStart - tStart;
+		const localEnd = overlapEnd - tStart;
+		// Skip slices that are pure whitespace (e.g., a "\n" between code-block
+		// lines that contributed only to the separator). They shouldn't get
+		// their own visible .
+		if (!t.data.slice(localStart, localEnd).trim()) continue;
+		if (!t.parentNode) continue;
 		try {
+			const range = document.createRange();
+			range.setStart(t, localStart);
+			range.setEnd(t, localEnd);
+			const wrapper = createMarkWrapper(mark, isActive);
 			range.surroundContents(wrapper);
 		} catch {
-			const frag = range.extractContents();
-			wrapper.appendChild(frag);
-			range.insertNode(wrapper);
+			// A text-node range shouldn't fail surroundContents under normal DOM,
+			// but if it does, skip rather than throw.
 		}
-	} catch {
-		// Range manipulation can fail on edge cases (e.g., needle spans across
-		// block elements). Skip silently — the right-rail card still shows the
-		// annotation so the reviewer's intent isn't lost.
 	}
 }
+
+function createMarkWrapper(mark: InlineMark, isActive: boolean): HTMLElement {
+	const wrapper = document.createElement('mark');
+	wrapper.className = CLASS_BY_KIND[mark.kind] + (isActive ? ' is-active' : '');
+	wrapper.dataset.annoId = mark.id;
+	return wrapper;
+}
+
+/**
+ * Collapses runs of whitespace in `s` to single spaces (trimming ends),
+ * returning the normalized string and a map from each normalized index to
+ * its corresponding index in the original string. Used to bridge the gap
+ * between needle (with \n separators from selection.toString()) and the
+ * flat text walked via TreeWalker (no separators).
+ */
+function normalizeWithMap(s: string): { normalized: string; rawPositions: number[] } {
+	let normalized = '';
+	const rawPositions: number[] = [];
+	let prevWS = true; // skip leading whitespace
+	for (let i = 0; i < s.length; i++) {
+		const ch = s[i];
+		if (isWS(ch)) {
+			if (!prevWS) {
+				normalized += ' ';
+				rawPositions.push(i);
+				prevWS = true;
+			}
+		} else {
+			normalized += ch;
+			rawPositions.push(i);
+			prevWS = false;
+		}
+	}
+	if (normalized.endsWith(' ')) {
+		normalized = normalized.slice(0, -1);
+		rawPositions.pop();
+	}
+	return { normalized, rawPositions };
+}
+
+function normalizeWS(s: string): string {
+	return s.replace(/\s+/g, ' ').trim();
+}
+
+function isWS(ch: string): boolean {
+	return ch === ' ' || ch === '\t' || ch === '\n' || ch === '\r' || ch === '\f' || ch === '\v';
+}
diff --git a/web/vite.config.ts b/web/vite.config.ts
index 0abd395..3ddaa7e 100644
--- a/web/vite.config.ts
+++ b/web/vite.config.ts
@@ -6,8 +6,10 @@ export default defineConfig({
 	plugins: [tailwindcss(), sveltekit()],
 	server: {
 		proxy: {
+			// Backend target overridable via ARC_PASTE_BACKEND so the dev server
+			// can drive a non-default port (e.g. arc-paste running on :7436).
 			'/api': {
-				target: 'http://localhost:7432',
+				target: process.env.ARC_PASTE_BACKEND ?? 'http://localhost:7432',
 				changeOrigin: true
 			}
 		}