diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.js b/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.js index 66a5ea10a7..6a6c3babb7 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.js @@ -219,9 +219,11 @@ const classifySegment = (ctx, segment) => { * stored authorEmail matches the current user's normalized email — including * when both sides have no email at all (default unidentified user typing). * - * Permission ownership and overlap parent decisions still require the - * high-confidence `classifySegment` path; this helper is only for "is this - * the same logical author for the purpose of coalescing contiguous edits". + * Permission ownership still requires the high-confidence `classifySegment` + * path; this helper answers "is this the same logical author for the purpose + * of refining their own pending edit" — coalescing contiguous typing, + * shrinking an own insertion on delete (SD-3352), and deciding that an own + * insertion is not a different-user overlap parent for a replacement. * * @param {*} ctx * @param {*} segment @@ -791,8 +793,14 @@ const applyTrackedDelete = ( change: getChangeAuthorIdentity(segmentAtPos?.attrs ?? insertMark.attrs), }); const ownership = isSameUserHighConfidence(classification) ? 'same-user' : 'different-user'; + // Deleting inside your own pending insertion must shrink that insertion, + // not stack a deletion on top of your own suggestion (SD-3352). Use the + // same permissive same-author gate as typing, so this also holds for + // users without an id/email (anonymous sessions). + const sameUserForRefinement = + ownership === 'same-user' || isSameUserForRefinement(ctx, segmentAtPos ?? { attrs: insertMark.attrs }); const shouldCollapseOwnInsertion = - !ctx.intent.preserveExistingReviewState && (ownership === 'same-user' || isImportedOwnInsertion(insertMark)); + !ctx.intent.preserveExistingReviewState && (sameUserForRefinement || isImportedOwnInsertion(insertMark)); if (shouldCollapseOwnInsertion) { // Own insertion → collapse (remove proposed content). ops.push({ kind: 'collapse', from: segFrom, to: segTo, changeId: insertMark.attrs.id }); @@ -1231,8 +1239,14 @@ const clampToDocSize = (size, pos) => Math.max(0, Math.min(size, pos)); const getSingleFullyCoveringOwnInsertedSegment = (ctx, segments, from, to) => { if (!segments.length) return null; + // Same permissive same-author gate as the delete half in applyTrackedDelete + // (SD-3352): replacing text wholly inside your own pending insertion must + // refine that insertion in place, including for anonymous sessions where the + // high-confidence classification cannot confirm identity. Otherwise the + // delete half collapses while the replacement text nests as a child + // insertion under the user's own suggestion. const inserted = segments.filter( - (s) => s.side === SegmentSide.Inserted && classifySegment(ctx, s) === 'same-user' && s.from <= from && s.to >= to, + (s) => s.side === SegmentSide.Inserted && isSameUserForRefinement(ctx, s) && s.from <= from && s.to >= to, ); if (inserted.length !== segments.length) return null; const [first] = inserted; @@ -1243,6 +1257,11 @@ const getSingleFullyCoveringOwnInsertedSegment = (ctx, segments, from, to) => { const getReplacementParentId = (ctx, segments) => { for (const segment of segments) { if (classifySegment(ctx, segment) !== 'different-user') continue; + // An inserted segment that matches the permissive same-author refinement + // gate (SD-3352, anonymous sessions) is the user's own pending content: + // the delete half collapses it in applyTrackedDelete, so it must not be + // classified as a different-user overlap parent for the inserted side. + if (segment.side === SegmentSide.Inserted && isSameUserForRefinement(ctx, segment)) continue; if (segment.attrs?.overlapParentId) return segment.attrs.overlapParentId; return segment.changeId || ''; } diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.test.js b/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.test.js index db284b74d3..26398360d3 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.test.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/review-model/overlap-compiler.test.js @@ -269,6 +269,101 @@ describe('overlap-compiler: same-user own-insertion refinement (SD-486-adjacent expect(change.type).toBe(CanonicalChangeType.Insertion); expect(result.updatedChangeIds).toContain(id); }); + + it('refines own anonymous-default-user insertion when replacing inside it (SD-3352)', () => { + // Replace-path counterpart of the SD-3352 delete tests: an anonymous + // session selecting text inside its own pending insertion and typing must + // refine that insertion in place — one logical change with the same id — + // not collapse the delete half while nesting the replacement text as a + // different-user child insertion under the user's own suggestion. + const parentId = 'ins-default-user'; + const anonDefaultUser = { name: 'Default SuperDoc user', email: '' }; + const { state } = stateFromTrackedSpans({ + schema, + spans: [ + { + text: 'ABCHELLOXYZ', + marks: [ + insertMark({ id: parentId, author: anonDefaultUser.name, authorEmail: '', sourceId: '', date: FIXED_DATE }), + ], + }, + ], + }); + // Replace "HELLO" at [4, 9) — wholly inside the pending insertion. + const intent = makeTextReplaceIntent({ + from: 4, + to: 9, + content: sliceFromText(schema, 'Q'), + replacements: 'paired', + user: anonDefaultUser, + date: FIXED_DATE, + source: 'native', + }); + const result = runCompile({ state, intent }); + expect(result.ok).toBe(true); + expect(textOf(result.tr)).toBe('ABCQXYZ'); + + const graph = buildReviewGraph({ state: { doc: result.tr.doc } }); + expect(graph.changes.size).toBe(1); + const parent = graph.changes.get(parentId); + expect(parent).toBeDefined(); + expect(parent.type).toBe(CanonicalChangeType.Insertion); + expect(parent.insertedSegments.map((segment) => segment.text).join('')).toBe('ABCQXYZ'); + expect(parent.insertedSegments.every((segment) => !segment.attrs.overlapParentId)).toBe(true); + expect(result.updatedChangeIds).toContain(parentId); + }); + + it('replacement spanning own anonymous insertion and live text pairs like an identified user (SD-3352)', () => { + // Partial overlap goes through getReplacementParentId rather than the + // fully-covering refinement path. The user's own (by the permissive + // anonymous gate) insertion must not be classified as a different-user + // overlap parent: the result is an ordinary paired replacement — the + // covered insertion slice collapses, the live slice gets the paired + // deletion — exactly what an identified user gets. + const parentId = 'ins-default-user'; + const anonDefaultUser = { name: 'Default SuperDoc user', email: '' }; + const { state } = stateFromTrackedSpans({ + schema, + spans: [ + { + text: 'NEW', + marks: [ + insertMark({ id: parentId, author: anonDefaultUser.name, authorEmail: '', sourceId: '', date: FIXED_DATE }), + ], + }, + { text: 'live' }, + ], + }); + // Replace "EWli" at [2, 6): spans the insertion tail and live text head. + const intent = makeTextReplaceIntent({ + from: 2, + to: 6, + content: sliceFromText(schema, 'Q'), + replacements: 'paired', + user: anonDefaultUser, + date: FIXED_DATE, + source: 'native', + }); + const result = runCompile({ state, intent }); + expect(result.ok).toBe(true); + + const graph = buildReviewGraph({ state: { doc: result.tr.doc } }); + // Two logical changes: the refined parent insertion ("N") and one paired + // replacement ("li" -> "Q"). No change is nested under the parent. + expect(graph.changes.size).toBe(2); + const parent = graph.changes.get(parentId); + expect(parent).toBeDefined(); + expect(parent.type).toBe(CanonicalChangeType.Insertion); + expect(parent.insertedSegments.map((segment) => segment.text).join('')).toBe('N'); + + const replacement = Array.from(graph.changes.values()).find((change) => change.id !== parentId); + expect(replacement).toBeDefined(); + expect(replacement.type).toBe(CanonicalChangeType.Replacement); + expect(replacement.deletedSegments.map((segment) => segment.text).join('')).toBe('li'); + expect(replacement.insertedSegments.map((segment) => segment.text).join('')).toBe('Q'); + expect(replacement.insertedSegments.every((segment) => !segment.attrs.overlapParentId)).toBe(true); + expect(replacement.deletedSegments.every((segment) => !segment.attrs.overlapParentId)).toBe(true); + }); }); describe('overlap-compiler: keystroke deletion coalescing excludes structured changes (PR #3610)', () => { @@ -642,7 +737,54 @@ describe('overlap-compiler: text-delete', () => { expect(result.removedChangeIds).toEqual([parentId]); }); - it('creates a child deletion inside a live anonymous no-email insertion', () => { + it('refines own anonymous-default-user insertion when deleting inside it (SD-3352)', () => { + // Integrators that pass no `user` run as the anonymous default identity + // (display name only, no id/email). Deleting inside your own pending + // insertion in the same session must refine the insertion in place — not + // mint an overlapping different-user child deletion. Word overlap case + // 11_same_insert_delete_inside_control is the oracle: one refined w:ins. + const parentId = 'ins-default-user'; + const anonDefaultUser = { name: 'Default SuperDoc user', email: '' }; + const { state } = stateFromTrackedSpans({ + schema, + spans: [ + { + text: 'ABCHELLOXYZ', + marks: [ + insertMark({ id: parentId, author: anonDefaultUser.name, authorEmail: '', sourceId: '', date: FIXED_DATE }), + ], + }, + ], + }); + // Delete "HELLO" at [4, 9) — wholly inside the pending insertion. + const intent = makeTextDeleteIntent({ + from: 4, + to: 9, + user: anonDefaultUser, + date: FIXED_DATE, + source: 'native', + }); + const result = runCompile({ state, intent }); + expect(result.ok).toBe(true); + expect(textOf(result.tr)).toBe('ABCXYZ'); + + const graph = buildReviewGraph({ state: { doc: result.tr.doc } }); + expect(graph.changes.size).toBe(1); + const parent = graph.changes.get(parentId); + expect(parent).toBeDefined(); + expect(parent.type).toBe(CanonicalChangeType.Insertion); + expect(parent.insertedSegments.map((segment) => segment.text).join('')).toBe('ABCXYZ'); + expect(result.updatedChangeIds).toContain(parentId); + expect(result.deletionMarks).toHaveLength(0); + }); + + it('collapses inside a live unattributed insertion when the current user is equally anonymous (SD-3352 gate unification)', () => { + // Same permissive rule as typing (matchesSameUserRefinement): a truly + // unattributed live insertion deleted by an equally anonymous session + // refines in place. Before SD-3352 this minted a different-user child + // deletion while typing into the very same insertion coalesced — the + // delete path simply used the stricter gate. Named no-email authors stay + // protected (see the named no-email test above). const parentId = 'ins-live-anonymous'; const { state } = stateFromTrackedSpans({ schema, @@ -662,20 +804,15 @@ describe('overlap-compiler: text-delete', () => { }); const result = runCompile({ state, intent }); expect(result.ok).toBe(true); - expect(textOf(result.tr)).toBe('live-review-comment'); + expect(textOf(result.tr)).toBe('live--comment'); const graph = buildReviewGraph({ state: { doc: result.tr.doc } }); - expect(graph.changes.size).toBe(2); + expect(graph.changes.size).toBe(1); const parent = graph.changes.get(parentId); expect(parent).toBeDefined(); expect(parent.type).toBe(CanonicalChangeType.Insertion); - expect(parent.insertedSegments.map((segment) => segment.text).join('')).toBe('live-review-comment'); - - const child = Array.from(graph.changes.values()).find((change) => change.id !== parentId); - expect(child).toBeDefined(); - expect(child.type).toBe(CanonicalChangeType.Deletion); - expect(child.deletedSegments[0].text).toBe('review'); - expect(child.deletedSegments[0].attrs.overlapParentId).toBe(parentId); + expect(parent.insertedSegments.map((segment) => segment.text).join('')).toBe('live--comment'); + expect(result.updatedChangeIds).toContain(parentId); }); it('no-ops when deleting inside own deletion', () => { diff --git a/tests/behavior/tests/comments/sd-3352-same-author-delete-inside-insertion.spec.ts b/tests/behavior/tests/comments/sd-3352-same-author-delete-inside-insertion.spec.ts new file mode 100644 index 0000000000..6982a0aea0 --- /dev/null +++ b/tests/behavior/tests/comments/sd-3352-same-author-delete-inside-insertion.spec.ts @@ -0,0 +1,149 @@ +import { test, expect, type SuperDocFixture } from '../../fixtures/superdoc.js'; + +// SD-3352 / TC-SAME-001: deleting text wholly inside your OWN pending tracked +// insertion must refine the insertion in place — one shrunken suggestion, the +// way Word handles it (overlap case 11_same_insert_delete_inside_control) — +// not stack an overlapping deletion on top of your own insertion. +// +// The regression trigger was an anonymous session (integrator passes no +// `user`): typing recognized the anonymous user as itself, but the delete +// path classified the same user as a different reviewer and minted a nested +// tracked deletion over their own suggestion. + +test.use({ config: { toolbar: 'full', comments: 'off', trackChanges: true } }); + +// Mirror of SuperDoc's DEFAULT_USER when the integrator passes no `user`. +const ANONYMOUS_DEFAULT = { id: null, name: 'Default SuperDoc user', email: null }; +const REVIEWER = { name: 'Guest Reviewer', email: 'track@example.com' }; + +const setUser = (superdoc: SuperDocFixture, user: Record): Promise => + superdoc.page.evaluate((u) => { + (window as any).editor.setOptions({ user: u }); + }, user); + +/** + * Snapshot tracked-change state: text grouped by logical change id per mark + * kind, plus the public document-api read model. + */ +const snapshotTrackedState = ( + superdoc: SuperDocFixture, +): Promise<{ + insertById: Record; + deleteById: Record; + docText: string; + listItems: Array<{ type: string; text: string }>; +}> => + superdoc.page.evaluate(async () => { + const editor = (window as any).editor; + const insertById: Record = {}; + const deleteById: Record = {}; + editor.state.doc.descendants((node: any) => { + if (!node.isText || !node.text) return; + for (const mark of node.marks ?? []) { + if (mark.type?.name === 'trackInsert') { + const id = String(mark.attrs?.id ?? ''); + insertById[id] = (insertById[id] ?? '') + node.text; + } else if (mark.type?.name === 'trackDelete') { + const id = String(mark.attrs?.id ?? ''); + deleteById[id] = (deleteById[id] ?? '') + node.text; + } + } + }); + const list = await editor.doc.trackChanges.list(); + return { + insertById, + deleteById, + docText: editor.state.doc.textContent, + listItems: (list?.items ?? []).map((item: any) => ({ + type: String(item.type), + text: String(item.insertedText ?? item.deletedText ?? ''), + })), + }; + }); + +/** Type a pending insertion in suggesting mode, then delete "HELLO" inside it. */ +async function typeThenDeleteInsideOwnInsertion(superdoc: SuperDocFixture): Promise { + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + await superdoc.type('ABCHELLOXYZ'); + await superdoc.waitForStable(); + + const pos = await superdoc.findTextPos('HELLO'); + await superdoc.setTextSelection(pos, pos + 'HELLO'.length); + await superdoc.waitForStable(); + await superdoc.press('Backspace'); + await superdoc.waitForStable(); +} + +const expectSingleRefinedInsertion = (state: Awaited>) => { + expect(state.docText).toBe('ABCXYZ'); + // No overlapping deletion was minted over the user's own suggestion. + expect(Object.keys(state.deleteById)).toHaveLength(0); + // One logical insertion remains, refined to the surviving text. + const insertIds = Object.keys(state.insertById); + expect(insertIds).toHaveLength(1); + expect(state.insertById[insertIds[0]]).toBe('ABCXYZ'); + // Public read model agrees: exactly one reviewable insert item. + expect(state.listItems).toEqual([{ type: 'insert', text: 'ABCXYZ' }]); +}; + +test('anonymous session: deleting inside own pending insertion refines it in place (SD-3352)', async ({ superdoc }) => { + await setUser(superdoc, ANONYMOUS_DEFAULT); + await typeThenDeleteInsideOwnInsertion(superdoc); + expectSingleRefinedInsertion(await snapshotTrackedState(superdoc)); +}); + +test('identified user: deleting inside own pending insertion refines it in place', async ({ superdoc }) => { + await setUser(superdoc, REVIEWER); + await typeThenDeleteInsideOwnInsertion(superdoc); + expectSingleRefinedInsertion(await snapshotTrackedState(superdoc)); +}); + +test('anonymous session: typing over a selection inside own insertion refines it in place (SD-3352)', async ({ + superdoc, +}) => { + // Replace-path variant: selecting inside your own pending insertion and + // typing must refine the insertion under its original id — not collapse the + // deleted half while minting the replacement text as a nested child + // insertion (overlapParentId) under your own suggestion. + await setUser(superdoc, ANONYMOUS_DEFAULT); + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + await superdoc.type('ABCHELLOXYZ'); + await superdoc.waitForStable(); + + const pos = await superdoc.findTextPos('HELLO'); + await superdoc.setTextSelection(pos, pos + 'HELLO'.length); + await superdoc.waitForStable(); + await superdoc.type('Q'); + await superdoc.waitForStable(); + + const state = await snapshotTrackedState(superdoc); + expect(state.docText).toBe('ABCQXYZ'); + expect(Object.keys(state.deleteById)).toHaveLength(0); + const insertIds = Object.keys(state.insertById); + expect(insertIds).toHaveLength(1); + expect(state.insertById[insertIds[0]]).toBe('ABCQXYZ'); + expect(state.listItems).toEqual([{ type: 'insert', text: 'ABCQXYZ' }]); +}); + +test('anonymous session: backspacing char-by-char through own insertion stays one refined insertion', async ({ + superdoc, +}) => { + await setUser(superdoc, ANONYMOUS_DEFAULT); + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + await superdoc.type('ABCHELLOXYZ'); + await superdoc.waitForStable(); + + // Caret after the final "O" of HELLO, then one Backspace per character. + const pos = await superdoc.findTextPos('HELLO'); + await superdoc.setTextSelection(pos + 'HELLO'.length); + await superdoc.waitForStable(); + for (let i = 0; i < 'HELLO'.length; i += 1) { + await superdoc.press('Backspace'); + await superdoc.waitForStable(); + } + + expectSingleRefinedInsertion(await snapshotTrackedState(superdoc)); +});