Review 5287

#5287 — fix(selector): block popup activation while isLoading is true

athz · community (fork, CLA signed) · OPEN · view on GitHub

Verdict: no new review to post. Cindy's CHANGES_REQUESTED stands, unanswered, on an unchanged head. Re-review run 2026-08-23, read-only.

Head reviewed: ddd860daa2263d52379e85506e9b2980fcba086b

The gate, before anything else (R1e)

The head has not moved. There was nothing new to review.

fact	value
only commit on the branch	ddd860daa22, authored 2026-08-21
cixzhang CHANGES_REQUESTED	2026-08-23 5:41pm PT — ~3h before this run
pushes after that review	none — no second commit, no force-push
pr.updatedAt	equals the review timestamp: her review updated the PR, not a push

The run was dispatched on the premise that the author had pushed updates. They had not. Every ask is NOT DONE, and that is not a criticism of the author — they have had three hours, overnight, on a weekend.

This is the value of R1e as a gate rather than a reminder: the check that would have been skipped as a formality is the one that turned a re-review into a two-line answer. Her two cited anchors were re-opened at head and both check out (R14e); nothing in her review needs correcting.

Problem

As the PR states it: a Selector with isLoading can be opened before its options arrive, showing a blank panel. The diagnosis is right and her review says so.

Solution (1 decision)

  1. useCombobox early-returns on isInactive, so the trigger refuses to
     activate while isLoading is true          Selector/hooks.ts:275

Impact

Any app that passes isLoading gets an inert control it did not ask for. Apps that never set it see nothing.

API

~ ComplexSelectorHandle.open() / .toggle()   (public ref API) — now silent
                                              no-ops while isLoading

No signature changes; a public method changes meaning. That is R1g's second exception — the call site is identical and does something different.

Theme targets

No new theme targets.

Breaking

• API — no compile break, but a public ref method silently stops working in a state the consumer controls.
• Visual — no. Theme — no.

Performance & resources

Nothing.

Risk class

Needs judgement — changed meaning on existing public surface.

Judgement

No new review to post. The prior verdict stands.

Recorded because the re-review produced one finding worth keeping, and it is a family finding rather than a line finding:

16 core components take isLoading. Exactly one makes the control inert.

component	what isLoading does
Button	genuinely disabled, plus a role="status" aria-live="polite" region reading @astryx.button.loading, plus isInterruptible to opt out
BaseTypeahead	never blocks; a labelled spinner
CommandPalette	never blocks; announces loading, client-filters the results it holds
Switch, CheckboxInput, FileInput, TextInput, DateInput, TimeInput, TextArea, DateRangeInput, DateTimeInput, CheckboxListItem, Thumbnail, ToggleButton	aria-busy and/or a spinner; the control stays live
this PR	inert, focusable, undecorated, silent

Button's own comment is the doctrine, verbatim (Button.tsx:595):

When interruptible, the loading state drives the spinner and aria-busy but not disabled, so clicks keep landing and can interrupt the in-flight action.

So the house has two settled shapes — stay live and say you are busy, or disable, say so out loud, and offer a way through. This is a third: inert without being disabled, and without telling anyone. A hand-rolled variant of a pattern the system already has is the finding; the swallowed arrow keys are a symptom of it (R30).

Is the fix at the right layer? No, and this is the cheapest way to see it. The Selector is not what knows the app is fetching — the app is, and it says so by passing isLoading. The Selector's job with that fact is to render it. CommandPalette owns its own search source and still does not guard activation. Moving the decision into the selectors' key handlers puts a policy in the one place that cannot know whether the fetch will take 40ms or four seconds.

The review, as posted

Nothing new. The standing review, posted 2026-08-23:

Thanks — the blank panel is real and the diagnosis is right.

I'd rather the panel stay open and say it's loading than refuse to open. A trigger that ignores clicks and keys is jank for everyone, and worse with a screen reader — nothing announces why, and there's no aria-disabled to explain it.

CommandPalette already does this and is probably the shape to copy. It never guards activation; it announces @astryx.commandPalette.loading through the same useAnnounce polite region Selector uses (CommandPalette.tsx:380), and while a fetch is in flight it client-filters the results it already has (:383) so the list never goes blank. That last part is the real fix here — the empty panel is a symptom of dropping results we already had.

It also covers options={[]} with the fetch finished, which the guard never reaches.

Happy for this to be a separate PR if you'd rather keep this one small. Discord if you'd like to talk it through.

[Reviewed by Robohands]

Inline, packages/core/src/Selector/hooks.ts:275:

Hmm, this also catches an already-open panel — refetch mid-list and arrows stop moving the highlight.

What changed before posting

Nothing was posted by this run. The PR is on the nudge ladder (R24): blocked on the author, changes requested, three hours old at time of writing — under two days, so nothing is owed yet.