NETOBSERV-428 aggregate topology edges between groups - #1618
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (7)
📝 WalkthroughWalkthroughChangesAggregate topology behavior
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/tabs/netflow-topology/2d/topology-content.tsx (1)
510-517: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate the
startCollapsedcomparison on a present previous options object.
usePreviousreturnsundefinedbefore the first previous value, and the TypeScript config enables strict nulls. UseprevOptions?.startCollapsedconsistently with the sibling comparisons, or reset the previous value whenoptionsitself is absent.🤖 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 510 - 517, Update the comparison in the topology content recomputation condition to use an optional access for prevOptions.startCollapsed, matching the other previous-option comparisons and safely handling the initial undefined value from usePrevious.Source: Pipeline failures
🧹 Nitpick comments (6)
web/src/components/tabs/netflow-topology/2d/topology-content.tsx (1)
375-412: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the
getOptions()result.It is invoked twice (Line 378, Line 402), each time recomputing
Math.max(...)over all metrics. Alsoconst selectedat Line 405 shadows theselectedprop.♻️ Proposed refactor
+ const opts = getOptions(); const updatedModel = generateDataModel( metrics, droppedMetrics, - getOptions(), + opts, @@ - updatedModel.edges = maybeAggregateEdges(updatedModel.nodes, updatedModel.edges, getOptions(), highlightedId, t); + updatedModel.edges = maybeAggregateEdges(updatedModel.nodes, updatedModel.edges, opts, highlightedId, t); // Highlight all selected aggregate path segments (selection can be multi-id). - const selected = selectedIdsRef.current; - if (selected.length > 1) { + const selectedSegmentIds = selectedIdsRef.current; + if (selectedSegmentIds.length > 1) { updatedModel.edges?.forEach(e => { - if (selected.includes(e.id)) { + if (selectedSegmentIds.includes(e.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 `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx` around lines 375 - 412, In the update flow around generateDataModel and maybeAggregateEdges, compute getOptions() once and reuse the resulting options for both calls. Rename the local selected variable used for selectedIdsRef.current to avoid shadowing the selected prop, and update its subsequent references accordingly.web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx (2)
372-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpreading the whole edge
databag as component props is fragile.
{...passedData}forwards every model field (bridgeKey,aggregatedEdgeIds,shadowed,bps, …) intoDefaultEdge, which may pass unknown props down to DOM nodes and will silently break if the model gains a key that collides with aDefaultEdgeprop. Pass the specific props the component needs. Alsodatais implicitlyanyhere — type it.As per coding guidelines: "Avoid 'any' types in TypeScript without justification".
🤖 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 372 - 396, Update the edge rendering logic around DefaultEdge to stop spreading the entire data or passedData object; explicitly pass only the model fields required by DefaultEdge, while preserving the existing explicit props and behavior. Add an appropriate TypeScript type for data based on the edge model instead of allowing it to remain implicitly any, and remove the now-unnecessary filtering/deletion logic.Source: Coding guidelines
355-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire aggregate edge selection through the provided PF handler.
Discarding
_onSelectskipswithSelection’s controller selection handling, including modifier-key behavior and selection events. Include the aggregate edge plus related segment IDs in the selection update that PF forwards; do not mutateSELECTION_STATEdirectly.🤖 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 355 - 366, The handleSelect function should delegate aggregate edge selection to the provided _onSelect/withSelection handler instead of mutating the controller state and firing selectionEvent directly. Pass the aggregate edge ID together with related segment IDs through the PF handler, preserving its modifier-key behavior and selection-event handling.web/src/model/__tests__/topology.spec.ts (1)
113-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared fixtures are mutated across tests, and
toEqual(edges)compares the array to itself.
aggregateByGroupEdgessetsedge.visiblein place, so the module-levelnodes/edgesconsts are mutated by the first test before the skip tests run. And sincemaybeAggregateEdgesreturns the same array reference on the early-return path,expect(result).toEqual(edges)is a tautology. Build fixtures per test and assert identity/content explicitly.♻️ Suggested change
- const nodes: NodeModel[] = [leaf('a1'), leaf('a2'), leaf('b1'), group('ns-a', ['a1', 'a2']), group('ns-b', ['b1'])]; - - const edges: EdgeModel[] = [edge('a1', 'b1', 100), edge('a2', 'b1', 50)]; + const makeNodes = (): NodeModel[] => [ + leaf('a1'), + leaf('a2'), + leaf('b1'), + group('ns-a', ['a1', 'a2']), + group('ns-b', ['b1']) + ]; + const makeEdges = (): EdgeModel[] => [edge('a1', 'b1', 100), edge('a2', 'b1', 50)]; @@ it('skips aggregation when groupEdges is false', () => { + const edges = makeEdges(); const result = maybeAggregateEdges( - nodes, + makeNodes(), edges, { ...DefaultOptions, groupTypes: 'namespaces', groupEdges: false }, '', t ); - expect(result).toEqual(edges); + expect(result).toBe(edges); expect(result.every(e => e.type === 'edge')).toBe(true); });Also applies to: 190-205
🤖 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 - 115, Replace the module-level mutable nodes and edges fixtures in topology.spec.ts with fresh fixtures created per test, ensuring aggregateByGroupEdges cannot leak visible-state mutations between tests. Update the maybeAggregateEdges early-return assertions to verify both that the result is the expected same reference and that its contents match an independently captured expected value, rather than comparing the array to itself.web/src/components/tabs/netflow-topology/2d/componentFactories/stylesComponentFactory.tsx (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
'aggregate-edge'is hardcoded in three places whileAGGREGATE_EDGE_TYPEis exported fromweb/src/model/topology.ts. Any rename of the constant silently breaks rendering, factory routing, and tag clearing.
web/src/components/tabs/netflow-topology/2d/componentFactories/stylesComponentFactory.tsx#L37-L38: importAGGREGATE_EDGE_TYPEand use it in thecase.web/src/components/tabs/netflow-topology/2d/componentFactories/componentFactory.ts#L19-L20: same — replace the literal in thecase.web/src/components/tabs/netflow-topology/2d/topology-content.tsx#L554-L554: comparee.getType()againstAGGREGATE_EDGE_TYPE.🤖 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/componentFactories/stylesComponentFactory.tsx` around lines 37 - 38, Replace the hardcoded aggregate-edge type with the exported AGGREGATE_EDGE_TYPE constant. Update the case in stylesComponentFactory.tsx, the corresponding case in componentFactory.ts, and the e.getType() comparison in topology-content.tsx; import the constant from topology.ts in each file that needs it.web/src/components/tabs/netflow-topology/2d/layouts/layout-edges.ts (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the shared filtering contract with focused tests.
Add cases for hidden edges,
exit/entryversusbridge, unresolved endpoints, self-loops, and bendpoint initialization. This helper now affects every layout using the shared pipeline.Also applies to: 23-45
🤖 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/layout-edges.ts` around lines 11 - 17, The shared edge-filtering pipeline lacks focused coverage for its relevant-edge contract. Add tests for isLayoutRelevantEdge and the associated layout edge-processing flow, covering hidden edges, exit/entry versus bridge roles, unresolved endpoints, self-loops, and bendpoint initialization; preserve the expected filtering and initialization behavior across all layouts using the shared pipeline.
🤖 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-98: Update the topology test around the group selection and
switch interactions to respect the disable rule in the topology display options
component: after selecting group type “none,” assert `#group-edges-switch` is
disabled and do not click it, or move the enabled-path interaction before
selecting “none.” Keep the subsequent disabled-state assertions consistent with
the selected group type.
In
`@web/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsx`:
- Around line 11-16: The locked-tag constants and all related closed-lock
references in the topology connector component must use the public top-level
LockIconConfig fields: svgPath, width, and height. Replace the icon-shaped
property access used by LOCK_PATH, LOCK_ICON_W, LOCK_ICON_H, and the affected
rendering/scaling references, while preserving the existing open-lock scaling
behavior.
In `@web/src/utils/create-aggregate-edges.ts`:
- Around line 306-362: Update the collapse-only aggregation path in
maybeAggregateEdges so the first remapped edge is replaced by its aggregate: add
the newly created aggregate to newEdges (and segmentIndex) in the else branch,
mark the source leaf edge invisible, and remove the unused pendingAggregates
collection. Preserve merging subsequent edges into the existing aggregate while
ensuring a single remapped edge still emits one visible aggregate.
---
Outside diff comments:
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 510-517: Update the comparison in the topology content
recomputation condition to use an optional access for
prevOptions.startCollapsed, matching the other previous-option comparisons and
safely handling the initial undefined value from usePrevious.
---
Nitpick comments:
In
`@web/src/components/tabs/netflow-topology/2d/componentFactories/stylesComponentFactory.tsx`:
- Around line 37-38: Replace the hardcoded aggregate-edge type with the exported
AGGREGATE_EDGE_TYPE constant. Update the case in stylesComponentFactory.tsx, the
corresponding case in componentFactory.ts, and the e.getType() comparison in
topology-content.tsx; import the constant from topology.ts in each file that
needs it.
In `@web/src/components/tabs/netflow-topology/2d/layouts/layout-edges.ts`:
- Around line 11-17: The shared edge-filtering pipeline lacks focused coverage
for its relevant-edge contract. Add tests for isLayoutRelevantEdge and the
associated layout edge-processing flow, covering hidden edges, exit/entry versus
bridge roles, unresolved endpoints, self-loops, and bendpoint initialization;
preserve the expected filtering and initialization behavior across all layouts
using the shared pipeline.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx`:
- Around line 372-396: Update the edge rendering logic around DefaultEdge to
stop spreading the entire data or passedData object; explicitly pass only the
model fields required by DefaultEdge, while preserving the existing explicit
props and behavior. Add an appropriate TypeScript type for data based on the
edge model instead of allowing it to remain implicitly any, and remove the
now-unnecessary filtering/deletion logic.
- Around line 355-366: The handleSelect function should delegate aggregate edge
selection to the provided _onSelect/withSelection handler instead of mutating
the controller state and firing selectionEvent directly. Pass the aggregate edge
ID together with related segment IDs through the PF handler, preserving its
modifier-key behavior and selection-event handling.
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 375-412: In the update flow around generateDataModel and
maybeAggregateEdges, compute getOptions() once and reuse the resulting options
for both calls. Rename the local selected variable used for
selectedIdsRef.current to avoid shadowing the selected prop, and update its
subsequent references accordingly.
In `@web/src/model/__tests__/topology.spec.ts`:
- Around line 113-115: Replace the module-level mutable nodes and edges fixtures
in topology.spec.ts with fresh fixtures created per test, ensuring
aggregateByGroupEdges cannot leak visible-state mutations between tests. Update
the maybeAggregateEdges early-return assertions to verify both that the result
is the expected same reference and that its contents match an independently
captured expected value, rather than comparing the array to itself.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: badf3a2b-9666-4a28-8a68-13b7cbce58f0
📒 Files selected for processing (16)
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/componentFactories/componentFactory.tsweb/src/components/tabs/netflow-topology/2d/componentFactories/stylesComponentFactory.tsxweb/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsxweb/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/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)
| const LOCK_PATH = LockIconConfig.icon.svgPathData; | ||
| const LOCK_ICON_W = LockIconConfig.icon.width; | ||
| const LOCK_ICON_H = LockIconConfig.icon.height; | ||
| const LOCK_SCALE = 0.022; | ||
| /** Match open-lock width to closed lock (PatternFly `LockOpenIcon` viewBox). */ | ||
| const OPEN_LOCK_SCALE = (LOCK_ICON_W * LOCK_SCALE) / LockOpenIconConfig.width; | ||
| const OPEN_LOCK_SCALE = (LOCK_ICON_W * LOCK_SCALE) / LockOpenIconConfig.icon.width; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'package.json' web -d 2 -x jq '.dependencies["`@patternfly/react-icons`"] // empty' {}
rg -n 'LockIconConfig|LockOpenIconConfig' web/src --type=ts --type=tsx -C2Repository: netobserv/netobserv-web-console
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -u
echo "== package versions =="
jq -r '.dependencies["`@patternfly/react-icons`"]' web/package.json
echo
echo "== lock config usages =="
rg -n 'LockIconConfig|LockOpenIconConfig' web/src --glob '*.ts' --glob '*.tsx' -C 2 || true
echo
echo "== relevant file outline/section =="
wc -l web/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsx
sed -n '1,150p' web/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsx
echo
echo "== dependency availability/type info if present =="
if [ -d web/node_modules/@patternfly/react-icons ]; then
echo "node_modules exists"
node - <<'JS'
const path = require('path');
const files = [
'LockIconConfig.js',
'LockOpenIconConfig.js',
'LockIcon.js',
'LockOpenIcon.js'
];
for (const f of files) {
try {
const p = require.resolve(`@patternfly/react-icons/dist/js/icons/${f}`, {paths:['./web/node_modules']});
console.log('resolved', f, '=>', p);
const m = require(`@patternfly/react-icons/dist/js/icons/${f}`);
console.log(f + ' keys=', Object.keys(m));
console.log(f + ' typeof=', typeof m);
if (typeof m === 'object' && m) console.log(JSON.stringify(m, null, 2).slice(0, 2000));
} catch (e) {
console.log('missing/error', f, e.message || e);
}
}
JS
else
echo "node_modules/@patternfly/react-icons absent"
fiRepository: netobserv/netobserv-web-console
Length of output: 10396
🏁 Script executed:
#!/bin/bash
set -u
echo "== lock config usages =="
rg -n 'LockIconConfig|LockOpenIconConfig' web/src --glob '!**/.git/**' -C 2 || true
echo
echo "== relevant file section =="
sed -n '1,150p' web/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsx
echo
echo "== dependency availability/type info if present =="
if [ -d web/node_modules/@patternfly/react-icons ]; then
echo "node_modules exists"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const icons = ['LockIconConfig', 'LockOpenIconConfig', 'LockIcon', 'LockOpenIcon'];
for (const name of icons) {
for (const ext of ['.js', '.d.ts']) {
const p = path.join('web/node_modules/@patternfly/react-icons/dist/js/icons', name + ext);
if (fs.existsSync(p)) {
console.log(`== ${name}${ext} ==`);
console.log(fs.readFileSync(p, 'utf8').slice(0, 1200));
}
}
}
JS
else
echo "node_modules/@patternfly/react-icons absent"
fiRepository: netobserv/netobserv-web-console
Length of output: 10271
🏁 Script executed (no clone):
#!/bin/bash
set -uo pipefail
echo "== npm metadata for `@patternfly/react-icons`@6.4.0 =="
npm view `@patternfly/react-icons`@6.4.0 dist-tags --json
npm view `@patternfly/react-icons`@6.4.0 version --json
npm view `@patternfly/react-icons`@6.4.0 --json \
| jq '{dependencies, peerDependencies, name, version}'
echo
echo "== fetch package tarball and inspect lock icon files =="
tmp="$(mktemp -d)"
curl -Lfo "${tmp}/react-icons-6.4.0.tgz" "https://registry.npmjs.org/@patternfly/react-icons/-/react-icons-6.4.0.tgz"
tar -tzf "${tmp}/react-icons-6.4.0.tgz" | grep -E 'package/(dist|src)/.*lock.*config|package/(dist|src)/.*LockIcon|package/(dist|src)/.*LockOpenIcon' | head -80 || true
echo
echo "== inspect exported config files =="
tar -xzf "${tmp}/react-icons-6.4.0.tgz" -C "${tmp}" || exit 1
for f in \
"${tmp}/package/dist/js/icons/LockIconConfig.js" \
"${tmp}/package/dist/js/icons/LockOpenIconConfig.js" \
"${tmp}/package/types/index.d.ts" \
"${tmp}/package/types/icons/LockIconConfig.d.ts"
do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,220p' "$f"
else
echo "missing $f"
fi
done
echo
echo "== TypeScript type-only runtime probe if compiler is available =="
if command -v tsc >/dev/null 2>&1; then
mkdir -p "${tmp}/probe/node_modules/@patternfly/react-icons/dist/js/icons"
for f in \
"${tmp}/package/dist/js/icons/LockIconConfig.js" \
"${tmp}/package/dist/js/icons/LockOpenIconConfig.js"
do
sed 's/^module\.exports = {/const export = {/' "$f" > "${tmp}/probe/node_modules/@patternfly/react-icons/dist/js/icons/$(basename "$f")"
done
tee "${tmp}/probe/probe.ts" > /dev/null <<'TS'
import * as LockIconConfig from '`@patternfly/react-icons/dist/js/icons/LockIconConfig`';
import * as LockOpenIconConfig from '`@patternfly/react-icons/dist/js/icons/LockOpenIconConfig`';
const LOCK_PATH = (LockIconConfig as any).icon?.svgPathData;
const LOCK_ICON_W = (LockIconConfig as any).icon?.width;
const LOCK_ICON_H = (Lock-iconConfig as any).icon?.height;
const OPEN_LOCK_SCALE = ((LockIconConfig as any).icon?.width * 0.022) / (LockOpenIconConfig as any).icon?.width;
export {};
TS
# Avoid running repository code; only parse/type-check this isolated temp file
tsc --noEmit --strict "${tmp}/probe/probe.ts" --moduleResolution node --module commonjs --esModuleInterop --baseUrl "${tmp}/probe" --paths '{"`@patternfly/react-icons/`*":["node_modules/@patternfly/react-icons/*"]}' 2>&1 || true
fi
Length of output: 4676
🌐 Web query:
@patternfly/react-icons 6.4.0 LockIconConfig svgPath icon.svgPathData width height typings
💡 Result:
In @patternfly/react-icons 6.4.0, icons are generated as React components derived from FontAwesome solid icons [1][2]. There is no publicly exposed LockIconConfig object in the package's public API [3][1][2]. Individual icons, such as LockIcon, are exported as functional components [3][4]. These icon components do not accept direct svgPathData, width, or height props for modification [1][5]. Instead, they are rendered as SVGs that inherently carry their own internal path and viewBox data [1][2]. For custom icon implementation or when needing to define metadata (such as width, height, and svgPathData), the PatternFly icons system uses an internal structure often handled via a customIcons.mjs file during the build process [3][1][2]. If you are working with these icons in a React application: 1. Icon Sizing: You should control the size of icons using the Icon container component from @patternfly/react-core rather than modifying the icon's internal width/height attributes directly [6][5][7]. The Icon component provides standardized sizing props (e.g., size, iconSize) [5][7]. 2. Static Assets: All icons are available as static SVG files in @patternfly/react-icons/dist/static, which include the necessary viewBox, width, and height attributes to ensure visual consistency [1][2]. 3. CSS: If using an icon outside of the PatternFly React-Core component system, the package recommends applying the following styles to maintain alignment: width="1em", height="1em", and vertical-align: -0.125em [2].
Citations:
- 1: https://github.com/patternfly/patternfly-react/blob/main/packages/react-icons/README.md
- 2: https://github.com/patternfly/patternfly-react/tree/main/packages/react-icons
- 3: https://www.npmjs.com/package/@patternfly/react-icons
- 4: https://github.com/patternfly/patternfly-react/wiki/PatternFly-React-icons
- 5: https://www.patternfly.org/components/icon
- 6: https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/components/Icon/examples/Icon.md
- 7: https://pf-react-staging.patternfly.org/components/icon/
Keep locked tags using the original LockIconConfig shape.
@patternfly/react-icons 6.4.0 exports the icon config with top-level svgPath, width, and height; switching these reads to icon.svgPathData/icon.width/icon.height leaves lockPath undefined for the closed lock and breaks scaling. Use the existing PatternFly fields, or avoid depending on a non-public LockIconConfig.icon shape. Applies to lines 11-16, 77-78, 96, 117.
🧰 Tools
🪛 GitHub Actions: pull request checks / 0_Build, lint, test frontend.txt
[error] 11-11: TypeScript (TS2339) in topology-connector-tag.tsx: Property 'icon' does not exist on type '{ name: "LockIcon"; height: 512; width: 448; svgPath: ... }'.
[error] 12-12: TypeScript (TS2339) in topology-connector-tag.tsx: Property 'icon' does not exist on type '{ name: "LockIcon"; height: 512; width: 448; svgPath: ... }'.
[error] 13-13: TypeScript (TS2339) in topology-connector-tag.tsx: Property 'icon' does not exist on type '{ name: "LockIcon"; height: 512; width: 448; svgPath: ... }'.
[error] 14-14: TypeScript (TS2339) in topology-connector-tag.tsx: Property 'icon' does not exist on type '{ name: "LockIcon"; height: 512; width: 448; svgPath: ... }'.
🪛 GitHub Actions: pull request checks / Build, lint, test frontend
[error] 11-14: TypeScript (webpack/ts-loader) error TS2339: Property 'icon' does not exist on type '{ name: "LockIcon"; height: 512; width: 448; svgPath: ... }'.
[error] 16-16: TypeScript (webpack/ts-loader) error TS2339: Property 'icon' does not exist on type '{ name: "LockOpenIcon"; height: 512; width: 576; svgPath: ... }'.
🤖 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/components/topology-connector-tag.tsx`
around lines 11 - 16, The locked-tag constants and all related closed-lock
references in the topology connector component must use the public top-level
LockIconConfig fields: svgPath, width, and height. Replace the icon-shaped
property access used by LOCK_PATH, LOCK_ICON_W, LOCK_ICON_H, and the affected
rendering/scaling references, while preserving the existing open-lock scaling
behavior.
Source: Pipeline failures
…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>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
web/src/model/__tests__/topology.spec.ts (1)
135-138: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert every expected segment role.
This fixture should yield two exits, one bridge, and one entry.
>= 3permits a missing entry or exit regression.Proposed test tightening
- expect(result.filter(e => e.type === AGGREGATE_EDGE_TYPE).length).toBeGreaterThanOrEqual(3); + expect(result.filter(e => e.data?.role === 'exit')).toHaveLength(2); + expect(result.filter(e => e.data?.role === 'entry')).toHaveLength(1);🤖 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 135 - 138, Update the assertions in the topology aggregation test to verify exact segment roles: assert that the result contains exactly two exits, one bridge, and one entry, using the appropriate role/type symbols already defined by the fixture. Replace the aggregate-edge count check so missing entry or exit segments cannot pass.web/src/components/tabs/netflow-topology/2d/topology-content.tsx (2)
417-426: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
isNode(e)instead of theas Nodecast.Matches the
isEdgeguard style used above and avoids an unchecked cast.♻️ Proposed refactor
controller.getElements().forEach(e => { - if (e.getType() === 'group') { + if (e.getType() === 'group' && isNode(e)) { const updatedGroup = updatedModel.nodes?.find(n => n.id === e.getId()); if (updatedGroup) { - updatedGroup.collapsed = (e as Node).isCollapsed(); + updatedGroup.collapsed = e.isCollapsed(); } } });As 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/topology-content.tsx` around lines 417 - 426, Update the element guard in the collapsed-group remapping loop to use the existing isNode(e) type guard instead of casting e to Node before calling isCollapsed(). Preserve the group filtering and updatedGroup lookup behavior unchanged.Source: Path instructions
275-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistent MobX idiom: use
runInActionfor one-off batched mutations. All three sites construct a throwaway action withaction(() => {...})();runInActionexpresses the same intent without allocating a wrapper per call.
web/src/components/tabs/netflow-topology/2d/topology-content.tsx#L275-L297: replaceaction(fn)()inclearAggregateEdgeEndpoints(and the highlight effect around Line 498) withrunInAction(fn).web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx#L240-L265: replaceaction(fn)()inapplySnapPlan(and inhandleSelectaround Line 397) withrunInAction(fn).web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx#L43-L48: replace the inneraction(() => setCollapsedOrig(...))()/action(() => setDimensionsOrig(...))()calls withrunInAction.🤖 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 275 - 297, Replace throwaway action wrappers with runInAction for one-off MobX mutations. Update clearAggregateEdgeEndpoints and the highlight effect in web/src/components/tabs/netflow-topology/2d/topology-content.tsx#L275-L297, applySnapPlan and handleSelect in web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx#L240-L265, and the setCollapsedOrig/setDimensionsOrig calls in web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx#L43-L48; preserve each mutation’s existing behavior.web/src/components/tabs/netflow-topology/2d/layouts/__tests__/layout-edges.spec.ts (1)
49-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntested branch: distinct layout nodes sharing an id.
collectLayoutLinksskips on bothsource === targetandsource.id === target.id. Only the identity case is covered here; the id-equality case (group endpoint resolving to the same leaf) is the one the comment in the implementation calls out.💚 Suggested extra case
it('skips exit/entry, unresolved ends, and self-loops while initializing bendpoints', () => {Add a separate case:
it('skips endpoints resolving to distinct nodes with the same id', () => { const initBendpoints = jest.fn(); const createLink = jest.fn(); const dupA = { id: 's' } as LayoutNode; const dupB = { id: 's' } as LayoutNode; const getLayoutNode = (_n: LayoutNode[], node: Node | null) => (node === sourceNode ? dupA : dupB); const links = collectLayoutLinks( [edgeWith({ role: 'bridge', source: sourceNode, target: targetNode })], [dupA, dupB], getLayoutNode, createLink, initBendpoints ); expect(links).toHaveLength(0); expect(initBendpoints).not.toHaveBeenCalled(); });🤖 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/__tests__/layout-edges.spec.ts` around lines 49 - 62, Add a separate test covering collectLayoutLinks when source and target resolve to distinct layout-node objects with the same id. Use duplicate-id nodes, assert no links are returned, and verify initBendpoints is not called while preserving the existing identity-case coverage.web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx (3)
417-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
data-testattribute to the clickable aggregate edge.This element handles
onSelectbut exposes no test hook, unlike the other topology elements. The new Cypress coverage in this PR will need one to target exit/bridge/entry segments.♻️ Proposed change
<DefaultEdge className={css('netobserv')} + data-test={`aggregate-edge-${role}-${element.getId()}`} element={element}As per path instructions, "Ensure clickable elements (buttons, links, inputs) have data-test attributes for E2E testing."
🤖 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 417 - 431, Add a stable data-test attribute to the clickable DefaultEdge rendered by the aggregate edge component, alongside its existing onSelect handler, so Cypress can target exit, bridge, and entry segments. Use the component’s existing role or aggregate-edge identifiers to distinguish these segments without changing selection behavior.Source: Path instructions
121-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent
catchhides real failures.Both this block and
findRelatedBridgeswallow everything. Since this runs per animation frame, avoid logging unconditionally, but a one-shot/debug-level log would make geometry regressions diagnosable instead of silently degrading to the ellipse fallback.As per coding guidelines, "Handle errors with proper error messages and consistent error handling 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/styles/styleAggregateEdge.tsx` around lines 121 - 166, The SVG path geometry block must stop silently swallowing failures. Update its catch and the corresponding catch in findRelatedBridge to emit a one-shot or debug-level diagnostic with the caught error, while preserving the existing fallback behavior to ellipseOnBounds and avoiding unconditional per-frame logging.Source: Coding guidelines
39-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFall through to
bridgeKeywhen thebridgeIdlookup misses.A stale/removed
bridgeIdcurrently returnsundefinedand silently disables snapping for the stub, even though thebridgeKeyscan below could still resolve it.♻️ Proposed fix
if (bridgeId) { try { - return stub.getController().getEdgeById(bridgeId); + const byId = stub.getController().getEdgeById(bridgeId); + if (byId) { + return byId; + } } catch { - return undefined; + // fall through to bridgeKey lookup } }🤖 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 39 - 49, Update the bridge lookup logic around getEdgeById so a failed bridgeId lookup does not return immediately; catch the miss and continue into the existing bridgeKey resolution path. Preserve the direct edge return when bridgeId resolves successfully and the existing undefined result when no bridgeKey is available.
🤖 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/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx`:
- Around line 244-263: Update the endpoint reconciliation logic around the
visible startFixed/endFixed checks so clearUnset endpoints are cleared before
any early return based on missing plans or unchanged movement. Ensure unset
start/end points are cleared independently even when the other endpoint is fixed
and stable, while preserving movement updates for fixed endpoints and existing
behavior when clearUnset is false.
- Around line 387-401: The handleSelect function bypasses the selection behavior
configured by withSelection, including its onSelect callback and Ctrl/Meta
multi-select options. Update handleSelect to delegate through or proxy the
withSelection selection handler while preserving aggregate related-segment
selection and selectionEvent behavior; only document disabled multi-select if
that behavior is explicitly intentional.
---
Nitpick comments:
In
`@web/src/components/tabs/netflow-topology/2d/layouts/__tests__/layout-edges.spec.ts`:
- Around line 49-62: Add a separate test covering collectLayoutLinks when source
and target resolve to distinct layout-node objects with the same id. Use
duplicate-id nodes, assert no links are returned, and verify initBendpoints is
not called while preserving the existing identity-case coverage.
In `@web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx`:
- Around line 417-431: Add a stable data-test attribute to the clickable
DefaultEdge rendered by the aggregate edge component, alongside its existing
onSelect handler, so Cypress can target exit, bridge, and entry segments. Use
the component’s existing role or aggregate-edge identifiers to distinguish these
segments without changing selection behavior.
- Around line 121-166: The SVG path geometry block must stop silently swallowing
failures. Update its catch and the corresponding catch in findRelatedBridge to
emit a one-shot or debug-level diagnostic with the caught error, while
preserving the existing fallback behavior to ellipseOnBounds and avoiding
unconditional per-frame logging.
- Around line 39-49: Update the bridge lookup logic around getEdgeById so a
failed bridgeId lookup does not return immediately; catch the miss and continue
into the existing bridgeKey resolution path. Preserve the direct edge return
when bridgeId resolves successfully and the existing undefined result when no
bridgeKey is available.
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 417-426: Update the element guard in the collapsed-group remapping
loop to use the existing isNode(e) type guard instead of casting e to Node
before calling isCollapsed(). Preserve the group filtering and updatedGroup
lookup behavior unchanged.
- Around line 275-297: Replace throwaway action wrappers with runInAction for
one-off MobX mutations. Update clearAggregateEdgeEndpoints and the highlight
effect in
web/src/components/tabs/netflow-topology/2d/topology-content.tsx#L275-L297,
applySnapPlan and handleSelect in
web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx#L240-L265,
and the setCollapsedOrig/setDimensionsOrig calls in
web/src/components/tabs/netflow-topology/2d/styles/styleGroup.tsx#L43-L48;
preserve each mutation’s existing behavior.
In `@web/src/model/__tests__/topology.spec.ts`:
- Around line 135-138: Update the assertions in the topology aggregation test to
verify exact segment roles: assert that the result contains exactly two exits,
one bridge, and one entry, using the appropriate role/type symbols already
defined by the fixture. Replace the aggregate-edge count check so missing entry
or exit segments cannot pass.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f5beddc-a749-4f8b-a1b3-119ffd2a625e
📒 Files selected for processing (18)
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/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/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)
🚧 Files skipped from review as they are similar to previous changes (12)
- web/src/components/tabs/netflow-topology/2d/layouts/breadthFirstLayout.ts
- web/src/components/tabs/netflow-topology/2d/componentFactories/componentFactory.ts
- web/locales/en/plugin__netobserv-plugin.json
- web/src/components/tabs/netflow-topology/2d/layouts/layoutFactory.ts
- web/src/components/tabs/netflow-topology/2d/layouts/colaGroupsLayout.ts
- web/src/components/tabs/netflow-topology/2d/componentFactories/stylesComponentFactory.tsx
- web/cypress/e2e/topology/topology.spec.ts
- web/src/components/dropdowns/topology-display-options.tsx
- web/src/components/tabs/netflow-topology/2d/layouts/layout-edges.ts
- web/src/components/tabs/netflow-topology/2d/layouts/baseLayout.ts
- web/src/model/topology.ts
- web/src/utils/create-aggregate-edges.ts
| const handleSelect = (e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
| if (!edge.hasController()) { | ||
| return; | ||
| } | ||
| const relatedIds = getRelatedSegmentIds(edge); | ||
| const ordered = [edge.getId(), ...relatedIds.filter(id => id !== edge.getId())]; | ||
| const state = edge.getController().getState<{ [selectionState]?: string[] }>(); | ||
| const allSelected = ordered.every(id => state[selectionState]?.includes(id)); | ||
| const selectedIds = allSelected ? [] : ordered; | ||
| action(() => { | ||
| state[selectionState] = selectedIds; | ||
| })(); | ||
| edge.getController().fireEvent(selectionEvent, selectedIds); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PatternFly react-topology 6.4 withSelection onSelect SELECTION_STATE controller state
💡 Result:
In PatternFly React Topology (v6.4), selection is managed via the withSelection higher-order component and the SELECTION_EVENT listener, rather than a single onSelect state property [1][2][3]. The recommended approach to handle selection state involves the following steps: 1. Wrap components: Use the withSelection higher-order component when defining your custom component factory to enable selection interactions on nodes or edges [1][3]. 2. Manage selection IDs: Initialize a state variable (e.g., selectedIds) in your parent component to track the IDs of selected elements [2][4]. 3. Listen to events: Register a listener for the SELECTION_EVENT on the Visualization controller to update your selectedIds state whenever the selection changes [1][2][4]. 4. Sync with VisualizationSurface: Pass the selection state to the VisualizationSurface via the state prop [2][4]. This ensures the topology view reflects the current selection state: const [selectedIds, setSelectedIds] = useState<string[]>([]); const controller = useMemo( => { const newController = new Visualization; //... register factories newController.addEventListener(SELECTION_EVENT, setSelectedIds); newController.fromModel(model, false); return newController; }, []); return ( <VisualizationSurface state={{ selectedIds }} /> ); Regarding the Controller's state: The controller implements a WithState interface, which provides getState() and setState(state) methods [5]. However, for selection specifically, the event-driven pattern described above is the standard implementation for keeping React state and the Topology controller synchronized [2][4]. If you need to programmatically update selection, you can access the controller's selection controller via controller.getSelectionController() (available on the controller instance) and use setSelection(ids) [1][3][5].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: https://github.com/patternfly/react-topology/blob/main/README.md
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 4: https://github.com/patternfly/react-topology
- 5: https://github.com/patternfly/react-topology/blob/main/packages/module/src/types.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the affected file and related imports/usages.
printf '--- file line count ---\n'
wc -l web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx
printf '\n--- imports and relevant code ---\n'
sed -n '1,460p' web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx
printf '\n--- occurrences of handleSelect/select event/withSelection in file ---\n'
rg -n "handleSelect|selectionEvent|relatedSegment|withSelection|SELECTION|onSelect|data-test|keyboard" web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx
printf '\n--- package versions for `@patternfly/react-topology` ---\n'
sed -n '1,240p' web/package.jsonRepository: netobserv/netobserv-web-console
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file line count ---'
wc -l web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx
printf '%s\n' ''
printf '%s\n' '--- imports and relevant code ---'
sed -n '1,460p' web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx
printf '%s\n' ''
printf '%s\n' '--- occurrences of handleSelect/select event/withSelection in file ---'
rg -n "handleSelect|selectionEvent|relatedSegment|withSelection|SELECTION|onSelect|data-test|keyboard" web/src/components/tabs/netflow-topology/2d/styles/styleAggregateEdge.tsx
printf '%s\n' ''
printf '%s\n' '--- package versions for `@patternfly/react-topology` ---'
sed -n '1,240p' web/package.jsonRepository: netobserv/netobserv-web-console
Length of output: 20189
🌐 Web query:
PatternFly react-topology 6.4.0 SelectionController setSelection ctrlShiftKey metaKey SELECTION_EVENT onSelect
💡 Result:
In PatternFly React Topology (version 6.4.0), selection is primarily handled through the Visualization controller, which manages the application state and listens for events [1][2]. Regarding your query about SelectionController, setSelection, and keyboard modifiers like ctrlShiftKey or metaKey: 1. Selection Mechanism: The Visualization controller does not have a distinct class named SelectionController. Instead, selection logic is typically managed by the withSelection higher-order component (HOC), which you wrap around your custom components (nodes, edges, etc.) to enable interaction [3][4][5]. 2. SELECTION_EVENT: The SELECTION_EVENT is the core mechanism for tracking changes [3][6]. You register a listener on the Visualization controller using controller.addEventListener(SELECTION_EVENT, callback) [3][1][2]. This callback receives the array of selected element IDs, allowing you to synchronize the selection state with your React component state [1][6]. 3. Modifier Keys (Ctrl/Shift/Meta): While you asked about ctrlShiftKey or metaKey specifically in a setSelection context, the standard withSelection HOC manages these browser-native modifier keys automatically to determine selection behavior (e.g., adding to vs. replacing current selection) [4][5]. The library typically abstracts this from the user; you do not usually need to manually invoke a setSelection method with explicit modifier key flags. 4. onSelect: If you need custom behavior when an element is selected, the onSelect prop is passed to the component wrapped in withSelection [4]. If you are attempting to implement custom multi-selection logic or programmatically force a selection state, you would typically interact with the Visualization controller's state or the underlying Graph model, rather than a non-existent SelectionController class [7]. For most use cases, wrapping your components in withSelection and using the VisualizationSurface with a state object containing selectedIds is the standard approach [1][2][6].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/README.md
- 2: https://github.com/patternfly/react-topology
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 4: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 5: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
- 6: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyGettingStartedDemo.tsx
- 7: https://github.com/patternfly/react-topology/blob/main/packages/module/src/types.ts
🌐 Web query:
site:github.com/patternfly/react-topology withSelection selector ctrlShiftKey getEvent ModifierEvent
💡 Result:
The provided search results do not contain information regarding a specific API, function, or event type named ModifierEvent, nor do they define a getEvent method involving ctrlShiftKey within the context of the withSelection higher-order component from @patternfly/react-topology. The withSelection component is a standard tool in @patternfly/react-topology used to enable selection functionality for graph elements like nodes and edges [1][2]. It is typically applied as a wrapper in a component factory [1][2]. The available documentation confirms its usage for making components selectable but does not document an interface containing ModifierEvent or a getEvent helper for handling specific key combinations like ctrlShiftKey [1][3][2].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
🌐 Web query:
PatternFly react-topology Source withSelection.tsx 6.4.0 onSelect event modifier keys
💡 Result:
In PatternFly React Topology version 6.4.0, the withSelection higher-order component and the underlying useSelection hook are used to manage selection state for topology elements [1][2]. The onSelect callback provided by withSelection is intended to trigger selection actions, but it does not natively accept or expose event modifier keys (such as Shift or Ctrl) as explicit arguments [1][3]. In the library's architecture, the selection logic—including multi-select behavior—is typically handled internally by the Visualization controller when multiSelect: true is configured in the withSelection options [2]. If you need to detect modifier keys during a selection event, you would typically handle this by attaching a standard React onClick or onMouseDown event listener to your custom component (or the component wrapped by withSelection), as these events provide access to the native browser MouseEvent which contains the modifier key information (e.g., event.shiftKey, event.ctrlKey). Key details for 6.4.0: - The withSelection utility accepts an Options object with properties: multiSelect (boolean), controlled (boolean), and raiseOnSelect (boolean) [2]. - The onSelect function provided by withSelection is primarily used to toggle the selection state of the element within the topology controller [1][3]. - Native mouse event data is not passed through the internal onSelect callback [3]. To implement custom modifier-based selection logic, you should manually intercept the interaction before or alongside the onSelect call by adding a standard DOM event listener to the element [3].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: patternfly/patternfly-react@patternfly:9cee687...patternfly:ba617ed
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
🌐 Web query:
repo:patternfly/react-topology "withSelection" "onSelect" "ctrlShiftKey"
💡 Result:
In the @patternfly/react-topology library, the withSelection higher-order component is used to enable selection functionality on graph elements such as nodes and edges [1][2]. Regarding the parameters mentioned: 1. withSelection: This is a higher-order component (HOC) used when defining custom component factories to wrap nodes or edges, making them selectable [1][2]. Usage typically looks like: case ModelKind.node: return withSelection(CustomNode); 2. onSelect: While there is no direct onSelect prop on the withSelection HOC itself, selection in PatternFly Topology is handled primarily through the Visualization controller [1][3]. You listen for selection changes by adding an event listener to the controller using the SELECTION_EVENT constant [1][2][3]. When an element is selected (or deselected), the controller emits this event, allowing you to manage the selected state (e.g., using React state) [1][2][3]. 3. ctrlShiftKey: There is no native or explicit ctrlShiftKey prop or configuration option within the @patternfly/react-topology withSelection HOC or the standard selection event handling. Selection behavior (like multi-select) is generally managed internally by the topology library or through standard browser event interactions on the surface. If you need to implement specific behavior for Ctrl+Shift+Click, you would typically handle this by adding custom event listeners to the graph or individual components, rather than relying on an existing property in withSelection. To manage selection, you should: - Register the withSelection HOC in your component factory [1][2]. - Add a listener for SELECTION_EVENT to your Visualization controller to track which elements are currently selected [1][2][3]. - Pass the selection state to the VisualizationSurface component [3][4].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 3: https://github.com/patternfly/react-topology
- 4: https://github.com/patternfly/react-topology/blob/main/README.md
🌐 Web query:
@patternfly/react-topology 6.4.0 react-selectable.tsx ctrlShiftKey handleOnSelect
💡 Result:
In @patternfly/react-topology 6.4.0, selection behavior is managed through the useSelection hook and the withSelection Higher-Order Component (HOC) [1][2]. While the library provides standardized selection handling, there is no direct public API function named react-selectable.tsx or handleOnSelect specifically exported to handle ctrlShiftKey logic as a standalone feature [1][3][4]. Instead, selection logic is typically controlled by wrapping components with the withSelection HOC [1][2]. This HOC injects the selected state and an onSelect callback into the component [1][2]. The default behavior for multi-selection in PatternFly Topology is handled by the framework internally, but custom selection logic—such as implementing specific ctrlShiftKey behavior—is generally managed at the application level by listening for selection events via the Visualization controller and updating the selection state accordingly [1][3][4]. Key points for implementation: 1. Selection Hook/HOC: Use withSelection to enable selection capabilities on nodes or edges [1][2]. 2. Event Handling: To implement custom multi-select behavior (e.g., ctrlShiftKey), you would register an event listener on the Visualization controller using newController.addEventListener(SELECTION_EVENT, callback) to track and manipulate the array of selected IDs [1][3][4]. 3. Documentation: Detailed examples of how to implement selection, including the use of VisualizationProvider and VisualizationSurface, are available in the official PatternFly Topology documentation and GitHub repository examples [1][3][4]. If you are encountering issues with selection state not updating, ensure you are correctly managing the selectedIds state in the component rendering the VisualizationSurface [1][4].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
- 3: https://github.com/patternfly/react-topology
- 4: https://github.com/patternfly/react-topology/blob/main/README.md
🌐 Web query:
github.com patternfly react-topology packages/module/src/components/topology/useSelection.tsx
💡 Result:
The file useSelection.tsx within the @patternfly/react-topology package is a React hook used to manage selection behavior for elements (such as nodes and edges) within a topology visualization [1][2]. Key details about useSelection: Functionality: It is typically used within custom component implementations to enable and handle selection events. It consumes an ElementContext to access the element's state and interaction hooks [2]. API: The hook accepts an options object as a parameter, which typically includes the following properties [3][2]: - multiSelect: A boolean indicating if multiple elements can be selected. - controlled: A boolean to define if the selection state is controlled externally. - raiseOnSelect: A boolean (defaulting to true) that determines if the selection event should be raised when an element is selected [2]. Usage: Developers commonly use this hook alongside the withSelection higher-order component (HOC) when building custom components for a topology view [4][5][6]. While withSelection wraps a component to provide selection functionality, useSelection provides the underlying logic for managing that selection state within the component's lifecycle [2]. Integration: In a typical PatternFly topology implementation, selection state is managed at the controller level using the SELECTION_EVENT, which allows the application to track and respond to changes in selected elements via state management in the parent component [4][7][8].
Citations:
- 1: patternfly/patternfly-react@fbe4ba8
- 2: patternfly/patternfly-react@patternfly:9cee687...patternfly:ba617ed
- 3: migrate to patternfly/react-topology openshift/console#5985
- 4: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 5: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
- 6: https://context7.com/patternfly/react-topology/llms.txt
- 7: https://github.com/patternfly/react-topology
- 8: https://github.com/patternfly/react-topology/blob/main/README.md
🌐 Web query:
github.com patternfly/react-topology blob 6.4.0 packages/module/src/components/topology/useSelection.tsx function handleOnSelect
💡 Result:
The file packages/module/src/components/topology/useSelection.tsx in @patternfly/react-topology version 6.4.0 defines a hook used for managing selection state within topology components [1]. In this implementation, handleOnSelect is an internal function used to manage the selection logic for elements (nodes, edges, or the graph itself). Its primary responsibility is to interact with the Visualization controller to either select or deselect an element based on user interaction (such as a click) and existing selection state [2][3][4]. Key characteristics of the useSelection hook and handleOnSelect include: 1. Controller Integration: The selection state is typically managed by the Visualization controller, which emits a SELECTION_EVENT when selection changes [2][5][3]. 2. Hook Functionality: The useSelection hook returns a boolean (indicating if the element is selected) and an onSelect function [4]. Components use this onSelect function (which wraps the internal handleOnSelect logic) to handle user click events [4]. 3. Customization: Developers generally do not interact with handleOnSelect directly. Instead, they use the withSelection higher-order component or the useSelection hook to enable selection behavior on their custom components [2][5][4]. For version 6.4.0, the selection mechanism is part of the standard react-topology behavior for enabling interactive nodes and edges [1]. Developers are advised to register the necessary component factories and listen for SELECTION_EVENT on the Visualization controller to update application state (such as an array of selectedIds) in response to selection changes [3][6].
Citations:
- 1: https://github.com/patternfly/react-topology/releases/tag/v6.4.0
- 2: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 3: https://github.com/patternfly/react-topology
- 4: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
- 5: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 6: https://github.com/patternfly/react-topology/blob/main/README.md
🌐 Web query:
github.com patternfly/react-topology v6.4.0 handleOnSelect const ctrlShiftKey onSelect
💡 Result:
In PatternFly React Topology v6.4.0, the onSelect prop is typically used in conjunction with the withSelection higher-order component (HOC) [1][2][3]. There is no standard, built-in constant named ctrlShiftKey or a pre-defined handleOnSelect function that automatically handles this specific keyboard modifier logic directly within the library's public API for components [1][2][4]. When implementing selection logic that requires checking for specific keys (such as Ctrl or Shift), developers must implement this custom logic within their own handler function passed to the onSelect prop [1][2][3]. Example of implementing custom selection logic including modifier keys: const CustomNode = ({ element, onSelect, selected }) => { const handleOnSelect = (event: React.MouseEvent) => { const isCtrlPressed = event.ctrlKey || event.metaKey; const isShiftPressed = event.shiftKey; if (onSelect) { onSelect(isCtrlPressed, isShiftPressed); } }; return ( <div onClick={handleOnSelect} className={selected? 'selected': ''}> {/* Node content */} ); }; In the official PatternFly examples, selection is managed via the Visualization controller, which listens for the SELECTION_EVENT and updates state (e.g., selectedIds) [1][2][4][3]. Modifier key logic is handled at the application level to determine whether to append to or replace the current selection set [1][3].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 4: https://github.com/patternfly/react-topology
🌐 Web query:
@patternfly/react-topology v6.4.0 packages/module/src/components/topology/useSelection.tsx source handleOnSelect
💡 Result:
In @patternfly/react-topology v6.4.0, the useSelection hook, located in the packages/module/src/components/topology/useSelection.tsx source file, is designed to manage the selection state of topology elements [1]. Regarding the handleOnSelect functionality, useSelection returns a tuple containing a boolean (indicating if the element is selected) and an OnSelect callback function [1]. Internally, this hook utilizes the ElementContext to access the current element and maintains its reference [1]. The OnSelect function returned by the hook serves as the handler for selection events on the component [1][2][3]. When building custom components, you can pass this onSelect handler to your component's props to manage selection state [2][3]. For example, when using the withSelection higher-order component, the selection logic is automatically integrated, and the handler is made available to the wrapped component [2][3]. Key characteristics of the hook include: - It accepts an options object as a parameter, which may include properties such as multiSelect, controlled, and raiseOnSelect (which defaults to true) [1]. - It manages the element's selection state within the Topology visualization context [1][2]. - The hook allows for external control over the selection behavior, enabling features like multi-selection [1]. For further implementation details and usage patterns, you can refer to the TopologySelectableDemo and TopologyDragDropDemo examples provided in the package's documentation [2][3].
Citations:
- 1: patternfly/patternfly-react@patternfly:9cee687...patternfly:ba617ed
- 2: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
🌐 Web query:
"@patternfly/react-topology" "handleOnSelect" "multiSelect" "ctrlKey" "metaKey"
💡 Result:
In @patternfly/react-topology, multi-selection behavior—including handling of ctrlKey or metaKey—is primarily managed by the useSelection hook and the withSelection higher-order component [1][2][3]. To enable multi-selection, you configure withSelection (or useSelection) with the multiSelect: true option [2]. When multi-selection is enabled, the underlying topology logic automatically handles standard modifier keys (Ctrl and Meta) to allow users to toggle or append to the current selection [2]. Key Implementation Details: 1. Enabling Multi-Selection: When registering components in your ComponentFactory, wrap your node or edge components with withSelection({ multiSelect: true }) [2][3]. 2. Selection State Management: The topology controller fires a SELECTION_EVENT whenever the selection state changes [1][4][3]. You should listen for this event on your controller to update your application's state (typically a list of selected IDs) [1][5]. 3. Handling Selection in UI: When passing state to the VisualizationSurface, provide the selectedIds array to the state prop [6][5][7]. This ensures the topology view reflects the current selection status: const [selectedIds, setSelectedIds] = useState<string[]>([]); controller.addEventListener(SELECTION_EVENT, setSelectedIds);... <VisualizationSurface state={{ selectedIds }} /> Modifier key behavior (Ctrl/Meta) is baked into the default interaction layer for elements wrapped with withSelection. You do not need to manually implement custom event listeners for these keys unless you are building a custom interaction layer that overrides the standard selection behavior [2].
Citations:
- 1: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologySelectableDemo.tsx
- 2: patternfly/patternfly-react@patternfly:9cee687...patternfly:ba617ed
- 3: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyControlBarDemo.tsx
- 4: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyGettingStartedDemo.tsx
- 5: https://github.com/patternfly/react-topology
- 6: https://github.com/patternfly/react-topology/blob/main/packages/module/patternfly-docs/content/examples/TopologyDragDropDemo.tsx
- 7: https://github.com/patternfly/react-topology/blob/main/README.md
Preserve the topology selection flow for aggregate edges.
handleSelect mutates SELECTION_STATE directly and fires SELECTION_EVENT, but drops the onSelect callback/multi-select options from withSelection. Either call/proxy that handler or document that Ctrl/Meta multi-select is intentionally disabled for related aggregate segments.
🤖 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 387 - 401, The handleSelect function bypasses the selection
behavior configured by withSelection, including its onSelect callback and
Ctrl/Meta multi-select options. Update handleSelect to delegate through or proxy
the withSelection selection handler while preserving aggregate related-segment
selection and selectionEvent behavior; only document disabled multi-select if
that behavior is explicitly intentional.
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>
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-console-plugin:27447076
quay.io/netobserv/network-observability-standalone-frontend:27447076They will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=27447076 make set-plugin-image |
|
@jpinsonneau: 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. |
|
/label qe-approved |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
|
/cherry-pick main-pf5 |
|
@jpinsonneau: new pull request created: #1698 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. |
4 participants
Description
web/src/utils/create-aggregate-edges.ts) because OpenShift Console provides@patternfly/react-topologyas a shared singleton — a custom package version in the plugin image is ignored at runtime until Console ships topology withgroupEdges(patternfly/react-topology#320).StyleAggregateEdgewith nested-group path stepping (touch each hull on the way out/in), LCA-based bridges at the outermost differing groups, and performance-minded snap (coarse outline while layout moves, precise hull refine after settle).Motivation
With groups enabled, dense leaf edges between pods in different namespaces/owners are hard to read. Group-edge aggregation merges parallel flows onto shared bridges while preserving per-connection exit/entry stubs and metric/TLS tags on the bridge.
Implementation notes
pod → owner → namespace → …)aggregate-edgecomponent; stubs/bridges snap to group outlines toward the far peercollectLayoutLinksfiltergroupTypes === none); Cypress coverageFollow-up
create-aggregate-edges.tsonce Console’s shared@patternfly/react-topologyincludesgroupEdgesand nested behavior is aligned (or upstream adopts LCA + nested hops).Test plan
namespaces,namespaces+owners) with Group edges on — bridges merge parallel flows; tags appear on bridgesmake frontend/ unit tests formaybeAggregateEdgesDependencies
n/a
Checklist
Summary by CodeRabbit
New Features
Bug Fixes