Skip to content

Add React component test infra + regression test for #228#338

Merged
TrevorBurnham merged 2 commits into
jameskerr:mainfrom
TrevorBurnham:test/imperative-update-regression
May 11, 2026
Merged

Add React component test infra + regression test for #228#338
TrevorBurnham merged 2 commits into
jameskerr:mainfrom
TrevorBurnham:test/imperative-update-regression

Conversation

@TrevorBurnham
Copy link
Copy Markdown
Collaborator

Follow-up to #337. That PR ships the fix without a regression test because the repo only had a node-env Jest setup with one TreeApi unit test — reproducing the bug needed real React rendering. This PR adds the infra and the test.

Depends on #337. This branch is based on the fix branch; the test fails without it. If #337 lands first, this rebases cleanly onto main.

Changes

  • Test infra
    • jest-environment-jsdom@^29.7.0 (paired with the existing jest@29)
    • @testing-library/react@^14 + @testing-library/dom@^9 (React 18-compatible majors)
    • react@^18 and react-dom@^18 as devDependencies (already declared as peers; needed locally to render under test)
    • jest.config.jstestEnvironment: "jsdom"
  • Test: src/components/provider.test.tsx — renders <Tree> with openByDefault={false}, calls api.update({ ...api.props, rowHeight: 48 }) via the imperative ref, opens a node, asserts api.rowHeight === 48.

Verified

  • Reverted the provider fix locally → test fails with Expected: 48, Received: 24.
  • Restored the fix → both tests pass.

Note on the test data shape

I initially tried closing an open-by-default root to trigger the state.nodes.open change. That surfaced a separate rendering issue: when the visible-row count shrinks, RowContainer is briefly asked for a stale higher index and throws ''Could not find node for index: N''. In jsdom this escapes to the test runner; in a real browser React likely swallows it via the recoverable-error path. Worth investigating separately, but not in scope here. The current test uses openByDefault={false} + api.open() so the visible-row count only grows, sidestepping that path while still exercising the state.nodes.open dependency.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 10, 2026 21:47
Copy link
Copy Markdown

Copilot AI left a comment

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 adds React component test infrastructure (jsdom + Testing Library) to the react-arborist module and introduces a regression test that ensures imperative tree.update() props are preserved across node open/close state changes (the #228 regression addressed in the dependent fix branch / #337).

Changes:

  • Switch Jest to run in a jsdom environment for component rendering tests.
  • Add React Testing Library + jsdom dependencies needed to render <Tree> in tests.
  • Add provider.test.tsx regression test covering api.update(...rowHeight) surviving api.open(...).

Reviewed changes

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

Show a summary per file
File Description
yarn.lock Locks newly added test/runtime packages (jsdom, Testing Library, React dev deps).
modules/react-arborist/src/components/provider.tsx Splits update logic so open-state changes rebuild visible nodes without clobbering imperatively updated props.
modules/react-arborist/src/components/provider.test.tsx Adds regression test rendering <Tree> and asserting rowHeight survives a node toggle.
modules/react-arborist/package.json Adds test dependencies (but currently drops React peer deps).
modules/react-arborist/jest.config.js Sets Jest testEnvironment to jsdom.
Comments suppressed due to low confidence (1)

modules/react-arborist/package.json:63

  • peerDependencies for react/react-dom were removed. Since this package imports React at runtime, those should remain declared as peers so consumers get version compatibility warnings (and to avoid accidental bundling/duplicate React installs). Keep react/react-dom as peerDependencies (and optionally also as devDependencies for tests), rather than dropping the peer deps entirely.
  "dependencies": {
    "react-dnd": "^14.0.3",
    "react-dnd-html5-backend": "^14.0.3",
    "react-window": "^1.8.11",
    "redux": "^5.0.0",
    "use-sync-external-store": "^1.2.0"
  },
  "devDependencies": {
    "@testing-library/dom": "^9.3.0",
    "@testing-library/react": "^14.0.0",
    "@types/jest": "^29.5.11",
    "@types/react": "^18.2.43",
    "@types/react-dom": "^18.2.0",
    "@types/react-window": "^1.8.8",
    "@types/use-sync-external-store": "^0.0.6",
    "jest": "^29.7.0",
    "jest-environment-jsdom": "^29.7.0",
    "npm-run-all": "^4.1.5",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "rimraf": "^5.0.5",
    "ts-jest": "^29.1.1",
    "typescript": "^5.6.0"
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 55 to +66
/* Make sure the tree instance stays in sync */
const updateCount = useRef(0);
useMemo(() => {
updateCount.current += 1;
api.update(treeProps);
}, [...Object.values(treeProps), state.nodes.open]);
}, [...Object.values(treeProps)]);

/* Rebuild visible nodes when open state changes, without clobbering
props set imperatively via api.update(). */
useMemo(() => {
api.update(api.props);
}, [state.nodes.open]);
@TrevorBurnham TrevorBurnham force-pushed the test/imperative-update-regression branch from b09dd93 to 5217087 Compare May 11, 2026 01:45
@TrevorBurnham
Copy link
Copy Markdown
Collaborator Author

Force-pushed two updates:

  1. Restored react/react-dom peerDependencies (per Copilot's review). They got dropped when yarn 4's `remove` ran during devDep setup; re-added them in a separate commit.
  2. Rebased onto the corrected Preserve imperative tree.update() props across node toggles #337, which now properly bumps `updateCount` on open-state changes (the gmail-demo regression). See Preserve imperative tree.update() props across node toggles #337 for details.

TrevorBurnham and others added 2 commits May 10, 2026 22:18
Adds jest-environment-jsdom, @testing-library/react, and a React
dev dependency for testing. Switches the jest testEnvironment to
jsdom and adds a regression test that renders <Tree>, calls
api.update() via the imperative ref to change rowHeight, then
opens a node — verifying the imperative prop survives the
resulting state.nodes.open change.

The test fails on main (without the jameskerr#337 fix) and passes with it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
yarn 4's remove dropped them when reinstalling react-dom as a
devDependency. Consumers still need the peer declaration for
version compatibility checks and to avoid duplicate React installs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@TrevorBurnham TrevorBurnham force-pushed the test/imperative-update-regression branch from 5217087 to deb8ab2 Compare May 11, 2026 02:18
@TrevorBurnham TrevorBurnham merged commit bc19735 into jameskerr:main May 11, 2026
1 check passed
@TrevorBurnham TrevorBurnham deleted the test/imperative-update-regression branch May 11, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants