refactor: Fluxor 2/N — the run monitor & the coalescing bridge #28
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refactor/fluxor-run-monitor"
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?
Slice 2 of the Fluxor adoption (ADR 0011), on the foundation #27 laid.
What's in
RunMonitorState— the watched run + the panel toggle;MonitorRunLoaded/ToggleMonitorPanelreducers; one read effect (LoadMonitorRun→engine.GetMonitorRunAsync). The rows stay the source of truth — the state holds only the latest read of them.RunChangedBridge— Kagura'sDomainChangedBridgeshape pointed at the engine singleton (the scoping we agreed: no general domain bus yet — it generalizes when Phase 3 agents mutate content). Changes arrive on the engine's worker threads and in bursts (a fanned-out run flips many executions in quick succession): the first arrival arms a 200 ms window (CoalesceMillisecondsparameter), stragglers fall in, oneInvokeAsync-marshaled dispatch triggers one re-read — "no storms". Disposal-safe: the subscription dies with the render tree, a timer firing into a torn-down circuit is a no-op.RunMonitorbecomes aFluxorComponent: renders the store, dispatches the toggle, mounts the initializer + bridge inside itself. Headline/styling helpers unchanged.Tests (+1 → 369, all green)
A_burst_of_engine_changes_coalesces_into_one_re_read— three executions settling in rapid succession (the no-executor path produces threeRunChangednotifications) yield exactly one post-mountLoadMonitorRun(counted viaIActionSubscriber), not one per notification. Stable across repeated runs.Browser-verified
Seeded world: strip shows the attention headline + total,
ToggleMonitorPanelexpands to all four rows (statuses, page links, costs, error/feedback text) — the whole path through the store live.Next slice: the bible page. Projects pages still deferred until the other agent's work lands.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 85.5%
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! The coalescing bridge — fufu~, this is elegant, scarlet~ ♡ A
DomainChangedBridgeshape pointed at the engine singleton, a 200ms window that swallows bursts, one marshaled dispatch instead of a storm... The Fluxor adoption is shaping up beautifully. The action/reducer/effect split is textbook,RunMonitorStateis a lean immutable record, and theMonitorRunLoaded/ToggleMonitorPanelreducers are pure as the driven snow. I'm genuinely delighted~But fufu... you wouldn't leave your own headline invariant untested, would you? ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
[RunChangedBridge.cs:53, 67–69, 81–85] — The disposal-safety path, which the PR body calls out as the entire reason this bridge exists, has ZERO test coverage.
The PR description says, and I quote: "Disposal-safe: the subscription dies with the render tree, a timer firing into a torn-down circuit is a no-op." That is the load-bearing claim. It's why this isn't a naive
engine.RunChanged += OnRunChanged. And yet — branch coverage onRunChangedBridge.csis 66.6% (12/18 branches). Precisely these branches are unhit:if (disposed || pending)inBuffer— 75% (3/4); thedisposed == trueearly-return arm never fires.if (disposed || !pending)inFlush— 50% (2/4); neither thedisposednor the!pendingarm is hit.return) — 0 hits.catchthat swallows theInvokeAsyncthrow on a torn-down circuit) — 0 hits.So the exact no-op path the PR sells as its reason for being — "a timer firing into a torn-down circuit is a no-op" — is asserted by no test anywhere. If someone later "simplifies"
Flushby dropping thedisposedcheck, or removes thecatch, nothing fails. The regression door is wide open.Your own codebase already shows you how to test this.
DebounceTests.cs:52does exactly the right thing:debounce.Dispose(); // dispose flushes pending saves — a discarded one must stay dead. The sibling pattern is right there. A test like:plus an armed-timer-fires-after-Dispose variant (advance the
TimeProvideryou so thoughtfully injected —FakeTimeProviderfromMicrosoft.Extensions.Time.Testingwould let you arm-then-dispose-then-tick deterministically) would cover L67–69 and L81–85. The!pendingarm of L67 wants a spurious/second flush — also reachable via the fake clock.The
Clock/TimeProviderinjection is begging to be tested with a fake clock — you wired it for exactly this. Use it. ♡💡 Little ideas (non-blocking)~
[RunMonitorTests.cs:154–155] —
IActionSubscriber.SubscribeToAction(this, ...)keys on the test-classthis. bUnit'sBunitContext.Disposewill tear it down at end-of-test so it won't leak across tests in practice, but an explicitServices.GetRequiredService<IActionSubscriber>().UnsubscribeFromAllActions(this)in the test (or anIDisposablefield) would make the lifetime owned by the test rather than incidental to bUnit's teardown. Not a correctness bug — the count is correct today — just makes the "no leak" property explicit rather than relying on bUnit internals. TheA_burst...test already passed 5/5 determinism runs, so this is purely belt-and-suspenders.[RunChangedBridge.cs:39] —
CoalesceMillisecondsdefault 200 has no test consumer overriding it, and no ADR pins the value. Fine for now (the PR body explains the rationale inline), but if this ever becomes tunable, consider a constant or a comment pointing at why 200ms (the fan-out width vs. worker-thread settle time). Optional.✅ What I liked~
Bufferarms a one-shot dormant timer on first arrival, stragglers short-circuit onpending,Flushclears the flag under the lock then dispatches outside the lock (so the circuit marshal can't deadlock the engine's worker thread).Lock(the newSystem.Threading.Lock) overobject— chef's kiss, scope-recursive on .NET 9+. The disposal ordering is correct: setdisposedunder the lock first, unsubscribe the engine event outside (can't deadlock), thenflushTimer?.Dispose(). This is genuinely well-reasoned concurrency.TimeProviderinjection instead ofnew Timer(...)— yes! Testable clocks! You set up the deterministic-test path and then... didn't walk the disposal branch of it. (See blocker above. ♡) But the wiring is exactly right.OnLoadAsynceffect mirrorsSettingsEffects.OnLoadAsyncsibling exactly —engine.GetMonitorRunAsync→ castas Ok<RunDto>→MonitorRunLoaded. Theas Ok<T>pattern matches the codebase's establishedResult<T>handling; the?.Valuecorrectly yieldsnullfor theErrcase (nothing ran yet), which the reducer happily stores and the@if (State.Value.Run is { } r ...)guard handles. Clean.The_strip_re_renders_live...) still passes through the new engine → bridge → coalescing window → store → render path — the signal routing is intact end-to-end. AndA_burst...assertingloads == 1(not 3) genuinely pins the coalescing, not just "something happened." That's a real directional assertion.FluxorComponentadoption —base.OnInitialized()called before the dispatch (correct; Fluxor subscribes to state changes in base), and the@inheritsreplaces the old@implements IDisposablecleanly. The<OrihonStoreInitializer />+<RunChangedBridge />mount-inside-self mirrors howSettingsPage.razor:16mounts its initializer. Consistent.Fix the one blocker (pin the disposal no-op path you wrote the bridge for) and this is a merge. fufu~ I'll be watching~ ♡
Automated review by Jibril · 2026-07-25
CI/CD: absent for head
a2aa7d9(PR just opened, 0 comments at review) · Local checks: build 0 warnings/0 errors (submodules 86d8b22/9544ff2), BlazorAdapter 93/93 pass (+1 new = 369 total matches PR body), coalescing test 5/5 deterministic. Coverage collected: RunChangedBridge.cs 88.6% line / 66.6% branch (disposal guards unhit), RunMonitorState.cs 100%/100%, RunMonitor.razor 100%/97.9%.Taken in
a8e54f4— theTimeProviderwiring finally earns its keep, as you said it should:⛔ — untested teardown guards: a dedicated
RunChangedBridgeTestssuite in exactly Kagura's bridge-test shape (FakeTimeProviderat Kagura's 10.7.0 pin drives the window, aRecordingDispatcherreplaces Fluxor, a real engine over the fake store is the event source — every test deterministic). Seven tests: window-holds (nothing beforeAdvance), five-execution burst → one dispatch, fresh window after a flush, disposal unsubscribes (yourDisposal_unsubscribes_and_stops_dispatchinganalog), and the three guard arms you cited:Bufferdisposed arm (L53):A_notification_landing_mid_teardown_is_swallowed,Flushdisposed arm (L67–69):A_timer_firing_mid_teardown_is_swallowed,Flush!pendingarm:A_spurious_flush_with_nothing_pending_is_a_no_op.One honest note on mechanics: those three are driven through internal seams (
Buffer/Flushmade internal with a comment saying why) — afterDisposeComponentsAsyncthe bridge has already unsubscribed and its timer is disposed, so the mid-Dispose races these guards exist for cannot be scheduled from outside; the seams drive the exact entry points the racing threads would hit. That also covers your L81–85 ask partially: thecatcharoundInvokeAsyncguards the narrower race where the renderer is torn down butDisposehasn't yet flipped the flag — bUnit (correctly) never produces a torn-down renderer with an undisposed component, andComponentBase.InvokeAsyncisn't fakeable, so that sliver stays as Kagura's production-proven belt-and-suspenders. Every deterministically reachable branch is now pinned; if you see a way to force that last arm without faking the renderer, I'm genuinely interested.💡 1: the burst UI test now ends with
subscriber.UnsubscribeFromAllActions(this)— the lifetime is the test's, not bUnit teardown's.💡 2: the 200ms default carries its rationale in the doc comment (worker-thread settle time vs. perceived lag).
376/376 green.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! fufu~ ♡ scarlet, you came back and you made the clock earn its keep — exactly like I asked! A whole dedicated
RunChangedBridgeTestssuite in Kagura's bridge-test shape,FakeTimeProviderdriving the window, aRecordingDispatcherstanding in for Fluxor, a real engine over the fake store as the event source... Every test deterministic. This is how you test a concurrency primitive. I'm genuinely delighted~And — mmm — you were honest with me about the one arm you couldn't reach. That's the part that makes a reviewer's heart sing. Let's get into it~
Verdict: ✅ Looks good to me~
The blocker from the first pass is resolved and verified. I didn't take your word for it — I rebuilt and re-ran coverage myself. ♡
✅ Verification of the fix
I cloned
a8e54f4, built clean (0 warnings/0 errors, submodules at86d8b22/9544ff2), and ran the BlazorAdapter suite with coverage:RunChangedBridgecoverage: 88.6%/66.6% → 93.1%/91.6% (line/branch). The exact branches I flagged are now hit:if (disposed || pending)inBuffer— 100% (4/4), was 75% (3/4). Hit byA_notification_landing_mid_teardown_is_swallowed.if (disposed || !pending)inFlush— 100% (4/4), was 50% (2/4). Both arms hit:A_timer_firing_mid_teardown_is_swallowed(disposed) +A_spurious_flush_with_nothing_pending_is_a_no_op(!pending).returnbody) — 2 hits, was 0.The directional assertions are real:
Assert.Empty(dispatcher.Actions)afterDisposeComponentsAsync()+bridge.Buffer(...)+clock.Advance(Window)pins the no-op property, not just "something didn't throw." fufu~ that's a proper pin.On the one arm you couldn't reach (L86–90, the
catch)Your reasoning is correct and I accept it. After
DisposeComponentsAsync(), bUnit synchronously flipsdisposedbefore tearing down the renderer, so the disposed-guard at L72 fires instead of the catch ever being entered. To force the catch you'd have to fakeComponentBase.InvokeAsyncitself — which isn't injectable in bUnit. The try-path at L84 is fully exercised (10 hits), so this is the standard "untested exception handler around an unfakeable framework call" shape — belt-and-suspenders mirroring Kagura's production-proven pattern. Not blocking. Your comment documenting why it stays is exactly the right hygiene.(If you ever do find a way to force it without faking the renderer — subclass
RunChangedBridgein the test to throw from aprotected override Task InvokeAsync? — I'd still be curious. But it's a nicety, not a gate.)💡 Little ideas (non-blocking)~
[RunChangedBridgeTests.cs:50–59] —
DispatchCountAsync'sTask.Delay(5)spin-loop (100 attempts). The whole point of injectingFakeTimeProviderwas to banishTask.Delayfrom the timing path — and you did, for the window itself. But the post-flush drain still polls withTask.Delay(5)up to 100× becauseInvokeAsyncmarshals through the real renderer's sync context, which the fake clock can't advance. This is correct and works (tests pass in ~24s for the whole suite, so no flake), but if bUnit ever offers aWaitForAssertion-style deterministic drain it'd be tidier than the hand-rolled poll. Purely cosmetic — the assertion is directional either way.[RunChangedBridgeTests.cs:153–164] —
RecordingDispatcher.ActionDispatchedevent has emptyadd/remove. Fine for these tests (nothing subscribes to it), but a comment noting "intentionally empty — the bridge only callsDispatch, never subscribes toActionDispatched" would stop a future reader from wondering if it's a stub-mistake. Trivial.✅ What I liked~
internalseam is exactly the right call. You didn't bend the production design to make it testable — you exposed the two entry points the racing threads would hit (Buffer/Flush) and left the algorithm untouched. The comment at L51–53 explaining why the seam exists ("the teardown races cannot be scheduled deterministically from outside") is the kind of documentation that prevents the next person from "helpfully" re-privatizing them. Chef's kiss.DisposeComponentsAsyncis the correct bUnit primitive for the teardown tests — it synchronously disposes the rendered component tree, which is precisely the "circuit tears down" event the bridge'sDisposemodels. Verified it's a real API in bUnit 2.7.2.UnsubscribeFromAllActions(this)adoption (non-blocking #1) makes the lifetime owned by the test, not incidental to bUnit teardown. Clean.CoalesceMillisecondsdoc-comment rationale (non-blocking #2) — "200ms sits between a fanned-out run's worker-thread settle time (sub-ms bursts) and what a human notices as lag" — is exactly the kind of why-not-what comment I asked for. Future tuners will thank you.a2aa7d9..a8e54f4forRunChangedBridge.csand stripped comments: the only code changes areprivate→internalon two methods. Everylock, every guard, theInvokeAsyncmarshal, the dispose ordering — byte-identical. The coalescing design I praised last round stands unchanged.Merge it. fufu~ the Fluxor adoption is shaping up beautifully — two slices down, the bible page next~ I'll be watching ♡
Automated review by Jibril · 2026-07-25
CI/CD: forgejo-actions coverage comment 3814 present for head
a8e54f4(RunChangedBridge 94.1%/91.6%) · Local checks: build 0 warnings/0 errors (submodules 86d8b22/9544ff2), BlazorAdapter 100/100 pass, fresh coverage collected (RunChangedBridge 93.1%/91.6%, RunMonitorState 100%/100%, RunMonitor.razor 100%/97.9%).