feat(deploy): add deploy-time db support for preview branches #66
Merged
Conversation
Summary by CodeRabbit
WalkthroughImplements preview-branch database provisioning for ChangesPreview Branch Database Setup
🚥 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)
✨ Simplify code
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/controllers/app.ts (1)
2266-2296:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
resolveProjectContext()TypeScript contract drift (missingenvProjectId+branch.id)In
packages/cli/src/controllers/app.ts(lines 2266-2296),resolveProjectContext()has two build-breaking issues:
optionsdoes not declareenvProjectId, but the function readsoptions?.envProjectIdwhen callingresolveProjectTarget(...).ResolvedAppProjectContext.branchrequires{ id: string; name: string; kind: BranchKind }, but the function returnsbranch: { name, kind }withoutid.🤖 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 `@packages/cli/src/controllers/app.ts` around lines 2266 - 2296, resolveProjectContext currently reads options?.envProjectId but options lacks that property and it omits branch.id when constructing the returned ResolvedAppProjectContext.branch; update the function signature to include envProjectId?: string on the options parameter, pass options.envProjectId into resolveProjectTarget (already referenced), and include the resolved deploy branch's id when returning branch (e.g., branch: { id: branch.id, name: branch.name, kind: toBranchKind(branch.name) }) so the return value matches ResolvedAppProjectContext; ensure you use the branch value produced by resolveDeployBranch and keep the toBranchKind call as-is.
🤖 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 `@docs/product/command-spec.md`:
- Around line 632-634: Add an explicit statement that the flags `--db` and
`--no-db` are mutually exclusive and that passing both is invalid; update the
behavior section near the `--db`, `--no-db`, and `--yes` bullets to include one
clear sentence such as "Note: `--db` and `--no-db` are mutually exclusive;
passing both will be treated as an error (CLI will reject/exit)." Also clarify
the `--yes` rule remains (i.e., `--yes` alone does not create a DB; CI must pass
`--db --yes`) so readers know the precedence and exact usage contract for
`--db`, `--no-db`, and `--yes`.
In `@packages/cli/src/lib/app/branch-database-deploy.ts`:
- Around line 95-114: The early-return treats envState.branchDatabaseUrl alone
as “already configured” and skips when options.db is true, which leaves partial
setups (only DATABASE_URL or only DIRECT_URL) unrepaired; update the logic
around envState.branchDatabaseUrl and the skip-return so it checks both
companion variables (e.g., DATABASE_URL and DIRECT_URL as represented by
envState and branchEnvVars) and only emits the skipped result via
emitBranchDatabaseWarning/returned status when both variables exist and are
consistent; if one is missing or mismatched, fall through to the
deployment/repair path to set or reconcile the missing companion var(s) instead
of returning skipped.
In `@packages/cli/src/lib/app/branch-database.ts`:
- Around line 143-165: scanDirectory currently sets state.schemaPath to the
first encountered "schema.prisma", which is nondeterministic; instead, stop
assigning on first hit and collect candidates (e.g., push entryPath into
state.schemaCandidates), continue whole scan, and after scan completes pick a
deterministic candidate—prefer a schema located in the top-level cwd directory,
otherwise choose the candidate with the shortest relative path and break ties
lexicographically—and assign that to state.schemaPath; update references to
state.schemaPath and ensure state.schemaCandidates is initialized before
scanning.
- Around line 225-238: The child process spawn in branch-database.ts always
creates stdout/stderr pipes but only consumes them when
!options.context.flags.json && !options.context.flags.quiet, which can cause the
child to block; change the spawn stdio behavior to be conditional: when
options.context.flags.json || options.context.flags.quiet set stdio to
non-piped/ignored (e.g., ["ignore","ignore","ignore"]) to avoid creating
backpressure, otherwise keep the current ["ignore","pipe","pipe"] and the
existing piping logic that attaches child.stdout/child.stderr to
options.context.output.stderr.
In `@packages/cli/tests/app-branch-database.test.ts`:
- Around line 452-504: The test suite lacks coverage for the failure path when
runBranchDatabaseSchemaSetup rejects; add a new test that mocks
runBranchDatabaseSchemaSetup to reject (e.g., vi.fn().mockRejectedValue(new
Error("..."))) while keeping createBranchDatabase mocked, then call runAppDeploy
with db: true and assert it rejects with the appropriate error code (e.g.,
SCHEMA_SETUP_FAILED), and assert createBranchDatabase was called but deployApp
was not; patch the module via vi.doMock("../src/lib/app/branch-database", ...)
to replace runBranchDatabaseSchemaSetup and reuse the existing test
setup/expectations around createBranchDatabase and deployApp.
- Around line 43-110: Extract the duplicate test helpers createProjectClient and
createResolveBranch into a shared test helper module (e.g.,
tests/helpers/mock-factories.ts): move the implementations of
createProjectClient and createResolveBranch from this file into that module,
export them, then import and use those functions here and in
app-controller.test.ts to remove duplication and keep behavior consistent;
ensure the exported signatures and default parameter behavior (projectId
default, options.branchExists/options.isDefault, and role default) match the
originals so existing tests using GET/POST mocks keep working.
---
Outside diff comments:
In `@packages/cli/src/controllers/app.ts`:
- Around line 2266-2296: resolveProjectContext currently reads
options?.envProjectId but options lacks that property and it omits branch.id
when constructing the returned ResolvedAppProjectContext.branch; update the
function signature to include envProjectId?: string on the options parameter,
pass options.envProjectId into resolveProjectTarget (already referenced), and
include the resolved deploy branch's id when returning branch (e.g., branch: {
id: branch.id, name: branch.name, kind: toBranchKind(branch.name) }) so the
return value matches ResolvedAppProjectContext; ensure you use the branch value
produced by resolveDeployBranch and keep the toBranchKind call as-is.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cce0d9a3-ef38-4b5e-9aa5-bf6f11c859c5
📒 Files selected for processing (15)
docs/product/command-spec.mddocs/product/error-conventions.mddocs/product/resource-model.mdpackages/cli/src/commands/app/index.tspackages/cli/src/controllers/app.tspackages/cli/src/lib/app/branch-database-deploy.tspackages/cli/src/lib/app/branch-database.tspackages/cli/src/lib/app/preview-branch-database.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/src/presenters/app.tspackages/cli/src/shell/command-meta.tspackages/cli/src/types/app.tspackages/cli/tests/app-branch-database.test.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/cli/src/controllers/app.ts`:
- Around line 2291-2297: resolveProjectContext() is eagerly calling
resolveExistingAppBranchId and populating branch.id (via the returned object
spread) which triggers a /branches?gitName= network lookup for every non-deploy
command; remove that eager resolution and keep branch.id lazy/absent here so
only deploy flows resolve it. Concretely, stop invoking
resolveExistingAppBranchId inside the object returned by resolveProjectContext
(leave branch as { name: branch.name, kind: toBranchKind(branch.name) } or omit
id), and move the call to resolveExistingAppBranchId into the deploy-specific
code path where toBranchDatabaseDeployBranch() runs so that branch IDs are
resolved only when deploy logic needs them.
- Around line 2507-2516: The thrown CliError for BRANCH_RESOLUTION_FAILED is
using domain: "app" but should use domain: "branch" so JSON consumers classify
it correctly; update the CliError instantiation (the object passed to new
CliError in the branch resolution failure block that includes code:
"BRANCH_RESOLUTION_FAILED", summary: `Failed to resolve branch "${branchName}"`,
why: ..., etc.) to set domain: "branch" instead of "app".
In `@packages/cli/src/lib/app/branch-database-deploy.ts`:
- Around line 214-248: The code upserts DATABASE_URL but leaves a previously set
DIRECT_URL unchanged when database.directUrl is falsy; update
upsertBranchDatabaseEnvVars to reconcile DIRECT_URL: when database.directUrl is
falsy and envState.branchDirectUrl exists, call upsertBranchDatabaseEnvVar (same
signature as used elsewhere) to clear/remove the DIRECT_URL entry for the branch
(e.g., by passing an empty/null value or invoking the provider's delete behavior
via upsertBranchDatabaseEnvVar) so DIRECT_URL does not point at a stale
database; keep existing behavior when database.directUrl is present (the
existing upsert path using upsertBranchDatabaseEnvVar for key "DIRECT_URL").
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c69c32c-f18d-44c8-96b4-443254f2aa20
📒 Files selected for processing (12)
docs/product/command-spec.mddocs/product/error-conventions.mdpackages/cli/src/commands/app/index.tspackages/cli/src/controllers/app.tspackages/cli/src/lib/app/branch-database-deploy.tspackages/cli/src/lib/app/branch-database.tspackages/cli/src/lib/app/preview-branch-database.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/tests/app-branch-database.test.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app.test.tspackages/cli/tests/helpers/mock-factories.ts
luanvdw
changed the title
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/cli/tests/app-controller.test.ts`:
- Around line 45-53: The async mock defaults in
withBranchDatabaseProviderDefaults are plain vi.fn()s which can return undefined
when awaited; change createBranchDatabase, deleteBranchDatabase,
createEnvironmentVariable, updateEnvironmentVariable, and
deleteEnvironmentVariable to return resolved Promises by default (e.g.,
vi.fn().mockResolvedValue(undefined) or mockResolvedValue(sensible default)) so
any code awaiting or chaining .catch won't get undefined; keep
listEnvironmentVariables as is or adjust similarly if needed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd8c7a22-b9ef-499e-8a0f-8ebdaa128a05
📒 Files selected for processing (8)
docs/product/command-spec.mddocs/product/error-conventions.mdpackages/cli/src/controllers/app.tspackages/cli/src/lib/app/branch-database-deploy.tspackages/cli/src/lib/app/preview-branch-database.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/tests/app-branch-database.test.tspackages/cli/tests/app-controller.test.ts
💤 Files with no reviewable changes (1)
- docs/product/error-conventions.md
Comment on lines
+45
to
+53
| function withBranchDatabaseProviderDefaults<T extends Record<string, unknown>>(provider: T) { | ||
| return { | ||
| token: "token", | ||
| GET: vi.fn().mockImplementation((pathName: string, request?: { params?: { query?: { gitName?: string } } }) => { | ||
| if (pathName === "/v1/projects") { | ||
| return { | ||
| data: { | ||
| data: [ | ||
| { | ||
| id: projectId, | ||
| name: projectId === "proj_456" ? "Billing API" : "Acme Dashboard", | ||
| slug: projectId === "proj_456" ? "billing-api" : "acme-dashboard", | ||
| workspace: { | ||
| id: "ws_123", | ||
| name: "Acme Inc", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (pathName === "/v1/projects/{projectId}/branches") { | ||
| const branchName = request?.params?.query?.gitName ?? "main"; | ||
| return { | ||
| data: { | ||
| data: options.branchExists === false ? [] : [branchRecord(branchName)], | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| throw new Error(`Unexpected path ${pathName}`); | ||
| }), | ||
| POST: vi.fn().mockImplementation((pathName: string, request?: { body?: { gitName?: string } }) => { | ||
| if (pathName === "/v1/projects/{projectId}/branches") { | ||
| const branchName = request?.body?.gitName ?? "main"; | ||
| return { | ||
| data: { | ||
| data: branchRecord(branchName), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| throw new Error(`Unexpected path ${pathName}`); | ||
| }), | ||
| createBranchDatabase: vi.fn(), | ||
| deleteBranchDatabase: vi.fn(), | ||
| listEnvironmentVariables: vi.fn().mockResolvedValue([]), | ||
| createEnvironmentVariable: vi.fn(), | ||
| updateEnvironmentVariable: vi.fn(), | ||
| deleteEnvironmentVariable: vi.fn(), | ||
| ...provider, |
There was a problem hiding this comment.
Return Promise-shaped defaults for async provider mocks.
Line 52 currently uses vi.fn() for an async contract. If that path is exercised, chaining .catch(...) can fail on undefined. Use resolved Promise defaults for the new async methods.
Suggested fix
function withBranchDatabaseProviderDefaults<T extends Record<string, unknown>>(provider: T) {
return {
createBranchDatabase: vi.fn(),
- deleteBranchDatabase: vi.fn(),
+ deleteBranchDatabase: vi.fn().mockResolvedValue(undefined),
listEnvironmentVariables: vi.fn().mockResolvedValue([]),
createEnvironmentVariable: vi.fn(),
updateEnvironmentVariable: vi.fn(),
- deleteEnvironmentVariable: vi.fn(),
+ deleteEnvironmentVariable: vi.fn().mockResolvedValue(undefined),
...provider,
};
}📝 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.
Suggested change
| function withBranchDatabaseProviderDefaults<T extends Record<string, unknown>>(provider: T) { | |
| return { | |
| token: "token", | |
| GET: vi.fn().mockImplementation((pathName: string, request?: { params?: { query?: { gitName?: string } } }) => { | |
| if (pathName === "/v1/projects") { | |
| return { | |
| data: { | |
| data: [ | |
| { | |
| id: projectId, | |
| name: projectId === "proj_456" ? "Billing API" : "Acme Dashboard", | |
| slug: projectId === "proj_456" ? "billing-api" : "acme-dashboard", | |
| workspace: { | |
| id: "ws_123", | |
| name: "Acme Inc", | |
| }, | |
| }, | |
| ], | |
| }, | |
| }; | |
| } | |
| if (pathName === "/v1/projects/{projectId}/branches") { | |
| const branchName = request?.params?.query?.gitName ?? "main"; | |
| return { | |
| data: { | |
| data: options.branchExists === false ? [] : [branchRecord(branchName)], | |
| }, | |
| }; | |
| } | |
| throw new Error(`Unexpected path ${pathName}`); | |
| }), | |
| POST: vi.fn().mockImplementation((pathName: string, request?: { body?: { gitName?: string } }) => { | |
| if (pathName === "/v1/projects/{projectId}/branches") { | |
| const branchName = request?.body?.gitName ?? "main"; | |
| return { | |
| data: { | |
| data: branchRecord(branchName), | |
| }, | |
| }; | |
| } | |
| throw new Error(`Unexpected path ${pathName}`); | |
| }), | |
| createBranchDatabase: vi.fn(), | |
| deleteBranchDatabase: vi.fn(), | |
| listEnvironmentVariables: vi.fn().mockResolvedValue([]), | |
| createEnvironmentVariable: vi.fn(), | |
| updateEnvironmentVariable: vi.fn(), | |
| deleteEnvironmentVariable: vi.fn(), | |
| ...provider, | |
| function withBranchDatabaseProviderDefaults<T extends Record<string, unknown>>(provider: T) { | |
| return { | |
| createBranchDatabase: vi.fn(), | |
| deleteBranchDatabase: vi.fn().mockResolvedValue(undefined), | |
| listEnvironmentVariables: vi.fn().mockResolvedValue([]), | |
| createEnvironmentVariable: vi.fn(), | |
| updateEnvironmentVariable: vi.fn(), | |
| deleteEnvironmentVariable: vi.fn().mockResolvedValue(undefined), | |
| ...provider, |
🤖 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 `@packages/cli/tests/app-controller.test.ts` around lines 45 - 53, The async
mock defaults in withBranchDatabaseProviderDefaults are plain vi.fn()s which can
return undefined when awaited; change createBranchDatabase,
deleteBranchDatabase, createEnvironmentVariable, updateEnvironmentVariable, and
deleteEnvironmentVariable to return resolved Promises by default (e.g.,
vi.fn().mockResolvedValue(undefined) or mockResolvedValue(sensible default)) so
any code awaiting or chaining .catch won't get undefined; keep
listEnvironmentVariables as is or adjust similarly if needed.
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.
Overview
This adds deploy-time branch database setup for preview app deployments, with a path that works for manual CLI use and CI. The CLI can create an isolated Prisma Postgres database, apply the local Prisma schema, and wire the usual branch-scoped env vars before the app deployment starts.
Changes
packages/cli/src/commands/app/index.tsexpandsapp deploywith--dband--no-db.--db --yesis the non-interactive CI path;--yesalone does not create database infrastructure.packages/cli/src/controllers/app.tsruns branch database setup beforedeployAppand keeps branch-id resolution scoped to the deploy flow.packages/cli/src/lib/app/branch-database-deploy.tsowns the deploy policy: production rejection, mutually exclusive flags, inlineDATABASE_URL/DIRECT_URLconflicts, interactive prompting, existing env checks, partial env repair, and structured setup errors.packages/cli/src/lib/app/branch-database.tsdetects local database intent fromschema.prismaandDATABASE_URLreferences, chooses schemas deterministically, and runsprisma migrate deployorprisma db push --skip-generatewith quiet/json-safe stdio handling.packages/cli/src/lib/app/preview-branch-database.tsandpackages/cli/src/lib/app/preview-provider.tsadd Management API helpers for creating branch databases and managing branch env vars through the existing project env model.packages/cli/src/types/app.tsandpackages/cli/src/presenters/app.tsadd deploy-result metadata and output for created databases, env vars, and schema setup.packages/cli/tests/app-branch-database.test.tscovers creation, prompting, inline-env rejection, fully wired env skip, partial env repair, staleDIRECT_URLcleanup, schema failure, and deterministic schema selection.docs/product/command-spec.md,docs/product/resource-model.md, anddocs/product/error-conventions.mddocument the new deploy flags, branch database resource shape, and setup errors.Why
DATABASE_URLandDIRECT_URL.--db --yes, which follows the existing deploy-confirmation pattern and avoids surprise infrastructure creation.