feat(desktop): support user-overridable setup/teardown scripts#1266
Conversation
📝 WalkthroughWalkthroughAdds project-scoped user override support for setup/teardown scripts and propagates Changes
Sequence Diagram(s)sequenceDiagram
participant Procedure as Workspace Procedure
participant SetupLoader as loadSetupConfig
participant Teardown as runTeardown
participant FS as Filesystem
Procedure->>SetupLoader: loadSetupConfig({ mainRepoPath, worktreePath?, projectName? })
alt projectName provided
SetupLoader->>FS: read ~/.superset/projects/{projectName}/config.json
FS-->>SetupLoader: config (valid) / missing / invalid
alt valid
SetupLoader-->>Procedure: return user override config
else not valid
SetupLoader->>FS: read <worktreePath>/.superset/config.json (if provided)
FS-->>SetupLoader: config or not
alt found
SetupLoader-->>Procedure: return worktree config
else
SetupLoader->>FS: read <mainRepoPath>/.superset/config.json
FS-->>SetupLoader: config or not
SetupLoader-->>Procedure: return main repo config or null
end
end
else no projectName
SetupLoader->>FS: read <worktreePath>/.superset/config.json (if provided)
FS-->>SetupLoader: config or not
alt found
SetupLoader-->>Procedure: return worktree config
else
SetupLoader->>FS: read <mainRepoPath>/.superset/config.json
FS-->>SetupLoader: config or not
SetupLoader-->>Procedure: return main repo config or null
end
end
Note right of Procedure: Teardown callers now pass options including projectName to runTeardown
Procedure->>Teardown: runTeardown({ mainRepoPath, worktreePath, workspaceName, projectName? })
Teardown->>SetupLoader: loadSetupConfig(...) (same resolution)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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/procedures/delete.ts (1)
205-210: 🛠️ Refactor suggestion | 🟠 MajorRefactor
runTeardownto use an object parameter instead of positional arguments.The function currently takes four positional parameters, violating the coding guideline for functions with 2+ parameters. More critically, the third parameter (
workspaceName) receives different semantic values at different call sites:
- Line 205–210: passes
workspace.name- Line 428–433: passes
worktree.branchThis confusion makes it easy to pass arguments in the wrong order. Refactor the function signature and both call sites to use an object parameter:
export async function runTeardown({ mainRepoPath, worktreePath, workspaceName, projectName, }: { mainRepoPath: string; worktreePath: string; workspaceName: string; projectName?: string; }): Promise<TeardownResult>Then update both call sites accordingly.
🧹 Nitpick comments (5)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)
12-17: Tests write to the real home directory — consider adding pre-test cleanup.
USER_CONFIG_DIRresolves to the actual~/.superset-dev/projects/test-project/(in dev). If a prior test run was interrupted beforeafterEach, stale files could cause false positives. Adding cleanup inbeforeEachas well would make tests more resilient.🛡️ Suggested fix
beforeEach(() => { mkdirSync(join(MAIN_REPO, ".superset"), { recursive: true }); + // Ensure no stale user override from a previous interrupted run + if (existsSync(USER_CONFIG_DIR)) { + rmSync(USER_CONFIG_DIR, { recursive: true, force: true }); + } });Also applies to: 24-32
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
14-20:runTeardownuses positional parameters — guideline recommends object parameters for 2+ args.With the addition of
projectName, this function now has 4 positional parameters. This makes call sites harder to read (e.g.,runTeardown(path1, path2, name, name2)— which string is which?). Consider switching to an object parameter in a follow-up.♻️ Suggested signature
-export async function runTeardown( - mainRepoPath: string, - worktreePath: string, - workspaceName: string, - projectName?: string, -): Promise<TeardownResult> { +export async function runTeardown({ + mainRepoPath, + worktreePath, + workspaceName, + projectName, +}: { + mainRepoPath: string; + worktreePath: string; + workspaceName: string; + projectName?: string; +}): Promise<TeardownResult> {As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments".
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.test.ts (1)
26-40: Same as setup.test.ts — consider addingUSER_CONFIG_DIRcleanup tobeforeEach.If a prior interrupted run left stale files in the real home directory, the "falls back to mainRepoPath" test (Line 197) could unexpectedly find a user override and fail.
🛡️ Suggested fix
beforeEach(() => { // Create test directories mkdirSync(join(MAIN_REPO, ".superset"), { recursive: true }); mkdirSync(WORKTREE, { recursive: true }); + // Ensure no stale user override from a previous interrupted run + if (existsSync(USER_CONFIG_DIR)) { + rmSync(USER_CONFIG_DIR, { recursive: true, force: true }); + } });apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
36-60: Validation of the parsed config is incomplete — array element types are not checked.
JSON.parse(content) as SetupConfigis an unchecked cast. The subsequent guards only verify thatsetup/teardownare arrays, but not that their elements are strings. A malformed config like{ "setup": [123, null] }would pass validation and likely cause obscure failures downstream when the values are used as shell commands.Since the project already uses Zod and the coding guidelines recommend Zod schemas at boundaries, consider validating with a small schema:
♻️ Suggested validation with Zod
+import { z } from "zod"; + +const setupConfigSchema = z.object({ + setup: z.array(z.string()).optional(), + teardown: z.array(z.string()).optional(), +}); + function readConfigFile(configPath: string): SetupConfig | null { if (!existsSync(configPath)) { return null; } try { const content = readFileSync(configPath, "utf-8"); - const parsed = JSON.parse(content) as SetupConfig; - - if (parsed.setup && !Array.isArray(parsed.setup)) { - throw new Error("'setup' field must be an array of strings"); - } - - if (parsed.teardown && !Array.isArray(parsed.teardown)) { - throw new Error("'teardown' field must be an array of strings"); - } - - return parsed; + const parsed = JSON.parse(content); + return setupConfigSchema.parse(parsed); } catch (error) { console.error( `[setup] Failed to read setup config at ${configPath}: ${error instanceof Error ? error.message : String(error)}`, ); return null; } }This also future-proofs the function if
SetupConfiggains new fields. As per coding guidelines, "Use Zod schemas for validating tRPC inputs and API route bodies at boundaries" and "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks."
54-58: Add[setup]prefix to the error log for consistency.Line 88 uses
[setup]as a log prefix, but this error log on Line 56 omits it. The coding guideline calls for consistent[domain/operation]prefixed logging.♻️ Suggested fix
} catch (error) { console.error( - `Failed to read setup config at ${configPath}: ${error instanceof Error ? error.message : String(error)}`, + `[setup] Failed to read setup config at ${configPath}: ${error instanceof Error ? error.message : String(error)}`, );
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
force-pushed
the
overwrite-setup-teardown
branch
from
a31a2df to
9733c3f
Compare
….superset/projects/ Allow per-project user overrides stored in ~/.superset/projects/<projectName>/config.json so users can customize setup/teardown behavior without modifying committed repo files.
saddlepaddle
force-pushed
the
overwrite-setup-teardown
branch
from
9733c3f to
cadde07
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts`:
- Around line 90-108: The current user-level override lookup accepts projectName
values like ".." and can escape the intended projects directory; before building
userConfigPath in the setup routine, validate projectName is a single safe path
segment: ensure it is non-empty, does not include path separators, equals
path.basename(projectName), and that path.normalize(projectName) does not start
with ".." or contain ".." segments (also explicitly reject "." and ".."); only
then call join(homedir(), SUPERSET_DIR_NAME, PROJECTS_DIR_NAME, projectName,
CONFIG_FILE_NAME) and readConfigFile(userConfigPath).
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
36-60:as SetupConfigcast bypasses type safety; array element types are not validated.
JSON.parse(content) as SetupConfigtrusts the file contents at the type level. The checks on lines 45–51 verifysetup/teardownare arrays, but don't confirm the elements are strings. A config like{ "setup": [123, null] }would pass validation and could cause runtime errors when the commands are later joined/spawned.Consider using Zod (already a project dependency) to parse and validate the config shape, or at minimum add an element-type check:
Proposed fix
- const parsed = JSON.parse(content) as SetupConfig; + const parsed = JSON.parse(content) as Record<string, unknown>; - if (parsed.setup && !Array.isArray(parsed.setup)) { + if (parsed.setup && (!Array.isArray(parsed.setup) || !parsed.setup.every((s): s is string => typeof s === "string"))) { throw new Error("'setup' field must be an array of strings"); } - if (parsed.teardown && !Array.isArray(parsed.teardown)) { + if (parsed.teardown && (!Array.isArray(parsed.teardown) || !parsed.teardown.every((s): s is string => typeof s === "string"))) { throw new Error("'teardown' field must be an array of strings"); } + + return parsed as SetupConfig;As per coding guidelines, "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks."
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)
12-17: Tests write to the real home directory — residual parent dirs are not cleaned up.
USER_CONFIG_DIRis computed fromhomedir(), so tests create~/.superset-dev/projects/test-project/on the real filesystem. TheafterEachremoves the leaftest-projectdirectory but leaves~/.superset-dev/projects/behind. This is likely acceptable since the app creates~/.superset-dev/in normal operation, but worth being aware of.If test isolation is a concern, consider making the base config directory injectable in
loadSetupConfig(e.g., via an optionalconfigHomeparam defaulting tohomedir()), which would let tests use a temp directory entirely.Also applies to: 29-32
| // 1. Check user-level override (~/.superset/projects/<projectName>/config.json) | ||
| if ( | ||
| projectName && | ||
| !projectName.includes("/") && | ||
| !projectName.includes("\\") | ||
| ) { | ||
| const userConfigPath = join( | ||
| homedir(), | ||
| SUPERSET_DIR_NAME, | ||
| PROJECTS_DIR_NAME, | ||
| projectName, | ||
| CONFIG_FILE_NAME, | ||
| ); | ||
| const config = readConfigFile(userConfigPath); | ||
| if (config) { | ||
| console.log(`[setup] Using user override config from ${userConfigPath}`); | ||
| return config; | ||
| } | ||
| } |
There was a problem hiding this comment.
Path traversal: projectName of ".." bypasses the slash check.
The guard on lines 92–94 rejects / and \, but ".." contains neither and would resolve join(homedir(), SUPERSET_DIR_NAME, PROJECTS_DIR_NAME, "..", CONFIG_FILE_NAME) to ~/.superset/config.json, escaping the projects/ directory. While impact is limited (reads only, within the user's home dir, and projectName comes from an internal database), it's still a validation gap.
Proposed fix
if (
projectName &&
!projectName.includes("/") &&
- !projectName.includes("\\")
+ !projectName.includes("\\") &&
+ projectName !== ".." &&
+ projectName !== "."
) {📝 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.
| // 1. Check user-level override (~/.superset/projects/<projectName>/config.json) | |
| if ( | |
| projectName && | |
| !projectName.includes("/") && | |
| !projectName.includes("\\") | |
| ) { | |
| const userConfigPath = join( | |
| homedir(), | |
| SUPERSET_DIR_NAME, | |
| PROJECTS_DIR_NAME, | |
| projectName, | |
| CONFIG_FILE_NAME, | |
| ); | |
| const config = readConfigFile(userConfigPath); | |
| if (config) { | |
| console.log(`[setup] Using user override config from ${userConfigPath}`); | |
| return config; | |
| } | |
| } | |
| // 1. Check user-level override (~/.superset/projects/<projectName>/config.json) | |
| if ( | |
| projectName && | |
| !projectName.includes("/") && | |
| !projectName.includes("\\") && | |
| projectName !== ".." && | |
| projectName !== "." | |
| ) { | |
| const userConfigPath = join( | |
| homedir(), | |
| SUPERSET_DIR_NAME, | |
| PROJECTS_DIR_NAME, | |
| projectName, | |
| CONFIG_FILE_NAME, | |
| ); | |
| const config = readConfigFile(userConfigPath); | |
| if (config) { | |
| console.log(`[setup] Using user override config from ${userConfigPath}`); | |
| return config; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts` around lines 90
- 108, The current user-level override lookup accepts projectName values like
".." and can escape the intended projects directory; before building
userConfigPath in the setup routine, validate projectName is a single safe path
segment: ensure it is non-empty, does not include path separators, equals
path.basename(projectName), and that path.normalize(projectName) does not start
with ".." or contain ".." segments (also explicitly reject "." and ".."); only
then call join(homedir(), SUPERSET_DIR_NAME, PROJECTS_DIR_NAME, projectName,
CONFIG_FILE_NAME) and readConfigFile(userConfigPath).
1 participant
Summary
~/.superset/projects/<projectName>/config.jsonChanges
PROJECTS_DIR_NAMEconstantreadConfigFilehelper fromreadConfigFromPathfor reuseprojectNameparam toloadSetupConfigandrunTeardownTest plan
bun test apps/desktop/src/lib/trpc/routers/workspaces/utils/)~/.superset-dev/projects/<project>/config.jsonwith custom commands, create workspace, verify custom commands usedSummary by CodeRabbit
New Features
Behavior
Documentation
Tests