diff --git a/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.js b/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.js index cdb7b6dfdf..f42b4feadd 100644 --- a/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.js +++ b/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.js @@ -507,11 +507,10 @@ export const CommentsPlugin = Extension.create({ } // Check for changes in the actively selected comment - const trChangedActiveComment = meta?.type === 'setActiveComment'; - if ((!tr.docChanged && tr.selectionSet) || trChangedActiveComment) { + if (!tr.docChanged && tr.selectionSet) { const { selection } = tr; + let currentActiveThread = getActiveCommentId(newEditorState.doc, selection); - if (trChangedActiveComment) currentActiveThread = meta.activeThreadId; if ( meta?.type === 'setCursorById' && meta.preferredActiveThreadId && @@ -521,7 +520,20 @@ export const CommentsPlugin = Extension.create({ } const previousSelectionId = pluginState.activeThreadId; - if (previousSelectionId !== currentActiveThread) { + // getActiveCommentId returns undefined for any non-collapsed selection + // (its first line is `if ($from.pos !== $to.pos) return;`), and that + // undefined gets coerced to null below, which would emit + // commentsUpdate({activeCommentId: null}) and clear an active comment. + // For tracked-change comments, presentation.navigateTo dispatches a + // collapsed cursor placement immediately followed by a non-collapsed + // NodeSelection on the SDT wrapper. That second tx would otherwise + // overwrite the just-activated comment, the host's commentsUpdate + // listener would re-assert it, and the loop would flicker forever + // (issue #2861). Treat "undefined from non-collapsed" as "no + // information" rather than "no active comment". + const isNonCollapsedClear = + currentActiveThread == null && selection && selection.$from.pos !== selection.$to.pos; + if (previousSelectionId !== currentActiveThread && !isNonCollapsedClear) { // Update both the plugin state and the local variable pluginState.activeThreadId = currentActiveThread; const update = { diff --git a/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.test.js b/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.test.js index 7d300ae1b9..6236ae3f07 100644 --- a/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.test.js +++ b/packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.test.js @@ -1641,6 +1641,149 @@ describe('SD-1940: no recursive dispatch from apply() on selection change', () = }); }); +// Issue #2861: clicking a comment whose range overlaps a tracked change inside an inline +// SDT triggers `presentation.navigateTo`, which dispatches a collapsed cursor placement +// followed by a non-collapsed NodeSelection on the SDT wrapper. `getActiveCommentId` early- +// returns `undefined` for any non-collapsed selection, and the plugin used to coerce that +// `undefined` into `commentsUpdate({activeCommentId: null})`, clearing the just-activated +// comment. The host's `commentsUpdate` listener then re-asserts the comment, the plugin +// re-emits, and the highlight class flickers ~400 times/second until something else changes. +describe('SD-2861: non-collapsed selection does not clear active comment', () => { + it('preserves activeThreadId when a non-collapsed selection follows explicit activation', () => { + const schema = createCommentSchema(); + const commentMark = schema.marks[CommentMarkName].create({ commentId: 'thread-1', internal: true }); + // "Hello" (1..6) carries the comment, " World" (6..12) does not. + const paragraph = schema.node('paragraph', null, [schema.text('Hello', [commentMark]), schema.text(' World')]); + const doc = schema.node('doc', null, [paragraph]); + const { view, editor } = createPluginStateEnvironment({ schema, doc }); + + // Step 1: Explicitly activate the comment (mirrors the sidebar click path). + view.dispatch( + view.state.tr.setMeta(CommentsPluginKey, { + type: 'setActiveComment', + activeThreadId: 'thread-1', + forceUpdate: true, + }), + ); + expect(CommentsPluginKey.getState(view.state).activeThreadId).toBe('thread-1'); + editor.emit.mockClear(); + + // Step 2: Non-collapsed selection that straddles the comment boundary, mimicking the + // NodeSelection that `presentation.navigateTo` produces on the SDT wrapper. + view.dispatch(view.state.tr.setSelection(TextSelection.create(view.state.doc, 1, 8))); + + // The active comment must survive. `getActiveCommentId` returned undefined (because the + // selection was non-collapsed); the plugin used to treat that as "no comment" and emit + // commentsUpdate({activeCommentId: null}). It must not. + expect(CommentsPluginKey.getState(view.state).activeThreadId).toBe('thread-1'); + expect(editor.emit).not.toHaveBeenCalledWith('commentsUpdate', expect.objectContaining({ activeCommentId: null })); + }); + + it('preserves activeThreadId when a follow-up non-collapsed selection sits entirely within the comment range', () => { + const schema = createCommentSchema(); + const commentMark = schema.marks[CommentMarkName].create({ commentId: 'thread-1', internal: true }); + const trackedMark = schema.marks[TrackInsertMarkName].create({ id: 'tc-1' }); + // The repro case from #2861: text carries both a comment mark and a tracked-change mark. + const paragraph = schema.node('paragraph', null, [schema.text('WYSIWYG', [commentMark, trackedMark])]); + const doc = schema.node('doc', null, [paragraph]); + const { view, editor } = createPluginStateEnvironment({ schema, doc }); + + view.dispatch( + view.state.tr.setMeta(CommentsPluginKey, { + type: 'setActiveComment', + activeThreadId: 'thread-1', + forceUpdate: true, + }), + ); + editor.emit.mockClear(); + + // Non-collapsed selection that wraps the entire comment range — what navigateTo + // produces when it lands a NodeSelection on the SDT containing the tracked change. + view.dispatch(view.state.tr.setSelection(TextSelection.create(view.state.doc, 1, 8))); + + expect(CommentsPluginKey.getState(view.state).activeThreadId).toBe('thread-1'); + expect(editor.emit).not.toHaveBeenCalledWith('commentsUpdate', expect.objectContaining({ activeCommentId: null })); + }); + + it('still clears activeThreadId when a collapsed cursor moves outside the comment range', () => { + // Regression guard: the fix only suppresses clears for non-collapsed selections. + // A real "user moved off the comment" interaction (collapsed cursor outside any comment) + // must still propagate. + const schema = createCommentSchema(); + const commentMark = schema.marks[CommentMarkName].create({ commentId: 'thread-1', internal: true }); + const paragraph = schema.node('paragraph', null, [schema.text('Hello', [commentMark]), schema.text(' World')]); + const doc = schema.node('doc', null, [paragraph]); + const { view, editor } = createPluginStateEnvironment({ schema, doc }); + + view.dispatch( + view.state.tr.setMeta(CommentsPluginKey, { + type: 'setActiveComment', + activeThreadId: 'thread-1', + forceUpdate: true, + }), + ); + editor.emit.mockClear(); + + // Collapsed cursor at pos 9 lands inside " World", which has no comment mark. + view.dispatch(view.state.tr.setSelection(TextSelection.create(view.state.doc, 9))); + + expect(CommentsPluginKey.getState(view.state).activeThreadId).toBeNull(); + expect(editor.emit).toHaveBeenCalledWith('commentsUpdate', expect.objectContaining({ activeCommentId: null })); + }); + + it('does not enter a dispatch loop when a host listener re-asserts after each commentsUpdate', () => { + // Models the live system: SuperDoc.vue's onEditorCommentsUpdate calls + // commentsStore.setActiveComment(superdoc, payload.activeCommentId), which dispatches + // another setActiveComment meta back into the editor. Before the fix, the cursor + range + // tx pair from navigateTo emitted (id, null) which the listener echoed back, looping. + const schema = createCommentSchema(); + const commentMark = schema.marks[CommentMarkName].create({ commentId: 'thread-1', internal: true }); + const paragraph = schema.node('paragraph', null, [schema.text('Hello', [commentMark])]); + const doc = schema.node('doc', null, [paragraph]); + + const { editor, view, extension } = createEditorEnvironment(schema, doc); + const plugins = extension.addPmPlugins(); + view.state = EditorState.create({ schema, doc, selection: TextSelection.create(doc, 1), plugins }); + + let dispatchCount = 0; + const originalDispatch = (tr) => { + view.state = view.state.apply(tr); + }; + view.dispatch = vi.fn((tr) => { + dispatchCount += 1; + if (dispatchCount > 10) throw new Error('Dispatch loop detected — exceeded 10 dispatches'); + originalDispatch(tr); + }); + + // Mirror the host's reaction: every commentsUpdate triggers a setActiveComment meta back + // into the editor. The store does this even when the payload's activeCommentId === null. + editor.emit = vi.fn((eventName, payload) => { + if (eventName !== 'commentsUpdate') return; + view.dispatch( + view.state.tr.setMeta(CommentsPluginKey, { + type: 'setActiveComment', + activeThreadId: payload?.activeCommentId ?? null, + forceUpdate: true, + }), + ); + }); + + // Tx 1: cursor placement at pos 3 inside the comment. + view.dispatch( + view.state.tr + .setSelection(TextSelection.create(view.state.doc, 3)) + .setMeta(CommentsPluginKey, { type: 'setCursorById', preferredActiveThreadId: 'thread-1' }), + ); + // Tx 2: non-collapsed selection — the SDT-wrapper case from navigateTo. + view.dispatch(view.state.tr.setSelection(TextSelection.create(view.state.doc, 1, 6))); + + // Without the fix, dispatchCount would explode (each clear emit triggers another + // setActiveComment dispatch). The non-collapsed-clear suppression keeps it bounded. + expect(dispatchCount).toBeLessThanOrEqual(5); + expect(CommentsPluginKey.getState(view.state).activeThreadId).toBe('thread-1'); + }); +}); + describe('Headless mode plugin behavior', () => { it('creates a state-only plugin in headless mode (no props or view)', () => { const editor = { diff --git a/tests/behavior/tests/comments/comment-on-tracked-change.spec.ts b/tests/behavior/tests/comments/comment-on-tracked-change.spec.ts index d7228e2308..873e14f7cd 100644 --- a/tests/behavior/tests/comments/comment-on-tracked-change.spec.ts +++ b/tests/behavior/tests/comments/comment-on-tracked-change.spec.ts @@ -147,3 +147,115 @@ test('switching highlighted threads does not trigger a second delayed floating-s await expect(targetDialog.locator('.comment-body .comment').nth(0)).toContainText('abc'); await expect(targetDialog.locator('.comment-body .comment').nth(1)).toContainText('xyz'); }); + +// SD-2861 regression: explicit comment activation followed by a non-collapsed selection +// (the shape `presentation.navigateTo` produces when landing a NodeSelection on the SDT +// wrapper around a tracked change) must not enter a feedback loop. The plugin used to +// coerce `getActiveCommentId`'s `undefined` return for non-collapsed selections into +// `commentsUpdate({activeCommentId: null})`. The Vue host re-asserted the comment, the +// plugin re-emitted, and `.track-change-focused` toggled ~400 times/second. +// +// This test programmatically reproduces the two-transaction pattern at the live editor +// level so it covers the integrated path (plugin -> PresentationEditor bridge -> store +// listener -> commands.setActiveComment) without depending on a fixture that happens to +// have an SDT-wrapped tracked change. +test('explicit comment activation survives a follow-up non-collapsed selection', async ({ superdoc }) => { + await superdoc.loadDocument(DOC_PATH); + await superdoc.page.waitForSelector('.superdoc-comment-highlight', { timeout: 30_000 }); + await superdoc.waitForStable(); + await assertDocumentApiReady(superdoc.page); + + const result = await superdoc.page.evaluate(async () => { + const editor = (window as any).editor; + const view = editor?.view; + if (!view?.dispatch) throw new Error('editor.view.dispatch not available'); + + // Find the first comment-marked range in the doc and capture its id + bounds. + const commentInfo = ((): { id: string; from: number; to: number } | null => { + let id: string | null = null; + let from = -1; + let to = -1; + view.state.doc.descendants((node: any, pos: number) => { + for (const mark of node.marks ?? []) { + if (mark.type.name !== 'commentMark') continue; + const candidateId = mark.attrs?.commentId ?? mark.attrs?.importedId; + if (!candidateId) continue; + if (id === null) { + id = candidateId; + from = pos; + to = pos + node.nodeSize; + } else if (candidateId === id) { + to = Math.max(to, pos + node.nodeSize); + } + } + }); + return id !== null ? { id, from, to } : null; + })(); + if (!commentInfo) throw new Error('No comment-marked range found in fixture'); + + let dispatchCount = 0; + const originalDispatch = view.dispatch.bind(view); + view.dispatch = (tr: unknown) => { + dispatchCount += 1; + return originalDispatch(tr); + }; + + let toggles = 0; + const observer = new MutationObserver((muts) => { + muts.forEach((m) => { + if (m.attributeName !== 'class') return; + const oldVal = String(m.oldValue ?? ''); + const newVal = String((m.target as Element).className ?? ''); + if (oldVal === newVal) return; + const before = oldVal.includes('track-change-focused'); + const after = newVal.includes('track-change-focused'); + if (before !== after) toggles += 1; + }); + }); + const pages = document.querySelector('.presentation-editor__pages'); + if (pages) { + observer.observe(pages, { + attributes: true, + attributeOldValue: true, + subtree: true, + attributeFilter: ['class'], + }); + } + + try { + // Tx 1: explicit activation, mirrors the sidebar-click path. + editor.commands.setActiveComment({ commentId: commentInfo.id }); + + // Tx 2: non-collapsed selection that wraps the comment range, mirrors the + // NodeSelection from `presentation.navigateTo` on an SDT wrapper. + const SelectionCtor = view.state.selection.constructor as any; + view.dispatch(view.state.tr.setSelection(SelectionCtor.create(view.state.doc, commentInfo.from, commentInfo.to))); + + // Sample for 800ms. With the bug, the (id, null) emit pair plus the host re-assert + // produces 200+ toggles/sec; the fix keeps it bounded. + await new Promise((r) => setTimeout(r, 800)); + } finally { + observer.disconnect(); + view.dispatch = originalDispatch; + } + + const activePluginState = view.state.plugins + .map((p: any) => p.getState?.(view.state)) + .find((s: any) => s && 'activeThreadId' in s); + + return { + dispatchCount, + toggles, + finalActiveThreadId: activePluginState?.activeThreadId ?? null, + expectedActiveCommentId: commentInfo.id, + }; + }); + + await superdoc.waitForStable(); + + // Without the fix: dispatchCount climbs into the dozens (the host re-asserts on every + // commentsUpdate emit) and toggles climbs into the hundreds. With the fix: bounded. + expect(result.dispatchCount).toBeLessThanOrEqual(15); + expect(result.toggles).toBeLessThanOrEqual(3); + expect(result.finalActiveThreadId).toBe(result.expectedActiveCommentId); +});