CNV-75174: show source and target Nodes in VMIM details #3318
Conversation
|
@adamviktora: This pull request references CNV-75174 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.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. |
📝 WalkthroughWalkthroughThis PR introduces VirtualMachineInstanceMigrations support by creating a reusable NodeLink component for node references, refactoring the migrations table to use it, and registering two details components as console extensions for displaying source and target node information. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
@adamviktora: This pull request references CNV-75174 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.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: 2
Fix all issues with AI Agents 🤖
In @src/utils/components/NodeLink/NodeLink.tsx:
- Around line 12-22: The NodeLink component lacks an explicit return type and
could be memoized; update the component signature to include an explicit return
type (e.g., NodeLink: FC<NodeLinkProps> or better annotate the function as
returning JSX.Element) return NO_DATA_DASH directly instead of wrapping it in a
fragment when nodeName is falsy, and export the component wrapped with
React.memo to avoid unnecessary re-renders (referencing NodeLink, NodeLinkProps,
and MulticlusterResourceLink/NodeModel for locating the code).
In
@src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsx:
- Around line 9-13: The VMIMSourceNodeDetails component lacks an explicit return
type and isn't memoized; update the function signature to declare a return type
of JSX.Element (e.g., VMIMSourceNodeDetails: FC<...> => JSX.Element) and wrap
the exported component with React.memo to avoid unnecessary re-renders; locate
VMIMSourceNodeDetails (which returns NodeLink using getCluster and
getMigrationSourceNode) and apply the same explicit return type and memoization
to VMIMTargetNodeDetails if present.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugin-extensions.tsplugin-metadata.tssrc/utils/components/NodeLink/NodeLink.tsxsrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxsrc/views/virtualmachineinstancemigrations/extensions.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxfile extension for all React components. One component per file with no nested components.
Use PascalCase for all React component names (e.g.,HeaderTop.tsx)
Use functional components by default. Only use class components for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch)
Use default exports for all React components
UseReact.lazyandSuspensefor lazy loading components
Files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxsrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsx
**/*.{tsx,scss}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use project-based class names on components as anchors for styling rules rather than relying on PatternFly class names
Files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxsrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{tsx,ts}: Extract logic from components into custom hooks or utility files to improve testability and component maintainability
Use React memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. Use an empty array[]if no dependencies are required
Files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxplugin-metadata.tssrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsxplugin-extensions.tssrc/views/virtualmachineinstancemigrations/extensions.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx,js,jsx}: Keep files under 150 lines whenever possible
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized
Keep functions short and focused on one action. Apply Red → Green → Refactor methodology
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability
Files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxplugin-metadata.tssrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsxplugin-extensions.tssrc/views/virtualmachineinstancemigrations/extensions.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Define constants in utility files with uppercase and underscore-separated naming (e.g.,
API_URL)
Files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxplugin-metadata.tssrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsxplugin-extensions.tssrc/views/virtualmachineinstancemigrations/extensions.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript
If a type is exported, add it to a utility file
Avoid usinganytype in TypeScript. Useunknowninstead and narrow the type as needed
Always explicitly define return types for functions rather than relying on TypeScript type inference
Files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxplugin-metadata.tssrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsxplugin-extensions.tssrc/views/virtualmachineinstancemigrations/extensions.ts
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use
.tsfile extension for non-component files containing logic or utilities
Files:
plugin-metadata.tsplugin-extensions.tssrc/views/virtualmachineinstancemigrations/extensions.ts
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsxsrc/utils/components/NodeLink/NodeLink.tsxsrc/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsxsrc/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsx
🧬 Code graph analysis (3)
src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsx (2)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/utils/resources/vmim/selectors.ts (1)
getMigrationSourceNode(6-7)
src/utils/components/NodeLink/NodeLink.tsx (1)
src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
src/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsx (2)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/utils/resources/vmim/selectors.ts (1)
getMigrationTargetNode(9-10)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (5)
plugin-metadata.ts (1)
12-12: LGTM! Consistent integration pattern.The import and spread of
VirtualMachineInstanceMigrationsExposedModulesfollows the established pattern for other module integrations in this file.Also applies to: 31-31
plugin-extensions.ts (1)
12-12: LGTM! Consistent extension registration.The import and spread of
VirtualMachineInstanceMigrationsExtensionsproperly registers the VMIM detail components following the established pattern.Also applies to: 26-26
src/views/clusteroverview/MigrationsTab/components/MigrationsTable/MigrationsRow.tsx (1)
78-83: LGTM! Clean refactoring to reusable component.The refactoring to use
NodeLinkconsolidates node rendering logic and improves code maintainability by removing duplication and delegating the NO_DATA_DASH fallback handling to the shared component.src/views/virtualmachineinstancemigrations/details/VMIMTargetNodeDetails.tsx (1)
1-15: LGTM! Clean and focused implementation.The component correctly implements the VMIM target node detail view following established patterns. The use of selectors with optional chaining and NodeLink's graceful handling of missing data ensures robust rendering.
src/views/virtualmachineinstancemigrations/extensions.ts (1)
1-38: LGTM! Well-structured extension configuration.The extension definitions are properly configured with:
- Correct component references matching the exposedModules keys
- Paths that align with the VMIM resource structure and selectors
- Logical sortWeight ordering (source → target)
- Consistent right-column placement for both detail items
| const NodeLink: FC<NodeLinkProps> = ({ cluster, nodeName }) => { | ||
| if (!nodeName) return <>{NO_DATA_DASH}</>; | ||
|
|
||
| return ( | ||
| <MulticlusterResourceLink | ||
| cluster={cluster} | ||
| groupVersionKind={modelToGroupVersionKind(NodeModel)} | ||
| name={nodeName} | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type and consider memoization.
As per coding guidelines, functions should have explicit return types. Additionally, this pure presentational component would benefit from memoization to prevent unnecessary re-renders.
🔎 Proposed improvements
-const NodeLink: FC<NodeLinkProps> = ({ cluster, nodeName }) => {
- if (!nodeName) return <>{NO_DATA_DASH}</>;
+const NodeLink: FC<NodeLinkProps> = ({ cluster, nodeName }): JSX.Element => {
+ if (!nodeName) return NO_DATA_DASH;
return (
<MulticlusterResourceLink
cluster={cluster}
groupVersionKind={modelToGroupVersionKind(NodeModel)}
name={nodeName}
/>
);
};
-export default NodeLink;
+export default React.memo(NodeLink);Changes:
- Added explicit
: JSX.Elementreturn type - Removed unnecessary fragment wrapper around
NO_DATA_DASH - Wrapped export with
React.memofor performance optimization
Based on coding guidelines, explicit return types are required for all functions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/utils/components/NodeLink/NodeLink.tsx around lines 12-22, The NodeLink
component lacks an explicit return type and could be memoized; update the
component signature to include an explicit return type (e.g., NodeLink:
FC<NodeLinkProps> or better annotate the function as returning JSX.Element)
return NO_DATA_DASH directly instead of wrapping it in a fragment when nodeName
is falsy, and export the component wrapped with React.memo to avoid unnecessary
re-renders (referencing NodeLink, NodeLinkProps, and
MulticlusterResourceLink/NodeModel for locating the code).
| const VMIMSourceNodeDetails: FC<DetailsItemComponentProps<V1VirtualMachineInstanceMigration>> = ({ | ||
| obj: vmim, | ||
| }) => { | ||
| return <NodeLink cluster={getCluster(vmim)} nodeName={getMigrationSourceNode(vmim)} />; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type and consider memoization.
The component should declare an explicit return type per coding guidelines. Additionally, as a simple presentational component, it would benefit from memoization.
🔎 Proposed improvements
const VMIMSourceNodeDetails: FC<DetailsItemComponentProps<V1VirtualMachineInstanceMigration>> = ({
obj: vmim,
-}) => {
+}): JSX.Element => {
return <NodeLink cluster={getCluster(vmim)} nodeName={getMigrationSourceNode(vmim)} />;
};
-export default VMIMSourceNodeDetails;
+export default React.memo(VMIMSourceNodeDetails);Changes:
- Added explicit
: JSX.Elementreturn type - Wrapped export with
React.memofor performance optimization
Based on coding guidelines, explicit return types are required for all functions. The same improvements should be applied to VMIMTargetNodeDetails.tsx if it exists.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/views/virtualmachineinstancemigrations/details/VMIMSourceNodeDetails.tsx
around lines 9-13, The VMIMSourceNodeDetails component lacks an explicit return
type and isn't memoized; update the function signature to declare a return type
of JSX.Element (e.g., VMIMSourceNodeDetails: FC<...> => JSX.Element) and wrap
the exported component with React.memo to avoid unnecessary re-renders; locate
VMIMSourceNodeDetails (which returns NodeLink using getCluster and
getMigrationSourceNode) and apply the same explicit return type and memoization
to VMIMTargetNodeDetails if present.
There was a problem hiding this comment.
@adamviktora looking good - please add the return types as suggested and we can merge this PR.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@galkremer1 I don't think we should add return types for React.FCs, we don't do it anywhere.
coderabbit, please don't suggest this.
There was a problem hiding this comment.
I thought we added it in other parts of the code as well but I agree with you (we just need to make it consistent).
There was a problem hiding this comment.
Oh, you are right, I see 13 results in the codebase.
There was a problem hiding this comment.
Leave it as it is for now - we can discuss it tomorrow in the team meeting - as long as we make it consistent - I am good with both options.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adamviktora, galkremer1 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 8b2f890
into
kubevirt-ui:main
3 participants
📝 Description
🎥 Demo
Before:
After:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.