OCPBUGS-80989: Skip to fetch resource if model not found and load the other resources in list-page#16230
OCPBUGS-80989: Skip to fetch resource if model not found and load the other resources in list-page#16230vikram-raj wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@vikram-raj: This pull request references Jira Issue OCPBUGS-80989, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
component/sdk
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@vikram-raj: This pull request references Jira Issue OCPBUGS-80989, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
📝 WalkthroughWalkthroughThe changes modify how ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts (1)
55-63: Fallback model resolution logic is sound.The approach correctly handles version mismatches by:
- Normalizing
core→''to match the inverse ofgetReferenceForModel- Filtering candidates by
kind+apiGroup- Preferring exact version match, falling back to first candidate
Consider adding a brief inline comment explaining why
coreis normalized to empty string, as this inverts the convention ink8s-ref.tsand may confuse future maintainers.📝 Suggested documentation
if (!resourceModel && modelReference?.split('~').length === 3) { const { group, version, kind } = getGroupVersionKindForReference(modelReference); + // Normalize 'core' → '' to match K8sModel.apiGroup convention (inverse of getReferenceForModel) const normalizedGroup = group === 'core' ? '' : group;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts` around lines 55 - 63, The fallback model resolution works but lacks a clarifying comment about normalizing group 'core' to ''—add a brief inline comment next to the normalization line (where normalizedGroup is assigned) explaining that this inverts the "core" → '' convention used by getReferenceForModel/getReferenceForModel in k8s-ref.ts so lookups match allK8sModels; reference modelReference, getGroupVersionKindForReference, normalizedGroup, and allK8sModels in the comment to make intent clear for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts`:
- Around line 128-133: The plural hook useK8sWatchResources currently treats
missing models by setting loadError: undefined, which differs from
useK8sWatchResource's behavior; update the branch that checks
reduxIDs?.[key]?.noModel inside useK8sWatchResources so it sets loadError to new
NoModelError() (same class used by useK8sWatchResource) while preserving data
and loaded values, ensuring consumers see the same error representation across
both hooks (refer to reduxIDs, resources, loadError and NoModelError in the
function).
---
Nitpick comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts`:
- Around line 55-63: The fallback model resolution works but lacks a clarifying
comment about normalizing group 'core' to ''—add a brief inline comment next to
the normalization line (where normalizedGroup is assigned) explaining that this
inverts the "core" → '' convention used by
getReferenceForModel/getReferenceForModel in k8s-ref.ts so lookups match
allK8sModels; reference modelReference, getGroupVersionKindForReference,
normalizedGroup, and allK8sModels in the comment to make intent clear for future
maintainers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4a2c19a0-e6a9-4488-a841-bc12a7079b27
📒 Files selected for processing (2)
frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__/useK8sWatchResources.spec.tsxfrontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__/useK8sWatchResources.spec.tsx
🔇 Additional comments (2)
frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__/useK8sWatchResources.spec.tsx (1)
368-420: Test updates correctly reflect the new silent-skip semantics.The assertions now expect
loadError: undefinedinstead of an error object, and the test descriptions clearly communicate the behavioral change. The tests validate that no API calls are made when models are unknown, which is the intended optimization.frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts (1)
50-53: Simplified model resolution by removing ref caching — good trade-off.Replacing the
usePrevious/useRefcaching pattern with directallK8sModelsusage reduces complexity. Since model definitions rarely change at runtime, the potential for extra recomputations is minimal compared to the maintenance burden of the previous approach.Also applies to: 77-77
| if (reduxIDs?.[key]?.noModel) { | ||
| acc[key] = { | ||
| data: resources[key].isList ? [] : {}, | ||
| loaded: true, | ||
| loadError: new NoModelError(), | ||
| loadError: undefined, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current NoModelError usage in useK8sWatchResource and any consumers checking for it
rg -n "NoModelError" --type ts -C 3Repository: openshift/console
Length of output: 2813
🏁 Script executed:
#!/bin/bash
# Check consumers of both hooks and their return type expectations
echo "=== Searching for useK8sWatchResources (plural) usage ==="
rg -n "useK8sWatchResources" --type ts -A 2 | head -40
echo -e "\n=== Searching for useK8sWatchResource (singular) usage ==="
rg -n "useK8sWatchResource[^s]" --type ts -A 2 | head -40Repository: openshift/console
Length of output: 7774
🏁 Script executed:
#!/bin/bash
# Check type definitions for return types
echo "=== Type definition for useK8sWatchResources ==="
rg -n "type UseK8sWatchResources|interface.*useK8sWatchResources" --type ts -A 5
echo -e "\n=== Type definition for useK8sWatchResource ==="
rg -n "type UseK8sWatchResource[^s]|interface.*useK8sWatchResource[^s]" --type ts -A 5Repository: openshift/console
Length of output: 1615
🏁 Script executed:
#!/bin/bash
# Check if consumers explicitly check for NoModelError
rg -n "NoModelError|instanceof.*NoModelError" --type ts | grep -v "import\|export\|class NoModelError"Repository: openshift/console
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Check WatchK8sResult and WatchK8sResults type definitions
rg -n "type WatchK8sResult|interface WatchK8sResult" --type ts -A 5Repository: openshift/console
Length of output: 2130
🏁 Script executed:
#!/bin/bash
# Check how consumers handle the loadError from useK8sWatchResource
rg -n "loadError.*NoModelError|instanceof.*NoModelError" --type tsRepository: openshift/console
Length of output: 43
API inconsistency with useK8sWatchResource (singular hook).
The singular hook useK8sWatchResource returns new NoModelError() when a model is not found (line 60), while this plural hook returns loadError: undefined. This divergence in error semantics creates an inconsistent contract for consumers:
| Hook | Missing model behavior |
|---|---|
useK8sWatchResource |
[data, loaded, new NoModelError()] |
useK8sWatchResources |
{ data, loaded: true, loadError: undefined } |
Though the type system allows both (using any), this inconsistency can confuse developers switching between hooks or extending the SDK. Consider aligning both to return the same error representation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts`
around lines 128 - 133, The plural hook useK8sWatchResources currently treats
missing models by setting loadError: undefined, which differs from
useK8sWatchResource's behavior; update the branch that checks
reduxIDs?.[key]?.noModel inside useK8sWatchResources so it sets loadError to new
NoModelError() (same class used by useK8sWatchResource) while preserving data
and loaded values, ensuring consumers see the same error representation across
both hooks (refer to reduxIDs, resources, loadError and NoModelError in the
function).
frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts
Outdated
Show resolved
Hide resolved
vikram-raj
force-pushed
the
ocpbugs-80989
branch
from
57c6b95 to
42ab08d
Compare
vikram-raj
changed the title
|
@vikram-raj: This pull request references Jira Issue OCPBUGS-80989, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, vikram-raj 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 |
|
/retest |
The fix looks good to me |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by 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. |
|
/test e2e-gcp-console |
|
/hold Revision 42ab08d was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/cancel hold |
|
/test e2e-gcp-console |
|
@vikram-raj: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Descriptions:
If any model is not found, then the Resources list page does not show any resources, instead display a error message
Error loading Model does not exist.Before:
After:
Summary by CodeRabbit