feat: implement interest zone management with customizable labels and…#189
feat: implement interest zone management with customizable labels and…#189bancho22 wants to merge 3 commits intovbuch:mainfrom
Conversation
|
Someone is attempting to deploy a commit to the Valery Buchinsky's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This pull request implements multi-zone management functionality allowing users to create and manage multiple interest zones with customizable labels and colors. The feature introduces a new UI with segmented controls to switch between viewing zones and events, along with a modal-driven zone creation flow that aligns with issue #188.
Changes:
- Added zone type system with predefined labels and colors (Home, Office, Parents, School, Gym, Other) stored in
ZONE_TYPESconstant - Extended
Interestdata model with optionallabelandcolorfields throughout the stack (web, ingest, API, database) - Refactored map circle rendering from React components to imperative native Google Maps API for better performance and cleanup
- Implemented new UI components:
AddZoneModal,SegmentedControl,ZoneList, andZoneBadgesfor zone management - Modified onboarding flow to show zone creation prompt only once per user session using localStorage
- Removed floating "Add Interest" button in favor of integrated zone management in sidebar
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| web/lib/zoneTypes.ts | Defines predefined zone types with labels and hex colors |
| web/lib/types.ts | Extends Interest interface with optional label and color fields |
| web/lib/hooks/useOnboardingFlow.ts | Adds hasSeenZoneCreationPrompt flag to prevent repeated prompts |
| web/lib/hooks/useOnboardingFlow.test.ts | Tests for new zone creation prompt dismissal behavior |
| web/lib/hooks/useInterests.ts | Updates addInterest to accept label/color metadata |
| web/lib/hooks/useInterestManagement.ts | Passes pending zone metadata through target mode |
| web/lib/hooks/useInterestManagement.test.ts | Updates tests for new metadata parameters |
| web/components/onboarding/LoadingButton.tsx | Removes outdated comment reference |
| web/components/onboarding/AddZoneModal.tsx | New modal for selecting zone type, color, and radius |
| web/components/onboarding/AddZoneModal.test.tsx | Tests for zone modal interactions |
| web/components/onboarding/AddInterestButton.tsx | Removed - replaced by integrated zone management |
| web/components/ZoneList.tsx | Displays zones as a list with colored badges on desktop |
| web/components/ZoneBadges.tsx | Displays zones as inline badges on mobile |
| web/components/SegmentedControl.tsx | Toggle component for switching between zones/events views |
| web/components/SegmentedControl.test.tsx | Tests for segmented control |
| web/components/MessagesGrid.tsx | Adds headerContent prop to support custom headers |
| web/components/MapContainer.tsx | Removes AddInterestButton, passes pendingColor to target mode |
| web/components/MapComponent.tsx | Passes map instance and pendingColor to child components |
| web/components/InterestTargetMode.tsx | Refactored to use native Google Maps Circle with color support |
| web/components/InterestCircles.tsx | Complete refactor to native Google Maps API for proper cleanup |
| web/components/HomeContent.tsx | Integrates view mode switching, zone list/badges, and add zone modal |
| web/app/api/interests/route.ts | Handles label/color in POST/PATCH, includes in responses |
| web/mocks/handlers.ts | Updates mock API to handle label/color fields |
| web/mocks/fixtures/interests.ts | Adds sample data with labels and colors |
| ingest/notifications/interest-fetcher.ts | Fetches label/color fields from database |
| ingest/lib/types.ts | Adds label/color fields to Interest type |
| docs/features/multi-zone-support.md | Documentation for new multi-zone feature |
| web/package.json | Adds @testing-library/user-event dependency |
| pnpm-lock.yaml | Lockfile updates for new dependency |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > | ||
| <span | ||
| className="w-8 h-8 rounded-full flex-shrink-0" | ||
| style={{ backgroundColor: color }} |
There was a problem hiding this comment.
Using user-provided color values directly in inline styles without validation creates a potential XSS vulnerability. The color value should be validated against a hex color pattern (e.g., /^#[0-9a-f]{6}$/i) before being used in styles to prevent injection attacks.
| export default function AddZoneModal({ onConfirm, onCancel }: AddZoneModalProps) { | ||
| const [selectedTypeId, setSelectedTypeId] = useState<string>(ZONE_TYPES[0].id); | ||
| const [radius, setRadius] = useState(DEFAULT_RADIUS); | ||
|
|
||
| const handleConfirm = () => { | ||
| const zoneType = ZONE_TYPES.find((t) => t.id === selectedTypeId) ?? ZONE_TYPES[0]; | ||
| onConfirm({ label: zoneType.label, color: zoneType.color, radius }); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| {/* Backdrop */} | ||
| <div | ||
| className={`fixed inset-0 bg-black/40 ${zIndex.modalBackdrop}`} | ||
| onClick={onCancel} | ||
| aria-hidden="true" | ||
| /> | ||
|
|
||
| {/* Dialog */} | ||
| <div | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="add-zone-title" | ||
| className={`fixed inset-0 flex items-center justify-center ${zIndex.modalContent} p-4`} | ||
| > | ||
| <div className="bg-white rounded-lg shadow-xl max-w-sm w-full p-6 flex flex-col gap-5"> | ||
| <h2 | ||
| id="add-zone-title" | ||
| className="text-base font-semibold text-neutral-dark" | ||
| > | ||
| Добави зона | ||
| </h2> | ||
|
|
||
| {/* Zone type grid */} | ||
| <div className="grid grid-cols-3 gap-2"> | ||
| {ZONE_TYPES.map((type) => { | ||
| const isSelected = type.id === selectedTypeId; | ||
| return ( | ||
| <button | ||
| key={type.id} | ||
| type="button" | ||
| onClick={() => setSelectedTypeId(type.id)} | ||
| className={`flex flex-col items-center gap-1.5 p-3 rounded-lg border-2 transition-all duration-150 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-primary/50 ${ | ||
| isSelected | ||
| ? "border-[var(--color-primary)] bg-neutral-light" | ||
| : "border-neutral-border bg-white hover:bg-neutral-light" | ||
| }`} | ||
| > | ||
| <span | ||
| className="w-8 h-8 rounded-full flex-shrink-0" | ||
| style={{ backgroundColor: type.color }} | ||
| aria-hidden="true" | ||
| /> | ||
| <span className="text-xs font-medium text-neutral-dark leading-tight text-center"> | ||
| {type.label} | ||
| </span> | ||
| </button> | ||
| ); | ||
| })} | ||
| </div> | ||
|
|
||
| {/* Radius slider */} | ||
| <div className="flex flex-col gap-2"> | ||
| <div className="flex justify-between items-center"> | ||
| <label | ||
| htmlFor="zone-radius" | ||
| className="text-sm font-medium text-neutral-dark" | ||
| > | ||
| Радиус | ||
| </label> | ||
| <span className="text-sm text-neutral">{radius} м</span> | ||
| </div> | ||
| <input | ||
| id="zone-radius" | ||
| type="range" | ||
| min={MIN_RADIUS} | ||
| max={MAX_RADIUS} | ||
| step={50} | ||
| value={radius} | ||
| onChange={(e) => setRadius(Number(e.target.value))} | ||
| className="w-full accent-primary" | ||
| /> | ||
| <div className="flex justify-between text-xs text-neutral"> | ||
| <span>{MIN_RADIUS} м</span> | ||
| <span>{MAX_RADIUS} м</span> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Actions */} | ||
| <div className="flex gap-3 justify-end"> | ||
| <button | ||
| type="button" | ||
| onClick={onCancel} | ||
| className={`${buttonSizes.md} ${buttonStyles.secondary} ${borderRadius.sm}`} | ||
| > | ||
| Отказ | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={handleConfirm} | ||
| className={`${buttonSizes.md} ${buttonStyles.primary} ${borderRadius.sm}`} | ||
| > | ||
| Избери на картата | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
The modal should support keyboard navigation for better accessibility. Consider adding an Escape key handler to close the modal (onCancel) and potentially Enter key support to confirm the selection when focus is on the modal content.
| if (typeof body.label === "string") { | ||
| updates.label = body.label; | ||
| } | ||
|
|
||
| // Update color if provided | ||
| if (typeof body.color === "string") { |
There was a problem hiding this comment.
The PATCH endpoint allows empty strings for label and color, which could create interests with empty string metadata fields. Consider adding validation to either reject empty strings (length > 0) or explicitly handle them by deleting the field from the database (setting to undefined).
| if (typeof body.label === "string") { | |
| updates.label = body.label; | |
| } | |
| // Update color if provided | |
| if (typeof body.color === "string") { | |
| if (typeof body.label === "string") { | |
| if (body.label.trim().length === 0) { | |
| return NextResponse.json( | |
| { error: "Label cannot be empty" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| updates.label = body.label; | |
| } | |
| // Update color if provided | |
| if (typeof body.color === "string") { | |
| if (body.color.length === 0) { | |
| return NextResponse.json( | |
| { error: "Color cannot be empty" }, | |
| { status: 400 }, | |
| ); | |
| } |
| <div className="bg-white rounded-lg shadow-xl max-w-sm w-full p-6 flex flex-col gap-5"> | ||
| <h2 | ||
| id="add-zone-title" | ||
| className="text-base font-semibold text-neutral-dark" | ||
| > | ||
| Добави зона | ||
| </h2> |
There was a problem hiding this comment.
The modal dialog container should stop click propagation to prevent accidentally triggering the backdrop click handler when clicking inside the modal. Add onClick={(e) => e.stopPropagation()} to the inner dialog div (line 48) to prevent clicks from bubbling to the backdrop.
| <div className="bg-white rounded-lg shadow-xl max-w-sm w-full p-6 flex flex-col gap-5"> | |
| <h2 | |
| id="add-zone-title" | |
| className="text-base font-semibold text-neutral-dark" | |
| > | |
| Добави зона | |
| </h2> | |
| <div | |
| className="bg-white rounded-lg shadow-xl max-w-sm w-full p-6 flex flex-col gap-5" | |
| onClick={(e) => e.stopPropagation()} | |
| > | |
| <h2 | |
| id="add-zone-title" | |
| className="text-base font-semibold text-neutral-dark" | |
| > | |
| Добави зона |
| style={{ | ||
| borderColor: color, | ||
| backgroundColor: `${color}15`, | ||
| color, | ||
| }} |
There was a problem hiding this comment.
Using user-provided color values directly in inline styles (lines 56-60) without validation creates a potential XSS vulnerability. While the color comes from the database, it should still be validated as a hex color format before being used in styles. Consider validating the color against a regex pattern (e.g., /^#[0-9a-f]{6}$/i) or using a CSS variable with proper escaping.
| if (typeof color === "string" && color.length > 0) { | ||
| interestData.color = color; | ||
| } |
There was a problem hiding this comment.
The color field should be validated to ensure it's a valid hex color code (e.g., /^#[0-9a-f]{6}$/i). Currently, any non-empty string is accepted, which could lead to invalid color values being stored in the database and potentially causing rendering issues or XSS vulnerabilities if the color is used in CSS without proper sanitization.
| for (const [id, circle] of nativeCircles.entries()) { | ||
| if (!desiredIds.has(id)) { | ||
| circle.setMap(null); | ||
| nativeCircles.delete(id); |
There was a problem hiding this comment.
Event listeners added to Google Maps Circle objects are not explicitly cleaned up when circles are removed or updated. While setting the circle's map to null should clean up most resources, explicitly calling google.maps.event.clearInstanceListeners(circle) before removing circles would ensure no memory leaks from event listeners.
| // Cleanup: remove the circle from the map when the component unmounts | ||
| return () => { | ||
| if (circleRef.current) { | ||
| circleRef.current.setMap(null); | ||
| circleRef.current = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Event listeners added to the circle in InterestTargetMode are not explicitly cleaned up. When the circle is removed (line 50 or line 91), consider calling google.maps.event.clearInstanceListeners(circleRef.current) before setting it to null to prevent potential memory leaks.
| if (typeof label === "string" && label.length > 0) { | ||
| interestData.label = label; | ||
| } |
There was a problem hiding this comment.
The label field should have a maximum length validation to prevent excessively long strings from being stored. Consider adding a reasonable limit (e.g., 50 characters) to maintain database integrity and prevent potential DoS attacks through large payloads.
| <button | ||
| key={type.id} | ||
| type="button" | ||
| onClick={() => setSelectedTypeId(type.id)} | ||
| className={`flex flex-col items-center gap-1.5 p-3 rounded-lg border-2 transition-all duration-150 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-primary/50 ${ | ||
| isSelected | ||
| ? "border-[var(--color-primary)] bg-neutral-light" | ||
| : "border-neutral-border bg-white hover:bg-neutral-light" | ||
| }`} | ||
| > | ||
| <span | ||
| className="w-8 h-8 rounded-full flex-shrink-0" | ||
| style={{ backgroundColor: type.color }} | ||
| aria-hidden="true" | ||
| /> | ||
| <span className="text-xs font-medium text-neutral-dark leading-tight text-center"> | ||
| {type.label} | ||
| </span> | ||
| </button> |
There was a problem hiding this comment.
The zone type selection buttons should have explicit aria-label attributes to improve screen reader accessibility. The colored circle is marked aria-hidden, but the button should clearly announce its purpose, e.g., aria-label="Изберете тип зона: Дома" to provide better context for screen reader users.
| | `userId` | string | Owner | | ||
| | `coordinates` | {lat,lng}| Center of the zone | | ||
| | `radius` | number | Radius in meters (100–1000) | | ||
| | `label` | string? | Zone type label (e.g. "Дома") | |
There was a problem hiding this comment.
"Дома" is really weird Bulgarian. It should be "Вкъщи" all around.
| { id: "parents", label: "Родители", color: "#10B981" }, | ||
| { id: "school", label: "Училище", color: "#F59E0B" }, | ||
| { id: "gym", label: "Фитнес", color: "#F97316" }, | ||
| { id: "other", label: "Друго", color: "#6B7280" }, |
There was a problem hiding this comment.
Do we really need to enumerate? Can we just let the user label it themselves and give them examples or a placeholder?
Also this is now PII as discussed with @laboletory earlier. If they say "this is where I live" this is quite sensitive data...
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
A lot of feedback from me now.
Would it make sense to keep the segmented control in the not logged in state but make the "Събития" the default segment? Just an idea. This would also "advertise" the "my zones" feature without.
|
#188