-
Notifications
You must be signed in to change notification settings - Fork 304
fix(settings): allow negative coordinates in position config #1381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { PositionValidationSchema } from "./position.ts"; | ||
|
|
||
| const validBase = { | ||
| positionBroadcastSecs: 0, | ||
| positionBroadcastSmartEnabled: false, | ||
| fixedPosition: false, | ||
| gpsUpdateInterval: 0, | ||
| positionFlags: 0, | ||
| rxGpio: 0, | ||
| txGpio: 0, | ||
| broadcastSmartMinimumDistance: 0, | ||
| broadcastSmartMinimumIntervalSecs: 0, | ||
| gpsEnGpio: 0, | ||
| gpsMode: 0, | ||
| }; | ||
|
|
||
| describe("PositionValidationSchema", () => { | ||
| it("accepts positive latitude and longitude with 7 decimal places", () => { | ||
| const result = PositionValidationSchema.safeParse({ | ||
| ...validBase, | ||
| latitude: 34.1147648, | ||
| longitude: 28.3166667, | ||
| }); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it("accepts negative latitude and longitude with 7 decimal places", () => { | ||
| const result = PositionValidationSchema.safeParse({ | ||
| ...validBase, | ||
| latitude: -34.1147648, | ||
| longitude: -122.4194165, | ||
| }); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it("rejects latitude with more than 7 decimal places", () => { | ||
| const result = PositionValidationSchema.safeParse({ | ||
| ...validBase, | ||
| latitude: -34.11476481, | ||
| }); | ||
| expect(result.success).toBe(false); | ||
| }); | ||
|
|
||
| it("rejects longitude with more than 7 decimal places", () => { | ||
| const result = PositionValidationSchema.safeParse({ | ||
| ...validBase, | ||
| longitude: -122.41941654, | ||
| }); | ||
| expect(result.success).toBe(false); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
|
Comment on lines
+53
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Test both ends of each coordinate range. These cases test only latitude values above 🤖 Prompt for AI Agents |
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,12 @@ import { z } from "zod/v4"; | |
|
|
||
| const GpsModeEnum = z.enum(Protobuf.Config.Config_PositionConfig_GpsMode); | ||
|
|
||
| const maxDecimalPlaces = (places: number) => (value: number | undefined) => { | ||
| if (value === undefined) return true; | ||
| const [, decimals] = value.toString().split("."); | ||
| return !decimals || decimals.length <= places; | ||
| }; | ||
|
Comment on lines
+6
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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.
🤖 Prompt for AI Agents |
||
|
|
||
| export const PositionValidationSchema = z.object({ | ||
| positionBroadcastSecs: z.coerce.number().int().min(0), | ||
| positionBroadcastSmartEnabled: z.boolean(), | ||
|
|
@@ -15,8 +21,18 @@ export const PositionValidationSchema = z.object({ | |
| broadcastSmartMinimumIntervalSecs: z.coerce.number().int().min(0), | ||
| gpsEnGpio: z.coerce.number().int().min(0), | ||
| gpsMode: GpsModeEnum, | ||
| latitude: z.coerce.number().min(-90).max(90).optional(), | ||
| longitude: z.coerce.number().min(-180).max(180).optional(), | ||
| 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" }), | ||
|
Comment on lines
+24
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ 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
[low effort] 🤖 Prompt for AI Agents |
||
| altitude: z.coerce.number().optional(), | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: meshtastic/web
Length of output: 215
🏁 Script executed:
Repository: meshtastic/web
Length of output: 6853
🏁 Script executed:
Repository: 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. ValidInfinitytext 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
📝 Committable suggestion
🤖 Prompt for AI Agents