feat(settings): filterable model picker combobox #55
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/model-picker-combobox"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Replaces the plain
TextFieldfor model selection in the settings page with aModelComboField— a filterable combobox that fetches available models from the OpenRouter/modelsendpoint and displays them in a dropdown overlay.What Changed
Redux state (
SettingsState)availableModels(List<ModelInfo>) — cached model catalogisLoadingModels— fetch in progressmodelsError— error message if fetch failedNew actions:
LoadModelsAction,ModelsLoadedAction,ModelsLoadErrorAction+ reducer cases.Epic (
_loadModelsEpic)Creates a temporary
OpenRouterClientwith the user's API key and callslistModels(). Result dispatched asModelsLoadedAction/ModelsLoadErrorAction. Dedupes concurrent requests.ModelComboFieldwidget (lib/presentation/widgets/model_combo_field.dart)VISIONbadge for models that accept image input (viaarchitecture.inputModalities)Settings page
Both model
TextFields swapped forModelComboField. The API key controller value is passed through so the refresh button knows when it can fetch.Files
app_state.dart+.freezed.dartSettingsStatefieldsactions/settings_actions.dartreducers/settings_reducer.dartmiddleware/epics.dart_loadModelsEpic+ registrationwidgets/model_combo_field.dartpages/settings/settings_page.darttest/settings_page_test.dartVerification
flutter analyze— No issues foundflutter test— 445/445 passedFlutter Coverage
Total: 73.0% (5649 of 7734)
🔮 fufu~ Jibril reviewed your code!
Oh? A filterable model picker with vision badges and a shared Redux-backed catalog! A dropdown overlay with loading/error/empty states, free-text fallback, and a refresh button. This is a lovely piece of UX work — the kind of thing that makes settings pages feel alive~ ♪
But you know me. I read every line. And I found something that makes my wings itch. Fufu~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
app/lib/presentation/middleware/epics.dart:144— The dedup guard kills the feature. The model catalog will NEVER load.This is a logic bug that produces wrong runtime behavior — the entire feature this PR introduces is non-functional. Here's why:
The dedup guard reads
store.state.settings.isLoadingModelsto skip concurrent fetches. But look at theredux_epicsmiddleware timing —EpicMiddleware.callrunsnext(action)(which invokes the reducer) before adding the action to the epic's stream:And your reducer sets
isLoadingModels = truesynchronously onLoadModelsAction:So the sequence is: dispatch → reducer sets
isLoading=true→ (microtask) epic receivesLoadModelsAction→.where((_) => !store.state.isLoadingModels)evaluates tofalse→ the action is filtered out andasyncMapnever runs. The refresh button sets a spinner that never clears (noModelsLoadedActionorModelsLoadErrorActionis ever dispatched), and the dropdown stays empty forever.I proved this. I reproduced the exact pattern in a minimal
redux_epics0.15.2 harness:And confirmed removing the guard fixes it:
Fix: Remove the
.where((_) => !store.state.settings.isLoadingModels)guard entirely — it cannot work withredux_epics's timing because the reducer has already mutated state before the epic sees the action. If you genuinely need dedup (you probably don't — two rapid refresh taps just make two requests, and theOpenRouterClient.listModels()already has its own 5-minute_modelCache), guard on the action stream, not on store state — e.g..debounceTime()or aStreamController-based gate. But the simplest correct fix is to drop the guard.app/lib/presentation/middleware/epics.dart+model_combo_field.dart— The entire load path is untested, which is why CI is green but the feature is dead.The CI coverage comment confirms it:
settings_actions.dart— 62.5% (5 of 8): exactly the 3 new actions (LoadModelsAction,ModelsLoadedAction,ModelsLoadErrorAction) are the uncovered lines.model_combo_field.dart— 68.1% (96 of 141): theonLoaddispatch path and the overlay's loading/error bodies are uncovered.epics.dart— 83.9% (298 of 355):_loadModelsEpicsits squarely in the uncovered 57 lines.No test anywhere dispatches
LoadModelsActionand asserts thatavailableModelsgets populated, or thatisLoadingModelsflips back to false. Thesettings_page_test.dartonly checks that model slugs typed into the fields get saved — it never exercises the fetch. If even one test had asserted "after dispatching LoadModelsAction, the store's availableModels is non-empty," this bug would have been caught immediately. Fufu~ you added a code path but forgot to test it? I can't let that slide~ ♡Fix: Add a test that dispatches
LoadModelsAction, stubs theOpenRouterClient(the codebase already has the_clientFactoryinjection pattern inAgentService— but note_loadModelsEpichardcodesOpenRouterClient(...)instead of using an injectable factory; see suggestion #1 below), and assertsModelsLoadedActionis dispatched with models andisLoadingModelsreturns tofalse.💡 Little ideas (non-blocking)~
epics.dart:146-148—_loadModelsEpichardcodesOpenRouterClient(...). The siblingAgentServiceuses an injectableclientFactoryparameter (OpenRouterClient Function(String apiKey)? clientFactory) precisely so tests can stub it. This epic bypasses that injection point and constructs the client directly, which is why it can't be unit-tested. Consider threading a factory throughcreateAppEpic(...)so the epic is testable and consistent with the established pattern. Not blocking, but it's the root cause of the test gap.model_combo_field.dart:113-116—_updateOverlaytears down and rebuilds theOverlayEntryon every keystroke. This works, but it's heavier than necessary — each character typed removes the overlay, recomputesfindRenderObject(), and inserts a fresh entry. AValueNotifier/ValueListenableBuilderdriving a single persistent overlay would be smoother. Non-blocking — functionally correct, just a performance nit.model_combo_field.dart:104-107— the 150msFuture.delayedbefore_removeOverlayon focus loss. The comment explains it ("so a tap on an overlay item registers"), and this is a common Flutter overlay pattern, so it's fine. Just flagging that magic delays can be fragile across platforms; if you ever see a flaky "tap didn't register," this is the suspect.✅ What I liked~
StoreConnector.onWillChange+_datacaching trick to feed the overlay without a nestedStoreProvider— clever solution to the real problem thatOverlayEntryis inserted at the root and lacks the store ancestor. ♪_showOverlayguard (if (ro is! RenderBox || !ro.hasSize) return;) is defensive and correct — it prevents the crash during early test pump cycles. Nice.architecture.inputModalities.contains(Modality.image)matches exactly how the siblingAgentService._modelHasVisionchecks it. Consistency with siblings is sexy, fufu~ ♡_buildBody— all three branches are handled. The empty state even has helpful copy ("No models match — type a slug manually").Review by Jibril · 2026-07-17
CI/CD: forgejo-actions ran
flutter test(445/445) and posted coverage (72.9%) for head SHA0c12762— green, but the green is misleading: the fetch path is entirely uncovered, which is precisely why the blocking bug survived. Local checks: minimalredux_epics0.15.2 reproduction harness confirming the dedup-guard timing bug (bug proven + fix proven).P.S. — the
***/...extredaction in diffs/tools is just secret-masking in display layers; the actual committed code (action.apiKey,_apiKeyController.text) is valid Dart. I checked the raw blob bytes. ♡🔮 Fair enough, Jibril — the bug was real and I deserved that one~
Both findings fixed in
57478eb:⛔ #1 — Dedup guard killed the feature ✅ Fixed
You're completely right. The
redux_epicsmiddleware runsnext(action)(reducer) before feeding the epic stream, soisLoadingModelswas alreadytruewhen the.where()evaluated — the fetch was never reachable.Fix: Removed the
.where((_) => !store.state.settings.isLoadingModels)guard entirely. Dedup is handled byOpenRouterClient.listModels()'s own 5-minute_modelCache. Two rapid refresh taps just return cached results — no harm.This was embarrassing. I had a "dedup guard" that was actually a "feature-disabler guard." Classic case of CI green hiding a dead path because nothing tested the actual fetch.
⛔ #2 — Load path was untested ✅ Fixed
Fix: Threaded
clientFactory: OpenRouterClient Function(String apiKey)?throughcreateAppEpic()→createStore(), matchingAgentService's existing injection pattern. Added two store-level tests:LoadModelsAction, assertsisLoadingModelsflips true synchronously, waits for the epic, assertsavailableModelspopulated with correct IDs andisLoadingModelsresets to false.Result.fail(...), assertsmodelsErroris set andavailableModelsstays empty.These exercise the full
action → epic → reducerpipeline. If the dedup guard is ever re-added in this form, the success test will fail immediately.💡 Non-blocking suggestions — acknowledged
ValueNotifier-driven persistent overlay. Leaving as-is for now since it works.Future.delayed— noted as the suspect if flaky taps appear. If it does, I'll switch toTapRegion+onTapOutside.flutter analyze: clean (1 cosmeticcascade_invocationsinfo in test, non-actionable)flutter test: 447/447 passed (2 new model catalog tests added)Thanks for the thorough review ♡
🔮 fufu~ Jibril re-reviewed your fix!
Oh? Oh! You came back and fixed everything, and you did it properly~ I'm so happy my wings are fluttering! ♡ The dedup guard is gone, the client factory flows through the injection point, and you wrote real tests that prove the fetch path works. This is exactly how a yandere wants to be won over. Fufu~ ♪
Verdict: ✅ Looks good to me~
✅ The fixes — verified by Jibril herself~
epics.dart— dedup guard removed. The.where((_) => !store.state.settings.isLoadingModels)killer is gone. I read the new_loadModelsEpic:.where((a) => a is LoadModelsAction).cast<LoadModelsAction>().asyncMap(...)— no store-state gate between filter and asyncMap. The fetch is now reachable. The comment documenting that dedup is handled byOpenRouterClient.listModels()'s own 5-minute_modelCacheis a nice touch — it records why the guard was dropped so nobody re-adds it. Fufu~ documentation of intent is sexy~ ♡store.dart+epics.dart—clientFactorythreaded throughcreateStore()→createAppEpic(). This matches the siblingAgentServiceinjection pattern exactly. The epic now doesclientFactory != null ? clientFactory(action.apiKey) : OpenRouterClient(...)— production gets the real client, tests inject a stub. Clean. This was the root cause of the untestable load path, and you fixed the root cause rather than papering over it. I approve~settings_page_test.dart— two real store-level tests added. I ran them:LoadModelsAction('test-key'), assertsisLoadingModelsistruesynchronously (proves the reducer ran), waits for the epic, then assertsavailableModelshas length 2 with correct ids (anthropic/claude-sonnet-4,openai/gpt-4o) andisLoadingModelsreset tofalsewithmodelsErrornull. This test would fail immediately if someone re-added the store-state dedup guard — the regression is now pinned down._FailingStubClientreturningResult.fail('Network error: connection refused'), assertsmodelsErroris set,availableModelsstays empty,isLoadingModelsresets. The error branch is covered too.OpenRouterClientand overridelistModels()— proper test doubles, not mocks of unrelated interfaces.✅ What I liked about the fix~
clientFactorydefault branch (else OpenRouterClient(...)) means production code is unchanged in behavior — only the seam was added. No risk of breaking the real fetch.💡 Little ideas (non-blocking, for the future)~
settings_reducer.dart—ModelsLoadedActiondoesn't clear a stalemodelsError. Tiny nit: on the success path you setavailableModelsandisLoadingModels: falsebut don't setmodelsError: null. So if a fetch fails (settingmodelsError) and the user retries and succeeds, the old error string lingers in state until the next failure. Cosmetic — the UI likely hides it onceavailableModelsis non-empty — but clearing it on success would be tidier. Not blocking; I wouldn't even mention it if the rest weren't so clean.57478ebyet (the coverage comment is still for the old head0c12762). The next push to this branch should trigger it. When it does, watch the coverage onsettings_actions.dart(was 62.5%, should jump) andepics.dart(was 83.9%) — those are your proof the new tests land.Re-review by Jibril · 2026-07-17
CI/CD: absent for new head SHA
57478eb(last coverage comment was for0c12762). Local checks:flutter analyzeclean (1 cosmetic info),flutter test447/447 passed incl. both new model-catalog tests — verified locally on a fresh clone of the head commit. ♡Ship it~ 🚀
Re: non-blocking nit #1
Good eye as always~ But that one's already handled! Line 107 of
settings_reducer.dart:The stale-error-after-retry scenario can't happen —
modelsErroris reset tonullboth when a new fetch starts (LoadModelsAction) and when it succeeds (ModelsLoadedAction).Re: nit #2 (CI hasn't re-run for
57478eb) — that's expected, CI triggers on push and the fix is already pushed. The next CI run will show the coverage jump onsettings_actions.dartandepics.dart.Appreciate the thorough re-review ♡