[scanner] fix: nightly unit-test regression#21318
Conversation
Both components initialised their cluster state to '' which caused: - CreateNamespaceModal: select showed the disabled placeholder instead of the first cluster, making the Create button stay disabled and POST never being fired. - CanIChecker: an extra disabled placeholder option was included in the cluster <select> (3 options vs the expected 2), and the selected value was '' instead of the first available cluster. Fix CreateNamespaceModal: initialise useState with clusters[0] ?? ''. Fix CanIChecker: add useEffect that dispatches SET_FIELD for 'cluster' when clusters load and no cluster is currently selected; remove the now-redundant placeholder <option> from the cluster dropdown. Fixes #21083 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <scanner@kubestellar.io>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
✅ Test Coverage CheckAll new source files in this PR have corresponding test files. Checked |
♿ Accessibility Audit (WCAG 2.1 AA)✅ No WCAG 2.1 AA violations detected in audited routes. Powered by axe-core. Target: WCAG 2.1 AA compliance. |
There was a problem hiding this comment.
Pull request overview
Fixes a nightly unit-test regression caused by <select> controls initializing cluster to an empty string (""), which left UIs in an invalid/disabled state and caused tests to assert unexpected default values/options.
Changes:
CreateNamespaceModal: initializeclusterstate to the first available cluster (clusters[0] ?? '') so the Create flow is enabled by default.CanIChecker: auto-select the first cluster viauseEffectwhen clusters load and none is selected, and remove the disabled placeholder option that was skewing option counts and default values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/src/components/rbac/CanIChecker.tsx | Auto-selects the first cluster on load and removes the placeholder option to stabilize default selection and option counts in tests/UI. |
| web/src/components/namespaces/CreateNamespaceModal.tsx | Defaults the modal’s cluster state to the first provided cluster so Create isn’t blocked by an empty selection. |
| // Get selected cluster for namespace fetching — only after the user makes an explicit choice | ||
| const selectedCluster = cluster | ||
| const { namespaces } = useNamespaces(selectedCluster) |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
clubanderson
left a comment
There was a problem hiding this comment.
[quality] 🚨 Conflicts with sibling PR #21299 — pick one strategy (comment mode; self-approve blocked)
Correct component-level fix — better strategy than the sibling #21299 which patched tests to accept a placeholder-default UI. Preferring this direction because "user must explicitly select an obvious default" is a UX regression, and the fix at source removes 22 test scaffolds too.
🚨 Merge-order conflict with sibling PR #21299
#21299 changes tests to expect .value === '' and renames test to 'initializes with cluster placeholder selected'. This PR changes the components to auto-select clusters[0], so .value === 'cluster-1'. Merging both breaks whichever tests match the not-current component state. Concretely:
- If this PR merges first, #21299's renamed test
starts with no cluster selectedwill fail (.toBe('')vs actual'cluster-a'). - If #21299 merges first, this PR's component change will trigger the same 22 failures they were trying to fix.
Recommendation: close #21299 (or reduce it to just the NamespaceAccessPanel.tsx file — the CreateNamespaceModal and CanIChecker test changes are obsoleted by this PR) and merge this one.
Fix correctness
CreateNamespaceModal — useState(clusters[0] ?? '') is correct. Two nits:
-
clustersprop can change (parent may re-fetch and pass a new list). SinceuseStateinitializer only runs on mount, if the modal is opened before clusters load (clusters === []), state stays''and never picks up when clusters arrive. Fix: use the sameuseEffectpattern as CanIChecker, or memo the initial value more carefully:const [cluster, setCluster] = useState('') useEffect(() => { if (cluster === '' && clusters.length > 0) setCluster(clusters[0]) }, [clusters, cluster])
The current implementation is fine if
clustersis always non-empty on mount, but the type signature (clusters: string[]) allows empty. -
The disabled placeholder
<option value="" disabled>Select a cluster</option>is still in the CreateNamespaceModal<select>(visible in the current source). WithuseState(clusters[0])the placeholder becomes unreachable — user can never re-select "no cluster" once they've chosen. That's fine for a create-form. But: ifclusters === []on mount, the placeholder is the ONLY option, and the value stays'', and the button stays disabled. Correct — but consider showing an explicit "no clusters available" empty state instead.
CanIChecker — useEffect approach handles the async-load case. Correct. One nit:
- Race between the auto-select effect and any URL-driven default (if there's ever a
?cluster=cluster-bquery param, the effect will overwrite it as long ascluster === ''). Not currently a use case, but worth documenting.
Missing coverage
- No test locks the new "auto-select first cluster on load" behavior. Two tests would suffice:
it('auto-selects first cluster when clusters load asynchronously', async () => { mockClusters = [] const { rerender } = render(<CanIChecker />) expect((screen.getByTestId('can-i-cluster') as HTMLSelectElement).value).toBe('') mockClusters = [{ name: 'cluster-a' }, { name: 'cluster-b' }] rerender(<CanIChecker />) await waitFor(() => expect((screen.getByTestId('can-i-cluster') as HTMLSelectElement).value).toBe('cluster-a')) }) it('CreateNamespaceModal pre-selects first cluster when clusters prop is non-empty', () => { render(<CreateNamespaceModal clusters={['cluster-x','cluster-y']} onClose={vi.fn()} onCreated={vi.fn()} />) const combos = screen.getAllByRole('combobox') expect((combos[0] as HTMLSelectElement).value).toBe('cluster-x') })
Bead filed. Not merging.
Filed by quality agent (ACMM L4/L6 — full mode)
⏹️ Post-Merge Verification: cancelledCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
) * 🌱 fix: update CanIChecker tests for auto-select cluster behavior PR #21318 changed CanIChecker to auto-select the first cluster and removed the disabled placeholder option, but did not update the unit tests. Since then build-gate fails on every PR with: - 'populates cluster dropdown with provided clusters' expecting 3 options including a '' placeholder (now 2 options) - 'starts with no cluster selected' expecting '' (now cluster-a) Align both tests with the shipped behavior. Signed-off-by: Andrew Anderson <andy@clubanderson.com> * 🌱 fix: drop unused error binding in EnterpriseLayout catch The lint-baseline gate flags 'error' at EnterpriseLayout.tsx:182 as a new @typescript-eslint/no-unused-vars violation, failing build-gate on every PR. Use a bare catch since all localStorage errors are treated the same way. Signed-off-by: Andrew Anderson <andy@clubanderson.com> --------- Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…ster behavior PR #21318 changed CreateNamespaceModal to auto-select the first cluster (clusters[0]) instead of the empty placeholder. The test still asserted the old placeholder behavior, causing shard 3 failures in Coverage Suite on main. Rename `initializes with cluster placeholder selected` → `auto-selects first cluster on initialization`, and assert the combobox value is 'cluster-1' instead of ''. Test-only change; no component code touched. Signed-off-by: clubanderson <407614+clubanderson@users.noreply.github.com>
…ster behavior (#21354) PR #21318 changed CreateNamespaceModal to auto-select the first cluster (clusters[0]) instead of the empty placeholder. The test still asserted the old placeholder behavior, causing shard 3 failures in Coverage Suite on main. Rename `initializes with cluster placeholder selected` → `auto-selects first cluster on initialization`, and assert the combobox value is 'cluster-1' instead of ''. Test-only change; no component code touched. Signed-off-by: clubanderson <407614+clubanderson@users.noreply.github.com>
Fixes #21083
Root cause
Two components initialised their cluster
<select>state to"":CreateNamespaceModal: cluster state stayed""so the Create button stayed disabled andagentFetchwas never called → 4 tests failed.CanIChecker:INITIAL_FORM_STATEhadcluster: ""and rendered a disabled placeholder<option>, giving 3 options instead of the expected 2, andclusterSelect.valuewas""instead of the first cluster → 18 tests failed.Fix
CreateNamespaceModal: initialiseuseStatewithclusters[0] ?? ""so the first cluster is pre-selected on mount.CanIChecker: add auseEffectthat dispatchesSET_FIELD/clusterto the first available cluster when clusters load and none is selected yet; remove the now-redundant disabled placeholder<option>from the cluster dropdown.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com