Skip to content

Commit 69130e2

Browse files
committed
Combobox/Select review fixes: open-on-chevron race, snug popup, full-height hit area, dev auto-load
From the live review of the dropdown work: - `ui/Combobox`: stop hand-controlling `open`. Ark's machine owns it via `openOnClick` (click text opens, chevron toggles, typing opens). The old focus-driven `open` raced the trigger's own toggle (the trigger focuses the input, which reopened it just as the toggle closed it), so the first chevron click flashed the list shut. - The loading spinner is now a non-interactive overlay (`position: absolute` + `pointer-events: none`) instead of a flex child, so a click on it falls through to the input and still opens the list on the first try. - The chevron `Trigger` stretches to full height with wider padding: the whole right side is the hit area, not just the icon. - Tighten the popup gutter to 2px on both `ui/Select` and `ui/Combobox` so the list sits snug under the field (was Ark's 8px default). - `AiCloudSection` on-open model load now fires in dev and prod, suppressed only in automated E2E (was prod-only, so dev needed a manual "Test connection"). Cache hits still serve everywhere. - `CloudProviderSetup.test.ts`: focus the model input for real (`.focus()` + `focusin`) before typing; the combobox machine only processes input once focused, and a synthetic `focus` event doesn't drive it there like a user does.
1 parent 4a9aa71 commit 69130e2

4 files changed

Lines changed: 31 additions & 31 deletions

File tree

apps/desktop/src/lib/onboarding/CloudProviderSetup.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,11 @@ describe('CloudProviderSetup', () => {
346346
await advanceTimers(1500)
347347
const modelInput = mounted.target.querySelector<HTMLInputElement>('input[aria-label="Model"]')
348348
if (!modelInput) throw new Error('model input missing')
349-
modelInput.dispatchEvent(new Event('focus', { bubbles: true }))
349+
// Focus for real (not a bare `focus` Event): the combobox's machine only processes input
350+
// changes once focused, and a synthetic `focus` event doesn't drive it there like a user click
351+
// does. `.focus()` + `focusin` mirrors the real focus path the user takes before typing.
352+
modelInput.focus()
353+
modelInput.dispatchEvent(new Event('focusin', { bubbles: true }))
350354
await settle()
351355
modelInput.value = 'gpt-4o'
352356
modelInput.dispatchEvent(new Event('input', { bubbles: true }))

apps/desktop/src/lib/settings/sections/AiCloudSection.svelte

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -162,11 +162,9 @@
162162
163163
/**
164164
* On open, serve the model list from the session cache instantly, or kick off a check that
165-
* fills it. Gated so it only fires a real request in prod: for no-key providers
166-
* (`custom`/`ollama`/`lm-studio`) `hasCheckableConfig` is true with the preset base URL, so a
167-
* mount-trigger would otherwise hit a live endpoint in dev/E2E. A warm cache hit still works in
168-
* dev/E2E (no network). Also skips when a check is already scheduled (for example from a
169-
* just-handled provider switch) so we don't double-fire.
165+
* fills it (dev and prod both auto-load; only automated E2E is suppressed, since it has no real
166+
* provider). A warm cache hit still works everywhere, including E2E. Also skips when a check is
167+
* already scheduled (for example from a just-handled provider switch) so we don't double-fire.
170168
*/
171169
async function populateModelsOnOpen(): Promise<void> {
172170
if (!hasCheckableConfig) return
@@ -177,9 +175,10 @@
177175
connectionStatus = 'connected'
178176
return
179177
}
180-
// No mount-triggered network in dev/E2E (the auto-check is the only request that fires
181-
// without a user action). Cache hits above still work everywhere.
182-
if (getAppMode() !== 'prod') return
178+
// Auto-loading the list is the only request that fires without a user action; suppress it
179+
// only in automated E2E (no real provider there, so it'd just add network flakiness). Dev and
180+
// prod both auto-load. Cache hits above still work everywhere, including E2E.
181+
if (getAppMode() === 'e2e') return
183182
if (connectionCheckTimer || connectionStatus === 'checking') return
184183
scheduleConnectionCheck()
185184
}

apps/desktop/src/lib/ui/Combobox.svelte

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,12 @@
2222
* - `allowCustomValue` accepts the custom value on close.
2323
* - `value` is intentionally left uncontrolled / unused: a typed custom value persists.
2424
*
25-
* Open-on-focus is wired via a controlled `open` state (Ark has no `openOnFocus` prop). `loading`
26-
* is OUR in-field spinner overlay (Ark has no loading prop). No `Portal` (keeps the viewer's
27-
* restricted capability set unaffected). No entrance animation by default.
25+
* Open state is owned entirely by Ark (uncontrolled) with `openOnClick`: clicking the text opens
26+
* it, the chevron `Trigger` toggles it, typing opens it (`openOnChange`). Don't reintroduce a
27+
* controlled `open` driven from the input's focus: the `Trigger` focuses the input on click, so a
28+
* focus-open handler races the trigger's own toggle and flashes the popup shut. `loading` is OUR
29+
* in-field spinner overlay (Ark has no loading prop). No `Portal` (keeps the viewer's restricted
30+
* capability set unaffected). No entrance animation by default.
2831
*/
2932
import { Combobox, createListCollection } from '@ark-ui/svelte/combobox'
3033
import IconChevronDown from '~icons/lucide/chevron-down'
@@ -62,37 +65,24 @@
6265
}),
6366
)
6467
65-
let open = $state(false)
66-
6768
function handleInputValueChange(details: { inputValue: string }): void {
6869
onInputValueChange(details.inputValue)
6970
}
70-
71-
function handleOpenChange(details: { open: boolean }): void {
72-
open = details.open
73-
}
7471
</script>
7572

7673
<div class="combobox-wrapper">
7774
<Combobox.Root
7875
{collection}
7976
{inputValue}
80-
{open}
8177
{disabled}
78+
openOnClick
79+
positioning={{ gutter: 2 }}
8280
selectionBehavior="preserve"
8381
allowCustomValue
8482
onInputValueChange={handleInputValueChange}
85-
onOpenChange={handleOpenChange}
8683
>
8784
<Combobox.Control class="combobox-control">
88-
<Combobox.Input
89-
class="combobox-input"
90-
{placeholder}
91-
aria-label={ariaLabel}
92-
onfocus={() => {
93-
open = true
94-
}}
95-
/>
85+
<Combobox.Input class="combobox-input" {placeholder} aria-label={ariaLabel} />
9686
{#if loading}
9787
<span class="spinner spinner-sm combobox-spinner" aria-label="Loading suggestions" role="status"
9888
></span>
@@ -136,14 +126,14 @@
136126
}
137127
138128
:global(.combobox-control) {
129+
position: relative;
139130
display: flex;
140131
align-items: center;
141132
gap: var(--spacing-xs);
142133
width: 100%;
143134
border: 1px solid var(--color-border);
144135
border-radius: var(--radius-sm);
145136
background: var(--color-bg-primary);
146-
padding-right: var(--spacing-xs);
147137
}
148138
149139
:global(.combobox-control:focus-within) {
@@ -171,16 +161,22 @@
171161
opacity: 0.5;
172162
}
173163
164+
/* Decorative overlay, not a flex child: floats just left of the chevron and passes clicks
165+
through to the input beneath, so a click on the spinner still opens the suggestions. */
174166
.combobox-spinner {
175-
align-self: center;
167+
position: absolute;
168+
right: 28px;
169+
pointer-events: none;
176170
}
177171
172+
/* Full-height, padded hit area: the whole right side toggles the list, not just the icon. */
178173
:global(.combobox-trigger) {
179174
display: inline-flex;
180175
align-items: center;
181176
justify-content: center;
177+
align-self: stretch;
182178
flex-shrink: 0;
183-
padding: 0;
179+
padding: 0 var(--spacing-sm);
184180
border: none;
185181
background: transparent;
186182
cursor: default;

apps/desktop/src/lib/ui/Select.svelte

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@
9494
value={value ? [value] : []}
9595
onValueChange={handleValueChange}
9696
onHighlightChange={handleHighlightChange}
97+
positioning={{ gutter: 2 }}
9798
{disabled}
9899
>
99100
<Select.Control>

0 commit comments

Comments
 (0)