feat: migrate api requests table to shared DataTable component#5386
Open
MichaelUnkey wants to merge 10 commits intomainfrom
Open
feat: migrate api requests table to shared DataTable component#5386MichaelUnkey wants to merge 10 commits intomainfrom
MichaelUnkey wants to merge 10 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (3)
📝 WalkthroughWalkthroughReplaces a local VirtualTable with the shared TanStack-based DataTable for key/log overview tables, centralizes column and cell definitions into a new api-requests-table module and Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MichaelUnkey
requested review from
Flo4604,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/apps/dashboard/components/api-requests-table/schema/keys-overview.schema.ts (1)
14-14:⚠️ Potential issue | 🟡 MinorRedundant
.nullable()modifier.The
cursorfield has.nullable().optional().nullable()which appliesnullabletwice. This is redundant and likely unintentional.🔧 Proposed fix
- cursor: z.number().nullable().optional().nullable(), + cursor: z.number().nullable().optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/api-requests-table/schema/keys-overview.schema.ts` at line 14, The `cursor` schema entry currently uses `.nullable()` twice (`cursor: z.number().nullable().optional().nullable()`); remove the duplicate modifier so the field reads e.g. `cursor: z.number().nullable().optional()` (or `z.number().optional().nullable()`), keeping only a single `.nullable()` and preserving `.optional()` as intended on the `cursor` symbol in keys-overview.schema.ts.
🧹 Nitpick comments (2)
web/apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx (1)
53-61: Type assertionas SortFieldsat API boundary.While the cast on line 57 is pragmatic for bridging TanStack's
stringid type to yourSortFieldsunion, it bypasses compile-time safety. Consider adding a runtime guard or using a type predicate to validate the column ID.♻️ Safer alternative with runtime validation
+const isSortField = (id: string): id is SortFields => + Object.values(API_REQUEST_COLUMN_IDS).some((col) => col.id === id); + const handleSortingChange = useCallback( (updater: SortingState | ((old: SortingState) => SortingState)) => { const next = typeof updater === "function" ? updater(sorting) : updater; setSorts( - next.map((s) => ({ column: s.id as SortFields, direction: s.desc ? "desc" : "asc" })), + next + .filter((s) => isSortField(s.id)) + .map((s) => ({ column: s.id as SortFields, direction: s.desc ? "desc" : "asc" })), ); }, [sorting, setSorts], );As per coding guidelines: "Never compromise type safety: No
any, no!(non-null assertion), noas Type"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/app/`(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx around lines 53 - 61, The code uses an unsafe type assertion (s.id as SortFields) in handleSortingChange; replace it with a runtime guard that validates s.id before converting to your SortFields union: implement a type predicate like isSortField(id): id is SortFields and use it to filter/map the incoming next array (from sorting) so you only call setSorts with entries whose s.id passes isSortField, and handle or ignore invalid ids instead of casting; update references in handleSortingChange and any related consumers that assume SortFields.web/apps/dashboard/components/api-requests-table/columns/create-api-request-columns.tsx (1)
16-22: Consider extractingTruncatedTextCellfor reuse.This helper is defined inline but could be useful in other column definitions. Consider moving it to the shared cells components in
@unkey/uiif truncation with tooltip is a common pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/api-requests-table/columns/create-api-request-columns.tsx` around lines 16 - 22, TruncatedTextCell is defined inline and should be extracted for reuse: create a shared component (e.g., TruncatedTextCell) inside the shared cells library (`@unkey/ui`) that accepts a value prop (string) and preserves the current layout/props (flex container, mono font, max-w-37.5, truncate, title tooltip). Replace the inline definition in create-api-request-columns.tsx with an import from `@unkey/ui` and update any other column files to import the shared TruncatedTextCell to avoid duplication.
🤖 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/app/`(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx:
- Around line 63-66: Change getRowClassName to accept a nullable second argument
and remove the unsafe cast: update the function signature in get-row-class.ts
(the exported getRowClassName) to take (log: KeysOverviewLog, selectedLog:
KeysOverviewLog | null) and adjust any internal typings accordingly (it already
uses optional chaining). Then in logs-table.tsx remove the "as KeysOverviewLog"
cast in getRowClassNameMemoized so it calls getRowClassName(log, selectedLog)
with selectedLog possibly null, and ensure getRowClassNameMemoized still depends
on [selectedLog].
In
`@web/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx`:
- Around line 26-27: The KEY column metadata is using accessorKey: "key" but the
RootKey model exposes the field as start and the cell renderer reads
row.original; update the KEY column in create-root-key-columns (symbol KEY) to
correctly align the accessor with the data by replacing accessorKey: "key" with
either accessorKey: "start" or an explicit accessorFn: (row) => row.start so
TanStack can use the accessor for sorting/filtering/export; ensure the custom
cell renderer that uses row.original continues to work after this change.
---
Outside diff comments:
In
`@web/apps/dashboard/components/api-requests-table/schema/keys-overview.schema.ts`:
- Line 14: The `cursor` schema entry currently uses `.nullable()` twice
(`cursor: z.number().nullable().optional().nullable()`); remove the duplicate
modifier so the field reads e.g. `cursor: z.number().nullable().optional()` (or
`z.number().optional().nullable()`), keeping only a single `.nullable()` and
preserving `.optional()` as intended on the `cursor` symbol in
keys-overview.schema.ts.
---
Nitpick comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx:
- Around line 53-61: The code uses an unsafe type assertion (s.id as SortFields)
in handleSortingChange; replace it with a runtime guard that validates s.id
before converting to your SortFields union: implement a type predicate like
isSortField(id): id is SortFields and use it to filter/map the incoming next
array (from sorting) so you only call setSorts with entries whose s.id passes
isSortField, and handle or ignore invalid ids instead of casting; update
references in handleSortingChange and any related consumers that assume
SortFields.
In
`@web/apps/dashboard/components/api-requests-table/columns/create-api-request-columns.tsx`:
- Around line 16-22: TruncatedTextCell is defined inline and should be extracted
for reuse: create a shared component (e.g., TruncatedTextCell) inside the shared
cells library (`@unkey/ui`) that accepts a value prop (string) and preserves the
current layout/props (flex container, mono font, max-w-37.5, truncate, title
tooltip). Replace the inline definition in create-api-request-columns.tsx with
an import from `@unkey/ui` and update any other column files to import the shared
TruncatedTextCell to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5e39c64-7baf-45aa-b255-629c97645372
📒 Files selected for processing (27)
web/apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/logs-client.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/utils/get-row-class.tsweb/apps/dashboard/components/api-requests-table/columns/create-api-request-columns.tsxweb/apps/dashboard/components/api-requests-table/columns/index.tsweb/apps/dashboard/components/api-requests-table/components/key-identifier-column.tsxweb/apps/dashboard/components/api-requests-table/components/log-details/components/log-header.tsxweb/apps/dashboard/components/api-requests-table/components/log-details/components/log-outcome-distribution-section.tsxweb/apps/dashboard/components/api-requests-table/components/log-details/components/log-section.tsxweb/apps/dashboard/components/api-requests-table/components/log-details/components/roles-permissions.tsxweb/apps/dashboard/components/api-requests-table/components/log-details/index.tsxweb/apps/dashboard/components/api-requests-table/hooks/use-keys-overview-query.tsweb/apps/dashboard/components/api-requests-table/schema/keys-overview.schema.tsweb/apps/dashboard/components/api-requests-table/utils/calculate-blocked-percentage.tsweb/apps/dashboard/components/api-requests-table/utils/get-row-class.tsweb/apps/dashboard/components/logs/hooks/use-sort.tsxweb/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsxweb/apps/dashboard/components/root-keys-table/components/skeletons/render-root-key-skeleton-row.tsxweb/apps/dashboard/lib/trpc/routers/api/keys/query-overview-logs/index.tsweb/apps/dashboard/lib/trpc/routers/api/keys/query-overview-logs/utils.tsweb/internal/ui/src/components/data-table/components/cells/index.tsweb/internal/ui/src/components/data-table/components/cells/invalid-count-cell.tsxweb/internal/ui/src/components/data-table/components/cells/outcome-popover-cell.tsxweb/internal/ui/src/components/data-table/components/empty/empty-api-requests.tsxweb/internal/ui/src/components/data-table/components/empty/index.tsweb/internal/ui/src/components/data-table/data-table.tsxweb/internal/ui/src/components/data-table/index.ts
💤 Files with no reviewable changes (1)
- web/apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/table/utils/get-row-class.ts
...s/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx
Show resolved
Hide resolved
web/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx (2)
89-105: Minor inconsistency: PERMISSIONS column doesn't use its descriptor's accessorKey.The descriptor defines
accessorKey: "permissions"but the column definition omits it. While this works sinceenableSorting: falsemeans TanStack won't use the accessor for sorting, it's inconsistent with the other columns that fully utilize their descriptors.Consider either adding
accessorKey: ROOT_KEY_COLUMN_IDS.PERMISSIONS.accessorKeyfor consistency, or removing the unusedaccessorKeyfrom the descriptor definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx` around lines 89 - 105, Add the missing accessorKey to the PERMISSIONS column for consistency: update the column object that uses ROOT_KEY_COLUMN_IDS.PERMISSIONS to include accessorKey: ROOT_KEY_COLUMN_IDS.PERMISSIONS.accessorKey (leave other props like enableSorting and cell using AssignedItemsCell unchanged). This keeps the column descriptor consistent with the other columns that use their descriptor.accessorKey, or alternatively remove the unused accessorKey from the descriptor itself if you prefer that approach; reference ROOT_KEY_COLUMN_IDS.PERMISSIONS, accessorKey, the column object in create-root-key-columns.tsx, and the cell render that reads rootKey.permissionSummary/selectedRootKeyId when making the change.
149-151: Minor inconsistency: ACTION column header doesn't use its descriptor.The descriptor defines
header: "Action"but line 151 hardcodesheader: "". This works functionally since action columns typically don't display headers, but creates an inconsistency with how other columns derive their headers.Consider either using
ROOT_KEY_COLUMN_IDS.ACTION.headerhere or changing the descriptor's header to an empty string to accurately reflect usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx` around lines 149 - 151, The ACTION column header is hardcoded to an empty string but the descriptor ROOT_KEY_COLUMN_IDS.ACTION defines a non-empty header, creating an inconsistency; update the column definition in create-root-key-columns (the object with id: ROOT_KEY_COLUMN_IDS.ACTION.id) to use ROOT_KEY_COLUMN_IDS.ACTION.header instead of "" (or alternatively change ROOT_KEY_COLUMN_IDS.ACTION.header to "" if you prefer the descriptor to reflect no visible header) so the header source is consistent with other columns.
🤖 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/app/`(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx:
- Around line 40-42: The visual selection mismatch is caused by getRowClassName
comparing log.key_id to selectedLog?.key_id while rowSelection and getRowId use
request_id; modify getRowClassName (in get-row-class.ts) to check equality using
log.request_id === selectedLog?.request_id (or otherwise derive the same row id
used by getRowId) so the CSS highlight matches the controlled rowSelection state
that keys by request_id.
- Line 31: The paginated hook useKeysOverviewLogsQuery returns loadMore and
isLoadingMore but the component only destructures historicalLogs, isLoading,
hasMore and hides the footer; update the destructure to include loadMore and
isLoadingMore, remove the explicit hide: true on the footer, and wire the footer
props by passing onLoadMore: loadMore and isFetchingNextPage: isLoadingMore to
loadMoreFooterProps (keep using historicalLogs/isLoading/hasMore as before);
alternatively, if pagination should be removed, modify useKeysOverviewLogsQuery
to return the full dataset without loadMore and update calls that expect
loadMore/isLoadingMore accordingly.
- Around line 53-58: The handleSortingChange callback currently casts s.id with
"as SortFields", which breaks type safety; replace that cast by
validating/parsing s.id before calling setSorts: implement a small guard or
mapping (e.g. isSortField(id): id is SortFields or a map from string to
SortFields) and use it when transforming next.map(s => ...), filtering out or
rejecting unknown ids, and then call setSorts with only validated SortFields;
reference handleSortingChange, SortingState, SortFields, s.id and setSorts so
the unsafe assertion is removed and only parsed/guarded sort ids reach setSorts.
---
Nitpick comments:
In
`@web/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx`:
- Around line 89-105: Add the missing accessorKey to the PERMISSIONS column for
consistency: update the column object that uses ROOT_KEY_COLUMN_IDS.PERMISSIONS
to include accessorKey: ROOT_KEY_COLUMN_IDS.PERMISSIONS.accessorKey (leave other
props like enableSorting and cell using AssignedItemsCell unchanged). This keeps
the column descriptor consistent with the other columns that use their
descriptor.accessorKey, or alternatively remove the unused accessorKey from the
descriptor itself if you prefer that approach; reference
ROOT_KEY_COLUMN_IDS.PERMISSIONS, accessorKey, the column object in
create-root-key-columns.tsx, and the cell render that reads
rootKey.permissionSummary/selectedRootKeyId when making the change.
- Around line 149-151: The ACTION column header is hardcoded to an empty string
but the descriptor ROOT_KEY_COLUMN_IDS.ACTION defines a non-empty header,
creating an inconsistency; update the column definition in
create-root-key-columns (the object with id: ROOT_KEY_COLUMN_IDS.ACTION.id) to
use ROOT_KEY_COLUMN_IDS.ACTION.header instead of "" (or alternatively change
ROOT_KEY_COLUMN_IDS.ACTION.header to "" if you prefer the descriptor to reflect
no visible header) so the header source is consistent with other columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 200753c0-be6d-410e-9477-8cabbd7e079e
📒 Files selected for processing (3)
web/apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsxweb/apps/dashboard/components/api-requests-table/utils/get-row-class.tsweb/apps/dashboard/components/root-keys-table/columns/create-root-key-columns.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/apps/dashboard/components/api-requests-table/utils/get-row-class.ts
...s/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx
Show resolved
Hide resolved
...s/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx
Show resolved
Hide resolved
...s/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/table/logs-table.tsx
Outdated
Show resolved
Hide resolved
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
1 participant
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?
Replaces the custom VirtualTable implementation in the keys overview
with the shared DataTable from @unkey/ui, following the same pattern
established by root-keys-table.
columns, hooks, schema, utils, and log-details sub-components
formatOutcomeName/getOutcomeColor exports to @unkey/ui
defs are stable across row selections instead of rebuilding on click
propagation in DataTable rows, preventing useOnClickOutside from
firing before the row click resolves
Fixes # (issue)
Type of change
How should this be tested?
Table Rendering
Used
Row Selection
Sorting
Details Panel
Filters
Regression
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated