[main-pf5] NETOBSERV-428 aggregate topology edges between groups - #1698
jpinsonneau merged 5 commits into
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe topology now supports optional grouped edge aggregation across collapsed groups. It adds aggregate segment generation, model metadata, layout filtering, snapping and selection rendering, collapse resynchronization, in-place highlighting, localized controls, and automated coverage. ChangesAggregate topology edges
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Hi @openshift-cherrypick-robot. Thanks for your PR. I'm waiting for a netobserv 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
New images: quay.io/netobserv/network-observability-console-plugin:a75ca48c
quay.io/netobserv/network-observability-standalone-frontend:a75ca48cThey will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=a75ca48c make set-plugin-image |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
web/cypress/e2e/topology/topology.spec.ts (1)
88-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the dedicated
data-testselector.The component adds
data-test="group-edges-switch"at Line 190, but the E2E test queries#group-edges-switch. Use the dedicated selector to reduce coupling to the rendered input id.As per path instructions,
web/cypress/**/*.tsmust verify selector resilience.🤖 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/e2e/topology/topology.spec.ts` around lines 88 - 100, Update the topology E2E test queries for the group-edges control to use its dedicated data-test selector, matching the component’s group-edges-switch hook, instead of the rendered `#group-edges-switch` ID. Apply this to every reference to that control while preserving the existing assertions and interactions.Source: Path instructions
web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx (1)
33-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this hook to
web/src/utils.
useMobxSafeCollapseMutationsis reusable component logic. Place it in a focused hook file, for exampleweb/src/utils/mobx-collapse-hook.ts, and import it here.As per coding guidelines: "Extract custom React component logic into focused hooks in web/src/utils/*-hook.ts".
🤖 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/styleGroup.tsx` around lines 33 - 54, Move useMobxSafeCollapseMutations into a focused hook file under web/src/utils, such as mobx-collapse-hook.ts, preserving its existing MobX wrapping and cleanup behavior. Export the hook from that file and update the netflow topology styleGroup module to import and use it instead of defining it locally.Source: Coding guidelines
web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx (1)
341-354: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid the graph-wide bridge scan during render.
geoKeycallsfindRelatedBridgeon every render forexitandentryroles. WhenbridgeIdresolution fails, that helper iteratesgetGraph().getEdges()for each segment. During a Cola tick every edge re-renders per frame, so the cost becomes O(segments × edges) per frame. Cache the resolved bridge in a ref keyed bybridgeId/bridgeKey, or build one bridge index per graph revision.Per path instructions: "Check React hooks dependencies, component re-render optimization, and TypeScript type safety."
🤖 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 logic surrounding geoKey and findRelatedBridge to avoid scanning all graph edges on every render. Cache the resolved bridge by bridgeId/bridgeKey in a React ref, or maintain an equivalent bridge index scoped to the current graph revision, and reuse it when constructing geoKey for entry and exit roles. Ensure hook dependencies remain correct and preserve TypeScript-safe handling when no bridge resolves.Source: Path instructions
web/src/components/tabs/netflow-topology/2d/layouts/breadthFirstLayout.ts (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the pass-through constructor.
The constructor only forwards to
super. TypeScript inherits the base signature when the constructor is absent. The same pattern exists incolaGroupsLayout.ts.🤖 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/breadthFirstLayout.ts` around lines 16 - 18, Remove the redundant pass-through constructor from PfBreadthFirstLayout so the class inherits the constructor signature from its base class. Apply the same cleanup to the corresponding constructor in colaGroupsLayout.ts.web/src/utils/create-aggregate-edges.ts (2)
242-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
leafSourceparameter fromcreateSegmentEdge.
createSegmentEdgeacceptsleafSourcebut never reads it. It hardcodesbidirectional: false.mergeSegmentusesleafSource, so the asymmetry is easy to misread as a bug. Drop the parameter, or add a comment that the first leaf always defines the segment direction.Also applies to: 270-277
🤖 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 - 250, Remove the unused leafSource parameter from createSegmentEdge and update every call site, including mergeSegment, to match the revised signature. Keep leafSource handling in mergeSegment unchanged and preserve createSegmentEdge’s existing hardcoded bidirectional behavior.
204-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
bridgeIdfromsegmentIdinstead of duplicating the format string.
segmentIdbuilds the bridge id asaggregate_bridge_${segment.bridgeKey}at Line 209.createSegmentEdgerebuilds the same string at Line 260. If the bridge id scheme changes in one place, stubbridgeIdvalues point at a non-existent edge and runtime bridge resolution falls back silently.♻️ Proposed fix
+const bridgeSegmentId = (bridgeKey: string): string => `aggregate_bridge_${bridgeKey}`; + const segmentId = (segment: PathSegment, legacyBridgeId = false): string => { if (segment.role === 'bridge') { if (legacyBridgeId) { return `aggregate_${segment.source}_${segment.target}`; } - return `aggregate_bridge_${segment.bridgeKey}`; + return bridgeSegmentId(segment.bridgeKey); }// O(1) bridge lookup for exit/entry stub snapping (avoids scanning all graph edges). - ...(segment.role !== 'bridge' ? { bridgeId: `aggregate_bridge_${segment.bridgeKey}` } : {}), + ...(segment.role !== 'bridge' ? { bridgeId: bridgeSegmentId(segment.bridgeKey) } : {}),Also applies to: 256-265
🤖 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 204 - 213, Update createSegmentEdge to derive the stub bridgeId by calling segmentId with the current segment and non-legacy mode, instead of duplicating the aggregate_bridge format string. Keep the existing edge creation behavior unchanged while ensuring bridge references use the same identifier generated by segmentId.web/src/model/__tests__/topology.spec.ts (1)
113-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for collapsed groups and bidirectional bridges.
The fixtures never set
collapsed: true, so the collapse remap branch ofcreateAggregateEdgesis untested here. The fixtures also only use one direction (a1→b1,a2→b1), somergeSegmentnever setsbidirectional: true. Two extra cases would lock both behaviors:
- a collapsed
ns-awith an edge froma1tob1, asserting the bridge ends atns-a;- edges
a1→b1andb1→a2, assertingbridges[0].data.bidirectional === true.I can write these tests if you want.
🤖 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 113 - 141, Extend the topology aggregation tests around baseNodes/baseEdges and maybeAggregateEdges to cover both missing branches: add a case with ns-a collapsed and an a1→b1 edge, asserting the aggregate bridge terminates at ns-a, and add a bidirectional case with a1→b1 plus b1→a2, asserting the resulting bridge data has bidirectional set to true.
🤖 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/e2e/topology/topology.spec.ts`:
- Around line 92-100: The topology test currently verifies group-edges disabling
only with groupTypes set to none. Add a separate assertion path that selects a
non-none group type, disables edges, and verifies `#group-edges-switch` is
disabled, while retaining the existing none-group assertion independently.
- Around line 87-94: Update the toggle-switch flow in the topology test to
assert that `#group-edges-switch` starts checked, verify it becomes unchecked
after the click, and click it back on before collapsing groups. Preserve the
existing disabled-state assertions after selecting “none” so the test explicitly
covers the enabled group-edges path.
In `@web/src/components/tabs/netflow-topology/2d/layouts/layoutFactory.ts`:
- Around line 12-13: Update ConcentricLayout, DagreLayout, DagreGroupsLayout,
ForceLayout, and GridLayout to filter aggregate stub edges from their layout
links, matching the existing filtering behavior. Leave Cola and ColaNoForce
unchanged because they already inherit it from the vendored BaseLayout.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx`:
- Around line 42-58: Update the bridge resolution logic around getEdgeById so it
only returns when an edge is actually found; if the lookup returns undefined,
continue to the existing bridgeKey resolution path. Preserve the current error
handling and fallback behavior for thrown lookup errors.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx`:
- Around line 39-54: Remove the method reassignment logic from
useMobxSafeCollapseMutations, especially the setDimensions and setCollapsed
overrides, because MobX-managed methods must not be overwritten. Use the
supported collapse handler API to wrap the required mutations instead; if shared
patching is unavoidable, implement reference-counted ownership with cleanup
rather than a boolean marker.
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 512-528: Update highlighting in topology-content.tsx and the
corresponding logic in topology.ts to use exact peer, source, and target ID
equality rather than includes or delimiter-based prefix/suffix matching. For
aggregated edges, retain or reuse structured leaf endpoint data instead of
splitting aggregatedEdgeIds on '.', and mark highlights only when an exact
endpoint/peer ID matches highlightedId.
---
Nitpick comments:
In `@web/cypress/e2e/topology/topology.spec.ts`:
- Around line 88-100: Update the topology E2E test queries for the group-edges
control to use its dedicated data-test selector, matching the component’s
group-edges-switch hook, instead of the rendered `#group-edges-switch` ID. Apply
this to every reference to that control while preserving the existing assertions
and interactions.
In `@web/src/components/tabs/netflow-topology/2d/layouts/breadthFirstLayout.ts`:
- Around line 16-18: Remove the redundant pass-through constructor from
PfBreadthFirstLayout so the class inherits the constructor signature from its
base class. Apply the same cleanup to the corresponding constructor in
colaGroupsLayout.ts.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx`:
- Around line 341-354: Update the render logic surrounding geoKey and
findRelatedBridge to avoid scanning all graph edges on every render. Cache the
resolved bridge by bridgeId/bridgeKey in a React ref, or maintain an equivalent
bridge index scoped to the current graph revision, and reuse it when
constructing geoKey for entry and exit roles. Ensure hook dependencies remain
correct and preserve TypeScript-safe handling when no bridge resolves.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx`:
- Around line 33-54: Move useMobxSafeCollapseMutations into a focused hook file
under web/src/utils, such as mobx-collapse-hook.ts, preserving its existing MobX
wrapping and cleanup behavior. Export the hook from that file and update the
netflow topology styleGroup module to import and use it instead of defining it
locally.
In `@web/src/model/__tests__/topology.spec.ts`:
- Around line 113-141: Extend the topology aggregation tests around
baseNodes/baseEdges and maybeAggregateEdges to cover both missing branches: add
a case with ns-a collapsed and an a1→b1 edge, asserting the aggregate bridge
terminates at ns-a, and add a bidirectional case with a1→b1 plus b1→a2,
asserting the resulting bridge data has bidirectional set to true.
In `@web/src/utils/create-aggregate-edges.ts`:
- Around line 242-250: Remove the unused leafSource parameter from
createSegmentEdge and update every call site, including mergeSegment, to match
the revised signature. Keep leafSource handling in mergeSegment unchanged and
preserve createSegmentEdge’s existing hardcoded bidirectional behavior.
- Around line 204-213: Update createSegmentEdge to derive the stub bridgeId by
calling segmentId with the current segment and non-legacy mode, instead of
duplicating the aggregate_bridge format string. Keep the existing edge creation
behavior unchanged while ensuring bridge references use the same identifier
generated by segmentId.
🪄 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: d24c2f7e-e7eb-4b35-9965-4ea10736df16
📒 Files selected for processing (20)
web/cypress/e2e/topology/topology.spec.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)
|
[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 |
github-actions
Bot
removed
the
ok-to-test
Ignoring rabbit feedback as those were not triggered in main branch and not mandatory here |
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-console-plugin:f13f7718
quay.io/netobserv/network-observability-standalone-frontend:f13f7718They will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=f13f7718 make set-plugin-image |
|
@openshift-cherrypick-robot: The following tests 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. |
|
/label qe-approved |
3 participants
This is an automated cherry-pick of #1618
/assign jpinsonneau
Summary by CodeRabbit
New Features
Bug Fixes
Tests