Fix duplicate Leaflet map controls on Overview and Deployment pages#508
Conversation
- Map.tsx: Merge duplicate top-left Control blocks (Paintbrush, TrackDB, Layers, Center, Fit bounds, Add markers) into a single control group - index.tsx: Add useIsDesktop so only one layout (desktop or mobile) renders - [...deployment].tsx: Add useIsDesktop for same layout fix on Deployment page Prevents duplicate map controls when both desktop and mobile layouts mount.
There was a problem hiding this comment.
Pull request overview
This pull request fixes duplicate Leaflet map controls that appeared when both desktop and mobile layouts were mounted simultaneously on the Overview and Deployment pages. The fix ensures only one layout renders at a time based on screen size, preventing duplicate UI controls.
Changes:
- Consolidated duplicate Control blocks in Map.tsx into a single top-left control group
- Added useIsDesktop hook to conditionally render either desktop or mobile layout in index.tsx
- Added useIsDesktop hook to conditionally render either desktop or mobile layout in [...deployment].tsx
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react-ui/src/Map/Map.tsx | Merged duplicate top-left Control blocks into single consolidated control group |
| apps/lrauv-dash2/pages/index.tsx | Added useIsDesktop hook and conditional rendering to prevent duplicate map instances |
| apps/lrauv-dash2/pages/vehicle/[...deployment].tsx | Added useIsDesktop hook and conditional rendering to prevent duplicate map instances |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const useIsDesktop = () => { | ||
| const [isDesktop, setIsDesktop] = useState(true) | ||
| useEffect(() => { | ||
| const mq = window.matchMedia('(min-width: 1280px)') | ||
| setIsDesktop(mq.matches) | ||
| const handler = (e: MediaQueryListEvent) => setIsDesktop(e.matches) | ||
| mq.addEventListener('change', handler) | ||
| return () => mq.removeEventListener('change', handler) | ||
| }, []) | ||
| return isDesktop | ||
| } |
There was a problem hiding this comment.
The useIsDesktop hook is duplicated between index.tsx and [...deployment].tsx. This violates the DRY principle and creates a maintenance burden - any fixes or improvements need to be applied in two places. Consider extracting this hook to a shared location like apps/lrauv-dash2/lib/useIsDesktop.ts.
There was a problem hiding this comment.
Good idea long term. For now we’re leaving the hook in place. Dash5 is desktop-only for the moment, so we’ll consider extracting to a shared util when we add mobile support.
| const [isDesktop, setIsDesktop] = useState(true) | ||
| useEffect(() => { | ||
| const mq = window.matchMedia('(min-width: 1280px)') | ||
| setIsDesktop(mq.matches) | ||
| const handler = (e: MediaQueryListEvent) => setIsDesktop(e.matches) | ||
| mq.addEventListener('change', handler) | ||
| return () => mq.removeEventListener('change', handler) | ||
| }, []) |
There was a problem hiding this comment.
The useIsDesktop hook defaults to true in useState, then updates to the actual media query value in useEffect. This can cause a hydration mismatch in Next.js SSR: the server renders thinking it's desktop, but on the client the actual media query might indicate mobile. This can lead to React hydration warnings. Consider adding a typeof window check or using a null initial state to avoid SSR issues, or set the initial state to undefined and handle that case in the component logic.
| const [isDesktop, setIsDesktop] = useState(true) | |
| useEffect(() => { | |
| const mq = window.matchMedia('(min-width: 1280px)') | |
| setIsDesktop(mq.matches) | |
| const handler = (e: MediaQueryListEvent) => setIsDesktop(e.matches) | |
| mq.addEventListener('change', handler) | |
| return () => mq.removeEventListener('change', handler) | |
| }, []) | |
| const [isDesktop, setIsDesktop] = useState(() => { | |
| if (typeof window === 'undefined') { | |
| // On the server, default to desktop to match existing behavior. | |
| return true | |
| } | |
| return window.matchMedia('(min-width: 1280px)').matches | |
| }) | |
| useEffect(() => { | |
| if (typeof window === 'undefined') { | |
| return | |
| } | |
| const mq = window.matchMedia('(min-width: 1280px)') | |
| setIsDesktop(mq.matches) | |
| const handler = (e: MediaQueryListEvent) => setIsDesktop(e.matches) | |
| mq.addEventListener('change', handler) | |
| return () => mq.removeEventListener('change', handler) | |
| }, []) |
There was a problem hiding this comment.
Thanks for the review. We’re keeping the current approach: useState(true) to avoid hydration mismatches, and we’ll hold off on extracting the hook for now since dash5 is desktop-only. We may revisit when we add mobile support.
| type MobileView = 'main' | 'sidebar' | ||
|
|
||
| const useIsDesktop = () => { | ||
| const [isDesktop, setIsDesktop] = useState(true) |
There was a problem hiding this comment.
The useIsDesktop hook defaults to true in useState, then updates to the actual media query value in useEffect. This can cause a hydration mismatch in Next.js SSR: the server renders thinking it's desktop, but on the client the actual media query might indicate mobile. This can lead to React hydration warnings. Consider adding a typeof window check or using a null initial state to avoid SSR issues, or set the initial state to undefined and handle that case in the component logic.
There was a problem hiding this comment.
Thanks for the review. We’re keeping the current approach: useState(true) to avoid hydration mismatches, and we’ll hold off on extracting the hook for now since dash5 is desktop-only. We may revisit when we add mobile support.
|
Hi Zach, Ready for review. Copilot suggested hydration and hook-extraction changes; I’ve commented on why we’re keeping the current approach. |
ZackLyon
left a comment
ZackLyon
left a comment
There was a problem hiding this comment.
I think this is fine for now. We can always revisit it if proper mobile views are added.
Prevents duplicate map controls when both desktop and mobile layouts mount.