Conversation
...s/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/index.tsx
Show resolved
Hide resolved
|
@mcstepp can you check again I really wanna merge this one today. Lemme know if something comes up |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
apps/dashboard/app/(app)/authorization/roles/components/upsert-role/components/assign-permission/create-permission-options.tsx (1)
38-38: Add accessibility attributes to decorative icons.As mentioned in previous reviews, these
<Page2>icons are decorative and should includearia-hidden="true"to prevent screen readers from announcing them during list navigation.-<Page2 size="sm-regular" className="text-grayA-11" /> +<Page2 size="sm-regular" className="text-grayA-11" aria-hidden="true" />Also applies to: 121-121
apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (1)
64-64: Add accessibility attributes for the icon-only button.The icon-only button lacks proper accessibility attributes. Consider adding
aria-label="Select permission"androle="checkbox"witharia-checked={isSelected}to make it accessible to screen readers.-{!isSelected && !isHovered && <Page2 size="sm-regular" className="text-gray-12" />} +{!isSelected && !isHovered && <Page2 size="sm-regular" className="text-gray-12" aria-hidden="true" />}Additionally, the parent container should have appropriate ARIA attributes for the interactive button.
apps/dashboard/components/ui/form-combobox.tsx (1)
35-38: Resolve potential type conflict with native HTML attribute.The
titleprop conflicts with the native HTMLtitleattribute. Consider renaming it to avoid type conflicts:-/** - * Tooltip text displayed on hover - */ -title?: string; +/** + * Tooltip text displayed on hover + */ +tooltipTitle?: string;apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-permission/permissions-field.tsx (4)
70-74: Fix potential runtime error with description filtering.The code calls
includes()onpermission.description?.toLowerCase()which can be undefined, causing a runtime error.-permission.description?.toLowerCase().includes(searchTerm), +(permission.description?.toLowerCase().includes(searchTerm) ?? false),
146-154: Improve fallback permission object structure.The fallback permission object is minimal and might cause issues if other UI components expect complete permission objects.
-// Fallback for unknown permissions -permissionsList.push({ - id: permissionId, - name: permissionId, - slug: permissionId, - description: null, - source: "direct" as const, - isInherited: false, -}); +// Fallback for unknown permissions +permissionsList.push({ + id: permissionId, + name: `Unknown Permission (${permissionId})`, + slug: `unknown-${permissionId}`, + description: null, + source: "direct" as const, + isInherited: false, +});
242-244: Handle undefined role name in tooltip.The tooltip content could display "Inherited from role: undefined" if the role is not found.
-content={`Inherited from role: ${ - assignedRoleDetails.find((r) => r.id === permission.roleId)?.name -}`} +content={`Inherited from role: ${ + assignedRoleDetails.find((r) => r.id === permission.roleId)?.name ?? permission.roleId +}`}
30-31: Consider optimizing search term usage.As suggested in previous comments, consider defining
searchTermonce and reusing it to reduce repetitivetrim()operations:const [searchValue, setSearchValue] = useState(""); -const trimmedSearchVal = searchValue.trim(); +const searchTerm = searchValue.trim().toLowerCase();Then use
searchTermthroughout the component instead of recalculating it multiple times.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-role/role-field.tsx (4)
112-117: Prevent sentinel value from corrupting form state.The
handleAddRolefunction doesn't filter out the"__load_more__"sentinel value, which could corrupt the form payload.const handleAddRole = (roleId: string) => { + if (roleId === "__load_more__") return; // ignore sentinel option if (!value.includes(roleId)) { onChange([...value, roleId]); } setSearchValue(""); };
75-106: Optimize selectedRoles computation for better performance.The
selectedRolesmemoization performs expensive.find()operations on potentially large arrays. Consider creating lookup maps for O(1) access:const selectedRoles = useMemo(() => { + // Create lookup maps for O(1) access + const assignedRoleMap = new Map(assignedRoleDetails.map(r => [r.id, r])); + const allRoleMap = new Map(allRoles.map(r => [r.id, r])); + return value .map((roleId) => { - const preLoadedRole = assignedRoleDetails.find((r) => r.id === roleId); + const preLoadedRole = assignedRoleMap.get(roleId); if (preLoadedRole) { return { id: preLoadedRole.id, name: preLoadedRole.name, description: preLoadedRole.description, }; } - const loadedRole = allRoles.find((r) => r.id === roleId); + const loadedRole = allRoleMap.get(roleId); if (loadedRole) { return { id: loadedRole.id, name: loadedRole.name, description: loadedRole.description, }; } // ... rest remains the same }) .filter((role): role is NonNullable<typeof role> => role !== undefined); }, [value, allRoles, assignedRoleDetails]);
138-138: Add ARIA attributes to loading spinner.The loading spinner lacks proper accessibility attributes:
-<div className="animate-spin h-3 w-3 border border-gray-6 border-t-gray-11 rounded-full" /> +<div + className="animate-spin h-3 w-3 border border-gray-6 border-t-gray-11 rounded-full" + role="status" + aria-label="Loading" +/>
166-166: Add accessibility attribute to decorative icon.The
<Tag>icon is decorative and should includearia-hidden="true":-renderIcon={() => <Tag size="sm-regular" className="text-grayA-11" />} +renderIcon={() => <Tag size="sm-regular" className="text-grayA-11" aria-hidden="true" />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-permission/permissions-field.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-role/role-field.tsx(1 hunks)apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx(2 hunks)apps/dashboard/app/(app)/authorization/roles/components/upsert-role/components/assign-permission/create-permission-options.tsx(3 hunks)apps/dashboard/components/ui/form-combobox.tsx(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (4)
apps/dashboard/lib/trpc/routers/index.ts (2)
46-51: RBAC imports look well-structured and properly named.The new imports follow consistent naming patterns and are organized logically for the RBAC functionality.
140-141: New key-level endpoints are appropriately placed.The
queryPermissionSlugsandconnectedRolesAndPermsendpoints are correctly positioned at the key router level and follow the established naming conventions.apps/dashboard/app/(app)/authorization/permissions/components/table/permissions-list.tsx (1)
5-7: LGTM - Import consolidation and icon update.The import path change to use the UI package utilities and icon replacement with
Page2aligns with the broader iconography update across the RBAC components.apps/dashboard/components/ui/form-combobox.tsx (1)
34-34: LGTM - Loading prop addition.The
loadingprop addition enhances UI feedback for async operations in combobox components.
...e/components/actions/components/edit-rbac/components/assign-permission/permissions-field.tsx
Show resolved
Hide resolved
mcstepp
left a comment
mcstepp
left a comment
There was a problem hiding this comment.
I viewed this like 4 times lol because i kept losing my place 🫠
I also tested it, and no blocking issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-role/create-key-options.tsx (1)
48-55: Duplicate selection issue remains unaddressed.The "Already assigned" badge provides visual feedback but doesn't prevent duplicate selections, which could lead to form state issues and server errors. The past review comment about this issue has not been resolved.
Apply this fix to prevent duplicate selections:
const options = roles.map((role) => ({ label: ( // ... existing JSX ), selectedLabel: ( // ... existing JSX ), value: role.id, + disabled: previouslySelectedRoleIds.includes(role.id) || role.keys.some(k => k.id === keyId), searchValue: `${role.name} ${role.description || ""} ${role.id}`.trim(), }));Ensure the consuming Select component respects the
disabledfield to prevent selection of already assigned roles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-role/create-key-options.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-rbac/components/assign-role/create-key-options.tsx (3)
1-3: LGTM! Clean imports with appropriate UI components.The imports are well-organized and include all necessary components for the tooltip-rich role selection interface.
5-26: Well-structured type definitions.The
RoleandRoleSelectorPropstypes are comprehensive and clearly define the expected data structure and function parameters.
134-138: Good event handling for load more button.The event prevention and propagation stoppage correctly handles the button click within the select dropdown context.
.../table/components/actions/components/edit-rbac/components/assign-role/create-key-options.tsx
Show resolved
Hide resolved
.../table/components/actions/components/edit-rbac/components/assign-role/create-key-options.tsx
Show resolved
Hide resolved
2 participants
What does this PR do?
This PR adds a dialog to the
/keyspage to connect keys to roles and permissions. It allows binding keys to permissions and roles separately. When a permission is inherited through a role, we display indicators and prevent users from deleting those permissions unless they first remove the associated roles.The connection between keys, roles, and permissions is complex, but our UI allows users to perform actions without submitting the form. For example, they can remove a role (which deletes all its connected permissions) and then add that role as a direct permission to the key.
We prefetch all dialog-related data before opening to improve the user experience. This PR also includes optimizations for /roles and /permissions pages, primarily around prefetching and pagination.
Also introduces
<SelectItem />component for showing details of selected item with tooltips and icons.Fixes # (issue)
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?
Case - 1
Inherited from roleand when you hover it shows where its being inherited from.Case - 2
Case - 3
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation