summaryrefslogtreecommitdiffhomepage
path: root/report.md
diff options
context:
space:
mode:
authorAdam Malczewski <[email protected]>2026-05-28 08:21:23 +0900
committerAdam Malczewski <[email protected]>2026-05-28 08:21:23 +0900
commitd2e2e67425e5106025ee8082a0768989b5de814f (patch)
tree831858182b4b3083beeb0cfa84968b5b73ded575 /report.md
parent2f14260bb0f1a51d51e516feda285b68f793ae1b (diff)
downloaddispatch-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.md53
1 files changed, 24 insertions, 29 deletions
diff --git a/report.md b/report.md
index 7ec0a40..ba9d466 100644
--- a/report.md
+++ b/report.md
@@ -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).