Merged
Conversation
…onarmy into SANC-58-vehicle-logs-frontend # Conflicts: # src/app/driver/home/page.tsx # src/server/db/booking-schema.ts
…onarmy into SANC-58-vehicle-logs-frontend # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…onarmy into SANC-58-vehicle-logs-frontend
…onarmy into SANC-58-vehicle-logs-frontend
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.
|
📝 WalkthroughWalkthroughAdds vehicle logs end-to-end: DB seeding support, a new tRPC router, frontend components (form + AG Grid table), an admin page for add/edit flows, UI constants and styles, and new SVG icons. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant UI as VehicleLogsPage (Client)
participant API as tRPC vehicleLogsRouter
participant DB as Vehicle Logs Table
participant Seeder as drizzle/seed CLI
Admin->>UI: open page / request logs
UI->>API: query getAll()
API->>DB: SELECT logs ORDER BY date DESC
DB-->>API: rows
API-->>UI: formatted log list
UI->>Admin: render table
Admin->>UI: open modal / submit form
UI->>API: (future) mutation create/update
API->>DB: INSERT/UPDATE log row
DB-->>API: result
API-->>UI: success
UI->>Admin: close modal / notify
Admin->>Seeder: run seed (--logs / --clear)
Seeder->>DB: INSERT synthetic logs
DB-->>Seeder: seed result
Seeder-->>Admin: summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🧹 Nitpick comments (7)
src/app/_components/vehiclelogcomponents/vehicle-log-table-view.module.scss (1)
3-5: Hardcoded table height may not adapt to different viewports.The fixed
height: 650pxcould cause issues on smaller screens (excessive whitespace or clipping). Consider using a viewport-relative unit ormin-height/max-heightcombination. That said, this mirrors the existing pattern in the agency table view, so it's acceptable for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.module.scss` around lines 3 - 5, The .tableContainer rule uses a hardcoded height (height: 650px) which doesn't adapt to varying viewports; update the CSS for .tableContainer to use responsive sizing (e.g., a viewport-relative unit like height: 65vh or replace with min-height/max-height combo such as min-height: 400px; max-height: 80vh) so the table scales on smaller screens while preserving intended layout constraints; change the declaration in vehicle-log-table-view.module.scss under the .tableContainer selector accordingly.src/app/_components/vehiclelogcomponents/vehicle-log-form.module.scss (1)
1-1: Unused variables import.
@use "@/styles/variables" as *is imported but no variables from it are referenced in this file. Consider removing it to keep the stylesheet clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-form.module.scss` at line 1, The stylesheet contains an unused import: the statement '@use "@/styles/variables" as *' in vehicle-log-form.module.scss is not referenced anywhere in this module; remove that import line (or replace it with specific variable imports if you intend to use variables) to eliminate dead code and keep the stylesheet clean.src/server/api/routers/vehicle-logs.ts (1)
5-34: No pagination ongetAll— consider adding limits for scalability.The endpoint returns all vehicle logs without any pagination or limit. As the
logstable grows, this will become a performance bottleneck. For an admin-only tool this is acceptable short-term, but worth planning for.🤖 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 5 - 34, getAll currently returns all rows and should be converted to a paginated query: change vehicleLogsRouter.getAll (the adminProcedure.query) to accept input params (e.g., page and limit) via adminProcedure.input, validate defaults, then use ctx.db with .limit(limit) and .offset((page-1)*limit) (or your ORM's pagination helpers) when selecting from logs and ordering by desc(logs.date); also return pagination metadata (total count or hasMore) alongside the transformed results so the frontend can handle paging.src/app/admin/vehicle-logs/page.tsx (1)
100-109: PlaceholdersetTimeoutalways fires success — no actual mutation wired upThe
handleConfirmfunction simulates a 2-second delay and unconditionally shows a success notification without saving any data to the database. The// TODO: Call tRPC mutationindicates this is known. Until the mutation is wired, the UI gives users false confirmation.Would you like me to scaffold the tRPC
create/updatemutation call (including the router procedure and the React hook invocation) and open a follow-up issue to track it?🤖 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 100 - 109, The placeholder setTimeout in handleConfirm unconditionally shows success and never persists data; replace it by calling the appropriate tRPC mutation instead of the timeout: import and invoke the vehicleLog.create or vehicleLog.update procedure (depending on isEditMode) from your trpc hook, await the mutation, handle loading/errors via setLoading and notify (use notify.error on failure), close the modal and call form.reset() only after a successful response, and remove the setTimeout; keep references to handleConfirm, isEditMode, form.reset, setLoading, setShowModal, and notify so you update the correct logic path.src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx (3)
77-77:themeanddefaultColDefare recreated on every renderBoth
theme(line 77) anddefaultColDef(lines 145–152) are constructed inside the component body without memoization, producing new object references on each render. AG Grid performs reference equality checks on these props, meaning every re-render (e.g., from a tRPC refetch) triggers unnecessary grid reconfiguration.♻️ Proposed fix
Hoist both outside the component (they have no component-level dependencies):
+const theme = themeQuartz.withParams(TABLE_THEME_PARAMS); + +const defaultColDef: ColDef = { + flex: 1, + width: DEFAULT_COLUMN_WIDTH, + minWidth: MIN_COLUMN_WIDTH, + resizable: false, + sortable: true, + filter: true, +}; export default function VehicleLogTableView({ onRowClick }: VehicleLogTableViewProps = {}) { - const theme = themeQuartz.withParams(TABLE_THEME_PARAMS); ... - const defaultColDef: ColDef = { ... };Also applies to: 145-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx` at line 77, The theme object and defaultColDef are being re-created on every render (see theme = themeQuartz.withParams(TABLE_THEME_PARAMS) and defaultColDef) which forces AG Grid to reconfigure; fix this by hoisting their creation out of the React component (or memoizing them once) so they retain stable references: move the theme = themeQuartz.withParams(TABLE_THEME_PARAMS) call and the defaultColDef object to module scope (or wrap them in useMemo with empty deps) so AG Grid receives stable props across re-renders.
129-136:KM_DRIVENformatter inconsistently skipstoLocaleString()The
ODOMETER_STARTandODOMETER_ENDformatters (lines 116–127) useparams.value.toLocaleString()for locale-aware thousand-separator formatting, but theKM_DRIVENformatter uses a plain template literal. For a value like1234.5, this displays1234.5 KMinstead of1,234.5 KM.♻️ Proposed fix
-return `${params.value} KM`; +return `${params.value.toLocaleString()} KM`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx` around lines 129 - 136, The KM_DRIVEN column's valueFormatter (inside createColumnDef(COLUMN_IDS.KM_DRIVEN) with headerComponent HeaderWithIcon) returns a plain template literal and thus misses locale-aware formatting; update its valueFormatter to mirror ODOMETER_START/ODOMETER_END by checking for null/undefined and returning "" when absent, otherwise calling params.value.toLocaleString() and appending " KM" so values like 1234.5 render as "1,234.5 KM".
80-80: No error or loading state for thegetAllqueryThe
useQuerycall silently defaults to[]on both the loading and error states, so a network failure or a server error is indistinguishable from an empty log list. Users would see a blank table with no feedback.♻️ Proposed fix
-const { data: vehicleLogs = [] } = api.vehicleLogs.getAll.useQuery(); +const { data: vehicleLogs = [], isLoading, isError } = api.vehicleLogs.getAll.useQuery();Then render appropriate loading/error states in the JSX before the grid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx` at line 80, The getAll query currently destructures only data with a default empty array (const { data: vehicleLogs = [] } = api.vehicleLogs.getAll.useQuery()), which hides loading and error states; update the call to also capture isLoading and error (e.g., const { data: vehicleLogs = [], isLoading, error } = api.vehicleLogs.getAll.useQuery()) and then render explicit UI branches in the component: show a loading indicator when isLoading is true, show an error message or retry option when error is set, and only render the empty-state/table when neither loading nor error is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/seed.ts`:
- Around line 481-521: The generated logs assign cumulativeOdometer while dates
are randomized, then sort logsData by date causing odometer inconsistencies; fix
by deferring odometer assignment until after sorting: keep creating entries with
date, departureTime, arrivalTime and tripDistance (but leave
odometerStart/odometerEnd unset), then run the existing logsData.sort(...) and
afterwards iterate the sorted logsData updating cumulativeOdometer and filling
odometerStart/odometerEnd for each entry (using the same tripDistance logic),
ensuring cumulativeOdometer, logsData, startOdometer/endOdometer fields and the
sorting call remain the key references.
In `@src/app/_components/vehiclelogcomponents/vehicle-log-form.tsx`:
- Around line 39-52: The DateTimePicker instances (DateTimePicker with props
using minDate={now}, key={form.key("date")}, and other pickers referencing
form.values.departureTime) are preventing editing of past-dated records; update
vehicle-log-form.tsx to only apply minDate when creating a new record (use the
isEditMode prop or a passed-in flag) — e.g., replace static minDate={now} with a
conditional that sets minDate to undefined when isEditMode is true (or use a
minDateProp passed from the parent), and similarly change the other pickers
using form.values.departureTime to use form.values.departureTime ?? (isEditMode
? undefined : now) so past values are editable while still enforcing a min date
for add mode.
- Around line 73-94: The Departure Time DateTimePicker is missing an onBlur
handler so field-level validation won't run on blur; add an onBlur prop to the
DateTimePicker that triggers the form's blur/validation for "departureTime"
(e.g., call form.setFieldTouched('departureTime', true) and/or
form.validateField('departureTime') inside the onBlur) while keeping the
existing onChange logic that sets departureTime and clears arrivalTime when
invalid; reference the DateTimePicker component, form.setFieldValue, and the
"departureTime" field when making the change.
In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx`:
- Line 162: The onRowClicked handler passes RowClickedEvent.data (which can be
undefined) directly into onRowClick; update the handler to guard against
undefined before invoking onRowClick so the parent handleRowClick never receives
undefined. Specifically, in the onRowClicked prop (in
vehicle-log-table-view.tsx) check that event.data !== undefined (or
event.node?.data) and only call onRowClick(event.data) when present, otherwise
return/do nothing; this prevents handleRowClick from getting an undefined value
and avoids crashes in functions like handleRowClick that call
log.ODOMETER_START.toString().
- Around line 26-35: VehicleLogData is duplicated; export the canonical
interface from this file by adding an export for VehicleLogData (the interface
name) and then remove the local duplicate in page.tsx and import VehicleLogData
into page.tsx instead (or alternatively move the exported interface to a shared
types module and import it in both files); update any references in
functions/components that used the local type so they use the imported/exported
VehicleLogData symbol.
In `@src/server/api/routers/vehicle-logs.ts`:
- Around line 24-33: The mapped API response is dropping the database id and
vehicle fields so the frontend cannot identify or update logs; update the
transformation in vehicle-logs.ts (the results.map(...) block that returns
DATE/DESTINATION/DEPARTURE_TIME/ARRIVAL_TIME/ODOMETER_START/ODOMETER_END/KM_DRIVEN/DRIVER)
to include id: row.id and vehicle: row.vehicle (or row.vehicleName as
appropriate) so the response carries the primary key and vehicle info, and then
update the VehicleLogData interface in src/app/admin/vehicle-logs/page.tsx and
src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx to include
id (and vehicle if missing) so the edit modal and handleRowClick can pass the id
to the tRPC update mutation.
---
Duplicate comments:
In `@src/app/admin/vehicle-logs/page.tsx`:
- Around line 14-23: The VehicleLogData interface is duplicated; remove the
local declaration in page.tsx and reuse the existing VehicleLogData from
vehicle-log-table-view.tsx by importing it there (or export it from
vehicle-log-table-view.tsx if not already exported), ensuring any references in
page.tsx still use the VehicleLogData type; update imports to reference the
exported symbol from vehicle-log-table-view.tsx.
---
Nitpick comments:
In `@src/app/_components/vehiclelogcomponents/vehicle-log-form.module.scss`:
- Line 1: The stylesheet contains an unused import: the statement '@use
"@/styles/variables" as *' in vehicle-log-form.module.scss is not referenced
anywhere in this module; remove that import line (or replace it with specific
variable imports if you intend to use variables) to eliminate dead code and keep
the stylesheet clean.
In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.module.scss`:
- Around line 3-5: The .tableContainer rule uses a hardcoded height (height:
650px) which doesn't adapt to varying viewports; update the CSS for
.tableContainer to use responsive sizing (e.g., a viewport-relative unit like
height: 65vh or replace with min-height/max-height combo such as min-height:
400px; max-height: 80vh) so the table scales on smaller screens while preserving
intended layout constraints; change the declaration in
vehicle-log-table-view.module.scss under the .tableContainer selector
accordingly.
In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx`:
- Line 77: The theme object and defaultColDef are being re-created on every
render (see theme = themeQuartz.withParams(TABLE_THEME_PARAMS) and
defaultColDef) which forces AG Grid to reconfigure; fix this by hoisting their
creation out of the React component (or memoizing them once) so they retain
stable references: move the theme = themeQuartz.withParams(TABLE_THEME_PARAMS)
call and the defaultColDef object to module scope (or wrap them in useMemo with
empty deps) so AG Grid receives stable props across re-renders.
- Around line 129-136: The KM_DRIVEN column's valueFormatter (inside
createColumnDef(COLUMN_IDS.KM_DRIVEN) with headerComponent HeaderWithIcon)
returns a plain template literal and thus misses locale-aware formatting; update
its valueFormatter to mirror ODOMETER_START/ODOMETER_END by checking for
null/undefined and returning "" when absent, otherwise calling
params.value.toLocaleString() and appending " KM" so values like 1234.5 render
as "1,234.5 KM".
- Line 80: The getAll query currently destructures only data with a default
empty array (const { data: vehicleLogs = [] } =
api.vehicleLogs.getAll.useQuery()), which hides loading and error states; update
the call to also capture isLoading and error (e.g., const { data: vehicleLogs =
[], isLoading, error } = api.vehicleLogs.getAll.useQuery()) and then render
explicit UI branches in the component: show a loading indicator when isLoading
is true, show an error message or retry option when error is set, and only
render the empty-state/table when neither loading nor error is present.
In `@src/app/admin/vehicle-logs/page.tsx`:
- Around line 100-109: The placeholder setTimeout in handleConfirm
unconditionally shows success and never persists data; replace it by calling the
appropriate tRPC mutation instead of the timeout: import and invoke the
vehicleLog.create or vehicleLog.update procedure (depending on isEditMode) from
your trpc hook, await the mutation, handle loading/errors via setLoading and
notify (use notify.error on failure), close the modal and call form.reset() only
after a successful response, and remove the setTimeout; keep references to
handleConfirm, isEditMode, form.reset, setLoading, setShowModal, and notify so
you update the correct logic path.
In `@src/server/api/routers/vehicle-logs.ts`:
- Around line 5-34: getAll currently returns all rows and should be converted to
a paginated query: change vehicleLogsRouter.getAll (the adminProcedure.query) to
accept input params (e.g., page and limit) via adminProcedure.input, validate
defaults, then use ctx.db with .limit(limit) and .offset((page-1)*limit) (or
your ORM's pagination helpers) when selecting from logs and ordering by
desc(logs.date); also return pagination metadata (total count or hasMore)
alongside the transformed results so the frontend can handle paging.
Comment on lines
39
to
52
| <DateTimePicker | ||
| withAsterisk | ||
| label="Date" | ||
| placeholder="Select date" | ||
| minDate={now} | ||
| key={form.key("date")} | ||
| {...form.getInputProps("date")} | ||
| timePickerProps={{ | ||
| withDropdown: true, | ||
| popoverProps: { withinPortal: false }, | ||
| format: "12h", | ||
| }} | ||
| clearable | ||
| /> |
There was a problem hiding this comment.
minDate={now} blocks editing of existing past-dated records
All three date/time pickers enforce minDate={now}, which restricts calendar selection to dates ≥ now. In edit mode (when a row is clicked), the form is populated with past departure/arrival times from the server, and users cannot modify or re-select those past values since the calendar navigation is disabled for past dates. This effectively breaks the edit flow whenever a past trip is viewed.
Fix: only apply minDate in add mode, or remove it entirely if retroactive logging is a valid use case. The isEditMode prop (or a flag) from the parent page can gate this.
🔧 Proposed fix — conditionally apply `minDate`
-interface VehicleLogFormProps {
- form: UseFormReturnType<VehicleLogFormData>;
-}
+interface VehicleLogFormProps {
+ form: UseFormReturnType<VehicleLogFormData>;
+ isEditMode?: boolean;
+}
-export const VehicleLogForm = ({ form }: VehicleLogFormProps) => {
+export const VehicleLogForm = ({ form, isEditMode = false }: VehicleLogFormProps) => {
const now = new Date();
+ const minDateProp = isEditMode ? undefined : now;Then replace minDate={now} with minDate={minDateProp} and minDate={form.values.departureTime || now} with minDate={form.values.departureTime ?? (isEditMode ? undefined : now)} on each picker.
And in page.tsx:
-<VehicleLogForm form={form} />
+<VehicleLogForm form={form} isEditMode={isEditMode} />Also applies to: 73-95, 97-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/_components/vehiclelogcomponents/vehicle-log-form.tsx` around lines
39 - 52, The DateTimePicker instances (DateTimePicker with props using
minDate={now}, key={form.key("date")}, and other pickers referencing
form.values.departureTime) are preventing editing of past-dated records; update
vehicle-log-form.tsx to only apply minDate when creating a new record (use the
isEditMode prop or a passed-in flag) — e.g., replace static minDate={now} with a
conditional that sets minDate to undefined when isEditMode is true (or use a
minDateProp passed from the parent), and similarly change the other pickers
using form.values.departureTime to use form.values.departureTime ?? (isEditMode
? undefined : now) so past values are editable while still enforcing a min date
for add mode.
Comment on lines
26
to
35
| interface VehicleLogData { | ||
| DATE: string; | ||
| DESTINATION: string; | ||
| DEPARTURE_TIME: string; | ||
| ARRIVAL_TIME: string; | ||
| ODOMETER_START: number; | ||
| ODOMETER_END: number; | ||
| KM_DRIVEN: number; | ||
| DRIVER: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
VehicleLogData is duplicated — export it from one canonical location
VehicleLogData (lines 26–35 here) is defined again with an identical shape in page.tsx (lines 14–23). If the server-side data shape ever changes, both definitions need to be updated. Export the interface from this file (or a shared types file) and import it in page.tsx.
♻️ Proposed fix
-interface VehicleLogData {
+export interface VehicleLogData {
DATE: string;
...
}In page.tsx:
+import type { VehicleLogData } from "@/app/_components/vehiclelogcomponents/vehicle-log-table-view";
-interface VehicleLogData {
- ...
-}📝 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.
Suggested change
| interface VehicleLogData { | |
| DATE: string; | |
| DESTINATION: string; | |
| DEPARTURE_TIME: string; | |
| ARRIVAL_TIME: string; | |
| ODOMETER_START: number; | |
| ODOMETER_END: number; | |
| KM_DRIVEN: number; | |
| DRIVER: string; | |
| } | |
| export interface VehicleLogData { | |
| DATE: string; | |
| DESTINATION: string; | |
| DEPARTURE_TIME: string; | |
| ARRIVAL_TIME: string; | |
| ODOMETER_START: number; | |
| ODOMETER_END: number; | |
| KM_DRIVEN: number; | |
| DRIVER: string; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx` around
lines 26 - 35, VehicleLogData is duplicated; export the canonical interface from
this file by adding an export for VehicleLogData (the interface name) and then
remove the local duplicate in page.tsx and import VehicleLogData into page.tsx
instead (or alternatively move the exported interface to a shared types module
and import it in both files); update any references in functions/components that
used the local type so they use the imported/exported VehicleLogData symbol.
src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
24
to
33
| return results.map((row) => ({ | ||
| DATE: row.date || "", | ||
| DESTINATION: row.destination || "", | ||
| DEPARTURE_TIME: row.departureTime || "", | ||
| ARRIVAL_TIME: row.arrivalTime || "", | ||
| ODOMETER_START: row.odometerStart || 0, | ||
| ODOMETER_END: row.odometerEnd || 0, | ||
| KM_DRIVEN: row.kilometersDriven || 0, | ||
| DRIVER: row.driverName || "Unknown", | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the vehicle-logs.ts file and check its full content
find . -name "vehicle-logs.ts" -type fRepository: Code-the-Change-YYC/salvationarmy
Length of output: 117
🏁 Script executed:
# Find schema files that might define vehicle logs
find . -name "*schema*" -o -name "*vehicle*" | grep -E "\.(ts|sql)$" | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 275
🏁 Script executed:
# Search for usage of vehicle logs API in frontend code
rg -n "vehicle.*log" -i --type=ts --type=tsx | head -30Repository: Code-the-Change-YYC/salvationarmy
Length of output: 104
🏁 Script executed:
# Read the vehicle-logs.ts router file
cat -n ./src/server/api/routers/vehicle-logs.tsRepository: Code-the-Change-YYC/salvationarmy
Length of output: 1498
🏁 Script executed:
# Read the vehicle-log.ts schema file
cat -n ./src/server/db/vehicle-log.tsRepository: Code-the-Change-YYC/salvationarmy
Length of output: 2242
🏁 Script executed:
# Search for frontend usage with correct file types
rg -n "vehicle.*log|vehicleLog" -i --type=ts -g '*' | head -40Repository: Code-the-Change-YYC/salvationarmy
Length of output: 4042
🏁 Script executed:
# Check VehicleLogData interface in the page
sed -n '14,25p' ./src/app/admin/vehicle-logs/page.tsxRepository: Code-the-Change-YYC/salvationarmy
Length of output: 328
🏁 Script executed:
# Check VehicleLogData interface in the table view component
sed -n '26,36p' ./src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsxRepository: Code-the-Change-YYC/salvationarmy
Length of output: 284
🏁 Script executed:
# Check the handleRowClick and form submission logic
sed -n '74,110p' ./src/app/admin/vehicle-logs/page.tsxRepository: Code-the-Change-YYC/salvationarmy
Length of output: 1097
🏁 Script executed:
# Check if there's an update/edit mutation in the vehicle-logs router
cat -n ./src/server/api/routers/vehicle-logs.tsRepository: Code-the-Change-YYC/salvationarmy
Length of output: 1498
🏁 Script executed:
# Search for any update/mutation endpoints related to vehicle logs
rg -n "update|create|edit" ./src/server/api/routers/vehicle-logs.ts -iRepository: Code-the-Change-YYC/salvationarmy
Length of output: 192
🏁 Script executed:
# Check if there are any other vehicle logs endpoints that might handle updates
rg -n "vehicleLogs\." -A3 ./src/app/admin/vehicle-logs/page.tsxRepository: Code-the-Change-YYC/salvationarmy
Length of output: 59
Include id in the response mapping — the edit workflow cannot identify which log to update.
The router selects id from the database (line 10) but drops it in the transformation (lines 24-33). The frontend has edit functionality with a modal and handleRowClick handler, and the code contains a TODO to implement the tRPC mutation for saving changes. Without id in the API response, the update mutation will have no way to identify which record to update. Additionally, the vehicle field is also not returned despite being selected in the schema.
🔧 Proposed fix: include id and vehicle in the response
return results.map((row) => ({
+ id: row.id,
DATE: row.date || "",
DESTINATION: row.destination || "",
DEPARTURE_TIME: row.departureTime || "",
ARRIVAL_TIME: row.arrivalTime || "",
ODOMETER_START: row.odometerStart || 0,
ODOMETER_END: row.odometerEnd || 0,
KM_DRIVEN: row.kilometersDriven || 0,
DRIVER: row.driverName || "Unknown",
+ VEHICLE: row.vehicle || "",
}));Also update the VehicleLogData interface in src/app/admin/vehicle-logs/page.tsx and src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx to include the id field.
📝 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.
Suggested change
| return results.map((row) => ({ | |
| DATE: row.date || "", | |
| DESTINATION: row.destination || "", | |
| DEPARTURE_TIME: row.departureTime || "", | |
| ARRIVAL_TIME: row.arrivalTime || "", | |
| ODOMETER_START: row.odometerStart || 0, | |
| ODOMETER_END: row.odometerEnd || 0, | |
| KM_DRIVEN: row.kilometersDriven || 0, | |
| DRIVER: row.driverName || "Unknown", | |
| })); | |
| return results.map((row) => ({ | |
| id: row.id, | |
| DATE: row.date || "", | |
| DESTINATION: row.destination || "", | |
| DEPARTURE_TIME: row.departureTime || "", | |
| ARRIVAL_TIME: row.arrivalTime || "", | |
| ODOMETER_START: row.odometerStart || 0, | |
| ODOMETER_END: row.odometerEnd || 0, | |
| KM_DRIVEN: row.kilometersDriven || 0, | |
| DRIVER: row.driverName || "Unknown", | |
| VEHICLE: row.vehicle || "", | |
| })); |
🤖 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 24 - 33, The mapped API
response is dropping the database id and vehicle fields so the frontend cannot
identify or update logs; update the transformation in vehicle-logs.ts (the
results.map(...) block that returns
DATE/DESTINATION/DEPARTURE_TIME/ARRIVAL_TIME/ODOMETER_START/ODOMETER_END/KM_DRIVEN/DRIVER)
to include id: row.id and vehicle: row.vehicle (or row.vehicleName as
appropriate) so the response carries the primary key and vehicle info, and then
update the VehicleLogData interface in src/app/admin/vehicle-logs/page.tsx and
src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx to include
id (and vehicle if missing) so the edit modal and handleRowClick can pass the id
to the tRPC update mutation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
drizzle/seed.ts (1)
487-499: Minor comment inaccuracies in range bounds.
- Line 487:
Math.floor(Math.random() * 8) + 8yields 8–15 (3 PM max), not 4 PM as stated.- Line 498:
Math.floor(Math.random() * 50) + 10yields 10–59 km, not 10–60 km.✏️ Proposed comment fixes
- // Random departure time between 8 AM and 4 PM + // Random departure time between 8 AM and 3 PM const departureHour = Math.floor(Math.random() * 8) + 8;- // Trip distance: 10-60 km + // Trip distance: 10-59 km const tripDistance = Math.floor(Math.random() * 50) + 10;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/seed.ts` around lines 487 - 499, The comments misstate the generated bounds: update the departure hour generation (departureHour) to include 4 PM by using a 9-hour span (so hours 8..16) instead of 8 (currently yields 8..15), and update the trip distance generation (tripDistance) to include 60 km by using a 51-value span (so distances 10..60) instead of 50 (currently yields 10..59); keep departureMinute and arrivalTime logic as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@drizzle/seed.ts`:
- Around line 487-499: The comments misstate the generated bounds: update the
departure hour generation (departureHour) to include 4 PM by using a 9-hour span
(so hours 8..16) instead of 8 (currently yields 8..15), and update the trip
distance generation (tripDistance) to include 60 km by using a 51-value span (so
distances 10..60) instead of 50 (currently yields 10..59); keep departureMinute
and arrivalTime logic as-is.
Contributor
Author
|
Update
|
promatty
requested changes
Feb 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/app/admin/vehicle-logs/page.tsx (1)
85-92: Usevalidation.hasErrorsdirectly instead ofObject.keys(...).length > 0.
form.validate()returns an object with ahasErrorsboolean property, making theObject.keyscheck redundant and less readable.♻️ Proposed refactor
-const validation = form.validate(); -const hasErrors = Object.keys(validation.errors).length > 0; - -if (hasErrors) { +const { hasErrors } = form.validate(); +if (hasErrors) {🤖 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 85 - 92, The code currently computes hasErrors with Object.keys(validation.errors).length > 0 after calling form.validate(); replace that with the built-in boolean returned by form.validate(), i.e., use validation.hasErrors directly (update the hasErrors assignment and subsequent conditional that calls notify.error and setLoading) so the check becomes if (validation.hasErrors) {...} using the existing validation, form.validate(), notify.error and setLoading symbols.src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx (2)
154-161:defaultColDefis recreated on every render — consider stabilizing it.Like the
themeobject above,defaultColDefis a plain object literal re-allocated on every render. AG Grid uses referential equality for prop comparison; a stable reference avoids unnecessary re-renders. Define it outside the component or wrap inuseMemo.♻️ Proposed fix
+const defaultColDef: ColDef = { + flex: 1, + width: DEFAULT_COLUMN_WIDTH, + minWidth: MIN_COLUMN_WIDTH, + resizable: false, + sortable: true, + filter: true, +}; + export default function VehicleLogTableView(...) { ... - const defaultColDef: ColDef = { - flex: 1, - width: DEFAULT_COLUMN_WIDTH, - minWidth: MIN_COLUMN_WIDTH, - resizable: false, - sortable: true, - filter: true, - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx` around lines 154 - 161, The defaultColDef object (defaultColDef: ColDef) is re-created on every render causing unnecessary AG Grid updates; stabilize it by moving its declaration outside the component function or memoizing it with useMemo (e.g., const defaultColDef = useMemo(() => ({ flex:1, width: DEFAULT_COLUMN_WIDTH, minWidth: MIN_COLUMN_WIDTH, resizable:false, sortable:true, filter:true }), [])) so the reference remains stable when passed to the grid in vehicle-log-table-view.tsx.
82-82:themeis recreated on every render — hoist it or memoize it.
TABLE_THEME_PARAMSis a module-level constant, sothemeQuartz.withParams(...)produces a new object on every render with identical inputs. AG Grid compares thethemeprop by reference, so this triggers unnecessary internal reconciliation. Move it outside the component (oruseMemo(() => ..., [])).♻️ Proposed fix (hoist outside component)
+const theme = themeQuartz.withParams(TABLE_THEME_PARAMS); + export default function VehicleLogTableView({ onRowClick }: VehicleLogTableViewProps = {}) { - // Custom theme for the table - const theme = themeQuartz.withParams(TABLE_THEME_PARAMS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx` at line 82, The local const theme = themeQuartz.withParams(TABLE_THEME_PARAMS) is recreated on every render causing AG Grid to re-reconcile; hoist the theme creation out of the component (make a module-level const) or memoize it with useMemo(() => themeQuartz.withParams(TABLE_THEME_PARAMS), []) so the reference is stable; update references to the existing theme variable and ensure TABLE_THEME_PARAMS and themeQuartz.withParams are used only once to produce the single shared theme instance.src/assets/icons/vehicle.tsx (1)
15-18: Hardcodedfill="#434343"ignores CSS theming.All other icons in the repo (
user,clock,calendar) use stroke-based CSS-variable-driven colors. This path hardcodes#434343, so it won't adapt if the app's colour palette changes or a dark mode is added. Consider accepting afillprop (defaulting to"currentColor") like:🎨 Proposed fix
<path - fill="#434343" + fill={props.fill ?? "currentColor"} d="M11 7.5a1.5 1.5 0 0 0-1.415 1.002L8 13H6..." />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/icons/vehicle.tsx` around lines 15 - 18, The vehicle icon currently hardcodes fill="#434343" on the <path> which prevents CSS theming; update the component in src/assets/icons/vehicle.tsx (the exported Vehicle icon component and its <path>) to accept a fill prop (default "currentColor") or use "currentColor" directly instead of "#434343", pass that prop into the <path> fill attribute, and ensure the component forwards className/style so theme CSS variables can control its color like the other icons (user, clock, calendar).
🤖 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/vehiclelogcomponents/vehicle-log-table-view.tsx`:
- Around line 135-141: The KM_DRIVEN column's valueFormatter (for
COLUMN_IDS.KM_DRIVEN) returns the raw number instead of using toLocaleString
like the odometer columns; update the valueFormatter used in
vehicle-log-table-view.tsx for COLUMN_IDS.KM_DRIVEN to call
params.value.toLocaleString() before appending " KM" and keep the null check
(e.g., if params.value == null return ""), matching the pattern used by the
odometer formatters and HeaderWithIcon.
In `@src/app/admin/vehicle-logs/page.tsx`:
- Around line 38-55: The validations in odometerStart and odometerEnd accept 0
(num < 0) but the error text says "Must be a positive number"; update the
messages to reflect the actual rule (e.g., change both occurrences of "Must be a
positive number" to "Must be a non-negative number") so the text matches the
validation logic, keeping the existing numeric checks (Number.parseFloat,
Number.isNaN, num < 0) and the odometerEnd start-vs-end comparison unchanged.
- Around line 67-80: handleRowClick is discarding the log.ID so the update
mutation can't target the row; update the form's shape to include an id field in
initialValues and set that id when opening the editor. Specifically, add an id
property to the form's initialValues (matching the form instance used in this
component) and in handleRowClick call form.setValues({... , id: log.ID}) so the
edit path (when setIsEditMode(true) and the tRPC update runs) receives the
record ID to update.
---
Nitpick comments:
In `@src/app/_components/vehiclelogcomponents/vehicle-log-table-view.tsx`:
- Around line 154-161: The defaultColDef object (defaultColDef: ColDef) is
re-created on every render causing unnecessary AG Grid updates; stabilize it by
moving its declaration outside the component function or memoizing it with
useMemo (e.g., const defaultColDef = useMemo(() => ({ flex:1, width:
DEFAULT_COLUMN_WIDTH, minWidth: MIN_COLUMN_WIDTH, resizable:false,
sortable:true, filter:true }), [])) so the reference remains stable when passed
to the grid in vehicle-log-table-view.tsx.
- Line 82: The local const theme = themeQuartz.withParams(TABLE_THEME_PARAMS) is
recreated on every render causing AG Grid to re-reconcile; hoist the theme
creation out of the component (make a module-level const) or memoize it with
useMemo(() => themeQuartz.withParams(TABLE_THEME_PARAMS), []) so the reference
is stable; update references to the existing theme variable and ensure
TABLE_THEME_PARAMS and themeQuartz.withParams are used only once to produce the
single shared theme instance.
In `@src/app/admin/vehicle-logs/page.tsx`:
- Around line 85-92: The code currently computes hasErrors with
Object.keys(validation.errors).length > 0 after calling form.validate(); replace
that with the built-in boolean returned by form.validate(), i.e., use
validation.hasErrors directly (update the hasErrors assignment and subsequent
conditional that calls notify.error and setLoading) so the check becomes if
(validation.hasErrors) {...} using the existing validation, form.validate(),
notify.error and setLoading symbols.
In `@src/assets/icons/vehicle.tsx`:
- Around line 15-18: The vehicle icon currently hardcodes fill="#434343" on the
<path> which prevents CSS theming; update the component in
src/assets/icons/vehicle.tsx (the exported Vehicle icon component and its
<path>) to accept a fill prop (default "currentColor") or use "currentColor"
directly instead of "#434343", pass that prop into the <path> fill attribute,
and ensure the component forwards className/style so theme CSS variables can
control its color like the other icons (user, clock, calendar).
ℹ️ 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.tsxsrc/app/admin/vehicle-logs/page.tsxsrc/assets/icons/vehicle.tsxsrc/constants/VehicleLogTableConstants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/_components/vehiclelogcomponents/vehicle-log-form.tsx
Comment on lines
+67
to
+80
| const handleRowClick = (log: VehicleLogData) => { | ||
| setIsEditMode(true); | ||
| form.setValues({ | ||
| date: log.DATE || null, | ||
| destination: log.DESTINATION, | ||
| departureTime: log.DEPARTURE_TIME || null, | ||
| arrivalTime: log.ARRIVAL_TIME || null, | ||
| odometerStart: log.ODOMETER_START.toString(), | ||
| odometerEnd: log.ODOMETER_END.toString(), | ||
| driver: log.DRIVER, | ||
| vehicle: log.VEHICLE, | ||
| }); | ||
| setShowModal(true); | ||
| }; |
There was a problem hiding this comment.
log.ID is available but not tracked — edit mutation will have no row to target.
handleRowClick receives the full VehicleLogData (including ID) but only maps field values, discarding ID. The form's initialValues has no id field and there is no separate useState for it. When the tRPC mutation is wired up, the edit path will have no way to identify which row to update. The PR comment specifically notes "Added id… ensured they are passed to the update API", but this isn't reflected in the code.
Add the id field to the form's initialValues and populate it in handleRowClick:
🐛 Proposed fix
const form = useForm({
initialValues: {
+ id: null as number | null,
date: null as string | null,
destination: "",
...
},
...
});
const handleRowClick = (log: VehicleLogData) => {
setIsEditMode(true);
form.setValues({
+ id: log.ID,
date: log.DATE || null,
destination: log.DESTINATION,
...
});
setShowModal(true);
};
// And in handleAddToLog, form.reset() already clears it to null — no change needed.🤖 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 67 - 80, handleRowClick is
discarding the log.ID so the update mutation can't target the row; update the
form's shape to include an id field in initialValues and set that id when
opening the editor. Specifically, add an id property to the form's initialValues
(matching the form instance used in this component) and in handleRowClick call
form.setValues({... , id: log.ID}) so the edit path (when setIsEditMode(true)
and the tRPC update runs) receives the record ID to update.
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!
promatty
approved these changes
Feb 25, 2026
Contributor
promatty
left a comment
promatty
left a comment
There was a problem hiding this comment.
lgtm. be sure to address the log.ID tracking issue in sanc60
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.
Changes:
src/server/api/routers/vehicle-logs.tswhich is a tRPC router to fetch all from thelogstableI believe we need a router since we want to keep
vehicle-logsas a client component, so we cannot directly access the database from the browser. The tRPC router provides a secure server-side layer which handles authentication and db queries, keeping the client component only fetching data from the tRPC client.Summary by CodeRabbit