-
Notifications
You must be signed in to change notification settings - Fork 647
perf(TreeView): Cache tree items in typeahead for better INP #7334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
996e3c4
perf(TreeView): Cache tree items in typeahead for better INP
mattcosta7 60d291c
update for 0 vs null
mattcosta7 9df1de5
Merge branch 'main' into perf/treeview-typeahead-cache
mattcosta7 9b3a5b3
test(TreeView): add tests for useTreeItemCache hook
mattcosta7 e6ff1b6
Merge branch 'main' into perf/treeview-typeahead-cache
mattcosta7 0456ccd
Merge branch 'main' into perf/treeview-typeahead-cache
siddharthkp c7887ff
use .ts
mattcosta7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| '@primer/react': patch | ||
| --- | ||
|
|
||
| perf(TreeView): Cache tree items in typeahead for better INP | ||
|
|
||
| - Add useTreeItemCache hook to cache DOM queries for tree items | ||
| - Update useRovingTabIndex and useTypeahead to use cached items | ||
| - Add documentation for acceptable :has() selector usage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| import {act, renderHook} from '@testing-library/react' | ||
| import {describe, it, expect, beforeEach, afterEach} from 'vitest' | ||
| import {useTreeItemCache} from './useTreeItemCache' | ||
|
|
||
| describe('useTreeItemCache', () => { | ||
| let container: HTMLDivElement | ||
|
|
||
| beforeEach(() => { | ||
| container = document.createElement('div') | ||
| container.setAttribute('role', 'tree') | ||
| document.body.appendChild(container) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| document.body.removeChild(container) | ||
| }) | ||
|
|
||
| function createTreeItem(id: string): HTMLDivElement { | ||
| const item = document.createElement('div') | ||
| item.setAttribute('role', 'treeitem') | ||
| item.setAttribute('id', id) | ||
| return item | ||
| } | ||
|
|
||
| it('returns tree items from the container', () => { | ||
| container.appendChild(createTreeItem('item-1')) | ||
| container.appendChild(createTreeItem('item-2')) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items = result.current.getTreeItems() | ||
| expect(items).toHaveLength(2) | ||
| expect(items[0].id).toBe('item-1') | ||
| expect(items[1].id).toBe('item-2') | ||
| }) | ||
|
|
||
| it('returns empty array when container is null', () => { | ||
| const containerRef = {current: null} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items = result.current.getTreeItems() | ||
| expect(items).toHaveLength(0) | ||
| }) | ||
|
|
||
| it('returns empty array for empty tree and caches it', () => { | ||
| // Container has no tree items | ||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| // First call should return empty array | ||
| const items1 = result.current.getTreeItems() | ||
| expect(items1).toHaveLength(0) | ||
|
|
||
| // Add an item - but cache should still be empty until invalidated | ||
| // Note: In real usage, MutationObserver would catch this, but we're testing | ||
| // the cache logic directly here | ||
| const item = createTreeItem('item-1') | ||
| container.appendChild(item) | ||
|
|
||
| // Need to wait for MutationObserver to fire | ||
| return new Promise<void>(resolve => { | ||
| setTimeout(() => { | ||
| // Now the cache should be invalidated and return the new item | ||
| const items2 = result.current.getTreeItems() | ||
| expect(items2).toHaveLength(1) | ||
| resolve() | ||
| }, 0) | ||
| }) | ||
| }) | ||
|
|
||
| it('caches results on subsequent calls', () => { | ||
| container.appendChild(createTreeItem('item-1')) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items1 = result.current.getTreeItems() | ||
| const items2 = result.current.getTreeItems() | ||
|
|
||
| // Should be the same array reference (cached) | ||
| expect(items1).toBe(items2) | ||
| }) | ||
|
|
||
| it('invalidates cache on structural changes (childList)', async () => { | ||
| container.appendChild(createTreeItem('item-1')) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items1 = result.current.getTreeItems() | ||
| expect(items1).toHaveLength(1) | ||
|
|
||
| // Add a new item | ||
| await act(async () => { | ||
| container.appendChild(createTreeItem('item-2')) | ||
| // Wait for MutationObserver | ||
| await new Promise(resolve => setTimeout(resolve, 0)) | ||
| }) | ||
|
|
||
| const items2 = result.current.getTreeItems() | ||
| expect(items2).toHaveLength(2) | ||
| // Should be a different array reference (cache was invalidated) | ||
| expect(items1).not.toBe(items2) | ||
| }) | ||
|
|
||
| it('invalidates cache when role attribute changes', async () => { | ||
| const item = createTreeItem('item-1') | ||
| container.appendChild(item) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items1 = result.current.getTreeItems() | ||
| expect(items1).toHaveLength(1) | ||
|
|
||
| // Change role to something else | ||
| await act(async () => { | ||
| item.setAttribute('role', 'none') | ||
| // Wait for MutationObserver | ||
| await new Promise(resolve => setTimeout(resolve, 0)) | ||
| }) | ||
|
|
||
| const items2 = result.current.getTreeItems() | ||
| expect(items2).toHaveLength(0) | ||
| // Should be a different array reference (cache was invalidated) | ||
| expect(items1).not.toBe(items2) | ||
| }) | ||
|
|
||
| it('does NOT invalidate cache on aria-expanded changes', async () => { | ||
| const item = createTreeItem('item-1') | ||
| item.setAttribute('aria-expanded', 'false') | ||
| container.appendChild(item) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items1 = result.current.getTreeItems() | ||
| expect(items1).toHaveLength(1) | ||
|
|
||
| // Change aria-expanded | ||
| await act(async () => { | ||
| item.setAttribute('aria-expanded', 'true') | ||
| // Wait for any potential MutationObserver | ||
| await new Promise(resolve => setTimeout(resolve, 0)) | ||
| }) | ||
|
|
||
| const items2 = result.current.getTreeItems() | ||
| // Should be the SAME array reference (cache was NOT invalidated) | ||
| expect(items1).toBe(items2) | ||
| }) | ||
|
|
||
| it('handles nested tree items', () => { | ||
| const parent = createTreeItem('parent') | ||
| const subtree = document.createElement('div') | ||
| subtree.setAttribute('role', 'group') | ||
| const child = createTreeItem('child') | ||
| subtree.appendChild(child) | ||
| parent.appendChild(subtree) | ||
| container.appendChild(parent) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items = result.current.getTreeItems() | ||
| expect(items).toHaveLength(2) | ||
| expect(items[0].id).toBe('parent') | ||
| expect(items[1].id).toBe('child') | ||
| }) | ||
|
|
||
| it('clears cache when effect runs', () => { | ||
| container.appendChild(createTreeItem('item-1')) | ||
|
|
||
| const containerRef = {current: container} | ||
| const {result, rerender} = renderHook(() => useTreeItemCache(containerRef)) | ||
|
|
||
| const items1 = result.current.getTreeItems() | ||
| expect(items1).toHaveLength(1) | ||
|
|
||
| // Force effect to re-run by rerendering | ||
| rerender() | ||
|
|
||
| // Cache should be cleared, so next call rebuilds it | ||
| const items2 = result.current.getTreeItems() | ||
| // Items should be equal in content but may or may not be same reference | ||
| // depending on timing - the key thing is the data is correct | ||
| expect(items2).toHaveLength(1) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import React from 'react' | ||
|
|
||
| /** | ||
| * A hook that caches tree items to avoid expensive querySelectorAll calls on every keypress. | ||
| * The cache is invalidated when the tree structure changes (via MutationObserver). | ||
| * | ||
| * PERFORMANCE: This is critical for INP because querySelectorAll('[role="treeitem"]') | ||
| * on large trees can take 10-50ms, which directly blocks user input response. | ||
| * | ||
| * Note: useRovingTabIndex also uses querySelectorAll for Home/End/PageUp/PageDown navigation, | ||
| * but those are infrequent single keypresses. Typeahead fires on every character typed, | ||
| * making it the priority optimization target. | ||
| */ | ||
| export function useTreeItemCache(containerRef: React.RefObject<HTMLElement>) { | ||
| // Use null to distinguish "not cached" from "cached as empty array" | ||
| const cacheRef = React.useRef<HTMLElement[] | null>(null) | ||
|
|
||
| // Invalidate cache when tree structure changes | ||
| React.useEffect(() => { | ||
| const container = containerRef.current | ||
| if (!container) return | ||
|
|
||
| // Watch for structural changes (items added/removed) | ||
| // Note: We only watch childList changes, not aria-expanded. | ||
| // aria-expanded changes don't affect which items exist, only their visibility. | ||
| // The caller (useTypeahead, useRovingTabIndex) handles visibility filtering. | ||
| const observer = new MutationObserver(mutations => { | ||
| // Only invalidate on structural changes (childList) or role attribute changes | ||
| // Ignore aria-expanded changes as they don't affect the set of treeitems | ||
| const hasStructuralChange = mutations.some( | ||
| mutation => | ||
| mutation.type === 'childList' || (mutation.type === 'attributes' && mutation.attributeName === 'role'), | ||
| ) | ||
mattcosta7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (hasStructuralChange) { | ||
| cacheRef.current = null | ||
| } | ||
| }) | ||
| observer.observe(container, { | ||
| childList: true, | ||
| subtree: true, | ||
| attributes: true, | ||
| attributeFilter: ['role'], | ||
| }) | ||
|
|
||
| // Clear cache on mount to ensure fresh query | ||
| cacheRef.current = null | ||
|
|
||
| return () => observer.disconnect() | ||
| }, [containerRef]) | ||
|
|
||
| const getTreeItems = React.useCallback((): HTMLElement[] => { | ||
| const container = containerRef.current | ||
| if (!container) return [] | ||
|
|
||
| // Return cached items if valid (null means not cached, [] means cached as empty) | ||
| if (cacheRef.current !== null) { | ||
| return cacheRef.current | ||
| } | ||
|
|
||
| // Rebuild cache | ||
| cacheRef.current = Array.from(container.querySelectorAll('[role="treeitem"]')) as HTMLElement[] | ||
| return cacheRef.current | ||
| }, [containerRef]) | ||
|
|
||
| return {getTreeItems} | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function still uses direct DOM queries instead of the new
useTreeItemCachehook. For consistency with the performance improvements inuseTypeahead, consider using the cached tree items here as well to avoid redundant queries.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we aren't doing this because the cost isn't worth it for these interactions, which need to do this anyway - but we might revisit that if we need to.