Repository navigation
[AGILE-278] Move selected work packages atomically - #24778
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Implements atomic, ordered batch movement of Backlogs work packages when dragging a selected card, via a new collection move endpoint and corresponding client-side batch drag behavior. This extends the existing sortable-lists selection foundation by adding a batch-aware move contract, server-side locking/validation for correctness under concurrency, and updated announcements/UX affordances (dragging marks + preview badge).
Changes:
- Added
PUT /projects/:id/backlogs/work_packages/move(collection action) and server-sideBacklogs::WorkPackages::BatchUpdateServiceto move ordered batches atomically with advisory locks and revalidation under lock. - Updated
sortable-listsStimulus controllers to freeze the drag batch at drag start, submit orderedids[]to the collection endpoint, exclude selected ids when resolvingprev_id, and handle optimistic rollback/announcements for batch moves. - Added/updated routing, request, service, and Selenium feature specs; extended i18n strings and internal developer docs (frontend AGENTS).
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/support/pages/backlog.rb | Adds a drag helper tailored for rejected moves (no frame reload). |
| modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_spec.rb | New spec coverage for batch move service behavior (atomicity, locks, staleness). |
| modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb | Verifies routing for the new collection move endpoint. |
| modules/backlogs/spec/requests/work_packages/move_collection_spec.rb | New request specs for validation, success/failure responses, and invisibility flashes. |
| modules/backlogs/spec/requests/backlogs/backlog_spec.rb | Ensures the Backlogs page includes collection move URL + announcement scope values. |
| modules/backlogs/spec/features/work_packages/batch_move_spec.rb | New Selenium feature spec covering batch drag, collapse behavior, and failure preservation. |
| modules/backlogs/lib/open_project/backlogs/engine.rb | Grants permission for the new move_collection action. |
| modules/backlogs/config/routes.rb | Adds the collection move route mapped to move_collection. |
| modules/backlogs/config/locales/js-en.yml | Adds Backlogs-specific move announcement strings for batch/singular moves. |
| modules/backlogs/config/locales/en.yml | Adds server-side batch move error messages and plural invisible-after-move notice. |
| modules/backlogs/app/views/backlogs/backlog/show.html.erb | Wires new Stimulus values (collection move URL + move announcement scope) into the view. |
| modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb | New atomic batch move service chaining member moves under locks/transaction. |
| modules/backlogs/app/controllers/backlogs/work_packages_controller.rb | Adds move_collection action, validation/loading of ordered batch ids, and optimistic-skip logic. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts | Adds drag batch snapshotting and post-success silent selection clearing. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts | Unit tests for batch snapshotting and silent clear behavior. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts | Updates root port mocks to match new drag-batch API. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts | Adds a batch-count badge overlay to the drag preview for multi-row drags. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts | Tests preview badge behavior for batch vs single drags. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts | Updates root port mocks to match new drag-batch API. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts | Updates append-resolution helper to exclude a set of ids (batch-aware). |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts | Updates and extends tests for excluded-id append resolution. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts | Begins drag batch on preview and drag start; passes batch size to preview renderer. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Tests preview badge behavior and beginDragBatch invocation ordering. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts | Adds ids[] payload support and predecessor resolution that excludes selected/batch ids. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts | Tests batch form payload and excluded-id predecessor resolution. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts | Implements frozen drag batch lifecycle, batch row marking/cleanup, collection move submission, and batch announcements/failure handling. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Extensive unit coverage for batch drag/drop behavior, requests, cleanup, and announcements. |
| frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts | Consumes both singular and batch moved events and refreshes cached work packages accordingly. |
| frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts | Tests batch event consumption, uncached skipping, and no-id events. |
| frontend/AGENTS.md | Updates internal documentation to reflect new batch drag + move announcement scope behavior. |
| config/locales/js-en.yml | Adds generic sortable-lists batch announcement strings for non-Backlogs consumers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
myabc
added
feature
DO NOT MERGE
javascript
This comment was marked as outdated.
This comment was marked as outdated.
|
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. |
myabc
force-pushed
the
implementation/AGILE-278-batch-move
branch
from
August 15, 2026 21:53
a31087a to
320b49b
Compare
myabc
commented
Aug 15, 2026
myabc
commented
Aug 15, 2026
myabc
commented
Aug 15, 2026
|
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. |
4 of 7 tasks
myabc
force-pushed
the
implementation/AGILE-278-batch-move
branch
from
August 17, 2026 12:02
2ed9b04 to
1773e7f
Compare
myabc
marked this pull request as ready for review
August 17, 2026 12:21
This comment was marked as outdated.
This comment was marked as outdated.
myabc
commented
Aug 18, 2026
myabc
force-pushed
the
implementation/AGILE-278-batch-move
branch
from
August 18, 2026 13:40
1773e7f to
5b40a07
Compare
|
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. |
myabc
force-pushed
the
implementation/AGILE-278-batch-move
branch
from
August 18, 2026 17:18
2f3d5a6 to
2e471bc
Compare
This was referenced Aug 18, 2026
Both preview.ts and the stylesheet carried the overhang widths and the layer cap, held in step by comments alone, so retuning the stack would have left the overhang uncontained. Sass now publishes the widths as custom properties and holds any deeper batch at the maximum itself.
The selection orchestrator honours only the platform's own multi-select modifier now, and the suites pin the platform to Windows, so a Meta- modified click there is an ordinary click that collapses the selection. Drives the batch-move examples through Ctrl, as the selection specs do.
Takes one advisory lock per container before the batch runs, so two concurrent moves cannot interleave their placements, and wraps the batch in its own savepoint: joined into an enclosing transaction, a rollback would otherwise be swallowed and half the batch would commit. Reports the member that refused through the result's dependent errors rather than a bespoke failure object.
Moves the shape of a batch move request behind a params contract the service consults itself, so a caller reaching past the controller gets the same refusal, and keeps the contract's errors off the project it validates against.
Gives the orchestrator one action-scope API in place of four gesture-named collapse methods, and splits the drag lifecycle so the batch is frozen once in the preview callback and its rows marked at drag start. A drag now always selects the card it carries; a menu move collapses the batch onto the card it names instead, and speaks only when a wider batch really collapsed.
Resolves each batch member's permitted destinations once at drag start and gates every drop on their intersection, so a batch carrying a confined member can only land where that member may go. Refuses a drag whose batch exceeds the server's cap before it starts, with an assertive announcement, rather than letting it fail on the drop. Destinations are frozen as identities, not list elements: a morph that replaces a permitted list mid-drag would otherwise leave the payload naming a detached node.
Reduces a drop target's payload to the identity a drop actually reads, so the batch-aware fields are computed once for the dragged source instead of on every dragover, and hands an external drop every member of the batch rather than the one card under the pointer.
Pairs the pressed-state writes with the aria-current helpers they mirror, and adds feature coverage for a batch dropped into an empty list, at the top of a list, selected with Ctrl/Cmd+A, and behind the inbox's truncation fold.
Every item drop target resolves its owner list on each dragover by scanning the list outlets. The owner cannot change while a drag is in flight, so the root now remembers it from drag start to drop, alongside the frozen batch.
Reads the collection URL once per drop and lets it alone pick the collection or single-item contract, instead of inferring the choice from a null batch and testing the URL twice on the way.
Renames the identity-only predicate and checks the outer source payload shape without iterating destination entries during dragover. Consolidates destination construction while preserving defensive ID normalization and adds coverage for both payload boundaries.
Moves the HTTP(S) link policy into a dedicated external-data module and accepts mixed-case schemes without changing the URL text. Adds the first helper tests while preserving rejection of unsupported schemes.
Limits ordinary selection rendering to changed membership while keeping full DOM repair for reconciliation and setup or teardown. Skips the traversal entirely when membership is unchanged. Adds mutation-observer coverage for untouched rows, composite identities, nested roots, silent clearing and repair after attributes are stripped.
Exercises independent picks, range resizing and reversal, Select All narrowing, cancellation and failed retries through the batch move path. Verifies same-list optimistic cleanup preserves unrelated descriptions and drops the old anchor. Confirms registration healing and synthetic selection clearing retain frozen membership. Extends existing missing- member refusal coverage through healing and adds a Shift-built feature case.
Removes duplicate rendering after click and Space toggles. Leaves presentation and announcements owned by toggleWithinCohort. Relates to AGILE-278.
Acquires the project inbox placement mutex for explicit anchors as well as unanchored moves. Prevents disjoint batches from shifting each other's predecessor positions between read and insertion. Covers distinct anchors with a deterministic concurrency regression. Relates to AGILE-278.
Clarifies that the paused insertion has already read its predecessor position. Leaves the reviewed locking implementation unchanged.
Destroys fixture work packages before their project so journal cleanup callbacks run. Prevents 54 leaked journals from breaking later bulk controller examples that count work-package journals.
myabc
force-pushed
the
implementation/AGILE-278-batch-move
branch
from
September 11, 2026 14:58
78d16db to
9d80cb2
Compare
|
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. |
ulferts
approved these changes
Sep 15, 2026
ulferts
left a comment
ulferts
left a comment
Contributor
There was a problem hiding this comment.
Completed my second pass @myabc . The performance is now way better. Starting to drag larger batches of work packages does not pose a problem any more.
I still ran into performance problems, i.e. when dropping larger batches. The bottlenecks seem to be both on the frontend as well as on the backend. Personally, I'd lower the MAX_BATCH_SIZE for now until we have established a realistic number the system can cope with. I'd also continue working on improving the performance as that to me is currently the major problem of d&d. Combined with the already non optimal performance outside of d&d, the performance topic is something to look into. Using the application for multiple drags led to the browser freezing up.
I left another batch of comments. However I am not opposed to merging this PR as long as the comments are handled eventually (or refuted)
| # contract would clear and repopulate the live project's own error bag. | ||
| def errors | ||
| @errors ||= ActiveModel::Errors.new(self) | ||
| end |
Contributor
There was a problem hiding this comment.
Probably outside of the scope of this PR: Building on it's own error object (which used to be the case a while back) makes sense on a ParamsContract. Potentially, ParamsContracts shouldn't have a model to begin with.
|
|
||
| # Bounds the recursion in with_ordered_locks (one stack frame per member | ||
| # plus the anchor). Enforced by the controller before it loads the batch. | ||
| MAX_BATCH_SIZE = 500 |
Contributor
There was a problem hiding this comment.
I tried moving a couple of batches on a larger page a couple of times in a row. That page had a larger set of work package on it but nothing I would call exaggerated (~1000). Moving them did not feel fluid.
Good news is that after the changes, the first drag feels fluid even with a larger set being dragged. When dropping, the cards take a while to become unselected which is understandable since it takes a while for the background job to process the changes. The dropped work packages were already at their new destination. But it was still not possible to continue working as the frontend was not responding (sometimes neither opening a work package in the split view nor selecting another item worked, sometimes it did). Sometimes, especially when dragging larger batches the "browser tab unresponsive" warning would show up.
I did not track down the reason for this. The observations point to the frontend being the bigger problem. But maybe we should:
- reduce the MAX_BATCH_SIZE
- identify the performance bottlenecks
- optimize
- increase the MAX_BATCH_SIZE
Can be done in a follow up PR.
| return ServiceResult.failure(errors: contract.errors) unless contract.valid? | ||
|
|
||
| target = Backlogs::Target.from_list(list_type, list_id) | ||
| return mixed_projects_failure unless work_packages.map(&:project_id).uniq.one? |
Contributor
There was a problem hiding this comment.
Preference: Why not place this in the BatchMoveParamsContract. In the end, the failure is caused by faulty params.
Contributor
Author
There was a problem hiding this comment.
Will tackle in a follow up.
| return stale_batch_failure unless cohort_intact? | ||
|
|
||
| lock_destination_row!(destination) | ||
| return unavailable_target_failure unless target_available?(target) |
Contributor
There was a problem hiding this comment.
Could those two statements be folded into one? This would have the following benefits:
- Not acquiring a lock before checking for the target being available. Edge case but one could exploit this.
- Issuing only a single database statement instead of two
- Avoiding to update
destinationunnecessarily
The code could look like this:
Sprint.assignable(project: batch_project, user:).lock.exists?(id: target.list_id)
I am aware that this might contradict the changes to the code done in the follow up PR so double check.
Honestly, I am not 100% convinced that the FOR UPDATE lock is necessary at all. I agree that it gives extra certainty but at least the service based updates should acquire advisory locks.
| @@ -392,13 +520,29 @@ export default class SortableListsController extends Controller<HTMLElement> imp | |||
| return this.ownerListOf(itemElement)?.rowsContainer ?? null; | |||
Contributor
There was a problem hiding this comment.
Could this.dragOwnerDestinations be used here as well to avoid querying the DOM?
| const anchor = listRows(rowsContainer) | ||
| .find((row) => resolvePreviousItemId(row, rowsContainer) === previousItemId); | ||
|
|
||
| return (anchor as HTMLElement|undefined) ?? null; |
Contributor
There was a problem hiding this comment.
Will work today, but it does not contain the type so nested lists might no longer work later on.
| const identity = itemIdentity(item); | ||
| if (changedKeys && (!identity || !changedKeys.has(selectionKey(identity)))) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
I'm torn here. There is the performance gain from having changedKeys. But if that ever gets stale, and lastRenderedKeys is now maintained in three methods, it will lead to the wrong items staying selected. Maybe, it would be better to read the previous selection from DOM
const selected = identity !== null && keys.has(selectionKey(identity));
if (selected === item.hasAttribute(batchSelectedAttribute)) {
continue;
}
This assumes it is less costly to read the DOM than to write to it which I didn't check.
As a follow up, the three maintainers of lastRenderedKeys (renderSelection, clearSilently and renderRangeRestart might be consolidated into a single method (hoping here) by separating the painting from announcing and calling the announcing method with parameters that potentially silences it.
The old "join" mode replaced the selection whenever the card was outside it, so its name and comment described behavior it never had. Removes the selectedItems() enumeration, which production never called; specs observe rendered selection and hasSelection instead.
Rails.logger dropped the backtrace, and the method-wide rescue also turned preparation bugs into a generic flash. Rescues only around the transaction and reports through OpenProject.logger with the batch ids.
Dropping large batches still freezes the page on a heavily populated backlog. Keeps the cap provisional until the drop path is profiled; the frontend limit follows the constant.
Batch announcements never read the item label, so the lookup and its fallback now run only on the singular paths. Also simplifies the default drop exclusion, spells out the sameDestination null guard and corrects a comment multi-item moves outdated.
A batch drops only where every member may go, which the README did not say.
Contributor
Author
|
@ulferts I've lowered the cap to 50 for now. Profiling should be a follow up. |
|
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. |
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.
Note
This PR is part of a stack. Please review and merge in this order: #24525 → this PR. #24844 has been merged into this PR.
Ticket
https://community.openproject.org/wp/AGILE-278
also incorporates https://community.openproject.org/wp/AGILE-396 (previously approved and merged)
What are you trying to accomplish?
Dragging a selected Backlogs card now moves the complete batch selection as one ordered, atomic operation; dragging an unselected card keeps the existing collapse-and-move-alone behavior. This is the batch-movement slice of AGILE-181, building on the selection capability from #24525.
PUT projects/:id/backlogs/work_packages/move) accepts orderedids[]with a three-stateprev_id(after an anchor, top, append) and rejects blanks, duplicates, unresolvable members, invalid targets, and stale or in-batch predecessors outright.Backlogs::WorkPackages::BatchUpdateServicechains each member after the previously moved one inside one outer transaction, acquires advisory locks in ascending id order across the batch and its anchor, and revalidates both the anchor and the project cohort under lock — the batch commits as one contiguous block or not at all, and after-commit hooks only ever observe the completed batch.work_package_idsfrom the collection action while the member action keeps its scalar, and the split-view synchronizer consumes both.What approach did you choose and why?
The collection action is deliberately not an overloaded member action: existing singular callers (menu moves, non-selection sortable roots) keep the member route untouched, while a selection-enabled root submits one or many cards through the collection contract. The service wraps the existing single-work-package
UpdateServiceper member rather than reimplementing the move, so validation and journaling stay in one place; a realBatchFailureexception (neverActiveRecord::Rollback, which joined transactions swallow) carries the failed result out of the transaction. Advisory locks are pre-acquired in canonical ascending-id order because each inner update takes the same per-work-package lock with an unbounded wait — opposing batches would otherwise deadlock. Two subtleacts_as_liststaleness bugs surfaced during review and are covered by regression tests: in-memory positions feedingremove_from_listthresholds mid-batch (which could persist duplicate positions), and after-commit hook contexts observing interim positions.On the client, the frozen batch lives in root-owned state consumed exactly once per drop, so Escape or a Turbo morph mid-drag can never change what is submitted, and the drag preview freezes the batch itself because Pragmatic renders previews before
onDragStartfires.Keep-batch-on-menu-invocation, positional batch actions, and batch destination dialogs are follow-up slices: AGILE-362, AGILE-363, AGILE-364.
Inbox batch placement is serialized per project for the full transaction, including explicitly anchored moves. Acquisition retains the mutex helper’s existing polling without a configured timeout, so another batch may wait for all member updates to finish. This coordinates collection moves; older member-service callers do not yet share the destination mutex. That remaining race is tracked in AGILE-426.
Merge checklist