BasicsStep: Extract hooks, add docs, tests, stories#601
BasicsStep: Extract hooks, add docs, tests, stories#601skoeva wants to merge 2 commits intoAzure:mainfrom
Conversation
Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
skoeva
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors the BasicsStep wizard step by extracting its stateful logic into dedicated hooks (useBasicsStep, useRegisterCluster), and adds unit tests plus Storybook coverage for the resulting behavior.
Changes:
- Introduces
useBasicsStepto encapsulate focus, auto-selection, option mapping, cluster readiness/missing checks, and helper text derivation (with exported pure helpers for isolated testing). - Introduces
useRegisterClusterto encapsulate the async “register AKS cluster” flow and related loading/error/success state. - Adds Vitest unit tests for both hooks and Storybook stories for
BasicsStep.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/aks-desktop/src/components/CreateAKSProject/hooks/useRegisterCluster.ts | New hook encapsulating async AKS cluster registration flow and status state. |
| plugins/aks-desktop/src/components/CreateAKSProject/hooks/useRegisterCluster.test.ts | Unit tests validating register flow behavior and messaging. |
| plugins/aks-desktop/src/components/CreateAKSProject/hooks/useBasicsStep.ts | New hook + pure helpers for Basics-step derived state and callbacks. |
| plugins/aks-desktop/src/components/CreateAKSProject/hooks/useBasicsStep.test.ts | Unit tests for pure helpers and hook-derived behaviors. |
| plugins/aks-desktop/src/components/CreateAKSProject/components/BasicsStep.tsx | Converts BasicsStep and RegisterCluster to presentational components using new hooks. |
| plugins/aks-desktop/src/components/CreateAKSProject/components/BasicsStep.stories.tsx | Adds Storybook stories covering key visual states. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for this.
Can you please address the open review comments when you have time?
Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [success, setSuccess] = useState<string | undefined>(undefined); | ||
|
|
||
| const handleRegister = async () => { | ||
| if (!cluster || !subscription) { |
There was a problem hiding this comment.
handleRegister currently bails out only when cluster or subscription are empty. registerAKSCluster also requires a non-empty resourceGroup, and calling it with '' will fail (and may produce a confusing error). Consider including resourceGroup in the guard (or surface a clear error when it’s missing).
| if (!cluster || !subscription) { | |
| if (!cluster || !resourceGroup || !subscription) { |
| subtitle: `Tenant: ${sub.tenantName} - (${sub.tenant}) • Status: ${sub.status}`, | ||
| })); | ||
|
|
||
| const clusterOptions: SearchableSelectOption[] = clusters.map(cluster => ({ | ||
| value: cluster.name, | ||
| label: cluster.name, | ||
| subtitle: `${cluster.location} • ${cluster.version} • ${cluster.nodeCount} nodes • ${cluster.status}`, |
There was a problem hiding this comment.
subscriptionOptions / clusterOptions subtitles include user-visible English strings (Tenant:, Status:, nodes) that bypass i18n. Since this UI already uses useTranslation(), these labels should be wrapped in t(...) (or otherwise localized) so they appear correctly in non-English locales.
| subtitle: `Tenant: ${sub.tenantName} - (${sub.tenant}) • Status: ${sub.status}`, | |
| })); | |
| const clusterOptions: SearchableSelectOption[] = clusters.map(cluster => ({ | |
| value: cluster.name, | |
| label: cluster.name, | |
| subtitle: `${cluster.location} • ${cluster.version} • ${cluster.nodeCount} nodes • ${cluster.status}`, | |
| subtitle: `${t('Tenant')}: ${sub.tenantName} - (${sub.tenant}) • ${t('Status')}: ${sub.status}`, | |
| })); | |
| const clusterOptions: SearchableSelectOption[] = clusters.map(cluster => ({ | |
| value: cluster.name, | |
| label: cluster.name, | |
| subtitle: `${cluster.location} • ${cluster.version} • ${cluster.nodeCount} ${t('nodes')} • ${cluster.status}`, |
| * Initial state: subscriptions loaded, no subscription or cluster selected yet. | ||
| */ | ||
| export const Default = Template.bind({}); | ||
| Default.args = BASE_PROPS; |
There was a problem hiding this comment.
The JSDoc for the Default story says “no subscription … selected yet”, but BASE_FORM_DATA already sets subscription: 'sub-123', so the rendered state won’t match the description. Either clear the subscription in Default.args or adjust the story comment to match the actual initial state.
| Default.args = BASE_PROPS; | |
| Default.args = { | |
| ...BASE_PROPS, | |
| formData: { | |
| ...BASE_FORM_DATA, | |
| subscription: '', | |
| }, | |
| }; |
These changes extract all logic from
BasicsStepinto dedicateduseBasicsStepanduseRegisterClusterhooks, add comprehensive TSDocs, unit tests, and Storybook stories.Fixes: #95
Summary
useBasicsStepwith a narrowUseBasicsStepInputinterfaceRegisterClusterasync flow intouseRegisterCluster, making the component purely presentationalgetClusterHelperText,isClusterNonReady,getClusterStateMessage) for isolated testingTesting
cd plugins/aks-desktop && npm testand ensure the tests pass