refactor (Desktop): Make it so when you make a new terminal it makes it in your screen#381
refactor (Desktop): Make it so when you make a new terminal it makes it in your screen#381
Conversation
WalkthroughAdds optional pane-creation options to the tabs store and updates the TabsView to create either a new pane on the active tab (with options) or a new tab, refining preset handling and command UI closure. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (UI)
participant TabsView as TabsView Component
participant TabsStore as Tabs Store
participant PaneFactory as Pane Creation Logic
User->>TabsView: Trigger "Add Tab"/select preset
TabsView->>TabsView: Determine activeWorkspaceId / activeTabId
alt activeTab exists
TabsView->>TabsStore: addPane(activeTabId, options)
TabsStore->>PaneFactory: createPane(activeTabId, "terminal", options)
PaneFactory-->>TabsStore: newPaneId
TabsStore-->>TabsView: pane created
else no activeTab
TabsView->>TabsStore: addTab(activeWorkspaceId, options)
TabsStore-->>TabsView: newTabId
TabsView->>TabsStore: (optional) renameTab(newTabId, name)
end
TabsView->>TabsView: setCommandOpen(false)
TabsView-->>User: UI updated (tab/pane shown)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
AviPeltz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(3 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)
🧰 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/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/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/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
CreatePaneOptions(38-41)createPane(46-62)
⏰ 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). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Deploy Marketing
- GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
321-326: LGTM! Clean extension of pane creation API.The signature extension to accept optional
CreatePaneOptionsand passing them tocreatePaneis correct and enables callers to specify initial commands and working directory when adding panes.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (3)
58-60: LGTM! Active tab resolution.Correctly derives
activeTabIdfrom the store'sactiveTabIdsmap using the currentactiveWorkspaceId, enabling the new pane-vs-tab logic.
62-72: LGTM! Pane-first creation strategy.The new logic aligns with the PR objective: when an active tab exists, a pane is added to it (keeping the terminal in the visible screen); otherwise, a new tab is created. The guard clause and command dialog closure are correctly placed.
79-99: LGTM! Consistent preset handling with conditional tab renaming.The preset handling correctly:
- Builds
presetOptionswith the preset's commands and cwd- Follows the same pane-first strategy (adds pane to active tab when available)
- Only renames the tab when creating a new one (line 93-95), which makes sense since renaming an existing tab would be unexpected
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
27-31: Consider consolidatingAddTabOptionswithCreatePaneOptions.Both
AddTabOptions(lines 27-31) andCreatePaneOptions(imported from utils.ts) now have identical structure with fieldsinitialCommands,initialCwd, andname. This duplication violates the DRY principle and creates maintenance overhead.Consider one of these approaches:
Option 1: Use
CreatePaneOptionsdirectly-/** - * Options for creating a tab with preset configuration - */ -export interface AddTabOptions { - initialCommands?: string[]; - initialCwd?: string; - name?: string; -} /** * Actions available on the tabs store */ export interface TabsStore extends TabsState { // Tab operations addTab: ( workspaceId: string, - options?: AddTabOptions, + options?: CreatePaneOptions, ) => { tabId: string; paneId: string };Option 2: Make
AddTabOptionsextendCreatePaneOptionsif future divergence is expected/** * Options for creating a tab with preset configuration */ -export interface AddTabOptions { - initialCommands?: string[]; - initialCwd?: string; - name?: string; -} +export interface AddTabOptions extends CreatePaneOptions { + // Add tab-specific options here if needed in the future +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(3 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(3 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(2 hunks)
🧰 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/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/types.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/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/types.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/types.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/stores/tabs/types.ts
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
🧠 Learnings (3)
📚 Learning: 2025-12-12T05:45:09.686Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.686Z
Learning: Applies to apps/desktop/src/main/lib/*.ts : Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Applied to files:
apps/desktop/src/renderer/stores/tabs/types.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} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/stores/tabs/types.ts
📚 Learning: 2025-12-12T05:45:09.686Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.686Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/renderer/stores/tabs/types.ts
🧬 Code graph analysis (1)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
CreatePaneOptions(38-42)
⏰ 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). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (6)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
41-41: LGTM! Clean implementation of optional pane naming.The addition of the
namefield toCreatePaneOptionsand its usage increatePaneis well-implemented with a sensible fallback to "Terminal".Also applies to: 58-58
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (3)
58-60: LGTM! Clean derivation of active tab.The activeTabId derivation correctly handles the case when there's no active workspace.
62-76: LGTM! Correct implementation of the PR objective.The refactored logic correctly adds a pane to the active tab (keeping the terminal in the current view) before falling back to creating a new tab. The early return, fallback logic, and command dialog closure are all properly handled.
83-111: LGTM! Preset handling with correct tab naming.The preset flow correctly:
- Adds pane to the active tab with preset options when possible
- Falls back to creating a new tab when needed
- Only renames the tab to the preset name when creating a new tab (not when adding a pane to an existing tab)
The comment on line 104 clearly documents the intentional behavior.
apps/desktop/src/renderer/stores/tabs/types.ts (2)
3-3: LGTM! Import addition supports the enhanced addPane signature.The type-only import of
CreatePaneOptionsis correctly added to support the updatedaddPanemethod signature, addressing the previous review comment about type consistency.
55-55: LGTM! Signature correctly usesCreatePaneOptions.The
addPanesignature has been properly updated to accept optionalCreatePaneOptions, successfully addressing the previous review comment about type consistency. This enhancement allows callers to specifyname,initialCommands, andinitialCwdwhen creating panes.
| export interface AddTabOptions { | ||
| initialCommands?: string[]; | ||
| initialCwd?: string; | ||
| name?: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicate type definitions.
AddTabOptions and CreatePaneOptions now have identical shapes (all three fields match: initialCommands?, initialCwd?, and name?). This duplication violates DRY principles and creates maintenance risk—if one interface changes, the other must be kept in sync manually.
Consider one of these approaches:
Option 1 (recommended): Use CreatePaneOptions directly
Remove AddTabOptions and use CreatePaneOptions everywhere:
-/**
- * Options for creating a tab with preset configuration
- */
-export interface AddTabOptions {
- initialCommands?: string[];
- initialCwd?: string;
- name?: string;
-}
-
/**
* Actions available on the tabs store
*/
export interface TabsStore extends TabsState {
// Tab operations
addTab: (
workspaceId: string,
- options?: AddTabOptions,
+ options?: CreatePaneOptions,
) => { tabId: string; paneId: string };Option 2: Make AddTabOptions extend CreatePaneOptions
If AddTabOptions may diverge in the future:
/**
* Options for creating a tab with preset configuration
*/
-export interface AddTabOptions {
- initialCommands?: string[];
- initialCwd?: string;
- name?: string;
-}
+export interface AddTabOptions extends CreatePaneOptions {
+ // Add tab-specific options here if needed in the future
+}📝 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.
| export interface AddTabOptions { | |
| initialCommands?: string[]; | |
| initialCwd?: string; | |
| name?: string; | |
| } | |
| export interface AddTabOptions extends CreatePaneOptions { | |
| // Add tab-specific options here if needed in the future | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/types.ts around lines 27-31,
AddTabOptions duplicates CreatePaneOptions; remove the duplicate by deleting the
AddTabOptions interface and replace its usages with CreatePaneOptions (or, if
you expect divergence, make AddTabOptions extend CreatePaneOptions instead).
Update all imports/usages across the codebase to reference CreatePaneOptions,
export CreatePaneOptions if not already exported, and run the TypeScript
build/tests to ensure no remaining references to AddTabOptions.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.