[DREAM-959] Change table rows through one mutation protocol by myabc · Pull Request #26507 · opf/openproject

myabc · 2026-10-31T20:14:14Z

Note

This PR is stacked on #26503 (DREAM-959 phase 1). Please review/merge that PR first.

Ticket

https://community.openproject.org/wp/DREAM-959

What are you trying to accomplish?

tl;dr for humans: Every change to table rows now goes through one entry point, so screen-reader state and keyboard focus are updated in the same order every time, and small changes no longer repaint the whole table.

Second phase of DREAM-959: table.mutate(scope, change) captures focus ownership, runs the change, paints the scope, tells keyboard navigation synchronously and bumps a committedRows signal. Producers state what changed; none of them paints, captures or notifies any more. No user-visible change.

What approach did you choose and why?

A small RowMutations module tracks nesting: nested calls widen the outermost scope and settle once, in a finally. The full render keeps its pending-render token and commits through the same exit. The former rowsRefreshed$ stream, notifyRowsRefreshed, reconcileSection and the microtask latch in captureOwnership are gone; keyboard navigation is told directly with rowsChanged.

Name the edge-row keys in one module and give the harness holdFrames() / nextMutation()
RowMutations, table.mutate, committedRows, direct rowsChanged notify; full render, collapse and refreshRows converted
Pin the pending-render token: two redraws in one frame, collapse or refresh while pending, destroy while pending, consumer mutation after commit
Sums row, inline-create and the column header mutate with their own keys
Edit reset repaints the row before focus returns
Drag-handle swap and drag-and-drop staging/rollback mutate; the handle pass runs only for the current draft
Selection repaints only the rows whose selection or focus marker changed
KB ADR draft gains the phase 2 bracket section

Focus ownership is captured on every mutation entry, not only the outermost one: a nested entry adds to the outer capture instead of replacing it, and a render being scheduled counts as pending from its first capture. So if focus enters the table while a change is already running (or an editor overlay is retired by the render that reset it), focus is still restored afterwards instead of landing on the page body. The design review asked for this; the keeps ownership captured…, captures ownership on every entry… and returns to the origin when a full render retires an editor… specs pin it.

Keyed scopes of ledger occurrence keys keep carried row positions; an empty set, an edge-row key or 'all' recomputes positions from the DOM order, so structural changes still withdraw or renumber.

Measured in Chromium with the render benchmark at 1000 rows: one selection toggle drops from 8.5 ms to 5.1 ms. The remaining per-mutation cost is keyboard navigation re-classifying the rows in recover; coalescing that is a follow-up.

Follow-ups kept out of this PR: coalescing navigation's row re-classification per mutation; naming the key vocabulary (OccurrenceKey | EdgeRowKey) and the classifier's edge-key lookup cast, both from the TypeScript audit.

Hints for dev review

Start with wp-fast-table/row-mutations.ts, then WorkPackageTable.mutate / settle / commitRender in wp-fast-table.ts, then rowsChanged and captureOwnership(accumulate) in keyboard-navigation-transformer.ts. The DREAM-886/895 keyboard specs are the regression net for ordering and passed without assertion changes.

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

github-actions · 2026-10-31T21:42:42Z

Warning

Flaky specs

rspec ./spec/features/work_packages/sorting/manual_sorting_spec.rb[1:5:2]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #26507, linked for reference only):

- `rspec ./spec/features/work_packages/sorting/manual_sorting_spec.rb[1:5:2]`

Treat this as a standalone task, unrelated to PR #26507. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26507 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Copilot

🟡 Changes recommended

Active findings identify missing pressed state on first-row activation, lost focus during nested mutation redraws, and stale aria-rowindex values when structural and keyed mutation scopes are combined.

3 open issues

What changed in this PR

This second phase of DREAM-959 builds on #26503 by routing work-package table row changes through one mutation protocol, coordinating repainting, keyboard navigation, and focus restoration.

Changes:

Adds nested mutation handling, shared row keys, and a committedRows signal.
Migrates rendering, selection, editing, inline creation, totals, and drag-and-drop updates to the protocol.
Extends regression coverage for scoped repaints, pending renders, and focus recovery.

File	Description
frontend/src/app/features/work-packages/components/wp-table/wp-table.component.ts	Routes header updates through mutations.
frontend/src/app/features/work-packages/components/wp-table/wp-table.component.spec.ts	Tests header and inline-create mutations.
frontend/src/app/features/work-packages/components/wp-table/wp-table-sums-row/wp-table-sums-row.directive.ts	Migrates totals updates to mutations.
frontend/src/app/features/work-packages/components/wp-table/wp-table-sums-row/wp-table-sums-row.directive.spec.ts	Verifies totals mutation ordering.
frontend/src/app/features/work-packages/components/wp-inline-create/wp-inline-create.component.ts	Wraps inline-create row changes in mutations.
frontend/src/app/features/work-packages/components/wp-inline-create/wp-inline-create.component.spec.ts	Tests focus restoration after reset.
frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts	Centralizes mutation settlement and render commits.
frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.spec.ts	Tests nested mutations and commit signals.
frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table-lifecycle.spec.ts	Covers pending renders and drag-handle timing.
frontend/src/app/features/work-packages/components/wp-fast-table/testing/table-harness.ts	Adds frame control and mutation helpers.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/treegrid-semantics.spec.ts	Observes semantics through mutation completion.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts	Exposes position-withdrawal state.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts	Updates position and refresh regression tests.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/row-state-painter.ts	Preserves positions for occurrence-key scopes.
frontend/src/app/features/work-packages/components/wp-fast-table/semantics/row-classifier.ts	Uses shared edge-row keys.
frontend/src/app/features/work-packages/components/wp-fast-table/row-mutations.ts	Implements nested scope tracking and settlement.
frontend/src/app/features/work-packages/components/wp-fast-table/row-mutations.spec.ts	Tests nesting, exceptions, and ownership capture.
frontend/src/app/features/work-packages/components/wp-fast-table/row-keys.ts	Defines shared row-key constants.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/selection-transformer.ts	Limits repaints to changed selection and focus markers.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/selection-transformer.spec.ts	Tests scoped selection and focus repaints.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/keyboard/navigable-rows.ts	Re-exports shared navigation row keys.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/keyboard/keyboard-navigation-transformer.ts	Receives synchronous mutation notifications.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/keyboard/keyboard-navigation-transformer.spec.ts	Covers Tab stops and pending-render focus recovery.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/keyboard/cell-interaction.spec.ts	Checks repaint ordering before focus returns.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.ts	Wraps staged moves and rollback in mutations.
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.spec.ts	Verifies staging and rollback mutations.
frontend/src/app/features/work-packages/components/wp-fast-table/builders/drag-and-drop/drag-drop-handle-render-pass.ts	Guards current passes and mutates handle replacements.
frontend/src/app/features/work-packages/components/wp-edit-form/table-edit-form.ts	Mutates edited cells before returning focus.

_{Review effort: Balanced}

Moves the edge-row keys (header, totals, inline-create, empty results) into one row-keys module shared by the classifier and the keyboard navigation, so later producers address rows in one vocabulary. Adds a holdFrames verb to the table harness that intercepts the table's frame scheduling and commits held frames on demand, replacing the per-spec requestAnimationFrame and uiWork.frame spies. https://community.openproject.org/wp/DREAM-959

Adds RowMutations, which tracks how deeply row changes are nested and widens the paint scope of the outermost mutation, so nested changes paint once at the outermost exit. The exit runs in a finally block, so a throwing change still paints and releases the mutation. Entry reports whether ownership should accumulate, which is the case while an outer mutation or a pending render is open. https://community.openproject.org/wp/DREAM-959

Adds WorkPackageTable.mutate(scope, change): it captures focus ownership, runs the change, paints the scope, notifies keyboard navigation synchronously and bumps a committedRows signal. Nested calls widen the outermost scope and settle once. The full render commits through the same exit with a full flag, collapse and group toggles mutate the whole table because the owner row's expansion state changes along with its hidden descendants, and refreshRows mutates the work package's occurrence keys. Keyboard navigation is told directly through rowsChanged instead of subscribing to tableRendered and rowsRefreshed$, and captureOwnership takes an accumulate flag from the mutation depth in place of the microtask latch. The sums directive wraps its refresh and clear in a mutation so ownership is captured before the row changes. Keyed scopes of ledger occurrence keys keep carried row positions; empty sets, edge-row keys and full scopes recompute positions from the DOM order, so structural changes still withdraw or renumber. The harness gains nextMutation() for specs that waited on the removed stream. https://community.openproject.org/wp/DREAM-959

Covers the full-render token with specs: only the latest of two redraws in one frame commits, a group collapse or a resource refresh while a render is pending rebuilds the draft so the commit carries current content, a destroyed table drops its token and commits nothing, and a mutation raised by a render consumer settles after the commit as its own mutation. Makes refreshRows update the indexed row object and rebuild a pending draft, and makes destroy clear the token. https://community.openproject.org/wp/DREAM-959

Converts the inline-create component and the column header render to table.mutate with their own row keys, so the create row and the header are painted and keyboard navigation is told after each change without the component calling the painter or capturing ownership. Deletes notifyRowsRefreshed and reconcileSection, whose callers all mutate now. https://community.openproject.org/wp/DREAM-959

Wraps the edit reset's cell refresh in a table mutation, so the row is repainted and keyboard navigation is told before the form returns focus to the cell. Keeps the ownership captured at the start of a full render while the render is being scheduled: the editing reset inside it now mutates, and its own capture must accumulate rather than overwrite, or focus in an editor overlay retired by the render would not come back. https://community.openproject.org/wp/DREAM-959

Wraps the drag-handle swap and the drag-and-drop staging and rollback moves in table mutations. The handle pass runs only while its render pass is current: the draft it started from may still be waiting for its frame, so the guard accepts the pending draft and rejects only a superseded one. Keeps the Tab stop where it is when a mutation settles without a recorded focus position: a staged reorder no longer moves it to the new first row. https://community.openproject.org/wp/DREAM-959

Diffs the selected work package ids and the focused work package against what was painted last, maps the difference through the ledger's occurrence keys and mutates that scope, falling back to the whole table when more than half the rows change (select all). A render resets the baseline because the render path paints every row. Measured in Chromium with the render benchmark at 1000 rows: one selection toggle drops from 8.5 ms to 5.1 ms. The remaining cost is keyboard navigation re-classifying the rows on every mutation. https://community.openproject.org/wp/DREAM-959

Adds `WorkPackageTable#mutateWorkPackages`, which resolves the rendered occurrences of the given work packages itself, and moves `keysOf` from the row-state painter to the occurrence ledger, where the occurrences live. The table edit form and the selection transformer tell the table which work packages changed instead of asking the painter for keys and handing them back. https://community.openproject.org/wp/DREAM-959

github-actions · 2026-11-02T21:15:31Z

Warning

Flaky specs

rspec ./modules/documents/spec/features/documents/project/show_edit_document_spec.rb[1:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #26507, linked for reference only):

- `rspec ./modules/documents/spec/features/documents/project/show_edit_document_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #26507. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #26507 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Oct 31, 2026

myabc added feature javascript Pull requests that update Javascript code needs review labels Oct 31, 2026

myabc added this pull request to stack #26508 October 31, 2026 20:17

myabc marked this pull request as ready for review October 31, 2026 20:27

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-2 branch from ab7762f to bd46e34 Compare October 31, 2026 21:28

myabc mentioned this pull request Oct 31, 2026

[DREAM-959] Clean up the table keyboard modules #26510

Open

8 of 10 tasks

myabc requested a balanced review from Copilot November 1, 2026 00:54

Copilot started reviewing on behalf of myabc November 1, 2026 00:54

View session

Copilot AI reviewed Nov 1, 2026

View reviewed changes

myabc mentioned this pull request Nov 1, 2026

[DREAM-967] Pin the table paths DREAM-874 changed #26512

Open

10 of 12 tasks

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-2 branch 2 times, most recently from 2a14f3c to 8e9b1fc Compare November 1, 2026 13:50

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-2 branch 2 times, most recently from eac83f4 to bd828db Compare November 2, 2026 15:18

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-2 branch from bd828db to bb23b26 Compare November 2, 2026 20:06

myabc mentioned this pull request Nov 2, 2026

[DREAM-959] Paint table row state from one module #26503

Open

7 of 9 tasks

myabc added 8 commits November 2, 2026 20:33

myabc force-pushed the code-maintenance/dream-959-fast-table-cleanup-2 branch from bb23b26 to 91a86da Compare November 2, 2026 21:01

myabc mentioned this pull request Nov 2, 2026

[DREAM-974] Move actions for work packages in manually sorted tables #26569

Draft

6 of 8 tasks

Navigation Menu

Repository navigation

[DREAM-959] Change table rows through one mutation protocol - #26507

[DREAM-959] Change table rows through one mutation protocol#26507
myabc wants to merge 9 commits into
code-maintenance/dream-959-fast-table-cleanup-1from
code-maintenance/dream-959-fast-table-cleanup-2

myabc commented Oct 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

myabc commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Hints for dev review

AI involvement

Merge checklist

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

myabc commented Oct 31, 2026 •

edited

Loading