diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js b/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js index c6b7eb41e1..9a4867d27d 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js @@ -295,9 +295,11 @@ describe('CommentDialog.vue', () => { // Custom handler should be called expect(customAcceptHandler).toHaveBeenCalledWith(baseComment, superdocStub.activeEditor); - // Default behavior should NOT be called + // Default accept command should NOT be called (custom handler replaces it) expect(superdocStub.activeEditor.commands.acceptTrackedChangeById).not.toHaveBeenCalled(); - expect(baseComment.resolveComment).not.toHaveBeenCalled(); + + // resolveComment should ALWAYS be called to prevent ghost bubbles (SD-2049) + expect(baseComment.resolveComment).toHaveBeenCalled(); // Cleanup should still happen await nextTick(); @@ -325,9 +327,11 @@ describe('CommentDialog.vue', () => { // Custom handler should be called expect(customRejectHandler).toHaveBeenCalledWith(baseComment, superdocStub.activeEditor); - // Default behavior should NOT be called + // Default reject command should NOT be called (custom handler replaces it) expect(superdocStub.activeEditor.commands.rejectTrackedChangeById).not.toHaveBeenCalled(); - expect(baseComment.resolveComment).not.toHaveBeenCalled(); + + // resolveComment should ALWAYS be called to prevent ghost bubbles (SD-2049) + expect(baseComment.resolveComment).toHaveBeenCalled(); // Cleanup should still happen await nextTick(); @@ -399,9 +403,11 @@ describe('CommentDialog.vue', () => { // Handler was called expect(noOpHandler).toHaveBeenCalledWith(baseComment, superdocStub.activeEditor); - // Default behavior should NOT run + // Default accept command should NOT run (custom handler replaces it) expect(superdocStub.activeEditor.commands.acceptTrackedChangeById).not.toHaveBeenCalled(); - expect(baseComment.resolveComment).not.toHaveBeenCalled(); + + // resolveComment should ALWAYS be called to prevent ghost bubbles (SD-2049) + expect(baseComment.resolveComment).toHaveBeenCalled(); // Cleanup should still happen (dialog closes even though handler did nothing) await nextTick(); diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue index 5a5c112a44..e4b9f03cd8 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue @@ -356,17 +356,21 @@ const handleReject = () => { const customHandler = proxy.$superdoc.config.onTrackedChangeBubbleReject; if (props.comment.trackedChange && typeof customHandler === 'function') { - // Custom handler replaces default behavior customHandler(props.comment, proxy.$superdoc.activeEditor); } else if (props.comment.trackedChange) { + proxy.$superdoc.activeEditor.commands.rejectTrackedChangeById(props.comment.commentId); + } else { + commentsStore.deleteComment({ superdoc: proxy.$superdoc, commentId: props.comment.commentId }); + } + + // Always resolve tracked changes so resolvedTime is set and the bubble + // disappears from getFloatingComments — even when a custom handler is used (SD-2049). + if (props.comment.trackedChange) { props.comment.resolveComment({ email: superdocStore.user.email, name: superdocStore.user.name, superdoc: proxy.$superdoc, }); - proxy.$superdoc.activeEditor.commands.rejectTrackedChangeById(props.comment.commentId); - } else { - commentsStore.deleteComment({ superdoc: proxy.$superdoc, commentId: props.comment.commentId }); } // Always cleanup the dialog state @@ -381,20 +385,21 @@ const handleResolve = () => { const customHandler = proxy.$superdoc.config.onTrackedChangeBubbleAccept; if (props.comment.trackedChange && typeof customHandler === 'function') { - // Custom handler replaces default behavior customHandler(props.comment, proxy.$superdoc.activeEditor); } else { if (props.comment.trackedChange) { proxy.$superdoc.activeEditor.commands.acceptTrackedChangeById(props.comment.commentId); } - - props.comment.resolveComment({ - email: superdocStore.user.email, - name: superdocStore.user.name, - superdoc: proxy.$superdoc, - }); } + // Always resolve so resolvedTime is set and the bubble disappears + // from getFloatingComments — even when a custom handler is used (SD-2049). + props.comment.resolveComment({ + email: superdocStore.user.email, + name: superdocStore.user.name, + superdoc: proxy.$superdoc, + }); + // Always cleanup the dialog state nextTick(() => { commentsStore.lastUpdate = new Date(); diff --git a/packages/superdoc/src/stores/comments-store.test.js b/packages/superdoc/src/stores/comments-store.test.js index 3c57fa3a29..d3de3f69e1 100644 --- a/packages/superdoc/src/stores/comments-store.test.js +++ b/packages/superdoc/src/stores/comments-store.test.js @@ -890,4 +890,61 @@ describe('comments-store', () => { expect(comment.resolvedByName).toBe('User'); }); }); + + describe('getFloatingComments filters resolved tracked changes', () => { + it('includes unresolved tracked changes that have position keys', () => { + store.commentsList = [ + { commentId: 'tc-1', trackedChange: true, resolvedTime: null, createdTime: 1 }, + { commentId: 'tc-2', trackedChange: true, resolvedTime: null, createdTime: 2 }, + ]; + store.editorCommentPositions = { + 'tc-1': { start: 1, end: 5, bounds: { top: 0, left: 0 } }, + 'tc-2': { start: 10, end: 15, bounds: { top: 0, left: 0 } }, + }; + + const floating = store.getFloatingComments; + expect(floating.map((c) => c.commentId)).toEqual(['tc-1', 'tc-2']); + }); + + it('excludes tracked changes once resolvedTime is set', () => { + store.commentsList = [ + { commentId: 'tc-1', trackedChange: true, resolvedTime: Date.now(), createdTime: 1 }, + { commentId: 'tc-2', trackedChange: true, resolvedTime: null, createdTime: 2 }, + ]; + store.editorCommentPositions = { + 'tc-1': { start: 1, end: 5, bounds: { top: 0, left: 0 } }, + 'tc-2': { start: 10, end: 15, bounds: { top: 0, left: 0 } }, + }; + + const floating = store.getFloatingComments; + expect(floating.map((c) => c.commentId)).toEqual(['tc-2']); + }); + + it('excludes the last tracked change when resolved (regression: SD-2049)', () => { + store.commentsList = [{ commentId: 'tc-only', trackedChange: true, resolvedTime: Date.now(), createdTime: 1 }]; + // Position key still present (editor doesn't fire update for last mark removal) + store.editorCommentPositions = { + 'tc-only': { start: 1, end: 5, bounds: { top: 0, left: 0 } }, + }; + + const floating = store.getFloatingComments; + expect(floating).toEqual([]); + }); + + it('returns empty when all tracked changes are resolved', () => { + store.commentsList = [ + { commentId: 'tc-1', trackedChange: true, resolvedTime: Date.now(), createdTime: 1 }, + { commentId: 'tc-2', trackedChange: true, resolvedTime: Date.now(), createdTime: 2 }, + { commentId: 'tc-3', trackedChange: true, resolvedTime: Date.now(), createdTime: 3 }, + ]; + store.editorCommentPositions = { + 'tc-1': { start: 1, end: 5, bounds: { top: 0, left: 0 } }, + 'tc-2': { start: 10, end: 15, bounds: { top: 0, left: 0 } }, + 'tc-3': { start: 20, end: 25, bounds: { top: 0, left: 0 } }, + }; + + const floating = store.getFloatingComments; + expect(floating).toEqual([]); + }); + }); }); diff --git a/tests/behavior/tests/comments/sd-2049-custom-tc-handler-ghost-bubble.spec.ts b/tests/behavior/tests/comments/sd-2049-custom-tc-handler-ghost-bubble.spec.ts new file mode 100644 index 0000000000..cc9e69c1ea --- /dev/null +++ b/tests/behavior/tests/comments/sd-2049-custom-tc-handler-ghost-bubble.spec.ts @@ -0,0 +1,117 @@ +import { test, expect } from '../../fixtures/superdoc.js'; +import { assertDocumentApiReady, listTrackChanges } from '../../helpers/document-api.js'; +import { activateCommentDialog } from '../../helpers/comments.js'; + +test.use({ config: { toolbar: 'full', comments: 'on', trackChanges: true } }); + +test('SD-2049 last TC bubble disappears when using custom accept handler', async ({ superdoc }) => { + await assertDocumentApiReady(superdoc.page); + + // Type two words on separate lines so we get two independent tracked changes + await superdoc.type('first'); + await superdoc.newLine(); + await superdoc.type('second'); + await superdoc.waitForStable(); + + // Switch to suggesting mode + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + + // Create tracked change on "first" — select and replace + const pos1 = await superdoc.findTextPos('first'); + await superdoc.setTextSelection(pos1, pos1 + 'first'.length); + await superdoc.waitForStable(); + await superdoc.type('alpha'); + await superdoc.waitForStable(); + + // Create tracked change on "second" — select and replace + const pos2 = await superdoc.findTextPos('second'); + await superdoc.setTextSelection(pos2, pos2 + 'second'.length); + await superdoc.waitForStable(); + await superdoc.type('beta'); + await superdoc.waitForStable(); + + // Wait for both tracked changes to be registered + await expect.poll(async () => (await listTrackChanges(superdoc.page)).total).toBeGreaterThanOrEqual(2); + + // Inject custom accept handler — this is the SD-2049 scenario + await superdoc.page.evaluate(() => { + const sd = (window as any).superdoc; + sd.config.onTrackedChangeBubbleAccept = (comment: any, editor: any) => { + editor.commands.acceptTrackedChangeById(comment.commentId); + }; + }); + + // Accept tracked changes one by one via bubble UI + const dialog1 = await activateCommentDialog(superdoc, 'alpha'); + await expect(dialog1).toBeVisible({ timeout: 5_000 }); + + // Click the accept button — use force because the overflow-menu icon + // sits inside the comment-header and Playwright sees the header as intercepting + await dialog1.locator('.overflow-menu .overflow-menu__icon').first().click({ force: true }); + await superdoc.waitForStable(); + await superdoc.page.waitForTimeout(500); + + // Now handle the second (last) tracked change — this is where SD-2049 would fail + const remainingTCs = await listTrackChanges(superdoc.page); + + if (remainingTCs.total > 0) { + const dialog2 = await activateCommentDialog(superdoc, 'beta'); + await expect(dialog2).toBeVisible({ timeout: 5_000 }); + + // Accept the last bubble + await dialog2.locator('.overflow-menu .overflow-menu__icon').first().click({ force: true }); + await superdoc.waitForStable(); + } + + // The key assertion: no unresolved floating comment dialogs should remain. + // Before the SD-2049 fix, the last bubble would persist as a ghost. + await expect(superdoc.page.locator('.comment-placeholder .comments-dialog:not(.is-resolved)')).toHaveCount(0, { + timeout: 5_000, + }); + + await superdoc.snapshot('sd-2049-no-ghost-bubbles'); +}); + +test('SD-2049 last TC bubble disappears when using custom reject handler', async ({ superdoc }) => { + await assertDocumentApiReady(superdoc.page); + + await superdoc.type('hello world'); + await superdoc.waitForStable(); + + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + + // Create a single tracked change + const pos = await superdoc.findTextPos('hello'); + await superdoc.setTextSelection(pos, pos + 'hello'.length); + await superdoc.waitForStable(); + await superdoc.type('goodbye'); + await superdoc.waitForStable(); + + await expect.poll(async () => (await listTrackChanges(superdoc.page)).total).toBeGreaterThanOrEqual(1); + + // Inject custom reject handler + await superdoc.page.evaluate(() => { + const sd = (window as any).superdoc; + sd.config.onTrackedChangeBubbleReject = (comment: any, editor: any) => { + editor.commands.rejectTrackedChangeById(comment.commentId); + }; + }); + + // Activate the TC bubble and click reject (second icon in overflow menu) + const dialog = await activateCommentDialog(superdoc, 'goodbye'); + await expect(dialog).toBeVisible({ timeout: 5_000 }); + await dialog.locator('.overflow-menu .overflow-menu__icon').nth(1).click({ force: true }); + await superdoc.waitForStable(); + + // No ghost bubbles should remain + await expect(superdoc.page.locator('.comment-placeholder .comments-dialog:not(.is-resolved)')).toHaveCount(0, { + timeout: 5_000, + }); + + // Text should be reverted since we rejected + await superdoc.assertTextContains('hello'); + + await superdoc.snapshot('sd-2049-reject-no-ghost-bubble'); +});