fix(drop-zone): do not intercept non-file drops - #6168
Conversation
|
This Pull Request targets Consider targeting |
There was a problem hiding this comment.
🟡 Changes recommended
The newly added tests have correctness/robustness issues (mocked event propagation and potentially false-positive assertions) that should be addressed before relying on them as regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an editor-wide drop zone regression where non-file drop events were being intercepted and stopped before they could reach nested editors (e.g. dmn-js decision table), preventing internal drag-and-drop operations like rule reordering. It also adds unit/e2e coverage to lock in the expected behavior.
Changes:
- Add an
isDragAllowed(event)guard toDropHandler#handleDropso non-file drops are not prevented/stopped. - Add a DropZone unit test ensuring non-file drops bubble past the drop zone.
- Add a Playwright e2e scenario (plus DMN fixture + page helpers) to verify decision table rule reordering after editing a cell.
File summaries
| File | Description |
|---|---|
| client/src/app/drop-zone/DropHandler.js | Skip handling drop for non-file drags so internal DnD isn’t blocked. |
| client/src/app/drop-zone/tests/DropZoneSpec.js | Add regression test asserting non-file drops bubble to outer listeners. |
| test/e2e/pages/DmnEditorPage.js | Add DMN decision table helpers to edit a cell and drag-reorder a rule. |
| test/e2e/specs/dmn-modeling.spec.js | Add e2e repro/assertion for rule reordering after cell edit + save verification. |
| test/e2e/fixtures/decision-table-rows.dmn | Add minimal DMN fixture with two rules for reorder test. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🤖 (human-reviewed) Addressed the Copilot review comments — see individual replies on each thread; fixed in f645c38. Also retargeted this PR to The commit that introduced the regression: Before that commit, When drop handling was extracted into the new It sat unnoticed for a long time because it only breaks a |
|
🤖 (human-reviewed) Found the commit that actually made this regression user-visible at the reported version boundary. The defect itself ( Under React 16 (
So: the code defect is |
There was a problem hiding this comment.
🟢 Approval recommended
The change is minimal and targeted, and it is backed by both a focused unit regression test and an end-to-end test covering the reported DMN drag-and-drop failure.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
The app-wide drop zone (used to open a diagram by dragging a file onto the window) wraps the whole editor area, so every native `drop` event happening inside it - including inside a nested dmn-js decision table - reaches it first. `handleDragOver`/`handleDragLeave` already skip non-file drags via `isDragAllowed()`, but `handleDrop` was missing that guard and always called `stopPropagation()`. This silently broke dmn-js's own document-level `drop` listener, so dragging a decision table row/column never actually reordered it, even though the drag-over highlight looked correct throughout the gesture. The defect itself dates back to 2f320ff8 ("chore: move drop handling to a separate component"), but stayed harmless until 7b284466c ("deps: update to react@18") switched the app from React 16's `ReactDOM.render` to React 18's `createRoot`, moving React's own delegated event listener off `document` and onto the app's root container - which is what let `stopPropagation()` here actually block dmn-js's `document`-level listener from ever seeing the event. Closes #6166 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f645c38 to
ac5d851
Compare
|
🤖 (human-reviewed) Opened a draft upstream PR in table-js addressing the same fragility at the source (scoping its drag-and-drop listeners to the table container instead of |
AlekseyManetov
left a comment
There was a problem hiding this comment.
Works great!
Proposed Changes
The app-wide drop zone (used to open a diagram by dragging a file onto the window,
client/src/app/drop-zone/) wraps the whole editor area, so every nativedropevent happening inside it — including inside a nested dmn-js decision table — reaches it first.DropHandler#handleDragOverand#handleDragLeavealready skip non-file drags by checkingisDragAllowed(event)before touching the event.#handleDropwas missing that same guard and always calledevent.stopPropagation(), regardless of what was being dragged. That silently killed dmn-js's owndocument-leveldroplistener for any internal (non-file) drag, so dragging a decision table row or column to reorder it never actually applied the move — even though the drag-over highlight looked correct the whole time, which made the bug easy to miss.The fix mirrors the existing guard from
handleDragOver/handleDragLeaveontohandleDrop.Closes #6166
Why this surfaced now, between 5.44 and 5.45
The defect itself is old — introduced in
2f320ff8("chore: move drop handling to a separate component", Sep 2024, first released in v5.29.0) — but it was harmless until7b284466c("deps: update toreact@18", first released in v5.45.0, matching the reporter's bisect exactly).That commit switched
client/src/index.jsfrom React 16'sReactDOM.render(<App/>, rootElement)to React 18'screateRoot(rootElement).render(<App/>):document— the same nodetable-jsbinds its decision-table drag-and-dropdroplistener to. Multiple listeners on the same node all fire regardless of order, soDropHandler's buggy unconditionalstopPropagation()never actually blockedtable-js's sibling listener from also running. Dragging worked despite the latent bug.createRoot(from 5.45.0) moves that listener to the app's own root container, belowdocument. Now thedropevent hits React's listener — andDropHandler'sstopPropagation()— before it can reachdocument, sotable-js's listener never fires and the reorder command never applies.The fix in this PR is unaffected by any of this —
handleDropshould have had the same guard as its siblings all along, and fixing that is correct and sufficient regardless of which React root API is in use.Full analysis history (including an earlier, since-corrected hypothesis) is on the issue: comment 1, correction, timeline.
Steps to try out
The included e2e test (
test/e2e/specs/dmn-modeling.spec.js) automates this exact repro with a real (non-synthetic) Playwright-driven drag.Checklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}