fix(settings): allow negative coordinates in position config - #1381
fix(settings): allow negative coordinates in position config#1381dzienisz wants to merge 2 commits into
Conversation
|
Someone is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
|
Kamil Dzieniszewski seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughNumeric input handling now preserves intermediate and invalid values. Position fields support longer coordinate values. Position validation limits optional latitude and longitude values to seven decimal places while retaining range checks. ChangesNumeric position input handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
…sion Preserve intermediate input values (-, empty string, trailing decimal point) in number fields instead of converting them to "NaN". Raise the latitude/longitude fieldLength limits so negative values with 7 decimal places fit (-34.1147648 / -180.0000000). Add Zod refine validation that enforces a maximum of 7 decimal places on latitude and longitude, replacing the incorrect string-length-based check. Fixes meshtastic#1308 Generated with [Devin](https://devin.ai)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@apps/web/src/components/Form/FormInput.tsx`:
- Around line 73-75: Update the onChange normalization around parsed in
FormInput so numeric validation covers the complete newValue rather than
accepting parseFloat prefixes. Preserve intermediate empty, negative, and
decimal-typing states, accept only finite complete numeric values (including
-34.1147648), and retain invalid text such as "1e", "12abc", "1.2.3", and
"Infinity"; add regression tests for the specified cases.
🪄 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: 3e792b89-18de-453a-9a77-c6a6f620759f
📒 Files selected for processing (2)
apps/web/src/components/Form/FormInput.tsxapps/web/src/components/PageComponents/Settings/Position.tsx
💤 Files with no reviewable changes (1)
- apps/web/src/components/PageComponents/Settings/Position.tsx
| const parsed = Number.parseFloat(newValue); | ||
| controllerField.onChange( | ||
| field.type === "number" | ||
| ? Number.parseFloat(newValue).toString() | ||
| : newValue, | ||
| Number.isNaN(parsed) ? newValue : parsed.toString(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'NODE'
for (const value of ["1e", "1e-", "12abc", "1.2.3", "Infinity"]) {
const parsed = Number.parseFloat(value);
console.log(value, "=>", Number.isNaN(parsed) ? value : parsed.toString());
}
NODERepository: meshtastic/web
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target component around the reported lines.
printf '%s\n' '--- target file vicinity ---'
sed -n '1,120p' apps/web/src/components/Form/FormInput.tsx | cat -n
printf '%s\n' '--- focused relevant section ---'
sed -n '60,85p' apps/web/src/components/Form/FormInput.tsx | cat -n
printf '%s\n' '--- related tests/usages if present ---'
rg -n "FormInput|parseFloat|Number.isFinite|Number\\(" -S apps/web/src/components/Form apps/web/src 2>/dev/null | head -80 || trueRepository: meshtastic/web
Length of output: 6853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'NODE'
const cases = [
"1e",
"1e-",
"12abc",
"1.2.3",
"Infinity",
"Infinityabc",
"-34.1147648",
"-34.1147648abc",
".1147648",
"+1.2.3",
"-1.2.3",
"",
"-",
".",
"-.",
"-.",
];
console.log("parseFloat results:");
for (const value of cases) {
const parsed = Number.parseFloat(value);
const normalized = Number.isNaN(parsed) ? value : parsed.toString();
console.log(JSON.stringify(value), "=>", JSON.stringify(parsed), "normalized =>", JSON.stringify(normalized));
}
console.log("\nNumber with full-string regex results:");
const floatRe = /^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?$/;
for (const value of cases) {
const parsed = Number(value);
const complete = Number.isFinite(parsed) && (value === "" || floatRe.test(value));
console.log(
JSON.stringify(value),
"=>",
JSON.stringify(parsed),
"complete number =>",
complete.toString().toLowerCase(),
"normalized =>",
JSON.stringify(complete ? parsed.toString() : value),
);
}
NODERepository: meshtastic/web
Length of output: 1847
Validate the complete numeric string before normalizing.
Number.parseFloat(newValue) accepts numeric prefixes. "1e", "1e-", "12abc", and "1.2.3" are normalized to "1", "1", "12", and "1.2", so valid prefix text is lost. Valid Infinity text is also preserved through this path.
Use a Number.isFinite() parse result for valid strings and preserve intermediate empty, negative, or decimal-typing states separately. Add regression tests for "1e", "12abc", "Infinity", and -34.1147648.
Proposed fix
- const parsed = Number.parseFloat(newValue);
+ const parsed = Number(newValue);
+ const isCompleteNumber =
+ Number.isFinite(parsed) &&
+ /^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?$/.test(newValue);
controllerField.onChange(
- Number.isNaN(parsed) ? newValue : parsed.toString(),
+ isCompleteNumber ? parsed.toString() : newValue,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsed = Number.parseFloat(newValue); | |
| controllerField.onChange( | |
| field.type === "number" | |
| ? Number.parseFloat(newValue).toString() | |
| : newValue, | |
| Number.isNaN(parsed) ? newValue : parsed.toString(), | |
| const parsed = Number(newValue); | |
| const isCompleteNumber = | |
| Number.isFinite(parsed) && | |
| /^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?$/.test(newValue); | |
| controllerField.onChange( | |
| isCompleteNumber ? parsed.toString() : newValue, |
🤖 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/Form/FormInput.tsx` around lines 73 - 75, Update the
onChange normalization around parsed in FormInput so numeric validation covers
the complete newValue rather than accepting parseFloat prefixes. Preserve
intermediate empty, negative, and decimal-typing states, accept only finite
complete numeric values (including -34.1147648), and retain invalid text such as
"1e", "12abc", "1.2.3", and "Infinity"; add regression tests for the specified
cases.
2b016f0 to
ce35dc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/validation/config/position.ts`:
- Around line 24-35: Update the latitude and longitude schemas in the position
validation definition to normalize blank string inputs to undefined before
numeric coercion, preserving optional behavior for cleared fields. Ensure
clearing either coordinate, or both coordinates, produces undefined rather than
numeric zero in the form output consumed by submit.
- Around line 6-10: Update maxDecimalPlaces to account for exponent digits when
validating decimal precision, rather than relying only on splitting
value.toString() at the decimal point. Ensure exponential values such as 1.2e-7
are rejected when their effective decimal places exceed places, while preserving
the existing undefined and valid-precision behavior.
🪄 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: a9d656bb-d944-4eb8-80b7-8b93708f6c19
📒 Files selected for processing (3)
apps/web/src/components/Form/FormInput.tsxapps/web/src/components/PageComponents/Settings/Position.tsxapps/web/src/validation/config/position.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/Form/FormInput.tsx
| const maxDecimalPlaces = (places: number) => (value: number | undefined) => { | ||
| if (value === undefined) return true; | ||
| const [, decimals] = value.toString().split("."); | ||
| return !decimals || decimals.length <= places; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'NODE'
for (const value of [1e-7, 1.2e-7]) {
const [, decimals] = value.toString().split(".");
console.log({
value,
representation: value.toString(),
currentResult: !decimals || decimals.length <= 7,
});
}
NODERepository: meshtastic/web
Length of output: 278
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate position.ts =="
fd 'position\.ts$' . | sed -n '1,20p'
echo "== file overview =="
for f in $(fd 'position\.ts$' .); do
echo "--- $f"
wc -l "$f"
sed -n '1,80p' "$f" | cat -n
done
echo "== search schema/usages =="
rg -n "maxDecimalPlaces|positionSchema|position" apps/web/src/validation apps/web/src -S | sed -n '1,200p'
echo "== small value string forms for DecimalPlaces =="
node <<'NODE'
for (const places of [6,7,8,9]) {
for (const value of [1e-7, 1.2e-7, 1.2345678e-2]) {
const [, decimals] = value.toString().split(".");
console.log(JSON.stringify({ value, places, toString: value.toString(), decimalFraction: decimals, currentResult: !decimals || decimals.length <= places }));
}
}
NODERepository: meshtastic/web
Length of output: 27657
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== inspect Zod/number coercion path if present =="
rg -n "preprocess|number\(\)|z\\.number|schema:|schema:" apps/web/src/validation apps/web/src/schema -S | sed -n '1,220p'
fd '.*\.ts$' apps/web/src/validation/apps/web/src/schema 2>/dev/null | xargs -r sed -n '1,180p' | sed -n '1,180p'Repository: meshtastic/web
Length of output: 8156
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== validate.ts =="
sed -n '1,80p' apps/web/src/validation/validate.ts | cat -n
echo "== settings position component relevant fields =="
sed -n '220,255p' apps/web/src/components/PageComponents/Settings/Position.tsx | cat -nRepository: meshtastic/web
Length of output: 2320
Count decimal places across exponential notation.
Number.prototype.toString() can produce exponential form for small coordinates. split(".") then misses the exponent, so values like 1.2e-7 pass as only one decimal place when they represent eight. Handle the exponent explicitly or validate the original string before coercion.
🤖 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/validation/config/position.ts` around lines 6 - 10, Update
maxDecimalPlaces to account for exponent digits when validating decimal
precision, rather than relying only on splitting value.toString() at the decimal
point. Ensure exponential values such as 1.2e-7 are rejected when their
effective decimal places exceed places, while preserving the existing undefined
and valid-precision behavior.
| latitude: z.coerce | ||
| .number() | ||
| .min(-90) | ||
| .max(90) | ||
| .optional() | ||
| .refine(maxDecimalPlaces(7), { message: "Max 7 decimal precision" }), | ||
| longitude: z.coerce | ||
| .number() | ||
| .min(-180) | ||
| .max(180) | ||
| .optional() | ||
| .refine(maxDecimalPlaces(7), { message: "Max 7 decimal precision" }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'NODE'
for (const raw of ["", " ", "-", "34."]) {
console.log(JSON.stringify({ raw, coerced: Number(raw) }));
}
NODERepository: meshtastic/web
Length of output: 253
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Files:"
fd -a 'position\.ts|Position\.tsx' . | sed 's#^\./##'
echo
echo "position.ts outline:"
ast-grep outline apps/web/src/validation/config/position.ts || true
echo
echo "position.ts content:"
cat -n apps/web/src/validation/config/position.ts
echo
echo "Position.tsx relevant section:"
sed -n '120,185p' apps/web/src/components/PageComponents/Settings/Position.tsx | cat -n
echo
echo "Search position validation usages:"
rg -n "PositionValidationSchema|latitude|longitude" apps/web/src -g '*.ts' -g '*.tsx' | head -120Repository: meshtastic/web
Length of output: 11787
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "DynamicForm definition locations:"
rg -n "DynamicForm|type .*FormProps|componentProps|ReactHTML|submit" -g '*.ts' -g '*.tsx' apps/web/src | head -200
echo
echo "Search for DynamicForm implementation:"
rg -n "export .*DynamicForm|function DynamicForm|const DynamicForm" apps/web/src -g '*.ts' -g '*.tsx'Repository: meshtastic/web
Length of output: 18021
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "DynamicForm implementation:"
cat -n apps/web/src/components/Form/DynamicForm.tsx | sed -n '76,195p'
echo
echo "DynamicFormField implementation:"
cat -n apps/web/src/components/Form/DynamicFormField.tsx | sed -n '1,140p'
echo
echo "Position settings relevant input fields:"
cat -n apps/web/src/components/PageComponents/Settings/Position.tsx | sed -n '215,260p'
echo
echo "FormInput implementation:"
cat -n apps/web/src/components/Form/FormInput.tsx | sed -n '1,120p'
echo
echo "Root validation imports:"
cat -n apps/web/src/validation/config/position.ts | sed -n '1,60p'
echo
echo "Node coercion behavior for blank inputs:"
node - <<'NODE'
const inputs = ["", " ", "\t", "-"];
for (const value of inputs) {
console.log(JSON.stringify({
display: value || "<empty>",
asNumberString: Number(String(value)),
passedToNumberSchemaLike: Number(String(value)),
}));
}
NODERepository: meshtastic/web
Length of output: 15398
Map empty coordinates to undefined before numeric coercion.
z.coerce.number() treats "" as 0 before .optional(), so clearing a latitude or longitude field can pass a defined zero coordinate to submit and later send a fixed position with that coordinate cleared to 0. Normalize blank strings to undefined before coercion, or update the form output contract and add tests for clearing one coordinate and both coordinates.
[low effort]
🤖 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/validation/config/position.ts` around lines 24 - 35, Update the
latitude and longitude schemas in the position validation definition to
normalize blank string inputs to undefined before numeric coercion, preserving
optional behavior for cleared fields. Ensure clearing either coordinate, or both
coordinates, produces undefined rather than numeric zero in the form output
consumed by submit.
Covers positive/negative latitude and longitude with 7 decimal places, rejection of values exceeding 7 decimal places, and rejection of values outside the valid range. Generated with [Devin](https://devin.ai)
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 `@apps/web/src/validation/config/position.test.ts`:
- Around line 53-67: Extend the tests around PositionValidationSchema to cover
both sides of each coordinate bound: reject latitude -91 and longitude 181, and
verify the inclusive boundary values -90, 90, -180, and 180 are accepted using
validBase.
🪄 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: dab35dee-e7ee-455d-8eac-b526957fd03f
📒 Files selected for processing (1)
apps/web/src/validation/config/position.test.ts
| it("rejects latitude outside the valid range", () => { | ||
| const result = PositionValidationSchema.safeParse({ | ||
| ...validBase, | ||
| latitude: 91, | ||
| }); | ||
| expect(result.success).toBe(false); | ||
| }); | ||
|
|
||
| it("rejects longitude outside the valid range", () => { | ||
| const result = PositionValidationSchema.safeParse({ | ||
| ...validBase, | ||
| longitude: -181, | ||
| }); | ||
| expect(result.success).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test both ends of each coordinate range.
These cases test only latitude values above 90 and longitude values below -180. A schema that accepts latitude below -90 or longitude above 180 still passes this suite. Add rejection tests for -91 and 181. Add acceptance tests for -90, 90, -180, and 180 if the documented bounds are inclusive.
🤖 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/validation/config/position.test.ts` around lines 53 - 67, Extend
the tests around PositionValidationSchema to cover both sides of each coordinate
bound: reject latitude -91 and longitude 181, and verify the inclusive boundary
values -90, 90, -180, and 180 are accepted using validBase.
2 participants
Fixes #1308.
Number.parseFloat("-").toString()produced"NaN"while typing a negative latitude/longitude, preventing users in the southern/western hemisphere from entering coordinates. This change preserves intermediate values (-, empty string, trailing decimal point) and only normalizes complete numbers.Also fixes the latitude/longitude field length limits so negative values with 7 decimal places fit (
-34.1147648/-180.0000000).Adds Zod refine validation on latitude and longitude to enforce a maximum of 7 decimal places, matching the wire format precision (
latitudeI/longitudeIare stored as degrees × 10⁷).Adds unit tests for the position schema covering positive/negative coordinates, 7 decimal precision, and out-of-range values.
Generated with Devin
Summary by CodeRabbit