[ACM-30324] Policy sets dropdown menu missing border and shadow#5769
Merged
Ginxo merged 11 commits intostolostron:mainfrom Mar 9, 2026
Merged
[ACM-30324] Policy sets dropdown menu missing border and shadow#5769Ginxo merged 11 commits intostolostron:mainfrom
Ginxo merged 11 commits intostolostron:mainfrom
Conversation
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPolicySetsPage now tracks which card's action menu is open and passes that state into PolicySetCard; PolicyCardDropdown was converted to a controlled component (isOpen/onOpenChange). Tests and a small CSS rule were added to exercise and style the behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Page as PolicySetsPage
participant Card as PolicySetCard
participant Dropdown as PolicyCardDropdown
participant Drawer as DrawerContext
User->>Card: click kebab/menu button (cardID)
Card->>Dropdown: onOpenChange(true)
Dropdown->>Page: setCardIdActionMenuOpen(cardID)
Page-->>Card: prop cardIdActionMenuOpen updated
Card-->>Dropdown: isOpen = (cardIdActionMenuOpen === cardID)
User->>Dropdown: click "View details"
Dropdown->>Page: onOpenChange(false) / setCardIdActionMenuOpen(undefined)
Dropdown->>Card: invoke onView -> onSelect(cardID)
Card->>Drawer: openDetails(cardID)
Drawer-->>User: Drawer opens with details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Ginxo
commented
Mar 3, 2026
| @@ -0,0 +1,3 @@ | |||
| .pf-v6-c-card__actions{ | |||
Ginxo
commented
Mar 3, 2026
| </MenuToggle> | ||
| )} | ||
| isOpen={isKebabOpen} | ||
| isPlain={true} |
Contributor
Author
There was a problem hiding this comment.
this is really the fix for the issue
Ginxo
commented
Mar 3, 2026
Comment on lines
+11
to
+12
| onOpenChange: (isOpen: boolean) => void | ||
| isOpen: boolean |
Contributor
Author
There was a problem hiding this comment.
whether the open or not has been delegated to parent component
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.css (1)
1-3: Scope the card-actions override to this feature area.Line 1 uses a global PatternFly class selector, so this can unintentionally alter spacing for unrelated card actions in the same bundle/page. Prefer a local wrapper class (e.g., policy-set card container) and scope this rule beneath it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.css` around lines 1 - 3, The rule currently targets the global PatternFly class .pf-v6-c-card__actions and should be scoped to this component; wrap it under a local container class (e.g., .policy-set-card-container or .policy-set-card-dropdown) so the selector becomes something like .policy-set-card-container .pf-v6-c-card__actions { gap: 0; } and update the PolicyCardDropdown component to add that container class to its root element.
🤖 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/src/routes/Governance/policy-sets/components/PolicySetCard.tsx`:
- Around line 87-91: The onSelect handler currently toggles selection (const
newSelectedCard = cardId === selectedCardID ? '' : cardId) and is being reused
for the “View details” action, which causes an already-selected card to close
the details drawer; add a dedicated handler (e.g., onViewDetails) that always
opens details by calling setSelectedCardID(cardId) and openDetails(cardId) (no
toggle), and replace usages of onSelect for the “View details” action with
onViewDetails so clicking View details never closes the drawer.
- Around line 79-80: Replace the brittle querySelector usage that interpolates
runtime IDs with getElementById: locate the code using cardId and
document.querySelector(`#${cardId}`) (the cardElement variable and subsequent
cardElement?.scrollIntoView call) and change the lookup to
document.getElementById(cardId), preserving the optional chaining and
scrollIntoView call so behavior remains the same but avoids
selector-special-character bugs and is faster.
---
Nitpick comments:
In
`@frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.css`:
- Around line 1-3: The rule currently targets the global PatternFly class
.pf-v6-c-card__actions and should be scoped to this component; wrap it under a
local container class (e.g., .policy-set-card-container or
.policy-set-card-dropdown) so the selector becomes something like
.policy-set-card-container .pf-v6-c-card__actions { gap: 0; } and update the
PolicyCardDropdown component to add that container class to its root element.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
frontend/src/routes/Governance/policy-sets/PolicySets.tsxfrontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.cssfrontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.tsxfrontend/src/routes/Governance/policy-sets/components/PolicySetCard.tsx
frontend/src/routes/Governance/policy-sets/components/PolicySetCard.tsx
Outdated
Show resolved
Hide resolved
Ginxo
commented
Mar 3, 2026
| @@ -2,52 +2,60 @@ | |||
| import { Divider, Dropdown, DropdownItem, DropdownList, MenuToggle, MenuToggleElement } from '@patternfly/react-core' | |||
Contributor
Author
There was a problem hiding this comment.
nothing really changed from this component, apart from open state delegation and isPlain removed and a bit of reordering and renaming
Ginxo
commented
Mar 3, 2026
| @@ -35,68 +35,85 @@ export default function PolicySetCard(props: { | |||
| setSelectedCardID: React.Dispatch<React.SetStateAction<string>> | |||
Contributor
Author
There was a problem hiding this comment.
in this file the isSelectable stuff
Ginxo
commented
Mar 3, 2026
| const [perPage, setPerPage] = useState<number>(10) | ||
| const [filteredPolicySets, setFilteredPolicySets] = useState<PolicySet[]>(policySets) | ||
| const [selectedCardID, setSelectedCardID] = useState<string>('') | ||
| const [cardIdActionMenuOpen, setCardIdActionMenuOpen] = useState<string>() |
Contributor
Author
There was a problem hiding this comment.
and in this file just keeping one action menu open
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.test.tsx (2)
76-81: Strengthen the disabled-item test with behavioral assertions.Line 76 currently only checks “does not throw.” Add assertions that disabled click does not trigger close/change callbacks, so regressions are caught.
Diff suggestion
- test('clicking disabled Edit does not throw', () => { - render(<PolicyCardDropdown onView={() => {}} onOpenChange={() => {}} isOpen={true} />) + test('clicking disabled Edit does not trigger state change', () => { + const onOpenChange = jest.fn() + render(<PolicyCardDropdown onView={() => {}} onOpenChange={onOpenChange} isOpen={true} />) const editItem = screen.getByRole('menuitem', { name: 'Edit' }) expect(editItem).toHaveAttribute('aria-disabled', 'true') fireEvent.click(editItem) + expect(onOpenChange).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.test.tsx` around lines 76 - 81, The test for PolicyCardDropdown should assert behavioral outcomes when clicking a disabled menu item: render PolicyCardDropdown with jest.fn() spies for onView and onOpenChange (instead of empty functions), locate the 'Edit' menuitem, verify it's aria-disabled, fireEvent.click(editItem), then assert the spy functions (e.g., the onOpenChange mock and any edit handler) were not called and that no state-change callback was invoked; this ensures clicking a disabled item does not trigger handlers or close the menu.
10-11: Use a stable toggle selector instead of array index access.At Line 10 and Line 24,
getAllByRole('button')[0]is brittle if another button appears. Prefer selecting the explicit toggle (e.g., byexpandedstate or accessible name).Diff suggestion
- const buttons = screen.getAllByRole('button') - expect(buttons.length).toBeGreaterThanOrEqual(1) + const toggle = screen.getByRole('button', { expanded: false }) + expect(toggle).toBeInTheDocument() ... - const buttons = screen.getAllByRole('button') - fireEvent.click(buttons[0]) + const toggle = screen.getByRole('button', { expanded: false }) + fireEvent.click(toggle)Also applies to: 24-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.test.tsx` around lines 10 - 11, The test uses brittle array-index access via screen.getAllByRole('button')[0]; instead, locate the specific toggle button directly by an accessible selector: use screen.getByRole('button', { name: /toggle label/i }) or query by its aria-expanded state (e.g., screen.getByRole('button', { expanded: false }) or using getByTestId if a test id exists) instead of indexing the buttons array; update occurrences in PolicyCardDropdown.test.tsx (references: screen.getAllByRole, the buttons variable and any direct [0] access) to assert and interact with the explicit toggle element.
🤖 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/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx`:
- Around line 149-169: The test "setSelectedCardID is called when clicking card
title" only verifies that the callback was invoked; change it to assert the
exact payload by replacing or augmenting the final assertion with a check that
setSelectedCardID was called with the expected card id (use the known policySet
identifier from the test fixture, e.g. policySet.metadata.name or the id used by
PolicySetCard) and the expected call count (e.g. toHaveBeenCalledTimes(1) and
toHaveBeenCalledWith(...)) so the contract of PolicySetCard.setSelectedCardID is
validated.
In `@frontend/src/routes/Governance/policy-sets/PolicySets.test.tsx`:
- Around line 121-122: The test currently checks button counts on cardA and
cardB which is brittle; instead, update the assertions to exercise the
action-menu behavior: use the existing cardA and cardB elements to find and
click the actions trigger (e.g., the element that opens the dropdown/action menu
on each PolicySet card), then assert the action menu appears and contains the
expected action items (like "Edit", "Delete" or whatever your UI exposes) and
that the menu can be closed. Replace the two lines referencing
querySelectorAll('button').length with steps that: locate the actions trigger
within cardA/cardB, simulate a user click to open the menu, assert the menu DOM
is present and visible and contains the expected action labels, and optionally
assert the menu closes after selecting or clicking outside.
---
Nitpick comments:
In
`@frontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.test.tsx`:
- Around line 76-81: The test for PolicyCardDropdown should assert behavioral
outcomes when clicking a disabled menu item: render PolicyCardDropdown with
jest.fn() spies for onView and onOpenChange (instead of empty functions), locate
the 'Edit' menuitem, verify it's aria-disabled, fireEvent.click(editItem), then
assert the spy functions (e.g., the onOpenChange mock and any edit handler) were
not called and that no state-change callback was invoked; this ensures clicking
a disabled item does not trigger handlers or close the menu.
- Around line 10-11: The test uses brittle array-index access via
screen.getAllByRole('button')[0]; instead, locate the specific toggle button
directly by an accessible selector: use screen.getByRole('button', { name:
/toggle label/i }) or query by its aria-expanded state (e.g.,
screen.getByRole('button', { expanded: false }) or using getByTestId if a test
id exists) instead of indexing the buttons array; update occurrences in
PolicyCardDropdown.test.tsx (references: screen.getAllByRole, the buttons
variable and any direct [0] access) to assert and interact with the explicit
toggle element.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
frontend/src/routes/Governance/policy-sets/PolicySets.test.tsxfrontend/src/routes/Governance/policy-sets/components/PolicyCardDropdown.test.tsxfrontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx
frontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx
Show resolved
Hide resolved
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx`:
- Around line 124-127: The test mutates Element.prototype.scrollIntoView in the
beforeEach of PolicySetCard.test.tsx which leaks into other tests; save the
original implementation (e.g., const originalScroll =
Element.prototype.scrollIntoView) and restore it in an afterEach cleanup so the
global prototype is returned to its prior state; apply the same save-and-restore
pattern for the other mock at the later block (around the second beforeEach at
lines 223-226) to avoid cross-test leakage.
In `@frontend/src/routes/Governance/policy-sets/components/PolicySetCard.tsx`:
- Around line 56-85: openDetails schedules a scroll timeout each call but never
cancels prior timers, causing stale auto-scrolls; modify openDetails to track
the timeout ID (e.g., via a React ref like scrollTimeoutRef) and call
clearTimeout(scrollTimeoutRef.current) before creating a new setTimeout, store
the new ID in that ref, and also clear the timeout when closing the drawer (the
else branch where setDrawerContext(undefined)) and on component unmount to avoid
leaked timers; update references around openDetails, setDrawerContext, and
getElementById to use the tracked timeout ID.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f008a70-6573-4d7f-baf0-8e1d10433a29
📒 Files selected for processing (3)
frontend/src/routes/Governance/policy-sets/PolicySets.test.tsxfrontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsxfrontend/src/routes/Governance/policy-sets/components/PolicySetCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/Governance/policy-sets/PolicySets.test.tsx
frontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx
Show resolved
Hide resolved
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx (1)
200-204: Use an accessible query for the actions toggle to reduce test brittleness.The current button lookup depends on empty text content and button order, which is fragile across markup/icon changes. Prefer role+name querying scoped to the card.
Proposed refactor
-import { fireEvent, render, screen } from '@testing-library/react' +import { fireEvent, render, screen, within } from '@testing-library/react' ... - const buttonsWithinCard = card!.querySelectorAll('button') - const kebabButton = - Array.from(buttonsWithinCard).find((b) => !b.textContent?.trim()) ?? - buttonsWithinCard[buttonsWithinCard.length - 1] - fireEvent.click(kebabButton) + const kebabButton = within(card!).getByRole('button', { name: /actions/i }) + fireEvent.click(kebabButton)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx` around lines 200 - 204, The button lookup in PolicySetCard.test.tsx is brittle (it uses buttonsWithinCard and kebabButton determined by empty textContent); instead scope an accessible query to the card element (use within(card) from `@testing-library/react`) and select the actions toggle by role and accessible name (e.g., getByRole('button', { name: /actions|more options|open menu/i }) or getByLabelText) so the test targets the intended control reliably; replace the Array.from(...).find(...) logic with a within(card).getByRole/... call and then fireEvent.click on that result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx`:
- Around line 200-204: The button lookup in PolicySetCard.test.tsx is brittle
(it uses buttonsWithinCard and kebabButton determined by empty textContent);
instead scope an accessible query to the card element (use within(card) from
`@testing-library/react`) and select the actions toggle by role and accessible
name (e.g., getByRole('button', { name: /actions|more options|open menu/i }) or
getByLabelText) so the test targets the intended control reliably; replace the
Array.from(...).find(...) logic with a within(card).getByRole/... call and then
fireEvent.click on that result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 111aa2a5-dcb9-471b-a6ad-346f2a67e380
📒 Files selected for processing (1)
frontend/src/routes/Governance/policy-sets/components/PolicySetCard.test.tsx
Contributor
Author
|
/test unit-tests-sonarcloud |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
zlayne
approved these changes
Mar 9, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ginxo, zlayne 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 |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
New changes are detected. LGTM label has been removed. |
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
2 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.
📝 Summary
the issue itself was just about removing
isPlain={true}from the Dropdown, but then I started to notice few things:Before
Video_2026-03-03_18-08-11.mp4
After
Video_2026-03-03_18-07-40.mp4
Video_2026-03-04_14-41-42.mp4
Ticket Summary (Title):
Policy sets dropdown menu missing border and shadow
Ticket Link:
https://issues.redhat.com/browse/ACM-30324
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
New Features
Style
Tests