Merged
Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Contributor
📝 WalkthroughWalkthroughA generic, reusable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataTable
participant TRPC
participant Toast
participant Page
User->>DataTable: Interacts (sort/filter/select/delete)
DataTable->>TRPC: Calls delete mutation for selected keys
TRPC-->>DataTable: Returns success/error
DataTable->>Toast: Shows success/error notification
DataTable->>Page: Refreshes page on mutation completion
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/dashboard/components/dashboard/root-key-table/index.tsx(2 hunks)apps/dashboard/components/dashboard/root-key-table/table.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: MichaelUnkey
PR: unkeyed/unkey#3103
File: apps/dashboard/app/(app)/settings/general/page.tsx:36-36
Timestamp: 2025-04-14T13:39:22.635Z
Learning: The Unkey team uses Linear for issue tracking rather than GitHub Issues.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3380
File: apps/dashboard/app/(app)/ratelimits/[namespaceId]/namespace-navbar.tsx:40-66
Timestamp: 2025-06-19T10:39:29.388Z
Learning: tRPC has built-in caching that prevents skeleton flashing during component re-renders and navigation, so concerns about `!data || isLoading` conditions causing loading state flashes are generally not needed.
apps/dashboard/components/dashboard/root-key-table/index.tsx (4)
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx:74-75
Timestamp: 2024-10-04T17:27:09.821Z
Learning: In `apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx`, the hidden `<input>` elements for `workspaceId` and `keyAuthId` work correctly without being registered with React Hook Form.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3292
File: apps/dashboard/lib/vault.ts:80-97
Timestamp: 2025-06-02T11:08:56.397Z
Learning: The vault.ts file in apps/dashboard/lib/vault.ts is a duplicate of the vault package from the `api` directory and should be kept consistent with that original implementation.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3380
File: apps/dashboard/app/(app)/ratelimits/[namespaceId]/namespace-navbar.tsx:40-66
Timestamp: 2025-06-19T10:39:29.388Z
Learning: tRPC has built-in caching that prevents skeleton flashing during component re-renders and navigation, so concerns about `!data || isLoading` conditions causing loading state flashes are generally not needed.
apps/dashboard/components/dashboard/root-key-table/table.tsx (3)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83
Timestamp: 2024-12-03T14:17:08.016Z
Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.
Learnt from: p6l-richard
PR: unkeyed/unkey#2085
File: apps/www/components/glossary/search.tsx:16-20
Timestamp: 2024-10-23T16:21:47.395Z
Learning: For the `FilterableCommand` component in `apps/www/components/glossary/search.tsx`, refactoring type definitions into an interface is not necessary at this time.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0
Timestamp: 2025-01-30T20:38:00.058Z
Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.
🧬 Code Graph Analysis (1)
apps/dashboard/components/dashboard/root-key-table/index.tsx (1)
apps/dashboard/lib/trpc/server.ts (1)
trpc(7-14)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (4)
apps/dashboard/components/dashboard/root-key-table/index.tsx (2)
22-22: LGTM! UI library consolidation.Moving to the centralized
@unkey/uipackage for the Badge component improves consistency across the dashboard.
45-45: TRPC endpoint update verified across codebaseAll occurrences of the old
trpc.rootKey.deleteendpoint have been removed and replaced withtrpc.settings.rootKeys.delete. The backend route definition in
apps/dashboard/lib/trpc/routers/settings/root-keys/delete.ts
exists and aligns with the frontend usage.• Old endpoint usage: none found
• New endpoint usage in:
apps/dashboard/components/dashboard/root-key-table/index.tsxapps/dashboard/components/dashboard/root-key-table/table.tsxapps/dashboard/app/(app)/settings/root-keys/.../use-delete-root-key.tsNo further changes required.
apps/dashboard/components/dashboard/root-key-table/table.tsx (2)
50-207: Consider performance optimizations for large datasets.For better performance with large datasets, consider these optimizations:
- Memoize the table instance to prevent unnecessary re-renders
- Add virtualization for tables with many rows (similar to LogsTable)
- Debounce the filter input
Would you like me to provide an implementation with these performance optimizations?
⛔ Skipped due to learnings
Learnt from: ogzhanolguncu PR: unkeyed/unkey#2143 File: apps/dashboard/app/(app)/logs/logs-page.tsx:77-83 Timestamp: 2024-12-03T14:17:08.016Z Learning: The `<LogsTable />` component already implements virtualization to handle large datasets efficiently.Learnt from: p6l-richard PR: unkeyed/unkey#2085 File: apps/www/components/glossary/terms-rolodex-desktop.tsx:26-37 Timestamp: 2024-10-23T16:20:19.324Z Learning: When reviewing React components in this project, avoid suggesting manual memoization with `useMemo` for performance optimizations, as the team prefers to rely on the React compiler to handle such optimizations.Learnt from: ogzhanolguncu PR: unkeyed/unkey#2825 File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0 Timestamp: 2025-01-30T20:38:00.058Z Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.Learnt from: ogzhanolguncu PR: unkeyed/unkey#2825 File: apps/dashboard/lib/trpc/routers/logs/query-distinct-paths.ts:29-36 Timestamp: 2025-01-30T20:46:29.482Z Learning: The team prefers to implement pagination and other optimizations only when they become necessary, rather than prematurely optimizing. This was demonstrated in the context of unique paths query where a simple LIMIT clause was deemed sufficient until proven otherwise.Learnt from: ogzhanolguncu PR: unkeyed/unkey#2143 File: apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx:37-49 Timestamp: 2024-12-03T14:23:07.189Z Learning: In `apps/dashboard/app/(app)/logs/components/log-details/resizable-panel.tsx`, the resize handler is already debounced.
97-128: Add keyboard accessibility for bulk actions.The bulk delete dialog should be keyboard accessible. Consider adding keyboard shortcuts for power users.
<DialogTrigger asChild> - <Button className="min-w-full md:min-w-min ">Revoke selected keys</Button> + <Button + className="min-w-full md:min-w-min" + aria-label={`Revoke ${Object.keys(rowSelection).length} selected keys`} + > + Revoke selected keys + </Button> </DialogTrigger>⛔ Skipped due to learnings
Learnt from: ogzhanolguncu PR: unkeyed/unkey#2143 File: apps/dashboard/components/ui/group-button.tsx:21-31 Timestamp: 2024-12-03T14:07:45.173Z Learning: In the `ButtonGroup` component (`apps/dashboard/components/ui/group-button.tsx`), avoid suggesting the use of `role="group"` in ARIA attributes.Learnt from: ogzhanolguncu PR: unkeyed/unkey#2825 File: apps/dashboard/app/(app)/logs-v2/components/controls/components/logs-datetime/index.tsx:0-0 Timestamp: 2025-01-30T20:38:00.058Z Learning: In the logs dashboard, keyboard shortcuts that toggle UI elements (like popovers) should be implemented in the component that owns the state being toggled, not in the presentational wrapper components. For example, the 'T' shortcut for toggling the datetime filter is implemented in DatetimePopover, not in LogsDateTime.Learnt from: MichaelUnkey PR: unkeyed/unkey#3173 File: apps/docs/security/delete-protection.mdx:32-36 Timestamp: 2025-04-22T17:33:28.162Z Learning: In the Unkey dashboard UI for delete protection, the button/link to initiate the process is labeled "Disable Delete Protection" while the confirmation button is labeled "Disable API Delete Protection". The documentation should maintain these different labels to match the actual UI.Learnt from: MichaelUnkey PR: unkeyed/unkey#3173 File: apps/docs/security/delete-protection.mdx:21-24 Timestamp: 2025-04-22T17:34:04.438Z Learning: In the Unkey dashboard UI for enabling delete protection, the button/link to initiate the process is labeled "Enable Delete Protection" while the confirmation button is labeled "Enable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
Comment on lines
+129
to
+134
| <Input | ||
| placeholder="Filter keys" | ||
| value={(table.getColumn("start")?.getFilterValue() as string) ?? ""} | ||
| onChange={(event) => table.getColumn("start")?.setFilterValue(event.target.value)} | ||
| className="max-w-sm md:max-w-2xl" | ||
| /> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add proper ARIA labels for better accessibility.
The filter input should have proper labeling for screen readers.
<Input
placeholder="Filter keys"
+ aria-label="Filter root keys"
value={(table.getColumn("start")?.getFilterValue() as string) ?? ""}
onChange={(event) => table.getColumn("start")?.setFilterValue(event.target.value)}
className="max-w-sm md:max-w-2xl"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Input | |
| placeholder="Filter keys" | |
| value={(table.getColumn("start")?.getFilterValue() as string) ?? ""} | |
| onChange={(event) => table.getColumn("start")?.setFilterValue(event.target.value)} | |
| className="max-w-sm md:max-w-2xl" | |
| /> | |
| <Input | |
| placeholder="Filter keys" | |
| aria-label="Filter root keys" | |
| value={(table.getColumn("start")?.getFilterValue() as string) ?? ""} | |
| onChange={(event) => table.getColumn("start")?.setFilterValue(event.target.value)} | |
| className="max-w-sm md:max-w-2xl" | |
| /> |
🤖 Prompt for AI Agents
In apps/dashboard/components/dashboard/root-key-table/table.tsx around lines 129
to 134, the Input component used for filtering keys lacks proper ARIA labels,
which reduces accessibility for screen reader users. Add an appropriate
aria-label attribute to the Input element that clearly describes its purpose,
such as "Filter keys input," to improve accessibility compliance.
Comment on lines
+115
to
+119
| const keyIds = table | ||
| .getSelectedRowModel() | ||
| // @ts-ignore | ||
| .rows.map((row) => row.original.id as string); | ||
| deleteKey.mutate({ keyIds }); |
Contributor
There was a problem hiding this comment.
Remove @ts-ignore and fix the type issue properly.
The @ts-ignore comment indicates a TypeScript issue that should be resolved. Based on the context, the row data should be properly typed.
- const keyIds = table
- .getSelectedRowModel()
- // @ts-ignore
- .rows.map((row) => row.original.id as string);
+ const keyIds = table
+ .getSelectedRowModel()
+ .rows.map((row) => (row.original as { id: string }).id);Alternatively, ensure the generic type TData extends a base type with an id property:
interface BaseRowData {
id: string;
}
export function DataTable<TData extends BaseRowData, TValue>({ data, columns }: DataTableProps<TData, TValue>) {🤖 Prompt for AI Agents
In apps/dashboard/components/dashboard/root-key-table/table.tsx around lines 115
to 119, remove the @ts-ignore comment and fix the type issue by properly typing
the row data. Ensure that the generic type TData extends a base interface that
includes an id property (e.g., interface BaseRowData { id: string }). Update the
DataTable component's generic constraint to TData extends BaseRowData so that
row.original.id is recognized as a string without type errors.
| onError: (err, variables) => { | ||
| router.refresh(); | ||
| console.error(err); | ||
| toast.error(`Could not delete key ${JSON.stringify(variables)}`); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency in toast notifications.
The error toast here uses toast.error(), while in index.tsx it uses toast(). Use consistent toast methods across components.
- toast.error(`Could not delete key ${JSON.stringify(variables)}`);
+ toast(`Could not delete key ${JSON.stringify(variables)}`);Or update both files to use the semantic toast.error() method for error cases.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| toast.error(`Could not delete key ${JSON.stringify(variables)}`); | |
| toast(`Could not delete key ${JSON.stringify(variables)}`); |
🤖 Prompt for AI Agents
In apps/dashboard/components/dashboard/root-key-table/table.tsx at line 70, the
toast notification for errors uses toast.error(), which is inconsistent with the
toast() method used in index.tsx. To maintain consistency, update the toast call
in this file to use the same method as in index.tsx, or alternatively, update
both files to use toast.error() for error notifications to keep semantic
clarity. Choose one approach and apply it consistently across both files.
MichaelUnkey
approved these changes
Jul 1, 2025
perkinsjr
approved these changes
Jul 1, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes # (issue)
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?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor