Skip to content

add isConditional to fix bug

1c643ab
Select commit
Loading
Failed to load commit list.
Open

feat(roles): redesign org roles page and better perm multiselect #6027

add isConditional to fix bug
1c643ab
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 22, 2026 in 33m 15s

Code review found 1 important issue

Found 3 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 1
🟡 Nit 1
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx:351-375 Trash button hidden for non-conditional subjects in three existing callers
🟡 Nit frontend/src/pages/organization/RoleByIDPage/components/RolePermissionsSection/RolePermissionsSection.tsx:167-173 Nested elements in AccordionTrigger via triggerSuffix

Annotations

Check failure on line 375 in frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx

See this annotation in the file changed.

@claude claude / Claude Code Review

Trash button hidden for non-conditional subjects in three existing callers

The trash icon visibility condition at GeneralPermissionPolicies.tsx:351 (`fields.length > 1 || isConditional || !!onRemoveLastRule`) hides the per-row Remove button in three existing callers — `MembershipProjectAdditionalPrivilegeModifySection.tsx:439`, `IdentityProjectAdditionalPrivilegeModifySection.tsx:443`, and `ProjectTemplateEditRoleForm.tsx:205` — because none pass `onRemoveLastRule`. For non-conditional subjects with a single rule (Member, Groups, Tags, Integrations, Webhooks, Settings,

Check warning on line 173 in frontend/src/pages/organization/RoleByIDPage/components/RolePermissionsSection/RolePermissionsSection.tsx

See this annotation in the file changed.

@claude claude / Claude Code Review

Nested <button> elements in AccordionTrigger via triggerSuffix

Passing OrgPermissionQuickSelect as triggerSuffix nests a native <button> (SelectTrigger from @radix-ui/react-select) inside another native <button> (AccordionTrigger from @radix-ui/react-accordion), producing invalid HTML (<button>...<button>...</button>...</button>) and a React validateDOMNesting console warning. The role="none" wrapper and onClick stopPropagation only affect ARIA/event handling — they do not change DOM structure. Fix: move the preset Select out of the AccordionTrigger (e.g. r