Fix Model Pad picker overlap and double focus chrome - #94
Conversation
Keep model details in the popover layout so they sit beside the List/Pad menu instead of painting over the canvas, and draw keyboard focus inside the pad so the global offset outline no longer forms a second ring. Co-authored-by: Sambit Biswas <sambitcreate@users.noreply.github.com>
There was a problem hiding this comment.
ℹ️ Minor suggestions — the fix reads sound and verified clean; two small points below.
Reviewed changes — One commit (c7fdc660) on top of d40d00f. The composer picker popover now lays the menu chrome and the hover-details card out as in-flow flex siblings, so Radix collision measures the real footprint and the right: 244 collisionPadding hack disappears; the pad's keyboard focus chrome moves from the global offset outline to an inset --focus-ring ring (resting insets preserved) and drops the full-canvas bg-list-selection fill; a new source-reading contract test pins the in-flow layout, the absent abs/hack patterns, and the CSS ring.
- Popover flex restructure —
PopoverContentbecomesflex w-max … items-start gap-2, with the menu chrome (rounded-popover bg-popover shadow-popover overflow-hidden, fixedmin(19.75rem, …)width) moving to an innerdivandModelHoverDetailsrendered as a sibling. The chrome moves correctly despite thecnmerge overmenuContentClass(bg/rounded/shadow/padding are all overridden per conflict group), and footer↔details-card exclusivity is semantically preserved. - Pad focus ring —
.model-pad:focus-visible(styles.css:620) overrides the global[role="listbox"]outline rule correctly: equal specificity, both!important, later source order wins. The ring uses the neutral--focus-ringtoken per convention. - Contract test — asserts the new classNames, absence of the old abs-positioning/hack strings, and the CSS rule; it genuinely fails against the base code (verified), passes on head, and the file is already registered in
test,test:preflight, andtest:slash-commands.
Verified locally: composer.test.tsx 10/10, model-pad + interface-polish suites 54/54, eslint clean on all three changed files. Remaining unknowns (popover geometry near viewport edges, ring appearance) are in-app visual QA.
ℹ️ Nitpicks
- The new chrome wrapper at
renderer/components/model-picker.tsx:549left its entire body (lines 550–717) one level under-indented;npm run format(oxfmt) would re-indent the block. Cosmetic, not CI-enforced.
DeepSeek Flash | 𝕏
| assert.match( | ||
| modelPicker, | ||
| /className="flex w-max max-w-\[calc\(100vw-1\.5rem\)\] items-start gap-2 overflow-visible bg-transparent p-0 shadow-none"/u, | ||
| ); | ||
| assert.match( | ||
| modelPicker, | ||
| /className="relative w-\[min\(19\.75rem,calc\(100vw-1\.5rem\)\)\] overflow-hidden rounded-popover bg-popover shadow-popover"/u, | ||
| ); | ||
| assert.match( | ||
| modelPicker, | ||
| /className="pointer-events-none w-56 shrink-0 rounded-popover bg-popover p-3 text-primary shadow-popover"/u, | ||
| ); | ||
| assert.doesNotMatch(modelPicker, /left-\[calc\(100%\+0\.5rem\)\]/u); |
There was a problem hiding this comment.
The green assertions all still pass if <ModelHoverDetails> moves back inside the chrome wrapper — nesting it under the overflow-hidden box (model-picker.tsx:549) reintroduces exactly the clipping this PR fixes without tripping a single pin. Anchoring the sibling relation — the details conditional appearing after the chrome box's closing </div> — would lock the layout contract the change actually relies on.
Technical details
# Test does not pin the aside as a sibling of the chrome box
## Affected sites
- renderer/components/composer.test.tsx:262-281 — the new "details sit beside the menu" test
## Required outcome
- The guard should fail if `ModelHoverDetails` is ever nested inside the `overflow-hidden` chrome wrapper at renderer/components/model-picker.tsx:549, which would clip the details card without re-introducing the abs-positioning classes the current `doesNotMatch` pins look for.
## Suggested approach (optional)
- Assert adjacency between the chrome box close and the details conditional, e.g.:
`assert.match(modelPicker, /<\/div>\s*\{showExternalDetails && \(view === "list" \|\| hasPadModels\) \? \(/u);`
Problem
Opening the composer model picker on Pad view showed two layout glitches:
listbox, so the globaloutline-offset: 2pxring landed in the popover padding and looked like a second chrome border around the canvas.Change
flex+ gap), so collision detection includes both and the details card no longer overlays the pad..model-pad:focus-visibleinstead of the offset outline, and drop the full-canvaslist-selectionfill on focus.Tests
renderer/components/composer.test.tsxto lock the in-flow details layout and inset pad focus contract.