Fix: Improve project deletion robustness #121
Conversation
…ns and ensure related project members are deleted, add `onDelete: Cascade`
WalkthroughReplaces a direct project delete with a try/catch-wrapped deleteMany call in the action logic and adds error handling/logging; updates Prisma schema to set Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
src/actions/projects/deleteProject/logic.ts (2)
10-12: Consider verifying that the project was actually deleted.Using
deleteManyinstead ofdeletewon't throw an error if the project doesn't exist—it will return{count: 0}. While this provides idempotency (safe to retry), it also means the function returnssuccess(undefined)even when no project was deleted, potentially masking bugs where invalid IDs are passed.Consider checking the deletion count:
- await prisma.$transaction(async (tx) => { - await tx.project.deleteMany({ - where: { id: input.id } - }); - }); + const result = await prisma.project.deleteMany({ + where: { id: input.id } + }); + + if (result.count === 0) { + return err("Project not found.", ErrorCodes.NOT_FOUND); + } + return success(undefined);Note: This also removes the unnecessary transaction.
15-18: Consider providing more specific error information.The generic error message and missing error code make it difficult for callers to handle different failure scenarios appropriately. For instance, foreign key constraint violations, permission errors, and network failures would all return the same message.
If distinguishing error types isn't needed here, you could at least pass an error code:
} catch (error) { console.error("Failed to delete project:", error); - return err("Failed to delete project due to an unexpected error."); + return err("Failed to delete project due to an unexpected error.", ErrorCodes.UNKNOWN_ERROR); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/actions/projects/deleteProject/logic.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/actions/projects/deleteProject/logic.ts (3)
src/actions/projects/deleteProject/schema.ts (1)
DeleteProjectInput(7-7)src/lib/result.ts (3)
Result(18-18)success(25-30)error(38-44)src/lib/prisma.ts (1)
prisma(14-18)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
prisma/schema.prisma(1 hunks)src/actions/projects/deleteProject/logic.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/actions/projects/deleteProject/logic.ts (3)
src/actions/projects/deleteProject/schema.ts (1)
DeleteProjectInput(7-7)src/lib/result.ts (3)
Result(18-18)success(25-30)error(38-44)src/lib/prisma.ts (1)
prisma(14-18)
🔇 Additional comments (2)
src/actions/projects/deleteProject/logic.ts (2)
4-4: Import addition looks good.The addition of
errto the imports is correct and necessary for the error handling below.
8-16: Error handling is solid, but PR description claims transaction that isn't present.The try/catch pattern with error logging is appropriate. Using
deleteManyinstead ofdeleteis unusual for a single record but acceptable—it won't throw if the project doesn't exist, which aligns with the PR's goal of graceful handling.However, the PR objectives state: "deletion of a project and its related data is wrapped in a database transaction to ensure atomicity." No transaction wrapper is present in this code. A single
deleteManyoperation is already atomic, so a transaction isn't technically needed here, but the PR description should be updated to reflect the actual implementation.With the schema change adding
onDelete: Cascade, related ProjectMember records will be automatically deleted by the database when a project is deleted, which is the correct approach.Minor: Consider whether
deleteManyvsdeleteis intentional.If the goal is to fail when the project doesn't exist (stricter validation), use
delete. If the goal is to succeed silently when the project is already gone (idempotent deletion),deleteManyis appropriate. The current choice seems intentional for robustness.
| userId String | ||
| role Role @default(USER) | ||
| project Project @relation(fields: [projectId], references: [id]) | ||
| project Project @relation(fields: [projectId], references: [id], onDelete: Cascade) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual schema.prisma file at line 61 to confirm the change
sed -n '55,65p' prisma/schema.prismaRepository: Authormaton/authormaton
Length of output: 396
🏁 Script executed:
# Check if prisma/migrations directory exists and list recent migrations
fd -type d -name migrations prisma/ && fd -e sql . prisma/migrations 2>/dev/null | tail -10 || echo "No migrations found"Repository: Authormaton/authormaton
Length of output: 274
🏁 Script executed:
# Search for any migration containing CASCADE delete
fd -e sql . prisma/migrations 2>/dev/null -x grep -l "ON DELETE CASCADE" {} \; || echo "No migrations with CASCADE found"Repository: Authormaton/authormaton
Length of output: 99
Migration file is missing for the cascade delete schema change.
The onDelete: Cascade configuration has been added to the ProjectMember.project relation in the schema, but no corresponding Prisma migration file exists in prisma/migrations/. Without generating and applying the migration, the database schema remains unchanged and will not enforce cascade deletes. This means deleting a project will still cause foreign key constraint violations at the database level.
Run prisma migrate dev --name add_cascade_delete_projectmember to generate the migration, then commit both the schema and migration files.
🤖 Prompt for AI Agents
In prisma/schema.prisma around line 61, you added onDelete: Cascade to the
ProjectMember.project relation but did not create/apply the corresponding Prisma
migration; run prisma migrate dev --name add_cascade_delete_projectmember from
the project root to generate and apply the migration, then commit the updated
prisma/schema.prisma plus the new timestamped folder under prisma/migrations
(include migration SQL and schema file) to ensure the DB enforces cascade
deletes.
|
removing the dependency checks in deletion logic simplifies the process nicely; approved. |
Overview: This PR enhances the project deletion logic to handle cases where related records might be missing and wraps the deletion in a safe transaction.
Changes
src/actions/projects/deleteProject/logic.tsto gracefully handle scenarios where dependent records (e.g., members, analytics) might not exist.logic.tsand remain easily testable.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.