fix: collapse full-access role categories into "All <category> actions" in Access Control#647
Conversation
UdaraWickramarathne
requested review from
Mirage20,
kaviththiranga,
sameerajayasoma and
stefinie123
as code owners
|
Warning Review limit reached
More reviews will be available in 46 minutes and 29 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesRole actions display normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…RolesTable Signed-off-by: Udara Wickramarathne <bimsaraudara25@gmail.com>
UdaraWickramarathne
force-pushed
the
fix/2907-collapse-actions
branch
from
07643a8 to
bb6feca
Compare
LakshanSS
left a comment
LakshanSS
left a comment
There was a problem hiding this comment.
Thank you for your contribution @UdaraWickramarathne
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Udara Wickramarathne <bimsaraudara25@gmail.com>
UdaraWickramarathne
force-pushed
the
fix/2907-collapse-actions
branch
from
1d6f37f to
fdf5d72
Compare
|
@UdaraWickramarathne Even when actions are normalized and collapsed the action count should reflect the actual number of granular actions, not the number of collapsed actions. Shall we fix this as well |
Fixed with fe530cf |
…ns count Signed-off-by: Udara Wickramarathne <bimsaraudara25@gmail.com>
UdaraWickramarathne
force-pushed
the
fix/2907-collapse-actions
branch
from
a609b5d to
fe530cf
Compare
![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
🧹 Nitpick comments (2)
plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.ts (1)
13-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for stale/unknown action preservation.
Please add one case where
actionscontains an entry absent fromAVAILABLE_ACTIONSand assert normalization keeps it. This locks in non-destructive behavior.🧪 Suggested test case
describe('normalizeActions', () => { + it('preserves stale actions that are missing from the current catalog', () => { + const actions = ['component:view', 'legacy:read']; + expect(normalizeActions(actions, AVAILABLE_ACTIONS)).toEqual([ + 'component:view', + 'legacy:read', + ]); + }); + it('returns the input unchanged when the action catalog has not loaded yet', () => {🤖 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 `@plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.ts` around lines 13 - 65, Add a new test case within the describe('normalizeActions') block that verifies the function preserves unknown or stale actions. Create a test that calls normalizeActions with an array containing both recognized actions from AVAILABLE_ACTIONS and an action that does not exist in the catalog, then assert that the function returns the result with the unknown action preserved unchanged. This ensures the normalizeActions function has non-destructive behavior when encountering actions outside the available catalog.plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.test.tsx (1)
68-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for preserving user input across async action-catalog load.
Please add a create-mode test that types a role name before actions finish loading, then simulates catalog arrival and verifies the typed name and selected template/actions are not reset.
🤖 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 `@plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.test.tsx` around lines 68 - 166, Add a new test case within the RoleDialog describe block that tests preserving user input across async action-catalog loads. The test should render the dialog in create mode with mockUseActions initially returning a loading state (loading: true), then use userEvent to type a role name, optionally click a template button to select actions, then update the mock to return the actual actions (simulating catalog load completion), and finally verify using screen queries that the typed role name and selected actions/template have been preserved and not reset.
🤖 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 `@plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.ts`:
- Around line 123-126: The normalization process in the return statement using
`expandWildcards` and `convertToWildcards` silently drops any actions from the
input that don't exist in the `availableActions` catalog, causing permission
loss on save. Modify the logic to preserve these unknown or stale actions
alongside the normalized catalog-backed actions. After calling
`convertToWildcards(expandWildcards(actions, availableActions),
availableActions)`, merge the result with any actions from the input that were
not present in `availableActions` to ensure no permissions are lost during the
normalization round-trip.
In `@plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.tsx`:
- Around line 321-333: The useEffect hook that initializes the form state in the
RoleDialog component has availableActions in its dependency list, causing it to
re-run whenever the action metadata updates and resetting the form with initial
values, which wipes unsaved changes in both create and edit modes. Remove
availableActions from the dependency array of this useEffect, keeping only
editingRole and open as dependencies, since availableActions should not trigger
a reset of the in-progress form state; this way the form will only reinitialize
when explicitly opening the dialog or switching to edit a different role.
In `@plugins/openchoreo/src/components/AccessControl/RolesTab/RolesTable.tsx`:
- Around line 214-217: The calculation in the Chip label at line 216 uses
displayActions.length - 5, but displayActions has already had wildcard actions
collapsed (e.g., component:* counts as 1 item). To correctly show how many
actual granted actions are hidden, replace displayActions.length with the count
of the original ungrouped actions before any collapse or grouping was applied.
Track or pass the actual total action count from the source data and use that
instead of the collapsed displayActions array length in the calculation.
---
Nitpick comments:
In
`@plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.ts`:
- Around line 13-65: Add a new test case within the describe('normalizeActions')
block that verifies the function preserves unknown or stale actions. Create a
test that calls normalizeActions with an array containing both recognized
actions from AVAILABLE_ACTIONS and an action that does not exist in the catalog,
then assert that the function returns the result with the unknown action
preserved unchanged. This ensures the normalizeActions function has
non-destructive behavior when encountering actions outside the available
catalog.
In
`@plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.test.tsx`:
- Around line 68-166: Add a new test case within the RoleDialog describe block
that tests preserving user input across async action-catalog loads. The test
should render the dialog in create mode with mockUseActions initially returning
a loading state (loading: true), then use userEvent to type a role name,
optionally click a template button to select actions, then update the mock to
return the actual actions (simulating catalog load completion), and finally
verify using screen queries that the typed role name and selected
actions/template have been preserved and not reset.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b06a7350-0ffc-4796-ad03-bb34efd2b45c
📒 Files selected for processing (7)
.changeset/normalize-role-actions-display.mdplugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RolesTable.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RolesTable.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.tsplugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.ts
…le edits Signed-off-by: Udara Wickramarathne <bimsaraudara25@gmail.com>
Signed-off-by: Udara Wickramarathne <bimsaraudara25@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.ts (1)
13-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for
['*', stale]normalization.The suite validates global collapse and stale preservation independently, but not together. Please add a case asserting
normalizeActions(['*', 'foo:bar'], AVAILABLE_ACTIONS)returns['*', 'foo:bar']so global-collapse behavior stays stable when stale permissions are preserved.🧪 Suggested test addition
describe('normalizeActions', () => { @@ it('does not duplicate a known category wildcard when preserving', () => { @@ }); + + it('keeps global wildcard collapsed while preserving stale actions', () => { + expect(normalizeActions(['*', 'foo:bar'], AVAILABLE_ACTIONS)).toEqual([ + '*', + 'foo:bar', + ]); + });🤖 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 `@plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.ts` around lines 13 - 91, Add a regression test case to the normalizeActions describe block that validates the function correctly handles the combination of global wildcard collapse and stale action preservation together. Create a test that calls normalizeActions with an array containing the global wildcard '*' and a stale action 'foo:bar' against AVAILABLE_ACTIONS, and assert that the result preserves both elements as ['*', 'foo:bar'] unchanged. This ensures that global-collapse behavior remains stable when stale permissions coexist with the global wildcard.
🤖 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 `@plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.ts`:
- Around line 123-126: Stale or unknown actions in the actions array are
preventing the global wildcard from collapsing because they increase the size
checked by the gate in convertToWildcards. Before passing actions to
expandWildcards on line 123, filter out any actions that are not present in
availableActions. This ensures only valid known actions are processed, allowing
convertToWildcards to properly collapse to the global '*' wildcard when
appropriate instead of returning category wildcards.
---
Nitpick comments:
In
`@plugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.ts`:
- Around line 13-91: Add a regression test case to the normalizeActions describe
block that validates the function correctly handles the combination of global
wildcard collapse and stale action preservation together. Create a test that
calls normalizeActions with an array containing the global wildcard '*' and a
stale action 'foo:bar' against AVAILABLE_ACTIONS, and assert that the result
preserves both elements as ['*', 'foo:bar'] unchanged. This ensures that
global-collapse behavior remains stable when stale permissions coexist with the
global wildcard.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cf8f956-8919-4621-9a0f-2fdbf3ad66b0
📒 Files selected for processing (5)
plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RolesTable.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.test.tsplugins/openchoreo/src/components/AccessControl/RolesTab/actionUtils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.test.tsx
- plugins/openchoreo/src/components/AccessControl/RolesTab/RolesTable.tsx
- plugins/openchoreo/src/components/AccessControl/RolesTab/RoleDialog.tsx
Signed-off-by: Udara Wickramarathne <bimsaraudara25@gmail.com>
3 participants
Purpose
Resolves openchoreo/openchoreo#2907
(openchoreo/openchoreo#2907).
In Access Control, a role's actions were rendered individually. A role granted
every action in a category (e.g. all alerts or all metrics operations,
persisted one-by-one) listed each operation as its own chip, producing long,
noisy lists. The action selection dialog already collapses these on
confirm, so the same role looked different depending on where you viewed it
(Roles table vs. edit dialog vs. selection dialog).
Goals
Display role actions consistently in their collapsed form everywhere: when a
role grants every action in a category, show a single chip — e.g. "All alerts
actions", "All metrics actions" — instead of listing each operation, matching
what the selection dialog produces.
Approach
normalizeActions()in a newRolesTab/actionUtils.ts. It round-trips arole's stored actions through
expandWildcards→convertToWildcards,reusing the same per-category collapsing logic the selection dialog applies
on confirm, so display and edit stay in sync. If the action catalog hasn't
loaded yet (
availableActionsempty) it returns the input unchanged, sonothing is dropped before we can tell which categories are complete.
useActions(), render thenormalized actions as chips, and label fully-granted categories through
getActionDisplayLabelas "All actions" (e.g. "All alertsactions"). The "+N more" overflow now counts the collapsed chips.
applying a role template; the effect re-runs once
availableActionsresolves.
actionUtils.test.tscover the expand/collapse/normalize pathsand the empty-catalog passthrough.
Before the change
Before.webm
After the change
After.webm
User stories
As a platform engineer managing roles, I want a role's permissions shown
compactly and consistently across the Roles table and edit dialog, so I can see
at a glance which categories a role fully grants.
Release note
Access Control: when a role grants every action in a category, the Roles table
and Role dialog now show a single collapsed chip (e.g. "All alerts actions",
"All metrics actions") instead of listing each individual action, matching the
action selection dialog.
Documentation
N/A
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
Security checks
Samples
N/A
Related PRs
N/A
Migrations (if applicable)
N/A
Test environment
1.2.0-m.1Learning
N/A
Summary by CodeRabbit