web: fix topology device markers placed in the water - #655
Conversation
Device markers on the topology map and globe were positioned as a metro centroid plus a 0.3 degree (~25 mi) radial fanout, ignoring real device locations. For coastal metros like NYC that arc placed markers offshore in the Atlantic and the harbor (#652). - Extract the duplicated calculateDevicePosition from the map and globe into one shared, unit-tested helper (computeDevicePositions). - Default to a much tighter metro-centroid fanout (radius 0.3 -> 0.04 degrees) so co-located devices stay separated without landing in the water. - Add a "Precise locations" toggle (map and globe) that anchors each device at its real facility coordinates, with a small jitter so devices sharing a facility remain individually clickable; devices without facility coordinates fall back to the fanout. - Expose per-device facility latitude/longitude from the topology API (LEFT JOIN dz_facilities_current via location_pk).
ben-dz
left a comment
There was a problem hiding this comment.
One robustness gap to fix before merge; otherwise a clean, well-tested fix.
High — api/handlers/topology.go:250: the new LEFT JOIN dz_facilities_current f ON d.location_pk = f.pk lacks the isUnknownIdentifierError fallback that every sibling handler already has for this exact column (devices.go:204-217, facilities.go:202/:290, metros.go:246). d.location_pk isn't guaranteed present in all environments (e.g. remote proxy tables predating the migration). The query runs unguarded inside an errgroup goroutine, so on such an environment FetchTopologyData fails wholesale and /api/topology returns 500 — taking down map, globe, and the non-geo graph — while the devices/facilities pages on the same deployment keep working via their fallback. Fix: on isUnknownIdentifierError, retry with the join removed and 0 as latitude, 0 as longitude (frontend already treats 0/0 as no-coords and falls back to the metro fanout).
Low: device query has no ORDER BY, so fanout/facility-jitter ring slots (assigned by array index) can swap between refetches; sort each group by pk for stable placement.
Security review found nothing: static SQL (no interpolation, no user input into FetchTopologyData), endpoint gating unchanged, facility lat/lng already public via /api/dz/facilities, URL overlay param parsed against a fixed key whitelist.
| CROSS JOIN total_stake ts | ||
| LEFT JOIN device_stats ds ON d.pk = ds.device_pk | ||
| LEFT JOIN dz_contributors_current c ON d.contributor_pk = c.pk | ||
| LEFT JOIN dz_facilities_current f ON d.location_pk = f.pk |
There was a problem hiding this comment.
The new LEFT JOIN dz_facilities_current f ON d.location_pk = f.pk references d.location_pk, which is not guaranteed present in every environment the API serves (e.g. remote proxy tables created before the location_pk migration). Every sibling handler that touches this column guards against its absence: devices.go:204-217 builds a fallback query stripping the identical join and retries on isUnknownIdentifierError (ClickHouse code 47); facilities.go:202/:290 and metros.go:246 do the same.
This query runs unguarded inside an errgroup goroutine, so on an environment lacking location_pk the error fails FetchTopologyData wholesale and /api/topology returns 500 — taking down map, globe, and the non-geo graph — while the devices/facilities pages on the same deployment keep working via their fallback.
Fix: mirror the sibling pattern — on isUnknownIdentifierError, retry with the join removed and 0 as latitude, 0 as longitude (the frontend already treats 0/0 as "no facility coords" and falls back to the metro fanout, so it degrades cleanly).
| for (const [metroPk, group] of byMetro) { | ||
| const metro = metroMap.get(metroPk) | ||
| if (!metro) continue | ||
| group.forEach((d, i) => { |
There was a problem hiding this comment.
The device query has no ORDER BY, so ClickHouse row order isn't stable across refetches, and both fanoutByMetro (here) and the facility-jitter loop (line 102) assign ring slots by array index. Markers within a fanout ring or co-located facility cluster can swap positions between page loads. Pre-existing for fanout; this PR extends it to facility jitter. Sort each group by pk before forEach for stable placement.
Resolves: #652
Summary of Changes
0.3°(~25 mi) radial fanout, ignoring real device locations; for coastal metros like NYC that arc placed markers offshore in the Atlantic and the harbor.0.3°→0.04°, ~3 mi) so co-located devices stay visually separated without landing in the water.calculateDevicePositionfrom the map and globe into one shared, unit-tested helper (computeDevicePositions).latitude/longitudefrom the topology API (LEFT JOINdz_facilities_currentvialocation_pk).The graph view is non-geo (force-directed), so it is unaffected.
Diff Breakdown
api.tstypedevicePositions.test.ts(new)Most of the change is the shared positioning helper plus wiring; the rest is the toggle and the API field.
Key files (click to expand)
web/src/components/topology/devicePositions.ts— new pure helper: fanout (default, tight radius) + facility-anchored (with co-located jitter) + fallbackweb/src/components/topology-map.tsx/topology-globe.tsx— use the shared helper, keyed off the toggle; removed the duplicated local functionsweb/src/components/topology/TopologyContext.tsx— newpreciseLocationsoverlay toggle (URL-synced like other overlays)web/src/components/topology/TopologyControlBar.tsx— "Precise locations" control on map and globeapi/handlers/topology.go— facility lat/lng on the topology DeviceTesting Verification
[0,0].