Joel/forms revamp#106
Merged
Merged
Conversation
…ickups/deliveries
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 52 minutes and 45 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request enhances the client request and donation forms with stricter validation, required apartment fields, and optional email support. It introduces a new photo upload component, removes item size tracking, and adds inventory category deletion. Filtering logic for pickups and deliveries adapts to client capability flags. Type system, UI components, and layouts are updated throughout to support these changes. ChangesForms, Inventory, and Filtering Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/donate/steps/Step3Review.tsx (2)
50-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix forbidden UUID generation and release photo object URLs in
Step3Review
- Replace
crypto.randomUUID()(atapp/donate/steps/Step3Review.tsx:50and:70) withuuidv4per the project UUID rule (importv4 as uuidv4from"uuid"and update both IDs).URL.createObjectURL(photo)(around:250) is used for previews withoutURL.revokeObjectURL(...)cleanup; add revocation (e.g., viauseEffect) to avoid leaking object URLs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/donate/steps/Step3Review.tsx` at line 50, Replace forbidden crypto.randomUUID() uses in Step3Review with the project's UUID helper: import { v4 as uuidv4 } from "uuid" and replace the two id assignments (the ones currently using crypto.randomUUID() at the spots around id: crypto.randomUUID(), and the second occurrence) with uuidv4(). Also stop leaking image object URLs created with URL.createObjectURL(photo) by creating a previewUrl state (or using the existing preview variable) and adding a useEffect that calls URL.revokeObjectURL(previewUrl) when the photo/previewUrl changes or when the component unmounts; ensure revoke runs in the cleanup function so every created object URL is revoked.
250-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
URL.createObjectURLin render without cleanup
URL.createObjectURL(photo)is created inline in render (e.g.,app/donate/steps/Step3Review.tsxandcomponents/form/PhotoDropzone.tsx) and there’s noURL.revokeObjectURLcleanup anywhere in the repo, which can leak memory across rerenders/navigation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/donate/steps/Step3Review.tsx` at line 250, The image src currently uses URL.createObjectURL(photo) inline in render (in the Step3Review component and PhotoDropzone component), which leaks object URLs; change to create the object URL in an effect and store it in state (e.g., photoPreview or previewUrl) and use that state for src, and inside the same useEffect return a cleanup that calls URL.revokeObjectURL(previewUrl); update any places referencing src={URL.createObjectURL(photo)} to use the new preview state and ensure cleanup runs whenever the photo/file changes or the component unmounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client-request-form/steps/Step1ClientInfo.tsx`:
- Around line 49-50: The validation currently allows whitespace-only values for
preferredLanguage and apt; update the checks in Step1ClientInfo (where
client.questions.clientSpeaksEnglish / client.questions.preferredLanguage and
the apt validation at the other check) to call .trim() on the string before
testing emptiness (e.g., use client.questions.preferredLanguage?.trim() and
apt?.trim()) and only set newErrors.preferredLanguage or newErrors.apt when the
trimmed value is empty; preserve existing branching logic for
clientSpeaksEnglish and required fields.
In `@app/donate/steps/Step1PersonalInfo.tsx`:
- Around line 64-66: Step1PersonalInfo: the apartment/unit field (address.apt)
is now required but not validated or marked in the UI; update the validation in
the component (where formState.donorInfo.address is checked and newErrors is
populated) to check formState.donorInfo.address?.apt and set newErrors.apt =
"Apt/Unit is required" when empty, and also mark the Apt input element as
required (add the required attribute/prop and any aria-required) so users cannot
proceed without it; apply the same validation and UI required flag in the other
related validation block referenced around the 216-232 region.
- Line 320: The string in Step1PersonalInfo (inside the JSX text block near the
pickup-range message) contains an unescaped apostrophe ("isn't"), triggering
react/no-unescaped-entities; update that text to escape the apostrophe (e.g.,
replace "isn't" with "isn't" or "isn&`#39`;t") in the JSX within the
Step1PersonalInfo component so the lint rule is satisfied.
- Around line 97-100: The scroll-to-error logic in the setTimeout callback
inside Step1PersonalInfo.tsx only queries ".form-error" and therefore misses
acknowledgement errors; update the callback used with setTimeout to query for
multiple selectors (e.g., ".form-error, .acknowledgement-error,
[aria-invalid='true']") or otherwise include the acknowledgement error element
(or its aria-invalid state) so the first matching element is found and scrolled
into view using scrollIntoView; modify the document.querySelector call in that
setTimeout block accordingly to pick the appropriate selector(s).
In `@app/donate/steps/Step2AddDonations.tsx`:
- Around line 224-229: The delete button in Step2AddDonations.tsx is an
icon-only control and needs an accessible name; update the button element that
calls removeDonationItem(item.id) to include an aria-label (for example "Remove
item" or "Remove item {item.name}" / include item.id for uniqueness) so screen
readers can identify the control; ensure the attribute is added to the same
button element that renders the Trash icon.
In `@components/client-requests/RequestDetails.tsx`:
- Line 72: Replace the direct nullish coalescing that renders raw booleans for
questions.isVeteran with a ternary that maps true->"Yes" and false->"No", while
still falling back to "—" when the value is null/undefined; locate the display
in RequestDetails.tsx where questions.isVeteran is used and change the rendering
logic to first check for null/undefined and otherwise return "Yes" or "No".
- Line 76: The boolean field questions.wasChronic is being rendered raw which
shows "true"/"false"; update the rendering in RequestDetails (where the JSX
contains questions.wasChronic) to map true->"Yes", false->"No" and keep the
fallback "—" for null/undefined (e.g., use a conditional/ternary that checks
questions.wasChronic === true ? "Yes" : questions.wasChronic === false ? "No" :
"—"). Ensure the same pattern is applied consistently for other boolean fields
in this component that previously rendered raw booleans.
In `@components/form/PhotoDropzone.tsx`:
- Around line 79-88: The remove-photo button (the icon-only element rendering
CloseIcon with the onClick that calls onChange(photos.filter((p) => p !==
photo))) is missing an accessible name; add an aria-label (e.g.,
aria-label="Remove photo" or aria-label={`Remove photo: ${photo.name ||
index}`}) to the button element so screen readers can describe the action while
keeping the existing onClick, onChange, photos, photo and CloseIcon logic
intact.
- Around line 75-76: The render currently calls URL.createObjectURL(photo)
directly in PhotoDropzone which leaks object URLs; change to create and store
the object URL in a React state (e.g., photoPreviewUrl) and use a useEffect that
sets photoPreviewUrl = URL.createObjectURL(photo) when photo changes and calls
URL.revokeObjectURL(oldUrl) in the cleanup (and on unmount) to avoid leaks. Also
update the icon-only remove <button> that renders CloseIcon to include an
accessible name (add an aria-label and/or title like "Remove photo") so screen
readers can announce the action; ensure the same button handler (e.g.,
onRemovePhoto or handleRemove) is preserved.
---
Outside diff comments:
In `@app/donate/steps/Step3Review.tsx`:
- Line 50: Replace forbidden crypto.randomUUID() uses in Step3Review with the
project's UUID helper: import { v4 as uuidv4 } from "uuid" and replace the two
id assignments (the ones currently using crypto.randomUUID() at the spots around
id: crypto.randomUUID(), and the second occurrence) with uuidv4(). Also stop
leaking image object URLs created with URL.createObjectURL(photo) by creating a
previewUrl state (or using the existing preview variable) and adding a useEffect
that calls URL.revokeObjectURL(previewUrl) when the photo/previewUrl changes or
when the component unmounts; ensure revoke runs in the cleanup function so every
created object URL is revoked.
- Line 250: The image src currently uses URL.createObjectURL(photo) inline in
render (in the Step3Review component and PhotoDropzone component), which leaks
object URLs; change to create the object URL in an effect and store it in state
(e.g., photoPreview or previewUrl) and use that state for src, and inside the
same useEffect return a cleanup that calls URL.revokeObjectURL(previewUrl);
update any places referencing src={URL.createObjectURL(photo)} to use the new
preview state and ensure cleanup runs whenever the photo/file changes or the
component unmounts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3622c4d0-222d-4f4e-910c-1623829a8ea3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
app/client-request-form/steps/Step1ClientInfo.tsxapp/client-request-form/steps/Step2Requests.tsxapp/client-request-form/steps/Step3Review.tsxapp/control-panel/categories/page.tsxapp/donate/DonorFormContext.tsxapp/donate/layout.tsxapp/donate/steps/Step1PersonalInfo.tsxapp/donate/steps/Step2AddDonations.tsxapp/donate/steps/Step3Review.tsxapp/donate/steps/Step4Confirmation.tsxapp/globals.cssapp/pickups-deliveries/scheduled/page.tsxapp/pickups-deliveries/unscheduled/page.tsxcomponents/client-requests/RequestDetails.tsxcomponents/control-panel/CategoryModal.tsxcomponents/donation-requests/DRContentsTable.tsxcomponents/donation-requests/ItemReviewModal.tsxcomponents/form/PhotoDropzone.tsxcomponents/form/StepIndicator.tsxcomponents/inventory/WarehouseGallery.tsxcomponents/pickups-deliveries/Request.tsxcomponents/schedule/ShiftDetailOverlay.tsxlib/queries/inventory.tslib/services/inventory.tspackage.jsontypes/client-requests.tstypes/general.tstypes/inventory.ts
💤 Files with no reviewable changes (3)
- app/donate/DonorFormContext.tsx
- components/inventory/WarehouseGallery.tsx
- types/inventory.ts
Comment on lines
+64
to
+66
| if (!formState.donorInfo.address?.city) { | ||
| newErrors.city = "City/Town is required"; | ||
| } |
There was a problem hiding this comment.
Enforce required apartment/unit in validation and UI.
apt is now part of required address input, but this step doesn’t validate it and the field isn’t marked required, so users can proceed with an empty apartment/unit.
Suggested fix
if (!formState.donorInfo.address?.streetAddress) {
newErrors.streetAddress = "Street address is required";
}
+ if (!formState.donorInfo.address?.apt?.trim()) {
+ newErrors.apt = "Apt / Unit is required";
+ }
if (!formState.donorInfo.address?.city) {
newErrors.city = "City/Town is required";
}
@@
<FormInput
label="Apt / Unit"
+ required
value={formState.donorInfo.address?.apt || ""}
@@
/>
+ {errors.apt && <p className="form-error text-red-500 text-sm mt-1">{errors.apt}</p>}Also applies to: 216-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/donate/steps/Step1PersonalInfo.tsx` around lines 64 - 66,
Step1PersonalInfo: the apartment/unit field (address.apt) is now required but
not validated or marked in the UI; update the validation in the component (where
formState.donorInfo.address is checked and newErrors is populated) to check
formState.donorInfo.address?.apt and set newErrors.apt = "Apt/Unit is required"
when empty, and also mark the Apt input element as required (add the required
attribute/prop and any aria-required) so users cannot proceed without it; apply
the same validation and UI required flag in the other related validation block
referenced around the 216-232 region.
Comment on lines
224
to
229
| <button | ||
| type="button" | ||
| onClick={() => removeDonationItem(item.id)} | ||
| className="text-red-500 hover:text-red-700" | ||
| > | ||
| <svg | ||
| className="w-5 h-5" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| //svg path for the icon | ||
| d="M19 7l-.867 12.142A2 2 0 0116.138 21H7.862a2 2 0 01-1.995-1.858L5 7m5 4v6m4-6v6m1-10V4a1 1 0 00-1-1h-4a1 1 0 00-1 1v3M4 7h16" | ||
| /> | ||
| </svg> | ||
| <Trash className="w-4 h-4 text-text-1" /> | ||
| </button> |
There was a problem hiding this comment.
Add aria-label to the item delete button.
This icon-only control should expose an accessible name (e.g., aria-label="Remove item").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/donate/steps/Step2AddDonations.tsx` around lines 224 - 229, The delete
button in Step2AddDonations.tsx is an icon-only control and needs an accessible
name; update the button element that calls removeDonationItem(item.id) to
include an aria-label (for example "Remove item" or "Remove item {item.name}" /
include item.id for uniqueness) so screen readers can identify the control;
ensure the attribute is added to the same button element that renders the Trash
icon.
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.childrenInFamily ?? "—"}</div> | ||
| <div className="font-bold text-sm text-black pl-4 min-h-10 bg-[#FAFAFB] flex items-start py-2 border-t border-r border-light-border">Veteran status</div> | ||
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.isVeteran == null ? "—" : questions.isVeteran ? "Yes" : "No"}</div> | ||
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.isVeteran ?? "—"}</div> |
There was a problem hiding this comment.
Boolean values will display as "true"/"false" instead of "Yes"/"No".
The nullish coalescing operator ?? will render the raw boolean value when questions.isVeteran is true or false, displaying "true" or "false" to users instead of the expected "Yes" or "No".
🐛 Proposed fix
- <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.isVeteran ?? "—"}</div>
+ <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">
+ {questions.isVeteran == null ? "—" : (questions.isVeteran ? "Yes" : "No")}
+ </div>📝 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
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.isVeteran ?? "—"}</div> | |
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border"> | |
| {questions.isVeteran == null ? "—" : (questions.isVeteran ? "Yes" : "No")} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/client-requests/RequestDetails.tsx` at line 72, Replace the direct
nullish coalescing that renders raw booleans for questions.isVeteran with a
ternary that maps true->"Yes" and false->"No", while still falling back to "—"
when the value is null/undefined; locate the display in RequestDetails.tsx where
questions.isVeteran is used and change the rendering logic to first check for
null/undefined and otherwise return "Yes" or "No".
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.canPickUp ? "Yes" : "No"}</div> | ||
| <div className="font-bold text-sm text-black pl-4 min-h-10 bg-[#FAFAFB] flex items-start py-2 border-t border-r border-light-border">Chronic before housing?</div> | ||
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.wasChronic == null ? "—" : questions.wasChronic ? "Yes" : "No"}</div> | ||
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.wasChronic ?? "—"}</div> |
There was a problem hiding this comment.
Boolean values will display as "true"/"false" instead of "Yes"/"No".
Same issue as veteran status above: the raw boolean will be rendered to users instead of "Yes" or "No".
🐛 Proposed fix
- <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.wasChronic ?? "—"}</div>
+ <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">
+ {questions.wasChronic == null ? "—" : (questions.wasChronic ? "Yes" : "No")}
+ </div>📝 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
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.wasChronic ?? "—"}</div> | |
| <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border"> | |
| {questions.wasChronic == null ? "—" : (questions.wasChronic ? "Yes" : "No")} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/client-requests/RequestDetails.tsx` at line 76, The boolean field
questions.wasChronic is being rendered raw which shows "true"/"false"; update
the rendering in RequestDetails (where the JSX contains questions.wasChronic) to
map true->"Yes", false->"No" and keep the fallback "—" for null/undefined (e.g.,
use a conditional/ternary that checks questions.wasChronic === true ? "Yes" :
questions.wasChronic === false ? "No" : "—"). Ensure the same pattern is applied
consistently for other boolean fields in this component that previously rendered
raw booleans.
Comment on lines
+75
to
+76
| src={URL.createObjectURL(photo)} | ||
| alt="Item Photo" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant section of the file with line numbers
FILE="components/form/PhotoDropzone.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
# Show around the referenced line range (adjust if file shorter)
nl -ba "$FILE" | sed -n '1,220p' | sed -n '60,130p'
# Also scan for object URL usage and any revoke calls
rg -n "createObjectURL|revokeObjectURL" "$FILE" || true
rg -n "remove|trash|icon|aria-label|aria-labelledby|title" "$FILE" || trueRepository: Hack4Impact-UMD/journey-home
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/form/PhotoDropzone.tsx"
test -f "$FILE" || { echo "Missing file: $FILE"; exit 1; }
echo "== createObjectURL / revokeObjectURL in file =="
rg -n "createObjectURL|revokeObjectURL" "$FILE" || true
echo
echo "== Remove/trash/button accessibility hints in file =="
rg -n "(remove|trash|icon|aria-label|aria-labelledby|title=|role=)" "$FILE" || true
echo
echo "== Lines 60-130 of $FILE (with line numbers via awk) =="
awk 'NR>=60 && NR<=130 {printf "%5d %s\n", NR, $0}' "$FILE"Repository: Hack4Impact-UMD/journey-home
Length of output: 2799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find CloseIcon component file
fd -a "CloseIcon" . 2>/dev/null | head -n 50
# Also try common paths
fd -a "CloseIcon.tsx" . 2>/dev/null | head -n 50
# Print the CloseIcon.tsx content if found
FILES=$(fd -a "CloseIcon\\.tsx" . || true)
if [ -z "$FILES" ]; then
echo "No CloseIcon.tsx found"
exit 0
fi
echo "== CloseIcon candidates =="
echo "$FILES"
# show the first candidate file with some lines
FIRST=$(echo "$FILES" | head -n1)
echo
echo "== Contents of $FIRST =="
awk 'NR>=1 && NR<=200 {print}' "$FIRST"Repository: Hack4Impact-UMD/journey-home
Length of output: 2527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find CloseIcon component file
fd -a "CloseIcon" . 2>/dev/null | head -n 50
echo
fd -a "CloseIcon.tsx" . 2>/dev/null | head -n 50
FILES=$(fd -a "CloseIcon\\.tsx" . || true)
if [ -z "$FILES" ]; then
echo "No CloseIcon.tsx found"
exit 0
fi
echo
echo "== CloseIcon candidates =="
echo "$FILES"
FIRST=$(echo "$FILES" | head -n1)
echo
echo "== Contents of $FIRST =="
awk 'NR>=1 && NR<=200 {print}' "$FIRST"Repository: Hack4Impact-UMD/journey-home
Length of output: 2529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/form/PhotoDropzone.tsx"
awk 'NR>=1 && NR<=90 {printf "%5d %s\n", NR, $0}' "$FILE"Repository: Hack4Impact-UMD/journey-home
Length of output: 3998
Fix object URL lifecycle and add an accessible name for the photo remove button
components/form/PhotoDropzone.tsx(line 75) callsURL.createObjectURL(photo)during render, but never callsURL.revokeObjectURL, which can leak memory as photos are added/removed.- The icon-only remove
<button>usingCloseIcon(lines 79-88) has no accessible name (aria-label/title), so the action isn’t announced to screen readers.
Suggested fix (object URL revocation)
-import { useCallback } from "react";
+import { useCallback, useEffect, useMemo } from "react";
export default function PhotoDropzone({ photos, onChange, error }: PhotoDropzoneProps) {
+ const previewUrls = useMemo(
+ () => photos.map((photo) => ({ photo, url: URL.createObjectURL(photo) })),
+ [photos]
+ );
+
+ useEffect(() => {
+ return () => {
+ previewUrls.forEach(({ url }) => URL.revokeObjectURL(url));
+ };
+ }, [previewUrls]);
+
const onDrop = useCallback(
@@
- {photos.map((photo, index) => (
+ {previewUrls.map(({ photo, url }, index) => (
<div className="relative" key={index + " " + photo.name}>
<img
- src={URL.createObjectURL(photo)}
+ src={url}
alt="Item Photo"
className="max-h-20 max-w-20 md:max-h-32 md:max-w-32 rounded-sm object-contain"
/>📝 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
| src={URL.createObjectURL(photo)} | |
| alt="Item Photo" | |
| import { useCallback, useEffect, useMemo } from "react"; | |
| export default function PhotoDropzone({ photos, onChange, error }: PhotoDropzoneProps) { | |
| const previewUrls = useMemo( | |
| () => photos.map((photo) => ({ photo, url: URL.createObjectURL(photo) })), | |
| [photos] | |
| ); | |
| useEffect(() => { | |
| return () => { | |
| previewUrls.forEach(({ url }) => URL.revokeObjectURL(url)); | |
| }; | |
| }, [previewUrls]); | |
| const onDrop = useCallback( | |
| // ... rest of onDrop implementation | |
| ); | |
| return ( | |
| // ... JSX structure | |
| {previewUrls.map(({ photo, url }, index) => ( | |
| <div className="relative" key={index + " " + photo.name}> | |
| <img | |
| src={url} | |
| alt="Item Photo" | |
| className="max-h-20 max-w-20 md:max-h-32 md:max-w-32 rounded-sm object-contain" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/form/PhotoDropzone.tsx` around lines 75 - 76, The render currently
calls URL.createObjectURL(photo) directly in PhotoDropzone which leaks object
URLs; change to create and store the object URL in a React state (e.g.,
photoPreviewUrl) and use a useEffect that sets photoPreviewUrl =
URL.createObjectURL(photo) when photo changes and calls
URL.revokeObjectURL(oldUrl) in the cleanup (and on unmount) to avoid leaks. Also
update the icon-only remove <button> that renders CloseIcon to include an
accessible name (add an aria-label and/or title like "Remove photo") so screen
readers can announce the action; ensure the same button handler (e.g.,
onRemovePhoto or handleRemove) is preserved.
Comment on lines
+79
to
+88
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onChange(photos.filter((p) => p !== photo)); | ||
| }} | ||
| className="absolute top-[-.5em] right-[-.5em] bg-white rounded-full text-white text-lg" | ||
| > | ||
| <CloseIcon /> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible name to the remove-photo button.
The icon-only button needs an aria-label so screen-reader users can remove a photo.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/form/PhotoDropzone.tsx` around lines 79 - 88, The remove-photo
button (the icon-only element rendering CloseIcon with the onClick that calls
onChange(photos.filter((p) => p !== photo))) is missing an accessible name; add
an aria-label (e.g., aria-label="Remove photo" or aria-label={`Remove photo:
${photo.name || index}`}) to the button element so screen readers can describe
the action while keeping the existing onClick, onChange, photos, photo and
CloseIcon logic intact.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/client-requests/RequestDetails.tsx (1)
61-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate preferred-language rendering on explicit
false.
!questions.clientSpeaksEnglishalso matchesundefined/null, so the “Preferred language” row shows before that question is answered.Suggested fix
- {!questions.clientSpeaksEnglish && ( + {questions.clientSpeaksEnglish === false && ( <> <div className="font-bold text-sm text-black pl-4 min-h-10 bg-[`#FAFAFB`] flex items-start py-2 border-t border-r border-light-border">Preferred language</div> <div className="pl-4 min-h-10 flex items-start py-2 border-t border-light-border">{questions.preferredLanguage || "—"}</div> </> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/client-requests/RequestDetails.tsx` around lines 61 - 65, The Preferred language row is rendering when questions.clientSpeaksEnglish is undefined/null because the code uses !questions.clientSpeaksEnglish; change the conditional to explicitly check for false (questions.clientSpeaksEnglish === false) so the row only shows when the client explicitly does not speak English, keeping the display of questions.preferredLanguage (or fallback "—") unchanged; update the JSX around the Preferred language block that references questions.clientSpeaksEnglish and questions.preferredLanguage accordingly.
♻️ Duplicate comments (2)
app/donate/steps/Step1PersonalInfo.tsx (1)
61-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApt / Unit is still optional in validation and UI.
This step can proceed with an empty apartment/unit because
validateFormdoesn’t enforceaddress.apt, and the Apt field is not markedrequiredor shown with an inline error.Suggested fix
if (!formState.donorInfo.address?.streetAddress) { newErrors.streetAddress = "Street address is required"; } + if (!formState.donorInfo.address?.apt?.trim()) { + newErrors.apt = "Apt / Unit is required"; + } if (!formState.donorInfo.address?.city) { newErrors.city = "City/Town is required"; } <FormInput label="Apt / Unit" + required value={formState.donorInfo.address?.apt || ""} onChange={(e) => updateDonorInfo({ address: { @@ }) } /> + {errors.apt && <p className="form-error text-red-500 text-sm mt-1">{errors.apt}</p>}Also applies to: 216-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/donate/steps/Step1PersonalInfo.tsx` around lines 61 - 66, The Apt/Unit field is not being validated or shown as required; update the validation in Step1PersonalInfo (the validateForm logic that checks formState.donorInfo.address.*) to enforce formState.donorInfo.address.apt and set newErrors.apt when empty, and update the Apt input JSX in the Step1PersonalInfo component to include required/aria-required and render an inline error message (reuse the same error display pattern used for streetAddress/city); also make the identical change to the other validation block in this file that mirrors the same address checks so both places validate address.apt consistently.components/form/PhotoDropzone.tsx (1)
110-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the remove-photo button.
The icon-only control still has no
aria-label, so assistive tech can’t announce the action.Suggested fix
<button type="button" + aria-label={`Remove photo ${index + 1}`} + title="Remove photo" onClick={(e) => { e.stopPropagation(); onChange(photos.filter((p) => p !== photo)); }} className="absolute top-[-.5em] right-[-.5em] bg-white rounded-full text-white text-lg" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/form/PhotoDropzone.tsx` around lines 110 - 119, The remove-photo button in PhotoDropzone is an icon-only control (uses CloseIcon) and needs an accessible name; update the button element rendered in PhotoDropzone to include an aria-label (e.g., aria-label="Remove photo") or aria-labelledby that references a visible label so assistive tech can announce the action; ensure the onClick handler (which calls onChange(photos.filter((p) => p !== photo))) remains unchanged and that the aria attribute is applied to that same button element.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@components/client-requests/RequestDetails.tsx`:
- Around line 61-65: The Preferred language row is rendering when
questions.clientSpeaksEnglish is undefined/null because the code uses
!questions.clientSpeaksEnglish; change the conditional to explicitly check for
false (questions.clientSpeaksEnglish === false) so the row only shows when the
client explicitly does not speak English, keeping the display of
questions.preferredLanguage (or fallback "—") unchanged; update the JSX around
the Preferred language block that references questions.clientSpeaksEnglish and
questions.preferredLanguage accordingly.
---
Duplicate comments:
In `@app/donate/steps/Step1PersonalInfo.tsx`:
- Around line 61-66: The Apt/Unit field is not being validated or shown as
required; update the validation in Step1PersonalInfo (the validateForm logic
that checks formState.donorInfo.address.*) to enforce
formState.donorInfo.address.apt and set newErrors.apt when empty, and update the
Apt input JSX in the Step1PersonalInfo component to include
required/aria-required and render an inline error message (reuse the same error
display pattern used for streetAddress/city); also make the identical change to
the other validation block in this file that mirrors the same address checks so
both places validate address.apt consistently.
In `@components/form/PhotoDropzone.tsx`:
- Around line 110-119: The remove-photo button in PhotoDropzone is an icon-only
control (uses CloseIcon) and needs an accessible name; update the button element
rendered in PhotoDropzone to include an aria-label (e.g., aria-label="Remove
photo") or aria-labelledby that references a visible label so assistive tech can
announce the action; ensure the onClick handler (which calls
onChange(photos.filter((p) => p !== photo))) remains unchanged and that the aria
attribute is applied to that same button element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7be9ac47-8dfd-4aec-b607-96b6e107f3ec
📒 Files selected for processing (9)
app/client-request-form/steps/Step1ClientInfo.tsxapp/client-requests/admin/new/page.tsxapp/client-requests/admin/reviewed/page.tsxapp/donate/steps/Step1PersonalInfo.tsxapp/donate/steps/Step3Review.tsxcomponents/client-requests/RequestDetails.tsxcomponents/form/FormInput.tsxcomponents/form/PhotoDropzone.tsxlib/services/inventory.ts
✅ Files skipped from review due to trivial changes (1)
- app/client-requests/admin/new/page.tsx
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
1 participant
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.
its all the commit names
Summary by CodeRabbit
New Features
Bug Fixes
Improvements