From fb6c4e6c224089abe52843b4928dedbf79deeea4 Mon Sep 17 00:00:00 2001 From: Gabriel Chittolina Date: Mon, 11 May 2026 16:16:56 -0300 Subject: [PATCH 1/3] fix: skip track changes when resizing images --- .../src/editors/v1/components/ImageResizeOverlay.vue | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue b/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue index 03744143d2..f8d23de81e 100644 --- a/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue +++ b/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue @@ -583,7 +583,11 @@ function dispatchResizeTransaction(blockId, newWidth, newHeight) { tr.setNodeMarkup(imagePos, null, newAttrs); - // Dispatch transaction + // Word does not track image resizes as revisions; bypass tracking so the + // attribute change applies in place instead of being split into a tracked + // insert + delete (which would render as a duplicate image — SD-2974). + tr.setMeta('skipTrackChanges', true); + dispatch(tr); // Invalidate the measure cache for this image to force re-measurement with new size From d3af32b9c9872ebec0516a04e3e8abd5515f16a0 Mon Sep 17 00:00:00 2001 From: Gabriel Chittolina Date: Mon, 11 May 2026 16:45:02 -0300 Subject: [PATCH 2/3] test: behavior test to ensure image is not duplicated in tracked change mode --- .../image-resize-in-suggesting-mode.spec.ts | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts diff --git a/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts b/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts new file mode 100644 index 0000000000..aec89146fe --- /dev/null +++ b/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts @@ -0,0 +1,75 @@ +import { test, expect } from '../../fixtures/superdoc.js'; +import path from 'node:path'; + +/** + * SD-2974: resizing an image in suggesting mode must not produce a duplicate + * image. Word does not track image resizes as revisions; the resize should + * apply in place rather than being split into a tracked insert + delete. + */ + +test.use({ config: { toolbar: 'full', showSelection: true, trackChanges: true } }); + +const FIXTURE = path.resolve(import.meta.dirname, 'fixtures/sd-2323-image-resize-test.docx'); + +async function getImageCount(superdoc: any): Promise { + return superdoc.page.evaluate(() => { + const doc = (window as any).editor?.state?.doc; + if (!doc) throw new Error('Editor document is unavailable.'); + let count = 0; + doc.descendants((node: any) => { + if (node.type?.name === 'image') count += 1; + }); + return count; + }); +} + +test.describe('Image resize in suggesting mode (SD-2974)', () => { + test.beforeEach(async ({ superdoc }) => { + await superdoc.loadDocument(FIXTURE); + }); + + test('@behavior SD-2974: resizing an image in suggesting mode does not duplicate the image', async ({ superdoc }) => { + // Sanity: exactly one image to start with. + expect(await getImageCount(superdoc)).toBe(1); + + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + await superdoc.assertDocumentMode('suggesting'); + + // Hover the image to surface the resize overlay. + const img = superdoc.page.locator('.superdoc-inline-image').first(); + await expect(img).toBeAttached({ timeout: 5000 }); + await img.hover(); + await superdoc.waitForStable(); + + const overlay = superdoc.page.locator('.superdoc-image-resize-overlay'); + await expect(overlay).toBeAttached({ timeout: 5000 }); + + // Drag the SE handle to shrink the image. The exact end-size doesn't + // matter — we only need a commit large enough to exceed the + // 1px DIMENSION_CHANGE_THRESHOLD so the transaction actually dispatches. + const handle = overlay.locator('.resize-handle--se'); + await expect(handle).toBeAttached({ timeout: 5000 }); + const handleBox = await handle.boundingBox(); + if (!handleBox) throw new Error('Could not locate SE resize handle.'); + + const startX = handleBox.x + handleBox.width / 2; + const startY = handleBox.y + handleBox.height / 2; + + await superdoc.page.mouse.move(startX, startY); + await superdoc.page.mouse.down(); + // Move toward NW to shrink. Use multiple steps so the throttled + // mousemove handler in ImageResizeOverlay.vue produces an updated + // constrainedWidth/Height before mouseup commits. + await superdoc.page.mouse.move(startX - 60, startY - 60, { steps: 10 }); + await superdoc.page.mouse.up(); + + await superdoc.waitForStable(); + + // After the resize commit there must still be exactly one image node. + // Before the SD-2974 fix the tracked-change machinery split the + // ReplaceStep (generated by setNodeMarkup on the leaf image node) into + // a tracked insert + delete, leaving two image nodes in the document. + expect(await getImageCount(superdoc)).toBe(1); + }); +}); From 82848f89daa1c2321b707b6fd3938cecb7e896ae Mon Sep 17 00:00:00 2001 From: Gabriel Chittolina Date: Wed, 3 Jun 2026 12:13:27 -0300 Subject: [PATCH 3/3] fix(images): resize via AttrStep instead of skipTrackChanges meta (SD-2974) Address review feedback: - Replace setNodeMarkup + skipTrackChanges with tr.setNodeAttribute, which emits an AttrStep. AttrSteps pass through the track-changes machinery untracked and never trip the protect-tracked-review-state guard, so the resize applies in place even when the image is itself a pending tracked insertion. - Bump the containing blocks' sdBlockRev in the same transaction: AttrSteps have no changed range, so blockNodePlugin never increments the rev and the layout engine would reuse the cached FlowBlock, leaving the paint stale (same pattern as numberingPlugin's bumpBlockRev). - Harden the behavior test against false-greens by asserting the image's size attrs actually changed after the drag. - Add coverage for repeated resizes (AC #3) and for resizing an image that is itself a pending tracked insertion. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v1/components/ImageResizeOverlay.test.js | 39 ++-- .../v1/components/ImageResizeOverlay.vue | 38 ++-- .../image-resize-in-suggesting-mode.spec.ts | 185 +++++++++++++++--- 3 files changed, 210 insertions(+), 52 deletions(-) diff --git a/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.test.js b/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.test.js index fb6faf50a6..093d5f128d 100644 --- a/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.test.js +++ b/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.test.js @@ -9,12 +9,29 @@ vi.mock('@superdoc/layout-bridge', () => ({ })); function createMockEditor(overrides = {}) { + // Block ancestor carrying sdBlockRev, as resolved when bumping the + // containing block's revision after the AttrStep resize. + const paragraphNode = { + isBlock: true, + type: { name: 'paragraph', spec: { attrs: { sdBlockId: {}, sdBlockRev: {} } } }, + attrs: { sdBlockRev: 3 }, + }; return { options: { documentMode: 'editing' }, isEditable: true, view: { dom: document.createElement('div'), - state: { doc: { nodeAt: vi.fn() }, tr: { setNodeMarkup: vi.fn().mockReturnThis() } }, + state: { + doc: { + nodeAt: vi.fn(), + resolve: vi.fn(() => ({ + depth: 1, + node: () => paragraphNode, + before: () => 0, + })), + }, + tr: { setNodeAttribute: vi.fn().mockReturnThis() }, + }, dispatch: vi.fn(), }, ...overrides, @@ -153,7 +170,7 @@ describe('ImageResizeOverlay', () => { expect(wrapper.vm.dragState).toBe(null); expect(wrapper.find('.resize-guideline').exists()).toBe(false); expect(editor.view.dispatch).not.toHaveBeenCalled(); - expect(editor.view.state.tr.setNodeMarkup).not.toHaveBeenCalled(); + expect(editor.view.state.tr.setNodeAttribute).not.toHaveBeenCalled(); wrapper.unmount(); remove(); @@ -207,11 +224,10 @@ describe('ImageResizeOverlay', () => { document.dispatchEvent(new MouseEvent('mousemove', { clientX: 150, clientY: 90 })); document.dispatchEvent(new MouseEvent('mouseup', { clientX: 150, clientY: 90 })); - expect(editor.view.state.tr.setNodeMarkup).toHaveBeenCalledWith( - 0, - null, - expect.objectContaining({ size: { width: 140, height: 70 } }), - ); + expect(editor.view.state.tr.setNodeAttribute).toHaveBeenCalledWith(0, 'size', { width: 140, height: 70 }); + // AttrSteps have no changed range, so the containing block's sdBlockRev + // must be bumped explicitly for the layout engine to repaint. + expect(editor.view.state.tr.setNodeAttribute).toHaveBeenCalledWith(0, 'sdBlockRev', 4); expect(editor.view.dispatch).toHaveBeenCalledWith(editor.view.state.tr); wrapper.unmount(); @@ -274,11 +290,10 @@ describe('ImageResizeOverlay', () => { expect(headerFooterEditor.view.state.doc.nodeAt).toHaveBeenCalledWith(0); expect(bodyEditor.view.state.doc.nodeAt).not.toHaveBeenCalled(); - expect(headerFooterEditor.view.state.tr.setNodeMarkup).toHaveBeenCalledWith( - 0, - null, - expect.objectContaining({ size: { width: 140, height: 70 } }), - ); + expect(headerFooterEditor.view.state.tr.setNodeAttribute).toHaveBeenCalledWith(0, 'size', { + width: 140, + height: 70, + }); expect(headerFooterEditor.view.dispatch).toHaveBeenCalledWith(headerFooterEditor.view.state.tr); expect(bodyEditor.view.dispatch).not.toHaveBeenCalled(); diff --git a/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue b/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue index d41d2f5679..e8e03982ad 100644 --- a/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue +++ b/packages/super-editor/src/editors/v1/components/ImageResizeOverlay.vue @@ -23,6 +23,7 @@ import { ref, computed, watch, onBeforeUnmount } from 'vue'; import { measureCache } from '@superdoc/layout-bridge'; import { isContentLockedMode } from '../extensions/structured-content/lockModes.js'; +import { nodeAllowsSdBlockRevAttr } from '../extensions/block-node/block-node.js'; // Configuration constants const OVERLAY_EXPANSION_PX = 2000; @@ -622,21 +623,30 @@ function dispatchResizeTransaction(blockId, newWidth, newHeight) { return; } - // Store pixel dimensions directly (converted to EMU only during DOCX export) - const newAttrs = { - ...imageNode.attrs, - size: { - width: Math.round(newWidth), - height: Math.round(newHeight), - }, - }; - - tr.setNodeMarkup(imagePos, null, newAttrs); + // Store pixel dimensions directly (converted to EMU only during DOCX export). + // Use setNodeAttribute (AttrStep) instead of setNodeMarkup (ReplaceStep): + // Word does not track image resizes as revisions, and AttrSteps pass through + // the track-changes machinery untracked, so the resize applies in place in + // suggesting mode — even when the image is itself a pending tracked + // insertion. A ReplaceStep would be split into a tracked insert + delete, + // rendering as a duplicate image (SD-2974). + tr.setNodeAttribute(imagePos, 'size', { + width: Math.round(newWidth), + height: Math.round(newHeight), + }); - // Word does not track image resizes as revisions; bypass tracking so the - // attribute change applies in place instead of being split into a tracked - // insert + delete (which would render as a duplicate image — SD-2974). - tr.setMeta('skipTrackChanges', true); + // AttrSteps have no changed range, so blockNodePlugin won't bump the + // containing blocks' sdBlockRev and the layout engine would reuse the + // cached FlowBlock (stale paint). Bump the revs here, same pattern as + // numberingPlugin's bumpBlockRev. + const $imagePos = state.doc.resolve(imagePos); + for (let depth = $imagePos.depth; depth > 0; depth--) { + const ancestor = $imagePos.node(depth); + if (!nodeAllowsSdBlockRevAttr(ancestor)) continue; + const currentRev = Number.parseInt(ancestor.attrs?.sdBlockRev, 10); + if (!Number.isFinite(currentRev)) continue; + tr.setNodeAttribute($imagePos.before(depth), 'sdBlockRev', currentRev + 1); + } dispatch(tr); diff --git a/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts b/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts index aec89146fe..82b0028432 100644 --- a/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts +++ b/tests/behavior/tests/images/image-resize-in-suggesting-mode.spec.ts @@ -23,6 +23,72 @@ async function getImageCount(superdoc: any): Promise { }); } +async function getFirstImageSize(superdoc: any): Promise<{ width: number; height: number }> { + return superdoc.page.evaluate(() => { + const doc = (window as any).editor?.state?.doc; + if (!doc) throw new Error('Editor document is unavailable.'); + let size: { width: number; height: number } | null = null; + doc.descendants((node: any) => { + if (!size && node.type?.name === 'image') { + size = node.attrs?.size ?? null; + } + }); + if (!size) throw new Error('No image node with a size attr found.'); + return { width: size.width, height: size.height }; + }); +} + +async function firstImageIsPendingTrackedInsertion(superdoc: any): Promise { + return superdoc.page.evaluate(() => { + const doc = (window as any).editor?.state?.doc; + if (!doc) throw new Error('Editor document is unavailable.'); + let tracked = false; + let found = false; + doc.descendants((node: any) => { + if (!found && node.type?.name === 'image') { + found = true; + tracked = node.marks?.some((mark: any) => mark.type?.name === 'trackInsert') ?? false; + } + }); + if (!found) throw new Error('No image node found.'); + return tracked; + }); +} + +/** + * Hover the image to surface the resize overlay, then drag the SE handle by + * (dx, dy). Negative deltas shrink the image. The exact end-size doesn't + * matter — the delta only needs to exceed the 1px DIMENSION_CHANGE_THRESHOLD + * so the transaction actually dispatches; callers assert the size changed. + */ +async function dragSeResizeHandle(superdoc: any, dx: number, dy: number): Promise { + const img = superdoc.page.locator('.superdoc-inline-image').first(); + await expect(img).toBeAttached({ timeout: 5000 }); + await img.hover(); + await superdoc.waitForStable(); + + const overlay = superdoc.page.locator('.superdoc-image-resize-overlay'); + await expect(overlay).toBeAttached({ timeout: 5000 }); + + const handle = overlay.locator('.resize-handle--se'); + await expect(handle).toBeAttached({ timeout: 5000 }); + const handleBox = await handle.boundingBox(); + if (!handleBox) throw new Error('Could not locate SE resize handle.'); + + const startX = handleBox.x + handleBox.width / 2; + const startY = handleBox.y + handleBox.height / 2; + + await superdoc.page.mouse.move(startX, startY); + await superdoc.page.mouse.down(); + // Use multiple steps so the throttled mousemove handler in + // ImageResizeOverlay.vue produces an updated constrainedWidth/Height + // before mouseup commits. + await superdoc.page.mouse.move(startX + dx, startY + dy, { steps: 10 }); + await superdoc.page.mouse.up(); + + await superdoc.waitForStable(); +} + test.describe('Image resize in suggesting mode (SD-2974)', () => { test.beforeEach(async ({ superdoc }) => { await superdoc.loadDocument(FIXTURE); @@ -36,40 +102,107 @@ test.describe('Image resize in suggesting mode (SD-2974)', () => { await superdoc.waitForStable(); await superdoc.assertDocumentMode('suggesting'); - // Hover the image to surface the resize overlay. - const img = superdoc.page.locator('.superdoc-inline-image').first(); - await expect(img).toBeAttached({ timeout: 5000 }); - await img.hover(); + const before = await getFirstImageSize(superdoc); + await dragSeResizeHandle(superdoc, -60, -60); + + // The size must actually have changed — otherwise the drag no-opped (the + // dispatch is skipped under the 1px threshold) and the duplicate-count + // assertion below would pass without exercising the fix. + const after = await getFirstImageSize(superdoc); + expect(after.width).toBeLessThan(before.width); + + // After the resize commit there must still be exactly one image node. + // Before the SD-2974 fix the tracked-change machinery split the + // ReplaceStep (generated by setNodeMarkup on the leaf image node) into + // a tracked insert + delete, leaving two image nodes in the document. + expect(await getImageCount(superdoc)).toBe(1); + }); + + test('@behavior SD-2974: repeated resizes do not accumulate duplicate images', async ({ superdoc }) => { + expect(await getImageCount(superdoc)).toBe(1); + + await superdoc.setDocumentMode('suggesting'); await superdoc.waitForStable(); + await superdoc.assertDocumentMode('suggesting'); - const overlay = superdoc.page.locator('.superdoc-image-resize-overlay'); - await expect(overlay).toBeAttached({ timeout: 5000 }); + const initial = await getFirstImageSize(superdoc); + await dragSeResizeHandle(superdoc, -60, -60); - // Drag the SE handle to shrink the image. The exact end-size doesn't - // matter — we only need a commit large enough to exceed the - // 1px DIMENSION_CHANGE_THRESHOLD so the transaction actually dispatches. - const handle = overlay.locator('.resize-handle--se'); - await expect(handle).toBeAttached({ timeout: 5000 }); - const handleBox = await handle.boundingBox(); - if (!handleBox) throw new Error('Could not locate SE resize handle.'); + const afterFirst = await getFirstImageSize(superdoc); + expect(afterFirst.width).toBeLessThan(initial.width); + expect(await getImageCount(superdoc)).toBe(1); - const startX = handleBox.x + handleBox.width / 2; - const startY = handleBox.y + handleBox.height / 2; + await dragSeResizeHandle(superdoc, -30, -30); - await superdoc.page.mouse.move(startX, startY); - await superdoc.page.mouse.down(); - // Move toward NW to shrink. Use multiple steps so the throttled - // mousemove handler in ImageResizeOverlay.vue produces an updated - // constrainedWidth/Height before mouseup commits. - await superdoc.page.mouse.move(startX - 60, startY - 60, { steps: 10 }); - await superdoc.page.mouse.up(); + const afterSecond = await getFirstImageSize(superdoc); + expect(afterSecond.width).toBeLessThan(afterFirst.width); + expect(await getImageCount(superdoc)).toBe(1); + }); +}); +test.describe('Resize of a pending tracked image insertion (SD-2974)', () => { + test('@behavior SD-2974: resizing an image that is itself a pending tracked insertion does not duplicate it', async ({ + superdoc, + }) => { + // Set up an image that is itself a pending tracked insertion (insert in + // suggesting mode → resize before acceptance — the customer flow in + // SD-2974's Loom). This is the case most likely to regress: a ReplaceStep + // resize of a tracked-inserted node is re-routed through the tracking + // machinery by the protect-tracked-review-state guard regardless of + // skipTrackChanges. The AttrStep resize never enters that machinery. + // + // The state is constructed directly (image node carrying a trackInsert + // mark) rather than via editor.commands.setImage in suggesting mode: + // image registration (imageRegistrationPlugin) asynchronously rewrites + // freshly inserted images via an untracked setNodeMarkup, which has a + // suggesting-mode duplication of its own and would contaminate this + // test's premise. + await superdoc.setDocumentMode('suggesting'); await superdoc.waitForStable(); + await superdoc.assertDocumentMode('suggesting'); - // After the resize commit there must still be exactly one image node. - // Before the SD-2974 fix the tracked-change machinery split the - // ReplaceStep (generated by setNodeMarkup on the leaf image node) into - // a tracked insert + delete, leaving two image nodes in the document. + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { state } = editor; + const mark = state.schema.marks.trackInsert.create({ + id: 'sd-2974-pending-insert', + author: 'Behavior Test', + authorEmail: 'test@example.com', + date: new Date().toISOString(), + }); + const image = state.schema.nodes.image.create( + { + src: 'assets/image-landscape.png', + alt: 'Pending insertion', + size: { width: 240, height: 160 }, + sdImageId: 'sd-2974-pending-image', + // Mark the image as already registered so imageRegistrationPlugin + // does not asynchronously re-dispatch a setNodeMarkup for it (that + // un-tracked rewrite path has its own suggesting-mode duplication, + // which would contaminate this test's premise). + rId: 'rId-sd-2974-test', + }, + null, + [mark], + ); + const tr = state.tr.insert(state.selection.from, image); + tr.setMeta('skipTrackChanges', true); + editor.view.dispatch(tr); + }); + await superdoc.waitForStable(); + + // Sanity: one image, and it is a pending tracked insertion. + expect(await getImageCount(superdoc)).toBe(1); + expect(await firstImageIsPendingTrackedInsertion(superdoc)).toBe(true); + + const before = await getFirstImageSize(superdoc); + await dragSeResizeHandle(superdoc, -40, -40); + + const after = await getFirstImageSize(superdoc); + expect(after.width).toBeLessThan(before.width); expect(await getImageCount(superdoc)).toBe(1); + + // The resize must not have disturbed the pending insertion itself. + expect(await firstImageIsPendingTrackedInsertion(superdoc)).toBe(true); }); });