fix(chat): render the sidebar filter menu's pickers inline on a phone - #6909
Conversation
The Recent window picker and the dormant-collapse threshold were nested Radix submenus. Radix hardcodes a submenu to side="right" and its popper shifts only on the cross axis, so a submenu never moves along the axis that is short. At phone width neither side fits beside a menu that already spans most of the screen: measured at a 390px viewport the Recent flyout came out 249px wide with 192px of it past the right edge (--radix-popper-available-width: 57px), and flip's bestFit fallback can land it off the LEFT edge instead, which is what was reported. No width or padding tuning recovers that. On a phone the options now render inline inside the one open menu — the Recent presets and custom amount under their row, the dormant-collapse thresholds as chips rather than seven more rows, and the folders of "New chat in folder" under a caption. The menu also gains a viewport width cap: Radix sizes the popper wrapper to max-content, so an inline caption sentence alone took the menu to 510px inside a 390px viewport. A wide viewport keeps the flyout untouched, at the same measured geometry.
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Call-site fix for a primitive-level constraint: three hand-rolled inline branches ship while five submenus — one in this same sidebar — stay broken on phones. Watch
Suggestions
[DESIGN-REVIEWED] e674e9c |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The declared phone-picker fix is sound at cause level, but the patch carries ~10 undeclared changes — at least three provably belong to PRs already merged on the base, so the diff base is stale. What this change shipsIntent: let a phone user actually reach the Recent-window and dormant-collapse pickers, which opened off the screen edge — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] e674e9c |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/pages/ChatSidebar.tsx:5382 -- clicking an active desktop Recent trigger disables the filter, then [GPT-REVIEWED] e674e9c False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — ✅ PASSUX-level review of The PR's actual surface (ChatSidebar inline pickers, select fixes, screenshots) is well-executed: existing i18n keys reused, captions match the FILTER/SORT BY pattern, the picker is gated so an inactive filter can't show a lying control, chips carry UX-Verdict: PASS Inline pickers match the menu's existing caption/chip vocabulary, the gated Recent picker prevents a lying control, and screenshots prove the phone fix without touching desktop. [UX-REVIEWED] e674e9c |
github-actions
Bot
added
readiness: passed
The Recent window picker and the dormant-collapse threshold were nested Radix submenus. Radix hardcodes a submenu to side="right" and its popper shifts only on the cross axis, so a submenu never moves along the axis that is short. At phone width neither side fits beside a menu that already spans most of the screen: measured at a 390px viewport the Recent flyout came out 249px wide with 192px of it past the right edge (--radix-popper-available-width: 57px), and flip's bestFit fallback can land it off the LEFT edge instead, which is what was reported. No width or padding tuning recovers that. On a phone the options now render inline inside the one open menu — the Recent presets and custom amount under their row, the dormant-collapse thresholds as chips rather than seven more rows, and the folders of "New chat in folder" under a caption. The menu also gains a viewport width cap: Radix sizes the popper wrapper to max-content, so an inline caption sentence alone took the menu to 510px inside a 390px viewport. Bringing the picker into view exposed a defect that had been hiding in the off-screen flyout: NativeSelect draws its chevron as an overlay at right-3, and `cn` is tailwind-merge, which cannot split a caller's `px-*` shorthand — so the recency-unit control lost its right gutter and the arrow painted over the word. The gutter is now an inline constant of the component, independent of class ordering, and a caller's `style` lands on the positioning wrapper (the layout box) on both SimpleSelect paths rather than on the w-full control where a flex rule is inert. The Recent picker also shows only while the filter is on. Choosing a window does not enable it, so an always-visible picker turned a chip green while the list did not change. The dormant-collapse row is a section caption rather than an inert row styled like the tappable ones. A wide viewport keeps the flyout untouched, at the same measured geometry.
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
readiness: passed
github-actions
Bot
removed
the
readiness: passed
|
Disposition for the two advisory CONCERNS on FixedDesign: the "picker while filter off changes nothing" defect survived on wide viewports. Correct, and it was my own asymmetry — I gated the mobile branch and left the flyout alone. Fixing it on one modality is not fixing it. Committing a window now enables the filter wherever the window was chosen ( First Principles subtractions. Both confirmed zero-consumer and dropped: Corrected in the descriptionFirst Principles is right that item 5 was under-declared, and my own count was wrong. Not 3 other call sites — 20 pass The bot cited Rebutted — the "~10 undeclared changes / stale diff base" findingThe bot diagnosed this itself ("the diff was cut against a commit older than the merge-base") and it is right. Measured:
Standing, not re-litigated
|
Follow-up to #6909. Picking a Recent window stored the window and nothing else, so on an inactive filter the chip went green and the session list did not change. #6909 hid the picker while the filter was off, but only on the phone -- the wide viewport reaches the same picker through the flyout, so the defect stayed live there. Picking a window now enables the filter, wherever it was picked. Intent is decided at the call site rather than by comparing the new window to the stored one: an explicit pick (preset chip, unit change) enables unconditionally, because an identical value does not mean the user did nothing -- DEFAULT_RECENT_WINDOW_MS equals the "1 hour" preset, so a value-equality gate would swallow exactly the chip a fresh user is most likely to click. The identical-value test survives only on the amount field, which commits on blur as well as Enter and is the one path where "the user did nothing" is real. The mobile-only picker gate is also gone. It existed because picking a window did not enable the filter, so a visible picker on an inactive filter reported an effect it was not having -- this change makes that reason false, and a gate on one viewport is the per-modality thinking that caused the defect. Removing it also makes the mobile test leg real: it used to tap the row to reveal the gated picker, writing the filter flag before the pick, so its assertion passed without the fix. Also drops NativeSelect's wrapperClassName (no consumer) and unexports CHEVRON_GUTTER: a test importing it could only assert the padding equals the constant it was set from, which is true by construction. The test now states the value itself, which is what makes a changed gutter redden.
Follow-up to #6909. Picking a Recent window stored the window and nothing else, so on an inactive filter the chip went green and the session list did not change. #6909 hid the picker while the filter was off, but only on the phone -- the wide viewport reaches the same picker through the flyout, so the defect stayed live there. Picking a window now enables the filter, wherever it was picked. Intent is decided at the call site rather than by comparing the new window to the stored one: an explicit pick (preset chip, unit change) enables unconditionally, because an identical value does not mean the user did nothing -- DEFAULT_RECENT_WINDOW_MS equals the "1 hour" preset, so a value-equality gate would swallow exactly the chip a fresh user is most likely to click. The identical-value test survives only on the amount field, which commits on blur as well as Enter and is the one path where "the user did nothing" is real. The mobile-only picker gate is also gone. It existed because picking a window did not enable the filter, so a visible picker on an inactive filter reported an effect it was not having -- this change makes that reason false, and a gate on one viewport is the per-modality thinking that caused the defect. Removing it also makes the mobile test leg real: it used to tap the row to reveal the gated picker, writing the filter flag before the pick, so its assertion passed without the fix. Also drops NativeSelect's wrapperClassName (no consumer) and unexports CHEVRON_GUTTER: a test importing it could only assert the padding equals the constant it was set from, which is true by construction. The test now states the value itself, which is what makes a changed gutter redden.
…7327) The 'New ephemeral chat' entry in the + New caret dropdown was an unconditional Radix flyout submenu. At phone width a flyout pins to the trigger's side and only shifts vertically, so it opened past the left viewport edge and the Incognito/Temporary rows were unreadable. Apply the same treatment the folder picker and crew submenu received in #6909: at phone width, list the two ephemeral rows inline under a caption label; wider viewports keep the flyout unchanged. Rows are hoisted into a shared fragment (folderRows pattern) so the two branches cannot drift. Two mobile-viewport tests added alongside the existing four desktop tests.
2 participants
Tapping Recent in the sessions filter menu on a phone opened its window picker off the screen edge — partly visible on one side, the rest unreachable.
Root cause
Both duration pickers in that menu are nested Radix submenus.
@radix-ui/react-menuhardcodes a submenu toside="right"(it is not a prop), and@radix-ui/react-poppergives itshift({ mainAxis: true, crossAxis: false })— for a right-side placement that is the vertical axis. So a submenu is free to slide up and down and never moves along the axis that is actually short.At phone width neither side fits beside a menu that already spans most of the screen. Measured on the built SPA at a 390px viewport:
--radix-popper-available-widthflip'sbestFitfallback picks whichever side overflows less, so the same defect surfaces on the left edge on some viewports — which is what the report showed. No width, padding orcollisionPaddingtuning recovers it.Change
On a phone the options render inline inside the one open menu:
Withinpresets + the custom amount/unit row sit under their row, and only while the filter is ON. The row keeps its only job (toggle the filter) and loses the chevron, since there is nothing left to open.The menu also gains
max-w-[calc(100vw-1rem)]. Radix sizes the popper wrapper tomax-content, so the dormant-collapse caption sentence alone stretched the menu to 510px inside a 390px viewport once it moved inline.A wide viewport keeps the flyout, at identical measured geometry (249px wide, 0px clipped, 117.3px tall before and after).
Two defects the inline move exposed
Both were already there, hidden inside a flyout nobody could see, and both are now front and centre on the surface this PR fixes. Raised by UX Review.
NativeSelectdraws its chevron as an overlay atright-3, andcnis tailwind-merge, which cannot split a caller'spx-*shorthand — so this call site'spx-1.5took the right gutter away and the arrow painted over the word (hou⌄rs). Separately,SimpleSelect's touch branch put the caller'sstyleon the<select>, which isw-fullinside the wrapper, so aflex: 1 1 0%there was inert and the control stayed content-width. The gutter is now an inline constant of the component (no dependency on class ordering) andstylelands on the positioning wrapper on bothSimpleSelectpaths. 20 call sites passstyletoSimpleSelectand every one of them was affected on touch — my first count of three was wrong. The values are all layout intent (minWidth,flexShrink: 0,flex: '0 0 140px',width: '100%',marginLeft: 'auto'), and on the<select>they were inert or actively wrong: aminWidthlarger than the wrapper pushed the control out from under the chevron, and a flex rule on a non-flex-item did nothing. Moving them to the wrapper makes each one effective, so some touch-path selects will legitimately change size. Note what the suite does and does not prove:useIsTouchDeviceis false under jsdom, so the 26,161 passing tests exercise the Radix path, not the touch path, for those 20 — the touch path is covered by the newNativeSelect/SimpleSelecttests and the phone screenshots here.1 weekon an inactive Recent turned the chip green and changed nothing in the list. The picker is now gated on the filter being on, and committing a window turns the filter on — the gate alone would have fixed only the phone, since the wide viewport reaches the same picker through the flyout. Gated on a real change, so a blur that re-commits the identical value is not a hidden toggle.Known limitation
This fixes the three submenus reachable from the sidebar's header menus. The same Radix constraint applies to every nested submenu in the app, and these still have no phone branch:
FolderMoveSubmenu(rendered in this same sidebar, one tap from the reported screen),SendToInstanceSubmenu,CronRowActions,ChatPage,WebPreviewPanel— 41DropdownMenuSub*occurrences across 8 files. The cause-level fix is a phone-aware Sub primitive inui/dropdown-menu.tsx, which cannot be done from insideSubContent(the Sub/SubTrigger/SubContent triple is structural) and so is a larger change than this bug report warrants. Tracked in #6911 rather than left implied.Evidence
Before / after, phone (390px), dark — the flyout hanging off the right edge vs. the picker inline:
Light theme, same pair:
The dormant-collapse thresholds inline (menu scrolled to its end), and the wide-viewport flyout left untouched:
Verification
website/scripts/capture-sidebar-duration-pickers-narrow.mjs(new) drives the menu open on the real built SPA and throws if any open menu surface falls outside the viewport. It fails on this PR's base with the 192px overflow above, and passes on the fix — so the evidence is a falsification pair, not two pictures.ChatSidebar.narrowDurationPickers.test.tsx(6 tests) pins the switch from both sides: the inline controls exist at phone width and are hidden while Recent is off, and the rows are stillaria-haspopup="menu"with nothing of the picker in the DOM on a wide viewport.NativeSelect.chevronGutter.test.tsx(5 tests) pins the gutter and the wrapper routing, including throughSimpleSelectitself rather than only the primitive.stylerouted back onto the control, picker un-gated, caption back to a menu item. Each reddens a test.native-select.tsxis shared by 141 call sites, so consumer safety is the full suite:tscclean, eslint 0 errors (warning count unchanged from base), jscpd 0 clones,vitest run1,658 files / 26,159 tests passed. The 17 failures in thetest:electronlane reproduce identically on unmodifiedmainand are unrelated to this diff.No linked issue — reported directly in chat with a phone screenshot.