fix(desktop): copy gitignored .superset/ to worktrees#549
fix(desktop): copy gitignored .superset/ to worktrees#549AviPeltz merged 2 commits intosuperset-sh:mainfrom
Conversation
📝 WalkthroughWalkthroughCopies the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
5-5: Consider using the new constant inloadSetupConfigfor consistency.The
loadSetupConfigfunction on line 32 hardcodes".superset"while a constantSUPERSET_CONFIG_DIRis now defined. Using the constant would improve maintainability.🔎 Proposed refactor
export function loadSetupConfig(mainRepoPath: string): SetupConfig | null { - const configPath = join(mainRepoPath, ".superset", "config.json"); + const configPath = join(mainRepoPath, SUPERSET_CONFIG_DIR, "config.json");Also applies to: 32-32
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/marketing/src/app/scripts/page.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/marketing/src/app/scripts/page.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
🧠 Learnings (1)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
copySupersetConfigToWorktree(12-29)
⏰ 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 (3)
apps/marketing/src/app/scripts/page.tsx (1)
185-186: Documentation accurately reflects the new behavior.The updated tip clearly communicates that
.superset/will be automatically copied to new worktrees, helping users understand that their gitignored personal configs will work seamlessly.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
127-129: LGTM! Well-placed and clearly documented.The function call is correctly positioned after the worktree is created on disk and before setup scripts run, ensuring gitignored
.superset/directories are available when needed.apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
7-29: LGTM! Solid implementation with appropriate error handling.The function correctly implements the copy logic with proper existence checks and best-effort error handling. The approach of only copying when the destination is missing ensures it won't overwrite any manual changes in worktrees.
When .superset/ is gitignored, it doesn't exist in new worktrees since git only includes tracked files. This caused setup scripts like './.superset/setup.sh' to fail with 'no such file or directory'. Now .superset/ is automatically copied from the main repo to worktrees if it exists in main but not in the worktree.
andreasasprou
force-pushed
the
fix-gitignored-superset-config
branch
from
73e498e to
a7a8ca7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
11-28: Consider returning success status or throwing errors for better error propagation.The function currently logs errors but returns
void, preventing the caller from detecting copy failures. If the copy fails, the worktree will be created but setup scripts will be missing, yet the UI may not reflect this issue.Consider one of these approaches:
- Return
booleanto indicate success/failure- Throw errors and let the caller decide whether to fail worktree creation or show a warning
🔎 Option 1: Return boolean for success status
export function copySupersetConfigToWorktree( mainRepoPath: string, worktreePath: string, -): void { +): boolean { const mainSupersetDir = join(mainRepoPath, PROJECT_SUPERSET_DIR_NAME); const worktreeSupersetDir = join(worktreePath, PROJECT_SUPERSET_DIR_NAME); // Only copy if it exists in main repo but not in worktree if (existsSync(mainSupersetDir) && !existsSync(worktreeSupersetDir)) { try { cpSync(mainSupersetDir, worktreeSupersetDir, { recursive: true }); + return true; } catch (error) { console.error( `Failed to copy ${PROJECT_SUPERSET_DIR_NAME} to worktree: ${error instanceof Error ? error.message : String(error)}`, ); + return false; } } + return true; // Nothing to copy or already exists }
11-11: Optional: Consider renaming to reflect that entire directory is copied.The function name
copySupersetConfigToWorktreesuggests only the config file is copied, but it copies the entire.supersetdirectory (including setup scripts and other files). Consider a more descriptive name likecopySupersetDirToWorktreefor clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/marketing/src/app/scripts/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
- apps/marketing/src/app/scripts/page.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🧠 Learnings (1)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
apps/desktop/src/shared/constants.ts (2)
PROJECT_SUPERSET_DIR_NAME(33-33)CONFIG_FILE_NAME(35-35)apps/desktop/src/shared/types/config.ts (1)
SetupConfig(1-4)
⏰ 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 (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
31-35: LGTM! Good refactoring to use shared constants.Using
PROJECT_SUPERSET_DIR_NAMEandCONFIG_FILE_NAMEconstants instead of hardcoded strings improves maintainability and ensures consistency across the codebase.
1-1: Node.js version is compatible withcpSync.Electron 39.1.2 bundles Node.js v22.21.1, which fully supports
cpSync(available since Node.js v16.7.0). No compatibility concerns.
There was a problem hiding this comment.
Wait actually on second thought it might be better to just remove the .gitignore comment from the docs because .superset probably shouldn't be .gitignored. If you make a change in a worktree to your setup/teardown script and gitignore .superset it, the changes wont be pushed to main which seems like confusing behavior. Let me know your thoughts, I could be wrong here.
but we could also keep this and then just add a change getting rid of the comment in the docs, so we have the best of both worlds?
Replace advice to gitignore .superset/ with guidance to commit it. The gitignore advice caused the original issue - gitignored files don't exist in worktrees. Now we recommend committing the config and using environment variables for machine-specific values. The copySupersetConfigToWorktree fallback remains as a safety net. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
AviPeltz
left a comment
AviPeltz
left a comment
There was a problem hiding this comment.
i added a doc change and i think its good to merge now
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/marketing/src/app/scripts/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/marketing/src/app/scripts/page.tsx
⏰ 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
| Commit{" "} | ||
| <code className="text-amber-500 dark:text-amber-400"> | ||
| .superset/ | ||
| </code>{" "} | ||
| to your{" "} | ||
| <code className="text-amber-500 dark:text-amber-400"> | ||
| .gitignore | ||
| </code>{" "} | ||
| if you don't want to share configs | ||
| to share workspace setup with your team | ||
| </li> | ||
| <li> | ||
| For machine-specific values, use environment variables in your | ||
| scripts instead of hardcoding paths | ||
| </li> |
There was a problem hiding this comment.
Document the automatic .superset/ copying behavior.
The tips recommend committing .superset/ but don't mention the automatic copying behavior that this PR introduces. According to the PR objectives, the documentation should clarify that users can either commit .superset/ or keep it gitignored and Superset will automatically copy it to new worktrees.
Consider adding a tip that explains this fallback mechanism so users who prefer to gitignore .superset/ understand it will still work in worktrees.
📝 Suggested documentation addition
<li>
Commit{" "}
<code className="text-amber-500 dark:text-amber-400">
.superset/
</code>{" "}
to share workspace setup with your team
</li>
+ <li>
+ If{" "}
+ <code className="text-amber-500 dark:text-amber-400">
+ .superset/
+ </code>{" "}
+ is gitignored, Superset will automatically copy it from your main
+ repository to new worktrees
+ </li>
<li>
For machine-specific values, use environment variables in your
scripts instead of hardcoding paths
</li>📝 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.
| Commit{" "} | |
| <code className="text-amber-500 dark:text-amber-400"> | |
| .superset/ | |
| </code>{" "} | |
| to your{" "} | |
| <code className="text-amber-500 dark:text-amber-400"> | |
| .gitignore | |
| </code>{" "} | |
| if you don't want to share configs | |
| to share workspace setup with your team | |
| </li> | |
| <li> | |
| For machine-specific values, use environment variables in your | |
| scripts instead of hardcoding paths | |
| </li> | |
| Commit{" "} | |
| <code className="text-amber-500 dark:text-amber-400"> | |
| .superset/ | |
| </code>{" "} | |
| to share workspace setup with your team | |
| </li> | |
| <li> | |
| If{" "} | |
| <code className="text-amber-500 dark:text-amber-400"> | |
| .superset/ | |
| </code>{" "} | |
| is gitignored, Superset will automatically copy it from your main | |
| repository to new worktrees | |
| </li> | |
| <li> | |
| For machine-specific values, use environment variables in your | |
| scripts instead of hardcoding paths | |
| </li> |
🤖 Prompt for AI Agents
In apps/marketing/src/app/scripts/page.tsx around lines 177 to 186, add a new
list item documenting the automatic .superset/ copying behavior introduced by
this PR: state that users can either commit .superset/ to the repo or keep it
gitignored — Superset will automatically copy a .superset/ directory into new
worktrees as a fallback so workspaces still get the default config; insert this
as an additional <li> immediately after the existing items and keep the note
concise and mention machine-specific values should still use env vars.
2 participants
Summary
Fixes the bug where gitignored
.superset/directories don't exist in new worktrees, causing setup scripts to fail.Before: Users following the docs tip to add
.superset/to.gitignorewould getzsh: no such file or directory: ./.superset/setup.rbwhen creating workspaces.After: Gitignored
.superset/is automatically copied from main repo to new worktrees.Details
The Problem
The scripts documentation suggested adding
.superset/to.gitignorefor personal configs. However, git worktrees only contain tracked files, so gitignored directories don't exist in worktrees. This caused setup commands like./.superset/setup.shto fail.The Fix
Added
copySupersetConfigToWorktree()that runs after worktree creation:.superset/exists in main repo.superset/exists in worktree (it won't if gitignored)Also updated the docs tip to clarify this behavior:
Test Plan
.superset/config.jsoncontaining{"setup": ["./.superset/setup.sh"]}.superset/setup.shwith some test command (e.g.,echo "setup ran").superset/to.gitignoreFiles Changed
Modified
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts- AddedcopySupersetConfigToWorktree()functionapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts- Call copy function after worktree creationapps/marketing/src/app/scripts/page.tsx- Updated docs tip to explain auto-copy behaviorSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.