Public: anyone can read this; the crew can change it.
Design: a pull's inline review comments use §5's comment, and the rc-* classes leave §5d #3813
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3648-design-a-pull-s-inline-review-comments-u → main 6 commits fork
4 files
changed +81
−24
docs/DESIGN.md +2 −2
Diff not shown — a line 2,062 characters wide (limit 2,000). +2 −2
View filesrc/web/diff-view.ts +9 −8
| @@ -309,22 +309,23 @@ function reviewRows( | ||
| 309 | 309 | const currentHash = hashLine(currentText); |
| 310 | 310 | const thread = comments.map((c) => { |
| 311 | 311 | const outdated = c.lineHash !== currentHash; |
| 312 | return html`<div class="rc"> | |
| 313 | <div class="rc-head"> | |
| 312 | return html`<div class="card comment"> | |
| 313 | <div class="card-head"> | |
| 314 | 314 | ${userLink(c.authorName, { mark: opts.marks?.get(c.authorName) })} |
| 315 | 315 | ${time(c.createdAt)} |
| 316 | 316 | ${when( |
| 317 | 317 | outdated, |
| 318 | () => html`<span class="badge rc-outdated">outdated</span>`, | |
| 318 | () => html`<span class="badge" data-variant="danger">outdated</span>`, | |
| 319 | 319 | )} |
| 320 | 320 | </div> |
| 321 | <div class="prose rc-body">${renderMarkdown(c.body)}</div> | |
| 321 | <div class="comment-body prose">${renderMarkdown(c.body)}</div> | |
| 322 | 322 | </div>`; |
| 323 | 323 | }); |
| 324 | 324 | |
| 325 | 325 | const form = isActive |
| 326 | 326 | ? html`<form |
| 327 | class="rc-form" | |
| 327 | class="card comment-form" | |
| 328 | data-submit-key | |
| 328 | 329 | method="post" |
| 329 | 330 | action="${opts.reviewBase}/review" |
| 330 | 331 | id="rc" |
| @@ -332,10 +333,10 @@ function reviewRows( | ||
| 332 | 333 | <input type="hidden" name="path" value="${path}" /> |
| 333 | 334 | <input type="hidden" name="side" value="${side}" /> |
| 334 | 335 | <input type="hidden" name="line" value="${line}" /> |
| 335 | <label class="sr-only" for="rc-body">Comment on this line</label> | |
| 336 | <label class="sr-only" for="rc-text">Comment on this line</label> | |
| 336 | 337 | <textarea |
| 337 | 338 | class="input" |
| 338 | id="rc-body" | |
| 339 | id="rc-text" | |
| 339 | 340 | name="body" |
| 340 | 341 | rows="3" |
| 341 | 342 | data-grow |
| @@ -370,7 +371,7 @@ function reviewRows( | ||
| 370 | 371 | const body = |
| 371 | 372 | state?.resolvedAt != null |
| 372 | 373 | ? html`<div class="rc-resolved"> |
| 373 | <span class="rc-resolved-line" | |
| 374 | <span class="muted small" | |
| 374 | 375 | >${icon("check")} |
| 375 | 376 | resolved${state.resolvedBy === null |
| 376 | 377 | ? null |
src/web/style/content.ts +4 −14
| @@ -745,23 +745,13 @@ form.inline { display: inline; margin-left: auto; } | ||
| 745 | 745 | padding: 8px 12px; background: var(--muted); |
| 746 | 746 | border-left: 3px solid var(--accent); white-space: normal; font-family: var(--font); |
| 747 | 747 | } |
| 748 | .rc { background: var(--card); border: 1px solid var(--border); border-radius: var(--radius); padding: 8px 12px; margin-bottom: 8px; } | |
| 749 | .rc-head { display: flex; align-items: center; gap: 8px; margin-bottom: 2px; font-size: 13px; } | |
| 750 | .rc-body { font-size: 14px; padding: 0; } | |
| 751 | .rc-body p:first-child { margin-top: 0; } | |
| 752 | .rc-body p:last-child { margin-bottom: 0; } | |
| 753 | .rc-outdated { color: var(--del-fg); border-color: color-mix(in srgb, var(--del-fg) 40%, var(--border)); } | |
| 754 | .rc-form { display: block; } | |
| 755 | .rc-form textarea { width: 100%; } | |
| 756 | ||
| 748 | .dl-review .comment { margin-bottom: 8px; } | |
| 749 | .dl-review .comment-form { margin-top: 0; } | |
| 757 | 750 | /* A settled thread is one line plus a disclosure: the point of resolving is |
| 758 | 751 | to get an answered question out of the way of the unanswered ones. */ |
| 759 | 752 | .rc-resolved { display: flex; align-items: center; gap: 8px; flex-wrap: wrap; } |
| 760 | .rc-resolved-line { | |
| 761 | display: inline-flex; align-items: center; gap: 6px; | |
| 762 | font-size: 13px; color: var(--muted-fg); | |
| 763 | } | |
| 764 | .rc-resolved-line .ico { width: 14px; height: 14px; color: var(--success); } | |
| 753 | .rc-resolved .muted { display: inline-flex; align-items: center; gap: 6px; } | |
| 754 | .rc-resolved .ico { width: 14px; height: 14px; color: var(--success); } | |
| 765 | 755 | .rc-hidden { flex-basis: 100%; } |
| 766 | 756 | .rc-hidden > summary { |
| 767 | 757 | cursor: pointer; font-size: 13px; color: var(--muted-fg); |
test/diff-review-comment.test.ts added +66
| @@ -0,0 +1,66 @@ | ||
| 1 | import { describe, expect, test } from "bun:test"; | |
| 2 | import { anchorKey, hashLine, type ReviewComment } from "../src/core/reviews.ts"; | |
| 3 | import type { Diff } from "../src/git/diff.ts"; | |
| 4 | import { renderDiff } from "../src/web/diff-view.ts"; | |
| 5 | ||
| 6 | const OLD_NAMES = ["rc-body", "rc-form", "rc-head", "rc-outdated", "rc-resolved-line"]; | |
| 7 | ||
| 8 | const diff: Diff = { | |
| 9 | files: [ | |
| 10 | { | |
| 11 | status: "modified", oldPath: "a.txt", newPath: "a.txt", path: "a.txt", | |
| 12 | binary: false, additions: 2, deletions: 0, similarity: null, | |
| 13 | hunks: [ | |
| 14 | { | |
| 15 | oldStart: 1, oldLines: 0, newStart: 1, newLines: 2, section: "", | |
| 16 | lines: [ | |
| 17 | { kind: "add", oldNumber: null, newNumber: 1, content: "alpha", noNewline: false }, | |
| 18 | { kind: "add", oldNumber: null, newNumber: 2, content: "beta", noNewline: false }, | |
| 19 | ], | |
| 20 | }, | |
| 21 | ], | |
| 22 | }, | |
| 23 | ], | |
| 24 | additions: 2, | |
| 25 | deletions: 0, | |
| 26 | }; | |
| 27 | ||
| 28 | const comment = (line: number, body: string, content: string): ReviewComment => ({ | |
| 29 | id: `c${line}`, authorName: "alice", body, createdAt: "2026-01-01T00:00:00Z", | |
| 30 | path: "a.txt", line, side: "new", commitOid: "abc", lineHash: hashLine(content), | |
| 31 | originAuthor: null, | |
| 32 | }); | |
| 33 | ||
| 34 | describe("inline review comments use the comment component", () => { | |
| 35 | const out = renderDiff(diff, { | |
| 36 | reviewBase: "/o/r/pulls/1", | |
| 37 | comments: new Map([ | |
| 38 | [anchorKey("a.txt", "new", 1), [comment(1, "open **question**", "alpha")]], | |
| 39 | [anchorKey("a.txt", "new", 2), [comment(2, "stale remark", "changed")]], | |
| 40 | ]), | |
| 41 | threads: new Map([ | |
| 42 | [anchorKey("a.txt", "new", 2), { anchor: "x", resolvedAt: "2026-01-02T00:00:00Z", resolvedBy: "bob", mayResolve: false }], | |
| 43 | ]), | |
| 44 | active: { path: "a.txt", side: "new", line: 1 }, | |
| 45 | }).toString(); | |
| 46 | ||
| 47 | test("none of the retired rc-* names is in the page", () => { | |
| 48 | for (const n of OLD_NAMES) expect(out).not.toContain(n); | |
| 49 | }); | |
| 50 | ||
| 51 | test("no element carries the bare `rc` class", () => { | |
| 52 | const classes = [...out.matchAll(/class="([^"]*)"/g)].flatMap((m) => m[1]!.split(/\s+/)); | |
| 53 | expect(classes).not.toContain("rc"); | |
| 54 | }); | |
| 55 | ||
| 56 | test("the text, the outdated mark, the resolved line and the reply form are", () => { | |
| 57 | expect(out).toContain("<strong>question</strong>"); | |
| 58 | expect(out).toContain("stale remark"); | |
| 59 | expect(out).toContain('class="card comment'); | |
| 60 | expect(out).toContain("outdated"); | |
| 61 | expect(out).toContain("resolved"); | |
| 62 | expect(out).toContain('action="/o/r/pulls/1/review"'); | |
| 63 | expect(out).toContain('name="body"'); | |
| 64 | expect(out).toContain("data-submit-key"); // Ctrl-Enter sends it | |
| 65 | }); | |
| 66 | }); |
new above
offline