Conversation
Characterises how resolveDropIntent treats a batch released over the row of another selected member when no item target is sticky: the source-row guard matches the dragged item only, so the release lands as a list-only drop. Documents current behaviour ahead of the drag session extraction; the product decision stays open.
Introduces DragSession, one object for the state of one drag: prospective members resolved once, permitted destinations and owner memo, a batch frozen at most once, data-dragging marks set on start and cleared on a single end. Nothing uses it yet; the root and item controllers adopt it next. Moves itemElementsByKey into selection.ts so the session and the root share it.
Replaces the root controller's frozen-batch field, owner memo and row-marking helpers with one DragSession. The item controller begins it in canDrag and reads it back through the root in every later callback; the root's monitor starts it on drag start and ends it once, from the drop monitor or disconnect, so an item controller replaced between preview and drag start cannot strand the batch. The root contract loses freezeDragBatch, markDragBatch, dragPermittedDestinations, dragRefused and externalDragItems and gains beginDrag and dragSession. Pragmatic checks the drag handle after canDrag, so a press outside the handle leaves a session that never froze; owner answers go live again until a session is frozen. Two deliberate changes: the owner memo is forgotten on a morph mid-drag so a reparented row answers with its live list, and canDrag runs the pointer check before the batch cap so a press on a button inside a card no longer announces an oversized batch.
Renames the Stimulus SortableItemData to SortableDragSourceData. Two types shared the name with different shapes: the engine payload in core-common names an item and its list id, the Stimulus one the dragged source, its root and the destinations its batch may reach. The identity symbol follows suit. The wire and the DOM contract are unchanged.
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Native drag-event ordering across Turbo reconnections needs browser-level validation and final human review.
Review effort: Balanced
Findings: None
What changed in this PR
Extracts drag state into a root-owned DragSession, simplifying sortable-list controllers without changing consumer markup or request parameters.
Changes:
- Centralizes drag lifecycle, batch freezing, row marking, and cleanup.
- Simplifies the root contract and renames the drag-source payload.
- Expands lifecycle, cancellation, and Turbo morph regression coverage.
| File | Description |
|---|---|
| frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts | Extracts identity-to-element lookup. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts | Updates root mocks and payload naming. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md | Documents session-owned batch movement. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts | Updates mocks and payload references. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts | Delegates drag state to the session. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Tests session integration and pointer guards. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts | Introduces drag lifecycle ownership. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.spec.ts | Tests lifecycle, destinations, and cleanup. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts | Simplifies the root interface and renames payloads. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts | Adopts renamed payload helpers. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts | Hosts sessions and handles morph recovery. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Covers lifecycle integration and existing drop behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
1 of 3 tasks
myabc
added this pull request to stack #25797
October 3, 2026 20:39
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
No work package yet; the branch is interim and will be renamed once one exists.
What are you trying to accomplish?
The Stimulus
sortable-listsroot controller carried the state of a drag in two nullable fields (activeDragBatch,dragOwnerDestinations) plusdata-draggingmarks cleared by two controllers, and its item-facing contract exposed five members (freezeDragBatch,markDragBatch,dragPermittedDestinations,dragRefused,externalDragItems) whose calling order was documented only in comments. The batch was resolved up to four times per drag start. Three independent architecture reviews of this area picked this extraction as the first deepening step.This PR moves that state into one
DragSessionobject with an explicit lifetime (prospective → frozen → started → ended): members resolved once incanDrag, batch frozen once in the preview callback, rows marked on drag start by the root's own Pragmatic monitor, ended exactly once from the drop monitor or disconnect. The root contract shrinks tobeginDraganddragSession. No DOM attributes, wire parameters or consumer markup change.What approach did you choose and why?
Objects where state has a lifetime, functions for placement maths: the session is a class because it owns sequencing and cleanup;
list-dom.tsand friends stay as they are for a later slice.The root holds the session rather than the item controller, because Pragmatic dispatches
onDragStarta frame after the preview and an item controller replaced in between (a Turbo morph) would otherwise strand the batch. Pragmatic also checks the drag handle aftercanDrag, so a press outside the handle leaves a session that never froze; the root ignores such a session when answering owner lookups.Two deliberate behaviour changes, both pinned by specs: the per-drag owner memo is forgotten on a
turbo:morph-elementheal, so a row reparented mid-drag answers with its live list; andcanDragruns the pointer check before the batch-cap check, so a press on a button inside a card no longer announces "batch too large".The first commit characterises current behaviour for a batch released over a batch-mate's row (it lands as a list-only drop); whether that is wanted is an open product decision, not changed here. The last commit renames the Stimulus payload type to
SortableDragSourceData, sinceSortableItemDatanamed two different shapes in two files.AI involvement
Merge checklist