From d3f6307bede835be09d4695efe636c8f618ef7b0 Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 12:10:44 -0400 Subject: [PATCH 01/48] =?UTF-8?q?spec:=20review=20walkthrough=20two-column?= =?UTF-8?q?=20stops=20=E2=80=94=20diagrams=20lifted=20to=20a=20full-width?= =?UTF-8?q?=20row,=20code=20column=20beside=20the=20text,=20reviewed.json?= =?UTF-8?q?=20marks,=20collapsed=20comment=20box,=20scoped=20type=20and=20?= =?UTF-8?q?color=20(THI-443)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) --- ...026-10-29-walkthrough-two-column-design.md | 358 ++++++++++++++++++ 1 file changed, 358 insertions(+) create mode 100644 docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md diff --git a/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md b/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md new file mode 100644 index 00000000..ee05bcc5 --- /dev/null +++ b/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md @@ -0,0 +1,358 @@ +# Review walkthrough: two-column stops, legible diagrams, reviewed marks (THI-443) + +The individual walkthrough view (`static/src/review/Walkthrough.jsx`) gets a +new layout and a type and color pass. Each stop becomes a card with its +diagrams across the full width, its text on the left at a readable line +length, and its files and code changes in a column on the right. The page +gains a fixed header, a contents list, per-stop "reviewed" marks saved in +the review folder, and a quieter comment box. Builds on +`2026-10-26-review-walkthrough-design.md`. D1 (periscope never mutates git +or GitHub), D2 (review state is files under `~/.claude/reviews//`, +one writer per file) and D3 (the walkthrough.md format) are unchanged; this +replaces D5's layout. + +## Problem + +Measured in the running app on a 13-stop walkthrough (2026-10-29; the full +list is THI-443's 22 findings): + +- **Text is a narrow centered column.** `.md-doc` (shared with file previews) + sets `max-width: 72ch; margin: 0 auto`, and `.wt-stop-body` raises it to + 80ch. The body is 672px wide inside a 2085px card, centered, while the + title, refs, hunks and comment box are full-width and left-aligned. The + problem statement is a third column: 72ch, left-aligned, 13px. +- **Diagrams are unreadable.** `.md-mermaid-svg svg { max-width: 100% }` + fits each diagram to that 672px column. The widest is naturally 2274px, so + it draws at 30% scale with labels around 5px. It never grows and can't be + opened larger. They also use mermaid's stock dark theme (Trebuchet MS, + near-black nodes), not periscope's fonts or colors. +- **Code changes are hard to reach.** A wall of full paths in small mono + comes before the text, a small "▸ 3 hunks" toggle sits below it, and + expanding it lists every cited file, most reading "— not in this diff". + Clicking a path opens all hunks rather than going to that file's. +- **No way through a long walkthrough.** No contents list or stop numbers, + the header scrolls away, and nothing records which stops you've read. +- **Typography and color.** Eleven font sizes (10–16px; stop titles are + smaller than body text), four kinds of text below 4.5:1 contrast, the + staleness warning in the same grey as the repo name, stop kinds visible + only in a 10px badge, inline code brighter than the prose around it, and + cards with no fill. The problem statement drops its markdown. +- **Comment boxes everywhere.** Every stop has an open textarea and a + disabled button drawn in the browser default (1.4:1). ⌘↩ posts, unannounced. + +## Decisions + +### D1. Diagrams are lifted out of a stop's text by the parser + +`parseWalkthrough` already slices each stop out of the source by mdast +positions. Each stop now also returns `diagrams`: the code of every +**top-level** ```` ```mermaid ```` fence in its body, in order. Its +`bodyMarkdown` is the body with exactly those fences cut out (by their +`position` offsets). A mermaid fence nested inside a list or blockquote is +not lifted. It stays in the text and renders inline as today, because +moving it out of its list item can't be done without reordering content. + +Rejected: moving diagrams with CSS (turning the text container into grid +items is fragile with an unknown number of paragraphs), and a "collect" mode +in the shared renderer (couples `renderMarkdown` to one page's layout). + +The problem statement comes back as `problemMarkdown` (its source slice) in +addition to the existing plain-text `problem`, and renders as markdown. + +### D2. Page structure: a fixed header over one scrolling body + +`.walkthrough` becomes a flex column: + +- **Header** (`.wt-head`, doesn't scroll): the title at 22px, then + repo · branch · PR link at 12px. Beside them: + - a **staleness badge**: "stale: N commits" in `--s-needs` when + `stale_by > 0`; a neutral "no worktree" badge when the walkthrough has a + header but no diff; nothing when current; + - **progress**: "4 / 13 reviewed" (D7). +- **Scrolling body** (`.wt-scroll`, `overflow: auto`, + `container-type: size; container-name: wt`). Everything below lives here. + +The top of the body uses the same two-column grid as a stop: the problem +statement on the left (15px, the prose measure), a numbered **contents list** +on the right. Each entry is the stop number, a dot in its kind's color, the +title, and ✓ once reviewed. Clicking an entry calls `scrollIntoView` on the +stop (buttons, not `#` links, so the URL and periscope's routing are +untouched). + +### D3. A stop is a card: title row, diagram row, then two columns + +``` +┌──────────────────────────────────────────────────────────────┐ +│▌ 4. Adds the Project resource… [NEW-SUBSYSTEM] [reviewed] │ title row +│▌ ┌──────────────────── diagram, full width ────────────────┐ │ diagram row +│▌ └──────────────────────────────────────────────────────────┘ │ (if any) +│▌ prose (≤ 72ch) │ files: payload.ts 2 changes │ +│▌ … │ compiler.ts 1 change │ +│▌ comments │ 8 not in this diff │ +│▌ comment │ ─ hunks, expanded ─ │ +└──────────────────────────────────────────────────────────────┘ +``` + +- **Card:** `--bg-1` fill and a 3px left edge in the stop kind's accent (D9). +- **Title row:** "N." plus the title at 18px, the kind badge, and a + "reviewed" toggle at the right edge. +- **Diagram row:** present only when `stop.diagrams` is non-empty (D4). +- **Two columns** (`.wt-cols`): `grid-template-columns: minmax(0, 72ch) + minmax(0, 1fr)` with the grid's font size at 15px so `ch` tracks body + text. Left: the prose, then the stop's comments and the comment control + (D8). Right: the code-changes column (D5). +- **Narrow:** `@container wt (width < 1000px)` switches every `.wt-cols` + grid to one column, with the prose first and the code changes after. The + page never scrolls sideways; long code lines scroll inside their hunk. The + 1000px breakpoint is a starting value to tune in the browser. +- **Reviewed stops fold** to the title row, as "viewed" folds a file in the + Changes tab. On a reviewed stop the title itself is a button. Pressing it + unfolds or refolds the stop for this visit without unmarking it; the + "reviewed" toggle beside it is a separate button and only marks or + unmarks. The unfolded state is local and not saved. + +### D4. Diagrams render near natural size, in periscope's theme, and open full size + +- **Size.** In the diagram row the SVG is `width: 100%` capped by mermaid's + own inline `max-width` (its natural width), so it fills the card but never + scales a small diagram up. `max-height: 70vh` keeps a tall top-down chart + from pushing the stop's text off screen. Several diagrams stack in order. +- **Full size on click.** With the new `zoom: true` renderer option, a + diagram (and an image in the text) is focusable (`tabindex=0`, + `role="button"`, `aria-label="open diagram full size"`) and opens on click + or Enter in a lightbox. The lightbox is a dark backdrop with the content at + natural size, scrolling both ways when larger than the window, closed by + Esc (`useEscape`), a backdrop click or a ✕ button. For a diagram it copies + the rendered SVG markup (still only mermaid's own sanitized output, per + `Mermaid.jsx`'s innerHTML rule) and sets its width to the viewBox width. + Only the walkthrough passes `zoom: true`; file previews keep their existing + in-place image zoom. +- **Theme (app-wide).** `Mermaid.jsx` initializes with `theme: "base"` and + `themeVariables` from a pure `mermaidTheme(read)` builder, where + `read(name)` returns a token's color as `#rrggbb`. The intent is: + + | Element | Token | + |---|---| + | node fill | `--bg-2` | + | node text | `--fg-0` | + | node border, arrows | `--fg-2` (non-text contrast ≥ 3:1 on `--bg-1`) | + | edge-label background | `--bg-1` | + | groups / clusters | `--bg-1-hi` | + | other text | `--fg-1` | + | font | `--sans` (Hanken Grotesk), 14px | + + `darkMode: true`. Mermaid's color parser can't read `oklch()`, so tokens + are converted to hex at load by painting them on a 1×1 canvas and reading + the pixel. The stylesheet stays the single source of truth, and only opaque + tokens are used. This restyles diagrams in file previews too. +- Render failures are unchanged: the source and the error show as text. + +### D5. The code-changes column + +A pure `evidenceFor(refs, diffFiles)` (built on `matchRefs`) returns: + +- `shown`: refs with at least one matched hunk, each with its basename, any + line range, the hunk count and the hunks; +- `hidden`: refs grouped as `missing` (no such file in the diff), + `generated` (the server dropped its hunks) and `unchanged` (the file is in + the diff but no hunk meets the cited lines); +- with `diffFiles === null` (no worktree), every ref goes to `shown` with no + hunks, and the column shows "no worktree, so code changes are unavailable". + +The column (`.wt-evidence`) renders: + +- **File list:** one row per `shown` ref with the file name in mono, the line + range, and "N changes", full path in `title`. Clicking a row scrolls the + column to that file's block (`col.scrollTo`, not `scrollIntoView`, so the + page itself doesn't jump) and flashes it. A cite chip in the prose does the + same. Chips with no match stay inert, as today. +- **One closing summary line** for `hidden`, e.g. "8 not in this diff · 1 + generated", a `
` that expands to the paths. +- **The hunks,** expanded, under each file's path, drawn by `Hunk` from + `diff/ChangesTab.jsx` with `onOpen={null}`. +- **Wide panes:** `position: sticky; top: 12px; align-self: start; + max-height: calc(100cqh - 24px); overflow: auto`. The column stays beside + the text while the stop is on screen, scrolls on its own when taller, and + leaves with the stop. **Narrow:** static, no max-height. +- A stop with no refs leaves the column empty. + +The old `md-meta` refs row above the prose and the `wt-evidence-toggle` go +away. + +### D6. Reviewed marks live in `reviewed.json` in the review folder + +- **File:** `~/.claude/reviews///reviewed.json`, a JSON object + of `{"": ""}`. periscope is its only writer (D2 of the + original spec). "clear" removes it with the folder. +- **Signature:** `stopSig(stop)`, an FNV-1a 32-bit hash (8 hex chars) of the + stop's full source slice: heading, metadata and body before diagrams are + lifted. Any rewrite of the stop's title, kind, refs or text yields a new + sig. Computed in the browser so it always matches what is displayed. +- **Expiry:** a stop is reviewed only while `reviewed[title] === stopSig(stop)`. + A re-prepare that rewrites a stop lapses its mark, mirroring the Changes + tab's `viewedSig`. Stale entries stay in the file harmlessly until the + stop is marked or unmarked again. +- **Keyed by title**, like comments. Two stops with the same title share one + slot, and since their sigs differ only one can show reviewed. Accepted, + since the skill keeps titles distinct. +- **Server** (`periscope/reviews.py`): + - `reviewed_path(repo, branch)`; + - `read_reviewed(repo, branch) -> dict[str, str]`, where missing, + unreadable or non-object JSON reads as `{}` and non-string values are + dropped; + - `set_reviewed(repo, branch, stop, sig | None)`, a read-modify-write under + a module-level `threading.Lock`, written atomically (temp file in the + same dir, then `os.replace`). `None` removes the entry, and an empty + result deletes the file. +- **Routes** (`periscope/routes/reviews.py`): + - `POST /api/reviews/reviewed` with `{repo, branch, stop, sig, reviewed}`: + `_check(repo, branch)`; 400 for an empty stop or one with a newline (the + comment route's rule), or a sig not matching `^[0-9a-f]{8}$`; 404 when + the walkthrough file doesn't exist (so a mark never creates a folder); + otherwise `{"ok": true}`. + - `GET /api/reviews/walkthrough` gains `"reviewed": read_reviewed(...)`. +- **Frontend:** the toggle updates a local copy at once and POSTs. On failure + it reverts and shows the error beside the toggle. + +### D7. Progress and the contents list read the same marks + +The header's "N / M reviewed" and the contents list's ✓ use the same +`isReviewed(stop, reviewed)` as the cards, so the three can't disagree. + +### D8. The comment control is collapsed until used + +The stop's comments still list under its prose. The always-open textarea +becomes a "comment" text button. It opens the box, focused, with "send" and +"cancel" and a hint "⌘↩ to send" ("Ctrl+↩ to send" when the platform isn't +a Mac). A successful post closes the box and shows the existing delivery +note. A 409 also closes it, with the "saved, not delivered" note. Any other +error keeps it open with the error. The buttons get periscope's normal +button styling rather than the browser default. + +### D9. Type and color are scoped to the walkthrough + +On `.walkthrough`, so `.md-doc` and file previews don't change: + +- **Five sizes** as custom properties: + + | Property | Size | Used for | + |---|---|---| + | `--wt-fs-meta` | 12px | header line, file paths, badges, comment timestamps, summary line | + | `--wt-fs-ui` | 13px | buttons, links, contents list, comment text | + | `--wt-fs-body` | 15px | prose and the problem statement | + | `--wt-fs-stop` | 18px | stop titles | + | `--wt-fs-title` | 22px | page title | + + Headings inside a stop's prose render at 15px semibold. Hunk code keeps + the Changes tab's own sizes. +- **Contrast:** all text at 4.5:1 or better, estimated from the tokens' + oklch lightness (luminance ≈ L³) and checked in the browser: + + | Text | `--bg-0` (page) | `--bg-1` (card) | `--bg-1-hi` | `--bg-2` (chips) | + |---|---|---|---|---| + | `--fg-3` | 3.0:1 | 2.6:1 | 2.3:1 | 2.2:1 | + | `--fg-2` | 5.4:1 | 4.7:1 | 4.1:1 | 3.9:1 | + | `--fg-1` | 10.6:1 | 9.2:1 | 8.1:1 | 7.6:1 | + + So metadata moves from `--fg-3` to `--fg-2` on the page and on cards, and + any text on a `--bg-1-hi` or `--bg-2` chip (badges, the file list's hover, + inline code) uses `--fg-1`. `--fg-3` is not used for text in the view. +- **Inline code** in prose takes `--fg-1` (the body's color) on its chip + background. `strong` stays `--fg-0`. +- **Kind tokens:** each kind gets `--kind-` (an accent for the card + edge and the contents dot, at ≥ 3:1 against `--bg-1`) beside its existing + badge background and text pair, moved into `:root` as tokens. Unknown + kinds use a neutral `--kind-other`. + +## Mechanics + +### Frontend + +- **`review/walkthrough.js`:** + - `parseWalkthrough` adds `problemMarkdown`, and per stop `source`, + `diagrams` and the trimmed `bodyMarkdown` (D1); + - new pure `stopSig`, `isReviewed` and `evidenceFor`. +- **`split/mermaidTheme.js`** (new, pure): `mermaidTheme(read)`, beside + `Mermaid.jsx`, since the theme is app-wide and not review-specific. The + canvas-based token reader lives in `Mermaid.jsx`, so the builder stays + testable without a DOM. +- **`review/Walkthrough.jsx`** splits in two: + - `Walkthrough` fetches and owns `reviewed`, the posting nonce and the + lightbox host; + - a pure `WalkthroughView({data, repo, branch, pr, hl, reviewed, + onToggleReviewed, onPosted})` renders the page, so render-to-string can + test it. + + Stops become `Stop` → `StopTitle`, `Diagrams`, `Prose`, `Evidence`, + `Comments`, `CommentBox`. +- **`split/markdown.jsx`:** a new `zoom` option. `img` gets the lightbox + handlers, and `Mermaid` receives `zoom`. +- **`split/Mermaid.jsx`:** base theme from `mermaidTheme`, plus the `zoom` + prop (click or Enter opens the lightbox). +- **`split/Lightbox.jsx`** (new): a `lightbox` signal and `` + (backdrop, content, ✕, `useEscape`). The walkthrough mounts the host. +- **`static/styles.css`:** the walkthrough section is rewritten: tokens, + grids, the container query, sticky column, cards, badges, comment control + and lightbox. + +### Server + +`reviews.py` and `routes/reviews.py` per D6. No change to the queue, +`list_reviews`, comments or prepare. + +### Skill + +`skills/walkthrough/SKILL.md`, Diagrams section: one line saying the view +draws a stop's diagrams above its text at full width, so a diagram should +make sense before the prose explains it. + +## Tests + +- **vitest, pure** (`review/__tests__/walkthrough.test.js`): + - `parseWalkthrough`: top-level diagrams lifted in order; nested fence left + in place; `bodyMarkdown` loses only the lifted fences; `problemMarkdown` + keeps inline markdown; + - `stopSig`: stable for the same source and changed by a one-character + edit to title, metadata or body; + - `isReviewed`: match, mismatch, missing; + - `evidenceFor`: shown, missing, generated, unchanged and no-diff cases, + plus the summary counts. +- **vitest, pure** (`split/__tests__/mermaidTheme.test.js`, new): + `mermaidTheme` maps a stub token reader to the D4 table. +- **vitest, render** (`review/__tests__/walkthroughRender.test.jsx`, new), + against `WalkthroughView`: + - the staleness badge class and text, and the progress count; + - contents numbering and ✓; + - card kind classes; + - the diagram row present only with diagrams; + - the two-column markup; + - the hidden summary line; + - a reviewed stop folded to its title row; + - the comment link versus the open box. + + Buttons are pressed by calling components and invoking `onClick`, as + `hunk.test.jsx` and `reviewsQueueRender.test.jsx` do. +- **pytest** (`tests/test_reviews.py`, `tests/routes/test_reviews.py`), per + `docs/testing.md`: + - `read_reviewed`: missing, corrupt, non-object and non-string values; + - `set_reviewed`: set, overwrite, unset, and deleting the file when empty; + - the route: 200 round-trip visible in `GET /api/reviews/walkthrough`; 400 + for a bad stop and a bad sig; 404 with no walkthrough; `_check` + rejection. +- **In the browser,** on the dev server (:17375) with the real 13-stop + walkthrough, at a wide and a narrow width: + - diagrams are legible; the lightbox opens and closes (click, Enter, Esc, + backdrop); + - the code column sticks and scrolls on its own; a ref click lands on its + file; + - no sideways page scroll when narrow; + - a reviewed mark survives a reload and lands in `reviewed.json`; + - contrast measured. + +## Out of scope + +- **Why most refs read "not in this diff"** (THI-443 finding 11). It may be + a ref-matching or diff-base bug on the server, and gets its own ticket. +- **The Reviews queue and file previews,** apart from the app-wide diagram + theme (D4). +- **Showing reviewed progress in the queue.** From f402443595515039d5c897af5453396e14bea07e Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 12:47:32 -0400 Subject: [PATCH 02/48] =?UTF-8?q?plan:=20review=20walkthrough=20two-column?= =?UTF-8?q?=20stops=20=E2=80=94=2013=20tasks=20from=20parser=20split=20to?= =?UTF-8?q?=20browser=20check=20(THI-443)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) --- .../2026-10-29-walkthrough-two-column.md | 2004 +++++++++++++++++ 1 file changed, 2004 insertions(+) create mode 100644 docs/superpowers/plans/2026-10-29-walkthrough-two-column.md diff --git a/docs/superpowers/plans/2026-10-29-walkthrough-two-column.md b/docs/superpowers/plans/2026-10-29-walkthrough-two-column.md new file mode 100644 index 00000000..28aed7bf --- /dev/null +++ b/docs/superpowers/plans/2026-10-29-walkthrough-two-column.md @@ -0,0 +1,2004 @@ +# Walkthrough two-column stops: implementation plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Rebuild the review walkthrough view as the THI-443 spec describes (`docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md`): + +- a fixed header over one scrolling body; +- each stop a card, with its diagrams in a full-width row, then its prose beside its files and code changes; +- legible, themed diagrams that open full size; +- per-stop reviewed marks saved in `reviewed.json`; +- a collapsed comment box; +- a scoped type scale and contrast pass. + +**Architecture:** +- **Pure helpers** in `static/src/review/walkthrough.js` (parsing, signatures, evidence grouping) and `static/src/split/mermaidTheme.js` carry the logic, all tested without a DOM. +- **`Walkthrough.jsx`** splits into a fetching `Walkthrough` and a pure `WalkthroughView`, so render-to-string tests can see the page. +- **Server side** is two small helpers in `periscope/reviews.py` and one POST route in `periscope/routes/reviews.py`. +- **CSS** is scoped under `.walkthrough`, because the Reviews queue shares `.wt-head`, `.wt-sub` and `.wt-empty`. + +**Tech stack:** Preact + `@preact/signals`, Vite, vitest (`preact-render-to-string`, `environment: "node"`, no DOM), FastAPI, pytest, mermaid 11. + +**Ground rules for this repo:** +- Work in the worktree `/Users/thibaultdody/github/tomplex/periscope/.claude/worktrees/thi-443-walkthrough-layout`, branch `thibaultdody/thi-443-review-walkthrough-full-width-layout-legible-diagrams`. +- Comments explain *why*, tersely. Match the surrounding style. Biome is lint-only; keep the terse hand-written UI style. +- Routes raise `HTTPException(status, detail)` with real status codes. +- Commit after every task. Single-line messages ending with the `Co-Authored-By` trailer from the session's attribution rule. +- Never push or open a PR without the user's go-ahead. +- `static/dist/app.js` is committed and must be rebuilt (`npm run build`) after `static/src/` changes. That's done once, in Task 11. + +--- + +## File map + +| File | Change | Responsibility | +|---|---|---| +| `static/src/review/walkthrough.js` | modify | `parseWalkthrough` lifts diagrams and returns `problemMarkdown` and `source`; new `stopSig`, `isReviewed`, `withMark`, `evidenceFor`, `hiddenSummary`, `evidenceIndex` | +| `static/src/review/__tests__/walkthrough.test.js` | modify | cases for all of the above | +| `static/src/split/mermaidTheme.js` | create | pure `mermaidTheme({color, font})` → mermaid config | +| `static/src/split/__tests__/mermaidTheme.test.js` | create | the token mapping | +| `static/src/split/Lightbox.jsx` | create | `lightbox` signal, `openLightbox`, `closeLightbox`, `zoomKeys`, `` | +| `static/src/split/__tests__/lightbox.test.jsx` | create | host render, open/close, keys | +| `static/src/split/Mermaid.jsx` | modify | base theme from `mermaidTheme`, `zoom` prop | +| `static/src/split/markdown.jsx` | modify | `zoom` option for images and diagrams | +| `static/src/split/__tests__/markdown.test.jsx` | modify | zoom cases | +| `periscope/reviews.py` | modify | `reviewed_path`, `read_reviewed`, `set_reviewed` | +| `tests/test_reviews.py` | modify | helper cases | +| `periscope/routes/reviews.py` | modify | `POST /api/reviews/reviewed`, `reviewed` in the walkthrough GET, shared `_check_stop` | +| `tests/routes/test_reviews.py` | modify | route cases | +| `static/src/review/Walkthrough.jsx` | rewrite | `Walkthrough` (fetch, marks) and `WalkthroughView` (pure page), `Stop`, `Evidence`, `Contents`, `ReviewedToggle`, `CommentForm`, `CommentBox`, `sendHint`, `commentOutcome` | +| `static/src/review/__tests__/walkthroughRender.test.jsx` | create | the page's render states | +| `static/styles.css` | modify | kind tokens in `:root`; the walkthrough block rewritten; lightbox | +| `docs/superpowers/specs/2026-10-26-review-walkthrough-design.md` | modify | D5 points at the new spec | +| `skills/walkthrough/SKILL.md` | modify | one line in Diagrams | +| `CLAUDE.md` | modify | the reviews row names the new spec | + +--- + +### Task 0: Set up the worktree + +- [ ] **Step 1: Install frontend dependencies and sync Python** + +Run (in the worktree): +```bash +npm ci --no-audit --no-fund +uv sync +``` +Expected: both finish without errors. + +- [ ] **Step 2: Confirm the suites are green before changing anything** + +Run: +```bash +npx vitest run static/src/review static/src/split +uv run pytest tests/test_reviews.py tests/routes/test_reviews.py -q +``` +Expected: all pass. + +--- + +### Task 1: The parser lifts diagrams and keeps the problem's markdown + +**Files:** +- Modify: `static/src/review/walkthrough.js` (`parseWalkthrough`) +- Test: `static/src/review/__tests__/walkthrough.test.js` + +- [ ] **Step 1: Write the failing tests** + +Append to the end of `static/src/review/__tests__/walkthrough.test.js`: + +````js +describe("parseWalkthrough lifts diagrams (THI-443 D1)", () => { + const FENCES = `# T + +> branch: feat/x · head: abc + +Panes **cost** more and \`nothing\` said so. + +## Adds a gauge +> kind: new-subsystem · refs: a.py + +Before. + +\`\`\`mermaid +flowchart LR + a --> b +\`\`\` + +Between. + +- a list item + + \`\`\`mermaid + flowchart TD + nested --> stays + \`\`\` + +\`\`\`mermaid +flowchart LR + c --> d +\`\`\` + +After. +`; + const w = parseWalkthrough(FENCES); + const s = w.stops[0]; + + it("returns the top-level mermaid fences in order", () => { + expect(s.diagrams).toEqual(["flowchart LR\n a --> b", "flowchart LR\n c --> d"]); + }); + it("cuts only those fences out of the body and leaves a nested one in place", () => { + expect(s.bodyMarkdown).not.toContain("a --> b"); + expect(s.bodyMarkdown).not.toContain("c --> d"); + for (const kept of ["Before.", "Between.", "After.", "nested --> stays"]) expect(s.bodyMarkdown).toContain(kept); + }); + it("keeps the stop's whole source, diagrams included, for its signature", () => { + expect(s.source.startsWith("## Adds a gauge\n> kind: new-subsystem")).toBe(true); + expect(s.source).toContain("a --> b"); + }); + it("returns the problem statement as markdown beside the plain text", () => { + expect(w.problemMarkdown).toBe("Panes **cost** more and `nothing` said so."); + expect(w.problem).toBe("Panes cost more and nothing said so."); + }); + it("gives a stop without diagrams an empty list", () => { + expect(parseWalkthrough(DOC).stops[0].diagrams).toEqual([]); + }); +}); +```` + +(`DOC` is the constant already defined at the top of this file.) + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `npx vitest run static/src/review/__tests__/walkthrough.test.js` +Expected: the 5 new tests fail (`s.diagrams` is `undefined`, `w.problemMarkdown` is `undefined`). + +- [ ] **Step 3: Implement** + +In `static/src/review/walkthrough.js`, inside `parseWalkthrough`: + +Replace +```js + let problem = ""; +``` +with +```js + let problem = ""; + let problemMarkdown = ""; +``` + +Replace +```js + if (blocks[i]?.type === "paragraph") { problem = textOf(blocks[i]); i++; } +``` +with +```js + if (blocks[i]?.type === "paragraph") { + problem = textOf(blocks[i]); + problemMarkdown = src.slice(blocks[i].position.start.offset, blocks[i].position.end.offset); + i++; + } +``` + +Replace +```js + const bodyEnd = j < blocks.length ? blocks[j].position.start.offset : src.length; + const base = slug(stopTitle); + const n = (ids.get(base) || 0) + 1; + ids.set(base, n); + stops.push({ + id: n === 1 ? base : `${base}-${n}`, + title: stopTitle, + kind, + knownKind: kind != null && KINDS.has(kind), + refs, + bodyMarkdown: src.slice(bodyStart, bodyEnd), + }); +``` +with +```js + const bodyEnd = j < blocks.length ? blocks[j].position.start.offset : src.length; + // Top-level mermaid fences move to the stop's full-width diagram row + // (THI-443 D1). A fence inside a list item stays where it is: lifting it + // out would reorder content we can't place cleanly. + const diagrams = []; + let body = ""; + let cursor = bodyStart; + for (let k = i + 1; k < j; k++) { + const node = blocks[k]; + if (node.type !== "code" || node.lang !== "mermaid") continue; + diagrams.push(node.value); + body += src.slice(cursor, node.position.start.offset); + cursor = node.position.end.offset; + } + body += src.slice(cursor, bodyEnd); + const base = slug(stopTitle); + const n = (ids.get(base) || 0) + 1; + ids.set(base, n); + stops.push({ + id: n === 1 ? base : `${base}-${n}`, + title: stopTitle, + kind, + knownKind: kind != null && KINDS.has(kind), + refs, + // The whole stop as written, diagrams included: stopSig hashes this. + source: src.slice(b.position.start.offset, bodyEnd), + diagrams, + bodyMarkdown: body, + }); +``` + +Replace +```js + return { title, header, problem, stops }; +``` +with +```js + return { title, header, problem, problemMarkdown, stops }; +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `npx vitest run static/src/review/__tests__/walkthrough.test.js` +Expected: all pass, the existing `parseWalkthrough` cases included. + +- [ ] **Step 5: Commit** + +```bash +git add static/src/review/walkthrough.js static/src/review/__tests__/walkthrough.test.js +git commit -m "reviews walkthrough: parseWalkthrough lifts top-level mermaid fences into stop.diagrams, keeps stop.source and the problem's markdown (THI-443)" +``` + +--- + +### Task 2: Stop signatures and reviewed marks (pure) + +**Files:** +- Modify: `static/src/review/walkthrough.js` +- Test: `static/src/review/__tests__/walkthrough.test.js` + +- [ ] **Step 1: Write the failing tests** + +Change the import line at the top of `static/src/review/__tests__/walkthrough.test.js` to: +```js +import { + filterRows, isReviewed, matchRefs, nextSort, parseWalkthrough, sortRows, staleLabel, stopSig, unresolvedTotal, withMark, +} from "../walkthrough.js"; +``` + +Append: +```js +describe("stopSig, isReviewed, withMark (THI-443 D6)", () => { + const w = parseWalkthrough(DOC); + const first = (doc) => parseWalkthrough(doc).stops[0]; + + it("is 8 hex chars and stable for the same source", () => { + expect(stopSig(w.stops[0])).toMatch(/^[0-9a-f]{8}$/); + expect(stopSig(first(DOC))).toBe(stopSig(w.stops[0])); + }); + it("changes with a one-character edit to the title, the metadata or the text", () => { + const sig = stopSig(w.stops[0]); + // String.replace with a string pattern edits the first stop only. + for (const edited of [ + DOC.replace("## Adds a gauge", "## Adds a gaugE"), + DOC.replace("kind: new-subsystem", "kind: new-subsystems"), + DOC.replace("Prose one.", "Prose one!"), + ]) expect(stopSig(first(edited))).not.toBe(sig); + }); + it("counts a stop reviewed only while its title maps to its current sig", () => { + const s = w.stops[2]; + expect(isReviewed(s, { [s.title]: stopSig(s) })).toBe(true); + expect(isReviewed(s, { [s.title]: "00000000" })).toBe(false); + expect(isReviewed(s, {})).toBe(false); + expect(isReviewed(s, null)).toBe(false); + }); + it("withMark sets or clears one title and leaves the rest alone", () => { + const marks = { A: "11111111", B: "22222222" }; + expect(withMark(marks, "A", "33333333")).toEqual({ A: "33333333", B: "22222222" }); + expect(withMark(marks, "A", null)).toEqual({ B: "22222222" }); + expect(marks).toEqual({ A: "11111111", B: "22222222" }); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `npx vitest run static/src/review/__tests__/walkthrough.test.js` +Expected: the 4 new tests fail with `stopSig is not a function` (or the same for `isReviewed` / `withMark`). The existing tests still pass, because a missing named export imports as `undefined` rather than failing the file. + +- [ ] **Step 3: Implement** + +Append to `static/src/review/walkthrough.js`, after `nextSort`: +```js +// FNV-1a, 32-bit, over the stop as written. A reviewed mark is tied to this: +// any rewrite of the title, metadata or text gives a new sig and lapses the +// mark (THI-443 D6), as the Changes tab's viewedSig lapses when a file changes. +export function stopSig(stop) { + const s = stop.source || ""; + let h = 0x811c9dc5; + for (let i = 0; i < s.length; i++) { + h ^= s.charCodeAt(i); + h = Math.imul(h, 0x01000193); + } + return (h >>> 0).toString(16).padStart(8, "0"); +} + +export function isReviewed(stop, reviewed) { + return !!reviewed && reviewed[stop.title] === stopSig(stop); +} + +// One stop's mark set (sig) or cleared (null), leaving the others alone. +export function withMark(reviewed, title, sig) { + const next = { ...reviewed }; + if (sig == null) delete next[title]; + else next[title] = sig; + return next; +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `npx vitest run static/src/review/__tests__/walkthrough.test.js` +Expected: every test in the file passes. + +- [ ] **Step 5: Commit** + +```bash +git add static/src/review/walkthrough.js static/src/review/__tests__/walkthrough.test.js +git commit -m "reviews walkthrough: stopSig (FNV-1a of the stop's source), isReviewed and withMark for per-stop reviewed marks (THI-443)" +``` + +--- + +### Task 3: Evidence grouping (pure) + +**Files:** +- Modify: `static/src/review/walkthrough.js` +- Test: `static/src/review/__tests__/walkthrough.test.js` + +- [ ] **Step 1: Write the failing tests** + +Change the import at the top of `static/src/review/__tests__/walkthrough.test.js` to: +```js +import { + evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, matchRefs, nextSort, + parseWalkthrough, sortRows, staleLabel, stopSig, unresolvedTotal, withMark, +} from "../walkthrough.js"; +import { parseCite } from "../../split/mdMeta.js"; +``` + +Append: +```js +describe("evidenceFor, hiddenSummary, evidenceIndex (THI-443 D5)", () => { + const files = [ + { path: "a/one.py", hunks: [ + { new_start: 1, lines: [{ kind: "add", text: "x" }] }, + { new_start: 50, lines: [{ kind: "ctx", text: "y" }] }, + ] }, + { path: "dist/app.js", hunks: [], generated: true }, + ]; + const refs = ["a/one.py", "a/one.py:200-210", "b/missing.py", "dist/app.js"].map(parseCite); + + it("shows refs with hunks and sorts the rest into missing, generated and unchanged", () => { + const ev = evidenceFor(refs, files); + expect(ev.noDiff).toBe(false); + expect(ev.shown.map((e) => [e.ref.raw, e.name, e.hunks.length])).toEqual([["a/one.py", "one.py", 2]]); + expect(ev.hidden.missing.map((r) => r.raw)).toEqual(["b/missing.py"]); + expect(ev.hidden.generated.map((r) => r.raw)).toEqual(["dist/app.js"]); + expect(ev.hidden.unchanged.map((r) => r.raw)).toEqual(["a/one.py:200-210"]); + }); + it("summarizes the hidden refs in one line, skipping empty groups", () => { + expect(hiddenSummary(evidenceFor(refs, files).hidden)).toBe( + "1 not in this diff · 1 generated · 1 unchanged at the cited lines", + ); + expect(hiddenSummary(evidenceFor(refs.slice(0, 1), files).hidden)).toBe(""); + }); + it("lists every ref with no hunks when there is no worktree diff", () => { + const ev = evidenceFor(refs, null); + expect(ev.noDiff).toBe(true); + expect(ev.shown.map((e) => [e.name, e.hunks.length])).toEqual([["one.py", 0], ["one.py", 0], ["missing.py", 0], ["app.js", 0]]); + expect(hiddenSummary(ev.hidden)).toBe(""); + }); + it("lands a prose cite on the first shown file with its path", () => { + const { shown } = evidenceFor(refs, files); + expect(evidenceIndex(shown, parseCite("a/one.py:3"))).toBe(0); + expect(evidenceIndex(shown, parseCite("b/missing.py"))).toBe(-1); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `npx vitest run static/src/review/__tests__/walkthrough.test.js` +Expected: FAIL, `evidenceFor is not a function`. + +- [ ] **Step 3: Implement** + +Append to `static/src/review/walkthrough.js`: +```js +const basename = (p) => p.slice(p.lastIndexOf("/") + 1); + +// The code-changes column (THI-443 D5). `shown` gets a file-list row and a +// block of hunks; `hidden` folds into one summary line, so files with nothing +// to show don't bury the ones that do. diffFiles null means no worktree: every +// ref is listed and none has hunks. +export function evidenceFor(refs, diffFiles) { + const hidden = { missing: [], generated: [], unchanged: [] }; + if (diffFiles == null) { + return { shown: refs.map((ref) => ({ ref, name: basename(ref.path), hunks: [] })), hidden, noDiff: true }; + } + const shown = []; + for (const m of matchRefs(refs, diffFiles)) { + if (!m.file) hidden.missing.push(m.ref); + else if (m.generated) hidden.generated.push(m.ref); + else if (!m.hunks.length) hidden.unchanged.push(m.ref); + else shown.push({ ref: m.ref, name: basename(m.ref.path), hunks: m.hunks }); + } + return { shown, hidden, noDiff: false }; +} + +export function hiddenSummary(hidden) { + const parts = []; + if (hidden.missing.length) parts.push(`${hidden.missing.length} not in this diff`); + if (hidden.generated.length) parts.push(`${hidden.generated.length} generated`); + if (hidden.unchanged.length) parts.push(`${hidden.unchanged.length} unchanged at the cited lines`); + return parts.join(" · "); +} + +// A cite chip in the prose lands on the first file-list entry for its path. +export function evidenceIndex(shown, cite) { + return shown.findIndex((e) => e.ref.path === cite.path); +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `npx vitest run static/src/review/__tests__/walkthrough.test.js` +Expected: every test in the file passes. + +- [ ] **Step 5: Commit** + +```bash +git add static/src/review/walkthrough.js static/src/review/__tests__/walkthrough.test.js +git commit -m "reviews walkthrough: evidenceFor splits a stop's refs into shown (with hunks) and hidden (missing, generated, unchanged); hiddenSummary and evidenceIndex (THI-443)" +``` + +--- + +### Task 4: Diagrams in periscope's theme + +**Files:** +- Create: `static/src/split/mermaidTheme.js` +- Create: `static/src/split/__tests__/mermaidTheme.test.js` +- Modify: `static/src/split/Mermaid.jsx` + +- [ ] **Step 1: Write the failing test** + +Create `static/src/split/__tests__/mermaidTheme.test.js`: +```js +import { describe, expect, it } from "vitest"; +import { mermaidTheme } from "../mermaidTheme.js"; + +describe("mermaidTheme", () => { + const hex = { + "--bg-1": "#111111", "--bg-1-hi": "#1a1a1a", "--bg-2": "#222222", + "--fg-0": "#f0f0f0", "--fg-1": "#d0d0d0", "--fg-2": "#a0a0a0", + }; + const t = mermaidTheme({ color: (n) => hex[n], font: "Hanken Grotesk, sans-serif" }); + + it("uses mermaid's configurable base theme in dark mode", () => { + expect(t.theme).toBe("base"); + expect(t.themeVariables.darkMode).toBe(true); + }); + it("maps periscope's tokens onto nodes, lines, labels and groups", () => { + expect(t.themeVariables).toMatchObject({ + primaryColor: "#222222", + primaryTextColor: "#f0f0f0", + nodeTextColor: "#f0f0f0", + primaryBorderColor: "#a0a0a0", + lineColor: "#a0a0a0", + clusterBorder: "#a0a0a0", + edgeLabelBackground: "#111111", + background: "#111111", + clusterBkg: "#1a1a1a", + secondaryColor: "#1a1a1a", + textColor: "#d0d0d0", + fontFamily: "Hanken Grotesk, sans-serif", + fontSize: "14px", + }); + }); + it("reads only opaque tokens, which the canvas converter can paint", () => { + const asked = new Set(); + mermaidTheme({ color: (n) => { asked.add(n); return "#000000"; }, font: "x" }); + expect([...asked].sort()).toEqual(["--bg-1", "--bg-1-hi", "--bg-2", "--fg-0", "--fg-1", "--fg-2"]); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `npx vitest run static/src/split/__tests__/mermaidTheme.test.js` +Expected: FAIL, `Failed to resolve import "../mermaidTheme.js"`. + +- [ ] **Step 3: Implement the builder** + +Create `static/src/split/mermaidTheme.js`: +```js +// Mermaid's "base" theme in periscope's colors (THI-443 D4), app-wide: file +// previews draw diagrams with it too. `color(name)` returns a token as +// #rrggbb. mermaid's color math can't parse the stylesheet's oklch(), so +// Mermaid.jsx converts each token before it gets here. Pure, so the mapping is +// testable without a DOM. +export function mermaidTheme({ color, font }) { + const card = color("--bg-1"); + const raised = color("--bg-1-hi"); + const line = color("--fg-2"); // ≥ 3:1 against a card: borders and arrows must read + return { + theme: "base", + themeVariables: { + darkMode: true, + fontFamily: font, + fontSize: "14px", + background: card, + primaryColor: color("--bg-2"), + primaryTextColor: color("--fg-0"), + nodeTextColor: color("--fg-0"), + titleColor: color("--fg-0"), + primaryBorderColor: line, + lineColor: line, + clusterBorder: line, + secondaryColor: raised, + clusterBkg: raised, + tertiaryColor: card, + edgeLabelBackground: card, + textColor: color("--fg-1"), + }, + }; +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `npx vitest run static/src/split/__tests__/mermaidTheme.test.js` +Expected: PASS (3 tests). + +- [ ] **Step 5: Wire it into `Mermaid.jsx`** + +In `static/src/split/Mermaid.jsx`: + +Add after the existing `import { useEffect, useRef, useState } from "preact/hooks";`: +```js +import { mermaidTheme } from "./mermaidTheme.js"; +``` + +Replace +```js +let mermaidP = null; +function loadMermaid() { + if (!mermaidP) { + mermaidP = import("mermaid").then((m) => { + const mermaid = m.default; + // The dashboard has one (dark) theme; following prefers-color-scheme + // would draw a light diagram on the dark UI whenever macOS is light. + // Without suppressErrorRendering, render() on bad source draws the "Syntax + // error" bomb SVG into a temp div on document.body and throws before + // removing it, leaving a stray error diagram on the dashboard. + mermaid.initialize({ + startOnLoad: false, + theme: "dark", + securityLevel: "strict", + suppressErrorRendering: true, + }); +``` +with +```js +const tokenValue = (name) => getComputedStyle(document.documentElement).getPropertyValue(name).trim(); + +let paint = null; +// Paint a token on a 1×1 canvas and read the pixel back: the portable way to +// turn the stylesheet's oklch() into the #rrggbb mermaid's color parser takes. +function tokenHex(name) { + if (!paint) paint = document.createElement("canvas").getContext("2d", { willReadFrequently: true }); + paint.clearRect(0, 0, 1, 1); + paint.fillStyle = "#000"; + paint.fillStyle = tokenValue(name); + paint.fillRect(0, 0, 1, 1); + const [r, g, b] = paint.getImageData(0, 0, 1, 1).data; + return `#${[r, g, b].map((x) => x.toString(16).padStart(2, "0")).join("")}`; +} + +let mermaidP = null; +function loadMermaid() { + if (!mermaidP) { + mermaidP = import("mermaid").then((m) => { + const mermaid = m.default; + // Periscope's own tokens on mermaid's base theme (THI-443 D4), read once: + // the dashboard has one dark theme, so following prefers-color-scheme + // would only ever draw a light diagram on the dark UI. + // Without suppressErrorRendering, render() on bad source draws the "Syntax + // error" bomb SVG into a temp div on document.body and throws before + // removing it, leaving a stray error diagram on the dashboard. + mermaid.initialize({ + startOnLoad: false, + securityLevel: "strict", + suppressErrorRendering: true, + ...mermaidTheme({ color: tokenHex, font: tokenValue("--sans") }), + }); +``` + +- [ ] **Step 6: Run the split tests** + +Run: `npx vitest run static/src/split` +Expected: all pass. The markdown mermaid tests only check the host markup; `loadMermaid` never runs under render-to-string. + +- [ ] **Step 7: Commit** + +```bash +git add static/src/split/mermaidTheme.js static/src/split/__tests__/mermaidTheme.test.js static/src/split/Mermaid.jsx +git commit -m "diagrams: mermaid's base theme in periscope's tokens (node fill bg-2, text fg-0, lines fg-2, Hanken Grotesk 14px), oklch tokens converted to hex on a 1×1 canvas (THI-443)" +``` + +--- + +### Task 5: Full-size view for diagrams and images + +**Files:** +- Create: `static/src/split/Lightbox.jsx` +- Create: `static/src/split/__tests__/lightbox.test.jsx` +- Modify: `static/src/split/Mermaid.jsx`, `static/src/split/markdown.jsx` +- Test: `static/src/split/__tests__/markdown.test.jsx` + +- [ ] **Step 1: Write the failing lightbox tests** + +Create `static/src/split/__tests__/lightbox.test.jsx`: +```jsx +import render from "preact-render-to-string"; +import { afterEach, describe, expect, it } from "vitest"; +import { closeLightbox, LightboxHost, lightbox, openLightbox, zoomKeys } from "../Lightbox.jsx"; + +afterEach(() => { lightbox.value = null; }); + +describe("lightbox", () => { + it("renders nothing while closed", () => { + expect(render()).toBe(""); + }); + it("shows an image at natural size with a close button, and closes", () => { + openLightbox({ kind: "img", src: "https://example.com/a.png", alt: "chart" }); + const html = render(); + expect(html).toContain('role="dialog"'); + expect(html).toContain('chart { + openLightbox({ kind: "svg", svg: "" }); + expect(render()).toContain('class="lightbox-svg"'); + }); + it("zoomKeys opens on Enter and Space only", () => { + let opened = 0; + const prevented = []; + const onKey = zoomKeys(() => { opened++; }); + for (const key of ["Enter", " ", "a", "Escape"]) onKey({ key, preventDefault: () => prevented.push(key) }); + expect(opened).toBe(2); + expect(prevented).toEqual(["Enter", " "]); + }); +}); +``` + +- [ ] **Step 2: Write the failing markdown zoom tests** + +Append to `static/src/split/__tests__/markdown.test.jsx` (it already defines `html`): +```jsx +describe("zoom", () => { + const img = "![chart](https://example.com/a.png)"; + it("leaves images plain without zoom", () => { + const out = html(img); + expect(out).toContain('chart { + expect(html(img, { zoom: true })).toMatch(/]*tabindex="0"[^>]*role="button"/); + const d = html("```mermaid\nflowchart LR\n a --> b\n```", { mermaid: true, zoom: true }); + expect(d).toMatch(/class="md-mermaid-svg is-zoomable"[^>]*role="button"/); + }); +}); +``` + +- [ ] **Step 3: Run both to verify they fail** + +Run: `npx vitest run static/src/split/__tests__/lightbox.test.jsx static/src/split/__tests__/markdown.test.jsx` +Expected: lightbox fails to resolve `../Lightbox.jsx`; the second zoom case fails (no `is-zoomable`). + +- [ ] **Step 4: Implement `Lightbox.jsx`** + +Create `static/src/split/Lightbox.jsx`: +```jsx +// Full-size view for a walkthrough diagram or image (THI-443 D4). One host, +// one signal: a zoomable diagram or calls openLightbox, and the host +// draws it at natural size over a backdrop that scrolls both ways. +import { signal } from "@preact/signals"; +import { useEffect, useRef } from "preact/hooks"; +import { useEscape } from "../hooks/useEscape.js"; + +// { kind: "svg", svg } | { kind: "img", src, alt }, plus `from`: the element +// that opened it, refocused on close so keyboard users land where they were. +export const lightbox = signal(null); + +export function openLightbox(item) { + lightbox.value = { ...item, from: typeof document === "undefined" ? null : document.activeElement }; +} + +export function closeLightbox() { + const from = lightbox.value?.from; + lightbox.value = null; + from?.focus?.(); +} + +// Enter or Space on a focusable diagram or image opens it, like a button. +export function zoomKeys(open) { + return (e) => { + if (e.key !== "Enter" && e.key !== " ") return; + e.preventDefault(); + open(); + }; +} + +export function LightboxHost() { + const item = lightbox.value; + const svgHost = useRef(null); + const closeBtn = useRef(null); + useEscape(closeLightbox, !!item); + useEffect(() => { + if (!item) return; + closeBtn.current?.focus(); + if (item.kind !== "svg" || !svgHost.current) return; + // Only ever mermaid's own sanitized SVG, copied from a rendered diagram + // (Mermaid.jsx's innerHTML rule). + svgHost.current.innerHTML = item.svg; + const svg = svgHost.current.querySelector("svg"); + const vb = svg?.viewBox?.baseVal; + if (svg && vb?.width) { + // Natural size: mermaid's inline max-width and width="100%" would refit it. + svg.style.maxWidth = "none"; + svg.setAttribute("width", String(Math.round(vb.width))); + svg.setAttribute("height", String(Math.round(vb.height))); + } + }, [item]); + if (!item) return null; + const onBackdrop = (e) => { if (e.target === e.currentTarget) closeLightbox(); }; + return ( +
) : null} - {ev.noDiff ?

no worktree, so code changes are unavailable

: ev.shown.map((e, i) => ( -
-
{e.ref.raw}
- {e.hunks.map((h, j) => )} -
- ))} + {ev.noDiff ?

no worktree, so code changes are unavailable

: ev.shown.map((e, i) => { + const n = lineCount(e.hunks); + return ( +
+
{e.ref.raw}
+ {n <= COLLAPSE_LINES || expanded.has(i) + ? e.hunks.map((h, j) => ) + : } +
+ ); + })} ); } function Stop({ stop, n, diffFiles, comments, repo, branch, hl, reviewed, toggleError, onToggleReviewed, onPosted }) { const [unfolded, setUnfolded] = useState(false); + // Large file blocks the reader has opened, by index into ev.shown. + const [expanded, setExpanded] = useState(() => new Set()); const col = useRef(null); const done = isReviewed(stop, reviewed); const folded = done && !unfolded; - const ev = evidenceFor(stop.refs, diffFiles); - // Wide, the column scrolls on its own, so move it rather than the page. - // Otherwise bring the file into view only if it isn't already ("nearest"), so - // a column that fits on screen never jumps the page (D5). - function goTo(i) { + const ev = useMemo(() => evidenceFor(stop.refs, diffFiles), [stop.refs, diffFiles]); + const expand = useCallback((i) => setExpanded((s) => (s.has(i) ? s : new Set(s).add(i))), []); + // Stable, so the memoized prose below keeps its cite chips across marks. + const goTo = useCallback((i) => { const c = col.current; const el = c?.querySelector(`[data-ev="${i}"]`); if (!el) return; - if (c.scrollHeight > c.clientHeight) c.scrollTo({ top: el.offsetTop - 8, behavior: motion() }); - else el.scrollIntoView({ block: "nearest", behavior: motion() }); - // Restart the flash even on a second click of the same file: the reflow - // between remove and add makes the browser replay the animation. - el.classList.remove("is-flash"); - el.getBoundingClientRect(); - el.classList.add("is-flash"); - } - const cite = (c) => { - const i = ev.noDiff ? -1 : evidenceIndex(ev.shown, c); - return ( - - ); - }; + // A collapsed block opens first, and the scroll waits for it to draw: + // "nearest" aimed at the button alone would leave its code off screen. + if (el.querySelector(".wt-ev-more")) { + expand(i); + requestAnimationFrame(() => reveal(c, el)); + } else reveal(c, el); + }, [expand]); + const prose = useMemo(() => { + const cite = (c) => { + const i = ev.noDiff ? -1 : evidenceIndex(ev.shown, c); + return ( + + ); + }; + return renderMarkdown(stop.bodyMarkdown, { demote: false, softBreaks: "space", mermaid: true, meta: true, cite, zoom: true }); + }, [stop.bodyMarkdown, ev, goTo]); const title = <>{`${n}.`} {stop.title}; return (
@@ -224,13 +265,11 @@ function Stop({ stop, n, diffFiles, comments, repo, branch, hl, reviewed, toggle ) : null}
-
- {renderMarkdown(stop.bodyMarkdown, { demote: false, softBreaks: "space", mermaid: true, meta: true, cite, zoom: true })} -
+
{prose}
- +
)} diff --git a/static/src/review/__tests__/walkthrough.test.js b/static/src/review/__tests__/walkthrough.test.js index 8586a0f5..6cae8887 100644 --- a/static/src/review/__tests__/walkthrough.test.js +++ b/static/src/review/__tests__/walkthrough.test.js @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { parseCite } from "../../split/mdMeta.js"; import { - evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, matchRefs, nextSort, + COLLAPSE_LINES, evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, lineCount, matchRefs, nextSort, parseWalkthrough, sortRows, staleLabel, stopSig, unresolvedTotal, withMark, } from "../walkthrough.js"; @@ -449,3 +449,15 @@ describe("filterRows", () => { expect(branches("periscope xyz")).toEqual([]); }); }); + +describe("lineCount, COLLAPSE_LINES (THI-443 review)", () => { + it("counts every diff line across a file's hunks", () => { + const h = (n) => ({ new_start: 1, lines: Array.from({ length: n }, () => ({ kind: "ctx", text: "" })) }); + expect(lineCount([h(3), h(4)])).toBe(7); + expect(lineCount([])).toBe(0); + expect(lineCount([{ new_start: 1 }])).toBe(0); + }); + it("collapses a file block past 300 lines", () => { + expect(COLLAPSE_LINES).toBe(300); + }); +}); diff --git a/static/src/review/__tests__/walkthroughRender.test.jsx b/static/src/review/__tests__/walkthroughRender.test.jsx index bbe79258..c6973e9b 100644 --- a/static/src/review/__tests__/walkthroughRender.test.jsx +++ b/static/src/review/__tests__/walkthroughRender.test.jsx @@ -3,7 +3,7 @@ // press the pure pieces (ReviewedToggle) by calling them, as hunk.test.jsx does. import render from "preact-render-to-string"; import { describe, expect, it } from "vitest"; -import { CommentForm, commentOutcome, ReviewedToggle, sendHint, WalkthroughView } from "../Walkthrough.jsx"; +import { CommentForm, commentOutcome, MemoHunk, ReviewedToggle, sendHint, WalkthroughView } from "../Walkthrough.jsx"; import { parseWalkthrough, stopSig } from "../walkthrough.js"; const DOC = `# Gauge @@ -86,6 +86,17 @@ describe(" body", () => { expect(html).toContain('1 change'); expect(html).toContain("1 not in this diff"); }); + it("collapses a file block past 300 lines to a button, and keeps its row and count", () => { + const lines = (n) => Array.from({ length: n }, () => ({ kind: "add", text: "x" })); + const big = { files: [{ path: "a/one.py", hunks: [{ new_start: 1, lines: lines(301) }] }] }; + const html = view({ diff: big }); + expect(html).toContain('1 change'); + expect(html).toContain(''); + expect(html).not.toContain('class="dhunk"'); + const edge = { files: [{ path: "a/one.py", hunks: [{ new_start: 1, lines: lines(300) }] }] }; + expect(view({ diff: edge })).not.toContain("wt-ev-more"); + expect(view({ diff: edge })).toContain('class="dhunk"'); + }); it("lists files without hunks when there is no worktree", () => { const html = view({ diff: null, stale_by: null }); expect(html).toContain('missing.py'); @@ -113,6 +124,14 @@ describe(" body", () => { }); describe("walkthrough pieces", () => { + it("MemoHunk redraws a hunk only when one of its props changes", () => { + const props = { hunk: diff.files[0].hunks[0], lang: "python", hl: null, onOpen: null }; + const m = new MemoHunk(props); + m.props = props; + expect(m.shouldComponentUpdate({ ...props })).toBe(false); + expect(m.shouldComponentUpdate({ ...props, hl: () => "" })).toBe(true); + expect(m.shouldComponentUpdate({ ...props, hunk: { ...props.hunk } })).toBe(true); + }); it("ReviewedToggle presses through to onToggle and reports its state", () => { let pressed = 0; const button = ReviewedToggle({ on: false, onToggle: () => { pressed++; }, error: null }).props.children[1]; diff --git a/static/src/review/walkthrough.js b/static/src/review/walkthrough.js index b50cf0d9..6dd41976 100644 --- a/static/src/review/walkthrough.js +++ b/static/src/review/walkthrough.js @@ -203,6 +203,11 @@ export function evidenceIndex(shown, cite) { return hit >= 0 ? hit : shown.findIndex((e) => e.ref.path === cite.path); } +// A whole-file cite can carry thousands of diff lines, and drawing them all +// at once stalled the page, so a block past COLLAPSE_LINES starts as a button. +export const COLLAPSE_LINES = 300; +export const lineCount = (hunks) => hunks.reduce((n, h) => n + (h.lines?.length || 0), 0); + // Merged rows sink to the bottom of the readiness order and of every column // sort, and their comments stop counting toward the header badge. const isMerged = (r) => r.pr?.state === "merged"; From c57f966c62190f10dfb06e82e91e6986c7683985 Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 14:48:32 -0400 Subject: [PATCH 26/48] reviews walkthrough comment box: focus moves only on the reader's open, cancel or send (not when a draft remounts it open), cancel discards the draft, one always-present status note, the link is named for its stop (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- static/src/review/Walkthrough.jsx | 57 ++++++++++++------- .../__tests__/walkthroughRender.test.jsx | 38 +++++++++++-- 2 files changed, 70 insertions(+), 25 deletions(-) diff --git a/static/src/review/Walkthrough.jsx b/static/src/review/Walkthrough.jsx index 4078d01c..eb2b6ca6 100644 --- a/static/src/review/Walkthrough.jsx +++ b/static/src/review/Walkthrough.jsx @@ -69,10 +69,11 @@ function Comments({ list }) { ); } -// The open box, pure, so a render test can see it. -export function CommentForm({ stop, text, busy, note, textRef, onInput, onSend, onCancel }) { +// The open box's text and buttons, pure, so a render test can see them. The +// note lives in CommentBox, outside the open and closed states. +export function CommentForm({ stop, text, busy, textRef, onInput, onSend, onCancel }) { return ( -
+ <> '); + cancel.props.onClick(); + expect(drafts.has(key)).toBe(false); + expect(view()).not.toContain(" { expect(view({}, { toggleErrors: { "Adds a gauge": "not saved: HTTP 500" } })) @@ -145,17 +170,18 @@ describe("walkthrough pieces", () => { expect(render( {}} error={null} />)) .toContain('aria-pressed="false">reviewed'); }); - it("CommentForm shows the text, the send hint and its buttons", () => { + it("CommentForm shows the text, the send hint and its buttons, and leaves the note to its box", () => { const html = render( - {}} onSend={() => {}} onCancel={() => {}} />, ); expect(html).toContain('aria-label="comment on Adds a gauge">why?'); expect(html).toContain(''); expect(html).toContain(''); expect(html).toMatch(/(⌘|Ctrl\+)↩ to send<\/span>/); + expect(html).not.toContain("wt-comment-note"); expect(render( - {}} onSend={() => {}} onCancel={() => {}} />, + {}} onSend={() => {}} onCancel={() => {}} />, )).toMatch(/
{folded ? null : ( diff --git a/static/src/review/__tests__/walkthrough.test.js b/static/src/review/__tests__/walkthrough.test.js index 6cae8887..2eb7a550 100644 --- a/static/src/review/__tests__/walkthrough.test.js +++ b/static/src/review/__tests__/walkthrough.test.js @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { parseCite } from "../../split/mdMeta.js"; import { - COLLAPSE_LINES, evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, lineCount, matchRefs, nextSort, + COLLAPSE_LINES, commentsLabel, evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, lineCount, matchRefs, nextSort, parseWalkthrough, sortRows, staleLabel, stopSig, unresolvedTotal, withMark, } from "../walkthrough.js"; @@ -461,3 +461,18 @@ describe("lineCount, COLLAPSE_LINES (THI-443 review)", () => { expect(COLLAPSE_LINES).toBe(300); }); }); + +describe("commentsLabel (THI-443 review)", () => { + const c = (resolved) => ({ ts: "t", stop: "S", resolved, body: "b" }); + it("counts a folded stop's comments and the open ones", () => { + expect(commentsLabel([c(false), c(true)])).toBe("2 comments · 1 open"); + expect(commentsLabel([c(false), c(false)])).toBe("2 comments · 2 open"); + }); + it("says one comment, and leaves out an open count of zero", () => { + expect(commentsLabel([c(true)])).toBe("1 comment"); + expect(commentsLabel([c(false)])).toBe("1 comment · 1 open"); + }); + it("is empty with no comments", () => { + expect(commentsLabel([])).toBe(""); + }); +}); diff --git a/static/src/review/__tests__/walkthroughRender.test.jsx b/static/src/review/__tests__/walkthroughRender.test.jsx index 45da8033..ac90e5da 100644 --- a/static/src/review/__tests__/walkthroughRender.test.jsx +++ b/static/src/review/__tests__/walkthroughRender.test.jsx @@ -148,6 +148,35 @@ describe(" body", () => { }); }); +describe(" comments", () => { + const comment = (stop, body, resolved = false) => ({ ts: "2026-10-29 10:00", stop, resolved, body }); + const comments = [comment("Adds a gauge", "why?"), comment("Adds a gauge", "fixed", true), comment("Gone stop", "orphan")]; + const stopAt = (html, title) => html.indexOf(` ${title}`); + it("lists each comment under its stop, and the ones for unknown titles last", () => { + const html = view({ comments }); + const one = stopAt(html, "Adds a gauge"); + const two = stopAt(html, "Reuses the cache"); + const loose = html.indexOf("unattached comments"); + expect(html.indexOf("why?")).toBeGreaterThan(one); + expect(html.indexOf("why?")).toBeLessThan(two); + expect(html.indexOf("fixed")).toBeLessThan(two); + expect(loose).toBeGreaterThan(two); + expect(html.indexOf("orphan")).toBeGreaterThan(loose); + expect(count(html, 'class="wt-comments"')).toBe(2); + }); + it("puts a folded stop's comment count in its title row, since folding hides the replies", () => { + const folded = view({ comments }, { reviewed: { "Adds a gauge": firstSig } }); + expect(folded).not.toContain("why?"); + expect(folded).toMatch(/class="wt-stop-head">.*2 comments · 1 open<\/span>/); + expect(count(folded, "wt-comment-chip")).toBe(1); + const resolved = view({ comments: [comment("Adds a gauge", "ok", true)] }, { reviewed: { "Adds a gauge": firstSig } }); + expect(resolved).toContain('1 comment'); + // Unfolded, the comments are on show and need no chip; folded with none, nothing to count. + expect(view({ comments })).not.toContain("wt-comment-chip"); + expect(view({}, { reviewed: { "Adds a gauge": firstSig } })).not.toContain("wt-comment-chip"); + }); +}); + describe("walkthrough pieces", () => { it("MemoHunk redraws a hunk only when one of its props changes", () => { const props = { hunk: diff.files[0].hunks[0], lang: "python", hl: null, onOpen: null }; diff --git a/static/src/review/walkthrough.js b/static/src/review/walkthrough.js index 6dd41976..2a9bfecd 100644 --- a/static/src/review/walkthrough.js +++ b/static/src/review/walkthrough.js @@ -208,6 +208,15 @@ export function evidenceIndex(shown, cite) { export const COLLAPSE_LINES = 300; export const lineCount = (hunks) => hunks.reduce((n, h) => n + (h.lines?.length || 0), 0); +// A folded stop's stand-in for its comment list. Folding hides the list, and +// with it any reply the owning pane wrote into a comment without rewriting the +// stop (which would have lapsed the mark). +export function commentsLabel(list) { + if (!list.length) return ""; + const open = list.filter((c) => !c.resolved).length; + return `${list.length} comment${list.length === 1 ? "" : "s"}${open ? ` · ${open} open` : ""}`; +} + // Merged rows sink to the bottom of the readiness order and of every column // sort, and their comments stop counting toward the header badge. const isMerged = (r) => r.pr?.state === "merged"; From 8815c8ada329b2e0adcc413f6a6fff0afc1b38bb Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 14:51:27 -0400 Subject: [PATCH 28/48] reviews walkthrough marks: markQueue sends a title's reviewed POSTs one at a time and, when its newest click settles, shows the last server-confirmed mark (seeded from an applied refetch); errors show detail only when it's a string, not FastAPI's 422 list (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- static/src/review/Walkthrough.jsx | 58 ++++++------ .../src/review/__tests__/walkthrough.test.js | 88 ++++++++++++++++++- .../__tests__/walkthroughRender.test.jsx | 6 ++ static/src/review/walkthrough.js | 45 ++++++++++ 4 files changed, 166 insertions(+), 31 deletions(-) diff --git a/static/src/review/Walkthrough.jsx b/static/src/review/Walkthrough.jsx index 47f44682..c91ee5c1 100644 --- a/static/src/review/Walkthrough.jsx +++ b/static/src/review/Walkthrough.jsx @@ -12,7 +12,8 @@ import { Mermaid } from "../split/Mermaid.jsx"; import { renderMarkdown } from "../split/markdown.jsx"; import { prStateMeta } from "../util.js"; import { - COLLAPSE_LINES, commentsLabel, evidenceFor, evidenceIndex, HIDDEN_WHY, hiddenSummary, isReviewed, lineCount, parseWalkthrough, stopSig, withMark, + COLLAPSE_LINES, commentsLabel, evidenceFor, evidenceIndex, HIDDEN_WHY, hiddenSummary, isReviewed, lineCount, markQueue, + parseWalkthrough, stopSig, withMark, } from "./walkthrough.js"; async function postJson(url, body) { @@ -21,6 +22,10 @@ async function postJson(url, body) { return { ok: res.ok, status: res.status, data }; } +// An error's `detail`, when it is words. FastAPI's 422s carry a list of +// objects there, which would show as "[object Object]". +const detailText = (data) => (typeof data?.detail === "string" && data.detail ? data.detail : null); + // The keys the comment box listens for, named the way this keyboard labels them. export function sendHint(platform) { return /Mac|iPhone|iPad/.test(platform || "") ? "⌘↩ to send" : "Ctrl+↩ to send"; @@ -31,8 +36,8 @@ const SEND_HINT = sendHint(typeof navigator === "undefined" ? "" : navigator.pla // closes it; anything else keeps the text so nothing typed is lost. export function commentOutcome(r) { if (r.ok) return { close: true, note: "sent to the owning pane" }; - if (r.status === 409) return { close: true, note: `saved, ${r.data?.detail || "not delivered"}` }; - return { close: false, note: r.data?.detail || `HTTP ${r.status}` }; + if (r.status === 409) return { close: true, note: `saved, ${detailText(r.data) || "not delivered"}` }; + return { close: false, note: detailText(r.data) || `HTTP ${r.status}` }; } // Smooth scrolling unless the viewer asked for less motion. @@ -384,11 +389,15 @@ export function Walkthrough({ repo, branch, pr, sig }) { const [nonce, setNonce] = useState(0); const [reviewed, setReviewed] = useState({}); const [toggleErrors, setToggleErrors] = useState({}); - // Marks in flight, and marks settled: a refetch must not apply a `reviewed` - // it read before a mark landed, whether it arrives mid-POST or just after - // ("send a comment, then mark the stop" starts exactly that refetch). - const pending = useRef(0); - const settled = useRef(0); + // One queue per review (Detail keys this component by repo/branch). + const marks = useMemo(() => markQueue({ + post: async (title, s, on) => { + const r = await postJson("/api/reviews/reviewed", { repo, branch, stop: title, sig: s, reviewed: on }); + if (!r.ok) throw new Error(detailText(r.data) || `HTTP ${r.status}`); + }, + show: (title, s) => setReviewed((x) => withMark(x, title, s)), + fail: (title, msg) => setToggleErrors((x) => ({ ...x, [title]: msg })), + }), [repo, branch]); useEffect(() => { let alive = true; @@ -398,7 +407,10 @@ export function Walkthrough({ repo, branch, pr, sig }) { useEffect(() => { let alive = true; - const marksAtStart = settled.current; + // A refetch must not apply a `reviewed` it read before a mark landed, + // whether it arrives mid-POST or just after ("send a comment, then mark + // the stop" starts exactly that refetch). + const marksAtStart = marks.settled; (async () => { try { const res = await fetch(`/api/reviews/walkthrough?repo=${encodeURIComponent(repo)}&branch=${encodeURIComponent(branch)}`); @@ -406,35 +418,21 @@ export function Walkthrough({ repo, branch, pr, sig }) { if (!alive) return; // Keep the last good data on a failed refetch: unmounting every Stop // would drop the open hunks and any half-typed comment. - if (!res.ok) { setState((s) => ({ loading: false, error: body.detail || `HTTP ${res.status}`, data: s.data })); return; } + if (!res.ok) { setState((s) => ({ loading: false, error: detailText(body) || `HTTP ${res.status}`, data: s.data })); return; } setState({ loading: false, error: null, data: body }); - if (!pending.current && settled.current === marksAtStart) setReviewed(body.reviewed || {}); + if (!marks.pending && marks.settled === marksAtStart) { + setReviewed(body.reviewed || {}); + marks.seed(body.reviewed || {}); + } } catch (e) { if (alive) setState((s) => ({ loading: false, error: String(e), data: s.data })); } })(); return () => { alive = false; }; - }, [repo, branch, nonce, sig]); + }, [repo, branch, nonce, sig, marks]); const onPosted = () => { setNonce((n) => n + 1); refreshReviews(); }; - - async function onToggleReviewed(stop, on) { - const s = stopSig(stop); - const before = reviewed[stop.title] ?? null; - setReviewed((r) => withMark(r, stop.title, on ? s : null)); - setToggleErrors((e) => ({ ...e, [stop.title]: null })); - pending.current++; - try { - const r = await postJson("/api/reviews/reviewed", { repo, branch, stop: stop.title, sig: s, reviewed: on }); - if (!r.ok) throw new Error(r.data.detail || `HTTP ${r.status}`); - } catch (e) { - setReviewed((r) => withMark(r, stop.title, before)); - setToggleErrors((x) => ({ ...x, [stop.title]: `not saved: ${e.message || e}` })); - } finally { - pending.current--; - settled.current++; - } - } + const onToggleReviewed = (stop, on) => marks.toggle(stop.title, stopSig(stop), on); if (state.loading) return
loading…
; if (!state.data) return
{state.error}
; diff --git a/static/src/review/__tests__/walkthrough.test.js b/static/src/review/__tests__/walkthrough.test.js index 2eb7a550..d188839c 100644 --- a/static/src/review/__tests__/walkthrough.test.js +++ b/static/src/review/__tests__/walkthrough.test.js @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { parseCite } from "../../split/mdMeta.js"; import { - COLLAPSE_LINES, commentsLabel, evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, lineCount, matchRefs, nextSort, + COLLAPSE_LINES, commentsLabel, evidenceFor, evidenceIndex, filterRows, hiddenSummary, isReviewed, lineCount, markQueue, matchRefs, nextSort, parseWalkthrough, sortRows, staleLabel, stopSig, unresolvedTotal, withMark, } from "../walkthrough.js"; @@ -476,3 +476,89 @@ describe("commentsLabel (THI-443 review)", () => { expect(commentsLabel([])).toBe(""); }); }); + +describe("markQueue (THI-443 review)", () => { + const SIG = "0123abcd"; + // A page stand-in: `marks` is what the toggles and contents would show, + // `errors` what sits beside each toggle, and every POST waits for the test. + function setup(seed = {}) { + const page = { marks: { ...seed }, errors: {}, calls: [] }; + const q = markQueue({ + post: (title, sig, on) => new Promise((resolve, reject) => { page.calls.push({ title, sig, on, resolve, reject }); }), + show: (title, sig) => { page.marks = withMark(page.marks, title, sig); }, + fail: (title, msg) => { page.errors[title] = msg; }, + }); + q.seed(seed); + return { page, q }; + } + const flush = () => new Promise((r) => setTimeout(r, 0)); + + it("shows a mark at once and keeps it once the server confirms", async () => { + const { page, q } = setup(); + const done = q.toggle("S", SIG, true); + expect(page.marks).toEqual({ S: SIG }); + expect(q.pending).toBe(1); + await flush(); + page.calls[0].resolve(); + await done; + expect(page.marks).toEqual({ S: SIG }); + expect(page.errors.S).toBeNull(); + expect(q.pending).toBe(0); + expect(q.settled).toBe(1); + }); + it("reverts a failed mark to the last confirmed state, with the error", async () => { + const { page, q } = setup({ S: SIG }); + const done = q.toggle("S", SIG, false); + expect(page.marks).toEqual({}); + await flush(); + page.calls[0].reject(new Error("HTTP 500")); + await done; + expect(page.marks).toEqual({ S: SIG }); + expect(page.errors.S).toBe("not saved: HTTP 500"); + }); + it("sends a title's toggles one at a time, in click order", async () => { + const { page, q } = setup(); + q.toggle("S", SIG, true); + const last = q.toggle("S", SIG, false); + q.toggle("T", SIG, true); + await flush(); + // S's second POST waits for its first; T's own queue isn't held up by S's. + expect(page.calls.map((c) => [c.title, c.on])).toEqual([["S", true], ["T", true]]); + page.calls[0].resolve(); + await flush(); + expect(page.calls.map((c) => [c.title, c.on])).toEqual([["S", true], ["T", true], ["S", false]]); + // The first settling leaves the newer click's mark on show. + expect(page.marks.S).toBeUndefined(); + page.calls[2].resolve(); + await last; + expect(page.marks.S).toBeUndefined(); + expect(q.pending).toBe(1); + }); + it("ends two failed toggles on what the server has, not on the first click's guess", async () => { + const { page, q } = setup(); + q.toggle("S", SIG, true); + const last = q.toggle("S", SIG, false); + await flush(); + page.calls[0].reject(new Error("HTTP 500")); + await flush(); + // Still showing the newer click while its POST runs. + expect(page.marks.S).toBeUndefined(); + page.calls[1].reject(new Error("HTTP 502")); + await last; + expect(page.marks).toEqual({}); + expect(page.errors.S).toBe("not saved: HTTP 502"); + }); + it("clears an earlier failure's error once the newest toggle lands", async () => { + const { page, q } = setup(); + q.toggle("S", SIG, true); + const last = q.toggle("S", SIG, true); + await flush(); + page.calls[0].reject(new Error("HTTP 500")); + await flush(); + expect(page.errors.S).toBe("not saved: HTTP 500"); + page.calls[1].resolve(); + await last; + expect(page.errors.S).toBeNull(); + expect(page.marks).toEqual({ S: SIG }); + }); +}); diff --git a/static/src/review/__tests__/walkthroughRender.test.jsx b/static/src/review/__tests__/walkthroughRender.test.jsx index ac90e5da..842df9a6 100644 --- a/static/src/review/__tests__/walkthroughRender.test.jsx +++ b/static/src/review/__tests__/walkthroughRender.test.jsx @@ -223,4 +223,10 @@ describe("walkthrough pieces", () => { expect(commentOutcome({ ok: false, status: 409, data: { detail: "no pane" } })).toEqual({ close: true, note: "saved, no pane" }); expect(commentOutcome({ ok: false, status: 500, data: {} })).toEqual({ close: false, note: "HTTP 500" }); }); + it("commentOutcome reads only a string detail: FastAPI's 422 detail is a list", () => { + const list = [{ loc: ["body", "stop"], msg: "Field required", type: "missing" }]; + expect(commentOutcome({ ok: false, status: 422, data: { detail: list } })).toEqual({ close: false, note: "HTTP 422" }); + expect(commentOutcome({ ok: false, status: 409, data: { detail: list } })).toEqual({ close: true, note: "saved, not delivered" }); + expect(commentOutcome({ ok: false, status: 400, data: { detail: "bad stop" } })).toEqual({ close: false, note: "bad stop" }); + }); }); diff --git a/static/src/review/walkthrough.js b/static/src/review/walkthrough.js index 2a9bfecd..70da2901 100644 --- a/static/src/review/walkthrough.js +++ b/static/src/review/walkthrough.js @@ -132,6 +132,51 @@ export function withMark(reviewed, title, sig) { return next; } +// The reviewed toggles' POSTs, one at a time per stop title, so a double click +// can't land out of order. A click shows at once. When a title's newest click +// settles, the title shows what the server last confirmed, so a failure +// reverts to that, not to a guess captured at click time that an earlier +// failure may have made wrong. `post(title, sig, on)` throws to fail; `show` +// and `fail` are the page's setters (null clears). `pending` and `settled` let +// a refetch tell whether its marks predate a click; `seed` takes a refetch's +// marks as confirmed once the page applies them. +export function markQueue({ post, show, fail }) { + const tail = new Map(); + const newest = new Map(); + let confirmed = {}; + let clicks = 0; + const q = { + pending: 0, + settled: 0, + seed(reviewed) { + confirmed = { ...reviewed }; + }, + toggle(title, sig, on) { + const id = ++clicks; + newest.set(title, id); + show(title, on ? sig : null); + fail(title, null); + q.pending++; + const run = (tail.get(title) || Promise.resolve()).then(async () => { + try { + await post(title, sig, on); + confirmed = withMark(confirmed, title, on ? sig : null); + if (newest.get(title) === id) fail(title, null); + } catch (e) { + fail(title, `not saved: ${e?.message || e}`); + } finally { + q.pending--; + q.settled++; + if (newest.get(title) === id) show(title, confirmed[title] ?? null); + } + }); + tail.set(title, run); + return run; + }, + }; + return q; +} + // New-side span of a hunk: new_start through new_start + ctx/add lines − 1. function newEnd(h) { const n = (h.lines || []).filter((l) => l.kind === "ctx" || l.kind === "add").length; From b9f1fc253b37e8eeeaff5012b195a74b908bb8a0 Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 14:53:03 -0400 Subject: [PATCH 29/48] reviews walkthrough a11y: a contents press moves focus to the stop (tabindex -1), stop and comment headings demote below the stop's h3, each code column is named for its stop and each reviewed toggle described by the stop title, the contents check is role=img 'reviewed', inert cite chips are aria-disabled (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- static/src/review/Walkthrough.jsx | 41 +++++++++---- .../__tests__/walkthroughRender.test.jsx | 58 ++++++++++++++++++- 2 files changed, 85 insertions(+), 14 deletions(-) diff --git a/static/src/review/Walkthrough.jsx b/static/src/review/Walkthrough.jsx index c91ee5c1..e37f41e9 100644 --- a/static/src/review/Walkthrough.jsx +++ b/static/src/review/Walkthrough.jsx @@ -67,7 +67,7 @@ function Comments({ list }) {
  • {c.ts} {c.resolved ? "resolved" : "open"} -
    {renderMarkdown(c.body, { demote: false, softBreaks: "space" })}
    +
    {renderMarkdown(c.body, { demote: true, softBreaks: "space" })}
    • ))} @@ -157,11 +157,14 @@ function CommentBox({ repo, branch, stop, onPosted }) { // One accessible name in both states: aria-pressed carries the state, and the ✓ // is decoration (a changing label plus "pressed" would be announced twice). -export function ReviewedToggle({ on, onToggle, error }) { +// `describedBy` is the stop title's id: thirteen buttons all named "reviewed" +// need it to tell them apart. +export function ReviewedToggle({ on, onToggle, error, describedBy }) { return ( {error ? {error} : null} - @@ -186,11 +189,11 @@ export class MemoHunk extends Component { // The right column (THI-443 D5): the stop's files, one summary line for the // ones with nothing to show, then the hunks expanded, except a block past // COLLAPSE_LINES, which waits behind a button. -function Evidence({ ev, hl, colRef, expanded, onGo, onExpand }) { +function Evidence({ ev, title, hl, colRef, expanded, onGo, onExpand }) { const summary = hiddenSummary(ev.hidden); if (!ev.shown.length && !summary) return
    ; return ( -
    - + )} @@ -311,11 +321,18 @@ function Contents({ stops, reviewed }) {
  • {/* A button, not a #link: the URL and periscope's routing stay put. */}
    • ))} diff --git a/static/src/review/__tests__/walkthroughRender.test.jsx b/static/src/review/__tests__/walkthroughRender.test.jsx index 842df9a6..761769a3 100644 --- a/static/src/review/__tests__/walkthroughRender.test.jsx +++ b/static/src/review/__tests__/walkthroughRender.test.jsx @@ -16,7 +16,7 @@ Panes cost more and \`nothing\` said so. ## Adds a gauge > kind: system-touch · refs: a/one.py, b/missing.py -Prose one. +Prose one. See \`a/one.py:1\` and \`b/missing.py\`. \`\`\`mermaid flowchart LR @@ -107,7 +107,7 @@ describe(" body", () => { it("folds a reviewed stop to its title row, and only while its sig matches", () => { const folded = view({}, { reviewed: { "Adds a gauge": firstSig } }); expect(folded).toContain('class="wt-stop wt-kind-system-touch is-folded"'); - expect(folded).toContain('class="wt-fold"'); + expect(folded).toMatch(/'); + expect(html).toContain( + '', + ); + // No worktree: nothing to scroll to, so every chip is inert. + const bare = view({ diff: null, stale_by: null }); + expect(count(bare, 'class="md-cite is-inert"')).toBe(2); + expect(bare).not.toContain('class="md-cite"'); + }); + it("names each stop's code column and describes its toggle by the stop's title", () => { + const html = view(); + expect(html).toContain('aria-label="files and code changes: Adds a gauge"'); + expect(html).toContain('

    '); + expect(html).toContain('aria-describedby="stop-adds-a-gauge-title" aria-pressed="false">reviewed'); + expect(html).toContain('aria-describedby="stop-reuses-the-cache-title"'); + }); + it("gives the contents ✓ a name", () => { + expect(view({}, { reviewed: { "Adds a gauge": firstSig } })) + .toContain('✓'); + }); + it("demotes headings in a stop and its comments below the stop's own h3", () => { + const text = "# T\n\n## Stop\n> kind: reuse\n\nBody.\n\n### Detail\n"; + const html = view({ text, comments: [{ ts: "t", stop: "Stop", resolved: false, body: "## Fix\n\nthis" }] }); + expect(html).toContain('

    Detail
    '); + expect(html).toContain('

    Fix

    '); + }); + it("moves focus to the stop a contents entry scrolls to", () => { + let entry; + const calls = []; + const el = { scrollIntoView: (o) => calls.push(["scroll", o.block]), focus: (o) => calls.push(["focus", o.preventScroll]) }; + const prev = options.vnode; + const hadDoc = "document" in globalThis; + const prevDoc = globalThis.document; + options.vnode = (v) => { + prev?.(v); + if (v.props?.class === "wt-toc-item wt-kind-system-touch") entry = v; + }; + globalThis.document = { getElementById: (id) => (id === "stop-adds-a-gauge" ? el : null) }; + try { + // Focusable by script only, so the stop takes focus without joining the tab order. + expect(view()).toContain('id="stop-adds-a-gauge" tabindex="-1"'); + entry.props.onClick(); + expect(calls).toEqual([["scroll", "start"], ["focus", true]]); + } finally { + options.vnode = prev; + if (hadDoc) globalThis.document = prevDoc; + else delete globalThis.document; + } + }); +}); + describe(" comments", () => { const comment = (stop, body, resolved = false) => ({ ts: "2026-10-29 10:00", stop, resolved, body }); const comments = [comment("Adds a gauge", "why?"), comment("Adds a gauge", "fixed", true), comment("Gone stop", "orphan")]; From f0e9cc00837ecebee02515cd01ef40632d72fd90 Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 14:54:45 -0400 Subject: [PATCH 30/48] reviews walkthrough: a failed refresh shows in the fixed header (an always-present role=status line), code columns blame the worktree only when the badge does, no empty problem block, 'no stops in this walkthrough', and reveal asks the column's overflow so a static column scrolls the page; tests pin the header PR link (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- static/src/review/Walkthrough.jsx | 53 ++++++++----- .../__tests__/walkthroughRender.test.jsx | 75 ++++++++++++++++++- 2 files changed, 107 insertions(+), 21 deletions(-) diff --git a/static/src/review/Walkthrough.jsx b/static/src/review/Walkthrough.jsx index e37f41e9..ce23cab2 100644 --- a/static/src/review/Walkthrough.jsx +++ b/static/src/review/Walkthrough.jsx @@ -48,9 +48,12 @@ const kindOf = (stop) => (stop.knownKind ? stop.kind : "other"); // Bring a file block into view and flash it. Wide, the column scrolls on its // own, so move it rather than the page. Otherwise bring the file into view only // if it isn't already ("nearest"), so a column that fits on screen never jumps -// the page (D5). -function reveal(c, el) { - if (c.scrollHeight > c.clientHeight) c.scrollTo({ top: el.offsetTop - 8, behavior: motion() }); +// the page (D5). Narrow, the column is static: asked by its overflow, not its +// heights, so it always takes the page path. +export function reveal(c, el) { + if (getComputedStyle(c).overflowY !== "visible" && c.scrollHeight > c.clientHeight) { + c.scrollTo({ top: el.offsetTop - 8, behavior: motion() }); + } else el.scrollIntoView({ block: "nearest", behavior: motion() }); // Restart the flash even on a second click of the same file: the reflow // between remove and add makes the browser replay the animation. @@ -189,7 +192,7 @@ export class MemoHunk extends Component { // The right column (THI-443 D5): the stop's files, one summary line for the // ones with nothing to show, then the hunks expanded, except a block past // COLLAPSE_LINES, which waits behind a button. -function Evidence({ ev, title, hl, colRef, expanded, onGo, onExpand }) { +function Evidence({ ev, title, noWorktree, hl, colRef, expanded, onGo, onExpand }) { const summary = hiddenSummary(ev.hidden); if (!ev.shown.length && !summary) return
    ; return ( @@ -222,7 +225,9 @@ function Evidence({ ev, title, hl, colRef, expanded, onGo, onExpand }) { ) : null} - {ev.noDiff ?

    no worktree, so code changes are unavailable

    : ev.shown.map((e, i) => { + {/* diff is also null in a worktree whose diff base wasn't found, so the + worktree gets the blame only when the header's badge does. */} + {ev.noDiff ?

    {noWorktree ? "no worktree, so code changes are unavailable" : "code changes are unavailable"}

    : ev.shown.map((e, i) => { const n = lineCount(e.hunks); return (
    @@ -237,7 +242,7 @@ function Evidence({ ev, title, hl, colRef, expanded, onGo, onExpand }) { ); } -function Stop({ stop, n, diffFiles, comments, repo, branch, hl, reviewed, toggleError, onToggleReviewed, onPosted }) { +function Stop({ stop, n, diffFiles, noWorktree, comments, repo, branch, hl, reviewed, toggleError, onToggleReviewed, onPosted }) { const [unfolded, setUnfolded] = useState(false); // Large file blocks the reader has opened, by index into ev.shown. const [expanded, setExpanded] = useState(() => new Set()); @@ -304,7 +309,8 @@ function Stop({ stop, n, diffFiles, comments, repo, branch, hl, reviewed, toggle
    - + )} @@ -344,9 +350,9 @@ function Contents({ stops, reviewed }) { // stale_by is null both with no worktree (no diff either) and when the // worktree no longer has the recorded head (rebased away); the queue calls the // second "stale: unknown", and so does this. -function Staleness({ staleBy, header, diff }) { +function Staleness({ staleBy, header, diff, noWorktree }) { if (staleBy > 0) return {`stale: ${staleBy} commit${staleBy === 1 ? "" : "s"}`}; - if (staleBy == null && header && !diff) return no worktree; + if (noWorktree) return no worktree; if (staleBy == null && header && diff) return stale: unknown; return null; } @@ -361,6 +367,8 @@ export function WalkthroughView({ data, repo, branch, pr, hl, reviewed, toggleEr const attached = new Set(w.stops.map((s) => s.title)); const unattached = comments.filter((c) => !attached.has(c.stop)); const done = w.stops.filter((s) => isReviewed(s, reviewed)).length; + // The header's badge and every code column say "no worktree" on one rule. + const noWorktree = stale_by == null && !!w.header && !diff; return (
    @@ -371,20 +379,29 @@ export function WalkthroughView({ data, repo, branch, pr, hl, reviewed, toggleEr {w.header?.pr ? (pr?.url && pr.number === w.header.pr ? {`PR #${w.header.pr}`} : {`PR #${w.header.pr}`}) : null} - + {w.stops.length ? {`${done} / ${w.stops.length} reviewed`} : null}
    + {/* In the fixed header, so it shows however far down the reader is. A + status, not an alert: a background refresh isn't the reader's doing. + Present while empty, as a status that mounts with its text is often + not announced. */} +
    {error ? `refresh failed: ${error}` : ""}
    - {error ? : null} -
    -
    {renderMarkdown(w.problemMarkdown, { demote: false, softBreaks: "space" })}
    - -
    + {w.problemMarkdown || w.stops.length ? ( +
    + {w.problemMarkdown + ?
    {renderMarkdown(w.problemMarkdown, { demote: false, softBreaks: "space" })}
    + : null} + +
    + ) : null} + {w.stops.length ? null :

    no stops in this walkthrough

    } {w.stops.map((s, i) => ( - + ))} {unattached.length ? (
    diff --git a/static/src/review/__tests__/walkthroughRender.test.jsx b/static/src/review/__tests__/walkthroughRender.test.jsx index 761769a3..984a9395 100644 --- a/static/src/review/__tests__/walkthroughRender.test.jsx +++ b/static/src/review/__tests__/walkthroughRender.test.jsx @@ -3,8 +3,10 @@ // press the pure pieces (ReviewedToggle) by calling them, as hunk.test.jsx does. import { options } from "preact"; import render from "preact-render-to-string"; -import { describe, expect, it } from "vitest"; -import { CommentForm, commentOutcome, drafts, MemoHunk, ReviewedToggle, sendHint, WalkthroughView } from "../Walkthrough.jsx"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + CommentForm, commentOutcome, drafts, MemoHunk, ReviewedToggle, reveal, sendHint, WalkthroughView, +} from "../Walkthrough.jsx"; import { parseWalkthrough, stopSig } from "../walkthrough.js"; const DOC = `# Gauge @@ -54,6 +56,41 @@ describe(" header", () => { // stale_by null with a diff: the head was rebased away (reviews.stale_by). expect(view({ stale_by: null })).toContain('stale: unknown'); }); + it("links the PR only when the queue's PR is the one the walkthrough names", () => { + const pr = { url: "https://github.com/o/r/pull/12", number: 12, state: "merged" }; + expect(view({}, { pr })).toContain( + 'PR #12', + ); + const other = view({}, { pr: { ...pr, number: 13 } }); + expect(other).toContain("PR #12"); + expect(other).not.toContain("rq-pr-link"); + }); + it("shows a failed refresh in the fixed header, in a status line that is always there", () => { + expect(view()).toMatch(/
    .*
    <\/div><\/header>/); + expect(view({}, { error: "HTTP 500" })) + .toMatch(/
    .*
    refresh failed: HTTP 500<\/div><\/header>/); + }); +}); + +describe(" sparse documents", () => { + it("falls back to the branch for the title and leaves out an absent problem statement", () => { + const html = view({ text: "## Only\n> kind: reuse\n\nBody.\n", stale_by: null }); + expect(html).toContain('

    feat/x

    '); + expect(html).not.toContain("wt-problem"); + expect(html).toContain('Only'); + expect(html).toContain("Body."); + expect(html).not.toContain("wt-empty-stops"); + }); + it("says so when there are no stops", () => { + const html = view({ text: "" }); + expect(html).toContain('

    no stops in this walkthrough

    '); + expect(html).not.toContain("wt-intro"); + expect(html).not.toContain("wt-progress"); + const problemOnly = view({ text: "# T\n\nJust the problem.\n" }); + expect(problemOnly).toContain('class="md-doc wt-problem">

    Just the problem.

    '); + expect(problemOnly).toContain("no stops in this walkthrough"); + expect(problemOnly).not.toContain("wt-toc"); + }); }); describe(" body", () => { @@ -101,9 +138,15 @@ describe(" body", () => { it("lists files without hunks when there is no worktree", () => { const html = view({ diff: null, stale_by: null }); expect(html).toContain('missing.py'); - expect(html).toContain("no worktree, so code changes are unavailable"); + expect(html).toContain('

    no worktree, so code changes are unavailable

    '); expect(html).not.toContain("wt-file-count"); }); + it("blames the worktree for missing code changes only when the header says there is none", () => { + // A worktree whose diff base couldn't be found: staleness known, diff null. + const html = view({ diff: null, stale_by: 0 }); + expect(html).toContain('

    code changes are unavailable

    '); + expect(html).not.toContain("no worktree"); + }); it("folds a reviewed stop to its title row, and only while its sig matches", () => { const folded = view({}, { reviewed: { "Adds a gauge": firstSig } }); expect(folded).toContain('class="wt-stop wt-kind-system-touch is-folded"'); @@ -231,6 +274,32 @@ describe(" comments", () => { }); }); +describe("reveal", () => { + afterEach(() => { vi.unstubAllGlobals(); }); + function setup(overflowY) { + vi.stubGlobal("getComputedStyle", () => ({ overflowY })); + const calls = []; + const col = { scrollHeight: 900, clientHeight: 300, scrollTo: (o) => calls.push(["column", o.top]) }; + const el = { + offsetTop: 400, + scrollIntoView: (o) => calls.push(["page", o.block]), + getBoundingClientRect: () => ({}), + classList: { remove: () => {}, add: (k) => calls.push(["flash", k]) }, + }; + return { col, el, calls }; + } + it("scrolls a column that scrolls on its own, not the page", () => { + const { col, el, calls } = setup("auto"); + reveal(col, el); + expect(calls).toEqual([["column", 392], ["flash", "is-flash"]]); + }); + it("scrolls the page as little as it can when the column is static, however tall", () => { + const { col, el, calls } = setup("visible"); + reveal(col, el); + expect(calls).toEqual([["page", "nearest"], ["flash", "is-flash"]]); + }); +}); + describe("walkthrough pieces", () => { it("MemoHunk redraws a hunk only when one of its props changes", () => { const props = { hunk: diff.files[0].hunks[0], lang: "python", hl: null, onOpen: null }; From 3d703d553c516e0b0331d36aef150f7095016cfd Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 14:55:32 -0400 Subject: [PATCH 31/48] reviews walkthrough: reveal's if/else braced, its narrow-layout comment reworded (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- static/src/review/Walkthrough.jsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/static/src/review/Walkthrough.jsx b/static/src/review/Walkthrough.jsx index ce23cab2..17ca0bfa 100644 --- a/static/src/review/Walkthrough.jsx +++ b/static/src/review/Walkthrough.jsx @@ -48,13 +48,14 @@ const kindOf = (stop) => (stop.knownKind ? stop.kind : "other"); // Bring a file block into view and flash it. Wide, the column scrolls on its // own, so move it rather than the page. Otherwise bring the file into view only // if it isn't already ("nearest"), so a column that fits on screen never jumps -// the page (D5). Narrow, the column is static: asked by its overflow, not its -// heights, so it always takes the page path. +// the page (D5). Narrow, the column is static; asking its overflow, not just +// its heights, sends it down the page path every time. export function reveal(c, el) { if (getComputedStyle(c).overflowY !== "visible" && c.scrollHeight > c.clientHeight) { c.scrollTo({ top: el.offsetTop - 8, behavior: motion() }); + } else { + el.scrollIntoView({ block: "nearest", behavior: motion() }); } - else el.scrollIntoView({ block: "nearest", behavior: motion() }); // Restart the flash even on a second click of the same file: the reflow // between remove and add makes the browser replay the animation. el.classList.remove("is-flash"); From b5b817184b1358bb07c7daf99310102e5416ea03 Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 14:56:34 -0400 Subject: [PATCH 32/48] spec: folded stops show their comment count; hunks are memoized and files over 300 lines start collapsed (review of task 8) (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- .../2026-10-29-walkthrough-two-column-design.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md b/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md index 53eb0726..810559b3 100644 --- a/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md +++ b/docs/superpowers/specs/2026-10-29-walkthrough-two-column-design.md @@ -114,7 +114,11 @@ untouched). unfolds or refolds the stop for this visit without unmarking it; the "reviewed" toggle beside it is a separate button and only marks or unmarks. The unfolded state is local and not saved. A half-typed comment - survives its stop folding and unfolding (D8). + survives its stop folding and unfolding (D8). A folded stop with comments + shows a chip in its title row, e.g. "2 comments · 1 open". The owning + pane answers inside a comment and resolves it without rewriting the stop + (the skill's step 6), so the stop stays folded and its reply would + otherwise go unseen. ### D4. Diagrams render near natural size, in periscope's theme, and open full size @@ -198,7 +202,11 @@ The column (`.wt-evidence`) renders: - **One closing summary line** for `hidden`, e.g. "8 not in this diff · 1 generated", a `
    ` that expands to the paths. - **The hunks,** expanded, under each file's path, drawn by `Hunk` from - `diff/ChangesTab.jsx` with `onOpen={null}`. + `diff/ChangesTab.jsx` with `onOpen={null}`, memoized so a reviewed toggle + doesn't re-highlight them. A file whose hunks total more than 300 lines + (`COLLAPSE_LINES`) starts collapsed behind "show N lines". Whole-file + cites of new files made one real walkthrough mount about 10.7k + highlighted lines. - **Wide panes:** `position: sticky; top: 12px; align-self: start; max-height: calc(100cqh - 24px); overflow: auto`. The column stays beside the text while the stop is on screen, scrolls on its own when taller, and From 4f591d8d68c8511254102f0258b3101435b3eb6d Mon Sep 17 00:00:00 2001 From: Thibault Dody Date: Thu, 29 Oct 2026 15:02:41 -0400 Subject: [PATCH 33/48] reviews walkthrough: open file blocks keyed by ref, not index, so a refetch can't move the open state to another file; stop title ids are stop-__title, which no stop id can equal; cancel is held while a comment sends (THI-443) Co-Authored-By: Claude Opus 5.5 (1M context) --- static/src/review/Walkthrough.jsx | 27 +++++---- .../__tests__/walkthroughRender.test.jsx | 58 +++++++++++++++++-- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/static/src/review/Walkthrough.jsx b/static/src/review/Walkthrough.jsx index 17ca0bfa..bfc8724e 100644 --- a/static/src/review/Walkthrough.jsx +++ b/static/src/review/Walkthrough.jsx @@ -88,7 +88,8 @@ export function CommentForm({ stop, text, busy, textRef, onInput, onSend, onCanc onKeyDown={(e) => { if (e.key === "Enter" && (e.metaKey || e.ctrlKey)) onSend(); }} />
    - + {/* Held while sending: cancel discards the draft, so cancel then a failed send would lose the text. */} + {SEND_HINT}
    @@ -193,7 +194,7 @@ export class MemoHunk extends Component { // The right column (THI-443 D5): the stop's files, one summary line for the // ones with nothing to show, then the hunks expanded, except a block past // COLLAPSE_LINES, which waits behind a button. -function Evidence({ ev, title, noWorktree, hl, colRef, expanded, onGo, onExpand }) { +export function Evidence({ ev, title, noWorktree, hl, colRef, expanded, onGo, onExpand }) { const summary = hiddenSummary(ev.hidden); if (!ev.shown.length && !summary) return
    ; return ( @@ -211,7 +212,7 @@ function Evidence({ ev, title, noWorktree, hl, colRef, expanded, onGo, onExpand
  • {ev.noDiff ? {label} - : } + : }
    • ); })} @@ -233,9 +234,9 @@ function Evidence({ ev, title, noWorktree, hl, colRef, expanded, onGo, onExpand return (
    {e.ref.raw}
    - {n <= COLLAPSE_LINES || expanded.has(i) + {n <= COLLAPSE_LINES || expanded.has(e.ref.raw) ? e.hunks.map((h, j) => ) - : } + : }
    ); })} @@ -245,22 +246,24 @@ function Evidence({ ev, title, noWorktree, hl, colRef, expanded, onGo, onExpand function Stop({ stop, n, diffFiles, noWorktree, comments, repo, branch, hl, reviewed, toggleError, onToggleReviewed, onPosted }) { const [unfolded, setUnfolded] = useState(false); - // Large file blocks the reader has opened, by index into ev.shown. + // Large file blocks the reader has opened, by ref: an index into ev.shown + // would pass the open state to whatever file a refetch put at that index. const [expanded, setExpanded] = useState(() => new Set()); const col = useRef(null); const done = isReviewed(stop, reviewed); const folded = done && !unfolded; const ev = useMemo(() => evidenceFor(stop.refs, diffFiles), [stop.refs, diffFiles]); - const expand = useCallback((i) => setExpanded((s) => (s.has(i) ? s : new Set(s).add(i))), []); + const expand = useCallback((key) => setExpanded((s) => (s.has(key) ? s : new Set(s).add(key))), []); // Stable, so the memoized prose below keeps its cite chips across marks. - const goTo = useCallback((i) => { + // `i` finds the block in this render's column, `key` is the ref it opens. + const goTo = useCallback((i, key) => { const c = col.current; const el = c?.querySelector(`[data-ev="${i}"]`); if (!el) return; // A collapsed block opens first, and the scroll waits for it to draw: // "nearest" aimed at the button alone would leave its code off screen. if (el.querySelector(".wt-ev-more")) { - expand(i); + expand(key); requestAnimationFrame(() => reveal(c, el)); } else reveal(c, el); }, [expand]); @@ -271,7 +274,7 @@ function Stop({ stop, n, diffFiles, noWorktree, comments, repo, branch, hl, revi + onClick={() => { if (i >= 0) goTo(i, ev.shown[i].ref.raw); }}>{c.raw} ); }; // demote: a "### Detail" in a stop would otherwise be an h3, a sibling of @@ -279,7 +282,9 @@ function Stop({ stop, n, diffFiles, noWorktree, comments, repo, branch, hl, revi return renderMarkdown(stop.bodyMarkdown, { demote: true, softBreaks: "space", mermaid: true, meta: true, cite, zoom: true }); }, [stop.bodyMarkdown, ev, goTo]); const title = <>{`${n}.`} {stop.title}; - const titleId = `stop-${stop.id}-title`; + // "__": slug() never emits "_", so no stop's own id (a stop titled "X + // title" is stop-x-title) can collide with this. + const titleId = `stop-${stop.id}__title`; return ( // tabIndex -1: the contents list moves focus here, without the stop // joining the tab order. diff --git a/static/src/review/__tests__/walkthroughRender.test.jsx b/static/src/review/__tests__/walkthroughRender.test.jsx index 984a9395..31e2638a 100644 --- a/static/src/review/__tests__/walkthroughRender.test.jsx +++ b/static/src/review/__tests__/walkthroughRender.test.jsx @@ -4,10 +4,11 @@ import { options } from "preact"; import render from "preact-render-to-string"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { parseCite } from "../../split/mdMeta.js"; import { - CommentForm, commentOutcome, drafts, MemoHunk, ReviewedToggle, reveal, sendHint, WalkthroughView, + CommentForm, commentOutcome, drafts, Evidence, MemoHunk, ReviewedToggle, reveal, sendHint, WalkthroughView, } from "../Walkthrough.jsx"; -import { parseWalkthrough, stopSig } from "../walkthrough.js"; +import { evidenceFor, parseWalkthrough, stopSig } from "../walkthrough.js"; const DOC = `# Gauge @@ -135,6 +136,39 @@ describe(" body", () => { expect(view({ diff: edge })).not.toContain("wt-ev-more"); expect(view({ diff: edge })).toContain('class="dhunk"'); }); + it("keeps a large block open by its ref, wherever a refetch moves it in the list", () => { + const lines = (n) => Array.from({ length: n }, () => ({ kind: "add", text: "x" })); + const small = { path: "a/small.py", hunks: [{ new_start: 1, lines: lines(2) }] }; + const big = { path: "a/big.py", hunks: [{ new_start: 1, lines: lines(301) }] }; + const refs = [parseCite("a/small.py"), parseCite("a/big.py")]; + const pressed = []; + let more; + let row; + const prev = options.vnode; + options.vnode = (v) => { + prev?.(v); + if (v.props?.class === "wt-ev-more") more = v; + if (v.props?.class === "wt-file" && v.props.title === "a/big.py") row = v; + }; + const ev = (files) => evidenceFor(refs, files); + const col = (e, expanded) => render( + pressed.push(["go", i, key])} onExpand={(key) => pressed.push(["open", key])} />, + ); + try { + expect(col(ev([small, big]), new Set())).toContain('class="wt-ev-more"'); + more.props.onClick(); + row.props.onClick(); + expect(pressed).toEqual([["open", "a/big.py"], ["go", 1, "a/big.py"]]); + // Opened, then the refetched diff puts it first: still open, and small stays as it was. + const moved = col(ev([big, small]), new Set(["a/big.py"])); + expect(moved).not.toContain("wt-ev-more"); + expect(count(moved, 'class="dhunk"')).toBe(2); + expect(col(ev([big, small]), new Set(["a/small.py"]))).toContain("show 301 lines"); + } finally { + options.vnode = prev; + } + }); it("lists files without hunks when there is no worktree", () => { const html = view({ diff: null, stale_by: null }); expect(html).toContain('missing.py'); @@ -206,9 +240,17 @@ describe(" cites and accessibility", () => { it("names each stop's code column and describes its toggle by the stop's title", () => { const html = view(); expect(html).toContain('aria-label="files and code changes: Adds a gauge"'); - expect(html).toContain('

    '); - expect(html).toContain('aria-describedby="stop-adds-a-gauge-title" aria-pressed="false">reviewed'); - expect(html).toContain('aria-describedby="stop-reuses-the-cache-title"'); + expect(html).toContain('

    '); + expect(html).toContain('aria-describedby="stop-adds-a-gauge__title" aria-pressed="false">reviewed'); + expect(html).toContain('aria-describedby="stop-reuses-the-cache__title"'); + }); + it("keeps title ids apart from stop ids, whatever the titles", () => { + // Stop "X title" slugs to x-title: with a "-title" suffix, stop X's heading + // would have claimed the same id as that stop's section. + const html = view({ text: "# T\n\n## X\n\na\n\n## X title\n\nb\n" }); + for (const id of ["stop-x", "stop-x__title", "stop-x-title", "stop-x-title__title"]) { + expect(count(html, `id="${id}"`)).toBe(1); + } }); it("gives the contents ✓ a name", () => { expect(view({}, { reviewed: { "Adds a gauge": firstSig } })) @@ -336,6 +378,12 @@ describe("walkthrough pieces", () => { {}} onSend={() => {}} onCancel={() => {}} />, )).toMatch(/