Add Localized Map Card to Device Details Page#3751
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a localized device map card that lazily renders a read-only ChangesLocalized Device Map Card
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (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 |
|
New azure vertex changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/vertex/components/features/devices/device-location-card.tsx (1)
71-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFalsy-zero bug: latitude/longitude of
0incorrectly renders "N/A".
lat ? lat.toFixed(6) : "N/A"and the longitude equivalent treat0as falsy, even though0is a legitimate coordinate (equator/prime meridian). This is inconsistent with the newhasCoordinatescheck just above (line 27), which correctly uses!== null. Worth aligning while this data path is being touched for the new map feature.🐛 Proposed fix
<div className="text-base font-mono"> - {lat ? lat.toFixed(6) : "N/A"} + {lat !== null ? lat.toFixed(6) : "N/A"} </div> ... <div className="text-base font-mono"> - {lon ? lon.toFixed(6) : "N/A"} + {lon !== null ? lon.toFixed(6) : "N/A"} </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 `@src/vertex/components/features/devices/device-location-card.tsx` around lines 71 - 82, The latitude/longitude rendering in device-location-card.tsx is using truthy checks, so a valid coordinate of 0 shows as “N/A”. Update the display logic in the location card to match the existing hasCoordinates/null-check approach by checking for null/undefined rather than falsiness, and keep the logic localized around the latitude and longitude fields so 0 renders correctly while missing values still show “N/A”.
🧹 Nitpick comments (2)
src/vertex/components/features/mini-map/mini-map.tsx (1)
174-191: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMarker gating looks correct. Draggable flag and handler attachment are properly gated by
readOnly, matching the stated contract (no drag/click-to-move in read-only mode).One minor note:
readOnlyis read inside themap.on('load', ...)closure but isn't part of the effect's dependency array ([mapMode], line 259). If a caller ever togglesreadOnlyafter mount without changingmapMode, the marker's draggable state and handlers would remain stale. This mirrors the existing pattern for other static props (center,zoom,height), so it's likely intentional givenMiniMapis only ever mounted with a fixedreadOnlyvalue today (e.g.,DeviceLocationCardalways passes it astrue) — flagging for awareness rather than as a regression.🤖 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 `@src/vertex/components/features/mini-map/mini-map.tsx` around lines 174 - 191, The `MiniMap` effect in `mini-map.tsx` reads `readOnly` inside the `map.on('load', ...)` setup, but the effect only depends on `mapMode`, so toggling `readOnly` later would leave the marker and event handlers stale. Update the effect dependency list or otherwise re-initialize the map/marker setup when `readOnly` changes, and make sure the `markerRef.current` draggable state plus the `dragend` and `click` handlers in the `MiniMap` setup always reflect the current `readOnly` value.src/vertex/components/features/devices/device-location-card.tsx (1)
19-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating geographic bounds, not just finiteness.
PR objectives state the card should fall back to the empty state when coordinates are "unavailable or invalid."
toNumberOrNull/hasCoordinatesonly checkNumber.isFinite, so a stray value likelat = 999would still be treated as valid and passed intoMiniMap.♻️ Proposed range check
const toNumberOrNull = (v: unknown) => { if (v === null || v === undefined || v === '') return null; const n = Number(String(v)); return Number.isFinite(n) ? n : null; }; const lat = toNumberOrNull(device.latitude); const lon = toNumberOrNull(device.longitude); - const hasCoordinates = lat !== null && lon !== null; + const hasCoordinates = + lat !== null && lon !== null && + Math.abs(lat) <= 90 && Math.abs(lon) <= 180;🤖 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 `@src/vertex/components/features/devices/device-location-card.tsx` around lines 19 - 27, The coordinate validation in device-location-card.tsx only checks finiteness, so invalid geographic values can still reach MiniMap; update toNumberOrNull and the hasCoordinates check to reject out-of-range latitude/longitude values as well. Use the existing lat/lon handling in DeviceLocationCard to enforce valid bounds before rendering the map, and fall back to the empty state whenever either coordinate is missing or outside accepted geographic limits.
🤖 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 `@src/vertex/components/features/devices/device-location-card.tsx`:
- Around line 71-82: The latitude/longitude rendering in
device-location-card.tsx is using truthy checks, so a valid coordinate of 0
shows as “N/A”. Update the display logic in the location card to match the
existing hasCoordinates/null-check approach by checking for null/undefined
rather than falsiness, and keep the logic localized around the latitude and
longitude fields so 0 renders correctly while missing values still show “N/A”.
---
Nitpick comments:
In `@src/vertex/components/features/devices/device-location-card.tsx`:
- Around line 19-27: The coordinate validation in device-location-card.tsx only
checks finiteness, so invalid geographic values can still reach MiniMap; update
toNumberOrNull and the hasCoordinates check to reject out-of-range
latitude/longitude values as well. Use the existing lat/lon handling in
DeviceLocationCard to enforce valid bounds before rendering the map, and fall
back to the empty state whenever either coordinate is missing or outside
accepted geographic limits.
In `@src/vertex/components/features/mini-map/mini-map.tsx`:
- Around line 174-191: The `MiniMap` effect in `mini-map.tsx` reads `readOnly`
inside the `map.on('load', ...)` setup, but the effect only depends on
`mapMode`, so toggling `readOnly` later would leave the marker and event
handlers stale. Update the effect dependency list or otherwise re-initialize the
map/marker setup when `readOnly` changes, and make sure the `markerRef.current`
draggable state plus the `dragend` and `click` handlers in the `MiniMap` setup
always reflect the current `readOnly` value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9aa4e11b-220a-470d-baf7-f1d4fdd0dc2c
📒 Files selected for processing (3)
src/vertex/app/changelog.mdsrc/vertex/components/features/devices/device-location-card.tsxsrc/vertex/components/features/mini-map/mini-map.tsx
|
New azure vertex changes available for preview here |
There was a problem hiding this comment.
Pull request overview
Adds a localized, embedded map view to the Device Details page by reusing the existing MiniMap component in a read-only mode, and documents the change in the Vertex changelog.
Changes:
- Added a
readOnlyprop toMiniMapto disable marker dragging and click-to-move interactions. - Embedded a lazy-loaded
MiniMap(with a loading skeleton) into the Device Location/Site Details card, with an explicit empty state when coordinates are missing. - Added a changelog entry describing the new Device Details localized map card and the
MiniMapAPI update.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vertex/components/features/mini-map/mini-map.tsx |
Adds readOnly support to prevent coordinate mutation in view-only contexts. |
src/vertex/components/features/devices/device-location-card.tsx |
Renders a lazy-loaded, read-only MiniMap when coordinates exist; otherwise shows an empty state placeholder. |
src/vertex/app/changelog.md |
Documents the new Device Details map card and the MiniMap readOnly prop addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,8 +1,12 @@ | |||
| import { lazy, Suspense } from "react"; | |||
| <div className="px-3 pb-3"> | ||
| {hasCoordinates ? ( | ||
| <Suspense fallback={<div className="h-48 rounded-md bg-muted animate-pulse" />}> | ||
| <MiniMap | ||
| latitude={String(lat)} | ||
| longitude={String(lon)} | ||
| readOnly | ||
| scrollZoom={false} | ||
| height="h-48" | ||
| zoom={13} | ||
| /> | ||
| </Suspense> | ||
| ) : ( | ||
| <div className="h-48 rounded-md border border-dashed flex flex-col items-center justify-center gap-2 text-muted-foreground bg-muted/30"> | ||
| <MapPin className="h-6 w-6 opacity-40" /> | ||
| <p className="text-sm text-center px-4"> | ||
| Location data is currently unavailable for this device | ||
| </p> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
@Baalmart Writing tests are not part of this PR.
There was a problem hiding this comment.
Hi @BwanikaRobert , thanks for the clarification.
We might want to start writing tests in each of our PRs going forward since it is extremely hard to write tests for the entire application in one go
|
New azure vertex changes available for preview here |
|
New azure vertex changes available for preview here |
| </div> | ||
| )} | ||
| </div> | ||
|
|
There was a problem hiding this comment.
Please add this new feature as a component under components/features/devices/ @BwanikaRobert
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/vertex/components/features/devices/device-location-map.tsx`:
- Around line 27-29: Update the empty-state copy in the device location map
component so it matches the required wording exactly. In the
device-location-map.tsx component, adjust the message rendered by the location
unavailable state to include the full context “for this device,” keeping the
change scoped to the existing empty-state paragraph.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c258352-5774-4476-8468-b19f582556ca
📒 Files selected for processing (3)
src/vertex/app/changelog.mdsrc/vertex/components/features/devices/device-location-card.tsxsrc/vertex/components/features/devices/device-location-map.tsx
✅ Files skipped from review due to trivial changes (1)
- src/vertex/app/changelog.md
|
New azure vertex changes available for preview here |
|
New azure vertex changes available for preview here |
Matches the linked issue's specified copy for the device location map empty state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
New azure vertex changes available for preview here |
Summary of Changes (What does this PR do?)
Adds an embedded map to the Device Location card on the Device Details page.
MiniMapat zoom 13 centred on thedevice's stored coordinates
React.lazy+Suspenseto prevent SSR issueswith Mapbox GL JS; shows an animated skeleton while loading
(dashed border, MapPin icon, and a short message) instead of a blank space
readOnlyprop toMiniMap— disables pin dragging andclick-to-move so the view-only context cannot accidentally mutate coordinates
Status of maturity (all need to be checked before merging):
How should this be manually tested?
renders at the correct location
empty state renders (MapPin icon + "Location data is currently
unavailable for this device")
skeleton (pulse animation) shows before the map tiles load
pin dragging (the
readOnlyprop defaults tofalse)What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit