From 8c445c1c54dccb453d2f81750cdfe26689816cd3 Mon Sep 17 00:00:00 2001 From: Cui Chen Date: Mon, 9 Nov 2026 13:13:44 +0800 Subject: [PATCH 1/2] fix: keep note decisions independent across LaTeX environments --- engine/src/core/changes.ts | 21 ++---- engine/src/core/environment-edits.ts | 82 +++++++++++++++++++++++ engine/tests/ts/fine-diff.test.ts | 48 +++++++++++++ scripts/check-note-option-preparation.mjs | 19 +++++- src/server/note-results.ts | 21 +++++- tests/note-results.test.ts | 51 +++++++++++++- 6 files changed, 221 insertions(+), 21 deletions(-) create mode 100644 engine/src/core/environment-edits.ts diff --git a/engine/src/core/changes.ts b/engine/src/core/changes.ts index aa5106f7..9b254455 100644 --- a/engine/src/core/changes.ts +++ b/engine/src/core/changes.ts @@ -2,6 +2,7 @@ import { Repo, within } from "./git.ts"; import { groups, opcodes } from "./diff.ts"; import { entrySpans } from "./bib.ts"; import { textEdits } from "./text-edits.ts"; +import { groupEnvironmentEdits } from "./environment-edits.ts"; import type { Scope, Unit, Op } from "../types.ts"; export const EXCERPT = 6000; export function textOf(data: Buffer | null): string | null { @@ -73,22 +74,10 @@ function unitsOf( }; const l = offsets(left), r = offsets(right); - let fine = !whole && !file.endsWith(".bib") ? textEdits(a!, b!) : null; - // Environment boundary edits are reviewed together, including unchanged - // text between the opening and closing commands. - const boundary = (text: string) => - /\\(?:begin|end)\b/.test(text) && - !/^\\begin\{([^}]+)\}[^]*\\end\{\1\}$/.test(text); - if ( - fine?.some( - ([, i, j, x, y]) => - boundary(a!.slice(i, j)) || boundary(b!.slice(x, y)), - ) - ) { - const first = fine[0], - last = fine.at(-1)!; - fine = [["replace", first[1], last[2], first[3], last[4]]]; - } + const fine = + !whole && !file.endsWith(".bib") + ? groupEnvironmentEdits(a!, b!, textEdits(a!, b!)) + : null; const lineAt = (offsets: number[], offset: number) => { let lo = 0, hi = offsets.length - 1; diff --git a/engine/src/core/environment-edits.ts b/engine/src/core/environment-edits.ts new file mode 100644 index 00000000..bcf2bf1c --- /dev/null +++ b/engine/src/core/environment-edits.ts @@ -0,0 +1,82 @@ +import type { Op } from "../types.ts"; +import { structuralSource } from "./tex-source.ts"; + +/** Changing an environment's delimiters couples only the edits inside that + * environment. Unrelated prose elsewhere in the file remains independent. */ +export function groupEnvironmentEdits( + before: string, + after: string, + edits: Op[], +): Op[] { + const ranges: [number, number][] = []; + const scan = (text: string, side: 1 | 3) => { + const stack: { name: string; start: number; end: number }[] = []; + const endSide = side === 1 ? 2 : 4; + const touches = (op: Op, start: number, end: number) => + op[side] === op[endSide] + ? start < op[side] && op[side] < end + : op[side] < end && start < op[endSide]; + for (const match of structuralSource(text).matchAll( + /\\(begin|end)\s*\{([^}]+)\}/g, + )) { + const token = { + name: match[2], + start: match.index, + end: match.index + match[0].length, + }; + if (match[1] === "begin") { + stack.push(token); + continue; + } + const opening = stack.at(-1); + if (!opening || opening.name !== token.name) continue; + stack.pop(); + if ( + !edits.some( + (op) => + touches(op, opening.start, opening.end) || + touches(op, token.start, token.end), + ) + ) + continue; + const members = edits.flatMap((op, index) => + touches(op, opening.start, token.end) ? [index] : [], + ); + if (members.length > 1) ranges.push([members[0], members.at(-1)!]); + } + }; + scan(before, 1); + scan(after, 3); + ranges.sort((a, b) => a[0] - b[0]); + const merged: [number, number][] = []; + for (const range of ranges) { + const previous = merged.at(-1); + if (previous && range[0] <= previous[1]) + previous[1] = Math.max(previous[1], range[1]); + else merged.push([...range]); + } + const result: Op[] = []; + for (let index = 0, group = 0; index < edits.length; index++) { + const range = merged[group], + first = edits[index]; + if (!range || range[0] !== index) { + result.push(first); + continue; + } + const last = edits[range[1]]; + result.push([ + first[1] === last[2] + ? "insert" + : first[3] === last[4] + ? "delete" + : "replace", + first[1], + last[2], + first[3], + last[4], + ]); + index = range[1]; + group++; + } + return result; +} diff --git a/engine/tests/ts/fine-diff.test.ts b/engine/tests/ts/fine-diff.test.ts index 813552a4..0babf950 100644 --- a/engine/tests/ts/fine-diff.test.ts +++ b/engine/tests/ts/fine-diff.test.ts @@ -140,6 +140,54 @@ it("keeps multiline math and arguments together even across unchanged source lin ).toBe(next); }); +it("keeps a rewritten display's nested environment separate from surrounding prose", () => { + const old = 'A small example.\n\\[\\begin{aligned}x &= 1 \\\\ y &= 2\\end{aligned}\\]\nThe result is useful.\n'; + const next = old.replace('small', 'short').replace('y &= 2', 'y &= 3').replace('useful', 'practical'); + f.write(METHOD, old); + const before = f.repo.snapshot(['paper']); + f.write(METHOD, next); + const units = diffUnits(f.repo, before, f.repo.snapshot(['paper'])); + expect(units).toHaveLength(3); + expect(units[1].old_text).toBe('\\[\\begin{aligned}x &= 1 \\\\ y &= 2\\end{aligned}\\]'); + expect(applyUnits(Buffer.from(old), Buffer.from(next), [units[0]])?.toString()).toBe(old.replace('small', 'short')); + expect(applyUnits(Buffer.from(old), Buffer.from(next), [units[2]])?.toString()).toBe(old.replace('useful', 'practical')); +}); + +for (const [label, oldBody, newBody] of [ + ['remove a list', '\\begin{enumerate}\n\\item A fast result.\n\\item A stable estimate.\n\\end{enumerate}', 'A fast result.\nA stable estimate.'], + ['add a list', 'A fast result.\nA stable estimate.', '\\begin{enumerate}\n\\item A fast result.\n\\item A stable estimate.\n\\end{enumerate}'], + ['rename a list', '\\begin{enumerate}\n\\item A fast result.\n\\item A stable estimate.\n\\end{enumerate}', '\\begin{itemize}\n\\item A fast result.\n\\item A robust estimate.\n\\end{itemize}'], + ['remove a nested list', '\\begin{quote}\n\\begin{itemize}\n\\item A fast result.\n\\end{itemize}\n\\end{quote}', 'A fast result.'], +]) it(`groups only the affected environment when edits ${label}`, () => { + const old = `A small example.\n\n${oldBody}\n\nThe result is useful.\n`; + const next = old.replace('small', 'short').replace(oldBody, newBody).replace('useful', 'practical'); + f.write(METHOD, old); + const before = f.repo.snapshot(['paper']); + f.write(METHOD, next); + const units = diffUnits(f.repo, before, f.repo.snapshot(['paper'])); + expect(units).toHaveLength(3); + for (let mask = 0; mask < 8; mask++) { + const selected = units.filter((_, index) => mask & (1 << index)); + let expected = old; + if (mask & 1) expected = expected.replace('small', 'short'); + if (mask & 2) expected = expected.replace(oldBody, newBody); + if (mask & 4) expected = expected.replace('useful', 'practical'); + expect(applyUnits(Buffer.from(old), Buffer.from(next), selected)?.toString()).toBe(expected); + expect(revertUnits(Buffer.from(old), Buffer.from(next), units.filter(unit => !selected.includes(unit)))?.toString()).toBe(expected); + } +}); + +it("does not couple prose edits because of delimiters inside comments or literal examples", () => { + const old = 'A small example.\n% \\begin{enumerate}\nA useful result.\n\\verb|\\end{enumerate}|\nA stable estimate.\n'; + const next = old.replace('small', 'short').replace('% \\begin{enumerate}', '% \\begin{itemize}').replace('useful', 'practical').replace('stable', 'robust'); + f.write(METHOD, old); + const before = f.repo.snapshot(['paper']); + f.write(METHOD, next); + const units = diffUnits(f.repo, before, f.repo.snapshot(['paper'])); + expect(units).toHaveLength(4); + expect(applyUnits(Buffer.from(old), Buffer.from(next), [units[2]])?.toString()).toBe(old.replace('useful', 'practical')); +}); + it("does not attach a comment to another change on the same source line", async () => { const old = "The method is fast and the result is useful.\n"; f.write(METHOD, old); diff --git a/scripts/check-note-option-preparation.mjs b/scripts/check-note-option-preparation.mjs index 022fefaf..6d83202f 100644 --- a/scripts/check-note-option-preparation.mjs +++ b/scripts/check-note-option-preparation.mjs @@ -17,8 +17,19 @@ f.write('paper/main.tex', String.raw`\documentclass{book} \end{document} `); f.write(METHOD, String.raw`\chapter{Method} +A small example. + +\[\begin{array}{c}x = 1\end{array}\] + The model is fast and the estimates are stable. The surrounding context remains here. +\begin{enumerate} +\item First check. +\item Second check. +\end{enumerate} + +The surrounding discussion stays unchanged. It separates the independent observations below. + We evaluate the model on three datasets. This is a different note. `); f.write('paper/sections/results.tex', '\\chapter{Results}\n' + 'Unchanged results and their explanation.\n\n'.repeat(120)); @@ -30,6 +41,9 @@ const task = app.tasks.create('options', 'Prepared note options', [], ['paper'], const source = f.read(METHOD); f.edit(task, METHOD, 'fast', 'efficient'); f.edit(task, METHOD, 'three', 'five'); +f.edit(task, METHOD, 'small', 'short'); +f.edit(task, METHOD, 'x = 1', 'x = 2'); +f.edit(task, METHOD, '\\begin{enumerate}\n\\item First check.\n\\item Second check.\n\\end{enumerate}', 'First check.\nSecond check.'); const originalBuild = Builder.prototype.build, builds = []; Builder.prototype.build = function (rev, targets, seed, options) { builds.push({ rev, targets }); @@ -51,7 +65,10 @@ try { assert.ok(builds.every(build => JSON.stringify(build.targets) === '["sections/method"]'), 'No whole-document compilation for options'); const first = results[0], scope = noteComparisonScope(first, [selected.id]); assert.ok(scope.sections.length || scope.loose.length); - assert.ok(scope.units.every(unit => unit.new_text !== 'five'), 'The other note’s separate group stays outside the drawer'); + assert.ok(scope.units.every(unit => unit.new_text !== 'five'), 'The other note’s separate group stays outside the drawer: ' + JSON.stringify(scope.units.map(unit => [unit.id, unit.new_text]))); + assert.ok(scope.units.every(unit => unit.new_text !== 'short'), 'The leading edit does not appear in every note’s alternatives'); + assert.equal(results[0].units.find(unit => unit.id === selected.id).new_text, 'quick'); + assert.equal(results[1].units.find(unit => unit.id === selected.id).new_text, 'computationally efficient'); assert.ok(first.units.some(unit => unit.new_text === 'five'), 'The complete candidate still preserves the other edit'); const paths = results.map(detail => path.join(app.state, 'cache', detail.files.replace('/option-files/', 'option-previews/'), 'preview.json')); assert.ok(paths.every(file => fs.existsSync(file)), 'Both manifests persist on disk'); diff --git a/src/server/note-results.ts b/src/server/note-results.ts index ed93f2af..f4ce0280 100644 --- a/src/server/note-results.ts +++ b/src/server/note-results.ts @@ -57,11 +57,26 @@ function quoteSource(text: string, start: number) { function quotedUnit(note: NoteSummary, unit: NoteRound['units'][number]): boolean | null { if (!unit.text_ops || !unit.old_context || !note.quote.trim()) return null; const { text, start } = unit.old_context, { normalized, letters } = quoteSource(text, start); - const quote = normalize(note.quote), at = normalized.indexOf(quote); + const quote = normalize(note.quote); // Exact units must fail closed when a quote cannot be resolved. Falling back // to its source line would silently accept unrelated edits on that line. - if (!quote || at < 0 || normalized.indexOf(quote, at + 1) >= 0) return false; - const low = letters[at], last = letters[at + quote.length - 1], high = last + String.fromCodePoint(text.codePointAt(last - start)!).length; + if (!quote) return false; + let occurrences: { low: number; high: number }[] = []; + for (let at = normalized.indexOf(quote); at >= 0; at = normalized.indexOf(quote, at + 1)) { + const last = letters[at + quote.length - 1]; + occurrences.push({ low: letters[at], high: last + String.fromCodePoint(text.codePointAt(last - start)!).length }); + } + if (occurrences.length > 1) { + // A short selection can occur elsewhere in the context excerpt. Original + // manuscript lines disambiguate it, but an older review's lines cannot. + const anchor = noteAnchor(note), origin = unit.text_ops[0]?.[1]; + if (anchor.round !== undefined || origin === undefined || origin < start || origin > start + text.length) return false; + const newlines = (a: number, b: number) => (text.slice(a - start, b - start).match(/\n/g) || []).length; + const lineAt = (offset: number) => unit.old_start + (offset < origin ? -newlines(offset, origin) : newlines(origin, offset)); + occurrences = occurrences.filter(({ low, high }) => anchor.sources.some(place => place.file === unit.file && Math.max(place.lines[0], lineAt(low)) <= Math.min(place.lines[1], lineAt(high - 1)))); + } + if (occurrences.length !== 1) return false; + const { low, high } = occurrences[0]; return unit.text_ops.some(([, a, b]) => a === b ? low <= a && a <= high : a < high && b > low); } diff --git a/tests/note-results.test.ts b/tests/note-results.test.ts index 765407fb..16d9b25a 100644 --- a/tests/note-results.test.ts +++ b/tests/note-results.test.ts @@ -7,6 +7,7 @@ import type { NoteSummary, Run, Task, TimelineItem } from '../src/shared.ts'; import type { ReviewClient } from '../src/server/review.ts'; import { diffUnits } from '../engine/src/core/changes.ts'; import { Fixture, fakeRound, METHOD } from '../engine/tests/ts/helpers.ts'; +import { optionCandidate } from '../engine/src/core/option-preview.ts'; const note: NoteSummary = { id: 'n1', where: 'Page 2 · main.tex lines 10–12', quote: 'The original technical sentence.', comment: 'Shorten it.', anchor: { sources: [{ file: 'main.tex', lines: [10, 12] }] } }; @@ -98,13 +99,61 @@ test('formatted PDF quotes select only their exact source edits, and unresolved assert.deepEqual(matchNoteChanges({ ...annotation, quote: 'the estimates are stable' }, round, {}).flatMap(change => change.units), [units[1].id]); assert.deepEqual(matchNoteChanges({ ...annotation, quote: 'Text generated by a macro' }, round, {}), [], 'a failed quote match must not widen to all edits on the source line'); const repeated = { ...round, units: units.map(unit => ({ ...unit, old_context: { text: before + before, start: 0 } })) }; - assert.deepEqual(matchNoteChanges(annotation, repeated, {}), [], 'an ambiguous quote must not widen to all edits either'); + assert.deepEqual(matchNoteChanges({ ...annotation, where: 'main.tex lines 1–2' }, repeated, {}), [], 'an ambiguous quote must not widen to all edits either'); + assert.deepEqual(matchNoteChanges(annotation, repeated, {}).flatMap(change => change.units), [units[0].id], 'the original manuscript line disambiguates repeated text'); for (const command of ['\\label', '\\cite', '\\custom']) { const hidden = { ...round, units: units.map(unit => ({ ...unit, old_context: { text: before + `${command}{hidden phrase}`, start: 0 } })) }; assert.deepEqual(matchNoteChanges({ ...annotation, quote: 'hidden phrase' }, hidden, {}), [], 'macro arguments are not proof of printed text'); } }); +test('repeated short quotes use original source lines, including clipped context excerpts', () => { + const before = 'Header.\nA small example.\nUnchanged.\nAnother small example.\n'; + const after = before.replaceAll('small', 'short'); + const repo = { changedFiles: () => ['main.tex'], blobs: (rev: string) => ({ 'main.tex': Buffer.from(rev === 'old' ? before : after) }) } as unknown as Parameters[0]; + const units = diffUnits(repo, 'old', 'new').map(unit => ({ ...unit, old_context: { text: before.slice(4), start: 4 } })); + const round: NoteRound = { round: 1, units, sections: [], loose: units.map(unit => ({ kind: 'change', units: [unit.id] })) }; + const annotation = { where: 'main.tex line 2', quote: 'small', comment: 'Shorten.' }; + assert.deepEqual(matchNoteChanges(annotation, round, {}).flatMap(change => change.units), [units[0].id]); + assert.deepEqual(matchNoteChanges({ ...annotation, where: 'main.tex line 4' }, round, {}).flatMap(change => change.units), [units[1].id]); + assert.deepEqual(matchNoteChanges({ ...annotation, where: 'main.tex lines 2–4' }, round, {}), []); + assert.deepEqual(matchNoteChanges({ ...annotation, where: 'Round 1 · main.tex line 2' }, round, {}), [], 'review lines refer to a different source revision'); + assert.deepEqual(matchNoteChanges({ ...annotation, where: 'Page 1' }, round, {}), []); + const sameLine = { ...round, units: units.map(unit => ({ ...unit, old_context: { text: before.replaceAll('\n', ' '), start: 0 } })) }; + assert.deepEqual(matchNoteChanges(annotation, sameLine, {}), [], 'two occurrences on the same line remain ambiguous'); +}); + +test('notes beside rewritten math and lists keep independent badges, options and acceptance', async () => { + const fixture = new Fixture(); + const before = 'A small example.\n\n\\[\\begin{aligned}x &= 1 \\\\ y &= 2\\end{aligned}\\]\n\nThe intermediate result is clear.\n\n\\begin{enumerate}\n\\item First check.\n\\item Second check.\n\\end{enumerate}\n\nThe compiler gives immediate feedback.\n\nThe Boolean argument reveals an error.\n'; + const after = before.replace('small', 'short').replace('y &= 2', 'y &= 3').replace('clear', 'concise') + .replace('\\begin{enumerate}\n', '').replaceAll('\\item ', '').replace('\\end{enumerate}\n', '') + .replace('immediate', 'early').replace('an error', 'a type error'); + try { + fixture.write(METHOD, before); + const app = fixture.app(); await app.ready; + const task = app.tasks.create('independent-notes', 'Independent notes', [], ['paper']); + fixture.edit(task, METHOD, before, after); + const published = fakeRound(app, task.name), detail = app.review.roundDetail(task.name, published.number); + // All changes can share a PDF crop without sharing their decision units. + const round: NoteRound = { ...detail, sections: [], loose: [{ kind: 'change', units: detail.units.map(unit => unit.id) }] }; + const quotes = ['small', 'The intermediate result is clear.', 'The compiler gives immediate feedback.', 'The Boolean argument reveals an error.']; + const links = quotes.map(quote => matchNoteChanges({ where: `${METHOD} line ${before.slice(0, before.indexOf(quote)).split('\n').length}`, quote, comment: 'Clarify.' }, round, {}).flatMap(change => change.units)); + assert.ok(links.every(ids => ids.length > 0)); + assert.equal(new Set(links.flat()).size, links.flat().length, 'the four note badges never point at the same unit'); + const selected = published.units.filter(unit => links[1].includes(unit.id)); + assert.equal(selected.length, 1); + const variants = ['brief', 'explicit'].map(text => optionCandidate(published.units, [{ unit: selected[0].id, text }], () => after)); + assert.notEqual(variants[0].files[METHOD]?.toString(), variants[1].files[METHOD]?.toString()); + assert.equal(variants[0].files[METHOD]?.toString(), after.replace('concise', 'brief')); + assert.equal(variants[1].files[METHOD]?.toString(), after.replace('concise', 'explicit')); + await app.review.decide(task.name, { action: 'accept', round: published.number, changes: links[1] }); + assert.equal(fixture.read(METHOD), before.replace('clear', 'concise'), 'accepting note 7 cannot accept note 3, 8, 9 or the math/list'); + const decisions = app.review.decisions(task.name, published); + assert.ok([...links[0], ...links[2], ...links[3]].every(id => decisions[id] === 'pending')); + } finally { await fixture.close(); } +}); + test('short included sources retain exact note identity and decisions across option rounds', async () => { const text = 'A fast dog.\n', unit = { id: 'a', file: 'sentence.tex', old_start: 1, old_end: 1, old_text: 'fast', new_text: 'quick', text_ops: [['replace', 2, 6, 2, 7]] as NoteRound['units'][number]['text_ops'], old_context: { text, start: 0 } }; const rounds: Record = { From 72dd53a45e1781c44098da0573897211e3cb0a78 Mon Sep 17 00:00:00 2001 From: Cui Chen Date: Mon, 9 Nov 2026 13:30:31 +0800 Subject: [PATCH 2/2] fix: focus alternative previews within shared PDF groups --- scripts/check-note-results.mjs | 10 ++++++++- src/web/note-comparison-scope.ts | 33 ++++++++++++++++++++++++----- src/web/note-option-diff.tsx | 12 ++++++++--- tests/note-comparison-scope.test.ts | 25 +++++++++++++++++++++- 4 files changed, 70 insertions(+), 10 deletions(-) diff --git a/scripts/check-note-results.mjs b/scripts/check-note-results.mjs index 386fa903..3ff6a870 100644 --- a/scripts/check-note-results.mjs +++ b/scripts/check-note-results.mjs @@ -44,6 +44,11 @@ const strip = text => { const src = 'data:image/svg+xml,' + encodeURIComponent(`${text.replaceAll('&', '&').replaceAll('<', '<')}`); return { src, clean: src, width: 600, height: 30, page: 1, label: '1', text: [{ text, chars: Array.from(text, (text, index) => ({ text, rect: [40 + index * 5, 8, 45 + index * 5, 22] })) }] }; }; +const sharedStrip = text => { + const image = strip(text); + const src = 'data:image/svg+xml,' + encodeURIComponent(`Unrelated opening${text.replaceAll('&', '&').replaceAll('<', '<')}Unrelated ending`); + return { ...image, src, clean: src, height: 800, text: image.text.map(line => ({ ...line, chars: line.chars.map(char => ({ ...char, rect: [char.rect[0], char.rect[1] + 392, char.rect[2], char.rect[3] + 392] })) })), marks: [{ unit: 'c1', kind: 'added', rect: [40, 400, 300, 414] }, { unit: 'other', kind: 'added', rect: [40, 700, 300, 720] }] }; +}; await page.route('**/api/**', async route => { const url = new URL(route.request().url()); if (url.pathname.startsWith('/review/')) return route.fallback(); @@ -58,7 +63,8 @@ await page.route('**/api/**', async route => { assert.equal(url.searchParams.get('comparison'), comparisonVersion); const round = Number(url.searchParams.get('round')), option = url.searchParams.get('option'); const candidate = round === 2 ? [...units[round].map(unit => ({ ...unit, new_text: option === 'brief' ? alternateSource : unit.new_text })), unit('other', 80, 'Three datasets.', 'Five datasets.')] : units[round]; - return route.fulfill({ json: { ...rounds[round], units: candidate, task: task.name, before: 'before', after: `candidate-${option}-${comparisonVersion}`, files: '', warnings: [], loose: [...candidate.map(unit => ({ kind: 'change', units: [unit.id], before: [strip(unit.old_text)], after: [strip(unit.new_text)] })), ...Array.from({ length: 5 }, (_, i) => ({ kind: 'change', units: ['space-' + i], identical: true, before: [], after: [] }))] } }); + const blocks = round === 2 ? [{ kind: 'change', units: candidate.map(unit => unit.id), before: [strip('Unrelated leading strip'), sharedStrip(candidate[0].old_text)], after: [strip('Unrelated leading strip'), sharedStrip(candidate[0].new_text)] }] : candidate.map(unit => ({ kind: 'change', units: [unit.id], before: [strip(unit.old_text)], after: [strip(unit.new_text)] })); + return route.fulfill({ json: { ...rounds[round], units: candidate, task: task.name, before: 'before', after: `candidate-${option}-${comparisonVersion}`, files: '', warnings: [], loose: [...blocks, ...Array.from({ length: 5 }, (_, i) => ({ kind: 'change', units: ['space-' + i], identical: true, before: [], after: [] }))] } }); } if (url.pathname.endsWith('/note-options')) { const body = route.request().method() !== 'GET' ? route.request().postDataJSON() : null; @@ -132,6 +138,8 @@ try { assert.equal(await controls().locator('.note-option-reason').textContent(), 'Preserve the author’s technical term.', 'The preferred proposal uses the AI-provided title and reason'); assert.equal(await controls().locator('.note-comparison-side-label').count(), 2, 'Only this group has before/after labels'); assert.equal(await controls().locator('.note-comparison-change[data-units="other"]').count(), 0, 'Other groups remain in the main review'); + await expect(controls().locator('.note-comparison-crop').first()).toHaveAttribute('data-crop-top', '376'); + assert.ok((await controls().locator('.note-comparison-crop').first().boundingBox()).height < 200, 'The shared PDF opens at this note instead of a page of unrelated edits'); // A shared engine strip must follow live workspace language changes even // though this page has no viewer ?lang parameter or workspace reload. const firstStrip = controls().locator('.pdf-strip .pdf-text').first(); diff --git a/src/web/note-comparison-scope.ts b/src/web/note-comparison-scope.ts index 86ba0308..2c2df064 100644 --- a/src/web/note-comparison-scope.ts +++ b/src/web/note-comparison-scope.ts @@ -1,11 +1,34 @@ -import type { Block, RoundDetail } from '../../engine/src/types.ts'; +import type { Block, RoundDetail, Strip } from '../../engine/src/types.ts'; -/** The drawer compares the current note's visual groups, not every change in - * the round. Keep their native PDF context and neighboring edits within the - * same crop, but only the note's own units receive highlights. */ +const focusedMarks = (image: Strip, focus: Set) => (image.marks || []).filter(mark => mark.kind !== 'context' && focus.has(mark.unit)); + +/** Keep complete nearby text lines around the note's marks. Coordinates stay + * in the original PDF strip so selection and highlighting remain aligned. */ +export function noteComparisonCrop(image: Strip, focus: string[]) { + const marks = focusedMarks(image, new Set(focus)); + if (!marks.length) return { top: 0, height: image.height }; + let top = Math.max(0, Math.min(...marks.map(mark => mark.rect[1])) - 24); + let bottom = Math.min(image.height, Math.max(...marks.map(mark => mark.rect[3])) + 24); + for (const line of image.text || []) { + if (!line.chars.length) continue; + const low = Math.min(...line.chars.map(char => char.rect[1])), high = Math.max(...line.chars.map(char => char.rect[3])); + if (low < bottom && high > top) { top = Math.max(0, Math.min(top, low)); bottom = Math.min(image.height, Math.max(bottom, high)); } + } + return { top, height: Math.max(1, bottom - top) }; +} + +/** Use attributed marks to focus a note inside a shared PDF group. Older + * comparisons without attribution retain their original groups and context. */ export function noteComparisonScope(detail: RoundDetail, focus: string[]) { const focused = new Set(focus); + const all = [...detail.sections.flatMap(section => section.blocks), ...detail.loose]; + const precise = all.some(block => block.kind === 'change' && block.units.some(id => focused.has(id)) && [...block.before, ...block.after].some(image => focusedMarks(image, focused).length)); const select = (blocks: Block[]) => { + if (precise) return blocks.flatMap(block => { + if (block.kind !== 'change' || !block.units.some(id => focused.has(id))) return []; + const before = block.before.filter(image => focusedMarks(image, focused).length), after = block.after.filter(image => focusedMarks(image, focused).length); + return before.length || after.length ? [{ ...block, units: block.units.filter(id => focused.has(id)), before, after }] : []; + }); const included = new Set(); blocks.forEach((block, index) => { if (block.kind !== 'change' || !block.units.some(id => focused.has(id))) return; @@ -19,7 +42,7 @@ export function noteComparisonScope(detail: RoundDetail, focus: string[]) { }; const groups = [...detail.sections.flatMap(section => section.blocks), ...detail.loose] .filter(block => block.kind === 'change' && block.units.some(id => focused.has(id))); - const sourceIds = new Set([...focus, ...groups.flatMap(block => block.kind === 'change' ? block.units : [])]); + const sourceIds = new Set([...focus, ...(precise ? [] : groups.flatMap(block => block.kind === 'change' ? block.units : []))]); return { sections: detail.sections.map(section => ({ ...section, blocks: select(section.blocks) })).filter(section => section.blocks.length), loose: select(detail.loose), diff --git a/src/web/note-option-diff.tsx b/src/web/note-option-diff.tsx index 13cdd11c..9ddb7f4d 100644 --- a/src/web/note-option-diff.tsx +++ b/src/web/note-option-diff.tsx @@ -3,7 +3,7 @@ import type { Block, RoundDetail, Strip } from '../../engine/src/types.ts'; import { PdfStrip } from '../../engine/src/web/PdfStrip.tsx'; import { wordDiff } from '../../engine/src/web/model.ts'; import { t, useLanguage } from './i18n.ts'; -import { noteComparisonScope } from './note-comparison-scope.ts'; +import { noteComparisonCrop, noteComparisonScope } from './note-comparison-scope.ts'; // Keep nearby PDF context compact. Scan rendered pixels rather than text // bounds so the visible crop retains figures and other non-text content. @@ -57,8 +57,14 @@ function CompactContext({ image, base, edge }: { image: Strip; base: string; edg export function NoteOptionDiff({ detail, focus }: { detail: RoundDetail; focus: string[] }) { const { language } = useLanguage(); const scoped = noteComparisonScope(detail, focus); - const strips = (images: Strip[], label: string, clean = false) => images.map((image, index) => - ); + const strips = (images: Strip[], label: string) => images.map((image, index) => { + const { top, height } = noteComparisonCrop(image, focus); + return
+
+ +
+
; + }); const block = (item: Block, index: number, blocks: Block[]) => { if (item.kind === 'gap') return
⋯
; if (item.kind === 'context') return ; diff --git a/tests/note-comparison-scope.test.ts b/tests/note-comparison-scope.test.ts index 14ab0b74..57050385 100644 --- a/tests/note-comparison-scope.test.ts +++ b/tests/note-comparison-scope.test.ts @@ -1,6 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { noteComparisonScope } from '../src/web/note-comparison-scope.ts'; +import { noteComparisonCrop, noteComparisonScope } from '../src/web/note-comparison-scope.ts'; import type { ChangeBlock, RoundDetail, Strip } from '../engine/src/types.ts'; test('only the current group appears, with native context and shared units but no empty PDF rows', () => { @@ -16,3 +16,26 @@ test('only the current group appears, with native context and shared units but n assert.deepEqual(noteComparisonScope(detail, ['restored-to-original']).sections, []); assert.equal(detail.sections[0].blocks.length, 4, 'The main review is unchanged'); }); + +test('shared PDF groups open at the selected note, excluding unrelated strips and source edits', () => { + const image = (unit: string, y: number): Strip => ({ src: unit + '.png', clean: unit + '-clean.png', width: 600, height: 800, page: 1, label: '1', marks: [{ kind: 'added', unit, rect: [100, y, 300, y + 10] }] }); + const first = image('one', 20), shared = { ...image('two', 400), marks: [...image('two', 400).marks!, ...image('three', 700).marks!] }; + const group: ChangeBlock = { kind: 'change', units: ['one', 'two', 'three'], identical: false, before: [first, shared], after: [first, shared] }; + const moved: ChangeBlock = { ...group, before: [], after: [{ ...first, marks: [] }] }; + const detail = { units: ['one', 'two', 'three'].map(id => ({ id })), sections: [], loose: [group, moved] } as unknown as RoundDetail; + const scoped = noteComparisonScope(detail, ['two']); + assert.deepEqual(scoped.units.map(unit => unit.id), ['two']); + assert.deepEqual(scoped.loose, [{ ...group, units: ['two'], before: [shared], after: [shared] }]); + assert.deepEqual(noteComparisonCrop(shared, ['two']), { top: 376, height: 58 }); + assert.deepEqual(noteComparisonCrop(shared, ['three']), { top: 676, height: 58 }); + assert.equal(group.after.length, 2, 'The main review retains its complete group'); +}); + +test('focused crops retain whole intersecting lines and legacy strips without marks', () => { + const image: Strip = { src: 'a.png', clean: 'b.png', width: 600, height: 800, page: 1, label: '1', marks: [{ kind: 'removed', unit: 'one', rect: [100, 100, 200, 110] }], text: [ + { text: 'preceding', chars: [{ text: 'preceding', rect: [100, 70, 200, 80] }] }, + { text: 'following', chars: [{ text: 'following', rect: [100, 130, 200, 140] }] }, + ] }; + assert.deepEqual(noteComparisonCrop(image, ['one']), { top: 70, height: 70 }); + assert.deepEqual(noteComparisonCrop({ ...image, marks: undefined }, ['one']), { top: 0, height: 800 }); +});