diff options
| author | Adam Malczewski <[email protected]> | 2026-05-28 08:21:23 +0900 |
|---|---|---|
| committer | Adam Malczewski <[email protected]> | 2026-05-28 08:21:23 +0900 |
| commit | d2e2e67425e5106025ee8082a0768989b5de814f (patch) | |
| tree | 831858182b4b3083beeb0cfa84968b5b73ded575 /report.md | |
| parent | 2f14260bb0f1a51d51e516feda285b68f793ae1b (diff) | |
| download | dispatch-d2e2e67425e5106025ee8082a0768989b5de814f.tar.gz dispatch-d2e2e67425e5106025ee8082a0768989b5de814f.zip | |
feat: restore tab layout + in-flight chunks on browser reopen; agents keep running in background
Implements the 'background-running agents + restore-layout-on-reopen'
feature. Full design and parallel-implementation plan in
`plan-bg-restore.md`; Gemini code review (SHIP verdict, no findings) in
`report.md`.
User-visible behaviors:
1. Browser-close keeps agents alive. If an agent is mid-stream when
the browser closes / reloads / loses the network, it continues
processing on the backend. (This was already the case in code —
agents run fire-and-forget in app.ts:77-79 — but it was previously
pointless because the UI never restored the tab to receive the
output.)
2. Layout restore on browser reopen. Every tab that existed at the
time the window was closed is restored, in original `position`
order, with full persisted message history. Tabs whose agents
finished while disconnected appear with the completed message.
Tabs whose agents are still running appear streaming live — the
in-flight assistant message is reconstructed from the backend's
in-memory `currentChunks` (sent over the wire on connect) and
accumulates new deltas as they arrive.
3. Explicit tab-close cancels + forgets. Clicking the X still
cancels the agent (existing `stopTab` in DELETE /tabs/:id) and
archives the row (`is_open = 0`), so it is not restored. No
change to that path.
The gap that the implementation closes: previously, App.svelte:onMount
unconditionally called `createNewTab()` with a fresh UUID, ignoring
every existing row in the `tabs` table. Every browser open was a
clean slate. The DB had the conversation history but no way for the
UI to discover it.
Implementation:
• New `TabStatusSnapshot` interface in
packages/core/src/types/index.ts (auto-exported via existing
`export * from "./types"`):
interface TabStatusSnapshot {
status: AgentStatus;
currentChunks?: Chunk[]; // present iff running
currentAssistantId?: string; // present iff running
}
• `agent-manager.ts:getAllStatuses()` rewritten to return
`Record<string, TabStatusSnapshot>` (was
`Record<string, AgentStatus>`). For running tabs only, attaches a
defensive shallow copy of `tabAgent.currentChunks` (the live
streaming array the per-message loop appends to) plus the DB id
of the in-flight assistant message. The defensive copy is the
consumer's to mutate. Idle / error tabs get `{ status }` only.
`GET /status` and the WS `onOpen` snapshot both pick up the new
shape automatically — neither call site changed.
• Frontend mirror of `TabStatusSnapshot` in
packages/frontend/src/lib/types.ts; `AgentEvent.statuses` variant
updated to use `Record<string, TabStatusSnapshot>`.
• New `hydrateFromBackend()` on the tab store
(packages/frontend/src/lib/tabs.svelte.ts). Sequence on app
mount:
1. Bail with 0 if `tabs.length > 0` (hot-reload idempotency).
2. GET /tabs → list of `is_open=1` rows in
`position` order.
3. GET /status → in-flight TabStatusSnapshot map.
4. GET /tabs/:id/messages for each tab in parallel via
Promise.all → persisted ChatMessage[].
5. Build the Tab objects, splicing the snapshot's live
chunks into the in-flight assistant message for every
running tab (two paths: merge into the existing DB row
with matching id, or append a fresh in-flight message
if no row matches).
6. `tabs = restored; activeTabId = restored[0]?.id ?? null;`
Every fetch is wrapped in try/catch so one tab's failure can't
destroy the whole restore pass.
• WS `statuses` handler in `tabs.svelte.ts:handleEvent` rewritten
for the new shape. Still fires `reloadTabMessagesFromApi` on the
desync case (frontend thinks running, backend says idle — the
pre-existing recovery path is preserved). When backend says
running, seeds in-flight chunks into the assistant message
matching `snap.currentAssistantId` (creating it if needed).
When backend says non-running, clears `isStreaming` on the
previous in-flight message and nulls `currentAssistantId`.
• `App.svelte:onMount` now awaits `tabStore.hydrateFromBackend()`
before deciding whether to fall back to `createNewTab()`.
Fallback condition is the doubly-defensive
`restored === 0 && tabStore.tabs.length === 0`. `wsClient.connect()`
fires in parallel with hydration — the resulting WS `statuses`
event is per-tab idempotent against the hydrated state, so there
is no race even if it arrives mid-hydration.
What was NOT done (deliberately, deferred to wishlist):
• Pre-existing inconsistency: core `AgentStatus` includes
"waiting_for_key" but frontend `TabStatusSnapshot.status` uses
only the existing 3-state pattern ("idle" | "running" | "error").
Not introduced here; mirrored the existing precedent.
• Restored tabs use defaults for `reasoningEffort`, `agentSlug`,
`agentScope`, `agentModels`, `workingDirectory` — these are not
in the DB `tabs` schema. Future schema expansion.
• Per-delta DB flushing — not needed; the in-memory snapshot
covers the gap between flushAssistant calls.
• LocalStorage cache of tab ids — backend DB is the source of truth.
Process notes:
• Implemented via parallel programmer subagents (flash agents were
requested but unavailable in this environment — substituted with
"programmer" agents, which share the "reads a plan, implements a
single step" charter). Backend (Segment A: getAllStatuses + 5
tests) and frontend (Segment B: types + hydrateFromBackend +
statuses handler + onMount + 8 tests) ran disjoint-file-ownership
in parallel.
• Gemini code review (yolo mode for tool access, explicit
prompt-level write restriction to `report.md` only) returned a
SHIP verdict with no findings against the plan.
• Self-review surfaced one followup gap that Gemini's earlier
plan-mode pass also caught: no explicit test for
`/tabs/:id/messages` failure isolation. Added a test covering
both HTTP-500 and network-error variants alongside a healthy
tab, asserting per-tab failures don't destroy the whole restore.
Tests:
• api/tests/agent-manager.test.ts: +5 (snapshot empty record,
idle-tab field omission, running-tab field inclusion, defensive
copy invariant, omits chunks for running tab with null
currentChunks). 31 total (was 26).
• frontend/tests/chat-store.test.ts: +9 (restore-with-messages,
in-flight seeding, /tabs failure → 0 returned, empty /tabs
array, idempotency when tabs already exist, idle-status when
/status omits, running-snapshot statuses handler seeding,
idle-snapshot statuses handler clearing, per-tab failure
isolation across HTTP-500 and network-error). 44 total (was 35).
Totals: 243 tests across 3 packages all green; typecheck clean on
core + api + frontend; biome clean across 124 files.
Diffstat (limited to 'report.md')
| -rw-r--r-- | report.md | 53 |
1 files changed, 24 insertions, 29 deletions
@@ -1,36 +1,31 @@ -# Code Review — DeepSeek `reasoning_content` fix +# Background Agents + Layout Restore Review ## Verdict -**Ship-with-followups.** The removal of the v4-era middleware correctly resolves the primary bug: `reasoning_content` is now passed to the AI SDK as `{ type: "reasoning" }` parts, which the v6 SDK successfully serializes to the wire format instead of burying it in a dead `providerMetadata` key. However, the fix strictly relies on the AI SDK's native serialization, which introduces a critical edge case for empty reasoning blocks that the reference OpenCode implementation explicitly handles. +SHIP -## Bugs found (must-fix) -*(None blocking the immediate happy-path fix, but see edge cases below)* +## Block-level findings +None. The implementation adheres strictly to the `plan-bg-restore.md` specification across all tiers (core, API, and frontend). -## Edge cases / risks -* **The Empty-Reasoning Edge Case:** DeepSeek (and other models) can occasionally emit an empty reasoning block. In Dispatch, `toModelMessages` will correctly emit `{ type: "reasoning", text: "" }`. However, `@ai-sdk/[email protected]` strips `reasoning_content` entirely if the text is empty: - ```javascript - // node_modules/@ai-sdk/openai-compatible/dist/index.mjs:245 - ...reasoning.length > 0 ? { reasoning_content: reasoning } : {}, - ``` - If DeepSeek emitted an empty reasoning block in a previous turn, the SDK will drop `reasoning_content` from the subsequent request payload. DeepSeek requires this field to be present (even if empty) during a thinking-mode session and will crash with the exact same "must be passed back" error. +## Ship-with-followup findings +None. -## opencode reference parity gaps -* **Forced empty string serialization:** The OpenCode reference implementation specifically works around the SDK's empty-string stripping behavior in `packages/opencode/src/provider/transform.ts:233-241`: - ```typescript - // Always set the field even when empty — some providers (e.g. DeepSeek) may return empty - // reasoning_content which still needs to be sent back in subsequent requests. - ``` - It enforces this by stripping the `reasoning` parts and injecting the `providerOptions.openaiCompatible[field]` directly. Dispatch's fix misses this workaround because it relies 100% on the AI SDK's native serialization. +## Nits +None. -## Test coverage gaps -* **Empty Reasoning Handling:** The new `agent.test.ts` integration test ("openai-compatible reasoning round-trip...") only tests the happy path with non-empty reasoning (`text: "let me reason about this"`). It should include a test asserting that `text: ""` still results in a valid request (which currently would fail against the wire, though testing this fully might require mocking the provider wire payload rather than just `streamText` arguments). -* **Tool calls + Reasoning:** There is no test verifying that an assistant message with both reasoning and a tool call correctly passes both to `streamText` for the `openai-compatible` provider (though the logic in `toModelMessages` handles it). -* **Multiple reasoning blocks:** The AI SDK concatenates multiple reasoning parts natively, but an integration test verifying that Dispatch's `toModelMessages` correctly emits all of them would be valuable. +## What was checked +- **A. getAllStatuses correctness**: PASS. The method in `packages/api/src/agent-manager.ts:714-751` returns `Record<string, TabStatusSnapshot>`. It conditionally includes `currentChunks` and `currentAssistantId` only for `running` tabs and performs a defensive shallow copy `[...tabAgent.currentChunks]`. +- **B. Frontend TabStatusSnapshot mirror**: PASS. `packages/frontend/src/lib/types.ts:96-100` mirrors the core interface exactly. +- **C. hydrateFromBackend**: PASS. `packages/frontend/src/lib/tabs.svelte.ts:432-562` implements the full hydration flow (GET /tabs -> GET /status -> parallel GET /tabs/:id/messages). It handles failure modes without throwing, implements the required idempotency check (`tabs.length > 0`), and correctly seeds in-flight messages. +- **D. WS statuses handler**: PASS. `packages/frontend/src/lib/tabs.svelte.ts:581-644` reconciles status, seeds in-flight chunks for running tabs, and clears pointers for idle tabs. The desync recovery path (`reloadTabMessagesFromApi`) is preserved. +- **E. App.svelte onMount**: PASS. `packages/frontend/src/App.svelte:78-105` sequences `hydrateFromBackend` before the fallback `createNewTab` and only creates a fresh tab if hydration yields nothing. WS connection lifecycle is preserved. +- **F. Behavior preservation**: PASS. No new `beforeunload` or `unload` handlers were added. WS `onClose` in `packages/api/src/index.ts:60-66` correctly only unsubscribes. Explicit tab close in `tabs.svelte.ts` still calls `DELETE /tabs/:id`, which cancels and archives as before. +- **G. Test coverage**: PASS. + - API tests in `agent-manager.test.ts` cover empty state, idle snapshot, running snapshot, and defensive copy. + - Frontend tests in `chat-store.test.ts` cover successful restore, in-flight seeding, failure tolerance, and idempotency. +- **H. Race conditions**: PASS. `hydrateFromBackend` idempotency and the per-tab reconcile logic in the WS handler mitigate potential races between HTTP and WS data. +- **I. Wire-shape symmetry**: PASS. The frontend mirror matches the core definition. +- **J. Type-only vs runtime imports**: PASS. `agent-manager.ts:40` uses `type TabStatusSnapshot`. -## Style / consistency -* **Clean removal:** The deletion of the obsolete middleware and the cleanup of `packages/core/src/llm/provider.ts` is idiomatic and cleanly delegates responsibility to the v6 AI SDK. -* **Comments:** The new block comment in `provider.ts` accurately explains why the old middleware broke DeepSeek and why the SDK handles it natively now. - -## Other observations -* **Redacted Reasoning:** The removed middleware stripped `redacted-reasoning` types. This is no longer necessary because Dispatch's `toModelMessages` only knows how to emit `text`, `reasoning`, and `tool-call` parts; it intrinsically filters out any other internal chunk types. This means no regressions are introduced regarding redacted reasoning. -* **Empty part filtering for Anthropic:** The `applyAnthropicStructuralNormalisations` function correctly strips empty reasoning/text parts, but this is safely gated behind `usesAnthropicSDK`, so it will not interfere with DeepSeek's `openai-compatible` path.
\ No newline at end of file +## What was NOT checked +- Performance with extremely high tab counts (>100) was not verified empirically, though the implementation uses `Promise.all` for message fetching to minimize latency. +- Direct database state verification (the review was limited to code analysis and existing test coverage). |
