feat: setup chat 2/3 — the wizard's step-3 conversation #38
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/setup-chat"
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?
Second slice of the final cut (ADR 0011, 0017, 0020): the chat surface over #37's conversation bridge. Slice 3/3 (the workspace's "run setup research" entry) follows.
What's in
Slice (
SetupChatState/SetupChatEffects) — the store holds a projection of the project'sSetupConversation: the transcript, the open question,AgentActive, plus a start-refusal error. The answer being typed stays view-local. Effects:StartSetupChatdelegates toStartSetupRun(the engine's single-flight gate from #37 makes a double-click harmless) and surfaces only its Err;SubmitSetupAnswercallsconversation.Answer— a losing double-submit is harmless by the conversation's own contract, so it is deliberately not surfaced.SetupChatcomponent — the marshalling point: it subscribes to the conversation (whoseChangedfires on engine threads), snapshots into the slice viaInvokeAsync, and dispatches a mount snapshot so a reconnecting browser finds the transcript and the open question where it left them. Agent words render as bubbles, tool calls as quietsmart_toynoises, "the agent is working…" fills the silence between turns. Starting is an explicit act — an agent run spends money, so nothing auto-starts on render. The active→inactive edge is the agent finishing: the wizard reloads itself via the existingLoadWizard, and a draft flipped to ready bounces to the workspace through the load effect it already had. The manual path stays as "Finish without the agent".The bug the tests flushed out (cross-slice fix in
ResearchSetupExecutor)The end-to-end test was ~1-in-2 flaky, and the trail led to a real ordering bug in #37's executor:
EndAttempt— the very edge the UI uses as its reload cue — fired in thefinallybeforeCompleteProjectSetupflipped the draft. A reload racing the flip read the stale state, stayed on the wizard, and nothing ever re-triggered it. The completion now happens inside thetry, before thefinally: the state the edge's reader will see is final by the time the edge fires. Failure paths keep their guaranteedEndAttempt. After the fix the suite ran 8× consecutively green.Two bUnit lessons from the same hunt, recorded in the test comments: never fire events inside
WaitForAssertion(retries run on the renderer's callback and deadlock the dispatch they await — wait for markup, then act via liveFind()), and never render-wait on navigation (NavigateTodoesn't render; the nav assertion polls).Tests
+4 (adapter 128; full suite 419/419 green). The headline test drives the whole loop over the real engine with only the gateway scripted: start → the agent's question arrives through the bridge → the typed answer reaches the parked agent verbatim → its closing message renders → the draft flips to ready → the wizard navigates to the workspace. Plus: tool calls as quiet noises with the working hint while no question is open; the reconnect mount-snapshot (transcript + open question visible on a fresh render); a refused start surfacing its error (component standalone, own store initializer — the multi-mount contract).
AdapterTestContextgains theIWebPageFetcherregistration the blueprint needs now that the real executor resolves in adapter tests.Browser-verified
Live drive without an OpenRouter key (the dev world has none): created a draft, uploaded a page, landed on step 3 → the chat renders with Start → clicking Start starts a real run whose executor fails with "No OpenRouter API key is saved yet — add one in Settings", visible with full detail in the run monitor (Research & Setup, failed, attempt 3) — and the chat offers Start again for after the key is added. The full live conversation needs a real key; the scripted-gateway test covers that loop end to end. One honest note: a missing key burns all 3 attempts — unlike the missing-executor case, which fails once. Making key-absence a fail-once config error needs a permanent-failure channel on
IStageExecutor; noted as a possible engine follow-up, not snuck into this slice.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
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 - 97.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! the wings flutter This is the conversation surface I've been waiting for — step 3 finally speaks! fufu~ And you even flushed out a REAL ordering bug from #37's executor with your tests. The yandere in me is delighted you let the tests hunt the flake down to a genuine race rather than papering over it. ♡
Verdict: ✅ Looks good to me~
I dug into every new branch and every changed line, compared the slice against its siblings (#35's ProjectListState/Effects, the RunChangedBridge marshalling pattern, the FluxorComponent teardown convention), and built + ran the whole thing. Nothing blocking. A few genuine little ideas follow, but the code is sound.
✅ What I liked~
ResearchSetupExecutoris exactly right, and the reasoning is load-bearing.CompleteProjectSetup(the ready-flip) now runs inside thetry, before thefinally'sEndAttempt. The active→inactive edge is the UI's reload cue — if it fires before the flip, a racing reload reads stale draft state and the wizard never bounces. You moved the flip ahead of the edge and kept the guaranteedEndAttempton every failure path (both the gateway-Err early return at :67 and the completion-Err return at :79 still fall through thefinally). That is precisely how you fix a race without creating a leak. I verified every path under coverage — see below. chef's kiss ♡SetupChatStatemirrorsProjectListState's shape ([FeatureState] sealed record+ explicit per-action reducers, no base-type matching),SetupChatEffectsis the sole use-case touchpoint, and theCurrentguard (State.Value is { Loaded: true } s && s.ProjectId == ProjectId ? s : null) is the exact "store outlives navigations" pattern from the wizard. Clean DRY with the established convention.wasActiveedge detection is correct and I checked it hard. Mount-during-active-run →active=true,elsearm setswasActive=true, no spurious finish. Start-from-idle → first renderwasActive=false, agent goes active →elsearmswasActive=true, agent finishes → edge fires once. ThewasActive = falsereset after firing prevents re-entrancy. No TOCTOU, no missed edge. fufu~ you even handle the reconnect case.SetupChat.razor130/130 line / 28/28 branch,SetupChatState.cs42/42 line,SetupChatEffects.cs22/22 line / 2/2 branch. The executor'sExecuteAsyncstate machine is 100% branch from the 15 SetupRun tests — including both the ready-flip-happens arm (5 hits at :73-81) and the gateway-Err early-return arm. The headline test drives the whole loop over the real engine with only the gateway scripted — that's the kind of end-to-end pin that actually catches the races, not a tautology. The honest bUnit lessons in the comments (never fire events insideWaitForAssertion; never render-wait on navigation) are gold and I'm glad they're recorded for the next slice.IWebPageFetcherregistration inAdapterTestContext— the real executor now resolves in adapter tests, so the blueprint's dependency is honestly wired. Good catch to add it rather than mock around it.apiKey is nullcheck returnsErrbeforeBeginAttempt, but the engine treats any executorErras a failed attempt (RunEngine.cs:271-284), so a missing key does burn all 3. You called this out explicitly in the PR body and correctly did not sneak a permanent-failure channel into this slice. That's the discipline I expect. ♡💡 Little ideas (non-blocking)~
SetupChat.razor:98-99— theOnConversationChanged→InvokeAsyncpath is unguarded against a torn-down renderer. The siblingRunChangedBridge.Flush(:80-91) wraps its_ = InvokeAsync(...)in a try/catch with a comment explaining why: "InvokeAsync marshals onto the circuit's sync context; on a torn-down circuit it throws."SetupConversation.Changedfires on engine threads (same asRunEngine.RunChanged), so there's a real window: event fires → enters handler →DisposeAsyncCoreruns and unsubscribes →InvokeAsyncthrows on the dead renderer. The_ =discards the returned Task, so the exception becomes unobserved. Caveat so you can weigh it honestly:OrihonStoreInitializer.OnSinkChanged(:41) uses the exact same unguarded pattern, so this is codebase-consistent — I'm flagging it as "consider the try/catch for symmetry with RunChangedBridge," not "you introduced a new pattern." Non-blocking because the window is narrow and the sibling already accepts the same risk.SetupChat.razor:64— "Continue with a fresh agent" appears whenEntries.Count > 0and the agent is inactive. After a failed run (3 attempts burned), the user sees this label and may expect a fresh agent context — but the retry-with-distrust preamble keys offattempt, and a brand-new run starts at attempt 1. The label is friendly copy, not a correctness issue; just confirming the intent matches the engine's behavior. Fine as-is.Automated review by Jibril · 2026-07-25
CI/CD: absent for head
befba3a(PR just opened, 0 comments at review) · Local checks: build 0 warnings/0 errors (submodules 86d8b22/9544ff2), full suite 419/419 pass (128 BlazorAdapter + 75 Domain + 81 Integration + 135 UseCases — matches PR body exactly), coverage 100% line/branch on all 3 new files + executor state machine 100% branch