Repository navigation
Refactor tabs to windows/panes model - #220
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@saddlepaddle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis PR replaces the tab-focused model with a window/pane model across store, UI, and terminal flows: tabs, group/layout helpers, and many tab-specific components/hooks/tests are removed; a WindowsStore (windows, panes, activeWindowIds, focusedPaneIds) plus WindowView/WindowPane UI are added; terminal/session logic is migrated from tabId to paneId. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI (WindowView / Terminal)
participant Store as WindowsStore
participant TRPC as Terminal TRPC (createOrAttach / stream / write / detach)
UI->>Store: addWindow(workspaceId) → { windowId, paneId }
Store-->>UI: { windowId, paneId }
UI->>TRPC: createOrAttach(paneId, paneName, initialCommands?)
TRPC-->>UI: session established
UI->>TRPC: stream.subscribe(paneId)
TRPC-->>UI: terminal output events
UI->>TRPC: write(paneId, input)
UI->>Store: setFocusedPane(windowId, paneId) %% on focus
Store-->>UI: focusedPaneIds updated
UI->>TRPC: detach(paneId) %% on unmount/close
TRPC-->>UI: session ended
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 3
🧹 Nitpick comments (13)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
173-187: Consider removing unused parameters.The parameters
_windowIdand_paneIdare marked as unused with underscore prefixes but are still required by the function signature. If they're not needed for the focus listener logic, consider simplifying the signature to just(xterm: XTerm, onFocus: () => void).If these are kept for API consistency with the caller (to match the shape expected elsewhere), that's acceptable—but worth a brief comment explaining why they're retained.
export function setupFocusListener( xterm: XTerm, - _windowId: string, - _paneId: string, onFocus: () => void, ): (() => void) | null {apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
49-75: Consider memoizing or extracting theneedsAttentioncomputation.The
collectPaneIdsfunction is redefined on every render, and the entire computation involvingfilter,flatMap, andsomeruns on each render cycle. This can be optimized usinguseMemoto avoid unnecessary recalculations whenwindowsandpaneshaven't changed.+import { useMemo, useState } from "react"; -import { useState } from "react"; ... + // Extract pane IDs from a MosaicNode layout + const collectPaneIds = (node: unknown): string[] => { + if (typeof node === "string") return [node]; + if ( + node && + typeof node === "object" && + "first" in node && + "second" in node + ) { + const branch = node as { first: unknown; second: unknown }; + return [...collectPaneIds(branch.first), ...collectPaneIds(branch.second)]; + } + return []; + }; + + const needsAttention = useMemo(() => { + const workspaceWindows = windows.filter((w) => w.workspaceId === id); + const workspacePaneIds = new Set( + workspaceWindows.flatMap((w) => collectPaneIds(w.layout)), + ); + return Object.values(panes) + .filter((p) => workspacePaneIds.has(p.id)) + .some((p) => p.needsAttention); + }, [windows, panes, id]);Alternatively, extract
collectPaneIdsto a shared utility in your store'sutils.tsfile for reuse elsewhere.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx (1)
74-76: Prop naming inconsistency:tabIdvspaneId.The Terminal component still expects a
tabIdprop while this code passes thepaneId. While this works because Terminal internally aliasestabIdtopaneId(as seen in Terminal.tsx lines 18-19), this creates a confusing naming mismatch in the API surface.Consider renaming the Terminal prop in a follow-up to complete the migration:
// Future cleanup: rename tabId to paneId in TerminalProps <Terminal paneId={paneId} workspaceId={workspaceId} />apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
223-231: Effect dependency onterminalThemecauses full terminal re-initialization on theme changes.Including
terminalThemein the main effect's dependencies (line 229) means the entire terminal instance is disposed and recreated when the theme changes. However, there's a separate effect (lines 234-239) that handles live theme updates viaxterm.options.theme.Consider removing
terminalThemefrom the main effect's dependencies since theme changes can be applied without re-initialization:}, [ paneId, workspaceId, pane?.windowId, workspaceCwd, paneName, - terminalTheme, handleTerminalFocus, ]);This would prevent unnecessary terminal re-creation and preserve terminal state (scrollback, cursor position) when switching themes.
#!/bin/bash # Verify that terminalTheme is only used in createTerminalInstance within this effect rg -n "terminalTheme" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (2)
15-21: Consider extracting shared drag constants.
DRAG_TYPEandDragItemare duplicated in bothTabsViewandWindowItem. Consider moving them to a shared location (e.g., aconstants.tsortypes.tsfile within this folder) to maintain DRY principles.
95-100: Simplify the ref callback.The type assertion for
containerRefis awkward. Consider using a callback that handles both refs more cleanly.<nav - ref={(node) => { - drop(node); - (containerRef as React.MutableRefObject<HTMLElement | null>).current = - node; - }} + ref={(node) => { + drop(node); + containerRef.current = node; + }} className="flex flex-col h-full p-2" >And update the ref type on line 31:
- const containerRef = useRef<HTMLElement>(null); + const containerRef = useRef<HTMLElement | null>(null);apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx (3)
87-94: Consider cleanup for the setTimeout.If the component unmounts before the timeout fires,
inputRef.current?.focus()will attempt to operate on a potentially stale ref. While this is unlikely to cause issues in practice, adding cleanup would be more robust.const startRename = () => { setRenameValue(window.name || displayName); setIsRenaming(true); - setTimeout(() => { - inputRef.current?.focus(); - inputRef.current?.select(); - }, 0); + // Use requestAnimationFrame for safer timing with React's rendering + requestAnimationFrame(() => { + inputRef.current?.focus(); + inputRef.current?.select(); + }); };
69-73: Consider memoizing the panes calculation.Filtering
paneson every render could be avoided withuseMemo, especially if the panes object is large.+ const windowPanes = useMemo( + () => Object.values(panes).filter((p) => p.windowId === window.id), + [panes, window.id], + ); - // Check if any pane in this window needs attention - const windowPanes = Object.values(panes).filter( - (p) => p.windowId === window.id, - ); const needsAttention = windowPanes.some((p) => p.needsAttention);Don't forget to add
useMemoto the imports on line 2.
30-36: Verify the trpc query usage pattern.Each
WindowIteminstance makes its owntrpc.workspaces.getActive.useQuery()call. Since this data is likely shared across all window items, consider lifting this query to the parentTabsViewand passingactiveWorkspaceIdas a prop to reduce redundant network requests.apps/desktop/src/renderer/stores/tabs/types.ts (1)
41-47: Consider consistency in nullable types.
activeWindowIdsusesstring | nullto represent no active window, butfocusedPaneIdsuses juststring. If a window can exist without a focused pane (e.g., during initialization), consider usingstring | nullfor consistency:export interface WindowsState { windows: Window[]; panes: Record<string, Pane>; activeWindowIds: Record<string, string | null>; // workspaceId → windowId - focusedPaneIds: Record<string, string>; // windowId → paneId (last focused pane in each window) + focusedPaneIds: Record<string, string | null>; // windowId → paneId (last focused pane in each window) windowHistoryStacks: Record<string, string[]>; // workspaceId → windowId[] (MRU history) }apps/desktop/src/renderer/stores/tabs/utils.ts (2)
7-9: Potential ID collision in rapid succession.Using
Date.now()as part of the ID could lead to collisions if called multiple times within the same millisecond (e.g., during batch operations). Consider using a counter or UUID library for more robust uniqueness.+let idCounter = 0; + export const generateId = (prefix: string): string => { - return `${prefix}-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; + return `${prefix}-${Date.now()}-${++idCounter}-${Math.random().toString(36).substring(2, 11)}`; };
65-80: Improve efficiency of generateWindowName.The current implementation has O(n²) complexity due to the
whileloop withincludes(). For a large number of windows, this could be optimized using a Set:export const generateWindowName = (existingWindows: Window[]): string => { - const existingNumbers = existingWindows + const existingNumbers = new Set(existingWindows .map((w) => { const match = w.name.match(/^Window (\d+)$/); return match ? Number.parseInt(match[1], 10) : 0; }) - .filter((n) => n > 0); + .filter((n) => n > 0)); // Find the next available number let nextNumber = 1; - while (existingNumbers.includes(nextNumber)) { + while (existingNumbers.has(nextNumber)) { nextNumber++; } return `Window ${nextNumber}`; };apps/desktop/src/renderer/stores/tabs/store.ts (1)
298-326: Consider returning undefined or throwing on failure.
addPanereturns an empty string""when the window is not found (line 302). This could cause issues if callers don't check for empty string. Consider returningundefinedornullto make the failure case more explicit.- addPane: (windowId) => { + addPane: (windowId): string | null => { const state = get(); const window = state.windows.find((w) => w.id === windowId); - if (!window) return ""; + if (!window) return null;This would also require updating the
WindowsStoreinterface intypes.ts:- addPane: (windowId: string) => string; + addPane: (windowId: string) => string | null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(8 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/index.tsx(3 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/validation.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/hooks.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/index.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
💤 Files with no reviewable changes (25)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
- apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts
- apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx
- apps/desktop/src/renderer/stores/tabs/helpers/validation.ts
- apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts
- apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts
- apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts
- apps/desktop/src/renderer/stores/tabs/store.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx
- apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
- apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts
- apps/desktop/src/renderer/stores/tabs/index.ts
- apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts
- apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts
- apps/desktop/src/renderer/stores/tabs/drag-logic.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/hooks.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/hooks.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/hooks.ts
**/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.tsx: Create one folder per component with structure:ComponentName/ComponentName.tsx+index.tsfor barrel export
Co-locate tests next to the component file they test (e.g.,ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nestedcomponents/subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent'scomponents/folder
One component per file - avoid multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/hooks.ts
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods using
window.ipcRenderer.invoke()with object parameters - TypeScript will infer the exact response type automatically
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Call IPC methods using `window.ipcRenderer.invoke()` with object parameters - TypeScript will infer the exact response type automatically
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to packages/ui/src/**/*.tsx : Use shadcn/ui components and TailwindCSS v4 for all UI component styling in the shared UI package
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
🧬 Code graph analysis (13)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (2)
useWindows(4-4)usePanes(5-5)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/useWorkspaceRename.ts (1)
useWorkspaceRename(4-64)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (2)
usePanes(5-5)useSetFocusedPane(29-30)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
setupFocusListener(173-187)setupResizeHandlers(189-213)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
useWindows(4-4)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (1)
WindowView(32-136)
apps/desktop/src/renderer/screens/main/index.tsx (1)
apps/desktop/src/renderer/stores/tabs/hooks.ts (4)
useSplitPaneVertical(37-38)useSplitPaneHorizontal(39-40)useActiveWindowIds(6-7)useFocusedPaneIds(8-9)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx (1)
packages/ui/src/components/context-menu.tsx (5)
ContextMenu(241-241)ContextMenuTrigger(242-242)ContextMenuContent(243-243)ContextMenuItem(244-244)ContextMenuSeparator(248-248)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-560)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (4)
useWindows(4-4)useAddWindow(11-11)useReorderWindowById(20-21)useActiveWindowIds(6-7)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx (1)
WindowItem(30-180)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-560)apps/desktop/src/main/lib/terminal-manager.ts (1)
createOrAttach(41-174)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (3)
Window(30-36)PaneType(6-6)Pane(12-24)apps/desktop/src/renderer/stores/tabs/utils/terminal-naming.ts (1)
generateTerminalName(41-46)
apps/desktop/src/renderer/stores/tabs/store.ts (3)
apps/desktop/src/renderer/stores/tabs/utils.ts (5)
createWindowWithPane(86-108)getPaneIdsForWindow(113-120)extractPaneIdsFromLayout(22-27)createPane(38-59)isLastPaneInWindow(125-130)apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/lib/electron-storage.ts (1)
electronStorage(20-20)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (6)
useWindows(4-4)useActiveWindowIds(6-7)useFocusedPaneIds(8-9)useAddWindow(11-11)useSetActiveWindow(16-17)useRemovePane(27-27)apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(28-151)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-560)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (31)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)
27-35: Naming mismatch between frontendpaneIdand backendtabId.The code correctly adapts to the new windows/panes model by passing
paneIdastabIdfor backwards compatibility with the terminal manager. However, this creates a naming inconsistency that could cause confusion during future maintenance.Consider adding a brief comment explaining this mapping, or plan to update the backend API (
terminal-manager.ts) to usepaneIdterminology in a follow-up PR.if ( Array.isArray(data.initialCommands) && data.initialCommands.length > 0 ) { const { paneId } = addWindow(data.workspace.id); - // Pre-create terminal session with initial commands - // Terminal component will attach to this session when it mounts + // Pre-create terminal session with initial commands + // Terminal component will attach to this session when it mounts + // Note: paneId is passed as tabId for backwards compatibility with terminal-manager createOrAttach.mutate({ tabId: paneId,apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
26-35: LGTM!The
isAlreadyActivecheck correctly validates both workspace and focused pane to determine if attention is needed. The backwards compatibility comment on line 16-17 is helpful for maintainers understanding the tabId→paneId mapping.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx (1)
1-33: LGTM!Clean, focused component with proper typing and appropriate use of the context menu primitives. The destructive styling for "Close Window" is good UX practice.
The component is correctly co-located as a private sub-component within the
WindowItemfolder since it's used only once byWindowItem. No separate barrel export is needed.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (3)
56-67: Potential race condition between pane removal and layout update.Calling
removePanefor each removed pane in a loop (lines 62-64) beforeupdateWindowLayout(line 67) may cause state inconsistencies. The layout update happens after individual pane removals, but eachremovePanecall might trigger its own layout recalculation in the store.Consider batching the pane removals or ensuring the store handles this atomically:
- // Remove panes that were removed via Mosaic UI - for (const removedId of removedPaneIds) { - removePane(removedId); - } - - // Update the layout - updateWindowLayout(window.id, newLayout); + // Update the layout first, then remove orphaned panes + updateWindowLayout(window.id, newLayout); + + // Remove panes that were removed via Mosaic UI + for (const removedId of removedPaneIds) { + removePane(removedId); + }Alternatively, verify that your store actions are designed to handle this sequence correctly.
72-110: Well-structured renderPane callback with proper defensive handling.The fallback for missing panes (lines 77-83) is a good defensive practice that prevents crashes if the panes store becomes out of sync with the layout.
112-136: Clean implementation of the Mosaic layout with appropriate empty state handling.The empty state message clearly communicates that the window will close when no panes remain, which aligns well with the PR's invariant that windows always have ≥1 pane.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
1-35: Clean refactor to window-centric rendering.The simplified logic correctly derives the active window from workspace context and renders the appropriate view. The implementation is concise and follows the minimal diff principle.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx (1)
51-79: Well-structured pane component with proper event handling.The component correctly:
- Uses
stopPropagationon close to prevent focus events- Wires context menu actions with the path parameter for split operations
- Applies focused styling conditionally
apps/desktop/src/renderer/screens/main/index.tsx (2)
67-77: Verify split direction mapping is intentional.The hotkey mapping appears swapped:
SPLIT_HORIZONTAL(Cmd+D) callssplitPaneVerticalSPLIT_VERTICAL(Cmd+Shift+D) callssplitPaneHorizontalThis could be intentional depending on naming convention: "horizontal split" might mean "create a horizontal divider" (which results in vertical stacking) vs "split horizontally" (side-by-side).
Please verify this matches the expected user experience described in the hotkey labels:
SPLIT_HORIZONTAL: "Split Terminal Window Horizontal"SPLIT_VERTICAL: "Split Terminal Window Vertical"
43-55: Clean derivation of active window and focused pane state.The chained lookups properly handle null cases and follow the expected data flow from workspace → window → pane.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (3)
26-31: Memoization correctly applied for filtered windows.The useMemo correctly depends on
activeWorkspaceIdandallWindows, ensuring the filtered list only recomputes when necessary.
56-70: Window navigation logic is correct but doesn't wrap around.The navigation stops at the first/last window rather than wrapping. This is a valid UX choice. If wrap-around is desired in the future, the boundary checks could be adjusted.
42-53: Terminal shortcuts correctly wired to window/pane operations.The mapping makes sense from a user perspective:
- "New Terminal" creates a new window (with its initial pane)
- "Close Terminal" closes the focused pane (potentially closing the window if last pane)
The inline comment on line 49 helpfully documents the cascading close behavior.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
17-22: Clear documentation of the prop aliasing during migration.The comment on line 18 clearly explains the temporary aliasing approach. This is a pragmatic migration strategy that maintains API compatibility while transitioning internals.
84-89: Proper focus handling with defensive null check.The callback correctly guards against undefined
windowIdbefore setting focused pane state.
196-202: ThewindowIdparameter passed tosetupFocusListenerat line 199 is unused. The function signature declares it as_windowId(underscore prefix indicating intentional non-usage), and the implementation only usesxterm.textareaand theonFocuscallback. The empty string fallback has no functional impact and doesn't mask any issues.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
103-149: LGTM!The window rendering logic with framer-motion animations and drop indicators is well-structured. The conditional rendering of drop indicators before items and at the end of the list provides good visual feedback during drag operations.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx (1)
119-179: LGTM!The component structure is well-organized with proper keyboard accessibility (tabIndex, onKeyDown handlers), context menu integration, and visual feedback for drag states. The inline rename flow with blur/Enter/Escape handling is solid.
apps/desktop/src/renderer/stores/tabs/types.ts (2)
3-36: LGTM!The type definitions are well-structured with clear JSDoc documentation. The separation of
PaneandWindowtypes, along with the use of discriminated properties for different pane types (terminal vs browser), follows good TypeScript patterns.
49-91: LGTM!The
WindowsStoreinterface provides a comprehensive API surface for window/pane management. The organization into window operations, pane operations, split operations, and query helpers is clean and intuitive.apps/desktop/src/renderer/stores/tabs/utils.ts (2)
38-59: LGTM!The
createPanefunction properly generates unique terminal names based on existing panes. The default type of "terminal" and the conditional naming logic are well-handled.
136-164: LGTM!The
removePaneFromLayoutfunction correctly handles mosaic tree manipulation with proper null propagation and branch collapsing when one side becomes empty.apps/desktop/src/renderer/stores/tabs/store.ts (4)
217-230: Splicing filtered arrays is safe but subtle.The
spliceoperations on lines 226-227 and 250-251 work correctly because they operate on filtered array copies, not the originalstate.windows. The finalset()replaces the state with the reordered results. This is correct but the pattern could be clearer with explicit array manipulation.Also applies to: 232-254
126-171: LGTM!The
removeWindowimplementation properly handles:
- Killing terminals for all panes in the window
- Cleaning up pane state
- Finding the next window to activate via history or position
- Cleaning up focused pane tracking
The cascading cleanup logic is thorough.
18-76: LGTM!The
findNextWindowhelper implements a sensible priority order: MRU history first, then positional (next/prev), then fallback to first available. This provides good UX for window navigation after closing.
430-525: LGTM!The split operations (
splitPaneVerticalandsplitPaneHorizontal) correctly useupdateTreefrom react-mosaic-component for path-based splits and handle the simpler case of appending to the root layout. The focus is properly updated to the new pane.apps/desktop/src/renderer/stores/tabs/hooks.ts (5)
1-9: Clean zustand hook exports for window/pane state.The hooks follow idiomatic zustand patterns with simple selectors. This aligns with the coding guidelines preferring zustand for state management.
11-23: Window action hooks are correctly exported.Action selectors return stable function references from the store, which is the recommended pattern for zustand actions.
25-34: Pane action hooks follow consistent patterns.The pane-level operations (add, remove, rename, focus, attention tracking) are cleanly exposed as individual hooks, maintaining separation of concerns.
36-40: Split hooks align with the new pane-based model.Vertical and horizontal pane splitting operations are correctly exposed, supporting the PR's goal of pane-level content management within windows.
42-50: Query helper hooks export imperative lookup functions.These hooks return functions that perform lookups via
get()internally. This is a valid pattern for use in event handlers or effects. Note that components using these won't automatically re-render when windows/panes change—they'll need to also subscribe touseWindows()orusePanes()if reactivity is required.
| // Clear drop index when not hovering | ||
| if (!isOver && dropIndex !== null) { | ||
| setDropIndex(null); | ||
| } |
There was a problem hiding this comment.
Avoid setting state during render.
Calling setDropIndex(null) synchronously during the render phase can cause React warnings and unexpected re-render loops. Move this side effect into a useEffect.
+ // Clear drop index when not hovering
+ React.useEffect(() => {
+ if (!isOver && dropIndex !== null) {
+ setDropIndex(null);
+ }
+ }, [isOver, dropIndex]);
- // Clear drop index when not hovering
- if (!isOver && dropIndex !== null) {
- setDropIndex(null);
- }
-📝 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.
| // Clear drop index when not hovering | |
| if (!isOver && dropIndex !== null) { | |
| setDropIndex(null); | |
| } | |
| // Clear drop index when not hovering | |
| React.useEffect(() => { | |
| if (!isOver && dropIndex !== null) { | |
| setDropIndex(null); | |
| } | |
| }, [isOver, dropIndex]); |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
around lines 89-92: the component is calling setDropIndex(null) directly during
render when !isOver && dropIndex !== null, which produces React warnings; move
this state change into a useEffect that depends on isOver and dropIndex, and
inside the effect check the same condition and call setDropIndex(null) there so
the state update runs as a side effect after render rather than during render.
| name: "windows-storage", | ||
| storage: electronStorage, | ||
| }, | ||
| ), | ||
| { name: "TabsStore" }, | ||
| { name: "WindowsStore" }, | ||
| ), | ||
| ); | ||
|
|
||
| // Keep old export name for backwards compatibility during migration | ||
| export const useTabsStore = useWindowsStore; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "tabs-storage" --type=tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
rg -n "tabs-storage" --type=ts --type=jsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
fd -e ts -e tsx -e js -e jsx | xargs grep -l "migrate\|migration" | head -20Repository: superset-sh/superset
Length of output: 146
🏁 Script executed:
rg -n "windows-storage" --type=ts -A 5 -B 5Repository: superset-sh/superset
Length of output: 831
🏁 Script executed:
cat -n apps/desktop/src/renderer/stores/tabs/store.ts | head -100Repository: superset-sh/superset
Length of output: 3590
🏁 Script executed:
cat -n apps/desktop/src/renderer/stores/tabs/store.ts | tail -100Repository: superset-sh/superset
Length of output: 3299
🏁 Script executed:
cat -n apps/cli/src/lib/storage/adapter.tsRepository: superset-sh/superset
Length of output: 1987
🏁 Script executed:
rg -n "migration|migrate" apps/desktop/src/renderer/stores --type=ts -B 3 -A 3Repository: superset-sh/superset
Length of output: 440
🏁 Script executed:
rg -n "electronStorage" apps/desktop/src/renderer --type=ts -B 2 -A 5Repository: superset-sh/superset
Length of output: 3584
🏁 Script executed:
fd -name "*electron*storage*" -type fRepository: superset-sh/superset
Length of output: 294
🏁 Script executed:
cat -n apps/desktop/src/renderer/lib/electron-storage.tsRepository: superset-sh/superset
Length of output: 883
Storage migration for old data is missing.
While the useTabsStore alias provides good backward compatibility for code imports, there is no migration logic to handle data persisted under the old storage key. Users upgrading will lose their window and pane state.
Add an onRehydrateStorage handler (similar to the pattern in apps/desktop/src/renderer/stores/theme/store.ts) to migrate persisted data from the previous storage key to "windows-storage", or document the data loss as an acceptable trade-off for this release.
95aa0f8 to
3072c96
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
47-87: MovesetDropIndexout of render phase (useuseEffect)
setDropIndex(null)is currently called during render when!isOver && dropIndex !== null(lines 89-92), which can trigger React warnings and unstable render loops. This is the same issue previously flagged; it should be handled as a side effect.Here’s a minimal fix using
useEffect:-import { useMemo, useRef, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; @@ - const [{ isOver }, drop] = useDrop<DragItem, void, { isOver: boolean }>({ + const [{ isOver }, drop] = useDrop<DragItem, void, { isOver: boolean }>({ @@ - collect: (monitor) => ({ - isOver: monitor.isOver(), - }), - }); - - // Clear drop index when not hovering - if (!isOver && dropIndex !== null) { - setDropIndex(null); - } + collect: (monitor) => ({ + isOver: monitor.isOver(), + }), + }); + + // Clear drop index when not hovering + useEffect(() => { + if (!isOver && dropIndex !== null) { + setDropIndex(null); + } + }, [isOver, dropIndex]);This preserves behavior while ensuring state updates happen after render.
Also applies to: 89-92
apps/desktop/src/renderer/stores/tabs/store.ts (1)
546-553: Storage migration for old data is missing.This duplicates a concern from a previous review. The storage key changed from
"tabs-storage"to"windows-storage", but there's no migration logic. Users upgrading will lose their existing window/pane state. Consider adding anonRehydrateStoragehandler to migrate data from the old key, or explicitly document this as an acceptable breaking change for the release.
🧹 Nitpick comments (9)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx (1)
1-33: Consider component folder structure and test coverage.As per coding guidelines, components should follow the structure
ComponentName/ComponentName.tsxwith anindex.tsfor barrel export, and tests should be co-located. Since this is a sub-component ofWindowItem, consider:
- Moving to
WindowContextMenu/WindowContextMenu.tsxwith aWindowContextMenu/index.tsbarrel export.- Adding a co-located test file
WindowContextMenu.test.tsxto verify the menu renders correctly and callbacks are invoked.</review_comment_end>
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (2)
30-31: SimplifycontainerReftyping and remove cast inrefcallbackYou can avoid the
React.MutableRefObjectcast and make the ref type-safe by includingnullin the ref type and assigning directly in the callback:-import { useMemo, useRef, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; @@ -const TabsView() { +const TabsView() { @@ - const [dropIndex, setDropIndex] = useState<number | null>(null); - const containerRef = useRef<HTMLElement>(null); + const [dropIndex, setDropIndex] = useState<number | null>(null); + const containerRef = useRef<HTMLElement | null>(null); @@ - <nav - ref={(node) => { - drop(node); - (containerRef as React.MutableRefObject<HTMLElement | null>).current = - node; - }} + <nav + ref={(node) => { + drop(node); + containerRef.current = node; + }}(Effect import is also needed for the next comment’s fix; if you apply that separately, drop the
useEffectpart here.)Also applies to: 95-100
23-29: Window-centric filtering and rendering look consistent with store APIThe switch from tabs to windows is wired cleanly:
windowsis filtered byactiveWorkspaceId,addWindowis guarded on workspace presence, andactiveWindowIds[activeWorkspaceId || ""] === window.idkeeps the active state scoped per workspace. The framer-motion layout group and drop indicators integrate well with the newWindowItemcomponent.Optional: consider renaming
TabsViewto something window-oriented (e.g.,WindowsSidebar) in a later cleanup to better reflect the new model, but this is not blocking.Also applies to: 33-45, 103-149
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
50-76: Consider extractingcollectPaneIdsoutside the component for clarity and reuse.The
collectPaneIdshelper is re-created on every render inside theflatMapcallback. While not a significant performance issue, extracting it as a standalone utility (or importingextractPaneIdsFromLayoutfromrenderer/stores/tabs/utilswhich already exists per the relevant snippets) would improve readability and reduce duplication.The utility
extractPaneIdsFromLayoutinrenderer/stores/tabs/utils.tsappears to do the same thing. Consider using it:- const workspacePaneIds = new Set( - workspaceWindows.flatMap((w) => { - // Extract pane IDs from the layout (which is a MosaicNode<string>) - const collectPaneIds = (node: unknown): string[] => { - if (typeof node === "string") return [node]; - if ( - node && - typeof node === "object" && - "first" in node && - "second" in node - ) { - const branch = node as { first: unknown; second: unknown }; - return [ - ...collectPaneIds(branch.first), - ...collectPaneIds(branch.second), - ]; - } - return []; - }; - return collectPaneIds(w.layout); - }), - ); + const workspacePaneIds = new Set( + workspaceWindows.flatMap((w) => extractPaneIdsFromLayout(w.layout)), + );You'd need to add the import:
import { extractPaneIdsFromLayout } from "renderer/stores/tabs/utils";apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (1)
46-70: Consider batching pane removals to avoid multiple state updates.When multiple panes are removed via the Mosaic UI (e.g., closing a split), calling
removePanein a loop triggers multiple store updates. While React 18+ batches updates within event handlers, the store'sremovePaneimplementation (from the relevant snippet at store.ts) performs terminal cleanup and may trigger side effects per call.Consider calling
updateWindowLayoutfirst and letting the store'supdateWindowLayoutimplementation handle the pane cleanup (which it already does per lines 238-268 in store.ts), avoiding redundant removals.If the store's
updateWindowLayoutalready handles removed pane cleanup, you can simplify:const handleLayoutChange = useCallback( (newLayout: MosaicNode<string> | null) => { if (!newLayout) { // This shouldn't happen as we handle last pane removal in removePane return; } - const oldPaneIds = extractPaneIdsFromLayout(window.layout); - const newPaneIds = extractPaneIdsFromLayout(newLayout); - - // Find removed panes (e.g., from Mosaic close button) - const removedPaneIds = oldPaneIds.filter( - (id) => !newPaneIds.includes(id), - ); - - // Remove panes that were removed via Mosaic UI - for (const removedId of removedPaneIds) { - removePane(removedId); - } - // Update the layout updateWindowLayout(window.id, newLayout); }, - [window.id, window.layout, updateWindowLayout, removePane], + [window.id, updateWindowLayout], );apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx (1)
68-77: Consider renamingtabIdprop topaneIdin Terminal for consistency.The Terminal component still uses
tabIdas its prop name while receivingpaneId. This works for backwards compatibility but creates naming confusion in the new window/pane model. Consider a follow-up task to rename the Terminal prop.This is a follow-up cleanup item - the current code works correctly. When ready, update Terminal's interface:
// In Terminal component interface TerminalProps { paneId: string; // was: tabId workspaceId: string; }apps/desktop/src/renderer/stores/tabs/utils.ts (1)
7-9: ID generation is adequate but consider UUID for stronger uniqueness.The current approach (
Date.now()+ random string) works for typical client-side usage. If IDs ever need to be globally unique across clients or persisted long-term, consider usingcrypto.randomUUID()which is available in modern browsers.apps/desktop/src/renderer/stores/tabs/store.ts (2)
16-16: Function name still references "Tab" but operates on panes.The import
killTerminalForTabshould ideally be renamed tokillTerminalForPaneto align with the new naming conventions. This is a minor inconsistency in the refactoring.
222-235: Array mutation occurs on a filtered copy, so it's safe but could be clearer.The
spliceoperations on lines 231-232 mutateworkspaceWindows, which is a new array created byfilter(). This is technically safe sincefilter()returns a shallow copy, but using immutable operations would make the intent clearer.- reorderWindows: (workspaceId, startIndex, endIndex) => { - const state = get(); - const workspaceWindows = state.windows.filter( - (w) => w.workspaceId === workspaceId, - ); - const otherWindows = state.windows.filter( - (w) => w.workspaceId !== workspaceId, - ); - - const [removed] = workspaceWindows.splice(startIndex, 1); - workspaceWindows.splice(endIndex, 0, removed); - - set({ windows: [...otherWindows, ...workspaceWindows] }); - }, + reorderWindows: (workspaceId, startIndex, endIndex) => { + const state = get(); + const workspaceWindows = state.windows.filter( + (w) => w.workspaceId === workspaceId, + ); + const otherWindows = state.windows.filter( + (w) => w.workspaceId !== workspaceId, + ); + + const reordered = [...workspaceWindows]; + const [removed] = reordered.splice(startIndex, 1); + reordered.splice(endIndex, 0, removed); + + set({ windows: [...otherWindows, ...reordered] }); + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(8 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/index.tsx(3 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/validation.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/hooks.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/index.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
💤 Files with no reviewable changes (25)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx
- apps/desktop/src/renderer/stores/tabs/store.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts
- apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts
- apps/desktop/src/renderer/stores/tabs/helpers/validation.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts
- apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts
- apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts
- apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
- apps/desktop/src/renderer/stores/tabs/index.ts
- apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts
- apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx
- apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts
- apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts
- apps/desktop/src/renderer/stores/tabs/drag-logic.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts
- apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
- apps/desktop/src/renderer/stores/tabs/hooks.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/utils.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/utils.ts
**/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.tsx: Create one folder per component with structure:ComponentName/ComponentName.tsx+index.tsfor barrel export
Co-locate tests next to the component file they test (e.g.,ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nestedcomponents/subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent'scomponents/folder
One component per file - avoid multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/utils.ts
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods using
window.ipcRenderer.invoke()with object parameters - TypeScript will infer the exact response type automatically
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to packages/ui/src/**/*.tsx : Use shadcn/ui components and TailwindCSS v4 for all UI component styling in the shared UI package
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Call IPC methods using `window.ipcRenderer.invoke()` with object parameters - TypeScript will infer the exact response type automatically
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
🧬 Code graph analysis (9)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (2)
useWindows(4-4)usePanes(5-5)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/useWorkspaceRename.ts (1)
useWorkspaceRename(4-64)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/types.ts (1)
DragItem(1-4)apps/desktop/src/renderer/stores/tabs/hooks.ts (4)
useWindows(4-4)useAddWindow(11-11)useReorderWindowById(20-21)useActiveWindowIds(6-7)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx (1)
WindowItem(30-174)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx (1)
packages/ui/src/components/context-menu.tsx (5)
ContextMenu(241-241)ContextMenuTrigger(242-242)ContextMenuContent(243-243)ContextMenuItem(244-244)ContextMenuSeparator(248-248)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (4)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Window(30-36)Pane(12-24)apps/desktop/src/renderer/stores/tabs/utils.ts (2)
getPaneIdsForWindow(113-120)extractPaneIdsFromLayout(22-27)apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-554)
apps/desktop/src/renderer/screens/main/index.tsx (1)
apps/desktop/src/renderer/stores/tabs/hooks.ts (4)
useSplitPaneVertical(36-37)useSplitPaneHorizontal(38-39)useActiveWindowIds(6-7)useFocusedPaneIds(8-9)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (2)
apps/desktop/src/renderer/stores/tabs/hooks.ts (6)
useWindows(4-4)useActiveWindowIds(6-7)useFocusedPaneIds(8-9)useAddWindow(11-11)useSetActiveWindow(16-17)useRemovePane(27-27)apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(28-151)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (3)
Window(30-36)PaneType(6-6)Pane(12-24)apps/desktop/src/renderer/stores/tabs/utils/terminal-naming.ts (1)
generateTerminalName(41-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (17)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (2)
16-33: LGTM! Agent-complete handling correctly migrated to pane-centric model.The logic properly looks up the pane, determines if it's already focused by checking both
activeWindowIdsandfocusedPaneIds, and only sets attention when the pane isn't currently visible. The backwards compatibility comment fortabId→paneIdis helpful.
34-56: LGTM! Focus-tab handling correctly chains workspace switch with window/pane focus.The handler properly:
- Validates the pane exists before proceeding
- Switches workspace first via mutation
- Fetches fresh state in
onSuccessto avoid stale closures- Sets both active window and focused pane
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx (1)
30-49: LGTM! Clean handler implementations.The
handleFocusandhandleClosePanehandlers are properly implemented with appropriate event propagation handling.apps/desktop/src/renderer/screens/main/index.tsx (2)
68-78: Verify hotkey-to-split-function mapping is intentional.The naming is potentially confusing:
SPLIT_HORIZONTAL("Split Right") callssplitPaneVerticalSPLIT_VERTICAL("Split Down") callssplitPaneHorizontalBased on the store implementation (where
splitPaneVerticalusesdirection: "row"for side-by-side splits andsplitPaneHorizontalusesdirection: "column"for stacked splits), this mapping appears correct. The function names refer to the orientation of the resulting panes, not the divider direction.Please confirm this matches the intended UX:
Cmd+D("Split Right") → creates a new pane to the rightCmd+Shift+D("Split Down") → creates a new pane below
53-56: LGTM! Correct derivation chain for active pane.The logic properly chains:
activeWorkspaceId→activeWindowId→focusedPaneId, with null checks at each step.apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (3)
4-11: LGTM!The migration from tab-based hooks to window/pane hooks is clean and follows the established import patterns. The alias paths are correctly using the
renderer/storesbarrel export as defined in the tsconfig.
26-39: LGTM!The memoization of
windowsis appropriate since it involves filtering. The derived state foractiveWindowIdandfocusedPaneIdis correctly computed from the store state without unnecessary memoization.
56-70: Navigation wrapping behavior is worth confirming.The prev/next terminal shortcuts stop at the first/last window rather than wrapping around. This is a valid UX choice, but verify this matches the intended behavior—some users expect circular navigation.
apps/desktop/src/renderer/stores/tabs/utils.ts (3)
22-33: LGTM!The recursive extraction handles both leaf nodes (strings) and branch nodes correctly. The spread operator approach is clean and readable.
65-80: LGTM!The window naming logic correctly finds gaps in the sequence (e.g., if "Window 1" and "Window 3" exist, it will assign "Window 2" to the next window). This is a nice touch for maintaining predictable naming.
136-164: LGTM!The
removePaneFromLayoutfunction correctly handles all edge cases: leaf removal, branch collapse when one child is removed, and preservation of structure when both children remain. The null propagation is clean.apps/desktop/src/renderer/stores/tabs/types.ts (3)
3-24: LGTM!The
Paneinterface cleanly separates terminal-specific properties (command,cwd) from browser/preview properties (url) while maintaining a common base. The JSDoc comments provide helpful context.
41-47: LGTM!The state structure is well-designed:
windowsas array maintains ordering for sidebar displaypanesasRecord<string, Pane>enables O(1) lookupsactiveWindowIdsandfocusedPaneIdsas maps allow per-workspace/per-window trackingwindowHistoryStackssupports MRU-based navigation
52-90: LGTM!The action signatures are comprehensive and cover all CRUD operations plus specialized behaviors (splits, focus, attention flags). The query helpers return derived state appropriately.
apps/desktop/src/renderer/stores/tabs/store.ts (3)
89-124: LGTM!The
addWindowaction correctly enforces the invariant that windows always have at least one pane by usingcreateWindowWithPane. The history stack management properly prepends the current active window before switching.
333-378: LGTM!The
removePaneaction correctly implements the "last pane closes window" behavior. The defensive check on line 352-356 is a good safeguard even thoughisLastPaneInWindowshould prevent reaching that state.
424-471: LGTM!The split operations correctly handle both cases: splitting at a specific path (using
updateTree) and splitting the root layout. The new pane is properly registered and focused after the split.
| <ContextMenuItem onSelect={onClose} className="text-destructive"> | ||
| Close Window | ||
| </ContextMenuItem> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for window state persistence/serialization
echo "=== Searching for window state persistence patterns ==="
rg -nP --type=ts -C3 'persist.*[Ww]indow|save.*[Ww]indow|serialize.*[Ww]indow|restore.*[Ww]indow' apps/desktop/src 2>/dev/null | head -50
echo -e "\n=== Searching for undo/history/revert mechanisms ==="
rg -nP --type=ts -C3 'undo|history|revert' apps/desktop/src 2>/dev/null | head -50
echo -e "\n=== Searching for closeWindow implementation ==="
rg -nP --type=ts 'closeWindow|close.*[Ww]indow' apps/desktop/src --max-count=20
echo -e "\n=== Checking WindowContextMenu usage and callback definition ==="
rg -nP --type=ts -B5 -A5 'WindowContextMenu' apps/desktop/src 2>/dev/null | head -60Repository: superset-sh/superset
Length of output: 12598
🏁 Script executed:
#!/bin/bash
# Check the tRPC window.close implementation
echo "=== tRPC window.close implementation ==="
rg -nP --type=ts -B5 -A10 'window\.close' apps/desktop/src --max-count=20 | head -80
# Check handleRemoveWindow implementation
echo -e "\n=== handleRemoveWindow implementation in WindowItem ==="
rg -nP --type=ts -B3 -A8 'handleRemoveWindow' apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx
# Check for confirmation dialogs in the entire codebase
echo -e "\n=== Searching for confirmation/alert dialogs related to window/close ==="
rg -nP --type=ts 'AlertDialog|confirm|ConfirmDialog' apps/desktop/src/renderer --max-count=15 | head -50
# Check if there's middleware or backend validation
echo -e "\n=== Checking for window close routes/handlers in main process ==="
fd -e ts -e tsx apps/desktop/src -type f -exec grep -l 'window-close\|window\.close' {} \; 2>/dev/nullRepository: superset-sh/superset
Length of output: 6051
Add confirmation dialog for destructive Close Window action.
The "Close Window" action in the context menu currently closes windows without confirmation. While closed windows cannot be recovered within a session (only via app restart), this behavior is inconsistent with how other destructive actions are handled—the codebase already implements DeleteWorkspaceDialog with confirmation for workspace deletion. Add an alert dialog to confirm window closure before executing removeWindow().
| const name = | ||
| type === "terminal" ? generateTerminalName(existingNames) : `${type} 1`; |
There was a problem hiding this comment.
Non-terminal pane naming doesn't account for existing panes of the same type.
For non-terminal types, the name is hardcoded to ${type} 1 regardless of how many panes of that type already exist. This will cause duplicate names when multiple browser/editor/preview panes are created.
- const name =
- type === "terminal" ? generateTerminalName(existingNames) : `${type} 1`;
+ let name: string;
+ if (type === "terminal") {
+ name = generateTerminalName(existingNames);
+ } else {
+ const existingTypeNames = Object.values(existingPanes)
+ .filter((pane) => pane.type === type)
+ .map((pane) => pane.name);
+ const count = existingTypeNames.length + 1;
+ name = `${type} ${count}`;
+ }🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/utils.ts around lines 49-50, the
non-terminal pane name is always set to `${type} 1`, causing duplicate names;
change the logic to compute the next available numeric suffix for that pane type
by scanning existingNames for entries that start with `${type}` (and parse
trailing numbers), determine the highest existing index, and set the new name to
`${type} ${highestIndex + 1}` so each new non-terminal pane of the same type
gets a unique incremented name.
3072c96 to
bfe895c
Compare
- Refactor from "tabs can be singles OR groups" to "windows always contain panes" model - Windows now always have ≥1 pane (invariant enforced at creation time) - Remove ungroup concepts - panes can only be split/closed - Simplify sidebar to show only windows (no expandable children) - Drag-and-drop now only reorders windows in sidebar - Window names are now static at creation time (e.g., "Window 5" stays "Window 5") - Add validation in setActiveWindow and split operations - Add efficient useWindowNeedsAttention selector hook - Delete ~3,200 lines of unused code (old tab grouping logic, tests, helpers) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
bfe895c to
d311ab0
Compare
- Delete hooks.ts containing ~20 one-liner wrapper hooks - Update all consumers to use useWindowsStore selectors directly - Simplify PaneType to just "terminal" (removed speculative unused types) - Remove unused Pane properties (command, cwd, url) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The test was incorrectly catching the "Directory should not exist" error which has no .code property, causing confusing "Received: undefined" failures instead of clear "directory still exists" messages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
84-87: State update during render persists.This synchronous
setDropIndex(null)call during render can cause React warnings and re-render loops. The previous review suggested moving this to auseEffect.Apply this diff:
+import { useMemo, useRef, useState, useEffect } from "react"; -import { useMemo, useRef, useState } from "react"; // ... after useDrop hook ... - // Clear drop index when not hovering - if (!isOver && dropIndex !== null) { - setDropIndex(null); - } + // Clear drop index when not hovering + useEffect(() => { + if (!isOver && dropIndex !== null) { + setDropIndex(null); + } + }, [isOver, dropIndex]);apps/desktop/src/renderer/stores/tabs/store.ts (1)
547-549: Storage migration for existing data is still missing.The storage key changed from
"tabs-storage"to"windows-storage", but there's no migration logic to handle existing persisted data. Users upgrading will lose their previous state.apps/desktop/src/renderer/stores/tabs/utils.ts (1)
49-50: Non-terminal pane naming still doesn't account for duplicates.This is the same issue from the previous review. While
PaneTypecurrently only includes"terminal", the code structure anticipates other pane types (e.g., browser, editor, preview). When those types are added, hardcoding${type} 1will create duplicate names.
🧹 Nitpick comments (7)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
332-336: Existence check now swallows all fs errors, not just ENOENTThe simplified
stat().then(...).catch(() => false)is concise, but it will treat anyfs.statfailure (including unexpected ones like EACCES) as "directory does not exist", potentially masking real issues. Consider preserving the previous behavior by only mappingENOENTtofalseand rethrowing other errors, e.g.:- const exists = await fs - .stat(historyDir) - .then(() => true) - .catch(() => false); + const exists = await fs + .stat(historyDir) + .then(() => true) + .catch((error: NodeJS.ErrnoException) => { + if (error.code !== "ENOENT") throw error; + return false; + });This keeps the test readable while ensuring unexpected filesystem problems still fail the test, rather than silently passing. Based on learnings, this maintains repeatable, trustworthy tests.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
50-76: Consider using the existingextractPaneIdsFromLayoututility.The inline
collectPaneIdsfunction duplicates logic that already exists inrenderer/stores/tabs/utils. This would reduce code duplication and ensure consistent behavior.+import { extractPaneIdsFromLayout } from "renderer/stores/tabs/utils"; ... // Check if any pane in windows belonging to this workspace needs attention const workspaceWindows = windows.filter((w) => w.workspaceId === id); - const workspacePaneIds = new Set( - workspaceWindows.flatMap((w) => { - // Extract pane IDs from the layout (which is a MosaicNode<string>) - const collectPaneIds = (node: unknown): string[] => { - if (typeof node === "string") return [node]; - if ( - node && - typeof node === "object" && - "first" in node && - "second" in node - ) { - const branch = node as { first: unknown; second: unknown }; - return [ - ...collectPaneIds(branch.first), - ...collectPaneIds(branch.second), - ]; - } - return []; - }; - return collectPaneIds(w.layout); - }), - ); + const workspacePaneIds = new Set( + workspaceWindows.flatMap((w) => extractPaneIdsFromLayout(w.layout)), + );
46-47: Minor: Consider combining store selectors to reduce subscriptions.Two separate
useWindowsStorecalls create two subscriptions. If performance becomes a concern, you could combine them:- const windows = useWindowsStore((s) => s.windows); - const panes = useWindowsStore((s) => s.panes); + const { windows, panes } = useWindowsStore((s) => ({ + windows: s.windows, + panes: s.panes, + }));Note: This requires Zustand's
shallowequality check to avoid unnecessary re-renders, or useuseShallowfromzustand/react/shallow.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (1)
62-65: Consider batching pane removals for efficiency.Multiple
removePanecalls in a loop will trigger multiple state updates. While Zustand batches updates within the same tick, consider adding aremovePanes(paneIds: string[])batch method to the store if users frequently close multiple panes at once (e.g., via Mosaic drag operations that reorganize layouts).apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (2)
90-95: Ref combination pattern works but could be cleaner.The ref assignment combining
dropandcontainerRefworks correctly. Consider using a callback ref utility oruseMergeRefsfrom a library for cleaner composition, though this is not blocking.
118-121: Consider early return if no workspace.The fallback
activeWorkspaceId || ""for the lookup works but could mask issues. SincehandleAddWindowalready guards for!activeWorkspaceId, consider adding an early return at the component level if there's no active workspace.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
18-23: Props type should be updated to reflect pane terminology.The aliasing
const paneId = tabIdworks but the underlyingTerminalPropstype still usestabId. Consider updating the interface to usepaneIdfor consistency with the new model, even if it requires updating callers.#!/bin/bash # Check TerminalProps definition and usages rg -n "interface TerminalProps" apps/desktop/src/renderer --type=ts -A 5 rg -n "tabId.*TerminalProps\|TerminalProps.*tabId" apps/desktop/src/renderer --type=ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
apps/desktop/src/main/lib/terminal-manager.test.ts(1 hunks)apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(8 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(3 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/validation.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/hooks.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/index.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
💤 Files with no reviewable changes (26)
- apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts
- apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts
- apps/desktop/src/renderer/stores/tabs/helpers/validation.ts
- apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts
- apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts
- apps/desktop/src/renderer/stores/tabs/store.test.ts
- apps/desktop/src/renderer/stores/tabs/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx
- apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
- apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts
- apps/desktop/src/renderer/stores/tabs/drag-logic.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
- apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts
- apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts
- apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts
- apps/desktop/src/renderer/stores/tabs/hooks.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/WindowItem/WindowContextMenu.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
🧰 Additional context used
📓 Path-based instructions (8)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods using
window.ipcRenderer.invoke()with object parameters - TypeScript will infer the exact response type automatically
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
**/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.tsx: Create one folder per component with structure:ComponentName/ComponentName.tsx+index.tsfor barrel export
Co-locate tests next to the component file they test (e.g.,ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nestedcomponents/subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent'scomponents/folder
One component per file - avoid multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx
🧠 Learnings (8)
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Call IPC methods using `window.ipcRenderer.invoke()` with object parameters - TypeScript will infer the exact response type automatically
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to apps/desktop/src/lib/**/*.ts : Never import Node.js modules in shared code like `src/lib/electron-router-dom.ts` - this code runs in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
🧬 Code graph analysis (5)
apps/desktop/src/renderer/screens/main/index.tsx (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-554)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
useAgentHookListener(9-59)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (3)
Window(25-31)PaneType(6-6)Pane(12-19)apps/desktop/src/renderer/stores/tabs/utils/terminal-naming.ts (1)
generateTerminalName(41-46)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useWindowsStore(78-554)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/useWorkspaceRename.ts (1)
useWorkspaceRename(4-64)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (5)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Window(25-31)Pane(12-19)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getPaneIdsForWindow(113-120)apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/WindowPane.tsx (1)
WindowPane(30-80)apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/stores/tabs/store.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
WindowsState(36-42)WindowsStore(47-85)apps/desktop/src/renderer/stores/tabs/utils.ts (5)
createWindowWithPane(86-108)getPaneIdsForWindow(113-120)extractPaneIdsFromLayout(22-27)createPane(38-59)isLastPaneInWindow(125-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (22)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
1-85: Well-structured type definitions for the window/pane model.The type hierarchy is clean and correctly models the invariant that windows always contain panes. The use of
MosaicNode<string>for layouts with paneId leaves is appropriate.apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
13-56: Clean migration from tab-centric to pane-centric event handling.The backwards compatibility comments (Lines 16-17, 35-36) are helpful for understanding the migration. The logic correctly translates tab events to the new window/pane model, and using
getState()inside callbacks properly avoids stale closure issues.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
188-200: Good simplification of the focus listener API.Removing the unused window/pane ID parameters and delegating focus handling to a callback makes this function more reusable and aligns with the new pane-centric architecture where the caller manages focus state.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/WindowView/index.tsx (3)
40-45: Auto-close window effect correctly addresses the stated invariant.This effect properly enforces that windows with no panes are removed, which aligns with the PR objectives. The previous review comment about this was addressed.
118-128: Clean Mosaic integration.The component correctly wires up the Mosaic layout with the store's layout state, handles layout changes, and delegates pane rendering to
WindowPane. The use of a shareddragDropManagerensures consistent drag-and-drop behavior across the app.
78-84: This review comment is based on an incorrect assumption. The code already implements the suggested cleanup:cleanLayout()is called on line 38 to remove orphaned pane IDs from the layout before rendering, and the cleaned layout is passed to the Mosaic component on line 122. The fallback "Pane not found" handler on lines 78-84 is an appropriate defensive measure for rare edge cases (e.g., race conditions), not a primary concern.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (2)
12-17: Good use of individual store selectors.The granular selector pattern (one selector per state slice) optimizes re-renders by only subscribing to the specific state pieces needed. This follows zustand best practices.
41-46: Closing the focused pane is correctly implemented.The guard
if (focusedPaneId)properly prevents callingremovePanewhen no pane is focused. The store'sremovePanehandles the case where closing the last pane should close the window.apps/desktop/src/renderer/screens/main/index.tsx (1)
39-51: Store access and derived state look correct.The chained derivation of
activeWindowId→focusedPaneIdfollows the new window/pane model correctly. Guards for null values are appropriately placed.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
85-91: Focus handler implementation is correct.The
handleTerminalFocuscallback properly guards against undefinedpaneand correctly sets the focused pane when the terminal gains focus.
223-230: Effect will recreate terminal if pane is renamed.Including
paneNamein the dependency array means renaming a pane will detach and recreate the terminal session. If pane renaming should preserve the terminal session, consider removingpaneNamefrom deps and only using it for the initialcreateOrAttachcall.Is recreating the terminal on pane rename the intended behavior? If panes can be renamed by users, this could be disruptive.
apps/desktop/src/renderer/stores/tabs/store.ts (6)
18-76: Well-structured window activation fallback logic.The
findNextWindowhelper correctly prioritizes MRU history, then positional adjacency, then any remaining window. Edge cases are handled properly.
88-124: Window creation with history tracking is well implemented.The
addWindowoperation correctly creates a window with an initial pane, manages the MRU history stack, and returns the created IDs for caller use.
126-171: Window removal properly cleans up all related state.The
removeWindowoperation correctly kills terminals, removes panes, updates active window selection, and cleans up focus tracking and history in the right order.
222-259: Reorder operations correctly scope to workspace.Both
reorderWindowsandreorderWindowByIdproperly filter windows by workspace, perform the reorder, and merge back with windows from other workspaces.
333-378: Pane removal handles edge cases correctly.The
removePaneoperation correctly handles the last-pane-closes-window case, cleans up the terminal, updates the layout, and reassigns focus when needed.
424-519: Split operations correctly handle both path-based and direct splits.The
splitPaneVerticalandsplitPaneHorizontaloperations properly validate inputs, create new panes, and update layouts using eitherupdateTreefor specific paths or direct wrapping for top-level splits.apps/desktop/src/renderer/stores/tabs/utils.ts (5)
7-9: LGTM! Clean utility implementations.The ID generation, window display name retrieval, and recursive layout traversal logic are all correct and well-implemented.
Also applies to: 15-17, 22-33
65-80: LGTM! Window creation logic is solid.Both
generateWindowNameandcreateWindowWithPaneare well-implemented:
- Window numbering correctly finds the next available number, handling gaps in the sequence
- Atomic window+pane creation ensures the invariant that windows always have ≥1 pane
- Workspace-scoped window naming aligns with the PR objectives
Also applies to: 86-108
113-120: LGTM! Pane query helpers are correct.Both functions correctly identify panes belonging to a window and check for the last-pane condition, which is essential for window lifecycle management.
Also applies to: 125-130
136-164: LGTM! Mosaic removal logic is sound.The recursive pane removal correctly handles all cases:
- Leaf node removal
- Branch collapsing when one side becomes empty
- Tree restructuring when both branches remain valid
169-196: LGTM! Layout validation and traversal helpers are well-designed.
cleanLayoutproperly validates and prunes invalid pane references with a smart optimization (lines 187-189) that preserves object identity when no changes are needed—avoiding unnecessary React re-renders.
getFirstPaneIdcorrectly traverses to the leftmost leaf for focus fallback.Also applies to: 201-208
| useHotkeys(HOTKEYS.SPLIT_HORIZONTAL.keys, () => { | ||
| if (isWorkspaceView && activeWorkspaceId) { | ||
| splitTabVertical(activeWorkspaceId); | ||
| if (isWorkspaceView && activeWindowId && focusedPaneId) { | ||
| splitPaneVertical(activeWindowId, focusedPaneId); | ||
| } | ||
| }, [activeWorkspaceId, splitTabVertical, isWorkspaceView]); | ||
| }, [activeWindowId, focusedPaneId, splitPaneVertical, isWorkspaceView]); | ||
|
|
||
| useHotkeys(HOTKEYS.SPLIT_VERTICAL.keys, () => { | ||
| if (isWorkspaceView && activeWorkspaceId) { | ||
| splitTabHorizontal(activeWorkspaceId); | ||
| if (isWorkspaceView && activeWindowId && focusedPaneId) { | ||
| splitPaneHorizontal(activeWindowId, focusedPaneId); | ||
| } | ||
| }, [activeWorkspaceId, splitTabHorizontal, isWorkspaceView]); | ||
| }, [activeWindowId, focusedPaneId, splitPaneHorizontal, isWorkspaceView]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check hotkey definitions
rg -n "SPLIT_HORIZONTAL|SPLIT_VERTICAL" apps/desktop/src --type ts --type tsx -B 2 -A 3Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check the split pane implementations in the store
rg -n "splitPaneVertical|splitPaneHorizontal" apps/desktop/src/shared --type ts --type tsx -B 2 -A 5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check if these functions are defined in the windows store
fd -e ts -e tsx apps/desktop/src | xargs grep -l "splitPaneVertical\|splitPaneHorizontal" | head -5Repository: superset-sh/superset
Length of output: 433
🏁 Script executed:
# Check hotkey definitions in hotkeys file
find apps/desktop/src -name "hotkeys*" -type fRepository: superset-sh/superset
Length of output: 98
🏁 Script executed:
# Look for SPLIT_HORIZONTAL and SPLIT_VERTICAL definitions
rg "SPLIT_HORIZONTAL|SPLIT_VERTICAL" apps/desktop/src -g "*.ts" -g "*.tsx" -B 2 -A 3Repository: superset-sh/superset
Length of output: 1782
🏁 Script executed:
# Check split pane implementations
rg "splitPaneVertical|splitPaneHorizontal" apps/desktop/src -g "*.ts" -g "*.tsx" -B 1 -A 5Repository: superset-sh/superset
Length of output: 11608
Fix reversed split pane function calls in hotkey handlers.
The hotkey handlers are calling the wrong split functions. SPLIT_HORIZONTAL ("Split Right") calls splitPaneVertical, and SPLIT_VERTICAL ("Split Down") calls splitPaneHorizontal—this is backwards. In WindowPane.tsx, the context menu correctly maps onSplitHorizontal to splitPaneHorizontal and onSplitVertical to splitPaneVertical. Update lines 65 and 71 to call the semantically correct functions.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/index.tsx around lines 63 to 73, the
hotkey handlers call the wrong split functions: the SPLIT_HORIZONTAL handler
currently calls splitPaneVertical and the SPLIT_VERTICAL handler calls
splitPaneHorizontal; change the SPLIT_HORIZONTAL handler (line ~65) to call
splitPaneHorizontal(activeWindowId, focusedPaneId) and change the SPLIT_VERTICAL
handler (line ~71) to call splitPaneVertical(activeWindowId, focusedPaneId) so
the hotkeys match the semantic actions used elsewhere.
Replace verbose inline import("react-mosaic-component").MosaicNode<string>
with proper import at top of file for cleaner type annotations.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Remove numbered terminal naming (was globally numbered, not per-window) - All terminals now simply named "Terminal" - Delete unused terminal-naming.ts and its test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.