Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): take the running turn from the run, not from session status - #1987
Merged
Conversation
…atus Sending a message did not show Stop or "正在处理…" until the reply began, and the state flickered mid-turn. "Is a turn running" is a fact about the live process, but the UI was reading it off `SessionHeader.status`, which is three steps removed from that fact: - it is written only at the END of `AgentRun.begin`, and nothing announced it — no SessionEvent marks a turn's START, only its end; - it carries no turn identity and reads the same (`active`) before a turn starts and after it ends; - it is persisted, so a crash between a turn's end and its status write leaves `running` behind for good. The renderer armed a live-turn projection at send with no lag, then ANDed it with that status, so the send opened nothing until a status round-trip landed. Worse, any session list resolving inside that window looked byte-identical to one taken after the turn ended, so `settledSessionTransientIds` retired the arm outright — the first content event then rebuilt it as `'streamed'`, silently downgrading the prominent "正在处理…" to the calm "继续中…". Replace the AND with two witnesses that cannot veto each other: - the local arm answers for the turn this renderer sent. It carries an `unconfirmed` bit until the authority says something about that exact turn, which is what stops a snapshot older than the send from retiring it. `onRunStarted` now broadcasts a `sessions:changed` naming the turn — the earliest seam at which the run is live and anything can say so — and `SessionChangedEvent.turnId` makes it an answer to a specific send rather than a bare invalidation. - `SessionSummary.runningTurnId` answers for a turn this renderer did not send: another client, an automation, or one still running across a reload, none of which could show Stop before. It is projected from the live run and never persisted, so a restart reports the truth by itself rather than inheriting a stuck `running`. Read only when it names a turn other than the arm's — for the arm's own turn the local projection knows more, having seen the terminal event first. This also removes `markSessionRunningOptimistic` and its four rollback sites: the optimistic flip lived in the wholesale-replaced session list, so any refresh erased it, and the rollback could revert a genuinely running status.
… control on it
Follow-up to the two-witness change, from review.
`runningTurnId` was a single value, which is the same dimension collapse
the change is arguing against: a session can carry concurrent runs, and
"is anything OTHER than the turn I sent still running" cannot be
answered from an arbitrary one of them. With turn A ended locally but
not yet unregistered, a sibling B genuinely running would read as
`runningTurnId === armedTurnId` and drop Stop. Now `runningTurnIds`.
Three places were still deciding "is a turn running" from the persisted
status the change had just demoted:
- The permission / Plan / Swarm / Graph gates in AppShell. Deleting
`markSessionRunningOptimistic` left them reading `status === 'running'`,
so through the whole send→run-start window — seconds on a cold backend
activation — they were live again. A mode change landing there alters
the execution config of the turn already sent. They read `turnActive`
now, the same witness Stop reads.
- `settledSessionTransientIds`, which can now be wrong in both
directions: a status that has not caught up, and one a crash left
behind. The live runs decide first.
- `sessions:stop`, which named no turn. Stopping is the one turn ending a
client can be waiting on without having seen the turn start, so an
unnamed stop left that claim with nothing to release it — Stop, unable
to undo Stop.
Also narrows the `SessionChangedEvent.turnId` contract text to what is
actually guaranteed. It claimed every single-turn change names its turn;
a linked child agent's turns do not, and correctly so — no client
submitted them and none is waiting on them. They are reported by
`runningTurnIds` instead.
The refusal path in `streamEvents` throws synchronously, and that is
load-bearing: it carries the failure out through the caller's `void`, so
the client disarms in its own catch rather than sitting on `{ ok: true }`
with a claim nothing can confirm. Made async it would be swallowed. Now
pinned by a test.Astro-Hanforce-pushed
the
fix/desktop-turn-running-arm-authority
branch
from
August 3, 2026 11:13
b8b9d2c to
97e3448CompareAstro-Han
marked this pull request as ready for review
August 3, 2026 11:25
Uh oh!
There was an error while loading. Please reload this page.
Astro-Han added a commit
that referenced
this pull request
Aug 3, 2026
Three of these tests could not fail for the reason they named.
The settled-turn snapshot case built `{...armLiveTurn(id), terminal: true}`,
a projection the reducer cannot produce: `complete` and `abort` both return
`undefined` for a turn with no steps. It also only asserted on fields, while
the two #1987 Stop witnesses actually meet one layer up, in `useShellLiveTurn`
— which had no behavioural test at all. Build the projection from a real
`text_delta` + `complete` and assert on `turnActive` through that adapter, so
dropping `turnId` on the way in fails here.
The render-boundary case selected through a COPY of the shell's snapshot
selector, the very pattern this branch removed elsewhere. Fold its positive
assertions (the arm and the first token each cost one render) into the case
that drives `useAppShellSessionUiReads` itself, which was asserting only that
deltas cost nothing — leaving the snapshot comparator free to be gutted.Astro-Han added a commit
that referenced
this pull request
Aug 3, 2026
…surface reads (#1998) * perf(desktop): give session UI state per-subscriber notification `createAppShellSessionUiStateController` is an external store, but it only ever had a single `onChange` wired to one `forceRender()`. Add `subscribe` and a selector hook so a component can follow one derived reading of the store instead of every write to it (#1985). The selector caches on a caller-supplied equality: `useSyncExternalStore` requires a snapshot that keeps its identity while nothing it selects changed, so a selector deriving a fresh object must say what "unchanged" means for it. Without that the shell's own snapshot selector loops rather than merely over-rendering, which the new contract test pins. `LiveTurnSnapshot` is the low-entropy reading of a live turn — phase, a few booleans, the settled message id — everything the shell derives from the active projection except the streamed content itself. A text delta cannot change it. The aggregate top-level subscription still stands; moving its readers is the next commit. * perf(desktop): move session UI reads to the boundary that owns them AppShell destructured the whole session UI store, so a write to any slice re-rendered the entire shell — including one write per streamed token. The subscription now matches what each surface reads (#1985): - AppShell selects the six low-frequency maps by raw reference, plus a `LiveTurnSnapshot` and the sidebar's pulse set by value. None of them change when a delta grows the streamed text. - ChatMessageSurface subscribes to the projection and the shell-run record itself. It is their only renderer, so they never reach the shell. - The per-delta reconcile moves into <LiveTurnReconciler/>, which follows every delta and owns no subtree. `useShellLiveTurn` now takes the snapshot rather than the projection, and `deriveModelWait` takes booleans — it only ever asked whether the buffers were empty. This needs no `memo`: the sidebar and composer stop re-rendering because their parent does, not because a comparison blocks them. * fix(desktop): pass the stable action to LiveTurnReconciler, not an effect event `useEffectEvent` returns a fresh identity every render and must not cross a component boundary or enter a dep array. `reconcilePersistedMessages` already comes from `useStableActions`, whose whole purpose is a fixed identity bound to the latest committed render — the wrapper made the dep array dishonest and re-ran the reconcile for unrelated shell renders. * refactor(desktop): name the shell's read of session UI state, and memoize it Three things the review surfaced, one cause: the selector seam was shaped so that neither the compiler nor the tests could see what the shell reads. - `useAppShellSessionUiReads` is now that list, in one place. The contract test drives the hook itself, so adding a token-rate selection to it fails the test — before, the test asserted against a copy of the list and a real regression in AppShell would have gone green. - Selectors are module-level and take what they vary by as `arg`, so the snapshot is memoized rather than published through a render-phase ref write. React permits that write only for lazy initialization; a discarded concurrent render would otherwise hand its selector to the committed subscription. `arg` also makes the activeId switch explicit, now covered. - `LiveTurnSnapshot.streamingTextComplete` is gone: `streamingMessageId` is set only when the text step completed, so its presence already carried that fact. The freed field is `turnId`, which `deriveTurnActive` needs. * test(desktop): pin the two live-turn snapshot facts the shell depends on A settled turn must drop its phase but keep its id — `deriveTurnActive` reads the first to retire this renderer's arm and the second to tell a sibling turn apart from it. And the handoff message id must stay absent while the text step is open, since its presence is what says the step closed. * fix(desktop): key the session UI snapshot cache by store state `useSyncExternalStore` reads a snapshot several times for one store state — in the subscription callback, during render, and again in a passive effect — and demands the same value each time. The cache only compared the previous VALUE, so a selector deriving a fresh object handed React a new identity on every call and force-rendered forever; the only thing standing between the app and a freeze was every caller remembering to pass `isEqual`. Key the cache by the state it derived from. Idempotence per store state now belongs to the one adapter that connects arbitrary derivations to the store, `isEqual` drops to what it should have been (carrying a value's identity ACROSS a state that did not change the selection), and each selector runs at most once per store change instead of once per read. * refactor(desktop): share one live-turn selector between its two subscribers `selectLiveTurn` was defined word-for-word in both the chat surface and the reconciler, so changing one would silently leave the other behind. Keep it with the rest of the session-UI selectors, where the snapshot selector can build on it too. `deriveLiveTurnSnapshot` also allocated a flattened tool array per call just to ask two yes/no questions of it. * test(desktop): drive the live-turn contracts through the real adapter Three of these tests could not fail for the reason they named. The settled-turn snapshot case built `{...armLiveTurn(id), terminal: true}`, a projection the reducer cannot produce: `complete` and `abort` both return `undefined` for a turn with no steps. It also only asserted on fields, while the two #1987 Stop witnesses actually meet one layer up, in `useShellLiveTurn` — which had no behavioural test at all. Build the projection from a real `text_delta` + `complete` and assert on `turnActive` through that adapter, so dropping `turnId` on the way in fails here. The render-boundary case selected through a COPY of the shell's snapshot selector, the very pattern this branch removed elsewhere. Fold its positive assertions (the arm and the first token each cost one render) into the case that drives `useAppShellSessionUiReads` itself, which was asserting only that deltas cost nothing — leaving the snapshot comparator free to be gutted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sending a message did not show Stop or "正在处理…" until the reply began, and the state flickered mid-turn.
"Is a turn running" is a fact about the live process, but the UI read it off
SessionHeader.status, which is three steps removed from that fact:AgentRun.begin, and nothing announced it — no SessionEvent marks a turn's START, only its end;active) before a turn starts and after it ends;runningbehind for good.The renderer armed a live-turn projection at send with no lag, then ANDed it with that status, so the send opened nothing until a status round-trip landed. Worse, any session list resolving inside that window looked byte-identical to one taken after the turn ended, so
settledSessionTransientIdsretired the arm outright — the first content event then rebuilt it as'streamed', silently downgrading the prominent "正在处理…" to the calm "继续中…".This replaces the AND with two witnesses that cannot veto each other:
unconfirmedbit until the authority says something about that exact turn, which is what stops a snapshot older than the send from retiring it.onRunStartednow broadcasts asessions:changednaming the turn — the earliest seam at which the run is live and anything can say so — andSessionChangedEvent.turnIdmakes it an answer to a specific send rather than a bare invalidation.SessionSummary.runningTurnIdanswers for a turn this renderer did not send: another client, an automation, or one still running across a reload, none of which could show Stop before. It is projected from the live run and never persisted, so a restart reports the truth by itself rather than inheriting a stuckrunning. It is read only when it names a turn other than the arm's — for the arm's own turn the local projection knows more, having seen the terminal event first.Also removes
markSessionRunningOptimisticand its four rollback sites: the optimistic flip lived in the wholesale-replaced session list, so any refresh erased it, and the rollback could revert a genuinely running status.Verification
npm run typecheck,npm run lint,npm run format:check— clean.@maka/core740 pass,@maka/ui262 pass,@maka/desktop1361 pass — 0 fail.@maka/runtime2760 pass / 4 fail. The 4 are pre-existing and unrelated (builtin-toolspath containment, failing identically on a cleanmaincheckout: macOS resolves the temp root as/private/var/...while the test passes/var/...).New coverage for the seams this depends on:
unconfirmedclaim's full lifecycle, including that any event about the turn clears it (@maka/ui);Review focus
SessionChangedEvent.turnIdandSessionSummary.runningTurnIdare contract additions inpackages/core. Both are optional, and the emitter obligation for each is stated at its declaration.runningTurnIdis populated on session LISTS only — a summary returned by a mutation describes the header alone.