fix(position): convert fixed altitude between feet and meters#1267
fix(position): convert fixed altitude between feet and meters#1267dkxmercury wants to merge 4 commits into
Conversation
…stic#1051) The fixed-altitude field is labelled Feet when the display units are set to Imperial, but the value was stored and loaded as raw meters with no conversion, so entering 1025 stored 1025 m instead of ~312 m. Add feetToMeters/metersToFeet helpers in core/utils/geo.ts and convert at the form load, submit, and geolocation-fill edges when units are Imperial. Altitude is rounded to integer meters to match the int32 protobuf field. Fixes meshtastic#1051
|
@dkxmercury is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAltitude conversion helpers were added and integrated into Position settings so imperial and metric altitude values are converted consistently for display, browser geolocation, fixed-position submission, and protobuf storage. Regression and component tests cover both unit systems. ChangesAltitude unit handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Position
participant UseBrowserLocationButton
participant BrowserGeolocation
participant GeoHelpers
participant ProtobufPayload
Position->>UseBrowserLocationButton: Pass isImperial
UseBrowserLocationButton->>BrowserGeolocation: Request location
BrowserGeolocation-->>UseBrowserLocationButton: Return altitude in meters
UseBrowserLocationButton->>GeoHelpers: Convert meters to feet when imperial
GeoHelpers-->>Position: Set displayed altitude
Position->>GeoHelpers: Convert entered feet to meters when imperial
GeoHelpers-->>ProtobufPayload: Provide rounded integer meters
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/PageComponents/Settings/Position.tsx (1)
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd integration tests for the Position conversion paths.
The current tests do not exercise Imperial form loading, browser geolocation, or the queued
setFixedPositionprotobuf message. Add coverage proving that1025ft is displayed/submitted as312meters and that metric behavior remains unchanged.Also applies to: 122-141, 164-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 `@apps/web/src/components/PageComponents/Settings/Position.tsx` around lines 61 - 68, Add integration tests for the Position component covering Imperial form loading and browser geolocation conversion, asserting that 1025 feet is displayed/submitted as 312 meters while metric values remain unchanged. Also exercise the queued setFixedPosition protobuf message path, including the converted altitude, using the existing Position test setup and geolocation mocks.
🤖 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.
Nitpick comments:
In `@apps/web/src/components/PageComponents/Settings/Position.tsx`:
- Around line 61-68: Add integration tests for the Position component covering
Imperial form loading and browser geolocation conversion, asserting that 1025
feet is displayed/submitted as 312 meters while metric values remain unchanged.
Also exercise the queued setFixedPosition protobuf message path, including the
converted altitude, using the existing Position test setup and geolocation
mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a186f92f-d5d2-4dc7-9c87-499ca2894aa6
📒 Files selected for processing (3)
apps/web/src/components/PageComponents/Settings/Position.tsxapps/web/src/core/utils/geo.test.tsapps/web/src/core/utils/geo.ts
meshtastic#1051) Render the real Position form and assert the conversion at each edge: Imperial load shows meters as feet, submitting feet queues integer meters into the setFixedPosition message, metric values pass through unchanged, and the browser-geolocation fill converts meters to feet.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/PageComponents/Settings/Position.test.tsx (1)
151-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for cleared/
NaNaltitude.The production path explicitly maps non-finite altitude input to
0meters, but this suite only submits finite values. Add a cleared-field case (andNaNif validation permits it) asserting the queued altitude is0.🤖 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 `@apps/web/src/components/PageComponents/Settings/Position.test.tsx` around lines 151 - 162, Add regression coverage in the Position test suite for cleared altitude input, using the existing setup, altitudeInput, and lastQueuedPosition helpers; submit an empty value and assert the queued position altitude is 0. If the validation path permits NaN input, add a corresponding case that also verifies altitude 0, without altering the existing finite-value tests.
🤖 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 `@apps/web/src/components/PageComponents/Settings/Position.test.tsx`:
- Around line 21-74: Move the shared mutable state and mock functions currently
declared as top-level let/const bindings into a single vi.hoisted state object.
Update the factories for useDevice, useConfigEditor, useMyNodeAsProto, and
useToast, along with mockGetEffectiveConfig and mockEditor, to reference the
corresponding mocks.* properties while preserving their existing behavior.
---
Nitpick comments:
In `@apps/web/src/components/PageComponents/Settings/Position.test.tsx`:
- Around line 151-162: Add regression coverage in the Position test suite for
cleared altitude input, using the existing setup, altitudeInput, and
lastQueuedPosition helpers; submit an empty value and assert the queued position
altitude is 0. If the validation path permits NaN input, add a corresponding
case that also verifies altitude 0, without altering the existing finite-value
tests.
🪄 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 Plus
Run ID: 84ef096f-ac08-4f99-baa5-c67343163ba7
📒 Files selected for processing (1)
apps/web/src/components/PageComponents/Settings/Position.test.tsx
| // --- Mutable per-test state (read lazily by the mock factories below) -------- | ||
| // Names are prefixed with `mock` so vitest's vi.mock hoisting whitelist allows | ||
| // the factories to reference them. | ||
| let mockDisplayUnits: Protobuf.Config.Config_DisplayConfig_DisplayUnits; | ||
| let mockPositionConfig: Protobuf.Config.Config_PositionConfig; | ||
| let mockMyNode: Protobuf.Mesh.NodeInfo | undefined; | ||
|
|
||
| const mockSetRadioSection = vi.fn(); | ||
| const mockQueueAdminMessage = vi.fn(); | ||
| const mockToast = vi.fn(); | ||
| const mockGetCurrentPosition = vi.fn(); | ||
|
|
||
| // A minimal signal-shaped object; the mocked useSignal just returns `.value`. | ||
| const mockEditor = { | ||
| radio: { value: {}, peek: () => ({}), subscribe: () => () => {} }, | ||
| setRadioSection: mockSetRadioSection, | ||
| queueAdminMessage: mockQueueAdminMessage, | ||
| }; | ||
|
|
||
| const mockGetEffectiveConfig = (configCase: string) => { | ||
| if (configCase === "display") { | ||
| return { units: mockDisplayUnits }; | ||
| } | ||
| if (configCase === "position") { | ||
| return mockPositionConfig; | ||
| } | ||
| return undefined; | ||
| }; | ||
|
|
||
| // useWaitForConfig throws a suspense promise until config is present; the render | ||
| // path here supplies config directly, so it is a no-op in tests. | ||
| vi.mock("@app/core/hooks/useWaitForConfig", () => ({ | ||
| useWaitForConfig: () => {}, | ||
| })); | ||
|
|
||
| vi.mock("@core/stores", () => ({ | ||
| useDevice: () => ({ | ||
| config: { position: mockPositionConfig }, | ||
| getEffectiveConfig: mockGetEffectiveConfig, | ||
| }), | ||
| })); | ||
|
|
||
| vi.mock("@meshtastic/sdk-react", () => ({ | ||
| useConfigEditor: () => mockEditor, | ||
| useSignal: (signal: { value?: unknown }) => signal?.value ?? {}, | ||
| })); | ||
|
|
||
| vi.mock("@core/hooks/useNodesAsProto.ts", () => ({ | ||
| useMyNodeAsProto: () => mockMyNode, | ||
| })); | ||
|
|
||
| vi.mock("@core/hooks/useToast.ts", () => ({ | ||
| useToast: () => ({ toast: mockToast }), | ||
| })); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '"vitest"|vi\.mock|vi\.hoisted|mockDisplayUnits|mockEditor' \
--glob 'package.json' --glob '*lock*' \
apps/web/src/components/PageComponents/Settings/Position.test.tsxRepository: meshtastic/web
Length of output: 763
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== vitest version ==\n'
rg -n '"vitest"' apps/web/package.json package.json pnpm-lock.yaml yarn.lock package-lock.json --max-count 5 || true
printf '\n== test file context ==\n'
sed -n '1,130p' apps/web/src/components/PageComponents/Settings/Position.test.tsxRepository: meshtastic/web
Length of output: 5096
🌐 Web query:
Vitest vi.mock hoisted mock prefix documentation vi.hoisted factory variables
💡 Result:
In Vitest, the vi.mock function is hoisted to the top of the file, meaning it is executed before all other imports and code [1][2]. Because of this behavior, you cannot directly reference variables declared in the outer scope within the vi.mock factory function [1][3]. To work around this limitation and pass variables into a mocked module, you should use the vi.hoisted function [1][4]. vi.hoisted: This function allows you to define variables that are hoisted along with your vi.mock calls, making them available within the factory function [1][4]. - It must be defined before the vi.mock call [1]. - Both vi.hoisted and vi.mock will be hoisted in the order they are defined in the file [4]. Example usage: const { someVariable } = vi.hoisted( => { return { someVariable: 'initial-value' } }) vi.mock('./module-path', => { return { getValue: => someVariable } }) Unlike Jest, Vitest does not automatically allow variables to be used inside vi.mock based on specific naming prefixes (like starting with 'mock'). You must explicitly use vi.hoisted to share state between your test logic and the factory. If you do not require hoisting and need access to variables in the local scope, you can use vi.doMock, which is not hoisted, though it will only affect subsequent imports [1][3].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 2: https://vitest.dev/guide/mocking
- 3: Unable to use local variable as argument for
vi.fn().mockReturnValue()vitest-dev/vitest#1381 - 4: https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/integrations/vi.ts
Move the shared mock state into vi.hoisted.
vi.mock is hoisted, so these factories shouldn’t close over normal top-level let/const bindings. Use one vi.hoisted(() => ({ ... })) state object and reference mocks.* inside each factory.
🤖 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 `@apps/web/src/components/PageComponents/Settings/Position.test.tsx` around
lines 21 - 74, Move the shared mutable state and mock functions currently
declared as top-level let/const bindings into a single vi.hoisted state object.
Update the factories for useDevice, useConfigEditor, useMyNodeAsProto, and
useToast, along with mockGetEffectiveConfig and mockEditor, to reference the
corresponding mocks.* properties while preserving their existing behavior.
Source: MCP tools
|
Thanks for the review. On the cleared / Happy to add a test asserting that a cleared field does not queue a message, if you would like that behavior documented. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@dkxmercury This looks really good. Look into the code change that CodeRabbit is suggesting. |
|
Also @dkxmercury can you look into setting up these values to work with i18n? Read CONTRIBUTING_I18N_DEVELOPER_GUIDE.md for guidelines. |
|
Also, did you investigate the remainder of the codebase for any other usages of meters or feet that should be updated? |
…ng (meshtastic#1051) Add a `unit.foot` entry to the English common.json (mirroring `unit.meter`) and use `t("unit.foot.plural")` / `t("unit.meter.plural")` for the altitude field's unit label and suffix. Other locales are handled by Crowdin, which uploads only the en sources.
…it (meshtastic#1051) A cleared number field becomes NaN, which fails the altitude zod validation, so the form never submits. Document that no setFixedPosition message is queued.
|
Thanks for the review! All three points addressed: i18n (@Hunter275): the altitude field now uses translation keys instead of hardcoded strings. I added a Cleared / Other feet/meters usages (@danditomaso): good call, I swept the codebase. This PR fixes the write side (the fixed-position config form). There's no shared display-units helper, and I found three read-side displays that render altitude in raw meters and ignore the Imperial/Metric setting (same bug class):
Two of the three are presentational components with no config access, so doing this right is best done as a small shared display-units helper applied consistently, which felt like scope creep on this form fix. I'd suggest a follow-up issue for the read-side altitude displays; happy to open it and take it on if you agree. All tests pass and lint is clean. |
4 participants
Description
The fixed-position altitude input is labelled "Feet" when the display units are set to Imperial, but the entered value is stored (and the node's value is loaded) as raw meters with no conversion. So a user who wants 1025 ft has to type 312, and the field shows the wrong unit for the stored value. The altitude protobuf field is an integer number of meters (
mesh.proto:optional int32 altitude = 3).This converts altitude between feet and meters at the edges of the Position form when the display units are Imperial, so the number the user types matches the label.
Related Issues
Fixes #1051
Changes Made
feetToMeters/metersToFeethelpers (andMETERS_PER_FOOT) inapps/web/src/core/utils/geo.ts.Position.tsxwhen units are Imperial: form load, submit (into thesetFixedPositionadmin message), and the "use browser location" fill.int32protobuf field, and guard against a cleared/NaN field.apps/web/src/core/utils/geo.test.tscovering the conversion and the exact [Bug]: Fixed altitude entry (no GPS) prompts for feet but treats value as meters. #1051 scenario (1025 ft to 312 m to ~1024 ft).Note: the report also mentioned decimal values being rejected. That is not reproducible on current
main(the input parses withNumber.parseFloatand validation isz.coerce.number()with no.int()), so this PR only addresses the units mismatch. Metric users are unaffected, since the default units are Metric.Testing Done
geo.test.ts(Vitest), 5 cases. Verified it fails on unmodified code (feetToMeters is not a function) and passes with the fix.oxlintclean on the changed files.vitest run src/core/utils/geo.test.tspasses (5/5).Checklist
{{unit}}interpolation is reused)Summary by CodeRabbit