Skip to content

Revert "perf(TreeView): replace O(n) TreeWalker with O(depth) sibling traversal"#7659

Merged
francinelucca merged 1 commit intomainfrom
revert-7544-hectahertz/perf-treeview-sibling-traversal
Mar 13, 2026
Merged

Revert "perf(TreeView): replace O(n) TreeWalker with O(depth) sibling traversal"#7659
francinelucca merged 1 commit intomainfrom
revert-7544-hectahertz/perf-treeview-sibling-traversal

Conversation

@francinelucca
Copy link
Copy Markdown
Member

Reverts #7544

Found failing checks in integration testing, reverting

@francinelucca francinelucca requested a review from a team as a code owner March 13, 2026 01:55
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 13, 2026

⚠️ No Changeset found

Latest commit: 5414e86

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added the staff Author is a staff member label Mar 13, 2026
@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Mar 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 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. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

Copy link
Copy Markdown
Contributor

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 reverts PR #7544, which replaced the TreeWalker-based O(n) navigation in getVisibleElement with an O(depth) sibling traversal approach. The revert was triggered by failing integration tests discovered after the original PR was merged.

Changes:

  • Restores the original TreeWalker-based implementation of getVisibleElement in useRovingTabIndex.ts, removing all the helper functions (getNextVisibleElement, getPreviousVisibleElement, getDeepestLastDescendant, etc.) that were introduced in #7544.
  • Removes the test asserting that collapsed subtree children are not rendered in the DOM, which was added alongside the sibling traversal approach.
  • Deletes the changeset file for the reverted change.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/react/src/TreeView/useRovingTabIndex.ts Reverts getVisibleElement back to the TreeWalker-based implementation
packages/react/src/TreeView/TreeView.test.tsx Removes the test for collapsed subtree DOM rendering added in #7544
.changeset/treeview-sibling-traversal.md Deletes the changeset for the reverted change

You can also share your feedback on Copilot code review. Take the survey.

@primer-integration
Copy link
Copy Markdown

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/15939

@primer-integration
Copy link
Copy Markdown

Integration test results from github/github-ui:

Passed  CI   Passed
Passed  VRT   Passed
Passed  Projects   Passed

All checks passed!

@francinelucca francinelucca disabled auto-merge March 13, 2026 13:57
@francinelucca francinelucca merged commit 64bccdb into main Mar 13, 2026
65 checks passed
@francinelucca francinelucca deleted the revert-7544-hectahertz/perf-treeview-sibling-traversal branch March 13, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm integration-tests: skipped manually Changes in this PR do not require an integration test staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants