Skip to content

Settings follow-ups: dropdown keyboard and names, per-identity reviews, auto-discovery in the cluster list - #1966

Merged
nadaverell merged 3 commits into
mainfrom
fix/settings-review-followups
Oct 3, 2026
Merged

nadaverell merged 3 commits into
mainfrom
fix/settings-review-followups

Conversation

@nadaverell

@nadaverell nadaverell commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Four fixes to Settings behavior and one wording fix:

  • Keyboard users can now Tab out of an open dropdown.
  • Screen-reader users hear the dropdown's current value.
  • The "Do these settings still apply?" review can no longer approve an integration based on another integration's comparison.
  • The per-cluster settings list no longer calls an integration saved when it uses auto-discovery.
  • The per-cluster settings list's messages now match its heading.

What changed

Dropdowns (SelectMenu in @skyhook-io/k8s-ui)

  • Tab leaves the list. Before, if you opened Cost source with Kubecost selected and pressed ↑ to reach OpenCost metrics, Tab jumped focus back to Kubecost inside the list. Tab now moves to the next control and closes the menu, and the value is unchanged. The tab stop stays on a rendered option even if the options are replaced while the menu is open.
  • The current value is announced. The trigger's accessible name now includes the selected value: "Cost source: Kubecost". Before, the fixed aria-label replaced the visible value, so screen readers heard only "Cost source", unlike the native select it replaced. Menus showing a placeholder, such as "Copy settings from…", keep their label.
  • Reach. This changes every SelectMenu, in Radar and in other consumers of @skyhook-io/k8s-ui. A test that finds a dropdown by its exact accessible name needs a prefix match once a value is selected.

Paused-settings review

When a kubeconfig context's server, certificate authority, proxy or user changes, each integration saved for that context pauses with the identity it was last accepted for. The review page compares one previous identity, the one for the integration you opened. But it used to list every paused integration for approval.

So if Argo CD had been accepted against a different server than Metrics, the page could show "Server: https://… (unchanged)" from Metrics' comparison while still offering Argo CD for approval.

Now:

  • Only integrations paused from the same previous identity are offered.
  • Others are named instead: "Argo CD was saved for a different previous cluster, so review it in its own tab."
  • When an identity was never recorded, the page makes no claim about it: "Cost needs a separate review in its own tab."

Review page offering Metrics and Cost, which share a previous identity, and sending Argo CD to its own review

Integration settings by cluster

What each integration uses. Each cluster's row now gives every integration's status, for example "Argo CD: saved · Cost: Automatic · Metrics: auto-discovery". Before, a cluster whose Metrics had been switched to auto-discovery, for example by declining paused settings, still read "Saved: Metrics". Switching keeps an empty record for the integration, and the list API returned only the kind.

List entries (GET /api/integrations/connections → connections[]) now include mode and clusterId, as the per-cluster profile already does. An integration reads as saved when its record has an endpoint, a credential, a Kubecost cluster mapping, an explicit cost source, or a validation error; otherwise it reads as auto-discovery, or Automatic for Cost, matching the action that set it.

Wording. The list's loading, empty, error and remove messages now use its heading's name: "No integration settings saved yet.", "Remove saved settings?", "Removed the saved Metrics settings for …". They still said "saved connections".

Spacing

Two empty elements no longer add about 32px of space above the review page's content.

Testing

  • make build; make tsc; the full k8s-ui vitest suite passes, including a new jsdom test that shrinks the options while the menu is open. Settings and cost vitest pass.
  • go test ./internal/connections ./internal/server ./internal/config. The decline test now checks that the list reports the saved Kubecost cluster ID, and auto-discovery with no mapping after declining.
  • npm run test:e2e:settings: 135/135. New or updated coverage:
    • keyboard open, ↑, then Tab leaves the list;
    • the accessible name carries the value;
    • the review page with the same identity, a different identity, and an unrecorded identity (one missing, and both missing);
    • the per-cluster list's new wording;
    • the list's per-integration status: saved credential, saved cluster mapping, invalid record, auto-discovery and Automatic.
  • Live check against a real GKE cluster, with Radar running from an isolated home directory:
    • settings paused by a kubeconfig user change;
    • the review page's rows and spacing;
    • declining to auto-discovery, after which Radar reconnected to the cluster's Prometheus;
    • removing a stale context's settings;
    • the dropdown keyboard path and accessible name.
  • The screenshot above uses fixture data.
  • The list's per-integration status is covered by the Go and e2e tests; it wasn't part of the live check.

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.
@nadaverell
nadaverell requested a review from hisco as a code owner October 3, 2026 21:24
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix dropdown accessibility and scope paused-settings reviews by identity

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Let keyboard users Tab out of open dropdowns and announce selected values to screen readers.
• Offer paused settings for approval only when they share the reviewed previous identity.
• Align per-cluster settings wording, remove extra review spacing, and extend end-to-end coverage.
Diagram

graph TD
  UI["Settings UI"] --> Menu["SelectMenu"]
  UI --> List["Cluster settings"] --> API["Connections API"]
  UI --> Review["Paused review"] --> Match{"Same prior identity?"}
  Match -->|yes| Eligible["Eligible settings"] --> API
  Match -->|no or unknown| Separate["Separate review"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Validate review groups in the API
  • ➕ Enforces the identity boundary for requests outside this UI.
  • ➕ Provides defense in depth if a client submits multiple integration kinds.
  • ➖ Does not by itself explain to users why another integration cannot be approved here.
  • ➖ Requires an API change and additional server-side tests.

Recommendation: The UI-side grouping is appropriate for this PR: it keeps checkboxes consistent with the identity shown and gives users a route to review excluded integrations. Consider complementary API validation if approval must be protected from other clients as well; it would supplement, not replace, the UI change.

Files changed (4) +99 / -52

Enhancement (1) +13 / -13
LocalConfigurationDetails.tsxMatch per-cluster list messages to its integration-settings heading +13/-13

Match per-cluster list messages to its integration-settings heading

• Replaces saved-connections wording in loading, empty, error, removal, and confirmation messages with integration-settings terminology. The removal action and credential warning remain intact.

web/src/components/settings/LocalConfigurationDetails.tsx

Bug fix (2) +31 / -11
SelectMenu.tsxKeep dropdown Tab order on the focused option and announce its value +4/-5

Keep dropdown Tab order on the focused option and announce its value

• Makes the highlighted option the list's Tab stop, so moving focus with an arrow key no longer sends Tab back to the selected option. Includes the selected value in the trigger's accessible name while leaving placeholder menus with their existing label.

packages/k8s-ui/src/components/ui/SelectMenu.tsx

LocalConnectionSettings.tsxRestrict paused-settings approval to a shared previous identity +27/-6

Restrict paused-settings approval to a shared previous identity

• Compares recorded previous identities before offering other paused integrations as approval choices, and names excluded integrations for review in their own tabs without claiming a different cluster when identity is unrecorded. Hides empty review-page elements that added spacing and revises a settings-load fallback message.

web/src/components/settings/LocalConnectionSettings.tsx

Tests (1) +55 / -28
settings-connections.spec.tsCover dropdown accessibility, review eligibility, and revised settings copy +55/-28

Cover dropdown accessibility, review eligibility, and revised settings copy

• Tests Tab behavior and the selected-value accessible name, plus reviews containing matching, different, and unrecorded previous identities. Updates selectors and assertions for the per-cluster settings terminology.

web/e2e/settings-connections.spec.ts

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Changing scan results can skip menu options ✓ Resolved
Description
SelectMenu assigns its only tabbable option from highlightedIndex without adjusting that index
when the options shrink. If a rightsizing scan replaces its results while a user has highlighted an
option beyond the new list’s end, every remaining option gets tabIndex=-1, so Tab skips the list.
Code

packages/k8s-ui/src/components/ui/SelectMenu.tsx[287]

+                  tabIndex={index === highlightedIndex ? 0 : -1}
Evidence
The changed expression gives no option a tab stop when highlightedIndex exceeds the rendered option
count. The rightsizing namespace and kind menus build their options from scan rows, and the scan
hook replaces the displayed result as new scan data arrives; unlike the searchable menu, these menus
previously kept the selected option or first option tabbable.

packages/k8s-ui/src/components/ui/SelectMenu.tsx[39-57]
packages/k8s-ui/src/components/ui/SelectMenu.tsx[283-290]
web/src/components/rightsizing/RightsizingScanView.tsx[189-190]
web/src/components/rightsizing/RightsizingScanView.tsx[731-750]
web/src/api/client.ts[3840-3849]
web/src/api/client.ts[3881-3887]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A non-searchable SelectMenu can lose its only tabbable option when its options shrink while open and highlightedIndex is out of range.
## Fix Focus Areas
- packages/k8s-ui/src/components/ui/SelectMenu.tsx[51-74]
- packages/k8s-ui/src/components/ui/SelectMenu.tsx[283-290]
## Recommended Fix
Clamp or reset the roving tab-stop index when the rendered options change, ensuring an available option remains tabbable. Preserve the intended behavior where Tab leaves the list after arrow-key navigation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/k8s-ui/src/components/ui/SelectMenu.tsx Outdated
The roving tab stop follows the highlighted index, which can point past the
end of the list if the options are replaced while the menu is open. Clamp it
to the rendered options so Tab still enters the list.
Switching an integration to auto-discovery keeps an empty record, and the list
API returned only the kind, so the row still said "Saved: Metrics". List
entries now carry mode and cluster ID, like the per-cluster profile, and each
integration reads as saved, auto-discovery, or Automatic for Cost.
@nadaverell nadaverell changed the title Fix dropdown Tab order and names, and keep paused-settings reviews per identity Settings follow-ups: dropdown keyboard and names, per-identity reviews, auto-discovery in the cluster list Oct 3, 2026
@nadaverell
nadaverell merged commit c7519c4 into main Oct 3, 2026
10 checks passed
@nadaverell
nadaverell deleted the fix/settings-review-followups branch October 3, 2026 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant