Skip to content

Commit 336c485

Browse files
david-crespodavid-crespo
authored
Fix firewall rule test flake in Firefox (#3059)
* try to fix test flake in ff fix firewall rule flake in firefox * format lol * fix for safari * format, add formatting post tool use hook
1 parent 5ea92cc commit 336c485

File tree

3 files changed

+212
-164
lines changed

3 files changed

+212
-164
lines changed

‎.claude/settings.json‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
{
2+
"hooks": {
3+
"PostToolUse": [
4+
{
5+
"matcher": "Edit|Write|NotebookEdit",
6+
"hooks": [
7+
{
8+
"type": "command",
9+
"command": "jq -r '.tool_input.file_path' | xargs ./node_modules/.bin/prettier --write --ignore-unknown",
10+
"timeout": 10
11+
}
12+
]
13+
}
14+
]
15+
}
16+
}

‎app/ui/lib/Combobox.tsx‎

Lines changed: 135 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
} from '@headlessui/react'
1616
import cn from 'classnames'
1717
import { matchSorter } from 'match-sorter'
18-
import { useEffect, useId, useState, type ReactNode, type Ref } from 'react'
18+
import { useEffect, useId, useRef, useState, type ReactNode, type Ref } from 'react'
1919

2020
import { SelectArrows6Icon } from '@oxide/design-system/icons/react'
2121

@@ -145,146 +145,161 @@ export const Combobox = ({
145145
}
146146
const zIndex = usePopoverZIndex()
147147
const id = useId()
148+
// Tracks whether the dropdown is open so the onKeyDown handler can
149+
// distinguish Enter-to-select (dropdown open, let HUI handle it) from
150+
// Enter-to-submit (dropdown closed, fire onEnter). We use a ref instead
151+
// of HUI's `open` render prop because our handler runs before HUI's
152+
// (useRender merges user props first) and the render prop can be stale.
153+
// The ref stays current because onClose sets it synchronously during
154+
// HUI's own keydown handler. With the stale render prop, the handler
155+
// could see the combobox as closed one keydown too late, firing onEnter
156+
// instead of letting HUI select — hard to notice manually but caused
157+
// consistent e2e flakes in Firefox.
158+
const isOpenRef = useRef(false)
148159
return (
149160
<HCombobox
150161
// necessary, as the displayed "value" is not the same as the actual selected item's *value*
151162
value={selectedItemValue}
152163
// fallback to '' allows clearing field to work
153164
onChange={(val) => onChange(val || '')}
154-
// we only want to keep the query on close when arbitrary values are allowed
155-
onClose={allowArbitraryValues ? undefined : () => setQuery('')}
165+
onClose={() => {
166+
isOpenRef.current = false
167+
if (!allowArbitraryValues) setQuery('')
168+
}}
156169
disabled={disabled || isLoading}
157170
immediate
158171
{...props}
159172
>
160-
{({ open }) => (
161-
<div>
162-
{label && (
163-
// TODO: FieldLabel needs a real ID
164-
<div className="mb-2">
165-
<FieldLabel
166-
id={`${id}-label`}
167-
htmlFor={`${id}-input`}
168-
optional={!required && !hideOptionalTag}
169-
>
170-
{label}
171-
</FieldLabel>
172-
{description && (
173-
<TextInputHint id={`${id}-help-text`}>{description}</TextInputHint>
174-
)}
175-
</div>
176-
)}
177-
<div
178-
className={cn(
179-
`flex rounded-md border focus-within:ring-2`,
180-
hasError
181-
? 'focus-error border-error-secondary focus-within:ring-error-secondary hover:border-error'
182-
: 'border-default focus-within:ring-accent-secondary hover:border-hover',
183-
disabled
184-
? 'text-disabled bg-disabled border-default! cursor-not-allowed'
185-
: 'bg-default',
186-
disabled && hasError && 'border-error-secondary!'
173+
{({ open }) => {
174+
// Sync open state to ref on render (handles the opening side)
175+
if (open) isOpenRef.current = true
176+
return (
177+
<div>
178+
{label && (
179+
// TODO: FieldLabel needs a real ID
180+
<div className="mb-2">
181+
<FieldLabel
182+
id={`${id}-label`}
183+
htmlFor={`${id}-input`}
184+
optional={!required && !hideOptionalTag}
185+
>
186+
{label}
187+
</FieldLabel>
188+
{description && (
189+
<TextInputHint id={`${id}-help-text`}>{description}</TextInputHint>
190+
)}
191+
</div>
187192
)}
188-
// Putting the inputRef on the div makes it so the div can be focused by RHF when there's an error.
189-
// We want to focus on the div (rather than the input) so the combobox doesn't open automatically
190-
// and obscure the error message.
191-
ref={inputRef}
192-
// tabIndex=-1 is necessary to make the div focusable
193-
tabIndex={-1}
194-
>
195-
<ComboboxInput
196-
id={`${id}-input`}
197-
// If an option has been selected, display either the selected item's label or value.
198-
// If no option has been selected yet, or the user has started editing the input, display the query.
199-
// We are using value here, as opposed to Headless UI's displayValue, so we can normalize
200-
// the value entered into the input (via the onChange event).
201-
value={
202-
selectedItemValue
203-
? allowArbitraryValues
204-
? selectedItemValue
205-
: selectedItemLabel
206-
: query
207-
}
208-
onChange={(event) => {
209-
const value = transform ? transform(event.target.value) : event.target.value
210-
// updates the query state as the user types, in order to filter the list of items
211-
setQuery(value)
212-
// if the parent component wants to know about input changes, call the callback
213-
onInputChange?.(value)
214-
}}
215-
onKeyDown={(e) => {
216-
// When the combobox is open, Enter is handled internally by
217-
// Headless UI (selects the highlighted item). When closed,
218-
// we need to prevent the default behavior to avoid submitting
219-
// the containing form. We also considered always preventing
220-
// default on Enter regardless of open status, but it appears
221-
// to break the combobox handling. Headless UI likely checks
222-
// e.defaultPrevented before processing item selection.
223-
if (e.key === 'Enter' && !open) {
224-
e.preventDefault()
225-
onEnter?.(e)
226-
}
227-
}}
228-
placeholder={placeholder}
229-
disabled={disabled || isLoading}
193+
<div
230194
className={cn(
231-
`text-sans-md text-raise placeholder:text-tertiary h-10 w-full rounded-md border-none! px-3 py-2 outline-hidden!`,
195+
`flex rounded-md border focus-within:ring-2`,
196+
hasError
197+
? 'focus-error border-error-secondary focus-within:ring-error-secondary hover:border-error'
198+
: 'border-default focus-within:ring-accent-secondary hover:border-hover',
232199
disabled
233200
? 'text-disabled bg-disabled border-default! cursor-not-allowed'
234201
: 'bg-default',
235-
hasError && 'focus-error'
202+
disabled && hasError && 'border-error-secondary!'
236203
)}
237-
/>
238-
{items.length > 0 && (
239-
<ComboboxButton
204+
// Putting the inputRef on the div makes it so the div can be focused by RHF when there's an error.
205+
// We want to focus on the div (rather than the input) so the combobox doesn't open automatically
206+
// and obscure the error message.
207+
ref={inputRef}
208+
// tabIndex=-1 is necessary to make the div focusable
209+
tabIndex={-1}
210+
>
211+
<ComboboxInput
212+
id={`${id}-input`}
213+
// If an option has been selected, display either the selected item's label or value.
214+
// If no option has been selected yet, or the user has started editing the input, display the query.
215+
// We are using value here, as opposed to Headless UI's displayValue, so we can normalize
216+
// the value entered into the input (via the onChange event).
217+
value={
218+
selectedItemValue
219+
? allowArbitraryValues
220+
? selectedItemValue
221+
: selectedItemLabel
222+
: query
223+
}
224+
onChange={(event) => {
225+
const value = transform
226+
? transform(event.target.value)
227+
: event.target.value
228+
// updates the query state as the user types, in order to filter the list of items
229+
setQuery(value)
230+
// if the parent component wants to know about input changes, call the callback
231+
onInputChange?.(value)
232+
}}
233+
onKeyDown={(e) => {
234+
// When the dropdown is open, Enter should select the
235+
// highlighted option (HUI handles this). When closed,
236+
// Enter should submit the subform via onEnter.
237+
if (e.key === 'Enter' && !isOpenRef.current) {
238+
e.preventDefault()
239+
onEnter?.(e)
240+
}
241+
}}
242+
placeholder={placeholder}
243+
disabled={disabled || isLoading}
240244
className={cn(
241-
'border-secondary my-1.5 flex items-center border-l px-3',
242-
disabled ? 'bg-disabled cursor-not-allowed' : 'bg-default'
245+
`text-sans-md text-raise placeholder:text-tertiary h-10 w-full rounded-md border-none! px-3 py-2 outline-hidden!`,
246+
disabled
247+
? 'text-disabled bg-disabled border-default! cursor-not-allowed'
248+
: 'bg-default',
249+
hasError && 'focus-error'
243250
)}
244-
aria-hidden
251+
/>
252+
{items.length > 0 && (
253+
<ComboboxButton
254+
className={cn(
255+
'border-secondary my-1.5 flex items-center border-l px-3',
256+
disabled ? 'bg-disabled cursor-not-allowed' : 'bg-default'
257+
)}
258+
aria-hidden
259+
>
260+
<SelectArrows6Icon title="Select" className="text-secondary w-2" />
261+
</ComboboxButton>
262+
)}
263+
</div>
264+
{(items.length > 0 || allowArbitraryValues) && (
265+
<ComboboxOptions
266+
anchor="bottom start"
267+
// 13px gap is presumably because it's measured from inside the outline or something
268+
className={`ox-menu pointer-events-auto ${zIndex} border-secondary relative w-[calc(var(--input-width)+var(--button-width))] overflow-y-auto border outline-hidden! [--anchor-gap:13px] empty:hidden`}
269+
modal={false}
245270
>
246-
<SelectArrows6Icon title="Select" className="text-secondary w-2" />
247-
</ComboboxButton>
271+
{filteredItems.map((item) => (
272+
<ComboboxOption
273+
key={item.value}
274+
value={item.value}
275+
className="border-secondary relative border-b last:border-0"
276+
>
277+
{({ focus, selected }) => (
278+
// This *could* be done with data-[focus] and data-[selected] instead, but
279+
// it would be a lot more verbose. those can only be used with TW classes,
280+
// not our .is-selected and .is-highlighted, so we'd have to copy the pieces
281+
// of those rules one by one. Better to rely on the shared classes.
282+
<div
283+
className={cn('ox-menu-item', {
284+
'is-selected': selected && query !== item.value,
285+
'is-highlighted': focus,
286+
})}
287+
>
288+
{item.label}
289+
</div>
290+
)}
291+
</ComboboxOption>
292+
))}
293+
{!allowArbitraryValues && filteredItems.length === 0 && (
294+
<ComboboxOption disabled value="no-matches" className="relative">
295+
<div className="ox-menu-item text-disabled!">No items match</div>
296+
</ComboboxOption>
297+
)}
298+
</ComboboxOptions>
248299
)}
249300
</div>
250-
{(items.length > 0 || allowArbitraryValues) && (
251-
<ComboboxOptions
252-
anchor="bottom start"
253-
// 13px gap is presumably because it's measured from inside the outline or something
254-
className={`ox-menu pointer-events-auto ${zIndex} border-secondary relative w-[calc(var(--input-width)+var(--button-width))] overflow-y-auto border outline-hidden! [--anchor-gap:13px] empty:hidden`}
255-
modal={false}
256-
>
257-
{filteredItems.map((item) => (
258-
<ComboboxOption
259-
key={item.value}
260-
value={item.value}
261-
className="border-secondary relative border-b last:border-0"
262-
>
263-
{({ focus, selected }) => (
264-
// This *could* be done with data-[focus] and data-[selected] instead, but
265-
// it would be a lot more verbose. those can only be used with TW classes,
266-
// not our .is-selected and .is-highlighted, so we'd have to copy the pieces
267-
// of those rules one by one. Better to rely on the shared classes.
268-
<div
269-
className={cn('ox-menu-item', {
270-
'is-selected': selected && query !== item.value,
271-
'is-highlighted': focus,
272-
})}
273-
>
274-
{item.label}
275-
</div>
276-
)}
277-
</ComboboxOption>
278-
))}
279-
{!allowArbitraryValues && filteredItems.length === 0 && (
280-
<ComboboxOption disabled value="no-matches" className="relative">
281-
<div className="ox-menu-item text-disabled!">No items match</div>
282-
</ComboboxOption>
283-
)}
284-
</ComboboxOptions>
285-
)}
286-
</div>
287-
)}
301+
)
302+
}}
288303
</HCombobox>
289304
)
290305
}

0 commit comments

Comments
 (0)