feat: nest HelmRelease inventory children in resource tree - #750
Closed
artemiyokulov wants to merge 1 commit into
Closed
feat: nest HelmRelease inventory children in resource tree#750artemiyokulov wants to merge 1 commit into
artemiyokulov wants to merge 1 commit into
Conversation
artemiyokulov
requested review from
Mirage20,
kaviththiranga,
sameerajayasoma and
stefinie123
as code owners
August 7, 2026 07:26
|
Changeset detected — the following file(s) will be released with this PR: |
📝 WalkthroughWalkthroughChangesHelm resource tree behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant treeLayoutUtils
participant nestHelmReleaseChildren
participant FluxInventoryEntries
participant TreeNodes
treeLayoutUtils->>nestHelmReleaseChildren: invoke after release nodes are appended
nestHelmReleaseChildren->>FluxInventoryEntries: parse inventory entry IDs
FluxInventoryEntries-->>nestHelmReleaseChildren: return resource references
nestHelmReleaseChildren->>TreeNodes: assign matching workload parents
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 2
🧹 Nitpick comments (4)
plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx (4)
149-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isOwnis never read.
fetchEventsForNodesetsisOwn: falsefor every event. The effect then overwrites it at line 211 withsource.id === node.id. No render code reads the field. The helper also cannot know ownership, so setting it here is misleading.Either remove
isOwnfromAggregatedResourceEventand from this helper, or use it in the table, for example to de-emphasize child rows.♻️ Proposed refactor: drop the field from the helper
const resourceRef = formatResourceRef(source); - return fetched.map(event => ({ - ...event, - resourceRef, - isOwn: false, - })); + return fetched.map(event => ({ ...event, resourceRef }));Change the helper return type to
Omit<AggregatedResourceEvent, 'isOwn'>[]and let the caller at line 209 attachisOwn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx` around lines 149 - 159, Remove the misleading isOwn assignment from fetchEventsForNode and change its return type to Omit<AggregatedResourceEvent, 'isOwn'>[]. Keep ownership assignment in the caller’s aggregation logic, where source.id === node.id can determine the correct value.
195-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the
eventSourcesmemo, and derivetruncatedChildren.The effect recomputes
collectEventSourceNodes(node, allNodes)at line 195. TheeventSourcesmemo at line 177 already holds the same value for the same inputs. Two independent computations of the fetch set can diverge later. The hint at line 304 readseventSources.lengthwhile the fetch usessources.
truncatedChildrenis a pure function ofchildrenandeventSources. Derive it withuseMemoinstead of storing it in state and setting it inside the effect. This removes one render pass and one source of stale state.♻️ Proposed refactor
- const [truncatedChildren, setTruncatedChildren] = useState(0); + const truncatedChildren = useMemo( + () => Math.max(0, children.length - (eventSources.length - 1)), + [children.length, eventSources.length], + );try { - const sources = collectEventSourceNodes(node, allNodes); - setTruncatedChildren( - Math.max(0, children.length - (sources.length - 1)), - ); - const batches = await Promise.all( - sources.map(async source => { + eventSources.map(async source => {Then replace
node,allNodes, andchildren.lengthin the dependency array at lines 256-258 witheventSourcesandnode.id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx` around lines 195 - 198, Reuse the existing eventSources memo in the fetch effect instead of recomputing collectEventSourceNodes, and update the fetch logic to use eventSources consistently. Replace truncatedChildren state and its effect-based setter with a useMemo derived from children and eventSources, then update the effect dependencies to use eventSources and node.id rather than node, allNodes, and children.length.
272-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the child display limit, and merge the duplicated hint blocks.
The literal
12appears at lines 272, 300, and 301, and the complementary condition repeats at line 308. If one occurrence changes, the hint text no longer matches the rendered list.The string 'Events below include this resource and its children.' appears at line 305 and line 314. The two
Typographyblocks are mutually exclusive and produce the same element. One block with a conditional prefix covers both cases.♻️ Proposed refactor
Add a constant near
MAX_EVENT_SOURCE_NODESusage:/** Max child resources listed above the events table. */ const MAX_LISTED_CHILDREN = 12;- {children.slice(0, 12).map(child => ( + {children.slice(0, MAX_LISTED_CHILDREN).map(child => (- {(children.length > 12 || truncatedChildren > 0) && ( - <Typography - variant="caption" - color="textSecondary" - className={classes.sectionHint} - > - {children.length > 12 - ? `Showing 12 of ${children.length} children. ` - : ''} - {truncatedChildren > 0 - ? `Events fetched for ${eventSources.length} resources (unhealthy first).` - : 'Events below include this resource and its children.'} - </Typography> - )} - {children.length <= 12 && truncatedChildren === 0 && ( - <Typography - variant="caption" - color="textSecondary" - className={classes.sectionHint} - > - Events below include this resource and its children. - </Typography> - )} + <Typography + variant="caption" + color="textSecondary" + className={classes.sectionHint} + > + {children.length > MAX_LISTED_CHILDREN + ? `Showing ${MAX_LISTED_CHILDREN} of ${children.length} children. ` + : ''} + {truncatedChildren > 0 + ? `Events fetched for ${eventSources.length} resources (unhealthy first).` + : 'Events below include this resource and its children.'} + </Typography>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx` around lines 272 - 316, Extract the child display limit into a named constant near the existing MAX_EVENT_SOURCE_NODES constant, then use it for slicing, truncation checks, and “Showing” text in ResourceEventsTable. Merge the mutually exclusive sectionHint Typography blocks into one, conditionally rendering the “Showing…” prefix while always preserving the shared events description.
200-218: 🚀 Performance & Scalability | 🔵 TrivialConsider limiting fetch concurrency.
This block issues up to
MAX_EVENT_SOURCE_NODES(25) requests at once when the user opens the Events tab on a large parent resource. The browser queues them against the per-host connection limit, and the backend receives a burst per drawer open. Each request also has no explicit timeout, so one slow child request delaysPromise.alland keeps the spinner visible.Two options:
- Run the requests in small batches, for example 5 at a time.
- Render the selected resource events first, then merge child results as they arrive.
The per-source
catchat line 213 correctly keeps one failed child from failing the whole view.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx` around lines 200 - 218, Limit concurrency in the source-fetching block around fetchEventsForNode by processing sources in small batches (for example, five at a time) instead of issuing all requests through one Promise.all. Preserve the per-source catch behavior that returns an empty AggregatedResourceEvent[] and continue aggregating every successful batch result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx`:
- Around line 220-234: Update the merged-event handling in ResourceEventsTable
so timestamps are compared with an explicit NaN-safe fallback for empty or
invalid lastTimestamp values, preserving deterministic ordering. After sorting,
cap the displayed list to 25 events using a MAX_AGGREGATED_EVENTS constant,
while retaining the existing setEvents flow and optionally indicating truncation
in the UI.
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.ts`:
- Around line 15-30: The hasAncestor function in resourceTreeDescendants.ts must
prevent cyclic and repeated ancestor traversal by accepting and propagating a
visited Set<string>, returning false when node.id is already visited. In
resourceTreeDescendants.test.ts, add coverage for mutually referencing nodes
completing without recursion failure and for a diamond-shaped graph where
collectDescendantNodes reports the shared descendant only once; apply the
changes at both specified sites.
---
Nitpick comments:
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsx`:
- Around line 149-159: Remove the misleading isOwn assignment from
fetchEventsForNode and change its return type to Omit<AggregatedResourceEvent,
'isOwn'>[]. Keep ownership assignment in the caller’s aggregation logic, where
source.id === node.id can determine the correct value.
- Around line 195-198: Reuse the existing eventSources memo in the fetch effect
instead of recomputing collectEventSourceNodes, and update the fetch logic to
use eventSources consistently. Replace truncatedChildren state and its
effect-based setter with a useMemo derived from children and eventSources, then
update the effect dependencies to use eventSources and node.id rather than node,
allNodes, and children.length.
- Around line 272-316: Extract the child display limit into a named constant
near the existing MAX_EVENT_SOURCE_NODES constant, then use it for slicing,
truncation checks, and “Showing” text in ResourceEventsTable. Merge the mutually
exclusive sectionHint Typography blocks into one, conditionally rendering the
“Showing…” prefix while always preserving the shared events description.
- Around line 200-218: Limit concurrency in the source-fetching block around
fetchEventsForNode by processing sources in small batches (for example, five at
a time) instead of issuing all requests through one Promise.all. Preserve the
per-source catch behavior that returns an empty AggregatedResourceEvent[] and
continue aggregating every successful batch result.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 164ebbd4-44b6-4415-a322-4a6e68a3764c
📒 Files selected for processing (8)
plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceDetailPanel.test.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceDetailPanel.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceDetailTabs.test.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceDetailTabs.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceTreeView.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.test.tsplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.ts
Comment on lines
+15
to
+30
| export function hasAncestor( | ||
| node: LayoutNode, | ||
| ancestorId: string, | ||
| nodesById: Map<string, LayoutNode>, | ||
| ): boolean { | ||
| for (const parentId of node.parentIds) { | ||
| if (parentId === ancestorId) { | ||
| return true; | ||
| } | ||
| const parent = nodesById.get(parentId); | ||
| if (parent && hasAncestor(parent, ancestorId, nodesById)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded ancestor traversal, and no test coverage for it. hasAncestor walks parentIds recursively with no visited set. A cycle in the node graph causes non-terminating recursion, and a diamond-shaped DAG causes repeated traversal of shared ancestors. The current tests only cover a strict tree, so neither case is detected.
plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.ts#L15-L30: add avisited: Set<string>parameter, returnfalsewhennode.idis already present, and pass the set through the recursive call.plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.test.ts#L67-L73: add two cases — a node pair whoseparentIdsreference each other, which must return without hanging, and a node reachable through two distinct parent paths, which must be reported once bycollectDescendantNodes.
📍 Affects 2 files
plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.ts#L15-L30(this comment)plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.test.ts#L67-L73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/resourceTreeDescendants.ts`
around lines 15 - 30, The hasAncestor function in resourceTreeDescendants.ts
must prevent cyclic and repeated ancestor traversal by accepting and propagating
a visited Set<string>, returning false when node.id is already visited. In
resourceTreeDescendants.test.ts, add coverage for mutually referencing nodes
completing without recursion failure and for a diamond-shaped graph where
collectDescendantNodes reports the shared descendant only once; apply the
changes at both specified sites.
artemiyokulov
force-pushed
the
feat/resource-drawer-child-events
branch
from
August 7, 2026 07:32
2971de5 to
0693e77
Compare
Attach Flux inventory workloads under their HelmRelease so Deployment/Pod appear as separate tree nodes with their own Events tabs, instead of flat siblings under RenderedRelease. Signed-off-by: Artemy Okulov <artemy.okulov@x5.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
artemiyokulov
changed the title
artemiyokulov
force-pushed
the
feat/resource-drawer-child-events
branch
from
August 7, 2026 08:30
0693e77 to
5cd20a4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/helmInventoryNesting.ts`:
- Around line 74-75: Update resourceKey to include the API group in its
generated key, then pass parsed.group and node.group at its two call sites so
resources from different groups remain distinct. Add a test covering
same-namespace, kind, and name resources from different API groups and verify
they do not collide or get incorrectly reparented.
- Around line 87-137: Scope the nesting logic in helmInventoryNesting to the
current RenderedRelease identified by releaseNodeId: filter helmReleases and
candidate nodes to that release before counting or reparenting, so prior
releases cannot affect the single-HelmRelease fallback or resource matching. Add
a regression test covering two rendered releases and verify resources from the
earlier release remain unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 32369387-bf7c-469a-873d-f671d531c806
📒 Files selected for processing (6)
.changeset/resource-tree-helm-children.mdplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/ResourceEventsTable.tsxplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/helmInventoryNesting.test.tsplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/helmInventoryNesting.tsplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/treeLayoutUtils.test.tsplugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/treeLayoutUtils.ts
Comment on lines
+74
to
+75
| function resourceKey(kind: string, name: string, namespace?: string): string { | ||
| return `${namespace ?? ''}/${kind}/${name}`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the API group in the inventory key.
Line 74 discards parsed.group, while each TreeNode also has group. Resources from different API groups can have the same namespace, kind, and name. Line 122 can then reparent both resources under the HelmRelease.
Add group to resourceKey, and pass parsed.group and node.group at the two call sites. Add a collision test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/helmInventoryNesting.ts`
around lines 74 - 75, Update resourceKey to include the API group in its
generated key, then pass parsed.group and node.group at its two call sites so
resources from different groups remain distinct. Add a test covering
same-namespace, kind, and name resources from different API groups and verify
they do not collide or get incorrectly reparented.
Comment on lines
+87
to
+137
| const helmReleases = nodes.filter(n => n.kind === 'HelmRelease'); | ||
| if (helmReleases.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| for (const hr of helmReleases) { | ||
| const inventoryKeys = new Set<string>(); | ||
| const entries = | ||
| ( | ||
| hr.specObject as | ||
| | { status?: { inventory?: { entries?: Array<{ id?: string }> } } } | ||
| | undefined | ||
| )?.status?.inventory?.entries ?? []; | ||
|
|
||
| for (const entry of entries) { | ||
| const parsed = parseFluxHelmInventoryEntryId(String(entry?.id || '')); | ||
| if (!parsed?.kind || !parsed?.name) { | ||
| continue; | ||
| } | ||
| inventoryKeys.add( | ||
| resourceKey(parsed.kind, parsed.name, parsed.namespace), | ||
| ); | ||
| } | ||
|
|
||
| for (const node of nodes) { | ||
| if (node.id === hr.id || node.isRoot || node.kind === 'RenderedRelease') { | ||
| continue; | ||
| } | ||
|
|
||
| const key = resourceKey(node.kind, node.name, node.namespace); | ||
| const listedInInventory = inventoryKeys.has(key); | ||
| const onlyUnderRelease = | ||
| node.parentIds.length === 1 && node.parentIds[0] === releaseNodeId; | ||
| const alreadyUnderHr = node.parentIds.includes(hr.id); | ||
|
|
||
| if (listedInInventory && !alreadyUnderHr) { | ||
| node.parentIds = [hr.id]; | ||
| continue; | ||
| } | ||
|
|
||
| if ( | ||
| onlyUnderRelease && | ||
| helmReleases.length === 1 && | ||
| (node.kind === 'Deployment' || | ||
| node.kind === 'StatefulSet' || | ||
| node.kind === 'DaemonSet' || | ||
| node.kind === 'PersistentVolumeClaim') && | ||
| !alreadyUnderHr | ||
| ) { | ||
| node.parentIds = [hr.id]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope nesting to the current RenderedRelease.
buildTreeNodes calls this helper after each rendered release, but nodes contains nodes from earlier releases. Line 87 counts HelmReleases from every rendered release. This disables the single-HelmRelease fallback when another rendered release has a HelmRelease. It can also reparent an earlier release resource when the current inventory has the same resource key.
Filter both HelmReleases and candidate nodes to releaseNodeId, or pass only the current release node set to this helper. Add a two-rendered-release regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/openchoreo/src/components/Environments/ReleaseDataRenderer/ResourceTreeView/helmInventoryNesting.ts`
around lines 87 - 137, Scope the nesting logic in helmInventoryNesting to the
current RenderedRelease identified by releaseNodeId: filter helmReleases and
candidate nodes to that release before counting or reparenting, so prior
releases cannot affect the single-HelmRelease fallback or resource matching. Add
a regression test covering two rendered releases and verify resources from the
earlier release remain unchanged.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Nest Flux HelmRelease inventory workloads (Deployment / PVC / …) under the HelmRelease in the runtime resource tree.
Previously those children often hung as flat siblings under
RenderedRelease(missingparentRefs), so the HelmRelease looked like a leaf and there was no place to open per-child Events.Behavior
HelmRelease.status.inventory.entriesand re-parent matching tree nodes under that HelmReleaseRenderedRelease, nest it under the HelmReleaseOut of scope / companion
k8sresources/tree(empty inventory and no live objects) needs openchoreo-api expansion (parentRefs + optional Flux label LIST). This PR only fixes nesting for nodes already returned by the tree API.Test plan
helmInventoryNesting.test.ts,treeLayoutUtilsinventory nesting caseSummary by CodeRabbit
New Features
Bug Fixes
Tests