Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors the permissions table from a VirtualTable with cursor-based infinite scrolling to a DataTable with offset-based pagination and sorting; extracts reusable columns, cells, skeletons, and a URL-synced paginated hook into a shared permissions-table surface; and updates the server query to page/limit pagination with dynamic ordering and total calculation. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as PermissionsList (DataTable)
participant Hook as usePermissionsListPaginated
participant URL as URL State
participant TRPC as authorization.permissions.query (tRPC)
participant DB as Database
User->>UI: open page / change page or sort
UI->>Hook: request(page, pageSize, filters, sorting)
Hook->>URL: read/update page & sort state
Hook->>Hook: build server query params (filters, map sorting)
Hook->>TRPC: call query(page, limit, sortBy, sortOrder, filters)
TRPC->>DB: execute SQL with OFFSET/LIMIT and ORDER BY
DB-->>TRPC: return rows + total
TRPC-->>Hook: return permissions[], total
Hook->>URL: sync page/sort and prefetch next pages
Hook-->>UI: permissions, loading flags, page/meta, handlers
UI->>User: render DataTable + PaginationFooter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
…on-table-migration
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
web/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsx (2)
72-85: Consider using a switch or lookup map for skeleton selection.The current approach renders all six conditional expressions for every column. A lookup map would be more efficient and easier to maintain.
♻️ Proposed refactor using a lookup map
+const SKELETON_BY_COLUMN_ID: Record<string, () => React.JSX.Element> = { + [PERMISSION_COLUMN_IDS.PERMISSION.id]: PermissionColumnSkeleton, + [PERMISSION_COLUMN_IDS.SLUG.id]: SlugColumnSkeleton, + [PERMISSION_COLUMN_IDS.USED_IN_ROLES.id]: AssignedRolesColumnSkeleton, + [PERMISSION_COLUMN_IDS.ASSIGNED_TO_KEYS.id]: AssignedKeysColumnSkeleton, + [PERMISSION_COLUMN_IDS.LAST_UPDATED.id]: LastUpdatedColumnSkeleton, + [PERMISSION_COLUMN_IDS.ACTION.id]: ActionColumnSkeleton, +}; + export const renderPermissionsSkeletonRow = ({ columns }: RenderPermissionsSkeletonRowProps) => columns.map((column) => ( <td key={column.id} className={cn("text-xs align-middle whitespace-nowrap", column.meta?.cellClassName)} > - {column.id === PERMISSION_COLUMN_IDS.PERMISSION.id && <PermissionColumnSkeleton />} - {column.id === PERMISSION_COLUMN_IDS.SLUG.id && <SlugColumnSkeleton />} - {column.id === PERMISSION_COLUMN_IDS.USED_IN_ROLES.id && <AssignedRolesColumnSkeleton />} - {column.id === PERMISSION_COLUMN_IDS.ASSIGNED_TO_KEYS.id && <AssignedKeysColumnSkeleton />} - {column.id === PERMISSION_COLUMN_IDS.LAST_UPDATED.id && <LastUpdatedColumnSkeleton />} - {column.id === PERMISSION_COLUMN_IDS.ACTION.id && <ActionColumnSkeleton />} + {column.id && SKELETON_BY_COLUMN_ID[column.id]?.()} </td> ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsx` around lines 72 - 85, The renderPermissionsSkeletonRow function currently evaluates six conditional JSX expressions per column; replace that with a single lookup map or switch to pick the skeleton component by column id to avoid evaluating all conditions. Create a map keyed by PERMISSION_COLUMN_IDS.*.id values mapping to the corresponding components (e.g., PermissionColumnSkeleton, SlugColumnSkeleton, AssignedRolesColumnSkeleton, AssignedKeysColumnSkeleton, LastUpdatedColumnSkeleton, ActionColumnSkeleton) and inside renderPermissionsSkeletonRow render the component by doing a single lookup (fallback to null) and output that result in the <td> so only the matching skeleton is created per column.
67-72: UnusedrowHeightparameter.The
rowHeightproperty is defined inRenderPermissionsSkeletonRowPropsbut is destructured away and never used in the function body. Either use it to set row height styling or remove it from the type definition.♻️ Proposed fix to remove unused parameter
type RenderPermissionsSkeletonRowProps = { columns: DataTableColumnDef<Permission>[]; - rowHeight: number; }; -export const renderPermissionsSkeletonRow = ({ columns }: RenderPermissionsSkeletonRowProps) => +export const renderPermissionsSkeletonRow = ({ columns }: { columns: DataTableColumnDef<Permission>[] }) => columns.map((column) => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsx` around lines 67 - 72, The props type RenderPermissionsSkeletonRowProps declares rowHeight but renderPermissionsSkeletonRow doesn't use it; either remove rowHeight from RenderPermissionsSkeletonRowProps to eliminate the dead prop, or include rowHeight in the function signature (destructure it from props) and apply it to the skeleton row element's style/className (e.g., set style={{ height: rowHeight }} or translate to a CSS variable) so the row height is actually honored; update any call sites or tests to match the chosen approach.web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx (1)
8-16: Filename suggests implementation is for keys when it's actually for permissions.The file
keys-table-action.popover.constants.tsxexportsPermissionsTableActionsand handles permissions-specific logic (importsPermissiontype,DeletePermission,EditPermissioncomponents). The filename should reflect this purpose—consider renaming topermissions-table-action.popover.constants.tsxto avoid confusion during maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx` around lines 8 - 16, The dynamic import is pointing at a misnamed file (keys-table-action.popover.constants.tsx) while the exported symbol PermissionsTableActions and its contents are permissions-specific; rename the source file to permissions-table-action.popover.constants.tsx, update the dynamic import in create-permissions-columns.tsx to import from the new filename, and update any other imports/usages across the repo to the new filename so the PermissionsTableActions export and related Permission, DeletePermission, and EditPermission components match the filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.ts`:
- Around line 95-101: The code treats an empty sortParams array like a present
value, causing effectiveSortParams[0].column/.direction to crash for ?sort= or
empty parses; update the computation and the effect to treat empty arrays as
missing by checking length: replace uses of "sortParams ?? DEFAULT_SORT_PARAMS"
with a length-aware check (e.g., use sortParams && sortParams.length > 0 ?
sortParams : DEFAULT_SORT_PARAMS for effectiveSortParams) and change the
useEffect to set DEFAULT_SORT_PARAMS when (!sortParams || sortParams.length ===
0) before calling setSortParams; apply the same length-check fix to the other
occurrence around the code that mirrors lines 151-152.
- Around line 113-118: The mapped array creation is using an unnecessary type
assertion "as \"asc\" | \"desc\"" which violates the no-as guideline; in the
mapping for mapped (the block using COLUMN_ID_TO_SORT_FIELD and producing {
column, direction }), remove the type assertion and let the ternary (s.desc ?
"desc" : "asc") infer the correct SortDirection literal type so the mapped
entries match the expected type (same approach used in api-keys-list-query.ts
and root-keys-list-query.ts).
In
`@web/apps/dashboard/components/permissions-table/schema/permissions.schema.ts`:
- Around line 17-23: Remove the forbidden `as Record<...>` assertion by making
the reducer’s accumulator type explicit or by fixing the source array typing so
TypeScript can infer the shape: either mark permissionsListFilterFieldNames as a
literal tuple (e.g., use `as const` on its declaration) or supply a generic type
parameter to reduce so the accumulator is typed (e.g., use
reduce<Record<FilterFieldName, typeof baseFilterArraySchema>>). Update the
declaration referenced by permissionsListFilterFieldNames and then remove the
`as Record<FilterFieldName, typeof baseFilterArraySchema>` cast from
filterFieldsSchema so the type relationship between
permissionsListFilterFieldNames, FilterFieldName and the resulting
filterFieldsSchema is checked by the compiler.
In `@web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts`:
- Around line 47-60: The ORDER BY built in buildOrderBy (using
SORT_FIELD_TO_INNER_SQL / SORT_FIELD_TO_OUTER_SQL) is not stable for pagination;
modify buildOrderBy to always append a deterministic secondary sort key (use
"id" for inner context and "p.id" for outer context) to the ORDER BY so rows
with equal primary values never change order across pages, and ensure the
secondary key uses the same sort direction as the primary (use sortOrder to
choose ASC/DESC for the id/p.id tie-breaker); apply this both for the
computed-column fallback (innerColumn) and when column is present.
- Around line 83-89: The current logic for computed sorts (isComputedSort)
applies LIMIT/OFFSET in the outer query so when the requested page is past the
end it returns zero rows and the fallback incorrectly sets total = 0; fix by
adding a separate COUNT query or CTE that computes the total number of filtered
rows independent of the outer LIMIT/OFFSET and use that count to populate
pagination even when the paged slice is empty; update the code that builds
innerOrderBy/outerOrderBy and the empty-result fallback to prefer the standalone
count result for both the computed-sort path and the other branch (also apply
same change around the similar logic referenced later in the file).
---
Nitpick comments:
In
`@web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx`:
- Around line 8-16: The dynamic import is pointing at a misnamed file
(keys-table-action.popover.constants.tsx) while the exported symbol
PermissionsTableActions and its contents are permissions-specific; rename the
source file to permissions-table-action.popover.constants.tsx, update the
dynamic import in create-permissions-columns.tsx to import from the new
filename, and update any other imports/usages across the repo to the new
filename so the PermissionsTableActions export and related Permission,
DeletePermission, and EditPermission components match the filename.
In
`@web/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsx`:
- Around line 72-85: The renderPermissionsSkeletonRow function currently
evaluates six conditional JSX expressions per column; replace that with a single
lookup map or switch to pick the skeleton component by column id to avoid
evaluating all conditions. Create a map keyed by PERMISSION_COLUMN_IDS.*.id
values mapping to the corresponding components (e.g., PermissionColumnSkeleton,
SlugColumnSkeleton, AssignedRolesColumnSkeleton, AssignedKeysColumnSkeleton,
LastUpdatedColumnSkeleton, ActionColumnSkeleton) and inside
renderPermissionsSkeletonRow render the component by doing a single lookup
(fallback to null) and output that result in the <td> so only the matching
skeleton is created per column.
- Around line 67-72: The props type RenderPermissionsSkeletonRowProps declares
rowHeight but renderPermissionsSkeletonRow doesn't use it; either remove
rowHeight from RenderPermissionsSkeletonRowProps to eliminate the dead prop, or
include rowHeight in the function signature (destructure it from props) and
apply it to the skeleton row element's style/className (e.g., set style={{
height: rowHeight }} or translate to a CSS variable) so the row height is
actually honored; update any call sites or tests to match the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7cfb033a-6a9e-446a-a720-41aba4647db2
📒 Files selected for processing (23)
web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/components/last-updated.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/hooks/use-permissions-list-query.tsweb/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/query-logs.schema.tsweb/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/utils/get-row-class.tsweb/apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/components/selection-controls/index.tsxweb/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsxweb/apps/dashboard/components/permissions-table/components/actions/components/delete-permission.tsxweb/apps/dashboard/components/permissions-table/components/actions/components/edit-permission.tsxweb/apps/dashboard/components/permissions-table/components/actions/components/hooks/use-delete-permission.tsweb/apps/dashboard/components/permissions-table/components/actions/components/permission-info.tsxweb/apps/dashboard/components/permissions-table/components/actions/keys-table-action.popover.constants.tsxweb/apps/dashboard/components/permissions-table/components/assigned-items-cell.tsxweb/apps/dashboard/components/permissions-table/components/empty-permissions.tsxweb/apps/dashboard/components/permissions-table/components/permission-name-cell.tsxweb/apps/dashboard/components/permissions-table/components/selection-controls/index.tsxweb/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsxweb/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.tsweb/apps/dashboard/components/permissions-table/index.tsweb/apps/dashboard/components/permissions-table/schema/permissions.schema.tsweb/apps/dashboard/components/permissions-table/utils/get-row-class.tsweb/apps/dashboard/lib/trpc/routers/authorization/permissions/query.tsweb/internal/ui/src/index.ts
💤 Files with no reviewable changes (4)
- web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/components/last-updated.tsx
- web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/query-logs.schema.ts
- web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/utils/get-row-class.ts
- web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/hooks/use-permissions-list-query.ts
Handle empty sort params arrays that caused runtime crashes when accessing effectiveSortParams[0], add deterministic tiebreaker to ORDER BY for stable pagination, fix total count returning 0 when page is past the end, and remove unnecessary type assertion in sort direction mapping.
…ed/unkey into permission-table-migration
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts (1)
86-92: Skip the innerORDER BYfor computed sorts.When
sortByistotalConnectedRoles/totalConnectedKeys, the outer query re-sorts the full filtered set and applies pagination. KeepinginnerOrderByin that path adds a second full sort that doesn't change the result, but does make the slow path heavier.♻️ Suggested change
- const innerOrderBy = buildOrderBy(sortBy, sortOrder, "inner"); + const innerOrderBy = isComputedSort ? sql`` : buildOrderBy(sortBy, sortOrder, "inner"); const outerOrderBy = buildOrderBy(sortBy, sortOrder, "outer");Also applies to: 138-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts` around lines 86 - 92, When sortBy is a computed column (isComputedSort true for "totalConnectedRoles" or "totalConnectedKeys"), skip applying the inner ORDER BY so the heavy sort only runs in the outer query: change how innerOrderBy is built by conditionally calling buildOrderBy only when !isComputedSort (keep outerOrderBy as-is), and apply the same conditional change to the other occurrence where innerOrderBy is created; reference the symbols isComputedSort, innerOrderBy, outerOrderBy, buildOrderBy, sortBy and sortOrder to locate and update both spots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.ts`:
- Around line 95-97: The hook currently keeps an array (sortParams) but only
sends effectiveSortParams[0] to the backend; change this to a single normalized
sort entry: replace effectiveSortParams with a singular effectiveSortParam (take
sortParams?.[0] ?? DEFAULT_SORT_PARAMS[0]) and use that single object everywhere
(including the call to authorization.permissions.query and the table header/sort
state updates). Update places referencing effectiveSortParams (the
initialization and the update logic around the usePermissionsListQuery hook and
the header sort handlers) so the UI only advertises and persists one sort key
consistent with the backend.
- Around line 172-180: The effect currently clamps normalizedPage to totalPages
immediately, causing deep-linked pages to be reset while data is still
undefined; update the useEffect in use-permissions-list-query.ts to only run the
clamp when data (or data.total) is loaded — e.g., add a guard like if (data ==
null) return; before comparing normalizedPage and totalPages, then call
setPage(totalPages) if normalizedPage > totalPages; keep references to
totalCount/totalPages/normalizedPage/useEffect/setPage as-is.
---
Nitpick comments:
In `@web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts`:
- Around line 86-92: When sortBy is a computed column (isComputedSort true for
"totalConnectedRoles" or "totalConnectedKeys"), skip applying the inner ORDER BY
so the heavy sort only runs in the outer query: change how innerOrderBy is built
by conditionally calling buildOrderBy only when !isComputedSort (keep
outerOrderBy as-is), and apply the same conditional change to the other
occurrence where innerOrderBy is created; reference the symbols isComputedSort,
innerOrderBy, outerOrderBy, buildOrderBy, sortBy and sortOrder to locate and
update both spots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3e9ebf04-2637-40b5-a898-9a3b44162cda
📒 Files selected for processing (3)
web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsxweb/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.tsweb/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/apps/dashboard/components/permissions-table/components/actions/components/permission-info.tsx`:
- Around line 17-26: The description container is rendered even when
permissionDetails.description is falsy; update the JSX so the InfoTooltip and
its inner div are only rendered when permissionDetails.description exists (e.g.,
wrap the InfoTooltip block in a conditional check like
permissionDetails.description && ...), ensuring both the tooltip and the text
div (the truncated description) are omitted when there's no description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 84a9930a-0dfc-4ea3-b926-ef5e11b89576
📒 Files selected for processing (2)
web/apps/dashboard/components/permissions-table/components/actions/components/permission-info.tsxweb/apps/dashboard/components/permissions-table/components/permission-name-cell.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/apps/dashboard/components/permissions-table/components/permission-name-cell.tsx
permission description Prevent deep-linked page numbers from being reset to 1 before query data loads by guarding the clamp effect with a null check. Skip rendering the InfoTooltip and description div when no description exists. Fix cn import source to match directory convention.
chronark
left a comment
There was a problem hiding this comment.
seems to work great :)
| const tiebreaker = context === "inner" ? "id" : "p.id"; | ||
|
|
||
| return sql`ORDER BY ${sql.raw(primaryCol)} ${direction}, ${sql.raw(tiebreaker)} ${direction}`; | ||
| } |
There was a problem hiding this comment.
this feel iffy to me. evne though sortBy input is validated by the Zod enum (permissionsSortByEnum), and its currently safe, the defense relies on the Zod validation happening upstream. there's no local assertion that primaryCol is one of the expected values.
we want to be really careful when we use sql.raw because it bypasses parameterization, so any future code path that reaches it with unexpected input would be dangerous.
i would suggest a guard here of explicitly allowed columns just for some more defense-in-depth against sql injections.
// validate the resolved column is from the allowlist
const VALID_COLUMNS = new Set(Object.values(map));
if (!VALID_COLUMNS.has(primaryCol)) {
throw new Error(`Invalid sort column: ${primaryCol}`);
}
return sql`ORDER BY ${sql.raw(primaryCol)} ${direction}, ${sql.raw(tiebreaker)} ${direction}`;2. Tiebreaker column validated (security) 3. Duplicate roleFilterForCount removed 4. || to ?? for string null coercion — row.name || "" coerces all falsy values (including a legitimate empty string ""). Switched to ?? for the three string fields. Kept || for the numeric Number() fields because Number() returns NaN (not null) for bad input, and NaN || 0 correctly falls back to 0. 5. buildRoleFilter duplication (refactor) — The name and ID filter branches were 80+ lines of near-identical subquery logic differing only in r.name vs r.id. Extracted buildRoleCondition with a typed roleColumn: "r.name" | "r.id" parameter. 6. make fmt
What does this PR do?
Fixes # (issue)
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