Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
TL;DR — Fixes project deletion to clean up all branches and runtime entities, not just the main branch. Previously, deleting a project left orphaned Doltgres branches and branch-scoped runtime rows behind. Key changes
Summary ｜ 6 files ｜ 5 commits ｜ base: Delete all Doltgres branches for a project
Partial failures are tracked in a
Remove branch scoping from runtime cascade delete
This affects deletes on
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good overall. The change to delete all branches and runtime entities across all branches when deleting a project is a sensible fix — branch-scoped deletion left orphaned data. Two stale comment/doc issues to clean up.
Claude Opus ｜ 𝕏
| fullBranchName: string; | ||
| }): Promise<CascadeDeleteResult> => { | ||
| const { scopes, fullBranchName } = params; | ||
| async (params: { scopes: ProjectScopeConfig }): Promise<CascadeDeleteResult> => { |
There was a problem hiding this comment.
The JSDoc above (lines 172–174) still says "Delete all runtime entities for a project on a specific branch" and "Used when deleting a project from a branch." These are stale after removing the fullBranchName parameter — update to reflect cross-branch deletion.
| import { | ||
| doltBranch, | ||
| doltBranchExists, | ||
| doltCheckout, | ||
| doltDeleteBranch, | ||
| doltListBranches, | ||
| } from '../../dolt/branch'; |
There was a problem hiding this comment.
The JSDoc for deleteProjectWithBranch (lines 108–113 in the original) still says "Delete a project and its branch" (singular) and step 1 says "Gets the project from runtime DB to find the branch name." These should be updated to reflect the new all-branches behavior. The function name itself (deleteProjectWithBranch) is also now slightly misleading — consider renaming to deleteProjectWithBranches if you want to keep it accurate, though that's optional.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
🔴 1) projectLifecycle.test.ts Missing doltListBranches mock will break tests
Issue: The unit test mock at lines 12-17 does not include doltListBranches, but the updated deleteProjectWithBranch implementation now calls doltListBranches(configDb)(). This will cause all deleteProjectWithBranch tests to fail with an error like "doltListBranches is not a function".
Why: All existing tests in the deleteProjectWithBranch describe block will fail immediately when run against the updated code. This is a blocking regression that prevents the test suite from passing.
Fix: Add doltListBranches to the mock factory:
// In the vi.mock block (line 12-17):
vi.mock('../../dolt/branch', () => ({
doltBranch: vi.fn(),
doltBranchExists: vi.fn(),
doltCheckout: vi.fn(),
doltDeleteBranch: vi.fn(),
doltListBranches: vi.fn(), // ADD THIS
}));
// Import and type it (after line 20):
import { doltListBranches } from '../../dolt/branch';
const mockedDoltListBranches = doltListBranches as ReturnType<typeof vi.fn>;
// In each deleteProjectWithBranch test, add mock setup:
const mockDoltListBranchesFn = vi.fn().mockResolvedValue([
{ name: `${testTenantId}_delete-lifecycle-project_main` }
]);
mockedDoltListBranches.mockReturnValue(mockDoltListBranchesFn);Refs:
Inline Comments:
- 🔴 Critical:
cascade-delete.ts:172-178Stale docstring contradicts function behavior
🟠⚠️ Major (2) 🟠⚠️
🟠 1) projectLifecycle.ts:107-119 Docstring describes single-branch deletion but function now deletes all branches
Issue: The docstring says "Gets the project from runtime DB to find the branch name" (singular) and "Deletes the project branch from config DB" (singular), but the implementation now lists ALL branches matching the project prefix and deletes each one.
Why: A maintainer reading the docstring would expect this function to only delete the main branch. They might incorrectly think they need additional code to clean up feature branches.
Fix: Update the docstring to reflect the new behavior:
/**
* Delete a project and ALL its branches
*
* This utility:
* 1. Gets the project from runtime DB
* 2. Lists and deletes ALL branches matching the project prefix from config DB (Doltgres)
* 3. Deletes the project record from runtime DB
*
* Note: Callers should handle cascade deletion of runtime entities (conversations, etc.)
* before calling this function.
*/Refs:
🟠 2) projectLifecycle.ts:146-157 Partial branch deletion failures silently continue with misleading success log
Issue: The inner try/catch catches individual branch deletion failures and logs at ERROR level, but then the function logs "Deleted all project branches" at line 154 even when some branches failed to delete. This creates a misleading success state.
Why: When a branch deletion fails mid-loop, orphaned branches persist in Doltgres. The success log doesn't reflect that cleanup was partial. An engineer debugging later would see conflicting signals.
Fix: Track failed branches and adjust the summary log:
const failedBranches: string[] = [];
for (const branch of projectBranches) {
try {
await doltDeleteBranch(configDb)({ name: branch.name, force: true });
logger.debug({ branchName: branch.name }, 'Deleted project branch');
} catch (error) {
failedBranches.push(branch.name);
logger.error(
{ error, branchName: branch.name },
'Failed to delete project branch, continuing with remaining branches'
);
}
}
if (failedBranches.length > 0) {
logger.warn(
{ tenantId, projectId, failedBranches, successCount: projectBranches.length - failedBranches.length },
'Some project branches could not be deleted'
);
} else {
logger.info(
{ tenantId, projectId, branchCount: projectBranches.length },
'Deleted all project branches'
);
}Refs:
Inline Comments:
- 🟠 Major:
cascade-delete.ts:185Stale inline comment - 🟠 Major:
cascade-delete.ts:191Stale inline comment - 🟠 Major:
cascade-delete.ts:197Stale inline comment - 🟠 Major:
cascade-delete.ts:200Stale inline comment - 🟠 Major:
cascade-delete.ts:221Stale inline comment - 🟠 Major:
cascade-delete.ts:229Stale inline comment
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
cascadeDelete.test.ts:358Stale test comment
🚫 REQUEST CHANGES
Summary: The core logic change (deleting across all branches instead of a single branch) is correct and the test assertions have been updated appropriately. However, the projectLifecycle.test.ts mock is missing doltListBranches which will cause test failures. Additionally, multiple comments and docstrings throughout the code still reference the old "single branch" behavior and need to be updated to avoid confusing future maintainers. Please add the missing mock and update the stale documentation before merging.
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-comments |
9 | 2 | 0 | 0 | 7 | 0 | 0 |
pr-review-tests |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-errors |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-standards |
7 | 0 | 0 | 0 | 0 | 0 | 7 |
| Total | 23 | 4 | 0 | 0 | 8 | 0 | 11 |
Note: Standards reviewer findings were duplicates of comments reviewer findings and were discarded. Error reviewer's minor findings about return types were assessed as low priority and discarded.
| async (params: { scopes: ProjectScopeConfig }): Promise<CascadeDeleteResult> => { | ||
| const { scopes } = params; | ||
|
|
||
| // Delete contextCache for this project on this branch |
There was a problem hiding this comment.
🟠 MAJOR: Stale inline comment references branch filtering
Issue: Comment says "on this branch" but code no longer filters by branch.
Fix: (1-click apply)
| // Delete contextCache for this project on this branch | |
| // Delete contextCache for this project (across all branches) |
| .where(projectScopedWhere(contextCache, scopes)) | ||
| .returning(); | ||
|
|
||
| // Delete conversations for this project on this branch (cascades to messages) |
There was a problem hiding this comment.
🟠 MAJOR: Stale inline comment
Fix: (1-click apply)
| // Delete conversations for this project on this branch (cascades to messages) | |
| // Delete conversations for this project (cascades to messages) |
| .where(projectScopedWhere(conversations, scopes)) | ||
| .returning(); | ||
|
|
||
| // Delete tasks for this project on this branch (cascades to ledgerArtifacts, taskRelations) |
There was a problem hiding this comment.
🟠 MAJOR: Stale inline comment
Fix: (1-click apply)
| // Delete tasks for this project on this branch (cascades to ledgerArtifacts, taskRelations) | |
| // Delete tasks for this project (cascades to ledgerArtifacts, taskRelations) |
| .returning(); | ||
| const tasksResult = await db.delete(tasks).where(projectScopedWhere(tasks, scopes)).returning(); | ||
|
|
||
| // Delete trigger invocations for this project on this branch |
There was a problem hiding this comment.
🟠 MAJOR: Stale inline comment
Fix: (1-click apply)
| // Delete trigger invocations for this project on this branch | |
| // Delete trigger invocations for this project |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ All Previous Issues Addressed
The commit 74e0588a4 successfully addresses all 4 issues (1 Critical, 2 Major, 1 Minor) raised in the previous review:
| # | Severity | Issue | Resolution |
|---|---|---|---|
| 1 | 🔴 Critical | Missing doltListBranches mock in tests |
✅ Added mock, import, and test setup |
| 2 | 🔴 Critical | Stale docstring in cascade-delete.ts |
✅ Updated to "across all branches" |
| 3 | 🟠 Major | Stale docstring in projectLifecycle.ts |
✅ Updated to describe all-branches deletion |
| 4 | 🟠 Major | Misleading success log on partial failures | ✅ Now tracks and logs failedBranches separately |
| 5-10 | 🟠 Major | Stale inline comments in cascade-delete.ts |
✅ All updated to remove branch-specific wording |
| 11 | 🟡 Minor | Stale test comment | ✅ Updated to reflect project-wide cascade |
Delta Review Summary
The delta since the last review consists of a single commit that updates comments and documentation to correctly reflect the new behavior where project deletion:
- Deletes all Doltgres branches matching the
{tenantId}_{projectId}_prefix - Deletes all runtime entities across all branches (not just the targeted branch)
The code changes are documentation/comment-only and correctly align the prose with the previously-reviewed implementation.
✅ APPROVE
Summary: All previously raised issues have been addressed. The PR now correctly documents that project deletion cleans up all branches and runtime entities across all branches. Good work on the follow-up fixes! 🎉
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Previous Approval Stands
This re-review was triggered by a merge commit (362b208 — merging main into fix/delete-branch).
Delta analysis:
- Commits since last review: 1 (merge commit only)
- PR-specific files changed: 0 — all 6 files are identical to when approved
- Merge conflicts: None
The merge simply brings this branch up to date with main. No new code review is needed.
Prior Review Reference
The PR was approved on 2026-04-02 after all 4 issues (1 Critical, 2 Major, 1 Minor) from the initial review were addressed:
| Resolution | Issue |
|---|---|
| ✅ | Missing doltListBranches mock in tests |
| ✅ | Stale docstring in cascade-delete.ts |
| ✅ | Stale docstring in projectLifecycle.ts |
| ✅ | Misleading success log on partial failures |
| ✅ | Stale inline comments (6 locations) |
✅ APPROVE
Summary: Merge from main only — no PR changes since prior approval. Ship it! 🚀
1 participant
Fixes project deletion to clean up all branches and runtime entities, not just the main branch. Previously, deleting a project left orphaned Doltgres branches and branch-scoped runtime rows behind.