diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.test.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.test.ts index 9fa048a2d2..0b85700e72 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.test.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.test.ts @@ -352,6 +352,105 @@ describe('CommentHighlightDecorator', () => { // ── Edge cases ───────────────────────────────────────────────────── + // ── SD-2528 P2 #2: comment fill vs tracked-change background ─────── + // + // Per layout-engine styles.ts, only `.track-insert-dec.highlighted` and + // `.track-delete-dec.highlighted` paint a background; `.track-format-dec` + // only paints a `border-bottom`. The `.highlighted` modifier is ONLY + // applied in "review" (All Markup) mode — Original and Final modes use + // `.hidden` / `.normal` / `.before` and paint no background. + // + // The decorator must therefore suppress its comment-fill repaint ONLY in + // the narrow case where the TC actually paints a competing background. + // Otherwise the comment highlight is cleared with nothing to replace it + // and the comment becomes invisible in Original / Final / format-only. + describe('tracked-change-anchored elements (SD-2528 P2 #2)', () => { + const commentClass = 'superdoc-comment-highlight'; + + function tcCommentSpan(opts: { commentIds: string[]; tcClasses: string[] }): HTMLSpanElement { + const el = commentSpan({ commentIds: opts.commentIds }); + el.classList.add(...opts.tcClasses); + return el; + } + + it('suppresses comment fill when element is track-insert-dec AND highlighted', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-insert-dec', 'highlighted'] }); + container.appendChild(span); + + decorator.apply(); + + expect(span.style.backgroundColor).toBe(''); + expect(span.classList.contains(commentClass)).toBe(true); + }); + + it('suppresses comment fill when element is track-delete-dec AND highlighted', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-delete-dec', 'highlighted'] }); + container.appendChild(span); + + decorator.apply(); + + expect(span.style.backgroundColor).toBe(''); + }); + + it('keeps comment fill when track-insert-dec is present but .highlighted is NOT (Original/Final mode)', () => { + // In Original/Final modes the painter applies `.hidden` / `.normal` / + // `.before` instead of `.highlighted`, so no green background is drawn. + // The comment fill must remain visible — otherwise the comment bubble + // disappears with no replacement paint. + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-insert-dec', 'normal'] }); + container.appendChild(span); + + decorator.apply(); + + expect(span.style.backgroundColor).toBe(EXT); + }); + + it('keeps comment fill when track-delete-dec is present but .highlighted is NOT', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-delete-dec', 'hidden'] }); + container.appendChild(span); + + decorator.apply(); + + expect(span.style.backgroundColor).toBe(EXT); + }); + + it('keeps comment fill on track-format-dec.highlighted — format changes paint only a border, not a background', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-format-dec', 'highlighted'] }); + container.appendChild(span); + + decorator.apply(); + + expect(span.style.backgroundColor).toBe(EXT); + }); + + it('still suppresses comment fill in the active highlight branch when TC paints', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-insert-dec', 'highlighted'] }); + container.appendChild(span); + + setActiveCommentAndApply(decorator, 'c-1'); + + expect(span.style.backgroundColor).toBe(''); + }); + + it('still suppresses comment fill in the faded branch when TC paints and a different comment is active', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-delete-dec', 'highlighted'] }); + container.appendChild(span); + + setActiveCommentAndApply(decorator, 'other'); + + expect(span.style.backgroundColor).toBe(''); + }); + + it('keeps faded comment fill in Original mode (track-insert-dec without .highlighted)', () => { + const span = tcCommentSpan({ commentIds: ['c-1'], tcClasses: ['track-insert-dec', 'normal'] }); + container.appendChild(span); + + setActiveCommentAndApply(decorator, 'other'); + + expect(span.style.backgroundColor).toBe(EXT_FADED); + }); + }); + describe('edge cases', () => { it('apply() is a no-op when no container is set', () => { const dec = new CommentHighlightDecorator(); diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.ts index 3cc87e3e07..a1a2b61edc 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.ts @@ -171,9 +171,39 @@ export class CommentHighlightDecorator { // Determine if primary (first) comment is internal — used for uniform/faded colors. const primaryIsInternal = internalIds.has(ids[0]); + // SD-2528: a comment anchored on tracked-change text shows the TC's + // own background (green for trackInsert, red for trackDelete) via + // `.track-insert-dec.highlighted` / `.track-delete-dec.highlighted`. + // The comment highlight stacking on top of that paints pink/green + // over the TC color, making an "insert" look pink after re-import. + // Leave the background alone in that case so the TC color wins + // (matches Word — comments anchored on a redline don't recolor the + // redline). + // + // AIDEV-NOTE: SD-2528 P2 #2. Suppress comment fill ONLY when the TC + // is actually painting a competing background. Per layout-engine + // styles.ts:270-294, that requires both: + // - the base class `track-insert-dec` or `track-delete-dec` (not + // `track-format-dec`, which only paints a `border-bottom`); + // - the `.highlighted` modifier — only applied in "review" / All + // Markup mode per renderer.ts:909-928. In Original/Final modes + // the modifier is `hidden` / `normal` / `before` and no + // background is painted. + // Without this narrower gate the comment highlight was cleared with + // nothing to replace it, making the bubble invisible in Original / + // Final modes and on format-only changes. Hover/focus affordances + // still come from the TC's own `.track-change-focused` class. + const isTrackedChangeAnchored = + el.classList.contains('highlighted') && + (el.classList.contains('track-insert-dec') || el.classList.contains('track-delete-dec')); + if (activeId == null) { // No active comment → uniform light highlight - applyBgColor(el, primaryIsInternal ? H.INT : H.EXT); + if (!isTrackedChangeAnchored) { + applyBgColor(el, primaryIsInternal ? H.INT : H.EXT); + } else { + el.style.backgroundColor = ''; + } el.style.boxShadow = ''; continue; } @@ -184,7 +214,11 @@ export class CommentHighlightDecorator { if (matchedId != null) { // This element belongs to the active comment → bright highlight const matchIsInternal = internalIds.has(matchedId); - applyBgColor(el, matchIsInternal ? H.INT_ACTIVE : H.EXT_ACTIVE); + if (!isTrackedChangeAnchored) { + applyBgColor(el, matchIsInternal ? H.INT_ACTIVE : H.EXT_ACTIVE); + } else { + el.style.backgroundColor = ''; + } // Nested comments: other IDs besides the active one const hasNested = ids.length > 1; @@ -195,7 +229,11 @@ export class CommentHighlightDecorator { } } else { // Active comment is set but doesn't match this element → faded - applyBgColor(el, primaryIsInternal ? H.INT_FADED : H.EXT_FADED); + if (!isTrackedChangeAnchored) { + applyBgColor(el, primaryIsInternal ? H.INT_FADED : H.EXT_FADED); + } else { + el.style.backgroundColor = ''; + } el.style.boxShadow = ''; } } diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/documentCommentsImporter.js b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/documentCommentsImporter.js index 8a08844ebc..35827ba02d 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/documentCommentsImporter.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/documentCommentsImporter.js @@ -208,9 +208,6 @@ const generateCommentsWithExtendedData = ({ docx, comments, converter, threading } } - // Track the tracked change association but don't use it as parentCommentId - // This keeps comments and tracked changes as separate bubbles in the UI - // while preserving the relationship for export and visual purposes const trackedChangeParentId = isInsideTrackedChange ? trackedChangeParent.trackedChangeId : undefined; // Only use range-based parenting as fallback when: diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js index 5a80b027d3..b4f84e5680 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js @@ -158,8 +158,16 @@ export const createDocumentJson = (docx, converter, editor) => { const trackedChangeIdMapOptions = { replacements: converter.trackedChangesOptions?.replacements ?? 'paired', }; - converter.trackedChangeIdMap = buildTrackedChangeIdMap(docx, trackedChangeIdMapOptions); + // AIDEV-NOTE: SD-2528. The per-part map and the global map MUST share UUIDs + // for the same w:id, otherwise documentCommentsImporter (uses the global) + // and the ins/del translators (use the per-part) end up with two different + // UUIDs for the same tracked change — the comment's trackedChangeParentId + // never matches the tracked-change mark's id, breaking accept/reject + // cascading. converter.trackedChangeIdMapsByPart = buildTrackedChangeIdMapsByPart(docx, trackedChangeIdMapOptions); + converter.trackedChangeIdMap = + converter.trackedChangeIdMapsByPart.get('word/document.xml') ?? + buildTrackedChangeIdMap(docx, trackedChangeIdMapOptions); const comments = importCommentData({ docx, nodeListHandler, converter, editor }); const footnotes = importFootnoteData({ docx, nodeListHandler, converter, editor, numbering }); const endnotes = importEndnoteData({ docx, nodeListHandler, converter, editor, numbering }); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js index 1825db4489..ba6a766082 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js @@ -1,12 +1,52 @@ import { translateChildNodes } from '@converter/v2/exporter/helpers/index.js'; import { generateParagraphProperties } from './generate-paragraph-properties.js'; +const isTrackedChangeWrapper = (el) => el?.name === 'w:ins' || el?.name === 'w:del'; + +const isCommentMarker = (el) => { + if (!el) return false; + if (el.name === 'w:commentRangeStart' || el.name === 'w:commentRangeEnd') return true; + if (el.name === 'w:r' && el.elements?.length === 1 && el.elements[0]?.name === 'w:commentReference') return true; + return false; +}; + +// AIDEV-NOTE: SD-2528. The importer associates a comment with a tracked change +// by walking document.xml and noting commentRangeStart elements that appear +// inside a w:ins/w:del wrapper (see documentCommentsImporter.js' +// extractCommentRangesFromDocument). Word always emits commentRangeStart inside +// the wrapper; emitting it as a sibling silently loses the comment ↔ TC link +// on re-import. +function foldLeadingCommentStartsIntoTrackedChanges(elements) { + const result = []; + let i = 0; + while (i < elements.length) { + if (elements[i]?.name !== 'w:commentRangeStart') { + result.push(elements[i]); + i++; + continue; + } + const leadingStarts = []; + while (i < elements.length && elements[i]?.name === 'w:commentRangeStart') { + leadingStarts.push(elements[i]); + i++; + } + const next = elements[i]; + if (isTrackedChangeWrapper(next)) { + result.push({ ...next, elements: [...leadingStarts, ...(next.elements || [])] }); + i++; + } else { + result.push(...leadingStarts); + } + } + return result; +} + /** - * Merge consecutive tracked change elements (w:ins/w:del) with the same ID. - * Comment range markers between tracked changes with the same ID are included - * inside the merged wrapper, matching Word's OOXML structure. - * - * See SD-1519 for details on the ECMA-376 spec compliance. + * Merge consecutive tracked change elements (w:ins/w:del) with the same ID, + * and fold any commentRangeStart that immediately precedes a tracked-change + * wrapper INTO the wrapper as its first child(ren). Trailing commentRangeEnd + * and w:r→w:commentReference stay as siblings and are only absorbed when a + * same-id successor wrapper triggers an SD-1519 merge. * * @param {Array} elements The translated paragraph elements * @returns {Array} Elements with consecutive tracked changes merged @@ -14,59 +54,50 @@ import { generateParagraphProperties } from './generate-paragraph-properties.js' function mergeConsecutiveTrackedChanges(elements) { if (!Array.isArray(elements) || elements.length === 0) return elements; + elements = foldLeadingCommentStartsIntoTrackedChanges(elements); + const result = []; let i = 0; while (i < elements.length) { const current = elements[i]; - // Check if this is a tracked change wrapper (w:ins or w:del) - if (current?.name === 'w:ins' || current?.name === 'w:del') { + if (isTrackedChangeWrapper(current)) { const tcId = current.attributes?.['w:id']; const tcName = current.name; - // Collect consecutive elements that belong to this tracked change const mergedElements = [...(current.elements || [])]; + const pendingComments = []; + let didMerge = false; let j = i + 1; while (j < elements.length) { const next = elements[j]; - // Include comment markers - they can sit inside tracked changes per ECMA-376 - if (next?.name === 'w:commentRangeStart' || next?.name === 'w:commentRangeEnd') { - mergedElements.push(next); + if (isCommentMarker(next)) { + pendingComments.push(next); j++; continue; } - // Include comment references (w:r containing w:commentReference) - if (next?.name === 'w:r') { - const hasOnlyCommentRef = next.elements?.length === 1 && next.elements[0]?.name === 'w:commentReference'; - if (hasOnlyCommentRef) { - mergedElements.push(next); - j++; - continue; - } - } - - // Merge with next tracked change if same type and ID if (next?.name === tcName && next.attributes?.['w:id'] === tcId) { - mergedElements.push(...(next.elements || [])); + mergedElements.push(...pendingComments, ...(next.elements || [])); + pendingComments.length = 0; + didMerge = true; j++; continue; } - // Stop merging when we hit a different element break; } - // Create the merged wrapper - result.push({ - name: tcName, - attributes: { ...current.attributes }, - elements: mergedElements, - }); - + if (didMerge) { + result.push({ name: tcName, attributes: { ...current.attributes }, elements: mergedElements }); + result.push(...pendingComments); + } else { + result.push(current); + result.push(...pendingComments); + } i = j; } else { result.push(current); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js index 4b40bb4f83..7121fb383c 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js @@ -82,4 +82,106 @@ describe('translateParagraphNode', () => { }); expect(generateParagraphProperties).toHaveBeenCalledWith(params); }); + + describe('mergeConsecutiveTrackedChanges (via translateParagraphNode)', () => { + const helloRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ type: 'text', text: 'Hell' }] }] }; + const commentRangeStart = { name: 'w:commentRangeStart', attributes: { 'w:id': '0' } }; + const commentRangeEnd = { name: 'w:commentRangeEnd', attributes: { 'w:id': '0' } }; + const commentReferenceRun = { + name: 'w:r', + elements: [{ name: 'w:commentReference', attributes: { 'w:id': '0' } }], + }; + const restRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ type: 'text', text: ' rest' }] }] }; + + const buildDelWrapper = (id, innerText) => ({ + name: 'w:del', + attributes: { 'w:id': id, 'w:author': 'a', 'w:date': 'd' }, + elements: [{ name: 'w:r', elements: [{ name: 'w:delText', elements: [{ type: 'text', text: innerText }] }] }], + }); + + it('folds a leading commentRangeStart into the following tracked-change wrapper (SD-2528)', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([ + helloRun, + commentRangeStart, + buildDelWrapper('1', 'o worl'), + commentRangeEnd, + commentReferenceRun, + restRun, + ]); + + const result = translateParagraphNode(params); + const names = result.elements.map((e) => e.name); + + expect(names).toEqual(['w:r', 'w:del', 'w:commentRangeEnd', 'w:r', 'w:r']); + + const delNode = result.elements.find((e) => e.name === 'w:del'); + expect(delNode.elements.map((e) => e.name)).toEqual(['w:commentRangeStart', 'w:r']); + expect(delNode.elements[0].attributes['w:id']).toBe('0'); + }); + + it('folds multiple consecutive commentRangeStart siblings into the following wrapper', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + const startA = { name: 'w:commentRangeStart', attributes: { 'w:id': '0' } }; + const startB = { name: 'w:commentRangeStart', attributes: { 'w:id': '1' } }; + translateChildNodes.mockReturnValue([startA, startB, buildDelWrapper('7', 'x')]); + + const result = translateParagraphNode(params); + + expect(result.elements).toHaveLength(1); + const delNode = result.elements[0]; + expect(delNode.elements.map((e) => e.name)).toEqual(['w:commentRangeStart', 'w:commentRangeStart', 'w:r']); + expect(delNode.elements.map((e) => e.attributes?.['w:id']).slice(0, 2)).toEqual(['0', '1']); + }); + + it('leaves a commentRangeStart as a sibling when it is not followed by a tracked-change wrapper', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([helloRun, commentRangeStart, restRun, commentRangeEnd, commentReferenceRun]); + + const result = translateParagraphNode(params); + const names = result.elements.map((e) => e.name); + + expect(names).toEqual(['w:r', 'w:commentRangeStart', 'w:r', 'w:commentRangeEnd', 'w:r']); + }); + + it('merges two consecutive same-id tracked changes and absorbs comment markers between them (SD-1519)', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([ + buildDelWrapper('5', 'first'), + commentRangeEnd, + commentReferenceRun, + buildDelWrapper('5', 'second'), + ]); + + const result = translateParagraphNode(params); + + expect(result.elements).toHaveLength(1); + const delNode = result.elements[0]; + expect(delNode.name).toBe('w:del'); + const innerNames = delNode.elements.map((e) => e.name); + // first delText run, commentRangeEnd, commentReference run, second delText run + expect(innerNames).toEqual(['w:r', 'w:commentRangeEnd', 'w:r', 'w:r']); + }); + + it('does not merge wrappers with different ids and keeps comment markers between them as siblings', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([ + buildDelWrapper('1', 'first'), + commentRangeEnd, + buildDelWrapper('2', 'second'), + ]); + + const result = translateParagraphNode(params); + const names = result.elements.map((e) => e.name); + + expect(names).toEqual(['w:del', 'w:commentRangeEnd', 'w:del']); + expect(result.elements[0].elements).toHaveLength(1); + expect(result.elements[2].elements).toHaveLength(1); + }); + }); }); diff --git a/packages/super-editor/src/editors/v1/tests/export/sd-2528-tc-comment-roundtrip.test.js b/packages/super-editor/src/editors/v1/tests/export/sd-2528-tc-comment-roundtrip.test.js new file mode 100644 index 0000000000..6fa00aa8f4 --- /dev/null +++ b/packages/super-editor/src/editors/v1/tests/export/sd-2528-tc-comment-roundtrip.test.js @@ -0,0 +1,101 @@ +import { beforeAll, describe, expect, it } from 'vitest'; +import { loadTestDataForEditorTests, initTestEditor } from '@tests/helpers/helpers.js'; +import { carbonCopy } from '@core/utilities/carbonCopy.js'; +import { importCommentData } from '@converter/v2/importer/documentCommentsImporter.js'; + +// AIDEV-NOTE: SD-2528 — a comment attached to a tracked change must remain +// associated with that tracked change after a SuperDoc round-trip (export → +// re-import). The fixture used here is a real Google-Docs-originated DOCX that +// contains four TC+comment pairings (one delete, two inserts, one replace). +// The test asserts both halves of the association: +// 1. The exported document.xml places each `w:commentRangeStart` inside the +// `w:ins`/`w:del` wrapper that owns its anchored text — the only shape the +// importer's `extractCommentRangesFromDocument` walker recognises. +// 2. After re-importing the exported XML, every comment that was originally +// `trackedChange: true` still carries that flag. + +const getCommentJSONNodes = (comment) => { + if (Array.isArray(comment.elements) && comment.elements.length) { + return comment.elements; + } + return []; +}; + +const collectTrackedChangeIdsContainingCommentStarts = (documentXml) => { + const result = new Map(); + const walk = (elements, currentTcId) => { + if (!Array.isArray(elements)) return; + for (const el of elements) { + if (el?.name === 'w:commentRangeStart' && currentTcId != null) { + result.set(el.attributes?.['w:id'], currentTcId); + } + const isTc = el?.name === 'w:ins' || el?.name === 'w:del'; + const nextTcId = isTc ? (el.attributes?.['w:id'] ?? currentTcId) : currentTcId; + walk(el?.elements, nextTcId); + } + }; + walk(documentXml?.elements ?? []); + return result; +}; + +describe('SD-2528: tracked-change + comment round-trip', () => { + const filename = 'Google Docs Originated comments & TCs.docx'; + let docx; + let media; + let mediaFiles; + let fonts; + + beforeAll(async () => { + ({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests(filename)); + }); + + it('preserves the comment ↔ tracked-change association after export and re-import', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const originalTcComments = editor.converter.comments.filter((c) => !!c.trackedChangeParentId); + expect(originalTcComments.length).toBeGreaterThan(0); + + // The id space the comments importer uses (trackedChangeIdMap) must match + // the id space the ins/del translators use (trackedChangeIdMapsByPart). + // Otherwise the user comment's trackedChangeParentId never matches the + // tracked-change mark id in the PM doc and accept/reject can't cascade. + const bodyMap = editor.converter.trackedChangeIdMapsByPart?.get('word/document.xml'); + const globalMap = editor.converter.trackedChangeIdMap; + expect(bodyMap).toBeDefined(); + expect(globalMap).toBeDefined(); + expect([...globalMap.entries()]).toEqual([...bodyMap.entries()]); + + const tcMarkIds = new Set(); + editor.state.doc.descendants((node) => { + if (!node.isInline) return; + for (const m of node.marks || []) { + if (m.type.name === 'trackInsert' || m.type.name === 'trackDelete') tcMarkIds.add(m.attrs?.id); + } + }); + originalTcComments.forEach((comment) => { + expect(tcMarkIds.has(comment.trackedChangeParentId)).toBe(true); + }); + + const commentsForExport = editor.converter.comments.map((comment) => ({ + ...comment, + commentJSON: getCommentJSONNodes(comment), + })); + + await editor.exportDocx({ comments: commentsForExport, commentsType: 'external' }); + + const exportedXml = editor.converter.convertedXml; + const startsInsideTc = collectTrackedChangeIdsContainingCommentStarts(exportedXml['word/document.xml']); + expect(startsInsideTc.size).toBeGreaterThanOrEqual(originalTcComments.length); + + const exportedDocx = carbonCopy(exportedXml); + const reimportedComments = importCommentData({ docx: exportedDocx, converter: editor.converter }) ?? []; + expect(reimportedComments.length).toBe(editor.converter.comments.length); + + const tcCommentsAfterRoundTrip = reimportedComments.filter((c) => !!c.trackedChangeParentId); + expect(tcCommentsAfterRoundTrip.length).toBe(originalTcComments.length); + } finally { + editor.destroy(); + } + }); +}); diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue index 74a5e4d922..557a541eb8 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue @@ -14,6 +14,7 @@ import { import InternalDropdown from './InternalDropdown.vue'; import CommentHeader from './CommentHeader.vue'; import CommentInput from './CommentInput.vue'; +import { collectTrackedChangeThread } from './collect-tracked-change-thread.js'; import Avatar from '@superdoc/components/general/Avatar.vue'; const emit = defineEmits(['click-outside', 'ready', 'dialog-exit', 'resize']); @@ -302,45 +303,6 @@ const startReply = () => { }); }; -const isRangeThreadedComment = (comment) => { - if (!comment) return false; - return ( - comment.threadingStyleOverride === 'range-based' || - comment.threadingMethod === 'range-based' || - comment.originalXmlStructure?.hasCommentsExtended === false - ); -}; - -const collectTrackedChangeThread = (parentComment, allComments) => { - const trackedChangeId = parentComment.commentId; - const threadIds = new Set([trackedChangeId]); - const queue = []; - - allComments.forEach((comment) => { - if (comment.commentId === trackedChangeId) return; - const isDirectChild = comment.parentCommentId === trackedChangeId; - const isRangeBasedTrackedChangeComment = - comment.trackedChangeParentId === trackedChangeId && isRangeThreadedComment(comment); - - if (isDirectChild || isRangeBasedTrackedChangeComment) { - threadIds.add(comment.commentId); - queue.push(comment.commentId); - } - }); - - for (let i = 0; i < queue.length; i += 1) { - const parentId = queue[i]; - allComments.forEach((comment) => { - if (comment.parentCommentId === parentId && !threadIds.has(comment.commentId)) { - threadIds.add(comment.commentId); - queue.push(comment.commentId); - } - }); - } - - return allComments.filter((comment) => threadIds.has(comment.commentId)); -}; - const comments = computed(() => { const parentComment = props.comment; const allComments = commentsStore.commentsList; diff --git a/packages/superdoc/src/components/CommentsLayer/CommentHeader.vue b/packages/superdoc/src/components/CommentsLayer/CommentHeader.vue index ac6be1e6e5..40aab0b779 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentHeader.vue +++ b/packages/superdoc/src/components/CommentsLayer/CommentHeader.vue @@ -57,8 +57,12 @@ const generallyAllowed = computed(() => { const allowResolve = computed(() => { if (!generallyAllowed.value) return false; - // Do not allow child comments to resolve + // Do not allow child comments to resolve. A reply anchored to a tracked + // change keeps the linkage via trackedChangeParentId (no parentCommentId), + // so treat it as a child too — otherwise re-imported TC replies render an + // extra resolve affordance that the live pre-export state doesn't (SD-2528). if (props.comment.parentCommentId) return false; + if (props.comment.trackedChangeParentId) return false; const context = { comment: props.comment, @@ -134,8 +138,15 @@ const handleSelect = (value) => emit('overflow-select', value); // Imported comments have `origin` set (e.g. 'word'); imported tracked changes // don't carry `origin` but do carry `importedAuthor` from the mark attributes. +// SD-2528: suppress the IMPORTED tag when the current user is the author — +// re-opening your own exported file shouldn't relabel your own comments as +// "imported"; that visual churn is what made round-tripping look broken. const isImported = computed(() => { - return props.comment.origin != null || !!props.comment.importedAuthor?.name; + const hasImportOrigin = props.comment.origin != null || !!props.comment.importedAuthor?.name; + if (!hasImportOrigin) return false; + const currentUserEmail = proxy.$superdoc.config.user?.email; + if (currentUserEmail && props.comment.creatorEmail === currentUserEmail) return false; + return true; }); const getCurrentUser = computed(() => { diff --git a/packages/superdoc/src/components/CommentsLayer/collect-tracked-change-thread.js b/packages/superdoc/src/components/CommentsLayer/collect-tracked-change-thread.js new file mode 100644 index 0000000000..8f497ebb29 --- /dev/null +++ b/packages/superdoc/src/components/CommentsLayer/collect-tracked-change-thread.js @@ -0,0 +1,59 @@ +// @ts-check + +/** + * Collect every comment that should appear inside the tracked-change dialog + * for `parentComment`. Walks two sources of membership: + * + * 1. **Seed**: comments anchored to this TC via `trackedChangeParentId` + * whose conversational thread *starts here* (no `parentCommentId`), + * plus direct replies whose `parentCommentId === parentComment.commentId`. + * 2. **BFS**: replies-of-replies — any comment whose `parentCommentId` + * points to something already in the thread. + * + * AIDEV-NOTE: SD-2528 P2 #3. The importer (`documentCommentsImporter.js`) + * can produce a comment with BOTH a non-TC `parentCommentId` and a + * `trackedChangeParentId`: the comment's range lives inside a TC, but its + * actual reply parent is a regular comment outside the TC. Seeding such a + * comment via `trackedChangeParentId` alone would pull it into this TC + * dialog while it ALSO renders under its real parent's thread — a + * duplicate. Restricting the direct seed to roots (no `parentCommentId`) + * lets the BFS step below pick up chains of same-TC-anchored replies (the + * common case) without shadowing parent-thread membership. + * + * Pure function — no side effects, no Vue, no store. Extracted from + * CommentDialog.vue so the logic can be unit-tested in isolation. + * + * @template {{ commentId: string, parentCommentId?: string|null, trackedChangeParentId?: string|null }} Comment + * @param {Comment} parentComment The tracked-change comment whose dialog this collects. + * @param {ReadonlyArray} allComments All known comments in the store. + * @returns {Array} Comments belonging to this tracked-change thread, in original list order. + */ +export const collectTrackedChangeThread = (parentComment, allComments) => { + const trackedChangeId = parentComment.commentId; + const threadIds = new Set([trackedChangeId]); + /** @type {string[]} */ + const queue = []; + + allComments.forEach((comment) => { + if (comment.commentId === trackedChangeId) return; + const isDirectChild = comment.parentCommentId === trackedChangeId; + const isTrackedChangeAnchoredRoot = comment.trackedChangeParentId === trackedChangeId && !comment.parentCommentId; + + if (isDirectChild || isTrackedChangeAnchoredRoot) { + threadIds.add(comment.commentId); + queue.push(comment.commentId); + } + }); + + for (let i = 0; i < queue.length; i += 1) { + const parentId = queue[i]; + allComments.forEach((comment) => { + if (comment.parentCommentId === parentId && !threadIds.has(comment.commentId)) { + threadIds.add(comment.commentId); + queue.push(comment.commentId); + } + }); + } + + return allComments.filter((comment) => threadIds.has(comment.commentId)); +}; diff --git a/packages/superdoc/src/components/CommentsLayer/collect-tracked-change-thread.test.js b/packages/superdoc/src/components/CommentsLayer/collect-tracked-change-thread.test.js new file mode 100644 index 0000000000..41250984a8 --- /dev/null +++ b/packages/superdoc/src/components/CommentsLayer/collect-tracked-change-thread.test.js @@ -0,0 +1,89 @@ +import { describe, it, expect } from 'vitest'; +import { collectTrackedChangeThread } from './collect-tracked-change-thread.js'; + +// SD-2528 P2 #3 — the TC dialog must not shadow a regular parent thread. +// The importer (`documentCommentsImporter.js`) can produce a comment with +// BOTH a non-TC `parentCommentId` AND a `trackedChangeParentId` set: the +// comment's range lives inside a TC, but its conversation thread parent is +// a separate top-level comment outside the TC. Such a reply belongs in its +// real parent's thread, not duplicated inside the TC dialog. + +const tc = { commentId: 'tc-1', trackedChange: true }; + +describe('collectTrackedChangeThread', () => { + describe('legacy / existing-fixture behaviour (preserved)', () => { + it('returns just the TC itself when no comments are anchored', () => { + const sut = collectTrackedChangeThread(tc, [tc]); + expect(sut.map((c) => c.commentId)).toEqual(['tc-1']); + }); + + it('includes a TC-anchored root comment (no parentCommentId)', () => { + const root = { commentId: 'c-root', trackedChangeParentId: 'tc-1' }; + const sut = collectTrackedChangeThread(tc, [tc, root]); + expect(sut.map((c) => c.commentId).sort()).toEqual(['c-root', 'tc-1']); + }); + + it('includes a direct reply via parentCommentId === trackedChangeId (runtime-created replies)', () => { + const reply = { commentId: 'c-direct', parentCommentId: 'tc-1' }; + const sut = collectTrackedChangeThread(tc, [tc, reply]); + expect(sut.map((c) => c.commentId).sort()).toEqual(['c-direct', 'tc-1']); + }); + + it('picks up a bi-parented reply whose parent is itself TC-anchored on the same TC (BFS chain)', () => { + // Mirrors the test fixture: imported-099ba8eb has both parentCommentId + // (its conversational parent) and trackedChangeParentId on the same TC. + const root = { commentId: 'c-root', trackedChangeParentId: 'tc-1' }; + const reply = { commentId: 'c-reply', parentCommentId: 'c-root', trackedChangeParentId: 'tc-1' }; + const sut = collectTrackedChangeThread(tc, [tc, root, reply]); + expect(sut.map((c) => c.commentId).sort()).toEqual(['c-reply', 'c-root', 'tc-1']); + }); + + it('walks the BFS through a deep chain of TC-anchored replies', () => { + const r0 = { commentId: 'r0', trackedChangeParentId: 'tc-1' }; + const r1 = { commentId: 'r1', parentCommentId: 'r0', trackedChangeParentId: 'tc-1' }; + const r2 = { commentId: 'r2', parentCommentId: 'r1', trackedChangeParentId: 'tc-1' }; + const sut = collectTrackedChangeThread(tc, [tc, r0, r1, r2]); + expect(sut.map((c) => c.commentId).sort()).toEqual(['r0', 'r1', 'r2', 'tc-1']); + }); + }); + + describe('SD-2528 P2 #3 — bi-parented reply with non-TC-anchored parent must not be in TC dialog', () => { + it('excludes a reply whose parentCommentId points to a comment that is NOT TC-anchored on this TC', () => { + // Real-world shape from documentCommentsImporter.js:199 — `rangeParent` + // can resolve to a comment whose range lives OUTSIDE the TC. The reply + // gets `parentCommentId = ` AND `trackedChangeParentId = `. + // It belongs in the non-TC parent's thread, NOT here. + const realParent = { commentId: 'real-parent' /* not TC-anchored */ }; + const biParented = { + commentId: 'c-bi-parented', + parentCommentId: 'real-parent', + trackedChangeParentId: 'tc-1', + }; + const sut = collectTrackedChangeThread(tc, [tc, realParent, biParented]); + expect(sut.map((c) => c.commentId)).not.toContain('c-bi-parented'); + }); + + it('still includes a sibling TC-anchored root even when an unrelated bi-parented reply is filtered out', () => { + const root = { commentId: 'c-root', trackedChangeParentId: 'tc-1' }; + const realParent = { commentId: 'real-parent' }; + const biParented = { + commentId: 'c-bi-parented', + parentCommentId: 'real-parent', + trackedChangeParentId: 'tc-1', + }; + const sut = collectTrackedChangeThread(tc, [tc, root, realParent, biParented]); + expect(sut.map((c) => c.commentId).sort()).toEqual(['c-root', 'tc-1']); + }); + + it('excludes a reply whose parentCommentId points to a comment anchored on a DIFFERENT TC', () => { + const otherTcRoot = { commentId: 'other-root', trackedChangeParentId: 'tc-OTHER' }; + const biParented = { + commentId: 'c-bi-parented', + parentCommentId: 'other-root', + trackedChangeParentId: 'tc-1', + }; + const sut = collectTrackedChangeThread(tc, [tc, otherTcRoot, biParented]); + expect(sut.map((c) => c.commentId)).not.toContain('c-bi-parented'); + }); + }); +}); diff --git a/packages/superdoc/src/stores/comments-store.js b/packages/superdoc/src/stores/comments-store.js index bfcba4c3ca..be788405ed 100644 --- a/packages/superdoc/src/stores/comments-store.js +++ b/packages/superdoc/src/stores/comments-store.js @@ -116,18 +116,11 @@ export const useCommentsStore = defineStore('comments', () => { return getComment(comment.parentCommentId); }; - const isRangeThreadedComment = (comment) => { - if (!comment) return false; - return ( - comment.threadingStyleOverride === 'range-based' || - comment.threadingMethod === 'range-based' || - comment.originalXmlStructure?.hasCommentsExtended === false - ); - }; - + // SD-2528: a comment anchored on a tracked change must thread under that TC + // regardless of file origin. The previous range-threaded-only guard was + // Google-Docs-only and broke SuperDoc-exported documents on re-import. const shouldThreadWithTrackedChange = (comment) => { if (!comment?.trackedChangeParentId) return false; - if (!isRangeThreadedComment(comment)) return false; const trackedChange = getComment(comment.trackedChangeParentId); return Boolean(trackedChange?.trackedChange); }; @@ -674,15 +667,54 @@ export const useCommentsStore = defineStore('comments', () => { emitTrackedChangeEvent(emitData); } else if (event === 'resolve') { const existingTrackedChange = findTrackedChangeById(); - if (!existingTrackedChange || existingTrackedChange.resolvedTime) return; - - // Selection/toolbar reject emits tracked-change resolve events. Use the same - // resolution path as the comment dialog so one method owns state + sync + emit. - existingTrackedChange.resolveComment({ + const resolveArgs = { email: params.resolvedByEmail ?? superdoc?.user?.email ?? null, name: params.resolvedByName ?? superdoc?.user?.name ?? null, superdoc, - }); + }; + + if (existingTrackedChange && !existingTrackedChange.resolvedTime) { + // Selection/toolbar reject emits tracked-change resolve events. Use the same + // resolution path as the comment dialog so one method owns state + sync + emit. + existingTrackedChange.resolveComment(resolveArgs); + } + + // AIDEV-NOTE: SD-2528. User-attached comments on a tracked change carry + // trackedChangeParentId === . When the TC is accepted + // or rejected, those comment bubbles must also resolve — otherwise the + // comment lingers after the redline it referred to is gone. Defer to a + // microtask so the cascading resolveComment doesn't dispatch into a + // still-running acceptTrackedChangeById/rejectTrackedChangeById loop and + // collide with its mutable `tr`. + // + // AIDEV-NOTE: SD-2528 P2 #1. Mirror `findTrackedChangeById`'s + // documentId scope (see line 591-596). In multi-document sessions + // tracked-change ids can collide across documents (each imported file + // has its own w:id space); without this filter, accepting a change in + // document A would cascade-resolve comments anchored on document B + // that happen to share the same id. Single-document callers (no + // documentId on the event) keep the legacy global behaviour. + if (normalizedChangeId) { + const linkedToResolve = commentsList.value.filter((linkedComment) => { + if (!linkedComment || linkedComment === existingTrackedChange) return false; + if (linkedComment.resolvedTime) return false; + const linkedParentId = + linkedComment.trackedChangeParentId != null ? String(linkedComment.trackedChangeParentId) : null; + if (linkedParentId !== normalizedChangeId) return false; + if (normalizedDocumentId) { + return belongsToTrackedChangeSyncDocument(linkedComment, normalizedDocumentId); + } + return true; + }); + if (linkedToResolve.length) { + Promise.resolve().then(() => { + linkedToResolve.forEach((linkedComment) => { + if (linkedComment.resolvedTime) return; + linkedComment.resolveComment(resolveArgs); + }); + }); + } + } } }; diff --git a/packages/superdoc/src/stores/comments-store.test.js b/packages/superdoc/src/stores/comments-store.test.js index 8d8fbd7499..03e41cde31 100644 --- a/packages/superdoc/src/stores/comments-store.test.js +++ b/packages/superdoc/src/stores/comments-store.test.js @@ -649,6 +649,180 @@ describe('comments-store', () => { ); }); + it('cascades resolve to user comments anchored to the same tracked change (SD-2528)', async () => { + const superdoc = { + emit: vi.fn(), + user: { email: 'reviewer@example.com', name: 'Reviewer' }, + }; + + const trackedChangeComment = { + commentId: 'tc-1', + trackedChange: true, + resolvedTime: null, + getValues: vi.fn(() => ({ commentId: 'tc-1' })), + resolveComment: vi.fn(function () { + this.resolvedTime = Date.now(); + }), + }; + + const linkedUserComment = { + commentId: 'user-comment-1', + trackedChange: false, + trackedChangeParentId: 'tc-1', + resolvedTime: null, + getValues: vi.fn(() => ({ commentId: 'user-comment-1' })), + resolveComment: vi.fn(function () { + this.resolvedTime = Date.now(); + }), + }; + + const unrelatedUserComment = { + commentId: 'user-comment-2', + trackedChange: false, + trackedChangeParentId: 'tc-99', + resolvedTime: null, + getValues: vi.fn(() => ({ commentId: 'user-comment-2' })), + resolveComment: vi.fn(), + }; + + store.commentsList = [trackedChangeComment, linkedUserComment, unrelatedUserComment]; + + store.handleTrackedChangeUpdate({ + superdoc, + params: { event: 'resolve', changeId: 'tc-1' }, + }); + + expect(trackedChangeComment.resolveComment).toHaveBeenCalledTimes(1); + // Cascading runs in a microtask so we wait one turn before asserting. + await Promise.resolve(); + expect(linkedUserComment.resolveComment).toHaveBeenCalledTimes(1); + expect(linkedUserComment.resolveComment).toHaveBeenCalledWith({ + email: 'reviewer@example.com', + name: 'Reviewer', + superdoc, + }); + expect(unrelatedUserComment.resolveComment).not.toHaveBeenCalled(); + }); + + // SD-2528 P2 #1 — when the resolve event carries an explicit `documentId`, + // the cascade must filter linked comments by that document. `findTrackedChangeById` + // does this for the primary comment; the cascade scan one level down was + // missing the same guard. In multi-document sessions where imported TC ids + // happen to collide, accepting/rejecting a change in one document must not + // resolve comments anchored on a different document. + it('scopes cascade resolve to the active document when documentId is provided', async () => { + const superdoc = { emit: vi.fn(), user: { email: 'reviewer@example.com', name: 'Reviewer' } }; + __mockSuperdoc.documents.value = [ + { id: 'doc-A', type: 'docx' }, + { id: 'doc-B', type: 'docx' }, + ]; + + const trackedChangeOnDocA = { + commentId: 'tc-shared', + trackedChange: true, + resolvedTime: null, + fileId: 'doc-A', + trackedChangeAnchorKey: 'tc::body::tc-shared', + getValues: vi.fn(() => ({ commentId: 'tc-shared' })), + resolveComment: vi.fn(function () { + this.resolvedTime = Date.now(); + }), + }; + const linkedOnDocA = { + commentId: 'user-on-A', + trackedChange: false, + trackedChangeParentId: 'tc-shared', + resolvedTime: null, + fileId: 'doc-A', + getValues: vi.fn(() => ({})), + resolveComment: vi.fn(), + }; + const linkedOnDocB = { + commentId: 'user-on-B', + trackedChange: false, + trackedChangeParentId: 'tc-shared', + resolvedTime: null, + fileId: 'doc-B', + getValues: vi.fn(() => ({})), + resolveComment: vi.fn(), + }; + + store.commentsList = [trackedChangeOnDocA, linkedOnDocA, linkedOnDocB]; + + store.handleTrackedChangeUpdate({ + superdoc, + params: { event: 'resolve', changeId: 'tc-shared', documentId: 'doc-A' }, + }); + + await Promise.resolve(); + expect(linkedOnDocA.resolveComment).toHaveBeenCalledTimes(1); + expect(linkedOnDocB.resolveComment).not.toHaveBeenCalled(); + }); + + // Regression: when no documentId is passed, single-document behaviour is + // unchanged. Mirrors the legacy `cascades resolve` test contract. + it('cascades to every doc-anchored linked comment when no documentId is provided (single-doc)', async () => { + const superdoc = { emit: vi.fn(), user: { email: 'r@e', name: 'R' } }; + + const trackedChangeComment = { + commentId: 'tc-nodoc', + trackedChange: true, + resolvedTime: null, + getValues: vi.fn(() => ({})), + resolveComment: vi.fn(function () { + this.resolvedTime = Date.now(); + }), + }; + const linkedNoFileId = { + commentId: 'user-nodoc', + trackedChange: false, + trackedChangeParentId: 'tc-nodoc', + resolvedTime: null, + getValues: vi.fn(() => ({})), + resolveComment: vi.fn(), + }; + + store.commentsList = [trackedChangeComment, linkedNoFileId]; + + store.handleTrackedChangeUpdate({ + superdoc, + params: { event: 'resolve', changeId: 'tc-nodoc' }, + }); + + await Promise.resolve(); + expect(linkedNoFileId.resolveComment).toHaveBeenCalledTimes(1); + }); + + it('does not re-resolve already-resolved linked user comments', async () => { + const superdoc = { emit: vi.fn(), user: { email: 'a@a', name: 'A' } }; + + const trackedChangeComment = { + commentId: 'tc-2', + trackedChange: true, + resolvedTime: null, + getValues: vi.fn(() => ({})), + resolveComment: vi.fn(function () { + this.resolvedTime = Date.now(); + }), + }; + + const alreadyResolvedLinked = { + commentId: 'user-2', + trackedChange: false, + trackedChangeParentId: 'tc-2', + resolvedTime: 1234, + getValues: vi.fn(() => ({})), + resolveComment: vi.fn(), + }; + + store.commentsList = [trackedChangeComment, alreadyResolvedLinked]; + + store.handleTrackedChangeUpdate({ superdoc, params: { event: 'resolve', changeId: 'tc-2' } }); + + await Promise.resolve(); + expect(alreadyResolvedLinked.resolveComment).not.toHaveBeenCalled(); + }); + it('syncs and emits an update when add event dedupes an existing tracked change', () => { const superdoc = { emit: vi.fn(),