-
Notifications
You must be signed in to change notification settings - Fork 992
fix(desktop): collapse v2 chat double header into pane header #3805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import type { RendererContext } from "@superset/panes"; | ||
| import { useCallback } from "react"; | ||
| import { getV2NotificationSourcesForPane } from "renderer/stores/v2-notifications"; | ||
| import { V2NotificationStatusIndicator } from "../../../../../../components/V2NotificationStatusIndicator"; | ||
| import type { ChatPaneData, PaneViewerData } from "../../../../../../types"; | ||
| import { useWorkspaceChatController } from "../../hooks/useWorkspaceChatController"; | ||
| import { SessionSelector } from "../SessionSelector"; | ||
|
|
||
| interface ChatPaneTitleProps { | ||
| context: RendererContext<PaneViewerData>; | ||
| workspaceId: string; | ||
| } | ||
|
|
||
| export function ChatPaneTitle({ context, workspaceId }: ChatPaneTitleProps) { | ||
| const data = context.pane.data as ChatPaneData; | ||
| const { sessionId } = data; | ||
| const { actions } = context; | ||
|
|
||
| const onSessionIdChange = useCallback( | ||
| (nextSessionId: string | null) => { | ||
| actions.updateData({ ...data, sessionId: nextSessionId }); | ||
| }, | ||
| [actions, data], | ||
| ); | ||
|
|
||
| const { | ||
| sessionItems, | ||
| handleSelectSession, | ||
| handleNewChat, | ||
| handleDeleteSession, | ||
| } = useWorkspaceChatController({ | ||
| workspaceId, | ||
| sessionId, | ||
| onSessionIdChange, | ||
| }); | ||
|
Comment on lines
+26
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Inspect useWorkspaceChatController for side effects (queries, subscriptions, mutations, effects).
fd -t f 'useWorkspaceChatController' --exec cat {}
echo "----- callers -----"
rg -nP '\buseWorkspaceChatController\s*\(' -C2Repository: superset-sh/superset Length of output: 8096 🏁 Script executed: #!/bin/bash
# Check if ChatPaneTitle is imported/used in ChatPane
echo "=== Checking ChatPane.tsx for ChatPaneTitle import/usage ==="
cat "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/usePaneRegistry/components/ChatPane/ChatPane.tsx"
echo -e "\n=== Checking ChatPaneTitle full file ==="
cat "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/ChatPaneTitle/ChatPaneTitle.tsx"Repository: superset-sh/superset Length of output: 2825 🏁 Script executed: #!/bin/bash
# Search for imports of ChatPaneTitle
echo "=== Finding ChatPaneTitle imports ==="
rg 'ChatPaneTitle' --type ts --type tsx -g '!node_modules'
# Check what the pane registry structure looks like
echo -e "\n=== Checking pane registry hook to understand pane rendering ==="
fd -t f 'usePaneRegistry' --path '*/hooks/*' | head -5
</script>Repository: superset-sh/superset Length of output: 551 🏁 Script executed: #!/bin/bash
# Search more broadly for ChatPaneTitle
echo "=== Searching for ChatPaneTitle ==="
rg 'ChatPaneTitle' -t ts -t tsx
# Look for how pane titles/components are registered
echo -e "\n=== Finding usePaneRegistry hook ==="
find . -name '*usePaneRegistry*' -type f | grep -v node_modules
# Check if ChatPaneTitle is exported in a barrel export
echo -e "\n=== Checking ChatPane directory index files ==="
find "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/usePaneRegistry/components/ChatPane" -name 'index.*' -type f -exec cat {} \;Repository: superset-sh/superset Length of output: 3700 🏁 Script executed: #!/bin/bash
# Read usePaneRegistry to see how panes are structured
echo "=== usePaneRegistry.tsx ==="
cat "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx"Repository: superset-sh/superset Length of output: 15908 Lift
Consider lifting the hook call to a shared parent (the pane registry's chat config or a context provider scoped to the pane) and passing only the needed values as props to each child, or split the hook into separate "session-list" and "chat-runtime" hooks so side effects run exactly once per pane. 🤖 Prompt for AI Agents |
||
|
|
||
| return ( | ||
| <div className="flex min-w-0 flex-1 items-center gap-1.5"> | ||
| <SessionSelector | ||
| currentSessionId={sessionId} | ||
| sessions={sessionItems} | ||
| fallbackTitle="New Chat" | ||
| onSelectSession={handleSelectSession} | ||
| onNewChat={handleNewChat} | ||
| onDeleteSession={handleDeleteSession} | ||
| /> | ||
| <V2NotificationStatusIndicator | ||
| workspaceId={workspaceId} | ||
| sources={getV2NotificationSourcesForPane(context.pane)} | ||
| /> | ||
| </div> | ||
| ); | ||
|
Comment on lines
+14
to
+52
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The old inline Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/components/ChatPaneTitle/ChatPaneTitle.tsx
Line: 14-52
Comment:
**`context.isActive` unused — pane-focus text styling dropped**
The old inline `renderTitle` applied `ctx.isActive ? "text-foreground" : "text-muted-foreground"` to the title span. `ChatPaneTitle` receives the full `context` (which includes `context.isActive`) but never threads it into `SessionSelector`, so the header title no longer changes color when the pane gains or loses focus. Compare with `FilePaneTabTitle` which explicitly forwards `isActive` to apply that transition. If `SessionSelector` handles its own active-state styling this is fine; otherwise the pane header will appear static regardless of focus.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { ChatPaneTitle } from "./ChatPaneTitle"; |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useWorkspaceChatControllerper paneChatPaneTitle(rendered byrenderTitle) andChatPane(rendered byrenderPane) each independently calluseWorkspaceChatController, so two separateuseLiveQuerysubscriptions and twoworkspace.gettRPC queries run per chat pane.ChatPanestill needsorganizationId,workspacePath, andgetOrCreateSessionfrom the hook, so the overlap can't be eliminated completely — but the subscription to the session list is doubled for every open chat pane. This is consistent with howTerminalSessionDropdownworks in the terminal pane, but worth noting as a scalability consideration.Prompt To Fix With AI