perf(TreeView): replace O(n) TreeWalker with O(depth) sibling traversal#7544
perf(TreeView): replace O(n) TreeWalker with O(depth) sibling traversal#7544hectahertz wants to merge 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d3ca691 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
There was a problem hiding this comment.
Pull request overview
This PR improves TreeView keyboard navigation performance by replacing TreeWalker-based visible-item lookup with direct DOM sibling/parent traversal, avoiding full-tree scans on every ArrowUp/ArrowDown keypress.
Changes:
- Replaced
getVisibleElementimplementation inuseRovingTabIndex.tswith O(depth) traversal for next/previous visible treeitems. - Updated an ActionList story to keep hook-using components at module scope (React Compiler compatibility).
- Enabled React Compiler processing for ActionList by removing it from the “unsupported” list and added a patch changeset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react/src/TreeView/useRovingTabIndex.ts | Reworks ArrowUp/ArrowDown visible navigation to use sibling/parent traversal instead of TreeWalker. |
| packages/react/src/ActionList/ActionList.features.stories.tsx | Moves SideEffectDescription to module scope to satisfy React Compiler constraints in stories. |
| packages/react/script/react-compiler.mjs | Removes ActionList from the unsupported set so it’s included in React Compiler processing. |
| .changeset/treeview-sibling-traversal.md | Adds a patch changeset entry documenting the TreeView perf improvement. |
liuliu-dev
left a comment
There was a problem hiding this comment.
This looks great! Thanks for the detailed comments which really helped with understanding ❤️
I left some comments but non-blocking.
Also curious, are you considering similar improvements for the O(n) paths in getLastElement , getNextPageElement, and getPreviousPageElement?
a71ef36 to
eb73368
Compare
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
I’m seeing this failure in the integration tests: It’s happening across multiple integration test runs for this PR. Could this be related to this change (since it affects tab/focus behavior)? |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14853 |
|
Integration test results from github/github-ui:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
- Remove ActionList from the React Compiler unsupported list - Fix Rules of Hooks violation in ChildWithSideEffects story by moving SideEffectDescription to module scope
31e823d to
edacd6f
Compare
Closes #
Replaces the O(n)
TreeWalker-based navigation ingetVisibleElementwith O(depth) direct DOM sibling/parent traversal for ArrowUp/ArrowDown keyboard navigation.The previous implementation created a fresh
document.createTreeWalker(root, ...)on every ArrowUp/ArrowDown keystroke, then walked from the first node in the tree to the currently focused element before continuing. At position 25K in a 26K-item tree, this was essentially a full DOM traversal on every keypress.The new implementation walks sibling and parent/child edges directly:
This relies on
TreeView.SubTreeunmounting children when collapsed (return nullwhen!isExpanded), so collapsed subtree children are never in the DOM.Benchmarks (26,000 items, 289K DOM nodes)
Per-operation cost (single call):
Held key simulation (50 rapid ArrowDown from position 12,500):
scrollIntoViewforced reflow (separate issue)The remaining ~7ms per keypress is dominated by
scrollIntoViewforcing synchronous layout reflow on 289K DOM nodes, addressed separately in #7545.Changelog
New
N/A
Changed
getVisibleElementinuseRovingTabIndex.tsnow uses direct DOM sibling/parent traversal instead ofTreeWalker, reducing ArrowUp/ArrowDown navigation from O(n) to O(tree depth)Removed
N/A
Rollout strategy
Testing & Reviewing
Merge checklist