Enonic UI: Drag n drop fails #4604

[skjulestad]

Dropping components into empty final tree containers by using live row geometry and reusing projection data for final drop commit.

Set as draft for now, may go over after other changes done with page controller:
#4603

[ashklianko]

Significant

1. Drop silently lost when hint == null, UI still shows "allowed" — SortableList.tsx:412, :461. ?? true keeps the row styled as allowed, but !finalDrop.hint?.allowed discards the commit. projectTreeDrop returns null in reachable cases (treeProjection.test.ts:206). → ?? false, or leave the commit to the consumer — not both.
2. Undocumented contract change — SortableList.tsx:87, :108. onMove used to fire for every non-own-slot drop in projection mode (the story resolves it itself, SortableList.stories.tsx:298); now only when the mid-drag hint allowed it. Fine as a design, missing from the JSDoc.
3. overId mismatch aborts instead of recomputing — SortableList.tsx:456-464. With MeasuringStrategy.Always (:142) the final over can differ from the last move event; the drop then fails silently after a valid indicator. → Recompute getProjectionDragInfo + resolveDrop on mismatch.
4. The main change is untested — getProjectionDragInfo (:155) depends on dnd-kit rect semantics (over.rect measured with transforms; containers stay droppable since normalizeLocalDisabled(true) → {draggable: true, droppable: false}). Unverifiable by reading. → Export/extract it and pin side on synthetic rects: item above/below, container above/below.
5. New projectTreeDrop branch has no allowed test — treeProjection.ts:80-83 calls isContainerAllowed itself, but treeProjection.test.ts:176 only covers the old path. → Add "hovered empty container rejected".
6. "Drag down = step out" lost on empty-container rows — intentional? The removed assertion was documented behavior. A disallowed empty container now makes its whole row a dead zone (previously direction: 'down' offered the parent). Worth stating in the PR description.

Minor

• Stale docs: treeProjection.ts:1-12 header omits the override (which ignores side and direction); story text SortableList.stories.tsx:280-282 still claims down-drag steps out past the layout.
• Nested ternaries — SortableList.tsx:166-171, :176-183; forbidden by .claude/rules/typescript.md. → getDraggedMidpoint / getSide helpers.
• ProjectionDragEvent (:146) — DragEndEvent member now unused.
• over == null ? String(active.id) : String(over.id) duplicated at :158, :406, :456 → one getOverId(event).
• drop state and dropRef written as a pair in six places → single applyDrop(next) setter.
• onMove(oldIndex, finalDrop.info.overIndex, ...) (:465) mixes index sources; finalDrop.info.activeIndex equals oldIndex by construction.

[ashklianko]

Also when dragging an item to a new place and releasing it tends to end up in the wrong place.

[Copilot]

Pull request overview

Updates the sortable-list projection mode to better support dropping into empty tree containers (especially at the end of the flattened tree) by switching to midpoint-based “side” semantics and reusing computed projection data for the final drop commit.

Changes:

• Adjusts tree drop projection to treat container rows as two semantic zones (gap-before vs “enter container at index 0”).
• Introduces shared projection drag-info utilities (midpoint-based side + placeholder index) and uses them consistently during drag and on drag-end commit.
• Adds a DragOverlay-based “drag clone” in projection mode and updates tests/story text accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/main/resources/assets/admin/common/js/form2/components/sortable-list/treeProjection.ts	Updates tree drop targeting to explicitly support container-row “enter” semantics when hovering in the lower half.
src/main/resources/assets/admin/common/js/form2/components/sortable-list/treeProjection.test.ts	Adds/updates tests for empty-container targeting, container row zones, and allowed predicate behavior.
src/main/resources/assets/admin/common/js/form2/components/sortable-list/SortableList.tsx	Refactors projection-mode drag state to use shared drag-info helpers; adds DragOverlay and placeholder index strategy integration.
src/main/resources/assets/admin/common/js/form2/components/sortable-list/SortableList.stories.tsx	Updates demo copy to reflect new container-row zone semantics and placeholder/overlay behavior.
src/main/resources/assets/admin/common/js/form2/components/sortable-list/projectionDragInfo.ts	New helper module for midpoint-based drag info and placeholder index calculation.
src/main/resources/assets/admin/common/js/form2/components/sortable-list/projectionDragInfo.test.ts	Adds unit tests for projection drag info and placeholder index behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/main/resources/assets/admin/common/js/form2/components/sortable-list/SortableList.tsx:564

• In projection mode, the list’s visual placeholder index is computed via getProjectionPlaceholderIndex (used in projectionSortingStrategy), but on drag end the component calls onMove(..., finalInfo.overIndex, ...) instead. This makes the toIndex argument inconsistent with the list’s actual placeholder/reorder semantics and can break consumers that rely on toIndex even in projection mode. Consider passing the computed placeholder index as toIndex when resolveDrop is set.

                const finalHint = resolveDrop(finalInfo, items);
                if (!finalHint?.allowed) return;
                onMove(finalInfo.activeIndex, finalInfo.overIndex, finalInfo);
                return;

[ashklianko]

Looks much better, good work!

A minor issue I've found:
if you have a layout as a last page component in the components list and it's last region is empty, then you can't drag and drop anything after this last layout, only into it's empty region

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[edloidas]

I pulled the branch, ran the suite (1088 green) and then probed the pure functions directly rather than reading them — projectTreeDrop, getProjectionPlaceholderIndex, and the verticalListSortingStrategy the new strategy wraps — and read PageComponentsView.tsx downstream to see what actually consumes this.

The core of it holds up. The empty-final-region fix is real: a downward drag onto a container row's lower half resolves into that region at index 0, allowed: true, which is exactly the case the issue opens with. Re-resolving the projection at drop time instead of trusting the last dragMove is the right move and removes a whole class of stale-commit bug. The after zone behaves as the tests say.

What I found is one thing that needs a change and two questions. The change is that the rendered placeholder and the committed position are derived by two different functions that disagree — that comment also settles Copilot's open one at line 564, whose premise is correct even though its conclusion does not reach the current consumer. The questions are how far #4604's "last part of a layout" is meant to reach, and whether renderItem mounting twice should be a documented contract.

Everything here is numeric, from the exported functions and the resolved @dnd-kit/sortable artifact. I did not drive Storybook, so I have not reproduced @ashklianko's "ends up in the wrong place" end to end — the first comment is a candidate mechanism for it, not a confirmed explanation.

The one I would not merge without is the placeholder/commit divergence.

[edloidas]

The placeholder and the commit are computed by two different functions, and they disagree

// SortableList.tsx:471
overIndex: drop == null ? args.overIndex : getProjectionPlaceholderIndex(drop.info, ids.length),

Two problems, one cause: the gap the user sees and the position that gets committed come from independent derivations.

The strategy ignores the hint. This falls back to args.overIndex only when drop == null — it never checks drop.hint. So when resolveDrop returns null or {allowed: false}, the placeholder index is still computed and the rows still open a gap. Measured against verticalListSortingStrategy (the same function this wraps), four 40px rows, activeIndex: 0, overIndex: 2, side: 'below':

hint allowed:    sibling transforms y = [80, -40, -40, 0]
hint DISALLOWED: sibling transforms y = [80, -40, -40, 0]
hint NULL:       sibling transforms y = [80, -40, -40, 0]

Identical. Meanwhile projectedIndent is gated on drop?.hint != null (lines 621 and 640) and dropAllowed dims the clone — so the list opens a slot and marks it refused at the same time, and handleDragEnd discards it at line 562.

The committed index is not the one shown. This is Copilot's point at line 564, and its premise checks out — onMove commits finalInfo.overIndex while the gap was rendered at getProjectionPlaceholderIndex(...). They diverge in half the combinations:

side=below dir=up    toIndex=1  placeholder=2
side=above dir=down  toIndex=2  placeholder=1
side=below dir=down  toIndex=2  placeholder=2
side=above dir=up    toIndex=1  placeholder=1

I would push back on its conclusion, though: PageComponentsView.tsx takes _fromIndex/_toIndex underscored and re-resolves everything from info, so nothing reads the wrong index today. It is a trap for the next consumer, not a live bug.

Both are new on this branch. Gating the strategy on drop.hint?.allowed the way projectedIndent already is would fix the first; passing the placeholder index to onMove would close the second.

[edloidas]

The container zone rescues an empty final region, but a populated one is still unreachable going down

// treeProjection.ts:147
if (over.kind === 'container' && side === 'below') {
    return {type: 'container', container: over};
}

This works for the empty case — I confirmed a downward drag onto a container row's lower half returns {containerId: '/main/1/right', index: 0, allowed: true}. Every new test targets an empty region.

When the region already has items, the gap after its last item is still an ambiguous slot stack, and const chosen = direction === 'up' ? stack[0] : stack[stack.length - 1] at line 91 picks the shallowest slot for any downward drag. Probed on a populated /main/1/right, dragging an item that starts above the layout:

side='below' direction='down' -> {containerId: '/main', index: 1}         // out past the layout
side='below' direction='up'   -> {containerId: '/main/1/right', index: 1} // the append slot

direction is the sign of cumulative delta.y from drag start, so an item starting above the layout is permanently 'down' and never reaches the second result.

Line 91 predates this branch — it is from #4512, untouched here — so this is not a regression, and there is a two-drag workaround (enter at index 0 via this zone, then reorder). But #4604 says "into the last part of a layout" without qualifying that the part be empty. Finish it here, or scope #4604 to the empty case and file the populated one separately?

[edloidas]

The overlay turns renderItem into a twice-per-drag contract

SortableListDragOverlay calls renderItem(context, grip) at line 641 while SortableListItem calls it unconditionally at line 368 — two React subtrees rendering consumer content during every projection drag. inert and aria-hidden stop input and assistive tech, not effects, refs, or duplicate DOM ids.

The prop doc now tells consumers to "account for duplicated refs, effects, and DOM IDs", which makes it the contract rather than an oversight. That is a defensible call: PageComponentsItem.tsx has no effects, no refs and no id=, so nothing breaks today, and git tag --contains on the commit that introduced resolveDrop comes back empty, so projection mode has never shipped and there is no external consumer to break either.

Which is also what makes now the cheap moment to decide it deliberately — keep the double mount and live with the documented constraint, or give the overlay its own renderOverlayItem and keep renderItem single-mount. Your call either way; I would just rather it be a decision than a doc note.