fix: delete permission or role from footer issue #3939
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughSelection identity switched from names to stable IDs in permissions and roles tables. Checkbox handlers and selection checks now use permissionId and roleId when toggling and rendering selection state. Hover logic remains name-based. No exported/public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as Table Row Checkbox
participant C as Component State
participant R as UI Row
U->>T: Click checkbox
T->>C: toggleSelection(id)
alt ID not selected
C->>C: Add id to selected Set
else ID selected
C->>C: Remove id from selected Set
end
C-->>R: Re-render with isSelected(id)
R->>U: Checkbox state and row selection updated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
MichaelUnkey
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx (2)
27-40: Selection must use stable IDs, not object identity (prevents “phantom” selections).Using
Set<RoleBasic>with.has(role)relies on referential equality. After refetch/pagination/virtualization, row objects are re-instantiated, causing:
- Previously selected rows to render as unselected.
- Footer still showing N selected and attempting to delete stale refs.
Store
roleId(string) in the Set instead. Toggle by ID and derive UI selection fromrole.roleId.Apply:
- const [selectedRoles, setSelectedRoles] = useState<Set<RoleBasic>>(new Set()); + const [selectedRoles, setSelectedRoles] = useState<Set<string>>(new Set()); - const toggleSelection = useCallback((role: RoleBasic) => { + const toggleSelection = useCallback((roleId: string) => { setSelectedRoles((prevSelected) => { const newSelected = new Set(prevSelected); - if (newSelected.has(role)) { - newSelected.delete(role); + if (newSelected.has(roleId)) { + newSelected.delete(roleId); } else { - newSelected.add(role); + newSelected.add(roleId); } return newSelected; }); }, []);Follow-ups in this file and its callsite are covered below.
146-147: Dependency hygiene (optional).
columnsdepends onselectedRolesSet identity. If you later memoize the Set or mutate in place, consider depending onselectedRoles.sizeinstead to avoid stale closures.- [selectedRoles, toggleSelection, hoveredRoleName, selectedRole?.roleId], + [selectedRoles.size, toggleSelection, hoveredRoleName, selectedRole?.roleId],apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (2)
178-181: Clear selection after successful bulk delete.To prevent stale footer state, ensure the delete mutation clears the selection set and the single
selectedPermissionhighlight on success.If handled here, wire a success callback via props:
- <SelectionControls - selectedPermissions={selectedPermissions} - setSelectedPermissions={setSelectedPermissions} - /> + <SelectionControls + selectedPermissionIds={selectedPermissionIds} + setSelectedPermissionIds={setSelectedPermissionIds} + onDeleted={() => { + setSelectedPermission(null); + setSelectedPermissionIds(new Set()); + }} + />
28-41: Replace object-based selection with ID-based selection
UseSet<string>ofpermissionIdinpermissions-list.tsx(instead ofSet<Permission>) to avoid reference‐equality breaks across refetches/virtualization. Update the toggle callback, checkbox handlers, andloadMoreFooterPropsto useselectedPermissionIds/setSelectedPermissionIds, and adjustSelectionControls’ props and itsperformPermissionDeletemapping accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/authorization/permissions/components/table/components/selection-controls/index.tsx(2 hunks)apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx(3 hunks)apps/dashboard/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx(2 hunks)apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-19T11:48:05.070Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3324
File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18
Timestamp: 2025-06-19T11:48:05.070Z
Learning: In the authorization roles refactor, the RoleBasic type uses `roleId` as the property name for the role identifier, not `id`. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.
Applied to files:
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsxapps/dashboard/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: AkshayBandi027
PR: unkeyed/unkey#2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In `authorization/roles/[roleId]/update-role.tsx`, the tag `role-${role.id}` is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
Applied to files:
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsxapps/dashboard/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx
🧬 Code graph analysis (4)
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx (1)
apps/dashboard/lib/trpc/routers/authorization/roles/query.ts (1)
RoleBasic(16-16)
apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (1)
apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts (1)
Permission(19-19)
apps/dashboard/app/(app)/authorization/permissions/components/table/components/selection-controls/index.tsx (1)
apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts (1)
Permission(19-19)
apps/dashboard/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx (1)
apps/dashboard/lib/trpc/routers/authorization/roles/query.ts (1)
RoleBasic(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (3)
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx (1)
165-166: Confirm ID-based selection across consumers. Searches foruseState<Set<RoleBasic>>,.has(role), and allSelectionControlscallsites didn’t surface (TSX types unrecognized). Manually verify every consumer—including footer actions andtoggleSelectionhandlers—now usesSet<string>of IDs and that noSet<RoleBasic>remains.apps/dashboard/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx (1)
19-21: Confirm deletion hook contracts. VerifieduseDeleteRoleanduseDeletePermissionclear selection on success via theironSuccesscallbacks (receiving{ roleIds: string[] | string; message: string }and{ permissionIds: string[] | string; message: string }, respectively). Manually confirm the underlying TRPC procedures accept only{ roleIds: string[] }/{ permissionIds: string[] }as input and adjust if single-string cases sneak through.apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (1)
171-173: Good: stable keys and selection checks use permissionId.Using
permission.permissionIdfor keys and row highlighting is correct and avoids referential pitfalls.
...app/(app)/authorization/permissions/components/table/components/selection-controls/index.tsx
Outdated
Show resolved
Hide resolved
...app/(app)/authorization/permissions/components/table/components/selection-controls/index.tsx
Outdated
Show resolved
Hide resolved
...board/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx
Outdated
Show resolved
Hide resolved
...board/app/(app)/authorization/roles/components/table/components/selection-controls/index.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (2)
64-71: Prevent accidental row selection when clicking the checkboxIf VirtualTable rows handle onClick, the checkbox click may bubble and also change selectedItem. Stop propagation on the checkbox to avoid dual selection changes.
Apply within this block:
{(isSelected || isHovered) && ( <Checkbox checked={isSelected} className="size-4 [&_svg]:size-3" + onClick={(e) => e.stopPropagation()} onCheckedChange={() => toggleSelection(permission.permissionId)} /> )}
27-29: Make hover identity ID-based to avoid duplicate-name collisionsTwo permissions can share the same name; hovering one would light up both. Track hover by permissionId for 1:1 behavior.
- const [hoveredPermissionName, setHoveredPermissionName] = useState<string | null>(null); + const [hoveredPermissionId, setHoveredPermissionId] = useState<string | null>(null);- const isHovered = hoveredPermissionName === permission.name; + const isHovered = hoveredPermissionId === permission.permissionId;- onMouseEnter={() => setHoveredPermissionName(permission.name)} - onMouseLeave={() => setHoveredPermissionName(null)} + onMouseEnter={() => setHoveredPermissionId(permission.permissionId)} + onMouseLeave={() => setHoveredPermissionId(null)}- [selectedPermissions, toggleSelection, hoveredPermissionName, selectedPermission?.permissionId], + [selectedPermissions, toggleSelection, hoveredPermissionId, selectedPermission?.permissionId],Also applies to: 51-53, 61-63, 159-160
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx (1)
63-69: Stop bubbling from the checkbox to the row clickAvoid unintended setSelectedRole when toggling the checkbox.
{(isSelected || isHovered) && ( <Checkbox checked={isSelected} className="size-4 [&_svg]:size-3" + onClick={(e) => e.stopPropagation()} onCheckedChange={() => toggleSelection(role.roleId)} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx(3 hunks)apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3324
File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18
Timestamp: 2025-06-19T11:48:05.070Z
Learning: In the authorization roles refactor, the RoleBasic type uses `roleId` as the property name for the role identifier, not `id`. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.
📚 Learning: 2025-06-19T11:48:05.070Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3324
File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18
Timestamp: 2025-06-19T11:48:05.070Z
Learning: In the authorization roles refactor, the RoleBasic type uses `roleId` as the property name for the role identifier, not `id`. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.
Applied to files:
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: AkshayBandi027
PR: unkeyed/unkey#2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In `authorization/roles/[roleId]/update-role.tsx`, the tag `role-${role.id}` is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
Applied to files:
apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (4)
31-41: ID-based selection toggle: LGTMSwitching to permissionId and Set correctly decouples selection from mutable display fields and unblocks footer delete by ID.
51-53: UI check and handler now keyed by permissionId: LGTMUsing selectedPermissions.has(permission.permissionId) and passing permission.permissionId to toggleSelection brings the row checkbox in sync with footer actions.
Also applies to: 65-71
171-173: Key extractor switched to permissionId: LGTMStabilizes row identity across pagination and renames.
178-181: Verify SelectionControls prop types and delete mutation arguments
Confirm thatSelectionControlsnow accepts aSet<string>forselectedPermissionsand that the delete flow inpermissions-list.tsxinvokes the mutation with permission IDs (not names).apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx (3)
27-41: Move to Set and roleId-based toggle: LGTMThis aligns with the Roles API (RoleBasic.roleId) per prior refactors and fixes footer delete by using stable IDs.
50-53: Selection check and handler by roleId: LGTMCorrectly uses selectedRoles.has(role.roleId) and passes role.roleId to toggleSelection.
Also applies to: 65-69
158-165: Approve SelectionControls usage and delete mutation
SelectionControlsProps correctly usesSet<string>andsetSelectedRoles, and the delete TRPC mutation expectsroleIds(string | string[]) matching the hook’s payload.
| const [hoveredRoleName, setHoveredRoleName] = useState<string | null>(null); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer ID-based hover to prevent duplicate-name hover artifacts
Mirror the selection change for hover.
- const [hoveredRoleName, setHoveredRoleName] = useState<string | null>(null);
+ const [hoveredRoleId, setHoveredRoleId] = useState<string | null>(null);- const isHovered = hoveredRoleName === role.name;
+ const isHovered = hoveredRoleId === role.roleId;- onMouseEnter={() => setHoveredRoleName(role.name)}
- onMouseLeave={() => setHoveredRoleName(null)}
+ onMouseEnter={() => setHoveredRoleId(role.roleId)}
+ onMouseLeave={() => setHoveredRoleId(null)}- [selectedRoles, toggleSelection, hoveredRoleName, selectedRole?.roleId],
+ [selectedRoles, toggleSelection, hoveredRoleId, selectedRole?.roleId],Also applies to: 51-52, 60-62, 146-147
🤖 Prompt for AI Agents
In apps/dashboard/app/(app)/authorization/roles/components/table/roles-list.tsx
around lines 28-29 (and similarly update usages at 51-52, 60-62, 146-147), the
hover state currently stores role.name which causes incorrect hover artifacts
for roles with duplicate names; change the hover state to store the role ID
(e.g. hoveredRoleId: string | null) and update all handlers and comparisons to
use role.id instead of role.name so hover logic mirrors selection (which already
uses IDs). Ensure setter names and variable references are updated consistently
(setHoveredRoleId, comparisons in JSX/handlers) and keep null as the cleared
state.
What does this PR do?
Fixes # (issue)
ENG-1890
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Bug Fixes
Refactor