CONSOLE-5158: Add filter by group for nodes#16253
Conversation
|
@jeff-phillips-18: This pull request references CONSOLE-5158 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
component/core
|
@jeff-phillips-18: This pull request references CONSOLE-5158 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds group-based checkbox filtering to the Nodes page, two English locale strings, and changes the group Edit button to always render (disabled when unauthorized) with a tooltip; DetailsCard gets the same edit-button behavior. ChangesNode group filtering and edit UX
🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
705-711: UseisCSRResource()to filter before extracting group options.The
dataarray mixesNodeKindandCertificateSigningRequestKindobjects, butgetExistingGroupsexpectsNodeKind[]. While the current code works safely—getNodeGroupsdefensively handles CSRs via optional chaining—the type cast obscures this. SinceisCSRResource()is already available, filter explicitly:Suggested improvement
const nodeGroupFilterOptions = useMemo<DataViewFilterOption[]>(() => { - const groupNames = getExistingGroups(data as NodeKind[]); + const actualNodes = data.filter((item) => !isCSRResource(item)) as NodeKind[]; + const groupNames = getExistingGroups(actualNodes); return groupNames.map((groupName) => ({ value: groupName, label: groupName, })); }, [data]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx` around lines 705 - 711, nodeGroupFilterOptions currently casts mixed data to NodeKind[] before calling getExistingGroups, hiding that data may include CertificateSigningRequestKind; update the useMemo to first filter data using isCSRResource(item) to exclude CSRs (e.g. data.filter(item => !isCSRResource(item))) and then pass that filtered array to getExistingGroups without a type cast so getExistingGroups receives a proper NodeKind[] and the types are accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx`:
- Around line 769-775: The Groups checkbox filter (DataViewCheckboxFilter with
filterId="groups" using nodeGroupFilterOptions) must only be rendered when the
feature flag nodeMgmtV1Enabled is true; update the JSX to conditionally include
that DataViewCheckboxFilter based on nodeMgmtV1Enabled and ensure the same flag
is used when computing additionalFilterNodes for the NodeList (or move
generation of filter nodes into a memo that depends on nodeMgmtV1Enabled) so the
filter visibility matches the presence of the Groups column.
---
Nitpick comments:
In `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx`:
- Around line 705-711: nodeGroupFilterOptions currently casts mixed data to
NodeKind[] before calling getExistingGroups, hiding that data may include
CertificateSigningRequestKind; update the useMemo to first filter data using
isCSRResource(item) to exclude CSRs (e.g. data.filter(item =>
!isCSRResource(item))) and then pass that filtered array to getExistingGroups
without a type cast so getExistingGroups receives a proper NodeKind[] and the
types are accurate.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3a0f7827-0bdf-44d3-bd97-a24d9c17b5e5
📒 Files selected for processing (3)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsx
🔇 Additional comments (4)
frontend/packages/console-app/locales/en/console-app.json (1)
392-392: LGTM!New i18n strings are correctly added and follow the existing key/value conventions. The permission tooltip message is clear and consistent with other permission-related strings in the codebase.
Also applies to: 453-453
frontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsx (1)
104-119: LGTM!The tooltip pattern is correctly implemented. The wrapper
<div>is the right approach for attaching tooltips to disabled buttons (disabled elements don't receive focus/hover events). The conditional ref assignment ensures the tooltip only appears when permission is definitively denied (not during loading).frontend/packages/console-app/src/components/nodes/NodesPage.tsx (2)
831-840: LGTM!The groups filter logic correctly handles CSR resources by returning
falseearly, and the membership check usingsome()withincludes()follows the established pattern from the roles filter. Good consistency.
1080-1095: LGTM!Same tooltip pattern as
DetailsCard.tsx- wrapper span for the disabled button trigger, conditional ref assignment, and tooltip content for permission denial. Consistent implementation across both locations.
|
/test e2e-gcp-console |
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
cb1d6a3 to
f34d07d
Compare
|
/retest |
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
f34d07d to
d1edb51
Compare
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Thanks for this change! The filter implementation cleanly follows the existing roles/architecture patterns, the feature flag gating is properly applied, and the disabled-button-with-tooltip UX is a nice improvement over hiding the button entirely. The DetailsCard refactoring to OverviewDetailItem is also a welcome cleanup.
A few things to address — the always-mounted Tooltip, a ref type mismatch, and missing test coverage — but overall this is solid work. See inline comments for details.
| <Tooltip | ||
| triggerRef={editGroupButtonRef} | ||
| content={t('console-app~You do not have permission to edit groups.')} | ||
| /> |
There was a problem hiding this comment.
The <Tooltip> is always mounted in the DOM, even when the user has permission and the tooltip will never display. When canEdit is true, editGroupButtonRef.current is null — PF handles it gracefully, but it's an unnecessary component mount with internal effects/subscriptions.
Conditionally render it only when the button is actually disabled:
{!isEditLoading && !canEdit && (
<Tooltip
triggerRef={editGroupButtonRef}
content={t('console-app~You do not have permission to edit groups.')}
/>
)}Same issue in DetailsCard.tsx.
| const { t } = useTranslation(); | ||
| const launchOverlay = useOverlay(); | ||
| const nodeMgmtV1Enabled = useFlag(FLAG_NODE_MGMT_V1); | ||
| const editGroupButtonRef = useRef<HTMLDivElement>(null); |
There was a problem hiding this comment.
Ref type mismatch — useRef<HTMLDivElement> but it's attached to a <span> wrapper below (line 1087). Should be useRef<HTMLSpanElement>.
Note: in DetailsCard.tsx the wrapper is a <div>, so it's correct there — just inconsistent here.
| const nodeGroupFilterOptions = useMemo<DataViewFilterOption[]>(() => { | ||
| const groupNames = getExistingGroups(data as NodeKind[]); | ||
| return groupNames.map((groupName) => ({ | ||
| value: groupName, | ||
| label: groupName, | ||
| })); | ||
| }, [data]); |
There was a problem hiding this comment.
[NIT] [data] dependency means this recomputes whenever any node changes (not just group annotations), which then cascades through additionalFilterNodes. This mirrors how machineSetFilterOptions works so it's consistent, just noting for awareness — on large clusters with frequent metrics refreshes, getExistingGroups iterates all nodes on every update.
| </DescriptionListGroup> | ||
| <Flex justifyContent={{ default: 'justifyContentSpaceBetween' }}> | ||
| <FlexItem> | ||
| <OverviewDetailItem isLoading={!obj} title={t('console-app~Groups')}> |
There was a problem hiding this comment.
👍 Nice improvement — the previous code used raw <dt> elements with manual PF class names, which was fragile. Switching to OverviewDetailItem delegates the proper semantic structure while keeping the edit button positioned via Flex. Cleaner and more consistent.
| } | ||
| } | ||
|
|
||
| // Groups filter | ||
| if (filters.groups.length > 0) { | ||
| if (isCSR) { | ||
| return false; | ||
| } | ||
| const nodeGroups = getNodeGroups(resource as NodeKind); | ||
| if (!filters.groups.some((r) => nodeGroups.includes(r))) { | ||
| return false; |
There was a problem hiding this comment.
No tests added for the new filter behavior. The filter follows established patterns, but the matchesAdditionalFilters logic and the disabled-button-with-tooltip UX should have coverage — at minimum:
- Groups filter options are correctly derived from node data
- Filter correctly includes/excludes nodes by selected groups
- CSR resources are excluded from group filtering
- Filter only renders when
nodeMgmtV1Enabledis true
There was a problem hiding this comment.
While I agree there should be tests for these additions to the NodesPage, there are currently no tests for the NodesPage. I have attempted to create tests but as claude says:
I attempted extensively to render the full NodesPage component, but after persistent effort, encountered insurmountable issues:
- 20+ nested dependencies requiring mocking
- Circular dependency chains through PatternFly topology components
- Undefined component errors despite extensive mocking attempts
- Complex Redux/plugin system integrations that fail in Jest environment
Claude keeps creating mimic code and testing the mimic'd code rather than the actual code being used. I don't feel adding those tests serve any purpose.
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
d1edb51 to
307c398
Compare
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Thanks for the update! The renderGroupEditButton() rework in NodesPage.tsx is a nice improvement — the Tooltip now only renders when the button is actually disabled, which addresses the original concern cleanly.
Two items still open:
- DetailsCard.tsx still uses the always-mounted Tooltip pattern — the
useRef<HTMLDivElement>+<Tooltip triggerRef={editGroupButtonRef}>is still rendered unconditionally. Consider applying the same conditional pattern used inNodesPage.tsx. - No tests added — the filter logic, disabled button behavior, and feature flag gating still have no test coverage.
| <Tooltip | ||
| triggerRef={editGroupButtonRef} | ||
| content={t('console-app~You do not have permission to edit groups.')} | ||
| /> |
There was a problem hiding this comment.
The Tooltip is still always mounted here, even when the user has permission. NodesPage.tsx was updated to conditionally render the Tooltip — consider applying the same pattern here:
{!isEditLoading && !canEdit && (
<Tooltip content={t('console-app~You do not have permission to edit groups.')}>
<span>{editButton}</span>
</Tooltip>
)}
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
307c398 to
9465c37
Compare
|
@jeff-phillips-18 it looks like we need to re-run the |
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
9465c37 to
0ad79f0
Compare
|
@jeff-phillips-18 thank you the PR looks good. One more thing, could you please add tests based on the Test Plan. Thank you |
|
@jeff-phillips-18: This pull request references CONSOLE-5158 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
@jhadvig I have added the manual test steps to the PR description. |
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
0ad79f0 to
b4fb283
Compare
|
/retest |
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
b4fb283 to
ec9dff3
Compare
jeff-phillips-18
force-pushed
the
node-group-filter
branch
from
ec9dff3 to
d7ff01d
Compare
|
@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. |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
/lgtm
Should we add a Jira issue to track adding tests to the nodes dashboard?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-phillips-18, TheRealJon 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 |
openshift-ci
Bot
added
the
approved
|
5 participants
Summary
Adds a "Filter by groups" checkbox filter to the Nodes page, enabling users to filter nodes by their assigned group annotations. Also improves the UX of the "Edit groups" button by showing it in a disabled state with an informative tooltip when users lack permission, rather than hiding it completely.
Changes
Implementation details
Test plan
Manual Test Steps
Prerequisites
Test Case 1: Filter by Groups - Basic Functionality
Steps:
Expected Result:
Test Case 2: Filter Options Populate Dynamically
Steps:
Expected Result:
Test Case 3: Single Group Filtering
Steps:
Expected Result:
Test Case 4: Multiple Group Filtering
Steps:
Expected Result:
Test Case 5: Edit Groups Button - Disabled State (No Permission)
Steps:
Expected Result:
Test Case 6: Edit Groups Button - Enabled State (With Permission)
Steps:
Expected Result:
Test Case 7: Node Details Card - Edit Button Disabled (No Permission)
Steps:
Expected Result:
Test Case 8: Node Details Card - Edit Button Enabled (With Permission)
Steps:
Expected Result:
Test Case 9: Combined Filter Testing
Steps:
- Filter by status (e.g., "Ready")
- Filter by roles (e.g., "worker")
- Filter by groups (e.g., "production")
Expected Result:
Notes:
🤖 Generated with https://claude.com/claude-code
Summary by CodeRabbit
New Features
Localization