Consolidate number field min/max validation and revalidate when bounds change - #3376
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
fakemonster
reviewed
Sep 9, 2026
fakemonster
left a comment
fakemonster
left a comment
Contributor
There was a problem hiding this comment.
agreed that the faux-clamping behavior was a bug, but now the faux-clamping still happens WITH error messages, which i'm not sure is a huge improvement:
Collaborator
Author
|
I would say blocking form submit and adding the bizarre message is at worst a lateral move. But I'll PR the react-aria upgrade that fixes it in a minute anyway. |
david-crespo
added this pull request to stack #3378
September 9, 2026 23:11
david-crespo
force-pushed
the
number-field-validation
branch
from
September 11, 2026 21:48
9cef155 to
e83ba1e
Compare
david-crespo
added a commit
that referenced
this pull request
Sep 11, 2026
This is on top of #3376, which made `NumberField` validate `min` and `max` itself so we could stop relying on react-aria's clamping. This PR upgrades react-aria 3.44 → 3.52.1 and react-stately 3.32 → 3.50.0 and turns off clamping with the new `commitBehavior: 'validate'` option from adobe/react-spectrum#9679 (merged March 2026). It also adds whole-number validation to `NumberField`, which fixes a pre-existing bug: nothing client-side stopped you from submitting `2.5` CPUs. ### What changes for the user Before, typing an out-of-range value and leaving the field rewrote it to the nearest bound. Now, if a value is out of range, or fractional in a field that requires a whole number, submit is blocked and the field shows an error. The validation trigger is still react-hook-form's default: no errors until the first submit attempt, and after that it revalidates on change events. Form state updates on every keystroke that parses to a number, as long as the text is already in canonical form. react-aria's commit would rewrite `007` or `1.0` to `7` or `1`, so those wait for blur or Enter, as they did before. Out-of-range values no longer wait: they used to sit in the input until blur while form state kept the old value, and now they reach form state immediately, which is what lets the error clear as soon as you fix the number. Disk size, min 6 GiB from the selected image: | Step | Before | After | | --- | --- | --- | | Type `5` | `5` | `5` | | Tab away | Box becomes `6`, no message | Box stays `5`, no message | | Click submit | Creates a 6 GiB disk | Blocked, "Must be at least 6 GiB" | | Change to `20` | n/a | Error clears as soon as `20` is in the box | Prefix length on the external subnet form, where the max depends on the selected pool (32 for v4, 128 for v6): | Step | Before | After | | --- | --- | --- | | v6 pool, type `64` | `64` | `64` | | Switch to v4 pool | Box shows `32`, form still holds `64` | Box shows `64` | | Click submit | Sends `prefix_length: 64` (main). On #3376, blocked with "Can be at most 32" under a box that says 32 | Blocked, "Can be at most 32" | | Switch back to v6 | `64` reappears | Error clears, `64` | | Switch to v4 again | Box shows `32`, form holds `64` | Box shows `64`, "Can be at most 32" | The middle row is the bug that motivated this. `useNumberFieldState` clamps the `value` prop we pass in before storing it, so the text in the box was derived from the clamped 32 while the value react-hook-form held (and would submit) was still 64. The validation from #3376 at least prevents submission, but the input still showed the wrong value. With `commitBehavior: 'validate'`, the incoming value is used as-is. ### What stays the same - **Steppers still stop at the bounds.** `commitBehavior` only affects the typed value and the incoming `value` prop. Increment and decrement still go through `snapValueToStep(prev, minValue, maxValue, step)` and the buttons still disable at min and max. Pressing an arrow while the box holds an out-of-range value snaps it to the nearest bound (type `2` with min 10, press up, get `10`). - **Blur and Enter still canonicalize.** `007` becomes `7` and `1.0` becomes `1`, as before. ### Whole-number validation No form passes `step`, so react-aria never snapped typed values to integers. On main, `2.5` in the CPU field is accepted as-is and sent to the API as `ncpus: 2.5`, which Nexus 400s on. This is independent of clamping, but it is fixed here. `NumberField` now checks `Number.isInteger` by default. Typing `2.5` CPUs leaves `2.5` in the box, but submit is blocked with "Must be a whole number". Correcting it to `3` clears the error. This covers CPU counts and CPU quotas, prefix lengths, firewall rule priority, disk sizes, and instance memory, including resize. Memory and storage quotas on silo create and quota edit opt into `allowDecimals`. Those fields are entered in GiB and converted to bytes, so values like `1.5 GiB` are allowed. Instance memory and disk sizes still require whole GiB because the API enforces [1 GiB alignment for memory](https://github.com/oxidecomputer/omicron/blob/17e6fee5320ad8f1fb4cbfd3d6328b808fca7124/nexus/src/app/instance.rs#L2894-L2908) and [disk sizes](https://github.com/oxidecomputer/omicron/blob/17e6fee5320ad8f1fb4cbfd3d6328b808fca7124/nexus/src/app/disk.rs#L230-L244). Allowing decimals there exposed a pre-existing bug: the quota forms multiplied GiB by 2^30 without rounding, so `1.1 GiB` sent a fractional byte count that the API rejects at deserialization. The last commit rounds to whole bytes in both submit handlers. One thing left for a follow-up: the mock API accepts fractional CPU counts and unaligned sizes that Nexus would reject. The fix is in the next PR in the stack: #3380. ### Tests Every existing assertion that a value got clamped, in the `NumberInput` browser spec and in the e2e specs for disks, instance create, and external subnets, is rewritten to assert the value stays put and the error appears. New `NumberInput` cases cover the steppers stopping at the bounds and snapping an out-of-range typed value back into range. A new `NumberField` browser spec covers the whole-number rule and `allowDecimals`. The external subnet test is the regression test for the faux-clamping bug: type `64` on a v4 pool, submit, switch to v6 and back. It fails on main and on #3376, where the box shows 32. ### React Aria packaging change The lockfile got 1700 lines shorter because react-aria and react-stately now ship as single packages instead of dozens of `@react-aria/*` and `@react-stately/*` deps, which also forced two imports in `Popover.tsx` and `DateField.tsx` to move to the top-level package.
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Sep 12, 2026
oxidecomputer/console@700ff8b...96092fc * [96092fcc](oxidecomputer/console@96092fcc) oxidecomputer/console#3383 * [fc3d969e](oxidecomputer/console@fc3d969e) oxidecomputer/console#3380 * [cc7ab241](oxidecomputer/console@cc7ab241) oxidecomputer/console#3377 * [c187b75c](oxidecomputer/console@c187b75c) oxidecomputer/console#3376 * [add81de6](oxidecomputer/console@add81de6) oxidecomputer/console#3382 * [3a4ae2a6](oxidecomputer/console@3a4ae2a6) oxidecomputer/console#3320 * [4d1215e2](oxidecomputer/console@4d1215e2) oxidecomputer/console#3371 * [7b434ba9](oxidecomputer/console@7b434ba9) oxidecomputer/console#3374 * [915d68a0](oxidecomputer/console@915d68a0) oxidecomputer/console#3373 * [fb929929](oxidecomputer/console@fb929929) oxidecomputer/console#3369 * [e7ee1bbe](oxidecomputer/console@e7ee1bbe) oxidecomputer/console#3368 * [0a4aca0b](oxidecomputer/console@0a4aca0b) oxidecomputer/console#3367
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is prep for upgrading React Aria, which will let us turn off clamping in the number input and make
NumberFieldresponsible for enforcingminandmax. On main,DiskSizeFieldis the only number field that validates bounds for itself; the prefix length field on the external subnet form and every otherNumberFieldwith aminormaxrely on the clamping. Once clamping is off, those fields need explicit validation.So, in this PR we:
NumberFieldand deleteDiskSizeField. The validate rule that used to live inDiskSizeFieldnow runs for every number field with aminormax, with the units appended to the message ("Can be at most 32", "Must be at least 10 GiB"). That leftDiskSizeFieldas a wrapper settingunits="GiB",required, andmin={1}. Its two callers now pass those directly.depstoListboxFieldand use it on the external subnet form so changing the pool re-validates prefix length. Without this, a stale "Can be at most 32" would hang around after switching back to a v6 pool, since by default react-hook-form only re-runs validation on the field that changed.depsto disk type radio in disk create, so an existing size error updates (e.g., is cleared if appropriate) when switching between Local and Distributed instead of waiting for the next submit.This also fixes a bug on main: the clamp is display-only. Entering 64 on a v6 pool and switching to v4 shows 32 in the box, but form state still holds 64 and submit sends
prefix_length: 64. With this change, submit is blocked with a message instead.The existing e2e specs for disks, external subnets, instance create, and subnet pools pass unchanged. The external subnet prefix length test gets a submit assertion covering the display-only clamp bug.