Skip to content

fix(tui): repaint the model selector when a background refresh lands - #4154

Open
probepark wants to merge 1 commit into
Yeachan-Heo:devfrom
probepark:fix/model-selector-catalog-refresh
Open

fix(tui): repaint the model selector when a background refresh lands#4154
probepark wants to merge 1 commit into
Yeachan-Heo:devfrom
probepark:fix/model-selector-catalog-refresh

Conversation

@probepark

Copy link
Copy Markdown
Collaborator

Closes the MEDIUM half of #3847. The HIGH half shipped as #4147.

Defect

ModelSelectorComponent snapshots getAvailable() at construction. If the registry is still loading — the offline/cold-start case — a catalog that arrives afterwards is never reflected: the user stares at a stale or empty list until they close and reopen the selector.

There was no way to observe completion:

  • ModelRegistry emits nothing. Grepping the whole class (model-registry.ts:1189 → ~4095) for listener/emitter/subscribe/notify yields two hits, neither usable: an unrelated import at :80, and at :1263 the registry consuming someone else's event.
  • refreshInBackground (:1283-1299) returns void. Its completion promise lives in #backgroundRefresh (:1235) — an ES #private field with no accessor.
  • The catalog actually changes in #rebuildCanonicalIndex and #resumeRebuild; nothing was notified, the cache was just nulled.

Fix

ModelRegistry.onCatalogChanged(listener): () => void — a listener Set returning an unsubscribe closure, mirroring the proven AuthStorage shape at packages/ai/src/auth-storage.ts:1228. Fired at the two points where #models is committed, not from #invalidateAvailableModels (that only nulls a cache and would emit spurious events).

The selector subscribes and unsubscribes in a new dispose(). The selector previously had zero dispose occurrences even though Container.clear() calls child.dispose?.() (packages/tui/src/tui.ts:552) on the path the selector closes through — so any listener without it would leak on every close.

Rejected alternatives

  • authStorage.onGenerationChanged — reachable from the selector today via this.#modelRegistry.authStorage, and the obvious thing to grab. It is a trap: it fires on credential generation bumps, not catalog arrival. A background refresh discovering models over an already-authenticated provider bumps no generation, so it would never fire on the reported bug — while compiling cleanly and passing a test that manually bumped the auth generation. This component has burned three prior attempts on exactly this shape of false positive.
  • Returning the #backgroundRefresh promise — cheaper, but ordering is worse: refreshInBackground dedupes concurrent calls, so a selector opened after a refresh started gets nothing.
  • Polling getProviderDiscoveryState() — needs a timer. New subsystem.

The subscription is guarded, deliberately

Calling onCatalogChanged unconditionally in the constructor broke 68 tests across suites this change never touches:

TypeError: this.#modelRegistry.onCatalogChanged is not a function
  at new ModelSelectorComponent (model-selector.ts:487)

The repo has many hand-rolled registry fakes built with as unknown as ModelRegistry, and the cast hides a missing method from the compiler until runtime. Teaching a dozen fakes the new method is churn that breaks again with the next fake written, so the guard lives in production: a registry without the method simply gets no live catalog updates.

Verification

run result
bun test packages/coding-agent/test/model-selector-batch-thinking.test.ts 5 pass / 0 fail
mutation — remove both #notifyCatalogChanged() emits 3 pass / 1 fail on the new test
re-apply 5 pass / 0 fail
--test-name-pattern "ModelSelector|model.selector" 83 pass / 0 fail (was 14 pass / 68 fail before the guard)
bun --cwd=packages/coding-agent run check:types clean
biome 2.5.2 on all three changed files clean
preflight (head contains origin/dev) PASS

The mutation was re-run by me against the committed branch. It fails on the catalog path specifically — confirming the test does not merely ride the auth-generation route that defeated earlier attempts.

Not covered

Repaint behavior when a refresh fails midway. The listener fires only on successful catalog commit; a failed refresh leaves the previous list, which is the current behavior either way.

@probepark
probepark force-pushed the fix/model-selector-catalog-refresh branch 2 times, most recently from f5c3e8c to 7db1873 Compare August 10, 2026 07:16
@probepark
probepark force-pushed the fix/model-selector-catalog-refresh branch from 7db1873 to 847f720 Compare August 10, 2026 07:35
@probepark
probepark requested a review from Yeachan-Heo August 10, 2026 07:51
@probepark
probepark force-pushed the fix/model-selector-catalog-refresh branch 2 times, most recently from 0949c93 to 726f47a Compare August 10, 2026 09:39
@probepark
probepark force-pushed the fix/model-selector-catalog-refresh branch 5 times, most recently from 16b040b to 6d39225 Compare August 10, 2026 13:24
The selector snapshotted getAvailable() at construction. ModelRegistry
emitted nothing, and the completion promise for refreshInBackground()
lives in a #private field with no accessor, so a catalog arriving after
the selector opened was invisible until the user closed and reopened it.

ModelRegistry now exposes onCatalogChanged, mirroring the AuthStorage
listener shape, fired where #models is committed. The selector
subscribes and unsubscribes in a new dispose().

authStorage.onGenerationChanged was rejected as the signal: it fires on
credential generation bumps, not catalog arrival, so a refresh that
discovers models over an already-authenticated provider never fires it.
Wiring it would have compiled and passed a test that bumped the auth
generation while never firing on the reported bug.

The subscription is guarded because many test fakes are built with
`as unknown as ModelRegistry` and the cast hides a missing method from
the compiler; an unconditional call broke 68 tests in suites this
change never touched.

Lore-id: 3847d1fb
Constraint: no polling -- a timer would be a new subsystem
Rejected: authStorage.onGenerationChanged | fires on credentials, not catalog
Rejected: return the #backgroundRefresh promise | a selector opened after the refresh started gets nothing
Rejected: teach every fake the new method | churn, and the next fake written breaks again
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: removing both emits turns 4 pass / 0 fail into 3 pass / 1 fail; the ModelSelector cohort is 82 pass / 0 fail
Not-tested: repaint under a refresh that fails midway
@probepark
probepark force-pushed the fix/model-selector-catalog-refresh branch from 6d39225 to aae0903 Compare August 11, 2026 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant