Public: anyone can read this; the crew can change it.
A diff's review-comment row and its resolve control draw §5's card and button, and .dl-review, .rc-resolve* leave the markup #4713
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4706-a-diff-s-review-comment-row-and-its-reso → main 3 commits fork
4 files
changed +68
−42
docs/DESIGN.md +2 −2
| @@ -198,7 +198,7 @@ specification. | ||
| 198 | 198 | | **verdict badge** `verdictBadge()` | `.badge` / `.badge.verdict-stale` | `success` approved · `danger` changes requested · plain commented | a verdict passed before the latest push renders `.verdict-stale`: same words, no colour, and the line says "before the latest push". It is history, not a position — and an approval a push dismissed must never look current | `[✓ approved]` / `[! changes]`, dim when stale | |
| 199 | 199 | | **reviews chip** | `.reviews-chip` | — | one line inside the merge box: `✓ 2 approvals · ! changes requested by bob · an approval is required`. It names people, because a name is somebody to go and talk to and a number is not | the same line under the merge verdict | |
| 200 | 200 | | **review card** | `.card.review-card` (`.review-verdicts`) | — | one form, radios plus an optional body. Which radios appear is the rule made visible: no Approve for the author, none of the gating verdicts for a reader — both refused in `submitReview` too | a three-way prompt with a body field | |
| 201 | | **review thread** | `.dc-review` (`.rc-resolved`, `.rc-hidden`, `.rc-resolve`) | — | open: the comments, then Resolve. Settled: **one line** — `✓ resolved by alice · 2h ago` — with the conversation behind a `<details>` and Unresolve beside it. Resolving is for getting an answered question out of the way of the unanswered ones | a folded thread with a `✓` header | | |
| 201 | | **review thread** | `.dc-review` (a `card()` per thread) | — | open: the comments, then Resolve. Settled: **one line** — `✓ resolved by alice · 2h ago` — with the conversation behind a small `disclosure()` and Unresolve in the card's foot. Resolving is for getting an answered question out of the way of the unanswered ones | a folded thread with a `✓` header | | |
| 202 | 202 | | **propose card** | `fieldset.editor-target.propose` (`.propose-line`) | — | replaces "Where this lands" when the change is a contribution: the whole outcome in one sentence with **both** repository names, then branch, pull title and pull body. There is no "commit directly" radio, because there is no such option | the same sentence above the commit prompt | |
| 203 | 203 | | **run state badge** `runStateBadge()` | `.badge` | `queued` · `running` (attn) · `ok` (success: _Passed_, or _Done_ for an agent run) · `failed` (danger) · `cancelled` (outline) | the one badge a check wears on a pull, a run wears in a list, and a log page wears in its head; the log page's script repaints it on `done` | `[✓ passed]` / `[✗ failed]` / `[· running]` | |
| 204 | 204 | | **env state badge** `envStateBadge()` | `.badge` | `stopped` (outline) · `deploying` (attn) · `running` (success) · `failed` (danger) | what production, or a preview, is doing now; `envTone()` is the same answer as a `dot()` for a list row | the same word in the same tone after the environment's name | |
| @@ -398,7 +398,7 @@ gone, so this list only shrinks. | ||
| 398 | 398 | - **`style/content.ts`, page furniture** — what frames a page's content: page and repo heads, the front page and its tiles, explore, backups, boards, the fleet chart's series colours, lineage: `.bform` `.bgrid` `.bmanage` `.bp-h2` `.bp-h3` `.bp-w2` `.bp-w3` `.break` `.bt-bar` `.bt-grid` `.bt-group` `.bt-hist` `.bt-name` `.bt-procs` `.bt-row` `.bt-val` `.commit-body` `.commit-head` `.domain-records` `.fg-s0` `.fg-s1` `.fg-s2` `.fg-s3` `.fg-s4` `.fg-s5` `.fg-s6` `.fg-s7` `.filters` `.flag-form` `.front-admin` `.front-controls` `.front-crew` `.front-featured` `.front-featured-body` `.front-featured-look` `.front-issues` `.front-live` `.front-more` `.front-pick` `.frozen` `.frozen-form` `.frozen-sentence` `.headroom` `.issue-head` `.issue-meta` `.mirror-form` `.number` `.origin-mark` `.origin-name` `.origin-row` `.owner` `.page-head` `.path-head` `.policy-line` `.qf` `.repo-actions` `.repo-audience` `.repo-desc` `.repo-layout` `.repo-lineage` `.repo-title` `.search` `.slash` `.stage-detail` `.stage-dot` `.steps` `.sub` `.sys-pair` `.table-wrap` `.tok` `.where-sep` |
| 399 | 399 | - **`style/content.ts`, code view** — the source view and the syntax-highlight token classes `highlight-html.ts` builds as `tok-${kind}`: `.blob-view` `.ln` `.tok-attribute` `.tok-comment` `.tok-constant` `.tok-decorator` `.tok-deleted` `.tok-emphasis` `.tok-function` `.tok-heading` `.tok-inserted` `.tok-keyword` `.tok-link` `.tok-meta` `.tok-number` `.tok-operator` `.tok-plain` `.tok-property` `.tok-punctuation` `.tok-regex` `.tok-string` `.tok-tag` `.tok-type` `.tok-variable` |
| 400 | 400 | - **`style/content.ts`, prose (markdown)** — table cell alignment, built by `markdown.ts` as `ta-${align}`: `.ta-c` `.ta-r` |
| 401 | - **`style/content.ts`, diff viewer** — the diff table: its gutters, line kinds, file head and inline review comments: `.add` `.del` `.dh` `.diff` `.diff-body` `.diff-head` `.diff-note` `.diff-path` `.diff-stat` `.diff-stub` `.diff-summary` `.diff-table` `.dl` `.dl-review` `.dn` `.dn-comment` `.nonl` `.rc-hidden` `.rc-resolve` `.rc-resolved` `.wd` | |
| 401 | - **`style/content.ts`, diff viewer** — the diff table: its gutters, line kinds, file head and inline review comments: `.add` `.del` `.dh` `.diff` `.diff-body` `.diff-head` `.diff-note` `.diff-path` `.diff-stat` `.diff-stub` `.diff-summary` `.diff-table` `.dl` `.dn` `.dn-comment` `.nonl` `.wd` | |
| 402 | 402 | - **`style/content.ts`, apps, runs, and the crew** — run pages, their logs and checks (a pull's look is §5's **look**). |
| 403 | 403 | - **`style/content.ts`, search results** — search results, the inbox's ask cards: `.ask-acts` `.ask-card` `.ask-group` `.ask-kind` `.ask-meta` `.ask-more-acts` `.ask-pick` `.ask-pick-hit` `.ask-short` `.ask-waiting` |
| 404 | 404 | - **`pages/board.ts`** — the board itself is §5's **board**; the spend tables `spend.ts` draws wear §5's `.table` and `.stack`, no class of their own |
src/web/diff-view.ts +21 −23
| @@ -21,7 +21,7 @@ import { | ||
| 21 | 21 | import { composeLine } from "./code.ts"; |
| 22 | 22 | import type { Token } from "./highlight.ts"; |
| 23 | 23 | import { escapeHtml, html, raw, when, type Html } from "./html.ts"; |
| 24 | import { icon, time, userLink } from "./ui.ts"; | |
| 24 | import { btn, card, disclosure, icon, time, userLink } from "./ui.ts"; | |
| 25 | 25 | import { renderMarkdown } from "./markdown.ts"; |
| 26 | 26 | import { wordDiff, type Segment } from "./worddiff.ts"; |
| 27 | 27 | |
| @@ -367,11 +367,13 @@ function reviewRows( | ||
| 367 | 367 | // A settled thread collapses to one line: who settled it and when, with |
| 368 | 368 | // the conversation itself one click away. The point of resolving is to get |
| 369 | 369 | // an answered question out of the way of the unanswered ones, and a |
| 370 | // "resolved" badge above the same wall of text does not do that. | |
| 371 | const body = | |
| 372 | state?.resolvedAt != null | |
| 373 | ? html`<div class="rc-resolved"> | |
| 374 | <span class="muted small" | |
| 370 | // "resolved" badge above the same wall of text does not do that. It is the | |
| 371 | // card's muted state; the control to take it back sits in the card's foot. | |
| 372 | const resolved = state?.resolvedAt != null; | |
| 373 | const toggle = threadToggle(opts, state, !resolved); | |
| 374 | const body = resolved | |
| 375 | ? card( | |
| 376 | html`<span class="muted small" | |
| 375 | 377 | >${icon("check")} |
| 376 | 378 | resolved${state.resolvedBy === null |
| 377 | 379 | ? null |
| @@ -381,18 +383,17 @@ function reviewRows( | ||
| 381 | 383 | })}`} |
| 382 | 384 | · ${time(state.resolvedAt)}</span |
| 383 | 385 | > |
| 384 | ${threadToggle(opts, state, false)} | |
| 385 | <details class="rc-hidden"> | |
| 386 | <summary> | |
| 387 | Show ${comments.length} | |
| 388 | ${comments.length === 1 ? "comment" : "comments"} | |
| 389 | </summary> | |
| 390 | ${thread} | |
| 391 | </details> | |
| 392 | </div>` | |
| 393 | : html`${thread}${threadToggle(opts, state, true)}`; | |
| 386 | ${disclosure( | |
| 387 | html`Show ${comments.length} | |
| 388 | ${comments.length === 1 ? "comment" : "comments"}`, | |
| 389 | html`${thread}`, | |
| 390 | { small: true }, | |
| 391 | )}`, | |
| 392 | toggle ? { foot: toggle } : {}, | |
| 393 | ) | |
| 394 | : card(html`${thread}`, toggle ? { foot: toggle } : {}); | |
| 394 | 395 | |
| 395 | return html`<tr class="dl-review"> | |
| 396 | return html`<tr> | |
| 396 | 397 | <td class="dn"></td> |
| 397 | 398 | <td class="dn"></td> |
| 398 | 399 | <td class="dc-review">${body}${form}</td> |
| @@ -408,18 +409,15 @@ function threadToggle( | ||
| 408 | 409 | opts: DiffViewOptions, |
| 409 | 410 | state: ThreadView | undefined, |
| 410 | 411 | resolve: boolean, |
| 411 | ): Html { | |
| 412 | ): Html | undefined { | |
| 412 | 413 | if (state === undefined || !state.mayResolve || !opts.reviewBase) |
| 413 | return html``; | |
| 414 | return undefined; | |
| 414 | 415 | const verb = resolve ? "resolve" : "unresolve"; |
| 415 | 416 | return html`<form |
| 416 | class="rc-resolve" | |
| 417 | 417 | method="post" |
| 418 | 418 | action="${opts.reviewBase}/threads/${state.anchor}/${verb}" |
| 419 | 419 | > |
| 420 | <button class="btn" data-size="sm" data-variant="outline" type="submit"> | |
| 421 | ${resolve ? "Resolve" : "Unresolve"} | |
| 422 | </button> | |
| 420 | ${btn(resolve ? "Resolve" : "Unresolve", { size: "sm", variant: "outline" })} | |
| 423 | 421 | </form>`; |
| 424 | 422 | } |
| 425 | 423 |
src/web/style/content.ts +4 −17
| @@ -569,28 +569,15 @@ form.inline { display: inline; margin-left: auto; } | ||
| 569 | 569 | } |
| 570 | 570 | .dn-comment:hover { color: var(--accent-fg); background: var(--accent); border-radius: 4px; } |
| 571 | 571 | /* review comments ride under their line, full width */ |
| 572 | .dl-review .dc-review { | |
| 572 | .dc-review { | |
| 573 | 573 | padding: 8px 12px; background: var(--muted); |
| 574 | 574 | border-left: 3px solid var(--accent); white-space: normal; font-family: var(--font); |
| 575 | 575 | } |
| 576 | .dl-review .comment { margin-bottom: 8px; } | |
| 577 | .dl-review .comment-form { margin-top: 0; } | |
| 576 | .dc-review .card + .card, .dc-review .comment { margin-bottom: 8px; } | |
| 577 | .dc-review .comment-form { margin-top: 0; } | |
| 578 | 578 | /* A settled thread is one line plus a disclosure: the point of resolving is |
| 579 | 579 | to get an answered question out of the way of the unanswered ones. */ |
| 580 | .rc-resolved { display: flex; align-items: center; gap: 8px; flex-wrap: wrap; } | |
| 581 | .rc-resolved .muted { display: inline-flex; align-items: center; gap: 6px; } | |
| 582 | .rc-resolved .ico { width: 14px; height: 14px; color: var(--success); } | |
| 583 | .rc-hidden { flex-basis: 100%; } | |
| 584 | .rc-hidden > summary { | |
| 585 | cursor: pointer; font-size: 13px; color: var(--muted-fg); | |
| 586 | padding: 2px 0; list-style: none; | |
| 587 | } | |
| 588 | .rc-hidden > summary::-webkit-details-marker { display: none; } | |
| 589 | .rc-hidden > summary::before { content: "▸ "; } | |
| 590 | .rc-hidden[open] > summary::before { content: "▾ "; } | |
| 591 | .rc-hidden > summary:hover { color: var(--fg); } | |
| 592 | .rc-hidden[open] { margin-top: 6px; } | |
| 593 | .rc-resolve { display: inline-flex; margin-left: auto; } | |
| 580 | .dc-review .card-body > .muted .ico { width: 14px; height: 14px; color: var(--success); } | |
| 594 | 581 | |
| 595 | 582 | /* ---- review verdicts ---- */ |
| 596 | 583 | /* A verdict passed before the latest push is history, not a position: it |
test/diff-review-comment.test.ts +41
| @@ -64,3 +64,44 @@ describe("inline review comments use the comment component", () => { | ||
| 64 | 64 | expect(out).toContain("data-submit-key"); // Ctrl-Enter sends it |
| 65 | 65 | }); |
| 66 | 66 | }); |
| 67 | ||
| 68 | describe("a review thread is a card and its resolve control a button", () => { | |
| 69 | const anchor = anchorKey("a.txt", "new", 1); | |
| 70 | const render = (resolvedAt: string | null) => | |
| 71 | renderDiff(diff, { | |
| 72 | reviewBase: "/o/r/pulls/1", | |
| 73 | comments: new Map([[anchor, [comment(1, "a point", "alpha")]]]), | |
| 74 | threads: new Map([ | |
| 75 | [anchor, { anchor: "x", resolvedAt, resolvedBy: resolvedAt ? "bob" : null, mayResolve: true }], | |
| 76 | ]), | |
| 77 | }).toString(); | |
| 78 | ||
| 79 | test("an open thread is a card with a Resolve button", () => { | |
| 80 | const out = render(null); | |
| 81 | expect(out).toContain('action="/o/r/pulls/1/threads/x/resolve"'); | |
| 82 | expect(out).toMatch(/<button[^>]*class="btn"[^>]*>\s*Resolve\s*<\/button>/); | |
| 83 | expect(out).toContain('class="card-foot"'); | |
| 84 | }); | |
| 85 | ||
| 86 | test("a resolved thread is one line with an Unresolve button", () => { | |
| 87 | const out = render("2026-01-02T00:00:00Z"); | |
| 88 | expect(out).toContain("resolved by"); | |
| 89 | expect(out).toContain("resolved"); | |
| 90 | expect(out).toContain('action="/o/r/pulls/1/threads/x/unresolve"'); | |
| 91 | expect(out).toMatch(/<button[^>]*class="btn"[^>]*>\s*Unresolve\s*<\/button>/); | |
| 92 | }); | |
| 93 | ||
| 94 | test("a resolved thread hides its conversation in a small disclosure", () => { | |
| 95 | const out = render("2026-01-02T00:00:00Z"); | |
| 96 | expect(out).toContain('<details class="disclosure">'); | |
| 97 | expect(out).toMatch(/<summary class="muted small">\s*Show 1\s+comment\s*<\/summary>/); | |
| 98 | expect(render(null)).not.toContain("disclosure"); | |
| 99 | }); | |
| 100 | ||
| 101 | test("neither state carries the retired names", () => { | |
| 102 | for (const out of [render(null), render("2026-01-02T00:00:00Z")]) { | |
| 103 | expect(out).not.toContain("dl-review"); | |
| 104 | expect(out).not.toContain("rc-resolve"); | |
| 105 | } | |
| 106 | }); | |
| 107 | }); |
new above
offline