Set and clear default pool from silos list on pool detail#3060
Set and clear default pool from silos list on pool detail#3060david-crespo merged 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
e1ea1fb to
9ebfcf3
Compare
|
Very useful review from GPT-5.2 high in the codex CLI. 2 is trivial, thinking about 1.
|
| .catch(() => null) | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This whole inline fetch thing is weird, but it ensures we have the data before the modal is rendered, kind of like a loader. I considered an alternative where instead we render a component as the modal contents, which means it can call useQuery in order to render first and then fetch the uncached silo information as needed. The problem, as with all such render-then-fetch setups, is that you have visible pop-in of the result of the query. You can restructure the text so that the pop-in is less noticeable, but that makes it less readable, and you still have the pop-in. So I prefer the above even though it's a little weird. What it makes me really want is a way of doing something that works like a loader but for pseudo-navs like modal opens that are not route changes. There probably is a way to build that.
Diff of alternative approach
diff --git a/app/pages/system/networking/IpPoolPage.tsx b/app/pages/system/networking/IpPoolPage.tsx
index 16a4b47bff..19f404c38e 100644
--- a/app/pages/system/networking/IpPoolPage.tsx
+++ b/app/pages/system/networking/IpPoolPage.tsx
@@ -19,6 +19,7 @@
queryClient,
useApiMutation,
usePrefetchedQuery,
+ type IpPool,
type IpPoolRange,
type IpPoolSiloLink,
type Silo,
@@ -74,16 +75,7 @@
const selector = getIpPoolSelector(params)
await Promise.all([
queryClient.prefetchQuery(ipPoolView(selector)),
- // prefetch silo pool lists so "Make default" can show existing default name.
- // fire-and-forget: don't block page load, the action handler fetches on
- // demand if these haven't completed yet
- queryClient.fetchQuery(ipPoolSiloList(selector).optionsFn()).then((links) => {
- // only do first 50 to avoid kicking of a ridiculous number of requests if
- // the user has 500 silos for some reason
- for (const link of links.items.slice(0, 50)) {
- queryClient.prefetchQuery(siloIpPoolList(link.siloId))
- }
- }),
+ queryClient.prefetchQuery(ipPoolSiloList(selector).optionsFn()),
queryClient.prefetchQuery(ipPoolRangeList(selector).optionsFn()),
queryClient.prefetchQuery(ipPoolUtilizationView(selector)),
@@ -266,6 +258,50 @@
const silosColHelper = createColumnHelper<IpPoolSiloLink>()
+function SetDefaultModalContent({
+ siloId,
+ siloName,
+ pool,
+}: {
+ siloId: string
+ siloName?: string
+ pool: Pick<IpPool, 'name' | 'ipVersion' | 'poolType'>
+}) {
+ const { data: siloPools } = useQuery(siloIpPoolList(siloId))
+
+ const existingDefaultName = siloPools?.items.find(
+ (p) => p.isDefault && p.ipVersion === pool.ipVersion && p.poolType === pool.poolType
+ )?.name
+
+ const poolKind = `IP${pool.ipVersion} ${pool.poolType}`
+ // prettier-ignore
+ const siloLabel = siloName
+ ? <>silo <HL>{siloName}</HL></>
+ : 'that silo'
+
+ return (
+ <>
+ <p>
+ Are you sure you want to make <HL>{pool.name}</HL> the default {poolKind} pool for{' '}
+ {siloLabel}?
+ </p>
+ <p className="text-secondary">
+ Setting a new default replaces any existing default. Current default for {poolKind}:{' '}
+ {siloPools ? (
+ existingDefaultName ? (
+ <HL>{existingDefaultName}</HL>
+ ) : (
+ 'none'
+ )
+ ) : (
+ <span className="bg-tertiary inline-block h-4 w-20 rounded-md align-middle motion-safe:animate-pulse" />
+ )}
+ .
+ </p>
+ </>
+ )
+}
+
function LinkedSilosTable() {
const poolSelector = useIpPoolSelector()
const { data: pool } = usePrefetchedQuery(ipPoolView(poolSelector))
@@ -288,12 +324,9 @@
label: link.isDefault ? 'Clear default' : 'Make default',
className: link.isDefault ? 'destructive' : undefined,
onActivate() {
- const silo = queryClient.getQueryData<Silo>(
+ const siloName = queryClient.getQueryData<Silo>(
siloView({ silo: link.siloId }).queryKey
- )
- // in order to hit this fallback, the user would have to have more
- // than 1000 silos and be working on the 1001th
- const siloName = silo?.name
+ )?.name
// prettier-ignore
const siloLabel = siloName
? <>silo <HL>{siloName}</HL></>
@@ -319,52 +352,23 @@
actionType: 'danger',
})
} else {
- // fetch on demand (usually already cached by loader prefetch). on
- // failure, fall back to simpler modal copy. don't await, handle
- // errors internally to minimize blast radius of failure.
- void queryClient
- // ensureQueryData makes sure we use cached data, at the expense
- // of it possibly being stale. but you can't even change a silo
- // name, so it should be fine
- .ensureQueryData(siloIpPoolList(link.siloId))
- .catch(() => null)
- .then((siloPools) => {
- const existingDefaultName = siloPools?.items.find(
- (p) =>
- p.isDefault &&
- p.ipVersion === pool.ipVersion &&
- p.poolType === pool.poolType
- )?.name
-
- // all this conditional stuff is just to handle the remote but
- // real possibility of the fetch failing
- const modalContent = existingDefaultName ? (
- <p>
- Are you sure you want to change the default {poolKind} pool for{' '}
- {siloLabel} from <HL>{existingDefaultName}</HL> to <HL>{pool.name}</HL>?
- </p>
- ) : (
- <p>
- Are you sure you want to make <HL>{pool.name}</HL> the default{' '}
- {poolKind} pool for {siloLabel}?
- </p>
- )
-
- const verb = existingDefaultName ? 'change' : 'make'
- confirmAction({
- doAction: () =>
- updateSiloLink({
- path: { silo: link.siloId, pool: link.ipPoolId },
- body: { isDefault: true },
- }),
- modalTitle: `Confirm ${verb} default`,
- modalContent,
- errorTitle: `Could not ${verb} default`,
- actionType: 'primary',
- })
- })
- // be extra sure we don't have any unhandled promise rejections
- .catch(() => null)
+ confirmAction({
+ doAction: () =>
+ updateSiloLink({
+ path: { silo: link.siloId, pool: link.ipPoolId },
+ body: { isDefault: true },
+ }),
+ modalTitle: 'Confirm set default',
+ modalContent: (
+ <SetDefaultModalContent
+ siloId={link.siloId}
+ siloName={siloName}
+ pool={pool}
+ />
+ ),
+ errorTitle: 'Could not set default',
+ actionType: 'primary',
+ })
}
},
},
diff --git a/test/e2e/ip-pools.e2e.ts b/test/e2e/ip-pools.e2e.ts
index 9279b476d4..87e7d46603 100644
--- a/test/e2e/ip-pools.e2e.ts
+++ b/test/e2e/ip-pools.e2e.ts
@@ -133,12 +133,16 @@
await clickRowAction(page, 'pelerines', 'Make default')
- const dialog = page.getByRole('dialog', { name: 'Confirm make default' })
+ const dialog = page.getByRole('dialog', { name: 'Confirm set default' })
await expect(
dialog.getByText(
'Are you sure you want to make ip-pool-1 the default IPv4 unicast pool for silo pelerines?'
)
).toBeVisible()
+ await expect(
+ dialog.getByText('Setting a new default replaces any existing default.')
+ ).toBeVisible()
+ await expect(dialog.getByText('Current default for IPv4 unicast: none.')).toBeVisible()
await page.getByRole('button', { name: 'Confirm' }).click()
await expectRowVisible(table, { Silo: 'pelerines', 'Silo default': 'default' })
@@ -153,12 +157,18 @@
await clickRowAction(page, 'myriad', 'Make default')
- const dialog = page.getByRole('dialog', { name: 'Confirm change default' })
+ const dialog = page.getByRole('dialog', { name: 'Confirm set default' })
await expect(
dialog.getByText(
- 'Are you sure you want to change the default IPv4 unicast pool for silo myriad from ip-pool-1 to ip-pool-3?'
+ 'Are you sure you want to make ip-pool-3 the default IPv4 unicast pool for silo myriad?'
)
).toBeVisible()
+ await expect(
+ dialog.getByText('Setting a new default replaces any existing default.')
+ ).toBeVisible()
+ await expect(
+ dialog.getByText('Current default for IPv4 unicast: ip-pool-1.')
+ ).toBeVisible()
await page.getByRole('button', { name: 'Confirm' }).click()
await expectRowVisible(table, { Silo: 'myriad', 'Silo default': 'default' })There was a problem hiding this comment.
Had Mr. Claude figure out how a loader-like setup for modals would have to work, based on this PR and the above comment. It's quite helpful despite the Core Tensions and Key Insights. Spinner on the button that opens the modal is interesting, though we'd probably want to have it on a timer so it only shows up if the request takes longer than 300ms or something.
The core tension: route loaders give you fetch-before-render for free, but modals are orthogonal to routing. You either pay with imperative .then() chains or with visible loading states.
The best fit for console: prefetchQuery + startTransition + useSuspenseQuery
This is the pattern that composes most naturally with the existing TanStack Query + React 19 stack:
- Click handler calls
queryClient.prefetchQuery(...)to warm the cache, then wraps the modal-open state change instartTransition. - Modal content component calls
useSuspenseQuery(...), which suspends if data isn't cached yet. startTransitionprevents the Suspense fallback from flashing — React keeps the current UI visible while the modal's data resolves in the background.isPendingfromuseTransitioncan drive a subtle loading indicator on the trigger button.
If data is already cached (the common case after the loader prefetched), the modal opens instantly. If not, the button shows a brief spinner, then the modal appears fully populated. No .then() chains, no pop-in, and the modal component reads data declaratively.
The key insight is that startTransition is the missing piece that makes Suspense work for modals — without it, use() / useSuspenseQuery always shows the Suspense fallback initially, even for already-resolved promises (React issue #30701). With it, already-revealed content stays put while React renders the suspended tree offscreen.
Other patterns considered
TanStack Router route masking — Modals as real routes with real loaders, URL masked to the parent path. Elegant concept but requires a router migration, and has known issues (preloading broken for masked links, unexpected scroll-to-top).
Next.js parallel/intercepting routes — Same idea (modal as route with loader), framework-specific. Not applicable.
React 19 use() with promise-as-prop — Start a promise in the click handler, store it in state, call use(promise) in the modal. Works but redundant with TanStack Query's cache, and requires startTransition anyway to avoid the fallback flash.
React Router modal-as-route — Pass previous location as state, render the new route as a modal over the old one. The modal route gets a loader. Requires restructuring modals as routes, which is a big lift for the dozens of modals in console.
Hover prefetching — Call prefetchQuery on onMouseEnter/onFocus of the trigger. Orthogonal to the above and can be layered on top for even faster perceived performance. TanStack Router does this with defaultPreload="intent" on links.
What this would look like for the confirmAction pattern
The zustand-based confirmAction currently takes a pre-built modalContent: ReactNode, which means data must be fully resolved before calling it. To support the useSuspenseQuery pattern, confirmAction (or a new variant) would need to accept a component/render function instead of a static ReactNode, so the modal content can suspend. The ConfirmActionModal component would wrap that content in a <Suspense> boundary, and the caller would use useTransition to open it. This is a meaningful but contained refactor of the confirm-action store.
oxidecomputer/console@bf5d6ef...64a717d * [64a717df](oxidecomputer/console@64a717df) oxidecomputer/console#3060 * [336c4850](oxidecomputer/console@336c4850) oxidecomputer/console#3059 * [5ea92ccd](oxidecomputer/console@5ea92ccd) oxidecomputer/console#3058 * [7e3ca93e](oxidecomputer/console@7e3ca93e) oxidecomputer/console#3056
oxidecomputer/console@bf5d6ef...64a717d * [64a717df](oxidecomputer/console@64a717df) oxidecomputer/console#3060 * [336c4850](oxidecomputer/console@336c4850) oxidecomputer/console#3059 * [5ea92ccd](oxidecomputer/console@5ea92ccd) oxidecomputer/console#3058 * [7e3ca93e](oxidecomputer/console@7e3ca93e) oxidecomputer/console#3056
Closes #2092
Closes #2177
I realized while adding set/unset default that the
DEFAULTalone in the table isn't sufficient, and you can't tell on the pool detail page what IP version the pool is or whether it's multicast or unicast. So I changed the capacity bar thing at the top to a properties table that includes those details.2026-02-07-ip-pool-make-default.mp4