From 63a48a8a4dd530172f04237920361cddae876393 Mon Sep 17 00:00:00 2001 From: Nadav Erell Date: Sat, 3 Oct 2026 16:00:03 +0300 Subject: [PATCH 1/3] Fix dropdown Tab order and names, and keep paused reviews per identity SelectMenu's Tab stop now follows the focused option, so Tab leaves an open list instead of returning to the selected option, and the trigger's accessible name includes the selected value ("Cost source: Kubecost"), as a native select announces it. Placeholder menus keep their label. The paused-settings review compares against one previous identity, so it now only offers integrations paused from that same identity; the others are named for review in their own tab, without claiming a different cluster when an identity was never recorded. The per-cluster list's loading, empty, error and remove wording now matches its heading, and two zero-height siblings no longer add extra space above the review page. --- .../k8s-ui/src/components/ui/SelectMenu.tsx | 9 +- web/e2e/settings-connections.spec.ts | 83 ++++++++++++------- .../settings/LocalConfigurationDetails.tsx | 26 +++--- .../settings/LocalConnectionSettings.tsx | 33 ++++++-- 4 files changed, 99 insertions(+), 52 deletions(-) diff --git a/packages/k8s-ui/src/components/ui/SelectMenu.tsx b/packages/k8s-ui/src/components/ui/SelectMenu.tsx index a0c11cc8aa..c98c611dde 100644 --- a/packages/k8s-ui/src/components/ui/SelectMenu.tsx +++ b/packages/k8s-ui/src/components/ui/SelectMenu.tsx @@ -55,7 +55,6 @@ export function SelectMenu({ if (!normalized) return options return options.filter((option) => `${option.label} ${option.description ?? ''}`.toLowerCase().includes(normalized)) }, [options, query]) - const selectedIsVisible = filteredOptions.some((option) => option.value === value) const focusTabbableOption = () => { listRef.current?.querySelector('[role="option"][tabindex="0"]')?.focus() @@ -153,7 +152,9 @@ export function SelectMenu({ ref={triggerRef} id={id} type="button" - aria-label={ariaLabel} + // The fixed label would otherwise hide the visible value from screen + // readers, which a native select announces. + aria-label={selected ? `${ariaLabel}: ${selected.label}` : ariaLabel} aria-describedby={ariaDescribedBy} aria-haspopup="listbox" aria-expanded={open} @@ -283,9 +284,7 @@ export function SelectMenu({ role="option" aria-selected={active} aria-disabled={option.disabled || undefined} - tabIndex={ - searchPlaceholder ? (index === highlightedIndex ? 0 : -1) : active || (!selectedIsVisible && index === 0) ? 0 : -1 - } + tabIndex={index === highlightedIndex ? 0 : -1} onClick={() => selectOption(option.value)} onFocus={() => setHighlightedIndex(index)} className={clsx( diff --git a/web/e2e/settings-connections.spec.ts b/web/e2e/settings-connections.spec.ts index 95f3e3d93a..22009f6f7a 100644 --- a/web/e2e/settings-connections.spec.ts +++ b/web/e2e/settings-connections.spec.ts @@ -655,7 +655,7 @@ test('copy after a target change warns before replacement and reload clears stal }) async function chooseCostSource(page: Page, label: 'Automatic' | 'OpenCost metrics' | 'Kubecost') { - await page.getByRole('button', { name: 'Cost source', exact: true }).click() + await page.getByRole('button', { name: /^Cost source:/ }).click() await page.getByRole('option', { name: new RegExp(`^${label}`) }).click() } @@ -702,10 +702,10 @@ test('old context cleanup lives in Connection and clearly scopes credential remo await page.getByRole('tab', { name: 'Connection', exact: true }).click() await expect(page.getByRole('heading', { name: 'Integration settings by cluster', exact: true })).toBeVisible() await expect(page.getByText('Not in kubeconfig', { exact: true })).toBeVisible() - const remove = page.getByRole('button', { name: 'Remove saved Metrics connection for old-cluster', exact: true }) + const remove = page.getByRole('button', { name: 'Remove saved Metrics settings for old-cluster', exact: true }) await remove.click() - await expect(page.getByText('Remove the saved Metrics connection and credentials for old-cluster?', { exact: false })).toBeVisible() - await expect(page.getByRole('dialog').filter({ has: page.getByRole('heading', { name: 'Remove saved connection?' }) })).toBeVisible() + await expect(page.getByText('Remove the saved Metrics settings and credentials for old-cluster?', { exact: false })).toBeVisible() + await expect(page.getByRole('dialog').filter({ has: page.getByRole('heading', { name: 'Remove saved settings?' }) })).toBeVisible() await page.keyboard.press('Escape') await expect(remove).toBeFocused() await remove.click() @@ -722,12 +722,12 @@ test('Connection groups stale integrations by context and removes only the confi await page.getByRole('textbox', { name: 'Metrics backend URL' }).fill('https://metrics.example/draft') await page.getByRole('tab', { name: 'Connection', exact: true }).click() await expect(page.getByText('old-cluster', { exact: true })).toHaveCount(1) - await page.getByRole('button', { name: 'Remove saved Cost connection for old-cluster', exact: true }).click() - await page.getByRole('button', { name: 'Remove connection', exact: true }).click() - await expect(page.getByText('Removed the saved Cost connection for old-cluster.', { exact: true })).toBeVisible() + await page.getByRole('button', { name: 'Remove saved Cost settings for old-cluster', exact: true }).click() + await page.getByRole('button', { name: 'Remove settings', exact: true }).click() + await expect(page.getByText('Removed the saved Cost settings for old-cluster.', { exact: true })).toBeVisible() expect(state.writes).toEqual([{ action: 'forget', kind: 'cost', binding: 'old', sourceRevision: 'old-cost', confirmRemoval: true }]) - await expect(page.getByRole('button', { name: 'Remove saved Cost connection for old-cluster', exact: true })).toHaveCount(0) - await expect(page.getByRole('button', { name: 'Remove saved Argo CD connection for old-cluster', exact: true })).toBeVisible() + await expect(page.getByRole('button', { name: 'Remove saved Cost settings for old-cluster', exact: true })).toHaveCount(0) + await expect(page.getByRole('button', { name: 'Remove saved Argo CD settings for old-cluster', exact: true })).toBeVisible() await page.getByRole('tab', { name: 'Metrics', exact: true }).click() await expect(page.getByRole('textbox', { name: 'Metrics backend URL' })).toHaveValue('https://metrics.example/draft') }) @@ -745,9 +745,9 @@ test('Connection preserves the removed-versus-unavailable distinction and cannot await expect(page.getByText('config.json', { exact: true })).toBeVisible() await page.getByRole('tab', { name: 'Connection', exact: true }).click() await expect(page.getByText('Kubeconfig not loaded', { exact: true })).toBeVisible() - await expect(page.getByRole('button', { name: 'Remove saved Metrics connection for development', exact: true })).toHaveCount(0) - await expect(page.getByRole('button', { name: 'Remove saved Cost connection for staging', exact: true })).toBeVisible() - await page.getByRole('button', { name: 'Remove saved Cost connection for staging', exact: true }).click() + await expect(page.getByRole('button', { name: 'Remove saved Metrics settings for development', exact: true })).toHaveCount(0) + await expect(page.getByRole('button', { name: 'Remove saved Cost settings for staging', exact: true })).toBeVisible() + await page.getByRole('button', { name: 'Remove saved Cost settings for staging', exact: true }).click() await expect(page.getByText(/This entry may still exist in another kubeconfig/)).toBeVisible() }) @@ -757,22 +757,22 @@ test('saved settings load failure offers recovery rather than an empty-state cla await page.route('**/api/integrations/connections', route => fail ? route.fulfill({ status: 500, json: { error: 'Could not read saved settings.' } }) : route.fallback()) await openSettings(page, 'Connection') await expect(page.getByRole('alert')).toContainText('Could not read saved settings.') - await expect(page.getByText('No saved connections yet.', { exact: true })).toHaveCount(0) + await expect(page.getByText('No integration settings saved yet.', { exact: true })).toHaveCount(0) fail = false - await page.getByRole('button', { name: 'Reload saved connections', exact: true }).click() - await expect(page.getByText('No saved connections yet.', { exact: true })).toBeVisible() + await page.getByRole('button', { name: 'Reload integration settings', exact: true }).click() + await expect(page.getByText('No integration settings saved yet.', { exact: true })).toBeVisible() }) test('cleanup blocks close while committing and keeps remaining cards mounted without a refetch', async ({ page }) => { const state = await fixture(page) state.connections.push(...(['metrics', 'cost'] as const).map(integration => ({ ...discoverySettings, binding: 'old', integration, context: 'old-cluster', source: '/test/old', inFileName: 'old-cluster', availability: 'removed' as const, revision: `old-${integration}` }))) await openSettings(page, 'Connection') - const remaining = page.getByRole('button', { name: 'Remove saved Cost connection for old-cluster', exact: true }) + const remaining = page.getByRole('button', { name: 'Remove saved Cost settings for old-cluster', exact: true }) const node = await remaining.elementHandle() - await page.getByRole('button', { name: 'Remove saved Metrics connection for old-cluster', exact: true }).click() - const confirmation = page.getByRole('dialog').filter({ has: page.getByRole('heading', { name: 'Remove saved connection?', exact: true }) }) + await page.getByRole('button', { name: 'Remove saved Metrics settings for old-cluster', exact: true }).click() + const confirmation = page.getByRole('dialog').filter({ has: page.getByRole('heading', { name: 'Remove saved settings?', exact: true }) }) state.delayApply() - await confirmation.getByRole('button', { name: 'Remove connection', exact: true }).click() + await confirmation.getByRole('button', { name: 'Remove settings', exact: true }).click() await expect.poll(() => state.writes.length).toBe(1) await expect(page.getByRole('button', { name: 'Close settings', exact: true })).toBeDisabled() await expect(confirmation.getByRole('button', { name: 'Cancel', exact: true })).toBeDisabled() @@ -781,7 +781,7 @@ test('cleanup blocks close while committing and keeps remaining cards mounted wi // Hold catalog refreshes so a redundant reload cannot hide the row unnoticed. await page.route('**/api/integrations/connections', async route => { if (route.request().method() === 'GET') return; await route.fallback() }) state.releaseApply() - await expect(page.getByText('Removed the saved Metrics connection for old-cluster.', { exact: true })).toBeVisible() + await expect(page.getByText('Removed the saved Metrics settings for old-cluster.', { exact: true })).toBeVisible() await expect(remaining).toBeVisible() expect(await node!.evaluate(el => el.isConnected)).toBe(true) await expect(page.getByRole('heading', { name: 'Integration settings by cluster', exact: true })).toBeFocused() @@ -795,17 +795,17 @@ test('stale cleanup revisions require reload and keep keyboard focus in the conf ? route.fulfill({ status: 409, json: { error: 'Settings changed; reload latest settings.' } }) : route.fallback()) await openSettings(page, 'Connection') - await page.getByRole('button', { name: 'Remove saved Metrics connection for old-cluster', exact: true }).click() - const confirmation = page.getByRole('dialog').filter({ has: page.getByRole('heading', { name: 'Remove saved connection?', exact: true }) }) - await confirmation.getByRole('button', { name: 'Remove connection', exact: true }).click() + await page.getByRole('button', { name: 'Remove saved Metrics settings for old-cluster', exact: true }).click() + const confirmation = page.getByRole('dialog').filter({ has: page.getByRole('heading', { name: 'Remove saved settings?', exact: true }) }) + await confirmation.getByRole('button', { name: 'Remove settings', exact: true }).click() await expect(confirmation.getByRole('alert')).toBeFocused() await page.keyboard.press('Shift+Tab') await expect.poll(() => confirmation.evaluate(el => el.contains(document.activeElement))).toBe(true) - await expect(confirmation.getByRole('button', { name: 'Remove connection', exact: true })).toBeDisabled() + await expect(confirmation.getByRole('button', { name: 'Remove settings', exact: true })).toBeDisabled() await confirmation.getByRole('button', { name: 'Cancel', exact: true }).click() - await page.getByRole('button', { name: 'Reload saved connections', exact: true }).click() + await page.getByRole('button', { name: 'Reload integration settings', exact: true }).click() await expect(page.getByRole('alert')).toHaveCount(0) - await expect(page.getByRole('button', { name: 'Remove saved Metrics connection for old-cluster', exact: true })).toBeVisible() + await expect(page.getByRole('button', { name: 'Remove saved Metrics settings for old-cluster', exact: true })).toBeVisible() }) for (const management of ['operator', 'cloud']) { @@ -1229,12 +1229,14 @@ test('confirming one changed integration removes it from the sibling review pick state.profiles.cost.state = 'target_changed' await openSettings(page, 'Cost') await page.getByRole('button', { name: 'Review changes', exact: true }).click() + await expect(page.getByText('Metrics needs a separate review in its own tab.', { exact: true })).toBeVisible() await page.getByRole('checkbox', { name: /Cost/ }).check() await page.getByRole('button', { name: 'Keep selected settings for this cluster', exact: true }).click() await expect(page.getByRole('tabpanel').getByText('Saved · Connected', { exact: true })).toBeVisible() await page.getByRole('tab', { name: 'Metrics', exact: true }).click() await page.getByRole('button', { name: 'Review changes', exact: true }).click() await expect(page.getByRole('checkbox', { name: /Cost/ })).toHaveCount(0) + await expect(page.getByText(/different previous cluster/)).toHaveCount(0) await page.getByRole('checkbox', { name: /Metrics/ }).check() await page.getByRole('button', { name: 'Keep selected settings for this cluster', exact: true }).click() await expect.poll(() => state.writes.length).toBe(2) @@ -1512,7 +1514,7 @@ test('Cost reset focuses the source picker, which works from the keyboard', asyn const state = await fixture(page) await openSettings(page, 'Cost') const reset = page.getByRole('button', { name: 'Reset to Automatic', exact: true }) - const source = page.getByRole('button', { name: 'Cost source', exact: true }) + const source = page.getByRole('button', { name: /^Cost source:/ }) await reset.focus() await page.keyboard.press('Enter') await expect(source).toBeFocused() @@ -1523,7 +1525,16 @@ test('Cost reset focuses the source picker, which works from the keyboard', asyn await expect(page.getByRole('option', { name: /^Kubecost/ })).toBeFocused() await page.keyboard.press('Enter') await expect(source).toBeFocused() - await expect(source).toContainText('Kubecost') + await expect(source).toHaveAccessibleName('Cost source: Kubecost') + await page.keyboard.press('Enter') + await expect(page.getByRole('option', { name: /^Kubecost/ })).toBeFocused() + await page.keyboard.press('ArrowUp') + await expect(page.getByRole('option', { name: /^OpenCost metrics/ })).toBeFocused() + await page.keyboard.press('Tab') + await expect(page.getByRole('option', { name: /^Kubecost/ })).not.toBeFocused() + await expect(source).toHaveAttribute('aria-expanded', 'false') + await expect(source).toHaveAccessibleName('Cost source: Kubecost') + await source.focus() await page.keyboard.press('Enter') await expect(page.getByRole('option', { name: /^Kubecost/ })).toBeFocused() await page.keyboard.press('Escape') @@ -1541,6 +1552,7 @@ test('the paused-settings review names only what changed', async ({ page }) => { const identity = state.profiles.metrics.target.identity Object.assign(state.profiles.metrics, { state: 'target_changed', previousIdentity: { ...identity, server: 'https://old.example' } }) Object.assign(state.profiles.argocd, { state: 'target_changed', previousIdentity: { ...identity, trust: 'old-ca', proxy: 'http://proxy.example' } }) + Object.assign(state.profiles.cost, { state: 'target_changed', previousIdentity: { ...identity, server: 'https://old.example' } }) const dialog = await openSettings(page, 'Metrics') await dialog.getByRole('button', { name: 'Review changes', exact: true }).click() const rows = dialog.getByRole('tabpanel').locator('dl') @@ -1548,12 +1560,27 @@ test('the paused-settings review names only what changed', async ({ page }) => { await expect(rows).toContainText('https://old.example') await expect(rows).toContainText('developer (unchanged)') await expect(rows).toContainText('Unchanged') + await expect(dialog.getByRole('checkbox', { name: /Argo CD/ })).toHaveCount(0) + await expect(dialog.getByRole('checkbox', { name: /Cost/ })).toBeVisible() + await expect(dialog.getByText('Argo CD was saved for a different previous cluster, so review it in its own tab.', { exact: true })).toBeVisible() await dialog.getByRole('button', { name: 'Back to Metrics', exact: true }).click() await dialog.getByRole('tab', { name: 'Argo CD', exact: true }).click() await dialog.getByRole('button', { name: 'Review changes', exact: true }).click() await expect(rows).toContainText('https://cluster.example (unchanged)') await expect(rows).not.toContainText('Previous server') await expect(rows).toContainText('CA trust changed · Proxy changed') + await expect(dialog.getByText('Metrics and Cost were saved for a different previous cluster, so review them in their own tabs.', { exact: true })).toBeVisible() +}) + +test('an unrecorded previous identity is reviewed separately without claiming a different cluster', async ({ page }) => { + const state = await fixture(page) + const identity = state.profiles.metrics.target.identity + Object.assign(state.profiles.metrics, { state: 'target_changed', previousIdentity: { ...identity, server: 'https://old.example' } }) + Object.assign(state.profiles.cost, { state: 'target_changed' }) + const dialog = await openSettings(page, 'Metrics') + await dialog.getByRole('button', { name: 'Review changes', exact: true }).click() + await expect(dialog.getByRole('checkbox', { name: /Cost/ })).toHaveCount(0) + await expect(dialog.getByText('Cost needs a separate review in its own tab.', { exact: true })).toBeVisible() }) test('AI investigations saves through the shared row and is guarded on close', async ({ page }) => { diff --git a/web/src/components/settings/LocalConfigurationDetails.tsx b/web/src/components/settings/LocalConfigurationDetails.tsx index f1c48298ff..4dc6d70bb7 100644 --- a/web/src/components/settings/LocalConfigurationDetails.tsx +++ b/web/src/components/settings/LocalConfigurationDetails.tsx @@ -168,7 +168,7 @@ export function SavedClusterConnections({ .then(async (response) => { const data = (await response.json().catch(() => ({}))) as ConnectionResponse if (!response.ok) - throw new Error(data.error || 'Could not load saved connections.') + throw new Error(data.error || 'Could not load integration settings.') if (controller.signal.aborted || getApiBase() !== base) return setUses(data.connections) }) @@ -215,10 +215,10 @@ export function SavedClusterConnections({ const data = (await response.json().catch(() => ({}))) as ConnectionResponse if (controller.signal.aborted || getApiBase() !== base) return if (!response.ok) - throw new Error(data.error || 'Could not remove the saved connection.') + throw new Error(data.error || 'Could not remove the saved settings.') setUses(data.connections) setMessage( - `Removed the saved ${integrationNames[selected.integration]} connection for ${selected.context}.`, + `Removed the saved ${integrationNames[selected.integration]} settings for ${selected.context}.`, ) focusSummary.current = true setSelected(null) @@ -261,12 +261,12 @@ export function SavedClusterConnections({

{loading && (

- Loading saved connections… + Loading integration settings…

)} {!loading && !error && clusters.size === 0 && (

- No saved connections yet. + No integration settings saved yet.

)} {!loading && @@ -300,8 +300,8 @@ export function SavedClusterConnections({

{!current && entry.availability === 'removed' && (

- This kubeconfig entry is gone, but its connections are still - saved. To reuse one, select the new context and choose “Copy + This kubeconfig entry is gone, but its integration settings + are still saved. To reuse one, select the new context and choose “Copy settings from…” in that integration’s settings.

)} @@ -325,7 +325,7 @@ export function SavedClusterConnections({ type="button" disabled={busy} className="text-accent-text hover:underline disabled:opacity-50" - aria-label={`Remove saved ${integrationNames[use.integration]} connection for ${use.context}`} + aria-label={`Remove saved ${integrationNames[use.integration]} settings for ${use.context}`} onClick={() => { setSelected(use) setError('') @@ -353,7 +353,7 @@ export function SavedClusterConnections({ className="underline" onClick={() => setVersion((value) => value + 1)} > - Reload saved connections + Reload integration settings

)} @@ -362,13 +362,13 @@ export function SavedClusterConnections({ open={!!selected} onClose={() => setSelected(null)} onConfirm={() => void remove()} - title="Remove saved connection?" + title="Remove saved settings?" message={ selected - ? `Remove the saved ${integrationNames[selected.integration]} connection and credentials for ${selected.context}? Other saved connections are unchanged. Nothing is deleted from Kubernetes or the backend.${selected.availability === 'unavailable' ? ' This entry may still exist in another kubeconfig; removal also affects any CLI or Desktop using that entry.' : ''}` + ? `Remove the saved ${integrationNames[selected.integration]} settings and credentials for ${selected.context}? Other saved settings are unchanged. Nothing is deleted from Kubernetes or the backend.${selected.availability === 'unavailable' ? ' This entry may still exist in another kubeconfig; removal also affects any CLI or Desktop using that entry.' : ''}` : '' } - confirmLabel="Remove connection" + confirmLabel="Remove settings" isLoading={busy} confirmDisabled={!!error} showWarning={false} @@ -380,7 +380,7 @@ export function SavedClusterConnections({ role="alert" className="text-sm text-warning-text" > - {error} Cancel and reload saved connections before trying again. + {error} Cancel and reload integration settings before trying again.

)} diff --git a/web/src/components/settings/LocalConnectionSettings.tsx b/web/src/components/settings/LocalConnectionSettings.tsx index 3658cfbed8..e650fe4b57 100644 --- a/web/src/components/settings/LocalConnectionSettings.tsx +++ b/web/src/components/settings/LocalConnectionSettings.tsx @@ -126,6 +126,11 @@ const automaticAction: Record = { argocd: 'Use auto-discovery', cost: 'Reset to Automatic' } +function sameIdentity(a?: TargetIdentity, b?: TargetIdentity) { + return !!a && !!b && a.server === b.server && a.user === b.user && + a.tlsName === b.tlsName && a.trust === b.trust && a.proxy === b.proxy && + !!a.insecureTls === !!b.insecureTls +} const names: Record = { metrics: 'Metrics', argocd: 'Argo CD', @@ -180,9 +185,18 @@ export function LocalConnectionSettings({ const confirmationCopy = useRef({ title: '', message: '', label: '' }) const [confirmBack, setConfirmBack] = useState(false) const [accepted, setAccepted] = useState([]) - const acceptedChanges = accepted.filter( + // The review page compares against one previous identity, so it can only + // approve integrations that were paused from that same identity. + const pausedKinds = (Object.keys(snapshot) as IntegrationKind[]).filter( (k) => snapshot[k].state === 'target_changed' ) + const reviewKinds = pausedKinds.filter( + (k) => k === kind || sameIdentity(snapshot[k].previousIdentity, profile.previousIdentity) + ) + const separateReviewKinds = pausedKinds.filter((k) => !reviewKinds.includes(k)) + const separateIdentityKnown = !!profile.previousIdentity && + separateReviewKinds.every((k) => !!snapshot[k].previousIdentity) + const acceptedChanges = accepted.filter((k) => reviewKinds.includes(k)) const request = useRef(null) const catalogRequest = useRef(null) const heading = useRef(null) @@ -220,7 +234,7 @@ export function LocalConnectionSettings({ .then(async (response) => { const data = (await response.json().catch(() => ({}))) as ConnectionResponse if (!response.ok) - throw new Error(data.error || 'Could not load saved connections.') + throw new Error(data.error || 'Could not load other clusters’ settings.') if (!controller.signal.aborted && getApiBase() === base) { setCatalog(data.connections) setCatalogError(false) @@ -836,7 +850,7 @@ export function LocalConnectionSettings({ ) : null} )} -
+
{editor && (kind === 'metrics' ? ( ))}
- {!editor && + {!editor && task === 'main' &&

{feedback?.message}

} - {(Object.keys(snapshot) as IntegrationKind[]) - .filter((k) => snapshot[k].state === 'target_changed') + {reviewKinds .map((k) => (