feat(desktop): add chat pane UI and tabs rework#1205
Conversation
- Add chat pages (list + detail view) with message rendering - Add ChatPane component for workspace tab integration - Add ChatInputButton to top bar - Rework tabs store to support chat pane type - Add "chat" pane type to ui-state schema - Update CSP to allow localhost connections for streams - Add STREAMS_URL env variable - Remove old chat-panel-state store - Bump @tanstack/db, electric-db-collection, react-db versions
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive chat feature to the desktop application, including environment configuration for a streams server, new type definitions for chat panes, Zustand stores for session management, reusable chat UI components, new dashboard routes, chat pane implementation for workspaces, and integration with the sidebar navigation. Additionally, PR-related functionality is removed including the associated hotkey. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ChatInputButton as Chat Button
participant ChatStore as Chat Store
participant StreamsServer as Streams Server
participant Router as Router
participant ChatPane as Chat Pane
User->>ChatInputButton: Click "Ask AI..."
activate ChatInputButton
ChatInputButton->>ChatStore: createSession()
activate ChatStore
ChatStore->>ChatStore: generate UUID & timestamp
ChatStore->>StreamsServer: POST sessionId, title
StreamsServer-->>ChatStore: ack
ChatStore-->>ChatInputButton: sessionId
deactivate ChatStore
ChatInputButton->>StreamsServer: createStream(sessionId, URL)
activate StreamsServer
StreamsServer-->>ChatInputButton: stream active
deactivate StreamsServer
ChatInputButton->>Router: navigate to /chats/{sessionId}
Router->>ChatPane: mount with sessionId
activate ChatPane
ChatPane->>StreamsServer: useChatSession(sessionId)
activate StreamsServer
StreamsServer-->>ChatPane: session messages, streaming data
deactivate StreamsServer
ChatPane->>User: render chat interface
deactivate ChatPane
deactivate ChatInputButton
sequenceDiagram
actor User
participant ChatPane as Chat Pane
participant StreamsServer as Streams Server
participant AssistantBackend as AI Backend
participant ChatPane as Chat Display
User->>ChatPane: type message & send
activate ChatPane
ChatPane->>StreamsServer: sendMessage(content, sessionId)
activate StreamsServer
StreamsServer->>AssistantBackend: process message
activate AssistantBackend
loop Stream Response
AssistantBackend-->>StreamsServer: content chunk
StreamsServer-->>ChatPane: update streaming message
ChatPane->>ChatPane: render streaming content
ChatPane->>User: display chunk with thinking/tool calls
end
AssistantBackend-->>StreamsServer: response complete
deactivate AssistantBackend
StreamsServer-->>ChatPane: finalize message
ChatPane->>ChatPane: append to message history
deactivate StreamsServer
ChatPane->>User: show completed assistant message
deactivate ChatPane
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@apps/desktop/electron.vite.config.ts`:
- Around line 147-150: The NEXT_PUBLIC_STREAMS_URL fallback uses
"http://localhost:8080" which is inconsistent with other NEXT_PUBLIC_* defaults
and will break production; update the defineEnv usage for
process.env.NEXT_PUBLIC_STREAMS_URL so that in production it either throws or
requires the env to be present or returns the production endpoint used by the
other NEXT_PUBLIC_* variables; specifically modify the call to
defineEnv(process.env.NEXT_PUBLIC_STREAMS_URL, "http://localhost:8080") to
instead use the production fallback URL (matching the pattern of
NEXT_PUBLIC_API_URL/NEXT_PUBLIC_WEB_URL/NEXT_PUBLIC_DOCS_URL) or add a guard
that throws during production builds when process.env.NEXT_PUBLIC_STREAMS_URL is
undefined, ensuring chat functionality isn't left pointing to localhost.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/components/ChatView/ChatView.tsx`:
- Around line 85-103: The current construction of allMessages in the useMemo
blindly casts m.role as "user" | "assistant", which is unsafe if m.role can be
other values (e.g., "system" or "tool"); update the mapping in allMessages to
validate m.role before casting: add a small role check (or helper like
isValidRole) that accepts only "user" or "assistant" and use it to set role on
the Message objects, and for unexpected values either map to a safe fallback
role (e.g., "assistant") or preserve the original value in a broader union type;
adjust the handling of messages, streamingMessage and types
Message/StreamingMessage accordingly so no unchecked type assertion remains.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/stores/chatStore.ts`:
- Around line 29-42: syncToStreamsServer currently only handles network failures
via .catch and ignores HTTP error responses; change it to await the fetch (make
syncToStreamsServer async) or chain a .then to inspect the Response, and
explicitly check response.ok—if false, read useful details (status and
body/text) and log them using the tidy log prefix pattern (e.g. "[chats/sync]")
instead of "[chatStore]"; ensure any thrown errors are still caught and logged
with the same prefix so both HTTP and network errors are reported.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx`:
- Around line 15-18: The createStream call inside handleClick should be wrapped
in a try/catch so errors are not swallowed: call createSession(), then await
createStream(STREAM_SERVER_URL, session.id) inside a try block and in the catch
block log the error with the same prefixed style used elsewhere (e.g.,
"ChatInputButton:createStream error:" plus the error object), and return early
(do not call navigate) or surface a user-facing error/toast—update handleClick
to reference createStream, createSession, STREAM_SERVER_URL and navigate
accordingly.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/WorkspaceSidebarHeader.tsx`:
- Line 29: isChatOpen currently uses exact matching via matchRoute({ to:
"/chats" }) so nested routes like /chats/:chatId won't mark the Chat button
active; update the check to use fuzzy matching (matchRoute({ to: "/chats",
fuzzy: true })) or equivalent so nested chat routes are included—modify the
isChatOpen declaration where matchRoute is called to add fuzzy: true.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 101-109: The handleAddChat flow creates a session, starts a
stream, and adds a pane without error handling; wrap the createStream and
addChatPane calls in a try/catch (inside handleAddChat) so failures are caught,
and only call addChatPane after createStream succeeds. If createStream fails,
ensure you clean up the created session (or defer session creation until after
createStream) and surface/log the error via your existing logger/UI; reference
handleAddChat, createSession, createStream, addChatPane and STREAM_SERVER_URL
when making the change.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx`:
- Around line 252-258: The mapping of messages to ChatMessageItem unsafely casts
m.role to "user" | "assistant"; update the logic in the messages.map block (the
result variable creation) to validate the role first—either filter out messages
with unexpected roles or normalize them to a safe default (e.g., fallback to
"assistant" or "user") before assigning to the ChatMessageItem.role field—so you
avoid unchecked type assertions and handle unexpected role values gracefully.
- Line 117: Several debug console.log calls in the ChatPane component (e.g., the
render log using paneId/sessionId and other logs around connectionStatus,
isSessionActive, auto-start, etc.) must be removed or gated; update ChatPane to
either remove the console.log statements or wrap them behind a configurable
debug flag/logger (for example a DebugLogger utility or a prop/env flag like
NODE_ENV or isDebugEnabled) and replace any direct console.log usages in
functions/methods within the ChatPane component (search for occurrences of
console.log in ChatPane, including the render path that references paneId and
sessionId and the sections that log connectionStatus, isSessionActive,
auto-starting session, and other listed spots) so that logs only run when
debugging is enabled or use a production-safe logger that can be disabled.
🧹 Nitpick comments (15)
apps/desktop/package.json (1)
57-59: Inconsistent version specifiers for TanStack packages.
@tanstack/electric-db-collectionuses an exact version (0.2.30) while the other two TanStack packages use caret ranges (^0.5.24,^0.1.68). Since these packages are designed to work together, consider using consistent version specifiers to avoid potential compatibility issues during dependency resolution.♻️ Suggested fix for consistency
- "@tanstack/electric-db-collection": "0.2.30", + "@tanstack/electric-db-collection": "^0.2.30",Alternatively, if exact versions are intentional for stability, apply exact versions to all three packages.
apps/desktop/src/shared/tabs-types.ts (1)
129-129: Consider making chat state required for chat panes.
Becausechatis optional, a"chat"pane can be created without asessionId. A discriminated union (bytype) would enforce the correct shape and prevent inconsistent state.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx (2)
7-7: Prefer tsconfig alias for the chat store import.
Use therenderer/...alias (as withenv) to keep imports stable and consistent.As per coding guidelines: Use alias as defined in tsconfig.json when possible.Suggested fix
-import { useChatStore } from "../../../../chats/stores/chatStore"; +import { useChatStore } from "renderer/routes/_authenticated/_dashboard/chats/stores/chatStore";
15-18: Consider an object-parameter wrapper forcreateStream.
Guideline prefers object params for 2+ arguments. If you control@superset/ai-chat/stream, add a wrapper/overload to avoid positional ambiguity.As per coding guidelines: Use object parameters for functions with 2 or more parameters instead of positional arguments.Possible wrapper
+type CreateChatStreamParams = { + streamUrl: string; + sessionId: string; +}; + +function createChatStream({ streamUrl, sessionId }: CreateChatStreamParams) { + return createStream(streamUrl, sessionId); +} + const handleClick = useCallback(async () => { const session = createSession(); - await createStream(STREAM_SERVER_URL, session.id); + await createChatStream({ streamUrl: STREAM_SERVER_URL, sessionId: session.id }); navigate({ to: "/chats/$chatId", params: { chatId: session.id } }); }, [navigate, createSession]);apps/desktop/src/renderer/env.renderer.ts (1)
20-20: Avoid defaulting streams URL to localhost in production.
Defaulting tohttp://localhost:8080means production bundles will target localhost if the env var is missing. Consider making it required in production or limiting the fallback to development.apps/desktop/src/renderer/index.html (1)
14-18: Consider narrowing the localhost CSP allowance.
http://localhost:*is permissive for all builds. If this is primarily for dev, consider gating it by build mode or substituting the concrete streams host (e.g., via an env placeholder) to reduce exposure.apps/desktop/src/lib/trpc/routers/ui-state/index.ts (1)
36-36: Consider enforcing chat payload when type ischat.
Withchatoptional, persisted panes can betype: "chat"withoutsessionId. A discriminated union (bytype) would prevent invalid state.Also applies to: 46-46
apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/stores/chatStore.ts (1)
19-23: Use object params forrenameSession.Guideline preference is to avoid positional args for 2+ parameters; this also makes call sites clearer.
♻️ Proposed signature update
interface ChatStore { sessions: ChatSession[]; createSession: (name?: string) => ChatSession; deleteSession: (id: string) => void; - renameSession: (id: string, name: string) => void; + renameSession: (params: { id: string; name: string }) => void; } @@ - renameSession: (id: string, name: string) => { + renameSession: ({ id, name }: { id: string; name: string }) => { set((state) => ({ sessions: state.sessions.map((s) => s.id === idAlso applies to: 76-83
apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/components/ToolCallPart/ToolCallPart.tsx (1)
40-54: Render non-string tool output safely.If
result.outputis an object/array, it will render as[object Object]. Consider stringifying non-strings while preserving empty strings.🧩 Safer output rendering
- {result.output || "(empty)"} + {result.output == null + ? "(empty)" + : typeof result.output === "string" + ? result.output + : JSON.stringify(result.output, null, 2)}apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/$chatId/page.tsx (2)
29-49: Code duplication withchats/page.tsx.The
handleCreateChatandhandleSelectChatcallbacks are nearly identical to those in the siblingpage.tsx. Consider extracting these to a shared hook (e.g.,useChatNavigation) to reduce duplication and ensure consistent behavior.♻️ Suggested extraction
Create a shared hook in the
chatsfolder:// hooks/useChatNavigation.ts export function useChatNavigation() { const navigate = useNavigate(); const { createSession } = useChatStore(); const handleCreateChat = useCallback(async () => { const session = createSession(); try { await createStream(env.NEXT_PUBLIC_STREAMS_URL, session.id); } catch (err) { console.error("[chats] Failed to create stream:", err); } navigate({ to: "/chats/$chatId", params: { chatId: session.id } }); }, [navigate, createSession]); const handleSelectChat = useCallback( async (id: string) => { try { await createStream(env.NEXT_PUBLIC_STREAMS_URL, id); } catch (err) { console.error("[chats] Failed to ensure stream exists:", err); } navigate({ to: "/chats/$chatId", params: { chatId: id } }); }, [navigate], ); return { handleCreateChat, handleSelectChat }; }
51-88: Sidebar UI duplicated with index page.The entire sidebar component (lines 52-88) is duplicated between this file and
chats/page.tsx. Extract this to a sharedChatSidebarcomponent to maintain consistency and reduce duplication.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/WorkspaceSidebarHeader.tsx (1)
46-48: Chat navigation doesn't toggle like Workspaces.Unlike
handleWorkspacesClickwhich toggles between/workspacesand/workspace,handleChatClickalways navigates to/chats. This may be intentional, but worth confirming whether users should be able to toggle the chat view closed by clicking the button again.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
40-42: Consider extracting the fallback URL to a named constant or config.The fallback
"http://localhost:8080"is a magic string. For consistency with the coding guidelines, consider defining this as a named constant at module top or in a shared config file.♻️ Proposed refactor
+const DEFAULT_STREAM_SERVER_URL = "http://localhost:8080"; + const STREAM_SERVER_URL = - env.NEXT_PUBLIC_STREAMS_URL || "http://localhost:8080"; + env.NEXT_PUBLIC_STREAMS_URL || DEFAULT_STREAM_SERVER_URL;apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/components/ChatView/ChatView.tsx (1)
132-141: Use theButtoncomponent from@superset/uifor consistency.The "Start Session" button uses a raw
<button>element with inline Tailwind classes. For consistency with the rest of the codebase and better maintainability, consider using theButtoncomponent that's already imported elsewhere in this PR.♻️ Proposed refactor
+import { Button } from "@superset/ui/button"; ... - <button - type="button" - onClick={handleStartSession} - disabled={ - startSessionMutation.isPending || !mostRecentWorkspacePath - } - className="px-4 py-2 bg-primary text-primary-foreground rounded-md hover:bg-primary/90 disabled:opacity-50" - > - {startSessionMutation.isPending ? "Starting..." : "Start Session"} - </button> + <Button + onClick={handleStartSession} + disabled={ + startSessionMutation.isPending || !mostRecentWorkspacePath + } + > + {startSessionMutation.isPending ? "Starting..." : "Start Session"} + </Button>apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx (1)
106-112: Consider removing unused props or documenting why they're accepted.Several props are accepted but prefixed with underscore and unused:
_splitPaneHorizontal,_splitPaneVertical,_availableTabs,_onMoveToTab,_onMoveToNewTab. If these are intentionally reserved for future use, consider adding a comment. Otherwise, they could be removed from the interface to reduce confusion.
| "process.env.NEXT_PUBLIC_STREAMS_URL": defineEnv( | ||
| process.env.NEXT_PUBLIC_STREAMS_URL, | ||
| "http://localhost:8080", | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the electron.vite.config.ts file around lines 147-150
cat -n apps/desktop/electron.vite.config.ts | sed -n '140,160p'Repository: superset-sh/superset
Length of output: 917
🏁 Script executed:
# Let's see the full context of the defineEnv usage and the surrounding code
cat -n apps/desktop/electron.vite.config.ts | head -200Repository: superset-sh/superset
Length of output: 7056
🏁 Script executed:
# Find where defineEnv is defined
rg "defineEnv" apps/desktop/ -A 3 -B 3Repository: superset-sh/superset
Length of output: 6264
🏁 Script executed:
# Check if there are environment-specific branching or guards
rg "process.env.NODE_ENV|development|production" apps/desktop/electron.vite.config.ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 707
🏁 Script executed:
# Let's examine the defineEnv function implementation fully
cat -n apps/desktop/vite/helpers.ts | sed -n '1,150p' | grep -A 10 "export function defineEnv"Repository: superset-sh/superset
Length of output: 421
🏁 Script executed:
# Let's see the complete defineEnv function with more context
sed -n '/export function defineEnv/,/^}/p' apps/desktop/vite/helpers.tsRepository: superset-sh/superset
Length of output: 195
🏁 Script executed:
# Check if NEXT_PUBLIC_STREAMS_URL is used anywhere else in the codebase
rg "NEXT_PUBLIC_STREAMS_URL" --type ts --type tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check if there's any runtime validation for streams URL
rg "STREAMS_URL|streams.*url" apps/desktop/src/renderer -i -B 2 -A 2Repository: superset-sh/superset
Length of output: 7748
🏁 Script executed:
# Let's check the env validation more carefully to understand the full picture
cat -n apps/desktop/src/renderer/env.renderer.tsRepository: superset-sh/superset
Length of output: 2482
🏁 Script executed:
# Check if there's build-time environment setup or documentation about required env vars
find apps/desktop -name "*.md" -o -name ".env*" -o -name "*.env" | head -20Repository: superset-sh/superset
Length of output: 929
🏁 Script executed:
# Let's verify what other NEXT_PUBLIC URLs default to
grep -n "NEXT_PUBLIC.*default\|NEXT_PUBLIC.*fallback" apps/desktop/electron.vite.config.ts apps/desktop/src/renderer/env.renderer.tsRepository: superset-sh/superset
Length of output: 518
🏁 Script executed:
# Check for any build scripts or CI configuration that might set these env vars
find . -name "*.yml" -o -name "*.yaml" -o -name "Dockerfile*" -o -name "*.sh" | grep -E "build|deploy|ci|github" | head -20Repository: superset-sh/superset
Length of output: 439
🏁 Script executed:
# Let's check if there's any documentation about building for production
cat -n apps/desktop/BUILDING.md 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 534
🏁 Script executed:
# Check if there's any distinction in how env vars are set between dev and prod builds
rg "NEXT_PUBLIC_STREAMS_URL" -B 5 -A 5 | head -50Repository: superset-sh/superset
Length of output: 4600
🏁 Script executed:
# Let's also verify the pattern: do other URLs also have production fallbacks, and what's the intended pattern?
grep -n "NEXT_PUBLIC.*:" apps/desktop/electron.vite.config.ts | grep "define"Repository: superset-sh/superset
Length of output: 673
Fix inconsistent localhost default for NEXT_PUBLIC_STREAMS_URL.
The fallback to "http://localhost:8080" is inconsistent with other NEXT_PUBLIC URLs (NEXT_PUBLIC_API_URL, NEXT_PUBLIC_WEB_URL, NEXT_PUBLIC_DOCS_URL), which all default to production endpoints. This will break chat functionality in production if the environment variable is not set at build time. Either require NEXT_PUBLIC_STREAMS_URL for production builds or provide a production-appropriate fallback.
🤖 Prompt for AI Agents
In `@apps/desktop/electron.vite.config.ts` around lines 147 - 150, The
NEXT_PUBLIC_STREAMS_URL fallback uses "http://localhost:8080" which is
inconsistent with other NEXT_PUBLIC_* defaults and will break production; update
the defineEnv usage for process.env.NEXT_PUBLIC_STREAMS_URL so that in
production it either throws or requires the env to be present or returns the
production endpoint used by the other NEXT_PUBLIC_* variables; specifically
modify the call to defineEnv(process.env.NEXT_PUBLIC_STREAMS_URL,
"http://localhost:8080") to instead use the production fallback URL (matching
the pattern of NEXT_PUBLIC_API_URL/NEXT_PUBLIC_WEB_URL/NEXT_PUBLIC_DOCS_URL) or
add a guard that throws during production builds when
process.env.NEXT_PUBLIC_STREAMS_URL is undefined, ensuring chat functionality
isn't left pointing to localhost.
| const allMessages = useMemo((): Array<Message | StreamingMessage> => { | ||
| const result: Array<Message | StreamingMessage> = messages.map((m) => ({ | ||
| id: m.id, | ||
| role: m.role as "user" | "assistant", | ||
| content: m.content, | ||
| contentBlocks: m.contentBlocks, | ||
| toolResults: m.toolResults, | ||
| createdAt: m.createdAt, | ||
| })); | ||
| if (streamingMessage) { | ||
| result.push({ | ||
| type: "streaming", | ||
| content: streamingMessage.content, | ||
| contentBlocks: streamingMessage.contentBlocks, | ||
| toolResults: streamingMessage.toolResults, | ||
| }); | ||
| } | ||
| return result; | ||
| }, [messages, streamingMessage]); |
There was a problem hiding this comment.
Type assertion on role may be unsafe.
The cast m.role as "user" | "assistant" assumes the role will always be one of these values. If the upstream data contains other values (e.g., "system" or "tool"), this could lead to rendering issues. Consider adding validation or handling unexpected roles.
🛡️ Proposed fix to validate roles
const allMessages = useMemo((): Array<Message | StreamingMessage> => {
- const result: Array<Message | StreamingMessage> = messages.map((m) => ({
- id: m.id,
- role: m.role as "user" | "assistant",
- content: m.content,
- contentBlocks: m.contentBlocks,
- toolResults: m.toolResults,
- createdAt: m.createdAt,
- }));
+ const result: Array<Message | StreamingMessage> = messages
+ .filter((m) => m.role === "user" || m.role === "assistant")
+ .map((m) => ({
+ id: m.id,
+ role: m.role as "user" | "assistant",
+ content: m.content,
+ contentBlocks: m.contentBlocks,
+ toolResults: m.toolResults,
+ createdAt: m.createdAt,
+ }));🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/components/ChatView/ChatView.tsx`
around lines 85 - 103, The current construction of allMessages in the useMemo
blindly casts m.role as "user" | "assistant", which is unsafe if m.role can be
other values (e.g., "system" or "tool"); update the mapping in allMessages to
validate m.role before casting: add a small role check (or helper like
isValidRole) that accepts only "user" or "assistant" and use it to set role on
the Message objects, and for unexpected values either map to a safe fallback
role (e.g., "assistant") or preserve the original value in a broader union type;
adjust the handling of messages, streamingMessage and types
Message/StreamingMessage accordingly so no unchecked type assertion remains.
| function syncToStreamsServer(session: ChatSession): void { | ||
| const streamsUrl = env.NEXT_PUBLIC_STREAMS_URL; | ||
| if (!streamsUrl) return; | ||
|
|
||
| fetch(`${streamsUrl}/sessions`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| sessionId: session.id, | ||
| title: session.name, | ||
| }), | ||
| }).catch((err) => { | ||
| console.warn("[chatStore] Failed to sync session to streams server:", err); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
fd -t f "chatStore.ts" --search-path "apps/desktop"Repository: superset-sh/superset
Length of output: 148
🏁 Script executed:
# Check if file exists and read it
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/stores/chatStore.ts" | head -100Repository: superset-sh/superset
Length of output: 2735
🏁 Script executed:
# Search for fetch patterns in the renderer to see how errors are typically handled
rg "fetch\(" "apps/desktop/src/renderer/" -A 5 --context 3 | head -80Repository: superset-sh/superset
Length of output: 8778
Handle non-OK sync responses explicitly.
The fetch call only catches network errors via .catch(), but HTTP error responses (4xx/5xx) don't reject—they return a Response with ok: false that goes unhandled. Also, the log prefix should follow the [domain/operation] pattern per logging guidelines.
Suggested update
fetch(`${streamsUrl}/sessions`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
sessionId: session.id,
title: session.name,
}),
- }).catch((err) => {
- console.warn("[chatStore] Failed to sync session to streams server:", err);
- });
+ })
+ .then((res) => {
+ if (!res.ok) {
+ console.warn(
+ "[chatStore/syncToStreamsServer] Streams sync failed:",
+ res.status,
+ res.statusText,
+ );
+ }
+ })
+ .catch((err) => {
+ console.warn(
+ "[chatStore/syncToStreamsServer] Streams sync errored:",
+ err,
+ );
+ });🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/stores/chatStore.ts`
around lines 29 - 42, syncToStreamsServer currently only handles network
failures via .catch and ignores HTTP error responses; change it to await the
fetch (make syncToStreamsServer async) or chain a .then to inspect the Response,
and explicitly check response.ok—if false, read useful details (status and
body/text) and log them using the tidy log prefix pattern (e.g. "[chats/sync]")
instead of "[chatStore]"; ensure any thrown errors are still caught and logged
with the same prefix so both HTTP and network errors are reported.
| const handleClick = useCallback(async () => { | ||
| const session = createSession(); | ||
| await createStream(STREAM_SERVER_URL, session.id); | ||
| navigate({ to: "/chats/$chatId", params: { chatId: session.id } }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and read the relevant lines
if [ -f "apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx" ]; then
echo "=== File found, reading lines 1-30 ==="
head -n 30 "apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx" | cat -n
else
echo "File not found at expected path"
# Try to find it
find . -name "ChatInputButton.tsx" -type f 2>/dev/null | head -5
fiRepository: superset-sh/superset
Length of output: 1382
🏁 Script executed:
# Search for createStream definition to understand what it is
echo "=== Searching for createStream definition ==="
rg "createStream" --type ts --type tsx -B 2 -A 5 | head -50Repository: superset-sh/superset
Length of output: 154
🏁 Script executed:
# Check imports in the file to understand dependencies
echo "=== Checking the full ChatInputButton file ==="
wc -l "apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx"Repository: superset-sh/superset
Length of output: 237
🏁 Script executed:
# Search for createStream without the unrecognized tsx flag
echo "=== Searching for createStream function definition ==="
rg "createStream" -B 3 -A 8 | head -60Repository: superset-sh/superset
Length of output: 6747
🏁 Script executed:
# Check if the file has full error handling elsewhere
echo "=== Reading full ChatInputButton file ==="
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx"Repository: superset-sh/superset
Length of output: 1602
🏁 Script executed:
# Find the `@superset/ai-chat` package to understand createStream
echo "=== Locating ai-chat package and stream module ==="
find . -path "*/ai-chat/*" -name "*stream*" -type f 2>/dev/null | grep -v node_modules | head -10Repository: superset-sh/superset
Length of output: 114
Add error handling and logging to createStream call.
Lines 15-18: The createStream call has no error handling and violates the guideline to never swallow errors silently. Wrap in try/catch with prefixed logging consistent with the pattern used elsewhere in the codebase (e.g., apps/desktop/src/renderer/routes/_authenticated/_dashboard/chats/page.tsx).
Suggested fix
const handleClick = useCallback(async () => {
const session = createSession();
- await createStream(STREAM_SERVER_URL, session.id);
- navigate({ to: "/chats/$chatId", params: { chatId: session.id } });
+ try {
+ await createStream(STREAM_SERVER_URL, session.id);
+ navigate({ to: "/chats/$chatId", params: { chatId: session.id } });
+ } catch (err) {
+ console.error("[chats] Failed to create stream:", err);
+ }
}, [navigate, createSession]);📝 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 handleClick = useCallback(async () => { | |
| const session = createSession(); | |
| await createStream(STREAM_SERVER_URL, session.id); | |
| navigate({ to: "/chats/$chatId", params: { chatId: session.id } }); | |
| const handleClick = useCallback(async () => { | |
| const session = createSession(); | |
| try { | |
| await createStream(STREAM_SERVER_URL, session.id); | |
| navigate({ to: "/chats/$chatId", params: { chatId: session.id } }); | |
| } catch (err) { | |
| console.error("[chats] Failed to create stream:", err); | |
| } | |
| }, [navigate, createSession]); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ChatInputButton/ChatInputButton.tsx`
around lines 15 - 18, The createStream call inside handleClick should be wrapped
in a try/catch so errors are not swallowed: call createSession(), then await
createStream(STREAM_SERVER_URL, session.id) inside a try block and in the catch
block log the error with the same prefixed style used elsewhere (e.g.,
"ChatInputButton:createStream error:" plus the error object), and return early
(do not call navigate) or surface a user-facing error/toast—update handleClick
to reference createStream, createSession, STREAM_SERVER_URL and navigate
accordingly.
|
|
||
| const isWorkspacesListOpen = !!matchRoute({ to: "/workspaces" }); | ||
| const isTasksOpen = !!matchRoute({ to: "/tasks", fuzzy: true }); | ||
| const isChatOpen = !!matchRoute({ to: "/chats" }); |
There was a problem hiding this comment.
Route matching may not highlight on nested chat routes.
The isChatOpen check uses exact matching without fuzzy: true, so navigating to /chats/$chatId won't highlight the Chat button. Compare with isTasksOpen which uses fuzzy: true to match nested routes.
🐛 Proposed fix
- const isChatOpen = !!matchRoute({ to: "/chats" });
+ const isChatOpen = !!matchRoute({ to: "/chats", fuzzy: true });🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/WorkspaceSidebarHeader.tsx`
at line 29, isChatOpen currently uses exact matching via matchRoute({ to:
"/chats" }) so nested routes like /chats/:chatId won't mark the Chat button
active; update the check to use fuzzy matching (matchRoute({ to: "/chats",
fuzzy: true })) or equivalent so nested chat routes are included—modify the
isChatOpen declaration where matchRoute is called to add fuzzy: true.
| const handleAddChat = useCallback(async () => { | ||
| if (!activeWorkspaceId) return; | ||
| const session = createSession(); | ||
| await createStream(STREAM_SERVER_URL, session.id); | ||
| addChatPane(activeWorkspaceId, { | ||
| sessionId: session.id, | ||
| name: session.name, | ||
| }); | ||
| }, [activeWorkspaceId, createSession, addChatPane]); |
There was a problem hiding this comment.
Add error handling for chat creation flow.
The handleAddChat function creates a session, starts a stream, and adds a pane without error handling. If createStream fails, the session is created but the pane won't be added, leaving orphaned state. Consider wrapping in try/catch and cleaning up on failure.
🛡️ Proposed fix to add error handling
const handleAddChat = useCallback(async () => {
if (!activeWorkspaceId) return;
- const session = createSession();
- await createStream(STREAM_SERVER_URL, session.id);
- addChatPane(activeWorkspaceId, {
- sessionId: session.id,
- name: session.name,
- });
+ const session = createSession();
+ try {
+ await createStream(STREAM_SERVER_URL, session.id);
+ addChatPane(activeWorkspaceId, {
+ sessionId: session.id,
+ name: session.name,
+ });
+ } catch (err) {
+ console.error("[GroupStrip/handleAddChat] Failed to create chat:", err);
+ // TODO: Consider deleting the orphaned session or notifying the user
+ }
}, [activeWorkspaceId, createSession, addChatPane]);📝 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 handleAddChat = useCallback(async () => { | |
| if (!activeWorkspaceId) return; | |
| const session = createSession(); | |
| await createStream(STREAM_SERVER_URL, session.id); | |
| addChatPane(activeWorkspaceId, { | |
| sessionId: session.id, | |
| name: session.name, | |
| }); | |
| }, [activeWorkspaceId, createSession, addChatPane]); | |
| const handleAddChat = useCallback(async () => { | |
| if (!activeWorkspaceId) return; | |
| const session = createSession(); | |
| try { | |
| await createStream(STREAM_SERVER_URL, session.id); | |
| addChatPane(activeWorkspaceId, { | |
| sessionId: session.id, | |
| name: session.name, | |
| }); | |
| } catch (err) { | |
| console.error("[GroupStrip/handleAddChat] Failed to create chat:", err); | |
| // TODO: Consider deleting the orphaned session or notifying the user | |
| } | |
| }, [activeWorkspaceId, createSession, addChatPane]); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`
around lines 101 - 109, The handleAddChat flow creates a session, starts a
stream, and adds a pane without error handling; wrap the createStream and
addChatPane calls in a try/catch (inside handleAddChat) so failures are caught,
and only call addChatPane after createStream succeeds. If createStream fails,
ensure you clean up the created session (or defer session creation until after
createStream) and surface/log the error via your existing logger/UI; reference
handleAddChat, createSession, createStream, addChatPane and STREAM_SERVER_URL
when making the change.
| const sessionId = useTabsStore((s) => s.panes[paneId]?.chat?.sessionId); | ||
| const paneName = useTabsStore((s) => s.panes[paneId]?.name); | ||
|
|
||
| console.log(`[ChatPane] Render paneId=${paneId} sessionId=${sessionId}`); |
There was a problem hiding this comment.
Remove or gate extensive console.log statements before merging.
There are numerous console.log statements throughout this component for debugging. These should be removed or gated behind a debug flag before merging to avoid polluting the console in production.
🧹 Example locations to clean up
Line 117: console.log(`[ChatPane] Render paneId=${paneId} sessionId=${sessionId}`);
Line 140-142: console.log(`[ChatPane] connectionStatus=...`);
Line 151-153: console.log(`[ChatPane] isSessionActive=...`);
Line 178-179: console.log(`[ChatPane] Auto-starting session...`);
...and more
Consider either removing these logs or using a debug utility that can be disabled in production.
Also applies to: 140-142, 151-153, 178-179, 184-184, 187-188, 202-202, 207-207, 209-209, 219-219, 229-229, 249-251, 269-278, 328-330
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx`
at line 117, Several debug console.log calls in the ChatPane component (e.g.,
the render log using paneId/sessionId and other logs around connectionStatus,
isSessionActive, auto-start, etc.) must be removed or gated; update ChatPane to
either remove the console.log statements or wrap them behind a configurable
debug flag/logger (for example a DebugLogger utility or a prop/env flag like
NODE_ENV or isDebugEnabled) and replace any direct console.log usages in
functions/methods within the ChatPane component (search for occurrences of
console.log in ChatPane, including the render path that references paneId and
sessionId and the sections that log connectionStatus, isSessionActive,
auto-starting session, and other listed spots) so that logs only run when
debugging is enabled or use a production-safe logger that can be disabled.
| const result: ChatMessageItem[] = messages.map((m) => ({ | ||
| id: m.id, | ||
| role: m.role as "user" | "assistant", | ||
| content: m.content, | ||
| contentBlocks: m.contentBlocks, | ||
| toolResults: m.toolResults, | ||
| })); |
There was a problem hiding this comment.
Same type safety concern with role casting.
Similar to ChatView, the role is cast as "user" | "assistant" without validation. Consider filtering or validating roles to handle unexpected values gracefully.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPane.tsx`
around lines 252 - 258, The mapping of messages to ChatMessageItem unsafely
casts m.role to "user" | "assistant"; update the logic in the messages.map block
(the result variable creation) to validate the role first—either filter out
messages with unexpected roles or normalize them to a safe default (e.g.,
fallback to "assistant" or "user") before assigning to the ChatMessageItem.role
field—so you avoid unchecked type assertions and handle unexpected role values
gracefully.
|
Closing — stale and conflicting, will restart fresh if needed. |
1 participant
Summary
chat-panel-statestoreContext
Split from
ai-chatbranch. Depends on PR #1 (shared-package) + PR #3 (desktop-backend).Test plan
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores