feat: author-colored changed/added blocks in review; unchanged blocks render plain

This commit is contained in:
BenStullsBets
2026-06-26 13:54:24 -07:00
parent 0ad040711f
commit 08557827da
2 changed files with 99 additions and 54 deletions
+33 -15
View File
@@ -841,18 +841,26 @@ function renderReviewOp(
render: (src: string) => string, render: (src: string) => string,
colored: (raw: string) => string, colored: (raw: string) => string,
src: string, src: string,
changedHtml?: string,
): string { ): string {
// removed blocks and any changed block (atomic fences diffed whole; non-atomic prose // A non-atomic changed prose block is pre-rendered with author-colored ins/del
// word-merged) render via renderOp — no author sentinels (deletions neutral, spec §6.7). // (changedHtml). Removed blocks and atomic changed fences stay neutral via renderOp
// `src` is "" for a removed block (no live source — INV-36). // (standalone deletion → neutral, per the adjacency heuristic). `src` is "" for a
// removed block (no live source — INV-36).
if (op.kind === "changed" && !op.atomic && changedHtml !== undefined) {
return `<div class="cw-blk cw-changed"${src}>${changedHtml}</div>`;
}
if (op.kind === "removed" || op.kind === "changed") return renderOp(op, render, src); if (op.kind === "removed" || op.kind === "changed") return renderOp(op, render, src);
return `<div class="cw-blk ${op.kind === "added" ? "cw-added" : "cw-unchanged"}"${src}>${colored(op.block.raw)}</div>`; // "added": use changedHtml (author-colored word-diff from empty before, all insertions).
// "unchanged": colored() returns plain render(raw) — color means "changed since baseline".
return `<div class="cw-blk ${op.kind === "added" ? "cw-added" : "cw-unchanged"}"${src}>${changedHtml ?? colored(op.block.raw)}</div>`;
} }
/** /**
* On-state body (INV-33): the F7 baseline diff — added/changed PROSE author-colored * On-state body: the F7 baseline diff — added/changed PROSE author-colored via
* via colorByAuthor (F9 sentinels), deletions struck — overlaid with F4 pending * wordDiffByAuthor (cw-ins-{author}/cw-del-{author} per token), unchanged blocks render plain,
* proposals as blue cw-proposal blocks (✓/✗). One pass, pure, vscode-free. * deletions struck neutral — overlaid with F4 pending proposals as blue cw-proposal
* blocks (✓/✗). One pass, pure, vscode-free.
* *
* A resolved proposal renders INLINE, right after the current-side block its anchor * A resolved proposal renders INLINE, right after the current-side block its anchor
* falls in (#31) — so "what is Claude proposing, and where?" is answered by * falls in (#31) — so "what is Claude proposing, and where?" is answered by
@@ -903,11 +911,9 @@ export function renderReview(
const ops = diffBlocks(baselineText, landedText); const ops = diffBlocks(baselineText, landedText);
// #48: right after a PIN (baseline reason "pinned") with no changes since, the // #48: right after a PIN (baseline reason "pinned") with no changes since, the
// panel is fully clean: no change marks (already absent) AND no authorship // panel is fully clean: no change marks (already absent) AND no authorship
// coloring, so the pin reads as "this is my clean starting point". Skip // coloring, so the pin reads as "this is my clean starting point". Suppress
// colorByAuthor for every block in this case; data-src mapping and any pending // wordDiffByAuthor for every block in this case; data-src mapping and any pending
// proposals (review actions, not annotations) are kept below. A baseline advanced // proposals (review actions, not annotations) are kept below.
// by a machine-landing (accept) is ALSO zero-diff but is NOT pinned — it keeps
// its authorship coloring (F10 INV-33), so this is gated on the pin specifically.
const clean = opts.pinned === true && ops.every((o) => o.kind === "unchanged"); const clean = opts.pinned === true && ops.every((o) => o.kind === "unchanged");
// Map a currentText offset to its landedText offset (account for reverted spans // Map a currentText offset to its landedText offset (account for reverted spans
@@ -959,9 +965,21 @@ export function renderReview(
for (const op of ops) { for (const op of ops) {
const blockIndex = op.kind === "removed" ? -1 : ci; const blockIndex = op.kind === "removed" ? -1 : ci;
const blk = op.kind === "removed" ? undefined : ranges[ci++]; const blk = op.kind === "removed" ? undefined : ranges[ci++];
const colored = (raw: string): string => // Only ADDED and CHANGED prose blocks are author-colored (they differ since baseline);
blk && !clean ? colorByAuthor(raw, blk.start, landedSpans, render) : render(raw); // UNCHANGED blocks render plain (color means "changed since baseline").
bodyParts.push(renderReviewOp(op, render, colored, srcAttr(blk))); const colored = (raw: string): string => render(raw);
// A non-atomic changed prose block: author-colored word diff (ins by author,
// del by adjacency). An added block: word diff from empty before — all insertions.
// `blk.start` is the landed offset of the after-side text.
const changedHtml =
blk && !clean
? op.kind === "changed" && !op.atomic
? render(wordDiffByAuthor(op.before.raw, op.block.raw, blk.start, landedSpans))
: op.kind === "added"
? render(wordDiffByAuthor("", op.block.raw, blk.start, landedSpans))
: undefined
: undefined;
bodyParts.push(renderReviewOp(op, render, colored, srcAttr(blk), changedHtml));
const here = blockIndex >= 0 ? byBlock.get(blockIndex) : undefined; const here = blockIndex >= 0 ? byBlock.get(blockIndex) : undefined;
if (here) for (const p of here) bodyParts.push(proposalBlockHtml(p, render)); if (here) for (const p of here) bodyParts.push(proposalBlockHtml(p, render));
} }
+66 -39
View File
@@ -1,5 +1,7 @@
import { describe, it, test, expect } from "vitest"; import { describe, it, test, expect } from "vitest";
import MarkdownIt from "markdown-it";
import { splitBlocks, splitBlocksWithRanges, diffBlocks, diffToHunks, diffToBlockHunks, renderTrackChanges, colorByAuthor, authorAt, wordDiffByAuthor, type AuthorSpan } from "../src/trackChangesModel"; import { splitBlocks, splitBlocksWithRanges, diffBlocks, diffToHunks, diffToBlockHunks, renderTrackChanges, colorByAuthor, authorAt, wordDiffByAuthor, type AuthorSpan } from "../src/trackChangesModel";
const md = new MarkdownIt({ html: true, linkify: false, breaks: false });
describe("splitBlocks", () => { describe("splitBlocks", () => {
it("splits prose paragraphs on blank lines, dropping empties", () => { it("splits prose paragraphs on blank lines, dropping empties", () => {
@@ -170,12 +172,11 @@ describe("colorByAuthor", () => {
}); });
// #33: author-coloring must be sentinel-safe around markdown emphasis. These // #33: author-coloring must be sentinel-safe around markdown emphasis. These
// exercise the REAL markdown-it renderer (via renderReview on an unchanged doc), // exercise colorByAuthor directly with the real markdown-it renderer (unchanged
// since the failure is in markdown-it's inline parsing / element nesting. // blocks now render plain in renderReview, so sentinel testing requires a direct call).
// CASE1 — a span boundary strictly inside a delimiter run must not split it. // CASE1 — a span boundary strictly inside a delimiter run must not split it.
test("#33 CASE1: a span boundary inside ** does not break emphasis parsing", () => { test("#33 CASE1: a span boundary inside ** does not break emphasis parsing", () => {
const doc = "a**b**c"; const html = colorByAuthor("a**b**c", 0, [{ start: 0, end: 2, author: "human" }], src => md.render(src));
const html = renderReview(doc, doc, [{ start: 0, end: 2, author: "human" }], []);
expect(html).toContain("<strong>b</strong>"); // emphasis still renders expect(html).toContain("<strong>b</strong>"); // emphasis still renders
expect(html).not.toContain("**"); // no raw delimiters left expect(html).not.toContain("**"); // no raw delimiters left
expect(html).not.toContain("<em></em>"); // no stray empty emphasis (the parse-break symptom) expect(html).not.toContain("<em></em>"); // no stray empty emphasis (the parse-break symptom)
@@ -184,16 +185,14 @@ describe("colorByAuthor", () => {
// CASE3 — a span boundary inside an emphasis run must color the text without // CASE3 — a span boundary inside an emphasis run must color the text without
// misnesting span/element (the span is split at the element boundary). // misnesting span/element (the span is split at the element boundary).
test("#33 CASE3: a span boundary inside **bold** colors the text without misnesting", () => { test("#33 CASE3: a span boundary inside **bold** colors the text without misnesting", () => {
const doc = "**bold**"; const html = colorByAuthor("**bold**", 0, [{ start: 0, end: 4, author: "human" }], src => md.render(src)); // covers "**bo"
const html = renderReview(doc, doc, [{ start: 0, end: 4, author: "human" }], []); // covers "**bo"
expect(html).toContain("<strong>"); expect(html).toContain("<strong>");
expect(html).toContain('<span class="cw-by-human">bo</span>ld</strong>'); // span INSIDE strong, closed before "ld" expect(html).toContain('<span class="cw-by-human">bo</span>ld</strong>'); // span INSIDE strong, closed before "ld"
expect(html).not.toContain('cw-by-human"><strong>'); // NOT the old misnest (span wrapping the <strong> open) expect(html).not.toContain('cw-by-human"><strong>'); // NOT the old misnest (span wrapping the <strong> open)
}); });
// CASE2 — a span covering a whole emphasis run stays correct (regression). // CASE2 — a span covering a whole emphasis run stays correct (regression).
test("#33 CASE2: a span over the whole **bold** colors it correctly", () => { test("#33 CASE2: a span over the whole **bold** colors it correctly", () => {
const doc = "**bold**"; const html = colorByAuthor("**bold**", 0, [{ start: 0, end: 8, author: "human" }], src => md.render(src));
const html = renderReview(doc, doc, [{ start: 0, end: 8, author: "human" }], []);
expect(html).toContain("<strong>"); expect(html).toContain("<strong>");
expect((html.match(/cw-by-human/g) ?? []).length).toBe(1); expect((html.match(/cw-by-human/g) ?? []).length).toBe(1);
expect(html).toContain("bold"); expect(html).toContain("bold");
@@ -263,13 +262,15 @@ describe("renderPlain", () => {
import { renderReview, type ProposalView } from "../src/trackChangesModel"; import { renderReview, type ProposalView } from "../src/trackChangesModel";
describe("renderReview", () => { describe("renderReview", () => {
test("renderReview: human addition since baseline renders green ins / cw-by-human", () => { test("renderReview: human addition since baseline renders author-colored ins", () => {
const html = renderReview("hello", "hello world", [{ start: 6, end: 11, author: "human" }], []); // " world" is one diff token starting at offset 5 (the space), so the author span
expect(html).toMatch(/<ins>[^<]*world[^<]*<\/ins>|cw-by-human/); // must start at 5 or earlier to cover the token's start offset.
const html = renderReview("hello", "hello world", [{ start: 5, end: 11, author: "human" }], []);
expect(html).toContain("cw-ins-human");
}); });
test("renderReview: deletion since baseline renders struck del/cw-del", () => { test("renderReview: a standalone deletion since baseline renders neutral struck del", () => {
const html = renderReview("hello world", "hello", [], []); const html = renderReview("hello world", "hello", [], []);
expect(html).toMatch(/<del>|cw-del/); expect(html).toMatch(/cw-del-none|cw-removed/);
}); });
test("renderReview: a pending proposal renders a blue block with data-proposal-id and Accept/Reject actions", () => { test("renderReview: a pending proposal renders a blue block with data-proposal-id and Accept/Reject actions", () => {
const proposals: ProposalView[] = [{ id: "p1", anchorStart: 0, anchorEnd: 5, replaced: "hello", replacement: "goodbye" }]; const proposals: ProposalView[] = [{ id: "p1", anchorStart: 0, anchorEnd: 5, replaced: "hello", replacement: "goodbye" }];
@@ -301,13 +302,15 @@ describe("renderReview", () => {
test("renderReview: author-colors the correct block when two paragraphs are identical", () => { test("renderReview: author-colors the correct block when two paragraphs are identical", () => {
// Two identical paragraphs; baseline has only the first, so the SECOND is an // Two identical paragraphs; baseline has only the first, so the SECOND is an
// added block authored by human. Its span must color THAT block, not the first. // added block authored by human. Its span must color THAT block, not the first.
// (Unchanged first block renders plain; added second block uses wordDiffByAuthor.)
const baseline = "Hello world"; const baseline = "Hello world";
const current = "Hello world\n\nHello world"; const current = "Hello world\n\nHello world";
// second "Hello world" starts at offset 13; "world" at 19..24 // second "Hello world" starts at offset 13 (past the "\n\n"); span covers the block.
const spans = [{ start: 19, end: 24, author: "human" as const }]; // wordDiffByAuthor emits the whole block as one added token at offset 13.
const spans = [{ start: 13, end: 24, author: "human" as const }];
const html = renderReview(baseline, current, spans, []); const html = renderReview(baseline, current, spans, []);
// exactly one cw-by-human span (the added second block's "world"), not zero, not on the first. // exactly one cw-ins-human (the added second block), not zero, not on the first.
const count = (html.match(/cw-by-human/g) ?? []).length; const count = (html.match(/cw-ins-human/g) ?? []).length;
expect(count).toBe(1); expect(count).toBe(1);
}); });
@@ -386,22 +389,24 @@ describe("renderReview", () => {
expect(html).toContain('data-proposal-id="p1"'); // proposal still shows (it is an action) expect(html).toContain('data-proposal-id="p1"'); // proposal still shows (it is an action)
expect(html).toContain("Person"); expect(html).toContain("Person");
}); });
test("renderReview: zero diff WITHOUT a pin (e.g. machine-landing) keeps authorship coloring (INV-33)", () => { test("renderReview: zero diff (e.g. machine-landing accept) renders unchanged blocks plain (Task 2 behavior)", () => {
// accepting a Claude edit advances the baseline (zero diff) but is NOT a pin — // Task 2: unchanged blocks always render plain — color means "changed since baseline".
// the landed author coloring must remain. // After accepting a Claude edit the text is the new baseline; no diff → no coloring.
const doc = "Human wrote this.\n\nClaude wrote that."; const doc = "Human wrote this.\n\nClaude wrote that.";
const spans: AuthorSpan[] = [{ start: 19, end: doc.length, author: "claude" }]; const spans: AuthorSpan[] = [{ start: 19, end: doc.length, author: "claude" }];
const html = renderReview(doc, doc, spans, []); // no pinned flag const html = renderReview(doc, doc, spans, []); // no pinned flag
expect(html).toContain("cw-by-claude"); expect(html).not.toContain("cw-by-claude");
expect(html).toContain("Claude wrote that."); // text still present
}); });
test("renderReview: with REAL changes since a pin, author coloring returns", () => { test("renderReview: with REAL changes since a pin, author coloring returns on added blocks", () => {
// a genuine added block is still author-colored even when pinned (only the // a genuine added block is still author-colored even when pinned (only the
// zero-diff-after-pin state is clean). // zero-diff-after-pin state is clean). Task 2: coloring is via cw-ins-*.
const baseline = "Hello world"; const baseline = "Hello world";
const current = "Hello world\n\nHello world"; const current = "Hello world\n\nHello world";
const spans: AuthorSpan[] = [{ start: 19, end: 24, author: "human" }]; // span covers the added block from its start (offset 13) so the whole token is colored.
const spans: AuthorSpan[] = [{ start: 13, end: 24, author: "human" }];
const html = renderReview(baseline, current, spans, [], { pinned: true }); const html = renderReview(baseline, current, spans, [], { pinned: true });
expect((html.match(/cw-by-human/g) ?? []).length).toBe(1); expect((html.match(/cw-ins-human/g) ?? []).length).toBe(1);
}); });
test("renderReview is deterministic with mixed anchored/unanchored proposals", () => { test("renderReview is deterministic with mixed anchored/unanchored proposals", () => {
@@ -433,24 +438,26 @@ describe("renderReview", () => {
expect(html).not.toContain("<del>brown</del>"); // no double-render of the change as a baseline diff expect(html).not.toContain("<del>brown</del>"); // no double-render of the change as a baseline diff
}); });
test("renderReview keeps author coloring aligned on a block AFTER a length-changing pending proposal (INV-49/50)", () => { test("renderReview keeps author coloring aligned on an ADDED block AFTER a length-changing pending proposal (INV-49/50)", () => {
// block 1 has a pending proposal whose applied text is MUCH LONGER than its // block 1 has a pending proposal (MUCH LONGER than original); block 2 is a NEW
// original; block 2 carries a Claude authorship span (in currentText coords). // paragraph with a Claude authorship span (in currentText coords). The toLanded
const baseline = "one short.\n\ntwo stable here.\n"; // mapping must shift the span's coords so the added block is correctly colored.
const current = "one MUCH LONGER REPLACED TEXT.\n\ntwo stable here.\n"; const baseline = "old.";
const current = "LONGER REPLACEMENT.\n\nAdded para.";
const proposals: ProposalView[] = [{ const proposals: ProposalView[] = [{
id: "pr_1", id: "pr_1",
anchorStart: current.indexOf("one MUCH LONGER REPLACED TEXT."), anchorStart: 0,
anchorEnd: current.indexOf("one MUCH LONGER REPLACED TEXT.") + "one MUCH LONGER REPLACED TEXT.".length, anchorEnd: "LONGER REPLACEMENT.".length,
replaced: "one short.", replaced: "old.",
replacement: "one MUCH LONGER REPLACED TEXT.", replacement: "LONGER REPLACEMENT.",
original: "one short.", original: "old.",
}]; }];
const sStart = current.indexOf("stable"); // "Added" in currentText starts at: "LONGER REPLACEMENT.\n\n".length = 21
const authorSpans = [{ start: sStart, end: sStart + "stable".length, author: "claude" as const }]; const aStart = current.indexOf("Added");
const authorSpans = [{ start: aStart, end: aStart + "Added".length, author: "claude" as const }];
const html = renderReview(baseline, current, authorSpans, proposals); const html = renderReview(baseline, current, authorSpans, proposals);
// the Claude coloring wraps "stable" exactly — not shifted by the proposal's length delta. // "Added" in the landedText block should be claude-colored despite the coordinate shift.
expect(html).toMatch(/<span class="cw-by-claude">stable<\/span>/); expect(html).toContain("cw-ins-claude");
}); });
test("proposalBlockHtml renders Accept/Reject controls with dropdown carets (#64)", () => { test("proposalBlockHtml renders Accept/Reject controls with dropdown carets (#64)", () => {
@@ -464,6 +471,26 @@ describe("renderReview", () => {
expect(html).toMatch(/Accept/); expect(html).toMatch(/Accept/);
expect(html).toMatch(/Reject/); expect(html).toMatch(/Reject/);
}); });
test("renderReview: a changed prose block colors ins/del by author (claude replace)", () => {
// baseline "light" -> current "dark"; "dark" attributed to claude at 0..4
const html = renderReview("light", "dark", [{ start: 0, end: 4, author: "claude" }], []);
expect(html).toContain('cw-ins-claude');
expect(html).toContain('cw-del-claude'); // adjacency: struck "light" inherits claude
});
test("renderReview: an unchanged block renders plain (no author coloring)", () => {
const doc = "Alpha stays.";
const html = renderReview(doc, doc, [{ start: 0, end: 12, author: "human" }], []);
expect(html).not.toContain("cw-by-");
expect(html).not.toContain("cw-ins-");
});
test("renderReview: an added block is author-colored as an insertion", () => {
// baseline empty-ish -> add a new paragraph authored by human
const html = renderReview("Keep.", "Keep.\n\nFresh line.", [{ start: 6, end: 17, author: "human" }], []);
expect(html).toContain("cw-ins-human");
});
}); });
import { renderTrackChanges as rtc2 } from "../src/trackChangesModel"; import { renderTrackChanges as rtc2 } from "../src/trackChangesModel";