DevTools: Allow searching by component name in Profiler trace #331
Conversation
Implements search functionality for components in Profiler trace as requested in #32995. Features: - Adds search input that appears when a commit is selected - Supports text and regex search (e.g., /^Button/) - Shows result count and allows navigation between matches - Only searches components in the selected commit - Follows same pattern as Components panel search - Clears search when commit changes - Cmd+f / Ctrl+f keyboard shortcut support The search input appears in the Profiler toolbar between the root selector and settings button, only when a commit is selected in flame-chart or ranked-chart views.
Greptile OverviewGreptile SummaryThis PR implements component name search functionality for the Profiler's Flamegraph and Ranked views, mirroring the pattern used in the Components panel. The implementation adds search state management to ProfilerContext, creates a new ProfilerSearchInput component, and integrates it into the Profiler toolbar. Key Implementation DetailsArchitecture: The search functionality follows the same architectural pattern as ComponentSearchInput, with appropriate adaptations for the Profiler's context-based state management (using direct callbacks instead of a dispatcher pattern). The search operates exclusively on the selected commit's component tree. Search Behavior: When a user enters a search term, the implementation recursively traverses the commit tree, tests each component's display name against a regex pattern (supporting both text and State Management: Search state (text, results, and current index) is properly integrated into ProfilerContext and included in the context value's useMemo dependencies. A separate effect clears the search when the commit changes, which aligns with the stated requirement. Critical Issue FoundThere is a race condition between two useEffect hooks in ProfilerContext.js (lines 331-385):
This results in a fiber being selected that doesn't match any search criteria after commit navigation, creating a confusing UX. Recommended fix: Change the commit-change effect from Overall AssessmentThe implementation is well-structured and follows established codebase patterns. The core search logic is sound - proper tree traversal, regex handling, result navigation, and state synchronization. The integration is clean and appropriately conditional. However, the race condition bug needs to be addressed before merging, as it affects the user experience when navigating between commits with an active search. Confidence Score: 3/5
Important Files ChangedFile Analysis
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Update search results when search text, commit, or root changes | ||
| useEffect(() => { | ||
| if ( | ||
| searchText === '' || | ||
| selectedCommitIndex === null || | ||
| rootID === null || | ||
| !profilingData | ||
| ) { | ||
| setSearchResults([]); | ||
| setSearchIndex(null); | ||
| return; | ||
| } | ||
|
|
||
| const commitTree = profilerStore.profilingCache.getCommitTree({ | ||
| commitIndex: selectedCommitIndex, | ||
| rootID, | ||
| }); | ||
|
|
||
| const results = performSearch(searchText, commitTree); | ||
| setSearchResults(results); | ||
|
|
||
| // Set initial search index | ||
| if (results.length > 0) { | ||
| // If current selected fiber matches, find its index | ||
| if (selectedFiberID !== null && results.includes(selectedFiberID)) { | ||
| setSearchIndex(results.indexOf(selectedFiberID)); | ||
| } else { | ||
| setSearchIndex(0); | ||
| // Auto-select first result | ||
| const firstResultID = results[0]; | ||
| const node = commitTree.nodes.get(firstResultID); | ||
| if (node) { | ||
| selectFiber(firstResultID, node.displayName || 'Anonymous'); | ||
| } | ||
| } | ||
| } else { | ||
| setSearchIndex(null); | ||
| } | ||
| }, [ | ||
| searchText, | ||
| selectedCommitIndex, | ||
| rootID, | ||
| profilingData, | ||
| profilerStore, | ||
| performSearch, | ||
| selectedFiberID, | ||
| selectFiber, | ||
| ]); | ||
|
|
||
| // Clear search when commit changes | ||
| useEffect(() => { | ||
| setSearchTextState(''); | ||
| setSearchResults([]); | ||
| setSearchIndex(null); | ||
| }, [selectedCommitIndex]); |
There was a problem hiding this comment.
There's a race condition between the search effect and the commit-change effect. When selectedCommitIndex changes, both effects run, but the search effect runs first (lines 332-378) and sees the OLD searchText value combined with the NEW selectedCommitIndex. This causes:
- Search effect performs a search on the new commit using the old search text
- First matching result gets auto-selected (line 363)
- Then the commit-change effect (lines 381-385) clears the search state
- Result: A fiber is selected that doesn't match any search criteria
To fix this, the commit-change effect should use useLayoutEffect instead of useEffect to ensure it runs before the search effect on subsequent renders, or the search effect should check if it's running due to a commit change and skip the auto-selection in that case.
Example fix for the commit-change effect:
| // Update search results when search text, commit, or root changes | |
| useEffect(() => { | |
| if ( | |
| searchText === '' || | |
| selectedCommitIndex === null || | |
| rootID === null || | |
| !profilingData | |
| ) { | |
| setSearchResults([]); | |
| setSearchIndex(null); | |
| return; | |
| } | |
| const commitTree = profilerStore.profilingCache.getCommitTree({ | |
| commitIndex: selectedCommitIndex, | |
| rootID, | |
| }); | |
| const results = performSearch(searchText, commitTree); | |
| setSearchResults(results); | |
| // Set initial search index | |
| if (results.length > 0) { | |
| // If current selected fiber matches, find its index | |
| if (selectedFiberID !== null && results.includes(selectedFiberID)) { | |
| setSearchIndex(results.indexOf(selectedFiberID)); | |
| } else { | |
| setSearchIndex(0); | |
| // Auto-select first result | |
| const firstResultID = results[0]; | |
| const node = commitTree.nodes.get(firstResultID); | |
| if (node) { | |
| selectFiber(firstResultID, node.displayName || 'Anonymous'); | |
| } | |
| } | |
| } else { | |
| setSearchIndex(null); | |
| } | |
| }, [ | |
| searchText, | |
| selectedCommitIndex, | |
| rootID, | |
| profilingData, | |
| profilerStore, | |
| performSearch, | |
| selectedFiberID, | |
| selectFiber, | |
| ]); | |
| // Clear search when commit changes | |
| useEffect(() => { | |
| setSearchTextState(''); | |
| setSearchResults([]); | |
| setSearchIndex(null); | |
| }, [selectedCommitIndex]); | |
| // Clear search when commit changes | |
| useLayoutEffect(() => { | |
| setSearchTextState(''); | |
| setSearchResults([]); | |
| setSearchIndex(null); | |
| }, [selectedCommitIndex]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js
Line: 331:385
Comment:
There's a race condition between the search effect and the commit-change effect. When `selectedCommitIndex` changes, both effects run, but the search effect runs first (lines 332-378) and sees the OLD `searchText` value combined with the NEW `selectedCommitIndex`. This causes:
1. Search effect performs a search on the new commit using the old search text
2. First matching result gets auto-selected (line 363)
3. Then the commit-change effect (lines 381-385) clears the search state
4. Result: A fiber is selected that doesn't match any search criteria
To fix this, the commit-change effect should use `useLayoutEffect` instead of `useEffect` to ensure it runs before the search effect on subsequent renders, or the search effect should check if it's running due to a commit change and skip the auto-selection in that case.
Example fix for the commit-change effect:
```suggestion
// Clear search when commit changes
useLayoutEffect(() => {
setSearchTextState('');
setSearchResults([]);
setSearchIndex(null);
}, [selectedCommitIndex]);
```
How can I resolve this? If you propose a fix, please make it concise.
Mirror of facebook/react#35502
Original author: kanikaa88
Summary
Implements component name search functionality in the Profiler trace view as requested in #32995.
Changes
ProfilerContextProfilerSearchInputcomponent (similar toComponentSearchInput)Features
/^Button/)Implementation Details
createRegExputility for pattern matchingTesting
Fixes #32995