feat(desktop): add local session persistence and restore for chat#1287
Conversation
Sessions now survive pane close/reopen and app restart. The architecture is provider-agnostic with three cleanly separated layers: - AgentProvider interface (Claude SDK is first implementation) - Session metadata store (lowdb-backed, persists to ~/.superset/) - Session manager orchestrator (deactivate vs delete semantics) Key changes: - Closing a chat pane deactivates (not deletes) the session — messages persist in LMDB and can be restored later - SessionSelector dropdown in ChatPane toolbar for browsing, switching, creating, and deleting chat sessions - Claude agent endpoint persists sessionId mapping to disk and exposes GET /sessions/:id for provider session ID lookup - DurableStreamTestServer configured with persistent dataDir - Auto-title sessions from first user message - switchChatSession action added to tabs store
📝 WalkthroughWalkthroughReplaces Claude-specific session management with a provider-driven ChatSessionManager, adds a persistent SessionStore, introduces AgentProvider interfaces and a ClaudeSdkProvider, expands TRPC ai-chat endpoints (start/restore/deactivate/delete/rename/list/get), and updates UI to support session selection, restoration, and auto-titling. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant TRPC as TRPC Router
participant Store as SessionStore
participant CSM as ChatSessionManager
participant Provider as AgentProvider
participant Proxy as Proxy Server
UI->>TRPC: startSession({sessionId, workspaceId, cwd})
TRPC->>Store: create(session meta)
Store-->>TRPC: meta saved
TRPC->>Provider: getAgentRegistration({sessionId, cwd})
Provider-->>CSM: AgentRegistration
TRPC->>CSM: startSession({sessionId, workspaceId, cwd})
CSM->>Proxy: PUT /sessions/{sessionId} (registration)
Proxy-->>CSM: created
CSM->>CSM: emit('session_start')
CSM-->>TRPC: success
TRPC-->>UI: { success: true }
sequenceDiagram
participant UI as Client/UI
participant TRPC as TRPC Router
participant Store as SessionStore
participant CSM as ChatSessionManager
participant Provider as AgentProvider
participant Proxy as Proxy Server
UI->>TRPC: restoreSession({sessionId, cwd})
TRPC->>Store: get(sessionId)
Store-->>TRPC: session meta
TRPC->>CSM: restoreSession({sessionId, cwd})
CSM->>Provider: getAgentRegistration({sessionId, cwd})
Provider-->>CSM: AgentRegistration
CSM->>Proxy: PUT /sessions/{sessionId} (registration)
Proxy-->>CSM: restored
CSM->>CSM: emit('session_start')
CSM-->>TRPC: success
TRPC-->>UI: { success: true }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts (1)
285-323:⚠️ Potential issue | 🟠 Major
deleteSessioncan leave the session in a partial state ifprovider.cleanuporstore.archivethrows.Lines 311 and 314 are not wrapped in try/catch. If
provider.cleanupthrows, the session metadata won't be archived and the in-memory map entry won't be removed, but the proxy session is already deleted. Consider wrapping these in individual try/catch blocks or at least ensuringthis.sessions.deleteand thesession_endevent always execute (e.g., in afinallyblock).Proposed fix — ensure cleanup always completes
async deleteSession({ sessionId }: { sessionId: string }): Promise<void> { console.log(`[chat/session] Deleting session ${sessionId}`); const headers = buildProxyHeaders(); // Interrupt first try { await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/stop`, { method: "POST", headers, body: JSON.stringify({}), }); } catch { // Non-fatal } // Delete from proxy try { await fetch(`${PROXY_URL}/v1/sessions/${sessionId}`, { method: "DELETE", headers, }); } catch { // Non-fatal } - // Provider cleanup - await this.provider.cleanup(sessionId); - - // Archive metadata - await this.store.archive(sessionId); + // Provider cleanup (best-effort) + try { + await this.provider.cleanup(sessionId); + } catch (error) { + console.error(`[chat/session] Provider cleanup failed for ${sessionId}:`, error); + } + + // Archive metadata (best-effort) + try { + await this.store.archive(sessionId); + } catch (error) { + console.error(`[chat/session] Metadata archive failed for ${sessionId}:`, error); + } this.sessions.delete(sessionId);
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/agent-provider/claude-sdk-provider.ts`:
- Around line 55-57: The catch in getProviderSessionId is silently swallowing
errors; update the catch to log the error with context (e.g., include the
function name "getProviderSessionId", provider identifier or request details)
before returning undefined or rethrowing as appropriate; ensure you capture the
caught error object and pass it to your existing logger (e.g., processLogger,
logger, or console.error) so network/HTTP failures are visible during debugging.
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`:
- Around line 296-298: In deleteSession, replace the silent catch blocks around
the interrupt call and the proxy DELETE call with warnings that log the error
and contextual info (session id and operation) before continuing; specifically,
inside deleteSession add calls to the existing logger (e.g., processLogger.warn
or this.logger.warn) in the catch blocks for the interrupt/proxy DELETE
sections, include the caught error object and a clear message like "failed to
interrupt session {sessionId}" and "failed to proxy DELETE for session
{sessionId}" so errors are not swallowed silently.
- Around line 245-271: Both catch blocks silently swallow errors; update them to
log the error with context (including sessionId and operation) before treating
them as non-fatal. Specifically, in the fetch POST to
`${PROXY_URL}/v1/sessions/${sessionId}/stop` (and the surrounding try/catch) log
the caught error and the sessionId and operation name (e.g., "stop proxy
session") using the module/class logger (e.g., this.logger.error or
processLogger.error) then continue; likewise, in the try/catch around
this.provider.getProviderSessionId(sessionId) and subsequent
this.store.update(sessionId, ...) log the error with context (e.g., "provider
session id fetch or metadata update failed", sessionId, provider identifier)
before allowing deactivation to proceed.
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-store/session-store.ts`:
- Around line 54-66: ensureDb() can race when called concurrently because
multiple callers may call JSONFilePreset before this.db is assigned; introduce a
cached initialization promise (e.g., private dbInit?: Promise<SessionStoreData>)
and change ensureDb to: if (this.db) return it; if (this.dbInit) return await
it; otherwise set this.dbInit = JSONFilePreset<SessionStoreData>(STORE_PATH, {
sessions: [] }); await the promise, assign the resolved value to this.db, clear
this.dbInit, and ensure you catch errors to clear dbInit on failure so
subsequent calls can retry; reference ensureDb, JSONFilePreset, this.db,
this.dbInit, and STORE_PATH.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 163-170: The code unsafely asserts textPart.content as string and
misdetects truncation; fix by extracting the original text with a safe guard
(e.g., let originalText = textPart && "content" in textPart ?
String(textPart.content) : "") then compute truncated = originalText.slice(0,
80) and set title = originalText.length > 80 ? `${truncated}...` : (truncated ||
"Chat"); update the logic around the variables textPart, firstUserText (or
replace it with originalText/truncated), and title so you don't use a blind type
assertion and you compare originalText.length > 80 for truncation.
- Around line 150-178: The auto-title effect (uses hasAutoTitled ref and calls
renameSessionRef.current.mutate) is running on restored sessions and overwriting
existing titles; update the effect to early-return when the session already has
a non-default title (e.g., check session?.title and skip auto-rename if it's
present and not the placeholder like "Chat" or empty), so only sessions with no
meaningful title get renamed; keep the existing hasAutoTitled logic and
sessionId reset effect but add the title check before computing firstUserText
and calling renameSessionRef.current.mutate.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/components/SessionSelector/SessionSelector.tsx`:
- Around line 47-55: The title shows "Chat" because
electronTrpc.aiChat.listSessions.useQuery is disabled by { enabled: isOpen } so
sessions is undefined until the dropdown opens; change this by either always
enabling the list query or adding a lightweight query (e.g.,
electronTrpc.aiChat.getSession.useQuery keyed on currentSessionId, run on mount)
to fetch the current session title immediately; update SessionSelector to derive
displayTitle from the getSession result (fallback to sessions if present) or
simply remove the enabled flag so sessions/currentSession/currentSessionId
provide the correct displayTitle on initial render.
🧹 Nitpick comments (13)
apps/streams/src/claude-agent.ts (2)
82-92:writeFileSyncis non-atomic and blocks the event loop on the request path.
persistSessions()is called fromsetClaudeSessionIdduring SSE stream handling (line 131). Two concerns:
- Non-atomic write: If the process crashes mid-write,
claude-sessions.jsoncan be left truncated/corrupt, makingloadPersistedSessionsfail on next startup (gracefully, but sessions are lost).- Blocking I/O on request path:
writeFileSyncblocks the event loop. With ≤1000 entries the impact is small, but it's still on a hot path.Consider writing to a temp file and renaming atomically, or switching to async
writeFile. Libraries likewrite-file-atomicor a simple write-then-rename pattern would address both.♻️ Suggested async atomic write pattern
+import { writeFile, rename } from "node:fs/promises"; + function persistSessions(): void { try { if (!existsSync(SESSIONS_DIR)) { mkdirSync(SESSIONS_DIR, { recursive: true }); } const entries = Array.from(claudeSessions.entries()); - writeFileSync(SESSIONS_FILE, JSON.stringify(entries), "utf-8"); + const tmpFile = `${SESSIONS_FILE}.tmp`; + writeFileSync(tmpFile, JSON.stringify(entries), "utf-8"); + renameSync(tmpFile, SESSIONS_FILE); } catch (err) { console.warn("[claude-agent] Failed to persist sessions:", err); } }(Use
renameSyncfromnode:fsfor the sync approach, or convert the whole function to async for a non-blocking version.)
67-80: Parsed JSON is not validated before use.
loadPersistedSessionscastsJSON.parse(raw)directly toArray<[string, SessionEntry]>without validation. If the file contains unexpected shapes (e.g. from a corrupted write or version mismatch), iterating with destructuring could throw a less helpful error than an explicit validation check. The outertry/catchdoes provide a safety net, so this is not critical, but a Zod schema or a simple shape check would make the code more robust.apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-store/session-store.ts (1)
68-80:create()silently overwrites an existing session's fields on conflict.When an existing session is found,
Object.assign(existing, meta, { isArchived: false })overwrites all fields frommeta(includingworkspaceId,provider,cwd,createdAt). This is fine if it's the intended upsert semantic, but it means a caller can inadvertently changecreatedAtorprovideron an existing session. Consider whethercreatedAtshould be preserved on re-creation.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/components/SessionSelector/SessionSelector.tsx (1)
96-138: Brief "No previous sessions" flash while data loads on first dropdown open.When the dropdown opens for the first time,
sessionsisundefined(query just enabled), so the else branch at line 134 renders "No previous sessions" until data arrives. Consider showing a small loading indicator or deferring the content render untilsessions !== undefined.apps/desktop/src/lib/trpc/routers/ai-chat/utils/agent-provider/claude-sdk-provider.ts (1)
46-58: No timeout on fetch to Claude agent endpoint.If the Claude agent is unresponsive, this fetch will hang indefinitely. Since
getProviderSessionIdis called duringdeactivateSession, a stalled request could block session cleanup.Proposed fix
async getProviderSessionId(sessionId: string): Promise<string | undefined> { try { - const res = await fetch(`${CLAUDE_AGENT_URL}/sessions/${sessionId}`); + const res = await fetch(`${CLAUDE_AGENT_URL}/sessions/${sessionId}`, { + signal: AbortSignal.timeout(5_000), + }); if (!res.ok) return undefined;apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx (1)
59-68:deleteSessionin the dependency array defeatsuseCallbackmemoization.The mutation object returned by
useMutation()changes identity on each render (it carries mutable status fields). Including it in the deps array meanshandleDeleteSessionis recreated every render. Extract a stable reference or use just themutatefunction.Proposed fix
- const deleteSession = electronTrpc.aiChat.deleteSession.useMutation(); + const deleteSessionMutation = electronTrpc.aiChat.deleteSession.useMutation(); + const deleteSessionRef = useRef(deleteSessionMutation.mutate); + deleteSessionRef.current = deleteSessionMutation.mutate; const handleDeleteSession = useCallback( (sessionIdToDelete: string) => { - deleteSession.mutate({ sessionId: sessionIdToDelete }); + deleteSessionRef.current({ sessionId: sessionIdToDelete }); // If deleting the current session, switch to a new one if (sessionIdToDelete === sessionId) { handleNewChat(); } }, - [deleteSession, sessionId, handleNewChat], + [sessionId, handleNewChat], );Note: you'd also need to add
useRefto the import on line 1.apps/desktop/src/lib/trpc/routers/ai-chat/index.ts (3)
77-84:deleteSessionwill surface provider/store errors as opaqueINTERNAL_SERVER_ERROR.In the manager,
provider.cleanupandstore.archive(lines 311–314 in session-manager.ts) are not wrapped in try/catch. If either throws, the tRPC layer will return a generic 500 with no actionable context. Consider catching here and rethrowing as aTRPCErrorwith codeINTERNAL_SERVER_ERRORand a meaningful message, or adding try/catch in the manager'sdeleteSessionaround those calls.
86-98: Positional arguments onupdateSessionMeta— guideline preference for object params.
updateSessionMeta(sessionId, { title })uses two positional parameters. The coding guidelines prefer object parameters for functions with 2+ arguments. This is a minor alignment concern since the method is already defined this way in the manager class.As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments".
100-110:listSessionsandgetSessionbypasschatSessionManagerand accesssessionStoredirectly.All other procedures go through
chatSessionManager, but these two read directly from the store. This breaks the single-entry-point pattern and means the router now couples to two collaborators. Consider adding thin read-through methods onChatSessionManagerso the router only depends on one abstraction.apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts (4)
60-136:startSessionandrestoreSessionduplicate ~30 lines of proxy setup logic.Both methods perform the same proxy PUT → agent POST → sessions.set → emit sequence. Extract a shared helper (e.g.,
ensureProxySession(sessionId, cwd)) and call it from both methods to keep this DRY.♻️ Sketch of extracted helper
+ /** Ensure proxy session exists and agent is registered. */ + private async ensureProxySession({ + sessionId, + cwd, + }: { + sessionId: string; + cwd: string; + }): Promise<void> { + const headers = buildProxyHeaders(); + + const createRes = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}`, { + method: "PUT", + headers, + }); + if (!createRes.ok) { + throw new Error( + `PUT /v1/sessions/${sessionId} failed: ${createRes.status}`, + ); + } + + const registration = this.provider.getAgentRegistration({ + sessionId, + cwd, + }); + const registerRes = await fetch( + `${PROXY_URL}/v1/sessions/${sessionId}/agents`, + { + method: "POST", + headers, + body: JSON.stringify({ agents: [registration] }), + }, + ); + if (!registerRes.ok) { + throw new Error( + `POST /v1/sessions/${sessionId}/agents failed: ${registerRes.status}`, + ); + } + }Also applies to: 142-207
79-106: No timeout onfetchcalls to the proxy — requests may hang indefinitely.All fetch calls to
PROXY_URLlack anAbortSignal.timeout(). If the proxy is unresponsive,startSession,restoreSession,deactivateSession, anddeleteSessionwill block forever. Consider adding a timeout signal:const signal = AbortSignal.timeout(10_000); // 10s await fetch(url, { method: "PUT", headers, signal });Also applies to: 156-184
115-115: Hardcoded"New chat"magic string.Extract to a named constant at module level for reuse and discoverability.
As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic".
328-337:updateSessionMetauses positional arguments.Per coding guidelines, prefer an object parameter when there are 2+ arguments. Consider
updateSessionMeta({ sessionId, ...patch }).As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments".
| } catch { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Silent error swallowing in getProviderSessionId.
The catch block returns undefined without logging. Per coding guidelines, errors should at minimum be logged with context. A network failure here would be invisible during debugging.
Proposed fix
- } catch {
- return undefined;
+ } catch (error) {
+ console.error(`[agent-provider/claude] Failed to get provider session ID for ${sessionId}:`, error);
+ return undefined;
}As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
📝 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.
| } catch { | |
| return undefined; | |
| } | |
| } catch (error) { | |
| console.error(`[agent-provider/claude] Failed to get provider session ID for ${sessionId}:`, error); | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/agent-provider/claude-sdk-provider.ts`
around lines 55 - 57, The catch in getProviderSessionId is silently swallowing
errors; update the catch to log the error with context (e.g., include the
function name "getProviderSessionId", provider identifier or request details)
before returning undefined or rethrowing as appropriate; ensure you capture the
caught error object and pass it to your existing logger (e.g., processLogger,
logger, or console.error) so network/HTTP failures are visible during debugging.
| try { | ||
| await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/stop`, { | ||
| method: "POST", | ||
| headers: buildProxyHeaders(), | ||
| body: JSON.stringify({}), | ||
| }); | ||
| } catch { | ||
| // Non-fatal | ||
| } | ||
|
|
||
| // Capture provider session ID for future resume | ||
| try { | ||
| const providerSessionId = | ||
| await this.provider.getProviderSessionId(sessionId); | ||
| if (providerSessionId) { | ||
| await this.store.update(sessionId, { | ||
| providerSessionId, | ||
| lastActiveAt: Date.now(), | ||
| }); | ||
| } else { | ||
| await this.store.update(sessionId, { | ||
| lastActiveAt: Date.now(), | ||
| }); | ||
| } | ||
| } catch { | ||
| // Non-fatal — metadata update failure shouldn't block deactivation | ||
| } |
There was a problem hiding this comment.
Silent error swallowing — catch blocks with no logging.
Lines 251 and 269 catch errors without any logging. The coding guidelines require at minimum logging errors with context before handling them.
As per coding guidelines, "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly".
Proposed fix
} catch {
- // Non-fatal
+ // Non-fatal — best-effort interrupt
}→
- } catch {
- // Non-fatal
+ } catch (error) {
+ console.warn(`[chat/session] Interrupt during deactivation failed:`, error);
}And similarly for the metadata update catch:
- } catch {
- // Non-fatal — metadata update failure shouldn't block deactivation
+ } catch (error) {
+ console.warn(`[chat/session] Metadata update during deactivation failed:`, error);
}🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 245 - 271, Both catch blocks silently swallow errors; update them
to log the error with context (including sessionId and operation) before
treating them as non-fatal. Specifically, in the fetch POST to
`${PROXY_URL}/v1/sessions/${sessionId}/stop` (and the surrounding try/catch) log
the caught error and the sessionId and operation name (e.g., "stop proxy
session") using the module/class logger (e.g., this.logger.error or
processLogger.error) then continue; likewise, in the try/catch around
this.provider.getProviderSessionId(sessionId) and subsequent
this.store.update(sessionId, ...) log the error with context (e.g., "provider
session id fetch or metadata update failed", sessionId, provider identifier)
before allowing deactivation to proceed.
| } catch { | ||
| // Non-fatal | ||
| } |
There was a problem hiding this comment.
More silent catch blocks without logging in deleteSession.
Same guideline concern as in deactivateSession — the interrupt and proxy DELETE catches should at least log a warning.
As per coding guidelines, "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly".
Also applies to: 306-308
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 296 - 298, In deleteSession, replace the silent catch blocks around
the interrupt call and the proxy DELETE call with warnings that log the error
and contextual info (session id and operation) before continuing; specifically,
inside deleteSession add calls to the existing logger (e.g., processLogger.warn
or this.logger.warn) in the catch blocks for the interrupt/proxy DELETE
sections, include the caught error object and a clear message like "failed to
interrupt session {sessionId}" and "failed to proxy DELETE for session
{sessionId}" so errors are not swallowed silently.
| private async ensureDb() { | ||
| if (this.db) return this.db; | ||
|
|
||
| const dir = dirname(STORE_PATH); | ||
| if (!existsSync(dir)) { | ||
| mkdirSync(dir, { recursive: true }); | ||
| } | ||
|
|
||
| this.db = await JSONFilePreset<SessionStoreData>(STORE_PATH, { | ||
| sessions: [], | ||
| }); | ||
| return this.db; | ||
| } |
There was a problem hiding this comment.
Race condition in lazy ensureDb() initialization.
If two callers invoke ensureDb() concurrently before this.db is assigned, both will see null, both will call JSONFilePreset, and one result will be silently discarded. This could lead to lost writes if the discarded instance had already been mutated.
A standard fix is to cache the initialization promise:
🐛 Proposed fix to prevent duplicate initialization
export class SessionStore {
private db: Awaited<
ReturnType<typeof JSONFilePreset<SessionStoreData>>
> | null = null;
+ private dbInitPromise: Promise<
+ Awaited<ReturnType<typeof JSONFilePreset<SessionStoreData>>>
+ > | null = null;
private async ensureDb() {
if (this.db) return this.db;
+ if (this.dbInitPromise) return this.dbInitPromise;
- const dir = dirname(STORE_PATH);
- if (!existsSync(dir)) {
- mkdirSync(dir, { recursive: true });
- }
-
- this.db = await JSONFilePreset<SessionStoreData>(STORE_PATH, {
- sessions: [],
- });
- return this.db;
+ this.dbInitPromise = (async () => {
+ const dir = dirname(STORE_PATH);
+ if (!existsSync(dir)) {
+ mkdirSync(dir, { recursive: true });
+ }
+ const db = await JSONFilePreset<SessionStoreData>(STORE_PATH, {
+ sessions: [],
+ });
+ this.db = db;
+ return db;
+ })();
+
+ return this.dbInitPromise;
}🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-store/session-store.ts`
around lines 54 - 66, ensureDb() can race when called concurrently because
multiple callers may call JSONFilePreset before this.db is assigned; introduce a
cached initialization promise (e.g., private dbInit?: Promise<SessionStoreData>)
and change ensureDb to: if (this.db) return it; if (this.dbInit) return await
it; otherwise set this.dbInit = JSONFilePreset<SessionStoreData>(STORE_PATH, {
sessions: [] }); await the promise, assign the resolved value to this.db, clear
this.dbInit, and ensure you catch errors to clear dbInit on failure so
subsequent calls can retry; reference ensureDb, JSONFilePreset, this.db,
this.dbInit, and STORE_PATH.
| // Auto-title: after first user + assistant exchange, rename session | ||
| const hasAutoTitled = useRef(false); | ||
| useEffect(() => { | ||
| if (hasAutoTitled.current) return; | ||
| if (!sessionId) return; | ||
|
|
||
| const userMsg = messages.find((m) => m.role === "user"); | ||
| const assistantMsg = messages.find((m) => m.role === "assistant"); | ||
| if (!userMsg || !assistantMsg) return; | ||
|
|
||
| hasAutoTitled.current = true; | ||
|
|
||
| // Use first user message as title, truncated | ||
| const textPart = userMsg.parts?.find((p) => p.type === "text"); | ||
| const firstUserText = | ||
| (textPart && "content" in textPart | ||
| ? (textPart.content as string) | ||
| : undefined | ||
| )?.slice(0, 80) ?? "Chat"; | ||
| const title = | ||
| firstUserText.length === 80 ? `${firstUserText}...` : firstUserText; | ||
|
|
||
| renameSessionRef.current.mutate({ sessionId, title }); | ||
| }, [messages, sessionId]); | ||
|
|
||
| // Reset auto-title flag when session changes | ||
| useEffect(() => { | ||
| hasAutoTitled.current = false; | ||
| }, [sessionId]); |
There was a problem hiding this comment.
Auto-title will overwrite existing titles on restored sessions.
When a session is restored, its messages are reloaded from LMDB. Since hasAutoTitled is reset on sessionId change (line 177), and the restored session already has user + assistant messages, the auto-title effect will fire and overwrite whatever title the session already has.
Consider checking whether the session already has a non-default title before renaming:
Proposed fix sketch
// Auto-title: after first user + assistant exchange, rename session
const hasAutoTitled = useRef(false);
useEffect(() => {
if (hasAutoTitled.current) return;
if (!sessionId) return;
+ // Skip auto-titling for restored sessions that already have a real title
+ if (existingSession && existingSession.title && existingSession.title !== "New chat") return;
const userMsg = messages.find((m) => m.role === "user");
const assistantMsg = messages.find((m) => m.role === "assistant");
if (!userMsg || !assistantMsg) return;
hasAutoTitled.current = true;🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 150 - 178, The auto-title effect (uses hasAutoTitled ref and calls
renameSessionRef.current.mutate) is running on restored sessions and overwriting
existing titles; update the effect to early-return when the session already has
a non-default title (e.g., check session?.title and skip auto-rename if it's
present and not the placeholder like "Chat" or empty), so only sessions with no
meaningful title get renamed; keep the existing hasAutoTitled logic and
sessionId reset effect but add the title check before computing firstUserText
and calling renameSessionRef.current.mutate.
| const textPart = userMsg.parts?.find((p) => p.type === "text"); | ||
| const firstUserText = | ||
| (textPart && "content" in textPart | ||
| ? (textPart.content as string) | ||
| : undefined | ||
| )?.slice(0, 80) ?? "Chat"; | ||
| const title = | ||
| firstUserText.length === 80 ? `${firstUserText}...` : firstUserText; |
There was a problem hiding this comment.
Unsafe type assertion and off-by-one in truncation.
Two minor issues:
textPart.content as string(line 166) —contentcould be any shape; a type guard orString(...)coercion is safer.- The truncation check
firstUserText.length === 80(line 170) also triggers for messages that are exactly 80 characters (not actually truncated). Compare against the original length instead.
Proposed fix
- const textPart = userMsg.parts?.find((p) => p.type === "text");
- const firstUserText =
- (textPart && "content" in textPart
- ? (textPart.content as string)
- : undefined
- )?.slice(0, 80) ?? "Chat";
- const title =
- firstUserText.length === 80 ? `${firstUserText}...` : firstUserText;
+ const textPart = userMsg.parts?.find((p) => p.type === "text");
+ const rawText =
+ textPart && "content" in textPart ? String(textPart.content) : undefined;
+ const title = rawText
+ ? rawText.length > 80
+ ? `${rawText.slice(0, 80)}...`
+ : rawText
+ : "Chat";🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 163 - 170, The code unsafely asserts textPart.content as string and
misdetects truncation; fix by extracting the original text with a safe guard
(e.g., let originalText = textPart && "content" in textPart ?
String(textPart.content) : "") then compute truncated = originalText.slice(0,
80) and set title = originalText.length > 80 ? `${truncated}...` : (truncated ||
"Chat"); update the logic around the variables textPart, firstUserText (or
replace it with originalText/truncated), and title so you don't use a blind type
assertion and you compare originalText.length > 80 for truncation.
| const { data: sessions } = electronTrpc.aiChat.listSessions.useQuery( | ||
| { workspaceId }, | ||
| { enabled: isOpen }, | ||
| ); | ||
|
|
||
| const currentSession = sessions?.find( | ||
| (s) => s.sessionId === currentSessionId, | ||
| ); | ||
| const displayTitle = currentSession?.title ?? "Chat"; |
There was a problem hiding this comment.
Session title defaults to "Chat" until dropdown is first opened.
Because enabled: isOpen, the listSessions query never fires until the user opens the dropdown. Until then, sessions is undefined, currentSession is undefined, and displayTitle is always "Chat" — even for a restored session that already has a title in the store.
Consider fetching the current session's title independently (e.g., a lightweight getSession query keyed on currentSessionId that runs on mount), or always enabling the list query, to show the correct title immediately.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/components/SessionSelector/SessionSelector.tsx`
around lines 47 - 55, The title shows "Chat" because
electronTrpc.aiChat.listSessions.useQuery is disabled by { enabled: isOpen } so
sessions is undefined until the dropdown opens; change this by either always
enabling the list query or adding a lightweight query (e.g.,
electronTrpc.aiChat.getSession.useQuery keyed on currentSessionId, run on mount)
to fetch the current session title immediately; update SessionSelector to derive
displayTitle from the getSession result (fallback to sessions if present) or
simply remove the enabled flag so sessions/currentSession/currentSessionId
provide the correct displayTitle on initial render.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts (1)
276-285:⚠️ Potential issue | 🟠 MajorIf
provider.cleanupthrows, session is left in inconsistent state.Line 276: if
this.provider.cleanup(sessionId)throws,store.archive,sessions.delete, and thesession_endevent are all skipped. The session remains in the active map but the proxy resources were already torn down (lines 261-274). Consider wrapping cleanup and archive independently so the method always completes the local tear-down.Proposed fix
- await this.provider.cleanup(sessionId); - await this.store.archive(sessionId); + try { + await this.provider.cleanup(sessionId); + } catch (error) { + console.warn(`[chat/session] Provider cleanup failed for ${sessionId}:`, error); + } + + try { + await this.store.archive(sessionId); + } catch (error) { + console.warn(`[chat/session] Store archive failed for ${sessionId}:`, error); + }
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 117-137: The effect is being retriggered by changes to
existingSession (react-query .data) causing sessions to bounce; add a ref-based
lifecycle guard to ensure the start/restore/stop sequence runs only once per
sessionId: create a ref like lifecycleRanForSessionRef that stores the last
sessionId for which the lifecycle has been executed, return early from the
useEffect if lifecycleRanForSessionRef.current === sessionId, set
lifecycleRanForSessionRef.current = sessionId right before calling
restoreSessionRef.current.mutate or startSessionRef.current.mutate, and reset
lifecycleRanForSessionRef.current to undefined when sessionId becomes falsy or
in the cleanup after stopSessionRef.current.mutate so a new sessionId can run
the lifecycle again; keep using sessionId, cwd, workspaceId, restoreSessionRef,
startSessionRef and stopSessionRef references as in the current effect.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx`:
- Around line 59-67: The handleDeleteSession callback currently calls
deleteSession.mutate fire-and-forget and immediately switches to a new session
which can lose the user's session if deletion fails; change handleDeleteSession
to await the deletion (use deleteSession.mutateAsync in a try/catch) or use
deleteSession.mutate with an onSuccess/onError callback and only call
handleNewChat after a successful deletion, and on errors restore/keep the
current session and surface an error message to the user; update references in
handleDeleteSession, deleteSession.mutate/mutateAsync, and the success/error
handling to ensure no session switch occurs before confirmed deletion.
🧹 Nitpick comments (9)
apps/streams/src/index.ts (1)
19-21: Nit:existsSyncguard is redundant withrecursive: true.
mkdirSync(path, { recursive: true })is a no-op when the directory already exists and won't throw, so theexistsSynccheck can be dropped.♻️ Suggested simplification
-if (!existsSync(DATA_DIR)) { - mkdirSync(DATA_DIR, { recursive: true }); -} +mkdirSync(DATA_DIR, { recursive: true });This also eliminates the (theoretical) TOCTOU race between the check and the create.
apps/streams/src/claude-agent.ts (2)
36-49: Disk data is deserialized with an unsafe cast; validate the shape.
JSON.parse(raw) as Array<[string, SessionEntry]>trusts the file contents completely. A corrupted or hand-edited file could inject entries missingclaudeSessionIdorlastAccessedAt, causing silent downstream failures (e.g.,undefinedpassed to the Claude SDKresumeoption).Additionally, stale sessions (older than
SESSION_TTL_MS) loaded from disk won't be evicted until the nextsetClaudeSessionIdcall, meaning expired mappings can be served bygetClaudeSessionIdafter a restart.Consider adding schema validation and running eviction after loading:
Proposed fix
+const sessionEntrySchema = z.tuple([ + z.string(), + z.object({ + claudeSessionId: z.string(), + lastAccessedAt: z.number(), + }), +]); + function loadPersistedSessions(): void { try { if (existsSync(SESSIONS_FILE)) { const raw = readFileSync(SESSIONS_FILE, "utf-8"); - const entries = JSON.parse(raw) as Array<[string, SessionEntry]>; - for (const [key, entry] of entries) { - claudeSessions.set(key, entry); + const parsed = JSON.parse(raw); + if (!Array.isArray(parsed)) { + console.warn("[claude-agent] Persisted sessions file has unexpected shape, skipping"); + return; + } + let loaded = 0; + for (const item of parsed) { + const result = sessionEntrySchema.safeParse(item); + if (result.success) { + const [key, entry] = result.data; + claudeSessions.set(key, entry); + loaded++; + } } - console.log(`[claude-agent] Loaded ${entries.length} persisted sessions`); + console.log(`[claude-agent] Loaded ${loaded} persisted sessions`); } } catch (err) { console.warn("[claude-agent] Failed to load persisted sessions:", err); } }And after
loadPersistedSessions()on line 63:loadPersistedSessions(); +evictStaleSessions();As per coding guidelines, "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks".
51-61: Non-atomic write risks file corruption on crash.
writeFileSynctruncates the file before writing. If the process crashes mid-write, the sessions file will be left empty or partially written, andloadPersistedSessionswill fail to restore any sessions on next startup.A simple write-then-rename pattern avoids this:
Proposed fix
function persistSessions(): void { try { if (!existsSync(SESSIONS_DIR)) { mkdirSync(SESSIONS_DIR, { recursive: true }); } const entries = Array.from(claudeSessions.entries()); - writeFileSync(SESSIONS_FILE, JSON.stringify(entries), "utf-8"); + const tmpFile = `${SESSIONS_FILE}.tmp`; + writeFileSync(tmpFile, JSON.stringify(entries), "utf-8"); + renameSync(tmpFile, SESSIONS_FILE); } catch (err) { console.warn("[claude-agent] Failed to persist sessions:", err); } }Add
renameSyncto the import on line 1:-import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs";apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts (3)
288-297:updateSessionMetauses positional parameters instead of an object parameter.The coding guidelines require object parameters for functions with 2+ parameters.
Proposed fix
- async updateSessionMeta( - sessionId: string, - patch: { - title?: string; - messagePreview?: string; - providerSessionId?: string; - }, - ): Promise<void> { - await this.store.update(sessionId, patch); + async updateSessionMeta({ + sessionId, + patch, + }: { + sessionId: string; + patch: { + title?: string; + messagePreview?: string; + providerSessionId?: string; + }; + }): Promise<void> { + await this.store.update(sessionId, patch); }Note: This would require updating the call site in
ai-chat/index.ts(line 87) as well.As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments".
56-195:startSessionandrestoreSessionduplicate the proxy PUT + agent POST pattern.Both methods share the same proxy session creation (PUT) and agent registration (POST) flow. Consider extracting a private helper (e.g.,
ensureProxySession) to reduce duplication. The methods would then only differ in the store call (createvsupdate) and logging.
74-77: No timeouts on proxyfetchcalls — could block indefinitely.All
fetchcalls to the proxy (PUT, POST, DELETE) lack anAbortSignalwith a timeout. If the proxy is unresponsive,startSession,restoreSession,deactivateSession, anddeleteSessionwill hang. Consider addingsignal: AbortSignal.timeout(ms)to each request.apps/desktop/src/lib/trpc/routers/ai-chat/index.ts (3)
4-8:ClaudeStreamEventtype name is stale after the provider-agnostic refactor.The type is still named
ClaudeStreamEvent(imported on line 5, used on lines 118-119) even though the session manager is now provider-agnostic (ChatSessionManager). Consider renaming toChatStreamEventorSessionStreamEventfor consistency.
79-91: Consider adding basic validation totitleinput.
z.string()accepts empty strings and arbitrarily long values. Az.string().min(1).max(200)(or similar) would prevent blank titles and excessively long values from reaching the store.
93-103:listSessionsandgetSessionbypass the session manager and callsessionStoredirectly.This is fine for read-only queries, but it introduces a second dependency (
sessionStore) into the router alongsidechatSessionManager, which already holds a reference to the same store. If the manager later adds caching, filtering, or access checks, these queries won't benefit. Worth considering whether to route reads through the manager for consistency, though not blocking.
| useEffect(() => { | ||
| if (!sessionId || !cwd) return; | ||
| if (existingSession === undefined) return; | ||
|
|
||
| hasConnected.current = false; | ||
| setSessionReady(false); | ||
| startSessionRef.current.mutate({ sessionId, cwd }); | ||
|
|
||
| if (existingSession) { | ||
| restoreSessionRef.current.mutate({ sessionId, cwd }); | ||
| } else { | ||
| startSessionRef.current.mutate({ | ||
| sessionId, | ||
| workspaceId, | ||
| cwd, | ||
| }); | ||
| } | ||
|
|
||
| return () => { | ||
| stopSessionRef.current.mutate({ sessionId }); | ||
| }; | ||
| }, [sessionId, cwd]); | ||
| }, [sessionId, cwd, workspaceId, existingSession]); |
There was a problem hiding this comment.
existingSession in the dependency array can cause session re-creation on query refetches.
existingSession is the .data value from a react-query hook. Any background refetch that returns a new object reference (e.g., lastActiveAt changed by deactivateSession) will re-trigger this effect, running the cleanup (stopSession) and then restoreSession again — effectively bouncing the session.
Consider one of:
- Guard with a ref that tracks whether the lifecycle has already run for the current
sessionId. - Remove
existingSessionfrom the deps and instead read it inside the effect via a ref. - Use
staleTime: Infinityon thegetSessionquery to prevent refetches after the initial load.
Proposed fix (option 1 — lifecycle guard ref)
+ const lifecycleRanRef = useRef<string | null>(null);
+
useEffect(() => {
if (!sessionId || !cwd) return;
if (existingSession === undefined) return;
+ if (lifecycleRanRef.current === sessionId) return;
+ lifecycleRanRef.current = sessionId;
hasConnected.current = false;
setSessionReady(false);
if (existingSession) {
restoreSessionRef.current.mutate({ sessionId, cwd });
} else {
startSessionRef.current.mutate({
sessionId,
workspaceId,
cwd,
});
}
return () => {
stopSessionRef.current.mutate({ sessionId });
+ lifecycleRanRef.current = null;
};
}, [sessionId, cwd, workspaceId, existingSession]);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 117 - 137, The effect is being retriggered by changes to
existingSession (react-query .data) causing sessions to bounce; add a ref-based
lifecycle guard to ensure the start/restore/stop sequence runs only once per
sessionId: create a ref like lifecycleRanForSessionRef that stores the last
sessionId for which the lifecycle has been executed, return early from the
useEffect if lifecycleRanForSessionRef.current === sessionId, set
lifecycleRanForSessionRef.current = sessionId right before calling
restoreSessionRef.current.mutate or startSessionRef.current.mutate, and reset
lifecycleRanForSessionRef.current to undefined when sessionId becomes falsy or
in the cleanup after stopSessionRef.current.mutate so a new sessionId can run
the lifecycle again; keep using sessionId, cwd, workspaceId, restoreSessionRef,
startSessionRef and stopSessionRef references as in the current effect.
| const handleDeleteSession = useCallback( | ||
| (sessionIdToDelete: string) => { | ||
| deleteSession.mutate({ sessionId: sessionIdToDelete }); | ||
| if (sessionIdToDelete === sessionId) { | ||
| handleNewChat(); | ||
| } | ||
| }, | ||
| [deleteSession, sessionId, handleNewChat], | ||
| ); |
There was a problem hiding this comment.
Delete-then-switch has no error handling — user loses reference to the session on failure.
deleteSession.mutate is fire-and-forget. If it fails (network error, proxy down), the user has already been switched to a new session (line 63) and loses their reference to the original one. Consider using mutateAsync with a try/catch, or only switching after a successful deletion via the onSuccess callback.
Proposed fix (onSuccess approach)
const handleDeleteSession = useCallback(
(sessionIdToDelete: string) => {
- deleteSession.mutate({ sessionId: sessionIdToDelete });
- if (sessionIdToDelete === sessionId) {
- handleNewChat();
- }
+ deleteSession.mutate(
+ { sessionId: sessionIdToDelete },
+ {
+ onSuccess: () => {
+ if (sessionIdToDelete === sessionId) {
+ handleNewChat();
+ }
+ },
+ onError: (err) => {
+ console.error("[chat/pane] Delete session failed:", err);
+ },
+ },
+ );
},
[deleteSession, sessionId, handleNewChat],
);📝 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.
| const handleDeleteSession = useCallback( | |
| (sessionIdToDelete: string) => { | |
| deleteSession.mutate({ sessionId: sessionIdToDelete }); | |
| if (sessionIdToDelete === sessionId) { | |
| handleNewChat(); | |
| } | |
| }, | |
| [deleteSession, sessionId, handleNewChat], | |
| ); | |
| const handleDeleteSession = useCallback( | |
| (sessionIdToDelete: string) => { | |
| deleteSession.mutate( | |
| { sessionId: sessionIdToDelete }, | |
| { | |
| onSuccess: () => { | |
| if (sessionIdToDelete === sessionId) { | |
| handleNewChat(); | |
| } | |
| }, | |
| onError: (err) => { | |
| console.error("[chat/pane] Delete session failed:", err); | |
| }, | |
| }, | |
| ); | |
| }, | |
| [deleteSession, sessionId, handleNewChat], | |
| ); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx`
around lines 59 - 67, The handleDeleteSession callback currently calls
deleteSession.mutate fire-and-forget and immediately switches to a new session
which can lose the user's session if deletion fails; change handleDeleteSession
to await the deletion (use deleteSession.mutateAsync in a try/catch) or use
deleteSession.mutate with an onSuccess/onError callback and only call
handleNewChat after a successful deletion, and on errors restore/keep the
current session and surface an error message to the user; update references in
handleDeleteSession, deleteSession.mutate/mutateAsync, and the success/error
handling to ensure no session switch occurs before confirmed deletion.
1 participant
Summary
AgentProviderinterface, session metadata store (lowdb), and session manager orchestrator — so future providers (e.g., OpenAI Codex) can be added without touching the UI or persistence layer.Key changes
dataDir, claude agent persistssessionId→claudeSessionIdmapping to disk, newGET /sessions/:idendpointAgentProviderinterface +ClaudeSdkProviderimplementationChatSessionMetastore at~/.superset/chat-sessions.jsondeactivateSession(preserve) vsdeleteSession(destroy)restoreSession,deleteSession,renameSession,listSessions,getSessionswitchChatSessionactionTest plan
~/.superset/chat-sessions.jsonSummary by CodeRabbit