Repository navigation
fix(workspace-cleanup): make the Filters panel scrollable - #14629
Conversation
The height cap sat on the ScrollArea Root, but the Radix viewport inside is h-full — under an indefinite root height that collapses to auto, so the viewport grew to full content height with nothing to scroll while the root clipped at 420px. The last facet groups (Archived, and the tail of Workspace status) were unreachable behind the footer. Move the cap to the viewport, which is what the ScrollArea wrapper's viewportClassName exists for. Adds a regression test that pins the cap to the viewport; it fails against the previous markup.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated the workspace cleanup filter popover to use a capped flex-column layout. The facet list now fills the available space and scrolls without hiding the footer. Added a shared test renderer and assertions for the open popover height and scroll-area layout classes. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
|
Electron QA on Exercised Resource Manager → Clean up workspaces → Filters.
No filter/popover/scroll errors in renderer console or app logs. Desktop-only pass; no product changes, no merge. |
…every filter patch A profile carried `activity.idleMinDays = 20` that the user never set, hiding 253 of 799 workspaces on open. Chromium mutates a *focused* number input on every wheel tick, and before #14629 the facet panel could not scroll, so the natural response -- cursor into the panel, spin the wheel -- walked the threshold up and persisted it. Two fixes: - `FacetNumberField` renders `type="text" inputMode="numeric"`. A wheel cannot mutate a text input, and the parser already takes strings. `preventDefault` on a focused number input would also work, but it blocks the wheel's default action -- which includes scrolling the nearest scrollable ancestor -- and would re-break the panel scrolling #14629 just fixed, in exactly the reported gesture. All five numeric facets share this one field. - `patchFilters` closed over the render's `browse` snapshot, so two patches in one tick dropped one. It now writes through a functional update against the latest store state. `toggleSortField` and `clearFilters` had the same defect. Both tests were confirmed to fail against the unfixed source before being kept.
…every filter patch (#15298) A profile carried `activity.idleMinDays = 20` that the user never set, hiding 253 of 799 workspaces on open. Chromium mutates a *focused* number input on every wheel tick, and before #14629 the facet panel could not scroll, so the natural response -- cursor into the panel, spin the wheel -- walked the threshold up and persisted it. Two fixes: - `FacetNumberField` renders `type="text" inputMode="numeric"`. A wheel cannot mutate a text input, and the parser already takes strings. `preventDefault` on a focused number input would also work, but it blocks the wheel's default action -- which includes scrolling the nearest scrollable ancestor -- and would re-break the panel scrolling #14629 just fixed, in exactly the reported gesture. All five numeric facets share this one field. - `patchFilters` closed over the render's `browse` snapshot, so two patches in one tick dropped one. It now writes through a functional update against the latest store state. `toggleSortField` and `clearFilters` had the same defect. Both tests were confirmed to fail against the unfixed source before being kept.
…e content (#16206) * fix(popover): wheel-scroll a popover whose scroller is nested, not the content The workspace-cleanup Filters panel cannot be scrolled with a wheel. Only the scrollbar drag and focus-scroll work. Two independent Electron QA passes measured it on main: wheel events over that popover arrive defaultPrevented, panel scrollTop 0 -> 0, while the candidate list inside the dialog subtree scrolls normally. Cause: the popover portals outside the Radix dialog subtree, so react-remove-scroll's scroll-lock cancels wheel there. #14629 added a shim for exactly this, but it scrolls `event.currentTarget` -- the PopoverContent -- and only when that element is itself overflowing. The Filters panel is a flex column holding a ScrollArea above a pinned footer, so PopoverContent is overflow-hidden and the real scroller is a descendant. The shim looked at the wrong element and returned early. The shim now resolves the nearest scrollable element between the wheel target and the content, inclusive of both, so it handles the nested-viewport shape as well as the flat one. Still opt-in via `popover-scroll-content`; popovers without the class are untouched. The cleanup Filters panel opts in. This is the unfixed half of the original report. #14629 fixed the panel being clipped; the wheel -- what you actually reach for -- stayed dead. The nested test fails against the shipped shim and the other two stay green, so it reproduces the bug without redefining existing behaviour. * fix(popover): split the wheel-shim opt-in from the scroll-container styling Two fixes on top of the reviewer's round. **The reviewer's change, kept.** `resolvePopoverScroller` no longer falls back to the content element unconditionally; it must match the overflow test like any other candidate. The reviewer flagged that this could break the existing opted-in popovers, so I checked: `.popover-scroll-content` sets `overflow-y: auto` (main.css:601-607), so every one of them still resolves. The test needs an inline style only because happy-dom does not apply the stylesheet -- noted in the test so nobody reads it as a production concern. **A bug of mine the review did not reach.** Opting the cleanup Filters panel in via `popover-scroll-content` would also have applied that class's `max-height: min(15rem, ...)`, crushing the panel's 471px flex column to 240px. The class conflates 'style me as a scroll container' with 'run the wheel shim', and a popover that manages its own layout needs only the second. So the shim now accepts `popover-wheel-scroll` as a styling-free marker, and the Filters panel uses that. `popover-scroll-content` keeps implying it, so no existing caller changes.
…every filter patch (stablyai#15298) A profile carried `activity.idleMinDays = 20` that the user never set, hiding 253 of 799 workspaces on open. Chromium mutates a *focused* number input on every wheel tick, and before stablyai#14629 the facet panel could not scroll, so the natural response -- cursor into the panel, spin the wheel -- walked the threshold up and persisted it. Two fixes: - `FacetNumberField` renders `type="text" inputMode="numeric"`. A wheel cannot mutate a text input, and the parser already takes strings. `preventDefault` on a focused number input would also work, but it blocks the wheel's default action -- which includes scrolling the nearest scrollable ancestor -- and would re-break the panel scrolling stablyai#14629 just fixed, in exactly the reported gesture. All five numeric facets share this one field. - `patchFilters` closed over the render's `browse` snapshot, so two patches in one tick dropped one. It now writes through a functional update against the latest store state. `toggleSortField` and `clearFilters` had the same defect. Both tests were confirmed to fail against the unfixed source before being kept. (cherry picked from commit 4bc9e6b)
…e content (stablyai#16206) * fix(popover): wheel-scroll a popover whose scroller is nested, not the content The workspace-cleanup Filters panel cannot be scrolled with a wheel. Only the scrollbar drag and focus-scroll work. Two independent Electron QA passes measured it on main: wheel events over that popover arrive defaultPrevented, panel scrollTop 0 -> 0, while the candidate list inside the dialog subtree scrolls normally. Cause: the popover portals outside the Radix dialog subtree, so react-remove-scroll's scroll-lock cancels wheel there. stablyai#14629 added a shim for exactly this, but it scrolls `event.currentTarget` -- the PopoverContent -- and only when that element is itself overflowing. The Filters panel is a flex column holding a ScrollArea above a pinned footer, so PopoverContent is overflow-hidden and the real scroller is a descendant. The shim looked at the wrong element and returned early. The shim now resolves the nearest scrollable element between the wheel target and the content, inclusive of both, so it handles the nested-viewport shape as well as the flat one. Still opt-in via `popover-scroll-content`; popovers without the class are untouched. The cleanup Filters panel opts in. This is the unfixed half of the original report. stablyai#14629 fixed the panel being clipped; the wheel -- what you actually reach for -- stayed dead. The nested test fails against the shipped shim and the other two stay green, so it reproduces the bug without redefining existing behaviour. * fix(popover): split the wheel-shim opt-in from the scroll-container styling Two fixes on top of the reviewer's round. **The reviewer's change, kept.** `resolvePopoverScroller` no longer falls back to the content element unconditionally; it must match the overflow test like any other candidate. The reviewer flagged that this could break the existing opted-in popovers, so I checked: `.popover-scroll-content` sets `overflow-y: auto` (main.css:601-607), so every one of them still resolves. The test needs an inline style only because happy-dom does not apply the stylesheet -- noted in the test so nobody reads it as a production concern. **A bug of mine the review did not reach.** Opting the cleanup Filters panel in via `popover-scroll-content` would also have applied that class's `max-height: min(15rem, ...)`, crushing the panel's 471px flex column to 240px. The class conflates 'style me as a scroll container' with 'run the wheel shim', and a popover that manages its own layout needs only the second. So the shim now accepts `popover-wheel-scroll` as a styling-free marker, and the Filters panel uses that. `popover-scroll-content` keeps implying it, so no existing caller changes. (cherry picked from commit b085e67)
…every filter patch (stablyai#15298) A profile carried `activity.idleMinDays = 20` that the user never set, hiding 253 of 799 workspaces on open. Chromium mutates a *focused* number input on every wheel tick, and before stablyai#14629 the facet panel could not scroll, so the natural response -- cursor into the panel, spin the wheel -- walked the threshold up and persisted it. Two fixes: - `FacetNumberField` renders `type="text" inputMode="numeric"`. A wheel cannot mutate a text input, and the parser already takes strings. `preventDefault` on a focused number input would also work, but it blocks the wheel's default action -- which includes scrolling the nearest scrollable ancestor -- and would re-break the panel scrolling stablyai#14629 just fixed, in exactly the reported gesture. All five numeric facets share this one field. - `patchFilters` closed over the render's `browse` snapshot, so two patches in one tick dropped one. It now writes through a functional update against the latest store state. `toggleSortField` and `clearFilters` had the same defect. Both tests were confirmed to fail against the unfixed source before being kept.
…e content (stablyai#16206) * fix(popover): wheel-scroll a popover whose scroller is nested, not the content The workspace-cleanup Filters panel cannot be scrolled with a wheel. Only the scrollbar drag and focus-scroll work. Two independent Electron QA passes measured it on main: wheel events over that popover arrive defaultPrevented, panel scrollTop 0 -> 0, while the candidate list inside the dialog subtree scrolls normally. Cause: the popover portals outside the Radix dialog subtree, so react-remove-scroll's scroll-lock cancels wheel there. stablyai#14629 added a shim for exactly this, but it scrolls `event.currentTarget` -- the PopoverContent -- and only when that element is itself overflowing. The Filters panel is a flex column holding a ScrollArea above a pinned footer, so PopoverContent is overflow-hidden and the real scroller is a descendant. The shim looked at the wrong element and returned early. The shim now resolves the nearest scrollable element between the wheel target and the content, inclusive of both, so it handles the nested-viewport shape as well as the flat one. Still opt-in via `popover-scroll-content`; popovers without the class are untouched. The cleanup Filters panel opts in. This is the unfixed half of the original report. stablyai#14629 fixed the panel being clipped; the wheel -- what you actually reach for -- stayed dead. The nested test fails against the shipped shim and the other two stay green, so it reproduces the bug without redefining existing behaviour. * fix(popover): split the wheel-shim opt-in from the scroll-container styling Two fixes on top of the reviewer's round. **The reviewer's change, kept.** `resolvePopoverScroller` no longer falls back to the content element unconditionally; it must match the overflow test like any other candidate. The reviewer flagged that this could break the existing opted-in popovers, so I checked: `.popover-scroll-content` sets `overflow-y: auto` (main.css:601-607), so every one of them still resolves. The test needs an inline style only because happy-dom does not apply the stylesheet -- noted in the test so nobody reads it as a production concern. **A bug of mine the review did not reach.** Opting the cleanup Filters panel in via `popover-scroll-content` would also have applied that class's `max-height: min(15rem, ...)`, crushing the panel's 471px flex column to 240px. The class conflates 'style me as a scroll container' with 'run the wheel shim', and a popover that manages its own layout needs only the second. So the shim now accepts `popover-wheel-scroll` as a styling-free marker, and the Filters panel uses that. `popover-scroll-content` keeps implying it, so no existing caller changes.
…4629) * fix(workspace-cleanup): make the filter panel scrollable The height cap sat on the ScrollArea Root, but the Radix viewport inside is h-full — under an indefinite root height that collapses to auto, so the viewport grew to full content height with nothing to scroll while the root clipped at 420px. The last facet groups (Archived, and the tail of Workspace status) were unreachable behind the footer. Move the cap to the viewport, which is what the ScrollArea wrapper's viewportClassName exists for. Adds a regression test that pins the cap to the viewport; it fails against the previous markup. * fix(workspace-cleanup): keep filter footer visible
…every filter patch (stablyai#15298) A profile carried `activity.idleMinDays = 20` that the user never set, hiding 253 of 799 workspaces on open. Chromium mutates a *focused* number input on every wheel tick, and before stablyai#14629 the facet panel could not scroll, so the natural response -- cursor into the panel, spin the wheel -- walked the threshold up and persisted it. Two fixes: - `FacetNumberField` renders `type="text" inputMode="numeric"`. A wheel cannot mutate a text input, and the parser already takes strings. `preventDefault` on a focused number input would also work, but it blocks the wheel's default action -- which includes scrolling the nearest scrollable ancestor -- and would re-break the panel scrolling stablyai#14629 just fixed, in exactly the reported gesture. All five numeric facets share this one field. - `patchFilters` closed over the render's `browse` snapshot, so two patches in one tick dropped one. It now writes through a functional update against the latest store state. `toggleSortField` and `clearFilters` had the same defect. Both tests were confirmed to fail against the unfixed source before being kept.
…e content (stablyai#16206) * fix(popover): wheel-scroll a popover whose scroller is nested, not the content The workspace-cleanup Filters panel cannot be scrolled with a wheel. Only the scrollbar drag and focus-scroll work. Two independent Electron QA passes measured it on main: wheel events over that popover arrive defaultPrevented, panel scrollTop 0 -> 0, while the candidate list inside the dialog subtree scrolls normally. Cause: the popover portals outside the Radix dialog subtree, so react-remove-scroll's scroll-lock cancels wheel there. stablyai#14629 added a shim for exactly this, but it scrolls `event.currentTarget` -- the PopoverContent -- and only when that element is itself overflowing. The Filters panel is a flex column holding a ScrollArea above a pinned footer, so PopoverContent is overflow-hidden and the real scroller is a descendant. The shim looked at the wrong element and returned early. The shim now resolves the nearest scrollable element between the wheel target and the content, inclusive of both, so it handles the nested-viewport shape as well as the flat one. Still opt-in via `popover-scroll-content`; popovers without the class are untouched. The cleanup Filters panel opts in. This is the unfixed half of the original report. stablyai#14629 fixed the panel being clipped; the wheel -- what you actually reach for -- stayed dead. The nested test fails against the shipped shim and the other two stay green, so it reproduces the bug without redefining existing behaviour. * fix(popover): split the wheel-shim opt-in from the scroll-container styling Two fixes on top of the reviewer's round. **The reviewer's change, kept.** `resolvePopoverScroller` no longer falls back to the content element unconditionally; it must match the overflow test like any other candidate. The reviewer flagged that this could break the existing opted-in popovers, so I checked: `.popover-scroll-content` sets `overflow-y: auto` (main.css:601-607), so every one of them still resolves. The test needs an inline style only because happy-dom does not apply the stylesheet -- noted in the test so nobody reads it as a production concern. **A bug of mine the review did not reach.** Opting the cleanup Filters panel in via `popover-scroll-content` would also have applied that class's `max-height: min(15rem, ...)`, crushing the panel's 471px flex column to 240px. The class conflates 'style me as a scroll container' with 'run the wheel shim', and a popover that manages its own layout needs only the second. So the shim now accepts `popover-wheel-scroll` as a styling-free marker, and the Filters panel uses that. `popover-scroll-content` keeps implying it, so no existing caller changes.



ELI5
The filter panel in the workspace cleanup dialog was too tall for its box and could not be scrolled, so the bottom filters were stuck behind the footer. This makes the filter list scroll while keeping the footer visible, including in short windows.
What Changed
min-h-0 flex-1body of the popover.Why
The old ScrollArea Root had only
max-heightwhile its Radix viewport usedh-full. Without a definite height, the viewport expanded to the full facet content, so the Root clipped the panel instead of providing a scrollable region.Giving the popover a definite, available-height-aware flex layout lets the facet body shrink and scroll. The 471px normal cap preserves the previous 420px facet area plus its footer, while short windows reduce the scroll body instead of clipping the footer.
Linked Issue
Fixes #14628
Visual Proof
BEFORE — panel clipped at the cap, no scrollbar;
ARCHIVEDcollides with the footer and its controls are unreachable:AFTER — exact-head Electron screenshots are attached in the QA comment below, covering normal height, a 500px-tall renderer, and the final controls after scrolling.
Testing
Verified locally on macOS:
Focused filter-bar Vitest: 2 passed.
pnpm run typecheckclean.Direct oxlint, formatting, React Doctor changed-lines, and
git diff --checkclean.Electron/CDP on exact HEAD
583166d0b29b8f4f4ca37d2a963323fcd2dd9ecf:Manually tested in the rendered Electron app
Automated regression test added
Review
Checklist
Author