feat(desktop): setup/teardown scripts editor for v2 projects#4090
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the scripts editor into a shared view with two wrappers (V1 for electron TRPC, V2 for host-service client), adds a host-service TRPC router to read/write .superset/config.json, introduces V2 setup-card UI in the dashboard sidebar, and adds a persisted dismissal store for that card. ChangesVersioned Scripts Editor & Host Config Router
V2 Setup Card & Dismissals
Sequence Diagram(s)sequenceDiagram
participant UI as ProjectSettings UI
participant V1 as V1ScriptsEditor
participant TRPC as Electron TRPC
participant HostFS as Host Service / FS
UI->>V1: mount(projectId)
V1->>TRPC: config.getConfigContent({ projectId })
TRPC->>HostFS: resolve repoPath & read .superset/config.json
HostFS-->>TRPC: { content, exists }
TRPC-->>V1: return config
V1->>V1: render ScriptsEditorView(serverContent)
UI->>V1: edit textarea
V1->>V1: update local state
UI->>V1: blur
V1->>V1: commit() -> prepare arrays
V1->>TRPC: config.updateConfig({ projectId, setup, teardown, run })
TRPC->>HostFS: merge & write .superset/config.json
HostFS-->>TRPC: success
TRPC-->>V1: success
sequenceDiagram
participant UI as V2ProjectSettings UI
participant V2 as V2ScriptsEditor
participant RQ as React Query (host-service)
participant HostAPI as Host Service API
participant FS as File System
UI->>V2: mount(hostUrl, projectId)
V2->>RQ: client.config.getConfigContent({ projectId })
RQ->>HostAPI: request
HostAPI->>FS: read .superset/config.json
FS-->>HostAPI: content
HostAPI-->>RQ: { content, exists }
RQ-->>V2: return config
V2->>V2: render ScriptsEditorView(serverContent)
UI->>V2: edit textarea
V2->>V2: update local state
UI->>V2: blur
V2->>V2: commit()
V2->>RQ: client.config.updateConfig({ projectId, setup, teardown, run })
RQ->>HostAPI: request
HostAPI->>FS: write .superset/config.json
FS-->>HostAPI: success
HostAPI-->>RQ: ok
RQ-->>V2: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile Summary
Confidence Score: 3/5Not safe to merge as-is — the run array corruption bug permanently changes config data for any v2 project that stores multiple run commands. One P1 defect (multi-element run arrays silently rewritten as a single-element array on every save) and one P2 (async side-effect in setState updater). The P1 is a data-loss issue directly on the new v2 code path added by this PR. ScriptsEditor.tsx — toArrayCommand / commit logic around the run field needs to either split on newlines when serializing back or preserve the original array when the run tab is hidden.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/lib/trpc/routers/config/config.ts | Generalizes getConfigContent and updateConfig to accept either projectId or mainRepoPath via a union schema and shared resolveMainRepoPath helper. Logic is clean; z.intersection with a z.union behaves correctly at runtime. |
| apps/desktop/src/renderer/lib/project-scripts.ts | Updates invalidateProjectScriptQueries to accept ProjectScriptsTarget; conditionally skips shouldShowSetupCard invalidation for v2 (mainRepoPath) targets. Correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/project/$projectId/page.tsx | Trivial call-site update to pass { projectId } object to invalidateProjectScriptQueries. No issues. |
| apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/ProjectSettings.tsx | Updates ScriptsEditor prop from projectId string to target object. Single-line change, no issues. |
| apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx | Major refactor: save-on-blur, run tab hidden, generalized target prop. Contains a P1 bug where multi-element run arrays are silently collapsed to a single element on save, plus a P2 pattern of calling async commit from inside a setState updater. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx | Adds ScriptsEditor to v2 settings page, only rendered when hostProject.repoPath is available. Guard is correct; imports are clean. |
Sequence Diagram
sequenceDiagram
participant UI as ScriptsEditor (React)
participant tRPC as tRPC Router
participant FS as Filesystem
Note over UI: User blurs textarea
UI->>UI: handleBlur(kind) — trim, set isFocusedRef=false
UI->>UI: commit(next) — compare vs lastSavedRef
alt values changed
UI->>tRPC: updateConfig({ target | mainRepoPath, setup, teardown, run })
tRPC->>tRPC: resolveMainRepoPath(input)
alt mainRepoPath in input
tRPC-->>tRPC: use input.mainRepoPath directly
else projectId in input
tRPC->>tRPC: localDb lookup → project.mainRepoPath
end
tRPC->>FS: read existing config.json
tRPC->>FS: write merged config.json
tRPC-->>UI: { success: true }
UI->>tRPC: invalidateProjectScriptQueries(target)
Note over UI: saveStatus = "saved"
else no change
Note over UI: skip network call
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:155-165
**Multi-element `run` array silently corrupted on save**
When `.superset/config.json` contains `"run": ["cmd1", "cmd2"]` (multiple elements), `parseContentFromConfig` joins them as `"cmd1\ncmd2"`, and `toArrayCommand` wraps the whole trimmed string into a single element `["cmd1\ncmd2"]`. Every save of setup/teardown therefore overwrites a multi-element `run` array with a single-element array, permanently changing the config. The test plan specifically promises run values are preserved for v2 projects, but this round-trip breaks multi-element arrays on the first edit of any other tab.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:258-272
**Async side-effect inside `setState` updater**
`commit` (an async function) is called from within the `setValues` functional updater. React is permitted to call state updaters more than once (it does so intentionally in Strict Mode dev builds to detect impure updaters), which would fire two concurrent `mutateAsync` network requests on every blur. Extract the commit call outside the updater:
```
const handleBlur = useCallback(
(kind: ScriptKind) => {
isFocusedRef.current = false;
const trimmed = values[kind].trim();
const next =
trimmed === values[kind]
? values
: { ...values, [kind]: trimmed };
if (next !== values) setValues(next);
void commit(next);
},
[commit, values],
);
```
Reviews (1): Last reviewed commit: "feat(desktop): bring back setup/teardown..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+155
to
+165
| type="file" | ||
| accept=".sh,.bash,.zsh,.command" | ||
| onChange={handleFileInputChange} | ||
| className="hidden" | ||
| /> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| type SaveStatus = "idle" | "saving" | "saved"; | ||
| const TAB_DESCRIPTIONS: Record<ScriptKind, string> = { |
There was a problem hiding this comment.
run array silently corrupted on save
When .superset/config.json contains "run": ["cmd1", "cmd2"] (multiple elements), parseContentFromConfig joins them as "cmd1\ncmd2", and toArrayCommand wraps the whole trimmed string into a single element ["cmd1\ncmd2"]. Every save of setup/teardown therefore overwrites a multi-element run array with a single-element array, permanently changing the config. The test plan specifically promises run values are preserved for v2 projects, but this round-trip breaks multi-element arrays on the first edit of any other tab.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx
Line: 155-165
Comment:
**Multi-element `run` array silently corrupted on save**
When `.superset/config.json` contains `"run": ["cmd1", "cmd2"]` (multiple elements), `parseContentFromConfig` joins them as `"cmd1\ncmd2"`, and `toArrayCommand` wraps the whole trimmed string into a single element `["cmd1\ncmd2"]`. Every save of setup/teardown therefore overwrites a multi-element `run` array with a single-element array, permanently changing the config. The test plan specifically promises run values are preserved for v2 projects, but this round-trip breaks multi-element arrays on the first edit of any other tab.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx (1)
29-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
Array.isArrayguards so a malformed field doesn't wipe valid ones.If
.superset/config.jsonhas e.g.setup: "echo hi"(string instead of array),(parsed.setup ?? []).join("\n")throws, the catch path runs, and the user seesEMPTYforsetup,teardown, andrun— losing visibility into otherwise-valid fields. The dashboard'sparseConfigContentalready usesArray.isArrayfor exactly this reason.🛠️ Proposed fix
function parseContentFromConfig(content: string | null): ScriptValues { if (!content) return { ...EMPTY }; try { const parsed = JSON.parse(content); + const toLines = (v: unknown): string => + Array.isArray(v) + ? v.filter((s): s is string => typeof s === "string").join("\n") + : ""; return { - setup: (parsed.setup ?? []).join("\n"), - teardown: (parsed.teardown ?? []).join("\n"), - run: (parsed.run ?? []).join("\n"), + setup: toLines(parsed?.setup), + teardown: toLines(parsed?.teardown), + run: toLines(parsed?.run), }; } catch { return { ...EMPTY }; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx around lines 29 - 41, The parseContentFromConfig function currently JSON.parses and assumes parsed.setup/teardown/run are arrays, so a malformed one causes the catch branch and wipes all fields; update parseContentFromConfig to guard each field using Array.isArray before calling .join (e.g., for setup, use Array.isArray(parsed.setup) ? parsed.setup.join("\n") : ""), repeating the same pattern for teardown and run so a single bad field doesn't clear the others while still falling back to EMPTY when content is null or parsing fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/config/config.ts`:
- Around line 468-485: Tighten validation and clarify errors: make
configTargetSchema require a non-empty mainRepoPath (use z.string().min(1)) so
empty strings fail validation up-front, and in updateConfig split the failure
cases — if input.mainRepoPath is empty/invalid return a clear validation error
(e.g., "mainRepoPath must be a non-empty string"); keep the separate branch for
resolveMainRepoPath(input) returning falsy and throw "Project not found" (or
similar) so callers can distinguish an invalid input from a missing project;
ensure references in this logic point to configTargetSchema, updateConfig,
resolveMainRepoPath, getConfigContent and ensureConfigExists.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:
- Around line 177-180: The toArrayCommand function currently collapses
newline-joined multi-command text into a single-string array element; change its
logic to mirror splitCommands by splitting the trimmed value on newlines,
trimming each line, filtering out empty lines, and returning that array so
multi-command script arrays (run/setup/teardown) are preserved; update any other
uses in this file (including the blur/commit path and the similar code
referenced around lines 209–242) to call the revised toArrayCommand so commit
sends back a proper string[] rather than one newline-containing string.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:
- Around line 29-41: The parseContentFromConfig function currently JSON.parses
and assumes parsed.setup/teardown/run are arrays, so a malformed one causes the
catch branch and wipes all fields; update parseContentFromConfig to guard each
field using Array.isArray before calling .join (e.g., for setup, use
Array.isArray(parsed.setup) ? parsed.setup.join("\n") : ""), repeating the same
pattern for teardown and run so a single bad field doesn't clear the others
while still falling back to EMPTY when content is null or parsing fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa4c30ef-5374-4507-a5d2-8fc16c5994cd
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/renderer/lib/project-scripts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/project/$projectId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx (1)
43-46:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
toArrayCommandstill collapses multi-line content into a single array element — corrupts existingrunarrays on first save.This was flagged on a previous commit but the implementation is unchanged. Because
commit()always sends all three fields (includingrun, whose tab is now hidden), an existing config likerun: ["bun run dev", "echo started"]round-trips as:
parseContentFromConfigjoins the array →values.run === "bun run dev\necho started".- Any blur on Setup/Teardown calls
commit(values).toArrayCommand(values.run)returns["bun run dev\necho started"](single element with embedded\n).- Server persists the corrupted single-element array.
This silently breaks the PR's own test-plan assertion "Existing non-empty
runarrays in v2 configs are preserved despite the hidden Run tab." The dashboard'ssplitCommandsalready does the right thing.🛠️ Proposed fix
-function toArrayCommand(value: string): string[] { - const trimmed = value.trim(); - return trimmed ? [trimmed] : []; -} +function toArrayCommand(value: string): string[] { + return value + .split("\n") + .map((line) => line.trim()) + .filter((line) => line.length > 0); +}Also applies to: 234-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx around lines 43 - 46, The toArrayCommand function currently returns a single element containing embedded newlines; change it to split multi-line input into separate command strings by splitting on newlines (handle \r\n and \n), trim each line and filter out empty lines so an input like "bun run dev\necho started" becomes ["bun run dev", "echo started"]; update the function named toArrayCommand (and the duplicate implementation referenced in the same file) to perform value.split(/\r?\n/).map(s => s.trim()).filter(Boolean) and return that array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:
- Around line 43-46: The toArrayCommand function currently returns a single
element containing embedded newlines; change it to split multi-line input into
separate command strings by splitting on newlines (handle \r\n and \n), trim
each line and filter out empty lines so an input like "bun run dev\necho
started" becomes ["bun run dev", "echo started"]; update the function named
toArrayCommand (and the duplicate implementation referenced in the same file) to
perform value.split(/\r?\n/).map(s => s.trim()).filter(Boolean) and return that
array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57fffda9-2d56-422b-bea3-0f77e70bc915
📒 Files selected for processing (7)
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsxpackages/host-service/src/trpc/router/config/config.tspackages/host-service/src/trpc/router/config/index.tspackages/host-service/src/trpc/router/router.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/trpc/router/config/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/V2SetupScriptCard/V2SetupScriptCard.tsx (1)
48-73: 💤 Low value
exitanimation onmotion.divis unreachable
AnimatePresencelives insideV2SetupScriptCard, which returnsnull(not<AnimatePresence>) when the card should hide. Because the whole component unmounts before Framer Motion can orchestrate the exit,exit={{ opacity: 0, y: 10 }}never fires — the card disappears instantly on dismiss or whenshouldShowflips tofalse.To get the exit animation,
AnimatePresenceneeds to wrap the conditional render from the outside (e.g., inDashboardSidebar):♻️ Proposed fix — lift AnimatePresence to the caller
In
DashboardSidebar.tsx, replace the bare<V2SetupScriptCard>with:+<AnimatePresence> <V2SetupScriptCard hostUrl={activeHostUrl} projectId={activeWorkspaceProject?.id ?? null} projectName={activeWorkspaceProject?.name ?? null} isCollapsed={isCollapsed} /> +</AnimatePresence>And in
V2SetupScriptCard.tsx, drop theAnimatePresencewrapper but keepmotion.div:-return ( - <AnimatePresence> - <motion.div - key={projectId} - ... - > - <SidebarCard ... /> - </motion.div> - </AnimatePresence> -); +return ( + <motion.div + key={projectId ?? "card"} + initial={{ opacity: 0, y: 10 }} + animate={{ opacity: 1, y: 0 }} + exit={{ opacity: 0, y: 10 }} + transition={{ duration: 0.2 }} + className="px-3 pb-2" + > + <SidebarCard ... /> + </motion.div> +);And expose the inner conditional logic to the parent so
AnimatePresencecan track the child's lifecycle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/V2SetupScriptCard/V2SetupScriptCard.tsx` around lines 48 - 73, The exit animation never runs because AnimatePresence is inside V2SetupScriptCard so the component unmounts before Framer Motion can play the exit; move AnimatePresence out to the parent (DashboardSidebar) so it wraps the conditional render of V2SetupScriptCard, remove the AnimatePresence wrapper from V2SetupScriptCard and keep only the motion.div with its exit prop, and expose the card's show/hide conditional (e.g., the shouldShow/visible logic) to DashboardSidebar so AnimatePresence can track the child lifecycle and allow exit={{ opacity: 0, y: 10 }} on the motion.div to run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/V2SetupScriptCard/V2SetupScriptCard.tsx`:
- Around line 48-73: The exit animation never runs because AnimatePresence is
inside V2SetupScriptCard so the component unmounts before Framer Motion can play
the exit; move AnimatePresence out to the parent (DashboardSidebar) so it wraps
the conditional render of V2SetupScriptCard, remove the AnimatePresence wrapper
from V2SetupScriptCard and keep only the motion.div with its exit prop, and
expose the card's show/hide conditional (e.g., the shouldShow/visible logic) to
DashboardSidebar so AnimatePresence can track the child lifecycle and allow
exit={{ opacity: 0, y: 10 }} on the motion.div to run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd41ff2c-e3a7-4a2c-85ba-a9fe9dfc43ba
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/V2SetupScriptCard/V2SetupScriptCard.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/V2SetupScriptCard/index.tsapps/desktop/src/renderer/stores/v2-setup-card-dismissals/index.tsapps/desktop/src/renderer/stores/v2-setup-card-dismissals/store.tspackages/host-service/src/trpc/router/config/config.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/V2SetupScriptCard/index.ts
- apps/desktop/src/renderer/stores/v2-setup-card-dismissals/index.ts
Contributor
There was a problem hiding this comment.
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/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx (1)
33-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid wiping all script fields when one parsed field is malformed.
(parsed.setup ?? []).join("\n")assumes array shape. If one field is not an array, parsing falls intocatchand resets all values to empty, which can lead to destructive overwrite on next save.Proposed fix
function parseContentFromConfig(content: string | null): ScriptValues { if (!content) return { ...EMPTY }; try { const parsed = JSON.parse(content); + const toText = (value: unknown): string => + Array.isArray(value) + ? value + .filter((v): v is string => typeof v === "string") + .join("\n") + : ""; return { - setup: (parsed.setup ?? []).join("\n"), - teardown: (parsed.teardown ?? []).join("\n"), - run: (parsed.run ?? []).join("\n"), + setup: toText(parsed.setup), + teardown: toText(parsed.teardown), + run: toText(parsed.run), }; } catch { return { ...EMPTY }; } }Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx around lines 33 - 37, The current code blindly does (parsed.setup ?? []).join("\n") (and same for teardown/run) which treats non-array values as empty and can reset all fields if one parse error occurs; update ScriptsEditor to check each parsed field with Array.isArray(parsed.setup) ? parsed.setup.join("\n") : preserve the existing value (e.g., the current state or original script string) for setup, and do the same for teardown and run so a malformed field doesn't wipe the others — reference the parsed object and the setup/teardown/run properties when making these checks and use safe fallbacks instead of joining an assumed array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:
- Around line 33-37: The current code blindly does (parsed.setup ??
[]).join("\n") (and same for teardown/run) which treats non-array values as
empty and can reset all fields if one parse error occurs; update ScriptsEditor
to check each parsed field with Array.isArray(parsed.setup) ?
parsed.setup.join("\n") : preserve the existing value (e.g., the current state
or original script string) for setup, and do the same for teardown and run so a
malformed field doesn't wipe the others — reference the parsed object and the
setup/teardown/run properties when making these checks and use safe fallbacks
instead of joining an assumed array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 931628f9-b098-4104-84f0-2cae037adc6d
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx
Plan-only. Code changes deferred until the plan is reviewed. Scope is strictly v2 — no v1 ScriptsEditor / electronTrpc config router / v1 SetupScriptCard touches. Per project rule: v1 desktop UI is sunset.
Kitenite
force-pushed
the
setup-teardown-scripts
branch
from
ddcb14d to
7b4d4c4
Compare
Adds the v2 equivalent of the v1 project-settings scripts editor and makes v2 workspace creation honor the configured setup array (was only running a literal `<worktree>/.superset/setup.sh`). - host-service config loader resolves `<repoPath>/.superset/config.json` + `~/.superset/projects/<id>/config.json` user override + local `.superset/config.local.json` overlay; no worktree-level read so the main repo stays the single source of truth. - new host-service `config` router: getConfigContent, updateConfig, shouldShowSetupCard. - setup-terminal helper joins resolved setup commands with `&&` and falls back to `bash <repoPath>/.superset/setup.sh` against the main repo. Worktrees reach the canonical .superset/ via $SUPERSET_ROOT_PATH. - V2ScriptsEditor mounts in V2ProjectSettings (Setup/Teardown tabs, blur-only save, multi-line→array, drag-drop import). - V2SetupScriptCard sidebar CTA with a per-machine zustand+persist dismissals store, gated on the active v2 workspace route. v1 paths intentionally untouched.
…solver Adds integration tests for the v2 setup/teardown work: - config loader: malformed JSON, mixed-type rejection, override precedence, before/after/replace overlay, three-layer stacking, empty-array override clearing base, partial-keys passthrough, path-traversal guard, no worktree consultation. - config router (in-memory drizzle): getConfigContent / updateConfig (preserves run + unrelated keys, overwrites malformed) / shouldShowSetupCard (run-only, teardown-only, all-empty cases). - resolveInitialCommand: && joining, fallback to setup.sh, single-quote escape, config-wins-over-fallback, whitespace filter. Adds optional `homeDir` parameter to loadSetupConfig and resolveInitialCommand to allow test sandboxes without process.env mutation. Production callers don't pass it (defaults to os.homedir()).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.superset/config.jsonsetup and teardown scripts for v2 projects (the editor existed for v1 only).config.getConfigContent/updateConfigtRPC routes to accept either{ projectId }(v1 local-db lookup) or{ mainRepoPath }(v2 host project), so a singleScriptsEditorworks in both surfaces.<Textarea>, tighter heading + description pair, smallerh-7action buttons, subtler drag-import overlay.runvalues are preserved through load/save.Test plan
<repoPath>/.superset/config.json..shfile onto a textarea → contents replace the value; blur saves it.runarray → run value is preserved on subsequent saves of setup/teardown (not zeroed out by the hidden tab).Summary by CodeRabbit
Refactor
New Features
Chores