diff --git a/CLAUDE.md b/CLAUDE.md index 67c70d7..adb31a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -146,7 +146,14 @@ left unwritten. Code and tests win when they disagree. since that machine's tap lands where nobody is looking. `favicon.ts` is the quiet half of the same job: a dot on the tab icon while the inbox holds anything, re-derived on every read of the queue (`fetchReviews`) so no screen - can leave it stale + can leave it stale. `mdblocks.ts` turns a markdown file's patch back into the + document, in blocks that keep their source line numbers — a file the PR adds + opens that way, since there the diff is the file with `+` in front of every + line, and the block numbers are what let the review's comments stay where + they point. A file the PR edits stays a diff (the change is the point) and + says, when read, that it is only the hunks: removals are not in a document, + so the reading view marks the changed blocks, counts the lines it skipped, + and keeps the diff one click away ## Conventions diff --git a/README.md b/README.md index d401a89..8f22811 100644 --- a/README.md +++ b/README.md @@ -148,6 +148,14 @@ line and get the answer in the conversation below. A line the PR removes can be asked about too; a comment on one posts on the file, since GitHub only takes inline comments on the new side of a diff. +A markdown file the PR **adds** opens as the document it is, not as a thousand +rows of `+ ## Heading` — because reviewing a new spec means reading it, and +every line being an addition means the diff's markers say nothing. It is still +the review: the draft comments sit in the document under the paragraph they +point at, and any paragraph takes a comment or a question where it stands. A +markdown file the PR only *edits* stays a diff — there the change is the point +— with "read as a document" in its header when you want the prose instead. + Artifacts are plain JSON you can `cat`, edit, or pipe into anything. The queue is meant to be walked, not clicked through: `j`/`k` move the cursor diff --git a/SPEC.md b/SPEC.md index f09b98c..6a4c7ff 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1396,13 +1396,15 @@ on every queue fetch so no screen can leave it stale. ### 17.5 The Walkthrough Chapters render open — the walkthrough is the point of the page — with one -exception: a chapter holding more than 2,000 diff lines opens folded, and its -header MUST say so and say why ("34,961 diff lines, folded to keep the page +exception: a chapter with more than 2,000 lines to draw opens folded, and its +header MUST say so and say why ("34,961 lines to draw, folded to keep the page quick"). A rendered diff line is a table row and a dozen DOM nodes, so the catch-all chapter of a 368-file PR is 600,000 of them: the browser then spends -its time on layout rather than on the review, and scrolling collapses. The -fold MUST be decided while rendering, not corrected afterwards — folding a -chapter that has already been drawn pays the whole cost it exists to avoid. +its time on layout rather than on the review, and scrolling collapses. A file +that opens as a document (§17.7) is counted at a quarter of its lines, being +drawn a block at a time rather than a row at a time. The fold MUST be decided +while rendering, not corrected afterwards — folding a chapter that has already +been drawn pays the whole cost it exists to avoid. The fold is a default, not a refusal: one click opens it, and the user's choice stands for as long as they are on that review. It does not outlive the @@ -1421,17 +1423,60 @@ one is reported over the next. or request-changes with none), the cockpit points it out and offers a one-click chat turn asking the reviewer to re-true the verdict. It MUST NOT rewrite the verdict itself. +- A comment that cannot post inline — drifted, or with no line at all, which + is what a comment on a line the PR *removed* becomes, GitHub taking inline + comments on the new side only — MUST say so where it is read; nothing else + tells it apart from an inline one. It renders with its file, under that + file's header, whenever the chapter's patch contains the file; only a + comment naming no file in that patch renders loose above the diff. - A sent review renders read-only. Rows filed by cerber are labeled with the filing reason ("reviewed on GitHub"), never with a bare "reviewed" that would read as a click the user never made. - Opening a review triggers refresh (§13.2); a refresh failure is reported softly and the draft still reads. +- Every box the user types markdown into (a comment being edited, a comment + being written, a line composer, the chat input) renders that draft as it + will read, below the box, with no switch and no click. The preview MUST be + produced by the same markdown path as the finished render, so it cannot + drift from it, and a comment's preview MUST carry its grade badge (§7.4) — + that is the body GitHub gets. It MUST be suppressed when the render reads + back word-for-word as the source (whitespace runs flattened): a plain note + is told nothing by a second copy of itself. - The review's history renders as a collapsed card at the foot of the review, newest first — it is what you open when a review is not where you expected it, not part of reading one — with a rail jump that opens it on the way. An absent history MUST read as "predates cerber keeping one", never as an empty log (§5.4). +### 17.7 Markdown Files as Documents + +A markdown file in the diff can be read as the document it is, rendered +through the same markdown path as everything else the cockpit renders. A file +the PR **creates** MUST open that way: there every line is an addition, so the +diff's markers carry no information and cost the reader the document. Every +other file — including a markdown file the PR merely edits — is a diff until +the reader asks, from a control in that file's own header, and either choice +stands for as long as they are on the review (like a chapter's fold, §17.5). + +A document is not a detour off the review, so the reading view MUST keep what +the diff view offers: + +- The review's comments render in it, under the block holding the line each + points at (nearest preceding block for a line that is blank in the source). + No comment may be dropped for want of a place — an unplaceable one renders + after the document. +- A block takes a new comment or a question where it stands, anchored to the + block's first line. + +And it MUST NOT pass off a fragment as the whole: + +- A file the PR only edits carries the hunks alone, so its header says so, the + lines between hunks are marked as skipped and counted, and the blocks the PR + added are marked as changed. In a file the PR creates nothing is marked, + since everything is new. +- What the PR **removed** is not in the document at all. The diff is one click + away and is where that question is answered. + ## 18. CLI `cerber` (version derived from the package — see Appendix B.1): @@ -1712,7 +1757,7 @@ An implementation conforms when all of the following hold: | §14 GitHub | `src/core/gh.ts`, `src/core/trust.ts`, `src/core/send.ts` | | §15 auto-send | `src/core/autosend.ts` | | §16 HTTP API | `src/server/index.ts` | -| §17 cockpit | `web/src/inbox.ts`, `notify.ts`, `favicon.ts`, `review.ts` | +| §17 cockpit | `web/src/inbox.ts`, `notify.ts`, `favicon.ts`, `review.ts`, `Markdown.tsx`, `mdblocks.ts` | | §18 CLI | `src/cli/index.ts` | ## Appendix B. Known Divergences in the Reference Implementation (non-normative) diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index ac6f9aa..2b4cf1b 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -1,5 +1,5 @@ import { html } from "diff2html"; -import { ReactNode, useEffect, useMemo, useRef, useState } from "react"; +import { Fragment, ReactNode, useEffect, useMemo, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { diffLineCounts, patchForFiles, splitDiffByFile, unclaimedFiles } from "../../src/core/diff"; import { withGrade } from "../../src/core/severity"; @@ -21,7 +21,8 @@ import { } from "./api"; import { highlightDiff } from "./highlight"; import { Icon, IconName, Key } from "./Icon"; -import { Markdown } from "./Markdown"; +import { Markdown, MarkdownPreview, renderMarkdown } from "./Markdown"; +import { MdBlock, MdDocument, isMarkdownPath, readMarkdown } from "./mdblocks"; import { walkFrom } from "./inbox"; import { EVENT_LABEL, @@ -166,6 +167,7 @@ function LineComposer({ } }} /> +
+ )} +
+
+ ); +} + +/** + * A markdown file in the diff, read as the document it is. + * + * A PR that adds a spec renders as a thousand rows of `+ ## Heading` — the one + * form in which the document cannot be reviewed, because reviewing it means + * reading it. This is the same content through the same markdown renderer the + * rest of the cockpit uses, and it is not a detour off the review: the review's + * comments sit in it, under the paragraph they point at, and a paragraph you + * want to say something about takes a comment where it stands. + * + * What it cannot show is what a diff shows — the lines the PR took out, and + * which of these lines are new. So for a file the PR merely edits, the changed + * blocks are marked, the parts the diff never carried are marked as missing, + * and the diff itself is one click away. + */ +function MarkdownFile({ + path, + doc, + comments, + renderComment, + readOnly, + chatBusy, + onAddLineComment, + onAskAboutLine, + onShowDiff, +}: { + path: string; + doc: MdDocument; + comments: ReviewComment[]; + renderComment: (c: ReviewComment) => ReactNode; + readOnly: boolean; + chatBusy: boolean; + onAddLineComment: (pick: LinePick, body: string) => void; + onAskAboutLine: (pick: LinePick, message: string) => void; + onShowDiff: () => void; +}) { + const [pick, setPick] = useState(null); + + // Every comment lands under the block that holds its line — or, for a line + // that is blank in the source, under the block it follows. None is dropped: + // a comment the cockpit can't place would be a comment the reader never + // sees, and it still posts at send time. + const placed = useMemo(() => { + const at = new Map(); + const blocks = doc.items.flatMap((item, i) => (item.kind === "block" ? [{ item, i }] : [])); + const rest: ReviewComment[] = []; + for (const c of comments) { + const hit = + c.line == null + ? undefined + : (blocks.filter((b) => b.item.from <= c.line!).pop() ?? blocks[0]); + if (!hit) { + rest.push(c); + continue; + } + at.set(hit.i, [...(at.get(hit.i) ?? []), c]); + } + return { at, rest }; + }, [doc, comments]); + + return ( +
+
+ + {path} + {doc.isNew ? ( + new file + ) : ( + + the diff's lines only — what the PR removed isn't here + + )} + + +
+ {/* About the file rather than a block of it — under the header, where + the diff view puts the same thing. */} + {placed.rest.length > 0 && ( +
{placed.rest.map(renderComment)}
+ )} + {doc.items.map((item, i) => ( + + {item.kind === "gap" ? ( +
+ ⋯ {item.lines.toLocaleString()} line{item.lines === 1 ? "" : "s"} the diff skips +
+ ) : ( + setPick({ path, line: item.from, side: "new" })} + /> + )} + {(placed.at.get(i) ?? []).map(renderComment)} + {item.kind === "block" && pick?.line === item.from && ( + { + onAddLineComment(pick, body); + setPick(null); + }} + onAsk={(message) => { + onAskAboutLine(pick, message); + setPick(null); + }} + onClose={() => setPick(null)} + /> + )} + + ))} +
+ ); +} + +/** + * A chapter's diff: the files in the order the patch lists them, each drawn as + * the thing it is. + * + * Markdown files can be read as documents, and one that the PR *creates* opens + * that way — there, the diff markers carry nothing at all (every line is an + * addition) and cost the reader the document. Everything else, and every + * markdown file the PR merely edits, is a diff until asked otherwise. + */ +function DiffBlock({ + patch, + comments, + renderComment, + readOnly, + chatBusy, + onAddLineComment, + onAskAboutLine, +}: { + patch: string; + comments: ReviewComment[]; + renderComment: (c: ReviewComment) => ReactNode; + readOnly: boolean; + chatBusy: boolean; + onAddLineComment: (pick: LinePick, body: string) => void; + onAskAboutLine: (pick: LinePick, message: string) => void; +}) { + const files = useMemo(() => (patch.trim() ? splitDiffByFile(patch) : []), [patch]); + const docs = useMemo( + () => + new Map( + files + .filter((f) => isMarkdownPath(f.path)) + .map((f) => [f.path, readMarkdown(f.patch)] as const) + .filter(([, doc]) => doc.lines > 0), + ), + [files], + ); + // The reader's choices, by path; absent means the default still stands. Kept + // for as long as they are on this review, like a chapter's fold. + const [choice, setChoice] = useState>({}); + const isReading = (path: string) => choice[path] ?? docs.get(path)?.isNew ?? false; + + const groups: { kind: "diff" | "read"; paths: string[]; patch: string }[] = []; + for (const file of files) { + const last = groups[groups.length - 1]; + if (docs.has(file.path) && isReading(file.path)) { + groups.push({ kind: "read", paths: [file.path], patch: file.patch }); + continue; + } + // Consecutive diff files stay in one diff2html render, so the chapter reads + // in patch order rather than in "documents first, code after". + if (last?.kind === "diff") { + last.paths.push(file.path); + last.patch += `\n${file.patch}`; + } else groups.push({ kind: "diff", paths: [file.path], patch: file.patch }); + } + + if (files.length === 0) return

No diff for this chapter.

; + + const paths = new Set(files.map((f) => f.path)); + const commentsOn = (of: (path: string) => boolean) => comments.filter((c) => of(c.path)); + + return ( + <> + {groups.map((group) => { + const mine = new Set(group.paths); + if (group.kind === "read") { + const path = group.paths[0]!; + return ( + p === path)} + renderComment={renderComment} + readOnly={readOnly} + chatBusy={chatBusy} + onAddLineComment={onAddLineComment} + onAskAboutLine={onAskAboutLine} + onShowDiff={() => setChoice((c) => ({ ...c, [path]: false }))} + /> + ); + } + return ( + mine.has(p))} + renderComment={renderComment} + readOnly={readOnly} + chatBusy={chatBusy} + onAddLineComment={onAddLineComment} + onAskAboutLine={onAskAboutLine} + onRead={(path) => setChoice((c) => ({ ...c, [path]: true }))} + /> + ); + })} + {/* A comment on a file this chapter's patch doesn't contain has no group + to sit in, and dropping it would lose it from the page entirely. */} + {commentsOn((p) => !paths.has(p)).map(renderComment)} ); } @@ -431,13 +755,26 @@ function CommentCard({ {comment.line != null ? `:${comment.line}` : ""} {tag} - {comment.drifted && ( + {comment.drifted ? ( drifted — posts in the body + ) : ( + comment.line == null && ( + // Said out loud, because this is where a comment on a line the PR + // *removed* lands: GitHub takes inline comments on the new side + // only, so it became a comment about the file. Without the label + // it would read as an inline comment that lost its line. + + on the file — posts in the body + + ) )} {!comment.drifted && comment.originalLine != null && comment.originalLine !== comment.line && ( @@ -508,12 +845,19 @@ function CommentCard({ ))}
{editing ? ( -