refactor(desktop): enhance loadSetupConfig to support worktree paths#1252
refactor(desktop): enhance loadSetupConfig to support worktree paths#1252Kitenite merged 2 commits intosuperset-sh:mainfrom
Conversation
📝 WalkthroughWalkthroughRefactors setup configuration loading to be worktree-aware: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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: 1
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)
30-56: 🛠️ Refactor suggestion | 🟠 Major
readConfigFromPathandloadSetupConfigare duplicated inteardown.tswith inconsistent constant usage.Both
setup.ts(lines 30–71) andteardown.ts(lines 16–53) implement nearly identicalreadConfigFromPathandloadSetupConfigfunctions. The only functional difference is field validation (setupvsteardown). Additionally,teardown.tshardcodes the path strings".superset"and"config.json"instead of importing and using the shared constantsPROJECT_SUPERSET_DIR_NAMEandCONFIG_FILE_NAMEfromshared/constants(whichsetup.tscorrectly uses).Extract these functions into a shared utility module that accepts the field name to validate, or export
readConfigFromPathfromsetup.tsand reuse it inteardown.ts. Either way, ensureteardown.tsimports and uses the shared constants rather than hardcoding values.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts`:
- Around line 16-17: Replace the hardcoded ".superset" string in
readConfigFromPath with the PROJECT_SUPERSET_DIR_NAME constant: import {
PROJECT_SUPERSET_DIR_NAME } from "shared/constants" at the top of the file and
change the join call in readConfigFromPath to use PROJECT_SUPERSET_DIR_NAME
(i.e., join(basePath, PROJECT_SUPERSET_DIR_NAME, "config.json")) so it matches
setup.ts and avoids string drift.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (1)
43-49: Unsafeas SetupConfigcast on parsed JSON.
JSON.parse(content) as SetupConfigtrusts the file content entirely. The validation on line 45 only checksparsed.setupis an array (when present), but doesn't verify element types are strings or that unexpected fields don't cause downstream issues. Since coding guidelines recommend validating untrusted data with Zod, consider using a Zod schema here — you already depend onzodin the project.♻️ Suggested approach
+import { z } from "zod"; + +const setupConfigSchema = z.object({ + setup: z.array(z.string()).optional(), + teardown: z.array(z.string()).optional(), +}).passthrough(); + function readConfigFromPath(basePath: string): SetupConfig | null { const configPath = join(basePath, PROJECT_SUPERSET_DIR_NAME, CONFIG_FILE_NAME); 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"); - } - return parsed; + const parsed = JSON.parse(content); + return setupConfigSchema.parse(parsed) as SetupConfig; } catch (error) {This would also eliminate the duplicated validation in
teardown.tsif you share the helper.apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.ts (1)
61-98: Good coverage for the new worktree-aware behavior.The two new tests cover the primary happy paths well. One edge case worth considering: when the worktree has an invalid config (e.g., malformed JSON), the current implementation silently falls back to
mainRepoPath. A test for this scenario would document and lock in that behavior.apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
55-58: Pre-existing:runTeardownuses 3 positional parameters.Coding guidelines prefer object parameters for functions with 2+ params. This is pre-existing and out of scope for this PR, but worth noting for a future pass.
| function readConfigFromPath(basePath: string): SetupConfig | null { | ||
| const configPath = join(basePath, ".superset", "config.json"); |
There was a problem hiding this comment.
Hardcoded ".superset" instead of using PROJECT_SUPERSET_DIR_NAME constant.
setup.ts uses the PROJECT_SUPERSET_DIR_NAME constant from shared/constants, but this copy hardcodes ".superset". Use the constant for consistency and to avoid drift.
♻️ Proposed fix
+import { PROJECT_SUPERSET_DIR_NAME, CONFIG_FILE_NAME } from "shared/constants";
+
function readConfigFromPath(basePath: string): SetupConfig | null {
- const configPath = join(basePath, ".superset", "config.json");
+ const configPath = join(basePath, PROJECT_SUPERSET_DIR_NAME, CONFIG_FILE_NAME);📝 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.
| function readConfigFromPath(basePath: string): SetupConfig | null { | |
| const configPath = join(basePath, ".superset", "config.json"); | |
| import { PROJECT_SUPERSET_DIR_NAME, CONFIG_FILE_NAME } from "shared/constants"; | |
| function readConfigFromPath(basePath: string): SetupConfig | null { | |
| const configPath = join(basePath, PROJECT_SUPERSET_DIR_NAME, CONFIG_FILE_NAME); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts` around lines
16 - 17, Replace the hardcoded ".superset" string in readConfigFromPath with the
PROJECT_SUPERSET_DIR_NAME constant: import { PROJECT_SUPERSET_DIR_NAME } from
"shared/constants" at the top of the file and change the join call in
readConfigFromPath to use PROJECT_SUPERSET_DIR_NAME (i.e., join(basePath,
PROJECT_SUPERSET_DIR_NAME, "config.json")) so it matches setup.ts and avoids
string drift.
…ort from setup Removes the duplicated readConfigFromPath/loadSetupConfig from teardown.ts in favor of importing the shared version from setup.ts. Also adds teardown field validation to the shared reader and fixes the hardcoded ".superset" string (now uses PROJECT_SUPERSET_DIR_NAME constant).
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 69-74: loadSetupConfig currently treats a malformed worktree
config the same as a missing file because readConfigFromPath returns null on
both cases; change readConfigFromPath to throw an error for parse/validation
failures (but still return null when file is not found), then update
loadSetupConfig to call readConfigFromPath(worktreePath) inside a try/catch: if
it returns a config return it, if it throws rethrow or surface the error (do not
silently fall back to mainRepoPath), and only call
readConfigFromPath(mainRepoPath) when the worktree read returned null
(file-not-found) so broken worktree configs are not ignored; reference
readConfigFromPath and loadSetupConfig and the worktreePath/mainRepoPath
fallback logic when making the change.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts (2)
43-51: Validation doesn't verify array elements are strings.The error messages claim "array of strings" but only
Array.isArrayis checked. A config like{ "setup": [123, null] }would pass validation. Consider validating element types too, or using a Zod schema for the config shape.♻️ Suggested improvement
- 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"); - } + if ( + parsed.setup && + (!Array.isArray(parsed.setup) || + !parsed.setup.every((s) => typeof s === "string")) + ) { + throw new Error("'setup' field must be an array of strings"); + } + + if ( + parsed.teardown && + (!Array.isArray(parsed.teardown) || + !parsed.teardown.every((s) => typeof s === "string")) + ) { + throw new Error("'teardown' field must be an array of strings"); + }
54-59: Console log missing[domain/operation]prefix.Per coding guidelines, use prefixed console logging: e.g.,
[workspace/setup]. The same applies to the existing log incopySupersetConfigToWorktree(line 24), though that's outside this diff's scope.♻️ Suggested fix
console.error( - `Failed to read setup config at ${configPath}: ${error instanceof Error ? error.message : String(error)}`, + `[workspace/setup] Failed to read setup config at ${configPath}: ${error instanceof Error ? error.message : String(error)}`, );As per coding guidelines, "Use prefixed console logging with consistent context pattern: [domain/operation] message for entry/exit of significant operations, external API calls, and error conditions".
| if (worktreePath) { | ||
| const config = readConfigFromPath(worktreePath); | ||
| if (config) return config; | ||
| } | ||
|
|
||
| return readConfigFromPath(mainRepoPath); |
There was a problem hiding this comment.
Silent fallback when worktree config is malformed could mask errors.
If the worktree config file exists but contains invalid JSON or fails validation, readConfigFromPath returns null, and loadSetupConfig silently falls back to mainRepoPath. This means a broken worktree config is quietly ignored in favor of the main config — the exact scenario this PR aims to prevent.
Consider distinguishing "file not found" (legitimate fallback) from "file exists but is invalid" (should not fall back). One approach: have readConfigFromPath throw on parse/validation errors instead of returning null, and only return null for the missing-file case. Then loadSetupConfig can catch and decide.
♻️ Sketch of the approach
function readConfigFromPath(basePath: string): SetupConfig | null {
const configPath = join(basePath, PROJECT_SUPERSET_DIR_NAME, CONFIG_FILE_NAME);
if (!existsSync(configPath)) {
return null;
}
- try {
- const content = readFileSync(configPath, "utf-8");
- const parsed = JSON.parse(content) as SetupConfig;
- // ... validation ...
- return parsed;
- } catch (error) {
- console.error(...);
- return null;
- }
+ const content = readFileSync(configPath, "utf-8");
+ const parsed = JSON.parse(content) as SetupConfig;
+ // ... validation ...
+ return parsed;
}
export function loadSetupConfig({ mainRepoPath, worktreePath }: { ... }): SetupConfig | null {
if (worktreePath) {
- const config = readConfigFromPath(worktreePath);
- if (config) return config;
+ try {
+ const config = readConfigFromPath(worktreePath);
+ if (config) return config;
+ } catch (error) {
+ console.error(`[workspace/setup] Invalid config in worktree at ${worktreePath}: ${error instanceof Error ? error.message : String(error)}`);
+ return null; // Don't fall back to main — worktree config is authoritative but broken
+ }
}
- return readConfigFromPath(mainRepoPath);
+ try {
+ return readConfigFromPath(mainRepoPath);
+ } catch (error) {
+ console.error(`[workspace/setup] Invalid config in main repo at ${mainRepoPath}: ${error instanceof Error ? error.message : String(error)}`);
+ return null;
+ }
}🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts` around lines 69
- 74, loadSetupConfig currently treats a malformed worktree config the same as a
missing file because readConfigFromPath returns null on both cases; change
readConfigFromPath to throw an error for parse/validation failures (but still
return null when file is not found), then update loadSetupConfig to call
readConfigFromPath(worktreePath) inside a try/catch: if it returns a config
return it, if it throws rethrow or surface the error (do not silently fall back
to mainRepoPath), and only call readConfigFromPath(mainRepoPath) when the
worktree read returned null (file-not-found) so broken worktree configs are not
ignored; reference readConfigFromPath and loadSetupConfig and the
worktreePath/mainRepoPath fallback logic when making the change.
2 participants
Description
Setup/teardown config (
loadSetupConfig) always read.superset/config.jsonfrom the local main branch checkout (mainRepoPath), ignoring the version on the worktree's branch. When a user updates their config on remote (e.g. renaming.superset/setup.shtoscripts/setup-worktree.sh) and creates a new workspace before pulling main locally, the old config is used — causing a "no such file or directory" error for the old script path.This PR changes
loadSetupConfigto prefer reading config from the worktree path first (which has the branch-specific version ofconfig.json), falling back tomainRepoPathwhen the worktree doesn't have a config (e.g. when.supersetis fully gitignored).Changes
setup.ts— ExtractedreadConfigFromPathhelper;loadSetupConfignow accepts{ mainRepoPath, worktreePath? }and tries worktree firstteardown.ts— Same refactor (teardown had the identical bug)create.ts— Updated all 6 call sites to passworktreePath; forcreateFromPr, moved config loading intohandleExistingWorktree/handleNewWorktreewhere the worktree path is knowninit.ts— UpdatedgetSetupCommandsto pass worktree pathsetup.test.ts— Updated existing tests for new signature + added 2 new tests covering worktree preference and fallback behaviorRelated Issues
Type of Change
Testing
setup.test.ts(4 updated + 2 new)prefers worktree config over main repo config— verifies worktree config takes priorityfalls back to main repo when worktree has no config— verifies backward compatibilityScreenshots (if applicable)
N/A — backend logic change only.
Additional Notes
The root cause:
.superset/config.jsonis tracked in git, so worktrees created from remote branches already contain the updated version. ButloadSetupConfigwas hardcoded to read frommainRepoPath, which reflects the local (potentially stale) main branch. The fix reads from the worktree first, which always has the config matching its branch.Summary by CodeRabbit