feat(substitution): add conflict check for teacher substitutions#239
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds teacher-substitution period-overlap validation to ChangesTeacher Substitution Conflict Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@apps/chronos/src/routes/timetable/substitution.ts`:
- Line 738: The call to cancelPendingNotification(id, 'substitution') is
executed before the transaction begins, which means if the transaction fails
(due to conflict detection or database errors), the notification is already
cancelled but the substitution remains unchanged, creating an inconsistent
state. Move the cancelPendingNotification call to execute after the transaction
completes successfully, either by placing it after the transaction block or
within the transaction after all write operations are confirmed.
- Around line 774-779: The dispatchPendingNotification call for substitution
uses body.lessonIds ?? [] which defaults to an empty array when lessonIds is not
provided in the update, but validateUpdateTeacherConflict correctly uses
body.lessonIds ?? existingLessonIds to preserve the actual lesson IDs. Update
the notification dispatch to use the same fallback logic as the validation
function, either by fetching the existing lesson IDs separately or by
refactoring validateUpdateTeacherConflict to return the existingLessonRecords so
it can be reused in the notification payload, ensuring the notification includes
the actual associated lessons instead of an empty array.
- Around line 754-766: The condition checking `if (body.lessonIds)` evaluates to
true even when body.lessonIds is an empty array, since empty arrays are truthy
in JavaScript. This causes the code to delete existing substitutionLessonMTM
relationships and then attempt to insert an empty values array, which throws an
error. Fix this by modifying the condition to check both that body.lessonIds
exists and that it has at least one element, such as changing the condition to
check body.lessonIds?.length or body.lessonIds && body.lessonIds.length > 0,
ensuring the insert operation only runs when there are actual lessons to insert.
🪄 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 Plus
Run ID: f9b76eca-6192-4f49-a0d7-75697ba52e95
📒 Files selected for processing (1)
apps/chronos/src/routes/timetable/substitution.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: typecheck
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/chronos/src/routes/timetable/substitution.ts (4)
3-3: LGTM!
215-329: LGTM!
610-650: LGTM!
714-720: LGTM!
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)
apps/chronos/src/routes/timetable/substitution.ts (1)
562-567: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire at least one lesson in create schema to prevent empty insert.
The schema allows empty
lessonIdsarrays, buttx.insert(substitutionLessonMTM).values([])at line 645 will fail with an empty array. Adding.min(1)provides a clear API contract and early validation.Proposed fix
const createSchema = createInsertSchema(substitution) .omit({ id: true }) .extend({ date: z.coerce.date<Date>(), - lessonIds: z.string().array(), + lessonIds: z.string().array().min(1, 'At least one lesson is required'), });Also applies to: 639-645
🤖 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 `@apps/chronos/src/routes/timetable/substitution.ts` around lines 562 - 567, The lessonIds field in the createSchema validation allows empty arrays, which will cause the tx.insert(substitutionLessonMTM).values([]) call to fail. Add .min(1) constraint to the lessonIds field definition to require at least one lesson ID in the array, ensuring early validation and providing a clear API contract that substitutions must include at least one lesson.
🤖 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 `@apps/chronos/src/routes/timetable/substitution.ts`:
- Around line 788-792: The nullish coalescing operator `??` in the substituter
field of the dispatchPendingNotification call treats both null and undefined
identically. When a user explicitly sets substituter to null to cancel a
substitution, the expression `body.substituter ?? existing.substituter` will
incorrectly fall back to the existing substituter value instead of respecting
the explicit cancellation. Fix this by checking whether substituter is
explicitly present in the body object (using hasOwnProperty or the in operator)
to distinguish between an explicitly provided null value and an
undefined/unprovided value, then use the explicit null when present or the
existing value when not.
---
Outside diff comments:
In `@apps/chronos/src/routes/timetable/substitution.ts`:
- Around line 562-567: The lessonIds field in the createSchema validation allows
empty arrays, which will cause the tx.insert(substitutionLessonMTM).values([])
call to fail. Add .min(1) constraint to the lessonIds field definition to
require at least one lesson ID in the array, ensuring early validation and
providing a clear API contract that substitutions must include at least one
lesson.
🪄 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 Plus
Run ID: c9250ee3-b041-4b61-ad6d-dd8ee140b78e
📒 Files selected for processing (1)
apps/chronos/src/routes/timetable/substitution.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/chronos/src/routes/timetable/substitution.ts (6)
3-3: LGTM!
300-328: LGTM!
714-736: LGTM!
738-771: LGTM!
773-786: LGTM!
220-236: 🩺 Stability & AvailabilityThe proposed guard is unnecessary for the current Drizzle version.
The project uses
drizzle-ormversion 0.45.2, which safely handles empty arrays ininArray. Since Drizzle 0.32.1, empty arrays are handled gracefully—inArray(column, [])returnsfalserather than causing a SQL error. The code works correctly without the guard.An early return when
lessonIdsis empty could be added for clarity and to avoid an unnecessary database query, but it is not required for correctness or stability.> Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements