Skip to content

Adjust workspace setup logic to match conductor flow#167

Merged
saddlepaddle merged 2 commits intomainfrom
teal-thunder-99
Nov 28, 2025
Merged

Adjust workspace setup logic to match conductor flow#167
saddlepaddle merged 2 commits intomainfrom
teal-thunder-99

Conversation

@saddlepaddle
Copy link
Copy Markdown
Collaborator

@saddlepaddle saddlepaddle commented Nov 28, 2025

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes

Summary by CodeRabbit

  • Refactor

    • Setup no longer has a dedicated setup UI or setup-specific tab type; setup now runs in standard terminal tabs and related setup-only UI elements removed.
    • Setup configs no longer include file-copy results or copy-related fields.
  • New Features

    • Terminals accept and automatically run initial commands on creation.
    • Terminal environment exposes workspace/root path for setup commands.
  • Chores

    • Updated project setup script and improved .env handling and workspace naming.

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 28, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
website Ready Ready Preview Comment Nov 28, 2025 7:48pm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 28, 2025

Walkthrough

Removes setup file-copy functionality and UI, narrows setup config to commands only, and routes workspace setup commands into terminal creation flows by adding initialCommands and rootPath wiring across TRPC, TerminalManager, renderer stores, and UI.

Changes

Cohort / File(s) Summary
Setup loader & tests
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts, apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
Deleted copy-related logic and exports (copySetupFiles), removed fast-glob / fs copy imports, and tuned validation to accept only commands arrays; tests for copy behavior removed.
Shared types & IPC
apps/desktop/src/shared/types.ts, apps/desktop/src/shared/ipc-channels/worktree.ts
Removed copy?: string[] from SetupConfig and deleted SetupResult interface; removed optional setupResult from worktree-create IPC response.
Terminal TRPC & manager
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts, apps/desktop/src/main/lib/terminal-manager.ts
Added optional initialCommands: string[] and rootPath propagation to createOrAttach; TerminalManager.createOrAttach accepts rootPath and injects SUPERSET_ROOT_PATH and SUPERSET_WORKSPACE_PATH into spawned env; initialCommands are written to new terminals.
Workspace creation & renderer flow
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts, apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
Removed setup file-copying from workspace creation; workspace create now returns initialCommands (from setup config) and worktreePath; renderer now creates a terminal tab and calls terminal.createOrAttach with initialCommands instead of creating SetupTab.
Tab types, helpers & store
apps/desktop/src/renderer/stores/tabs/types.ts, apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts, apps/desktop/src/renderer/stores/tabs/store.ts
Removed Setup tab type and SetupTab interface and addSetupTab; handleAddTab now returns { newState, tabId }; addTab now returns the new tabId string.
Removed Setup UI & rendering
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx, .../SetupTerminal/SetupTerminal.tsx, .../SetupTerminal/index.ts, apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx, apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
Deleted SetupTabView and SetupTerminal components, removed Setup-related rendering and spinner/UI logic, and cleaned up related exports.
Setup scripts & config files
.superset/setup.json, conductor.json, superset-setup.sh
.superset/setup.json copy pattern removed and command changed to ./superset-setup.sh; conductor.json deleted; superset-setup.sh updated to conditionally link .envrc from SUPERSET_ROOT_PATH if present and derive workspace name from SUPERSET_WORKSPACE_NAME or directory basename.

Sequence Diagram(s)

sequenceDiagram
  participant Renderer as Renderer (UI)
  participant TRPC as TRPC Router (terminal)
  participant Main as Main Process (TerminalManager)
  participant Shell as Spawned Shell

  Renderer->>TRPC: createOrAttach(workspaceId, initialCommands, rootPath, cwd, cols, rows)
  TRPC->>Main: forward createOrAttach params (includes rootPath)
  Main->>Main: set env SUPERSET_ROOT_PATH = rootPath || ""
  Main->>Main: set env SUPERSET_WORKSPACE_PATH = workingDir
  Main->>Shell: spawn shell with env and pty settings
  Main-->>TRPC: return terminal/session info
  TRPC-->>Renderer: respond with attachment info
  alt initialCommands provided and terminal is new
    Renderer->>TRPC: request write of concatenated initialCommands
    TRPC->>Main: write data to terminal session
    Main->>Shell: shell receives commands and executes
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Pay attention to API/typing changes where Setup types and setupResult were removed — ensure no residual usages remain.
  • Verify all callers of handleAddTab / addTab updated for the new return shape and string tabId return.
  • Confirm rootPath propagation and environment variables (SUPERSET_ROOT_PATH, SUPERSET_WORKSPACE_PATH) are set and consumed as expected.
  • Review removed filesystem copy logic to ensure no remaining assumptions about copy results.

Possibly related PRs

Poem

🐰
I nibbled copies, left the crumbs behind,
Now commands hop in, one line at a time.
Tabs trimmed to order, shells ready to run,
A carrot for devs — setup by terminal fun! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is entirely empty—only template headings are present with no actual content, details, or context provided for any of the required sections. Fill in all required sections: provide a clear description of changes, link related issues, specify the type of change, describe testing performed, and add any relevant context or notes.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Adjust workspace setup logic to match conductor flow' clearly summarizes the main change: refactoring workspace setup logic to align with a conductor-based workflow pattern.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch teal-thunder-99

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe0981e and b19c98f.

📒 Files selected for processing (5)
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2 hunks)
  • apps/desktop/src/main/lib/terminal-manager.ts (3 hunks)
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1 hunks)
  • superset-setup.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/main/lib/terminal-manager.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • superset-setup.sh
⏰ 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). (2)
  • GitHub Check: Build - macOS (arm64)
  • GitHub Check: Build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
superset-setup.sh (2)

23-28: Document the new environment variables required by the setup script.

Lines 23-28 introduce conditional direnv linking based on SUPERSET_MAIN_REPO_PATH. This is a new environment variable that developers need to know about. Similarly, line 32 uses SUPERSET_WORKSPACE_NAME. These should be documented in a README or setup guide so users understand when and how to set them.

Consider adding comments in the script or creating/updating documentation that explains:

  • SUPERSET_MAIN_REPO_PATH: When and why to set it (optional, for direnv config from main repo)
  • SUPERSET_WORKSPACE_NAME: When and why to set it (optional, defaults to current directory basename)

Also verify these variables are documented in the project's setup or contributing guide.


39-40: Verify robustness of Neon connection string parsing.

Lines 39-40 parse connection URIs by index ([0] and [1]). This assumes the Neon API response structure is stable and returns URIs in this specific order. Consider adding error handling in case the response structure differs or indices are out of bounds.

apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)

16-18: Consider validating array element types.

The validation checks if commands is an array, but doesn't verify that all elements are strings. Invalid element types would only fail when the commands are executed later.

Apply this diff to add element type validation:

 if (parsed.commands && !Array.isArray(parsed.commands)) {
 	throw new Error("'commands' field must be an array of strings");
 }
+
+if (parsed.commands && !parsed.commands.every((cmd) => typeof cmd === "string")) {
+	throw new Error("'commands' field must contain only strings");
+}
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)

72-76: Consider command error handling behavior.

Commands are joined with &&, which means execution stops at the first failure. While this is often desired, it may not be clear to users when setup commands fail silently. Consider whether you want to:

  • Keep && for fail-fast behavior (current)
  • Use ; to continue on errors
  • Add error notification when commands fail

If fail-fast is intended, the current implementation is correct. Otherwise, consider using ; separator:

-const commandString = `${initialCommands.join(" && ")}\n`;
+const commandString = `${initialCommands.join("; ")}\n`;
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)

22-33: Consider adding error handling for terminal creation.

The createOrAttach mutation could fail, but there's no error handling. If terminal creation fails, the tab would exist but remain disconnected. Consider adding error handling to either:

  • Remove the tab on terminal creation failure
  • Show a user-facing error notification
  • Log the error for debugging

Add error handling to the mutation:

 createOrAttach.mutate({
 	tabId,
 	workspaceId: data.workspace.id,
 	tabTitle: "Terminal",
 	initialCommands: data.setupConfig,
+}, {
+	onError: (error) => {
+		console.error("Failed to create terminal for setup:", error);
+		// Consider: removeTab(tabId) or show user notification
+	}
 });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09274bb and c36ceb5.

📒 Files selected for processing (19)
  • .superset/setup.json (1 hunks)
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (2 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1 hunks)
  • apps/desktop/src/main/lib/terminal-manager.ts (3 hunks)
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/SetupTerminal.tsx (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/index.ts (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (0 hunks)
  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (2 hunks)
  • apps/desktop/src/renderer/stores/tabs/store.ts (1 hunks)
  • apps/desktop/src/renderer/stores/tabs/types.ts (2 hunks)
  • apps/desktop/src/shared/ipc-channels/worktree.ts (0 hunks)
  • apps/desktop/src/shared/types.ts (0 hunks)
  • conductor.json (0 hunks)
  • superset-setup.sh (1 hunks)
💤 Files with no reviewable changes (8)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx
  • apps/desktop/src/shared/ipc-channels/worktree.ts
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/SetupTerminal.tsx
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
  • conductor.json
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/index.ts
  • apps/desktop/src/shared/types.ts
🧰 Additional context used
📓 Path-based instructions (7)
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/main/lib/terminal-manager.ts
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
  • apps/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 in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary

Files:

  • apps/desktop/src/main/lib/terminal-manager.ts
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
  • apps/desktop/src/renderer/stores/tabs/store.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Avoid using any type - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code

Files:

  • apps/desktop/src/main/lib/terminal-manager.ts
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
  • apps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/main/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Node.js modules (fs, path, os, net, etc.) can be used in main process code only

Files:

  • apps/desktop/src/main/lib/terminal-manager.ts
apps/desktop/src/lib/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Never import Node.js modules in shared code like src/lib/electron-router-dom.ts - this code runs in both main and renderer processes

Files:

  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
apps/desktop/src/renderer/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only

Files:

  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/renderer/stores/tabs/store.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/lib/trpc/routers/workspaces/utils/setup.test.ts
🧠 Learnings (5)
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications

Applied to files:

  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.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/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible

Applied to files:

  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/main/**/*.{ts,tsx} : Node.js modules (fs, path, os, net, etc.) can be used in main process code only

Applied to files:

  • apps/desktop/src/lib/trpc/routers/workspaces/utils/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 independent

Applied to files:

  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.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 repeatable

Applied to files:

  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
🧬 Code graph analysis (5)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (2)
apps/desktop/src/main/lib/db/index.ts (1)
  • db (18-25)
apps/desktop/src/main/lib/terminal-manager.ts (1)
  • terminalManager (394-394)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
  • TabsState (28-32)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/types.ts (2)
  • Tab (44-56)
  • TabType (23-30)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
  • createOrAttach (43-189)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
  • handleAddTab (6-32)
⏰ 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 (10)
superset-setup.sh (1)

32-36: Verify hardcoded Neon project ID and fallback behavior.

Line 32 uses SUPERSET_WORKSPACE_NAME with a sensible fallback to basename "$PWD", which is good. However, line 34 contains a hardcoded Neon project ID (tiny-cherry-82420694).

Verify:

  1. This project ID is correct and appropriate for the shared workspace setup flow.
  2. Whether this should be made configurable (e.g., via environment variable or .superset/setup.json) for different deployment scenarios.
.superset/setup.json (2)

1-5: Verify script path and execution context.

The change from "bun i" to "./superset-setup.sh" shifts setup responsibility to the dedicated script. However, ensure:

  1. The relative path ./superset-setup.sh is correct for the execution context (is the working directory the repo root or the .superset/ directory?).
  2. The script is marked as executable (chmod +x superset-setup.sh) in the repository.
  3. The conductor/workspace utils that load this config properly handle the working directory.

2-4: Confirm alignment with removal of copy functionality.

The AI summary notes that copy patterns (e.g., "**/.env*") were removed from this configuration. The current state shows only the commands section. Verify that:

  1. All file-copying setup logic has been removed from the codebase and is no longer expected by the workspace/terminal initialization flow.
  2. No downstream code still references copy-related setup configuration that was removed.
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)

1-57: LGTM!

The test refactoring correctly reflects the simplified setup logic. Tests remain independent, readable, and appropriately cover the new behavior focusing on commands validation only.

apps/desktop/src/renderer/stores/tabs/store.ts (1)

43-51: LGTM with note on closure pattern.

The implementation correctly uses a closure to capture the tabId from the state update. This pattern works with Zustand since set() is called synchronously, ensuring tabId is assigned before the function returns.

apps/desktop/src/main/lib/terminal-manager.ts (1)

48-48: LGTM!

The mainRepoPath parameter addition is clean and follows existing patterns. The environment variable is properly set with a safe fallback, maintaining backward compatibility.

Also applies to: 57-66, 97-97

apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)

94-100: LGTM!

The simplified return structure correctly removes setup copying logic and returns only the essential data. This aligns with the PR's goal to shift setup handling to terminal-based operations.

apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)

6-32: LGTM!

The refactored return structure properly exposes the tabId while maintaining the state update pattern. This change enables the store to return the newly created tab ID to callers, which is essential for the new terminal-based setup flow.

apps/desktop/src/renderer/stores/tabs/types.ts (2)

21-27: Tab union simplification aligns with new workspace flow

Restricting Tab to SingleTab | TabGroup cleanly removes setup‑specific tab concepts and keeps the tab model focused on the remaining types. This is consistent with the conductor‑style flow and shared TabType usage.


34-36: Perfect! I've completed the verification. Let me confirm the findings:

Verification Results:

Implementation returns valid string: The addTab function initializes tabId as an empty string (line 44) and assigns it from result.tabId (line 47), then returns it (line 50). The return type is guaranteed to be a string.

Type signature is correct: The interface declaration addTab: (workspaceId: string, type?: TabType) => string accurately matches the implementation.

All call sites are type-correct:

  • WorkspaceView/index.tsx (line 40): addTab(activeWorkspaceId) - ignores return value ✓
  • TabsView/index.tsx (line 31): addTab(activeWorkspaceId) - ignores return value ✓
  • useCreateWorkspace.ts (line 24): const tabId = addTab(data.workspace.id) - captures and uses the return value as intended ✓

Return type change from void to string is correctly implemented and verified across all usage sites. No breaking changes detected.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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/lib/trpc/routers/workspaces/utils/setup.ts (1)

16-18: Enhance validation and fix misleading error message.

The error message claims to validate an "array of strings," but the code only checks Array.isArray() without validating element types. Since JSON.parse returns untyped data, adding element validation prevents downstream runtime errors.

Apply this diff to add proper validation:

 if (parsed.commands && !Array.isArray(parsed.commands)) {
-  throw new Error("'commands' field must be an array of strings");
+  throw new Error("'commands' field must be an array");
 }
+
+if (parsed.commands && !parsed.commands.every(cmd => typeof cmd === 'string')) {
+  throw new Error("'commands' field must be an array of strings");
+}
🧹 Nitpick comments (2)
superset-setup.sh (1)

32-32: Consider validating the workspace name early.

The workspace name is derived from SUPERSET_WORKSPACE_NAME environment variable (or falls back to directory basename) and passed directly to Neon without validation. Neon branch names have naming constraints (e.g., length limits, allowed characters). If an invalid name is passed, the script will fail at the neonctl call with an unclear error message.

Add early validation to improve user experience and provide clear feedback:

 # Create Neon branch for this workspace
 echo "🗄️  Creating Neon branch..."
 WORKSPACE_NAME="${SUPERSET_WORKSPACE_NAME:-$(basename "$PWD")}"
+
+# Validate workspace name (Neon has naming constraints)
+if ! [[ "$WORKSPACE_NAME" =~ ^[a-z0-9_-]+$ ]] || [ ${#WORKSPACE_NAME} -gt 63 ]; then
+  error "Invalid workspace name: '$WORKSPACE_NAME'. Use lowercase alphanumeric, hyphens, underscores. Max 63 chars."
+fi
+
 NEON_OUTPUT=$(neonctl branches create \
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)

27-32: Consider adding error handling for terminal creation.

The createOrAttach.mutate call is fire-and-forget. If terminal creation fails, the user won't be notified and the setup commands won't execute. Consider adding error handling:

 createOrAttach.mutate({
 	tabId,
 	workspaceId: data.workspace.id,
 	tabTitle: "Terminal",
 	initialCommands: data.setupConfig,
+}, {
+	onError: (error) => {
+		console.error("Failed to create terminal for setup commands:", error);
+	},
 });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c36ceb5 and fe0981e.

📒 Files selected for processing (19)
  • .superset/setup.json (1 hunks)
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (2 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1 hunks)
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1 hunks)
  • apps/desktop/src/main/lib/terminal-manager.ts (3 hunks)
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/SetupTerminal.tsx (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/index.ts (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (0 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (0 hunks)
  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (2 hunks)
  • apps/desktop/src/renderer/stores/tabs/store.ts (1 hunks)
  • apps/desktop/src/renderer/stores/tabs/types.ts (2 hunks)
  • apps/desktop/src/shared/ipc-channels/worktree.ts (0 hunks)
  • apps/desktop/src/shared/types.ts (0 hunks)
  • conductor.json (0 hunks)
  • superset-setup.sh (1 hunks)
💤 Files with no reviewable changes (8)
  • conductor.json
  • apps/desktop/src/shared/types.ts
  • apps/desktop/src/shared/ipc-channels/worktree.ts
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/index.ts
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTabView.tsx
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SetupTerminal/SetupTerminal.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
  • apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
  • apps/desktop/src/main/lib/terminal-manager.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
  • .superset/setup.json
🧰 Additional context used
📓 Path-based instructions (5)
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.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/renderer/stores/tabs/store.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: Please use alias as defined in tsconfig.json when 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.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/renderer/stores/tabs/store.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Avoid using any type - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code

Files:

  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/renderer/stores/tabs/store.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
apps/desktop/src/renderer/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only

Files:

  • apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts
  • apps/desktop/src/renderer/stores/tabs/store.ts
  • apps/desktop/src/renderer/stores/tabs/types.ts
apps/desktop/src/lib/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Never import Node.js modules in shared code like src/lib/electron-router-dom.ts - this code runs in both main and renderer processes

Files:

  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🧠 Learnings (1)
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications

Applied to files:

  • apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🧬 Code graph analysis (4)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
  • TabsState (28-32)
apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
  • createOrAttach (43-189)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
  • handleAddTab (6-32)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/types.ts (2)
  • Tab (44-56)
  • TabType (23-30)
⏰ 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). (2)
  • GitHub Check: Build - macOS (arm64)
  • GitHub Check: Build
🔇 Additional comments (7)
superset-setup.sh (1)

23-28: Conditional direnv linking looks sound.

The logic defensively checks both environment variable presence and file existence before creating the symlink. The use of ln -sf safely overwrites any existing symlink.

apps/desktop/src/renderer/stores/tabs/store.ts (1)

43-51: LGTM - addTab now correctly returns the new tab ID.

The implementation correctly captures the tabId from the handleAddTab result and returns it. Zustand's set() is synchronous, so the pattern of mutating tabId inside the callback and returning it after is safe and works as expected.

apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)

6-32: LGTM - Clean refactor of handleAddTab return type.

The function now correctly returns both the new state and the tab ID in a structured object. The implementation maintains the existing history stack logic while enabling callers to obtain the newly created tab's ID. Type annotation is explicit and accurate.

apps/desktop/src/renderer/stores/tabs/types.ts (1)

26-35: LGTM - Type definitions cleaned up appropriately.

The Tab union type correctly reflects the removal of SetupTab, and addTab return type is updated to string to match the implementation. The store's TabType enum (Single, Group) serves a different purpose than the shared TabType string union—the former describes tab structure while the latter describes content type.

apps/desktop/src/renderer/react-query/workspaces/useCreateWorkspace.ts (1)

27-32: <function_calls>
<invoke name="shell
<invoke_arguments>
#!/bin/bash

Get the full terminal router file

cat -n apps/desktop/src/lib/trpc/routers/terminal/terminal.ts

</invoke_arguments>
</function_calls>

apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)

1-2: LGTM! Clean removal of unused imports.

The import cleanup appropriately removes file-copy related dependencies (copyFile, mkdir, dirname, fast-glob) while retaining only what's needed for config loading.


5-27: SetupConfig type is correctly aligned with the narrowed function behavior.

Verification confirms the SetupConfig interface in apps/desktop/src/shared/types.ts (line 130-132) only defines a commands?: string[] field. The copy field does not exist in the type, and the function's validation logic correctly mirrors this—it only validates the commands field when present. No type mismatch exists.

- SUPERSET_MAIN_REPO_PATH → SUPERSET_ROOT_PATH
- Add SUPERSET_WORKSPACE_PATH for worktree path
- Rename setupConfig → initialCommands in workspace create response

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@saddlepaddle saddlepaddle merged commit f2c425b into main Nov 28, 2025
9 checks passed
@Kitenite Kitenite deleted the teal-thunder-99 branch November 29, 2025 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant