CONSOLE-5103: Add networking extensions to Node views#235
Open
jeff-phillips-18 wants to merge 1 commit into
Open
CONSOLE-5103: Add networking extensions to Node views#235jeff-phillips-18 wants to merge 1 commit into
jeff-phillips-18 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-phillips-18: This pull request references CONSOLE-5103 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jeff-phillips-18 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 |
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds a node network topology sub-navigation feature with a policies panel, extracts a reusable ControlBar component, exports an enactment correlation utility, adds a formatting helper for policy interface affects, normalizes import paths across topology components, and updates locale files, a forbidden-status constant, and jest config. ChangesNode Topology Sub-Nav Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant NodeTopologyPage
participant NodeTopology
participant K8sAPI as useK8sWatchResource
participant NodeNetworkingPoliciesPanel
User->>NodeTopologyPage: Open Network sub-nav tab
NodeTopologyPage->>NodeTopology: Render with visualization controller
NodeTopology->>K8sAPI: Watch node network state and enactments
K8sAPI-->>NodeTopology: Return state/enactments or forbidden error
alt Forbidden
NodeTopology-->>User: Render AccessDenied view
else Loaded
NodeTopology->>NodeTopology: transformDataToTopologyModel + controller.fromModel
NodeTopology-->>User: Render TopologyView with sidebar/toolbar/ControlBar
end
NodeTopologyPage->>NodeNetworkingPoliciesPanel: Render with node
NodeNetworkingPoliciesPanel->>K8sAPI: Watch NodeNetworkConfigurationPolicy list
K8sAPI-->>NodeNetworkingPoliciesPanel: Return policies
NodeNetworkingPoliciesPanel->>NodeNetworkingPoliciesPanel: Filter/sort policies applied to node
NodeNetworkingPoliciesPanel-->>User: Render policy table or empty state
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/nodenetworkconfiguration/manifest.ts (1)
1-2: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winImport the sub-nav-tab type before casting.
EncodedExtension<NodeSubNavTab>uses an undeclared type here, which will fail TypeScript compilation. Import the matching SDK type or drop the generic if it isn’t needed.🤖 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 `@src/views/nodenetworkconfiguration/manifest.ts` around lines 1 - 2, The manifest typing uses NodeSubNavTab in the EncodedExtension cast without declaring that type, so TypeScript will fail. Update the imports in manifest.ts to bring in the matching SDK type used by the sub-nav tab, or remove the generic from EncodedExtension if it is unnecessary. Use the existing HrefNavItem, RoutePage, and EncodedExtension imports as the reference point when fixing the cast.
🧹 Nitpick comments (5)
src/views/nodenetworkconfiguration/utils/utils.ts (1)
220-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn type doesn't account for
undefined.
Array.prototype.findcan returnundefined, and the tests confirm this happens (toBeUndefined()), but the declared return type isV1beta1NodeNetworkConfigurationEnactmentonly. Now that this is exported as a public helper, consumers relying on the type signature could be misled into skipping undefined checks.As per coding guidelines, "Type safety —
strictisfalsein tsconfig, but new code should use explicit types."♻️ Proposed fix
export const getCorrelatedEnactment = ( availableEnhancments: V1beta1NodeNetworkConfigurationEnactment[], nnsName: string, -): V1beta1NodeNetworkConfigurationEnactment => { +): V1beta1NodeNetworkConfigurationEnactment | undefined => { return availableEnhancments?.find((nnce) => nnce.metadata?.ownerReferences?.some((ref) => ref.name === nnsName), ); };🤖 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 `@src/views/nodenetworkconfiguration/utils/utils.ts` around lines 220 - 227, The exported helper getCorrelatedEnactment currently returns the result of Array.prototype.find, which can be undefined, but its signature only declares V1beta1NodeNetworkConfigurationEnactment. Update the return type to explicitly include undefined so consumers of getCorrelatedEnactment handle the missing-match case correctly, and keep the implementation as-is since the find call already reflects that behavior.Source: Coding guidelines
src/views/nodenetworkconfiguration/NodeTopologyPage.tsx (1)
40-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSelection navigation pushes a new history entry on every click.
navigate({ pathname, search })defaults to push navigation, so every node selection in the topology adds a browser history entry. For a page centered on clicking through many nodes, this will make the back button step through selections instead of leaving the page, degrading UX.♻️ Suggested fix
visualization.addEventListener(SELECTION_EVENT, (id) => { const newParams = new URLSearchParams({ [SELECTED_ID_QUERY_PARAM]: id }); - navigate({ pathname: pathnameRef.current, search: newParams.toString() }); + navigate({ pathname: pathnameRef.current, search: newParams.toString() }, { replace: true }); });🤖 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 `@src/views/nodenetworkconfiguration/NodeTopologyPage.tsx` around lines 40 - 43, The selection handler in NodeTopologyPage’s visualization.addEventListener callback is using navigate with the default push behavior, which adds a history entry for every node click. Update the navigate call to use replace behavior when syncing the SELECTED_ID_QUERY_PARAM so node selections don’t pollute browser history; keep the same pathnameRef.current and URLSearchParams flow, but ensure repeated selections overwrite the current entry instead of pushing a new one.src/views/nodenetworkconfiguration/components/TopologyControlBar/TopologyControlBar.tsx (2)
15-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
controlButtonsto avoid rebuilding callbacks on every render.
createTopologyControlButtons(...)runs on every render ofControlBar, recreating all four callbacks even whencontrollerhasn't changed. Consider wrapping inuseMemokeyed oncontroller.♻️ Proposed refactor
-const ControlBar: FC<ControlBarProps> = ({ controller }) => { +const ControlBar: FC<ControlBarProps> = ({ controller }) => { + const controlButtons = useMemo(() => { + if (!controller) return null; + return createTopologyControlButtons({ + ...defaultControlButtonsOptions, + zoomInCallback: action(() => { + const scale = controller.getGraph().getScale(); + controller.getGraph().setScale(scale * 1.1); + }), + zoomOutCallback: action(() => { + const scale = controller.getGraph().getScale(); + controller.getGraph().setScale(scale * 0.9); + }), + fitToScreenCallback: action(() => { + controller.getGraph().fit(40); + }), + resetViewCallback: action(() => { + controller.getGraph().reset(); + controller.getGraph().layout(); + }), + }); + }, [controller]); + if (!controller) { return null; } - return ( - <TopologyControlBar - controlButtons={createTopologyControlButtons({ - ...defaultControlButtonsOptions, - zoomInCallback: action(() => { - const scale = controller.getGraph().getScale(); - controller.getGraph().setScale(scale * 1.1); - }), - zoomOutCallback: action(() => { - const scale = controller.getGraph().getScale(); - controller.getGraph().setScale(scale * 0.9); - }), - fitToScreenCallback: action(() => { - controller.getGraph().fit(40); - }), - resetViewCallback: action(() => { - controller.getGraph().reset(); - controller.getGraph().layout(); - }), - })} - /> - ); + return <TopologyControlBar controlButtons={controlButtons} />; };🤖 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 `@src/views/nodenetworkconfiguration/components/TopologyControlBar/TopologyControlBar.tsx` around lines 15 - 41, The ControlBar component recreates controlButtons and its action-wrapped callbacks on every render even when controller is unchanged. Update ControlBar in TopologyControlBar.tsx to memoize the createTopologyControlButtons call with useMemo, using controller as the dependency, so the zoomInCallback, zoomOutCallback, fitToScreenCallback, and resetViewCallback are not rebuilt unnecessarily.
1-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFile name doesn't match exported component.
File is
TopologyControlBar.tsxbut the default export isControlBar. Consider renaming the file toControlBar.tsx(or the component toTopologyControlBar) to match the convention. As per coding guidelines,File & directory namingspecifiesReact components: PascalCase.tsx, implying filename should match the component name.🤖 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 `@src/views/nodenetworkconfiguration/components/TopologyControlBar/TopologyControlBar.tsx` around lines 1 - 44, The exported React component name and filename are inconsistent: this module default-exports ControlBar, but the file is named TopologyControlBar.tsx. Rename either the file to match ControlBar or the component to TopologyControlBar so the default export, component identifier, and filename are aligned with the naming convention used in the TopologyControlBar/ControlBar component.Source: Coding guidelines
src/views/nodenetworkconfiguration/NodeTopology.tsx (1)
42-127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a post-mount re-fit effect like
Topology.tsx.
Topology.tsxhas a secondary effect usingrequestAnimationFrameto re-runfit(40)after mount, to correct scale once the container size stabilizes (e.g., navigating from another flow).NodeTopology.tsxomits this, so the initial fit may look off in the same scenario. Self-recoverable via the zoom controls, but worth aligning for consistency.🤖 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 `@src/views/nodenetworkconfiguration/NodeTopology.tsx` around lines 42 - 127, Add the missing post-mount re-fit behavior to NodeTopology so the graph can correct its scale after the container stabilizes, matching the pattern used in Topology.tsx. In NodeTopology, introduce a secondary effect that runs after the initial render/initialization, uses requestAnimationFrame to call controller.getGraph().fit(40), and cleans up the frame handle on unmount. Keep the existing modelInitialized logic in NodeTopology and make sure the new effect is gated on controller and the topology being ready.
🤖 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
`@src/views/nodenetworkconfiguration/components/NodeNetworkingPoliciesPanel/utils/formatPolicyAffectsDisplay.ts`:
- Around line 20-39: The bridge formatting logic in formatBridgeInterfaceAffects
only returns the first nested bond’s ports and drops any other bridge ports, so
update it to aggregate all bridge ports instead of using ports.find(...). Keep
the existing name resolution in resolveBondPortName, but combine plain port
names and nested LINK_AGGREGATION port names into one list for iface.name, then
return the full joined set; also update the related
formatPolicyAffectsDisplay.test.ts expectation to include eth0 alongside the
bonded ports.
In `@src/views/nodenetworkconfiguration/components/TopologySidebar/Drawer.tsx`:
- Around line 20-33: The conditional rendering in the Drawer component is
ordered incorrectly, causing a selected policy to fall through to
StateDetailsPage when its interface is not yet present. Update the logic in
Drawer (using selectedPolicy, selectedInterface, and selectedState from
useSelectedResources) so the selectedPolicy branch is checked before the
selectedState branch, ensuring PolicyDrawer renders for pending policies.
In `@src/views/nodenetworkconfiguration/NodeTopology.tsx`:
- Around line 76-96: The initial-fit logic in NodeTopology currently uses
modelInitialized as a one-time gate, so switching to a different
obj.metadata.name in the same tab skips restoreNodePositions and
controller.getGraph().fit(40) for the new node. Update the effect around
transformDataToTopologyModel and controller.fromModel to track the last rendered
node name instead of only a boolean ref, and reset the fit/restore gate whenever
obj.metadata.name changes so each node gets its own initial framing.
- Around line 91-95: The mismatch is in the restoreNodePositions helper
signature: useVisualizationController() returns Controller, but
restoreNodePositions currently expects Visualization even though it only calls
getGraph(). Update restoreNodePositions in NodeTopology.tsx to accept Controller
instead of Visualization, and keep the call site in the modelInitialized.current
block unchanged so the types align.
In `@src/views/nodenetworkconfiguration/NodeTopologyPage.tsx`:
- Around line 40-43: The SELECTION_EVENT handler in NodeTopologyPage is writing
the raw selected value into the query string, but `@patternfly/react-topology`
provides a string array. Update the
visualization.addEventListener(SELECTION_EVENT, ...) callback to extract the
active ID from the array, and when the selection is empty remove
SELECTED_ID_QUERY_PARAM instead of serializing the array directly. Keep the
change localized around navigate, pathnameRef, and SELECTED_ID_QUERY_PARAM.
---
Outside diff comments:
In `@src/views/nodenetworkconfiguration/manifest.ts`:
- Around line 1-2: The manifest typing uses NodeSubNavTab in the
EncodedExtension cast without declaring that type, so TypeScript will fail.
Update the imports in manifest.ts to bring in the matching SDK type used by the
sub-nav tab, or remove the generic from EncodedExtension if it is unnecessary.
Use the existing HrefNavItem, RoutePage, and EncodedExtension imports as the
reference point when fixing the cast.
---
Nitpick comments:
In
`@src/views/nodenetworkconfiguration/components/TopologyControlBar/TopologyControlBar.tsx`:
- Around line 15-41: The ControlBar component recreates controlButtons and its
action-wrapped callbacks on every render even when controller is unchanged.
Update ControlBar in TopologyControlBar.tsx to memoize the
createTopologyControlButtons call with useMemo, using controller as the
dependency, so the zoomInCallback, zoomOutCallback, fitToScreenCallback, and
resetViewCallback are not rebuilt unnecessarily.
- Around line 1-44: The exported React component name and filename are
inconsistent: this module default-exports ControlBar, but the file is named
TopologyControlBar.tsx. Rename either the file to match ControlBar or the
component to TopologyControlBar so the default export, component identifier, and
filename are aligned with the naming convention used in the
TopologyControlBar/ControlBar component.
In `@src/views/nodenetworkconfiguration/NodeTopology.tsx`:
- Around line 42-127: Add the missing post-mount re-fit behavior to NodeTopology
so the graph can correct its scale after the container stabilizes, matching the
pattern used in Topology.tsx. In NodeTopology, introduce a secondary effect that
runs after the initial render/initialization, uses requestAnimationFrame to call
controller.getGraph().fit(40), and cleans up the frame handle on unmount. Keep
the existing modelInitialized logic in NodeTopology and make sure the new effect
is gated on controller and the topology being ready.
In `@src/views/nodenetworkconfiguration/NodeTopologyPage.tsx`:
- Around line 40-43: The selection handler in NodeTopologyPage’s
visualization.addEventListener callback is using navigate with the default push
behavior, which adds a history entry for every node click. Update the navigate
call to use replace behavior when syncing the SELECTED_ID_QUERY_PARAM so node
selections don’t pollute browser history; keep the same pathnameRef.current and
URLSearchParams flow, but ensure repeated selections overwrite the current entry
instead of pushing a new one.
In `@src/views/nodenetworkconfiguration/utils/utils.ts`:
- Around line 220-227: The exported helper getCorrelatedEnactment currently
returns the result of Array.prototype.find, which can be undefined, but its
signature only declares V1beta1NodeNetworkConfigurationEnactment. Update the
return type to explicitly include undefined so consumers of
getCorrelatedEnactment handle the missing-match case correctly, and keep the
implementation as-is since the find call already reflects that behavior.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: deeeb9d8-8852-4a85-a013-101ce5d1b5c9
📒 Files selected for processing (25)
jest.config.tslocales/en/plugin__nmstate-console-plugin.jsonlocales/es/plugin__nmstate-console-plugin.jsonlocales/fr/plugin__nmstate-console-plugin.jsonlocales/ja/plugin__nmstate-console-plugin.jsonlocales/ko/plugin__nmstate-console-plugin.jsonlocales/zh/plugin__nmstate-console-plugin.jsonsrc/views/nodenetworkconfiguration/NodeTopology.scsssrc/views/nodenetworkconfiguration/NodeTopology.tsxsrc/views/nodenetworkconfiguration/NodeTopologyPage.tsxsrc/views/nodenetworkconfiguration/Topology.tsxsrc/views/nodenetworkconfiguration/components/NodeNetworkingPoliciesPanel/NodeNetworkingPoliciesPanel.test.tsxsrc/views/nodenetworkconfiguration/components/NodeNetworkingPoliciesPanel/NodeNetworkingPoliciesPanel.tsxsrc/views/nodenetworkconfiguration/components/NodeNetworkingPoliciesPanel/utils/formatPolicyAffectsDisplay.test.tssrc/views/nodenetworkconfiguration/components/NodeNetworkingPoliciesPanel/utils/formatPolicyAffectsDisplay.tssrc/views/nodenetworkconfiguration/components/NodeTopologyToolbar/NodeTopologyToolbar.scsssrc/views/nodenetworkconfiguration/components/NodeTopologyToolbar/NodeTopologyToolbar.test.tsxsrc/views/nodenetworkconfiguration/components/NodeTopologyToolbar/NodeTopologyToolbar.tsxsrc/views/nodenetworkconfiguration/components/TopologyControlBar/TopologyControlBar.tsxsrc/views/nodenetworkconfiguration/components/TopologySidebar/Drawer.tsxsrc/views/nodenetworkconfiguration/components/TopologySidebar/TopologySidebar.scsssrc/views/nodenetworkconfiguration/components/TopologySidebar/TopologySidebar.tsxsrc/views/nodenetworkconfiguration/manifest.tssrc/views/nodenetworkconfiguration/utils/utils.test.tssrc/views/nodenetworkconfiguration/utils/utils.ts
Member
Author
|
/test e2e-tests |
|
@pcbailey please review |
jeff-phillips-18
force-pushed
the
node-configuration-network-tab
branch
from
af3f3be to
5262d28
Compare
pcbailey
reviewed
Jul 8, 2026
pcbailey
left a comment
pcbailey
left a comment
There was a problem hiding this comment.
Hi, Jeff! I just had a few comments.
| }, | ||
| } as EncodedExtension<RoutePage>, | ||
| { | ||
| type: 'console.node/sub-nav-tab', |
There was a problem hiding this comment.
Please move this code to a separate view directory named "node-topology-panel". This view is for the "Node network configuration" tab in the Networking section. Also, please use kebab-case for the view directory name. I plan to eventually go through and correct the others.
Could you also please create a "topology" directory in the "src/utils" directory and place any shared code there?
| if (!enactmentsLoaded || enactmentsError || isEmpty(enactments)) { | ||
| return []; | ||
| } | ||
| const nodeEnactments = getCorrelatedEnactment(enactments, obj.metadata.name); |
There was a problem hiding this comment.
Please extract this to a variable and use the getName selector.
| } | ||
| }, [controller, state, stateLoaded, stateError, availableEnactments, obj.metadata.name]); | ||
|
|
||
| if (stateError?.response?.status === 403) |
There was a problem hiding this comment.
Please extract 403 to a constant with a descriptive name. I can update any other references to point to it in a future PR.
jeff-phillips-18
force-pushed
the
node-configuration-network-tab
branch
from
5262d28 to
e008d09
Compare
Member
Author
|
/test e2e-tests |
|
@jeff-phillips-18: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes CONSOLE-5103
Closes CONSOLE-4947
Summary
Adds a new Network tab to Node detail views that displays network topology and associated policies.
Changes:
Screen Shots:
Technical Details:
isPolicyAppliedInNode()to show only relevant policies for the nodeTesting:
🤖 Generated with https://claude.com/claude-code
Summary by CodeRabbit
New Features
Bug Fixes