Simplify IpPoolSelector and call sites#3032
Merged
david-crespo merged 4 commits intoipv6_migrationfrom Feb 4, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
david-crespo
force-pushed
the
simplify-ip-selector
branch
from
fe4f244 to
688f05e
Compare
david-crespo
commented
Feb 4, 2026
| setValue(ipVersionFieldName, selectedPool.ipVersion) | ||
| } | ||
| } | ||
| }, [currentPool, sortedPools, ipVersionFieldName, setValue]) |
Collaborator
Author
There was a problem hiding this comment.
this is not necessary at all if you derive it at submit time
david-crespo
commented
Feb 4, 2026
| ipVersionFieldName, | ||
| setValue, | ||
| autoSelectDefault, | ||
| ]) |
Collaborator
Author
There was a problem hiding this comment.
I think this is only really necessary for instance create because that's the only one where the stack can change. We calculate it for ephemeral IP modal but we can do it once up front because it's in the context of an instance where the stack is fixed.
david-crespo
commented
Feb 4, 2026
| type: 'auto', | ||
| // v4 fallback here should maybe be an error instead because | ||
| // it probably won't work on the API side | ||
| ipVersion: hasV4Default ? 'v4' : hasV6Default ? 'v6' : 'v4', |
Collaborator
Author
There was a problem hiding this comment.
Note v4 fallback where when there are no defaults doesn't really make sense because auto will fail in that case. What we should do is make the user pick. The simplest thing would be to always make the user pick and just not use the auto side here.
Contributor
There was a problem hiding this comment.
I believe this is now how you have it, where the form is disabled with the No default compatible pool available; select a pool to continue message, so I don't know that anyone is going to end up hitting this.
Collaborator
Author
There was a problem hiding this comment.
Ah, that's right. Insight applied here for more cleanup 72b671a
david-crespo
added a commit
that referenced
this pull request
Feb 5, 2026
* Update to new networking API shape, with IPv6
* Update api generator to 0.13.1 and run
* Remove unused helper
* simpler handling, as action is impossible without a pool
* Updates to Networking Interfaces table
* Update tests
* Update form with IPv4, IPv6, dual stack
* proper v4 vs v6 filtering
* update types in form
* more defaults
* flatten default options
* Add instance create tests
* Update to latest Omicron; npm run gen-api
* Bump @oxide/openapi-gen-ts
* npm run gen-api
* Update UX for ephemeral IP attach modal
* e2e text flexibility
* fix bug when defaultPool was falsy
* fix runtime issue if siloPools haven't loaded
* Fix bug where when both IPv4 and IPv6 default pools exist, { poolSelector: { type: 'auto' } } fails unless ipVersion is specified
* Better handling of dual default pools
* Simplify default badging
* Fix v6 automatic pool assignment issue
* Remove DefaultPoolCell
* Ensure unicast pools are used for ephemeral IP form
* Fix incorrect pool issue with Floating IP create flow
* Proper handling of unicast pools in instance create
* make sure external IP version matches NIC type
* Better flow for IP Pool selector; add component
* only show NIC-version-matching IP pools
* fix crashing IP Pools list
* update tests
* Fix issue with sometimes blank field
* Better handling of IP versions to show during create flow
* Better IP type compatability handling
* Better default-pool + ipVersion handling
* Better handle defaults and empty states
* refactor / copy
* another refactor to defaults to match API
* revert copy for now
* external IP version compatibility should consider the primary NIC; add tests for Ephemeral IP attachment
* Ensure that when custom NIC is created, Ephemeral IP options match first custom NIC in list
* Disable ephemeral IP checkbox when instance has no compatible NICs
* Add IP version to silo IP Pools table
* Add IP version to IP Pool create flow
* Proper height for NIC table rows
* Better microcopy, tooltip, validation on IP Pool ranges
* Better microcopy, tooltip, validation on IP Pool ranges
* Update to latest omicron sha (92e0ae0c)
* post-review updates round 1
* clean up NIC table
* latest OMICRON_VERSION
* More updates, plus test fixes
* npm run fmt
* Fix unrelated test
* Better handle default / undefined data
* smarter mock exception handling
* Add logic to determine v4/v6 NICs during create flow
* Use available pools to determine default NIC type on instance create
* refactor logic around determining NIC and pool availability
* refactoring
* more cleanup
* Disable NICs when no VPCs
* Filter Floating IP options based on the available NICs on the instance
* update tests and mock data
* update OMICRON_VERSION; no API changes
* Defer to API for selecting default pool
* Default to selecting Ephemeral IP pool, rather than leaving blank, to reduce conflict when both v4 and v6 defaults exist
* Refactor, work on tests
* npm run fmt
* a few small tweaks before larger experiment on default NIC selection
* use ALL_ISH on NIC list
* decouple NIC defaults from external IP pool configuration
* address flaky/flakey vpc tests
* Fix flaking Firefox failure
* smarter defaults when pool list changes
* Use util to evaluate version without extra lookup
* refactor
* Bump omicron and update api
* Add links to docs in IP Pool Create form
* gap-0 on IP versions double row
* Remove icon
* Proper radio buttons for NetworkInterfaceField
* Use MiniTable with add/clear button pattern
* Fix IP Pool column title in test
* remove extra testing column
* add explicit blur step to help with Firefox in e2e tests
* update reducedMotion setting in Playwright config for e2e tests
* TS updates
* More playwright config tweaks; working locally but who knows
* Add blur back in; again, all working locally
* use remeda sortBy with array to sort pools
* remove falsy checks on things that can't be falsy
* split compatible pools filter into -cast and ip version
Renames the pool-filtering hasIpVersion to poolHasIpVersion to make
room for a predicate that filters by parsing the actual IP string.
* replace filterFloatingIpsByVersion with hasIpVersion predicate
Mirrors the poolHasIpVersion pattern: a curried predicate that can be
used directly with Array.filter, e.g. .filter(hasIpVersion(['v4'])).
* stable array for IP versions; ts-pattern for NIC type matching
Hoists ALL_IP_VERSIONS constant to avoid creating a new array on every
render when compatibleVersions prop is omitted, which was breaking
useMemo caching.
Rewrites getCompatibleVersionsFromNicType with exhaustive ts-pattern
match for clearer control flow.
* CLAUDE.md: add guidance on avoiding type casts
* network-interface-create: add form values type to avoid cast
* tests for IpPoolSelector
* placeholder copy
* test default pools with elaborate mock situation
* simplify resolveIpStack, move to utils file
* Simplify `IpPoolSelector` and call sites (#3032)
* refactor IpPoolSelector, derive ipVersion
* handle default pool in defaultValues, don't sort
* get rid of advanced according on floating IP create
* update e2es for removed advanced accordion
* always require pool on floating IP and ephemeral IP attach
* fix bad bad use of () => [...] sorter in remeda sortBy
* get rid of ephemeralIpVersion form state var
* instance-create: construct externalIps on submit
use explicit form state for ephemeral IP and floating IPs state,
build externalIps list out of those at submit time
* small fixes
* avoid required error on invisible pool selector
* fix e2e failures by picking a pool even when two defaults
* compatible versions constants to avoid render churn
* IpPoolSelector: always mount Listbox even when there are no pools
* no pools silo for testing
* ephemeral IP checkbox disabled tooltip on label hover
* use ben's styling suggestion on toIpPoolItem
* sort pools first, checkbox 'checked', no explicit any
---------
Co-authored-by: David Crespo <david.crespo@oxidecomputer.com>
Co-authored-by: David Crespo <david-crespo@users.noreply.github.com>
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.
IpPoolSelectoris used on instance create, floating IP create, and ephemeral IP attach, but most of the logic inside only applied to instance create. Here I:IpPoolSelectorand into instance create (the only call site that cares)ipVersionFieldName,currentPool, andsetValueuseEffects and intouseFormdefaultValuesuseQueryipVersionat submit time instead of keeping track of it in the form stateI don't think any actual behavior should change here.