Skip to content

feat: Permission table migration#5681

Open
MichaelUnkey wants to merge 17 commits intomainfrom
permission-table-migration
Open

feat: Permission table migration#5681
MichaelUnkey wants to merge 17 commits intomainfrom
permission-table-migration

Conversation

@MichaelUnkey
Copy link
Copy Markdown
Collaborator

What does this PR do?

  • Migrate the permissions table from VirtualTable (infinite scroll) to the @unkey/ui DataTable system with server-side pagination, sorting, and prefetching
  • Restructure permissions table components from a deeply nested route folder into a reusable @/components/permissions-table/ module with barrel exports
  • Convert the backend from cursor-based infinite queries (useInfiniteQuery) to page-based queries (useQuery) with LIMIT/OFFSET, supporting server-side ORDER BY on all columns including computed fields (connected roles/keys counts)
  • Extract inline column definitions, skeleton rows, empty state, and permission name cell into dedicated components
  • Fix selection controls animation in the roles table (switch from y-translate to height-based animation to prevent layout shift)

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

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Navigate to Authorization > Permissions page — table should load with permissions sorted by "Last Updated" descending by default
  • Click each sortable column header (Permission, Slug, Used in Roles, Assigned to Keys, Last Updated) — verify sort toggles asc/desc correctly, URL sort param updates, and page resets to 1
  • Verify pagination footer shows correct count (e.g., "Showing 50 of 120 permissions") and page controls work (next, previous, first, last)
  • Navigate to page 2+, then change sort — verify page resets to 1
  • Apply filters (name, slug, etc.) — verify results update, page resets to 1, and pagination reflects filtered count
  • With no permissions configured, verify the empty state renders with "No Permissions Found" and the docs link
  • Select permissions via checkbox — verify selection controls banner animates in/out smoothly
  • Click a permission row — verify the edit side panel opens correctly
  • Verify skeleton loading rows render correctly on initial load
  • Navigate to Authorization > Roles — verify the selection controls animation fix (height-based) works without layout shift
  • Verify the URL reflects page and sort query params and that refreshing the page preserves state

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Ran make fmt on /go directory
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
dashboard Ready Ready Preview, Comment Apr 16, 2026 3:26pm

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 9, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: bac15e7e-4813-4874-afa3-084e50cbf659

📥 Commits

Reviewing files that changed from the base of the PR and between 1f159c3 and 36d839f.

📒 Files selected for processing (2)
  • web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/components/deployment-row.tsx
  • web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts
✅ Files skipped from review due to trivial changes (1)
  • web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/components/deployment-row.tsx

📝 Walkthrough

Walkthrough

Refactors 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

Cohort / File(s) Summary
Removed legacy table internals
web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/components/last-updated.tsx, web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/hooks/use-permissions-list-query.ts, 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
Deleted legacy LastUpdated component, cursor-based infinite-query hook, old Zod query schema, and previous row-class utility tied to the VirtualTable/infinite-scroll implementation.
New shared permissions-table surface
web/apps/dashboard/components/permissions-table/index.ts, web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx, web/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.ts, web/apps/dashboard/components/permissions-table/schema/permissions.schema.ts, web/apps/dashboard/components/permissions-table/utils/get-row-class.ts
Added a barrel export and reusable artifacts: column factory (createPermissionsColumns + PERMISSION_COLUMN_IDS), URL-synced paginated hook (usePermissionsListPaginated), Zod schema (permissionsQueryPayload) for page/limit/sort, and a row-class utility re-exporting STATUS_STYLES.
UI components & skeletons
web/apps/dashboard/components/permissions-table/components/empty-permissions.tsx, web/apps/dashboard/components/permissions-table/components/permission-name-cell.tsx, web/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsx
Added EmptyPermissions, PermissionNameCell (selection/hover/description behavior), and a typed skeleton-row renderer with per-column skeletons for DataTable.
PermissionsList migration
web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.tsx, web/apps/dashboard/components/permissions-table/components/actions/components/edit-permission.tsx
Replaced VirtualTable + infinite scroll with DataTable + PaginationFooter; switched from cursor hook to paginated hook and extracted columns; updated UpsertPermissionDialog import path.
Backend: pagination & sorting
web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts
Replaced cursor-based pagination with page/limit (OFFSET/LIMIT); removed hasMore/nextCursor from response; added dynamic ORDER BY via buildOrderBy supporting computed sort fields; ensure total returned for empty pages and default null counts to 0.
Selection animation & tooltip tweaks
web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/roles/components/table/components/selection-controls/index.tsx, web/apps/dashboard/components/permissions-table/components/actions/components/permission-info.tsx
Changed selection-controls animation to height-based transitions; adjusted permission-info tooltip to show description only and tuned padding/width classes.
Package exports update
web/internal/ui/src/index.ts
Expanded UI package entry exports to include CSS and lib utils re-exports.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: migrating the permissions table from VirtualTable to DataTable with pagination.
Description check ✅ Passed The description covers all required sections: what the PR does, type of change (New feature selected), comprehensive testing instructions, and all required checklist items completed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch permission-table-migration

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Skeleton flash on page navigation
MAX_PAGE_SIZE mismatch
Inconsistent null coalescing
Misleading parameter name
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: Unused rowHeight parameter.

The rowHeight property is defined in RenderPermissionsSkeletonRowProps but 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.tsx exports PermissionsTableActions and handles permissions-specific logic (imports Permission type, DeletePermission, EditPermission components). The filename should reflect this purpose—consider renaming to permissions-table-action.popover.constants.tsx to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 587312b and 9ee0f4d.

📒 Files selected for processing (23)
  • web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/components/last-updated.tsx
  • web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/hooks/use-permissions-list-query.ts
  • web/apps/dashboard/app/(app)/[workspaceSlug]/authorization/permissions/components/table/permissions-list.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/roles/components/table/components/selection-controls/index.tsx
  • web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx
  • web/apps/dashboard/components/permissions-table/components/actions/components/delete-permission.tsx
  • web/apps/dashboard/components/permissions-table/components/actions/components/edit-permission.tsx
  • web/apps/dashboard/components/permissions-table/components/actions/components/hooks/use-delete-permission.ts
  • web/apps/dashboard/components/permissions-table/components/actions/components/permission-info.tsx
  • web/apps/dashboard/components/permissions-table/components/actions/keys-table-action.popover.constants.tsx
  • web/apps/dashboard/components/permissions-table/components/assigned-items-cell.tsx
  • web/apps/dashboard/components/permissions-table/components/empty-permissions.tsx
  • web/apps/dashboard/components/permissions-table/components/permission-name-cell.tsx
  • web/apps/dashboard/components/permissions-table/components/selection-controls/index.tsx
  • web/apps/dashboard/components/permissions-table/components/skeletons/render-permissions-skeleton-row.tsx
  • web/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.ts
  • web/apps/dashboard/components/permissions-table/index.ts
  • web/apps/dashboard/components/permissions-table/schema/permissions.schema.ts
  • web/apps/dashboard/components/permissions-table/utils/get-row-class.ts
  • web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts
  • web/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

Comment thread web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts Outdated
  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.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
web/apps/dashboard/lib/trpc/routers/authorization/permissions/query.ts (1)

86-92: Skip the inner ORDER BY for computed sorts.

When sortBy is totalConnectedRoles / totalConnectedKeys, the outer query re-sorts the full filtered set and applies pagination. Keeping innerOrderBy in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9ee0f4d and 199c0c2.

📒 Files selected for processing (3)
  • web/apps/dashboard/components/permissions-table/columns/create-permissions-columns.tsx
  • web/apps/dashboard/components/permissions-table/hooks/use-permissions-list-query.ts
  • web/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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 199c0c2 and a5a3508.

📒 Files selected for processing (2)
  • web/apps/dashboard/components/permissions-table/components/actions/components/permission-info.tsx
  • web/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.
Copy link
Copy Markdown
Collaborator

@chronark chronark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to work great :)

const tiebreaker = context === "inner" ? "id" : "p.id";

return sql`ORDER BY ${sql.raw(primaryCol)} ${direction}, ${sql.raw(tiebreaker)} ${direction}`;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants