fix: prevent disabled View checkbox from being selected when Write is toggled#4693
fix: prevent disabled View checkbox from being selected when Write is toggled#4693Grovy-3170 wants to merge 1 commit intomainfrom
Conversation
… toggled When the backend doesn't include the "read" scope for a permission module, the View checkbox is correctly shown as disabled. However, selecting the Write checkbox would still auto-select the disabled View checkbox because `handleScopeChange` unconditionally added Read when Write was toggled on. This fix checks `isReadAvailable` before auto-selecting Read, so disabled View checkboxes remain unselected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Review changes with |
WalkthroughA conditional logic refinement in the custom role creation component reorders boolean computations and makes the forced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/screens/UserManagement/UserRevamp/CreateCustomRoleV2.res (1)
48-48: Add a regression test for the “Write on + Read unavailable” path.Given this was a behavior bug, adding a focused test would protect this branch from future regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/UserManagement/UserRevamp/CreateCustomRoleV2.res` at line 48, Add a regression test that exercises the branch in CreateCustomRoleV2 where the tuple matches (Write, true) but isReadAvailable is false: call the logic that uses updateScope (referencing updateScope, newScopes, Add, Read and isReadAvailable) with a starting newScopes and isReadAvailable=false, then assert the result is exactly newScopes (i.e., no Read was added); place the test alongside existing CreateCustomRoleV2 tests and make it fail if updateScope is called to add Read in this scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/screens/UserManagement/UserRevamp/CreateCustomRoleV2.res`:
- Line 48: Add a regression test that exercises the branch in CreateCustomRoleV2
where the tuple matches (Write, true) but isReadAvailable is false: call the
logic that uses updateScope (referencing updateScope, newScopes, Add, Read and
isReadAvailable) with a starting newScopes and isReadAvailable=false, then
assert the result is exactly newScopes (i.e., no Read was added); place the test
alongside existing CreateCustomRoleV2 tests and make it fail if updateScope is
called to add Read in this scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b1eb015-bb40-4d6f-a63f-6ac96a7b0063
📒 Files selected for processing (1)
src/screens/UserManagement/UserRevamp/CreateCustomRoleV2.res
muditbhutani
left a comment
muditbhutani
left a comment
There was a problem hiding this comment.
Merge Confidence: 4.5/5 - Very Confident
"Ship it - low risk, well-tested"
Assessment
- ✓ Well-scoped 1-file, 1-hunk bug fix — minimal blast radius
- ✓ Correctly moves
isReadAvailable/isWriteAvailablebefore the closure that references them, fixing a variable ordering issue - ✓ Guard condition
isReadAvailable ? ... : newScopesis the right fix — prevents adding a scope that doesn't exist for the module - ✓ No API, auth, or data layer changes — purely client-side form state logic
- ⚠ The symmetric case
(Read, false) => Remove Writedoesn't checkisWriteAvailable, but this is safe assuming Write always implies Read availability
Review: Fix: Guard Read scope auto-add behind availability check in custom role creation
Small bug fix that prevents the Write scope toggle from unconditionally adding Read scope when Read isn't available for a given permission module. Also fixes variable declaration ordering so isReadAvailable is in scope within the handleScopeChange closure. Low risk, well-scoped change.
| @@ -35,22 +35,22 @@ module RenderPermissionModule = { | |||
| parentGroupsField.input.onChange(updatedGroups->Identity.arrayOfGenericTypeToFormReactEvent) | |||
There was a problem hiding this comment.
Verify conditional Read scope addition when Write is selected
Risk: LOW
Fixes a bug where selecting "Write" scope unconditionally added "Read", even for permission modules that don't support Read scope. Now isReadAvailable is checked before adding Read.
Two changes in one: (1) moved isReadAvailable/isWriteAvailable declarations above handleScopeChange so they're captured by the closure, and (2) added the isReadAvailable guard on the Write→Read auto-add logic.
The (Read, false) branch still unconditionally removes Write — this is safe because any module offering Write should always offer Read (Write implies Read), so removing Read should always strip Write too. Verify this invariant holds across all permission modules.
Reviewers should confirm that no permission module exists where Write is available but Read is not, as the (Read, false) => Remove Write path doesn't check isWriteAvailable.
Diagram:
stateDiagram-v2
[*] --> NoScope
NoScope --> ReadOnly: "Select Read (if available)"
NoScope --> WriteOnly: "Select Write (Read unavailable)"
NoScope --> ReadWrite: "Select Write (Read available)"
ReadOnly --> ReadWrite: "Select Write"
ReadOnly --> NoScope: "Deselect Read"
ReadWrite --> NoScope: "Deselect Read (removes Write)"
ReadWrite --> ReadOnly: "Deselect Write"
WriteOnly --> NoScope: "Deselect Write"
|
@control_center_review_bot review the pr |
|
@Grovy-3170 Please follow the PR description format. Also checks are failing please check |
3 participants
Type of Change
Description
When the backend doesn't include the
readscope for a permission module, the View checkbox is correctly shown as disabled. However, selecting the Write checkbox would still auto-select the disabled View checkbox.The root cause was in
handleScopeChangeinCreateCustomRoleV2.res— the(Write, true)case unconditionally addedReadto the scopes without checking if Read was actually available:Moved
isReadAvailable/isWriteAvailableabovehandleScopeChangeso the availability check is accessible inside the handler.Motivation and Context
Fixes #4694
On the Create Custom Roles page, each permission module has View and Write checkboxes. If the backend omits a scope (e.g.,
read), the corresponding checkbox is disabled. The bug caused a disabled View checkbox to become selected when Write was toggled on, which is incorrect — disabled checkboxes should never change state.How did you test it?
npm run re:build— compiles successfully with no errorsChecklist
npm run re:build