Conversation
wesleylui
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
jason-duong4509,
promatty,
tanvimahal and
themaxboucher
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a vehicles feature (DB schema, router, seeding, hook, admin UI), integrates vehicle selection into surveys and vehicle logs, implements transactional survey creation that can auto-insert a driver log, and expands vehicle-logs API with filtering plus create/update/delete mutations. Changes
Sequence DiagramsequenceDiagram
actor Driver
participant Client as Client (Browser)
participant SurveyAPI as Survey API
participant DB as Database
Driver->>Client: submit trip survey (includes vehicle)
Client->>SurveyAPI: call create survey
SurveyAPI->>DB: BEGIN transaction
SurveyAPI->>DB: INSERT postTripSurveys (with vehicle)
DB-->>SurveyAPI: survey row
alt trip not CANCELLED
SurveyAPI->>DB: lookup assigned driver (user)
DB-->>SurveyAPI: driver record
SurveyAPI->>DB: INSERT logs row (auto-created from survey + booking)
DB-->>SurveyAPI: log row
end
SurveyAPI->>DB: UPDATE bookings SET surveyCompleted = true
DB-->>SurveyAPI: booking updated
SurveyAPI->>DB: COMMIT
DB-->>SurveyAPI: transaction complete
SurveyAPI-->>Client: return survey (and log)
Client->>Client: invalidate caches / show notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/admin/vehicle-logs/page.tsx (1)
74-83:⚠️ Potential issue | 🟠 MajorOdometer validation/submission is inconsistent with backend constraints.
Frontend permits equal values and silently rounds decimals, while backend enforces strict integer
odometerEnd > odometerStart. This can pass client validation but still fail mutation (or mutate values unexpectedly).💡 Suggested alignment with backend rules
odometerStart: (value) => { if (value.trim().length === 0) return "Odometer start is required"; const num = Number.parseFloat(value); if (Number.isNaN(num)) return "Must be a valid number"; - if (num < 0) return "Must be a positive number"; + if (num < 0) return "Must be a non-negative number"; + if (!Number.isInteger(num)) return "Must be a whole number"; return null; }, odometerEnd: (value, values) => { if (value.trim().length === 0) return "Odometer end is required"; const num = Number.parseFloat(value); if (Number.isNaN(num)) return "Must be a valid number"; - if (num < 0) return "Must be a positive number"; + if (num < 0) return "Must be a non-negative number"; + if (!Number.isInteger(num)) return "Must be a whole number"; const start = Number.parseFloat(values.odometerStart); - if (!Number.isNaN(start) && num < start) { - return "End reading must be greater than or equal to start reading"; + if (!Number.isNaN(start) && num <= start) { + return "End reading must be greater than start reading"; } return null; }, @@ - const odometerStart = Math.round(Number.parseFloat(values.odometerStart)); - const odometerEnd = Math.round(Number.parseFloat(values.odometerEnd)); + const odometerStart = Number(values.odometerStart); + const odometerEnd = Number(values.odometerEnd);Also applies to: 122-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/admin/vehicle-logs/page.tsx` around lines 74 - 83, The odometerEnd validation in page.tsx currently allows equal values and accepts decimals which frontend may round, causing backend failures; update the odometerEnd validator to require an integer (no decimals) and enforce strictly odometerEnd > odometerStart (not >=), and mirror the same integer constraint in the odometerStart validation so both client-side checks match the backend rule; ensure submission/parsing uses integer parsing (or rejects non-integer input) instead of implicit float rounding so the mutation receives the exact integer values the backend expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/admin/vehicle-logs/page.tsx`:
- Line 18: The VehicleLogsPage component should be moved from the current route
to the canonical route: create a new page at /admin/driver-logs with the same
implementation (or move the file and adjust path), rename the component/export
to DriverLogsPage (or keep default export but update references) and update any
imports/usages and the navbar link to point to /admin/driver-logs; ensure there
are no remaining placeholders at /admin/driver-logs and remove or redirect the
old /admin/vehicle-logs route so the feature lives only under the canonical path
(update route references in route tables, navigation components, and tests
accordingly).
In `@src/server/api/routers/vehicle-logs.ts`:
- Around line 139-157: The update schema's .refine checks for odometer and time
pairs allow single-field updates (e.g., only odometerEnd or arrivalTime) to
bypass validation; modify the same refine logic in the update input schema so
that if one of a pair is provided the other must also be provided and then
enforce the ordering (for odometer: require odometerStart when odometerEnd is
present and check odometerEnd > odometerStart; for times: require departureTime
when arrivalTime is present and check new Date(arrivalTime) > new
Date(departureTime)); update the two .refine blocks that reference
odometerEnd/odometerStart and arrivalTime/departureTime to implement these
presence+ordering guards so partial single-field edits no longer bypass
validation.
- Around line 15-16: The dateFrom and dateTo fields in the input Zod schema
accept any string and must be validated to avoid DB errors; update the Zod
schema (the schema where dateFrom and dateTo are defined) to use a stricter
validator such as z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional().refine(v =>
!v || !Number.isNaN(Date.parse(v)), { message: "Invalid ISO date (YYYY-MM-DD)"
}) so malformed values are rejected before SQL is built, then convert the
validated strings to Date (or normalized ISO) when you build SQL predicates;
also add a cross-field check to ensure dateFrom <= dateTo and apply the same
validation change to the other identical schema block referenced (lines 29-34).
---
Outside diff comments:
In `@src/app/admin/vehicle-logs/page.tsx`:
- Around line 74-83: The odometerEnd validation in page.tsx currently allows
equal values and accepts decimals which frontend may round, causing backend
failures; update the odometerEnd validator to require an integer (no decimals)
and enforce strictly odometerEnd > odometerStart (not >=), and mirror the same
integer constraint in the odometerStart validation so both client-side checks
match the backend rule; ensure submission/parsing uses integer parsing (or
rejects non-integer input) instead of implicit float rounding so the mutation
receives the exact integer values the backend expects.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/_components/vehiclelogcomponents/vehicle-log-form.tsxsrc/app/_components/vehiclelogcomponents/vehicle-log-table-view.module.scsssrc/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsxsrc/app/admin/vehicle-logs/page.tsxsrc/server/api/routers/vehicle-logs.ts
| import { notify } from "@/lib/notifications"; | ||
| import { api } from "@/trpc/react"; | ||
|
|
||
| export default function VehicleLogsPage() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify canonical admin route usage for driver/vehicle logs
fd -t f 'navbar.tsx|page.tsx' src/app | head -20
rg -n '/admin/(driver-logs|vehicle-logs)' src/app src/lib --type tsx --type tsRepository: Code-the-Change-YYC/salvationarmy
Length of output: 696
🏁 Script executed:
#!/bin/bash
# Search for route patterns without file type flags
rg -n '/admin/(driver-logs|vehicle-logs)' src/app src/lib
# Also check navbar for navigation links
rg -n 'driver-logs|vehicle-logs' src/app/_components/common/navbar.tsx -A 2 -B 2Repository: Code-the-Change-YYC/salvationarmy
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Check the content of both page files to understand their purpose
echo "=== /admin/driver-logs/page.tsx ==="
cat src/app/admin/driver-logs/page.tsx
echo -e "\n=== /admin/vehicle-logs/page.tsx ==="
cat src/app/admin/vehicle-logs/page.tsxRepository: Code-the-Change-YYC/salvationarmy
Length of output: 6723
Move this page to /admin/driver-logs to match the canonical route for this project.
The fully functional vehicle logs page is currently at /admin/vehicle-logs, but the canonical route for this feature is /admin/driver-logs. The navbar should link to /admin/driver-logs, and the placeholder at that location should be replaced with the actual implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/admin/vehicle-logs/page.tsx` at line 18, The VehicleLogsPage
component should be moved from the current route to the canonical route: create
a new page at /admin/driver-logs with the same implementation (or move the file
and adjust path), rename the component/export to DriverLogsPage (or keep default
export but update references) and update any imports/usages and the navbar link
to point to /admin/driver-logs; ensure there are no remaining placeholders at
/admin/driver-logs and remove or redirect the old /admin/vehicle-logs route so
the feature lives only under the canonical path (update route references in
route tables, navigation components, and tests accordingly).
Comment on lines
+15
to
+16
| dateFrom: z.string().optional(), // ISO date string "YYYY-MM-DD" | ||
| dateTo: z.string().optional(), // ISO date string "YYYY-MM-DD" |
There was a problem hiding this comment.
Validate dateFrom / dateTo format before building SQL predicates.
These fields currently accept any string. Malformed values can surface as DB errors instead of clean input errors.
💡 Suggested hardening
getAll: adminProcedure
.input(
z
.object({
vehicle: z.string().optional(),
driverName: z.string().optional(),
- dateFrom: z.string().optional(), // ISO date string "YYYY-MM-DD"
- dateTo: z.string().optional(), // ISO date string "YYYY-MM-DD"
+ dateFrom: z
+ .string()
+ .regex(/^\d{4}-\d{2}-\d{2}$/, "dateFrom must be YYYY-MM-DD")
+ .optional(),
+ dateTo: z
+ .string()
+ .regex(/^\d{4}-\d{2}-\d{2}$/, "dateTo must be YYYY-MM-DD")
+ .optional(),
})
+ .refine((v) => !v.dateFrom || !v.dateTo || v.dateFrom <= v.dateTo, {
+ message: "dateFrom must be on or before dateTo",
+ path: ["dateTo"],
+ })
.optional(),
)Also applies to: 29-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/vehicle-logs.ts` around lines 15 - 16, The dateFrom
and dateTo fields in the input Zod schema accept any string and must be
validated to avoid DB errors; update the Zod schema (the schema where dateFrom
and dateTo are defined) to use a stricter validator such as
z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional().refine(v => !v ||
!Number.isNaN(Date.parse(v)), { message: "Invalid ISO date (YYYY-MM-DD)" }) so
malformed values are rejected before SQL is built, then convert the validated
strings to Date (or normalized ISO) when you build SQL predicates; also add a
cross-field check to ensure dateFrom <= dateTo and apply the same validation
change to the other identical schema block referenced (lines 29-34).
Comment on lines
+139
to
+157
| .refine( | ||
| (data) => { | ||
| if (data.odometerEnd !== undefined && data.odometerStart !== undefined) { | ||
| return data.odometerEnd > data.odometerStart; | ||
| } | ||
| return true; | ||
| }, | ||
| { | ||
| message: "Odometer end must be greater than odometer start", | ||
| path: ["odometerEnd"], | ||
| }, | ||
| ) | ||
| .refine( | ||
| (data) => { | ||
| if (data.arrivalTime !== undefined && data.departureTime !== undefined) { | ||
| return new Date(data.arrivalTime) > new Date(data.departureTime); | ||
| } | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
update allows single-field time/odometer edits that can bypass app validation.
If only one field in a pair is sent (e.g., only odometerEnd), current refinements skip pair checks and failures are deferred to DB constraints.
💡 Suggested guardrails
.object({
id: z.number().int().positive(),
date: z.string().min(1).optional(),
travelLocation: z.string().min(1).optional(),
departureTime: z.string().min(1).optional(),
arrivalTime: z.string().min(1).optional(),
odometerStart: z.number().int().nonnegative().optional(),
odometerEnd: z.number().int().nonnegative().optional(),
driverId: z.string().min(1).optional(),
driverName: z.string().min(1).optional(),
vehicle: z.string().min(1).optional(),
})
+ .refine(
+ (data) => (data.odometerStart === undefined) === (data.odometerEnd === undefined),
+ {
+ message: "Provide both odometerStart and odometerEnd together",
+ path: ["odometerEnd"],
+ },
+ )
+ .refine(
+ (data) => (data.departureTime === undefined) === (data.arrivalTime === undefined),
+ {
+ message: "Provide both departureTime and arrivalTime together",
+ path: ["arrivalTime"],
+ },
+ )
.refine(
(data) => {
if (data.odometerEnd !== undefined && data.odometerStart !== undefined) {
return data.odometerEnd > data.odometerStart;
}
return true;
},Also applies to: 168-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/vehicle-logs.ts` around lines 139 - 157, The update
schema's .refine checks for odometer and time pairs allow single-field updates
(e.g., only odometerEnd or arrivalTime) to bypass validation; modify the same
refine logic in the update input schema so that if one of a pair is provided the
other must also be provided and then enforce the ordering (for odometer: require
odometerStart when odometerEnd is present and check odometerEnd > odometerStart;
for times: require departureTime when arrivalTime is present and check new
Date(arrivalTime) > new Date(departureTime)); update the two .refine blocks that
reference odometerEnd/odometerStart and arrivalTime/departureTime to implement
these presence+ordering guards so partial single-field edits no longer bypass
validation.
…onarmy into SANC-60-vehicle-logs-create-api-endpoints-and-connect-to-frontend
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/server/api/routers/surveys.ts (1)
386-407: Survey updates don't sync to the auto-created vehicle log.When a survey is created, an associated vehicle log is auto-created. However, the update mutation doesn't modify the corresponding log entry. If a driver corrects odometer readings or times, the vehicle log will have stale data.
Consider whether this is the intended behavior. If logs should stay in sync:
- Add vehicle log updates within the survey update mutation, or
- Store a reference (e.g.,
surveyIdin logs) to enable future syncing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/surveys.ts` around lines 386 - 407, The survey update mutation (update: protectedProcedure in src/server/api/routers/surveys.ts) currently updates only the survey row, leaving the auto-created vehicle log out of sync; modify this mutation to also locate and update the associated vehicle log (by existing linkage or by adding/using a surveyId reference on the vehicle_logs table) with any changed fields (startReading, endReading, timeOfDeparture, timeOfArrival, destinationAddress, originalLocationChanged, etc.), perform both updates in a single transaction/atomic operation, and if vehicle logs lack a stable link to surveys add a surveyId to vehicle logs and backfill/create the reference so future syncs are possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/_components/drivercomponents/surveyForm/surveyForm.tsx`:
- Around line 185-196: The Vehicle Select in surveyForm.tsx should be disabled
when the trip is cancelled to match TripSurveyModal.tsx and validation logic;
update the Select (the instance with props key={form.key("vehicle")} and
{...form.getInputProps("vehicle")}, data={AVAILABLE_VEHICLES}) to include a
disabled prop set when tripCompletionStatus === BookingStatus.CANCELLED (ensure
BookingStatus and tripCompletionStatus are in scope or passed into the
component) so the UI behavior matches the validation in surveyNotification.tsx.
---
Nitpick comments:
In `@src/server/api/routers/surveys.ts`:
- Around line 386-407: The survey update mutation (update: protectedProcedure in
src/server/api/routers/surveys.ts) currently updates only the survey row,
leaving the auto-created vehicle log out of sync; modify this mutation to also
locate and update the associated vehicle log (by existing linkage or by
adding/using a surveyId reference on the vehicle_logs table) with any changed
fields (startReading, endReading, timeOfDeparture, timeOfArrival,
destinationAddress, originalLocationChanged, etc.), perform both updates in a
single transaction/atomic operation, and if vehicle logs lack a stable link to
surveys add a surveyId to vehicle logs and backfill/create the reference so
future syncs are possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5de246d2-9d69-448a-82cd-c84305ce8660
📒 Files selected for processing (8)
src/app/_components/common/navbar.tsxsrc/app/_components/drivercomponents/TripSurveyModal.tsxsrc/app/_components/drivercomponents/surveyForm/surveyForm.tsxsrc/app/_components/drivercomponents/surveyNotification/surveyNotification.tsxsrc/app/_components/vehiclelogcomponents/vehicle-log-form.tsxsrc/constants/vehicles.tssrc/server/api/routers/surveys.tssrc/server/db/post-trip-schema.ts
✅ Files skipped from review due to trivial changes (3)
- src/app/_components/common/navbar.tsx
- src/server/db/post-trip-schema.ts
- src/constants/vehicles.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/_components/vehiclelogcomponents/vehicle-log-form.tsx
Comment on lines
+185
to
+196
| <div className={styles.formRow}> | ||
| <Select | ||
| withAsterisk | ||
| label="Vehicle" | ||
| placeholder="Select a vehicle" | ||
| data={AVAILABLE_VEHICLES} | ||
| searchable | ||
| key={form.key("vehicle")} | ||
| {...form.getInputProps("vehicle")} | ||
| error={form.errors.vehicle} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Inconsistent disabled state for Vehicle field when trip is cancelled.
In TripSurveyModal.tsx (line 111), the Vehicle <Select> is disabled when tripCompletionStatus === BookingStatus.CANCELLED, but this implementation lacks that behavior. The validation in surveyNotification.tsx skips vehicle validation for cancelled trips, so the UI should match by disabling the field.
Proposed fix
<Select
withAsterisk
label="Vehicle"
placeholder="Select a vehicle"
data={AVAILABLE_VEHICLES}
searchable
key={form.key("vehicle")}
{...form.getInputProps("vehicle")}
error={form.errors.vehicle}
+ disabled={form.values.tripCompletionStatus === BookingStatus.CANCELLED}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/_components/drivercomponents/surveyForm/surveyForm.tsx` around lines
185 - 196, The Vehicle Select in surveyForm.tsx should be disabled when the trip
is cancelled to match TripSurveyModal.tsx and validation logic; update the
Select (the instance with props key={form.key("vehicle")} and
{...form.getInputProps("vehicle")}, data={AVAILABLE_VEHICLES}) to include a
disabled prop set when tripCompletionStatus === BookingStatus.CANCELLED (ensure
BookingStatus and tripCompletionStatus are in scope or passed into the
component) so the UI behavior matches the validation in surveyNotification.tsx.
Contributor
Author
|
Changes:
Note: To make the vehicle dropdown to select from the list of Salvation Army cars, i added it to |
Contributor
lets actually just store some hardcoded vehicles in the database for now, and pull from there. lets also add a new tab for admins to add vehicles to the db. sorry for the scope creep 😅 |
promatty
requested changes
Mar 26, 2026
| driverName: z.string().min(1, "Driver name is required"), | ||
| vehicle: z.string().min(1, "Vehicle is required"), | ||
| }) | ||
| .refine((data) => data.odometerEnd > data.odometerStart, { |
Contributor
There was a problem hiding this comment.
in ./src/app/admin/vehicle-logs/page.tsx we allow odometerEnd >= odometerStart. lets make this consistent
Contributor
Author
|
Changes:
Fix:
|
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)
drizzle/seed.ts (1)
23-28:⚠️ Potential issue | 🟡 MinorBug:
--vehiclesflag not included in the "no flags" check.When no flags are provided, the condition at line 23 doesn't check
flags.vehicles, so it defaults all seeders to true including vehicles. However, this is inconsistent—if a user passes only--users,flags.vehiclesstaysfalseas expected. The condition should includeflags.vehiclesfor consistency.Proposed fix
// If no flags provided, run all -if (!flags.users && !flags.bookings && !flags.logs) { +if (!flags.users && !flags.bookings && !flags.logs && !flags.vehicles) { flags.users = true; flags.bookings = true; flags.logs = true; flags.vehicles = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/seed.ts` around lines 23 - 28, The condition that detects "no flags provided" currently checks flags.users, flags.bookings, and flags.logs but omits flags.vehicles, causing inconsistent defaulting; update the conditional in the seeding entry point that references flags (the if block) to include flags.vehicles (e.g., change if (!flags.users && !flags.bookings && !flags.logs) to also && !flags.vehicles) so that when no flags are supplied all four (users, bookings, logs, vehicles) are set to true consistently.
🧹 Nitpick comments (8)
src/server/api/routers/vehicles.ts (1)
86-89: Soft-delete should updateupdatedAtfor consistency.The
deletemutation setsactive: falsebut doesn't updateupdatedAt, unlike theupdatemutation which explicitly sets it. This could make audit trails inconsistent.Proposed fix
delete: adminProcedure.input(z.object({ id: z.string() })).mutation(async ({ ctx, input }) => { - await ctx.db.update(vehicles).set({ active: false }).where(eq(vehicles.id, input.id)); + await ctx.db + .update(vehicles) + .set({ active: false, updatedAt: new Date() }) + .where(eq(vehicles.id, input.id)); return { success: true }; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/vehicles.ts` around lines 86 - 89, The soft-delete mutation (delete in adminProcedure.input -> .mutation) currently sets only active: false via ctx.db.update(vehicles).set(...) and should also set updatedAt for audit consistency; modify that .set call to include updatedAt: new Date() (matching the approach used in the update mutation) so the record's timestamp is updated when soft-deleted.drizzle/seed.ts (2)
567-576: Raw SQL table creation duplicates schema definition.The
CREATE TABLE IF NOT EXISTSstatement duplicates the schema invehicles-schema.ts. While this works for development seeding (per learnings about usingdrizzle-kit push), changes to the schema must be mirrored here manually. Consider adding a comment noting this dependency, or using Drizzle's schema introspection if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/seed.ts` around lines 567 - 576, The raw SQL CREATE TABLE in drizzzle/seed.ts duplicates the schema defined in vehicles-schema.ts and must be kept in sync; update the seed by either replacing the hardcoded CREATE TABLE block (the db.execute(...) call that creates "vehicles") with a note to use Drizzle schema-driven creation/introspection, or at minimum add a clear comment above the db.execute(...) CREATE TABLE IF NOT EXISTS block referencing vehicles-schema.ts and warning that changes to vehicles-schema.ts must be mirrored here; ensure you reference the "vehicles" table creation and the vehicles-schema.ts symbol in the comment so future editors know the dependency.
563-610: Missing--clearsupport for vehicles.Other seed functions (
seedBookings,seedLogs) checkflags.clearto delete existing data before seeding. TheseedVehicles()function doesn't support this, which may cause confusion when users expect--clearto reset all seeded data.Proposed fix
async function seedVehicles() { console.log("\n=== SEEDING VEHICLES ===\n"); try { + if (flags.clear) { + console.log("Clearing existing vehicles..."); + await db.delete(vehicles); + console.log("Existing vehicles cleared.\n"); + } + await db.execute(sql` CREATE TABLE IF NOT EXISTS "vehicles" (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/seed.ts` around lines 563 - 610, The seedVehicles() function is missing support for the flags.clear option; update seedVehicles to mirror seedBookings/seedLogs by checking flags.clear at the start and, when true, delete existing records from the vehicles table before inserting (e.g., perform db.delete(vehicles) or execute a "DELETE FROM vehicles" to clear rows), log the clearing action, and then continue with the existing insertion logic using generateId(), vehicles.name checks, and the existing db.insert(vehicles) flow.src/app/admin/vehicles/page.tsx (2)
162-168: Delete functionality is missing from the UI.The
vehiclesRouterincludes adeletemutation (soft-delete), but there's no delete button in the admin UI. Consider adding a delete/deactivate action to the table row actions, or remove the unused endpoint if it's not needed.Would you like me to generate the UI code to add a delete button with confirmation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/admin/vehicles/page.tsx` around lines 162 - 168, Add a delete action to each vehicle row: add a "Delete" Button next to the existing Edit Button (same Table.Td) and wire it to a new handler (e.g., handleDeleteVehicle) that shows a confirmation modal/confirm() and, on confirm, calls the vehiclesRouter.delete mutation (or the frontend delete mutation name) with the vehicle id; after success update the list (invalidate/query refetch) and handle errors via toast/logging. Ensure the new handler and button coexist with handleEditVehicle and reuse existing UI components (Button, Table.Td) and mutation utilities to keep behavior consistent.
195-195: Simplifyloadingprop to only check the relevant mutation.The Add modal checks both
createVehicle.isPending || updateVehicle.isPending, but onlycreateVehicleis relevant there. Similarly for the Edit modal. This is functionally correct but slightly imprecise.Proposed fix
<Modal opened={showAddModal} ... - loading={createVehicle.isPending || updateVehicle.isPending} + loading={createVehicle.isPending} ><Modal opened={showEditModal} ... - loading={createVehicle.isPending || updateVehicle.isPending} + loading={updateVehicle.isPending} >Also applies to: 222-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/admin/vehicles/page.tsx` at line 195, The loading prop currently combines both mutations but should target only the mutation used by each modal: update the Add modal's loading prop to use createVehicle.isPending only (replace createVehicle.isPending || updateVehicle.isPending) and update the Edit modal's loading prop to use updateVehicle.isPending only; locate the props on the modal components in src/app/admin/vehicles/page.tsx where createVehicle and updateVehicle are referenced and change each loading expression accordingly.src/app/hooks/useVehicles.ts (1)
13-17: Consider usingvehicle.idas the Select value instead ofvehicle.name.Using
vehicle.nameas bothvalueandlabelmeans if an admin renames a vehicle, historical references (e.g., in vehicle logs) could become inconsistent. Usingvehicle.idas the value provides referential integrity, though this may require updates to forms and the logs schema to store vehicle IDs instead of names.vehicleOptions: dbVehicles?.map((vehicle) => ({ - value: vehicle.name, + value: vehicle.id, label: vehicle.name, })) ?? [],This is a larger change that impacts how vehicle data is stored in logs. If vehicle names are considered immutable or the team prefers simplicity, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/hooks/useVehicles.ts` around lines 13 - 17, The Select options currently use vehicle.name for both value and label in vehicleOptions (dbVehicles?.map(...)), which can break referential integrity if names change; change the mapped value to vehicle.id while keeping label as vehicle.name, then update any form handlers, components and persistence logic that read/write the Select value (e.g. the form field that consumes vehicleOptions and any functions that serialize vehicle selection to logs) to store vehicle IDs instead of names, and adjust any log/schema fields expecting names to instead store or reference vehicle IDs (or add a migration if needed).src/app/_components/drivercomponents/surveyForm/surveyForm.tsx (1)
27-27: Consider removingwithAsteriskwhen trip is cancelled.Other fields in this form (e.g.,
endReadingat line 79,timeOfDepartureat line 101) conditionally setwithAsteriskbased ontripCompletionStatus !== BookingStatus.CANCELLED. The vehicle field should follow the same pattern for UI consistency.Proposed fix
<Select - withAsterisk + withAsterisk={form.values.tripCompletionStatus !== BookingStatus.CANCELLED} label="Vehicle"Also applies to: 186-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/drivercomponents/surveyForm/surveyForm.tsx` at line 27, The vehicle field's required indicator is currently always shown; update the JSX for the vehicle input(s) in surveyForm (the "vehicle" form control occurrences around the top and also the block at lines 186-198) to set withAsterisk conditionally using the existing tripCompletionStatus check—i.e., change the static withAsterisk to withAsterisk={tripCompletionStatus !== BookingStatus.CANCELLED} so it matches the pattern used by endReading and timeOfDeparture and hides the asterisk when BookingStatus.CANCELLED.src/server/db/vehicles-schema.ts (1)
7-8: Consider adding.$onUpdate()hook forupdatedAtconsistency.Per context snippet 2, other schemas in the codebase (vehicle-log, booking, auth schemas) use
.$onUpdate()onupdatedAt. While the router manually setsupdatedAt: new Date(), adding the hook ensures consistency if future code paths update vehicles without explicitly setting the timestamp.Proposed change
- updatedAt: timestamp("updated_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at").defaultNow().notNull().$onUpdate(() => new Date()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/db/vehicles-schema.ts` around lines 7 - 8, The updatedAt column in the vehicles schema should include the same .$onUpdate() hook used elsewhere to ensure it auto-updates on any DB-side updates; modify the updatedAt definition (timestamp("updated_at").defaultNow().notNull()) to chain .$onUpdate(new Date()) (matching patterns used in vehicle-log/booking/auth schemas) so updatedAt is maintained consistently even when code paths forget to set updatedAt (note: createdAt stays as timestamp("created_at").defaultNow().notNull()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/api/routers/vehicle-logs.ts`:
- Around line 84-91: The create mutation's Zod schema uses a non-strict check
(odometerEnd >= odometerStart) which contradicts the DB check constraint
(odometerEnd > odometerStart) and causes insert failures; change the refinement
in the create mutation schema to use strict greater-than (odometerEnd >
odometerStart) to match the DB, and also update the frontend validation/display
in page.tsx (the message that currently says "greater than or equal to start
reading") to say "greater than start reading" so validations are consistent;
reference the create mutation's schema refinement and compare with the existing
update mutation refinement which already uses strict > to ensure both
server-side schemas match the database constraint.
---
Outside diff comments:
In `@drizzle/seed.ts`:
- Around line 23-28: The condition that detects "no flags provided" currently
checks flags.users, flags.bookings, and flags.logs but omits flags.vehicles,
causing inconsistent defaulting; update the conditional in the seeding entry
point that references flags (the if block) to include flags.vehicles (e.g.,
change if (!flags.users && !flags.bookings && !flags.logs) to also &&
!flags.vehicles) so that when no flags are supplied all four (users, bookings,
logs, vehicles) are set to true consistently.
---
Nitpick comments:
In `@drizzle/seed.ts`:
- Around line 567-576: The raw SQL CREATE TABLE in drizzzle/seed.ts duplicates
the schema defined in vehicles-schema.ts and must be kept in sync; update the
seed by either replacing the hardcoded CREATE TABLE block (the db.execute(...)
call that creates "vehicles") with a note to use Drizzle schema-driven
creation/introspection, or at minimum add a clear comment above the
db.execute(...) CREATE TABLE IF NOT EXISTS block referencing vehicles-schema.ts
and warning that changes to vehicles-schema.ts must be mirrored here; ensure you
reference the "vehicles" table creation and the vehicles-schema.ts symbol in the
comment so future editors know the dependency.
- Around line 563-610: The seedVehicles() function is missing support for the
flags.clear option; update seedVehicles to mirror seedBookings/seedLogs by
checking flags.clear at the start and, when true, delete existing records from
the vehicles table before inserting (e.g., perform db.delete(vehicles) or
execute a "DELETE FROM vehicles" to clear rows), log the clearing action, and
then continue with the existing insertion logic using generateId(),
vehicles.name checks, and the existing db.insert(vehicles) flow.
In `@src/app/_components/drivercomponents/surveyForm/surveyForm.tsx`:
- Line 27: The vehicle field's required indicator is currently always shown;
update the JSX for the vehicle input(s) in surveyForm (the "vehicle" form
control occurrences around the top and also the block at lines 186-198) to set
withAsterisk conditionally using the existing tripCompletionStatus check—i.e.,
change the static withAsterisk to withAsterisk={tripCompletionStatus !==
BookingStatus.CANCELLED} so it matches the pattern used by endReading and
timeOfDeparture and hides the asterisk when BookingStatus.CANCELLED.
In `@src/app/admin/vehicles/page.tsx`:
- Around line 162-168: Add a delete action to each vehicle row: add a "Delete"
Button next to the existing Edit Button (same Table.Td) and wire it to a new
handler (e.g., handleDeleteVehicle) that shows a confirmation modal/confirm()
and, on confirm, calls the vehiclesRouter.delete mutation (or the frontend
delete mutation name) with the vehicle id; after success update the list
(invalidate/query refetch) and handle errors via toast/logging. Ensure the new
handler and button coexist with handleEditVehicle and reuse existing UI
components (Button, Table.Td) and mutation utilities to keep behavior
consistent.
- Line 195: The loading prop currently combines both mutations but should target
only the mutation used by each modal: update the Add modal's loading prop to use
createVehicle.isPending only (replace createVehicle.isPending ||
updateVehicle.isPending) and update the Edit modal's loading prop to use
updateVehicle.isPending only; locate the props on the modal components in
src/app/admin/vehicles/page.tsx where createVehicle and updateVehicle are
referenced and change each loading expression accordingly.
In `@src/app/hooks/useVehicles.ts`:
- Around line 13-17: The Select options currently use vehicle.name for both
value and label in vehicleOptions (dbVehicles?.map(...)), which can break
referential integrity if names change; change the mapped value to vehicle.id
while keeping label as vehicle.name, then update any form handlers, components
and persistence logic that read/write the Select value (e.g. the form field that
consumes vehicleOptions and any functions that serialize vehicle selection to
logs) to store vehicle IDs instead of names, and adjust any log/schema fields
expecting names to instead store or reference vehicle IDs (or add a migration if
needed).
In `@src/server/api/routers/vehicles.ts`:
- Around line 86-89: The soft-delete mutation (delete in adminProcedure.input ->
.mutation) currently sets only active: false via
ctx.db.update(vehicles).set(...) and should also set updatedAt for audit
consistency; modify that .set call to include updatedAt: new Date() (matching
the approach used in the update mutation) so the record's timestamp is updated
when soft-deleted.
In `@src/server/db/vehicles-schema.ts`:
- Around line 7-8: The updatedAt column in the vehicles schema should include
the same .$onUpdate() hook used elsewhere to ensure it auto-updates on any
DB-side updates; modify the updatedAt definition
(timestamp("updated_at").defaultNow().notNull()) to chain .$onUpdate(new Date())
(matching patterns used in vehicle-log/booking/auth schemas) so updatedAt is
maintained consistently even when code paths forget to set updatedAt (note:
createdAt stays as timestamp("created_at").defaultNow().notNull()).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fe10cdb-df52-4e53-8983-fc905d6a52a2
📒 Files selected for processing (13)
drizzle.config.tsdrizzle/seed.tssrc/app/_components/common/navbar.tsxsrc/app/_components/drivercomponents/TripSurveyModal.tsxsrc/app/_components/drivercomponents/surveyForm/surveyForm.tsxsrc/app/_components/vehiclelogcomponents/vehicle-log-form.tsxsrc/app/admin/vehicles/page.tsxsrc/app/hooks/useVehicles.tssrc/server/api/root.tssrc/server/api/routers/vehicle-logs.tssrc/server/api/routers/vehicles.tssrc/server/db/index.tssrc/server/db/vehicles-schema.ts
✅ Files skipped from review due to trivial changes (2)
- drizzle.config.ts
- src/server/db/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/_components/common/navbar.tsx
- src/app/_components/drivercomponents/TripSurveyModal.tsx
- src/app/_components/vehiclelogcomponents/vehicle-log-form.tsx
Comment on lines
+84
to
+91
| .refine((data) => data.odometerEnd >= data.odometerStart, { | ||
| message: "Odometer end must be greater than odometer start", | ||
| path: ["odometerEnd"], | ||
| }) | ||
| .refine((data) => new Date(data.arrivalTime) > new Date(data.departureTime), { | ||
| message: "Arrival time must be after departure time", | ||
| path: ["arrivalTime"], | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the vehicle-logs.ts file
cd src/server/api/routers
wc -l vehicle-logs.ts
head -150 vehicle-logs.tsRepository: Code-the-Change-YYC/salvationarmy
Length of output: 5294
🏁 Script executed:
# Search for the database schema definition with odometer constraints
find . -type f \( -name "*.ts" -o -name "*.sql" -o -name "*.js" \) | xargs grep -l "odometer_check\|odometerEnd.*odometerStart" | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 184
🏁 Script executed:
# Look for page.tsx mentioned in the comment
find . -type f -name "page.tsx" | xargs grep -l "odometer" 2>/dev/nullRepository: Code-the-Change-YYC/salvationarmy
Length of output: 114
🏁 Script executed:
cat src/server/db/vehicle-log.tsRepository: Code-the-Change-YYC/salvationarmy
Length of output: 1871
🏁 Script executed:
cat drizzle/0000_wise_bloodscream.sqlRepository: Code-the-Change-YYC/salvationarmy
Length of output: 8388
🏁 Script executed:
cat src/app/admin/vehicle-logs/page.tsx | head -300Repository: Code-the-Change-YYC/salvationarmy
Length of output: 6542
Odometer validation mismatch will cause database constraint violations.
The Zod refinement in the create mutation uses >= (line 84) but the database check constraint requires strict >:
check("odometer_check", sql`${table.odometerEnd} > ${table.odometerStart}`)When odometerEnd === odometerStart, the Zod validation passes, but the insert fails with a PostgreSQL constraint error. The update mutation (line 142) correctly uses strict >, and the frontend validation message incorrectly allows >= ("greater than or equal to start reading").
🔧 Proposed fix
})
- .refine((data) => data.odometerEnd >= data.odometerStart, {
+ .refine((data) => data.odometerEnd > data.odometerStart, {
message: "Odometer end must be greater than odometer start",
path: ["odometerEnd"],
})Also update the frontend validation in page.tsx to match the strict constraint (change "greater than or equal to" to "greater than").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/vehicle-logs.ts` around lines 84 - 91, The create
mutation's Zod schema uses a non-strict check (odometerEnd >= odometerStart)
which contradicts the DB check constraint (odometerEnd > odometerStart) and
causes insert failures; change the refinement in the create mutation schema to
use strict greater-than (odometerEnd > odometerStart) to match the DB, and also
update the frontend validation/display in page.tsx (the message that currently
says "greater than or equal to start reading") to say "greater than start
reading" so validations are consistent; reference the create mutation's schema
refinement and compare with the existing update mutation refinement which
already uses strict > to ensure both server-side schemas match the database
constraint.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server/api/routers/vehicle-logs.ts (2)
15-16:⚠️ Potential issue | 🟠 MajorValidate and order-check date filters before building SQL predicates.
Line 15 and Line 16 accept arbitrary strings, and
dateFrom > dateTois not rejected. This can leak malformed input into query construction and produce DB-level failures instead of clean input errors.Proposed hardening
z .object({ vehicle: z.string().optional(), driverName: z.string().optional(), - dateFrom: z.string().optional(), // ISO date string "YYYY-MM-DD" - dateTo: z.string().optional(), // ISO date string "YYYY-MM-DD" + dateFrom: z + .string() + .regex(/^\d{4}-\d{2}-\d{2}$/, "dateFrom must be YYYY-MM-DD") + .optional(), + dateTo: z + .string() + .regex(/^\d{4}-\d{2}-\d{2}$/, "dateTo must be YYYY-MM-DD") + .optional(), }) + .refine((v) => !v.dateFrom || !v.dateTo || v.dateFrom <= v.dateTo, { + message: "dateFrom must be on or before dateTo", + path: ["dateTo"], + }) .optional(),Also applies to: 29-34
139-157:⚠️ Potential issue | 🟠 MajorRequire odometer/time fields in pairs for
update.At Line 139 onward, sending only one field in a pair (
odometerEndwithoutodometerStart, orarrivalTimewithoutdepartureTime) bypasses application-level validation and defers failures to DB constraints.Proposed guardrails
.object({ id: z.number().int().positive(), date: z.string().min(1).optional(), travelLocation: z.string().min(1).optional(), departureTime: z.string().min(1).optional(), arrivalTime: z.string().min(1).optional(), odometerStart: z.number().int().nonnegative().optional(), odometerEnd: z.number().int().nonnegative().optional(), driverId: z.string().min(1).optional(), driverName: z.string().min(1).optional(), vehicle: z.string().min(1).optional(), }) + .refine( + (data) => (data.odometerStart === undefined) === (data.odometerEnd === undefined), + { + message: "Provide both odometerStart and odometerEnd together", + path: ["odometerEnd"], + }, + ) + .refine( + (data) => (data.departureTime === undefined) === (data.arrivalTime === undefined), + { + message: "Provide both departureTime and arrivalTime together", + path: ["arrivalTime"], + }, + ) .refine( (data) => { if (data.odometerEnd !== undefined && data.odometerStart !== undefined) { return data.odometerEnd >= data.odometerStart; } return true; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/vehicle-logs.ts` around lines 139 - 157, The update input schema currently allows sending only one side of paired fields, so add refinements to the schema in vehicle-logs.ts (the chain that includes the existing .refine calls) to enforce that odometerStart and odometerEnd must be provided together and arrivalTime and departureTime must be provided together: for each pair, validate that either both are undefined or both are present (and keep the existing comparison checks when both exist); update the refine message/path to indicate the missing partner (e.g., "odometerStart and odometerEnd must be provided together" with path ["odometerStart","odometerEnd"] and similarly for arrivalTime/departureTime) so single-field updates are rejected before hitting DB constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/api/routers/vehicle-logs.ts`:
- Around line 85-87: Update the validation error text for the odometer end check
so it matches the actual comparison operator (>=); find occurrences where the
validation error object uses path ["odometerEnd"] and message "Odometer end must
be greater than" and change them to "Odometer end must be greater than or equal
to" (this applies to both places in the vehicle-logs router where the
odometerEnd validation message is constructed).
---
Duplicate comments:
In `@src/server/api/routers/vehicle-logs.ts`:
- Around line 139-157: The update input schema currently allows sending only one
side of paired fields, so add refinements to the schema in vehicle-logs.ts (the
chain that includes the existing .refine calls) to enforce that odometerStart
and odometerEnd must be provided together and arrivalTime and departureTime must
be provided together: for each pair, validate that either both are undefined or
both are present (and keep the existing comparison checks when both exist);
update the refine message/path to indicate the missing partner (e.g.,
"odometerStart and odometerEnd must be provided together" with path
["odometerStart","odometerEnd"] and similarly for arrivalTime/departureTime) so
single-field updates are rejected before hitting DB constraints.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7568cad2-6fee-405d-beb0-fea59612dafb
📒 Files selected for processing (2)
src/server/api/routers/vehicle-logs.tssrc/server/db/vehicle-log.ts
promatty
deleted the
SANC-60-vehicle-logs-create-api-endpoints-and-connect-to-frontend
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
leftover from SANC58-vehicle-logs-frontend:
log.idbug by adding id to the forms initialValues so the update mutation can target the correct rowSANC60:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores