Revert "Refactor tabs to windows/panes model"#240
Conversation
This reverts commit 07a04b4.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request refactors the desktop app's terminal management architecture from a window/pane model to a tab-centric model. It introduces a new tab store, tab-based UI components, drag-and-drop logic for tabs, and group tab rendering while removing window/pane infrastructure throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Tab UI
participant Store as Tabs Store
participant Helper as Split Helper
participant Component as Terminal Component
User->>UI: Click split button (vertical)
UI->>Store: useSplitTabVertical(workspaceId, sourceTabId)
activate Store
Store->>Helper: handleSplitTabVertical()
activate Helper
Helper->>Helper: Find tab path in Mosaic layout
Helper->>Helper: Create new tab via createNewTab()
Helper->>Helper: Convert/update group layout
Helper->>Helper: Set parentId relationships
Helper-->>Store: Return updated tabs + layout
deactivate Helper
Store->>Store: Merge state: tabs, activeTabIds, group layouts
Store-->>UI: Notify subscribers
deactivate Store
UI->>Component: Render GroupTabView with new layout
Component->>Component: Extract child tab IDs from Mosaic
Component->>Component: Render each child as GroupTabPane
Component-->>User: Display split terminal panes
sequenceDiagram
participant User as User
participant DragUI as Tab Item (Dragged)
participant DropUI as Tab Item (Target)
participant DragLogic as Drag-to-Tab Logic
participant Store as Tabs Store
User->>DragUI: Drag tab A over tab B
DragUI->>DropUI: useDragTab emits drag start (tabId: A)
DropUI->>DropUI: useGroupDrop detects drop target (tabId: B)
User->>DropUI: Release (drop)
DropUI->>DragLogic: dragTabToTab(tabIdA, tabIdB)
activate DragLogic
alt Tab A != Tab B
DragLogic->>DragLogic: Check if B is group, child, or single
alt B is group
DragLogic->>DragLogic: Add A to B's layout
else B is single & A is single
DragLogic->>DragLogic: Create new group with A and B
else B is child
DragLogic->>DragLogic: Redirect to parent group
end
DragLogic->>Store: Update tabs, layouts, parentId
end
deactivate DragLogic
Store-->>DropUI: Subscribers notified
DropUI-->>User: UI re-renders with new structure
sequenceDiagram
participant User as User
participant Sidebar as Tab Sidebar
participant Store as Tabs Store
participant ActiveTab as Active Tab Helper
participant Terminal as Terminal Component
User->>Sidebar: Click tab to activate
Sidebar->>Store: setActiveTab(workspaceId, tabId)
activate Store
Store->>ActiveTab: handleSetActiveTab(state, workspaceId, tabId)
activate ActiveTab
ActiveTab->>ActiveTab: Update activeTabIds[workspaceId] = tabId
ActiveTab->>ActiveTab: Push tabId to tabHistoryStacks[workspaceId]
ActiveTab->>ActiveTab: Clear needsAttention flag
ActiveTab-->>Store: Return updated state
deactivate ActiveTab
Store->>Store: Merge state changes
Store-->>Sidebar: Subscribers notified
deactivate Store
Sidebar-->>Terminal: Tab state propagates to TabsContent
Terminal->>Terminal: Re-render active tab's Terminal component
Terminal->>User: Terminal gains focus / becomes visible
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (18)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts (1)
5-30: AligncanDrop/isDragOverwith actual drop predicateCurrent logic prevents no-op drops on the same group via
if (!didDrop && item.tabId !== groupId), butcanDropis always whatever react‑dnd’s default is, soisDragOvercan still betrueeven when the drop would be ignored.You could tighten this up by adding an explicit
canDrop:useDrop<DragItem, void, { isOver: boolean; canDrop: boolean }>({ accept: TAB_DND_TYPE, canDrop: (item) => item.tabId !== groupId, drop: (item, monitor) => { const didDrop = monitor.didDrop(); if (!didDrop && item.tabId !== groupId) { dragTabToTab(item.tabId, groupId); } }, collect: (monitor) => ({ isOver: monitor.isOver({ shallow: true }), canDrop: monitor.canDrop(), }), });This keeps hover styling in sync with what will actually drop.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (1)
1-13: Avoid duplicatingDragItem/TAB_DND_TYPEacross modulesThe typings here look correct, but there’s an identical
DragItem/TAB_DND_TYPEpair inContentView/TabsContent/types.ts. Keeping these in sync across files is brittle.Consider extracting them to a single shared module (e.g. a
tabs-dnd-types.tsco‑located with other tab DnD helpers) and importing from there in both Sidebar and ContentView code.apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts (1)
3-61: Active tab + history helpers look consistent
handleSetActiveTabcorrectly maintains per‑workspaceactiveTabIds, keeps a de‑duplicated LRU‑styletabHistoryStacks[workspaceId], and clearsneedsAttentiononly on the newly activated tab. The selectors (getTabsByWorkspace,getActiveTab,getLastActiveTabId) are straightforward and align with theTabsStateshape.If history growth ever becomes a concern, you could cap
newHistoryStackto a max length (e.g. last 50 tabs), but that’s not strictly necessary right now.apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)
4-40: Workspace → terminal tab creation flow looks correctHooking into
useTabsStoreto calladdTab(data.workspace.id)and then passing the resultingtabIdintoterminal.createOrAttachaligns with the tab-centric terminal model and keeps IPC on tRPC. The behavior described in the docstring (“Creates a terminal tab with setup commands if present”) matches the implementation.If you end up standardizing on the
renderer/stores/tabsbarrel for imports elsewhere, you could switch theuseTabsStoreimport to that path for consistency, but the current import is functionally fine.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts (1)
1-42: Tab drag/drop wiring is solid
useDragTabcleanly sets up a TAB drag source and a matching drop target that forwards todragTabToTabwhen dropping onto a different tab, withisDragging/isDragOverexposed for styling. The use ofreact-dndgenerics and the store selector keeps things type-safe and aligned with the central tab drag logic.If you ever need finer-grained hover behavior (e.g. avoiding nested drops), you could switch
monitor.isOver()tomonitor.isOver({ shallow: true }), but the current implementation is perfectly acceptable.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
26-27: Consider memoizinggetChildTabsto prevent unnecessary re-renders.This function is recreated on every render and passed implicitly through closures. If
TabItemuses this to derive child tabs, it may cause unnecessary re-renders. Consider usinguseCallback:- const getChildTabs = (parentId: string) => - allTabs.filter((tab) => tab.parentId === parentId); + const getChildTabs = useCallback( + (parentId: string) => allTabs.filter((tab) => tab.parentId === parentId), + [allTabs], + );Alternatively, since
getChildTabs(tab.id)is called inline at line 65, you could compute child tabs within the map and pass them directly.apps/desktop/src/renderer/stores/tabs/helpers/group-operations.test.ts (1)
7-54: Consider test data consistency:child3is not in the group layout.The test includes
child3in the tabs array but thelayoutonly referenceschild1andchild2. While this might work ifhandleRemoveChildTabFromGroupderives children fromparentIdrather than layout, this inconsistency could mask bugs or cause confusion.Based on learnings, tests should be readable - ensuring test data accurately represents real scenarios improves clarity.
{ id: "group1", title: "Group 1", workspaceId: "workspace1", type: TabType.Group, layout: { direction: "row", first: "child1", - second: "child2", + second: { + direction: "row", + first: "child2", + second: "child3", + splitPercentage: 50, + }, }, },apps/desktop/src/renderer/stores/tabs/store.test.ts (1)
244-274: Early return could silently pass the test if group is not found.Lines 246, 260 use early returns that would cause the test to pass without actually verifying the assertions below them. Consider using explicit assertions instead.
// Find the group tab const groupTab = state.tabs.find((t) => t.type === TabType.Group); expect(groupTab).toBeDefined(); - if (groupTab?.type !== TabType.Group) return; + expect(groupTab?.type).toBe(TabType.Group); + if (groupTab?.type !== TabType.Group) throw new Error("Expected group tab"); expect(groupTab.layout).toEqual({ direction: "row", first: "tab-1", second: expect.any(String), splitPercentage: 50, }); // Original tab should now be a child const originalTab = state.tabs.find((t) => t.id === "tab-1"); expect(originalTab?.parentId).toBe(groupTab.id); // New child should exist - if (typeof groupTab.layout === "string" || !groupTab.layout) return; + expect(typeof groupTab.layout).not.toBe("string"); + expect(groupTab.layout).not.toBeNull(); + if (typeof groupTab.layout === "string" || !groupTab.layout) throw new Error("Expected object layout");apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx (1)
45-72: Hover computation may cause performance issues with frequent state updates.The
setDropIndexcall on every hover event can trigger re-renders for each mouse movement. Consider adding a check to only update when the index actually changes.hover: (_item, monitor) => { if (!containerRef.current) return; const clientOffset = monitor.getClientOffset(); if (!clientOffset) return; const containerRect = containerRef.current.getBoundingClientRect(); const hoverY = clientOffset.y - containerRect.top; // Get all tab elements const tabElements = containerRef.current.querySelectorAll("[data-tab-item]"); let newDropIndex = 0; for (let i = 0; i < tabElements.length; i++) { const element = tabElements[i] as HTMLElement; const rect = element.getBoundingClientRect(); const elementY = rect.top - containerRect.top; const elementMiddle = elementY + rect.height / 2; if (hoverY < elementMiddle) { newDropIndex = i; break; } newDropIndex = i + 1; } - setDropIndex(newDropIndex); + // Only update state if the index changed to avoid unnecessary re-renders + setDropIndex((prev) => (prev !== newDropIndex ? newDropIndex : prev)); },apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (1)
28-45: Consider using the existinggetChildTabIdsutility instead of duplicating traversal logic.The
extractTabIdsFromLayoutfunction traverses the MosaicNode structure to extract tab IDs, but there's already agetChildTabIdsutility inrenderer/stores/tabs/utils.tsthat retrieves child tab IDs by filtering the tabs array. However, these serve different purposes—extractTabIdsFromLayoutextracts IDs from the layout tree structure, whilegetChildTabIdsqueries the store.The current implementation is correct for detecting layout-based changes. Consider adding a brief comment explaining why this traversal is needed (to detect removed tabs from the layout structure, not just store state).
apps/desktop/src/renderer/stores/tabs/utils/terminal-naming.test.ts (1)
109-127: Test violates "one assert per test" guideline.This test has multiple assertions checking sequential name generation. While the test logic is valid, it should be split into separate test cases per the coding guidelines. Based on learnings, tests should have one assert per test.
Consider splitting into separate tests:
- it("should generate sequential names for multiple new terminals", () => { - const strategy = new DefaultTerminalNamingStrategy(); - const existingNames: string[] = []; - - const name1 = strategy.generateName(existingNames); - expect(name1).toBe("Terminal"); - existingNames.push(name1); - - const name2 = strategy.generateName(existingNames); - expect(name2).toBe("Terminal (1)"); - existingNames.push(name2); - - const name3 = strategy.generateName(existingNames); - expect(name3).toBe("Terminal (2)"); - existingNames.push(name3); - - expect(existingNames).toEqual(["Terminal", "Terminal (1)", "Terminal (2)"]); - }); + it("should generate first terminal name", () => { + const strategy = new DefaultTerminalNamingStrategy(); + expect(strategy.generateName([])).toBe("Terminal"); + }); + + it("should generate second terminal name after first exists", () => { + const strategy = new DefaultTerminalNamingStrategy(); + expect(strategy.generateName(["Terminal"])).toBe("Terminal (1)"); + }); + + it("should generate third terminal name after first two exist", () => { + const strategy = new DefaultTerminalNamingStrategy(); + expect(strategy.generateName(["Terminal", "Terminal (1)"])).toBe("Terminal (2)"); + });apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
17-20: Tab lookup on every render may cause performance overhead.The
useTabs()hook returns all tabs, and filtering withfind()on every render could be inefficient if there are many tabs. Consider using a selector that returns only the needed tab:- const tabs = useTabs(); - const tab = tabs.find((t) => t.id === tabId); - const tabTitle = tab?.title || "Terminal"; + const tabTitle = useTabsStore( + (state) => state.tabs.find((t) => t.id === tabId)?.title || "Terminal", + );This creates a more targeted selector that only re-renders when the specific tab's title changes.
apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts (1)
37-38: Redundant tab lookup.The
tabToMovelookup on line 37 is redundant sincetabfrom line 25 is guaranteed to be inworkspaceTabs(already verified it has noparentIdat line 27).- const tabToMove = workspaceTabs.find((t) => t.id === tabId); - if (!tabToMove) return {}; - - const filteredTabs = workspaceTabs.filter((t) => t.id !== tabId); - filteredTabs.splice(targetIndex, 0, tabToMove); + const filteredTabs = workspaceTabs.filter((t) => t.id !== tabId); + filteredTabs.splice(targetIndex, 0, tab);apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (1)
193-230: Consider extracting duplicated repositioning logic.The tab repositioning logic (lines 193-207 and 215-230) is duplicated between the empty-group and non-empty branches. This could be extracted into a helper function.
const repositionTabInWorkspace = ( tabs: Tab[], tabId: string, workspaceId: string, targetIndex: number, ): Tab[] => { const workspaceTabs = tabs.filter( (t) => t.workspaceId === workspaceId && !t.parentId, ); const otherTabs = tabs.filter( (t) => t.workspaceId !== workspaceId || t.parentId, ); const tabToMove = workspaceTabs.find((t) => t.id === tabId); if (!tabToMove) return tabs; const filteredTabs = workspaceTabs.filter((t) => t.id !== tabId); filteredTabs.splice(targetIndex, 0, tabToMove); return [...otherTabs, ...filteredTabs]; };apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
203-203: Inconsistent ID generation pattern.Group IDs use
Date.now()alone (line 203, 391), whilecreateNewTabinutils.tsusesDate.now()-${random}. This inconsistency could lead to ID collisions in rapid operations or tests.Consider using the same pattern as
createNewTab:- const groupId = `tab-${Date.now()}-group`; + const groupId = `tab-${Date.now()}-${Math.random().toString(36).substring(2, 11)}-group`;Or extract a shared
generateId()utility.apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts (1)
80-184: Consider consolidating vertical and horizontal split handlers.
handleSplitTabVerticalandhandleSplitTabHorizontalare nearly identical, differing only in thedirectionparameter ("row" vs "column"). This could be refactored into a single internal handler.const handleSplitTab = ( state: TabsState, workspaceId: string, direction: "row" | "column", sourceTabId?: string, path?: MosaicBranch[], ): Partial<TabsState> => { // ... shared logic }; export const handleSplitTabVertical = ( state: TabsState, workspaceId: string, sourceTabId?: string, path?: MosaicBranch[], ): Partial<TabsState> => handleSplitTab(state, workspaceId, "row", sourceTabId, path); export const handleSplitTabHorizontal = ( state: TabsState, workspaceId: string, sourceTabId?: string, path?: MosaicBranch[], ): Partial<TabsState> => handleSplitTab(state, workspaceId, "column", sourceTabId, path);apps/desktop/src/renderer/stores/tabs/types.ts (1)
3-6: Naming conflict:TabTypeenum shadows shared type.This
TabTypeenum with valuesSingleandGroupconflicts with theTabTypeunion type defined inapps/desktop/src/shared/types.ts(which includes"terminal","editor","browser", etc.). This creates ambiguity and potential import errors when both types are needed in the same file.Consider renaming this enum to something more specific like
TabKindorTabVariantto avoid confusion with the sharedTabType.-export enum TabType { - Single = "single", - Group = "group", +export enum TabKind { + Single = "single", + Group = "group", }apps/desktop/src/renderer/stores/tabs/store.ts (1)
139-146: Remove redundant state spread.Zustand's
set()performs a shallow merge by default, so spreading...stateis unnecessary and adds overhead.setNeedsAttention: (tabId, needsAttention) => { - set((state) => ({ - ...state, - tabs: state.tabs.map((tab) => - tab.id === tabId ? { ...tab, needsAttention } : tab, - ), - })); + set((state) => ({ + tabs: state.tabs.map((tab) => + tab.id === tabId ? { ...tab, needsAttention } : tab, + ), + })); },
| const _handleFocus = () => { | ||
| setActiveTab(workspaceId, tabId); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused focus handler.
The _handleFocus function is defined but never called or passed as a prop. This is dead code that should be removed to keep the codebase clean.
Apply this diff:
- const _handleFocus = () => {
- setActiveTab(workspaceId, tabId);
- };
-
const handleCloseTab = (e: React.MouseEvent) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const _handleFocus = () => { | |
| setActiveTab(workspaceId, tabId); | |
| }; | |
| const handleCloseTab = (e: React.MouseEvent) => { |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx
around lines 41-43, the _handleFocus arrow function is defined but never used;
remove the unused _handleFocus declaration and any related dead-code references
(imports or variables solely used by it) so the file has no dangling, unused
handler and compiles cleanly.
| {tabToRender.type === TabType.Single && ( | ||
| <SingleTabView tab={tabToRender} isDropZone={isDropZone} /> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the SingleTabView file and check its props definition
fd "SingleTabView" apps/desktop/src/renderer --type f
# Check the file for interface definition
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView/SingleTabView.tsx | head -60
# Search for isDropZone usage in SingleTabView
rg -n "isDropZone" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView/Repository: superset-sh/superset
Length of output: 481
🏁 Script executed:
#!/bin/bash
# Read the SingleTabView.tsx file
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx | head -80
# Search for isDropZone in the file
rg -n "isDropZone" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
# Also check the props interface definition more carefully
rg -n "interface|type.*Props|export.*SingleTabView" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsxRepository: superset-sh/superset
Length of output: 1719
Remove unused isDropZone prop from SingleTabView.
The isDropZone prop is defined in SingleTabViewProps but neither destructured in the function signature (line 16) nor used within the component. Remove this prop from the interface and stop passing it when calling SingleTabView in TabsContent/index.tsx (line 45).
interface SingleTabViewProps {
tab: SingleTab;
}
export function SingleTabView({ tab }: SingleTabViewProps) {
// ...
}
And in TabsContent/index.tsx:
<SingleTabView tab={tabToRender} />
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
around lines 44-46 and the SingleTabView component file (where
SingleTabViewProps is defined), remove the unused isDropZone prop: delete it
from the SingleTabViewProps interface and the function signature (so the
interface only contains tab: SingleTab), and stop passing isDropZone when
rendering SingleTabView in TabsContent (change the JSX to pass only
tab={tabToRender}); ensure no other references to isDropZone remain and run TS
checks to update any affected imports/types.
| interface SingleTabViewProps { | ||
| tab: SingleTab; | ||
| isDropZone: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused isDropZone prop.
The isDropZone prop is defined in the interface (line 13) but is never destructured or used in the component implementation (line 16). This suggests it's either leftover from a previous implementation or intended for future use but currently serves no purpose.
Apply this diff:
interface SingleTabViewProps {
tab: SingleTab;
- isDropZone: boolean;
}
-export function SingleTabView({ tab }: SingleTabViewProps) {
+export function SingleTabView({ tab }: SingleTabViewProps) {Also applies to: 16-16
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
around lines 11-16, the interface defines an unused isDropZone prop and the
component signature also includes it on line 16; remove isDropZone from the
SingleTabViewProps interface and from the component props/signature, and update
any call sites that pass isDropZone to this component (remove the prop there as
well) so the prop is fully eliminated from type and usage.
| const _handleFocus = () => { | ||
| setActiveTab(tab.workspaceId, tab.id); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused focus handler.
The _handleFocus function is defined but never called or used. This is dead code that should be removed.
Apply this diff:
const handleClosePane = () => {
removeTab(tab.id);
};
-
- const _handleFocus = () => {
- setActiveTab(tab.workspaceId, tab.id);
- };
return (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const _handleFocus = () => { | |
| setActiveTab(tab.workspaceId, tab.id); | |
| }; | |
| const handleClosePane = () => { | |
| removeTab(tab.id); | |
| }; | |
| return ( |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
around lines 34-36, the _handleFocus function is defined but never used; remove
the entire _handleFocus function declaration (and any trailing blank line) to
eliminate dead code, and run a quick search in this file to ensure there are no
remaining references to _handleFocus to avoid breaking behavior.
| terminalTheme, | ||
| handleTerminalFocus, | ||
| ]); | ||
| }, [tabId, workspaceId, setActiveTab, workspaceCwd, tabTitle, terminalTheme]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx | head -250 | tail -50Repository: superset-sh/superset
Length of output: 119
🏁 Script executed:
git ls-files | grep -i terminal.tsx | grep -i rendererRepository: superset-sh/superset
Length of output: 173
🏁 Script executed:
wc -l apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxRepository: superset-sh/superset
Length of output: 177
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxRepository: superset-sh/superset
Length of output: 9359
🏁 Script executed:
rg -n "createOrAttach" apps/desktop/src --type ts --type tsx -B 2 -A 5 | head -80Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg -n "createOrAttach" apps/desktop/src -A 5 | head -100Repository: superset-sh/superset
Length of output: 7145
🏁 Script executed:
sed -n '41,120p' apps/desktop/src/main/lib/terminal-manager.tsRepository: superset-sh/superset
Length of output: 2093
🏁 Script executed:
sed -n '41,150p' apps/desktop/src/main/lib/terminal-manager.ts | tail -50Repository: superset-sh/superset
Length of output: 1259
🏁 Script executed:
rg "SUPERSET_TAB_TITLE" apps/Repository: superset-sh/superset
Length of output: 340
🏁 Script executed:
grep -n "tabTitle" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxRepository: superset-sh/superset
Length of output: 231
Remove tabTitle from effect dependencies to prevent terminal UI recreation on tab rename.
tabTitle is only used during initial terminal creation and restart operations. Including it in the dependency array causes the effect to rerun whenever the tab is renamed, destroying and recreating the xterm UI instance unnecessarily. Since tabTitle is accessed as part of the effect closure and the backend's createOrAttach is idempotent (reusing existing sessions by tabId), tabTitle should be removed from the dependencies.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
around line 219, the useEffect dependency array includes tabTitle which causes
the terminal UI to be destroyed and recreated when a tab is renamed; remove
tabTitle from the dependency array so the effect does not rerun on renames. If
you need the latest tabTitle for explicit restart/creation flows, read it
on-demand (e.g., from a ref or pass it into the restart handler) rather than
listing it as an effect dependency.
| <button | ||
| type="button" | ||
| tabIndex={-1} | ||
| onClick={handleRemoveTab} | ||
| className="cursor-pointer opacity-0 group-hover:opacity-100 ml-2 text-xs shrink-0" | ||
| > | ||
| <HiMiniXMark className="size-4" /> | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Close button missing accessible label.
The close button uses only an icon without accessible text. Screen reader users won't know the button's purpose.
<button
type="button"
tabIndex={-1}
onClick={handleRemoveTab}
className="cursor-pointer opacity-0 group-hover:opacity-100 ml-2 text-xs shrink-0"
+ aria-label={`Close ${tab.title}`}
>
<HiMiniXMark className="size-4" />
</button>📝 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.
| <button | |
| type="button" | |
| tabIndex={-1} | |
| onClick={handleRemoveTab} | |
| className="cursor-pointer opacity-0 group-hover:opacity-100 ml-2 text-xs shrink-0" | |
| > | |
| <HiMiniXMark className="size-4" /> | |
| </button> | |
| )} | |
| <button | |
| type="button" | |
| tabIndex={-1} | |
| onClick={handleRemoveTab} | |
| className="cursor-pointer opacity-0 group-hover:opacity-100 ml-2 text-xs shrink-0" | |
| aria-label={`Close ${tab.title}`} | |
| > | |
| <HiMiniXMark className="size-4" /> | |
| </button> | |
| )} |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
around lines 162-170, the close button only renders an icon and lacks an
accessible label; add an accessibility label (e.g., aria-label="Close tab" or
aria-label={`Close ${tabTitle}`}), or include visually hidden text for screen
readers, and keep the button keyboard-focusable (remove tabIndex={-1} or set to
0) so screen reader and keyboard users can identify and activate the control.
| interface TabContextMenuProps { | ||
| children: ReactNode; | ||
| tabId: string; | ||
| tabType: TabType; | ||
| hasParent?: boolean; | ||
| onClose: () => void; | ||
| onRename: () => void; | ||
| onUngroup?: () => void; | ||
| onMoveOutOfGroup?: () => void; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused tabId prop.
The tabId prop is defined in the TabContextMenuProps interface (line 13) but is never used in the component implementation. It's not destructured from the props (line 22-30) and serves no purpose.
Apply this diff:
interface TabContextMenuProps {
children: ReactNode;
- tabId: string;
tabType: TabType;
hasParent?: boolean;
onClose: () => void;
onRename: () => void;
onUngroup?: () => void;
onMoveOutOfGroup?: () => void;
}Also applies to: 22-30
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx
around lines 11-20 (and corresponding prop usage at 22-30), remove the unused
tabId prop from the TabContextMenuProps interface and from the component's props
destructuring; update the component signature and any local references to no
longer expect tabId, and remove or stop passing tabId from any parent/call sites
to avoid type errors. Ensure the props interface only includes children,
tabType, hasParent, onClose, onRename, onUngroup, and onMoveOutOfGroup so the
component compiles without unused fields.
| // Sync rename value when tab title changes | ||
| useEffect(() => { | ||
| setRenameValue(tabTitle); | ||
| }, [tabTitle]); |
There was a problem hiding this comment.
Sync effect may overwrite user input during active rename.
If tabTitle changes externally while the user is actively renaming (e.g., due to a concurrent update), this effect will overwrite their in-progress input. Consider guarding against this:
// Sync rename value when tab title changes
useEffect(() => {
+ if (!isRenaming) {
setRenameValue(tabTitle);
+ }
}, [tabTitle]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts
around lines 17-20, the effect that sets renameValue from tabTitle will
overwrite a user's in-progress rename when tabTitle changes externally; guard
the sync so it only updates renameValue when the user is not actively renaming
(e.g., add or use an isRenaming flag / input focus check, or only setRenameValue
when renameValue === previous tabTitle), and update the effect to check that
condition before calling setRenameValue so external title updates do not clobber
in-progress edits.
| const group = state.tabs.find( | ||
| (t) => t.id === tabToSplit.parentId && t.type === TabType.Group, | ||
| ); | ||
| if (!group || group.type !== TabType.Group || !group.layout) return state; |
There was a problem hiding this comment.
Inconsistent error return value.
This function returns state on error (line 196), while other handlers in this codebase return {} to indicate a no-op. Returning full state could mask issues and cause unexpected spread behavior.
- if (!group || group.type !== TabType.Group || !group.layout) return state;
+ if (!group || group.type !== TabType.Group || !group.layout) return {};📝 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.
| if (!group || group.type !== TabType.Group || !group.layout) return state; | |
| if (!group || group.type !== TabType.Group || !group.layout) return {}; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts around line
196, the error branch currently returns the full state (return state) which is
inconsistent with other handlers that return an empty object to indicate a
no-op; change the early-return to return {} instead of state and ensure the
function's callers expect an object (adjust typing if necessary) so error/no-op
behavior matches the codebase convention and avoids accidental state spread.
| const [removed] = workspaceTabs.splice(startIndex, 1); | ||
| workspaceTabs.splice(endIndex, 0, removed); |
There was a problem hiding this comment.
Missing bounds validation for startIndex.
If startIndex is out of bounds (e.g., negative or >= array length), splice returns an empty array, making removed undefined. This undefined value would then be inserted into the array at endIndex, potentially corrupting state.
Consider adding a guard:
+ if (startIndex < 0 || startIndex >= workspaceTabs.length) return {};
+
const [removed] = workspaceTabs.splice(startIndex, 1);
+ if (!removed) return {};
workspaceTabs.splice(endIndex, 0, removed);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts around lines
14-15, the code removes an element using workspaceTabs.splice(startIndex, 1)
without validating startIndex; if startIndex is out of bounds splice returns an
empty array and removed becomes undefined which may corrupt state when inserted.
Add a guard that verifies startIndex is an integer within [0,
workspaceTabs.length - 1] (and normalize negative indices if intended) before
performing the splice; if invalid, no-op or return the original array (or throw
a clear error) and only perform the two splice operations when startIndex is
valid. Ensure endIndex is also clamped into the valid range before inserting.
This refactor renames all internal 'window' terminology to 'tab' to avoid confusion with Electron windows. The changes include: - Window interface → Tab interface - WindowsState/WindowsStore → TabsState/TabsStore - useWindowsStore → useTabsStore - windowId → tabId (in Pane interface and throughout) - windows array → tabs array - activeWindowIds → activeTabIds - windowHistoryStacks → tabHistoryStacks - All window-related function names (addWindow → addTab, etc.) - Storage keys and DevTools names updated - Comments and documentation updated - Drag type constant: WINDOW → TAB All typechecks and tests pass. This aligns with the previous PR #240 which renamed tabs to windows, and now we're reverting that to use tabs for internal terminal containers to avoid confusion with Electron windows.
* refactor: rename internal terminal windows to tabs This refactor renames all internal 'window' terminology to 'tab' to avoid confusion with Electron windows. The changes include: - Window interface → Tab interface - WindowsState/WindowsStore → TabsState/TabsStore - useWindowsStore → useTabsStore - windowId → tabId (in Pane interface and throughout) - windows array → tabs array - activeWindowIds → activeTabIds - windowHistoryStacks → tabHistoryStacks - All window-related function names (addWindow → addTab, etc.) - Storage keys and DevTools names updated - Comments and documentation updated - Drag type constant: WINDOW → TAB All typechecks and tests pass. This aligns with the previous PR #240 which renamed tabs to windows, and now we're reverting that to use tabs for internal terminal containers to avoid confusion with Electron windows. * refactor: rename 'New Tab' button to 'New Terminal' and update tab names to 'Terminal N' - Change button text from 'New Tab' to 'New Terminal' - Update default tab names from 'Tab 1', 'Tab 2' to 'Terminal 1', 'Terminal 2' - Update fallback display name from 'Tab' to 'Terminal' * refactor: rename window references to tab in component props and variables * refactor: rename WindowItem to TabItem and WindowContextMenu to TabContextMenu * refactor: rename WindowView to TabView and WindowPane to TabPane
1 participant
Reverts #220
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.