Feature: Add Optimistic Concurrency Control to Project Updates #122
Conversation
WalkthroughAdds optimistic locking to project updates via an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Action
participant Logic
participant PrismaDB
Client->>Action: updateProject(input {id, changes, lastUpdatedAt?})
Action->>Logic: call updateProject(input, userId)
alt lastUpdatedAt provided
Logic->>PrismaDB: updateMany(where: {id, updatedAt: lastUpdatedAt}, data: changes)
PrismaDB-->>Logic: {count: n}
alt n == 0
Logic-->>Action: return CONFLICT (stale/modified)
else n > 0
Logic->>PrismaDB: findUnique(where: {id})
PrismaDB-->>Logic: updated project record
Logic-->>Action: return Success(project)
end
else no lastUpdatedAt
Logic->>PrismaDB: update(where: {id}, data: changes)
PrismaDB-->>Logic: updated project record or throw P2025
alt throws P2025
Logic-->>Action: return CONFLICT (mapped)
else
Logic-->>Action: return Success(project)
end
end
Action-->>Client: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/projects/updateProject/action.ts (1)
31-36: PropagateerrorCodeinstead of non-existentcodefromResult
updateProjectResultis aResult<Project>, whose error variant exposeserroranderrorCode. UsingupdateProjectResult.codewill always beundefined, so you lose the specific error code fromupdateProject(e.g., CONFLICT) and fall back to the default inerror(...).You likely want:
- return error(updateProjectResult.error, updateProjectResult.code); + return error(updateProjectResult.error, updateProjectResult.errorCode);so optimistic-lock conflicts and other specific error codes are correctly propagated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/actions/projects/updateProject/action.ts(1 hunks)src/actions/projects/updateProject/logic.ts(1 hunks)src/actions/projects/updateProject/schema.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/actions/projects/updateProject/action.ts (1)
src/lib/result.ts (1)
error(38-44)
src/actions/projects/updateProject/logic.ts (3)
src/actions/projects/updateProject/schema.ts (1)
UpdateProjectInput(9-9)src/lib/result.ts (3)
Result(18-18)success(25-30)error(38-44)src/lib/prisma.ts (1)
prisma(14-18)
🔇 Additional comments (1)
src/actions/projects/updateProject/schema.ts (1)
5-6: Schema extension for optimistic locking looks good
lastUpdatedAtas an optional datetime string aligns with the optimistic-locking logic, and the title normalization is reasonable and backwards compatible. No issues from my side.
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)
src/actions/projects/updateProject/logic.test.ts(1 hunks)src/actions/projects/updateProject/logic.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/actions/projects/updateProject/logic.ts (3)
src/actions/projects/updateProject/schema.ts (1)
UpdateProjectInput(9-9)src/lib/result.ts (3)
Result(18-18)error(38-44)success(25-30)src/lib/prisma.ts (1)
prisma(14-18)
src/actions/projects/updateProject/logic.test.ts (2)
src/lib/prisma.ts (1)
prisma(14-18)src/actions/projects/updateProject/logic.ts (1)
updateProject(10-60)
🔇 Additional comments (10)
src/actions/projects/updateProject/logic.test.ts (6)
1-16: LGTM! Clean test setup.The imports and Jest mock configuration properly cover all Prisma methods used in the implementation (
update,updateMany,findUnique).
18-29: LGTM! Proper test setup.The mock project data is well-structured, and
beforeEachensures test isolation by clearing mocks.
31-47: LGTM! Validates backwards compatibility.This test correctly verifies the standard update path when
lastUpdatedAtis not provided, ensuring existing behavior is preserved.
49-66: LGTM! Proper error handling test.This test correctly validates that Prisma's P2025 error (record not found) is mapped to a CONFLICT error with an appropriate user-friendly message.
68-92: LGTM! Validates optimistic locking success path.This test correctly verifies the optimistic locking mechanism when
lastUpdatedAtmatches, including the two-step process (updateMany+findUnique) and the updated timestamp.
116-124: LGTM! Validates error propagation.This test correctly ensures that unexpected errors (non-P2025) are re-thrown rather than being silently handled, which is important for debugging and error monitoring.
src/actions/projects/updateProject/logic.ts (4)
8-8: LGTM! Required import for error handling.The import of
errorandErrorCodesis necessary for the new error handling logic in the optimistic locking implementation.
11-12: LGTM! Properly extracts the new parameter.Correctly destructures the optional
lastUpdatedAtparameter from the input for use in the optimistic locking logic.
13-48: LGTM! Well-implemented optimistic locking with proper fallback.The implementation correctly:
- Uses
updateManywith bothidandupdatedAtfor optimistic locking (avoiding the past review's type-checking issue)- Checks
count === 0to detect conflicts- Follows up with
findUniqueto retrieve the updated record (necessary sinceupdateManydoesn't return it)- Includes a defensive null check for the unlikely race condition scenario
- Preserves backwards compatibility with the standard
updatepath whenlastUpdatedAtis omittedThe two-database-call approach in the optimistic path is necessary and well-documented.
51-59: LGTM! Proper error handling for not-found scenarios.The catch block correctly:
- Maps Prisma's P2025 error (record not found) to a user-friendly CONFLICT message
- Handles the case where the project ID doesn't exist in the standard update path
- Re-throws unexpected errors to ensure they're properly surfaced for debugging and monitoring
| it('should return CONFLICT error if lastUpdatedAt does not match (optimistic locking)', async () => { | ||
| (prisma.project.updateMany as jest.Mock).mockResolvedValue({ count: 0 }); | ||
|
|
||
| const input = { id: 'project-123', title: 'New Title', lastUpdatedAt: new Date('2025-01-01T09:00:00.000Z').toISOString() }; | ||
| const result = await updateProject(input); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| if (!result.success) { | ||
| expect(result.error).toBe('Project was modified by another user. Please refresh and try again.'); | ||
| expect(result.errorCode).toBe(ErrorCodes.CONFLICT); | ||
| } | ||
| expect(prisma.project.updateMany).toHaveBeenCalledWith({ | ||
| where: { | ||
| id: 'project-100', | ||
| updatedAt: new Date('2025-01-01T09:00:00.000Z'), | ||
| }, | ||
| data: { title: 'New Title' }, | ||
| }); | ||
| expect(prisma.project.findUnique).not.toHaveBeenCalled(); | ||
| expect(prisma.project.update).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Fix the project ID mismatch in the test assertion.
The test input uses id: 'project-123' (line 97), but the assertion checks for id: 'project-100' (line 107). This inconsistency could cause the test to validate incorrect behavior.
Apply this diff to fix the mismatch:
expect(prisma.project.updateMany).toHaveBeenCalledWith({
where: {
- id: 'project-100',
+ id: 'project-123',
updatedAt: new Date('2025-01-01T09:00:00.000Z'),
},
data: { title: 'New Title' },
});📝 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.
| it('should return CONFLICT error if lastUpdatedAt does not match (optimistic locking)', async () => { | |
| (prisma.project.updateMany as jest.Mock).mockResolvedValue({ count: 0 }); | |
| const input = { id: 'project-123', title: 'New Title', lastUpdatedAt: new Date('2025-01-01T09:00:00.000Z').toISOString() }; | |
| const result = await updateProject(input); | |
| expect(result.success).toBe(false); | |
| if (!result.success) { | |
| expect(result.error).toBe('Project was modified by another user. Please refresh and try again.'); | |
| expect(result.errorCode).toBe(ErrorCodes.CONFLICT); | |
| } | |
| expect(prisma.project.updateMany).toHaveBeenCalledWith({ | |
| where: { | |
| id: 'project-100', | |
| updatedAt: new Date('2025-01-01T09:00:00.000Z'), | |
| }, | |
| data: { title: 'New Title' }, | |
| }); | |
| expect(prisma.project.findUnique).not.toHaveBeenCalled(); | |
| expect(prisma.project.update).not.toHaveBeenCalled(); | |
| }); | |
| it('should return CONFLICT error if lastUpdatedAt does not match (optimistic locking)', async () => { | |
| (prisma.project.updateMany as jest.Mock).mockResolvedValue({ count: 0 }); | |
| const input = { id: 'project-123', title: 'New Title', lastUpdatedAt: new Date('2025-01-01T09:00:00.000Z').toISOString() }; | |
| const result = await updateProject(input); | |
| expect(result.success).toBe(false); | |
| if (!result.success) { | |
| expect(result.error).toBe('Project was modified by another user. Please refresh and try again.'); | |
| expect(result.errorCode).toBe(ErrorCodes.CONFLICT); | |
| } | |
| expect(prisma.project.updateMany).toHaveBeenCalledWith({ | |
| where: { | |
| id: 'project-123', | |
| updatedAt: new Date('2025-01-01T09:00:00.000Z'), | |
| }, | |
| data: { title: 'New Title' }, | |
| }); | |
| expect(prisma.project.findUnique).not.toHaveBeenCalled(); | |
| expect(prisma.project.update).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In src/actions/projects/updateProject/logic.test.ts around lines 94 to 114, the
test input uses id: 'project-123' but the updateMany assertion checks for id:
'project-100'; update the assertion to use id: 'project-123' so the mocked
prisma.project.updateMany call expectation matches the test input (leave the
updatedAt and data checks unchanged).
|
the optimistic locking addition in |
Overview
This PR introduces an optimistic concurrency control mechanism to project updates, helping to prevent accidental overwrites when multiple users might be editing simultaneously.
Changes
lastUpdatedAttimestamp to theupdateProjectaction insrc/actions/projects/updateProject/action.ts.lastUpdatedAtis provided, the update will only succeed if the project's currentupdatedAtfield matches the provided timestamp.lastUpdatedAtis not provided, the action defaults to the existing update behavior without the optimistic check.Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.