Nice Chat UI#1265
Conversation
📝 WalkthroughWalkthroughAdds a chat feature: new ChatPane and ChatInterface components with supporting UI subcomponents, store APIs and utils to create chat tabs/panes, pane/tab type and state additions, a "New Chat" control gated by a feature flag, and updated AI chat rollout docs. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant GroupStrip as GroupStrip
participant TabsStore as TabsStore
participant TabsUtils as TabsUtils
participant TabView as TabView
participant ChatPane as ChatPane
participant ChatInterface as ChatInterface
User->>GroupStrip: Click "New Chat"
GroupStrip->>TabsStore: addChatTab(workspaceId)
TabsStore->>TabsUtils: createChatTabWithPane(workspaceId)
TabsUtils-->>TabsStore: { tab, pane }
TabsStore->>TabsStore: update tabs, panes, active/focused ids, history
TabsStore-->>TabView: state change -> re-render
TabView->>ChatPane: render pane (type: "chat")
ChatPane->>ChatInterface: mount ChatInterface (workspace/tab/pane props)
ChatInterface-->>User: display chat UI, handle messages
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: 2
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface.tsx`:
- Around line 330-353: The Deny/Allow ConfirmationAction buttons currently have
no click handlers; add onClick handlers to these buttons to call a local handler
(e.g., handleApprovalChange) that updates the approval state for the given
thread context (tc) — set tc.approval.approved to false for Deny and true for
Allow and update the surrounding state/store (use the existing state updater or
dispatch used elsewhere in this component) and pass tc.state if needed;
alternatively, if these are intentionally non-functional placeholders, add a
brief inline comment above the Confirmation block explaining they are
intentionally inert to avoid confusion. Ensure you reference the Confirmation
component and the tc.approval/tc.state objects when wiring the handler.
In `@apps/desktop/src/renderer/stores/tabs/utils.ts`:
- Around line 242-248: The Chat tab naming currently uses
workspaceTabs.filter(...).length which can produce duplicate names after
deletions; replace that simple count with the same gap-filling logic used by
generateTabName: collect existing tab names in the workspace (from
workspaceTabs), extract numeric suffixes for names matching
/^Chat(?:\s+(\d+))?$/, find the smallest missing positive integer (treat plain
"Chat" as 1), and use that value to build the new name (e.g., "Chat" or "Chat
N"); update the tab creation code that sets name (id: tabId, name: ...) to call
or inline the generateTabName behavior so names are unique.
🧹 Nitpick comments (5)
apps/desktop/src/shared/tabs-types.ts (1)
121-130: Minor:ChatPaneStateis declared after its first use inPane.The interface works due to TypeScript hoisting, but every other pane-specific state type in this file (e.g.,
FileViewerState) is defined before thePaneinterface. Consider movingChatPaneStateabovePane(before Line 108) for consistency.apps/desktop/src/renderer/stores/tabs/utils.ts (1)
235-238: Consider using an object parameter forcreateChatTabWithPane.Per coding guidelines, functions with 2+ parameters should use object parameters. This would also be consistent with how other creation functions in this file accept options objects.
♻️ Suggested refactor
-export const createChatTabWithPane = ( - workspaceId: string, - existingTabs: Tab[] = [], -): { tab: Tab; pane: Pane } => { +export const createChatTabWithPane = ({ + workspaceId, + existingTabs = [], +}: { + workspaceId: string; + existingTabs?: Tab[]; +}): { tab: Tab; pane: Pane } => {As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments".
apps/desktop/src/renderer/stores/tabs/store.ts (1)
134-166: The history-stack update logic is duplicated acrossaddTab,addChatTab, andaddTabWithMultiplePanes.All three methods compute
newHistoryStackidentically (Lines 107-112, 141-146, 203-208). Consider extracting a small helper likepushToHistoryStack(stack, currentActiveId)to DRY this up — especially since more tab types may follow.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface.tsx (2)
85-172: Large mock dataset inlined in component file.Consider moving the mock data and associated types (
ChatMessage,ToolCall,PlanData) to a separate file (e.g.,mock-data.ts) within the same folder. This keeps the component file focused on rendering logic and makes it easier to swap in real data later.
197-208:setTimeoutwithout cleanup can update state after unmount.If the component unmounts before the 1s timeout fires,
setMessagesandsetIsLoadingwill be called on an unmounted component. Store the timer ID and clear it on cleanup — or, since this is mock code, at minimum add a comment noting the limitation.♻️ Suggested fix using useRef for cleanup
+import { useCallback, useRef, useState, useEffect } from "react"; ... export function ChatInterface() { const [messages, setMessages] = useState<ChatMessage[]>(MOCK_MESSAGES); const [isLoading, setIsLoading] = useState(false); + const timerRef = useRef<ReturnType<typeof setTimeout>>(null); + + useEffect(() => { + return () => { + if (timerRef.current) clearTimeout(timerRef.current); + }; + }, []); const handleSend = useCallback((message: { text: string }) => { if (!message.text.trim()) return; ... setIsLoading(true); - setTimeout(() => { + timerRef.current = setTimeout(() => { ... }, 1000); }, []);
| const workspaceTabs = existingTabs.filter( | ||
| (t) => t.workspaceId === workspaceId, | ||
| ); | ||
|
|
||
| const tab: Tab = { | ||
| id: tabId, | ||
| name: `Chat ${workspaceTabs.filter((t) => t.name.startsWith("Chat")).length + 1}`, |
There was a problem hiding this comment.
Chat tab naming can produce duplicate names.
The current logic counts tabs starting with "Chat" to compute the next number, but if a user deletes "Chat 1" and creates a new chat, the count stays the same and "Chat 2" could already exist. Compare with generateTabName (Line 261) which scans for gaps. Consider extracting a similar pattern:
♻️ Suggested fix
- const tab: Tab = {
- id: tabId,
- name: `Chat ${workspaceTabs.filter((t) => t.name.startsWith("Chat")).length + 1}`,
+ const existingNumbers = workspaceTabs
+ .map((t) => {
+ const match = t.name.match(/^Chat (\d+)$/);
+ return match ? Number.parseInt(match[1], 10) : 0;
+ })
+ .filter((n) => n > 0);
+
+ let nextNumber = 1;
+ while (existingNumbers.includes(nextNumber)) {
+ nextNumber++;
+ }
+
+ const tab: Tab = {
+ id: tabId,
+ name: `Chat ${nextNumber}`,📝 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 workspaceTabs = existingTabs.filter( | |
| (t) => t.workspaceId === workspaceId, | |
| ); | |
| const tab: Tab = { | |
| id: tabId, | |
| name: `Chat ${workspaceTabs.filter((t) => t.name.startsWith("Chat")).length + 1}`, | |
| const workspaceTabs = existingTabs.filter( | |
| (t) => t.workspaceId === workspaceId, | |
| ); | |
| const existingNumbers = workspaceTabs | |
| .map((t) => { | |
| const match = t.name.match(/^Chat (\d+)$/); | |
| return match ? Number.parseInt(match[1], 10) : 0; | |
| }) | |
| .filter((n) => n > 0); | |
| let nextNumber = 1; | |
| while (existingNumbers.includes(nextNumber)) { | |
| nextNumber++; | |
| } | |
| const tab: Tab = { | |
| id: tabId, | |
| name: `Chat ${nextNumber}`, |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/stores/tabs/utils.ts` around lines 242 - 248, The
Chat tab naming currently uses workspaceTabs.filter(...).length which can
produce duplicate names after deletions; replace that simple count with the same
gap-filling logic used by generateTabName: collect existing tab names in the
workspace (from workspaceTabs), extract numeric suffixes for names matching
/^Chat(?:\s+(\d+))?$/, find the smallest missing positive integer (treat plain
"Chat" as 1), and use that value to build the new name (e.g., "Chat" or "Chat
N"); update the tab creation code that sets name (id: tabId, name: ...) to call
or inline the generateTabName behavior so names are unique.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface.tsx`:
- Around line 510-531: The mapped elements in ChatInterface (Task, TaskItem, and
CodeBlock) use content-based keys (key={task.title}, key={file},
key={block.code}) which can collide; update the key props to use stable unique
identifiers (preferably a unique id on the data like task.id, fileId, block.id)
or fall back to a composite or index-based key (e.g., `${task.id ||
task.title}-${taskIndex}`, `${file}-${fileIndex}`, `${block.id ||
block.code}-${blockIndex}`) for Task, TaskItem, and CodeBlock to ensure keys are
unique and stable across renders.
- Around line 572-581: The Copy and Retry buttons inside the assistant message
block lack click handlers; update the two MessageAction elements (the one
wrapping HiMiniClipboard and the one wrapping HiMiniArrowPath) to include
onClick: for Copy, call navigator.clipboard.writeText(message.content) (with a
try/catch and fallback/notification) to copy message.content; for Retry, invoke
the existing retry function/prop (e.g., retryMessage, onRetryMessage, or
dispatch an event) passing the message or message.id so the message can be
re-sent—if no retry handler exists yet, add a clear TODO comment on the Retry
MessageAction indicating where to wire the retry handler. Ensure you reference
MessageAction, MessageActions, message.content, and the icon-wrapping elements
when making the change.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface.tsx (3)
339-367:selectedModelis tracked but never consumed.The
selectedModelstate (line 342) is set by the model selector but never passed tohandleSendor used when constructing messages. When you wire up the real backend, this will be easy to miss. Consider including it in the send payload now so the plumbing is ready.Also, rapid double-clicks on send can produce colliding IDs since both
Date.now()calls (lines 349, 359) may resolve to the same millisecond. A simple counter orcrypto.randomUUID()would be safer.
523-531: Unsafe type assertion onlanguageprop.
block.language as "typescript"silently discards any non-"typescript"value. If the mock data (or future real data) contains"json","bash", etc., the cast hides the mismatch.Drop the assertion and let the component accept the actual string, or narrow the type in the
codeBlocksinterface.Suggested fix
- <CodeBlock - key={block.code} - code={block.code} - language={block.language as "typescript"} - > + <CodeBlock + key={block.code} + code={block.code} + language={block.language} + >
149-337: ~190 lines of inline mock data adds significant noise.Consider extracting
MOCK_MESSAGES,MODELS, andSUGGESTIONSto a co-locatedmock-data.tsfile. This keeps the component focused on rendering logic and makes the mock data easier to swap out when the real backend is connected.
| {message.role === "assistant" && message.content && ( | ||
| <MessageActions> | ||
| <MessageAction tooltip="Copy"> | ||
| <HiMiniClipboard className="size-3.5" /> | ||
| </MessageAction> | ||
| <MessageAction tooltip="Retry"> | ||
| <HiMiniArrowPath className="size-3.5" /> | ||
| </MessageAction> | ||
| </MessageActions> | ||
| )} |
There was a problem hiding this comment.
Copy and Retry message actions have no click handlers.
Same pattern as the Confirmation buttons — MessageAction components render icons but perform no action on click. Wire up onClick for Copy (e.g., clipboard write of message.content) and Retry, or add a comment marking them as placeholder.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface.tsx`
around lines 572 - 581, The Copy and Retry buttons inside the assistant message
block lack click handlers; update the two MessageAction elements (the one
wrapping HiMiniClipboard and the one wrapping HiMiniArrowPath) to include
onClick: for Copy, call navigator.clipboard.writeText(message.content) (with a
try/catch and fallback/notification) to copy message.content; for Retry, invoke
the existing retry function/prop (e.g., retryMessage, onRetryMessage, or
dispatch an event) passing the message or message.id so the message can be
re-sent—if no retry handler exists yet, add a clear TODO comment on the Retry
MessageAction indicating where to wire the retry handler. Ensure you reference
MessageAction, MessageActions, message.content, and the icon-wrapping elements
when making the change.
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 480-488: The code unsafely asserts block.language as "typescript"
when rendering CodeBlock; update the types so CodeBlock's language prop is a
proper union of allowed literals (e.g., "typescript" | "javascript" | ...)
instead of string in the codeBlocks type definition (types.ts) and change
message.codeBlocks typing to use that union, or if CodeBlock accepts any string,
remove the cast and pass block.language directly to the CodeBlock component;
ensure you update the CodeBlock prop type and the codeBlocks element
(message.codeBlocks and block.language) consistently to restore proper type
checking.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/types.ts (1)
10-13: Consider a discriminated union forapproval.The current union forces consumers to use
"approved" in tc.approvalchecks (as seen inChatInterface.tsxline 509). A tagged discriminant would be more type-safe and idiomatic:♻️ Suggested refactor
- approval?: - | { id: string } - | { id: string; approved: true; reason?: string } - | { id: string; approved: false; reason?: string }; + approval?: + | { id: string; status: "pending" } + | { id: string; status: "approved"; reason?: string } + | { id: string; status: "denied"; reason?: string };This avoids boolean blindness and makes
switch (approval.status)possible. As per coding guidelines: "Avoid boolean blindness by using named properties in options objects instead of passing raw boolean values."apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ChatInterface.tsx (1)
88-294: Extract mock data to a separate file.~200 lines of mock constants (
MODELS,MOCK_MESSAGES,SUGGESTIONS) dominate this component file. Moving them to a siblingmock-data.tswould improve readability and make it easier to remove/replace when the real backend is wired up.
Shows token usage (input, output, reasoning, cache) with cost breakdown on hover, using the Context ai-element component.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ChatMessageItem/ChatMessageItem.tsx`:
- Around line 46-57: The keys for the Task and TaskItem elements use
user-provided strings (key={task.title} and key={file}) which can collide;
update the mapping over message.tasks and task.files to produce stable, unique
keys (e.g., use a composite key that includes the task index or a stable id and
the title, and for files include the parent task id/title plus the file name or
its index) so change the key generation for the Task component in the
message.tasks.map and the TaskItem in task.files.map to a composite key like
`${task.id ?? task.title}-${taskIndex}` and `${task.id ??
task.title}-${file}-${fileIndex}` (or similar) using the map indices or stable
ids to guarantee uniqueness.
- Around line 74-83: The MessageAction buttons rendered inside ChatMessageItem
(the MessageActions wrapper with MessageAction children containing
HiMiniClipboard and HiMiniArrowPath) have no onClick handlers, so the Copy and
Retry controls are non-functional; add appropriate onClick handlers to
MessageAction for copy and retry behavior (or conditionally hide them) by wiring
the Copy button to a copy handler that copies message.content (e.g.,
handleCopyMessage) and wiring the Retry button to a retry handler that
re-submits the message or invokes the existing retry logic (e.g.,
handleRetryMessage or call the parent prop like onRetryMessage), and ensure
these handlers are passed into/defined in ChatMessageItem and bound to the
MessageAction components.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/PlanBlock/PlanBlock.tsx`:
- Around line 21-22: The list key for step items in PlanBlock is using
step.label which can duplicate; update the key to prefer a unique identifier
(e.g., step.id) and fall back to the index when id is missing to avoid React
duplicate-key warnings. Locate the map over plan.steps in the PlanBlock
component and replace the key expression (currently step.label) with a
conditional that uses step.id if present, otherwise uses the index from map.
Ensure the map callback includes the index parameter so you can use it as the
fallback.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ToolCallBlock/ToolCallBlock.tsx`:
- Line 28: The conditional (toolCall.output || toolCall.errorText) in the
ToolCallBlock component wrongly skips rendering ToolOutput for valid falsy
outputs (0, false, "", etc.); change the check to a nullish test so outputs that
are not null/undefined still render—e.g., replace that condition with a
null/undefined check on toolCall.output (toolCall.output != null) combined with
a null/undefined check for toolCall.errorText (toolCall.output != null ||
toolCall.errorText != null) so ToolOutput is shown when output is present even
if falsy.
- Around line 46-51: The "Deny" and "Allow" ConfirmationAction buttons are
missing onClick handlers; update the JSX inside ToolCallBlock where
ConfirmationRequest renders (the block guarded by state ===
"approval-requested") to call the parent/SDK callback when clicked — e.g., add
onClick={() => onDecision('deny')} to the Deny button and onClick={() =>
onDecision('allow')} to the Allow button (or use the existing
submit/approve/deny prop name provided by the parent/AI SDK if different),
ensuring the component invokes the appropriate prop function (e.g., onDecision,
submitApprovalDecision, approveToolCall, denyToolCall) with the correct decision
value so the approval/denial is submitted.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ContextIndicator/ContextIndicator.tsx (1)
14-25: Hardcoded mock data — wire to real chat state when available.
MOCK_CONTEXTis fine for the initial UI scaffold, butContextIndicatorshould accept props (or read from a store) so it can display actual token usage. Consider defining props now so the call site contract is clear, even if you pass mock defaults temporarily.-const MOCK_CONTEXT = { - usedTokens: 84_200, - ... -} as const; - -export function ContextIndicator() { +interface ContextIndicatorProps { + usedTokens: number; + maxTokens: number; + modelId: string; + usage: { + inputTokens: number; + outputTokens: number; + totalTokens: number; + reasoningTokens: number; + cachedInputTokens: number; + }; +} + +export function ContextIndicator(props: ContextIndicatorProps) {This avoids a follow-up refactor to thread real data through and makes the component testable in isolation.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ChatMessageItem/ChatMessageItem.tsx (1)
59-67: Unsafe type assertion onlanguageand fragilekeyusage.Two issues in this block:
as "typescript"cast (Line 63): This silently coerces any language string into a specific literal type, masking type errors at compile time. If theCodeBlocklanguageprop accepts a union, either widen the prop type upstream or validate/map the value instead of casting.
key={block.code}(Line 61): Using the full code string as a React key is fragile — duplicate code blocks will produce key collisions, and long strings are inefficient for key diffing. Prefer an index or a stable identifier.Suggested fix
- {message.codeBlocks?.map((block) => ( + {message.codeBlocks?.map((block, index) => ( <CodeBlock - key={block.code} + key={index} code={block.code} - language={block.language as "typescript"} + language={block.language} > <CodeBlockCopyButton /> </CodeBlock> ))}If
languagegenuinely needs to be constrained, type thecodeBlocksarray with the accepted union inChatMessagerather than casting at the call site.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/ModelPicker.tsx (1)
32-32: Hardcoded"anthropic"provider will need updating for multi-provider support.The provider
"anthropic"is hardcoded in the trigger (Line 32) and in each list item (Line 50), and the group heading"Anthropic"is hardcoded at Line 40. This is consistent with the currentMODELSarray, but consider adding aproviderfield toModelOptionso the logo and grouping can be derived from data when additional providers are introduced.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/constants.ts (1)
21-202: Consider relocating mock data to a dedicated fixtures or test directory.
MOCK_MESSAGESis ~180 lines of demo/mock data that will likely not ship in production. Co-locating it alongside production constants (MODELS,SUGGESTIONS) makes the file harder to navigate and risks accidentally bundling test fixtures. Consider moving it to a__fixtures__or__mocks__directory, or a separatemock-data.tsfile, soconstants.tsstays lean and clearly production-only.
| {message.tasks?.map((task) => ( | ||
| <Task key={task.title}> | ||
| <TaskTrigger title={task.title} /> | ||
| <TaskContent> | ||
| {task.files.map((file) => ( | ||
| <TaskItem key={file}> | ||
| <TaskItemFile>{file}</TaskItemFile> | ||
| </TaskItem> | ||
| ))} | ||
| </TaskContent> | ||
| </Task> | ||
| ))} |
There was a problem hiding this comment.
key={task.title} and key={file} may collide if titles or filenames repeat.
task.title (Line 47) and file (Line 51) are user-supplied strings that aren't guaranteed unique. Duplicate keys cause React reconciliation bugs (silent dropped renders). Consider using the array index or a composite key if no stable id is available.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ChatMessageItem/ChatMessageItem.tsx`
around lines 46 - 57, The keys for the Task and TaskItem elements use
user-provided strings (key={task.title} and key={file}) which can collide;
update the mapping over message.tasks and task.files to produce stable, unique
keys (e.g., use a composite key that includes the task index or a stable id and
the title, and for files include the parent task id/title plus the file name or
its index) so change the key generation for the Task component in the
message.tasks.map and the TaskItem in task.files.map to a composite key like
`${task.id ?? task.title}-${taskIndex}` and `${task.id ??
task.title}-${file}-${fileIndex}` (or similar) using the map indices or stable
ids to guarantee uniqueness.
| {message.role === "assistant" && message.content && ( | ||
| <MessageActions> | ||
| <MessageAction tooltip="Copy"> | ||
| <HiMiniClipboard className="size-3.5" /> | ||
| </MessageAction> | ||
| <MessageAction tooltip="Retry"> | ||
| <HiMiniArrowPath className="size-3.5" /> | ||
| </MessageAction> | ||
| </MessageActions> | ||
| )} |
There was a problem hiding this comment.
Copy and Retry actions have no onClick handlers — buttons are non-functional.
Both MessageAction elements render icons but bind no behavior. Users will see clickable-looking controls that do nothing. If these are placeholders for future work, consider either wiring them up now or hiding them until functional (or adding a // TODO with tracking).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ChatMessageItem/ChatMessageItem.tsx`
around lines 74 - 83, The MessageAction buttons rendered inside ChatMessageItem
(the MessageActions wrapper with MessageAction children containing
HiMiniClipboard and HiMiniArrowPath) have no onClick handlers, so the Copy and
Retry controls are non-functional; add appropriate onClick handlers to
MessageAction for copy and retry behavior (or conditionally hide them) by wiring
the Copy button to a copy handler that copies message.content (e.g.,
handleCopyMessage) and wiring the Retry button to a retry handler that
re-submits the message or invokes the existing retry logic (e.g.,
handleRetryMessage or call the parent prop like onRetryMessage), and ensure
these handlers are passed into/defined in ChatMessageItem and bound to the
MessageAction components.
| {plan.steps.map((step) => ( | ||
| <li key={step.label} className="flex items-center gap-2"> |
There was a problem hiding this comment.
Using step.label as key may produce duplicates.
If two steps share the same label, React will emit a duplicate-key warning and may incorrectly reconcile list items. Prefer a unique identifier (e.g., an id field on each step) or fall back to the index.
Proposed fix (index fallback)
- {plan.steps.map((step) => (
- <li key={step.label} className="flex items-center gap-2">
+ {plan.steps.map((step, index) => (
+ <li key={step.id ?? index} className="flex items-center gap-2">🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/PlanBlock/PlanBlock.tsx`
around lines 21 - 22, The list key for step items in PlanBlock is using
step.label which can duplicate; update the key to prefer a unique identifier
(e.g., step.id) and fall back to the index when id is missing to avoid React
duplicate-key warnings. Locate the map over plan.steps in the PlanBlock
component and replace the key expression (currently step.label) with a
conditional that uses step.id if present, otherwise uses the index from map.
Ensure the map callback includes the index parameter so you can use it as the
fallback.
| /> | ||
| <ToolContent> | ||
| <ToolInput input={toolCall.input} /> | ||
| {(toolCall.output || toolCall.errorText) && ( |
There was a problem hiding this comment.
Falsy output values will skip rendering ToolOutput.
toolCall.output is typed as unknown, so valid falsy values like 0, false, "", or null would cause this condition to short-circuit, hiding the output. Use a nullish check instead.
Proposed fix
- {(toolCall.output || toolCall.errorText) && (
+ {(toolCall.output !== undefined || toolCall.errorText) && (📝 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.
| {(toolCall.output || toolCall.errorText) && ( | |
| {(toolCall.output !== undefined || toolCall.errorText) && ( |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ToolCallBlock/ToolCallBlock.tsx`
at line 28, The conditional (toolCall.output || toolCall.errorText) in the
ToolCallBlock component wrongly skips rendering ToolOutput for valid falsy
outputs (0, false, "", etc.); change the check to a nullish test so outputs that
are not null/undefined still render—e.g., replace that condition with a
null/undefined check on toolCall.output (toolCall.output != null) combined with
a null/undefined check for toolCall.errorText (toolCall.output != null ||
toolCall.errorText != null) so ToolOutput is shown when output is present even
if falsy.
| <ConfirmationRequest> | ||
| <ConfirmationActions> | ||
| <ConfirmationAction variant="outline">Deny</ConfirmationAction> | ||
| <ConfirmationAction>Allow</ConfirmationAction> | ||
| </ConfirmationActions> | ||
| </ConfirmationRequest> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the ConfirmationAction and ConfirmationRequest components
fd "Confirmation" --type f -e tsx -e ts | grep -E "(Confirmation|confirmation)" | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Also search for where these components are defined
rg -l "export.*ConfirmationAction|export.*ConfirmationRequest" --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check the ToolCallBlock.tsx file structure and context around lines 46-51
fd "ToolCallBlock.tsx" --type f | head -5Repository: superset-sh/superset
Length of output: 225
🏁 Script executed:
# Read the ToolCallBlock.tsx file
cat -n "apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ToolCallBlock/ToolCallBlock.tsx"Repository: superset-sh/superset
Length of output: 1917
🏁 Script executed:
# Search for ConfirmationAction and ConfirmationRequest definitions using correct flags
rg "export.*Confirmation" -t ts --type-list | head -10Repository: superset-sh/superset
Length of output: 453
🏁 Script executed:
# Broader search for Confirmation components
rg "ConfirmationAction|ConfirmationRequest" -t ts -lRepository: superset-sh/superset
Length of output: 281
🏁 Script executed:
# Read the confirmation component definition
cat -n packages/ui/src/components/ai-elements/confirmation.tsxRepository: superset-sh/superset
Length of output: 5274
🏁 Script executed:
# Check if there's additional context or if onclick handlers should be passed from parent
rg "ToolCallBlock" -A 5 -B 5 packages/ui/src/components/ai-elements/confirmation.tsxRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's a submission mechanism expected for these approval buttons
rg "approval.*onClick|approve|deny" -t ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/ -A 3Repository: superset-sh/superset
Length of output: 3744
🏁 Script executed:
# Search for how approval responses are typically handled
rg "approval.*respond|submitApproval|handleApproval" -t ts apps/desktop/src/renderer/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's an AI SDK integration or handler for approval state changes
rg "approval-responded|approval-requested" -t ts apps/desktop/src/renderer/ -B 3 -A 3Repository: superset-sh/superset
Length of output: 46
Wire onClick handlers to ConfirmationAction buttons to submit approval decisions.
The "Deny" and "Allow" buttons render but don't trigger any callback when clicked. The ConfirmationRequest component correctly handles visibility (rendering only when state === "approval-requested"), but the buttons themselves are inert and need onClick handlers to submit the approval or denial decision back to the parent component or AI SDK.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ToolCallBlock/ToolCallBlock.tsx`
around lines 46 - 51, The "Deny" and "Allow" ConfirmationAction buttons are
missing onClick handlers; update the JSX inside ToolCallBlock where
ConfirmationRequest renders (the block guarded by state ===
"approval-requested") to call the parent/SDK callback when clicked — e.g., add
onClick={() => onDecision('deny')} to the Deny button and onClick={() =>
onDecision('allow')} to the Allow button (or use the existing
submit/approve/deny prop name provided by the parent/AI SDK if different),
ensuring the component invokes the appropriate prop function (e.g., onDecision,
submitApprovalDecision, approveToolCall, denyToolCall) with the correct decision
value so the approval/denial is submitted.
Hide the "New Chat" button and ChatPane rendering behind the existing AI_CHAT PostHog feature flag (enabled for @superset.sh).
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/index.tsx`:
- Around line 164-178: Pane routing currently checks paneInfo.type === "chat"
but only renders <ChatPane> when hasAiChat is true, so when AI_CHAT is disabled
a chat pane falls through to the terminal <TabPane>; update the logic in
TabView's routing to detect paneInfo.type === "chat" regardless of the flag and
either render a DisabledFeaturePlaceholder (or similar) for chat panes when
hasAiChat is false, or call removePane(paneId) to auto-close legacy chat panes;
modify the block around ChatPane rendering (references: paneInfo.type,
hasAiChat, ChatPane, removePane, TabPane) to implement one of these behaviors
and avoid rendering a terminal for chat panes.
| // Route chat panes to ChatPane component | ||
| if (paneInfo.type === "chat" && hasAiChat) { | ||
| return ( | ||
| <ChatPane | ||
| paneId={paneId} | ||
| path={path} | ||
| isActive={isActive} | ||
| tabId={tab.id} | ||
| workspaceId={tab.workspaceId} | ||
| splitPaneAuto={splitPaneAuto} | ||
| removePane={removePane} | ||
| setFocusedPane={setFocusedPane} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Chat pane falls through to terminal rendering when feature flag is off.
If a chat pane was previously created and the AI_CHAT flag is later disabled, paneInfo.type === "chat" will be true but hasAiChat false, causing it to fall through to the default TabPane (terminal). This would render a broken terminal for a chat pane. Consider rendering a placeholder or auto-removing the pane instead.
// Route chat panes to ChatPane component
- if (paneInfo.type === "chat" && hasAiChat) {
+ if (paneInfo.type === "chat") {
+ if (!hasAiChat) {
+ return (
+ <div className="w-full h-full flex items-center justify-center text-muted-foreground">
+ AI Chat is not available
+ </div>
+ );
+ }
return (
<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.
| // Route chat panes to ChatPane component | |
| if (paneInfo.type === "chat" && hasAiChat) { | |
| return ( | |
| <ChatPane | |
| paneId={paneId} | |
| path={path} | |
| isActive={isActive} | |
| tabId={tab.id} | |
| workspaceId={tab.workspaceId} | |
| splitPaneAuto={splitPaneAuto} | |
| removePane={removePane} | |
| setFocusedPane={setFocusedPane} | |
| /> | |
| ); | |
| } | |
| // Route chat panes to ChatPane component | |
| if (paneInfo.type === "chat") { | |
| if (!hasAiChat) { | |
| return ( | |
| <div className="w-full h-full flex items-center justify-center text-muted-foreground"> | |
| AI Chat is not available | |
| </div> | |
| ); | |
| } | |
| return ( | |
| <ChatPane | |
| paneId={paneId} | |
| path={path} | |
| isActive={isActive} | |
| tabId={tab.id} | |
| workspaceId={tab.workspaceId} | |
| splitPaneAuto={splitPaneAuto} | |
| removePane={removePane} | |
| setFocusedPane={setFocusedPane} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx`
around lines 164 - 178, Pane routing currently checks paneInfo.type === "chat"
but only renders <ChatPane> when hasAiChat is true, so when AI_CHAT is disabled
a chat pane falls through to the terminal <TabPane>; update the logic in
TabView's routing to detect paneInfo.type === "chat" regardless of the flag and
either render a DisabledFeaturePlaceholder (or similar) for chat panes when
hasAiChat is false, or call removePane(paneId) to auto-close legacy chat panes;
modify the block around ChatPane rendering (references: paneInfo.type,
hasAiChat, ChatPane, removePane, TabPane) to implement one of these behaviors
and avoid rendering a terminal for chat panes.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Documentation