Repository navigation
fix(gui): portal select dropdowns to avoid clipping - #393
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughClaude settings dropdowns now support portal rendering outside clipped containers. The Select component calculates fixed viewport-aware placement, flips or clamps menus as needed, applies portal styling, and preserves inline rendering by default. Tests cover both rendering modes and placement behavior. ChangesSelect menu portal flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Select
participant Positioning
participant DocumentBody
User->>Select: Open dropdown
Select->>Positioning: Compute trigger-based menu style
Positioning-->>Select: Return fixed placement
Select->>DocumentBody: Render dropdown through portal
DocumentBody-->>User: Display positioned listbox
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The clipping bug in #340 is real and using a portal is the right general fix, but portaling every Select to document.body breaks the existing language-menu contract. The menu is no longer a descendant of .lang-toggle/.sidebar, so the contextual glass fallback and, more importantly, the mobile rule that moves .select-dropdown-beside above the trigger no longer apply. The shared right-placement calculation can also keep a 120px menu at top=700 in an 800px viewport and enforce maxHeight >=120, leaving it outside the viewport (the new test currently expects this overflow). Please carry the required variant/style explicitly into the portal, clamp or flip right-placement menus to the viewport, and add mounted desktop/mobile portal regressions rather than only testing the pure coordinate helper. Re-request after exact-head CI is green.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e8190b000
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ))} | ||
| </div> | ||
| )} | ||
| {dropdown && createPortal(dropdown, document.body)} |
There was a problem hiding this comment.
When a Select is used inside existing modals (for example the Dashboard update-channel select and the Models custom-context select), this portals the menu to document.body while .select-dropdown remains at --z-popover below .modal-overlay/native showModal() dialogs. Opening those selects now places the list behind the full-screen modal layer (and outside the native dialog top layer), so the options are not visible or clickable; portal into the owning modal/dialog or give the portal a modal-aware host/z-index.
Useful? React with 👍 / 👎.
| if (openAbove) { | ||
| return { | ||
| position: "fixed", | ||
| left: trigger.right + 6, |
There was a problem hiding this comment.
For placement="right" this positions the menu at trigger.right + 6 without any horizontal fit check. The language selector in App.tsx uses this placement inside the mobile drawer (width: min(280px, 84vw)), and because the dropdown is now portaled the old .sidebar .lang-toggle .select-dropdown-beside mobile override no longer applies; on narrow phones the menu starts near the drawer's right edge and extends off-screen, making some locale options hard or impossible to reach. Add a left/right fallback or clamp for beside placement.
Useful? React with 👍 / 👎.
| if (flipUp) { | ||
| const style: CSSProperties = { | ||
| position: "fixed", | ||
| bottom: vh - trigger.top + FLIP_GAP_PX, |
There was a problem hiding this comment.
When the viewport logic chooses the flip-up path, the returned style only sets bottom, but the same element still has .select-dropdown { top: calc(100% + 4px) } (or .select-dropdown-beside { top: 0 }) from CSS because the portal class only changes position. With both top and bottom applied, dropdowns near the bottom will be laid out at the stale top offset or stretched instead of appearing above the trigger; set top: "auto" in the flipped style (and similarly clear conflicting offsets) so the upward placement actually takes effect.
Useful? React with 👍 / 👎.
| if (placement === "right") { | ||
| const spaceBelow = vh - trigger.bottom - VIEWPORT_PAD_PX; | ||
| const overflowBelow = trigger.top + measuredHeight - (vh - VIEWPORT_PAD_PX); | ||
| const openAbove = overflowBelow > 0 && overflowBelow > spaceBelow; |
There was a problem hiding this comment.
For placement="right", this condition can leave a menu opening downward even when its fixed top plus the enforced 120px minimum height exceeds the viewport. The added test case with top: 700, bottom: 736, and menuHeight: 120 demonstrates it: openAbove is false, so the returned style uses top: 700 and maxHeight: 120, putting the bottom past an 800px viewport. Base the decision on whether the menu fits below (or clamp to the available space) rather than comparing overflow amount to the remaining space below.
Useful? React with 👍 / 👎.
…wn (#340) Address review feedback on #393: add mounted React regressions (happy-dom) proving (1) a portal Select renders its dropdown under document.body, outside the clipping card, with the select-dropdown-portal marker, and (2) a non-portal Select keeps the language-menu contract — dropdown stays a descendant of .custom-select and retains .select-dropdown-beside so the glass fallback and mobile upward-placement CSS still apply. The pure coordinate helper (select-position.test.ts) already covers viewport flip/clamp.
|
Re-reviewed against @Ingwannu's CHANGES_REQUESTED — the two behavioral concerns are already resolved on the current head, and I added the missing mounted regressions (pushed to this branch as maintainer edit, (A) language-menu contract — portaling is opt-in ( (B) right-placement overflow — (C) mounted regressions — added
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33aaa5bb4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| position: "fixed", | ||
| bottom: vh - trigger.top + FLIP_GAP_PX, | ||
| minWidth: width, | ||
| maxHeight: Math.max(MIN_MENU_HEIGHT_PX, spaceAbove - MENU_GAP_PX), |
There was a problem hiding this comment.
When a portaled select with many options opens near the bottom, this branch can return nearly the entire space above the trigger (for example, 688px), overriding the existing 280px CSS cap and the MAX_MENU_HEIGHT_PX limit used by every other branch. The Claude auto-summary select already has 12 options, so its menu expands well beyond 280px when flipped instead of remaining consistently sized and scrollable; cap this calculation with MAX_MENU_HEIGHT_PX as the below and beside branches do.
Useful? React with 👍 / 👎.
| ))} | ||
| </div> | ||
| )} | ||
| {portal ? (dropdown && createPortal(dropdown, document.body)) : dropdown} |
There was a problem hiding this comment.
When a keyboard user activates one of the new portaled Claude selects, focus remains on the trigger, while this portal places the option buttons at the end of document.body after the application root. Because Select implements neither arrow-key navigation nor programmatic option focus, pressing Tab visits every subsequent control in the page before reaching the open menu, regressing the adjacent tab order that the non-portaled dropdown had. Focus the selected or first option on open and restore the trigger on close, or implement an equivalent active-descendant keyboard interaction.
Useful? React with 👍 / 👎.
* fix(gui): portal select dropdowns to avoid clipping * fix(gui): scope select portals to clipped sidecar panels * fix(gui): clamp and flip right-placed select menus * test(gui): mounted portal/language-menu regressions for select dropdown (lidge-jun#340) Address review feedback on lidge-jun#393: add mounted React regressions (happy-dom) proving (1) a portal Select renders its dropdown under document.body, outside the clipping card, with the select-dropdown-portal marker, and (2) a non-portal Select keeps the language-menu contract — dropdown stays a descendant of .custom-select and retains .select-dropdown-beside so the glass fallback and mobile upward-placement CSS still apply. The pure coordinate helper (select-position.test.ts) already covers viewport flip/clamp. --------- Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
3 participants
Fixes #340
Summary
Selectdropdown menus todocument.bodyso they can escape clipped parentsTesting
bun test ./tests/select-position.test.tsbun run lint:guibun run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Tests