[AGILE-278] Move selected work packages atomically - #24778
Conversation
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
|
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. |
a31087a to
320b49b
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. |
2ed9b04 to
1773e7f
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. |
| def with_ordered_locks(entries, index = 0, &) | ||
| return yield if index >= entries.length | ||
|
|
||
| OpenProject::Mutex.with_advisory_lock_transaction(entries[index]) do | ||
| with_ordered_locks(entries, index + 1, &) | ||
| end | ||
| end |
There was a problem hiding this comment.
@ulferts recursion is probably ok in "normal use", but if someone were to submit a massive batch of work packages then the stack would overflow. Should we set a soft-limit somewhere?
There was a problem hiding this comment.
@ulferts I've set a batch size limit to 500 for the time-being.
There was a problem hiding this comment.
I guess the reason why this is a recursion to begin with is that with_advisory_lock_transaction does not provide a non block implementation. That is too bad as it leads to quite a deep stack that might as well have been avoided by an iterative approach.
A simple thing to change is to at least skip the transaction part of the with_advisory_lock_transaction call by with_advisory_lock_transaction(entries[index], nil, transaction: false). The only caller already wraps a transaction around the call.
Claude suggests that it would be possible to completely flatten the stack by this:
Put the name format and the acquisition next to each other in OpenProject::Mutex, so there's one definition of mutex_on__:
def advisory_lock_name(entry, suffix = nil)
name = +"mutex_on_#{entry.class.name}_#{entry.id}"
name << "_#{suffix}" if suffix
name
end
# Takes transaction-scoped advisory locks for several entries in the given
# order, flat rather than one nested block per entry. Requires an open
# transaction: the locks are released when it ends, never before.
#
# Blocking (pg_advisory_xact_lock), unlike with_advisory_lock_transaction,
# which polls pg_try_advisory_xact_lock with a Ruby sleep. The database
# queues the waiter and its deadlock detector sees the wait graph.
def acquire_advisory_locks(entries, suffix = nil)
connection = ActiveRecord::Base.connection
raise ArgumentError, "advisory locks require an open transaction" unless connection.transaction_open?
entries.each do |entry|
keys = connection.lock_keys_for(advisory_lock_name(entry, suffix)).map { Integer(it) }
connection.execute("SELECT pg_advisory_xact_lock(#{keys[0]}, #{keys[1]})")
end
end
and at the call site, with_ordered_locks disappears:
WorkPackage.transaction do
OpenProject::Mutex.acquire_advisory_locks(lock_entries(placement.anchor))
revalidate_cohort!
revalidate_target_availability!(target)
revalidate_anchor!(placement, target)
...
end
There was a problem hiding this comment.
After more digging as part of reviewing #24779 the above statement needs to be clarified. There is a bug in OpenProject::Mutex.with_advisory_lock_transaction as transaction: false cannot be overwritten. Either that is fixed and/or a method is added to add an advisory_lock without a transaction following the naming schema. This would not flatten the stack completely but would reduce it at least.
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. |
2f3d5a6 to
2e471bc
Compare
There was a problem hiding this comment.
Quite a mighty PR @myabc . It took me a while to go through this and I relied on AI to help me with it. Given the size, I concentrated on correctness, structure and functionality but I did not go into too much detail. Some errors might have been overlooked and wording and readability were not high up on my priority list.
As a user, the d&d behaviour really feels nice. It feels natural to select a bunch of work packages and drop them wherever it is indicated. I also find it quite nice that it is possible to drag from multiple lists at the same time. My comments next to the code show some areas where improvement is still possible ans some errors are also flagged. But overall, the UX is top notch.
When a page with more content is rendered, performance starts to become a problem. The selection needs a while to initialize after the page loads. If the user then starts dragging even few work packages, the performance becomes even worse with drop indicators being rendered sluggishly. When dragging more work packages, the browser can even freeze. All this happens before the backend is involved. I didn't identify the exact hotspots but some of the suspected inefficiencies have been noted in the comments. It would also be beneficial to have the locking mechanism in the backend changed from being recursive to being iterative to prevent StackLevel errors. It didn't fail for me with 100 work packages though. The frontend prevented me from trying out moving 500. Whatever limit we enforce, it should be communicated to the user as quickly as possible. With CTRL-A, it is quite easy to select a large number of work packages.
Please find more detailed feedback within the code comments.
Edit:
I initially forgot to mention one other topic:
- When dragging multiple items to an external application behaves in unexpected ways. Only the element actually dragged is dropped into an external application. This is understandable as we mimic the multi drag by tagging the other elements along on top of the HTML5 dragged element. Given that we cannot support dragging multiple work packages outside of the application, should we disable this visibly?
| - `./src/stimulus/` - Stimulus controllers | ||
| - `./src/turbo/` - Turbo integration | ||
| - `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list; selecting across lists is a deliberate gap, reserved for a separate mechanism. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Batch movement is not implemented: a drag still moves one card and collapses any wider selection onto it — that's a later work package. | ||
| - `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list; selecting across lists is a deliberate gap, reserved for a separate mechanism. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Dragging a selected card moves the whole batch: the root freezes the drag's batch at drag start (`beginDragBatch`), and a selection-enabled root with a `collectionMoveUrl` value submits ordered `ids[]` to the collection move action — for one dragged card or many. Dragging an unselected card still collapses any wider selection onto it. A root's `moveAnnouncementScope` value keys the move announcements the same way `announcementScope` keys the selection ones. |
There was a problem hiding this comment.
Following up on my comment on the previous PR, I'd still move this into the sortable lists directory
| // Confined when the item is, or when any batch-mate the drag would carry | ||
| // is. Members are same-list by construction, so one confined member pins | ||
| // the whole block to the list they all sit in. | ||
| dragConfined(itemElement:HTMLElement):boolean { |
There was a problem hiding this comment.
This comment is not correct. Batches can in fact span multiple list. Preliminary tests suggest, that this is behaving in a somewhat predictable manner so I wouldn't remove that possibility.
But the limitations are not correctly applied in some cases:
- Have a read only item in one container
- Have a writable item in another container
- Within the container of the writable item have more items
- Select the read only item first
- Then select the writable item
- Move the item within the container of the writable item
=> It looks as if the work packages can be moved inside the writable item's container.
In this case, the error message could also be better, stating errors per work package.
When dragging two read-only work packages from two different containers together with a writable one from a third container should find no valid drop target at all. Currently, it does - the third container.
There was a problem hiding this comment.
Not sure if it would be preemptive to already think about boards where one of the challenges will be to prevent a work package being dragged into a container (because the status is not in the workflow). This might then be used to prevent work packages being moved out of their container on read-only as well. This would change the paradigm from "the work package cannot be moved out of its container" to "Those are the containers the work packages can be moved into - currently the list of containers is empty".
| && resolveItemType(item.element) === type | ||
| )); | ||
|
|
||
| return outlet && outlet.element instanceof HTMLElement ? outlet.element : null; |
There was a problem hiding this comment.
This is rather inefficient. It iterates over all outlets and then uses those to find a single element. Ultimately, it is not interested in the outlet but only the element of it. An implementation similar to orderedItemElements(root) from selection.ts would be more efficient.
This is relevant as the method is called repeatedly when dragging for each dragged card and triggered by multiple callbacks: getInitialData, onGenerateDragPreview, onDragStart, handleDrop.
From a performance perspective, the best solution would be to have a map to lookup in constant time. That map should not be persisted as to not become stale on morphing the DOM, but it could be recreated by callback and then passed on. If that is used, itemElementFor could be remove altogether which would be a good side effect as it also is somewhat alien in this controller.
| WorkPackage.transaction do | ||
| with_ordered_locks(lock_entries(placement.anchor)) do | ||
| revalidate_cohort! | ||
| revalidate_target_availability!(target) |
There was a problem hiding this comment.
To be honest, while I acknowledge that the container might be changed mid-flight, I find it highly unlikely that it will. Additionally, what will happen if it does:
- container deleted -> rollback of all work packages (wp contract catches)
- container closed -> rollback of all work packages (wp contract catches)
- container renamed -> nothing
Maybe I am missing a case but as of now, I don't see a reason for a lock.
| itemIds?.forEach((id) => data.append('ids[]', id)); | ||
| data.append('list_type', type); | ||
| data.append('list_id', listId ?? ''); | ||
| data.append('prev_id', previousItemId ?? ''); |
There was a problem hiding this comment.
With this fallback, prev_id will never be null. Therefore, the :append method will never be used. Haven't checked yet if that is a problem.
| ) | ||
| end | ||
|
|
||
| ordered |
There was a problem hiding this comment.
Moving the checks into a contract might also help with this method having unexpected side effects. It is called load_collection_work_packages but depending on whether there is an error or not, it either returns the work packages or not with the side effect of errors being rendered.
| end | ||
|
|
||
| def collection_move_service_params | ||
| move_collection_params.to_h.symbolize_keys.except(:ids).compact |
There was a problem hiding this comment.
This method is called a couple of times during a single request. This could be memoized. Not sure if it will have a big effect so feel free to disregard.
| WorkPackage.transaction do | ||
| with_ordered_locks(lock_entries(placement.anchor)) do | ||
| revalidate_cohort! | ||
| revalidate_target_availability!(target) |
There was a problem hiding this comment.
Thought about this again for a bit. Maybe it is a good idea to acquire a lock for the container. With that in place, would have the benefit of knowing that the container does not change. Since it in effect also blocks parallel batch drops to the list, it should also reduce the likelihood of the positions column becoming corrupted.
| render_invisible_after_move_batch_flash(call.result) | ||
| else | ||
| render_error_flash_message_via_turbo_stream( | ||
| message: I18n.t(:notice_unsuccessful_update_with_reason, reason: call.message) |
There was a problem hiding this comment.
When a batch of work packages is moved, the error displayed is not very helpful to know which one errored:
There is no reference to the work package that causes this.
This might be improved by the proposed merging on ServiceResults and then using that information to print the error per work package. We do something similar printouts on bulk editing work packages and when copying projects, I think.
0a61ca8 to
09d345a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
4353a15 to
8df3314
Compare
8df3314 to
a344784
Compare
Adds PUT projects/:id/backlogs/work_packages/move taking ordered ids and a three-state prev_id (after an anchor, top, or append). A batch service chains each member after the previously moved one inside one outer transaction, acquires advisory locks in ascending id order, and revalidates the anchor and the project cohort under lock, so a batch commits as one contiguous block or not at all and after-commit hooks only ever observe the completed batch. The optimistic response skips the frame reload only once the persisted rows verifiably form the requested block, and the moved event carries the ordered ids. https://community.openproject.org/wp/AGILE-278
Dragging a selected card now moves the whole selection: the root freezes the batch at drag start, resolves the drop against the excluded selected ids so no member can anchor its own insertion, reorders every row as one block, and submits ordered ids to a root-configured collection URL for one card or many. Every represented row carries the dragging treatment and batches show a count on the preview, which renders before drag start and so freezes the batch itself. Success clears the selection, an unverifiable rollback warns even when the server flash stays silent, and move announcements speak the consumer's vocabulary through a dedicated scope value. https://community.openproject.org/wp/AGILE-278
Iterates the ordered work_package_ids from the collection event and falls back to the singular field, because no single scalar can name whichever batch member the split view has open. https://community.openproject.org/wp/AGILE-278
Adds the collection move URL and announcement scope values to the Backlogs root, with work-package wording for every announcement the shared controller can speak. https://community.openproject.org/wp/AGILE-278
Merges the singular and plural invisible-after-move notices into one count-pluralized key and converts the batch announcement keys to i18n-js plural hashes, so locales with richer plural rules than English can translate every form.
Proves the ordered cross-list block, the collapse-and-move-alone path, atomic rejection with preserved selection, and the reload- free optimistic same-list reorder against a real browser and database. https://community.openproject.org/wp/AGILE-278
View-only users had no synchronous response to a click or Enter until the frame visit landed, since the previous feedback relied on the permission-gated batch selection. Adds a visual-only mark.
A count in the corner was the only sign that a drag carried more than one card. Ghost layers behind the preview make the batch legible at a glance, composed into the card's own lift shadow so both survive. https://community.openproject.org/wp/AGILE-396
The stack drew two ghost layers whatever the batch, so a pair of cards and a dozen read alike. Its depth now follows the batch up to four cards, leaving the badge to carry the exact count. The overhang stays reserved at the deepest stack so the badge keeps one offset.
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.
a344784 to
429e9f9
Compare
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.
Merge checklist