diff --git a/CLAUDE.md b/CLAUDE.md index adb31a7..741ab9f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -192,6 +192,17 @@ left unwritten. Code and tests win when they disagree. rather than overwriting whatever they edited while the turn ran. The difference is deliberate: a re-review is asking for a new draft, a chat turn is asking for an edit to this one +- Don't put a second control over one decision. What Send posts follows the + verdict and the verdict buttons are right above it, so the cockpit has no + "send as…" switch beside the button — the way to post an approve is to say + the review approves. The summary is cerber's own reading of the PR, not + something to hand-edit; the body that posts is the thing worth editing. +- Don't let what posts drift from what the cockpit shows without saying so. + The GitHub body is composed from the draft (summary, walkthrough, the + comments that can't post inline, footer). The one exception is a body the + user wrote by hand in the send panel (`bodyOverride`) — it replaces the + composed one outright, footer included, and every line of that panel that + describes the payload has to say which of the two is about to post. - Don't add an accept step to a revision the user asked for. The chat agent writes the draft directly; Send is where a human vouches for what reaches GitHub, and one pre-chat snapshot is the way back. A per-turn undo is diff --git a/SPEC.md b/SPEC.md index 6a4c7ff..d2479c7 100644 --- a/SPEC.md +++ b/SPEC.md @@ -182,6 +182,7 @@ absent on read and materialize with the stated value. | `chapters` | Chapter[] | default `[]` | | `comments` | Comment[] | default `[]` | | `verdict` | Verdict \| null | default `null` | +| `bodyOverride` | string \| null | default `null`; the review body to post, written by hand — null means composed at send time (§14.4) | | `run` | RunInfo \| null | default `null` | | `sent` | SentInfo \| null | default `null` | | `filed` | FiledInfo \| null | default `null`; never set on a sent artifact | @@ -389,9 +390,10 @@ review began at that moment. and next artifacts over a fixed watchlist — appearance in the inbox, status changes, head movement, PR state/draft flips, run start (with its shape: model, source, trust, who asked) and finish (reviewed SHA, cost) or failure, -verdict set/changed, comment churn summarized as one line (added from the -review / written by you / edited / re-graded / dropped / restored / gone), -send, filing, refresh. A generic diff would bury the timeline under a running +verdict set/changed, the body to post leaving the review's composition or +returning to it, comment churn summarized as one line +(added from the review / written by you / edited / re-graded / dropped / +restored / gone), send, filing, refresh. A generic diff would bury the timeline under a running turn's narration, which is rewritten every couple of seconds. **Decision notes.** The poll's deliberate silences — the only kind of history @@ -621,8 +623,10 @@ another look — and then a new head means a new run. ### 8.5 What a Re-Review Destroys and What Survives -A re-review replaces the whole draft — summary, chapters, verdict, and **all -comments, the user's own included**. That is a decision, not a gap: +A re-review replaces the whole draft — summary, chapters, verdict, a +hand-written `bodyOverride` (it described a body for a +draft that no longer exists — the same reason the pre-chat snapshot goes), and +**all comments, the user's own included**. That is a decision, not a gap: half-keeping them (carrying on success, losing on failure) costs the code and still loses the work, so cerber does neither and says so plainly. @@ -1112,7 +1116,8 @@ folded onto the *current* artifact, three-way (`before` = at turn start, the first rule. - `status` is always `current`'s — a "mark reviewed" clicked mid-turn stands. - The verdict is the turn's only if the turn actually revised it; otherwise - `current`'s. + `current`'s. `bodyOverride` is always `current`'s — a turn never writes one, so writing or + clearing one mid-turn is the user's decision and stands. ### 12.6 Snapshot and Reset @@ -1225,6 +1230,16 @@ Payload construction (pure, previewable without side effects): - The body is `## Summary`, then `## Walkthrough` (chapter titles and explanations), then the folded notes, then a fixed footer crediting cerber: "drafted by AI, sent by a human." +- **Unless the user wrote one.** A non-null `bodyOverride` replaces the + composed body outright — footer and folded notes included. Half-honouring it + (keeping a footer they deleted, re-appending notes they cut) would post + something nobody wrote. It is posted **verbatim**: only the composed body is + trimmed, because a hand-written one that opens on an indented line is a + markdown code block, and trimming would silently repaint it as a paragraph. It changes the body alone: inline comments still + post, the event still applies, and the draft it replaced is untouched and + can compose the body again at any time. The split into inline and folded is + still computed, because the cockpit needs it to say which comments have no + line of their own to land on. - Grades are rendered into comment text via the one badge function (§7.4). - `commit_id` is the artifact's `pr.headSha` — so a review of a stale head fails with a 422 rather than landing inline comments on the wrong code. @@ -1311,7 +1326,7 @@ static assets included. No CORS: same-origin only. | `GET /api/reviews` | queue list items | derived counts: comments, drifted, blockers, graded | | `POST /api/reviews` | pull a PR in by URL/ref | **202** + artifact (run started); 200 existing; 409 in flight; 502 fetch failed, nothing left behind | | `GET /api/reviews/:key` | one artifact | 404 | -| `PATCH /api/reviews/:key` | settle | only `reviewed`/`skipped` accepted (§8.1); stamps `settledAt`, clears `filed` | +| `PATCH /api/reviews/:key` | settle · set the verdict · write the body to post | only `reviewed`/`skipped` accepted (§8.1); stamps `settledAt`, clears `filed`; `bodyOverride` takes a string or `null` (back to composed), and **400** on any other type — it is posted verbatim, so coercing `{}` into `"[object Object]"` is worse than refusing it | | `POST/PATCH/DELETE …/comments[/:id]` | comment CRUD | delete is user-origin only in the UI | | `POST …/refresh` | §13.2 | `{stale, changed, …}`; never an error for "nothing to do" | | `POST …/rerun?source=0\|1` | re-review, always forced | **202**; 409 sent; 409 in flight | @@ -1423,6 +1438,11 @@ 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. +- What Send posts follows the verdict, with nothing in between and no second + control for it: the verdict buttons sit directly above the button, so a + "send as…" switch beside it would be two controls over one decision — and + the way to post an approve is to say the review approves. (`cerber send -e` + keeps its own override; a terminal has no verdict buttons above it.) - 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 @@ -1442,6 +1462,19 @@ one is reported over the next. 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 body — GitHub's own comment on the review, the one part of the + payload attached to no line — is writable by hand from the send panel, which + is where the user is standing when they read that it says the wrong thing. + The editor MUST be seeded with the composed body, so writing one starts from + what would otherwise post. Because this is the one place what GitHub gets + stops being derived from what the cockpit shows, the panel MUST say which of + the two is about to be posted wherever it says anything about the payload: + the body strip says who wrote it (never "N folded into the body" over a body + the user may have cut those notes out of), the note under it says the body no + longer follows the summary or the comments, and building it from the review + again is always one click away. A body the user wrote MUST open shown rather + than behind "see what gets posted" — it is the one fact about the payload + that nothing else on the page carries. - 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 diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 77243ca..4b79a64 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -194,8 +194,8 @@ push convinced the guard the draft was current and the poll never re-reviewed that PR again. An artifact written before `reviewedSha` existed has none, and falls back to the old comparison. -**A re-review replaces the whole draft, including comments you wrote.** They -are dropped when the run starts and they do not come back — not on success, not +**A re-review replaces the whole draft, including comments you wrote and a send +body you wrote yourself.** They are dropped when the run starts and they do not come back — not on success, not if the run fails. This is a decision rather than an oversight, and it is stated here rather than left to be discovered: if you have written comments you want to keep, send the review or copy them out before pressing re-review. @@ -414,6 +414,12 @@ Written by other paths: - Every comment edit, add and drop is saved immediately (`editedByUser` is set when you rewrite an AI comment — it is the calibration signal *and* what makes the comment survive a re-review). +- A review body you write yourself (`bodyOverride`, from the send panel) is + stored beside the draft rather than replacing it: the summary, walkthrough and + comments stay as they read, and clearing the field composes the body again. + While it is set, that text is what posts — the folded comments included in the + composed body are only sent if the body you wrote kept them, which is why the + panel stops claiming them. A re-review clears it. - A chat turn writes `pendingChat` **before** it starts (so a reload mid-turn still shows it), streams `pendingChat.progress`, then folds its result onto whatever the artifact says now (`mergeConcurrentEdits`) and appends a diff --git a/src/core/artifact.ts b/src/core/artifact.ts index 6e01f36..9ad0c8d 100644 --- a/src/core/artifact.ts +++ b/src/core/artifact.ts @@ -312,6 +312,19 @@ export const ArtifactSchema = z.object({ chapters: z.array(ChapterSchema).default([]), comments: z.array(CommentSchema).default([]), verdict: VerdictSchema.nullable().default(null), + /** + * The review body to post, written by hand. Null — the normal case — means + * the body is composed from the draft at send time (§14.4: summary, + * walkthrough, the comments that cannot post inline, footer). + * + * Everywhere else in cerber, what GitHub gets is derived from what the + * cockpit shows. This is the one place the user can cut that link, so it is + * a field rather than a rewrite of the summary: the draft underneath stays + * exactly as the review wrote it, the body can always be built again, and + * the send panel says which of the two is about to be posted. A re-review + * clears it — it described a body for a draft that no longer exists. + */ + bodyOverride: z.string().nullable().default(null), run: RunInfoSchema.nullable().default(null), /** Set once the review was sent to GitHub (explicitly, or via opt-in auto-send). */ sent: SentInfoSchema.nullable().default(null), diff --git a/src/core/autosend.test.ts b/src/core/autosend.test.ts index af092f6..bee0ee6 100644 --- a/src/core/autosend.test.ts +++ b/src/core/autosend.test.ts @@ -32,6 +32,7 @@ function makeArtifact(overrides: Partial = {}): Artifact { chapters: [], comments: [], verdict: { recommendation: "approve", confidence: 95, reasoning: "clean" }, + bodyOverride: null, run: null, sent: null, refresh: null, diff --git a/src/core/history.test.ts b/src/core/history.test.ts index f81943b..7bd5aaa 100644 --- a/src/core/history.test.ts +++ b/src/core/history.test.ts @@ -31,6 +31,7 @@ function artifact(over: Partial = {}): Artifact { chapters: [], comments: [], verdict: null, + bodyOverride: null, run: null, sent: null, refresh: null, @@ -89,6 +90,13 @@ describe("describeChange", () => { ]); }); + it("records the body that posts leaving the review, and coming back", () => { + const plain = artifact(); + const own = artifact({ bodyOverride: "Ran it locally, ship it." }); + expect(describeChange(plain, own)).toEqual(["the body to post was written by hand"]); + expect(describeChange(own, plain)).toEqual(["the body to post follows the review again"]); + }); + it("records a push under the review", () => { const before = artifact(); const after = artifact({ pr: { ...before.pr, headSha: "5502944aaaa" } }); diff --git a/src/core/history.ts b/src/core/history.ts index 07e79b2..06d1589 100644 --- a/src/core/history.ts +++ b/src/core/history.ts @@ -176,6 +176,15 @@ export function describeChange(before: Artifact | null, after: Artifact): string ); } + // What posts stopped following the draft, or started following it again. + if ((before?.bodyOverride ?? null) !== (after.bodyOverride ?? null)) { + lines.push( + after.bodyOverride == null + ? "the body to post follows the review again" + : "the body to post was written by hand", + ); + } + const comments = describeComments(before?.comments ?? [], after.comments); if (comments) lines.push(comments); diff --git a/src/core/refresh.test.ts b/src/core/refresh.test.ts index ed428db..cf8dd8a 100644 --- a/src/core/refresh.test.ts +++ b/src/core/refresh.test.ts @@ -66,6 +66,7 @@ function makeArtifact(overrides: Partial = {}): Artifact { chapters: [], comments: [], verdict: null, + bodyOverride: null, run: null, sent: null, refresh: null, diff --git a/src/core/revise.test.ts b/src/core/revise.test.ts index b1febc4..3ecb214 100644 --- a/src/core/revise.test.ts +++ b/src/core/revise.test.ts @@ -48,6 +48,7 @@ function artifact(over: Partial = {}): Artifact { chapters: [{ id: "one", title: "One", explanation: "Explains one.", files: ["src/a.ts"] }], comments: [comment()], verdict: { recommendation: "comment", confidence: 70, reasoning: "because" }, + bodyOverride: null, run: null, sent: null, refresh: null, @@ -331,6 +332,14 @@ describe("mergeConcurrentEdits", () => { expect(merged.verdict).toEqual(mine); }); + it("keeps a send body the user wrote while the turn was running", () => { + // A turn never writes one, so whatever the artifact says now is the user's. + const b = before(); + const { artifact: after } = applyRevisions(b, [{ kind: "summary", body: "Rewritten." }]); + const current = artifact({ ...b, bodyOverride: "My own words." }); + expect(mergeConcurrentEdits(b, after, current).bodyOverride).toBe("My own words."); + }); + it("takes the turn's verdict when the turn did revise it", () => { const b = before(); const theirs = { recommendation: "request_changes" as const, confidence: 80, reasoning: "found one" }; diff --git a/src/core/revise.ts b/src/core/revise.ts index ea6d874..8eed3ae 100644 --- a/src/core/revise.ts +++ b/src/core/revise.ts @@ -91,6 +91,9 @@ export function mergeConcurrentEdits( // there only when it actually revised one. status: current.status, verdict: after.verdict === before.verdict ? current.verdict : after.verdict, + // A turn never writes a send body, so one written while it ran is the + // user's and stands — as does clearing one. + bodyOverride: current.bodyOverride, }; } diff --git a/src/core/send.test.ts b/src/core/send.test.ts index b562233..b146872 100644 --- a/src/core/send.test.ts +++ b/src/core/send.test.ts @@ -44,15 +44,16 @@ function makeArtifact(overrides: Partial = {}): Artifact { chapters: [{ id: "core", title: "Core", explanation: "Adds a const.", files: ["src/a.ts"] }], comments: [], verdict: { recommendation: "comment", confidence: 80, reasoning: "ok" }, + bodyOverride: null, run: null, sent: null, refresh: null, filed: null, settledAt: null, calibration: null, - chat: [], - preChat: null, - pendingChat: null, + chat: [], + preChat: null, + pendingChat: null, ...overrides, }; } @@ -117,6 +118,39 @@ describe("buildReviewPayload", () => { expect(payload.body).toContain("src/a.ts:~2"); }); + it("posts the body the user wrote instead of the composed one", () => { + // The whole body, footer included: a body half-honoured is one nobody + // wrote. The draft underneath is untouched — only what posts changed. + const artifact = makeArtifact({ + bodyOverride: "Looks good to me. I ran the migration locally.\n", + comments: [ + { id: "1", path: "src/a.ts", line: 2, body: "inline ok", chapterId: "core", severity: null, origin: "ai", status: "draft", editedByUser: false, originalLine: null, drifted: false }, + { id: "2", path: "src/a.ts", line: 999, body: "bad line", chapterId: "core", severity: null, origin: "ai", status: "draft", editedByUser: false, originalLine: null, drifted: false }, + ], + }); + const payload = buildReviewPayload(artifact, "APPROVE"); + // Verbatim, trailing newline and all: the panel says this is exactly what + // posts, and a body that opens on an indented line is a markdown code + // block — trimming it would silently repaint it as a paragraph. + expect(payload.body).toBe("Looks good to me. I ran the migration locally.\n"); + expect(payload.body).not.toContain("## Summary"); + expect(payload.body).not.toContain("cerber"); + // Inline comments are a separate half of the payload and still post, and + // the cockpit still needs to know which comments had no line to land on. + expect(payload.comments).toEqual([{ path: "src/a.ts", line: 2, side: "RIGHT", body: "inline ok" }]); + expect(payload.folded.map((c) => c.id)).toEqual(["2"]); + }); + + it("keeps a body that opens on an indented code block", () => { + const body = " const a = 1;\n\nThat is all it needed.\n"; + expect(buildReviewPayload(makeArtifact({ bodyOverride: body }), "COMMENT").body).toBe(body); + }); + + it("composes the body again once the override is cleared", () => { + const payload = buildReviewPayload(makeArtifact({ bodyOverride: null }), "COMMENT"); + expect(payload.body).toContain("## Summary"); + }); + it("anchors the review to the reviewed head commit", () => { expect(buildReviewPayload(makeArtifact(), "COMMENT").commitId).toBe("abc"); const noSha = makeArtifact({ pr: { ...makeArtifact().pr, headSha: "" } }); diff --git a/src/core/send.ts b/src/core/send.ts index 4e33bab..842e7d7 100644 --- a/src/core/send.ts +++ b/src/core/send.ts @@ -76,7 +76,16 @@ export function buildReviewPayload(artifact: Artifact, event: ReviewEvent): Revi return { event, - body: parts.join("\n").trim(), + // A body the user wrote by hand replaces the composed one outright, + // footer and folded notes included: it is the review's own comment, and + // half-honouring it — keeping a footer they deleted, re-appending notes + // they cut — would post something nobody wrote. It is not trimmed either, + // which is not fussiness: a body opening on an indented line is a markdown + // code block, and trimming it would silently repaint it as a paragraph. + // Only the composed body is trimmed — the parts above put a blank line in + // front of it. The composition still runs regardless, because `folded` is + // what the cockpit uses to say which comments have no line to land on. + body: artifact.bodyOverride ?? parts.join("\n").trim(), comments: inline, folded, commitId: artifact.pr.headSha || undefined, diff --git a/src/core/state.test.ts b/src/core/state.test.ts index 91500a5..2a11875 100644 --- a/src/core/state.test.ts +++ b/src/core/state.test.ts @@ -40,6 +40,7 @@ function artifact(over: Partial = {}): Artifact { chapters: [], comments: [], verdict: null, + bodyOverride: null, run: null, sent: null, refresh: null, diff --git a/src/runner/chat.test.ts b/src/runner/chat.test.ts index 4c852fe..234bb7f 100644 --- a/src/runner/chat.test.ts +++ b/src/runner/chat.test.ts @@ -54,6 +54,7 @@ function artifact(over: Partial = {}): Artifact { chapters: [{ id: "one", title: "One", explanation: "Explains one.", files: ["src/a.ts"] }], comments: [comment()], verdict: { recommendation: "comment", confidence: 70, reasoning: "because" }, + bodyOverride: null, run: { model: "opus", startedAt: "2026-08-19T00:00:00Z", diff --git a/src/runner/review-merge.test.ts b/src/runner/review-merge.test.ts index 9b86179..20b604d 100644 --- a/src/runner/review-merge.test.ts +++ b/src/runner/review-merge.test.ts @@ -82,6 +82,7 @@ function ready(comments: Comment[], headSha = "old-sha"): Artifact { chapters: [], comments, verdict: null, + bodyOverride: null, run: null, sent: null, filed: null, diff --git a/src/runner/review.test.ts b/src/runner/review.test.ts index 72df7cb..a9f9d0a 100644 --- a/src/runner/review.test.ts +++ b/src/runner/review.test.ts @@ -56,6 +56,7 @@ function artifact(status: ArtifactStatus, headSha: string): Artifact { chapters: [], comments: [], verdict: null, + bodyOverride: null, run: null, sent: null, filed: null, diff --git a/src/runner/review.ts b/src/runner/review.ts index 18d49ab..95dbdb1 100644 --- a/src/runner/review.ts +++ b/src/runner/review.ts @@ -234,6 +234,9 @@ async function performReview( chapters: [], comments: [], verdict: null, + // Not carried over. A hand-written send body described the draft this run + // is replacing — the same reason the pre-chat snapshot goes. + bodyOverride: null, run: { model: opts.model ?? null, startedAt: now(), diff --git a/src/server/chat.test.ts b/src/server/chat.test.ts index 8452e85..f14641d 100644 --- a/src/server/chat.test.ts +++ b/src/server/chat.test.ts @@ -49,6 +49,7 @@ function artifact(over: Partial = {}): Artifact { chapters: [], comments: [], verdict: { recommendation: "comment", confidence: 70, reasoning: "r" }, + bodyOverride: null, run: null, sent: null, refresh: null, diff --git a/src/server/daemon.ts b/src/server/daemon.ts index 5d8128f..e02610e 100644 --- a/src/server/daemon.ts +++ b/src/server/daemon.ts @@ -151,6 +151,7 @@ export function stubArtifact(ref: DiscoveredPr): Artifact { chapters: [], comments: [], verdict: null, + bodyOverride: null, run: null, sent: null, refresh: null, diff --git a/src/server/guards.test.ts b/src/server/guards.test.ts index 8c05bc9..539f63c 100644 --- a/src/server/guards.test.ts +++ b/src/server/guards.test.ts @@ -58,6 +58,7 @@ function artifact(status: ArtifactStatus): Artifact { chapters: [], comments: [], verdict: { recommendation: "approve", confidence: 90, reasoning: "fine" }, + bodyOverride: null, run: null, sent: null, filed: null, @@ -88,6 +89,15 @@ const patchStatus = async (status: string) => { }); }; +const patchBody = async (body: Record) => { + const app = await buildApp({}); + return app.request(`/api/reviews/${KEY}`, { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +}; + beforeEach(() => { vi.clearAllMocks(); submit.mockResolvedValue({ url: "https://github.com/acme/widgets/pull/42#r1" }); @@ -198,4 +208,34 @@ describe("PATCH /api/reviews/:key — only the statuses that are your decision", expect((await patchStatus(status)).status).toBe(400); } }); + + // The review's own comment, written by hand. It is the one thing GitHub gets + // that is not derived from the draft, so it is stored rather than composed — + // and `null` is how the user hands the body back to the composition. + it("takes a body the user wrote, and gives it back on null", async () => { + await saveArtifact(artifact("ready")); + + expect((await patchBody({ bodyOverride: "Ran it locally, ship it." })).status).toBe(200); + expect((await loadArtifact(ID))!.bodyOverride).toBe("Ran it locally, ship it."); + // The draft it replaces is untouched — this changes what posts, not the review. + expect((await loadArtifact(ID))!.summary).toBe("a draft"); + + expect((await patchBody({ bodyOverride: null })).status).toBe(200); + expect((await loadArtifact(ID))!.bodyOverride).toBeNull(); + // Settling is the other half of this route; writing a body is not a settle. + expect((await loadArtifact(ID))!.status).toBe("ready"); + expect((await loadArtifact(ID))!.settledAt).toBeNull(); + expect(submit).not.toHaveBeenCalled(); + }); + + // Coercion would be worse than a refusal here: `String({})` is + // "[object Object]", and this field is posted to GitHub verbatim. + it("refuses a body that is not a string", async () => { + await saveArtifact(artifact("ready")); + + for (const value of [{ a: 1 }, ["x"], true, 7]) { + expect((await patchBody({ bodyOverride: value })).status).toBe(400); + } + expect((await loadArtifact(ID))!.bodyOverride).toBeNull(); + }); }); diff --git a/src/server/index.ts b/src/server/index.ts index a6bbc49..d5eb7dd 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -309,6 +309,7 @@ export async function buildApp( chapters: [], comments: [], verdict: null, + bodyOverride: null, run: { model: null, startedAt: now, @@ -371,6 +372,15 @@ export async function buildApp( 400, ); } + // Coercing this one would be worse than refusing it: `String({})` is + // "[object Object]", and this field is posted to GitHub verbatim. + if ( + body.bodyOverride !== undefined && + body.bodyOverride !== null && + typeof body.bodyOverride !== "string" + ) { + return c.json({ error: "bodyOverride must be a string, or null to compose it" }, 400); + } const updated = await updateArtifactByKey(c.req.param("key"), (a) => { const next = { ...a }; if (body.status !== undefined) { @@ -386,6 +396,12 @@ export async function buildApp( // the moment you click. Same rule the poll's reopen follows. next.filed = null; } + // The review body as the user rewrote it — or `null` to hand the body + // back to the composition it came from. Only ever set from the send + // panel, where the text being replaced is on screen. + if (body.bodyOverride !== undefined) { + next.bodyOverride = body.bodyOverride; + } if (body.verdictRecommendation !== undefined && next.verdict) { next.verdict = { ...next.verdict, diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index 2b4cf1b..9959988 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -1,5 +1,5 @@ import { html } from "diff2html"; -import { Fragment, ReactNode, useEffect, useMemo, useRef, useState } from "react"; +import { Fragment, ReactNode, useCallback, 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"; @@ -1331,33 +1331,142 @@ function ChatPanel({ ); } +/** + * A textarea that grows to its text, up to most of the window. + * + * The two long-form boxes in a review — the summary and the body that gets + * posted — are paragraphs that wrap, so a row count guessed from newlines + * opens the user's own prose on a scrollbar, with a line cut in half at the + * bottom edge. + */ +function useGrowToFit(value: string) { + const box = useRef(null); + const fit = useCallback(() => { + const el = box.current; + if (!el) return; + el.style.height = "auto"; + el.style.height = `${Math.min(el.scrollHeight + 2, window.innerHeight * 0.7)}px`; + }, []); + useEffect(fit, [value, fit]); + // The cap is a share of the window, so it has to be re-taken when the window + // changes: a box grown in a tall one would otherwise stay taller than the + // short one it now sits in. Its own effect, or the listener would be torn + // down and rebuilt on every keystroke. + useEffect(() => { + window.addEventListener("resize", fit); + return () => window.removeEventListener("resize", fit); + }, [fit]); + return box; +} + +/** + * The review's own comment — the one paragraph GitHub gets that is not attached + * to a line — rewritten by hand. + * + * It is seeded with the composed body and replaces it outright, footer and + * folded notes included: a body half-honoured is one nobody wrote. What it is + * not is an edit of the review — the summary, walkthrough and comments stay + * exactly as they read, and `build it from the review again` is always one + * click away. + */ +function BodyEditor({ + initial, + onSave, + onCancel, +}: { + initial: string; + /** Resolves once the body is written; the caller closes the editor on that. */ + onSave: (text: string) => Promise; + onCancel: () => void; +}) { + const [draft, setDraft] = useState(initial); + const [saving, setSaving] = useState(false); + const [error, setError] = useState(null); + const box = useGrowToFit(draft); + + // This box is the only copy of what was typed — the summary and the comments + // it replaces are still the review's, and nothing else holds these words. So + // it stays open until the write actually lands, and a failed one keeps the + // draft with the reason next to it rather than closing over both. + const save = () => { + // ⌘↵ can be held down, and the button is not the only way in: without this + // a second write goes out while the first is still in flight. + if (saving) return; + setSaving(true); + setError(null); + // Only the failure path comes back here: a save that lands closes the + // editor, so a `finally` would be writing state into an unmounted box. + // `e?.message`: a rejection is not guaranteed to be an Error, and a catch + // that throws leaves the failure unreported — the one outcome this whole + // path exists to prevent. + onSave(draft).catch((e) => { + setError(String(e?.message ?? e)); + setSaving(false); + }); + }; + + return ( +
+