diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.test.ts index 96d88f1a67..8248360eac 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.test.ts @@ -6,6 +6,7 @@ import { TrackInsertMarkName, } from '../../extensions/track-changes/constants.js'; import { getTrackChanges } from '../../extensions/track-changes/trackChangesHelpers/getTrackChanges.js'; +import { enumerateStructuralRowChanges } from '../../extensions/track-changes/trackChangesHelpers/structuralRowChanges.js'; import { buildTrackedChangeCanonicalIdMap, groupTrackedChanges, @@ -18,6 +19,10 @@ vi.mock('../../extensions/track-changes/trackChangesHelpers/getTrackChanges.js', getTrackChanges: vi.fn(), })); +vi.mock('../../extensions/track-changes/trackChangesHelpers/structuralRowChanges.js', () => ({ + enumerateStructuralRowChanges: vi.fn(() => []), +})); + function makeEditor(options: Record = { trackedChanges: {} }): Editor { return { options, @@ -305,6 +310,103 @@ describe('groupTrackedChanges', () => { const grouped = groupTrackedChanges(makeEditor()); expect(grouped[0]?.from).toBeLessThan(grouped[1]?.from ?? 0); }); + + // A whole inserted/deleted table is ONE logical change. Inline cell marks + // inside its range (native authoring stamps these, and imported-with-text + // tables carry them) must be subsumed by the structural change, not returned + // as separate review items. + const wholeTableInsert = (overrides: Record = {}) => + ({ + id: 'logical-1', + side: 'insertion', + subtype: 'table-insert', + tableFrom: 10, + tableTo: 40, + tablePos: 10, + wholeTable: true, + decidable: true, + rows: [], + author: 'Alice', + sourceId: '7', + ...overrides, + }) as never; + + it('subsumes inline cell marks inside a decidable whole-table change', () => { + const cell = makeTrackMark(TrackInsertMarkName, 'inline-cell', { author: 'Alice' }); + vi.mocked(getTrackChanges).mockReturnValue([ + { ...cell, node: { text: 'Hi', marks: [cell.mark] }, from: 20, to: 25 }, + ] as never); + vi.mocked(enumerateStructuralRowChanges).mockReturnValueOnce([wholeTableInsert()]); + + const grouped = groupTrackedChanges(makeEditor()); + + // Only the structural change remains; the inline cell mark is owned by it. + expect(grouped).toHaveLength(1); + expect(grouped[0]?.id).toBe('word:structural:7'); + expect(grouped.find((change) => change.rawId === 'inline-cell')).toBeUndefined(); + }); + + it('does NOT subsume an inline change outside the whole-table range', () => { + const outside = makeTrackMark(TrackInsertMarkName, 'inline-outside', { author: 'Alice' }); + vi.mocked(getTrackChanges).mockReturnValue([ + { ...outside, node: { text: 'Out', marks: [outside.mark] }, from: 50, to: 55 }, + ] as never); + vi.mocked(enumerateStructuralRowChanges).mockReturnValueOnce([wholeTableInsert()]); + + const grouped = groupTrackedChanges(makeEditor()); + + // The structural change AND the unrelated inline change both surface. + expect(grouped.find((change) => change.id === 'word:structural:7')).toBeDefined(); + expect(grouped.find((change) => change.rawId === 'inline-outside')).toBeDefined(); + }); + + it('does NOT subsume inline marks for a NON-decidable (partial) structural shape', () => { + const cell = makeTrackMark(TrackInsertMarkName, 'inline-partial', { author: 'Alice' }); + vi.mocked(getTrackChanges).mockReturnValue([ + { ...cell, node: { text: 'Hi', marks: [cell.mark] }, from: 20, to: 25 }, + ] as never); + // Partial / undecidable structural shape is not one logical change. + vi.mocked(enumerateStructuralRowChanges).mockReturnValueOnce([ + wholeTableInsert({ wholeTable: false, decidable: false }), + ]); + + const grouped = groupTrackedChanges(makeEditor()); + expect(grouped.find((change) => change.rawId === 'inline-partial')).toBeDefined(); + }); + + it('subsumes a single revision id whose disjoint segments each sit in a different whole table', () => { + // One imported id reused across two tables: marks at [20,25] (table A 10-40) + // and [70,75] (table B 60-90). The collapsed envelope [20,75] spans the gap + // between the tables, so an envelope-only check would wrongly keep it — the + // per-segment rule correctly subsumes it (every segment is table-owned). + const shared = makeTrackMark(TrackInsertMarkName, 'shared-across-tables', { author: 'Alice' }); + vi.mocked(getTrackChanges).mockReturnValue([ + { ...shared, node: { text: 'A', marks: [shared.mark] }, from: 20, to: 25 }, + { ...shared, node: { text: 'B', marks: [shared.mark] }, from: 70, to: 75 }, + ] as never); + vi.mocked(enumerateStructuralRowChanges).mockReturnValueOnce([ + wholeTableInsert({ id: 'logical-a', sourceId: '7', tableFrom: 10, tableTo: 40, tablePos: 10 }), + wholeTableInsert({ id: 'logical-b', sourceId: '8', tableFrom: 60, tableTo: 90, tablePos: 60 }), + ]); + + const grouped = groupTrackedChanges(makeEditor()); + expect(grouped.find((change) => change.rawId === 'shared-across-tables')).toBeUndefined(); + // Both structural tables survive. + expect(grouped.filter((change) => change.id?.startsWith('word:structural:'))).toHaveLength(2); + }); + + it('does NOT subsume an inline change whose segment straddles a table edge', () => { + // Segment [35,45] crosses the table's right edge (table 10-40): it is not + // wholly inside any whole-table range, so the change is kept. + const straddle = makeTrackMark(TrackInsertMarkName, 'edge-straddle', { author: 'Alice' }); + vi.mocked(getTrackChanges).mockReturnValue([ + { ...straddle, node: { text: 'edge', marks: [straddle.mark] }, from: 35, to: 45 }, + ] as never); + vi.mocked(enumerateStructuralRowChanges).mockReturnValueOnce([wholeTableInsert()]); + + const grouped = groupTrackedChanges(makeEditor()); + expect(grouped.find((change) => change.rawId === 'edge-straddle')).toBeDefined(); + }); }); describe('resolveTrackedChange', () => { diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.ts index 2875210f14..1fc5423701 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/tracked-change-resolver.ts @@ -315,6 +315,11 @@ export function groupTrackedChanges(editor: Editor): GroupedTrackedChange[] { const marks = getRawTrackedMarks(editor); const byRawId = new Map(); + // Every underlying mark range per grouped id (not just the min/max envelope). + // A single revision id can have disjoint segments (Word reuses ids across the + // document), so whole-table subsumption below must test each segment, not the + // collapsed `from`/`to`. + const segmentsByRawId = new Map>(); for (const item of marks) { const attrs = item.mark?.attrs ?? {}; @@ -333,6 +338,10 @@ export function groupTrackedChanges(editor: Editor): GroupedTrackedChange[] { const excerptText = contributesToExcerpt ? getTrackedMarkText(editor, item) : ''; const range: [number, number] = [item.from, item.to]; + const priorSegments = segmentsByRawId.get(groupKey); + if (priorSegments) priorSegments.push({ from: item.from, to: item.to }); + else segmentsByRawId.set(groupKey, [{ from: item.from, to: item.to }]); + if (!existing) { byRawId.set(groupKey, { rawId: groupKey, @@ -397,7 +406,38 @@ export function groupTrackedChanges(editor: Editor): GroupedTrackedChange[] { // their own grouped changes. Their `id` is the shared Word revision id; the // accept/reject command routes by id through the review graph which owns the // node-level mutation plan. - for (const structural of enumerateStructuralRowChanges(editor.state)) { + const structuralChanges = enumerateStructuralRowChanges(editor.state); + + // Inline content inside a decidable whole-table structural change is OWNED by + // that change — a whole inserted/deleted table is ONE logical change, not the + // structural change plus a separate review item per tracked cell run. Native + // authoring (markInsertion/markDeletion on cell text) and imported-with-text + // tables both leave such inline marks, so drop the inline grouped entries + // whose content lives inside a whole-table range before the structural change + // is appended. Only decidable whole-table ranges suppress; a partial/ + // undecidable structural shape is not one logical change, so its inline marks + // stay as their own items. Mirrors the comments-store range suppression + // (`isInlineRangeInsideTrackedTable`): an inline change is owned only when + // EVERY one of its segments sits inside some whole-table range. Testing each + // segment (not the collapsed `from`/`to` envelope) matters when one revision + // id has disjoint marks across two tracked tables — the envelope would span + // the gap between them, yet every segment is table-owned. A segment that + // straddles a table edge is (correctly) not owned, so the change stays. + const wholeTableRanges = structuralChanges + .filter((structural) => structural.decidable && structural.wholeTable) + .map((structural) => ({ from: structural.tableFrom, to: structural.tableTo })); + if (wholeTableRanges.length > 0) { + const segmentInsideSomeTable = (segment: { from: number; to: number }) => + wholeTableRanges.some((range) => segment.from >= range.from && segment.to <= range.to); + for (let i = grouped.length - 1; i >= 0; i -= 1) { + const change = grouped[i]; + const segments = segmentsByRawId.get(change.rawId) ?? [{ from: change.from, to: change.to }]; + const ownedByTable = segments.every(segmentInsideSomeTable); + if (ownedByTable) grouped.splice(i, 1); + } + } + + for (const structural of structuralChanges) { const excerpt = normalizeExcerpt(editor.state.doc.textBetween(structural.tableFrom, structural.tableTo, ' ', '')); // Public id must be stable across import → export → reopen. The logical // `structural.id` is a fresh UUID minted on each import, so derive the