NETOBSERV-428: Edges aggregation automation - #1697
Conversation
…licker Clear fixed endpoints and force-resnap only on collapse and layout end, and wrap group collapse mutations in MobX action without deep-importing DefaultGroup (avoids duplicate ElementContext vs Console vendors). Co-authored-by: Cursor <cursoragent@cursor.com>
Clear unset endpoints before early-return in applySnapPlan, fall back from bridgeId to bridgeKey, and mirror Ctrl/Meta multi-select for related segments. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Amoghrd: This pull request references NETOBSERV-428 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 story to target the "5.0.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. |
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe topology now supports grouped aggregate edges with configurable controls, model aggregation, resnapping, layout filtering, aggregate selection, persistence, and Cypress coverage for toggling and collapse behavior. ChangesAggregate topology edges
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (9)
web/src/components/tabs/netflow-topology/2d/topology-content.tsx (3)
521-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe prefix and suffix checks are redundant.
id.includes(highlightedId)already matchesid.startsWith(\${highlightedId}.`)andid.endsWith(`.${highlightedId}`). If the intent is to avoid substring false positives, invert the logic: dropincludes` and keep the two anchored checks.♻️ Anchored matching without the broad `includes`
const leafIds = (data.aggregatedEdgeIds as string[] | undefined) || []; if (!highlighted && leafIds.length) { - highlighted = leafIds.some( - id => - id.includes(highlightedId) || id.startsWith(`${highlightedId}.`) || id.endsWith(`.${highlightedId}`) - ); + highlighted = leafIds.some( + id => id === highlightedId || id.startsWith(`${highlightedId}.`) || id.endsWith(`.${highlightedId}`) + ); }🤖 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 `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx` around lines 521 - 527, Update the highlighted leaf-ID matching in the highlighted check to remove the broad id.includes(highlightedId) condition and retain only the anchored startsWith and endsWith checks, preventing substring false positives.
289-297: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
bumpSnapGenerationre-renders the whole topology subtree on every layout end.
graphLayoutEndEventfires each time a layout completes, including repeated Cola settle events during drag. Each bump sets state, which re-rendersTopologyContentand every consumer ofAggregateEdgeSnapContext.Consider skipping the bump when no aggregate edge exists, or throttling it.
clearAggregateEdgeEndpointsalready walks the element list, so it can return whether it touched anything.As per coding guidelines: "Optimize frontend performance by avoiding unnecessary re-renders and optimizing query patterns".
🤖 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 `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx` around lines 289 - 297, Update clearAggregateEdgeEndpoints and onLayoutEnd so snap generation is bumped only when stale aggregate edge endpoints were actually removed; have clearAggregateEdgeEndpoints return whether it changed anything, and conditionally call bumpSnapGeneration based on that result to avoid unnecessary topology subtree re-renders.Source: Coding guidelines
489-539: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe highlight effect scans every element on each hover event.
selectedIdsis a new array reference on eachsetSelectedIdscall, so the effect re-runs even when the contents are unchanged. Combined withcontroller.getElements(), this is an O(elements) pass per hover and per selection update.Consider deriving a stable key such as
selectedIds.join('|')for the dependency, and building an element index once instead of a full scan.As per coding guidelines: "Optimize frontend performance by avoiding unnecessary re-renders and optimizing query patterns".
🤖 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 `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx` around lines 489 - 539, Optimize the highlight effect around controller.getElements() to avoid rerunning for unchanged selection contents and scanning all elements on every hover. Derive a stable selectedIds key such as selectedIds.join('|') for the effect dependency, and build or reuse an element index so highlighting updates only relevant nodes, groups, and edges while preserving shadow and multi-selection behavior.Source: Coding guidelines
web/src/model/__tests__/topology.spec.ts (1)
123-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a collapsed-group test case.
The suite covers expanded groups only.
maybeAggregateEdgesalways passescollapsedGroups: true, and the collapse path is where endpoints get remapped. Add a case withcollapsed: trueon the group nodes and assert that no aggregate segment references a hidden leaf node. This also pins the behavior discussed inweb/src/utils/create-aggregate-edges.tslines 403-411.🤖 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 `@web/src/model/__tests__/topology.spec.ts` around lines 123 - 141, Add a collapsed-group test alongside the existing maybeAggregateEdges coverage, marking the relevant group nodes as collapsed and invoking the same aggregation path. Assert that every aggregate segment’s endpoints avoid hidden leaf node IDs, covering endpoint remapping while preserving the existing expanded-group expectations.web/cypress/integration-tests/topology_groups.cy.ts (1)
112-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd settle waits and tighten the assertion in both toggle tests.
Two stability problems:
- After
uncheck(), the topology resets the graph and re-runs layout asynchronously.cy.get(...).its('length').then(...)does not retry the.thenbody, soleafCountcan capture the pre-toggle DOM. CalltopologyPage.isViewRendered()after each toggle.should('not.eq', leafCount)passes on any difference, including an increase. The test titles state that group edges "reduce edge count". Assertbe.lessThan.As per path instructions: "Verify E2E test stability, proper waits, and selector resilience".
💚 Proposed fix (apply the same shape to both tests)
// disable group edges and record leaf count cy.get(topologySelectors.groupEdgesToggle).uncheck() + topologyPage.isViewRendered() cy.get('`#drawer` ' + topologySelectors.edge).its('length').then(leafCount => { // enable group edges and verify count changes cy.get(topologySelectors.groupEdgesToggle).check() - cy.get('`#drawer` ' + topologySelectors.edge).its('length').should('not.eq', leafCount) + topologyPage.isViewRendered() + cy.get('`#drawer` ' + topologySelectors.edge).should('have.length.lessThan', leafCount) })🤖 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 `@web/cypress/integration-tests/topology_groups.cy.ts` around lines 112 - 142, Update both group-edge toggle tests around the existing uncheck/check calls: invoke topologyPage.isViewRendered() after each toggle so the graph settles before measuring or asserting edge counts, and replace the final not.eq comparison with a be.lessThan assertion to verify enabling group edges reduces the count.Source: Path instructions
web/src/utils/create-aggregate-edges.ts (2)
178-199: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOpposite-direction leaf edges create duplicate stub geometry.
Stub ids encode direction (
aggregate_exit_a_b_<key>vsaggregate_entry_b_a_<key>). For a leaf paira1→b1andb1→a1, the same node/group hop produces oneexitstub and oneentrystub that overlap visually. The bridge merges correctly, but the stubs do not.Consider keying stubs by the unordered node/group pair and deriving the arrow from the merged
bidirectionalflag. Confirm the intended visual before changing.🤖 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 `@web/src/utils/create-aggregate-edges.ts` around lines 178 - 199, Update the exit/entry stub generation around segments so opposite-direction leaf edges share one stub keyed by the unordered node/group pair rather than direction-specific IDs. Merge matching stubs and derive their arrow direction from the resulting bidirectional flag, while preserving the existing bridge segment behavior and distinct geometry for unrelated pairs.
242-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
leafSourceparameter.createSegmentEdgenever reads it. Remove it and update both call sites.🤖 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 `@web/src/utils/create-aggregate-edges.ts` around lines 242 - 268, Remove the unused leafSource parameter from createSegmentEdge and update both call sites to stop passing that argument, preserving the existing edge construction and labeling behavior.web/src/components/tabs/netflow-topology/2d/layouts/layoutFactory.ts (1)
20-23: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Colaand the default case still use the unfiltered PatternFly layout.
BreadthFirstandColaGroupsnow filter exit/entry stubs and self-loops.Cola,ColaNoForce, and the default branch still useColaLayoutfrom@patternfly/react-topology, so aggregate stub edges keep producing self-loop constraints there.Colais also the default layout. Confirm this is intentional, or extend the samecollectLayoutLinksoverride toColaLayout.Also applies to: 36-37
🤖 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 `@web/src/components/tabs/netflow-topology/2d/layouts/layoutFactory.ts` around lines 20 - 23, Update the Cola and default layout branches in the layout factory to use the filtered collectLayoutLinks behavior already applied to BreadthFirst and ColaGroups, including the ColaNoForce variant. Ensure aggregate stub edges and self-loops are excluded while preserving the existing Cola layout options.web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx (1)
341-354: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRender-path bridge lookup can scan all edges on every render.
findRelatedBridgeruns during render for everyexit/entryedge. WhenbridgeIdis absent, it falls back to a fullgetGraph().getEdges()scan, so the cost is O(E) per edge and O(E²) per frame during Cola ticks. The comment above states that the render path must stay cheap. Cache the resolved bridge per edge, or guarantee thatbridgeIdis always set by the model layer so the fallback never runs on the hot path.🤖 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 `@web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx` around lines 341 - 354, Update the render-path bridge resolution around findRelatedBridge in the hasController exit/entry branch so it cannot scan getGraph().getEdges() on every render. Prefer caching the resolved bridge per edge; alternatively, enforce bridgeId in the model layer and remove or bypass the full-edge fallback, while preserving the existing geoKey construction.
🤖 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 `@web/cypress/integration-tests/topology_edges_labels.cy.ts`:
- Around line 66-77: Update the edge-count assertions in the topology test
around the groupEdgesToggle flow to wait for deterministic aggregate-marker
state before reading counts: after uncheck(), wait for aggregate-edge-bridge,
aggregate-edge-exit, and aggregate-edge-entry elements to be absent; after
check(), wait for them to be present. Then compare the post-render leaf count
with aggregatedCount, preferably asserting the expected direction, and retain
the final aggregate count validation only after the presence gate.
In `@web/cypress/integration-tests/topology_groups.cy.ts`:
- Around line 112-142: Update the topology group edge tests in
web/cypress/integration-tests/topology_groups.cy.ts at lines 112-142 by calling
topologyPage.isViewRendered() after each groupEdgesToggle check() and uncheck(),
and assert the grouped edge count with have.length.lessThan rather than not.eq.
At lines 153-163, call topologyPage.isViewRendered() after each groupToggle
click and require at least one edge with have.length.gte, 1 instead of the
tautological zero-or-more assertion.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx`:
- Around line 39-54: Update useMobxSafeCollapseMutations to avoid direct
assignment to the non-writable prototype method setDimensions. If retaining the
patch, install both setCollapsed and setDimensions wrappers as own properties
via Object.defineProperty, and remove those own properties during cleanup so the
original prototype methods are restored; otherwise wrap the mutations at their
call site.
In `@web/src/utils/create-aggregate-edges.ts`:
- Around line 403-411: The no-segment branch of aggregateByGroupEdges must
preserve collapsed-group remapping: in web/src/utils/create-aggregate-edges.ts
lines 403-411, emit a bridge segment between the remapped source and target and
hide the original leaf when either endpoint changes, matching
aggregateByCollapsedGroups. Add coverage in
web/src/model/__tests__/topology.spec.ts lines 123-141 with sibling group nodes
marked collapsed, asserting that no returned segment references a hidden leaf
node.
---
Nitpick comments:
In `@web/cypress/integration-tests/topology_groups.cy.ts`:
- Around line 112-142: Update both group-edge toggle tests around the existing
uncheck/check calls: invoke topologyPage.isViewRendered() after each toggle so
the graph settles before measuring or asserting edge counts, and replace the
final not.eq comparison with a be.lessThan assertion to verify enabling group
edges reduces the count.
In `@web/src/components/tabs/netflow-topology/2d/layouts/layoutFactory.ts`:
- Around line 20-23: Update the Cola and default layout branches in the layout
factory to use the filtered collectLayoutLinks behavior already applied to
BreadthFirst and ColaGroups, including the ColaNoForce variant. Ensure aggregate
stub edges and self-loops are excluded while preserving the existing Cola layout
options.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx`:
- Around line 341-354: Update the render-path bridge resolution around
findRelatedBridge in the hasController exit/entry branch so it cannot scan
getGraph().getEdges() on every render. Prefer caching the resolved bridge per
edge; alternatively, enforce bridgeId in the model layer and remove or bypass
the full-edge fallback, while preserving the existing geoKey construction.
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 521-527: Update the highlighted leaf-ID matching in the
highlighted check to remove the broad id.includes(highlightedId) condition and
retain only the anchored startsWith and endsWith checks, preventing substring
false positives.
- Around line 289-297: Update clearAggregateEdgeEndpoints and onLayoutEnd so
snap generation is bumped only when stale aggregate edge endpoints were actually
removed; have clearAggregateEdgeEndpoints return whether it changed anything,
and conditionally call bumpSnapGeneration based on that result to avoid
unnecessary topology subtree re-renders.
- Around line 489-539: Optimize the highlight effect around
controller.getElements() to avoid rerunning for unchanged selection contents and
scanning all elements on every hover. Derive a stable selectedIds key such as
selectedIds.join('|') for the effect dependency, and build or reuse an element
index so highlighting updates only relevant nodes, groups, and edges while
preserving shadow and multi-selection behavior.
In `@web/src/model/__tests__/topology.spec.ts`:
- Around line 123-141: Add a collapsed-group test alongside the existing
maybeAggregateEdges coverage, marking the relevant group nodes as collapsed and
invoking the same aggregation path. Assert that every aggregate segment’s
endpoints avoid hidden leaf node IDs, covering endpoint remapping while
preserving the existing expanded-group expectations.
In `@web/src/utils/create-aggregate-edges.ts`:
- Around line 178-199: Update the exit/entry stub generation around segments so
opposite-direction leaf edges share one stub keyed by the unordered node/group
pair rather than direction-specific IDs. Merge matching stubs and derive their
arrow direction from the resulting bidirectional flag, while preserving the
existing bridge segment behavior and distinct geometry for unrelated pairs.
- Around line 242-268: Remove the unused leafSource parameter from
createSegmentEdge and update both call sites to stop passing that argument,
preserving the existing edge construction and labeling behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 711c494c-2261-4127-b3c0-41fc2cf3679e
📒 Files selected for processing (24)
web/cypress/e2e/topology/topology.spec.tsweb/cypress/integration-tests/topology_edges_labels.cy.tsweb/cypress/integration-tests/topology_groups.cy.tsweb/cypress/integration-tests/topology_view.cy.tsweb/cypress/views/netflow-page.tsweb/locales/en/plugin__netobserv-plugin.jsonweb/src/components/dropdowns/topology-display-options.tsxweb/src/components/tabs/netflow-topology/2d/aggregate-edge-snap-context.tsxweb/src/components/tabs/netflow-topology/2d/componentFactories/componentFactory.tsweb/src/components/tabs/netflow-topology/2d/componentFactories/stylesComponentFactory.tsxweb/src/components/tabs/netflow-topology/2d/components/edge.tsxweb/src/components/tabs/netflow-topology/2d/layouts/__tests__/layout-edges.spec.tsweb/src/components/tabs/netflow-topology/2d/layouts/baseLayout.tsweb/src/components/tabs/netflow-topology/2d/layouts/breadthFirstLayout.tsweb/src/components/tabs/netflow-topology/2d/layouts/colaGroupsLayout.tsweb/src/components/tabs/netflow-topology/2d/layouts/layout-edges.tsweb/src/components/tabs/netflow-topology/2d/layouts/layoutFactory.tsweb/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsxweb/src/components/tabs/netflow-topology/2d/styles/styleEdge.tsxweb/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsxweb/src/components/tabs/netflow-topology/2d/topology-content.tsxweb/src/model/__tests__/topology.spec.tsweb/src/model/topology.tsweb/src/utils/create-aggregate-edges.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netobserv/netobserv-operator(manual)netobserv/flowlogs-pipeline(manual)
| cy.get('#drawer ' + topologySelectors.edge).should('have.length', 29) | ||
| cy.get('#drawer ' + topologySelectors.node).should('have.length', 15) |
There was a problem hiding this comment.
I wonder if there is a better way than just counting edges here 🤔
Do you often change those values ?
There was a problem hiding this comment.
No, dont change it often. Very rarely when new features are developed around it and the numbers update.
As far as I remember, I have updated it twice until now from when I started working😅
There was a problem hiding this comment.
Let's keep as is then ! Thx
jpinsonneau
left a comment
jpinsonneau
left a comment
There was a problem hiding this comment.
LGTM, just rebase and ensure all tests pass 😃
|
@jpinsonneau I have 2 questions regarding #1618:
|
The 🐰 didn't produced the same feedback on my PR 😢 I already addressed what I wanted to cover so I guess we should just rebase and see what's produced with your changes only.
Yes, let me open a PR for that 👍 |
|
Okay that means these rabbit review comments are not necessary for your PR right? |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 integration-tests |
|
@Amoghrd: 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. |
|
/cherry-pick main-pf5 |
|
@Amoghrd: new pull request created: #1701 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 kubernetes-sigs/prow repository. |
Wait for demo Loki pods before Ready, type adminCLI exec results, make checkDashboards detach-safe, and wait for eBPF accordion panels. Topology edge counts are covered by Amogh's netobserv#1697 group-edges updates. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Description
Automation for edges groups aggregation automation
Dependencies
#1618
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests