MGMT-22463: Mass hostname change preview is not correctly aligned#3344
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgyselov: This pull request references MGMT-22463 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 sub-task to target the "4.22.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
bot
added
the
approved
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
openshift-ci
bot
added
the
size/M
|
@jgyselov: This pull request references MGMT-22463 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 sub-task to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx (1)
215-217: Consider adding a fallback for empty oldHostname.Similar to the fallback provided for
newHostnameat line 246, consider adding a fallback text whenhost.oldHostnameis empty or undefined to improve the user experience.🔎 Proposed refactor with fallback
{newHostnames.map((host, i) => ( <div className="hostname-column__text" key={`old-hostname-${i}`}> - <strong>{host.oldHostname}</strong> + <strong>{host.oldHostname || t('ai:Unknown hostname')}</strong> </div> ))}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
🧬 Code graph analysis (1)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx (1)
libs/ui-lib/lib/common/components/hosts/utils.ts (1)
getHostname(185-186)
⏰ 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: circular-deps
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: tests
🔇 Additional comments (2)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx (2)
6-6: LGTM: Content import added for semantic markup.The
Contentcomponent is appropriately used at line 245 to wrap the new hostname display.
50-68: LGTM: oldHostname added to support before/after comparison.The addition of
oldHostnameto the data structure enables the three-column preview layout. The logic correctly setsnewHostnametoundefinedand captures thereasonwhen hostname changes are not allowed.
jgyselov
force-pushed
the
pf6_cim_mass_hostname_change_alignment
branch
from
4fcec7d to
6c9a3e7
Compare
|
@jgyselov: This pull request references MGMT-22463 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 sub-task to target the "4.22.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. |
jgyselov
force-pushed
the
pf6_cim_mass_hostname_change_alignment
branch
from
6c9a3e7 to
f077a3f
Compare
|
@jgyselov: This pull request references MGMT-22463 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 sub-task to target the "4.22.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: 0
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx (1)
212-252: Well-structured three-column layout for the hostname preview.The Split-based layout with old hostname, divider, and new hostname columns is a clean solution for the alignment issue. A few minor observations:
Keys: Using array index as keys works here since the list is derived from
selectedHostswhich is stable during rendering. For better stability, consider including host IDs innewHostnamesand using them as keys.Accessibility for dividers: The
>character serves as a visual separator. Consider addingaria-hidden="true"to prevent screen readers from announcing it, since the layout semantics should be conveyed by the column structure.Optional: Improve accessibility for dividers
<SplitItem> {newHostnames.map((_, i) => ( - <div key={`divider-${i}`}> + <div key={`divider-${i}`} aria-hidden="true"> <strong>{'>'}</strong> </div> ))} </SplitItem>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
🧬 Code graph analysis (1)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx (1)
libs/ui-lib/lib/common/components/hosts/utils.ts (1)
getHostname(185-186)
🔇 Additional comments (2)
libs/ui-lib/lib/common/components/hosts/MassChangeHostnameModal.tsx (2)
2-19: LGTM!Import changes are appropriate. The
Contentcomponent is properly imported and used in the preview section for displaying hostname text.
56-67: LGTM!The addition of
oldHostnameto the returned object is necessary for displaying the old-to-new hostname mapping in the preview.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, LiorSoffer 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 cb7d646
into
openshift-assisted:master
3 participants
https://issues.redhat.com/browse/MGMT-22463
Before:
After:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.