[AGILE-251] Migrate Backlogs to Pragmatic DnD#23218
Conversation
8ac3e26 to
9994516
Compare
9994516 to
4a84694
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates Backlogs drag-and-drop from the legacy approach to Atlassian’s Pragmatic Drag and Drop, moving Stimulus wiring onto the inner work package card and introducing new frontend helpers/controllers to compute drop targets and ordering.
Changes:
- Introduces Pragmatic DnD-based Stimulus controllers/helpers for Backlogs item dragging and list drop targeting.
- Moves Backlogs-specific data attributes (story + draggable item wiring) from BorderBox rows to the inner work package card, and updates Backlogs views/components accordingly.
- Adds Atlaskit Pragmatic DnD dependencies and updates Ruby/JS specs to reflect the new DOM + data attributes.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/components/open_project/common/work_package_card_component_spec.rb | Adds coverage for forwarding system arguments to the root card element. |
| modules/backlogs/spec/support/pages/backlog.rb | Updates test selectors to target the new draggable item data attribute on the card. |
| modules/backlogs/spec/requests/backlogs/backlog_spec.rb | Adds assertions for Backlogs controller wiring and list targets in rendered backlog HTML. |
| modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb | Adjusts expectations to verify Backlogs Stimulus wiring is on the card, not the row. |
| modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb | Updates list/component specs for new target attributes and card-level data wiring. |
| modules/backlogs/spec/components/backlogs/story_points_component_spec.rb | Adds expectation around wrapper positioning for SR-only label placement. |
| modules/backlogs/spec/components/backlogs/sprint_component_spec.rb | Updates sprint DnD expectations to use Backlogs list targets + card-level draggable attributes. |
| modules/backlogs/spec/components/backlogs/inbox_component_spec.rb | Updates inbox drop-target assertions to new Backlogs list target attributes. |
| modules/backlogs/spec/components/backlogs/bucket_component_spec.rb | Updates bucket drop-target + card wiring assertions for new DnD approach. |
| modules/backlogs/app/views/backlogs/backlog/show.html.erb | Removes legacy content_controller wiring for Backlogs. |
| modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb | Replaces legacy generic-drag-and-drop wiring with data-controller="backlogs" root. |
| modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb | Moves Backlogs Stimulus wiring/data to the inner card; adds draggable card args. |
| modules/backlogs/app/components/backlogs/work_package_card_list_component.rb | Changes list target data attributes to data-backlogs-target="list" + id. |
| modules/backlogs/app/components/backlogs/work_package_card_component.rb | Forwards system arguments to the shared/common work package card. |
| modules/backlogs/app/components/backlogs/inbox_component.html.erb | Updates inbox list container data attributes to match Backlogs controller targets. |
| frontend/src/stimulus/controllers/dynamic/backlogs/item.controller.ts | Adds a Stimulus controller to make individual cards draggable + item drop targets. |
| frontend/src/stimulus/controllers/dynamic/backlogs/item.controller.spec.ts | Adds unit tests for item controller state rendering + preventUnhandled integration. |
| frontend/src/stimulus/controllers/dynamic/backlogs/drag-and-drop.ts | Adds helper utilities for resolving targets and computing prev_id for move requests. |
| frontend/src/stimulus/controllers/dynamic/backlogs/drag-and-drop.spec.ts | Adds unit tests for drag-and-drop helper logic (target resolution, ordering). |
| frontend/src/stimulus/controllers/dynamic/backlogs.controller.ts | Replaces HAL-events refresh behavior with Pragmatic DnD monitoring + move requests. |
| frontend/src/global_styles/primer/_overrides.sass | Updates styles for new drag/drop indicators and Box-card dragging visuals. |
| frontend/package.json | Adds Atlaskit Pragmatic DnD dependencies. |
| frontend/package-lock.json | Locks Atlaskit Pragmatic DnD dependencies and transitive packages. |
| app/components/open_project/common/work_package_card_component.rb | Allows forwarding system arguments; ensures Box-card class on root; adds card_arguments. |
| app/components/open_project/common/work_package_card_component.html.erb | Uses card_arguments to apply forwarded system arguments to the root element. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
cca6f5e to
10fa291
Compare
2ffd5e6 to
ba69795
Compare
8c94399 to
3e53b87
Compare
During functional testing the second card could not be dropped below the previously placed one, while the existing regression spec only pinned a drop above it. Adds an after: option to drag_work_package to cover the lower edge, which passes against the current branch.
The resting blankslate copy and the drag-over overlay copy had slightly
drifted apart. Single sentences lose their full stops, the drop label is
dropping the redundant container mention and goes singular ("this item",
to be revisited once multi-drag lands), and the inbox description merely
claims work packages outside a sprint or bucket land there.
Follows the core convention from spec/rails_helper.rb: support directories are meant for glob-required helpers, and _spec.rb files inside them risk being required into every suite and run twice. Specs covering test infrastructure live under spec/support_spec instead.
|
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. |
Restores the rows after a failed request only for as long as they remain exactly where the optimistic reorder had placed them. A concurrent morph that removed or repositioned them reflects the fresher server state that a rollback to the stale pre-move snapshot would silently undo.
Keeps row drop targets sticky only when pointer moves within the region. Over the list header or empty spaces, the sticky target releases itself. Otherwise the row would silently land next to its last-hovered item.
|
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. |
Adapts the add-existing-work-package flow from PR #23403 to this branch's (list_type, list_id) wire format: Target.parse/to_s give way to Target.from_list/to_list_params, and Target.for_work_package now encodes a work package's current location while Target.for keeps dev's container-based signature.
A type can only be absent on an unpersisted work package, but such a one raised a `NoMethodError` from `to_fs`, even corrupting RSpec failure messages that render work packages. Degrades to the standard-type layout instead, dropping the caption's colon too.
|
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 similar comment
|
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. |
|
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. |
A Turbo morph desyncs the drag-and-drop wiring in two ways: rows the morph replaces never receive the root reference, because Stimulus outlet-connected callbacks do not re-fire for them, so canDrag and canDrop refuse every interaction; and an element can keep Pragmatic's preserved drop-target marker attribute without its WeakMap registry entry, which silently dead-ends the drop-target search and kills all rows rendered underneath it. Both states persist until a full page reload. Re-hands the root and re-registers list, item and scrollable children from controller state once per morph batch.
The morph heal re-registers every child after a morph, restoring the marker attribute together with the registry entry it stands for, so preserving the attribute is redundant. Worse, preservation was the one mechanism that let the marker outlive its registration — the stranded state at which Pragmatic silently aborts its drop-target search. Letting the morph strip it keeps controller state the single source of truth.
The service also raises this failure when `list_id` is combined with the `direction` param, but the message only mentioned `list_type`.
The unified `list_type`/`list_id` wire format dropped the fail-fast on a missing target that `params.expect(:target_id)` previously enforced.
A `backlog_bucket_id` referencing no record cleared contract validation, only to crash on the foreign key constraint when saving. Sprints already have an equivalent guard through the assignable-sprints check.
|
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. |
A list-only drop back onto the source list was blanket-nulled while the list still rendered its drop outline, so the affordance lied. On start-position lists (the inbox) it also suppressed a legitimate move-to-top. Resolves such drops like any other list-only drop; the next commit suppresses drops that land at the source's current position before any request fires.
An item dropped at its own current position -- on an adjacent edge or back onto its source list -- still fired the full PUT, server reorder and reconciling frame reload for a DOM no-op. Compares the post- reorder placement against the pre-drop snapshot and returns before the request when nothing moved.
Stimulus outlets are document-scoped, so a broad outlet selector can match another root's lists and items; the heal then hands them the wrong root reference, last write wins. Backlogs pins its selectors under the frame id, so this is hardening for future multi-root pages rather than a live bug.
The move dialogs offer every sprint and bucket while the move URL keeps the page's active filters, so moving to a filter-hidden destination made the card silently vanish from the reloaded frame. Extends the invisibility check to the sprint/bucket filters and names the destination sprint in the existing flash message.
ulferts
left a comment
There was a problem hiding this comment.
What a ride but you reached the goal 🎉
Ticket
https://community.openproject.org/wp/AGILE-251
https://community.openproject.org/wp/AGILE-284 (optimistic reordering, folded in from #23643)
https://community.openproject.org/wp/AGILE-272
https://community.openproject.org/wp/AGILE-323
Resolves BUGs: https://community.openproject.org/wp/AGILE-252, https://community.openproject.org/wp/AGILE-258
What are you trying to accomplish?
Migrate Backlogs drag and drop to Pragmatic Drag and Drop while preserving the existing inbox / sprint / backlog-bucket move workflows.
The legacy generic (jQuery/Dragula) wiring is replaced by a small Stimulus suite on
@atlaskit/pragmatic-drag-and-drop. Move semantics and server endpoints are unchanged — only the client interaction layer. Backlogs cards now render as ViewComponents (server-rendered markup), and the root controller repairs DnD wiring after Turbo morphs (aturbo:morph-elementheal re-registers lists and items) so DnD survives stream updates.What approach did you choose and why?
The DnD layer is a reusable, Backlogs-agnostic
sortable-listsStimulus suite:sortable-lists.controller.ts— list "root": owns the move request, drag monitor, and auto-scroll; scopes each drop to its root (accepted type matches, source + resolved target contained) so multiple roots don't steal drops.item.controller.ts— per-card draggable + drop target with closest-edge indicators and a sanitized drag preview.drag-and-drop.ts— the Pragmatic payload contract plus pure, unit-testable drop-intent resolution.list-dom.ts— DnD-agnostic list/row DOM contract: selectors, item/row resolution (incl. truncation markers), optimistic reorder with snapshot/restore.turbo/pragmatic-dnd-morph-attributes.ts— aturbo:before-morph-attributeguard preserving client-owned DnD state attributes (drop indicators, selection) across idiomorph morphs. Pragmatic's internal registration marker is deliberately not preserved: registration is repaired by the root controller'sturbo:morph-elementheal instead, avoiding marker/registry divergence.list-refresh.controller.ts— non-DnD: reloads the frame when a work package changes outside a drag (split-pane / Angular edits emit HAL events, not a Turbo stream), hardened against a subscribe-after-disconnect race.Reordering is optimistic (AGILE-284, from #23643): the row moves on drop and persists in the background; the turbo-stream response reconciles on success, and a failure rolls back with a
prefers-reduced-motion-aware FLIP animation.Server moves still flow through
Backlogs::WorkPackages::UpdateService, which normalizeslist_idbefore validatinglist_type(sprint / backlog bucket / inbox) so an empty inbox id and an integer id are handled consistently.Keyboard handling is intentionally minimal: the card is the single focus target and the action menu is the accessible move path. Prototyped keyboard DnD and the drag-specific ARIA-role layer were removed rather than shipped half-complete; full accessible DnD is tracked under follow-up work.
Working checklist
draggable="false"so a native link/image drag can't pre-empt the card drag (complementscanDragFromPoint).positionMode: 'absolute'204 No Contentapproach was rejected in review)Addressed from review (2026-07-05 triage)
.op-border-box-listso the globaldata-draggingmarker no longer restyles admin/settings dragula lists (review#5)#9)#7)drop-position-valueinfrontend/CONTEXT.md(review#15)#13)list_typethroughBacklogs::Targetinstead of hand-spelled literals (review#14)Review follow-ups (deferred; 2026-07-05 triage)
movingguard until the reconciling frame reload lands, not on PUT resolve — a second drag races the about-to-morph DOM (review#2;sortable-lists.controller.ts) → https://community.openproject.org/wp/AGILE-338aria-currentremoval inside the backlogs frame (review#8a;pragmatic-dnd-morph-attributes.ts) → https://community.openproject.org/wp/AGILE-336 (server hydratesaria-current, controller re-syncs after morphs; drag-gating rejected — the guard exists for non-drag list-refresh morphs)#10;sortable-lists/drag-and-drop.ts) → https://community.openproject.org/wp/AGILE-337 (product input needed on target semantics)UpdateServicecommit + post-commitmove_after) in one transaction — a failure between the phases can leave a stale/nil position (review#12;work_packages/update_service.rb) — done in stacked PR [AGILE-322] Backend support for optimistic reordering #24157#4;work_packages_controller.rb)vi.mocknot at top level" deprecation in the sortable-lists specs (top-levelvi.mock+ aliasedcore-stimulus/test-helpers; vitest-v4 hoister × vite-alias, non-fatal today)pick_up_and_release_work_packageis now capped viaretry_block, but the stale node it retries on (likely a mid-drag list-refresh morph detaching the grabbed row) should be waited out or re-queried inside the drag so no retry is needed (spec/support/pages/backlog.rb) → root cause (mid-drag list refresh) tracked in https://community.openproject.org/wp/AGILE-336Review follow-ups (deferred; 2026-07-09 review)
sortable-lists/list.controller.ts)dispatchevents (discussion;backlogs/work-package.controller.ts)backlogs.controller.tssplit intolist-refresh/work-packagecontrollers into a separate PR — parked for now (discussion)Actionable issues from Codex review (2026-07-10)
dropPositioninstead of reusing the last-hovered row (sortable-lists/item.controller.ts).<ul>remains connected; restore only if the row is still at its optimistic placement (sortable-lists/list-dom.ts).Defer or roll back eagerRejected — eager selection on card activation is intentional:data-selectedupdates until Turbo accepts the split-pane visitdata-selectedis ephemeral client state (and becomes multi-select later), whilearia-currenttracks the URL-synced work package; the two are distinct axes. The review's failure model is also incorrect: the promoted frame visit firesturbo:before-visitfromfetchResponseLoaded— after the frame has already fetched and rendered — so a declined unsaved-changes prompt leaves the new pane visible, matching the eager selection; only the URL andaria-currentstay behind, by design. (Supersedes review#6.)Actionable issues from Codex + Copilot review (2026-07-13 triage)
dropPositioninstead of the blanket no-op guard, and short-circuit genuine no-op drops before the move request (sortable-lists/drag-and-drop.ts,sortable-lists.controller.ts)work_packages_controller.rb#work_package_invisible_after_move?)retryin the four DnD drag spec helpers viaretry_block(spec/support/pages/backlog.rb) — parked; revisit alongside [AGILE-322] Backend support for optimistic reordering #24157 (itsframe_reloadderivation touches the same helpers). Chrome 150 also surfaces mid-morph stale nodes asSelenium::WebDriver::Error::UnknownError("Node with given id does not belong to the document"), which no current rescue list catches — fold into the same pass.sortable-lists.controller.ts)Archived
bundle exec rspec modules/backlogs/spec/features/work_packagesFollow-up work
clickable?option on the BorderBox list work-package item (discussion)Merge checklist