MGMT-21584: Disable disconnected switch in cluster edit#3129
Conversation
WalkthroughReplaced inline PatternFly Switches with a new shared InstallDisconnectedSwitch component; updated ClusterDetailsForm and BasicStep to use it. ClusterDetailsForm passes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as ClusterDetailsForm / BasicStep
participant Switch as InstallDisconnectedSwitch
participant Context as ClusterWizardContext
User->>Form: Open form
Form->>Switch: Render(isDisabled = !!cluster?)
Switch->>Context: read installDisconnected
alt isDisabled == true
Note right of Switch #f9f0c1: Wrapped in tooltip — non-editable
User->>Switch: Attempt toggle
Switch-->>User: No-op (disabled)
else isDisabled == false
Note right of Switch #e6f4ea: Active switch — editable
User->>Switch: Toggle
Switch->>Context: setInstallDisconnected(newValue)
Context-->>Form: state updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
the
approved
|
/cherry-pick releases/v2.44 |
|
@rawagner: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
size/XS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (1)
169-176: Initialize installDisconnected from cluster when editingCurrently, in ClusterWizardContextProvider.tsx the disconnected‐installer switch is always initialized to
false(useState(false)), so when editing an existing cluster its “disconnected” flag never reflects the cluster’s real state:• File: libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
Lines ~94–95const [installDisconnected, setInstallDisconnected] = React.useState(false);• Because the switch is disabled (
isDisabled={!!cluster}) but always “off” on edit, users will see an incorrect frozen value if the cluster was created in disconnected mode.Mandatory fix: derive the initial value from the
clusterprop (or fromlocation.state) instead of hard-codingfalse. For example:const locationState = location.state as ClusterWizardFlowStateType|undefined; const initialDisconnected = locationState?.installDisconnected ?? Boolean(cluster?.spec?.proxy?.httpProxy || cluster?.spec?.proxy?.httpsProxy); const [installDisconnected, setInstallDisconnected] = React.useState(initialDisconnected);Or, if you prefer a
useEffect, setinstallDisconnectedon mount whenclusteris defined. This ensures that on edit the switch’s checked state accurately mirrors the cluster’s disconnected setting.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (2)
169-176: Also disable in viewer mode for consistent read-only behaviorIn viewer mode, users shouldn't be able to toggle the switch even in create flow. Consider extending the guard to include isViewerMode.
Apply this minimal diff:
- isDisabled={!!cluster} + isDisabled={!!cluster || isViewerMode}
169-176: Add a tooltip explaining why the switch is disabled (clarity/UX)A short explanation improves UX and reduces confusion when the control is non-interactive. Disabled elements don’t emit pointer events, so wrap the Switch in a span inside a Tooltip.
Apply this diff to the switch block:
- <Switch - id="disconnected-install-switch" - label="I'm installing on a disconnected/air-gapped/secured environment" - isChecked={installDisconnected} - onChange={(_, checked) => setInstallDisconnected(checked)} - ouiaId="DisconnectedInstall" - isDisabled={!!cluster} - /> + {(!!cluster || isViewerMode) ? ( + <Tooltip content="This setting can't be changed after cluster creation or in viewer mode"> + <span> + <Switch + id="disconnected-install-switch" + label="I'm installing on a disconnected/air-gapped/secured environment" + isChecked={installDisconnected} + onChange={(_, checked) => setInstallDisconnected(checked)} + ouiaId="DisconnectedInstall" + isDisabled + /> + </span> + </Tooltip> + ) : ( + <Switch + id="disconnected-install-switch" + label="I'm installing on a disconnected/air-gapped/secured environment" + isChecked={installDisconnected} + onChange={(_, checked) => setInstallDisconnected(checked)} + ouiaId="DisconnectedInstall" + /> + )}And add the Tooltip import at the top of the file:
// at the imports line where Switch is imported import { Grid, GridItem, Switch, Tooltip } from '@patternfly/react-core';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx(1 hunks)
🔇 Additional comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (1)
169-176: Disabling the switch on edit matches the PR goalAdding isDisabled={!!cluster} cleanly prevents post-create toggling of the disconnected setting. This aligns with the intent and keeps the UI state coherent.
rawagner
changed the title
|
@rawagner: This pull request references MGMT-21584 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
rawagner
force-pushed
the
disable_edit_above
branch
from
5654d30 to
2f29d74
Compare
openshift-ci
bot
added
size/M
|
@rawagner: This pull request references MGMT-21584 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx (1)
5-5: Minor: Extract props to a named type for reuse and clarityOptional polish that improves readability and discoverability via IDEs.
-const InstallDisconnectedSwitch = ({ isDisabled }: { isDisabled?: boolean }) => { +type InstallDisconnectedSwitchProps = { isDisabled?: boolean }; +const InstallDisconnectedSwitch = ({ isDisabled }: InstallDisconnectedSwitchProps) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: format
🔇 Additional comments (6)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (3)
26-26: Good consolidation by introducing InstallDisconnectedSwitchReplacing inline Switch usage with a shared component improves consistency and reduces duplication across steps. No behavioral regressions expected in this file.
30-30: Context destructuring trimmed correctlyOnly
moveNextis needed here after delegating the toggle logic to the shared component. Nice cleanup.
58-58: Using the shared switch here is appropriateCreation flow keeps the toggle enabled (no
isDisabled), matching the PR intent to only block edits post-creation.libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (3)
35-35: Importing and reusing InstallDisconnectedSwitch aligns UI/UX across stepsCentralizing the toggle helps keep labeling, OUIA IDs, and behavior consistent.
66-66: Correct removal of local installDisconnected wiringWith the new component owning state via context, keeping only
customManifestsStepandmoveNextis the right call.
169-170: Disable edits for existing clusters — verification completeAfter running the provided searches, there are no stray toggles or direct state mutations outside of the new shared component and its context:
- All references to
DisconnectedInstallandair-gappedappear only in
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx.- The only calls to
setInstallDisconnectedand reads ofinstallDisconnectedare within:
•InstallDisconnectedSwitch.tsx
•ClusterWizardContextProvider.tsx
•ClusterWizardContext.tsx- No legacy
<Switch>instances or direct calls tosetInstallDisconnectedexist elsewhere.The
isDisabled={!!cluster}prop correctly prevents edits on existing clusters, fulfilling the PR objective. All clear to merge.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx
Show resolved
Hide resolved
rawagner
force-pushed
the
disable_edit_above
branch
from
2f29d74 to
cda8b64
Compare
|
@rawagner: This pull request references MGMT-21584 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx (1)
21-26: Wrap disabled Switch to ensure Tooltip actually triggers (and is keyboard reachable)PF Tooltips don’t fire on disabled controls. Wrap the switch in a focusable span when disabled so hover/focus can trigger the tooltip. Add tabIndex=0 for keyboard users.
Apply:
- {switchBtn} + <span tabIndex={0} style={{ display: 'inline-block' }}>{switchBtn}</span>
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx (3)
23-23: Unify copy: avoid “draft cluster” and use the standard messageCurrent text may confuse users editing an existing cluster. Suggest aligning with the broader UI copy (“cannot be changed after the cluster is created”).
- content={<div>{t('ai:This option is not editable after the draft cluster is created')}</div>} + content={<div>{t('ai:This option cannot be changed after the cluster is created')}</div>}
6-6: Make id overridable to avoid duplicate DOM ids if rendered in multiple placesThis component may appear in more than one view; a fixed id risks duplication. Allow an optional id prop with a sensible default.
-const InstallDisconnectedSwitch = ({ isDisabled }: { isDisabled?: boolean }) => { +interface InstallDisconnectedSwitchProps { + isDisabled?: boolean; + id?: string; +} +const InstallDisconnectedSwitch = ({ isDisabled, id = 'disconnected-install-switch' }: InstallDisconnectedSwitchProps) => {- id="disconnected-install-switch" + id={id}Also applies to: 12-12
21-29: Optional: add a lightweight RTL test to lock behaviorConsider a unit test asserting:
- when isDisabled=true, the switch is disabled and wrapped; tooltip appears on focus/hover
- when isDisabled=false, no wrapper/tooltip
I can scaffold a jest/react-testing-library test if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
libs/locales/lib/en/translation.json(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx (2)
libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests
- GitHub Check: translation-files
- GitHub Check: lint
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: format
🔇 Additional comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx (2)
6-9: Nice extraction + state wiring looks clean
- Pulling the control into a focused component with context-backed state and i18n label is tidy.
- PF Switch onChange signature is used correctly.
Also applies to: 10-19
21-24: Translation Key VerifiedThe tooltip translation key
ai:This option is not editable after the draft cluster is createdis already present in all locale files:
- libs/locales/lib/en/translation.json (line 845)
- libs/locales/lib/es/translation.json (line 808)
- libs/locales/lib/fr/translation.json (line 808)
- libs/locales/lib/ja/translation.json (line 808)
- libs/locales/lib/ko/translation.json (line 808)
- libs/locales/lib/zh/translation.json (line 808)
No further action is required.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/InstallDisconnectedSwitch.tsx
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: celdrake, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit cfbbda4
into
openshift-assisted:master
|
@rawagner: new pull request created: #3130 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Summary by CodeRabbit
Bug Fixes
New Features