Fixed overrideFrequency Input Constraints#801
Conversation
…f 410 MHz and maximum of 930 MHz. Added 3 decimal point check.
|
@BMorgan1296 is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes input validation constraints for the overrideFrequency field in the LoRa configuration schema. The change removes the integer constraint and adds proper frequency range validation (410-930 MHz) while limiting precision to 3 decimal places.
- Replaces integer-only validation with float support and frequency range limits
- Adds validation to ensure maximum 3 decimal places precision
- Maintains the existing coercion behavior for backward compatibility
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| overrideDutyCycle: z.boolean(), | ||
| sx126xRxBoostedGain: z.boolean(), | ||
| overrideFrequency: z.coerce.number().int(), | ||
| overrideFrequency: z.coerce.number().min(410).max(930).refine(val => Number(val.toFixed(3)) === val), |
There was a problem hiding this comment.
The decimal precision validation logic is flawed. Number(val.toFixed(3)) === val will fail for valid 3-decimal numbers due to floating-point precision issues. For example, 410.001 may not equal Number((410.001).toFixed(3)) due to IEEE 754 representation. Consider using Math.round(val * 1000) === val * 1000 instead.
| overrideFrequency: z.coerce.number().min(410).max(930).refine(val => Number(val.toFixed(3)) === val), | |
| overrideFrequency: z.coerce.number().min(410).max(930).refine(val => Math.round(val * 1000) === val * 1000), |
There was a problem hiding this comment.
The AI was correct (for once), 410.001 failed and provided 410.0010070800781 on reboot.
However, the provided fix doesn't work and I tried several other methods.
Would a frequency slot validation function be appropriate?
Otherwise, valid inputs seem to work fine without rounding errors.
There was a problem hiding this comment.
If you want you can add a more descriptive error message than "Invalid Input" by passing
{ message: "formValidation.maxSigFig", params: { sigFigs: 3 } }
to refine() and add the translation key to packages/web/public/i18n/locales/en/common.json
There was a problem hiding this comment.
Done, although I wasn't sure how to make it parameterised for the frequency.
|
Nice! The duplicate error messages seems to be a regression. The error messages get inserted in both the FormWrapper and FormInput components but should not be in FormInput I think (they don't get added to the other input components in the same way). |
I've removed the FormInput error message, checked in other parts of the config and that looks to be much nicer without the duplicate. |
|
@philon- Can you approve this PR when you are satisfied it meets the code style and expectations of this project? |
philon-
left a comment
There was a problem hiding this comment.
I am not entirely sure we need to restrict the input to 3 decimals, especially if firmware will change the value to more decimals, as it would likely show validation errors after saving.
The addition of min and max and removing the int requirement is a no brainer.
| "generic": "Това поле е задължително.", | ||
| "managed": "Изисква се поне един администраторски ключ, ако възелът е управляван.", | ||
| "key": "Ключът е задължителен." | ||
| }, |
There was a problem hiding this comment.
Normally we let the community of native speakers add non-english language translations using Crowdin. You don't need to supply all languages in your PR (unless you've got proficiency)
There was a problem hiding this comment.
Normally we let the community of native speakers add non-english language translations using Crowdin. You don't need to supply all languages in your PR (unless you've got proficiency)
No worries. Would you prefer to leave them as English? I saw that was common in the other languages.
There was a problem hiding this comment.
How many decimal places should we accept then?
There was a problem hiding this comment.
If we leave it undefined (just z.coerce.number().min(410).max(930)) the firmware should deal with the rest
There was a problem hiding this comment.
Normally we let the community of native speakers add non-english language translations using Crowdin. You don't need to supply all languages in your PR (unless you've got proficiency)
No worries. Would you prefer to leave them as English? I saw that was common in the other languages.
I think that would work best with the Crowdin workflows 👍
… German. Also fixed a typo in src/components/UI/Input.tsx.
|
Ah my bad I thought I was resolving the merge conflict into my fork'ed version, apologies it is late here. Aside from that, I have simplified the error check, reverted the translated versions besides German and English and also fixed a small typo I noticed in src/components/UI/Input.tsx while chasing down something else. |
|
Looks good to me! |
Description
Fixed overrideFrequency to properly allow for floats, plus a minimum of 410 MHz and maximum of 930 MHz.
Also, only allows 3 decimal places.
Changes Made
Only changed a single line in
packages/web/src/validation/config/lora.ts:From:
overrideFrequency: z.coerce.number().int(),To:
overrideFrequency: z.coerce.number().min(410).max(930).refine(val => Number(val.toFixed(3)) === val),Testing Done
Checked input in Chrome and Edge, see attached screenshots.
I am not sure why there are two error messages, but the changes don't seem to be any different from other similar fields.
Screenshots
Checklist