Repository navigation
refactor(chat): move the turn lifecycle into a Main-process ChatStreamManager - #145
Merged
Merged
Conversation
…mManager Second step of #138, closing #140. #144 made the turn's semantics correct; this makes them live in one owner instead of in an IPC handler's closure. The live state of every running turn used to be: const activeStreams = new Map<string, AbortController>() plus a handful of local variables in `send-message` — the accumulated text, the reasoning, the usage, the settled outcome, and four closures (`settle`, `finishTurn`, `accountTurnTokens`, `announceOutcome`) that only existed for the duration of one call. Nothing could answer "what is this turn doing", nothing could be reached after the handler returned, and every future feature on the roadmap — a timeout, a retry, attach-and-replay, a renderer overlay — would have had to add another closure here. Now: | Role | What it is | | --- | --- | | The turn's state and rules | `ChatExecution` — one object per turn: accumulated answer, provider reason and usage, and the one terminal outcome | | The registry and lifecycle | `ChatStreamManager` — one entry per running turn, and the only place a turn is settled, persisted and announced | | Persistence port | `ChatTurnStore`, implemented by `services/chat/turnStore.ts` over `db/queries` | `chatHandlers.ts` is 260 lines shorter and assembles the prompt — user message, history, RAG, provenance — then hands the turn over. Every ending goes through one path in the manager: a stop the renderer asked for, a stream that failed or finished or just broke, and a turn that could not start at all. Three things are deliberate: - **The manager imports neither `db/queries` nor Electron.** It writes through the store port and reports through an `emit` callback supplied per turn, which is why `test/chatStreamManager.test.ts` can drive a complete turn — chunks in, one persisted record out — with no database and no Electron process. The portable `estimateTokens` moved from `SessionAutoSwitchService` to `shared/utils/tokenEstimate.ts` for the same reason: it is a pure function, and it was only unreachable outside Electron because it lived on a class that needs a database. - **`abort` settles before it cancels.** The `AbortError` the SDK returns afterwards is then noise after a decision rather than a second opinion about it, which is what the issue asked for and what a test now pins. - **A finished turn is removed from the registry before the renderer is told**, so a late signal cannot find it and cannot start a second life for it. Not in this PR, per the issue's scope: the chunk-stream protocol (#141), the renderer overlay (#142), retries, Continue, and the output-ceiling UI. One part of the issue is deferred rather than done — `seq` and the subscriber/attach API. A sequence number nobody reads is structure without a consumer, and #141's envelope is where it gets one; the buffer exists for the same reason. #140's checklist is updated to say so. Two behaviour notes: a failed or aborted turn now stores the *grounded* citation subset rather than the whole evidence list (the same #70 rule delivered answers already had), and a turn that cannot start writes its provenance before failing. Verified: `npm test` 306 pass (9 new in `test/chatStreamManager.test.ts`, which drive the real manager through the real `ModelClient` with a scripted provider — including an abortable stream that errors on the signal the way a transport does), `npm run typecheck`, `npm run check:design`, `npm run build:unpack` + `smoke:packaged` (23 checks), and the packaged smoke still passes after the message table's new columns.
This was referenced Sep 27, 2026
mrsibe
added a commit
that referenced
this pull request
Sep 27, 2026
… either way (#147) Fixes #146. A failed import used to produce nothing at all — no error, no row, and a panel that still showed its empty state, which reads as "the upload worked and the app is not showing it". #37 was closed with exactly that report and nothing in the code could have told that user what went wrong. Two halves, both needed: **The failure was discarded before it could be shown.** `SourcePanel` calls the store and ignores what it returns. The store reports failure by *returning* `{ success: false, error }` — the IPC layer already catches and describes it — so an unchecked return value makes a failure indistinguishable from a success. `handleFileUpload`, `handleUrlImport`, `handleTextPaste` and `handleNoteImport` all did this. **The list was only re-read when the import succeeded.** `KnowledgeService` inserts the source row and *then* indexes it, so a failure at the indexing step leaves a row with `status: 'failed'` — and `DocumentList` already has the feedback for it, an alert saying the document failed to embed. It never fired, because the list was never re-read. A failure *before* the row exists (an unsupported type, a corrupt or scanned file that extracts nothing) has no row and so needs the message to say so. Now one rule covers every entry point: an import attempt always ends by re-reading the library and recording the reason, and the reason reaches the reader as a toast. The rejection path re-reads too, deliberately: an IPC call can fail after the main process already wrote the source row, so the failure path cannot assume that nothing changed. Not in this PR: recording an attempt that failed before its source row existed (there is still no row for an unparseable file — that is #95's ingestion record), and any retry affordance (#95). The `hasEmbeddingModel` gate that disables `+` is untouched; after #134 the built-in local backend makes it effectively always open. One behaviour note: the empty-note case used to `alert()` from a branch that could not be reached — the store returns `{ success: false }` rather than throwing, so the `catch` never ran. It now shows the same message as a toast, like every other import failure, and names the note instead of printing `Note note_123 not found`. Verified: `npm test` 303 pass (6 new in `test/knowledgeStore.test.ts`, which drive all four entry points through the store with a stubbed `window.api`), `npm run typecheck`, `npm run check:design`, `npx eslint` on the changed files (0 problems), `npm run build`. `tsconfig.test.json` now also includes `src/preload/index.d.ts`: importing renderer code means typechecking it, and that file is where `window.api` is declared — without it a test cannot import a store that legitimately uses the preload global. Not run: `build:unpack` + `smoke:packaged`. Neither main nor preload changed; the packaged checks are in CI for this PR and were run in #145 for the last main-process change. Not hand-verified: the toast itself, which needs the GUI.
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 free
to 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.
Closes #140. Part of the chat execution epic, #138 — #144 made the semantics correct; this makes them live in one owner.
What was still wrong
The live state of every running turn was:
plus a handful of local variables in
send-message— the accumulated text, the reasoning, the usage, the settled outcome — and four closures (settle,finishTurn,accountTurnTokens,announceOutcome) that existed only for the duration of one call. Nothing could answer what is this turn doing, nothing was reachable after the handler returned, and every feature on the roadmap (a timeout, a retry, attach and replay, the renderer overlay) would have had to add one more closure to the same function.What this introduces
ChatExecution—src/main/services/chat/ChatExecution.ts: accumulated answer, the provider's reason and usage, and the one terminal outcomeChatStreamManager—src/main/services/chat/ChatStreamManager.ts: one entry per running turn, and the only place a turn is settled, persisted and announcedChatTurnStore, declared by the manager and implemented byservices/chat/turnStore.tsoverdb/querieschatHandlers.tsloses 260 lines: it assembles the prompt (user message, history, RAG, provenance) and hands the turn over. Every ending goes through one path in the manager — a stop the renderer asked for, a stream that failed or finished or just broke, and a turn that could not start at all (no prompt, no chat model).Three things are deliberate:
db/queriesnor Electron. It writes through the store port and reports through anemitcallback supplied per turn. That is what letstest/chatStreamManager.test.tsdrive a complete turn — chunks in, one persisted record out — with no database and no Electron process. For the same reasonestimateTokensmoved fromSessionAutoSwitchServicetoshared/utils/tokenEstimate.ts: it is a pure function, and it was only unreachable outside Electron because it lived on a class that needs a database.abortsettles before it cancels. TheAbortErrorthe SDK returns afterwards is noise after a decision instead of a second opinion about it — the invariant the issue asked for, now pinned by a test.The events forwarded to the renderer are unchanged, in the same shapes and the same order: changing that protocol is #141's job, and this PR must not.
Deferred, deliberately
seqand the subscriber/attach API. A sequence number nobody reads is structure without a consumer; #141's envelope is where it gets one, and the buffer that would hold replayed events exists for the same consumer. The issue's checklist is updated to say so rather than leaving two boxes unticked and unexplained.Not in this PR either, per the issue: the chunk-stream protocol (#141), the renderer overlay (#142), retries, Continue, and the output-ceiling UI.
Behaviour notes
Verification
npm testtest/chatStreamManager.test.tsnpm run typechecknpm run check:designnpm run build:unpack+smoke:packagednpx eslint <changed files>no-explicit-anywarnings inchatHandlers.tsare pre-existing linesThe new tests drive the real manager through the real
ModelClientwith a scripted provider fromai/test, including an abortable stream that errors on the signal the way a real transport does. They cover the issue's acceptance directly:finisheventpending→streamingon the first chunklengthcompletes astruncatedand is not reported as an errorAbortErrorthat follows cannot re-label the turnabort()returnsfalse) and does not re-label itdocs/architecture.mdgains the seam and its invariants, next to the existing module-seam tables.