diff --git a/src/trackChangesModel.ts b/src/trackChangesModel.ts index 3326d39..104a533 100644 --- a/src/trackChangesModel.ts +++ b/src/trackChangesModel.ts @@ -841,18 +841,26 @@ function renderReviewOp( render: (src: string) => string, colored: (raw: string) => string, src: string, + changedHtml?: string, ): string { - // removed blocks and any changed block (atomic fences diffed whole; non-atomic prose - // word-merged) render via renderOp — no author sentinels (deletions neutral, spec §6.7). - // `src` is "" for a removed block (no live source — INV-36). + // A non-atomic changed prose block is pre-rendered with author-colored ins/del + // (changedHtml). Removed blocks and atomic changed fences stay neutral via renderOp + // (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 `
${changedHtml}
`; + } if (op.kind === "removed" || op.kind === "changed") return renderOp(op, render, src); - return `
${colored(op.block.raw)}
`; + // "added": use changedHtml (author-colored word-diff from empty before, all insertions). + // "unchanged": colored() returns plain render(raw) — color means "changed since baseline". + return `
${changedHtml ?? colored(op.block.raw)}
`; } /** - * On-state body (INV-33): the F7 baseline diff — added/changed PROSE author-colored - * via colorByAuthor (F9 sentinels), deletions struck — overlaid with F4 pending - * proposals as blue cw-proposal blocks (✓/✗). One pass, pure, vscode-free. + * On-state body: the F7 baseline diff — added/changed PROSE author-colored via + * wordDiffByAuthor (cw-ins-{author}/cw-del-{author} per token), unchanged blocks render plain, + * 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 * 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); // #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 - // coloring, so the pin reads as "this is my clean starting point". Skip - // colorByAuthor for every block in this case; data-src mapping and any pending - // proposals (review actions, not annotations) are kept below. A baseline advanced - // 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. + // coloring, so the pin reads as "this is my clean starting point". Suppress + // wordDiffByAuthor for every block in this case; data-src mapping and any pending + // proposals (review actions, not annotations) are kept below. const clean = opts.pinned === true && ops.every((o) => o.kind === "unchanged"); // Map a currentText offset to its landedText offset (account for reverted spans @@ -959,9 +965,21 @@ export function renderReview( for (const op of ops) { const blockIndex = op.kind === "removed" ? -1 : ci; const blk = op.kind === "removed" ? undefined : ranges[ci++]; - const colored = (raw: string): string => - blk && !clean ? colorByAuthor(raw, blk.start, landedSpans, render) : render(raw); - bodyParts.push(renderReviewOp(op, render, colored, srcAttr(blk))); + // Only ADDED and CHANGED prose blocks are author-colored (they differ since baseline); + // UNCHANGED blocks render plain (color means "changed since baseline"). + 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; if (here) for (const p of here) bodyParts.push(proposalBlockHtml(p, render)); } diff --git a/test/trackChangesModel.test.ts b/test/trackChangesModel.test.ts index 87d8f13..2af831e 100644 --- a/test/trackChangesModel.test.ts +++ b/test/trackChangesModel.test.ts @@ -1,5 +1,7 @@ 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"; +const md = new MarkdownIt({ html: true, linkify: false, breaks: false }); describe("splitBlocks", () => { 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 - // exercise the REAL markdown-it renderer (via renderReview on an unchanged doc), - // since the failure is in markdown-it's inline parsing / element nesting. + // exercise colorByAuthor directly with the real markdown-it renderer (unchanged + // 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. test("#33 CASE1: a span boundary inside ** does not break emphasis parsing", () => { - const doc = "a**b**c"; - const html = renderReview(doc, doc, [{ start: 0, end: 2, author: "human" }], []); + const html = colorByAuthor("a**b**c", 0, [{ start: 0, end: 2, author: "human" }], src => md.render(src)); expect(html).toContain("b"); // emphasis still renders expect(html).not.toContain("**"); // no raw delimiters left expect(html).not.toContain(""); // 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 // misnesting span/element (the span is split at the element boundary). test("#33 CASE3: a span boundary inside **bold** colors the text without misnesting", () => { - const doc = "**bold**"; - const html = renderReview(doc, doc, [{ start: 0, end: 4, author: "human" }], []); // covers "**bo" + const html = colorByAuthor("**bold**", 0, [{ start: 0, end: 4, author: "human" }], src => md.render(src)); // covers "**bo" expect(html).toContain(""); expect(html).toContain('bold'); // span INSIDE strong, closed before "ld" expect(html).not.toContain('cw-by-human">'); // NOT the old misnest (span wrapping the open) }); // CASE2 — a span covering a whole emphasis run stays correct (regression). test("#33 CASE2: a span over the whole **bold** colors it correctly", () => { - const doc = "**bold**"; - const html = renderReview(doc, doc, [{ start: 0, end: 8, author: "human" }], []); + const html = colorByAuthor("**bold**", 0, [{ start: 0, end: 8, author: "human" }], src => md.render(src)); expect(html).toContain(""); expect((html.match(/cw-by-human/g) ?? []).length).toBe(1); expect(html).toContain("bold"); @@ -263,13 +262,15 @@ describe("renderPlain", () => { import { renderReview, type ProposalView } from "../src/trackChangesModel"; describe("renderReview", () => { - test("renderReview: human addition since baseline renders green ins / cw-by-human", () => { - const html = renderReview("hello", "hello world", [{ start: 6, end: 11, author: "human" }], []); - expect(html).toMatch(/[^<]*world[^<]*<\/ins>|cw-by-human/); + test("renderReview: human addition since baseline renders author-colored ins", () => { + // " world" is one diff token starting at offset 5 (the space), so the author span + // 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", [], []); - expect(html).toMatch(/|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", () => { 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", () => { // 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. + // (Unchanged first block renders plain; added second block uses wordDiffByAuthor.) const baseline = "Hello world"; const current = "Hello world\n\nHello world"; - // second "Hello world" starts at offset 13; "world" at 19..24 - const spans = [{ start: 19, end: 24, author: "human" as const }]; + // second "Hello world" starts at offset 13 (past the "\n\n"); span covers the block. + // 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, []); - // exactly one cw-by-human span (the added second block's "world"), not zero, not on the first. - const count = (html.match(/cw-by-human/g) ?? []).length; + // exactly one cw-ins-human (the added second block), not zero, not on the first. + const count = (html.match(/cw-ins-human/g) ?? []).length; 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("Person"); }); - test("renderReview: zero diff WITHOUT a pin (e.g. machine-landing) keeps authorship coloring (INV-33)", () => { - // accepting a Claude edit advances the baseline (zero diff) but is NOT a pin — - // the landed author coloring must remain. + test("renderReview: zero diff (e.g. machine-landing accept) renders unchanged blocks plain (Task 2 behavior)", () => { + // Task 2: unchanged blocks always render plain — color means "changed since baseline". + // 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 spans: AuthorSpan[] = [{ start: 19, end: doc.length, author: "claude" }]; 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 - // 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 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 }); - 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", () => { @@ -433,24 +438,26 @@ describe("renderReview", () => { expect(html).not.toContain("brown"); // 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)", () => { - // block 1 has a pending proposal whose applied text is MUCH LONGER than its - // original; block 2 carries a Claude authorship span (in currentText coords). - const baseline = "one short.\n\ntwo stable here.\n"; - const current = "one MUCH LONGER REPLACED TEXT.\n\ntwo stable here.\n"; + 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 (MUCH LONGER than original); block 2 is a NEW + // paragraph with a Claude authorship span (in currentText coords). The toLanded + // mapping must shift the span's coords so the added block is correctly colored. + const baseline = "old."; + const current = "LONGER REPLACEMENT.\n\nAdded para."; const proposals: ProposalView[] = [{ id: "pr_1", - anchorStart: current.indexOf("one MUCH LONGER REPLACED TEXT."), - anchorEnd: current.indexOf("one MUCH LONGER REPLACED TEXT.") + "one MUCH LONGER REPLACED TEXT.".length, - replaced: "one short.", - replacement: "one MUCH LONGER REPLACED TEXT.", - original: "one short.", + anchorStart: 0, + anchorEnd: "LONGER REPLACEMENT.".length, + replaced: "old.", + replacement: "LONGER REPLACEMENT.", + original: "old.", }]; - const sStart = current.indexOf("stable"); - const authorSpans = [{ start: sStart, end: sStart + "stable".length, author: "claude" as const }]; + // "Added" in currentText starts at: "LONGER REPLACEMENT.\n\n".length = 21 + const aStart = current.indexOf("Added"); + const authorSpans = [{ start: aStart, end: aStart + "Added".length, author: "claude" as const }]; const html = renderReview(baseline, current, authorSpans, proposals); - // the Claude coloring wraps "stable" exactly — not shifted by the proposal's length delta. - expect(html).toMatch(/stable<\/span>/); + // "Added" in the landedText block should be claude-colored despite the coordinate shift. + expect(html).toContain("cw-ins-claude"); }); test("proposalBlockHtml renders Accept/Reject controls with dropdown carets (#64)", () => { @@ -464,6 +471,26 @@ describe("renderReview", () => { expect(html).toMatch(/Accept/); 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";