From c2206465f04e5b72d87808e0205f8d37615ee83c Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Wed, 10 Jun 2026 15:42:21 -0500 Subject: [PATCH 1/7] Adaptive review rail: slim resolved-only gutter + expandable resolved chips (attn-d7y) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review right rail reserved a fixed 320px whenever the panel was open, leaving a nearly vacant column when the margin held only resolved threads — a single full-width dashed strip whose click just moved the cursor and never showed the resolved content. - New pure rail-mode derivation (closed/slim/full) in review/rail-mode.ts, exposed as reviewStore.railMode: resolved-only margins slim the aside to a 48px gutter of icon chips; the document reclaims the width. - Resolved strips become content-sized chips ("✓ author · resolved" in the full rail, icon-only in the gutter). Clicking a chip expands it in place to the full read-only ReviewMarginCard (resolved badge, quote, body, replies) with a Collapse button; Escape collapses too. - Unified the active-card and resolved-chip collision layout into one layoutCards pass, fixing the pre-existing overlap between the two independent passes and letting the expanded resolved card push its neighbors. - locallyDismissed moved from ReviewMargin into the store so the rail slims the same tick the last active card is resolved (UI-only Reject now survives a panel remount for the session — intended). - Resolved threads always get an anchor-Y (fallback 0) so the slim gutter never renders empty when positions can't resolve. - Tests: rail-mode.test.ts (new), margin-integration cases for the unified pass/chip pitch/visibility rule, and a hard E2E suite in test-review-e2e.sh (slim → expand → collapse → mixed) seeded through window.__attn_review_store__. Design doc §3 amended. Co-Authored-By: Claude Fable 5 --- planning/collab/ui/review-panel-design.md | 46 ++- scripts/test-review-e2e.sh | 145 +++++++- web/src/App.svelte | 6 +- web/src/lib/ReviewMargin.svelte | 340 ++++++++++++------ web/src/lib/ReviewMarginCard.svelte | 26 +- web/src/lib/review/margin-integration.test.ts | 60 +++- web/src/lib/review/rail-mode.test.ts | 103 ++++++ web/src/lib/review/rail-mode.ts | 42 +++ web/src/lib/review/store.svelte.ts | 78 ++++ 9 files changed, 716 insertions(+), 130 deletions(-) create mode 100644 web/src/lib/review/rail-mode.test.ts create mode 100644 web/src/lib/review/rail-mode.ts diff --git a/planning/collab/ui/review-panel-design.md b/planning/collab/ui/review-panel-design.md index 7795a5f..61c418c 100644 --- a/planning/collab/ui/review-panel-design.md +++ b/planning/collab/ui/review-panel-design.md @@ -233,31 +233,47 @@ spatial-align. ## 3. Resolved State -Resolved threads do **not** disappear. They shrink to a **single-line grey -strip** in place (same y-position as the active card would have had): +> **Amended by attn-d7y** (shipped): the full-width strip became a +> content-sized **chip**, the rail width adapts, and clicking a chip +> expands a read-only card. The original strip spec is superseded by the +> behavior below; the >5 "show all resolved" pill survives unchanged. + +Resolved threads do **not** disappear. They shrink to a **28px rounded +chip** in place (same y-position as the active card would have had): ```text -│ ─── ✓ rufus · resolved 2m · §Anchor resolver ─── │ +│ (✓ rufus · resolved) │ ← content-sized pill, full rail ``` -The strip is 24px tall, uses the `--muted-foreground` color, and is dismissed -from the document-order layout queue (it can be skipped over for collision -purposes — newer active cards collapse past it). - -If there are more than 5 collapsed strips visible in the current viewport, -the design collapses them further to a "show all resolved" pill at the -**bottom of the margin** (still inside the overlay, after the last anchored -card): +When the margin holds ONLY resolved threads (no active cards, no orphan +tray, nothing expanded) the right-rail aside slims from 320px to a **48px +gutter** and the chips render icon-only — `(✓)` centered in the gutter — +so the document reclaims the width instead of facing a vacant column. The +mode derivation lives in `web/src/lib/review/rail-mode.ts` +(`closed`/`slim`/`full`), exposed as `reviewStore.railMode`, and drives +both the aside width (App.svelte) and the chip variant (ReviewMargin). + +Clicking a chip expands it **in place to the full card** in read-only +resolved state (author, quote, body, replies, `resolved` badge) with a +single `Collapse` action — no Reply/Resolve/Accept/Reject. Expanding +forces the rail to full width; Collapse (or Escape) shrinks it back. The +expanded card participates in the SAME collision pass as active cards +(one unified `layoutCards` call), so it pushes neighbors instead of +overlapping them. + +If there are more than 5 collapsed chips visible in full mode, they +collapse further to a "show all resolved" pill at the **bottom of the +margin** (still inside the overlay, after the last anchored card): ```text │ ─────────────────────────────────── │ -│ ⌃ 12 resolved · show │ ← bottom pill, click to expand all strips +│ ⌃ 12 resolved · show │ ← bottom pill, click to expand all chips │ ─────────────────────────────────── │ ``` -Clicking the pill expands all strips inline at their anchor positions. -Clicking a strip pops it back to a full card for one cycle (until next focus -change). +Clicking the pill expands all chips inline at their anchor positions. +Slim mode ignores the pill (the 48px gutter can't fit it; icon chips are +cheap to render). --- diff --git a/scripts/test-review-e2e.sh b/scripts/test-review-e2e.sh index 28c32b4..ddba56d 100755 --- a/scripts/test-review-e2e.sh +++ b/scripts/test-review-e2e.sh @@ -232,11 +232,12 @@ for cb in reviewStatus reviewEvent reviewSnapshot reviewAnchorResolution; do done echo "" -echo "--- Review store scaffold (from 12.10, pending) ---" -# The review store module should be importable and expose a default shape. -# Today the module does not exist — record as PEND. +echo "--- Review store scaffold ---" +# The review store singleton is exposed for E2E seeding (App.svelte wires +# `window.__attn_review_store__` on mount). Hard assertion — the adaptive +# rail suite below depends on it. result=$("$ATTN" --eval "typeof window.__attn_review_store__") -expect_eq_soft "window.__attn_review_store__ exposed" "$result" '"object"' "attn-nnj.12.10" +assert_eq "window.__attn_review_store__ exposed" "$result" '"object"' screenshot "02-shape-asserted" @@ -321,6 +322,142 @@ echo " (run scripts/test-apply-e2e.sh to verify the contract directly)" screenshot "03-apply-flow-pending" +# =================================================================== +# TEST SUITE: Adaptive rail + expandable resolved chips (attn-d7y) +# =================================================================== +# +# Seeds the review store directly through `window.__attn_review_store__` +# (no relay needed): one comment thread + its CommentResolved event. With +# only resolved threads in the margin the rail must slim to the 48px icon +# gutter; clicking the chip expands the rail to 320px with the full +# read-only card; Collapse returns to slim; adding an unresolved comment +# forces full mode with a labeled chip. + +echo "" +echo "=== Review E2E: adaptive rail + resolved chips (attn-d7y) ===" + +# Poll an --eval expression until it returns the expected value (the aside +# width animates over 200ms, so one-shot reads race the transition). +poll_eval() { + local expr="$1" expected="$2" attempts=30 + local result="" + while [ $attempts -gt 0 ]; do + result=$("$ATTN" --eval "$expr" 2>/dev/null || echo "") + if [ "$result" = "$expected" ]; then + echo "$result" + return 0 + fi + sleep 0.1 + attempts=$((attempts - 1)) + done + echo "$result" +} + +"$ATTN" --wait-for '.ProseMirror' --timeout 10000 >/dev/null 2>&1 || true + +seed_js=$(cat <<'EOF' +(() => { + const s = window.__attn_review_store__; + if (!s) return 'no-store'; + const anchor = { + v: 2, fileId: 'file-x', snapshotId: 'snap-x', baseHash: 'h-x', + position: { byteRange: [0, 10], lineRange: [1, 1] }, + }; + const meta = (id) => ({ + v: 2, eventId: id, roomId: 'room-x', authorId: 'p-reviewer', + deviceId: 'd-x', createdAt: Date.now(), parentEventIds: [], + snapshotId: 'snap-x', + }); + const auth = { signature: 'sig', signingKeyId: 'kid' }; + s.applyEvent({ meta: meta('e-1'), body: { type: 'comment_created', threadId: 't-1', anchor, body: 'Consider tightening this wording.' }, auth }); + s.applyEvent({ meta: meta('e-2'), body: { type: 'comment_resolved', threadId: 't-1', resolvedBy: 'p-owner' }, auth }); + s.selectRoom('room-x'); + s.setCurrentFile('file-x'); + s.panelOpen = true; + return 'ok'; +})() +EOF +) +result=$("$ATTN" --eval "$seed_js") +assert_eq "Seeded resolved-only review thread" "$result" '"ok"' + +echo "" +echo "--- Slim gutter (resolved-only margin) ---" +result=$(poll_eval "document.querySelector('[data-slot=\\\"right-rail\\\"]')?.getAttribute('data-mode') ?? 'missing'" '"slim"') +assert_eq "Rail data-mode is slim" "$result" '"slim"' + +result=$(poll_eval "document.querySelector('[data-slot=\\\"right-rail\\\"]')?.offsetWidth <= 60" 'true') +assert_eq "Rail width collapsed to the gutter (≤60px)" "$result" "true" + +result=$("$ATTN" --query '[data-testid="review-margin-resolved-chip"]' | jq -r '.count' 2>/dev/null || echo "0") +assert_eq "One resolved chip rendered" "$result" "1" + +result=$("$ATTN" --eval "document.querySelector('[data-testid=\\\"review-margin-resolved-chip\\\"]')?.getAttribute('data-variant') ?? 'missing'") +assert_eq "Chip is icon variant in slim mode" "$result" '"icon"' + +screenshot "04-resolved-slim-gutter" + +echo "" +echo "--- Expand chip → full read-only card ---" +"$ATTN" --click '[data-testid="review-margin-resolved-chip"]' >/dev/null 2>&1 || true + +result=$(poll_eval "document.querySelector('[data-slot=\\\"right-rail\\\"]')?.getAttribute('data-mode') ?? 'missing'" '"full"') +assert_eq "Rail expands to full on chip click" "$result" '"full"' + +result=$(poll_eval "document.querySelector('[data-testid=\\\"review-margin-card\\\"][data-state=\\\"resolved\\\"]') !== null" 'true') +assert_eq "Resolved card rendered" "$result" "true" + +result=$("$ATTN" --eval "(() => { const c = document.querySelector('[data-testid=\\\"review-margin-card\\\"][data-state=\\\"resolved\\\"]'); if (!c) return 'no-card'; return [c.querySelector('[data-action=\\\"resolve\\\"]') === null, c.querySelector('[data-action=\\\"reply\\\"]') === null, c.querySelector('[data-testid=\\\"review-margin-card-collapse\\\"]') !== null].join(','); })()") +assert_eq "Card is read-only (no resolve/reply; collapse present)" "$result" '"true,true,true"' + +result=$("$ATTN" --eval "document.querySelector('[data-testid=\\\"review-margin-card\\\"][data-state=\\\"resolved\\\"]')?.textContent.includes('Consider tightening this wording.')") +assert_eq "Card shows the resolved comment body" "$result" "true" + +screenshot "05-resolved-expanded-card" + +echo "" +echo "--- Collapse → back to slim ---" +"$ATTN" --click '[data-testid="review-margin-card-collapse"]' >/dev/null 2>&1 || true + +result=$(poll_eval "document.querySelector('[data-slot=\\\"right-rail\\\"]')?.getAttribute('data-mode') ?? 'missing'" '"slim"') +assert_eq "Rail returns to slim after collapse" "$result" '"slim"' + +result=$(poll_eval "document.querySelector('[data-testid=\\\"review-margin-card\\\"][data-state=\\\"resolved\\\"]') === null" 'true') +assert_eq "Resolved card gone after collapse" "$result" "true" + +echo "" +echo "--- Mixed margin (active + resolved) → full rail, labeled chip ---" +mixed_js=$(cat <<'EOF' +(() => { + const s = window.__attn_review_store__; + if (!s) return 'no-store'; + const anchor = { + v: 2, fileId: 'file-x', snapshotId: 'snap-x', baseHash: 'h-x', + position: { byteRange: [20, 30], lineRange: [3, 3] }, + }; + s.applyEvent({ + meta: { v: 2, eventId: 'e-3', roomId: 'room-x', authorId: 'p-reviewer', deviceId: 'd-x', createdAt: Date.now(), parentEventIds: [], snapshotId: 'snap-x' }, + body: { type: 'comment_created', threadId: 't-2', anchor, body: 'An open question.' }, + auth: { signature: 'sig', signingKeyId: 'kid' }, + }); + return 'ok'; +})() +EOF +) +result=$("$ATTN" --eval "$mixed_js") +assert_eq "Seeded an additional unresolved thread" "$result" '"ok"' + +result=$(poll_eval "document.querySelector('[data-slot=\\\"right-rail\\\"]')?.getAttribute('data-mode') ?? 'missing'" '"full"') +assert_eq "Rail is full with an active thread present" "$result" '"full"' + +result=$(poll_eval "document.querySelector('[data-testid=\\\"review-margin-resolved-chip\\\"]')?.getAttribute('data-variant') ?? 'missing'" '"label"') +assert_eq "Chip flips to label variant in full mode" "$result" '"label"' + +result=$(poll_eval "document.querySelector('[data-testid=\\\"review-margin-card\\\"][data-state=\\\"open\\\"]') !== null" 'true') +assert_eq "Active card rendered alongside the chip" "$result" "true" + +screenshot "06-mixed-full-rail" + # =================================================================== # Summary # =================================================================== diff --git a/web/src/App.svelte b/web/src/App.svelte index 567ea68..7490ba1 100644 --- a/web/src/App.svelte +++ b/web/src/App.svelte @@ -87,6 +87,7 @@ loadMarkdownFromPath, markdownSourceUrl, } from './lib/markdown-layer'; + import { RAIL_WIDTH_PX } from './lib/review/rail-mode'; import { reviewStore } from './lib/review/store.svelte'; import ReviewMargin from './lib/ReviewMargin.svelte'; import ReviewFileNav from './lib/ReviewFileNav.svelte'; @@ -2349,7 +2350,7 @@ avoidWindowControls={!hasSidebar} fixed={!hasSidebar} topOffsetPx={34} - rightInsetPx={showReviewChrome && !reviewStore.panelOpen ? 328 : 16} + rightInsetPx={showReviewChrome && reviewStore.railMode !== 'full' ? 328 : 16} onNavigate={(dir) => openPath(dir, inferFileTypeFromTree(dir))} onShare={showBreadcrumbShare ? openShareDialog : undefined} shareEnabled={showBreadcrumbShare} @@ -2583,8 +2584,9 @@