OCPBUGS-85013: Fix Topology page TypeError when resource is null - #17071
OCPBUGS-85013: Fix Topology page TypeError when resource is null#17071platex-rehor-bot wants to merge 2 commits into
Conversation
The Topology view crashes with "can't access property kind, e is null"
when rendering nodes whose resource object has not yet loaded or is
unavailable (e.g. Helm release group nodes without a matching Secret).
getTopologyResourceObject() can return null, but several components
pass its result directly to referenceFor() which destructures {kind}
from the argument, causing a TypeError on null.
Add null guards with optional chaining across all affected call sites:
WorkloadNode, BaseNode, BindableNode, withEditReviewAccess,
OdcBaseNode, and getResourceKind utility.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@platex-rehor-bot: This pull request references Jira Issue OCPBUGS-85013, 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughTopology resource resolution now handles missing resources and metadata without throwing. Graph nodes pass optional values to model lookup, access review, resource-kind resolution, and pod watching. ChangesTopology resource safety
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change adds defensive handling for unavailable topology resources to prevent a rendering crash without changing behavior when resources are present. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
/jira refresh |
|
@platex-rehor-bot: This pull request references Jira Issue OCPBUGS-85013, which is invalid:
Comment 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/topology
|
Hi @platex-rehor-bot. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@platex-rehor-bot: This pull request references Jira Issue OCPBUGS-85013, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (mniranja@redhat.com), skipping review request. 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. |
stefanonardo
left a comment
stefanonardo
left a comment
There was a problem hiding this comment.
The defensive guards are a valid fix, but if other consumers are added in the future they'll hit the same trap
| node instanceof OdcBaseNode | ||
| ? (node as OdcBaseNode).getResourceKind() | ||
| : referenceFor(getTopologyResourceObject(node?.getData())); | ||
| export const getResourceKind = (node: Node): K8sResourceKindReference => { |
There was a problem hiding this comment.
| export const getResourceKind = (node: Node): K8sResourceKindReference => { | |
| export const getResourceKind = (node: Node): K8sResourceKindReference | undefined => { |
There was a problem hiding this comment.
Good catch — fixed in 7e9ae24. The return type now correctly reflects the | undefined possibility, matching the runtime behavior and the OdcBaseNode.getResourceKind() signature.
| @@ -234,8 +234,8 @@ const WorkloadNode: FC<WorkloadNodeProps> = observer(({ element, ...rest }) => { | |||
| const resource = getTopologyResourceObject(element.getData()); | |||
| const { podData, loadError, loaded } = usePodsWatcher( | |||
| resource, | |||
There was a problem hiding this comment.
resource is of type K8sResourceKind but it's potentially null
There was a problem hiding this comment.
You're right about the type gap. The GetTopologyResourceObject SDK type declares the return as K8sResourceKind, but the implementation can return null. Since updating the SDK type would be a breaking API change, we handle nullability at each call site with optional chaining (resource?.kind, resource?.metadata?.namespace). The usePodsWatcher hook also uses optional chaining internally, so null is handled gracefully at runtime — it skips watching when kind is undefined.
| const resourceObj = getResource(props.element); | ||
| const resourceModel = modelFor(referenceFor(resourceObj)); | ||
| const resourceModel = resourceObj ? modelFor(referenceFor(resourceObj)) : undefined; | ||
| const editAccess = useAccessReview({ |
There was a problem hiding this comment.
nit: with resourceModel being undefined, the RBAC request will fail, defaulting to setAllowed(true). So canEdit will be true for null resources. The server still enforces real access, but showing edit controls on unresolvable nodes seems wrong
There was a problem hiding this comment.
Great observation — fixed in 7e9ae24. When resourceObj is falsy, canEdit now defaults to false instead of relying on the RBAC check with undefined parameters. This prevents edit controls from appearing on unresolvable nodes.
- Fix getResourceKind return type to include undefined - Prevent edit controls showing on unresolvable nodes by defaulting canEdit to false when resource is null OCPBUGS-85013 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
The defensive guards are a valid fix, but if other consumers are added in the future they'll hit the same trap |
|
/retest |
|
/retest-required |
|
Good point — you're right that defensive guards at each call site don't prevent future consumers from hitting the same null trap. The root cause is that the SDK types The proper fix is to update those SDK types to This PR addresses the immediate TypeError; a follow-up can close the type gap for good. |
|
/retest-required |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: platex-rehor-bot, stefanonardo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test e2e-playwright-techpreview |
|
/test e2e-playwright |
|
@platex-rehor-bot: 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. |
|
/test e2e-playwright |
4 participants
Analysis / Root cause:
The Topology view crashes with
TypeError: can't access property kind, e is nullwhen rendering nodes whose resource object has not yet loaded or is unavailable. This occurs when installing Helm charts from the Developer Catalog — the Helm release group node may not have a matching Secret, causinggetTopologyResourceObject()to returnnull. Several topology components then pass thisnulltoreferenceFor(), which destructures{ kind, apiVersion }from its argument, producing the TypeError.The root cause is in
helm-data-transformer.tsline 89 whereresources.objis explicitly set tonullwhen no matching Secret exists:Multiple downstream consumers (
WorkloadNode,BaseNode,BindableNode,withEditReviewAccess,OdcBaseNode,getResourceKind) access.kindor callreferenceFor()on the result without null guards.Solution description:
Added null guards using optional chaining (
?.) and conditional expressions across all affected call sites in the topology package:resource?.kindandresource?.metadata?.namespaceinstead of direct accessreferenceFor(resourceObj)andresourceObj.metadata.*with null checksreferenceFor(resourceObj)with null checkreferenceFor(resourceObj),resourceModel.*, andresourceObj.metadata.*referenceFor(this.resource)when resource is undefinedreferenceFor()result ingetResourceKindutilityAll changes are minimal defensive additions — no logic changes, no new behavior when resources are present.
Screenshots / screen recording:
Test setup:
Install a Helm chart from the Developer Catalog (e.g. Edge Manager) and navigate to the Topology view while resources are still initializing.
Test cases:
Browser conformance:
Additional info:
Jira: https://redhat.atlassian.net/browse/OCPBUGS-85013
Reviewers and assignees:
Summary by CodeRabbit