feat: add device location height - #1068
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds terrain elevation lookup and optional above-ground height handling across device forms, routes, persistence, API schemas, GeoJSON output, configuration, localization, and tests. ChangesDevice elevation support
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to This change adds device-height handling across creation, editing, and API flows, but unresolved issues can silently alter or erase stored heights, persist invalid coordinates, make device creation fail or create duplicates after retries, and delay writes when elevation requests accumulate. The PR is not ready to merge until these correctness and availability risks are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Operator
participant LocationForm
participant DeviceRoute
participant ElevationService
participant DeviceModel
participant Database
Operator->>LocationForm: enter coordinates and heightAboveGround
LocationForm->>ElevationService: request terrain elevation
ElevationService-->>LocationForm: return terrain elevation and metadata
LocationForm->>DeviceRoute: submit validated location
DeviceRoute->>ElevationService: calculate height above sea level
DeviceRoute->>DeviceModel: persist calculated height
DeviceModel->>Database: save device height
DeviceModel-->>DeviceRoute: return device data
DeviceRoute-->>Operator: display 2D or 3D GeoJSON location
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/db/models/device.server.ts (1)
429-469: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
updateDeviceclears a stored height when the caller omits it.Line 469 writes
height ?? nullwheneverargs.locationis present.UpdateDeviceArgs.locationdeclaresheight?: number, so a partial update such as{ location: { lat, lng } }silently overwrites an existing height withnull.This diverges from the convention used at Lines 406-417, where the function only writes a column when the argument is not
undefined. Latitude and longitude are required insidelocation, but height is not.Write
heightonly when the caller supplies it.🛠️ Proposed fix to preserve an existing height
setColumns['latitude'] = lat setColumns['longitude'] = lng - setColumns['height'] = height ?? null + if (height !== undefined) { + setColumns['height'] = height + }If clearing the height must remain possible, widen the type to
height?: number | nulland keep theundefinedguard.
🧹 Nitpick comments (2)
app/db/models/device.server.ts (1)
679-682: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the coordinate-building logic into one helper.
The same conditional now appears at Lines 679-682, at Lines 802-809, and in
app/routes/api.boxes.tsat Lines 167-172. Three copies of the 2D/3D rule will drift.Add one exported helper and reuse it in all three places.
♻️ Proposed helper
// app/lib/location.ts export function toGeoJsonPosition( longitude: number, latitude: number, height: number | null | undefined, ): [number, number] | [number, number, number] { return height == null ? [longitude, latitude] : [longitude, latitude, height] }for (const device of devices) { - const coordinates = - device.height === null - ? [device.longitude, device.latitude] - : [device.longitude, device.latitude, device.height] + const coordinates = toGeoJsonPosition( + device.longitude, + device.latitude, + device.height, + ) const feature = point(coordinates, device)Also applies to: 802-809
tests/routes/api.boxes.spec.ts (1)
355-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the coordinate arity against the feature height.
expect([2, 3]).toContain(...)accepts both shapes unconditionally. A regression that drops the height from the coordinates still passes. Tie the expected length tofeature.properties.height.♻️ Proposed assertion
- expect([2, 3]).toContain(feature.geometry.coordinates.length) + expect(feature.geometry.coordinates.length).toBe( + feature.properties.height === null ? 2 : 3, + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 468a7a42-9e5c-45c9-9aa8-08641e7eee6a
📒 Files selected for processing (26)
app/components/device/new/location-info.tsxapp/components/device/new/new-device-stepper.tsxapp/components/device/new/summary-info.tsxapp/db/drizzle/0048_giant_carnage.sqlapp/db/drizzle/meta/0048_snapshot.jsonapp/db/drizzle/meta/_journal.jsonapp/db/models/device.server.tsapp/db/models/sensor.server.tsapp/db/schema/device.tsapp/lib/api-schemas/devices.tsapp/lib/device-transform.tsapp/lib/location.tsapp/lib/openapi/schemas/device.tsapp/lib/openapi/schemas/location.tsapp/routes/api.boxes.tsapp/routes/device.$deviceId.edit.location.tsxapp/routes/device.new.tsxapp/services/device-service.server.tspublic/locales/de/edit-device-general.jsonpublic/locales/de/newdevice.jsonpublic/locales/en/edit-device-general.jsonpublic/locales/en/newdevice.jsontests/lib/location.spec.tstests/lib/transform-to-api-format.spec.tstests/routes/api.boxes.$deviceId.spec.tstests/routes/api.boxes.spec.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/db/models/device.server.ts (1)
363-363: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAllow
nullin the update type.
updateDevicestoresheight: nullwhen null is explicitly provided at Lines 470-472. TheUpdateDeviceArgs.location.heighttype accepts onlynumber, so typed callers cannot clear an existing height.- location?: { lat: number; lng: number; height?: number } + location?: { lat: number; lng: number; height?: number | null }
🧹 Nitpick comments (1)
app/db/models/device.server.ts (1)
470-472: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression tests for height preservation and clearing.
When
heightis omitted, the existing value must remain unchanged. Whenheightis explicitlynull, the stored value must be cleared. Add tests for both update cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5aacfd67-1a7d-4da8-8594-7166b09b65d6
📒 Files selected for processing (3)
app/db/models/device.server.tsapp/lib/location.tstests/routes/api.boxes.spec.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
…in elevation - Add OpenTopoData API service for fetching terrain elevation - Uses multi-dataset query (eudem25m,srtm30m) for automatic fallback - Update location-info.tsx to display terrain elevation and final height - Users now input height above ground, terrain elevation is auto-fetched - Final height above sea level is calculated and stored - Update translations for new height labels and info text - Update device.new.tsx action to fetch elevation and calculate final height Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/lib/api-schemas/devices.ts (1)
18-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument the breaking change to
location.heightsemantics for API clients.
CreateDeviceSchemais the public create-device request shape. Before this change,heightwas stored as supplied. Now the server resolves terrain elevation and adds it, per the description onDeviceHeightAboveGroundSchemainapp/lib/openapi/schemas/location.tslines 69-73.An existing client that sends an absolute sea-level height keeps working, but the stored height shifts upward by the terrain elevation at that coordinate. The change is silent for that client.
Add a migration note to the OpenAPI description and the changelog. State the old meaning, the new meaning, and the effective date.
app/routes/device.new.tsx (1)
129-140: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDevice creation and integration creation are not atomic, and the new 500 response invites duplicate devices.
Line 129 creates the device. Line 131 creates the integrations. If line 131 throws, the device already exists, but the action now returns
device_creation_failedwith status 500 at lines 136-139. Previously it redirected to the profile.The client sees a total failure and the user retries. Each retry creates another device.
Either run both writes in one transaction, or return a success response that reports the integration failure separately so the user does not retry the device creation.
app/routes/api.boxes.$deviceId.ts (1)
450-477: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftA read-modify-write round trip inflates the stored height.
The PUT handler now treats
body.location.heightas height above ground and storesterrainElevation + height. The GET handler returns the stored height, which is above sea level. A client that reads a device, changes one unrelated field, and sends the payload back unchanged submits the sea-level height as an above-ground height. The stored height then grows by the terrain elevation on every such round trip. This is the normal read-modify-write pattern for a REST resource, so the corruption is easy to trigger.Consider one of these options:
- Accept an explicit field, for example
heightAboveGround, and keepheightas the absolute value.- Return the submitted above-ground height in the response so a round trip is idempotent.
- Reject a
location.heightthat is already resolved, using a request flag.Also confirm how the edit UI reads the stored height. If
app/routes/device.$deviceId.edit.location.tsxseeds its above-ground input from the persisted sea-leveldevice.height, the same inflation occurs on each save.#!/bin/bash # Description: Trace how stored device height is read back into above-ground inputs. fd -t f 'device.$deviceId.edit.location.tsx' app/routes --exec rg -n 'height|heightAboveGround|elevation' {} fd -t f 'location.ts' app/lib/openapi/schemas --exec cat -n {} rg -n 'heightAboveGround|height' app/lib/device-transform.ts app/services/device-service.server.tsapp/routes/api.boxes.ts (1)
186-189: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a nullish check for
device.height.The condition tests only
null. Ifdevice.heightisundefined, for example when a query selects a subset of columns, the third branch produces[longitude, latitude, undefined], which serializes to[lng, lat, null]and is not valid GeoJSON. Test for bothnullandundefined.🐛 Proposed fix
coordinates: - device.height === null + device.height == null ? [device.longitude, device.latitude] : [device.longitude, device.latitude, device.height],
🧹 Nitpick comments (9)
app/lib/openapi/schemas/location.ts (1)
100-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared long-form normalization transform.
Lines 100-108 repeat the transform already defined at lines 59-67 for
LocationObjectSchema. Both maplongitude/latitude/heighttolng/lat/height.Move the transform body into a single local helper and use it in both places.
♻️ Proposed refactor
+function normalizeLongForm(location: { + longitude: number + latitude: number + height?: number +}) { + return { lng: location.longitude, lat: location.latitude, height: location.height } +}Then use
normalizeLongForm(location)in both transforms..env.example (1)
16-19: 🩺 Stability & Availability | 🔵 TrivialPlan for the public OpenTopoData quota.
The example points at the shared public instance
api.opentopodata.org. That instance enforces a low request rate and a daily call cap. Device creation and location edits now block on this lookup, andapp/routes/device.new.tsxreturns HTTP 503 when the lookup fails.For production, host a private OpenTopoData instance or add a cache for repeated coordinates. Add a metric for elevation lookup failures so quota exhaustion is visible.
app/components/device/new/summary-info.tsx (1)
20-24: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the elevation result from the location step.
app/components/device/new/location-info.tsxline 15 already callsuseTerrainElevationfor the same coordinates. This call omitsinitialResult, so the summary step issues a second request for coordinates that were already resolved.app/routes/device.new.tsxthen performs a third lookup on the server. The public OpenTopoData endpoint is rate limited.Lift the resolved
TerrainElevationResultinto the stepper form state and pass it asinitialResult. The hook already short-circuits whenresultMatchesLocationmatches.app/routes/device.new.tsx (1)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
terrainElevationwith its type.
let terrainElevationwithout an annotation gets an evolving implicitany. The accessterrainElevation.elevationat line 88 is therefore unchecked.app/routes/device.$deviceId.edit.location.tsxline 162 declareslet terrainElevation: TerrainElevationResult.♻️ Proposed change
- let terrainElevation + let terrainElevation: TerrainElevationResultAdd the type import:
-import { calculateHeightAboveSeaLevel } from '~/lib/elevation' +import { + calculateHeightAboveSeaLevel, + type TerrainElevationResult, +} from '~/lib/elevation'app/lib/location.ts (1)
101-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared field-error mapping.
parseDeviceLocationInputFormDataandvalidateDeviceLocationInputFieldErrorsbuild the sameDeviceLocationInputFieldErrorsobject from a flattened error. Extract one helper and reuse it in both functions.♻️ Proposed refactor
+function toDeviceLocationInputFieldErrors( + error: z.ZodError, +): DeviceLocationInputFieldErrors { + const flattened = z.flattenError(error) + + return { + latitude: flattened.fieldErrors.latitude?.[0], + longitude: flattened.fieldErrors.longitude?.[0], + heightAboveGround: flattened.fieldErrors.heightAboveGround?.[0], + } +} + export function parseDeviceLocationInputFormData(formData: FormData): | { success: true data: DeviceLocationInput } | { success: false errors: DeviceLocationInputFieldErrors } { const parsed = deviceLocationInputSchema.safeParse({ latitude: formData.get('latitude'), longitude: formData.get('longitude'), heightAboveGround: formData.get('heightAboveGround'), }) if (parsed.success) return { success: true, data: parsed.data } - const flattened = z.flattenError(parsed.error) - - return { - success: false, - errors: { - latitude: flattened.fieldErrors.latitude?.[0], - longitude: flattened.fieldErrors.longitude?.[0], - heightAboveGround: flattened.fieldErrors.heightAboveGround?.[0], - }, - } + return { + success: false, + errors: toDeviceLocationInputFieldErrors(parsed.error), + } } export function validateDeviceLocationInputFieldErrors( value: unknown, ): DeviceLocationInputFieldErrors { const parsed = deviceLocationInputSchema.safeParse(value) if (parsed.success) return {} - const flattened = z.flattenError(parsed.error) - - return { - latitude: flattened.fieldErrors.latitude?.[0], - longitude: flattened.fieldErrors.longitude?.[0], - heightAboveGround: flattened.fieldErrors.heightAboveGround?.[0], - } + return toDeviceLocationInputFieldErrors(parsed.error) }app/components/device/new/location-info.tsx (2)
74-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one numeric field handler.
handleLatitudeChange,handleLongitudeChange, andhandleHeightChangerepeat the same trim,Number, andNumber.isFinitelogic. Extract a single helper that parses the input and returnsnumber | undefined, then use it in all three handlers.♻️ Proposed refactor
+ const parseNumericInput = (rawValue: string) => { + const value = rawValue.trim() + if (value === '') return undefined + const parsedValue = Number(value) + + return Number.isFinite(parsedValue) ? parsedValue : undefined + } + const handleLatitudeChange = (event: React.ChangeEvent<HTMLInputElement>) => { - const value = event.target.value.trim() - const parsedValue = Number(value) - const latitude = - value === '' || !Number.isFinite(parsedValue) ? '' : parsedValue + const parsed = parseNumericInput(event.target.value) + const latitude = parsed ?? '' setMarker((current) => ({ ...current, latitude })) - setValue('latitude', latitude === '' ? undefined : latitude, { + setValue('latitude', parsed, { shouldDirty: true, shouldValidate: true, }) }
243-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnounce elevation status changes and reference only existing element ids.
Two points for the elevation status block:
aria-describedby="height-info height-error"always referencesheight-error, but that element renders only whenerrors.heightAboveGround.messageexists. Build the value conditionally.- The loading, error, and result text replaces itself asynchronously without a live region. Screen readers do not announce the change. Add
aria-live="polite"to a wrapper around the status output.app/components/device/new/new-device-stepper.tsx (1)
147-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the error toast against repeated firing.
The effect depends on
t.react-i18nextreturns a newtidentity after a language change, so the same failedactionDatashows the toast again. Track the handledactionDatain a ref, or depend only onactionData.♻️ Proposed refactor
+ const handledActionDataRef = useRef<unknown>(null) + useEffect(() => { if (!actionData || actionData.ok) return + if (handledActionDataRef.current === actionData) return + handledActionDataRef.current = actionData toast({ title: t('device_creation_error'), description: t(actionData.error), variant: 'destructive', }) }, [actionData, t, toast])app/routes/resources.elevation.ts (1)
42-50: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMap lookup error codes to more precise statuses.
rate_limitedandtimeoutboth return 503. Return 429 forrate_limited, withRetry-After, and 504 fortimeout. Clients can then apply correct backoff instead of retrying every failure the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e546959-39c1-4a61-8565-df9577de21de
📒 Files selected for processing (22)
.env.exampleREADME.mdapp/components/device/new/location-info.tsxapp/components/device/new/new-device-stepper.tsxapp/components/device/new/summary-info.tsxapp/hooks/use-terrain-elevation.tsapp/lib/api-schemas/devices.tsapp/lib/elevation.tsapp/lib/env.server.tsapp/lib/location.tsapp/lib/openapi/schemas/location.tsapp/routes/api.boxes.$deviceId.tsapp/routes/api.boxes.tsapp/routes/device.$deviceId.edit.location.tsxapp/routes/device.new.tsxapp/routes/resources.elevation.tsapp/services/elevation-service.server.tspublic/locales/de/edit-device-general.jsonpublic/locales/de/newdevice.jsonpublic/locales/en/edit-device-general.jsonpublic/locales/en/newdevice.jsontests/lib/location.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- public/locales/de/edit-device-general.json
- public/locales/en/edit-device-general.json
- public/locales/de/newdevice.json
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/device.new.tsx (1)
74-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake terrain elevation lookup conditional on
heightAboveGround.When
heightAboveGroundis omitted, this action still persists the terrain elevation becausecalculateHeightAboveSeaLeveltreats the value as0. This creates a non-nullheightand makes UI device creation depend on the elevation service. InitializefinalHeighttonulland calculate it only whenheightAboveGround !== undefined, matchingapp/routes/api.boxes.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62f881cd-c158-41b3-8427-d8d3806a5b64
📒 Files selected for processing (12)
app/components/device/new/new-device-stepper.tsxapp/components/device/new/sensors-info.tsxapp/components/device/new/summary-info.tsxapp/db/models/device.server.tsapp/hooks/use-terrain-elevation.tsapp/lib/device-general.tsapp/lib/new-device-form.tsapp/routes/device.$deviceId.edit.location.tsxapp/routes/device.new.tsxapp/services/elevation-service.server.tspublic/locales/de/newdevice.jsonpublic/locales/en/newdevice.json
💤 Files with no reviewable changes (2)
- public/locales/en/newdevice.json
- public/locales/de/newdevice.json
🚧 Files skipped from review as they are similar to previous changes (1)
- app/routes/device.$deviceId.edit.location.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/routes/api.boxes.$deviceId.spec.ts (1)
19-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert elevation-service call arguments.
Both mocks ignore latitude and longitude. A reversed argument order would still return
250 + heightand pass the current assertions. Use hoistedvi.fnmocks and assert(latitude, longitude, height)for the existing three-dimensional and zero-height POST and PUT cases in both files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 033ea010-9d4a-4a95-8533-bfd3e61986be
📒 Files selected for processing (2)
tests/routes/api.boxes.$deviceId.spec.tstests/routes/api.boxes.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
|
When creating a new device, I select a location and then enter a height (e.g. 10 m) and the computed height is updated accordingly. Note: I played with some reeeeelly large numbers (e.g. 
When I then move the location to another place without adjusting the height, the computed height is not updated (and stays 25 m in my example): 
Bonus finding: When setting the location into the middle of the ocean, the heights are not only not updated properly but 
|
|
I added a height of 5 meter to an existing box without height and when I open the settings of that device, I see 
|
|
Wouldn't it also be interesting to display the height above sea level here as well? 
|
|
With your current implementation, some tests seem to fail. |
There was a problem hiding this comment.
Is it a good idea to abort the whole process when we are not able to derive a height above zero based on the input height?
| const sensorsProvided = validatedData.sensors?.length > 0 | ||
| // Extract longitude and latitude from location array [longitude, latitude] | ||
| const [longitude, latitude] = validatedData.location | ||
| // Request height is relative to ground; persisted height is above sea level. |
There was a problem hiding this comment.
I think you once mentioned that we persist the raw height (above ground) and compute the height above sea level were needed. Here, it looks like we store the absolute height above sea level. Intended?
| export default function EditLocation() { | ||
| const { device } = useLoaderData<typeof loader>() | ||
| const { t } = useTranslation('edit-device-general') | ||
| const initialHeightAboveGround = device.height === null ? null : undefined |
There was a problem hiding this comment.
Here we simply load the height of the device (not above the sea level). I'm slightly confused what we actually store for each device.
|
|
||
| const serverErrors: LocationFieldErrors = | ||
| const derivedHeight = Number( | ||
| (device.height - originalElevation.result.elevation).toFixed(3), |
There was a problem hiding this comment.
Ahhhh! We store the height above sea level and, where needed, compute the height above ground again on the fly?
|
Looks good to me, except one detail: If the elevation api can not be called, I can't update the location of an existing device and I am also not able to create a new device! 
In this case, we should simply not allow adding a height. |
|
Another consideration: Since we are sending locations to an external service, we might need to include that detail in the privacy policy. |
2 participants
Type of Change
Implementation
Checklist
devbranchAdditional Information