Skip to content

[DREAM-808] Fix TreeView form state for dynamically inserted/removed checked nodes - #515

Open
myabc wants to merge 2 commits into
fix/eslint-component-typescriptfrom
fix/tree-view-observer-ignores-aria-checked
Open

[DREAM-808] Fix TreeView form state for dynamically inserted/removed checked nodes#515
myabc wants to merge 2 commits into
fix/eslint-component-typescriptfrom
fix/tree-view-observer-ignores-aria-checked

Conversation

@myabc

@myabc myabc commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

What are you trying to accomplish?

Keep a TreeView form's hidden inputs in sync when checked tree items are inserted or removed after the component has connected, as happens when an async fragment returns nodes that are already checked, or when a checked subtree is replaced.

Previously, the form observer watched both aria-checked attributes and child-list mutations, but its guard only recognized attribute changes. A checked node inserted by a fragment therefore appeared selected in the tree while being omitted from an immediate form submission; conversely, removing a checked node left a stale hidden input behind that kept submitting the deleted node's value.

Integration

No downstream integration changes are required. Consumers that insert or remove checked TreeView nodes dynamically will now receive the same form payload behavior as nodes present during initial rendering.

In OpenProject, this unblocks replacing the app-side form sync in the types "Projects" tab (see opf/openproject#24757 for OP-19878): its Stimulus controller maintains a parallel MutationObserver and JSON hidden input, which can be dropped in favour of TreeView's native form_arguments: hidden inputs once this fix ships in the gem.

List the issues that this change affects.

Tracked in DREAM-808. Follow-up to #512, where the insertion edge case was identified during review; the removal gap was found in a follow-up code review of this branch.

Risk Assessment

  • Low risk — the change is isolated to TreeView form-input synchronization, is covered by browser regression tests, and is easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

The existing mutation guard now inspects nodes added and removed by childList mutations (via a shared private helper). It rebuilds hidden form inputs only when such a node is itself a checked tree item or contains one.

The guard deliberately does not react to every child-list mutation. Rebuilding the form inputs replaces the hidden-input container's children, which produces additional mutations; filtering for checked tree items prevents that observer from triggering itself recursively.

The observer now also passes attributeOldValue: true, so the pre-existing oldValue comparison in the guard actually filters out same-value aria-checked rewrites (previously m.oldValue was always null, making the check vacuous and rebuilding the inputs on every write).

The seven pre-existing ESLint errors that used to be fixed on this branch now live in their own PR, #520, which this PR is stacked on: its base is fix/eslint-component-typescript, so the diff above shows only the TreeView changes. Review and merge #520 first; this PR then retargets to main automatically. npm run lint is green on every commit of both. This PR adds a changeset.

Regression tests: one clones a real rendered, checked TreeView item into an already-connected tree and verifies the inserted node is present in the submitted payload; the mirror test removes a checked node and verifies its hidden input disappears and the payload omits it.

Anything you want to highlight for special attention from reviewers?

Validation performed:

  • npm run lint — 0 errors
  • npm run build:js
  • Complete TreeView system suite: 86 runs, 305 assertions, 0 failures
  • git diff --check

Accessibility

  • No new axe scan violation — this change does not alter rendered markup, focus behavior, or accessibility semantics; it only synchronizes hidden form inputs with existing checked state.

Merge checklist

  • Added/updated tests
  • Added/updated documentation — not required for this internal bug fix
  • Added/updated previews (Lookbook) — not required; the regressions are covered by system tests
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6b8a6e3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@openproject/primer-view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to fix TreeView hidden form input synchronization when pre-checked nodes are dynamically inserted into the tree (i.e., via DOM childList mutations), ensuring form submission reflects the updated checked state.

Changes:

  • Update the TreeView mutation handling to treat insertion of checked nodes as a meaningful change that triggers hidden input recomputation.
  • Add a system test to cover the regression: inserting a cloned checked node should update the form payload.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
app/components/primer/alpha/tree_view/tree_view.ts Extends the “something changed” detection to react to inserted checked nodes so hidden inputs are recomputed.
test/system/alpha/tree_view_test.rb Adds a system test that inserts a checked node and asserts the form payload includes the new checked path.

Comment thread app/components/primer/alpha/tree_view/tree_view.ts
@myabc
myabc force-pushed the fix/tree-view-observer-ignores-aria-checked branch from 513ac01 to 047ab32 Compare August 14, 2026 16:10
@myabc myabc changed the title Fix TreeView form state updates for inserted checked nodes [DREAM-808] Fix TreeView form state for dynamically inserted/removed checked nodes Aug 14, 2026
@myabc
myabc marked this pull request as ready for review August 14, 2026 16:23
myabc added 2 commits August 14, 2026 18:00
https://community.openproject.org/wp/DREAM-808

Extends the TreeView form observer guard to recognize childList
mutations that add checked tree items. Previously only aria-checked
attribute changes triggered a rebuild of the hidden form inputs, so
checked nodes inserted after connect (e.g. from async fragments)
appeared selected but were omitted from form submission.

Adds a browser regression test cloning a checked node into an
already-connected tree.
https://community.openproject.org/wp/DREAM-808

Extends the childList guard to inspect removed nodes symmetrically
with added ones, so removing a checked tree item also rebuilds the
hidden form inputs instead of leaving a stale input that submits the
deleted node's value. Extracts the shared check into a private
helper.

Also passes attributeOldValue: true to the observer so the existing
oldValue comparison actually filters out same-value aria-checked
rewrites instead of rebuilding the inputs on every write.

Adds a browser regression test removing a checked node from a
connected tree, and a changeset for the insertion and removal fixes.
@myabc
myabc changed the base branch from main to fix/eslint-component-typescript August 14, 2026 17:00
@myabc
myabc force-pushed the fix/tree-view-observer-ignores-aria-checked branch from 047ab32 to 6b8a6e3 Compare August 14, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants