[AGILE-364] Apply batch selection to contextual ActionMenus - #24781
[AGILE-364] Apply batch selection to contextual ActionMenus#24781myabc wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR unifies Backlogs work package ActionMenu behavior across all entry points (More button, right-click, Context Menu key, Shift+F10) by settling the live action scope before menu presentation, and by splitting the menu into stable singular vs batch action groups while preserving the one-card experience.
Changes:
- Add a shared “prepareActionMenu” pre-open path (contextual
beforeOpen+ popoverbeforetoggle) so every invocation settles selection/action scope consistently. - Render two stable ActionMenu groups (“This work package” vs “Selected work packages”) and let the item controller project visibility + pluralized headings based on the resolved scope.
- Strengthen batch move locking to include ordered source/target lifecycle locks and serialize unanchored placements (append/top), with expanded concurrency coverage.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/support/pages/backlog.rb | Adds higher-level helpers for opening/validating menus via all entry points and probing batch move payloads. |
| modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_spec.rb | Updates lock-order expectations to match new lifecycle/placement locking. |
| modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_concurrency_spec.rb | Adds concurrency scenarios covering inbox placement/top moves and sprint-finish races. |
| modules/backlogs/spec/features/work_packages/move_via_menu_spec.rb | Switches to new batch-aware menu action helper for positional actions. |
| modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb | Uses unified menu-opening helper for right-click and Shift+F10. |
| modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb | New feature spec ensuring consistent batch projection across all menu entry points. |
| modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb | Verifies stable ActionMenu groups/headings and fixed-work-package behavior. |
| modules/backlogs/config/locales/js-en.yml | Adds pluralized JS translation for “selected work packages” heading. |
| modules/backlogs/config/locales/en.yml | Adds “This work package” translation for the menu group heading. |
| modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb | Extends batch move synchronization: ordered lifecycle locks + placement serialization + tighter cohort validation. |
| modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb | Renders two Primer ActionMenu groups with hidden headings and moves batch actions into the batch group. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts | Removes now-unused move-collapse hook as action scope settling moves to menu preparation. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts | Adds coverage for fixed invoker behavior without disturbing selection anchors. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts | Updates root stub to include the new prepareActionMenu port. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts | Updates root stub to include the new prepareActionMenu port. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts | Adds pre-open listeners, projects grouped menu visibility/headings, and updates availability projection to use prepared scope. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Adds extensive tests for pre-open sequencing and grouped heading/group projection. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts | Updates SortableListsRoot port documentation and adds prepareActionMenu to the interface. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts | Implements prepareActionMenu by delegating to selection settling (selectForAction). |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Adds tests for action menu invocation scope behavior and idempotent duplicate delivery. |
| frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts | Adds tests ensuring beforeOpen dispatch order, cancelability, and cancellation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9062c50 to
b01e410
Compare
b01e410 to
8c9c0ad
Compare
967002f to
ac54117
Compare
4ae962a to
249c17e
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. |
249c17e to
1f2c6d0
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. |
446b4a8 to
3b7852b
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. |
2576bba to
e994e8c
Compare
e994e8c to
f8c47f1
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. |
26d44ad to
8d11d33
Compare
8d11d33 to
0503d7b
Compare
0503d7b to
94a4008
Compare
Every sortable-lists consumer offers the same four directional moves, but each surface hand-maintains its own labels, icons and direction values. Introduces one descriptor so the vocabulary has a single home and Backlogs renders from it. The vocabulary carries the acts_as_list `move_to` value alongside the relative direction, so the surfaces that still persist server-side can adopt the table before they move onto the `prev_id` wire. https://community.openproject.org/wp/DREAM-775
Resolves each action menu scope before presentation so contextual and More-button invocation share one synchronous selection policy. The item projects availability from that settled scope while retaining non-mutating late refreshes for deferred menu content. https://community.openproject.org/wp/AGILE-364
Locks all card-menu entry points to one observable action-scope contract so presenter changes cannot silently collapse or retarget selected batches. https://community.openproject.org/wp/AGILE-364
Projects the settled action scope before exposing deferred batch destinations and position controls, so a fixed invoker cannot present stale batch actions the server would refuse. https://community.openproject.org/wp/AGILE-364
Presents either the invoker's own actions or the batch actions, never both: UI/UX review rejected the sectioned menu that showed headings over both groups at once. A group toggle replaces the headings, keyed to how many actions the menu presents rather than how many are executable, so disable-mode consumers keep their items on screen. The batch context moves into the invoker's tooltip, whose text is the button's accessible name through Primer's aria-labelledby wiring. The rename applies only while the menu is open and reverts on close; the singular name arrives as a server-rendered value, since the tooltip's own text is no restore source once a Turbo snapshot captures it mid-rename, and connecting restores it over such a snapshot. Reopening an already-loaded menu needs its own focus correction: Primer's open-time pick checks [hidden] one level deep, so it still matches the first singular item under a hidden group and focusing it no-ops. Focus also parks on a presented batch item before its former group hides, because Primer dismisses the menu the moment focus leaves it. The shared dismiss helper closes menus with Escape: the compact batch menu can leave a live menuitem under the overlay's centre point, so the old outside click activated an action instead of dismissing. https://community.openproject.org/wp/AGILE-364
Keeps the inherited selection-persistence scenario on direct card navigation now that opening an unselected action menu intentionally settles a new one-card action scope. https://community.openproject.org/wp/AGILE-364
Primer's toggle lifecycle, group and divider visibility, menu naming and open-time focus repair had grown into a second state machine inside the controller that owns sortable behaviour. A plain collaborator wrapping the ActionMenu element keeps each of them answerable on its own. Naming moves with it, and gains the menu itself: Primer labels the list by its invoker button, whose own name comes from a further aria-labelledby, and those references do not chain. https://community.openproject.org/wp/AGILE-364
The menu took its batch name from an I18n key and its singular name from a literal string the component read out of another key, so the two scopes were named by different mechanisms and could drift apart. One key with plural forms names both: `one` is the menu of a single card, which is also what a batch of one presents. https://community.openproject.org/wp/AGILE-364
Escape dismissal races the dialog a menu item is still loading. The modal guard reads the DOM before showModal, so the key arrives after it and closes the dialog, which the dialog stream action then removes from the DOM. The spec sees a menu action that opened nothing. Primer already hides the menu when an item is activated, so the click path only waits for that instead of forcing the key. Menu inspection keeps the Escape dismissal, and so does the one invocation whose response morphs the card list out from under the open menu. Reproduced by holding the dismissal until the dialog opens, which fails every run before this change and passes after. https://community.openproject.org/wp/AGILE-364
94a4008 to
d65431a
Compare
Note
This PR is part of a stack. Please review and merge in this order: #24525 → #24778 → #24779 → #24780 → this PR.
Ticket
https://community.openproject.org/wp/AGILE-364
What are you trying to accomplish?
Make every Backlogs ActionMenu entry point settle and present the same action scope, and present either the invoker's own actions or the batch actions — never both at once, per UI/UX review.
The change:
aria-labelledbywiring; the name reverts when the menu closesSortableLists::MoveMenuItemsdescriptor (DREAM-775), which Backlogs now renders throughScreenshots
What approach did you choose and why?
The Backlogs item controller listens at the two established pre-open boundaries: contextual
beforeOpenand the menu popover's capture-phasebeforetoggle. Both settle the scope through the root'sselectForAction; duplicate delivery is idempotent, and the late include-fragment refresh remains non-mutating and scoped to the card menu's own popover.The group toggle is keyed to how many actions the menu presents, not how many are executable: with
hideUnavailableoff a consumer keeps its items on screen disabled, and the group holding them must not disappear. Before the singular group hides, focus parks on a presented batch item — Primer dismisses the menu the moment focus leaves it.The batch name is a separate, complete, plural-aware string rather than the singular name plus a suffix, so other locales keep their own preposition and word order. The controller reads the key from a Stimulus value on the row and carries no Backlogs vocabulary; rewriting the tooltip's text moves the visible label and the accessible name together, avoiding a WCAG 2.5.3 divergence.
Merge checklist