Fetch and point new workspaces to origin/main#177
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds git helper functions (fetchOriginMain, checkNeedsRebase), wires them into worktree creation and a new refreshGitStatus RPC, extends DB schemas with gitStatus/githubStatus, and removes WorkspaceChannels/WorktreeChannels IPC channel definitions and files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as WorkspacesRouter
participant Git as GitUtils
participant DB
Client->>Router: createWorktree(input)
Router->>Git: fetchOriginMain(mainRepoPath) [best-effort]
alt fetch succeeds
Git-->>Router: originMainRef
Router->>Git: createWorktree(..., startPoint=origin/main)
else fetch fails
Router->>Git: createWorktree(..., startPoint=default)
end
Git-->>Router: worktree created
Router->>DB: insert worktree with gitStatus (branch, needsRebase=false, lastRefreshed)
DB-->>Router: confirm
Router-->>Client: worktree created (with gitStatus)
Client->>Router: refreshGitStatus({workspaceId})
Router->>DB: validate workspace & get paths
Router->>Git: fetchOriginMain(mainRepoPath)
Router->>Git: checkNeedsRebase(worktreePath)
Git-->>Router: needsRebase boolean
Router->>DB: update worktree.gitStatus
DB-->>Router: updated status
Router-->>Client: { gitStatus }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
50-55: Redundant explicit startPoint parameter.The
createWorktreefunction already defaultsstartPointto"origin/main"(line 53 in git.ts). Passing it explicitly here is redundant.Apply this diff to remove the redundant parameter:
await createWorktree( project.mainRepoPath, branch, worktreePath, - "origin/main", );
454-501: Consider error handling consistency for refreshGitStatus.The
refreshGitStatusprocedure callsfetchOriginMain(line 481) without a try-catch, meaning any network or git errors will cause the entire mutation to fail. This differs from the best-effort approach in thecreateprocedure (lines 43-48).Consider:
- Should
refreshGitStatusalso use best-effort error handling since it's called on-demand?- If the user explicitly requests a refresh, should they be notified of fetch failures?
- Or should it fail fast to alert users of connectivity/repository issues?
The current approach (fail fast) is valid but worth documenting or reconsidering based on UX expectations.
If best-effort is preferred, apply this diff:
// Fetch origin/main to get latest -await fetchOriginMain(project.mainRepoPath); +try { + await fetchOriginMain(project.mainRepoPath); +} catch (error) { + console.warn("Failed to fetch origin/main:", error); + // Continue with local origin/main state +} // Check if worktree branch is behind origin/main const needsRebase = await checkNeedsRebase(worktree.path);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(2 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(3 hunks)apps/desktop/src/main/lib/db/schemas.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/desktop/src/shared/ipc-channels/workspace.ts(0 hunks)apps/desktop/src/shared/ipc-channels/worktree.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/shared/ipc-channels/workspace.ts
- apps/desktop/src/shared/ipc-channels/worktree.ts
🧰 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/main/lib/db/schemas.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/db/schemas.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - 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/db/schemas.tsapps/desktop/src/shared/ipc-channels/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.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/db/schemas.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/git.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.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 packages/db/src/**/*.ts : Define all database schema in `packages/db/src/` using Drizzle ORM
Applied to files:
apps/desktop/src/main/lib/db/schemas.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/shared/ipc-channels.ts : Define all IPC channel types with request and response interfaces before implementing handlers
Applied to files:
apps/desktop/src/shared/ipc-channels/index.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,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/shared/ipc-channels/index.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/lib/*-ipcs.ts : IPC handlers must accept a single object parameter, not positional parameters, to match type-safe renderer calls
Applied to files:
apps/desktop/src/shared/ipc-channels/index.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 **/*.{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/git.ts
🧬 Code graph analysis (3)
apps/desktop/src/main/lib/db/schemas.ts (1)
apps/desktop/src/shared/types.ts (1)
Worktree(58-68)
apps/desktop/src/shared/ipc-channels/index.ts (1)
apps/desktop/src/shared/ipc-channels/tab.ts (1)
TabChannels(13-74)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
fetchOriginMain(126-131)createWorktree(49-69)checkNeedsRebase(138-146)apps/desktop/src/main/lib/db/index.ts (1)
db(18-25)
🔇 Additional comments (7)
apps/desktop/src/shared/ipc-channels/index.ts (1)
28-36: Migration to tRPC is complete and properly implemented.Verification confirms:
- No remaining references to
WorkspaceChannelsorWorktreeChannelsin the codebase- tRPC workspace router exists at
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts- No orphaned IPC handler registrations for old channels
- The
IpcChannelsinterface cleanly removes these types in favor of tRPCThe removal correctly aligns with the coding guideline to use tRPC for Electron interprocess communication.
apps/desktop/src/main/lib/db/schemas.ts (2)
11-27: Verify schema definition approach aligns with project conventions.The new interfaces use plain TypeScript types, but the retrieved learning indicates database schemas should be defined using Drizzle ORM in
packages/db/src/. Since this file is atapps/desktop/src/main/lib/db/schemas.ts, please confirm whether:
- This desktop app uses a different schema approach than the broader project, or
- These interfaces should be migrated to Drizzle ORM definitions
Based on learnings, database schemas are typically defined with Drizzle ORM.
35-36: The review comment is incorrect - these are intentionally separate Worktree types serving different architectural purposes.The codebase has two distinct Worktree interfaces by design:
apps/desktop/src/main/lib/db/schemas.ts (line 29): Database schema layer with
gitStatus?: GitStatusandgithubStatus?: GitHubStatuscontaining detailed PR information (pr.url,pr.number,pr.title, etc.)apps/desktop/src/shared/types.ts (line 58): IPC-shared types with
prUrl?: stringfor simplified cross-process communicationThis separation is architecturally sound—the database schema stores comprehensive PR details while the shared interface provides only essential information for IPC. The
githubStatus.pr.urlin the schema andprUrlin shared types are intentionally separate, not duplicates requiring reconciliation.No changes needed.
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
49-69: LGTM!The addition of the
startPointparameter with a default of"origin/main"is well-implemented. The git command syntax is correct, and the log message appropriately reflects the new parameter.
121-131: LGTM!The
fetchOriginMainfunction correctly fetches origin/main and returns the commit SHA. Error propagation to the caller is appropriate, especially given the best-effort usage pattern in the workspace creation flow.
133-146: The original review comment is based on a misunderstanding of git worktree architecture and should be resolved.Git worktrees created with
git worktree adddon't have independent git directories—they share the main repository's.gitdirectory. WhenfetchOriginMainruns onmainRepoPath(line 481 in workspaces.ts), the updatedorigin/mainref is immediately available to all worktrees, including whencheckNeedsRebaseruns onworktree.path(line 484).The code pattern is correct and safe: fetch in the main repo, then check the worktree. Both access the same underlying git refs through the shared
.gitdirectory. No changes are needed.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
63-67: LGTM!The
gitStatusinitialization is correct. SettingneedsRebase: falsefor a freshly created worktree from origin/main is appropriate. The best-effort fetch approach means this might be slightly stale if the fetch failed, but therefreshGitStatusprocedure can update it later.
saddlepaddle
force-pushed
the
silver-thunder-16
branch
from
79e6ea3 to
df51d52
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
superset-teardown.sh (1)
36-39: Fragile warning filtering: relies on specific neonctl output format.Line 39 pipes to
grep -v "^WARNING:"to filter warnings, but this assumes neonctl warnings always start with"WARNING:". If neonctl changes its output format, this filter may break or inadvertently remove valid output.Consider redirecting stderr separately to isolate warnings:
-NEON_OUTPUT=$(neonctl branches create \ +NEON_OUTPUT=$(neonctl branches create \ --project-id $NEON_PROJECT_ID \ --name "$WORKSPACE_NAME" \ - --output json 2>&1 | grep -v "^WARNING:") + --output json 2>/dev/null)Alternatively, check if
neonctlhas a--quietor--no-warningsflag in its documentation to suppress warnings at the source.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
51-56: Consider removing the redundant startPoint parameter.The
createWorktreefunction already defaultsstartPointto"origin/main", making the explicit parameter unnecessary.Apply this diff to simplify the call:
-await createWorktree( - project.mainRepoPath, - branch, - worktreePath, - "origin/main", -); +await createWorktree(project.mainRepoPath, branch, worktreePath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(2 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(3 hunks)apps/desktop/src/main/lib/db/schemas.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/desktop/src/shared/ipc-channels/workspace.ts(0 hunks)apps/desktop/src/shared/ipc-channels/worktree.ts(0 hunks)superset-teardown.sh(1 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/shared/ipc-channels/workspace.ts
- apps/desktop/src/shared/ipc-channels/worktree.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
- apps/desktop/src/main/lib/db/schemas.ts
- apps/desktop/src/shared/ipc-channels/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/lib/trpc/routers/workspaces/workspaces.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - 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/lib/trpc/routers/workspaces/workspaces.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/workspaces.ts
🧬 Code graph analysis (2)
superset-teardown.sh (1)
superset-setup.sh (2)
error(8-8)success(9-9)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
fetchOriginMain(126-131)createWorktree(49-69)checkNeedsRebase(138-146)apps/desktop/src/main/lib/db/index.ts (1)
db(18-25)
⏰ 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/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
9-11: LGTM!The new git utility imports are necessary for the enhanced worktree creation flow and the new refreshGitStatus functionality.
44-49: LGTM!The best-effort fetch approach with silent failure is appropriate here. It ensures the worktree is created from the latest origin/main when possible, without blocking workspace creation due to network issues.
64-68: LGTM!The gitStatus initialization is correct. A newly created worktree branched from origin/main logically doesn't need rebasing, and recording the lastRefreshed timestamp is appropriate.
| refreshGitStatus: publicProcedure | ||
| .input(z.object({ workspaceId: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| const workspace = db.data.workspaces.find( | ||
| (w) => w.id === input.workspaceId, | ||
| ); | ||
| if (!workspace) { | ||
| throw new Error(`Workspace ${input.workspaceId} not found`); | ||
| } | ||
|
|
||
| const worktree = db.data.worktrees.find( | ||
| (wt) => wt.id === workspace.worktreeId, | ||
| ); | ||
| if (!worktree) { | ||
| throw new Error( | ||
| `Worktree for workspace ${input.workspaceId} not found`, | ||
| ); | ||
| } | ||
|
|
||
| const project = db.data.projects.find( | ||
| (p) => p.id === workspace.projectId, | ||
| ); | ||
| if (!project) { | ||
| throw new Error(`Project ${workspace.projectId} not found`); | ||
| } | ||
|
|
||
| // Fetch origin/main to get latest | ||
| await fetchOriginMain(project.mainRepoPath); | ||
|
|
||
| // Check if worktree branch is behind origin/main | ||
| const needsRebase = await checkNeedsRebase(worktree.path); | ||
|
|
||
| const gitStatus = { | ||
| branch: worktree.branch, | ||
| needsRebase, | ||
| lastRefreshed: Date.now(), | ||
| }; | ||
|
|
||
| // Update worktree in db | ||
| await db.update((data) => { | ||
| const wt = data.worktrees.find((w) => w.id === worktree.id); | ||
| if (wt) { | ||
| wt.gitStatus = gitStatus; | ||
| } | ||
| }); | ||
|
|
||
| return { gitStatus }; | ||
| }), |
There was a problem hiding this comment.
Add error handling and worktree existence validation.
The refreshGitStatus mutation has several reliability concerns:
-
Line 476:
fetchOriginMainis not wrapped in try-catch, unlike the best-effort approach in the create mutation (lines 45-49). Network failures will cause the entire operation to fail with an unclear error. -
Line 479:
checkNeedsRebasecan throw if git operations fail or the worktree is in a bad state, but is not wrapped in error handling. -
Missing validation: The mutation doesn't verify that the worktree exists on disk using
worktreeExistsbefore attempting git operations. If the worktree was manually deleted, git operations will fail. The delete mutation performs this check (lines 325-328).
These issues create a poor user experience with unclear error messages and inconsistent error handling across mutations.
Apply this diff to add error handling and validation:
refreshGitStatus: publicProcedure
.input(z.object({ workspaceId: z.string() }))
.mutation(async ({ input }) => {
const workspace = db.data.workspaces.find(
(w) => w.id === input.workspaceId,
);
if (!workspace) {
throw new Error(`Workspace ${input.workspaceId} not found`);
}
const worktree = db.data.worktrees.find(
(wt) => wt.id === workspace.worktreeId,
);
if (!worktree) {
throw new Error(
`Worktree for workspace ${input.workspaceId} not found`,
);
}
const project = db.data.projects.find(
(p) => p.id === workspace.projectId,
);
if (!project) {
throw new Error(`Project ${workspace.projectId} not found`);
}
+
+ // Verify worktree exists on disk
+ const exists = await worktreeExists(project.mainRepoPath, worktree.path);
+ if (!exists) {
+ throw new Error(
+ `Worktree at ${worktree.path} not found on disk. It may have been manually removed.`,
+ );
+ }
- // Fetch origin/main to get latest
- await fetchOriginMain(project.mainRepoPath);
+ // Fetch origin/main to get latest (best-effort)
+ try {
+ await fetchOriginMain(project.mainRepoPath);
+ } catch (error) {
+ console.warn('Failed to fetch origin/main:', error);
+ // Continue with stale origin/main - we can still check rebase status
+ }
- // Check if worktree branch is behind origin/main
- const needsRebase = await checkNeedsRebase(worktree.path);
+ // Check if worktree branch is behind origin/main
+ let needsRebase = false;
+ try {
+ needsRebase = await checkNeedsRebase(worktree.path);
+ } catch (error) {
+ console.error('Failed to check rebase status:', error);
+ throw new Error(
+ `Failed to check git status: ${error instanceof Error ? error.message : String(error)}`,
+ );
+ }
const gitStatus = {
branch: worktree.branch,
needsRebase,
lastRefreshed: Date.now(),
};
// Update worktree in db
await db.update((data) => {
const wt = data.worktrees.find((w) => w.id === worktree.id);
if (wt) {
wt.gitStatus = gitStatus;
}
});
return { gitStatus };
}),🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts around lines
449-496, validate the worktree exists on disk before running git ops (call
worktreeExists and throw a clear error if missing), wrap fetchOriginMain in
try/catch and treat it as best-effort (log the error but continue), and wrap
checkNeedsRebase in try/catch so git failures don’t crash the mutation (on error
set needsRebase to false or a safe default and log the error); then update the
worktree.gitStatus and return as before. Ensure error messages are descriptive
and consistent with other mutations.
| success() { echo -e "${GREEN}✓${NC} $1"; } | ||
|
|
||
| echo "🧹 Tearing down Superset workspace..." | ||
| echo "🚀 Setting up Superset workspace..." |
There was a problem hiding this comment.
Filename mismatch: file is superset-teardown.sh but implements setup.
The file is named superset-teardown.sh but now performs setup tasks (dependency installation, Neon branch creation, .env file generation). This creates confusion and maintenance risk. Consider renaming to superset-setup.sh or clarifying the actual purpose.
🤖 Prompt for AI Agents
In superset-teardown.sh around line 11: the script currently performs Superset
setup tasks (installing dependencies, creating Neon branch, generating .env) but
the filename indicates teardown; either rename the file to superset-setup.sh or
change the script to perform teardown actions. To fix, choose one approach and
update all references: if renaming, move/rename the file to superset-setup.sh,
update any CI/configuration/README references and the script header
(shebang/description) to reflect “setup”; if keeping the teardown name, replace
the setup logic with proper teardown steps (dropping created resources, cleaning
env, reversing changes) and update the echo message. Ensure executable bit and
any docs/automation that call this script are updated to match the new
name/purpose.
| # Create Neon branch for this workspace | ||
| echo "🗄️ Creating Neon branch..." | ||
| WORKSPACE_NAME="${SUPERSET_WORKSPACE_NAME:-$(basename "$PWD")}" | ||
| NEON_OUTPUT=$(neonctl branches create \ | ||
| --project-id tiny-cherry-82420694 \ | ||
| --name "$WORKSPACE_NAME" \ | ||
| --output json 2>&1 | grep -v "^WARNING:") |
There was a problem hiding this comment.
Hard-coded Neon project ID and missing neonctl error handling.
Line 37 contains a hard-coded project ID (tiny-cherry-82420694). This should be externalized as an environment variable or configuration for reusability across environments. Additionally, there is no validation that the neonctl branches create command actually succeeded before attempting to parse its output.
Apply this diff to parameterize the project ID and add error handling:
# Create Neon branch for this workspace
echo "🗄️ Creating Neon branch..."
WORKSPACE_NAME="${SUPERSET_WORKSPACE_NAME:-$(basename "$PWD")}"
+NEON_PROJECT_ID="${NEON_PROJECT_ID:-tiny-cherry-82420694}"
+
-NEON_OUTPUT=$(neonctl branches create \
+NEON_OUTPUT=$(neonctl branches create \
--project-id $NEON_PROJECT_ID \
--name "$WORKSPACE_NAME" \
- --output json 2>&1 | grep -v "^WARNING:")
+ --output json 2>&1 | grep -v "^WARNING:") || error "Failed to create Neon branch"
+
+[ -z "$NEON_OUTPUT" ] && error "Neon branch creation returned empty output"Committable suggestion skipped: line range outside the PR's diff.
| DATABASE_URL=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_uri') | ||
| POOLER_HOST=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.pooler_host') | ||
| PASSWORD=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.password') | ||
| ROLE=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.role') | ||
| DATABASE=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.database') | ||
| DATABASE_POOLED_URL="postgresql://${ROLE}:${PASSWORD}@${POOLER_HOST}/${DATABASE}?sslmode=require" |
There was a problem hiding this comment.
Missing validation of jq extraction: could write null values to .env.
Lines 42–47 extract connection parameters using jq without validating that the values are non-empty. If the Neon output structure differs or contains null values, the DATABASE_POOLED_URL will be malformed, resulting in silent configuration failures.
Add validation to ensure all extracted values are non-empty:
# Parse connection strings from create output
DATABASE_URL=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_uri')
POOLER_HOST=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.pooler_host')
PASSWORD=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.password')
ROLE=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.role')
DATABASE=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.database')
+[ -z "$DATABASE_URL" ] && error "Failed to extract DATABASE_URL from Neon output"
+[ -z "$POOLER_HOST" ] && error "Failed to extract POOLER_HOST from Neon output"
+[ -z "$PASSWORD" ] && error "Failed to extract PASSWORD from Neon output"
+[ -z "$ROLE" ] && error "Failed to extract ROLE from Neon output"
+[ -z "$DATABASE" ] && error "Failed to extract DATABASE from Neon output"
+
DATABASE_POOLED_URL="postgresql://${ROLE}:${PASSWORD}@${POOLER_HOST}/${DATABASE}?sslmode=require"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In superset-teardown.sh around lines 42 to 47, the jq extractions for
DATABASE_URL, POOLER_HOST, PASSWORD, ROLE, and DATABASE are not validated and
may produce empty/null values that create a malformed DATABASE_POOLED_URL;
update the script to check each extracted variable is non-empty (e.g., test with
-z or conditional), emit a clear error and exit non-zero if any are missing, and
only construct and export DATABASE_POOLED_URL when all required values are
present.
| cat > .env << EOF | ||
| DATABASE_URL=$DATABASE_URL | ||
| DATABASE_POOLED_URL=$DATABASE_POOLED_URL | ||
| EOF |
There was a problem hiding this comment.
Destructive .env overwrite without backup or confirmation.
Line 49 silently overwrites .env without creating a backup or warning the user. If the script is re-run or if .env contains manual edits, data is lost.
Consider adding a backup or preserving existing .env entries:
-cat > .env << EOF
+[ -f .env ] && cp .env .env.bak
+cat > .env << EOF
DATABASE_URL=$DATABASE_URL
DATABASE_POOLED_URL=$DATABASE_POOLED_URL
EOF
+success ".env created (previous version backed up to .env.bak)"Alternatively, if you want to preserve other variables in .env, use sed or grep to merge instead of overwriting.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In superset-teardown.sh around lines 49 to 52, the script unconditionally
overwrites .env which can destroy existing user edits; change this to create a
timestamped backup of the existing .env (or prompt for confirmation when
interactive), and then write the new variables either by merging with the
existing file (update or append DATABASE_URL and DATABASE_POOLED_URL using
sed/grep logic) or by writing to a new file and atomically moving it into place;
ensure backup is optional via a flag or an interactive confirmation to avoid
silent data loss.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.