[DevTools] Add resizable sidebar to Profiler tab and toolbar compaction#1
[DevTools] Add resizable sidebar to Profiler tab and toolbar compaction#1albert-cgai wants to merge 1 commit intomainfrom
Conversation
Add drag-to-resize columns to the Profiler panel — the sidebar was previously fixed at max 300px and is now user-resizable with persisted state, matching the Components tab behavior. The existing resize logic is extracted into a shared useResizableColumns hook as part of this. Add progressive toolbar compaction for the Profiler — when the left column becomes too narrow, tab labels collapse to icons only, then the root selector and snapshot selector wrap to a second row.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughThis pull request refactors resizable column behavior across DevTools views by extracting inline resize logic from Components.js into a new reusable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/react-devtools-shared/src/devtools/views/Profiler/RootSelector.js (1)
42-45: Let the parent own this spacing.
RootSelectoris now rendered in two different layouts. Keeping the leadingSpacerinside the control makes the wrapped row start with an empty flex item and burns width in compact mode. Consider moving that gap to the parent or making it opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-devtools-shared/src/devtools/views/Profiler/RootSelector.js` around lines 42 - 45, The leading spacer inside RootSelector (styles.Spacer) unnecessarily consumes space in some layouts; remove the hardcoded <div className={styles.Spacer} /> from RootSelector and let the parent component add spacing when needed (or add an optional prop like showLeadingSpacer to RootSelector and conditionally render the spacer based on that prop), updating any usages that rely on the current spacing to either pass the new prop or wrap the <select> with the spacer in the parent; adjust references to rootID and handleChange remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js`:
- Around line 138-170: The effect using useLayoutEffect with toolbarRef and
checkOverflow should invalidate stored compaction thresholds when the toolbar's
children change; update the effect that references expandThresholdsRef,
checkOverflow, compactLevel, and didRecordCommits to also reset or recompute
expandThresholdsRef.current (e.g. set it to an empty array or zeroed values)
whenever the toolbar composition changes, or attach a MutationObserver to
toolbar inside the same effect to clear/refresh expandThresholdsRef.current on
childList mutations before calling checkOverflow (ensure observer is
disconnected in the cleanup); this prevents stale thresholds from keeping the
toolbar unnecessarily compacted when selectors/tabs appear or disappear.
In `@packages/react-devtools-shared/src/devtools/views/TabBar.js`:
- Around line 120-126: When compact mode hides the text label, add an explicit
accessible name so screen readers still get the tab name: on the Icon element in
TabBar.js (the Icon component used with props icon, className), pass an
aria-label prop set to title ?? label (or conditionally add aria-hidden={false}
with aria-label when compact) or alternatively render a visually hidden span
containing {title ?? label} when compact is true; update the Icon invocation to
include aria-label={title ?? label} (or insert the hidden label) so the
icon-only radio retains an accessible name.
In `@packages/react-devtools-shared/src/devtools/views/useResizableColumns.js`:
- Around line 68-85: The effect currently debounces saving
horizontalPercentage/verticalPercentage and cancels the pending timeout on
cleanup, which can lose the final values when unmounting quickly; update the
cleanup (and/or ensure onResizeEnd) to synchronously persist the latest values
instead of simply clearing timeout: inside the useEffect cleanup for
useResizableColumns.js, if timeoutID exists or on unmount (and when
isFirstRenderRef.current is false) call localStorageSetItem(localStorageKey,
JSON.stringify({ horizontalPercentage, verticalPercentage })) before clearing
timeout so the final resize is flushed to storage (or alternatively ensure
onResizeEnd triggers the same immediate write).
- Around line 164-174: The current clamp uses fixed 0.1/0.9 percentages which
can violate the MINIMUM_SIZE pixel constraint on narrow/wide panes; replace the
hard-coded clamps in the block handling
data.horizontalPercentage/data.verticalPercentage by first measuring the wrapper
container size (e.g. get its width/height via the existing wrapper element or
wrapperRef), compute minPercent = MINIMUM_SIZE / wrapperSize and maxPercent = 1
- minPercent, then clamp horizontalPercentage and verticalPercentage to
[minPercent, maxPercent] instead of [0.1,0.9] so persisted percentages always
respect the MINIMUM_SIZE pixel bound.
---
Nitpick comments:
In `@packages/react-devtools-shared/src/devtools/views/Profiler/RootSelector.js`:
- Around line 42-45: The leading spacer inside RootSelector (styles.Spacer)
unnecessarily consumes space in some layouts; remove the hardcoded <div
className={styles.Spacer} /> from RootSelector and let the parent component add
spacing when needed (or add an optional prop like showLeadingSpacer to
RootSelector and conditionally render the spacer based on that prop), updating
any usages that rely on the current spacing to either pass the new prop or wrap
the <select> with the spacer in the parent; adjust references to rootID and
handleChange remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c4ea416-4758-43a8-a09d-f2020a243195
📒 Files selected for processing (12)
packages/react-devtools-shared/src/devtools/views/Components/Components.csspackages/react-devtools-shared/src/devtools/views/Components/Components.jspackages/react-devtools-shared/src/devtools/views/DevTools.csspackages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.csspackages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.csspackages/react-devtools-shared/src/devtools/views/Profiler/Profiler.csspackages/react-devtools-shared/src/devtools/views/Profiler/Profiler.jspackages/react-devtools-shared/src/devtools/views/Profiler/RootSelector.csspackages/react-devtools-shared/src/devtools/views/Profiler/RootSelector.jspackages/react-devtools-shared/src/devtools/views/TabBar.csspackages/react-devtools-shared/src/devtools/views/TabBar.jspackages/react-devtools-shared/src/devtools/views/useResizableColumns.js
| useLayoutEffect(() => { | ||
| const toolbar = toolbarRef.current; | ||
| if (toolbar == null) { | ||
| return; | ||
| } | ||
|
|
||
| const checkOverflow = () => { | ||
| if (toolbar.scrollWidth > toolbar.clientWidth) { | ||
| // Content doesn't fit — remember how wide it was, then compact more. | ||
| if (compactLevel < COMPACT_WRAP) { | ||
| expandThresholdsRef.current[compactLevel] = toolbar.scrollWidth; | ||
| setCompactLevel(compactLevel + 1); | ||
| } | ||
| } else if (compactLevel > COMPACT_NONE) { | ||
| // Content fits — but only expand back if the container is now | ||
| // at least as wide as the content was before we compacted. | ||
| const prevThreshold = expandThresholdsRef.current[compactLevel - 1]; | ||
| if (prevThreshold > 0 && toolbar.clientWidth >= prevThreshold) { | ||
| setCompactLevel(compactLevel - 1); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const observer = new ResizeObserver(checkOverflow); | ||
| observer.observe(toolbar); | ||
|
|
||
| // Check immediately for content changes (e.g. profile data loaded) | ||
| checkOverflow(); | ||
|
|
||
| return () => observer.disconnect(); | ||
| // Re-check when didRecordCommits changes because new elements | ||
| // (e.g. SnapshotSelector) appear and may cause overflow. | ||
| }, [compactLevel, didRecordCommits]); |
There was a problem hiding this comment.
Invalidate the compaction thresholds when the toolbar contents change.
expandThresholdsRef is measured for one toolbar shape, but the children change when the selected tab flips, selectors appear/disappear, or the wrapped row is introduced. With the current [compactLevel, didRecordCommits] effect, those old widths can keep the toolbar stuck more compact than necessary, or miss a needed re-compact, until the container is resized again. Please reset/recompute the thresholds when the toolbar composition changes, or observe child-list mutations in addition to the container resize.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js`
around lines 138 - 170, The effect using useLayoutEffect with toolbarRef and
checkOverflow should invalidate stored compaction thresholds when the toolbar's
children change; update the effect that references expandThresholdsRef,
checkOverflow, compactLevel, and didRecordCommits to also reset or recompute
expandThresholdsRef.current (e.g. set it to an empty array or zeroed values)
whenever the toolbar composition changes, or attach a MutationObserver to
toolbar inside the same effect to clear/refresh expandThresholdsRef.current on
childList mutations before calling checkOverflow (ensure observer is
disconnected in the cleanup); this prevents stale thresholds from keeping the
toolbar unnecessarily compacted when selectors/tabs appear or disappear.
| <Icon | ||
| className={`${ | ||
| disabled ? styles.IconDisabled : '' | ||
| } ${iconSizeClassName}`} | ||
| } ${iconSizeClassName} ${compact ? styles.IconCompact : ''}`} | ||
| type={icon} | ||
| /> | ||
| <span className={tabLabelClassName}>{label}</span> | ||
| {!compact && <span className={tabLabelClassName}>{label}</span>} |
There was a problem hiding this comment.
Keep an accessible name in compact mode.
When compact is true, the text label disappears and this becomes an icon-only radio. Please keep an explicit accessible name here (aria-label={title ?? label} or a visually hidden label), otherwise screen readers lose the tab name.
🛠️ Suggested fix
<input
type="radio"
className={styles.Input}
checked={currentTab === id}
disabled={disabled}
name={groupName}
+ aria-label={compact ? title ?? label : undefined}
value={id}
onChange={onChange}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-devtools-shared/src/devtools/views/TabBar.js` around lines 120
- 126, When compact mode hides the text label, add an explicit accessible name
so screen readers still get the tab name: on the Icon element in TabBar.js (the
Icon component used with props icon, className), pass an aria-label prop set to
title ?? label (or conditionally add aria-hidden={false} with aria-label when
compact) or alternatively render a visually hidden span containing {title ??
label} when compact is true; update the Icon invocation to include
aria-label={title ?? label} (or insert the hidden label) so the icon-only radio
retains an accessible name.
| useEffect(() => { | ||
| if (isFirstRenderRef.current) { | ||
| isFirstRenderRef.current = false; | ||
| return; | ||
| } | ||
|
|
||
| const timeoutID = setTimeout(() => { | ||
| localStorageSetItem( | ||
| localStorageKey, | ||
| JSON.stringify({ | ||
| horizontalPercentage, | ||
| verticalPercentage, | ||
| }), | ||
| ); | ||
| }, 500); | ||
|
|
||
| return () => clearTimeout(timeoutID); | ||
| }, [localStorageKey, horizontalPercentage, verticalPercentage]); |
There was a problem hiding this comment.
Don't drop the last resize when the view unmounts quickly.
This cleanup only cancels the pending write. If the user drags and immediately switches tabs or closes DevTools, the final percentages never reach storage, so the new split is lost. Flush the latest values in onResizeEnd or in an unmount-only cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-devtools-shared/src/devtools/views/useResizableColumns.js`
around lines 68 - 85, The effect currently debounces saving
horizontalPercentage/verticalPercentage and cancels the pending timeout on
cleanup, which can lose the final values when unmounting quickly; update the
cleanup (and/or ensure onResizeEnd) to synchronously persist the latest values
instead of simply clearing timeout: inside the useEffect cleanup for
useResizableColumns.js, if timeoutID exists or on unmount (and when
isFirstRenderRef.current is false) call localStorageSetItem(localStorageKey,
JSON.stringify({ horizontalPercentage, verticalPercentage })) before clearing
timeout so the final resize is flushed to storage (or alternatively ensure
onResizeEnd triggers the same immediate write).
| if (typeof data.horizontalPercentage === 'number') { | ||
| horizontalPercentage = Math.min( | ||
| Math.max(data.horizontalPercentage, 0.1), | ||
| 0.9, | ||
| ); | ||
| } | ||
| if (typeof data.verticalPercentage === 'number') { | ||
| verticalPercentage = Math.min( | ||
| Math.max(data.verticalPercentage, 0.1), | ||
| 0.9, | ||
| ); |
There was a problem hiding this comment.
The restore clamp doesn't match the 100px resize bounds.
Live resizing is limited in pixels via MINIMUM_SIZE, but hydration hard-codes 0.1..0.9. A saved size can therefore reopen larger on wide panes and smaller than 100px on narrow panes, so the persisted splitter no longer round-trips correctly. Clamp after measuring the wrapper, not with fixed percentages here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-devtools-shared/src/devtools/views/useResizableColumns.js`
around lines 164 - 174, The current clamp uses fixed 0.1/0.9 percentages which
can violate the MINIMUM_SIZE pixel constraint on narrow/wide panes; replace the
hard-coded clamps in the block handling
data.horizontalPercentage/data.verticalPercentage by first measuring the wrapper
container size (e.g. get its width/height via the existing wrapper element or
wrapperRef), compute minPercent = MINIMUM_SIZE / wrapperSize and maxPercent = 1
- minPercent, then clamp horizontalPercentage and verticalPercentage to
[minPercent, maxPercent] instead of [0.1,0.9] so persisted percentages always
respect the MINIMUM_SIZE pixel bound.
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| {compactLevel >= COMPACT_WRAP && ( | ||
| <div className={styles.SnapshotSelectorWrapper}> | ||
| <RootSelector /> | ||
| {isLegacyProfilerSelected && didRecordCommits && ( | ||
| <SnapshotSelector /> | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🟡 Empty SnapshotSelectorWrapper bar renders when compacted but no content is needed
When compactLevel >= COMPACT_WRAP, the SnapshotSelectorWrapper div is unconditionally rendered. However, both of its children can be null: RootSelector returns null when there's no profiling data or only one root (RootSelector.js:37-39), and SnapshotSelector is only rendered when isLegacyProfilerSelected && didRecordCommits. This means on the Timeline tab (or when there are no recorded commits) with a single root, an empty div with height: 2.25rem and a bottom border is shown, wasting space in an already-narrow layout.
Scenario that triggers this
- Select the Timeline tab in the Profiler
- Have only one React root (common case)
- Make the devtools narrow enough to trigger COMPACT_WRAP
- An empty bar appears below the toolbar
| {compactLevel >= COMPACT_WRAP && ( | |
| <div className={styles.SnapshotSelectorWrapper}> | |
| <RootSelector /> | |
| {isLegacyProfilerSelected && didRecordCommits && ( | |
| <SnapshotSelector /> | |
| )} | |
| </div> | |
| )} | |
| {compactLevel >= COMPACT_WRAP && | |
| (profilingData?.dataForRoots.size > 1 || | |
| (isLegacyProfilerSelected && didRecordCommits)) && ( | |
| <div className={styles.SnapshotSelectorWrapper}> | |
| <RootSelector /> | |
| {isLegacyProfilerSelected && didRecordCommits && ( | |
| <SnapshotSelector /> | |
| )} | |
| </div> | |
| )} |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| flex-direction: column; | ||
| flex: 1 1 100px; | ||
| max-width: 300px; | ||
| flex: 1 1 35%; |
There was a problem hiding this comment.
🚩 Profiler RightColumn layout change removes max-width constraint
The old .RightColumn CSS had flex: 1 1 100px; max-width: 300px, capping the sidebar at 300px. The new CSS uses flex: 1 1 35% with no max-width. This means the sidebar can now grow beyond 300px when the LeftColumn is narrow (since the sidebar gets the remaining space). This is likely intentional to work with the new resizable columns, but it changes the visual behavior of the sidebar — it will be noticeably wider by default (35% vs up to 300px).
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
Copy of facebook#36202 (by @hyzyla).
useResizableColumnshook.Fixes facebook#22138
How did you test this change?
This is a direct cherry-pick of the original PR commit (980abf2) from facebook/react.
Link to Devin session: https://staging.itsdev.in/sessions/8e8578c8e3cd4656852ffa655b6c5fbe
Requested by: @albert-cgai
Summary by CodeRabbit
New Features
Improvements