SANC-62: Review Transport Request Modal Frontend#60
Draft
jason-duong4509 wants to merge 3 commits intomainfrom
Draft
SANC-62: Review Transport Request Modal Frontend#60jason-duong4509 wants to merge 3 commits intomainfrom
jason-duong4509 wants to merge 3 commits intomainfrom
Conversation
- Added the review transport request modal
jason-duong4509
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
promatty,
tanvimahal,
themaxboucher and
wesleylui
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Fixed the coloring of the rating faces - Changed the Contact Info field to Phone Number - Made the Contact Info field optional - Renamed the bookings.passenger_info column to bookings.passenger_name
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRenamed booking/survey field Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 7
🧹 Nitpick comments (3)
src/assets/icons/rating4.tsx (1)
9-35: LGTM — color updates look correct; pre-existing duplicate<path>can be cleaned up optionally.Lines 22–28 have an identical
dattribute to lines 15–21 (both draw the same circle outline). With matching stroke and fill, there is no visible effect, but the redundant element is unnecessary.♻️ Optional cleanup
<path stroke="#a2e101" strokeLinecap="round" strokeLinejoin="round" strokeWidth={3} d="M16 29.333c7.364 0 13.333-5.97 13.333-13.333 0-7.364-5.97-13.333-13.333-13.333C8.636 2.667 2.667 8.637 2.667 16c0 7.364 5.97 13.333 13.333 13.333Z" /> - <path - stroke="#a2e101" - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth={3} - d="M16 29.333c7.364 0 13.333-5.97 13.333-13.333 0-7.364-5.97-13.333-13.333-13.333C8.636 2.667 2.667 8.637 2.667 16c0 7.364 5.97 13.333 13.333 13.333Z" - />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/icons/rating4.tsx` around lines 9 - 35, There is a duplicated SVG circle path in the Rating4 component—two <path> elements with identical stroke/strokeWidth/strokeLinecap/strokeLinejoin and the same d="M16 29.333...". Remove the redundant <path> (either the first or second occurrence around the circle outline) so only one circle outline path remains, leaving the other unique paths (the mouth/eyes path) untouched.src/app/_components/drivercomponents/surveyNotification/surveyNotification.tsx (1)
141-141: Prefer|| undefinedover|| ""to avoid storing an empty string whenpassengerNameis absent.
z.string().optional()on the API accepts empty strings, so|| ""would persist""to the DB rather than leaving the column null/unset. Usingundefinedcauses the field to be omitted from the payload entirely, which is the expected behaviour for an optional field.✏️ Proposed fix
- passengerName: survey.passengerName || "", + passengerName: survey.passengerName || undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/drivercomponents/surveyNotification/surveyNotification.tsx` at line 141, The object construction in surveyNotification (the field passengerName: survey.passengerName || "") should use undefined instead of an empty string so optional z.string().optional() fields are omitted rather than persisted as ""; change the assignment to passengerName: survey.passengerName || undefined so the payload omits the key when absent and avoids storing an empty string in the DB.src/app/_components/admincomponents/transport-request-modal.tsx (1)
62-68: Remove redundant props that are already MantineModal defaults.
withCloseButton,closeOnClickOutside, andcloseOnEscapeall default totruein Mantine, andsize={"lg"}can be written assize="lg". Explicitly setting them to their defaults adds noise without value.♻️ Proposed cleanup
- size={"lg"} + size="lg" fullScreen={isMobile} padding="xl" - withCloseButton={true} - closeOnClickOutside={true} - closeOnEscape={true} transitionProps={{ transition: "slide-left" }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/admincomponents/transport-request-modal.tsx` around lines 62 - 68, In the TransportRequestModal component, remove redundant Mantine Modal props: drop withCloseButton, closeOnClickOutside, and closeOnEscape since they default to true, and simplify size={"lg"} to size="lg"; update the JSX where the Modal (or the component rendering it) sets these props so only non-default props like fullScreen={isMobile}, padding="xl", and transitionProps remain.
🤖 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/admincomponents/admin-dashboard.tsx`:
- Around line 84-110: Remove the temporary Button labeled "temp review transport
modal button" and delete all console.log calls inside the TransportRequestModal
handlers; update the onApprove prop handler to call
setShowTransportRequestModal(false) (matching onDecline/onClose) so approving
also closes the modal — locate the Button element that triggers
setShowTransportRequestModal, and the TransportRequestModal props onClose,
onDecline, and onApprove in admin-dashboard.tsx to apply these changes.
In `@src/app/_components/admincomponents/transport-request-modal.module.scss`:
- Around line 1-37: Update the SCSS to satisfy Stylelint: change the top comment
to "/* Variables */" (add inner spaces), set $headings-font-weight from 1000 to
900, replace "word-break: break-word" in .agencyName with "overflow-wrap:
break-word", convert .fieldHeadings to use a %placeholder (e.g. %fieldHeadings)
and update .pastHistoryHeading to "@extend %fieldHeadings;" instead of extending
the class, add a blank line before the padding-bottom override in
.pastHistoryHeading and change "padding-bottom: 0rem" to "padding-bottom: 0".
In `@src/app/_components/admincomponents/transport-request-modal.tsx`:
- Around line 13-15: The props for TransportRequestModal use overly permissive
callback types; update the prop type signatures for onClose, onDecline, and
onApprove in transport-request-modal.tsx from (...args: any[]) => void to () =>
void so callers must pass no-arg functions and TypeScript can catch accidental
argument usage; locate the props/interface or type where these three symbols are
declared and replace their types accordingly, then run type-check to ensure all
call sites (e.g., admin-dashboard.tsx and MantineModal usage) remain valid.
- Around line 45-50: The Past History visibility logic incorrectly treats
requestedDiffLocation=false as missing; update the guard that sets
showPastHistory to only consider requestedDiffLocation undefined (not falsy). In
the block that uses transportRating, prevDriverNotes, and requestedDiffLocation
(where showPastHistory is set), change the requestedDiffLocation check to an
explicit undefined check (e.g., requestedDiffLocation === undefined) so that a
legitimate false value does not hide the Past History section; leave the other
checks for transportRating and prevDriverNotes as-is.
- Around line 70-250: The left-column static labels are rendered with <Textarea>
which is focusable and not semantically linked to their value fields; replace
those label Textarea usages with non-focusable semantics (e.g., <Text> or
<label>) and ensure each value Textarea (e.g., those showing residentName,
phoneNumber, additionalInfo, dateAndTime, purpose, destinationAddress,
prevDriverNotes, requestedDiffLocation) has either an id and the label uses
htmlFor or the value Textarea has aria-labelledby referencing the label element;
update all rows (including the Past History block controlled by showPastHistory
and the transportRating display) to follow this pattern so labels are not
tabbable and screen readers correctly associate label→value.
In `@src/server/api/routers/trip.ts`:
- Line 27: The passengerName template currently adds an unconditional space
before the conditional additionalInfo which yields a trailing space when
additionalInfo is absent; update the passengerName expression (the assignment
using input.residentName and input.additionalInfo) so the space is included only
when input.additionalInfo is present (e.g., append `' ' + input.additionalInfo`
or include the space inside the truthy branch) to avoid creating "John Doe "
when additionalInfo is undefined.
In `@src/server/db/booking-schema.ts`:
- Line 14: You renamed the column passenger_info to passenger_name in the schema
(passengerName: text("passenger_name").notNull()), so create and commit a
migration that renames the column on the database to avoid data loss: add a SQL
migration containing ALTER TABLE bookings RENAME COLUMN passenger_info TO
passenger_name; and ALTER TABLE post_trip_surveys RENAME COLUMN passenger_info
TO passenger_name; then run your migration tool (e.g., drizzle-kit push or
migrate) to apply it and verify both tables use passenger_name.
---
Duplicate comments:
In `@src/app/_components/common/calendar/calendar-view.tsx`:
- Line 70: The assignment passengerName: booking.passengerName can produce the
same null/coercion mismatch as in table-view.tsx; fix it by normalizing the
value to the expected type (e.g. string) using a safe fallback like
passengerName: booking?.passengerName ?? '' or passengerName:
String(booking?.passengerName ?? '') so the field never becomes null/undefined
and matches the expected type used elsewhere (mirror the fix applied in
table-view.tsx).
In `@src/server/db/post-trip-schema.ts`:
- Line 44: The schema change renames passenger_info → passenger_name for the
post_trip_surveys table (field passengerName: text("passenger_name")), but
there’s no migration to preserve existing data; add a proper migration that
renames the column on post_trip_surveys from passenger_info to passenger_name
(or copies data then drops old column) so data isn’t lost, and update the schema
reference (passengerName) to match; mirror the same migration pattern used for
bookings (see booking-schema.ts) to ensure safe rollout and rollback.
---
Nitpick comments:
In `@src/app/_components/admincomponents/transport-request-modal.tsx`:
- Around line 62-68: In the TransportRequestModal component, remove redundant
Mantine Modal props: drop withCloseButton, closeOnClickOutside, and
closeOnEscape since they default to true, and simplify size={"lg"} to size="lg";
update the JSX where the Modal (or the component rendering it) sets these props
so only non-default props like fullScreen={isMobile}, padding="xl", and
transitionProps remain.
In
`@src/app/_components/drivercomponents/surveyNotification/surveyNotification.tsx`:
- Line 141: The object construction in surveyNotification (the field
passengerName: survey.passengerName || "") should use undefined instead of an
empty string so optional z.string().optional() fields are omitted rather than
persisted as ""; change the assignment to passengerName: survey.passengerName ||
undefined so the payload omits the key when absent and avoids storing an empty
string in the DB.
In `@src/assets/icons/rating4.tsx`:
- Around line 9-35: There is a duplicated SVG circle path in the Rating4
component—two <path> elements with identical
stroke/strokeWidth/strokeLinecap/strokeLinejoin and the same d="M16 29.333...".
Remove the redundant <path> (either the first or second occurrence around the
circle outline) so only one circle outline path remains, leaving the other
unique paths (the mouth/eyes path) untouched.
Comment on lines
+1
to
+37
| @use "@/styles/_variables" as *; | ||
|
|
||
| /*Variables in this file*/ | ||
| $headings-font-weight: 1000; | ||
| $padding-bottom-amount: 1rem; | ||
|
|
||
| /*Styles*/ | ||
| .title { | ||
| font-weight: $headings-font-weight; | ||
| font-size: x-large; | ||
| } | ||
|
|
||
| .agencyName { | ||
| word-break: break-word; | ||
| font-size: larger; | ||
| font-weight: $headings-font-weight; | ||
| } | ||
|
|
||
| .fieldHeadings { | ||
| color: $color-cherry-red; | ||
| font-weight: $headings-font-weight; | ||
| font-size: large; | ||
| padding-bottom: $padding-bottom-amount; | ||
| } | ||
|
|
||
| .pastHistoryHeading { | ||
| @extend .fieldHeadings; | ||
| padding-bottom: 0rem; | ||
| } | ||
|
|
||
| .grid { | ||
| padding-bottom: $padding-bottom-amount; | ||
| } | ||
|
|
||
| .ratingText { | ||
| font-size: small; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Address all Stylelint errors before merge.
Several issues flagged by Stylelint (reported as errors):
| Line | Rule | Fix |
|---|---|---|
| 3, 7 | comment-whitespace-inside |
/* Variables */ → add inner spaces |
| 14 | declaration-property-value-keyword-no-deprecated |
word-break: break-word is deprecated — replace with overflow-wrap: break-word |
| 27 | scss/at-extend-no-missing-placeholder |
Extend a %placeholder, not a class selector, to avoid unintended CSS coupling |
| 28 | declaration-empty-line-before |
Add a blank line before the padding-bottom override |
Additionally, padding-bottom: 0rem (line 28) should be padding-bottom: 0 (units are meaningless on zero), and $headings-font-weight: 1000 (line 4) exceeds what most fonts expose — 900 is the practical maximum for font-weight.
♻️ Proposed fixes
-/*Variables in this file*/
+/* Variables in this file */
$headings-font-weight: 1000;
+/* ^^^ consider 900 — 1000 is spec-valid but rarely distinguishable */
$padding-bottom-amount: 1rem;
-/*Styles*/
+/* Styles */
-.fieldHeadings {
+%fieldHeadings {
color: $color-cherry-red;
font-weight: $headings-font-weight;
font-size: large;
padding-bottom: $padding-bottom-amount;
}
+.fieldHeadings {
+ `@extend` %fieldHeadings;
+}
.pastHistoryHeading {
- `@extend` .fieldHeadings;
- padding-bottom: 0rem;
+ `@extend` %fieldHeadings;
+
+ padding-bottom: 0;
}
.agencyName {
word-break: break-word; /* deprecated */
+ overflow-wrap: break-word;
- word-break: break-word;
font-size: larger;
font-weight: $headings-font-weight;
}🧰 Tools
🪛 Stylelint (17.3.0)
[error] 27-27: Expected a placeholder selector (e.g. %placeholder) to be used in @extend (scss/at-extend-no-missing-placeholder)
(scss/at-extend-no-missing-placeholder)
[error] 1-1: Unexpected leading underscore in imported partial name (scss/load-no-partial-leading-underscore)
(scss/load-no-partial-leading-underscore)
[error] 3-3: Expected whitespace after "/*" (comment-whitespace-inside)
(comment-whitespace-inside)
[error] 3-3: Expected whitespace before "*/" (comment-whitespace-inside)
(comment-whitespace-inside)
[error] 7-7: Expected whitespace after "/*" (comment-whitespace-inside)
(comment-whitespace-inside)
[error] 7-7: Expected whitespace before "*/" (comment-whitespace-inside)
(comment-whitespace-inside)
[error] 28-28: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 14-14: Unexpected deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/_components/admincomponents/transport-request-modal.module.scss`
around lines 1 - 37, Update the SCSS to satisfy Stylelint: change the top
comment to "/* Variables */" (add inner spaces), set $headings-font-weight from
1000 to 900, replace "word-break: break-word" in .agencyName with
"overflow-wrap: break-word", convert .fieldHeadings to use a %placeholder (e.g.
%fieldHeadings) and update .pastHistoryHeading to "@extend %fieldHeadings;"
instead of extending the class, add a blank line before the padding-bottom
override in .pastHistoryHeading and change "padding-bottom: 0rem" to
"padding-bottom: 0".
src/app/_components/admincomponents/transport-request-modal.tsx
Outdated
Show resolved
Hide resolved
src/app/_components/admincomponents/transport-request-modal.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+70
to
+250
| <Text className={ui.fieldHeadings}>Personal Information</Text> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Resident Name"} /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly autosize maxRows={2} value={residentName} /> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| {phoneNumber && ( | ||
| <> | ||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Phone Number"} /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly autosize maxRows={2} value={phoneNumber} /> | ||
| </GridCol> | ||
| </Grid> | ||
| </> | ||
| )} | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| rows={5} | ||
| value={ | ||
| "Additional Information (pets, personal belongings, support required entering / exiting vehicle, etc.)" | ||
| } | ||
| /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly rows={5} value={additionalInfo} /> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| <Text className={ui.fieldHeadings}>Logistics</Text> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| autosize | ||
| maxRows={3} | ||
| minRows={2} | ||
| value={"Date and Time of Transport"} | ||
| /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly autosize maxRows={3} value={dateAndTime} /> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Purpose"} /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly autosize maxRows={5} value={purpose} /> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| autosize | ||
| maxRows={2} | ||
| value={"Destination Address"} | ||
| /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly autosize maxRows={2} value={destinationAddress} /> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| {showPastHistory && ( | ||
| <> | ||
| <Text className={ui.pastHistoryHeading}>Past History</Text> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| autosize | ||
| className={ui.grid} | ||
| value={ | ||
| "This resident has previously used the Navigation Centre's services.\nView the details from their previous ride below." | ||
| } | ||
| /> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| autosize | ||
| maxRows={2} | ||
| value={"Transport fitness rating"} | ||
| /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Group gap={"xs"}> | ||
| {transportRating === 1 && ( | ||
| <> | ||
| <Rating1 /> | ||
| <Text className={ui.ratingText}>Poor</Text> | ||
| </> | ||
| )} | ||
| {transportRating === 2 && ( | ||
| <> | ||
| <Rating2 /> | ||
| <Text className={ui.ratingText}>Okay</Text> | ||
| </> | ||
| )} | ||
| {transportRating === 3 && ( | ||
| <> | ||
| <Rating3 /> | ||
| <Text className={ui.ratingText}>Standard</Text> | ||
| </> | ||
| )} | ||
| {transportRating === 4 && ( | ||
| <> | ||
| <Rating4 /> | ||
| <Text className={ui.ratingText}>Good</Text> | ||
| </> | ||
| )} | ||
| {transportRating === 5 && ( | ||
| <> | ||
| <Rating5 /> | ||
| <Text className={ui.ratingText}>Excellent</Text> | ||
| </> | ||
| )} | ||
| </Group> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| autosize | ||
| maxRows={4} | ||
| value={"Previous driver notes"} | ||
| /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly autosize maxRows={4} value={prevDriverNotes} /> | ||
| </GridCol> | ||
| </Grid> | ||
|
|
||
| <Grid gutter={"xl"} className={ui.grid}> | ||
| <GridCol span={4}> | ||
| <Textarea | ||
| variant="unstyled" | ||
| readOnly | ||
| rows={5} | ||
| value={ | ||
| "Did the passenger request to be dropped off at a different location than originally booked?" | ||
| } | ||
| /> | ||
| </GridCol> | ||
|
|
||
| <GridCol span={8}> | ||
| <Textarea readOnly rows={1} value={requestedDiffLocation ? "Yes" : "No"} /> | ||
| </GridCol> | ||
| </Grid> | ||
| </> |
There was a problem hiding this comment.
Accessibility: <Textarea readOnly> must not be used for static label text.
Every left-column cell (e.g., "Resident Name", "Date and Time of Transport", "Previous driver notes") is rendered as a <textarea> HTML element. This causes two problems:
- Keyboard navigation: Read-only
<textarea>elements are still focusable by default; keyboard users must tab through every label before reaching the actual value, creating unnecessary focus noise. - No semantic association: None of the label textareas are linked to their adjacent value fields via
htmlFor/idoraria-labelledby, so screen readers cannot announce what data a value field represents.
Replace the static label column with <Text> (or <label>), and add proper aria-label or id/aria-labelledby pairing if keeping value <Textarea> elements.
♻️ Example fix for one grid row (apply pattern to all rows)
- <GridCol span={4}>
- <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Resident Name"} />
- </GridCol>
- <GridCol span={8}>
- <Textarea readOnly autosize maxRows={2} value={residentName} />
- </GridCol>
+ <GridCol span={4}>
+ <Text id="label-resident-name" fw={500}>Resident Name</Text>
+ </GridCol>
+ <GridCol span={8}>
+ <Textarea
+ readOnly
+ autosize
+ maxRows={2}
+ value={residentName}
+ aria-labelledby="label-resident-name"
+ />
+ </GridCol>📝 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
| <Text className={ui.fieldHeadings}>Personal Information</Text> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Resident Name"} /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={2} value={residentName} /> | |
| </GridCol> | |
| </Grid> | |
| {phoneNumber && ( | |
| <> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Phone Number"} /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={2} value={phoneNumber} /> | |
| </GridCol> | |
| </Grid> | |
| </> | |
| )} | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| rows={5} | |
| value={ | |
| "Additional Information (pets, personal belongings, support required entering / exiting vehicle, etc.)" | |
| } | |
| /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly rows={5} value={additionalInfo} /> | |
| </GridCol> | |
| </Grid> | |
| <Text className={ui.fieldHeadings}>Logistics</Text> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| autosize | |
| maxRows={3} | |
| minRows={2} | |
| value={"Date and Time of Transport"} | |
| /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={3} value={dateAndTime} /> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea variant="unstyled" readOnly autosize maxRows={2} value={"Purpose"} /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={5} value={purpose} /> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| autosize | |
| maxRows={2} | |
| value={"Destination Address"} | |
| /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={2} value={destinationAddress} /> | |
| </GridCol> | |
| </Grid> | |
| {showPastHistory && ( | |
| <> | |
| <Text className={ui.pastHistoryHeading}>Past History</Text> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| autosize | |
| className={ui.grid} | |
| value={ | |
| "This resident has previously used the Navigation Centre's services.\nView the details from their previous ride below." | |
| } | |
| /> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| autosize | |
| maxRows={2} | |
| value={"Transport fitness rating"} | |
| /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Group gap={"xs"}> | |
| {transportRating === 1 && ( | |
| <> | |
| <Rating1 /> | |
| <Text className={ui.ratingText}>Poor</Text> | |
| </> | |
| )} | |
| {transportRating === 2 && ( | |
| <> | |
| <Rating2 /> | |
| <Text className={ui.ratingText}>Okay</Text> | |
| </> | |
| )} | |
| {transportRating === 3 && ( | |
| <> | |
| <Rating3 /> | |
| <Text className={ui.ratingText}>Standard</Text> | |
| </> | |
| )} | |
| {transportRating === 4 && ( | |
| <> | |
| <Rating4 /> | |
| <Text className={ui.ratingText}>Good</Text> | |
| </> | |
| )} | |
| {transportRating === 5 && ( | |
| <> | |
| <Rating5 /> | |
| <Text className={ui.ratingText}>Excellent</Text> | |
| </> | |
| )} | |
| </Group> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| autosize | |
| maxRows={4} | |
| value={"Previous driver notes"} | |
| /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={4} value={prevDriverNotes} /> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| rows={5} | |
| value={ | |
| "Did the passenger request to be dropped off at a different location than originally booked?" | |
| } | |
| /> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly rows={1} value={requestedDiffLocation ? "Yes" : "No"} /> | |
| </GridCol> | |
| </Grid> | |
| </> | |
| <Text className={ui.fieldHeadings}>Personal Information</Text> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-resident-name" fw={500}>Resident Name</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={2} value={residentName} aria-labelledby="label-resident-name" /> | |
| </GridCol> | |
| </Grid> | |
| {phoneNumber && ( | |
| <> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-phone-number" fw={500}>Phone Number</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={2} value={phoneNumber} aria-labelledby="label-phone-number" /> | |
| </GridCol> | |
| </Grid> | |
| </> | |
| )} | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-additional-info" fw={500}>Additional Information (pets, personal belongings, support required entering / exiting vehicle, etc.)</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly rows={5} value={additionalInfo} aria-labelledby="label-additional-info" /> | |
| </GridCol> | |
| </Grid> | |
| <Text className={ui.fieldHeadings}>Logistics</Text> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-date-time" fw={500}>Date and Time of Transport</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={3} value={dateAndTime} aria-labelledby="label-date-time" /> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-purpose" fw={500}>Purpose</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={5} value={purpose} aria-labelledby="label-purpose" /> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-destination" fw={500}>Destination Address</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={2} value={destinationAddress} aria-labelledby="label-destination" /> | |
| </GridCol> | |
| </Grid> | |
| {showPastHistory && ( | |
| <> | |
| <Text className={ui.pastHistoryHeading}>Past History</Text> | |
| <Textarea | |
| variant="unstyled" | |
| readOnly | |
| autosize | |
| className={ui.grid} | |
| value={ | |
| "This resident has previously used the Navigation Centre's services.\nView the details from their previous ride below." | |
| } | |
| /> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-transport-rating" fw={500}>Transport fitness rating</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Group gap={"xs"} aria-labelledby="label-transport-rating"> | |
| {transportRating === 1 && ( | |
| <> | |
| <Rating1 /> | |
| <Text className={ui.ratingText}>Poor</Text> | |
| </> | |
| )} | |
| {transportRating === 2 && ( | |
| <> | |
| <Rating2 /> | |
| <Text className={ui.ratingText}>Okay</Text> | |
| </> | |
| )} | |
| {transportRating === 3 && ( | |
| <> | |
| <Rating3 /> | |
| <Text className={ui.ratingText}>Standard</Text> | |
| </> | |
| )} | |
| {transportRating === 4 && ( | |
| <> | |
| <Rating4 /> | |
| <Text className={ui.ratingText}>Good</Text> | |
| </> | |
| )} | |
| {transportRating === 5 && ( | |
| <> | |
| <Rating5 /> | |
| <Text className={ui.ratingText}>Excellent</Text> | |
| </> | |
| )} | |
| </Group> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-prev-driver-notes" fw={500}>Previous driver notes</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly autosize maxRows={4} value={prevDriverNotes} aria-labelledby="label-prev-driver-notes" /> | |
| </GridCol> | |
| </Grid> | |
| <Grid gutter={"xl"} className={ui.grid}> | |
| <GridCol span={4}> | |
| <Text id="label-diff-location" fw={500}>Did the passenger request to be dropped off at a different location than originally booked?</Text> | |
| </GridCol> | |
| <GridCol span={8}> | |
| <Textarea readOnly rows={1} value={requestedDiffLocation ? "Yes" : "No"} aria-labelledby="label-diff-location" /> | |
| </GridCol> | |
| </Grid> | |
| </> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/_components/admincomponents/transport-request-modal.tsx` around lines
70 - 250, The left-column static labels are rendered with <Textarea> which is
focusable and not semantically linked to their value fields; replace those label
Textarea usages with non-focusable semantics (e.g., <Text> or <label>) and
ensure each value Textarea (e.g., those showing residentName, phoneNumber,
additionalInfo, dateAndTime, purpose, destinationAddress, prevDriverNotes,
requestedDiffLocation) has either an id and the label uses htmlFor or the value
Textarea has aria-labelledby referencing the label element; update all rows
(including the Past History block controlled by showPastHistory and the
transportRating display) to follow this pattern so labels are not tabbable and
screen readers correctly associate label→value.
| pickupAddress: input.pickupAddress, | ||
| destinationAddress: input.destinationAddress, | ||
| passengerInfo: `${input.residentName} ${input.additionalInfo ? `${input.additionalInfo}` : ""}`, | ||
| passengerName: `${input.residentName} ${input.additionalInfo ? `${input.additionalInfo}` : ""}`, |
There was a problem hiding this comment.
Trailing space in passengerName when additionalInfo is absent.
The current template produces "John Doe " (with a trailing space) when additionalInfo is undefined, because the space before the conditional is unconditional.
🐛 Proposed fix
- passengerName: `${input.residentName} ${input.additionalInfo ? `${input.additionalInfo}` : ""}`,
+ passengerName: `${input.residentName}${input.additionalInfo ? ` ${input.additionalInfo}` : ""}`,📝 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
| passengerName: `${input.residentName} ${input.additionalInfo ? `${input.additionalInfo}` : ""}`, | |
| passengerName: `${input.residentName}${input.additionalInfo ? ` ${input.additionalInfo}` : ""}`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/trip.ts` at line 27, The passengerName template
currently adds an unconditional space before the conditional additionalInfo
which yields a trailing space when additionalInfo is absent; update the
passengerName expression (the assignment using input.residentName and
input.additionalInfo) so the space is included only when input.additionalInfo is
present (e.g., append `' ' + input.additionalInfo` or include the space inside
the truthy branch) to avoid creating "John Doe " when additionalInfo is
undefined.
| destinationAddress: text("destination_address").notNull(), | ||
| purpose: text("purpose"), | ||
| passengerInfo: text("passenger_info").notNull(), | ||
| passengerName: text("passenger_name").notNull(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check directory structure for migrations
echo "=== Checking for migrations directory ==="
fd . -type d -name migrations
echo -e "\n=== Searching for migration files ==="
fd -e sql -e ts . -path '*/migration*' 2>/dev/null | head -20
echo -e "\n=== Searching for 'passenger' references in migration files ==="
rg "passenger" --type sql --type ts -g '**/{migration*,drizzle}/**' 2>/dev/null || echo "No matches found"
echo -e "\n=== Checking for ALTER TABLE RENAME in any SQL/TS migrations ==="
rg "ALTER TABLE.*RENAME|RENAME COLUMN" --type sql --type ts 2>/dev/null | head -20Repository: Code-the-Change-YYC/salvationarmy
Length of output: 1235
🏁 Script executed:
#!/bin/bash
echo "=== Directory structure of drizzle folder ==="
fd . drizzle --type f 2>/dev/null | head -30
echo -e "\n=== All .sql files in repository ==="
fd -e sql 2>/dev/null
echo -e "\n=== Checking git status for any migration files ==="
git ls-files | grep -i migration
echo -e "\n=== Check if drizzle.config exists and what it references ==="
fd drizzle.config -e ts -e js
cat drizzle.config.ts 2>/dev/null | head -30Repository: Code-the-Change-YYC/salvationarmy
Length of output: 884
Migration required for column rename.
No migration files exist in this repository. Renaming passenger_info to passenger_name in both bookings and post_trip_surveys tables requires an explicit migration. Without it, drizzle-kit push may drop and recreate the column (losing existing data), while drizzle-kit generate requires interactive rename confirmation. On deployed databases, the schema mismatch will cause runtime failures.
Generate and commit a migration that includes:
ALTER TABLE bookings RENAME COLUMN passenger_info TO passenger_name;
ALTER TABLE post_trip_surveys RENAME COLUMN passenger_info TO passenger_name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/db/booking-schema.ts` at line 14, You renamed the column
passenger_info to passenger_name in the schema (passengerName:
text("passenger_name").notNull()), so create and commit a migration that renames
the column on the database to avoid data loss: add a SQL migration containing
ALTER TABLE bookings RENAME COLUMN passenger_info TO passenger_name; and ALTER
TABLE post_trip_surveys RENAME COLUMN passenger_info TO passenger_name; then run
your migration tool (e.g., drizzle-kit push or migrate) to apply it and verify
both tables use passenger_name.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/_components/admincomponents/transport-request-modal.tsx (2)
62-65: Remove redundant props that restate Mantine Modal's built-in defaults.Mantine's Modal
defaultPropsexplicitly setscloseOnClickOutside: true,closeOnEscape: true, andwithCloseButton: true, so all three props on lines 63–65 are no-ops and only add noise.♻️ Proposed cleanup
size={"lg"} fullScreen={isMobile} padding="xl" - withCloseButton={true} - closeOnClickOutside={true} - closeOnEscape={true} transitionProps={{ transition: "slide-left" }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/admincomponents/transport-request-modal.tsx` around lines 62 - 65, Remove the redundant Mantine Modal props that restate defaults: in the TransportRequestModal component where Modal is rendered (the JSX with padding="xl"), delete withCloseButton={true}, closeOnClickOutside={true}, and closeOnEscape={true} so the Modal relies on Mantine's defaultProps; keep other props like padding intact.
62-65: Remove redundant Modal props that match Mantine defaults.
withCloseButton={true},closeOnClickOutside={true}, andcloseOnEscape={true}all match Mantine v8 Modal's out-of-the-box defaults. Removing them eliminates visual noise without changing behavior.♻️ Proposed cleanup
size={"lg"} fullScreen={isMobile} padding="xl" - withCloseButton={true} - closeOnClickOutside={true} - closeOnEscape={true} transitionProps={{ transition: "slide-left" }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/admincomponents/transport-request-modal.tsx` around lines 62 - 65, The Modal component in transport-request-modal.tsx is passing redundant props that match Mantine v8 defaults: remove withCloseButton={true}, closeOnClickOutside={true}, and closeOnEscape={true} from the <Modal ... /> usage so the code is cleaner without changing behavior; locate the Modal JSX in the TransportRequestModal component and delete those three props (leave other props intact).
🤖 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/admincomponents/transport-request-modal.tsx`:
- Around line 45-48: The inline comment on the showPastHistory boolean is
incorrect: the expression const showPastHistory = transportRating !== undefined
&& prevDriverNotes !== undefined && requestedDiffLocation !== undefined;
evaluates to true only when all three optional fields (transportRating,
prevDriverNotes, requestedDiffLocation) are defined, not when one is blank;
update the trailing comment to accurately state "True if all three optional
fields are defined (none are blank)" or remove the stale comment entirely so it
no longer misleads future maintainers.
- Around line 45-48: The inline comment next to the showPastHistory declaration
incorrectly states the opposite meaning; update the comment for showPastHistory
(the const in transport-request-modal.tsx) to reflect that it is true only when
all three optional fields (transportRating, prevDriverNotes,
requestedDiffLocation) are defined (none are blank), e.g., change the comment to
indicate "true when all optional fields are present" or similar so future
maintainers aren't misled.
---
Duplicate comments:
In `@src/app/_components/admincomponents/transport-request-modal.tsx`:
- Around line 68-248: Replace all left-column label Textarea instances with
non-focusable Text components and add aria-labelledby attributes to the
corresponding right-column value Textarea components so assistive tech can
associate labels with values; specifically, change the label Textareas (the ones
with values like "Resident Name", "Phone Number", "Additional Information (…)",
"Date and Time of Transport", "Purpose", "Destination Address", the Past History
intro paragraph, "Transport fitness rating", "Previous driver notes", and "Did
the passenger request…") to use the Text component, give each label Text a
unique id (e.g., label-residentName, label-phoneNumber, etc.), then add
aria-labelledby="that-id" to the matching value Textarea elements rendering
residentName, phoneNumber, additionalInfo, dateAndTime, purpose,
destinationAddress, the past-history paragraph, transportRating display,
prevDriverNotes, and requestedDiffLocation so labels are not keyboard-focusable
and screen readers announce context correctly.
- Around line 68-248: Left-column label fields are rendered as focusable
<Textarea> elements causing tab trapping and no label linkage; replace each
left-column label Textarea (the ones whose value strings are "Resident Name",
"Phone Number", "Additional Information (...)", "Date and Time of Transport",
"Purpose", "Destination Address", the Past History intro paragraph, "Transport
fitness rating", "Previous driver notes", and the "Did the passenger request…"
prompt) with non-focusable <Text> components, and add matching id attributes to
those Text nodes (e.g., residentNameLabel, phoneLabel, additionalInfoLabel,
dateTimeLabel, purposeLabel, destinationLabel, pastHistoryIntroLabel,
transportRatingLabel, prevDriverNotesLabel, requestedDiffLocationLabel) then set
aria-labelledby on the corresponding readOnly value Textarea components (the
ones using residentName, phoneNumber, additionalInfo, dateAndTime, purpose,
destinationAddress, the past-history Textarea, transportRating Group container,
prevDriverNotes, and requestedDiffLocation value) so screen readers announce the
label and keyboard tab order skips the static labels; ensure ids are unique and
only applied to the left-column Text elements and aria-labelledby to the
right-column Textarea elements.
---
Nitpick comments:
In `@src/app/_components/admincomponents/transport-request-modal.tsx`:
- Around line 62-65: Remove the redundant Mantine Modal props that restate
defaults: in the TransportRequestModal component where Modal is rendered (the
JSX with padding="xl"), delete withCloseButton={true},
closeOnClickOutside={true}, and closeOnEscape={true} so the Modal relies on
Mantine's defaultProps; keep other props like padding intact.
- Around line 62-65: The Modal component in transport-request-modal.tsx is
passing redundant props that match Mantine v8 defaults: remove
withCloseButton={true}, closeOnClickOutside={true}, and closeOnEscape={true}
from the <Modal ... /> usage so the code is cleaner without changing behavior;
locate the Modal JSX in the TransportRequestModal component and delete those
three props (leave other props intact).
src/app/_components/admincomponents/transport-request-modal.tsx
Outdated
Show resolved
Hide resolved
- Fixed a bug caused when a passenger requested a new location than booked. Originally that would hide past history information from the modal. This does not happen anymore - Changed type definitions of 3 props to exclude functions that accept parameters
jason-duong4509
force-pushed
the
SANC-62-review-transport-request-modal-frontend
branch
from
da0b4e9 to
9587cb6
Compare
Contributor
|
waiting to hear back from the Salvation Army on whether we still need this |
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.
Added the review transport request modal
A video of the modal
Video.mp4
The current PR has changed the admin dashboard to include a sample usage of the modal. The modal appears with prefilled information when pressing the "temp review transport modal button"
Summary by CodeRabbit
New Features
Style
Refactor