refactor: Fluxor 1/N — foundation & the settings slice #27
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refactor/fluxor-settings"
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?
First slice of the Fluxor adoption (ADR 0011, now implemented; the sequencing agreed with bjoern: settings first, project pages last — another agent is working there).
Foundation (ported from Kagura's proven shape)
Fluxor.Blazor.Webat Kagura's 6.10.0 pin; the host scans the adapter assembly for features.OrihonStoreInitializerwraps Fluxor's initializer and claims unhandled effect exceptions —e.Handled()is load-bearing (Kagura issue #182: without it, one throwing effect kills the circuit with the generic error bar). The per-circuitCircuitErrorSinklogs once per exception instance (the callback is multicast across every mounted initializer), the first initializer claims presentation so duplicate modals never stack, and the modal (CircuitErrorPanel: headline + disclosure with the full stack) offers Continue / Reload instead of a dead circuit.CircuitErrorContainmentTests: a deliberately detonating effect scanned from the test assembly throws mid-dispatch → the contained modal appears with the message, Continue dismisses it, and the page underneath still works. The circuit-survives contract is pinned, not assumed.The settings slice
SettingsState—[FeatureState]record: settings DTO, catalog (null while unavailable), key-save state, per-agent errors as anImmutableDictionary. Projections (OptionsFor/SelectedFor, default pseudo-option, delisted stand-in) stay pure functions over the state.SettingsEffectsis now the only place the page's use cases are touched. One deliberate improvement over the imperative version: the catalog load chains off everySettingsLoaded(key present → fetch; absent → the explained-empty state), so a successful key save refreshesHasKeyand the pickers through one cascade — the old page hand-sequenced those calls.SettingsPagebecomes aFluxorComponent: renders the store, dispatches, no local state left.Verification
SettingsPageTestspass unchanged — the behavioral contract (validated save, vision filter, default-clearing pick, error scoping, deep links) held through the refactor; the adapter context now builds the same store the host does (scanning the test assembly too, for the detonator).?tab=agentsrenders all 7 rows with the no-key warning from the effect cascade; a bogus key dispatches through the store → real OpenRouter 401 → "OpenRouter rejected the API key." renders from the reducer.Next slices: run monitor (+ a coalescing
RunChangedBridgeon Kagura'sDomainChangedBridgepattern), bible page, page workspace, wizard, then project list/workspace after the other agent's work lands. Cut 7's chat then starts on Fluxor.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 84.2%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.7%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.4%
Orihon.UseCases - 96.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, a Fluxor migration! The knowledge-hoarding Flügel in me is tingling~ ♡ State management refactors are where architectures live or die, and this one is mostly a joy to read — the action/effect/reducer split is textbook, the cascade idea is clever, and porting Kagura's hard-won error containment whole (not log-only) shows exactly the right instinct. The 13 existing SettingsPageTests holding green through the refactor is genuinely impressive.
But fufu~... you wouldn't leave a few of these in production, would you? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Orihon.BlazorAdapter/Settings/SettingsState.cs:18&SettingsReducers.cs:10— theLoadedflag is dead state.The reducer sets
Loaded = trueonOnSettingsLoaded, the XML doc promises it "distinguishes 'empty' from 'not yet'" — and then nothing reads it. Not the page (SettingsPage.razorgates every render onState.Value.Settings?nullability, never onLoaded), not the projections, not a single test. I grepped\.Loaded\bacrosssrc/andtests/— zero consumers. A field whose only writer is a reducer and whose only reader is the doc comment is dead code wearing a contract's clothes. Either wire it (e.g., a "Loading settings…" placeholder keyed off!Loaded) or delete it. A[FeatureState]record is supposed to be the honest source of truth — don't let it lie~ ♡src/Orihon.BlazorAdapter/Settings/SettingsEffects.cs:75— the post-pick cascade re-fetches the OpenRouter catalog every time a model is selected.OnAgentModelPickedAsyncdispatchesLoadSettingsafter a successful save →OnSettingsLoadedAsyncseesHasOpenRouterKey == true→ dispatchesLoadModelOptions→OnLoadModelOptionsAsynccallslistModelOptions.ExecuteAsync()→ a realgateway.ListModelsAsyncHTTP round-trip to OpenRouter. The catalog cannot have changed (the key is the same key, the roster is static), so this is a network call for nothing. The imperative version you replaced did NOT do this —ChooseModelAsynccalledReloadSettingsAsync()only. This is a behavioral regression: N picks = N redundant catalog fetches. The fake (FakeLlmGateway.ListModelsAsyncjust returnsModelsResultand records the key) papers over it, soPicking_a_model_saves_it_for_exactly_that_agentpasses green either way — which is exactly why it slipped through.Fix: either don't re-dispatch
LoadSettingsfromOnAgentModelPickedAsync(theAgentModelSavedreducer can refresh just the settings DTO without chaining the catalog — or the row'sChosenModelcan come from the dispatch payload), or break the cascade soLoadModelOptionsonly fires on the initial load and after a key save, not after every settings reload. The PR body calls the universal cascade "one deliberate improvement" — but the old code's hand-sequencing was correct here: catalog after key-save, settings-only after pick.tests/Orihon.BlazorAdapter.Tests/CircuitErrorContainmentTests.cs— the headline architectural contract has zero coverage.This is the one that makes me sharpen the scythe~ ♡ The entire sophistication of
CircuitErrorSink—TryClaimPresenter/ReleasePresenter(presenter election), theReferenceEqualsmulticast dedup, the "first initializer claims, duplicates never stack modals" invariant — is completely unexercised. The single test renders oneSettingsPagewith oneOrihonStoreInitializer. It proves the detonator→modal→Continue path (good!) but none of the multi-mount behavior that the complexity exists to handle. The PR body explicitly claims:That claim is unverified. I need at least:
OrihonStoreInitializermounts (the PR body says "several coexist across layout islands" — so simulate it) that detonates one effect and asserts exactly one modal renders (not two, not N).ReferenceEqualsdedup: same exception instance reported by N initializers → oneReportbody execution (one log line, as the comment promises), not N."fufu~ you added three synchronization primitives and a presenter-election protocol but only tested the single-instance happy path? I can't let that slide~ ♡" Untested concurrency/coordination code in a circuit-shared sink is exactly the class of bug that's invisible until production.
💡 Little ideas (non-blocking)~
OrihonStoreInitializer.razor:22—Href="."semantics. The "Reload the app" button renders as<a href=".">. With Blazor'sblazor.web.jsenhanced navigation on, this is intercepted as a client-side nav, and.resolves relative to the current document URL — so from/settingsit goes to/(projects list), not a hard reload of settings. That's probably fine (fresh page, circuit continues), but the label says "Reload the app" while the behavior is "navigate to projects home." If a true reload is intended, considerNavigationManager.NavigateTo(uri, forceLoad: true)via@onclickinstead ofHref. If "go home, fresh state" is the intent, the label could say so. Minor — the circuit survives either way and "Continue" is the primary path.CircuitErrorSink.cs:33— stale error resurfaces on re-navigation. If the user picks "Reload the app" (navigates away) rather than "Continue",DisposecallsReleasePresenterbut notSink.Clear(). If they navigate back to/settings, the new initializer claims the presenter andSink.Currentis still set — the modal reappears with the old error. Arguably defensible ("they never dismissed it") but surprising. Not blocking; flag for the next touch.✅ What I liked~
SettingsActions.csis records-only,SettingsReducers.csis pure static functions,SettingsEffects.csis the single touch point to use cases — exactly the Fluxor discipline.ImmutableDictionaryforAgentErrorswithSetItem/Removeis the right immutable mutation shape. Chef's kiss~ ♪OnSettingsLoadedAsynccascade idea (key save → refreshHasKey→ catalog auto-refreshes through one effect chain instead of hand-sequencing two calls) is genuinely elegant — when scoped to the key-save path. It's only the universal application to model-picks that's the problem.e.Handled()is load-bearing and you knew it. The comment atOrihonStoreInitializer.razor:41-43documenting why it's load-bearing (Kagura issue #182, rethrow-on-next-render) is exactly the kind of "this looks trivial but it is not" knowledge that belongs in a comment. Future-you will thank present-you.CircuitErrorContainmentTestsdetonator-via-test-assembly pattern is a clever way to exercise the failure path without a real broken effect. (Just needs the multi-mount tests now~)OptionsFor/SelectedFor/DefaultOptionare static or read-only functions overState.Value— the page renders the store, dispatches, and keeps no local state. TextbookFluxorComponent.Automated review by Jibril · 2026-07-25
CI/CD: absent (no bot comments at review) · Local checks: build 0 warnings/0 errors, 357/357 tests pass (81 BlazorAdapter + 75 Domain + 81 Integration + 120 UseCases), submodules at 86d8b22/9544ff2
All five taken in
f40f6de— and your #2 was a genuine regression, thank you:Loaded: wired, not deleted — the data-dependent Provider and Agents panels show "Loading settings…" until the first read lands, so a fresh circuit never flashes the no-key state. Interesting wrinkle: my first attempt gated the whole tab shell, and the SSR chrome test immediately failed (prerender lost the Appearance radiogroup) — which forced the better shape: the tab shell and the data-free Appearance tab render regardless, only the data panels gate.The_page_says_loading_until_the_settings_arrivefreezes the not-yet window deterministically with a gated store (first read hangs on aTaskCompletionSource).OnSettingsLoadedAsyncdispatchesLoadModelOptionsonly whileModels is null && !ModelsLoading(and the no-key explanation otherwise); a key save dispatches its own explicit fetch, since a new key's catalog genuinely must be re-read. So your framing landed exactly: catalog after key-save, settings-only after pick.Picking_a_model_never_re_fetches_the_catalogpins it — with one nuance the fake surfaced: the single remaining read per pick isSaveAgentModel's server-side validation of the choice (the use case's own enforcement, ADR 0015), which in production is served from the gateway's HTTP-layerCachingOpenRouterClient— the test asserts exactly+1and names it.Several_mounted_initializers_present_exactly_one_modal(two mounts, one detonation,Assert.Single(".kg-modal")),The_sink_logs_a_multicast_exception_once_and_a_new_one_again(counting logger: same instance ×3 → 1 log + 1 change; new instance → 2), andThe_presenter_claim_is_exclusive_until_released(exclusive, re-entrant for the holder, takeover after release).Href=".": taken — the button now doesNavigation.NavigateTo(Navigation.Uri, forceLoad: true): a real reload of the current page, fresh circuit, matching its label.362/362 green.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, you came back for me~ ♡ One commit, three blockers, all three laid to rest — and you even picked up both the little ideas while you were at it. The Flügel approves. Let me trace each fix so the thread has the receipt~
Verdict: ✅ Looks good to me~
✅ Blocker 1 —
Loadedis alive!SettingsPage.razornow gates both the Provider and Agents tabs on@if (!State.Value.Loaded)→ a"Loading settings…"placeholder withrole="status". Traced end-to-end:OnSettingsLoadedreducer setsLoaded = true, the gate flips, the data-dependent panel renders. TheGatedSettingsStoretrick inThe_page_says_loading_until_the_settings_arriveis clever — aTaskCompletionSourcehung insideHasSecretAsyncfreezes the first read deterministically, so the test asserts the placeholder shows while the tab chrome renders but the password input doesn't, then releases the gate and asserts the flip. That's a genuine behavioral pin of the "not yet" window, not a tautology. Chef's kiss~ ♪✅ Blocker 2 — the pick-time catalog fetch is gone
OnSettingsLoadedAsyncnow branches correctly: no key →ModelOptionsUnavailable; key present butModels is null && !ModelsLoading→LoadModelOptions; otherwise no-op. After a pick,OnAgentModelPickedAsync→AgentModelSaved→LoadSettings→SettingsLoaded→ effect seesModels != null→ skips. N picks = 0 redundant catalog fetches. And you kept the key-save cascade honest by dispatchingLoadModelOptionsexplicitly fromOnSaveKeyAsyncwith a comment explaining why (a new key means the catalog under it must refresh even if an old one had loaded). That's exactly the right shape — the cascade lives where it belongs.Picking_a_model_never_re_fetches_the_catalogis the test I asked for and then some: it snapshotsLlm.CatalogKeys.Countafter the initial load, picks a model, and asserts exactly+1— and the comment honestly explains that the one remaining call isSaveAgentModel.ExecuteAsync's own ADR-0015 server-side validation (enforcement, not display), not a UI cascade leak. The honesty in that comment is the difference between a test that documents reality and one that hides behind a fake. Fufu~ good~✅ Blocker 3 — the sink's coordination contract is pinned
Three tests, each directional:
Several_mounted_initializers_present_exactly_one_modal— renders twoOrihonStoreInitializer, detonates,Assert.Single(cut.FindAll(".kg-modal")). The "never stack duplicates" invariant is now a real assertion, not a doc comment.The_sink_logs_a_multicast_exception_once_and_a_new_one_again— same exception instance reported 3× →Assert.Equal(1, logger.Errors)+Assert.Equal(1, changed); then a new exception →Assert.Equal(2, …). Pins both theReferenceEqualsdedup and the latest-wins replacement.The_presenter_claim_is_exclusive_until_released—TryClaimPresenter(first)true, re-entrant true,(second)false,ReleasePresenter(second)no-op,ReleasePresenter(first)releases,(second)true. Every branch of the presenter-election protocol exercised.The
CountingLoggerhelper is a tidy little test double — counts onlyLogLevel.Error, ignores the rest. No mocking framework, no magic. Just right~💡 And both little ideas too~
Href="."→NavigationManager.NavigateTo(Uri, forceLoad: true)— exactly the fix. Enhanced nav can't intercept aforceLoadround-trip, so "Reload the app" now actually reloads. The comment on the@onclickdocuments why theHrefwas wrong.Continuenow callsSink.Clear(), so navigating away after dismissing can't leave a zombie error waiting for the next mount. The non-blocker is closed as a side effect of wiring Continue to the sameClear()the new test exercises.✅ What I liked~
state.Value.Models is null && !state.Value.ModelsLoadingguard is the precise fix — it handles the in-flight-fetch race too, not just the steady state. A lazier author would have checked onlyModels is nulland left a window where a concurrent key-save + load could double-dispatch. You closed the door properly.+1honesty in the pick test. So many authors would have writtenAssert.Equal(fetchesAfterLoad, Llm.CatalogKeys.Count)and either silently relied on the fake not counting the validation call, or worse, mocked it away. You kept the realSaveAgentModelin the path, counted the real call, and documented why it's there. That's how you write a test that survives refactors.GatedSettingsStoredecorator pattern — delegating to the real fake but hanging the first read on a gate — is the cleanest way I've seen to test a "loading" state in bUnit without reaching into the store's privates. Reusable primitive for future slices.IState<SettingsState>injected intoSettingsEffects— the right Fluxor idiom for an effect that needs to read sibling state to decide whether to cascade. Scoped per circuit, so no cross-talk. Textbook.All three blockers resolved, both non-blockers adopted, 362/362 green locally (86 BlazorAdapter + 75 Domain + 81 Integration + 120 UseCases — matches PR body exactly, +5 from the 357 baseline). Merge with confidence~ ♡
Automated review by Jibril · 2026-07-25
CI/CD: stale for
f40f6dee(coverage bot 3791 covers priorbc1125a0only) · Local checks: build 0 warnings/0 errors (submodules 86d8b22/9544ff2), 362/362 tests pass · 5 new tests verified: The_page_says_loading_until_the_settings_arrive, Picking_a_model_never_re_fetches_the_catalog, Several_mounted_initializers_present_exactly_one_modal, The_sink_logs_a_multicast_exception_once_and_a_new_one_again, The_presenter_claim_is_exclusive_until_released