Conversation
WalkthroughThis PR introduces a workspace setup workflow that loads setup configuration from a setup.json file, copies specified files to new workspaces, and executes setup commands in a dedicated Setup Tab within the workspace's terminal interface. The feature integrates with the workspace creation flow, tab system, TRPC router, and terminal components. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CreateWorkspace as Workspace<br/>Creation Mutation
participant SetupUtils as Setup<br/>Utilities
participant FileSystem as File<br/>System
participant FS as File<br/>Copy
participant React as React Query<br/>Hook
participant Store as Tabs<br/>Store
participant UI as Setup<br/>Tab UI
User->>CreateWorkspace: Create workspace
CreateWorkspace->>SetupUtils: loadSetupConfig()
SetupUtils->>FileSystem: Read .superset/setup.json
FileSystem-->>SetupUtils: Config (or null)
alt Config exists with patterns
CreateWorkspace->>SetupUtils: copySetupFiles(patterns)
SetupUtils->>FS: Glob match files
FS->>FileSystem: Copy matched files
FileSystem-->>FS: Success/errors
FS-->>SetupUtils: { copied, errors }
end
CreateWorkspace-->>React: Return<br/>(workspace, setupConfig,<br/>setupCopyResults)
React->>Store: addSetupTab(setupCommands,<br/>setupCwd, copyResults)
Store-->>UI: Create SetupTab
UI->>UI: Launch SetupTerminal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
14-26: Consider validating array element types.The validation checks if
copyandcommandsare arrays, but doesn't verify that each element is a string. Invalid element types could cause runtime errors incopySetupFilesor when executing commands.Apply this diff to add element validation:
if (parsed.copy && !Array.isArray(parsed.copy)) { throw new Error("'copy' field must be an array of strings"); } + if (parsed.copy && !parsed.copy.every((item) => typeof item === "string")) { + throw new Error("'copy' field must contain only strings"); + } if (parsed.commands && !Array.isArray(parsed.commands)) { throw new Error("'commands' field must be an array of strings"); } + if (parsed.commands && !parsed.commands.every((item) => typeof item === "string")) { + throw new Error("'commands' field must contain only strings"); + }apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)
50-58: Consider adding test coverage forcommandsfield validation.The test suite validates that
copymust be an array, but there's no corresponding test for thecommandsfield validation (which has identical logic at lines 22-24 insetup.ts).Add a test case:
test("validates commands field must be an array", () => { writeFileSync( join(MAIN_REPO, ".superset", "setup.json"), JSON.stringify({ commands: "not-an-array" }), ); const config = loadSetupConfig(MAIN_REPO); expect(config).toBeNull(); });apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
109-114: Consider documenting the return type transformation.The function returns
setupConfig: setupConfig?.commands || null, extracting only the commands array from the full config. This transformation means the UI only receives commands, not the copy patterns. While this appears intentional (copy happens server-side), documenting this choice would help future maintainers.Add a comment:
+ // Return only commands (copy patterns already processed) return { workspace, setupConfig: setupConfig?.commands || null, setupCopyResults, worktreePath, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(3 hunks)apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(3 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(36-167)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(36-167)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
loadSetupConfig(7-33)copySetupFiles(35-79)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
apps/desktop/src/shared/types.ts (1)
SetupConfig(130-133)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
loadSetupConfig(7-33)copySetupFiles(35-79)
⏰ 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 (11)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
45-50: Verify symbolic link handling is intentional.The
followSymbolicLinks: falseoption prevents following symlinks. If users have symlinked configuration files (e.g., shared.envfiles), they won't be copied. Ensure this behavior aligns with the intended use case.
35-79: LGTM! Comprehensive error handling.The function properly handles errors at multiple levels (glob, file copy) and aggregates them for the caller. The use of
recursive: truefor directory creation is appropriate.apps/desktop/src/renderer/stores/tabs/store.ts (1)
48-63: LGTM! Consistent with existing patterns.The
addSetupTabaction follows the established pattern of delegating to a helper function and properly forwards all parameters through the Zustand state setter.apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)
75-124: LGTM! Comprehensive file copy testing.The test suite covers key scenarios: empty patterns, glob matching, nested directories, error cases, and multiple file matching. The cleanup logic properly removes test directories.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
176-214: Setup execution flow looks solid.The guard ensures setup runs exactly once per terminal attachment, copy results are displayed before command execution, and the tab state is updated to reflect completion. The implementation properly integrates with the existing terminal flow.
204-206: The review comment's concern is incorrect and based on a misunderstanding of shell behavior.When commands separated by newlines are written to a shell's stdin (via a PTY), the shell processes them sequentially by default. Each command waits for the previous one to complete before starting—this is standard POSIX shell behavior. The backend writes directly to the PTY using
node-ptywithout buffering or batching, so commands are delivered correctly to the shell.The code at lines 205-206 properly executes setup commands sequentially as written. Using
&&between commands would add error handling (stopping if a command fails), but it's not necessary for sequential execution.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
98-132: LGTM! Consistent implementation.The
handleAddSetupTabfunction properly mirrors thehandleAddTabpattern, correctly manages tab history, and sets all setup-related properties. The implementation is clean and maintains consistency with existing tab operations.apps/desktop/src/renderer/stores/tabs/types.ts (2)
15-18: LGTM! Well-defined setup properties.The setup-related properties are appropriately typed and optional, allowing tabs to exist with or without setup metadata. The structure clearly captures all necessary setup information.
40-45: LGTM! Clear method signature.The
addSetupTabsignature is well-defined with explicit types for all parameters, making the optionalsetupCopyResultsparameter clear and type-safe.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
84-107: Solid error handling for setup file copying.The implementation properly treats file copy errors as non-fatal, allowing workspace creation to succeed even if some files fail to copy. The error messages are informative and include context.
176-176: Good defensive programming.The optional chaining prevents runtime errors if
groupsMap.get()returns undefined, which shouldn't happen based on the logic but adds safety.
| const pendingEventsRef = useRef<TerminalStreamEvent[]>([]); | ||
| const [subscriptionEnabled, setSubscriptionEnabled] = useState(false); | ||
| const setActiveTab = useSetActiveTab(); | ||
| const setupExecutedRef = useRef(false); |
There was a problem hiding this comment.
Setup execution guard is component-scoped, not tab-scoped.
The setupExecutedRef persists across the component lifecycle. If the component renders for a different tabId (e.g., switching tabs and back), the ref won't reset, potentially preventing setup from running for a different setup tab. Consider making the ref tab-specific:
-const setupExecutedRef = useRef(false);
+const setupExecutedRef = useRef<Record<string, boolean>>({});Then update the guard condition at line 180:
- !setupExecutedRef.current
+ !setupExecutedRef.current[tabId]And set it at line 182:
-setupExecutedRef.current = true;
+setupExecutedRef.current[tabId] = true;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
around line 25 (and update usages at ~180 and ~182), the setupExecutedRef is
currently component-scoped and prevents setup from running when switching to a
different tabId; change it to be tab-scoped by replacing the single boolean ref
with a keyed store (e.g., useRef<Record<string, boolean>>({}) or useRef(new
Map<string, boolean>())), update the guard at line ~180 to check the entry for
the current tabId (e.g., !setupExecutedRef.current[tabId] or
!setupExecutedRef.current.get(tabId)), and set the tab-specific flag at line
~182 after running setup (e.g., setupExecutedRef.current[tabId] = true or
setupExecutedRef.current.set(tabId, true)).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx (1)
55-65: Remove leading space in className.The Button implementation is correct, but the className on line 61 has an unnecessary leading space.
Apply this diff:
- className=" hover:text-white/80" + className="hover:text-white/80"
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
14-21:onSessionReadyguard is component‑scoped; may skip callbacks for other tabIds.
sessionReadyCalledRefis a single boolean shared across all uses of thisTerminalinstance. If the same component instance is ever reused with a differenttabId(e.g., React keeps the component and just changes props), the effect will re‑run for the new tab, butonSessionReadywill not fire becausesessionReadyCalledRef.currentis alreadytrue. That can prevent setup logic (or any other caller ofonSessionReady) from running for subsequent tabs.Consider making the guard tab‑scoped instead of component‑scoped, e.g.:
-const sessionReadyCalledRef = useRef(false); +const sessionReadyCalledRef = useRef<Record<string, boolean>>({}); ... - if (onSessionReady && !sessionReadyCalledRef.current) { - sessionReadyCalledRef.current = true; + if (onSessionReady && !sessionReadyCalledRef.current[tabId]) { + sessionReadyCalledRef.current[tabId] = true; onSessionReady({ xterm, write: (data: string) => writeRef.current({ tabId, data }), }); }This preserves the “once per tab” behavior while still allowing
Terminalto be reused safely across differenttabIdvalues.Also applies to: 28-29, 168-217
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx (1)
1-49: SetupTabView wiring matches other tab views; consider trimming unused pieces.The split/remove wiring and use of
TabContentContextMenuplusSetupTerminallook consistent with the existing tab patterns. Right nowisDropZoneandhandleFocusare unused in this component; if there’s no near‑term plan to use them (e.g., for styling or manual focus), you could drop them to keep the surface minimal.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.ts (1)
10-15: Consider exportingUseTerminalSetupParamsfor consistency.While this interface is currently only used internally, exporting it would improve type consistency and enable potential future consumers to type-check their parameters before passing them to the hook.
-interface UseTerminalSetupParams { +export interface UseTerminalSetupParams { tabId: string; setupPending?: boolean; setupCommands?: string[]; setupCopyResults?: SetupCopyResults; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/desktop/src/lib/trpc/routers/notifications.ts(1 hunks)apps/desktop/src/main/lib/agent-setup.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.test.ts(2 hunks)apps/desktop/src/main/lib/terminal-manager.ts(1 hunks)apps/desktop/src/main/windows/main.ts(1 hunks)apps/desktop/src/renderer/globals.css(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(3 hunks)apps/desktop/src/shared/ipc-channels/external.ts(1 hunks)apps/desktop/test-setup.ts(1 hunks)apps/website/src/app/components/DownloadButton/index.ts(0 hunks)apps/website/src/app/components/Header/Header.tsx(1 hunks)apps/website/src/app/components/JoinWaitlistButton/index.ts(0 hunks)apps/website/src/app/components/SocialLinks/SocialLinks.tsx(0 hunks)apps/website/src/app/components/SocialLinks/index.ts(0 hunks)apps/website/src/app/components/VideoSection/VideoSection.tsx(0 hunks)apps/website/src/app/components/VideoSection/index.ts(0 hunks)packages/ui/src/components/alert-dialog.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- apps/website/src/app/components/VideoSection/VideoSection.tsx
- apps/website/src/app/components/DownloadButton/index.ts
- apps/website/src/app/components/SocialLinks/SocialLinks.tsx
- apps/website/src/app/components/VideoSection/index.ts
- apps/website/src/app/components/JoinWaitlistButton/index.ts
- apps/website/src/app/components/SocialLinks/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/components/alert-dialog.tsx
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceDropdown.tsx
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Biome formatting, linting, and import organization at the root level for all code files
Files:
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.tsapps/desktop/src/lib/trpc/routers/notifications.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/test-setup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/website/src/app/components/Header/Header.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype; use type-safe approaches instead, unless necessary
Ensure TypeScript type checking passes across all packages using bun run typecheck
Files:
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.tsapps/desktop/src/lib/trpc/routers/notifications.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/test-setup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/website/src/app/components/Header/Header.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
apps/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
apps/**/*.{tsx,ts}: Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.tsapps/desktop/src/lib/trpc/routers/notifications.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/test-setup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/website/src/app/components/Header/Header.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
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/helpers/tab-crud.tsapps/desktop/src/lib/trpc/routers/notifications.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/test-setup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/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/helpers/tab-crud.tsapps/desktop/src/lib/trpc/routers/notifications.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/test-setup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
apps/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one component per file; do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/website/src/app/components/Header/Header.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Files:
apps/desktop/src/main/lib/terminal-history.test.ts
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-history.test.ts
🧠 Learnings (15)
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules in apps/desktop/src/lib/electron-router-dom.ts as it is shared between main and renderer processes
Applied to files:
apps/desktop/src/lib/trpc/routers/notifications.tsapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-manager.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : Implement Electron IPC handlers in dedicated files under apps/desktop/src/main/lib/ (e.g., workspace-ipcs.ts, terminal-ipcs.ts)
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsxapps/desktop/src/main/windows/main.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
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/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.tsapps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.tsapps/desktop/src/main/lib/agent-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define all Electron IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/shared/ipc-channels/external.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : In Electron IPC handlers, accept object parameters instead of positional parameters to maintain type safety
Applied to files:
apps/desktop/src/shared/ipc-channels/external.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
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/shared/ipc-channels/external.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to @(apps/desktop/src/renderer|apps/desktop/src/shared)/**/*.{ts,tsx} : Never import Node.js modules in Electron renderer process or shared code; use IPC for communication between main and renderer processes
Applied to files:
apps/desktop/src/shared/ipc-channels/external.tsapps/desktop/src/main/lib/agent-setup.tsapps/desktop/src/main/lib/terminal-manager.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env file in apps/desktop/src/main/index.ts before any imports with override: true
Applied to files:
apps/desktop/src/main/lib/agent-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.test.{ts,tsx} : Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Applied to files:
apps/desktop/src/main/lib/terminal-history.test.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/main/lib/terminal-history.test.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.{tsx,ts} : Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Applied to files:
apps/desktop/src/main/lib/terminal-history.test.tsapps/website/src/app/components/Header/Header.tsx
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to packages/ui/**/*.{tsx,ts} : Use TailwindCSS v4 and shadcn/ui for UI component styling and design
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be fast
Applied to files:
apps/desktop/test-setup.ts
🧬 Code graph analysis (6)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsState(37-41)apps/desktop/src/shared/types.ts (1)
TabType(23-30)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.ts (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
TerminalSession(3-6)apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(36-167)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
SetupTab(27-33)apps/desktop/src/renderer/stores/tabs/hooks.ts (4)
useSplitTabHorizontal(21-22)useSplitTabVertical(19-20)useRemoveTab(8-8)useSetActiveTab(10-11)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabContentContextMenu.tsx (1)
TabContentContextMenu(17-40)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/SetupTerminal.tsx (1)
SetupTerminal(15-44)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/types.ts (1)
TabType(23-30)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (3)
apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx (1)
SetupTabView(16-49)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx (1)
DropOverlay(5-13)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.ts (1)
Terminal(2-2)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (2)
TerminalProps(8-13)TerminalStreamEvent(15-17)apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
useSetActiveTab(10-11)
⏰ 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 (20)
apps/desktop/src/main/lib/terminal-manager.ts (1)
5-5: Import reordering is safe and consistentReordering
getSupersetPathafterNOTIFICATIONS_PORTdoes not affect runtime behavior and keeps imports organized; no further changes needed here.apps/desktop/src/renderer/globals.css (2)
26-26: CSS variable normalization; no functional impact.The change from
oklch(0.20 ...)tooklch(0.2 ...)is a numeric normalization—both render identically. This is fine.
64-64: Verify: CSS change appears tangential to PR objectives.The AI summary states the dark theme value "remains unchanged," but line 64 is marked as modified. Additionally, this CSS color token adjustment appears unrelated to the PR's workspace setup flow (setup config loading, file copying, terminal integration). Please clarify whether this change is intentional and its connection to the PR objectives.
apps/desktop/src/lib/trpc/routers/notifications.ts (1)
2-5: LGTM!This is a trivial import reordering change, likely from Biome's import organization. The file correctly uses tRPC for Electron IPC as per the coding guidelines, and the subscription pattern with proper cleanup is well-implemented.
apps/website/src/app/components/Header/Header.tsx (1)
7-9: LGTM! Import reordering aligns with PR-wide adjustments.The import reorganization is consistent with the broader changes in this PR where certain components are no longer exported from index barrels. The direct imports are appropriate, and all imported components are correctly used in the JSX.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx (2)
1-2: LGTM!The imports are correct and properly used in the component. Button from the UI library and the close icon are appropriate for the close button functionality.
46-49: LGTM!The close handler correctly prevents event propagation and invokes the removal callback. The implementation is clean and type-safe.
apps/desktop/test-setup.ts (1)
51-57: LGTM! Consistent mock pattern.The BrowserWindow mock now uses the same
mock(() => ({ ... }))pattern as other mocks in this file (e.g.,getPath,getName,showOpenDialog), which improves consistency and correctly represents BrowserWindow as a callable constructor that returns a mock window object.apps/desktop/src/main/lib/agent-setup.ts (1)
1-1: LGTM! Import consolidation improves code organization.Moving
execSyncto the top-level imports and removing the duplicate is a good refactoring that follows best practices.apps/desktop/src/main/windows/main.ts (1)
8-13: Import reordering is safe and keeps types/runtime values clearly separatedThe reordered imports (placing
type AgentCompleteEventandNOTIFICATIONS_PORTtogether) are behaviorally neutral and improve readability without affecting runtime behavior. KeepingAgentCompleteEventas a type‑only import is also in line with TypeScript best practices.apps/desktop/src/shared/ipc-channels/external.ts (1)
7-7: Type alias change is safe; no augmentation ofExternalChannelsdetectedThe change from
interface ExternalChannels {}totype ExternalChannels = {}is non-breaking. Verification confirmsExternalChannelsis only referenced inapps/desktop/src/shared/ipc-channels/index.tswhere it is extended by theIpcChannelsinterface. Type aliases can be used withextends, so the interface composition continues to work. No declaration merging or augmentation patterns exist in the codebase.The empty type aligns with the file's purpose: backwards compatibility for the IPC channels type system while external operations now use tRPC (as documented in the file header).
apps/desktop/src/main/lib/terminal-history.test.ts (2)
4-11: LGTM! Import reordering aligns with formatting standards.The reordering of
HistoryReaderandHistoryWriterimports is a formatting improvement that aligns with Biome's import organization rules specified in the coding guidelines.
382-383: LGTM! Improved type error handling.The change from
@ts-ignoreto@ts-expect-erroris a best practice improvement.@ts-expect-erroris superior because it expects a type error to exist and will warn if the suppression becomes unnecessary, making the code more maintainable.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
7-55: Setup tab branch integrates cleanly into existing tab rendering.The new
TabType.Setupbranch reuses the single‑tab split behavior and overlay semantics without affecting group handling; the control flow and JSX structure look consistent with the prior logic.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/index.ts (1)
1-2: Barrel export forSetupTerminalis appropriate.Re‑exporting
SetupTerminalnext toTerminalmakes the terminal module’s public API clearer and simplifies imports for callers.apps/desktop/src/renderer/stores/tabs/types.ts (1)
3-50: Setup tab typing fits cleanly into the existing tabs model.Extending
TabTypewithSetup, addingSetupTab, and updatingTab/TabsStorepreserves the discriminated union pattern and keeps store APIs explicit for setup‑specific tabs.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
1-13: Terminal session typing aligns with component usage.
TerminalSessionand the extendedTerminalPropscleanly describe whatTerminalexposes viaonSessionReady, and match the object structure built inTerminal.tsx.apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
98-136:handleAddSetupTabmirrors existing tab‑creation semantics correctly.The helper constructs a
Setuptab with the expected fields, updates the active tab for the workspace, and maintains the history stack in line withhandleAddTab, so it slots neatly into the existing tabs store behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.ts (2)
1-3: LGTM!The imports follow the project's alias patterns and properly reference the local types file.
21-70: Well-designed hook structure.The hook correctly uses
useRefto track execution state without triggering re-renders, avoids unnecessaryuseEffectcalls, and provides a clean API. The guard conditions properly prevent duplicate execution via both the local ref and the store'ssetupPendingflag.
| interface SetupCopyResults { | ||
| copied: string[]; | ||
| errors: string[]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move SetupCopyResults to the shared types file.
According to the AI summary, SetupCopyResults is produced by setup utilities and consumed by multiple components. This type should be defined in types.ts (in the same directory) alongside TerminalSession for better organization and to avoid duplication.
Move the type definition to types.ts:
export interface SetupCopyResults {
copied: string[];
errors: string[];
}Then update the import in this file:
import { useRef } from "react";
import { useTabsStore } from "renderer/stores";
-import type { TerminalSession } from "./types";
+import type { SetupCopyResults, TerminalSession } from "./types";🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.ts
around lines 5-8, the local SetupCopyResults interface should be moved to the
shared types file (types.ts) next to TerminalSession; create/export
SetupCopyResults there with the shape { copied: string[]; errors: string[] },
remove the local definition from this file, and add an import for
SetupCopyResults from the types.ts file.
| const executeSetup = (session: TerminalSession) => { | ||
| // Only execute setup once and only if setup is pending | ||
| if (!setupPending || !setupCommands || setupExecutedRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| setupExecutedRef.current = true; | ||
|
|
||
| // Print copy results if available | ||
| if (setupCopyResults) { | ||
| const { copied, errors } = setupCopyResults; | ||
| if (copied.length > 0) { | ||
| session.xterm.writeln( | ||
| `\r\n\x1b[32m✓ Copied ${copied.length} file(s):\x1b[0m`, | ||
| ); | ||
| for (const file of copied) { | ||
| session.xterm.writeln(` - ${file}`); | ||
| } | ||
| } | ||
| if (errors.length > 0) { | ||
| session.xterm.writeln(`\r\n\x1b[33m⚠ Copy warnings:\x1b[0m`); | ||
| for (const error of errors) { | ||
| session.xterm.writeln(` ${error}`); | ||
| } | ||
| } | ||
| session.xterm.writeln("\r\n"); | ||
| } | ||
|
|
||
| // Send all commands sequentially | ||
| const commands = `${setupCommands.join("\n")}\n`; | ||
| session.write(commands); | ||
|
|
||
| // Mark setup as no longer pending | ||
| useTabsStore.setState((state) => ({ | ||
| tabs: state.tabs.map((t) => | ||
| t.id === tabId ? { ...t, setupPending: false } : t, | ||
| ), | ||
| })); | ||
| }; |
There was a problem hiding this comment.
Add error handling around session operations.
The executeSetup function writes to the terminal session without error handling. If session.write() or session.xterm.writeln() throw an exception:
setupExecutedRef.currentwill be set totrue(line 35 executes before the error)- The store update at lines 62-66 won't execute
- The tab's
setupPendingremainstrue, butsetupExecutedRefprevents retry - Setup commands never execute, leaving the terminal in an inconsistent state
Wrap session operations in try-catch:
const executeSetup = (session: TerminalSession) => {
// Only execute setup once and only if setup is pending
if (!setupPending || !setupCommands || setupExecutedRef.current) {
return;
}
- setupExecutedRef.current = true;
-
- // Print copy results if available
- if (setupCopyResults) {
- const { copied, errors } = setupCopyResults;
- if (copied.length > 0) {
- session.xterm.writeln(
- `\r\n\x1b[32m✓ Copied ${copied.length} file(s):\x1b[0m`,
- );
- for (const file of copied) {
- session.xterm.writeln(` - ${file}`);
- }
- }
- if (errors.length > 0) {
- session.xterm.writeln(`\r\n\x1b[33m⚠ Copy warnings:\x1b[0m`);
- for (const error of errors) {
- session.xterm.writeln(` ${error}`);
- }
- }
- session.xterm.writeln("\r\n");
- }
-
- // Send all commands sequentially
- const commands = `${setupCommands.join("\n")}\n`;
- session.write(commands);
-
- // Mark setup as no longer pending
- useTabsStore.setState((state) => ({
- tabs: state.tabs.map((t) =>
- t.id === tabId ? { ...t, setupPending: false } : t,
- ),
- }));
+ try {
+ setupExecutedRef.current = true;
+
+ // Print copy results if available
+ if (setupCopyResults) {
+ const { copied, errors } = setupCopyResults;
+ if (copied.length > 0) {
+ session.xterm.writeln(
+ `\r\n\x1b[32m✓ Copied ${copied.length} file(s):\x1b[0m`,
+ );
+ for (const file of copied) {
+ session.xterm.writeln(` - ${file}`);
+ }
+ }
+ if (errors.length > 0) {
+ session.xterm.writeln(`\r\n\x1b[33m⚠ Copy warnings:\x1b[0m`);
+ for (const error of errors) {
+ session.xterm.writeln(` ${error}`);
+ }
+ }
+ session.xterm.writeln("\r\n");
+ }
+
+ // Send all commands sequentially
+ const commands = `${setupCommands.join("\n")}\n`;
+ session.write(commands);
+
+ // Mark setup as no longer pending
+ useTabsStore.setState((state) => ({
+ tabs: state.tabs.map((t) =>
+ t.id === tabId ? { ...t, setupPending: false } : t,
+ ),
+ }));
+ } catch (error) {
+ console.error("Failed to execute terminal setup:", error);
+ setupExecutedRef.current = false; // Allow retry
+ // Optionally: display error to user via terminal or notification
+ }
};📝 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 executeSetup = (session: TerminalSession) => { | |
| // Only execute setup once and only if setup is pending | |
| if (!setupPending || !setupCommands || setupExecutedRef.current) { | |
| return; | |
| } | |
| setupExecutedRef.current = true; | |
| // Print copy results if available | |
| if (setupCopyResults) { | |
| const { copied, errors } = setupCopyResults; | |
| if (copied.length > 0) { | |
| session.xterm.writeln( | |
| `\r\n\x1b[32m✓ Copied ${copied.length} file(s):\x1b[0m`, | |
| ); | |
| for (const file of copied) { | |
| session.xterm.writeln(` - ${file}`); | |
| } | |
| } | |
| if (errors.length > 0) { | |
| session.xterm.writeln(`\r\n\x1b[33m⚠ Copy warnings:\x1b[0m`); | |
| for (const error of errors) { | |
| session.xterm.writeln(` ${error}`); | |
| } | |
| } | |
| session.xterm.writeln("\r\n"); | |
| } | |
| // Send all commands sequentially | |
| const commands = `${setupCommands.join("\n")}\n`; | |
| session.write(commands); | |
| // Mark setup as no longer pending | |
| useTabsStore.setState((state) => ({ | |
| tabs: state.tabs.map((t) => | |
| t.id === tabId ? { ...t, setupPending: false } : t, | |
| ), | |
| })); | |
| }; | |
| const executeSetup = (session: TerminalSession) => { | |
| // Only execute setup once and only if setup is pending | |
| if (!setupPending || !setupCommands || setupExecutedRef.current) { | |
| return; | |
| } | |
| try { | |
| setupExecutedRef.current = true; | |
| // Print copy results if available | |
| if (setupCopyResults) { | |
| const { copied, errors } = setupCopyResults; | |
| if (copied.length > 0) { | |
| session.xterm.writeln( | |
| `\r\n\x1b[32m✓ Copied ${copied.length} file(s):\x1b[0m`, | |
| ); | |
| for (const file of copied) { | |
| session.xterm.writeln(` - ${file}`); | |
| } | |
| } | |
| if (errors.length > 0) { | |
| session.xterm.writeln(`\r\n\x1b[33m⚠ Copy warnings:\x1b[0m`); | |
| for (const error of errors) { | |
| session.xterm.writeln(` ${error}`); | |
| } | |
| } | |
| session.xterm.writeln("\r\n"); | |
| } | |
| // Send all commands sequentially | |
| const commands = `${setupCommands.join("\n")}\n`; | |
| session.write(commands); | |
| // Mark setup as no longer pending | |
| useTabsStore.setState((state) => ({ | |
| tabs: state.tabs.map((t) => | |
| t.id === tabId ? { ...t, setupPending: false } : t, | |
| ), | |
| })); | |
| } catch (error) { | |
| console.error("Failed to execute terminal setup:", error); | |
| setupExecutedRef.current = false; // Allow retry | |
| // Optionally: display error to user via terminal or notification | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/useTerminalSetup.ts
around lines 29-67, wrap all interactions with session.xterm.writeln and
session.write in a try-catch so exceptions from terminal ops don't leave state
inconsistent; move setting setupExecutedRef.current = true until after the
terminal writes succeed, in the catch reset/setupExecutedRef.current = false and
log or surface the error, and ensure the useTabsStore update that clears
setupPending only runs after successful execution (i.e., inside the try after
writes) so failed writes allow retry.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
79-87: New tab/workspace metadata arguments are wired correctly into testsPassing
tabTitleandworkspaceNamethrough thesecreateOrAttachcall sites keeps the tests aligned with the updatedTerminalManagerAPI without changing existing behavior they assert on (spawn options, reuse semantics, resize, write). This looks correct and keeps the fixtures straightforward with stable literal values.If
TerminalManageris now expected to persist or expose these labels (e.g., viagetSessionmetadata or events), consider adding one focused test that asserts on that new behavior so the contract around these fields is locked in; otherwise, this change is sufficient for the current test intentions.Also applies to: 104-110, 115-120, 130-137, 139-146, 154-159, 181-186
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsx (1)
10-15: UnusedisDropZoneprop.The
isDropZoneprop is declared in the interface but never destructured or used in the component. Either remove it from the interface or implement the intended drop zone behavior.interface SetupTabViewProps { tab: SetupTab; - isDropZone: boolean; } -export function SetupTabView({ tab }: SetupTabViewProps) { +export function SetupTabView({ tab }: SetupTabViewProps) {apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx (2)
10-13: Consider reusing the existing type fromSetupTab.This interface duplicates
setupCopyResultstype already defined inSetupTab(seeapps/desktop/src/renderer/stores/tabs/types.ts). Consider importing or deriving from that type to maintain a single source of truth.+import type { SetupTab } from "renderer/stores"; + -interface SetupCopyResults { - copied: string[]; - errors: string[]; -} +type SetupCopyResults = NonNullable<SetupTab["setupCopyResults"]>;
143-153: Remove mutation objects from the dependency array.Including
createMutation,writeMutation,resizeMutation, anddetachMutationin the dependency array is non-standard. While tRPC mutation objects are referentially stable, the ESLint exhaustive-deps rule may have flagged them. ThesetupExecutedRefguard prevents re-execution anyway, so consider moving mutation calls outside the effect or using refs to avoid linter warnings while keeping intent clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.superset/setup.json(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(6 hunks)apps/desktop/src/main/windows/main.ts(1 hunks)apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/windows/main.ts
- apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Biome formatting, linting, and import organization at the root level for all code files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsxapps/desktop/src/main/lib/terminal-manager.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype; use type-safe approaches instead, unless necessary
Ensure TypeScript type checking passes across all packages using bun run typecheck
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsxapps/desktop/src/main/lib/terminal-manager.test.ts
apps/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
apps/**/*.{tsx,ts}: Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsxapps/desktop/src/main/lib/terminal-manager.test.ts
apps/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one component per file; do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx
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/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsxapps/desktop/src/main/lib/terminal-manager.test.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/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsxapps/desktop/src/main/lib/terminal-manager.test.ts
apps/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
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
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : Implement Electron IPC handlers in dedicated files under apps/desktop/src/main/lib/ (e.g., workspace-ipcs.ts, terminal-ipcs.ts)
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsxapps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.{tsx,ts} : Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsx
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsx (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
SetupTab(27-33)apps/desktop/src/renderer/stores/tabs/hooks.ts (3)
useSplitTabHorizontal(21-22)useSplitTabVertical(19-20)useRemoveTab(8-8)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabContentContextMenu.tsx (1)
TabContentContextMenu(17-40)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx (1)
SetupTerminal(26-160)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
createTerminalInstance(11-70)setupResizeHandlers(92-116)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
TerminalStreamEvent(15-17)
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTabView.tsx (1)
16-48: LGTM!The hooks usage, event handlers, and JSX structure are clean. The component correctly wires up split/close actions and passes the required props to SetupTerminal.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx (2)
136-142: Cleanup mutation may fire after unmount.Calling
detachMutation.mutate()in the cleanup function works but the mutation callback closures may reference stale state. This is generally fine for fire-and-forget detach operations, but be aware that anyonSuccess/onErrorhandlers would run after the component unmounts.
26-57: LGTM!The terminal initialization, stream handling, status message rendering, and JSX structure are well-implemented. The component correctly manages the xterm lifecycle and integrates cleanly with tRPC subscriptions.
Also applies to: 155-159
| "commands": [ | ||
| "bun i" | ||
| "echo 'Hello world'" | ||
| ] |
There was a problem hiding this comment.
Revert placeholder command — dependency installation is missing.
Line 6 replaces the meaningful "bun i" (dependency installation) command with "echo 'Hello world'" (a trivial placeholder). This breaks the workspace setup flow and prevents dependencies from being installed in new workspaces.
Revert to the original install command:
"commands": [
- "echo 'Hello world'"
+ "bun i"
]If this change was intentional and the setup flow no longer requires dependency installation, please clarify the updated setup strategy and ensure all downstream setup utilities (file copying, terminal execution) account for this change.
📝 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.
| "commands": [ | |
| "bun i" | |
| "echo 'Hello world'" | |
| ] | |
| "commands": [ | |
| "bun i" | |
| ] |
🤖 Prompt for AI Agents
In .superset/setup.json around lines 5 to 7, the commands array was changed to a
placeholder ("echo 'Hello world'") which removes the dependency installation
step; restore the original dependency install command (e.g., "bun i") in place
of the echo so the workspace setup installs required packages, or if the change
was intentional, update the setup.json to document the new flow and adjust any
downstream setup utilities to handle the absence of an install step.
| if (setupCommands && setupCommands.length > 0) { | ||
| xterm.writeln("\x1b[36mRunning setup commands...\x1b[0m\r\n"); | ||
| } | ||
|
|
||
| // Create terminal session and run commands | ||
| createMutation.mutate( | ||
| { | ||
| tabId, | ||
| workspaceId, | ||
| tabTitle: "Setup", | ||
| cols: xterm.cols, | ||
| rows: xterm.rows, | ||
| cwd: setupCwd, | ||
| }, | ||
| { | ||
| onSuccess: () => { | ||
| setSubscriptionEnabled(true); | ||
|
|
||
| // Send commands once | ||
| if (setupCommands && setupCommands.length > 0) { | ||
| const combinedCommands = `${setupCommands.join(" && ")} && exit\n`; | ||
| writeMutation.mutate({ tabId, data: combinedCommands }); | ||
| } | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Handle the case when setupCommands is empty or undefined.
When setupCommands is empty/undefined, a terminal session is created but no commands are sent and no exit is issued. The user sees "Setting up worktree..." but the terminal just hangs without completing. Consider either auto-exiting when there are no commands, or showing a completion message directly.
onSuccess: () => {
setSubscriptionEnabled(true);
// Send commands once
if (setupCommands && setupCommands.length > 0) {
const combinedCommands = `${setupCommands.join(" && ")} && exit\n`;
writeMutation.mutate({ tabId, data: combinedCommands });
+ } else {
+ // No commands to run, exit immediately
+ writeMutation.mutate({ tabId, data: "exit\n" });
}
},🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView/SetupTerminal.tsx
around lines 99–124, handle the case where setupCommands is empty/undefined by
sending an explicit completion or exit to the terminal after the session is
created: inside the createMutation.onSuccess block, if setupCommands is falsy or
has length 0, call the writeMutation to send either a completion message (e.g.
"Setup complete\n") followed by an exit ("\nexit\n" or "exit\n") or just send
"exit\n" so the terminal session closes; keep the existing behavior for when
setupCommands exist (combine commands + " && exit\n") and ensure
setSubscriptionEnabled(true) is still called before writing.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.