fix: move an image doesn't work with drag and drop (#2603)#2628
fix: move an image doesn't work with drag and drop (#2603)#2628miadnguyen wants to merge 1 commit intoTypeCellOS:mainfrom
Conversation
|
@miadnguyen is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts (1)
103-110:⚠️ Potential issue | 🟠 Major
pointercancelshould clearpreventShowWhileMouseDown, not set it.Line 107 contradicts the comment above it. In this file, the only reset path for
preventShowWhileMouseDownis thepointeruphandler, so setting it totruehere can leave the toolbar suppressed after a canceled drag/pointer sequence.Suggested change
dom.addEventListener( "pointercancel", () => { - preventShowWhileMouseDown = true; + preventShowWhileMouseDown = false; }, { signal, capture: true }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts` around lines 103 - 110, The pointercancel handler currently sets preventShowWhileMouseDown to true which contradicts its comment and leaves the toolbar suppressed; update the "pointercancel" event listener so it clears the flag (set preventShowWhileMouseDown to false) — mirror the reset behavior of the existing pointerup handler and ensure the handler on "pointercancel" (inside FormattingToolbar.ts near the preventShowWhileMouseDown logic) resets the state instead of setting it.
🧹 Nitpick comments (1)
tests/src/end-to-end/images/images.test.ts (1)
166-171: Please assert the drop result aftermouse.up().This currently proves only that the toolbar stays hidden mid-drag. It still passes if the handle path never reorders the document, which is the user-visible part of
#2603. Add a post-drop snapshot or DOM-order assertion.Possible follow-up
const toolbar = page.locator(".bn-formatting-toolbar"); await expect(toolbar).not.toBeVisible(); await page.mouse.up(); + await compareDocToSnapshot(page, "dragImageViaHandle"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/images/images.test.ts` around lines 166 - 171, After the drag is finished (after the existing await page.mouse.up()), add a post-drop assertion to verify the actual reorder happened rather than only that the toolbar is hidden: use the same test's DOM locators to either take a snapshot of the document and compare it (post-drop snapshot) or assert the DOM order of the moved elements (e.g., grab the image/handle nodes and assert their src/text/index positions have changed). Place this assertion immediately after await page.mouse.up() (near the existing toolbar locator) so the test fails if the handle never reorders the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts`:
- Around line 103-110: The pointercancel handler currently sets
preventShowWhileMouseDown to true which contradicts its comment and leaves the
toolbar suppressed; update the "pointercancel" event listener so it clears the
flag (set preventShowWhileMouseDown to false) — mirror the reset behavior of the
existing pointerup handler and ensure the handler on "pointercancel" (inside
FormattingToolbar.ts near the preventShowWhileMouseDown logic) resets the state
instead of setting it.
---
Nitpick comments:
In `@tests/src/end-to-end/images/images.test.ts`:
- Around line 166-171: After the drag is finished (after the existing await
page.mouse.up()), add a post-drop assertion to verify the actual reorder
happened rather than only that the toolbar is hidden: use the same test's DOM
locators to either take a snapshot of the document and compare it (post-drop
snapshot) or assert the DOM order of the moved elements (e.g., grab the
image/handle nodes and assert their src/text/index positions have changed).
Place this assertion immediately after await page.mouse.up() (near the existing
toolbar locator) so the test fails if the handle never reorders the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b850337-2b91-4ef2-bdbc-615cbe5cb0d4
📒 Files selected for processing (2)
packages/core/src/extensions/FormattingToolbar/FormattingToolbar.tstests/src/end-to-end/images/images.test.ts
1 participant
Summary
Fixes #2603. When dragging an image block using the side menu drag handle, the formatting toolbar incorrectly opens mid-drag.
Rationale
The formatting toolbar should only appear when the user clicks on the image or during text selection, not during a drag-and-drop operation. The unexpected toolbar appearance is disruptive.
Changes
preventShowWhileDraggingflag inFormattingToolbar.tsthat mirrors the existingpreventShowWhileMouseDownpatterndragstartanddragendlisteners oneditor.prosemirrorView.rootto set and clear this flag for the full duration of any drag operationImpact
The toolbar is now suppressed during any block drag operation. Normal toolbar behavior is unaffected.
Testing
tests/src/end-to-end/images/images.test.ts: "Formatting toolbar should not appear when dragging image block"Screenshots/Video
image-drag-drop-toolbar.mov
Checklist
Additional Notes
When the drag handle is used, ProseMirror sets a
NodeSelectionon the image node before the drag begins. The existingpreventShowWhileMouseDownflag did not cover HTML drag events (dragstart/dragend), only pointer events, so the toolbar was not suppressed during drags initiated from the side menu.Summary by CodeRabbit
Bug Fixes
Tests