docs(chat): v2 chat architecture proposals + reference research#3716
Merged
docs(chat): v2 chat architecture proposals + reference research#3716
Conversation
Plans-only PR. Adds the chat-architecture research and proposals for the v2 chat rearchitect, plus a pragmatic v1->host-service migration plan that unblocks the greenfield work. - v2-chat-greenfield-architecture.md: tRPC subscriptions over WS, EventLog interface, P0-P4 race-fix + LocalEventLog (SQLite), P5a (Postgres) / P5b (Durable Objects), P6 (handoff via git courier), P7 (mid-turn handoff, deferred). - v1-to-v2-fast-migration.md: ship-this-month plan. Move ChatRuntimeService into host-service, collapse two polling queries into one atomic getSnapshot, gate behind per-workspace flag, canary, GA, delete legacy paths. - t3code/opencode/background-agents reference docs: research notes informing the design.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInjects async workspace ops into editor/footer (file search, slash-preview), consolidates chat retrieval via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPC as TRPC API
participant Runtime as HostService Runtime
participant DB as Persistence
Client->>TRPC: query chat.getSnapshot({ sessionId, workspaceId })
TRPC->>Runtime: getSnapshot(sessionId, workspaceId)
Runtime->>DB: fetch messages & display state
DB-->>Runtime: messages + persisted state
Runtime-->>TRPC: { displayState, messages, observedAt }
TRPC-->>Client: snapshot response
Client->>TRPC: mutation chat.endSession({ sessionId, workspaceId })
TRPC->>Runtime: disposeRuntime(sessionId, workspaceId)
Runtime->>Runtime: abort harness & cleanup
Runtime-->>TRPC: { ok: true }
TRPC-->>Client: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Adds an Implementation Audit section (file inventory, what's solid, stubs, gaps, verified bugs), a Fixes-at-a-glance priority table, and restructures Phased Migration so P0 captures the two HIGH defects concretely: - Runtime leak on session delete (no disposeRuntime / endSession) - Cross-workspace sessionId race in runtimeCreations map P1 folds in the dual-poll race fix via a single getSnapshot query, the fps:60 -> default 4fps polling cadence drop, and the missing cloud lastActiveAt update on host send (selector ordering). P4 enumerates parity gaps with v1 file references; Stop and Notification hook events explicitly deferred per scope decision.
Lands the P0 + P1 fixes from plans/v1-to-v2-fast-migration.md.
Host-service runtime (packages/host-service/src/runtime/chat/chat.ts):
- runtimeCreations map now stores { workspaceId, promise } and
validates the workspace at the in-flight path. Concurrent requests
with the same sessionId across different workspaceIds no longer
silently share the wrong-workspace runtime.
- Adds disposeRuntime(sessionId) - aborts harness, drops from map,
idempotent. Closes the runtime leak on session delete.
- Adds getSnapshot(input) returning { displayState, messages,
observedAt } from a single observation. Reuses the existing
display-state shaping logic.
Host-service router (packages/host-service/src/trpc/router/chat/chat.ts):
- New chat.getSnapshot query.
- New chat.endSession mutation.
- chat.sendMessage fires-and-forgets ctx.api.chat.updateSession with
lastActiveAt: new Date() after success so the v2 session selector
keeps reordering after activity.
Cloud router (packages/trpc/src/router/chat/chat.ts):
- updateSession now accepts an optional lastActiveAt: z.date().
V2-workspace ChatPane:
- useWorkspaceChatDisplay: replaces dual getDisplayState +
listMessages queries with a single getSnapshot query. Derives
state from snapshot.displayState / snapshot.messages.
- ChatPaneInterface: drops fps: 60 override (~120 RPCs/sec/pane)
in favor of the hook's default fps: 4 (matches v1).
- useWorkspaceChatController: handleDeleteSession now also calls
workspaceTrpc.chat.endSession after the cloud delete to tear
down the host-service runtime.
P4 parity gaps (slash commands, file search, hooks, title
generation, MCP, model auth state) are deferred per scope.
Drops the last v1-IPC coupling from TiptapPromptEditor by switching the file-search and slash-command-preview calls to prop injection. v1's ChatInputFooter wires them to chatServiceTrpc (legacy IPC, unchanged); v2's wires searchFiles to workspaceTrpc.filesystem.searchFiles so the @-mention popup works for both local and remote host-service workspaces. The chip-anchored SlashCommandPreviewPopover becomes opt-in via prop and stays off in v2, which already mounts its own SlashCommandPreview using workspaceTrpc.chat.previewSlashCommand. usePaneRegistry now mounts the real <ChatPane> instead of the "temporarily disabled" placeholder, threading sessionId / launchConfig through ctx.actions.updateData.
…-web Conflicts were compose-don't-pick: - useWorkspaceChatController.ts: main switched the cloud delete to an optimistic transaction via useOptimisticCollectionActions. Kept that, then still fire workspaceTrpc.chat.endSession after to tear down the host-service runtime. - usePaneRegistry.tsx: main added a renderTitle with V2NotificationStatusIndicator, shrunk the chat icon to size-3.5, and dropped browserRuntimeRegistry from the imports (browser destruction is now handled by useGlobalBrowserLifecycle). Kept all of that and replaced the placeholder renderPane with the real <ChatPane>. Net effect: v2 chat is enabled, sessions delete optimistically, the host-service runtime is disposed on delete, and chat tabs show unread notification badges.
Greptile SummaryThis PR ships P0/P1 bug fixes alongside architecture docs: it replaces the dual
Confidence Score: 4/5Safe to merge after addressing the disposeRuntime inflight-creation leak. One P1 defect: disposeRuntime silently no-ops when the runtime is still being created, causing a guaranteed leak for sessions deleted immediately after first open. All other changes are clean. packages/host-service/src/runtime/chat/chat.ts — disposeRuntime method
|
| Filename | Overview |
|---|---|
| packages/host-service/src/runtime/chat/chat.ts | Adds getSnapshot (combined display-state + messages) and disposeRuntime. The dispose function has a P1 leak: it skips cleanup when the runtime is still in-flight in runtimeCreations. |
| packages/host-service/src/trpc/router/chat/chat.ts | Exposes getSnapshot query and endSession mutation; adds fire-and-forget lastActiveAt update on sendMessage. Logic is straightforward and correct. |
| packages/trpc/src/router/chat/chat.ts | Adds optional lastActiveAt field to updateSession schema and applies it to the DB update. Clean, no issues. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatDisplay/useWorkspaceChatDisplay.ts | Replaces dual getDisplayState + listMessages queries with a single getSnapshot query, eliminating the client-side dual-poll race. Migration is clean. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts | Adds endSession mutation call on session delete to tear down host-service runtime; correctly fire-and-forgets so UI delete is never blocked. |
Sequence Diagram
sequenceDiagram
participant UI as ChatPane (React)
participant TRPC as host-service tRPC
participant MGR as ChatRuntimeManager
participant H as RuntimeHarness
Note over UI,H: Before – dual poll (race condition)
UI->>TRPC: getDisplayState(sessionId)
UI->>TRPC: listMessages(sessionId)
TRPC-->>UI: displayState (t₁)
TRPC-->>UI: messages (t₂ ≠ t₁ → mismatch)
Note over UI,H: After – single snapshot
UI->>TRPC: getSnapshot(sessionId)
TRPC->>MGR: getSnapshot(sessionId, workspaceId)
MGR->>H: getDisplayState()
H-->>MGR: displayStateRaw
MGR->>H: listMessages()
H-->>MGR: messages
MGR-->>TRPC: {displayState, messages, observedAt}
TRPC-->>UI: snapshot
Note over UI,H: Session delete flow
UI->>TRPC: deleteSessionRecord(sessionId)
TRPC-->>UI: ok
UI-->>TRPC: endSession(sessionId) [fire-and-forget]
TRPC->>MGR: disposeRuntime(sessionId)
MGR->>H: abort()
MGR-->>TRPC: ok
Comments Outside Diff (2)
-
packages/host-service/src/runtime/chat/chat.ts, line 470-480 (link)disposeRuntimemisses inflight creations, causing a runtime leakIf
disposeRuntimeis called while the runtime forsessionIdis still being created (i.e. it exists inruntimeCreationsbut not yet inruntimes), the early-return on line 472 causes the function to silently no-op. The in-flightcreateRuntimepromise then completes, callsruntimeCreations.deletein its.finally(), and inserts itself intoruntimes— permanently leaking the runtime with no future cleanup path.This will reliably trigger when a user deletes a session immediately after opening it for the first time (before the first
getSnapshotpoll resolves).Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/runtime/chat/chat.ts Line: 470-480 Comment: **`disposeRuntime` misses inflight creations, causing a runtime leak** If `disposeRuntime` is called while the runtime for `sessionId` is still being created (i.e. it exists in `runtimeCreations` but not yet in `runtimes`), the early-return on line 472 causes the function to silently no-op. The in-flight `createRuntime` promise then completes, calls `runtimeCreations.delete` in its `.finally()`, and inserts itself into `runtimes` — permanently leaking the runtime with no future cleanup path. This will reliably trigger when a user deletes a session immediately after opening it for the first time (before the first `getSnapshot` poll resolves). How can I resolve this? If you propose a fix, please make it concise.
-
packages/host-service/src/runtime/chat/chat.ts, line 216-218 (link)observedAttimestamp is recorded after data collection, not beforeobservedAt = Date.now()is set afterlistMessages()resolves rather than before the first read. The timestamp therefore reflects the end of the snapshot window, not its start. For a value namedobservedAt(implying "the state as of this moment"), it would be more accurate to record the timestamp before readingdisplayStateRaw.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/runtime/chat/chat.ts Line: 216-218 Comment: **`observedAt` timestamp is recorded after data collection, not before** `observedAt = Date.now()` is set after `listMessages()` resolves rather than before the first read. The timestamp therefore reflects the end of the snapshot window, not its start. For a value named `observedAt` (implying "the state as of this moment"), it would be more accurate to record the timestamp before reading `displayStateRaw`. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/host-service/src/runtime/chat/chat.ts
Line: 470-480
Comment:
**`disposeRuntime` misses inflight creations, causing a runtime leak**
If `disposeRuntime` is called while the runtime for `sessionId` is still being created (i.e. it exists in `runtimeCreations` but not yet in `runtimes`), the early-return on line 472 causes the function to silently no-op. The in-flight `createRuntime` promise then completes, calls `runtimeCreations.delete` in its `.finally()`, and inserts itself into `runtimes` — permanently leaking the runtime with no future cleanup path.
This will reliably trigger when a user deletes a session immediately after opening it for the first time (before the first `getSnapshot` poll resolves).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/src/runtime/chat/chat.ts
Line: 216-218
Comment:
**`observedAt` timestamp is recorded after data collection, not before**
`observedAt = Date.now()` is set after `listMessages()` resolves rather than before the first read. The timestamp therefore reflects the end of the snapshot window, not its start. For a value named `observedAt` (implying "the state as of this moment"), it would be more accurate to record the timestamp before reading `displayStateRaw`.
```suggestion
const displayStateRaw = runtime.harness.getDisplayState();
const observedAt = Date.now();
const messages = await runtime.harness.listMessages();
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge origin/main into v2-chat-architect..." | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts:113">
P2: Do not silently swallow `endSession` failures; add at least warning-level logging so runtime teardown issues are observable.
(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/host-service/src/trpc/router/chat/chat.ts">
<violation number="1" location="packages/host-service/src/trpc/router/chat/chat.ts:64">
P2: Avoid silently swallowing `updateSession` failures; log or surface the error context while keeping the call non-blocking.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="plans/background-agents-chat-architecture-reference.md">
<violation number="1" location="plans/background-agents-chat-architecture-reference.md:3">
P3: This markdown link points to a non-existent repository path, creating a broken docs reference.</violation>
</file>
<file name="plans/t3code-chat-architecture-reference.md">
<violation number="1" location="plans/t3code-chat-architecture-reference.md:3">
P3: The new Markdown link points to a non-existent path, so readers get a broken link in this reference doc.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Failures here must not block the user-visible delete. | ||
| void endSessionMutation | ||
| .mutateAsync({ sessionId: sessionIdToDelete, workspaceId }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
P2: Do not silently swallow endSession failures; add at least warning-level logging so runtime teardown issues are observable.
(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts, line 113:
<comment>Do not silently swallow `endSession` failures; add at least warning-level logging so runtime teardown issues are observable.
(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.) </comment>
<file context>
@@ -105,6 +106,11 @@ export function useWorkspaceChatController({
+ // Failures here must not block the user-visible delete.
+ void endSessionMutation
+ .mutateAsync({ sessionId: sessionIdToDelete, workspaceId })
+ .catch(() => {});
posthog.capture("chat_session_deleted", {
workspace_id: workspaceId,
</file context>
Suggested change
| .catch(() => {}); | |
| .catch((error) => { | |
| console.warn("Failed to end chat session runtime", { | |
| sessionId: sessionIdToDelete, | |
| workspaceId, | |
| error, | |
| }); | |
| }); |
| // turn — the user already sees their message land via the snapshot. | ||
| void ctx.api.chat.updateSession | ||
| .mutate({ sessionId: input.sessionId, lastActiveAt: new Date() }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
P2: Avoid silently swallowing updateSession failures; log or surface the error context while keeping the call non-blocking.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/chat/chat.ts, line 64:
<comment>Avoid silently swallowing `updateSession` failures; log or surface the error context while keeping the call non-blocking.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.) </comment>
<file context>
@@ -41,15 +41,35 @@ export const chatRouter = router({
+ // turn — the user already sees their message land via the snapshot.
+ void ctx.api.chat.updateSession
+ .mutate({ sessionId: input.sessionId, lastActiveAt: new Date() })
+ .catch(() => {});
+ return result;
+ }),
</file context>
Suggested change
| .catch(() => {}); | |
| .catch((error) => { | |
| console.warn("Failed to update chat session lastActiveAt", error); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/src/router/chat/chat.ts (1)
86-105:⚠️ Potential issue | 🟡 MinorConsider deriving
lastActiveAtserver-side instead of accepting it from the client.Letting any authenticated caller pick
lastActiveAtmeans a misbehaving (or compromised) client can backdate or future-date sessions, distortingorderBy(lastActiveAt, "desc")in the sidebar (e.g.useWorkspaceChatController's live query). Since the only documented caller is host-service's fire-and-forget update right aftersendMessage, the simpler/safer shape is a boolean flag (e.g.touch: true) andupdates.lastActiveAt = new Date()here. If you want to keep the date on the wire for flexibility, consider validating it (<= now, not too far in the past) before persisting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/chat/chat.ts` around lines 86 - 105, The mutation currently accepts input.lastActiveAt from the client which allows clients to set/backdate session times; change the input shape and update logic so the server derives lastActiveAt instead: update the zod input schema to replace/augment lastActiveAt with a boolean flag (e.g. touch: z.boolean().optional()) and in the mutation handler (where you build updates: Partial<typeof chatSessions.$inferInsert>) stop trusting input.lastActiveAt and instead, when input.touch is true, set updates.lastActiveAt = new Date(); alternatively, if you must accept a date keep it and validate it server-side (ensure <= now and not excessively old) before assigning to updates.lastActiveAt to avoid tampering that would affect ordering in useWorkspaceChatController live queries.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/SlashCommandPreviewPopover.tsx (1)
72-86: Minor: staleslashPreviewlingers when input becomes too short orcwdclears.The early-return at line 74 skips both the call and any state reset, so a previously fetched preview stays in
slashPreviewuntil the next non-trivial input resolves. Today this is masked bypreviewMatchesInputCommand(canonical command name check) and theshowPopoverguard, so it's not user-visible — but it's fragile if those checks are ever loosened. Resetting tonullon the guard branch would make the state strictly reflect the latest input.♻️ Diff
useEffect(() => { - if (debouncedSlashPreviewInput.length <= 1 || !cwd) return; + if (debouncedSlashPreviewInput.length <= 1 || !cwd) { + setSlashPreview(null); + return; + } let cancelled = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/SlashCommandPreviewPopover.tsx` around lines 72 - 86, The stale preview issue: when debouncedSlashPreviewInput is too short or cwd is falsy the effect returns early and never clears previous state; update the useEffect to explicitly reset slashPreview by calling setSlashPreview(null) before returning in the guard branch (the branch that checks debouncedSlashPreviewInput.length <= 1 || !cwd), keeping the existing cancelled flag and previewSlashCommand promise flow intact so setSlashPreview is still used only when appropriate (refer to slashPreview, setSlashPreview, debouncedSlashPreviewInput, cwd, previewSlashCommand, and related previewMatchesInputCommand/showPopover checks).plans/v2-chat-greenfield-architecture.md (1)
391-421: Add a language identifier to the fenced code block (markdownlint MD040).The ASCII topology diagram block opens with bare
```, which tripsMD040 fenced-code-language. Usetext(or similar) to satisfy lint and keep rendering identical.📝 Diff
-``` +```text Clients (phone / web / desktop renderer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/v2-chat-greenfield-architecture.md` around lines 391 - 421, The fenced ASCII topology diagram block opening with ``` (the topology diagram showing Clients → Cloudflare → SessionDO/WorkspaceDO/D1) lacks a language identifier and triggers markdownlint MD040; fix it by adding a neutral language tag such as text (i.e., change the opening fence from ``` to ```text) so the renderer is unchanged and the lint rule is satisfied—update the fenced code block around the ASCII diagram in plans/v2-chat-greenfield-architecture.md (the block that starts with the Clients (phone / web / desktop renderer) line).apps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/TiptapPromptEditor.tsx (1)
149-167: Minor: stalefileResultsmay briefly leak across mention sessions.When the mention popup closes (e.g., Escape),
fileResultsretains the previous query's matches. Reopening the popup with a different query can flash those stale results until the new debounced fetch resolves. TheisMentionVisiblegate hides them while the popup is closed but doesn't reset the cache.♻️ Optional: clear results when popup is hidden
useEffect(() => { - if (!isMentionVisible || !cwd || debouncedMentionQuery.length === 0) return; + if (!isMentionVisible || !cwd || debouncedMentionQuery.length === 0) { + setFileResults([]); + return; + } let cancelled = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/TiptapPromptEditor.tsx` around lines 149 - 167, When the mention popup closes stale fileResults can flash on reopen; update the component to clear the cached results whenever the mention UI is hidden. Concretely, add a small effect or extend the existing useEffect that watches isMentionVisible (and/or mentionState) and calls setFileResults([]) when isMentionVisible becomes false (or when mentionState is null), while keeping the existing cancellation logic in the searchFiles promise chain; reference fileResults, setFileResults, isMentionVisible, debouncedMentionQuery, mentionState, searchFiles and the surrounding useEffect to locate where to add the reset.plans/opencode-electron-chat-architecture-reference.md (1)
11-58: Optional: tag the ASCII diagram fence.markdownlint flags MD040 because the fence has no language. Use
text(orplaintext) to silence the warning and keep GitHub's syntax highlighter from guessing.-``` +```text ┌────────────────────────── Electron (one Node / V8 process) ──────────────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/opencode-electron-chat-architecture-reference.md` around lines 11 - 58, The code block containing the ASCII architecture diagram is missing a language tag (triggers markdownlint MD040); update the opening fence (the triple backticks immediately before the line that starts with "┌────────────────────────── Electron (one Node / V8 process) ──────────────────────────┐") to include a language like text or plaintext (e.g., ```text) so the diagram block is tagged and the linter warning is suppressed.plans/v1-to-v2-fast-migration.md (1)
200-210: Plan/code drift: implementeddisposeRuntimeis narrower than this checklist.The actual
disposeRuntimeinpackages/host-service/src/runtime/chat/chat.tsonly callsharness.abort()and removes from the map. The checklist items "run any session-end hook (placeholder ok if hook wiring lands later)" and the workspace-deletion fan-out ("dispose all runtimes for sessions bound to it") are not yet wired. Either tighten the doc to reflect what landed in this PR (and move the missing items to P4) or land follow-up commits before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/v1-to-v2-fast-migration.md` around lines 200 - 210, The checklist in plans/v1-to-v2-fast-migration.md is out of sync with the implemented disposeRuntime: the current ChatRuntimeManager.disposeRuntime only calls harness.abort() and removes the entry from the runtimes map and does not run session-end hooks or fan out disposals on workspace deletion; update the doc to reflect the implemented behavior (explicitly state that disposeRuntime(sessionId) currently does: look up RuntimeSession, call harness.abort(), remove from runtimes, idempotent) and move the missing items ("run session-end hook", "dispose all runtimes for sessions bound to a workspace", plus adding chat.endSession mutation and wiring useWorkspaceChatController.ts and workspace deletion flow) into a follow-up TODO/raft with owner and priority, or alternatively implement the missing wiring by adding the session-end hook invocation in disposeRuntime, adding chat.endSession mutation, and wiring client call sites (useWorkspaceChatController and workspace deletion) as separate commits — reference disposeRuntime, ChatRuntimeManager, runtimes map, harness.abort(), chat.endSession, and useWorkspaceChatController when making the doc change or implementation.packages/host-service/src/trpc/router/chat/chat.ts (1)
57-66: Consider observability on the silent fire-and-forget.
.catch(() => {})swallows any cloudupdateSessionfailure, which is appropriate (must not block the turn) but means a persistently broken cloud → host connection silently breaks session-selector ordering with no signal. A.catch((err) => logger.debug(...))(orconsole.warn) would let this surface in diagnostics without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/chat/chat.ts` around lines 57 - 66, The fire-and-forget updateSession call currently swallows errors via .catch(() => {}) — change the catch to log the error (without rethrowing) so failures are observable; update the .catch on ctx.api.chat.updateSession.mutate (inside the mutation that calls ctx.runtime.chat.sendMessage) to call your existing logger (e.g., processLogger.debug or console.warn) and include context (sessionId/input) and the error details while preserving the non-blocking behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts:
- Around line 104-108: The silent .catch on the fire-and-forget call to
endSessionMutation.mutateAsync({ sessionId: sessionIdToDelete, workspaceId })
should log and report failures instead of swallowing them; change the catch to
e.g. .catch(err => { console.error("endSessionMutation failed for session",
sessionIdToDelete, "workspace", workspaceId, err);
posthog?.capture?.("chat.endSession_failure", { sessionId: sessionIdToDelete,
workspaceId, errorMessage: String(err) }); }) so runtime-disposal errors are
visible in console and PostHog while keeping the call fire-and-forget.
In `@packages/host-service/src/runtime/chat/chat.ts`:
- Around line 634-655: getSnapshot currently builds displayState from
displayStateRaw directly, which reintroduces the answered-question bug and
produces a different sandbox-question shape than getDisplayState; to fix, factor
the pendingQuestion construction into a single helper (e.g.,
buildDisplayStatePendingQuestion or buildDisplayState) and have both
getDisplayState and getSnapshot call it: the helper should accept
displayStateRaw and runtime, return a ChatDisplayState that applies the same
filter against runtime.answeredQuestionIds, and produce the same sandbox
question shape (top-level description from runtime.pendingSandboxQuestion.reason
and options with Yes/No descriptions), and preserve errorMessage resolution
(currentMessageError ?? runtime.lastErrorMessage) so both functions remain
consistent.
- Around line 524-534: disposeRuntime currently only aborts runtime.harness and
deletes from this.runtimes, causing race leaks, missing manager teardowns, and
no workspace validation; update disposeRuntime to accept workspaceId, validate
that the found runtime's workspaceId matches (like getOrCreateRuntime), and
refuse to dispose if it differs (matching endSession behavior); ensure you
handle in-flight creations by checking and cancelling/awaiting any matching
entry in this.runtimeCreations (or rechecking this.runtimes after a microtask
boundary) so a concurrent createRuntime cannot later insert a leaked runtime;
finally, invoke proper teardown on runtime.mcpManager and runtime.hookManager
(call their .dispose()/.close()/.shutdown() if available) before deleting the
entry and still wrap harness.abort in a try/catch to remain best-effort.
In `@packages/host-service/src/trpc/router/chat/chat.ts`:
- Around line 68-73: endSession currently calls ctx.runtime.chat.disposeRuntime
with only input.sessionId, which allows cross-workspace disposal; modify
endSession to pass both input.sessionId and input.workspaceId (or otherwise call
a runtime API that accepts workspaceId) and ensure the runtime manager verifies
the runtime's bound workspaceId before disposing, returning an error if they
mismatch; update the disposeRuntime signature (or add a new
disposeRuntimeWithWorkspace) and adjust call sites to enforce this check so only
the workspace that owns the session can end it.
In `@plans/v1-to-v2-fast-migration.md`:
- Line 18: The scope estimate is inconsistent between the PR description
("estimated 2–3 weeks for one engineer") and the "Scope:" line in this document
("roughly 1-2 weeks of implementation plus bake time"); update the "Scope:"
sentence in plans/v1-to-v2-fast-migration.md (the line starting with "Scope:")
so it matches the PR description's estimate (or vice versa) and use a single
reconciled wording (e.g., "estimated 2–3 weeks for one engineer, plus bake
time") to ensure consistency for reviewers and stakeholders.
---
Outside diff comments:
In `@packages/trpc/src/router/chat/chat.ts`:
- Around line 86-105: The mutation currently accepts input.lastActiveAt from the
client which allows clients to set/backdate session times; change the input
shape and update logic so the server derives lastActiveAt instead: update the
zod input schema to replace/augment lastActiveAt with a boolean flag (e.g.
touch: z.boolean().optional()) and in the mutation handler (where you build
updates: Partial<typeof chatSessions.$inferInsert>) stop trusting
input.lastActiveAt and instead, when input.touch is true, set
updates.lastActiveAt = new Date(); alternatively, if you must accept a date keep
it and validate it server-side (ensure <= now and not excessively old) before
assigning to updates.lastActiveAt to avoid tampering that would affect ordering
in useWorkspaceChatController live queries.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/SlashCommandPreviewPopover.tsx`:
- Around line 72-86: The stale preview issue: when debouncedSlashPreviewInput is
too short or cwd is falsy the effect returns early and never clears previous
state; update the useEffect to explicitly reset slashPreview by calling
setSlashPreview(null) before returning in the guard branch (the branch that
checks debouncedSlashPreviewInput.length <= 1 || !cwd), keeping the existing
cancelled flag and previewSlashCommand promise flow intact so setSlashPreview is
still used only when appropriate (refer to slashPreview, setSlashPreview,
debouncedSlashPreviewInput, cwd, previewSlashCommand, and related
previewMatchesInputCommand/showPopover checks).
In
`@apps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/TiptapPromptEditor.tsx`:
- Around line 149-167: When the mention popup closes stale fileResults can flash
on reopen; update the component to clear the cached results whenever the mention
UI is hidden. Concretely, add a small effect or extend the existing useEffect
that watches isMentionVisible (and/or mentionState) and calls setFileResults([])
when isMentionVisible becomes false (or when mentionState is null), while
keeping the existing cancellation logic in the searchFiles promise chain;
reference fileResults, setFileResults, isMentionVisible, debouncedMentionQuery,
mentionState, searchFiles and the surrounding useEffect to locate where to add
the reset.
In `@packages/host-service/src/trpc/router/chat/chat.ts`:
- Around line 57-66: The fire-and-forget updateSession call currently swallows
errors via .catch(() => {}) — change the catch to log the error (without
rethrowing) so failures are observable; update the .catch on
ctx.api.chat.updateSession.mutate (inside the mutation that calls
ctx.runtime.chat.sendMessage) to call your existing logger (e.g.,
processLogger.debug or console.warn) and include context (sessionId/input) and
the error details while preserving the non-blocking behavior.
In `@plans/opencode-electron-chat-architecture-reference.md`:
- Around line 11-58: The code block containing the ASCII architecture diagram is
missing a language tag (triggers markdownlint MD040); update the opening fence
(the triple backticks immediately before the line that starts with
"┌────────────────────────── Electron (one Node / V8 process)
──────────────────────────┐") to include a language like text or plaintext
(e.g., ```text) so the diagram block is tagged and the linter warning is
suppressed.
In `@plans/v1-to-v2-fast-migration.md`:
- Around line 200-210: The checklist in plans/v1-to-v2-fast-migration.md is out
of sync with the implemented disposeRuntime: the current
ChatRuntimeManager.disposeRuntime only calls harness.abort() and removes the
entry from the runtimes map and does not run session-end hooks or fan out
disposals on workspace deletion; update the doc to reflect the implemented
behavior (explicitly state that disposeRuntime(sessionId) currently does: look
up RuntimeSession, call harness.abort(), remove from runtimes, idempotent) and
move the missing items ("run session-end hook", "dispose all runtimes for
sessions bound to a workspace", plus adding chat.endSession mutation and wiring
useWorkspaceChatController.ts and workspace deletion flow) into a follow-up
TODO/raft with owner and priority, or alternatively implement the missing wiring
by adding the session-end hook invocation in disposeRuntime, adding
chat.endSession mutation, and wiring client call sites
(useWorkspaceChatController and workspace deletion) as separate commits —
reference disposeRuntime, ChatRuntimeManager, runtimes map, harness.abort(),
chat.endSession, and useWorkspaceChatController when making the doc change or
implementation.
In `@plans/v2-chat-greenfield-architecture.md`:
- Around line 391-421: The fenced ASCII topology diagram block opening with ```
(the topology diagram showing Clients → Cloudflare → SessionDO/WorkspaceDO/D1)
lacks a language identifier and triggers markdownlint MD040; fix it by adding a
neutral language tag such as text (i.e., change the opening fence from ``` to
```text) so the renderer is unchanged and the lint rule is satisfied—update the
fenced code block around the ASCII diagram in
plans/v2-chat-greenfield-architecture.md (the block that starts with the Clients
(phone / web / desktop renderer) line).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3f9f23f-bc0e-407a-96d0-e1684e8c0068
📒 Files selected for processing (16)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/ChatInputFooter.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/SlashCommandPreviewPopover.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/TiptapPromptEditor/TiptapPromptEditor.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/ChatPaneInterface.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/components/ChatInputFooter/ChatInputFooter.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatDisplay/useWorkspaceChatDisplay.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxpackages/host-service/src/runtime/chat/chat.tspackages/host-service/src/trpc/router/chat/chat.tspackages/trpc/src/router/chat/chat.tsplans/background-agents-chat-architecture-reference.mdplans/opencode-electron-chat-architecture-reference.mdplans/t3code-chat-architecture-reference.mdplans/v1-to-v2-fast-migration.mdplans/v2-chat-greenfield-architecture.md
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/ChatPaneInterface.tsx
Comment on lines
+104
to
+108
| // Tear down the host-service in-memory runtime so it doesn't leak. | ||
| // Failures here must not block the user-visible delete. | ||
| void endSessionMutation | ||
| .mutateAsync({ sessionId: sessionIdToDelete, workspaceId }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
Silent .catch(() => {}) loses observability on runtime-disposal failures.
If chat.endSession fails (network, host-service down, race with workspace teardown), the host-service runtime can leak — exactly the case this mutation is meant to prevent — and we'll have nothing in logs/PostHog to diagnose it. Keep the fire-and-forget shape, but at minimum log to console (and ideally a posthog event) so the failure mode is visible.
♻️ Suggested change
- void endSessionMutation
- .mutateAsync({ sessionId: sessionIdToDelete, workspaceId })
- .catch(() => {});
+ void endSessionMutation
+ .mutateAsync({ sessionId: sessionIdToDelete, workspaceId })
+ .catch((err) => {
+ console.error("Failed to end chat session runtime", err);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Tear down the host-service in-memory runtime so it doesn't leak. | |
| // Failures here must not block the user-visible delete. | |
| void endSessionMutation | |
| .mutateAsync({ sessionId: sessionIdToDelete, workspaceId }) | |
| .catch(() => {}); | |
| // Tear down the host-service in-memory runtime so it doesn't leak. | |
| // Failures here must not block the user-visible delete. | |
| void endSessionMutation | |
| .mutateAsync({ sessionId: sessionIdToDelete, workspaceId }) | |
| .catch((err) => { | |
| console.error("Failed to end chat session runtime", err); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts
around lines 104 - 108, The silent .catch on the fire-and-forget call to
endSessionMutation.mutateAsync({ sessionId: sessionIdToDelete, workspaceId })
should log and report failures instead of swallowing them; change the catch to
e.g. .catch(err => { console.error("endSessionMutation failed for session",
sessionIdToDelete, "workspace", workspaceId, err);
posthog?.capture?.("chat.endSession_failure", { sessionId: sessionIdToDelete,
workspaceId, errorMessage: String(err) }); }) so runtime-disposal errors are
visible in console and PostHog while keeping the call fire-and-forget.
Adds host-service auth surface and connects the v2 ModelPicker to it, so the picker can show real Anthropic/OpenAI authentication state instead of hardcoded `true`. Host-service: - types.ts: HostServiceRuntime gains an `auth: ChatService` field. - app.ts: instantiate a long-lived ChatService singleton (auth is per-machine, not per-workspace) and expose it via runtime.auth. - New trpc/router/auth/auth.ts router with 17 procedures mirroring v1's chatServiceTrpc.auth.* surface (getAnthropicStatus, startAnthropicOAuth, completeAnthropicOAuth, etc.). - Wired into appRouter as `auth`. ChatService delegates to mastra's createAuthStorage() under the hood, so this is a thin proxy — no auth logic is duplicated. Mastra's storage files are the same paths host-service already reads from when resolving credentials at runtime, so v1 settings/models writes and v2 reads stay in sync via disk. Renderer: - v2 ModelPicker now queries workspaceTrpc.auth.getAnthropicStatus and getOpenAIStatus; previously hardcoded both to authenticated. Refetches on selector open. The "open settings" action keeps navigating to /settings/models which is the existing v1 page (still works because it writes via Electron-main IPC to the same on-disk storage). Remote-host auth UI is a follow-on; for remote workspaces the status correctly reflects the remote machine's credentials, but the /settings/models flow only authenticates the local machine.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/host-service/src/trpc/router/auth/auth.ts (1)
16-18: Consider tighteningenvTextvalidation.
envText: z.string()permits empty strings, which then flow throughsetAnthropicEnvConfig. Since there's already a dedicatedclearAnthropicEnvConfigmutation, accepting""here makes the API ambiguous — callers can clear via either path with potentially different semantics insideChatService. Either explicitly disallow empty (.min(1)) or document that empty == clear.Also applies to: 49-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/auth/auth.ts` around lines 16 - 18, Tighten validation for the anthropicEnvConfigInput's envText so empty strings are rejected to avoid ambiguity with clearAnthropicEnvConfig: update the zod schema for anthropicEnvConfigInput (envText) to require at least one character (e.g., .min(1)) and adjust any related validation logic used by setAnthropicEnvConfig; ensure callers know that clearing must use clearAnthropicEnvConfig and that ChatService logic expects non-empty envText in setAnthropicEnvConfig.packages/host-service/src/types.ts (1)
2-2: LGTM — runtime now exposes provider auth surface.Typing
authasChatServiceis acceptable here given the comment inapp.tsexplaining the field is intentionally a narrow alias for the auth subset ofChatService. IfChatServicegrows non-auth methods later, consider tightening this field to a dedicated interface (e.g.ProviderAuthService) so the runtime type doesn't leak unrelated capabilities into all consumers.Also applies to: 15-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/types.ts` at line 2, The current typing of the auth field as ChatService is acceptable, but to avoid leaking non-auth methods create a narrow interface (e.g. ProviderAuthService) that contains only the auth methods used and replace the ChatService usage: define ProviderAuthService with the required auth method signatures, export it, and change the auth field type in types.ts (and the alias in app.ts) from ChatService to ProviderAuthService so consumers only receive the auth surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/components/ModelPicker/ModelPicker.tsx:
- Around line 45-53: The current useEffect triggers redundant refetches
(refetchAnthropicStatus/refetchOpenAIStatus) when the picker mounts and also
causes an "unauthenticated flash" because anthropicStatus/openAIStatus are
undefined while initial queries load; remove the Promise.all refetch effect and
instead rely on the original useQuery behavior or, if you need refetch-on-open,
only call refetch when the query is not already fetching (use the
isLoading/isFetching/isFetched flags from
workspaceTrpc.auth.getAnthropicStatus.useQuery and getOpenAIStatus.useQuery) and
change UI checks from treating undefined as false to treating undefined as
loading (e.g., use anthropicStatus === true for authenticated or guard with
isLoading/isFetched) so the providers aren’t shown as unauthenticated during the
initial load.
---
Nitpick comments:
In `@packages/host-service/src/trpc/router/auth/auth.ts`:
- Around line 16-18: Tighten validation for the anthropicEnvConfigInput's
envText so empty strings are rejected to avoid ambiguity with
clearAnthropicEnvConfig: update the zod schema for anthropicEnvConfigInput
(envText) to require at least one character (e.g., .min(1)) and adjust any
related validation logic used by setAnthropicEnvConfig; ensure callers know that
clearing must use clearAnthropicEnvConfig and that ChatService logic expects
non-empty envText in setAnthropicEnvConfig.
In `@packages/host-service/src/types.ts`:
- Line 2: The current typing of the auth field as ChatService is acceptable, but
to avoid leaking non-auth methods create a narrow interface (e.g.
ProviderAuthService) that contains only the auth methods used and replace the
ChatService usage: define ProviderAuthService with the required auth method
signatures, export it, and change the auth field type in types.ts (and the alias
in app.ts) from ChatService to ProviderAuthService so consumers only receive the
auth surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd8c9ced-ce2b-4719-9872-34df368e25d9
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/components/ModelPicker/ModelPicker.tsxpackages/host-service/src/app.tspackages/host-service/src/trpc/router/auth/auth.tspackages/host-service/src/trpc/router/auth/index.tspackages/host-service/src/trpc/router/router.tspackages/host-service/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/trpc/router/auth/index.ts
Comment on lines
+45
to
+53
| const { data: anthropicStatus, refetch: refetchAnthropicStatus } = | ||
| workspaceTrpc.auth.getAnthropicStatus.useQuery(); | ||
| const { data: openAIStatus, refetch: refetchOpenAIStatus } = | ||
| workspaceTrpc.auth.getOpenAIStatus.useQuery(); | ||
|
|
||
| useEffect(() => { | ||
| if (!open) return; | ||
| void Promise.all([refetchAnthropicStatus(), refetchOpenAIStatus()]); | ||
| }, [open, refetchAnthropicStatus, refetchOpenAIStatus]); |
Contributor
There was a problem hiding this comment.
Minor: duplicate initial fetch and "unauthenticated flash" risk.
useQuery already auto-fetches on mount, so when the picker mounts with open === true the effect at lines 50–53 will issue a redundant refetch() for both providers in addition to the in-flight initial query.
Separately, while the queries are loading, anthropicStatus/openAIStatus are undefined, so ?? false will render both providers as unauthenticated for the first frames after mount — previously these were hardcoded true, so users with valid OAuth will momentarily see disabled/auth-prompt UI on first open. Consider gating until data is loaded, or treating undefined as "loading" rather than "unauthenticated":
♻️ Suggested handling
- const { data: anthropicStatus, refetch: refetchAnthropicStatus } =
- workspaceTrpc.auth.getAnthropicStatus.useQuery();
- const { data: openAIStatus, refetch: refetchOpenAIStatus } =
- workspaceTrpc.auth.getOpenAIStatus.useQuery();
-
- useEffect(() => {
- if (!open) return;
- void Promise.all([refetchAnthropicStatus(), refetchOpenAIStatus()]);
- }, [open, refetchAnthropicStatus, refetchOpenAIStatus]);
+ const {
+ data: anthropicStatus,
+ isFetched: anthropicFetched,
+ refetch: refetchAnthropicStatus,
+ } = workspaceTrpc.auth.getAnthropicStatus.useQuery();
+ const {
+ data: openAIStatus,
+ isFetched: openAIFetched,
+ refetch: refetchOpenAIStatus,
+ } = workspaceTrpc.auth.getOpenAIStatus.useQuery();
+
+ // Refetch when picker re-opens, but skip the initial open since useQuery
+ // already fetched on mount.
+ useEffect(() => {
+ if (!open || !anthropicFetched || !openAIFetched) return;
+ void Promise.all([refetchAnthropicStatus(), refetchOpenAIStatus()]);
+ }, [open, anthropicFetched, openAIFetched, refetchAnthropicStatus, refetchOpenAIStatus]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/components/ModelPicker/ModelPicker.tsx
around lines 45 - 53, The current useEffect triggers redundant refetches
(refetchAnthropicStatus/refetchOpenAIStatus) when the picker mounts and also
causes an "unauthenticated flash" because anthropicStatus/openAIStatus are
undefined while initial queries load; remove the Promise.all refetch effect and
instead rely on the original useQuery behavior or, if you need refetch-on-open,
only call refetch when the query is not already fetching (use the
isLoading/isFetching/isFetched flags from
workspaceTrpc.auth.getAnthropicStatus.useQuery and getOpenAIStatus.useQuery) and
change UI checks from treating undefined as false to treating undefined as
loading (e.g., use anthropicStatus === true for authenticated or guard with
isLoading/isFetched) so the providers aren’t shown as unauthenticated during the
initial load.
Mastra defaults observational-memory observer + reflector models to
google/gemini-2.5-flash. Without GOOGLE_GENERATIVE_AI_API_KEY in the
host-service process env, the post-turn reflection step fails with
"Could not find API key process.env.GOOGLE_GENERATIVE_AI_API_KEY".
v1's ChatRuntimeService already handles this in
packages/chat/src/server/trpc/service.ts:88 — derive the OM model
from whichever provider the user is authenticated with (Anthropic ->
Haiku 4.5, OpenAI -> GPT-4.1 nano, Google env -> Gemini). Mirror that
helper in host-service and pass the result as initialState
{observerModelId, reflectorModelId} to createMastraCode.
When v2 chat migrated from chat.listMessages to chat.getSnapshot in c183954, sendMessageToSession kept writing optimistic user messages to the listMessages cache via setData. The display now reads from snapshot.messages, so the optimistic message landed in a dead cache and the first user message disappeared until the next snapshot poll fetched it back from the harness (~250ms+ at 4fps polling). Most visible on the first message of a fresh chat (page is empty before it), but every send had the gap. Update sendMessageToSession to write to chat.getSnapshot.setData. Two cases: - Existing snapshot in cache: spread, append optimistic message. - Fresh session, no snapshot yet: seed with a minimal displayState (the next poll fills it in) and an array containing just the optimistic message so it renders immediately. Rollback path mirrors: filter the optimistic id out of snapshot.messages on send failure.
Two small changes that eliminate forced rerenders when nothing has actually changed in the snapshot. 1. Drop observedAt from chat.getSnapshot The server was stamping every snapshot response with observedAt: Date.now(). Nothing on the client read it. But because the timestamp differs every poll, React Query's structuralSharing couldn't preserve the response object's identity — every 250ms (4fps polling) produced a new reference, forcing every component reading `snapshot` to rerender even when displayState and messages were byte-identical. Removing observedAt lets structural sharing kick in: idle polls now produce identical objects and downstream consumers don't rerender. Also drops the field from the optimistic-seed object in sendMessageToSession so it stays in sync with the new shape. 2. Memoize the slashCommands select mapper ChatPaneInterface.tsx had an inline `select: (commands) => commands.map(...)` in the getSlashCommands.useQuery options. The arrow function was recreated every render → mapper re-ran → new array reference → every consumer of `slashCommands` (TiptapPromptEditor, executor, etc.) rerendered even on identical input. useCallback the mapper and pass the stable reference instead. Together these two cuts close the cascade: poll resolves with structurally-equal data → React Query preserves identity → consumers see same reference → no rerender. Subsequent message-arrival polls still rerender (because data actually changed) — that's correct.
…t RQ cache User saw the message flicker in/out on the first send: thinking, then the message, then nothing, then thinking again, then the full result. Cause was a stale poll clobbering the optimistic update. The existing-session send path already does this correctly — useChatDisplay holds an `optimisticUserMessage` in component state and merges it into the messages output, outside React Query's cache. Stale polls can't overwrite local component state, so the message stays visible until the harness's response includes it. The new-session path (sendMessageToSession, only fired on first send) took a different route: write the optimistic message into the chat.getSnapshot React Query cache. With 4fps polling running continuously, a poll often resolved during the mutation with the harness's pre-message state and clobbered the optimistic write — message vanished, then reappeared on the next poll once the harness had processed it. Switch the new-session path to the same pattern: set pendingUserTurn local state in handleSend before calling sendMessageToSession. getVisibleMessagesWithPendingUserTurn already merges that into the displayed list and clears it once the server's messages array contains a matching entry. sendMessageToSession reduces to a thin mutation wrapper — no cache writes, no rollback. The restart flow already used pendingUserTurn for the same reason; this just brings the new-session path into parity.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/ChatPaneInterface.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/ChatPaneInterface.tsx:351">
P2: `sendMessageToSession` now assumes callers manage optimistic user-turn state, but other callers (like auto-launch) still invoke it without setting `pendingUserTurn`, causing inconsistent message visibility behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // and clobber the optimistic write, making the user message vanish | ||
| // briefly. The pendingUserTurn local state is merged in via | ||
| // getVisibleMessagesWithPendingUserTurn so it survives stale polls. | ||
| await sendMessageMutation.mutateAsync({ |
Contributor
There was a problem hiding this comment.
P2: sendMessageToSession now assumes callers manage optimistic user-turn state, but other callers (like auto-launch) still invoke it without setting pendingUserTurn, causing inconsistent message visibility behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/WorkspaceChatInterface/ChatPaneInterface.tsx, line 351:
<comment>`sendMessageToSession` now assumes callers manage optimistic user-turn state, but other callers (like auto-launch) still invoke it without setting `pendingUserTurn`, causing inconsistent message visibility behavior.</comment>
<file context>
@@ -343,63 +341,20 @@ export function ChatPaneInterface({
+ // and clobber the optimistic write, making the user message vanish
+ // briefly. The pendingUserTurn local state is merged in via
+ // getVisibleMessagesWithPendingUserTurn so it survives stale polls.
+ await sendMessageMutation.mutateAsync({
sessionId: targetSessionId,
workspaceId,
</file context>
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
1 participant
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
Plans-only PR. No code changes. Adds the chat-architecture research and proposals for the v2 chat rearchitect, plus a pragmatic v1→host-service migration plan that unblocks the greenfield work.
Greenfield architecture
plans/v2-chat-greenfield-architecture.md— proposed transport + state architecture. tRPC subscriptions over WS,EventLoginterface withLocalEventLog(SQLite) for P0–P4 and two cloud paths (Postgres / Durable Objects) for P5. P6 covers device handoff (turn-boundary, git-as-courier). P7 sketches mid-turn handoff but explicitly defers it.Fast migration
plans/v1-to-v2-fast-migration.md— ship-this-month plan. Move ChatRuntimeService into host-service, collapsegetDisplayState+listMessagesinto one atomicgetSnapshotquery (kills the dual-poll race), gate behind a per-workspace flag, canary, GA, delete legacy paths. ~2–3 weeks for one engineer.Reference research
Architectural notes from three external chat systems studied while designing the above:
plans/t3code-chat-architecture-reference.md— Effect-RPC + per-session SQLite event log + dual stream split.plans/opencode-electron-chat-architecture-reference.md— in-process Electron sidecar + REST writes / SSE reads + append-style delta events.plans/background-agents-chat-architecture-reference.md— Cloudflare Durable Objects per session, multiplayer, parallel sub-tasks, Modal sandboxes.Each reference doc has both a Mermaid and a plain-text architecture diagram, plus a "Signal for our rearchitecture" section.
Test plan
Summary by cubic
Adds v2 chat architecture proposals and reference research, and ships the v2 chat pane with snapshot-based state, real model‑provider auth, reliable runtime cleanup, pre‑session slash commands, and a fix for first‑message flicker. Also reduces idle rerenders by removing a timestamp from snapshots and memoizing the slash‑command mapping.
New Features
TiptapPromptEditorvia prop injection for file search and slash previews: v1 uses@superset/chat/client, v2 uses@superset/workspace-client; the chip preview stays off in v2.authrouter and a long‑livedChatService(@superset/chat/server/desktop); wire the v2ModelPickertoworkspaceTrpc.auth.*for real Anthropic/OpenAI status with refetch on open.workspaceTrpc.chat.resolveSlashCommand.Bug Fixes
chat.getSnapshot; render the first user message using local optimistic state (not the cache) so it appears immediately without flicker.buildDisplayState, restore theansweredQuestionIdsfilter, keep a consistent sandbox shape, and pass fallback observer/reflector models based on the authenticated provider to avoid post‑turn reflection failures without Google API keys.chat.endSession/disposeRuntime, now validated byworkspaceId, awaiting in‑flight creations, and disconnecting the MCP manager; router passesworkspaceId.workspaceId, update cloudlastActiveAton send, drop thefps: 60override to cut RPC load, and reduce idle rerenders by removingobservedAtfromgetSnapshotresponses and memoizing theslashCommandsselect mapper.Written for commit 6a8c264. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation