Profile Popup Modal#87
Conversation
- Updated the bunk field format in exampleCamper. - Enhanced CamperModal to support edit mode with state management for edited camper details. - Added input fields for bunk, swim level, emergency medication, and conflicts in edit mode. - Improved layout and styling for better user experience.
📝 WalkthroughWalkthroughAdds a CamperModal React component for viewing/editing camper fields (bunk, swim level, healthInfo, conflicts) with local edit state and an Changes
Sequence Diagram(s)(Skipped — changes add a UI modal component and a type extension without a multi-component control-flow requiring a sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (2)
src/components/CamperPopupModal.tsx (2)
97-103: Use "CANCEL" instead of "X" for clarity.The "X" button label is confusing since there's already a close icon (crossIcon) in the top-left. Using "CANCEL" would make it clearer that this button discards edits, not closes the modal.
Apply this diff:
<button onClick={handleCancel} className="px-3 text-camp-text-headingBody underline" > - X + CANCEL </button>
115-162: Standardize label styling for consistency.The field labels have inconsistent styling: "BUNK:" is italicized (line 117) while "Swim Level:", "Emergency Medication:", and "Conflicts:" are not. This appears unintentional.
Either add
italicto all labels or remove it from "BUNK:". For consistency, apply this diff:<div> - <span className="font-semibold text-camp-text-headingBody italic">BUNK:</span>{" "} + <span className="font-semibold text-camp-text-headingBody">BUNK:</span>{" "}Or add
italicto the other labels if that's the intended style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/icons/avatarIcon.pngis excluded by!**/*.png
📒 Files selected for processing (1)
src/components/CamperPopupModal.tsx(1 hunks)
| type CamperDetails = { | ||
| name: string; | ||
| bunk: string; | ||
| navStatus: "NAV" | "OCP" | string; | ||
| swimLevel: string; | ||
| emergencyMedication?: string; | ||
| conflicts: string[]; | ||
| } |
There was a problem hiding this comment.
Display the navStatus field as required by PR objectives.
The navStatus field is defined in the type but never displayed in the component. According to the PR objectives, the modal must include "Bunk and (NAV/OCP)" information. Add the navStatus display alongside the bunk information.
Additionally, the type "NAV" | "OCP" | string is redundant since string already covers all possible values. Simplify to just string or remove the trailing | string to enforce only "NAV" or "OCP":
- navStatus: "NAV" | "OCP" | string;
+ navStatus: "NAV" | "OCP";📝 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.
| type CamperDetails = { | |
| name: string; | |
| bunk: string; | |
| navStatus: "NAV" | "OCP" | string; | |
| swimLevel: string; | |
| emergencyMedication?: string; | |
| conflicts: string[]; | |
| } | |
| type CamperDetails = { | |
| name: string; | |
| bunk: string; | |
| navStatus: "NAV" | "OCP"; | |
| swimLevel: string; | |
| emergencyMedication?: string; | |
| conflicts: string[]; | |
| } |
🤖 Prompt for AI Agents
In src/components/CamperPopupModal.tsx around lines 8 to 15, the CamperDetails
type defines navStatus but the component doesn't render it and the type
currently uses the redundant `"NAV" | "OCP" | string`; update the type to either
just string or to `"NAV" | "OCP"` (choose enforcing union if you want to
restrict values), and modify the modal JSX to display bunk and navStatus
together (e.g., "Bunk: {bunk} ({navStatus})") so the UI shows the required "Bunk
and (NAV/OCP)" information.
| // Update edited camper when camper prop changes | ||
| useEffect(() => { | ||
| setEditedCamper(camper); | ||
| }, [camper]); |
There was a problem hiding this comment.
Prevent data loss by checking edit mode before syncing state.
The useEffect will overwrite editedCamper even when the user is actively editing, causing data loss if the parent component re-renders with a new camper reference.
Apply this diff to only sync when not in edit mode:
// Update edited camper when camper prop changes
useEffect(() => {
- setEditedCamper(camper);
- }, [camper]);
+ if (!isEditMode) {
+ setEditedCamper(camper);
+ }
+ }, [camper, isEditMode]);📝 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.
| // Update edited camper when camper prop changes | |
| useEffect(() => { | |
| setEditedCamper(camper); | |
| }, [camper]); | |
| // Update edited camper when camper prop changes | |
| useEffect(() => { | |
| if (!isEditMode) { | |
| setEditedCamper(camper); | |
| } | |
| }, [camper, isEditMode]); |
🤖 Prompt for AI Agents
In src/components/CamperPopupModal.tsx around lines 33-36, the useEffect
currently always syncs setEditedCamper(camper) on camper changes which
overwrites in-progress edits; change the effect to only setEditedCamper when the
component is NOT in edit mode by checking an edit flag (e.g., isEditing or
isEditMode) before calling setEditedCamper, add that flag to the effect
dependency array, and if there is no existing flag add a local state to track
whether the user has started editing (set it in your input change handlers) so
parent re-renders won’t overwrite in-progress edits.
| const handleSave = () => { | ||
| // TODO: Add save logic here (e.g., API call) | ||
| setIsEditMode(false); | ||
| }; |
There was a problem hiding this comment.
Implement save logic to persist changes.
The handleSave function exits edit mode without persisting the changes. All edits are lost since editedCamper is never communicated back to the parent component. You need a callback prop (e.g., onSave) to pass the updated camper data to the parent.
Add an onSave prop to CamperModalProps and call it with the edited data:
type CamperModalProps = {
camper: CamperDetails;
isOpen: boolean;
onClose: () => void;
onEdit?: () => void;
+ onSave?: (updatedCamper: CamperDetails) => void;
};Then update handleSave:
const handleSave = () => {
- // TODO: Add save logic here (e.g., API call)
+ if (onSave) {
+ onSave(editedCamper);
+ }
setIsEditMode(false);
};📝 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.
| const handleSave = () => { | |
| // TODO: Add save logic here (e.g., API call) | |
| setIsEditMode(false); | |
| }; | |
| const handleSave = () => { | |
| if (onSave) { | |
| onSave(editedCamper); | |
| } | |
| setIsEditMode(false); | |
| }; |
🤖 Prompt for AI Agents
In src/components/CamperPopupModal.tsx around lines 47-50, handleSave currently
only toggles edit mode and loses edits because editedCamper is never persisted;
add an onSave callback to CamperModalProps (with appropriate typing, e.g.,
(camper: Camper) => void), call props.onSave(editedCamper) inside handleSave
before setIsEditMode(false), and update any parent usage to pass a handler that
persists the camper (API call/state update) so edits are communicated back to
the parent.
| return ( | ||
| <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 px-4"> | ||
| <div className="relative w-full max-w-md bg-white p-6 text-camp-text-headingBody"> | ||
| <button | ||
| onClick={onClose} | ||
| className="absolute left-4 top-4" | ||
| > | ||
| <Image src={crossIcon} alt="Close" width={20} height={20} /> | ||
| </button> | ||
|
|
||
| <div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2"> | ||
| <div className="relative h-8 w-8"> | ||
| <Image src={avatarIcon} alt="Camper avatar"/> | ||
| </div> | ||
| <h2 className="text-xl font-semibold">{name}</h2> | ||
| </div> | ||
|
|
||
| {!isEditMode ? ( | ||
| <button | ||
| onClick={handleEditClick} | ||
| className="absolute right-4 top-3 px-3 text-camp-primary underline" | ||
| > | ||
| EDIT | ||
| </button> | ||
| ) : ( | ||
| <div className="absolute right-4 top-3 flex"> | ||
| <button | ||
| onClick={handleCancel} | ||
| className="px-3 text-camp-text-headingBody underline" | ||
| > | ||
| X | ||
| </button> | ||
| <span className="text-camp-text-headingBody">|</span> | ||
| <button | ||
| onClick={handleSave} | ||
| className="px-3 text-camp-primary underline font-semibold" | ||
| > | ||
| SAVE | ||
| </button> | ||
| </div> | ||
| )} | ||
|
|
||
|
|
||
| <section className="mt-10 text-sm font-semibold"> | ||
| <div> | ||
| <span className="font-semibold text-camp-text-headingBody italic">BUNK:</span>{" "} | ||
| {isEditMode ? ( | ||
| <input | ||
| type="text" | ||
| value={editedCamper.bunk} | ||
| onChange={(e) => handleFieldChange("bunk", e.target.value)} | ||
| className="border border-gray-300 px-2 py-1 rounded" | ||
| /> | ||
| ) : ( | ||
| <span>{bunk}</span> | ||
| )} | ||
| </div> | ||
| <div> | ||
| <span className="font-semibold text-camp-text-headingBody italic">Swim Level:</span>{" "} | ||
| {isEditMode ? ( | ||
| <input | ||
| type="text" | ||
| value={editedCamper.swimLevel} | ||
| onChange={(e) => handleFieldChange("swimLevel", e.target.value)} | ||
| className="border border-gray-300 px-2 py-1 rounded" | ||
| /> | ||
| ) : ( | ||
| <span>{swimLevel}</span> | ||
| )} | ||
| </div> | ||
|
|
||
| <div> | ||
| <span className="font-semibold text-camp-text-headingBody"> | ||
| Emergency Medication: | ||
| </span>{" "} | ||
| {isEditMode ? ( | ||
| <input | ||
| type="text" | ||
| value={editedCamper.emergencyMedication || ""} | ||
| onChange={(e) => handleFieldChange("emergencyMedication", e.target.value)} | ||
| className="border border-gray-300 px-2 py-1 rounded" | ||
| /> | ||
| ) : ( | ||
| <span>{emergencyMedication?.trim() || "None reported"}</span> | ||
| )} | ||
| </div> | ||
|
|
||
| <div className="flex items-start gap-3"> | ||
| <span className="font-semibold text-camp-text-headingBody mt-1"> | ||
| Conflicts: | ||
| </span> | ||
|
|
||
| {isEditMode ? ( | ||
| <textarea | ||
| value={editedCamper.conflicts.join(", ")} | ||
| onChange={(e) => handleFieldChange("conflicts", e.target.value.split(", ").filter(c => c.trim()))} | ||
| className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded" | ||
| rows={3} | ||
| /> | ||
| ) : ( | ||
| <div className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1"> | ||
| {conflicts.length > 0 ? conflicts.join(", ") : "None"} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </section> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Enhance modal accessibility with keyboard support and ARIA attributes.
The modal lacks several accessibility features that are standard for modal dialogs:
- No ESC key handler to close the modal
- No focus trap (keyboard users can tab to background elements)
- Missing ARIA attributes (
role="dialog",aria-modal="true",aria-labelledby)
Add keyboard support:
+ useEffect(() => {
+ const handleEsc = (e: KeyboardEvent) => {
+ if (e.key === 'Escape' && isOpen) {
+ if (isEditMode) {
+ handleCancel();
+ } else {
+ onClose();
+ }
+ }
+ };
+ document.addEventListener('keydown', handleEsc);
+ return () => document.removeEventListener('keydown', handleEsc);
+ }, [isOpen, isEditMode]);Add ARIA attributes to the modal container:
- <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 px-4">
- <div className="relative w-full max-w-md bg-white p-6 text-camp-text-headingBody">
+ <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 px-4" role="dialog" aria-modal="true">
+ <div className="relative w-full max-w-md bg-white p-6 text-camp-text-headingBody" role="document">For a complete solution, consider using a library like react-focus-lock for proper focus trapping.
📝 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.
| return ( | |
| <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 px-4"> | |
| <div className="relative w-full max-w-md bg-white p-6 text-camp-text-headingBody"> | |
| <button | |
| onClick={onClose} | |
| className="absolute left-4 top-4" | |
| > | |
| <Image src={crossIcon} alt="Close" width={20} height={20} /> | |
| </button> | |
| <div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2"> | |
| <div className="relative h-8 w-8"> | |
| <Image src={avatarIcon} alt="Camper avatar"/> | |
| </div> | |
| <h2 className="text-xl font-semibold">{name}</h2> | |
| </div> | |
| {!isEditMode ? ( | |
| <button | |
| onClick={handleEditClick} | |
| className="absolute right-4 top-3 px-3 text-camp-primary underline" | |
| > | |
| EDIT | |
| </button> | |
| ) : ( | |
| <div className="absolute right-4 top-3 flex"> | |
| <button | |
| onClick={handleCancel} | |
| className="px-3 text-camp-text-headingBody underline" | |
| > | |
| X | |
| </button> | |
| <span className="text-camp-text-headingBody">|</span> | |
| <button | |
| onClick={handleSave} | |
| className="px-3 text-camp-primary underline font-semibold" | |
| > | |
| SAVE | |
| </button> | |
| </div> | |
| )} | |
| <section className="mt-10 text-sm font-semibold"> | |
| <div> | |
| <span className="font-semibold text-camp-text-headingBody italic">BUNK:</span>{" "} | |
| {isEditMode ? ( | |
| <input | |
| type="text" | |
| value={editedCamper.bunk} | |
| onChange={(e) => handleFieldChange("bunk", e.target.value)} | |
| className="border border-gray-300 px-2 py-1 rounded" | |
| /> | |
| ) : ( | |
| <span>{bunk}</span> | |
| )} | |
| </div> | |
| <div> | |
| <span className="font-semibold text-camp-text-headingBody italic">Swim Level:</span>{" "} | |
| {isEditMode ? ( | |
| <input | |
| type="text" | |
| value={editedCamper.swimLevel} | |
| onChange={(e) => handleFieldChange("swimLevel", e.target.value)} | |
| className="border border-gray-300 px-2 py-1 rounded" | |
| /> | |
| ) : ( | |
| <span>{swimLevel}</span> | |
| )} | |
| </div> | |
| <div> | |
| <span className="font-semibold text-camp-text-headingBody"> | |
| Emergency Medication: | |
| </span>{" "} | |
| {isEditMode ? ( | |
| <input | |
| type="text" | |
| value={editedCamper.emergencyMedication || ""} | |
| onChange={(e) => handleFieldChange("emergencyMedication", e.target.value)} | |
| className="border border-gray-300 px-2 py-1 rounded" | |
| /> | |
| ) : ( | |
| <span>{emergencyMedication?.trim() || "None reported"}</span> | |
| )} | |
| </div> | |
| <div className="flex items-start gap-3"> | |
| <span className="font-semibold text-camp-text-headingBody mt-1"> | |
| Conflicts: | |
| </span> | |
| {isEditMode ? ( | |
| <textarea | |
| value={editedCamper.conflicts.join(", ")} | |
| onChange={(e) => handleFieldChange("conflicts", e.target.value.split(", ").filter(c => c.trim()))} | |
| className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded" | |
| rows={3} | |
| /> | |
| ) : ( | |
| <div className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1"> | |
| {conflicts.length > 0 ? conflicts.join(", ") : "None"} | |
| </div> | |
| )} | |
| </div> | |
| </section> | |
| </div> | |
| </div> | |
| ); | |
| return ( | |
| <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 px-4" role="dialog" aria-modal="true"> | |
| <div className="relative w-full max-w-md bg-white p-6 text-camp-text-headingBody" role="document"> | |
| <button | |
| onClick={onClose} | |
| className="absolute left-4 top-4" | |
| > | |
| <Image src={crossIcon} alt="Close" width={20} height={20} /> | |
| </button> | |
| <div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2"> | |
| <div className="relative h-8 w-8"> | |
| <Image src={avatarIcon} alt="Camper avatar"/> | |
| </div> | |
| <h2 className="text-xl font-semibold">{name}</h2> | |
| </div> | |
| {!isEditMode ? ( | |
| <button | |
| onClick={handleEditClick} | |
| className="absolute right-4 top-3 px-3 text-camp-primary underline" | |
| > | |
| EDIT | |
| </button> | |
| ) : ( | |
| <div className="absolute right-4 top-3 flex"> | |
| <button | |
| onClick={handleCancel} | |
| className="px-3 text-camp-text-headingBody underline" | |
| > | |
| X | |
| </button> | |
| <span className="text-camp-text-headingBody">|</span> | |
| <button | |
| onClick={handleSave} | |
| className="px-3 text-camp-primary underline font-semibold" | |
| > | |
| SAVE | |
| </button> | |
| </div> | |
| )} | |
| <section className="mt-10 text-sm font-semibold"> | |
| <div> | |
| <span className="font-semibold text-camp-text-headingBody italic">BUNK:</span>{" "} | |
| {isEditMode ? ( | |
| <input | |
| type="text" | |
| value={editedCamper.bunk} | |
| onChange={(e) => handleFieldChange("bunk", e.target.value)} | |
| className="border border-gray-300 px-2 py-1 rounded" | |
| /> | |
| ) : ( | |
| <span>{bunk}</span> | |
| )} | |
| </div> | |
| <div> | |
| <span className="font-semibold text-camp-text-headingBody italic">Swim Level:</span>{" "} | |
| {isEditMode ? ( | |
| <input | |
| type="text" | |
| value={editedCamper.swimLevel} | |
| onChange={(e) => handleFieldChange("swimLevel", e.target.value)} | |
| className="border border-gray-300 px-2 py-1 rounded" | |
| /> | |
| ) : ( | |
| <span>{swimLevel}</span> | |
| )} | |
| </div> | |
| <div> | |
| <span className="font-semibold text-camp-text-headingBody"> | |
| Emergency Medication: | |
| </span>{" "} | |
| {isEditMode ? ( | |
| <input | |
| type="text" | |
| value={editedCamper.emergencyMedication || ""} | |
| onChange={(e) => handleFieldChange("emergencyMedication", e.target.value)} | |
| className="border border-gray-300 px-2 py-1 rounded" | |
| /> | |
| ) : ( | |
| <span>{emergencyMedication?.trim() || "None reported"}</span> | |
| )} | |
| </div> | |
| <div className="flex items-start gap-3"> | |
| <span className="font-semibold text-camp-text-headingBody mt-1"> | |
| Conflicts: | |
| </span> | |
| {isEditMode ? ( | |
| <textarea | |
| value={editedCamper.conflicts.join(", ")} | |
| onChange={(e) => handleFieldChange("conflicts", e.target.value.split(", ").filter(c => c.trim()))} | |
| className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded" | |
| rows={3} | |
| /> | |
| ) : ( | |
| <div className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1"> | |
| {conflicts.length > 0 ? conflicts.join(", ") : "None"} | |
| </div> | |
| )} | |
| </div> | |
| </section> | |
| </div> | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
In src/components/CamperPopupModal.tsx around lines 72-180, the modal lacks
keyboard accessibility and ARIA attributes; add role="dialog", aria-modal="true"
and aria-labelledby to the modal container and give the title h2 an id (e.g.
camper-modal-title). Implement an ESC key handler inside a useEffect that
listens for keydown and calls onClose, cleaning up the listener on unmount. Add
focus management: save document.activeElement before open, set focus to the
first focusable element (or the close button) when the modal mounts, and restore
focus on unmount; for robust tab trapping, wrap the modal content with a
focus-trap (e.g. install/use react-focus-lock or react-focus-trap) to prevent
tabbing to background elements. Ensure all event listeners and focus restoration
are properly cleaned up in the effect cleanup.
| <div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2"> | ||
| <div className="relative h-8 w-8"> | ||
| <Image src={avatarIcon} alt="Camper avatar"/> | ||
| </div> | ||
| <h2 className="text-xl font-semibold">{name}</h2> |
There was a problem hiding this comment.
Add required width and height props to the Image component.
The Next.js Image component requires explicit dimensions via width and height props (or the fill prop with a positioned parent). Without these, the image may not render correctly.
Apply this diff:
<div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2">
<div className="relative h-8 w-8">
- <Image src={avatarIcon} alt="Camper avatar"/>
+ <Image src={avatarIcon} alt="Camper avatar" width={32} height={32} />
</div>
<h2 className="text-xl font-semibold">{name}</h2>
</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.
| <div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2"> | |
| <div className="relative h-8 w-8"> | |
| <Image src={avatarIcon} alt="Camper avatar"/> | |
| </div> | |
| <h2 className="text-xl font-semibold">{name}</h2> | |
| <div className="absolute left-1/2 top-4 flex -translate-x-1/2 items-center gap-2"> | |
| <div className="relative h-8 w-8"> | |
| <Image src={avatarIcon} alt="Camper avatar" width={32} height={32} /> | |
| </div> | |
| <h2 className="text-xl font-semibold">{name}</h2> |
🤖 Prompt for AI Agents
In src/components/CamperPopupModal.tsx around lines 82 to 86, the Next.js Image
is missing required size props; add explicit width and height (or use fill with
a positioned parent). Fix by giving the Image component width={32} and
height={32} to match the surrounding h-8 w-8 container (or alternatively replace
with fill and ensure the parent has position:relative and explicit size).
| {isEditMode ? ( | ||
| <textarea | ||
| value={editedCamper.conflicts.join(", ")} | ||
| onChange={(e) => handleFieldChange("conflicts", e.target.value.split(", ").filter(c => c.trim()))} | ||
| className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded" | ||
| rows={3} | ||
| /> | ||
| ) : ( | ||
| <div className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1"> | ||
| {conflicts.length > 0 ? conflicts.join(", ") : "None"} | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Improve conflicts parsing to handle inconsistent spacing.
The current logic splits on ", " (comma with space), which breaks if users type commas without spaces or with inconsistent spacing. Additionally, individual conflict names should be trimmed.
Apply this diff to handle various spacing patterns:
{isEditMode ? (
<textarea
value={editedCamper.conflicts.join(", ")}
- onChange={(e) => handleFieldChange("conflicts", e.target.value.split(", ").filter(c => c.trim()))}
+ onChange={(e) => handleFieldChange("conflicts", e.target.value.split(",").map(c => c.trim()).filter(c => c))}
className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded"
rows={3}
/>This splits on any comma, trims each item, and filters out empty strings.
📝 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.
| {isEditMode ? ( | |
| <textarea | |
| value={editedCamper.conflicts.join(", ")} | |
| onChange={(e) => handleFieldChange("conflicts", e.target.value.split(", ").filter(c => c.trim()))} | |
| className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded" | |
| rows={3} | |
| /> | |
| ) : ( | |
| <div className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1"> | |
| {conflicts.length > 0 ? conflicts.join(", ") : "None"} | |
| </div> | |
| )} | |
| </div> | |
| {isEditMode ? ( | |
| <textarea | |
| value={editedCamper.conflicts.join(", ")} | |
| onChange={(e) => handleFieldChange("conflicts", e.target.value.split(",").map(c => c.trim()).filter(c => c))} | |
| className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1 rounded" | |
| rows={3} | |
| /> | |
| ) : ( | |
| <div className="border border-black bg-gray-100 p-3 text-sm text-camp-text-headingBody italic flex-1"> | |
| {conflicts.length > 0 ? conflicts.join(", ") : "None"} | |
| </div> | |
| )} | |
| </div> |
🤖 Prompt for AI Agents
In src/components/CamperPopupModal.tsx around lines 164 to 176, the textarea
splitting currently uses ", " which fails on inconsistent spacing; change the
onChange handler to split on any comma, trim each entry, and filter out empty
strings (e.g. split by "," then map trim then filter(Boolean)), and ensure the
textarea value still displays conflicts joined with ", " for consistent
presentation.
nkanchinadam
force-pushed
the
main
branch
2 times, most recently
from
a884ab9 to
974f469
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/components/CamperModal.tsx:
- Around line 89-94: The Health Info field currently shows sensitive PII in
plain text; restrict access and mask it in read-only mode. Update CamperModal to
check user permissions (e.g., currentUser.roles or hasPermission) before
rendering or enabling edit of healthInfo, keep the existing handleFieldChange
for edits but prevent changes unless permitted, and render a masked value (e.g.,
asterisks) when !isEditMode with a "Show" toggle that only reveals the real
healthInfo after verifying permission; additionally, emit an audit/event log
whenever healthInfo is viewed or revealed (use your existing audit/logging
utility) so access is recorded.
- Around line 49-51: The destructured nonoList from the ternary (const { bunk,
level, healthInfo, nonoList } = isEditMode ? editedCamper : camper) is unused
and the Conflicts field still reads editedCamper.nonoList directly; fix by
making the data source consistent: either remove nonoList from the destructuring
and keep using editedCamper.nonoList, or (preferred) change the Conflicts
rendering to use the destructured nonoList variable so bunk, level and
healthInfo are handled the same way as nonoList when switching between
isEditMode, editedCamper and camper.
🧹 Nitpick comments (3)
src/components/CamperModal.tsx (3)
29-32: TODO: Implement save logic.The save functionality is currently stubbed. You'll need to add the API call to persist changes to the backend.
Do you want me to help generate the save implementation, or would you like me to open a tracking issue for this task?
95-106: Improve conflicts parsing logic.The current implementation has two issues:
- Line 102: Splitting by
", "(comma-space) fails if the user omits the space (e.g., "John,Mary" won't split correctly)- Line 102: The filter condition
c.trim()is truthy checking, but it's clearer to explicitly check for non-empty strings♻️ Proposed improvements
<Textarea label="Conflicts" value={nonoList.join(", ")} disabled={!isEditMode} onChange={(e) => handleFieldChange( "nonoList", - e.target.value.split(", ").filter((c) => c.trim()) + e.target.value.split(",").map(c => c.trim()).filter((c) => c !== '') ) } rows={3} />This change:
- Splits by comma only (works with or without spaces)
- Trims whitespace from each name
- Explicitly filters empty strings
77-88: Consider adding validation for numeric inputs.The Bunk and Swim Level fields currently accept any numeric value without validation. Consider adding constraints:
- Bunk number: likely has a valid range (e.g., min: 1, max: based on session capacity)
- Swim level: likely has a valid range (e.g., min: 1, max: 5)
Mantine's
NumberInputsupportsminandmaxprops for built-in validation.Example validation
<NumberInput label="Bunk" value={bunk} + min={1} + max={20} disabled={!isEditMode} onChange={(value) => handleFieldChange("bunk", value)} /> <NumberInput label="Swim Level" value={level} + min={1} + max={5} disabled={!isEditMode} onChange={(value) => handleFieldChange("level", value)} />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/CamperModal.tsxsrc/types/sessionTypes.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/CamperModal.tsx (2)
src/types/sessionTypes.ts (1)
CamperAttendeeID(27-27)src/utils/personUtils.ts (1)
getFullName(3-5)
🪛 GitHub Actions: Lint & Build on PR
src/components/CamperModal.tsx
[error] 49-49: ESLint error: 'nonoList' is assigned a value but never used. (no-unused-vars)
🔇 Additional comments (4)
src/components/CamperModal.tsx (4)
20-27: LGTM!The state initialization and synchronization logic is well-structured. The
useEffectproperly syncseditedCamperwith incoming prop changes, which is essential for controlled editing behavior.
39-47: LGTM!The generic field change handler is type-safe and provides a clean abstraction for updating multiple field types.
112-117: LGTM!The
openCamperModalhelper provides a clean API for opening the modal and follows Mantine's recommended patterns.
61-71: The color props"success"and"error"are valid for this project. The custom theme defined insrc/styles/theme.tsexplicitly includes these colors as custom theme keys. Mantine ActionIcon accepts any color key defined in the theme override, so no changes are needed.
| <TextInput | ||
| label="Health Info" | ||
| value={healthInfo} | ||
| disabled={!isEditMode} | ||
| onChange={(e) => handleFieldChange("healthInfo", e.target.value)} | ||
| /> |
There was a problem hiding this comment.
Consider access controls and data masking for health information.
The Health Info field displays sensitive PII in plain text without masking. Consider:
- Implementing role-based access controls to restrict who can view/edit health information
- Masking the field value in read-only mode (e.g., showing asterisks or "***" with a "Show" button)
- Adding an audit log for health info access
This complements the compliance concerns raised in sessionTypes.ts regarding the storage of this data.
🤖 Prompt for AI Agents
In @src/components/CamperModal.tsx around lines 89 - 94, The Health Info field
currently shows sensitive PII in plain text; restrict access and mask it in
read-only mode. Update CamperModal to check user permissions (e.g.,
currentUser.roles or hasPermission) before rendering or enabling edit of
healthInfo, keep the existing handleFieldChange for edits but prevent changes
unless permitted, and render a masked value (e.g., asterisks) when !isEditMode
with a "Show" toggle that only reveals the real healthInfo after verifying
permission; additionally, emit an audit/event log whenever healthInfo is viewed
or revealed (use your existing audit/logging utility) so access is recorded.
| level: number; | ||
| bunk: number; | ||
| swimOptOut: boolean; | ||
| healthInfo?: string; |
There was a problem hiding this comment.
Verify privacy and compliance requirements for health data.
The healthInfo field stores sensitive personal health information (PII). Ensure that:
- This data is not logged in application logs, error traces, or analytics
- Proper access controls and encryption are in place
- Compliance requirements (HIPAA, GDPR, CCPA, etc.) for handling health information are satisfied
- Consider adding documentation noting that this field contains PHI/PII
Based on learnings and best practices, health information requires special handling to avoid privacy violations.
2 participants
This is modal that pops up when you click on each individual camper after clicking on the camper name in the activity modal. This is a low fidelity design, so focus more on getting the functionality working and the general look of it. It does NOT need to be 100% faithful to this deisgn since it will change.
It should consist of their: —> accept this data as props as it will likely be fetched higher up in the component tree
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.