feat(desktop): connect chat UI to durable session infrastructure#1275
Conversation
Wire the desktop chat pane to the real durable session backend: - ChatInterface uses useDurableChat + tRPC session lifecycle (startSession/stopSession) - ChatMessageItem renders UIMessage.parts[] (text, thinking, tool-call) - ToolCallBlock maps TanStack AI ToolCallPart states to ToolDisplayState - packages/ui tool/confirmation components accept TanStack AI types natively - Fix infinite re-render: use tRPC onSuccess callback + refs instead of unstable mutation objects in useEffect dependency arrays - Update ai-chat-plan.md: mark Phase F as done
📝 WalkthroughWalkthroughAdds durable-session integration and rewrites chat UI to part-based, session-backed streaming: new getConfig tRPC query, session start/stop lifecycle, useDurableChat SSE wiring, updated tool/message types and UI components, CSP and package exports, and documentation updates. Changes
Sequence DiagramsequenceDiagram
participant CP as ChatPane
participant CI as ChatInterface
participant TR as tRPC Router
participant PS as Durable Proxy
participant AI as Claude Agent
CP->>CI: mount(sessionId, cwd)
CI->>TR: startSession({ sessionId, cwd })
TR->>PS: create/register session
TR-->>CI: startSession success
CI->>TR: getConfig()
TR-->>CI: { proxyUrl, authToken }
CI->>PS: connect (useDurableChat SSE + Authorization)
PS-->>CI: SSE stream (parts: thinking / text / tool-call / tool-result)
CI->>CI: render parts, show banners/errors
User->>CI: send message / approve / deny / stop
CI->>PS: send message or approval actions
PS->>AI: forward request
AI-->>PS: stream response
CI->>TR: stopSession({ sessionId }) on unmount
TR->>PS: cleanup session
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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: 4
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 67-94: Add error handling for startSession.mutate and
connectRef.current: add an onError handler to the startSession mutation (the
electronTrpc.aiChat.startSession.useMutation call) that logs the error and sets
a local error state or triggers a user toast so failures are surfaced; wrap both
calls to connectRef.current() (the onSuccess handler inside startSession and the
second useEffect that checks startSession.isSuccess) in try/catch (or use
.catch()) to log the error and set the same error/alert state and avoid marking
hasConnected.current = true on failure; keep stopSession.mutate({ sessionId })
behavior the same but ensure unmount cleanup still clears/handles any error
state so the UI can render an error message or retry option.
In `@docs/ai-chat-plan.md`:
- Around line 769-784: The fenced code block showing the ChatPane directory tree
(listing ChatPane.tsx, ChatInterface.tsx, constants.ts, types.ts,
map-tool-state.ts, ChatMessageItem, ToolCallBlock, ModelPicker,
ContextIndicator, PlanBlock, etc.) is missing a language identifier; update the
opening fence from ``` to a language specifier (for example ```text) so the
block is recognized as plain text and the MD040 lint warning is resolved.
In `@packages/ui/src/components/ai-elements/tool.tsx`:
- Around line 127-149: The ToolInput component assigns displayCode via
JSON.stringify which returns undefined for input === undefined/null; update
ToolInput to guard early for null/undefined (e.g., if input == null set
displayCode = "" or "{}") before attempting JSON.parse/JSON.stringify so
displayCode is always a string; adjust the branches around the existing typeof
input === "string" logic and ensure the value passed into CodeBlock (component
CodeBlock) is a string.
- Line 103: The title expression currently uses nullish coalescing so a
single-segment type like "bash" produces an empty string: update the fallback
logic in the JSX expression that references title and type (the expression using
title ?? type?.split("-").slice(1).join("-") ?? "tool") to guard against
empty-string results — e.g., replace the second nullish-coalescing with a
logical OR or add an explicit check so that if
type?.split("-").slice(1).join("-") yields "" the final fallback becomes "tool".
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/ai-chat/index.ts (1)
18-27: Consider extracting the default proxy URL to a named constant.The hardcoded
"http://localhost:8080"fallback appears here and again inChatInterface.tsx(line 54). Extracting it to a shared constant would prevent drift between the two defaults.♻️ Suggested refactor
In
index.ts(or a shared constants file):+const DEFAULT_PROXY_URL = "http://localhost:8080"; + getConfig: publicProcedure.query(() => ({ - proxyUrl: process.env.DURABLE_STREAM_URL || "http://localhost:8080", + proxyUrl: process.env.DURABLE_STREAM_URL || DEFAULT_PROXY_URL, authToken: process.env.DURABLE_STREAM_AUTH_TOKEN || process.env.DURABLE_STREAM_TOKEN || null, })),As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ToolCallBlock/ToolCallBlock.tsx (1)
55-78: Approval discriminant pattern is correct but could be more explicit.The
"approved" in approvalcheck works becausemapApprovalreturns{ id }(noapprovedkey) vs{ id, approved }. This relies on the absence of a key rather than an explicit discriminant. Consider making the type explicit to avoid future confusion.That said, this is a minor style point — the current logic is functionally correct.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx (1)
44-59: Duplicated default proxy URL — risk of drift.
"http://localhost:8080"appears here (line 54) and in the tRPCgetConfigrouter. If one changes without the other, the fallback behavior diverges. Consider importing a shared constant or omitting the client-side fallback entirely (theconfigquery will resolve the correct URL beforeconnect()is called).♻️ Suggested approach
Since
connect()is gated onconfig?.proxyUrlbeing truthy, you could avoid the fallback entirely and let the hook wait for the real value:const { messages, sendMessage, isLoading, stop, addToolApprovalResponse, connect, } = useDurableChat({ sessionId, - proxyUrl: config?.proxyUrl ?? "http://localhost:8080", + proxyUrl: config?.proxyUrl ?? "", autoConnect: false, stream: config?.authToken ? { headers: { Authorization: `Bearer ${config.authToken}` } } : undefined, });This avoids any accidental connection to the wrong URL if
useDurableChatever changes its behavior.
| ``` | ||
| apps/desktop/src/renderer/screens/chat/ | ||
| ├── index.tsx | ||
| ├── components/ | ||
| │ ├── ChatSidebar.tsx | ||
| │ ├── ChatMessageList.tsx | ||
| │ ├── ChatMessage.tsx -- Renders MessageRow with parts: TextPart, ToolCallPart, etc. | ||
| │ ├── ChatInput.tsx -- Reuse from @superset/durable-session/react | ||
| │ ├── PresenceBar.tsx -- Reuse from @superset/durable-session/react | ||
| │ └── TypingIndicator.tsx | ||
| └── stores/ | ||
| └── chat-store.ts | ||
| apps/desktop/src/renderer/.../ChatPane/ | ||
| ├── ChatPane.tsx -- Threads sessionId + cwd from pane store/workspace | ||
| ├── ChatInterface/ | ||
| │ ├── ChatInterface.tsx -- Core: useDurableChat + tRPC session lifecycle | ||
| │ ├── constants.ts -- MODELS, SUGGESTIONS | ||
| │ ├── types.ts -- ModelOption | ||
| │ ├── utils/ | ||
| │ │ └── map-tool-state.ts -- Maps TanStack AI ToolCallPart states → ToolDisplayState | ||
| │ └── components/ | ||
| │ ├── ChatMessageItem/ -- Renders UIMessage.parts[] (text, thinking, tool-call) | ||
| │ ├── ToolCallBlock/ -- ToolCallPart + ToolResultPart → Tool + Confirmation UI | ||
| │ ├── ModelPicker/ | ||
| │ ├── ContextIndicator/ | ||
| │ └── PlanBlock/ | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block.
The static analysis tool flags this block for missing a language identifier (MD040). Since it's a directory tree, use an empty identifier or a common one like text.
🔧 Proposed fix
-```
+```text
apps/desktop/src/renderer/.../ChatPane/📝 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.
| ``` | |
| apps/desktop/src/renderer/screens/chat/ | |
| ├── index.tsx | |
| ├── components/ | |
| │ ├── ChatSidebar.tsx | |
| │ ├── ChatMessageList.tsx | |
| │ ├── ChatMessage.tsx -- Renders MessageRow with parts: TextPart, ToolCallPart, etc. | |
| │ ├── ChatInput.tsx -- Reuse from @superset/durable-session/react | |
| │ ├── PresenceBar.tsx -- Reuse from @superset/durable-session/react | |
| │ └── TypingIndicator.tsx | |
| └── stores/ | |
| └── chat-store.ts | |
| apps/desktop/src/renderer/.../ChatPane/ | |
| ├── ChatPane.tsx -- Threads sessionId + cwd from pane store/workspace | |
| ├── ChatInterface/ | |
| │ ├── ChatInterface.tsx -- Core: useDurableChat + tRPC session lifecycle | |
| │ ├── constants.ts -- MODELS, SUGGESTIONS | |
| │ ├── types.ts -- ModelOption | |
| │ ├── utils/ | |
| │ │ └── map-tool-state.ts -- Maps TanStack AI ToolCallPart states → ToolDisplayState | |
| │ └── components/ | |
| │ ├── ChatMessageItem/ -- Renders UIMessage.parts[] (text, thinking, tool-call) | |
| │ ├── ToolCallBlock/ -- ToolCallPart + ToolResultPart → Tool + Confirmation UI | |
| │ ├── ModelPicker/ | |
| │ ├── ContextIndicator/ | |
| │ └── PlanBlock/ | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 769-769: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/ai-chat-plan.md` around lines 769 - 784, The fenced code block showing
the ChatPane directory tree (listing ChatPane.tsx, ChatInterface.tsx,
constants.ts, types.ts, map-tool-state.ts, ChatMessageItem, ToolCallBlock,
ModelPicker, ContextIndicator, PlanBlock, etc.) is missing a language
identifier; update the opening fence from ``` to a language specifier (for
example ```text) so the block is recognized as plain text and the MD040 lint
warning is resolved.
| <WrenchIcon className="size-4 text-muted-foreground" /> | ||
| <span className="font-medium text-sm"> | ||
| {title ?? type.split("-").slice(1).join("-")} | ||
| {title ?? type?.split("-").slice(1).join("-") ?? "tool"} |
There was a problem hiding this comment.
Title falls back to empty string for single-segment tool types.
?? (nullish coalescing) only short-circuits on null/undefined, not on "". If type is "bash", then type.split("-").slice(1).join("-") yields "", which ?? treats as a valid value — so the title renders as an empty string instead of "tool".
Use || instead of the second ??, or add an explicit empty-string guard.
🔧 Proposed fix
- {title ?? type?.split("-").slice(1).join("-") ?? "tool"}
+ {title || type?.split("-").slice(1).join("-") || "tool"}📝 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.
| {title ?? type?.split("-").slice(1).join("-") ?? "tool"} | |
| {title || type?.split("-").slice(1).join("-") || "tool"} |
🤖 Prompt for AI Agents
In `@packages/ui/src/components/ai-elements/tool.tsx` at line 103, The title
expression currently uses nullish coalescing so a single-segment type like
"bash" produces an empty string: update the fallback logic in the JSX expression
that references title and type (the expression using title ??
type?.split("-").slice(1).join("-") ?? "tool") to guard against empty-string
results — e.g., replace the second nullish-coalescing with a logical OR or add
an explicit check so that if type?.split("-").slice(1).join("-") yields "" the
final fallback becomes "tool".
| export const ToolInput = ({ className, input, ...props }: ToolInputProps) => { | ||
| let displayCode: string; | ||
| if (typeof input === "string") { | ||
| try { | ||
| displayCode = JSON.stringify(JSON.parse(input), null, 2); | ||
| } catch { | ||
| displayCode = input; | ||
| } | ||
| } else { | ||
| displayCode = JSON.stringify(input, null, 2); | ||
| } | ||
|
|
||
| return ( | ||
| <div className={cn("space-y-2 overflow-hidden p-4", className)} {...props}> | ||
| <h4 className="font-medium text-muted-foreground text-xs uppercase tracking-wide"> | ||
| Parameters | ||
| </h4> | ||
| <div className="rounded-md bg-muted/50"> | ||
| <CodeBlock code={displayCode} language="json" /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| ); | ||
| }; |
There was a problem hiding this comment.
JSON.stringify(undefined) returns undefined (not a string), which would break CodeBlock.
When input is undefined, the else branch on Line 136 assigns displayCode = JSON.stringify(undefined, null, 2), which evaluates to undefined — violating the string type expectation. Consider adding an early guard.
🛡️ Proposed fix — guard against undefined/null input
export const ToolInput = ({ className, input, ...props }: ToolInputProps) => {
+ if (input == null) {
+ return null;
+ }
+
let displayCode: string;
if (typeof input === "string") {📝 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.
| export const ToolInput = ({ className, input, ...props }: ToolInputProps) => { | |
| let displayCode: string; | |
| if (typeof input === "string") { | |
| try { | |
| displayCode = JSON.stringify(JSON.parse(input), null, 2); | |
| } catch { | |
| displayCode = input; | |
| } | |
| } else { | |
| displayCode = JSON.stringify(input, null, 2); | |
| } | |
| return ( | |
| <div className={cn("space-y-2 overflow-hidden p-4", className)} {...props}> | |
| <h4 className="font-medium text-muted-foreground text-xs uppercase tracking-wide"> | |
| Parameters | |
| </h4> | |
| <div className="rounded-md bg-muted/50"> | |
| <CodeBlock code={displayCode} language="json" /> | |
| </div> | |
| </div> | |
| </div> | |
| ); | |
| ); | |
| }; | |
| export const ToolInput = ({ className, input, ...props }: ToolInputProps) => { | |
| if (input == null) { | |
| return null; | |
| } | |
| let displayCode: string; | |
| if (typeof input === "string") { | |
| try { | |
| displayCode = JSON.stringify(JSON.parse(input), null, 2); | |
| } catch { | |
| displayCode = input; | |
| } | |
| } else { | |
| displayCode = JSON.stringify(input, null, 2); | |
| } | |
| return ( | |
| <div className={cn("space-y-2 overflow-hidden p-4", className)} {...props}> | |
| <h4 className="font-medium text-muted-foreground text-xs uppercase tracking-wide"> | |
| Parameters | |
| </h4> | |
| <div className="rounded-md bg-muted/50"> | |
| <CodeBlock code={displayCode} language="json" /> | |
| </div> | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@packages/ui/src/components/ai-elements/tool.tsx` around lines 127 - 149, The
ToolInput component assigns displayCode via JSON.stringify which returns
undefined for input === undefined/null; update ToolInput to guard early for
null/undefined (e.g., if input == null set displayCode = "" or "{}") before
attempting JSON.parse/JSON.stringify so displayCode is always a string; adjust
the branches around the existing typeof input === "string" logic and ensure the
value passed into CodeBlock (component CodeBlock) is a string.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/index.html`:
- Around line 14-18: The meta CSP in index.html currently includes hardcoded
localhost origins in the connect-src directive; update the build-time CSP
replacement logic in apps/desktop/vite/helpers.ts so the localhost entries
(http://localhost:8080 and http://localhost:8081) are only injected for
development builds (e.g., when NODE_ENV or VITE_DEV indicates dev) or when a
dedicated build-time flag (e.g., INCLUDE_LOCALHOST_CSP) is set; specifically,
change the code that replaces %NEXT_PUBLIC_API_URL% or constructs the CSP string
to conditionally append the localhost origins based on that env flag so
production builds omit them.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 54-61: The useDurableChat call in ChatInterface currently passes a
hardcoded fallback proxyUrl ("http://localhost:8080") via the proxyUrl prop
before config resolves, which can cause connections to target localhost or leak
requests; fix by either deferring the useDurableChat hook until config is
available (move the hook into an inner component or return early from
ChatInterface before any hooks run) so proxyUrl is passed only when
config?.proxyUrl is defined, or extract the fallback into a named constant
(e.g., DEFAULT_PROXY_URL) at module top and ensure the hook/reactive code
updates when config changes (verify connect() uses the latest proxyUrl rather
than a captured value). Make changes around the useDurableChat invocation
(sessionId, proxyUrl, autoConnect, stream) and any connect() logic to ensure the
real proxyUrl is used once config loads.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx (2)
110-119:connectionStatusin dependency array is unnecessary.
connectionStatusis only used for theconsole.logon Line 113, not for the send logic itself. Including it in the deps array causeshandleSend(and its dependentshandleSuggestion) to be recreated on every status change, which is wasteful. Consider removing it from deps or moving the log outside the callback.
37-38:selectedModelis tracked but never used for message sending.The
ModelPickerUI lets users select a model, butselectedModelis never passed tosendMessage,startSession, or any other API call. This means the selection has no effect on behavior. If model selection is planned for a future iteration, consider adding a brief comment; otherwise, the dead state may confuse contributors.
| - connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% http://localhost:8080 http://localhost:8081 https://*.posthog.com https://*.sentry.io sentry-ipc:: Allow WebSocket + API (includes Electric proxy) + Durable Streams proxy + PostHog + Sentry | ||
| - img-src 'self' data: %NEXT_PUBLIC_API_URL% https://*.public.blob.vercel-storage.com https://github.com https://avatars.githubusercontent.com: Allow images from same origin + data URIs + API (Linear image proxy) + Vercel blob storage + GitHub avatars | ||
| - font-src 'self': Allow fonts from same origin | ||
| --> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: %NEXT_PUBLIC_API_URL% https://*.public.blob.vercel-storage.com https://github.com https://avatars.githubusercontent.com; font-src 'self';" /> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% http://localhost:8080 http://localhost:8081 https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: %NEXT_PUBLIC_API_URL% https://*.public.blob.vercel-storage.com https://github.com https://avatars.githubusercontent.com; font-src 'self';" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the actual content of the HTML file at the lines in question
echo "=== Content of index.html lines 14-18 ==="
sed -n '14,18p' apps/desktop/src/renderer/index.html
echo -e "\n=== Full search for localhost in HTML files ==="
fd -e html -exec grep -n 'localhost:808[01]' {} +
echo -e "\n=== Search for NEXT_PUBLIC_API_URL usage ==="
rg 'NEXT_PUBLIC_API_URL' --type html --type ts --type js -n
echo -e "\n=== Check for build scripts that process index.html ==="
rg 'index\.html' -g '*.ts' -g '*.js' -g '*.mjs' -g '*.json' -nRepository: superset-sh/superset
Length of output: 11333
🏁 Script executed:
#!/bin/bash
# Check the vite helpers file that processes CSP
echo "=== Content of apps/desktop/vite/helpers.ts ==="
cat -n apps/desktop/vite/helpers.ts | head -100Repository: superset-sh/superset
Length of output: 2679
Hardcoded localhost origins in CSP will ship to production.
http://localhost:8080 and http://localhost:8081 are unconditionally included in the connect-src directive. The build-time CSP processing in apps/desktop/vite/helpers.ts only replaces the %NEXT_PUBLIC_API_URL% placeholder; it does not strip or conditionally inject localhost entries. These will be present in production builds, unnecessarily widening the CSP surface — allowing the renderer to connect to any local service on those ports.
Consider gating these behind a build-time variable or only injecting them in development builds.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/index.html` around lines 14 - 18, The meta CSP in
index.html currently includes hardcoded localhost origins in the connect-src
directive; update the build-time CSP replacement logic in
apps/desktop/vite/helpers.ts so the localhost entries (http://localhost:8080 and
http://localhost:8081) are only injected for development builds (e.g., when
NODE_ENV or VITE_DEV indicates dev) or when a dedicated build-time flag (e.g.,
INCLUDE_LOCALHOST_CSP) is set; specifically, change the code that replaces
%NEXT_PUBLIC_API_URL% or constructs the CSP string to conditionally append the
localhost origins based on that env flag so production builds omit them.
| } = useDurableChat({ | ||
| sessionId, | ||
| proxyUrl: config?.proxyUrl ?? "http://localhost:8080", | ||
| autoConnect: false, | ||
| stream: config?.authToken | ||
| ? { headers: { Authorization: `Bearer ${config.authToken}` } } | ||
| : undefined, | ||
| }); |
There was a problem hiding this comment.
Hardcoded fallback URL may cause unexpected behavior.
The fallback "http://localhost:8080" is a magic string that will be passed to useDurableChat before config resolves. Two concerns:
- If
useDurableChatcaptures theproxyUrlat hook initialization rather than reading it lazily atconnect()time, the connection will targetlocalhost:8080instead of the real proxy — even after config loads. - In production, falling back to
localhost:8080is almost certainly wrong and could leak requests to an unintended local service.
Consider either (a) deferring the hook initialization until config is available, or (b) extracting the fallback to a named constant and verifying that the hook reactively picks up URL changes.
♻️ Option (a): guard with early return
+ const { data: config } = electronTrpc.aiChat.getConfig.useQuery();
+
+ if (!config?.proxyUrl) {
+ return <div className="flex h-full items-center justify-center text-muted-foreground">Loading config…</div>;
+ }
+
const {
messages,
...
} = useDurableChat({
sessionId,
- proxyUrl: config?.proxyUrl ?? "http://localhost:8080",
+ proxyUrl: config.proxyUrl,
autoConnect: false,
...
});Note: this changes hook call order, so the early return must happen before all hooks, or you'd need to restructure (e.g., split into an inner component).
As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 54 - 61, The useDurableChat call in ChatInterface currently passes
a hardcoded fallback proxyUrl ("http://localhost:8080") via the proxyUrl prop
before config resolves, which can cause connections to target localhost or leak
requests; fix by either deferring the useDurableChat hook until config is
available (move the hook into an inner component or return early from
ChatInterface before any hooks run) so proxyUrl is passed only when
config?.proxyUrl is defined, or extract the fallback into a named constant
(e.g., DEFAULT_PROXY_URL) at module top and ensure the hook/reactive code
updates when config changes (verify connect() uses the latest proxyUrl rather
than a captured value). Make changes around the useDurableChat invocation
(sessionId, proxyUrl, autoConnect, stream) and any connect() logic to ensure the
real proxyUrl is used once config loads.
Remove ~100 lines of comments that restate what code does, decorative section dividers, numbered step comments, and verbose JSDoc. Extract getToolDisplayName() and formatJson() helpers in tool.tsx for readability.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/ai-elements/tool.tsx (1)
161-199:⚠️ Potential issue | 🟡 Minor
ToolOutputrenders botherrorTextandOutputsimultaneously.When
errorTextis provided, the component renders the error and the output (line 194-195). If the intent is to show the error instead of the output, theOutputnode should be conditionally omitted. If showing both is intentional (e.g., partial output + error), consider at least guarding theoutput as ReactNodecast on line 171 — for non-renderable values (e.g., plain objects that aren't React elements and don't match thetypeof === "object"+!isValidElementbranch), React will throw at runtime.Suggested guard
{errorText && <div>{errorText}</div>} - {Output} + {!errorText && Output}Or if both should render, guard the default branch:
- let Output = <div>{output as ReactNode}</div>; + let Output: ReactNode = null; if (typeof output === "object" && !isValidElement(output)) { Output = ( <CodeBlock code={JSON.stringify(output, null, 2)} language="json" /> ); } else if (typeof output === "string") { Output = <CodeBlock code={output} language="json" />; + } else if (isValidElement(output)) { + Output = output; }
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 88-96: The effect is re-triggering because the full mutation
objects (startSession and stopSession) are included in the deps; instead,
capture stable refs or the stable .mutate functions and use only those in the
dependency array. Concretely, create refs (e.g., startMutateRef.current =
startSession.mutate and stopMutateRef.current = stopSession.mutate) or
destructure const { mutate: startMutate } = startSession / const { mutate:
stopMutate } = stopSession outside the effect, then use only sessionId, cwd and
the stable mutate refs/functions inside the useEffect; call startMutate(...) on
mount and stopMutate(...) in the cleanup. See useTerminalConnection for the ref
pattern.
🧹 Nitpick comments (3)
packages/ui/src/components/ai-elements/confirmation.tsx (1)
14-25:ToolApprovalalready includesundefinedin its union — the?on the prop is redundant.
ToolApprovalis… | undefined, soapproval?: ToolApprovalinConfirmationProps(line 47) doubles up. This is harmless and TypeScript handles it fine, but it's worth noting for clarity — the| undefinedvariant in the type already communicates optionality.apps/desktop/src/lib/trpc/routers/ai-chat/index.ts (1)
11-17: Extract the fallback proxy URL to a named constant — it's duplicated across files.
"http://localhost:8080"appears here and again inChatInterface.tsx(line 54). Extract it to a shared constant (e.g., in a constants file importable by both main and renderer) to keep the two in sync.♻️ Suggested approach
+const DEFAULT_PROXY_URL = "http://localhost:8080"; + export const createAiChatRouter = () => { return router({ getConfig: publicProcedure.query(() => ({ - proxyUrl: process.env.DURABLE_STREAM_URL || "http://localhost:8080", + proxyUrl: process.env.DURABLE_STREAM_URL || DEFAULT_PROXY_URL, authToken: process.env.DURABLE_STREAM_AUTH_TOKEN || process.env.DURABLE_STREAM_TOKEN || null, })),Then remove the duplicate fallback in
ChatInterface.tsxand rely solely on the config query result.As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ChatMessageItem/ChatMessageItem.tsx (1)
44-49: ThetoolResultslookup map is rebuilt on every render.For typical message sizes this is fine, but if messages grow large (many tool calls), consider wrapping in
useMemokeyed onmessage.parts.
| useEffect(() => { | ||
| if (!sessionId || !cwd) return; | ||
| hasConnected.current = false; | ||
| startSession.mutate({ sessionId, cwd }); | ||
| return () => { | ||
| stopSession.mutate({ sessionId }); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- only on mount/unmount; mutations are stable transports | ||
| }, [sessionId, cwd, startSession, stopSession]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the root package.json for dependencies
find . -maxdepth 2 -name "package.json" -type f | head -1 | xargs cat | jq '.dependencies, .devDependencies' | grep -A2 -B2 'tanstack\|trpc'Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Read the actual ChatInterface component to understand the context
cat -n "apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx" | sed -n '80,110p'Repository: superset-sh/superset
Length of output: 1242
🏁 Script executed:
#!/bin/bash
# Check if there's a custom mutation hook or tRPC wrapper in the codebase
fd -e ts -e tsx "useMutation|useQuery" apps/desktop/src/renderer --max-depth 3 | head -10 | xargs rg "const.*Mutation|export.*Mutation" -A 3Repository: superset-sh/superset
Length of output: 50376
🏁 Script executed:
#!/bin/bash
# Find package.json files and check react-query/tanstack versions
find . -name "package.json" -type f | while read f; do
if grep -q "react-query\|@tanstack" "$f" 2>/dev/null; then
echo "=== $f ==="
cat "$f" | jq '.dependencies, .devDependencies' 2>/dev/null | grep -A1 -B1 "react-query\|@tanstack"
fi
done | head -30Repository: superset-sh/superset
Length of output: 1059
🏁 Script executed:
#!/bin/bash
# Get the full useTerminalConnection hook to understand the pattern better
head -100 "apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts"Repository: superset-sh/superset
Length of output: 2371
🌐 Web query:
@tanstack/react-query useMutation return value stability referential equality
💡 Result:
In @tanstack/react-query, the top-level object returned by useMutation is not referentially stable (it’s a new object every render), so you shouldn’t rely on mutationResult === previousMutationResult, and you shouldn’t put the whole result object into dependency arrays. [1][2]
What is kept stable “as much as possible” are the data references (via structural sharing), so unchanged data should usually keep its reference across renders. [1]
Recommended pattern (deps): destructure what you need and depend on those values (e.g. mutate) rather than the whole result object. The TanStack ESLint rule @tanstack/query/no-unstable-deps explicitly calls this out and shows mutate in the dependency array as the “correct” example. [2]
const { mutate, status, data } = useMutation({ mutationFn })
const onClick = useCallback(() => {
mutate(variables)
}, [mutate])Sources:
[1] TanStack Query “Render Optimizations” → “referential identity” section (v5)
[2] TanStack Query ESLint rule no-unstable-deps (v5)
Mutation result objects in deps will cause repeated start/stop cycles.
startSession and stopSession are the full objects returned by useMutation(). In @tanstack/react-query v5, these objects get a new reference on every render and state transition (idle → loading → success). Only the .mutate function itself is referentially stable.
Including them in the dependency array means: effect fires → startSession.mutate() → mutation status changes → startSession object ref changes → effect re-runs → startSession.mutate() fires again — a start/stop loop.
The eslint-disable comment says "mutations are stable transports," but that applies only to .mutate, not the containing object. This pattern is already handled correctly elsewhere in the codebase (see useTerminalConnection.ts which uses refs to .mutate).
♻️ Suggested fix: use only stable references in deps
+ const startSessionRef = useRef(startSession.mutate);
+ startSessionRef.current = startSession.mutate;
+ const stopSessionRef = useRef(stopSession.mutate);
+ stopSessionRef.current = stopSession.mutate;
+
useEffect(() => {
if (!sessionId || !cwd) return;
hasConnected.current = false;
- startSession.mutate({ sessionId, cwd });
+ startSessionRef.current({ sessionId, cwd });
return () => {
- stopSession.mutate({ sessionId });
+ stopSessionRef.current({ sessionId });
};
- // eslint-disable-next-line react-hooks/exhaustive-deps -- only on mount/unmount; mutations are stable transports
- }, [sessionId, cwd, startSession, stopSession]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- only re-run when session identity changes
+ }, [sessionId, cwd]);📝 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.
| useEffect(() => { | |
| if (!sessionId || !cwd) return; | |
| hasConnected.current = false; | |
| startSession.mutate({ sessionId, cwd }); | |
| return () => { | |
| stopSession.mutate({ sessionId }); | |
| }; | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- only on mount/unmount; mutations are stable transports | |
| }, [sessionId, cwd, startSession, stopSession]); | |
| const startSessionRef = useRef(startSession.mutate); | |
| startSessionRef.current = startSession.mutate; | |
| const stopSessionRef = useRef(stopSession.mutate); | |
| stopSessionRef.current = stopSession.mutate; | |
| useEffect(() => { | |
| if (!sessionId || !cwd) return; | |
| hasConnected.current = false; | |
| startSessionRef.current({ sessionId, cwd }); | |
| return () => { | |
| stopSessionRef.current({ sessionId }); | |
| }; | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- only re-run when session identity changes | |
| }, [sessionId, cwd]); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 88 - 96, The effect is re-triggering because the full mutation
objects (startSession and stopSession) are included in the deps; instead,
capture stable refs or the stable .mutate functions and use only those in the
dependency array. Concretely, create refs (e.g., startMutateRef.current =
startSession.mutate and stopMutateRef.current = stopSession.mutate) or
destructure const { mutate: startMutate } = startSession / const { mutate:
stopMutate } = stopSession outside the effect, then use only sessionId, cwd and
the stable mutate refs/functions inside the useEffect; call startMutate(...) on
mount and stopMutate(...) in the cleanup. See useTerminalConnection for the ref
pattern.
1 participant
Summary
packages/uiAI element components (tool.tsx,confirmation.tsx) to accept TanStack AI types natively via a localToolDisplayStatetype, eliminating all@ts-expect-errorhacksmap-tool-state.tsutility to bridge TanStack AIToolCallPart.state→ToolDisplayStatefor the UI layeronSuccesscallbacks + refs instead of unstable mutation objects inuseEffectdependency arraysdocs/ai-chat-plan.mdHow it works
ChatPanereadssessionIdfrom pane store +cwdfrom workspace queryChatInterfacemounts → tRPCstartSession→ main process → HTTP PUT to proxy (creates session + registers Claude agent)onSuccess→useDurableChat.connect()opens SSE stream from proxysendMessage()→ proxy → Claude agent → streamed response parts → reactive UI viaUIMessage.parts[]stopSessioncleans upKey files
ChatInterface.tsxuseDurableChat+ tRPC lifecycleChatMessageItem.tsxUIMessage.parts[](text, thinking, tool-call)ToolCallBlock.tsxToolCallPart+ToolResultPart, map statesmap-tool-state.tspackages/ui/.../tool.tsxToolDisplayStatetype, acceptunknowninputpackages/ui/.../confirmation.tsxToolDisplayState+ToolApprovaltypesai-chat/index.tsgetConfigquery (proxy URL + auth token)docs/ai-chat-plan.mdTest plan
bunx tsc --noEmitpasses cleanbun run lint:fixpasses cleanapps/streams), open desktop, create chat tab → verify session startsSummary by CodeRabbit
New Features
UI / Messaging
Documentation