CNV-74875: Fix serch suggestions for non-priv users #3322
Conversation
Signed-off-by: batyana <[email protected]>
|
@batyana: This pull request references CNV-74875 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. |
📝 WalkthroughWalkthroughAdds a new generic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
@batyana: This pull request references CNV-74875 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: 1
🧹 Nitpick comments (1)
src/views/virtualmachines/search/hooks/useAccessibleVMs.ts (1)
29-33: Consider conditionally disabling the global fetch for non-admin users.The
useKubevirtWatchResourcecall fetches all resources cluster-wide unconditionally. For non-admin users whereloadPerNamespaceis true, this global fetch is unused—potentially wasting bandwidth and memory.If the hook supports a disabled/skip option, consider passing it to avoid the unnecessary fetch.
🔎 Proposed approach (if supported by the hook API)
const [allResources, allResourcesLoaded] = useKubevirtWatchResource<T[]>({ groupVersionKind, isList: true, limit: OBJECTS_FETCHING_LIMIT, + enabled: !loadPerNamespace, // or skip: loadPerNamespace });If the hook doesn't support this, the current approach is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/views/virtualmachines/search/hooks/useAccessibleVMs.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use
.tsfile extension for non-component files containing logic or utilities
Files:
src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleVMs.ts
**/*.{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/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleVMs.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/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleVMs.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/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleVMs.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/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleVMs.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleVMs.ts
🧬 Code graph analysis (2)
src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts (1)
src/views/virtualmachines/search/hooks/useAccessibleVMs.ts (2)
useAccessibleVMs(73-79)useAccessibleVMIs(81-87)
src/views/virtualmachines/search/hooks/useAccessibleVMs.ts (1)
src/views/virtualmachines/utils/constants.ts (1)
OBJECTS_FETCHING_LIMIT(7-7)
⏰ 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 (6)
src/views/virtualmachines/search/hooks/useAccessibleVMs.ts (2)
1-18: LGTM!Imports are well-organized, and the generic
UseAccessibleResources<T>type is a clean abstraction for the internal hook's return value.
73-87: LGTM!The exported hooks are clean, well-typed wrappers that provide a clear API for consumers. Good use of property renaming for clarity.
src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts (4)
3-3: LGTM!Clean import refactoring—removed the now-unused direct resource watching imports and added the new hook imports.
Also applies to: 8-10
19-20: LGTM!Clean integration of the new hooks. The destructured properties match the expected return types.
22-68: LGTM!All memoized computations properly guard on the loaded state before filtering, and dependency arrays are complete and correct.
70-86: LGTM!The result construction and return value are correct. The combined
vmsLoaded && vmisLoadedensures consumers only see a loaded state when both resources are ready.
|
@batyana: This pull request references CNV-74875 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. |
|
@batyana: This pull request references CNV-74875 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. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts:
- Around line 23-29: The hook unconditionally calls useAccessibleResources
causing extra watches even when a namespace is provided; update
useVirtualMachineInstanceMigrations to avoid running that hook when namespace
exists by adding an opt-in flag to useAccessibleResources (e.g., enabled:
boolean) and pass enabled: !namespace, or split into two hooks and only call
useAccessibleResources when namespace is falsy; ensure you use the returned
accessibleVMIMs/accessibileVMIMsLoaded only when enabled and fall back to
namespacedVMIMs/namespacedVMIMsLoaded otherwise.
In @src/views/virtualmachines/list/VirtualMachinesList.tsx:
- Around line 133-145: When namespace is set we still call
useAccessibleResources for VMs and VMIs unnecessarily; update the two hook calls
to pass an enabled option that disables them when namespace is truthy (e.g.,
call
useAccessibleResources<V1VirtualMachine>(VirtualMachineModelGroupVersionKind, {
enabled: !namespace }) and similarly for V1VirtualMachineInstance) so
accessibleVMs, accessibleVMIs, accessibleVMsLoaded and accessibleVMIsLoaded
aren’t populated/watched when namespaced data (namespacedVMs/namespacedVMIs)
will be used instead; keep the subsequent
vms/vmis/vmsLoaded/vmisLoaded/vmsLoadError selection logic unchanged.
In @src/views/virtualmachines/search/hooks/useAccessibleResources.ts:
- Around line 11-15: The UseAccessibleResources<T> type exposes loadError?: any
which violates typing rules; change loadError to a safer type such as
loadError?: unknown or a concrete error type (e.g. Error | APIError) to avoid
any; update the type alias UseAccessibleResources and any uses of loadError in
callers (e.g. in useAccessibleResources hook consumers) to narrow or assert the
error before accessing properties.
- Around line 54-59: The computed loaded flag can become true as soon as any
namespace finishes because it uses Object.values(allowedResources).some(...);
change that to require all namespaces to be completed by using .every instead.
Update the expression that defines loaded (which currently uses
projectNamesLoaded, loadPerNamespace, allowedResources and allResourcesLoaded)
so the ternary branch becomes: if loadPerNamespace then
isEmpty(allowedResources) || Object.values(allowedResources).every(resource =>
resource.loaded || resource.loadError) else allResourcesLoaded, leaving the
surrounding variables (projectNamesLoaded, loadPerNamespace, allResourcesLoaded)
intact.
- Around line 26-30: The cluster-wide watch for allResources is always started
even when loadPerNamespace is true, causing 403s for non-admins; update the hook
signatures so watchOptions is optional: make watchOptions optional in
useKubevirtWatchResource and in useRedirectWatchHooks (since useK8sWatchData
already accepts null), then change the call that declares const [allResources,
allResourcesLoaded] = useKubevirtWatchResource<T[]>({ groupVersionKind, isList:
true, limit: OBJECTS_FETCHING_LIMIT }) to only pass watchOptions when needed
(i.e., when loadPerNamespace is false or the caller actually consumes
allResources) so the cluster-wide fetch is not initiated for users that
shouldn’t run it.
🧹 Nitpick comments (2)
src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts (1)
34-36: Consider exposingloadedstate for downstream filter readiness.The
loadedflag fromuseAccessibleResourcesis not destructured here. Ifvmsis used before loading completes, filters likeuseStorageClassFilterandgetArchitectureFiltermay briefly show incomplete options. This is likely a minor UX concern since filters should handle empty arrays, but worth verifying the downstream behavior.src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx (1)
57-59: Optional: Consider usingloadedstate for field loading indicators.The
loadedflag is not destructured. Form fields receivingvms(StorageClassField, LabelsField, ArchitectureField) will operate on an empty array until loading completes. If these fields support loading states, consider passing theloadedflag to improve UX during initial data fetch.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/search/hooks/useAccessibleResources.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleResources.tssrc/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
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.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleResources.tssrc/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension 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 as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.tssrc/views/virtualmachines/search/hooks/useAccessibleResources.tssrc/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx
📚 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/virtualmachines/list/VirtualMachinesList.tsxsrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx
🧬 Code graph analysis (6)
src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts (1)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-62)
src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts (1)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-62)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (2)
src/views/virtualmachines/utils/constants.ts (1)
OBJECTS_FETCHING_LIMIT(7-7)src/utils/utils/utils.ts (1)
isEmpty(32-33)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts (2)
src/views/virtualmachines/utils/constants.ts (1)
OBJECTS_FETCHING_LIMIT(7-7)src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-62)
src/views/virtualmachines/list/VirtualMachinesList.tsx (4)
src/views/virtualmachines/utils/constants.ts (1)
OBJECTS_FETCHING_LIMIT(7-7)src/views/virtualmachines/list/utils/constants.ts (2)
VM_FILTER_OPTIONS(1-9)VMI_FILTER_OPTIONS(11-14)src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-62)src/utils/components/GuidedTour/utils/guidedTourSignals.ts (1)
runningTourSignal(3-3)
src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx (1)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-62)
⏰ 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 (4)
src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts (1)
12-14: LGTM! Clean refactor to use the new access-aware hook.The migration from
useKubevirtWatchResourcetouseAccessibleResourcesis correctly implemented. The destructured variables align with existing downstream logic, and memoization dependencies are properly updated.Also applies to: 23-28
src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
147-147: LGTM! Tour guide VM handling preserved.The
vmsToShowmemo correctly preserves the tour guide functionality while adapting to the new dual-path data source.
301-301: LGTM! Error propagation correctly updated.
vmsLoadErroris properly selected based on the data source path and passed to the table.src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
17-19: Add explicit return type annotation.Per coding guidelines, always explicitly define return types for functions rather than relying on TypeScript type inference.
🔎 Proposed fix
The return type is already defined via
UseAccessibleResources<T>, so this is correctly annotated. No change needed.
| const { loaded: accessibleVMIMsLoaded, resources: accessibleVMIMs } = | ||
| useAccessibleResources<V1VirtualMachineInstanceMigration>( | ||
| VirtualMachineInstanceMigrationModelGroupVersionKind, | ||
| ); | ||
|
|
||
| const vmims = namespace ? namespacedVMIMs : accessibleVMIMs; | ||
| const loaded = namespace ? namespacedVMIMsLoaded : accessibleVMIMsLoaded; |
There was a problem hiding this comment.
Performance: useAccessibleResources runs unconditionally even when namespace is provided.
When a namespace is supplied, the hook uses namespacedVMIMs but useAccessibleResources still executes, potentially triggering unnecessary API calls for all accessible namespaces. This is especially impactful for non-admin users where per-namespace watches are created.
Consider conditionally invoking the accessible resources path only when needed:
🔎 Proposed approach
Since React hooks cannot be called conditionally, consider restructuring to avoid the redundant fetch. One option is to split this into two hooks or pass a flag to disable the accessible resources fetch when not needed:
+ const shouldFetchAccessible = !namespace;
+
const { loaded: accessibleVMIMsLoaded, resources: accessibleVMIMs } =
- useAccessibleResources<V1VirtualMachineInstanceMigration>(
+ useAccessibleResources<V1VirtualMachineInstanceMigration>(
VirtualMachineInstanceMigrationModelGroupVersionKind,
+ { enabled: shouldFetchAccessible }
);This would require updating useAccessibleResources to accept an enabled option.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts around
lines 23 - 29, The hook unconditionally calls useAccessibleResources causing
extra watches even when a namespace is provided; update
useVirtualMachineInstanceMigrations to avoid running that hook when namespace
exists by adding an opt-in flag to useAccessibleResources (e.g., enabled:
boolean) and pass enabled: !namespace, or split into two hooks and only call
useAccessibleResources when namespace is falsy; ensure you use the returned
accessibleVMIMs/accessibileVMIMsLoaded only when enabled and fall back to
namespacedVMIMs/namespacedVMIMsLoaded otherwise.
| type UseAccessibleResources<T> = { | ||
| loaded: boolean; | ||
| loadError?: any; | ||
| resources: T[]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid any type; use unknown or a specific error type.
Per coding guidelines, avoid any type in TypeScript. Consider using unknown or the specific error type returned by the hooks.
🔎 Proposed fix
type UseAccessibleResources<T> = {
loaded: boolean;
- loadError?: any;
+ loadError?: unknown;
resources: T[];
};📝 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.
| type UseAccessibleResources<T> = { | |
| loaded: boolean; | |
| loadError?: any; | |
| resources: T[]; | |
| }; | |
| type UseAccessibleResources<T> = { | |
| loaded: boolean; | |
| loadError?: unknown; | |
| resources: T[]; | |
| }; |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/search/hooks/useAccessibleResources.ts around
lines 11 - 15, The UseAccessibleResources<T> type exposes loadError?: any which
violates typing rules; change loadError to a safer type such as loadError?:
unknown or a concrete error type (e.g. Error | APIError) to avoid any; update
the type alias UseAccessibleResources and any uses of loadError in callers (e.g.
in useAccessibleResources hook consumers) to narrow or assert the error before
accessing properties.
src/views/virtualmachines/search/hooks/useAccessibleResources.ts
Outdated
Show resolved
Hide resolved
batyana
force-pushed
the
CNV-74875-Fix-search-suggestions-for-non-priv-user
branch
from
c543cb3 to
fe6c76a
Compare
|
@batyana: This pull request references CNV-74875 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts:
- Around line 23-26: The hook currently ignores load errors from
useAccessibleResources (and useKubevirtWatchResource) so callers can’t
distinguish error states; update the destructuring from
useAccessibleResources<V1VirtualMachineInstanceMigration>(VirtualMachineInstanceMigrationModelGroupVersionKind)
to also capture the loadError (e.g., accessibleVMIMsLoadError) and
propagate/return it from useVirtualMachineInstanceMigrations alongside
accessibleVMIMs and accessibleVMIMsLoaded, and likewise capture and include the
loadError returned by useKubevirtWatchResource so the hook returns both loaded,
data, and any error(s) to consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/search/hooks/useAccessibleResources.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts
- src/views/virtualmachines/search/hooks/useAccessibleResources.ts
- src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx
- src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
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.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/VirtualMachinesList.tsx
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension 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 as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/virtualmachines/list/VirtualMachinesList.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/VirtualMachinesList.tsx
📚 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/virtualmachines/list/VirtualMachinesList.tsx
🧬 Code graph analysis (2)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts (2)
src/views/virtualmachines/utils/constants.ts (1)
OBJECTS_FETCHING_LIMIT(7-7)src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-67)
src/views/virtualmachines/list/VirtualMachinesList.tsx (4)
src/views/virtualmachines/list/utils/constants.ts (1)
VM_FILTER_OPTIONS(1-9)src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-67)src/utils/components/GuidedTour/utils/guidedTourSignals.ts (1)
runningTourSignal(3-3)src/views/virtualmachines/utils/mappers.ts (1)
getVMIFromMapper(26-27)
⏰ 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 (8)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts (3)
7-21: LGTM: Conditional namespaced fetch pattern.The conditional
useKubevirtWatchResourcecall correctly returnsnullwhennamespaceis not provided, following React hooks best practices for conditional data fetching.
28-31: LGTM: Clean conditional data source selection.The conditional selection logic correctly switches between namespaced and accessible data sources based on the
namespaceparameter, with a safe fallback to an empty array.
23-26: Performance: UnconditionaluseAccessibleResourcesexecution.The
useAccessibleResourceshook runs even whennamespaceis provided, potentially triggering unnecessary API calls. This issue has been previously identified.Likely an incorrect or invalid review comment.
src/views/virtualmachines/list/VirtualMachinesList.tsx (5)
98-113: LGTM: Conditional namespaced VM fetch with proper error handling.The conditional resource fetching correctly captures all return values including
loadError, and properly integrates filter options and search queries.
121-123: LGTM: Clean conditional selection with proper error handling.Unlike the VMIM hook, this implementation correctly captures and propagates
loadErrorfrom both data sources.
209-209: LGTM: Consistent usage of refactored data sources.The usage of
filteredVMIsandvmsLoadErrorcorrectly reflects the dual-path data fetching refactoring, conditional on the verification requests above.Also applies to: 284-284
115-119: Performance: UnconditionaluseAccessibleResourcesexecution.Similar to the VMIM hook,
useAccessibleResourcesruns even whennamespaceis provided, potentially triggering unnecessary API calls. This issue has been previously identified.Likely an incorrect or invalid review comment.
145-148: No action required. TheuseVirtualMachineInstanceMapper()hook correctly handles multi-namespace scenarios. It fetches all VirtualMachineInstances without namespace restrictions (viauseKubevirtWatchResourcewithisList: trueand no namespace parameter), and organizes them in a mapper structure by[cluster][namespace][name]. WhengetVMIFromMapperis called with a VM, it uses the VM's own namespace to correctly retrieve the associated VMI from the mapper, ensuring proper lookups regardless of whether VMs span single or multiple namespaces.
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| const loaded = vmsLoaded && vmisLoaded && vmimsLoaded && !loadingFeatureProxy && loadedColumns; | ||
| const loaded = vmsLoaded && vmimsLoaded && !loadingFeatureProxy && loadedColumns; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the useVirtualMachineInstanceMapper hook implementation
fd -e ts -e tsx | xargs rg -l "useVirtualMachineInstanceMapper" | head -5Repository: kubevirt-ui/kubevirt-plugin
Length of output: 451
🏁 Script executed:
# Find where useVirtualMachineInstanceMapper is defined
rg -nP "export.*useVirtualMachineInstanceMapper|function useVirtualMachineInstanceMapper" --type=ts --type=tsxRepository: kubevirt-ui/kubevirt-plugin
Length of output: 98
🏁 Script executed:
# Get context from the VirtualMachinesList file around line 129 and 193
fd -name "VirtualMachinesList.tsx" -exec cat -n {} \; | sed -n '120,200p'Repository: kubevirt-ui/kubevirt-plugin
Length of output: 301
🏁 Script executed:
# Read the useVirtualMachineInstanceMapper hook
cat -n src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tsRepository: kubevirt-ui/kubevirt-plugin
Length of output: 1965
🏁 Script executed:
# Read relevant sections of VirtualMachinesList.tsx
head -200 src/views/virtualmachines/list/VirtualMachinesList.tsx | tail -80Repository: kubevirt-ui/kubevirt-plugin
Length of output: 2797
🏁 Script executed:
# Check the useKubevirtWatchResource hook to see what it returns
fd -name "useKubevirtWatchResource.ts" -o -name "useKubevirtWatchResource.tsx" | xargs cat -n | head -100Repository: kubevirt-ui/kubevirt-plugin
Length of output: 301
🏁 Script executed:
# Also check for any other uses of useVirtualMachineInstanceMapper to see if loading is handled elsewhere
rg "useVirtualMachineInstanceMapper" -B 2 -A 2 --type ts --type tsxRepository: kubevirt-ui/kubevirt-plugin
Length of output: 98
🏁 Script executed:
# Find and read the useKubevirtWatchResource hook
find . -name "useKubevirtWatchResource.ts" -o -name "useKubevirtWatchResource.tsx" | head -1 | xargs catRepository: kubevirt-ui/kubevirt-plugin
Length of output: 1796
🏁 Script executed:
# Check all uses of useVirtualMachineInstanceMapper
rg "useVirtualMachineInstanceMapper" -B 2 -A 2Repository: kubevirt-ui/kubevirt-plugin
Length of output: 3213
Expose VMI loading state from useVirtualMachineInstanceMapper.
The useVirtualMachineInstanceMapper hook calls useKubevirtWatchResource, which returns [resource, resourceLoaded, resourceLoadError], but only destructures the first element and discards the loading state (line 11). The hook then returns only the mapper without any loading information.
In VirtualMachinesList.tsx (line 193), the loaded flag does not account for VMI loading:
const loaded = vmsLoaded && vmimsLoaded && !loadingFeatureProxy && loadedColumns;This means the page can show as fully loaded while VMIs are still being fetched. Either:
- Modify
useVirtualMachineInstanceMapperto return both the mapper and its loading state, then include it in theloadedcalculation, or - Consume the loading state directly in the component if the hook can't expose it.
There was a problem hiding this comment.
removed for now as to not conflict with this pr that addresses this issue #3305
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check PR #3305 to understand what issue it addresses
gh pr view 3305 --json title,body,state,url,number
Length of output: 1569
@batyana, thank you for clarifying! That's a sensible approach to coordinate these related changes and avoid merge conflicts. Since PR #3305 will address the VMI loading state handling more comprehensively as part of its refactoring work, it makes sense to defer this to that PR.
I appreciate you keeping the PRs focused and managing the dependencies between them. 👍
🧬 Code Graph Analysis Results
[filename: src/utils/constants/constants.ts, lines 3-3]
export const DEFAULT_NAMESPACE = 'default';
[filename: src/views/virtualmachines/utils/constants.ts, lines 7-7]
export const OBJECTS_FETCHING_LIMIT = 10000;
[filename: src/utils/components/GuidedTour/utils/guidedTourSignals.ts, lines 3-3]
export const runningTourSignal = signal(false);
[filename: src/utils/hooks/useFeatures/constants.ts, lines 17-17]
export const KUBEVIRT_APISERVER_PROXY = 'kubevirtApiserverProxy';
[filename: src/utils/components/ListPageFilter/types.ts, lines 17-19]
export type ExposedFilterFunctions = {
onFilterChange: OnFilterChange;
};
[filename: src/multicluster/helpers/selectors.ts, lines 6-7]
export const getCluster = <A extends K8sResourceCommon = K8sResourceCommon>(resource?: A) =>
resource?.cluster;
[filename: src/utils/utils/utils.ts, lines 32-33]
export const isEmpty = (obj) =>
[Array, Object].includes((obj || {}).constructor) && !Object.entries(obj || {}).length;
[filename: src/views/virtualmachines/list/listPageBodySize.ts, lines 7-11]
export const getListPageBodySize = (width: number) => {
if (width < 660) return ListPageBodySize.sm;
if (width < 1100) return ListPageBodySize.md;
return ListPageBodySize.lg;
};
[filename: src/utils/resources/shared.ts, lines 333-334]
export const getName = <A extends K8sResourceCommon = K8sResourceCommon>(resource: A) =>
resource?.metadata?.name;
[filename: src/utils/resources/shared.ts, lines 341-342]
export const getNamespace = <A extends K8sResourceCommon = K8sResourceCommon>(resource: A) =>
resource?.metadata?.namespace;
[filename: src/views/virtualmachines/list/utils/constants.ts, lines 1-9]
export const VM_FILTER_OPTIONS = {
labels: 'metadata.labels',
name: 'metadata.name',
'rowFilter-os': [
'spec.template.metadata.annotations.vm\\.kubevirt\\.io/os',
'spec.preference.name',
],
'rowFilter-status': 'status.printableStatus',
} as const;
[filename: src/multicluster/urls.ts, lines 53-59]
export const getCatalogURL = (cluster: string, namespace?: string): string => {
const namespacePath = isAllNamespaces(namespace) ? ALL_NAMESPACES : `ns/${namespace}`;
return cluster
? `/k8s/cluster/${cluster}/${namespacePath}/catalog`
: `/k8s/${namespacePath}/catalog`;
};
[filename: src/utils/constants/page-constants.ts, lines 3-10]
export const PageTitles = {
BootableVolumes: t('Bootable volumes'),
Catalog: t('Catalog'),
Checkups: t('Checkups'),
VirtualMachineClusterInstanceTypes: t('VirtualMachineClusterInstanceTypes'),
VirtualMachineInstanceTypes: t('VirtualMachineInstanceTypes'),
VirtualMachines: t('VirtualMachines'),
};
[filename: src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts, lines 10-43]
export const useVirtualMachineInstanceMapper = () => {
const [vmis] = useKubevirtWatchResource<V1VirtualMachineInstance[]>({
groupVersionKind: VirtualMachineInstanceModelGroupVersionKind,
isList: true,
});
const vmiMapper: VMIMapper = useMemo(() => {
return (Array.isArray(vmis) ? vmis : [])?.reduce(
(acc, vmi) => {
const name = vmi?.metadata?.name;
const namespace = vmi?.metadata?.namespace;
const cluster = getCluster(vmi) || SINGLE_CLUSTER_KEY;
if (!acc.mapper[cluster]) {
acc.mapper[cluster] = {};
}
if (!acc.mapper[cluster][namespace]) {
acc.mapper[cluster][namespace] = {};
}
acc.mapper[cluster][namespace][name] = vmi;
const nodeName = vmi?.status?.nodeName;
if (nodeName && !acc?.nodeNames?.[nodeName]) {
acc.nodeNames[nodeName] = {
id: nodeName,
title: nodeName,
};
}
return acc;
},
{ mapper: {}, nodeNames: {} },
);
}, [vmis]);
return vmiMapper;
};
[filename: src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts, lines 25-81]
export const useVMListFilters = (
vmiMapper: VMIMapper,
pvcMapper: PVCMapper,
): {
filtersWithSelect: RowFilter<V1VirtualMachine>[];
hiddenFilters: RowFilter<V1VirtualMachine>[];
} => {
const isACMPage = useIsACMPage();
const { resources: vms } = useAccessibleResources<V1VirtualMachine>(
VirtualMachineModelGroupVersionKind,
);
const clusterFilter = useClusterFilter();
const projectFilter = useProjectFilter();
const statusFilter = getStatusFilter();
const osFilters = getOSFilter();
const storageClassFilter = useStorageClassFilter(vms, pvcMapper);
const hwDevicesFilter = getHWDevicesFilter();
const schedulingFilter = getSchedulingFilter();
const nodesFilter = useNodesFilter(vmiMapper);
const descriptionFilter = getDescriptionFilter();
const cpuFilter = getCPUFilter(vmiMapper);
const memoryFilter = getMemoryFilter(vmiMapper);
const dateFromFilter = getDateFilter('from');
const dateToFilter = getDateFilter('to');
const architectureFilter = getArchitectureFilter(vms);
const ipFilter = getIPFilter(vmiMapper);
const nadFilter = getNADsFilter();
const filtersWithSelect = [
projectFilter,
statusFilter,
osFilters,
storageClassFilter,
hwDevicesFilter,
schedulingFilter,
nodesFilter,
];
if (isACMPage) filtersWithSelect.unshift(clusterFilter);
return {
filtersWithSelect,
hiddenFilters: [
descriptionFilter,
cpuFilter,
memoryFilter,
dateFromFilter,
dateToFilter,
architectureFilter,
ipFilter,
nadFilter,
],
};
};
[filename: src/utils/hooks/usePVCMapper.ts, lines 13-26]
export const usePVCMapper = (namespace: string, cluster?: string) => {
const [pvcs] = useKubevirtWatchResource<IoK8sApiCoreV1PersistentVolumeClaim[]>({
cluster,
groupVersionKind: modelToGroupVersionKind(PersistentVolumeClaimModel),
isList: true,
limit: OBJECTS_FETCHING_LIMIT,
namespace,
namespaced: true,
});
const pvcMapper = useMemo(() => convertIntoPVCMapper(pvcs), [pvcs]);
return pvcMapper;
};
[filename: src/views/virtualmachines/list/utils/constants.ts, lines 96-145]
[doc] (tour guide constant used for guided tour VM)
export const tourGuideVM: V1VirtualMachine = {
apiVersion: 'kubevirt.io/v1',
kind: 'VirtualMachine',
metadata: {
name: 'rhel9-tour-guide',
namespace: 'default',
},
spec: {
dataVolumeTemplates: [
{
metadata: {
name: 'rhel9-volume-tour-guide',
},
spec: {
sourceRef: {
kind: 'DataSource',
name: 'rhel9',
},
storage: {
resources: {},
},
},
},
],
instancetype: {
inferFromVolume: 'rhel9-volume-tour-guide',
},
preference: {
inferFromVolume: 'rhel9-volume-tour-guide',
},
runStrategy: RUNSTRATEGY_ALWAYS,
template: {
spec: {
domain: {
devices: {},
resources: {},
},
terminationGracePeriodSeconds: 180,
volumes: [
{
dataVolume: {
name: 'rhel9-volume-tour-guide',
},
name: 'rhel9-volume-tour-guide',
},
],
},
},
},
};
[filename: src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMigrationMapper.ts, lines 10-43]
export const useVirtualMachineInstanceMapper = () => {
const [vmis] = useKubevirtWatchResource<V1VirtualMachineInstance[]>({
groupVersionKind: VirtualMachineInstanceModelGroupVersionKind,
isList: true,
});
// ... (context omitted)
};
🧠 Learnings used
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
…anced search Signed-off-by: batyana <[email protected]>
batyana
force-pushed
the
CNV-74875-Fix-search-suggestions-for-non-priv-user
branch
from
fe6c76a to
cf33f7a
Compare
|
@batyana: This pull request references CNV-74875 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/views/virtualmachines/list/VirtualMachinesList.tsx:
- Around line 115-123: useAccessibleResources is invoked unconditionally in
VirtualMachinesList causing unnecessary watches when namespace is provided;
update the hook to accept an `enabled` (or similar) option and only call/use it
when needed. Modify the hook signature (useAccessibleResources) to support an
options object with enabled:boolean, update its internal effect/watch to
early-return when enabled is false, and change the VirtualMachinesList call to
pass enabled: !namespace (or only call it when namespace is undefined) so that
namespacedVMs are used without starting global accessible watches.
- Line 209: filteredVMIs is currently created by mapping with getVMIFromMapper
and filtering truthy values at runtime, but TypeScript doesn't narrow the type;
add a type predicate like isTruthy<T>(value: T): value is NonNullable<T> and use
it in the useMemo that builds filteredVMIs (the map over filteredVMs using
getVMIFromMapper with vmiMapper) so filteredVMIs is typed as
V1VirtualMachineInstance[] and matches the VirtualMachineListSummary prop.
🧹 Nitpick comments (2)
src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx (1)
57-59: Consider capturingloadedstate for potential loading indicators.The
useAccessibleResourceshook returnsloadedandloadError, but onlyresourcesis destructured here. While the current usage may work (child components likely handle undefined gracefully), capturing the loading state could improve UX if loading indicators are needed in the future.🔎 Optional enhancement
- const { resources: vms } = useAccessibleResources<V1VirtualMachine>( + const { loaded: vmsLoaded, resources: vms } = useAccessibleResources<V1VirtualMachine>( VirtualMachineModelGroupVersionKind, );You could then use
vmsLoadedto conditionally render loading states for fields that depend onvms.src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts (1)
34-36: Potential duplicate resource fetching.This hook fetches VMs via
useAccessibleResources, but the parent component (VirtualMachinesList.tsx) also fetches VMs through the same hook. This could result in duplicate API watches for the same resources.Consider passing
vmsas a parameter to this hook instead of fetching independently, similar to howvmiMapperandpvcMapperare already passed in.🔎 Proposed refactor
export const useVMListFilters = ( + vms: V1VirtualMachine[], vmiMapper: VMIMapper, pvcMapper: PVCMapper, ): { filtersWithSelect: RowFilter<V1VirtualMachine>[]; hiddenFilters: RowFilter<V1VirtualMachine>[]; } => { const isACMPage = useIsACMPage(); - const { resources: vms } = useAccessibleResources<V1VirtualMachine>( - VirtualMachineModelGroupVersionKind, - ); const clusterFilter = useClusterFilter(); // ... rest unchangedThen update the caller in
VirtualMachinesList.tsxto pass the already-fetchedvms.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/search/hooks/useAccessibleResources.tssrc/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/virtualmachines/search/hooks/useVirtualMachineSearchSuggestions.ts
- src/views/virtualmachines/search/hooks/useAccessibleResources.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension 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 as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
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.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/list/VirtualMachinesList.tsx
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts
🧠 Learnings (3)
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.tssrc/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.tssrc/views/virtualmachines/list/VirtualMachinesList.tsx
📚 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/search/components/AdvancedSearchModal/AdvancedSearchModal.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsx
📚 Learning: 2025-11-20T20:28:17.043Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3118
File: src/views/checkups/self-validation/utils/constants.ts:62-68
Timestamp: 2025-11-20T20:28:17.043Z
Learning: For the kubevirt-ui/kubevirt-plugin repository: The test suite labels in TEST_SUITE_OPTIONS (Compute, Network, Storage, SSP, Tier2) in src/views/checkups/self-validation/utils/constants.ts should not be internationalized, even though they are displayed in the UI.
Applied to files:
src/views/virtualmachines/list/VirtualMachinesList.tsx
🧬 Code graph analysis (4)
src/views/search/components/AdvancedSearchModal/AdvancedSearchModal.tsx (1)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-67)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts (2)
src/views/virtualmachines/utils/constants.ts (1)
OBJECTS_FETCHING_LIMIT(7-7)src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-67)
src/views/virtualmachines/list/hooks/useVMListFilters/useVMListFilters.ts (1)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-67)
src/views/virtualmachines/list/VirtualMachinesList.tsx (4)
src/views/virtualmachines/search/hooks/useAccessibleResources.ts (1)
useAccessibleResources(17-67)src/utils/components/GuidedTour/utils/guidedTourSignals.ts (1)
runningTourSignal(3-3)src/utils/components/GuidedTour/utils/constants.tsx (1)
tourGuideVM(96-145)src/views/virtualmachines/utils/mappers.ts (1)
getVMIFromMapper(26-27)
⏰ 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 (4)
src/utils/resources/vmim/hooks/useVirtualMachineInstanceMigrations.ts (2)
7-34: Good improvement: Error handling now properly captured and returned.The hook now correctly captures
loadErrorfrom both data sources and returns it in the tuple. This addresses the previous concern about silent error handling and allows consumers to properly handle error states.
22-28:useAccessibleResourcesruns unconditionally, causing redundant API calls when namespace is provided.When a
namespaceis supplied, the hook usesnamespacedVMIMsbutuseAccessibleResourcesstill executes, potentially triggering unnecessary watches for all accessible namespaces. This is especially impactful for non-admin users.Since React hooks cannot be called conditionally, consider adding an
enabledoption touseAccessibleResourcesto skip fetching when not needed.⛔ Skipped due to learnings
Learnt from: galkremer1 Repo: kubevirt-ui/kubevirt-plugin PR: 3165 File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27 Timestamp: 2025-12-12T17:26:25.615Z Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types: 1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()` 2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
145-148: Clean derivation of filtered VMIs from VMs.Good use of
useMemoto derivefilteredVMIsfrom the filtered VMs using the mapper. Thefilter(Boolean)correctly handles cases wheregetVMIFromMapperreturns undefined.
121-123: Good: Proper selection of data source based on namespace.The conditional selection of
vms,vmsLoaded, andvmsLoadErrorbased on namespace presence is clean and correctly implements the dual-path pattern. This ensures namespaced users get namespace-scoped data while admins get cluster-wide access.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batyana, 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 |
3 participants
📝 Description
created useAccessibleResources hook to fetch resources by namespace for non privileged users and cluster wide for admins to fix errors in loading vm's in both search suggestions and advanced search.
🎥 Demo
Before:
After:
Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.