Rework sortable-lists topology and enable optimistic 204 moves - #24104
Closed
myabc wants to merge 4 commits into
Closed
Conversation
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the sortable-lists Stimulus controller suite to remove root→child outlet wiring, make list acceptance rules per-list (and more DOM-structure-agnostic), and fully enable “optimistic” same-list drag reorders by allowing a 204 No Content backend response while introducing a deterministic sortable-lists:moved settle signal used by Backlogs and tests.
Changes:
- Reworked controller topology so list/item controllers outlet to the root and read shared root state (e.g.,
moving) from that outlet rather than root-managed callbacks. - Introduced per-list
acceptedTypesplus a configurablerowsContainerSelector, and updated DOM-resolution/reorder logic accordingly (including nested/dual-role shapes). - Enabled optimistic same-list moves with a
204backend response and added asortable-lists:movedevent to drive Backlogs split-view lock_version refresh and Selenium settle waiting.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/capybara/wait_helpers.rb | Adds a Capybara helper that waits for sortable-lists:moved as the settle signal. |
| modules/backlogs/spec/support/pages/backlog.rb | Switches Backlogs drag helper to wait on either turbo reload (cross-list) or moved event (same-list). |
| modules/backlogs/spec/requests/backlogs/work_packages_move_spec.rb | Adds request coverage for 204 optimistic same-list moves vs turbo-stream reload paths. |
| modules/backlogs/spec/requests/backlogs/backlog_spec.rb | Updates expectations for new root data attributes (moveUrlTemplates). |
| modules/backlogs/spec/features/work_packages/edit_in_split_view_after_move_spec.rb | Adds a selenium scenario ensuring split-view edits work after same-list 204 move. |
| modules/backlogs/spec/features/work_packages/drag_in_sprint_spec.rb | Updates a cross-list drag call site to opt into the turbo-reload wait. |
| modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb | Verifies item controller outlets to the root in rendered attributes. |
| modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb | Verifies list outlet + acceptedTypes + rowsContainerSelector attributes. |
| modules/backlogs/spec/components/backlogs/sprint_component_spec.rb | Verifies sprint list wiring for the new list controller contract. |
| modules/backlogs/spec/components/backlogs/inbox_component_spec.rb | Verifies inbox list wiring for the new list controller contract. |
| modules/backlogs/spec/components/backlogs/bucket_component_spec.rb | Verifies bucket list wiring for the new list controller contract. |
| modules/backlogs/app/views/backlogs/backlog/show.html.erb | Updates root wiring to moveUrlTemplates + binds sortable-lists:moved for split-view sync. |
| modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb | Updates turbo-frame partial wiring to moveUrlTemplates. |
| modules/backlogs/app/controllers/backlogs/work_packages_controller.rb | Implements 204 response for optimistic same-list moves. |
| modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb | Adds item→root outlet attribute for sortable-lists item controllers. |
| modules/backlogs/app/components/backlogs/work_package_card_list_component.rb | Adds list→root outlet + per-list accepted types + rows container selector. |
| modules/backlogs/app/components/backlogs/inbox_component.html.erb | Updates inbox list wiring to new per-list values/outlet contract. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts | Converts list controller to outlet-to-root model; enforces per-list accepted types. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts | Updates unit tests to mount real root+list and cover acceptedTypes/outlet behavior. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts | Refactors DOM contract to support arbitrary row containers and row resolution. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts | Adds coverage for new container/row/item resolution and absolute-position calculations. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts | Converts item controller to outlet-to-root model; centralizes moving gate in canDrag. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Updates unit tests to use real outlets and validate moving gate + type/root scoping. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts | Updates payload/type rules; adds root identity scoping and absolute-position payload mode. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts | Expands helper tests for root scoping, per-list acceptance, rows containers, and payload shapes. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts | Refactors root controller to moveUrlTemplates, position mode, root-only aria-busy, and moved event dispatch. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Updates root controller tests for new templates map, moved-event ordering, and aria-busy semantics. |
| frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts | Adds handling for client-dispatched sortable-lists:moved to refresh lock_version cache. |
| frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts | Adds unit coverage for the new sortable-lists moved event path. |
myabc
added
javascript
Deploying openproject with ⚡ PullPreview
|
Resolves sortable-list rows through an explicit rows container so consumers are no longer tied to ul/li-only markup. Rows are the direct children of the container named by the list's optional rowsContainerSelector value (e.g. ':scope > ul' for Primer BorderBox lists, which own their inner ul); without it, the list element itself holds the rows. Row lookup never resolves an ancestor, so an inner row of a nested dual-role surface resolves its own item rather than the outer one. resolveDropIntent computes the rows container once and carries it on the returned intent, so list-only end drops append after the true last row. buildMoveFormData receives the whole drop intent and supports both payload modes: relative (prev_id) and absolute (target_id plus a position counted over item rows only).
Replaces the root-pushed child state (outlet callbacks, the connectRoot handshake, RootAwareChild) with leaf controllers that read the sortable-lists root through a child-declared Stimulus outlet: values act as props down, Pragmatic payloads as events up, and the outlet as the pull-based context read. Lists now declare their own acceptedTypes and item drop targets accept same-type items only; the root's acceptedType value is gone. The item's canDrag becomes the single moving gate: a drop ends the drag before the persist request starts, so the repeated checks in canDrop and canMonitor were unreachable. moving becomes a private field behind a getter, the data-sortable-lists-moving attribute disappears, and aria-busy is set on the root element only. Root turbo frames declare a single moveUrlTemplates value (item type to URL template, JSON-encoded) instead of separate accepted-type/move-url-template values and CSS-selector-based list/item outlet attributes. List components (inbox, sprint, bucket, work package card list) outlet directly to the #backlogs_container root and declare their own acceptedTypes and rowsContainerSelector values; item draggable data outlets to the same root. Updates the affected component and request specs to the new attribute contract with an explicit assertion per list component for the outlet and acceptedTypes wiring.
Extracts the shared cache-refresh logic in SplitViewSyncController into a private refreshWorkPackage helper and adds onSortableListsMoved, bound to the client-dispatched sortable-lists:moved document event. Same-list moves resolve with a 204 and never trigger the server-dispatched backlogs:work-package-moved event, so the split view's cached work package would otherwise keep a stale lock_version after such a move. Adds the second action binding to the backlogs turbo frame in show.html.erb, alongside two new spec cases covering the refresh and the no-item-id guard.
Makes sortable-lists mark safe moves as optimistic so Backlogs::WorkPackagesController#move can answer a same-list drag with 204 No Content: the client has already reordered the row and nothing further needs to render. Menu-driven moves carry no optimistic param and cross-list moves change lists, so both keep the turbo-stream frame reload they need for sprint totals, blank slates, and buckets that no longer match their filter. The param stays out of the service params (the update service takes keyword arguments) and is cast with ActiveModel::Type::Boolean, so optimistic=false is not treated as truthy. Truncated lists opt out of the optimistic param: their visible window (a head/tail slice plus a show-more marker row) is server-computed, so hasTruncationMarkerRow keeps them on the stream reload path while the optimistic DOM reorder still happens client-side for responsiveness. Also hardens resolveRowsContainer against invalid rowsContainerSelector values, warning and falling back to the list element instead of letting querySelector's SyntaxError break the drop. On success the root dispatches sortable-lists:moved strictly after its moving flag clears; the split view listens to it (the 204 path has no server-dispatched event) and the new wait_for_sortable_lists_moved helper makes same-list drag specs deterministic, while cross-list and truncated-list drags keep waiting on the frame reload. Request specs cover the optimistic same-list, menu same-list, cross-list, optimistic=false, and failure branches; Selenium specs cover the truncated inbox and a same-list drag with the split view open.
myabc
force-pushed
the
implementation/74970-backlogs-pragmatic-dnd-final-rework
branch
from
July 4, 2026 19:14
cbdcaaf to
6f399c7
Compare
myabc
commented
Jul 4, 2026
| controller: "sortable-lists--list", | ||
| sortable_lists__list_sortable_lists_outlet: "#backlogs_container", | ||
| sortable_lists__list_type_value: "inbox", | ||
| sortable_lists__list_accepted_types_value: ["work_package"].to_json, |
ulferts
reviewed
Jul 7, 2026
ulferts
left a comment
ulferts
left a comment
Contributor
There was a problem hiding this comment.
Since the PR is closed now, it makes no sense to continue with the review here. For the work to not become lost, I am just committing my comments now.
| class: "op-backlogs-page", | ||
| data: { | ||
| controller: "backlogs--list-refresh sortable-lists", | ||
| sortable_lists_move_url_templates_value: { work_package: backlogs_move_url_template(@project) }.to_json |
Contributor
There was a problem hiding this comment.
No controller is needed on this turbo-frame at all. Turbo will only take the content of the turbo frame to morph it in. Properties on the frame itself are just ignored. Because of this, the whole data hash can be removed.
| if optimistic_same_list_move?(call, source_list) | ||
| head :no_content | ||
| return | ||
| end |
Contributor
There was a problem hiding this comment.
This assumption is unfortunately not true. If the card menu has ever been opened, it is stored in the DOM. That leads to out of date move options if e.g. the card at the bottom is moved after its menu was opened:
I only know about this because I struggled with it in #23962 . There, after a move, the card is just fetched from browser cache to reset the menu.
We could check if the menu behaviour can be switched so that it needs to be loaded every time it is opened. To me, that would be acceptable as I think it will rarely happen that uses will reopen a menu after having closed it before.
Contributor
There was a problem hiding this comment.
I currently also tend towards letting the frontend decide what needs reloading. It knows its state better than the backend does. This is just my currently opinion, though. I am not 100% sure of it. But the change done in https://github.com/opf/openproject/pull/24077/changes#diff-e418bbea6dc64e0e24ffbd3b2e50a162b643688303f78071f9bbf9fd38519745R83-R86 looks promising to me. The backend could then always just return what it changed and the frontend could react to it.
Contributor
Author
There was a problem hiding this comment.
We could check if the menu behaviour can be switched so that it needs to be loaded every time it is opened. To me, that would be acceptable as I think it will rarely happen that uses will reopen a menu after having closed it before.
I don't think this is possible with the component as it currently stands.
| end | ||
|
|
||
| def optimistic_same_list_move?(call, source_list) | ||
| call.success? && optimistic_move? && [call.result.sprint_id, call.result.backlog_bucket_id] == source_list |
Contributor
There was a problem hiding this comment.
I am never sure whether this works but
call.result.sprint_id == call.result.sprint_id_before_last_save && call.result.backlog_bucket_id == call.result.backlog_bucket_id_before_last_save
would be easier to understand
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
🤖 Opened by an agent on Alex's behalf.
Ticket
https://community.openproject.org/wp/AGILE-251 (parent PR: #23218)
https://community.openproject.org/wp/AGILE-284 (optimistic reordering)
https://community.openproject.org/wp/AGILE-292 (controller topology decision)
What are you trying to accomplish?
This addresses the architectural feedback on #23218 and #23893 (outlets scepticism, safeguard reduction, per-list types) plus the remaining "fully enable optimistic reordering" checklist item, as a separate branch so it can be reviewed as one coherent diff before folding into #23218.
Controller communication. The root no longer declares outlets or pushes state down (
connectRoot/RootAwareChild/reflectMovingare gone). Instead the leaf controllers each declare asortable-listsoutlet pointing at the root — the Stimulus analogue of context injection: values are props down, Pragmatic payloads are events up, and the child→root outlet is the pull-based read of shared root state (moving, root identity). Noclosest()state lookups in controllers.Safeguards centralized.
item.canDragis the singlemovinggate — a drop ends the drag before the persist request starts, so the repeated checks incanDrop/canMonitor/handleDropwere unreachable and are removed.movingis now a private field with a getter (thedata-sortable-lists-movingattribute is gone; nothing consumed it), andaria-busyis set on the root only.Per-list types. Lists declare
acceptedTypes(array value); the root'sacceptedTypeis removed. Item drop targets accept same-type items only. A list is a drop target only with non-emptyacceptedTypesand a non-emptytype(the persistedlist_type); partial config warns.Generic beyond
ul/li. Rows are now structural — direct children of a rows container, resolved via the new optionalrowsContainerSelectorlist value (successor ofdata-target-container-accessor; Backlogs BorderBox lists use:scope > ul). Verified against the AGILE-292 dual-role nesting shape (item surface wrapping an inner list), with both-direction nested resolution tests.positionMode: 'absolute'and per-typemoveUrlTemplatesare included so #23893 rebases to a pure consumer.Optimistic 204 (AGILE-284). A same-list drag-and-drop move now returns
204 No Contentand the optimistic DOM order is final. Gated on anoptimisticparam the sortable-lists root appends — action-menu moves (Move up/down/to top/to bottom) have no optimistic DOM reorder and keep the full turbo-stream frame reload. On success the root dispatchessortable-lists:moved(after itsmovingflag clears), which drives split-view lock_version refresh on the 204 path and doubles as the deterministic settle signal for the Selenium drag specs (replacing the frame-reload wait for same-list drags).Notes for reviewers
resolveItemElementimplements the "prefer the row's own item surface" rule via self-or-descendant resolution and document order rather than a:scope >selector chain — equivalent for the agreed dual-role markup, and the ancestor-climbing variant was an actual bug (an inner card row would resolve to the outer bucket's item). Tests cover both directions.resolveItemPosition(absolute mode) appends to the end when the previous item is hidden behind a truncation marker row. Not reachable in Backlogs (relative mode); flagged for the Experiment/project attributes dnd #23893 rebase.optimisticis deliberately not in the permitted service params —Backlogs::WorkPackages::UpdateService#calltakes keyword args.Test evidence
optimistic=false) and a same-list-drag-with-split-view-open feature spec.Merge checklist