Add low hanging ui features#1304
Conversation
📝 WalkthroughWalkthroughA new thinking-toggle UI control and ThinkingToggle component were added; the UI now persists toggles and model selection via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ChatUI as ChatInterface
participant Router as tRPC Router
participant SessionMgr as ChatSessionManager
participant Proxy as Agent Proxy
User->>ChatUI: toggle thinking / change model
ChatUI->>ChatUI: update local state
ChatUI->>Router: call updateSessionConfig(sessionId, maxThinkingTokens, model)
Router->>SessionMgr: updateAgentConfig(sessionId, maxThinkingTokens, model)
SessionMgr->>Proxy: POST /v1/sessions/{sessionId}/agents with updated registration
Proxy-->>SessionMgr: response
SessionMgr-->>Router: success / throws on error
Router-->>ChatUI: mutation resolved
ChatUI->>ChatUI: reflect persisted state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
![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/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`:
- Around line 312-358: The updateAgentConfig method currently performs fetch
without try/catch; wrap the network call and response check in a try/catch
inside updateAgentConfig to mirror startSession/restoreSession: use
buildProxyHeaders and fetch as before, on any thrown error or non-ok response
call this.emit('error', { sessionId, action: 'updateAgentConfig', error }) and
log a contextual message (including sessionId and maxThinkingTokens), then
rethrow a new Error with contextual details (or rethrow the caught error) so
callers get consistent, informative errors; keep existing registration logic
(this.provider.getAgentRegistration) and response handling (res.ok check) but
move them inside the try block.
- Around line 327-330: updateAgentConfig rebuilds the agent registration without
paneId/tabId, causing routing context loss; add paneId and tabId fields to the
ActiveSession interface and ensure they are populated where sessions are created
(e.g., startSession and restoreSession) so updateAgentConfig can pass
session.paneId and session.tabId into getAgentRegistration when reconstructing
the registration; update references in updateAgentConfig, startSession,
restoreSession, and any session creation logic to set and read these new
ActiveSession properties.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 52-54: The thinkingEnabled local state is never initialized from
the server-side session/agent config so the toggle can be out of sync; update
ChatInterface to derive initial value from the session/agent config (check
session?.config?.maxThinkingTokens or agent?.config?.maxThinkingTokens) and call
setThinkingEnabled(Boolean(maxThinkingTokens)) in a useEffect that runs when the
session/agent changes, and ensure the toggle handler uses updateConfig.mutate to
set maxThinkingTokens to a default positive number when enabling and to 0 when
disabling so UI and backend stay synchronized.
- Around line 222-231: The handler handleThinkingToggle uses a hardcoded magic
number 10000 and calls updateConfig.mutate without error handling; extract the
magic number into a named constant (e.g., DEFAULT_MAX_THINKING_TOKENS in
constants.ts or at module top) and update the call to use that constant, and add
error handling for the mutation (use updateConfig.mutate or its equivalent with
an onError callback) to roll back or reset setThinkingEnabled when the mutation
fails and at minimum log the error so local state doesn't remain out of sync
with the server.
🧹 Nitpick comments (4)
apps/streams/src/claude-agent.ts (1)
35-35: Consider adding bounds validation formaxThinkingTokens.The schema accepts any number (including zero, negative, or extremely large values). The Claude SDK likely expects a positive integer within a reasonable range. Adding
.int().positive()or.min(1024).max(128000)would catch invalid values early at the API boundary rather than letting them propagate to the SDK.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."
Proposed schema tightening
- maxThinkingTokens: z.number().optional(), + maxThinkingTokens: z.number().int().positive().optional(),apps/desktop/src/lib/trpc/routers/ai-chat/index.ts (1)
91-104: Silent success when session is not found could mislead callers.When
updateAgentConfigis called with a non-existentsessionId, the session manager logs a warning and returns without error, so the mutation returns{ success: true }. The frontend won't know the config wasn't actually applied. This is consistent with howinterrupthandles missing sessions, but worth noting since the user may toggle thinking and believe it took effect on a stale/inactive session.Consider returning a
{ success: false }or throwing aTRPCErrorwithNOT_FOUNDcode if the session doesn't exist, so the UI can react accordingly.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ContextIndicator/ContextIndicator.tsx (1)
14-14:MAX_TOKENSis hardcoded but should vary by model.
modelIdis available as a prop but unused for determining the context window size. Different Claude models have different token limits (e.g., 200K for Sonnet, but other models may differ). Consider derivingmaxTokensfrommodelIdvia a lookup, or at minimum adding a comment explaining why 200K is a safe default.packages/ui/src/components/ai-elements/thinking-toggle.tsx (1)
14-16: Unnecessary"onToggle"in theOmit.
onToggleis not a standard prop onButton(or the underlyingHTMLButtonElement), so omitting it has no effect. Only"onClick"needs to be omitted here.Proposed fix
export type ThinkingToggleProps = Omit< ComponentProps<typeof Button>, - "onClick" | "onToggle" + "onClick" > & {
| async updateAgentConfig({ | ||
| sessionId, | ||
| maxThinkingTokens, | ||
| }: { | ||
| sessionId: string; | ||
| maxThinkingTokens?: number | null; | ||
| }): Promise<void> { | ||
| const session = this.sessions.get(sessionId); | ||
| if (!session) { | ||
| console.warn( | ||
| `[chat/session] Session ${sessionId} not found for config update`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const registration = this.provider.getAgentRegistration({ | ||
| sessionId, | ||
| cwd: session.cwd, | ||
| }); | ||
|
|
||
| if (maxThinkingTokens !== undefined) { | ||
| if (maxThinkingTokens === null) { | ||
| delete registration.bodyTemplate.maxThinkingTokens; | ||
| } else { | ||
| registration.bodyTemplate.maxThinkingTokens = maxThinkingTokens; | ||
| } | ||
| } | ||
|
|
||
| const headers = buildProxyHeaders(); | ||
| const res = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/agents`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify({ agents: [registration] }), | ||
| }); | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `POST /v1/sessions/${sessionId}/agents failed: ${res.status}`, | ||
| ); | ||
| } | ||
|
|
||
| console.log( | ||
| `[chat/session] Updated agent config for ${sessionId}`, | ||
| maxThinkingTokens !== undefined | ||
| ? `maxThinkingTokens=${maxThinkingTokens}` | ||
| : "", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing error handling around fetch — inconsistent with other methods.
startSession and restoreSession wrap their fetch calls in try/catch and emit error events. updateAgentConfig lets fetch exceptions propagate as raw Errors, which will surface as INTERNAL_SERVER_ERROR in tRPC without contextual information. Consider wrapping in try/catch for consistency and better error messages.
As per coding guidelines, "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
Proposed error handling
async updateAgentConfig({
sessionId,
maxThinkingTokens,
}: {
sessionId: string;
maxThinkingTokens?: number | null;
}): Promise<void> {
const session = this.sessions.get(sessionId);
if (!session) {
console.warn(
`[chat/session] Session ${sessionId} not found for config update`,
);
return;
}
const registration = this.provider.getAgentRegistration({
sessionId,
cwd: session.cwd,
});
if (maxThinkingTokens !== undefined) {
if (maxThinkingTokens === null) {
delete registration.bodyTemplate.maxThinkingTokens;
} else {
registration.bodyTemplate.maxThinkingTokens = maxThinkingTokens;
}
}
- const headers = buildProxyHeaders();
- const res = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/agents`, {
- method: "POST",
- headers,
- body: JSON.stringify({ agents: [registration] }),
- });
- if (!res.ok) {
- throw new Error(
- `POST /v1/sessions/${sessionId}/agents failed: ${res.status}`,
- );
- }
-
- console.log(
- `[chat/session] Updated agent config for ${sessionId}`,
- maxThinkingTokens !== undefined
- ? `maxThinkingTokens=${maxThinkingTokens}`
- : "",
- );
+ try {
+ const headers = buildProxyHeaders();
+ const res = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/agents`, {
+ method: "POST",
+ headers,
+ body: JSON.stringify({ agents: [registration] }),
+ });
+ if (!res.ok) {
+ throw new Error(
+ `POST /v1/sessions/${sessionId}/agents failed: ${res.status}`,
+ );
+ }
+
+ console.log(
+ `[chat/session] Updated agent config for ${sessionId}`,
+ maxThinkingTokens !== undefined
+ ? `maxThinkingTokens=${maxThinkingTokens}`
+ : "",
+ );
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ console.error(`[chat/session] Failed to update agent config:`, message);
+ throw error;
+ }
}🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 312 - 358, The updateAgentConfig method currently performs fetch
without try/catch; wrap the network call and response check in a try/catch
inside updateAgentConfig to mirror startSession/restoreSession: use
buildProxyHeaders and fetch as before, on any thrown error or non-ok response
call this.emit('error', { sessionId, action: 'updateAgentConfig', error }) and
log a contextual message (including sessionId and maxThinkingTokens), then
rethrow a new Error with contextual details (or rethrow the caught error) so
callers get consistent, informative errors; keep existing registration logic
(this.provider.getAgentRegistration) and response handling (res.ok check) but
move them inside the try block.
| const registration = this.provider.getAgentRegistration({ | ||
| sessionId, | ||
| cwd: session.cwd, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the specific file and lines mentioned
cat -n apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts | head -400 | tail -100Repository: superset-sh/superset
Length of output: 2982
🏁 Script executed:
#!/bin/bash
# Find the updateAgentConfig method to understand context
rg -n 'updateAgentConfig' apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts -B 10 -A 15Repository: superset-sh/superset
Length of output: 733
🏁 Script executed:
#!/bin/bash
# Find startSession and restoreSession to see how they call getAgentRegistration
rg -n 'startSession|restoreSession' apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts -B 5 -A 10Repository: superset-sh/superset
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Find the ActiveSession interface definition
rg -n 'interface ActiveSession' apps/desktop -A 20Repository: superset-sh/superset
Length of output: 2236
🏁 Script executed:
#!/bin/bash
# Find the AgentProvider interface to understand getAgentRegistration signature
rg -n 'getAgentRegistration' apps/desktop -B 2 -A 10 --type=ts | head -100Repository: superset-sh/superset
Length of output: 6883
updateAgentConfig rebuilds registration without paneId/tabId — routing context will be lost.
startSession and restoreSession pass paneId and tabId to getAgentRegistration, but updateAgentConfig omits them. Since ActiveSession does not store these values, they cannot be retrieved when the registration is rebuilt and POSTed back, causing any pane/tab routing information to be lost.
Store paneId and tabId in the ActiveSession interface so they're available when updating the agent config.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 327 - 330, updateAgentConfig rebuilds the agent registration
without paneId/tabId, causing routing context loss; add paneId and tabId fields
to the ActiveSession interface and ensure they are populated where sessions are
created (e.g., startSession and restoreSession) so updateAgentConfig can pass
session.paneId and session.tabId into getAgentRegistration when reconstructing
the registration; update references in updateAgentConfig, startSession,
restoreSession, and any session creation logic to set and read these new
ActiveSession properties.
| const [thinkingEnabled, setThinkingEnabled] = useState(false); | ||
|
|
||
| const updateConfig = electronTrpc.aiChat.updateSessionConfig.useMutation(); |
There was a problem hiding this comment.
thinkingEnabled is not initialized from the server-side session config.
If a session already has maxThinkingTokens set (e.g., after a restore), the toggle will show as disabled even though extended thinking is active on the backend. Consider initializing from the existing session or agent config to keep UI in sync.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 52 - 54, The thinkingEnabled local state is never initialized from
the server-side session/agent config so the toggle can be out of sync; update
ChatInterface to derive initial value from the session/agent config (check
session?.config?.maxThinkingTokens or agent?.config?.maxThinkingTokens) and call
setThinkingEnabled(Boolean(maxThinkingTokens)) in a useEffect that runs when the
session/agent changes, and ensure the toggle handler uses updateConfig.mutate to
set maxThinkingTokens to a default positive number when enabling and to 0 when
disabling so UI and backend stay synchronized.
| const handleThinkingToggle = useCallback( | ||
| (enabled: boolean) => { | ||
| setThinkingEnabled(enabled); | ||
| updateConfig.mutate({ | ||
| sessionId, | ||
| maxThinkingTokens: enabled ? 10000 : null, | ||
| }); | ||
| }, | ||
| [sessionId, updateConfig], | ||
| ); |
There was a problem hiding this comment.
Magic number 10000 and missing error handling on config mutation.
Two issues in this handler:
-
Magic number:
10000should be extracted to a named constant (e.g.,DEFAULT_MAX_THINKING_TOKENSinconstants.ts). As per coding guidelines, hardcoded magic numbers should be extracted to named constants at module top. -
No error handling: If
updateConfig.mutatefails, the localthinkingEnabledstate will be out of sync with the server. AddonErrorto roll back the optimistic state or at minimum log the failure — the coding guidelines require errors not be swallowed silently.
Proposed fix
In constants.ts (or top of file):
const DEFAULT_MAX_THINKING_TOKENS = 10_000;Then in the handler:
const handleThinkingToggle = useCallback(
(enabled: boolean) => {
setThinkingEnabled(enabled);
updateConfig.mutate(
{
sessionId,
- maxThinkingTokens: enabled ? 10000 : null,
+ maxThinkingTokens: enabled ? DEFAULT_MAX_THINKING_TOKENS : null,
+ },
+ {
+ onError: (err) => {
+ console.error("[chat/thinking] Failed to update config:", err);
+ setThinkingEnabled(!enabled);
+ },
},
);
},
[sessionId, updateConfig],
);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 222 - 231, The handler handleThinkingToggle uses a hardcoded magic
number 10000 and calls updateConfig.mutate without error handling; extract the
magic number into a named constant (e.g., DEFAULT_MAX_THINKING_TOKENS in
constants.ts or at module top) and update the call to use that constant, and add
error handling for the mutation (use updateConfig.mutate or its equivalent with
an onError callback) to roll back or reset setThinkingEnabled when the mutation
fails and at minimum log the error so local state doesn't remain out of sync
with the server.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`:
- Around line 233-242: handleModelSelect currently performs an optimistic update
by calling setSelectedModel(model) before updateConfig.mutate and lacks error
rollback; change it to capture the previous model (e.g., const previous =
selectedModel) before setting the new model, pass an onError handler to
updateConfig.mutate that resets selectedModel back to previous and logs the
error, and optionally handle onSettled/onSuccess for consistency; ensure the
onError closure references the correct previous value and that the callback
still depends on sessionId and updateConfig.
🧹 Nitpick comments (1)
apps/streams/src/claude-agent.ts (1)
35-36: Add tighter Zod constraints onmaxThinkingTokensandmodel.These fields come from external callers and are passed directly to the Claude SDK.
maxThinkingTokenscurrently accepts negatives, zero, and floats;modelaccepts an empty string. Both would likely produce opaque downstream API errors.Proposed fix
- maxThinkingTokens: z.number().optional(), - model: z.string().optional(), + maxThinkingTokens: z.number().int().positive().optional(), + model: z.string().min(1).optional(),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".
| const handleModelSelect = useCallback( | ||
| (model: ModelOption) => { | ||
| setSelectedModel(model); | ||
| updateConfig.mutate({ | ||
| sessionId, | ||
| model: model.id, | ||
| }); | ||
| }, | ||
| [sessionId, updateConfig], | ||
| ); |
There was a problem hiding this comment.
handleModelSelect has no error rollback — optimistic state can drift.
Same pattern as handleThinkingToggle: if the mutation fails, selectedModel will reflect a model the backend didn't accept. Add onError to revert and log.
Proposed fix
const handleModelSelect = useCallback(
(model: ModelOption) => {
+ const previousModel = selectedModel;
setSelectedModel(model);
- updateConfig.mutate({
- sessionId,
- model: model.id,
- });
+ updateConfig.mutate(
+ {
+ sessionId,
+ model: model.id,
+ },
+ {
+ onError: (err) => {
+ console.error("[chat/model] Failed to update model:", err);
+ setSelectedModel(previousModel);
+ },
+ },
+ );
},
- [sessionId, updateConfig],
+ [sessionId, updateConfig, selectedModel],
);📝 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 handleModelSelect = useCallback( | |
| (model: ModelOption) => { | |
| setSelectedModel(model); | |
| updateConfig.mutate({ | |
| sessionId, | |
| model: model.id, | |
| }); | |
| }, | |
| [sessionId, updateConfig], | |
| ); | |
| const handleModelSelect = useCallback( | |
| (model: ModelOption) => { | |
| const previousModel = selectedModel; | |
| setSelectedModel(model); | |
| updateConfig.mutate( | |
| { | |
| sessionId, | |
| model: model.id, | |
| }, | |
| { | |
| onError: (err) => { | |
| console.error("[chat/model] Failed to update model:", err); | |
| setSelectedModel(previousModel); | |
| }, | |
| }, | |
| ); | |
| }, | |
| [sessionId, updateConfig, selectedModel], | |
| ); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx`
around lines 233 - 242, handleModelSelect currently performs an optimistic
update by calling setSelectedModel(model) before updateConfig.mutate and lacks
error rollback; change it to capture the previous model (e.g., const previous =
selectedModel) before setting the new model, pass an onError handler to
updateConfig.mutate that resets selectedModel back to previous and logs the
error, and optionally handle onSettled/onSuccess for consistency; ensure the
onError closure references the correct previous value and that the callback
still depends on sessionId and updateConfig.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Style