CNV-TODO: Fix useGetModifyApplicationAction API usage #3252
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vojtechszocs 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 |
📝 WalkthroughWalkthroughUpdated three OpenShift dynamic plugin SDK devDependencies in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(1 hunks)src/views/topology/hooks/useModifyApplicationActionProvider.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsfile extension for non-component files containing logic or utilities.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
If a TypeScript type is exported, add it to a utility file.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{tsx,ts}: Extract as much logic as possible from components into custom hooks or utility files to improve testability. Avoid bloated components.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
Avoid usinganytype in TypeScript as it compromises type safety. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions in TypeScript rather than relying on type inference.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
🧬 Code graph analysis (1)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (2)
src/utils/utils/typeGuards.ts (1)
isVM(5-6)src/views/topology/utils/utils.ts (1)
isVMType(13-13)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: i18n
🔇 Additional comments (1)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (1)
14-23: Verify the logic for VM elements returning undefined resource.When
isVMis true (indicating a VM element), theresourceis set toundefined, yetuseGetModifyApplicationActionis always called withVirtualMachineModel. This seems counterintuitive—if the element is a VM, shouldn't the VM resource be passed to the hook instead of undefined?Confirm whether this is the intended behavior by examining the
useGetModifyApplicationActionhook implementation and its usage patterns elsewhere in the codebase to determine if passingundefinedas a resource withVirtualMachineModelis a valid and intentional design pattern.
vojtechszocs
force-pushed
the
bump-sdk-deps
branch
from
27fb21b to
a925d36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (1)
12-12: Variable naming collision persists.The local variable
isVMcontinues to shadow the imported utility functionisVMfromsrc/utils/utils/typeGuards.ts, creating confusion and violating the coding guideline to use descriptive names. Rename toisVirtualMachineElementorisVirtualMachineas previously suggested.Based on coding guidelines, use descriptive names and avoid shadowing imported utilities.
Apply this diff:
- const isVM = useMemo(() => isVMType(element.getType()), [element]); + const isVirtualMachineElement = useMemo(() => isVMType(element.getType()), [element]);And update subsequent usages:
const resource = useMemo<K8sResourceCommon>( - () => (isVM ? undefined : element.getData()?.resources?.obj), - [element, isVM], + () => (isVirtualMachineElement ? undefined : element.getData()?.resources?.obj), + [element, isVirtualMachineElement], ); - const actions = useMemo(() => isVM ? [] : [startAction], [isVM, startAction]); + const actions = useMemo(() => isVirtualMachineElement ? [] : [startAction], [isVirtualMachineElement, startAction]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(1 hunks)src/views/topology/hooks/useModifyApplicationActionProvider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsfile extension for non-component files containing logic or utilities.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
If a TypeScript type is exported, add it to a utility file.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{tsx,ts}: Extract as much logic as possible from components into custom hooks or utility files to improve testability. Avoid bloated components.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
Avoid usinganytype in TypeScript as it compromises type safety. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions in TypeScript rather than relying on type inference.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
🧠 Learnings (3)
📚 Learning: 2025-12-04T14:08:48.236Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-04T14:08:48.236Z
Learning: Applies to **/*.{ts,tsx} : Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Applied to files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
📚 Learning: 2025-12-04T14:08:48.236Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-04T14:08:48.236Z
Learning: Applies to **/*.tsx : Always specify dependencies in `useEffect` to avoid unnecessary re-renders or missed updates. Pass an empty array `[]` to run the effect only once if no dependencies are required.
Applied to files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
📚 Learning: 2025-12-04T14:08:48.236Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-04T14:08:48.236Z
Learning: Applies to **/*.tsx : Use React's memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders. Lazy load components with `React.lazy` and `Suspense`.
Applied to files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
🧬 Code graph analysis (1)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (2)
src/utils/utils/typeGuards.ts (1)
isVM(5-6)src/views/topology/utils/utils.ts (1)
isVMType(13-13)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: unit-test
- GitHub Check: i18n
🔇 Additional comments (2)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (2)
14-17: Resource memoization logic looks correct.The conditional resource determination appropriately returns
undefinedfor VM elements and extracts the resource object for non-VM elements. The memoization dependencies correctly include bothelementandisVM.
27-27: Return statement is correctly memoized.The return statement properly memoizes the result tuple with
actionsas a dependency. However, ensure that line 25's undefined handling is addressed so thatactionsalways contains valid Action objects.
vojtechszocs
force-pushed
the
bump-sdk-deps
branch
from
a925d36 to
4d11d43
Compare
|
|
||
| return useMemo(() => { | ||
| if (!actions) return [[], true, undefined]; | ||
| const startAction = useGetModifyApplicationAction( |
There was a problem hiding this comment.
@pcbailey @adamviktora We're calling Console useGetModifyApplicationAction React hook here, but the resource may be undefined - we should test whether this causes any problems at runtime.
vojtechszocs
force-pushed
the
bump-sdk-deps
branch
from
4d11d43 to
51e8bd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (2)
19-23: Acknowledge author's concern about undefined resource.The author (vojtechszocs) has already flagged the concern about passing
undefinedresource touseGetModifyApplicationActionin the comments. This needs runtime testing to ensure it doesn't cause issues.
25-25: Guard against undefined startAction in actions array.When
!isVMElementandstartActionisundefined, the code produces[undefined], violating theAction[]type contract. This could cause runtime errors for consumers expecting only valid Action objects.Apply this diff to filter out undefined:
- const actions = useMemo(() => (isVMElement ? [] : [startAction]), [isVMElement, startAction]); + const actions = useMemo( + () => (isVMElement || !startAction ? [] : [startAction]), + [isVMElement, startAction], + );Alternatively, use a filter approach:
- const actions = useMemo(() => (isVMElement ? [] : [startAction]), [isVMElement, startAction]); + const actions = useMemo( + () => (isVMElement ? [] : [startAction].filter(Boolean)), + [isVMElement, startAction], + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(1 hunks)src/views/topology/hooks/useModifyApplicationActionProvider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsfile extension for non-component files containing logic or utilities.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
If a TypeScript type is exported, add it to a utility file.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{tsx,ts}: Extract as much logic as possible from components into custom hooks or utility files to improve testability. Avoid bloated components.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
Avoid usinganytype in TypeScript as it compromises type safety. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions in TypeScript rather than relying on type inference.
Files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
🧠 Learnings (3)
📚 Learning: 2025-12-04T14:08:48.236Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-04T14:08:48.236Z
Learning: Applies to **/*.{ts,tsx} : Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Applied to files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
📚 Learning: 2025-12-04T14:08:48.236Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-04T14:08:48.236Z
Learning: Applies to **/*.tsx : Always specify dependencies in `useEffect` to avoid unnecessary re-renders or missed updates. Pass an empty array `[]` to run the effect only once if no dependencies are required.
Applied to files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
📚 Learning: 2025-12-04T14:08:48.236Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-04T14:08:48.236Z
Learning: Applies to **/*.tsx : Use React's memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders. Lazy load components with `React.lazy` and `Suspense`.
Applied to files:
src/views/topology/hooks/useModifyApplicationActionProvider.ts
🧬 Code graph analysis (1)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (1)
src/views/topology/utils/utils.ts (1)
isVMType(13-13)
⏰ 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). (3)
- GitHub Check: i18n
- GitHub Check: unit-test
- GitHub Check: build
🔇 Additional comments (3)
src/views/topology/hooks/useModifyApplicationActionProvider.ts (3)
12-12: LGTM!The VM element detection is correctly implemented with proper memoization and descriptive naming that avoids the previous shadowing issue.
27-27: LGTM!The return statement correctly memoizes the result tuple with appropriate dependencies to avoid unnecessary re-renders.
Based on learnings, React's memoization tools like
useMemoshould be used to avoid unnecessary re-renders.
5-5: Verify thatuseGetModifyApplicationActionhook safely handles undefined resource values.The import changed from the
getModifyApplicationActionfunction to theuseGetModifyApplicationActionhook. Sinceresourcecan beundefinedfor VM elements, confirm that this hook properly accepts and handlesundefinedas the second parameter. This is particularly important given the hook is from the internal SDK package and its parameter handling cannot be verified against public documentation.
| const resource = useMemo<K8sResourceCommon>( | ||
| () => (isVMElement ? undefined : element.getData()?.resources?.obj), | ||
| [element, isVMElement], | ||
| ); |
There was a problem hiding this comment.
Fix incorrect type annotation for resource.
The type annotation declares K8sResourceCommon, but the function returns undefined for VM elements (line 15). This violates TypeScript type safety and could lead to type errors.
Apply this diff to correct the type:
- const resource = useMemo<K8sResourceCommon>(
+ const resource = useMemo<K8sResourceCommon | undefined>(
() => (isVMElement ? undefined : element.getData()?.resources?.obj),
[element, isVMElement],
);Based on coding guidelines, always explicitly define return types for functions in TypeScript.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resource = useMemo<K8sResourceCommon>( | |
| () => (isVMElement ? undefined : element.getData()?.resources?.obj), | |
| [element, isVMElement], | |
| ); | |
| const resource = useMemo<K8sResourceCommon | undefined>( | |
| () => (isVMElement ? undefined : element.getData()?.resources?.obj), | |
| [element, isVMElement], | |
| ); |
🤖 Prompt for AI Agents
In src/views/topology/hooks/useModifyApplicationActionProvider.ts around lines
14 to 17, the resource variable is annotated as K8sResourceCommon but can be
undefined for VM elements; change the type to K8sResourceCommon | undefined and
update the useMemo generic to useMemo<K8sResourceCommon | undefined>(...) so the
return type is explicit and matches the actual possible undefined value; also
ensure any related function signatures in this file explicitly declare their
return types per the coding guidelines.
| const startAction = useGetModifyApplicationAction( | ||
| VirtualMachineModel, | ||
| resource, | ||
| 'vm-action-start', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit type annotation for startAction.
Per coding guidelines, always explicitly define return types for variables rather than relying on type inference. Add a type annotation for startAction.
Apply this diff:
- const startAction = useGetModifyApplicationAction(
+ const startAction: Action | undefined = useGetModifyApplicationAction(
VirtualMachineModel,
resource,
'vm-action-start',
);Based on coding guidelines, always explicitly define return types for functions in TypeScript.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startAction = useGetModifyApplicationAction( | |
| VirtualMachineModel, | |
| resource, | |
| 'vm-action-start', | |
| ); | |
| const startAction: Action | undefined = useGetModifyApplicationAction( | |
| VirtualMachineModel, | |
| resource, | |
| 'vm-action-start', | |
| ); |
🤖 Prompt for AI Agents
In src/views/topology/hooks/useModifyApplicationActionProvider.ts around lines
19 to 23, add an explicit type annotation for startAction; change the
declaration to include a concrete return type such as: const startAction:
ReturnType<typeof useGetModifyApplicationAction> =
useGetModifyApplicationAction(VirtualMachineModel, resource, 'vm-action-start');
this ensures the variable's type is explicit per coding guidelines (also audit
nearby function declarations and add explicit return types where missing).
@adamviktora Thanks, there seems to be an overlap between #3242 and #3249 in terms of updated plugin SDK dependencies and addressing Console The reason for targetting 4.21 prerelease for all plugin SDK packages in this PR was due to CodeRabbit complaining about consistency across all plugin SDK dependencies. I'll look at Console I'm not sure why |
vojtechszocs
force-pushed
the
bump-sdk-deps
branch
from
51e8bd3 to
802c50b
Compare
vojtechszocs
changed the title
vojtechszocs
changed the title
|
|
||
| return [actions, true, undefined]; | ||
| }, [actions]); | ||
| const actions = useMemo(() => (isVMElement ? [] : [startAction]), [isVMElement, startAction]); |
There was a problem hiding this comment.
There is a hidden bug. The purpose of this useModifyApplicationActionProvider hook is to add a startAction to VirtualMachine element. The logic here is reversed.
I tracked this to this commit, where a const isVMType = (type: string) => type !== VIRTUAL_MACHINE_TYPE; helper is created. It is used correctly but the naming should have been isNotVMType.
Later it is refactored here to ===, but the isVMType usage in this file is forgotten.
I checked other usages of isVMType and this is the only one wrong.
3 participants
📝 Description
This is a follow-up to openshift/console#15802 (comment)
This PR bumps all Console plugin SDK dependencies to latest 4.21 prerelease version for consistency.
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.