feat: Role and Permission rows open update dialog#4232
Merged
ogzhanolguncu merged 10 commits intomainfrom Nov 6, 2025
Merged
Conversation
…-make-role-click-open-edit-modal-for-better-ux
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds persistent EditPermission/EditRole modals to permissions and roles list components, prevents row-clicks via Checkbox stopPropagation, tweaks icon/Tag cursor and click behavior, and wraps VirtualTable outputs in fragments while preserving skeleton, loading, paging, and selection behavior. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ListComp as Authorization List
participant Modal as EditModal
User->>ListComp: Click icon or Checkbox
activate ListComp
ListComp->>ListComp: e.stopPropagation() for Checkbox
ListComp->>ListComp: set selectedItem (role/permission)
deactivate ListComp
alt selectedItem present
ListComp->>Modal: Render EditRole/EditPermission (isOpen)
activate Modal
User->>Modal: Edit & submit
Modal->>ListComp: emit update / close
deactivate Modal
ListComp->>ListComp: clear selectedItem
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
Contributor
|
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx(3 hunks)apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: AkshayBandi027
Repo: unkeyed/unkey PR: 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.
📚 Learning: 2025-01-07T19:55:33.055Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2792
File: apps/dashboard/app/(app)/settings/user/update-user-email.tsx:76-78
Timestamp: 2025-01-07T19:55:33.055Z
Learning: In the Unkey codebase, the Empty component can be used as a container for loading states, as demonstrated in the UpdateUserEmail component where it wraps the Loading component.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsxapps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-08-25T13:05:22.441Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3834
File: internal/ui/src/components/form/input.tsx:130-131
Timestamp: 2025-08-25T13:05:22.441Z
Learning: The Input component in internal/ui/src/components/form/input.tsx is designed to support interactive right icons (like X buttons for deletion) by enabling pointer events on the right icon container, allowing clicks to be handled by the icon rather than passed through to the input.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsxapps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-12-03T14:17:08.016Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsxapps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: AkshayBandi027
Repo: unkeyed/unkey PR: 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)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-06-19T11:48:05.070Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-12-03T14:07:45.173Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/components/ui/group-button.tsx:21-31
Timestamp: 2024-12-03T14:07:45.173Z
Learning: In the `ButtonGroup` component (`apps/dashboard/components/ui/group-button.tsx`), avoid suggesting the use of `role="group"` in ARIA attributes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
🧬 Code graph analysis (2)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx (2)
internal/icons/src/icons/page-2.tsx (1)
Page2(15-75)apps/dashboard/components/virtual-table/index.tsx (1)
VirtualTable(38-407)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (2)
internal/icons/src/icons/tag.tsx (1)
Tag(15-39)apps/dashboard/components/virtual-table/index.tsx (1)
VirtualTable(38-407)
⏰ 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). (1)
- GitHub Check: Test Dashboard / Test Dashboard
🔇 Additional comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx (1)
178-264: Modal wiring looks solid.Line 185’s onRowClick hands the row item straight into setSelectedPermission, and Lines 178-265 wrap the VirtualTable with the EditPermission modal without disturbing the table config. Nice, clean integration.
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
Outdated
Show resolved
Hide resolved
…omponents/table/roles-list.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (1)
242-248: Simplify redundantisOpenprop.Since you're already conditionally rendering
EditRolebased onselectedRole(line 242), theisOpen={!!selectedRole}prop is redundant. Consider simplifying toisOpen={true}or removing the prop entirely ifEditRoledefaults to open.{selectedRole && ( <EditRole role={selectedRole} - isOpen={!!selectedRole} + isOpen={true} onClose={() => setSelectedRole(null)} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: AkshayBandi027
Repo: unkeyed/unkey PR: 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)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-06-19T11:48:05.070Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-12-03T14:07:45.173Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/components/ui/group-button.tsx:21-31
Timestamp: 2024-12-03T14:07:45.173Z
Learning: In the `ButtonGroup` component (`apps/dashboard/components/ui/group-button.tsx`), avoid suggesting the use of `role="group"` in ARIA attributes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-04-22T17:33:28.162Z
Learnt from: MichaelUnkey
Repo: unkeyed/unkey PR: 3173
File: apps/docs/security/delete-protection.mdx:32-36
Timestamp: 2025-04-22T17:33:28.162Z
Learning: In the Unkey dashboard UI for delete protection, the button/link to initiate the process is labeled "Disable Delete Protection" while the confirmation button is labeled "Disable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-04-22T17:34:04.438Z
Learnt from: MichaelUnkey
Repo: unkeyed/unkey PR: 3173
File: apps/docs/security/delete-protection.mdx:21-24
Timestamp: 2025-04-22T17:34:04.438Z
Learning: In the Unkey dashboard UI for enabling delete protection, the button/link to initiate the process is labeled "Enable Delete Protection" while the confirmation button is labeled "Enable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-01-07T19:55:33.055Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2792
File: apps/dashboard/app/(app)/settings/user/update-user-email.tsx:76-78
Timestamp: 2025-01-07T19:55:33.055Z
Learning: In the Unkey codebase, the Empty component can be used as a container for loading states, as demonstrated in the UpdateUserEmail component where it wraps the Loading component.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-08-25T13:05:22.441Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3834
File: internal/ui/src/components/form/input.tsx:130-131
Timestamp: 2025-08-25T13:05:22.441Z
Learning: The Input component in internal/ui/src/components/form/input.tsx is designed to support interactive right icons (like X buttons for deletion) by enabling pointer events on the right icon container, allowing clicks to be handled by the icon rather than passed through to the input.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-12-03T14:17:08.016Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (2)
internal/icons/src/icons/tag.tsx (1)
Tag(15-39)apps/dashboard/components/virtual-table/index.tsx (1)
VirtualTable(38-407)
⏰ 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). (2)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (3)
9-9: LGTM! Clean modal integration.The EditRole import and modal rendering structure look good. The selectedRole state effectively manages which role to edit, and the conditional rendering is straightforward.
Also applies to: 27-27, 242-248
70-73: LGTM! Proper event handling to prevent unintended modal opens.The
stopPropagationcalls correctly prevent row clicks from triggering the edit modal when users interact with the selection controls. The Checkbox usesonCheckedChangefor the selection logic whileonClickjust stops propagation.Also applies to: 82-84
165-241: LGTM! Clean VirtualTable integration.The
onRowClick={setSelectedRole}correctly opens the edit modal when clicking a role row, achieving the PR objective. The separation between row selection (selectedRolefor editing) and bulk selection (selectedRolesfor batch operations) is clear and well-implemented.
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
Show resolved
Hide resolved
…-make-role-click-open-edit-modal-for-better-ux
… of https://github.com/unkeyed/unkey into eng-1850-make-role-click-open-edit-modal-for-better-ux
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (1)
234-240: Simplify redundantisOpenprop.Since the EditRole modal only renders when
selectedRoleis truthy (line 234), theisOpen={!!selectedRole}prop on line 237 will always be true. Consider simplifying toisOpen={true}or removing the conditional wrapper in favor of passingisOpen={!!selectedRole}without the outer condition.Apply this diff to remove the redundancy:
- {selectedRole && ( - <EditRole - role={selectedRole} - isOpen={!!selectedRole} - onClose={() => setSelectedRole(null)} - /> - )} + <EditRole + role={selectedRole} + isOpen={!!selectedRole} + onClose={() => setSelectedRole(null)} + />Note: This assumes EditRole handles null role gracefully, otherwise keep the conditional but simplify isOpen to
true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx(3 hunks)apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: AkshayBandi027
Repo: unkeyed/unkey PR: 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)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-06-19T11:48:05.070Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-12-03T14:07:45.173Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/components/ui/group-button.tsx:21-31
Timestamp: 2024-12-03T14:07:45.173Z
Learning: In the `ButtonGroup` component (`apps/dashboard/components/ui/group-button.tsx`), avoid suggesting the use of `role="group"` in ARIA attributes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-04-22T17:33:28.162Z
Learnt from: MichaelUnkey
Repo: unkeyed/unkey PR: 3173
File: apps/docs/security/delete-protection.mdx:32-36
Timestamp: 2025-04-22T17:33:28.162Z
Learning: In the Unkey dashboard UI for delete protection, the button/link to initiate the process is labeled "Disable Delete Protection" while the confirmation button is labeled "Disable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-04-22T17:34:04.438Z
Learnt from: MichaelUnkey
Repo: unkeyed/unkey PR: 3173
File: apps/docs/security/delete-protection.mdx:21-24
Timestamp: 2025-04-22T17:34:04.438Z
Learning: In the Unkey dashboard UI for enabling delete protection, the button/link to initiate the process is labeled "Enable Delete Protection" while the confirmation button is labeled "Enable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-01-07T19:55:33.055Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2792
File: apps/dashboard/app/(app)/settings/user/update-user-email.tsx:76-78
Timestamp: 2025-01-07T19:55:33.055Z
Learning: In the Unkey codebase, the Empty component can be used as a container for loading states, as demonstrated in the UpdateUserEmail component where it wraps the Loading component.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2025-08-25T13:05:22.441Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3834
File: internal/ui/src/components/form/input.tsx:130-131
Timestamp: 2025-08-25T13:05:22.441Z
Learning: The Input component in internal/ui/src/components/form/input.tsx is designed to support interactive right icons (like X buttons for deletion) by enabling pointer events on the right icon container, allowing clicks to be handled by the icon rather than passed through to the input.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
📚 Learning: 2024-12-03T14:17:08.016Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (2)
internal/icons/src/icons/tag.tsx (1)
Tag(15-39)apps/dashboard/components/virtual-table/index.tsx (1)
VirtualTable(38-407)
⏰ 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). (2)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx (3)
9-9: LGTM: EditRole import added for modal functionality.The import is correctly placed and used later to render the edit modal when a role is selected.
54-79: Past issues resolved by refactoring; stopPropagation is correct.The refactoring from Button to Tag icon resolves the previous concerns about incorrect aria-label and unused
group-hoverclasses. Thecursor-pointeris now appropriately placed on the Tag (line 67) rather than the container, and thestopPropagationon line 74 correctly prevents the Checkbox interaction from triggering the row click handler.
157-241: Excellent modal integration pattern.The fragment wrapping and rendering of the EditRole modal alongside the VirtualTable is a clean pattern. The modal opens when a row is clicked (via
onRowClick={setSelectedRole}on line 164) and closes by clearing the selection. This mirrors the pattern in permissions-list.tsx and provides good UX.
…-make-role-click-open-edit-modal-for-better-ux
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx (2)
66-66: Optional: Redundantcursor-pointerclass on icon.The
Page2icon hascursor-pointer, but the entire row already hascursor-pointerfrom the VirtualTable component, making this redundant. Consider removing it for cleaner styling.- <Page2 iconSize="sm-regular" className="text-gray-12 cursor-pointer" /> + <Page2 iconSize="sm-regular" className="text-gray-12" />
250-256: Optional: RedundantisOpenprop.The
isOpen={!!selectedPermission}is redundant since the component is already conditionally rendered based onselectedPermission. When the component renders,isOpenwill always betrue.Consider simplifying:
{selectedPermission && ( <EditPermission permission={selectedPermission} - isOpen={!!selectedPermission} + isOpen={true} onClose={() => setSelectedPermission(null)} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx(3 hunks)apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/roles-list.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-07T19:55:33.055Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2792
File: apps/dashboard/app/(app)/settings/user/update-user-email.tsx:76-78
Timestamp: 2025-01-07T19:55:33.055Z
Learning: In the Unkey codebase, the Empty component can be used as a container for loading states, as demonstrated in the UpdateUserEmail component where it wraps the Loading component.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx
📚 Learning: 2025-08-25T13:05:22.441Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3834
File: internal/ui/src/components/form/input.tsx:130-131
Timestamp: 2025-08-25T13:05:22.441Z
Learning: The Input component in internal/ui/src/components/form/input.tsx is designed to support interactive right icons (like X buttons for deletion) by enabling pointer events on the right icon container, allowing clicks to be handled by the icon rather than passed through to the input.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx
📚 Learning: 2024-12-03T14:17:08.016Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx (2)
internal/icons/src/icons/page-2.tsx (1)
Page2(15-75)apps/dashboard/components/virtual-table/index.tsx (1)
VirtualTable(38-407)
⏰ 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). (2)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx (3)
9-9: LGTM!The import is clean and the component is properly used later in the file.
72-72: Correct use of stopPropagation to isolate checkbox interaction.This prevents checkbox clicks from bubbling up to the row's
onRowClickhandler, ensuring that selecting a permission via checkbox doesn't inadvertently open the edit modal.
167-249: LGTM!The fragment wrapper correctly enables returning multiple elements (VirtualTable + modal), and all VirtualTable props are properly configured. The table behavior, loading states, pagination, and selection functionality are preserved.
|
|
ogzhanolguncu
deleted the
eng-1850-make-role-click-open-edit-modal-for-better-ux
branch
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/06/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
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
3 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.
What does this PR do?
Fixes # (issue)
Eng 1850
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps us understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated