Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough""" WalkthroughThis update introduces batch operations for API key management in the dashboard, enabling multi-selection of keys and bulk actions such as enabling/disabling, editing external IDs, and changing ownership. The backend procedures and input schemas are updated to support arrays of key IDs, and the UI is enhanced with selection controls, confirmation dialogs, and animated feedback for batch actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardUI
participant SelectionControls
participant BatchMutationHook
participant TRPCServer
participant Database
User->>DashboardUI: Select multiple keys in table
User->>SelectionControls: Click batch action (e.g., Enable, Edit External ID)
SelectionControls->>BatchMutationHook: Trigger batch mutation with keyIds[]
BatchMutationHook->>TRPCServer: Call batch mutation endpoint (keyIds[], action)
TRPCServer->>Database: Update all keys matching keyIds[]
Database-->>TRPCServer: Return updated/missing key IDs
TRPCServer-->>BatchMutationHook: Respond with update results
BatchMutationHook-->>SelectionControls: Show success/warning toast
SelectionControls-->>DashboardUI: Update UI state (e.g., refresh keys list)
Suggested labels
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 (
|
|
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}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
apps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
17-31: 🛠️ Refactor suggestion
shouldShowdisables headerContent interactions whenonLoadMoreis undefined
pointerEventsis toggled solely on the presence ofonLoadMore.
If a consumer passesheaderContent(e.g. selection controls) but omitsonLoadMore, the entire footer becomes inert.Consider basing
shouldShowon(headerContent || onLoadMore)or splitting the two concerns so interactive header controls remain usable even when there is nothing to “load more”.
🧹 Nitpick comments (18)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (1)
66-70: ClarifykeyIdspayload – send an array to avoid implicit server-side coercion
updateOwnerIdnow supports batch updates and the backend input schema already normalises single strings into arrays.
Passing a bare string works, but relying on that implicit transform (and on the server even being TS/JS) makes the contract fuzzy for future maintainers and non-JS callers.A tiny client-side change makes the intent explicit:
- keyIds: values.keyId, + keyIds: [values.keyId],This keeps the types aligned no matter how the server evolves and documents that we are sending “multiple” IDs even when it is currently just one.
apps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
43-66: Footer becomes non-responsive on tiny screens – replace hard width & use responsive utilities
w-[740px]forces the footer to overflow on viewports narrower than 740 px. Because the element isfixedthis can hide part of the UI and the newly-addedheaderContentcontrols on mobile.- <div className="w-[740px] border ... + <div className="max-w-[740px] w-full sm:w-[640px] md:w-[740px] border ...Using a responsive width (or plain
w-fullwithmax-w) keeps the layout intact across break-points while preserving the centred design you already have.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx (1)
51-85: Duplicate hook logic – consolidate into a single parametrised hook
useUpdateKeyStatusanduseBatchUpdateKeyStatusdiffer only in how they compose the success message. Duplicating the mutation definition increases maintenance overhead and the risk of divergence.A single hook could accept
onSuccessand abatchflag (or simply infer fromkeyIds.length) to DRY this up.apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (1)
27-35: Variable shadowing ofinArrayhides the imported helperThe parameter list of the
wherecallback re-declaresinArray, shadowing the top-levelinArrayimported from@/lib/db.
While the current scope separation prevents functional breakage, it is confusing and can lead to subtle bugs if the inner alias’ semantics change.Rename the destructured util or the import to keep them distinct:
-import { db, inArray, schema } from "@/lib/db"; +import { db, inArray as sqlInArray, schema } from "@/lib/db"; ... - where: (table, { eq, and, isNull, inArray }) => + where: (table, { eq, and, isNull, inArray }) => ... - .where(sqlInArray(schema.keys.id, foundKeyIds)) + .where(sqlInArray(schema.keys.id, foundKeyIds))apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts (1)
4-29: Prefer strongly-typed & future-proof error handling instead ofany
handleKeyOwnerUpdateErroracceptserr: any. In practice the mutation throws aTRPCClientError, so we lose all type-safety and IntelliSense by falling back toany.-const handleKeyOwnerUpdateError = (err: any) => { +import { TRPCClientError } from "@trpc/client"; + +const handleKeyOwnerUpdateError = (err: unknown) => { + const error = err as TRPCClientError<any>;If you don’t want to couple to
TRPCClientError, acceptunknownand type-guard the structure you actually use (data?.code,message).
This protects the function against unrelated exceptions and improves DX.apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (2)
45-54: Name-shadowing ofinArrayobscures intent
inArrayis imported at the top and then re-declared in thewherecallback’s argument list, silently shadowing the import.
Consider aliasing the import to avoid confusion:-import { type Key, db, inArray, schema } from "@/lib/db"; +import { type Key, db, inArray as sqlInArray, schema } from "@/lib/db"; … - where: (table, { eq, and, isNull, inArray }) => + where: (table, { eq, and, isNull, inArray }) =>or drop the destructured
inArrayaltogether and use the aliasedsqlInArrayinside the callback. This makes it obvious which implementation you are calling.
71-78: Return missing key IDs to the caller for richer client feedbackYou already detect
missingKeyIdsbut onlyconsole.warnthem.
If you includemissingKeyIdsin the mutation’s payload the frontend can
display exactly which keys failed instead of estimating via
variables.keyIds.length - updatedCount.return { keyIds: keys.map(k => k.id), updatedCount: keys.length, missingKeyIds, };This eliminates guess-work and improves the UX of the new batch feature.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx (1)
59-69: State update is safe but can avoid recreatingSeton every toggleCreating a new
Seteach time is fine, but we can keep the previous
reference and only clone when the selection actually changes, avoiding an
extra allocation when the checkbox toggles to the same state.Not urgent, but consider:
setSelectedKeys(prev => { const has = prev.has(keyId); - const next = new Set(prev); + const next = has ? new Set([...prev].filter(id => id !== keyId)) + : new Set(prev).add(keyId); return next; });apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx (6)
30-34: Consider handling potential errors in the callback.The callback to
useDeleteKeyclears selection but doesn't account for potential failures. You might want to only clear selections when deletion is successful.const deleteKey = useDeleteKey(() => { - setSelectedKeys(new Set()); + // Only clear selections if the operation was successful + if (!deleteKey.isError) { + setSelectedKeys(new Set()); + } });
39-44: Ensure error handling for status updates.The
performDisableKeysfunction doesn't handle potential errors from the mutation. Consider adding error handling or at least tracking the mutation state.const performDisableKeys = () => { updateKeyStatus.mutate({ enabled: false, keyIds: Array.from(selectedKeys), + }, { + onError: (error) => { + // Handle error appropriately + console.error("Failed to disable keys:", error); + } }); };
56-58: Variable naming could be more precise.The variable
keysWithExternalIdscounts keys with identity_id, but its name might suggest it contains the keys themselves. Consider renaming for clarity.-const keysWithExternalIds = keys.filter( +const keysWithExternalIdsCount = keys.filter( (key) => selectedKeys.has(key.id) && key.identity_id, ).length;
86-89: Consider accessibility improvements for the counter.The animated counter display could benefit from an accessibility enhancement to announce changes to screen readers.
<div className="items-center flex gap-2"> - <AnimatedCounter value={selectedKeys.size} /> + <AnimatedCounter + value={selectedKeys.size} + aria-live="polite" + aria-label={`${selectedKeys.size} items selected`} + /> <div className="text-accent-9 text-[13px] leading-6">selected</div> </div>
112-114: Button text inconsistency.The button text uses singular "key" even when multiple keys can be selected. Consider making the text dynamic based on selection count.
- <CircleCheck size="sm-regular" /> - Enable key + <CircleCheck size="sm-regular" /> + Enable {selectedKeys.size > 1 ? "keys" : "key"} // Similar changes for the Disable and Delete buttonsAlso applies to: 124-126, 136-138
193-235: Animation components are well-implemented but should consider performance.The AnimatedDigit and AnimatedCounter components provide nice visual feedback. However, be mindful of potential performance issues with many animated elements. Consider adding a performance optimization like memoization.
-const AnimatedDigit = ({ digit, index }: { digit: string; index: number }) => { +const AnimatedDigit = React.memo(({ digit, index }: { digit: string; index: number }) => { return ( <motion.span key={`${digit}-${index}`} initial={{ opacity: 0, y: -20 }} animate={{ opacity: 1, y: 0, transition: { type: "spring", stiffness: 300, damping: 30, delay: index * 0.1, }, }} exit={{ opacity: 0, y: 20 }} className="inline-block" > {digit} </motion.span> ); -}; +});apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/components/batch-edit-external-id.tsx (4)
26-28: Consider error handling in the callback.The callback to
useBatchEditExternalIdonly closes the modal, but doesn't handle potential errors. Consider ensuring the modal is only closed on successful operations.const updateKeyOwner = useBatchEditExternalId(() => { - onClose(); + // Only close if successful + if (!updateKeyOwner.isError) { + onClose(); + } });
44-53: Complex dialog open state management.The logic for handling dialog open state changes is somewhat complex and could benefit from clearer documentation.
const handleDialogOpenChange = (open: boolean) => { + // Prevent outer dialog from closing when confirmation popover is active if (isConfirmPopoverOpen && !isOpen) { - // If confirm popover is active don't let this trigger outer popover return; } + // Close dialog when requested (if confirmation popover is not active) if (!isConfirmPopoverOpen && !open) { onClose(); } };
55-63: Error handling in clearSelection.The
clearSelectionfunction usesmutateAsyncbut doesn't have try/catch error handling. Consider adding proper error handling.const clearSelection = async () => { + try { await updateKeyOwner.mutateAsync({ keyIds: selectedKeyIds, ownerType: "v2", identity: { id: null, }, }); + } catch (error) { + // Handle error appropriately + console.error("Failed to clear external IDs:", error); + } };
83-108: Button state logic could be simplified.The conditional rendering between update and clear buttons is clear, but consider extracting this logic to make the JSX more readable.
+const renderActionButton = () => { + if (showUpdateButton) { + return ( + <Button + type="button" + variant="primary" + size="xlg" + className="rounded-lg flex-1" + loading={updateKeyOwner.isLoading} + onClick={handleSubmit} + > + Update External ID + </Button> + ); + } + + return ( + <Button + type="button" + variant="primary" + color="danger" + size="xlg" + className="rounded-lg flex-1" + loading={updateKeyOwner.isLoading} + onClick={handleClearButtonClick} + ref={clearButtonRef} + disabled={!hasKeysWithExternalIds} + > + Clear External ID + </Button> + ); +}; <div className="w-full flex gap-2"> - {showUpdateButton ? ( - <Button - type="button" - variant="primary" - size="xlg" - className="rounded-lg flex-1" - loading={updateKeyOwner.isLoading} - onClick={handleSubmit} - > - Update External ID - </Button> - ) : ( - <Button - type="button" - variant="primary" - color="danger" - size="xlg" - className="rounded-lg flex-1" - loading={updateKeyOwner.isLoading} - onClick={handleClearButtonClick} - ref={clearButtonRef} - disabled={!hasKeysWithExternalIds} - > - Clear External ID - </Button> - )} + {renderActionButton()} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-enabled.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/disable-key.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/keys-table-action.popover.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/components/batch-edit-external-id.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx(5 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-button.tsx(1 hunks)apps/dashboard/components/confirmation-popover.tsx(1 hunks)apps/dashboard/components/virtual-table/components/loading-indicator.tsx(3 hunks)apps/dashboard/components/virtual-table/types.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/updateEnabled.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx (1)
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.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (1)
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.
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
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.
🧬 Code Graph Analysis (2)
apps/dashboard/lib/trpc/routers/index.ts (1)
apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (1)
updateKeysEnabled(7-120)
apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (2)
apps/dashboard/lib/trpc/trpc.ts (3)
t(7-7)requireUser(9-20)requireWorkspace(22-35)apps/dashboard/lib/audit.ts (2)
UnkeyAuditLog(58-97)insertAuditLogs(99-174)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (22)
apps/dashboard/components/confirmation-popover.tsx (1)
86-86: Minor UI enhancement implemented.The gap between the icon and text in the confirmation popover header has been reduced from 4 to 3 units, creating a more compact and visually balanced layout.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/keys-table-action.popover.tsx (1)
54-54: Improved hover state behavior.Changed from direct element hover (
hover:border-gray-8) to parent group hover (group-hover:border-gray-8), which better aligns with the component's group-based interaction pattern.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/disable-key.tsx (1)
78-78:Details
✅ Verification successful
API updated to support batch operations.
The mutation now accepts an array of key IDs instead of a single key ID, which is consistent with the batch operations support being added throughout the application.
Verify other key action components were updated similarly:
🏁 Script executed:
#!/bin/bash # Check if other key action components also use keyIds array format rg -A 3 -B 3 "keyIds: \[.*\]" --glob "*.tsx" apps/dashboard/app/\(app\)/apis/Length of output: 3150
Batch operations support verified across key action components
All key action components now sendkeyIds: [id]as required by the updated API:
- _components/.../actions/components/delete-key.tsx
- _components/.../actions/components/disable-key.tsx
- settings/delete-key.tsx
Approving these changes.
apps/dashboard/lib/trpc/routers/index.ts (2)
30-30: Updated import to support batch operations.Changed import from
updateKeyEnabledtoupdateKeysEnabled, matching the renamed procedure that supports multiple keys.
91-91: Updated router to use batch operation handler.Router now uses the
updateKeysEnabledprocedure which supports updating multiple keys at once, consistent with the batch operation implementation.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-enabled.tsx (1)
56-59: Update API to support batch operationsThe
onSubmitfunction is now correctly structured to call the mutation withkeyIdsparameter instead ofkeyId, aligning with the backend changes that support batch operations for key status updates.apps/dashboard/components/virtual-table/types.ts (1)
60-60: Good enhancement for table header customizationThe new
headerContentproperty allows for injecting custom React components in the table footer header, which will support the multi-selection controls for keys.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-external-id/index.tsx (3)
36-36: Updated parameter name for batch operations supportChanged from
keyIdtokeyIdsto align with backend API changes that support batch operations.
62-62: Updated parameter name for batch operations supportChanged from
keyIdtokeyIdsin the mutation call, consistent with the first change to support batch operations.
74-74: Improved text consistencyChanged "userId" to "userID" in the dialog subtitle for better naming consistency.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_components/table-action-button.tsx (1)
9-10: Enhanced hover behavior using group semanticsUpdated the button styling to use
group-hoverinstead of standalonehoverclasses, which improves the component's interaction with its parent container. This provides a more consistent hover experience across the application.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx (1)
4-25:⚠️ Potential issueToast helper references non-existent
toast.warningThe project-wide toaster only exposes
.success,.error, and optionally.info.
Usingtoast.warning(lines 65-70) will result in a runtime exception and the real error message will never reach the user.Either:
- toast.warning("Some Keys Not Found", { + toast.error("Some Keys Not Found", { // or .infoor extend the toaster util to include a
warningvariant everywhere.Likely an incorrect or invalid review comment.
apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (1)
115-119: Response omits transactional consistency check
updatedKeyIdsis derived fromfoundKeyIdsgathered before the update.
If the bulkUPDATEfails partially (constraint, trigger, etc.) the response may claim success even though nothing changed.After the
UPDATE, confirmsqlInArray(...).execute()returnedaffectedRows === foundKeyIds.length, or re-select to verify rows actually changed, and surface discrepancies (400/BAD_REQUEST or apartialflag).apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx (4)
1-16: Well-structured props and import organization.The component has clear type definitions and appropriate imports for the functionality it provides. The
getSelectedKeysStateprop function is a good approach for determining the collective status of multiple keys.
24-28: Good state management setup.The component properly initializes state for modals and refs. Using refs for the confirm popover triggers is the correct approach for positioning popovers relative to their trigger buttons.
153-165: Good implementation of confirmation dialogs.The confirmation popover for disabling keys is well implemented with appropriate messaging and clear button text. The dynamic pluralization of text based on selection count is a nice touch.
229-229: Array index key warning is appropriately ignored.The warning about using array index as key is correctly suppressed with a
biome-ignorecomment. In this specific case with digits in a number, using the index is acceptable since the digits' position is meaningful.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/components/batch-edit-external-id.tsx (5)
9-14: Well-defined props interface.The props interface is clear and includes appropriate documentation comments for the
keysWithExternalIdsprop, which helps with understanding its purpose.
22-24: Good state management setup.The component properly initializes state for the selected external ID and confirmation popover visibility.
75-77: Good use of dynamic text.The subtitle properly uses singular/plural forms based on the number of selected keys, which provides clear messaging to users.
121-141: Excellent warning implementation.The warning message for keys with existing external IDs is well implemented. It provides clear visual feedback with appropriate icons and conditional text based on whether all or some keys have external IDs.
150-166: Good implementation of confirmation popover.The confirmation popover for clearing external IDs is well implemented with appropriate messaging. The dynamic text based on the number of keys is a nice touch for clear user communication.
...Id]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts
Show resolved
Hide resolved
...dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx
Show resolved
Hide resolved
...apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx
Show resolved
Hide resolved
...apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx (2)
91-98: Disable "Change External ID" button when nothing is selected.The "Change External ID" button is currently always enabled. It should be disabled if there are no selected keys (or if none of the selected keys have existing external IDs).
<Button variant="outline" size="sm" className="text-gray-12 font-medium text-[13px]" + disabled={selectedKeys.size === 0 || keysWithExternalIds === 0} onClick={() => setIsBatchEditExternalIdOpen(true)} > <ArrowOppositeDirectionY size="sm-regular" /> Change External ID </Button>
181-188: Ensure BatchEditExternalId triggers a data refresh on success.You're currently only passing
onCloseto the modal, but the underlying hook likely supports anonSuccesscallback for triggering refetches. Without it, the parent list won't update when IDs are edited.<BatchEditExternalId selectedKeyIds={Array.from(selectedKeys)} keysWithExternalIds={keysWithExternalIds} isOpen={isBatchEditExternalIdOpen} onClose={() => setIsBatchEditExternalIdOpen(false)} + onSuccess={() => { + setIsBatchEditExternalIdOpen(false); + // Add a refetch mechanism here to update the parent list + }} />
🧹 Nitpick comments (4)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx (1)
34-35: Add safeguard for empty array access.The code assumes
data.updatedKeyIdsalways has at least one element. Consider adding a safeguard to handle empty arrays.- description: `Your key ${data.updatedKeyIds[0]} has been ${ + description: `Your key ${data.updatedKeyIds.length > 0 ? data.updatedKeyIds[0] : ''} has been ${apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx (3)
56-58: Variable name misleads about return type.The variable
keysWithExternalIdsreturns a number (length) but its name suggests it's an array of keys. Consider renaming it tokeysWithExternalIdsCountto better reflect what it represents.- const keysWithExternalIds = keys.filter( + const keysWithExternalIdsCount = keys.filter( (key) => selectedKeys.has(key.id) && key.identity_id, ).length;Make sure to update all references to this variable in your component.
221-223: Improve CSS class readability.The CSS class structure with linebreaks inside a template string is hard to read. Consider reformatting for better readability.
- className="size-[18px] text-[11px] leading-6 ring-2 ring-gray-6 flex items-center justify-center font-medium overflow-hidden -p-2 text-white dark:text-black bg-accent-12 hover:bg-accent-12/90 focus:hover:bg-accent-12 rounded-md border border-grayA-4 -" + className="size-[18px] text-[11px] leading-6 ring-2 ring-gray-6 flex items-center justify-center + font-medium overflow-hidden p-2 text-white dark:text-black bg-accent-12 + hover:bg-accent-12/90 focus:hover:bg-accent-12 rounded-md border border-grayA-4"
229-230: Consider using a more robust React key.While the biome-ignore comment acknowledges the issue, using array indices as keys can cause rendering problems when the array changes. Consider using a more robust key like combining the digit with its position.
- // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> - <AnimatedDigit key={index} digit={digit} index={index} /> + <AnimatedDigit key={`${digit}-${index}`} digit={digit} index={index} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/selection-controls/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx (2)
3-26: Well-structured error handling abstraction.Great job creating a centralized error handler function that handles different error scenarios with specific, user-friendly messages. This will help maintain consistency across both single and batch key operations.
52-86: Excellent batch operation implementation.The batch update hook is well-implemented with proper success and error handling, including a nice touch of pluralizing messages based on count and handling the case of missing keys separately.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx (1)
374-376: Skeleton renderer references non-existent "select" column.There's a reference to a "select" column in the
includes()array, but there's no "select" column defined in the columns array, making this condition dead code.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts (1)
41-47: Success toast omits "v1" branch – users see a blank description.When
ownerType === "v1"the description string remains empty, resulting in a toast without context.
🧹 Nitpick comments (2)
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (2)
132-132: Consider improving audit log description readability for many keys.For large batches, the current approach of joining all key IDs into a single string could create very long descriptions. Consider limiting the number of IDs displayed or using a count instead.
-const description = `Changed ownerId of keys [${keyIds}] to ${input.ownerId ?? "null"}`; +const keyCount = keys.length; +const displayKeyIds = keys.length <= 3 + ? keyIds + : `${keys[0].id}, ${keys[1].id}, ... and ${keyCount - 2} more`; +const description = `Changed ownerId of ${keyCount} keys [${displayKeyIds}] to ${input.ownerId ?? "null"}`;
212-212: Same readability concern for audit log description in updateOwnerV2.The description string construction has the same potential issue with large batches of keys.
-const description = `Updated identity of keys [${keyIds}] to ${input.identity.id ?? "null"}`; +const keyCount = keys.length; +const displayKeyIds = keys.length <= 3 + ? keyIds + : `${keys[0].id}, ${keys[1].id}, ... and ${keyCount - 2} more`; +const description = `Updated identity of ${keyCount} keys [${displayKeyIds}] to ${input.identity.id ?? "null"}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx(5 hunks)apps/dashboard/lib/trpc/routers/key/updateEnabled.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/lib/trpc/routers/key/updateEnabled.ts
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
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.
🧬 Code Graph Analysis (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts (6)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-update-key-status.tsx (2)
onSuccess(32-43)onSuccess(56-79)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-delete-key.ts (1)
onSuccess(8-19)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-key.tsx (1)
onSuccess(9-22)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-metadata.ts (1)
onSuccess(7-23)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-ratelimits.ts (1)
onSuccess(9-50)apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
updateKeyOwner(31-91)
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (2)
internal/db/src/types.ts (1)
Key(4-4)apps/dashboard/lib/audit.ts (1)
UnkeyAuditLog(58-97)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./apps/dashboard
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/keys-list.tsx (4)
84-121: Good implementation of conditional checkbox rendering.The implementation elegantly handles the state transitions between normal, hovered, and selected states. The checkbox appears only when needed (on hover or selection) and maintains visual consistency with the rest of the UI.
50-53: Well-structured state management for multi-selection.The state declarations for
selectedKeys(using a Set for efficient lookups) andhoveredKeyIdprovide a solid foundation for the multi-selection feature.
259-292: Good implementation of selected keys state calculation.The
getSelectedKeysStatefunction efficiently determines whether selected keys are all enabled, all disabled, or in a mixed state, with an early exit optimization when a mix is detected.
311-317: Clean integration of selection controls in the table footer.The
SelectionControlscomponent is well-integrated into the table's footer, providing a natural placement for batch operations UI.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-external-id.ts (3)
6-32: Excellent refactoring of error handling.The extracted
handleKeyOwnerUpdateErrorfunction provides consistent error handling across both hooks with clear, user-friendly error messages based on error codes.
39-39: Good adaptation for batch operation support.The line correctly extracts the first key ID from an array when handling individual key updates, ensuring backward compatibility with existing code.
66-114: Well-designed batch update hook with comprehensive success handling.The
useBatchEditExternalIdhook has:
- Proper pluralization in success messages
- Warning toasts for missing keys
- Consistent invalidation of the keys list cache
This aligns with similar batch hooks in the codebase, providing a uniform user experience.
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (6)
8-10: Clean input schema transformation for batch operations.The schema elegantly handles both single string and array inputs, normalizing them to arrays via transformation, which maintains backward compatibility while enabling batch operations.
37-42: Good validation for empty key ID arrays.The validation ensures at least one key ID is provided before proceeding with the operation, preventing unnecessary database queries.
45-60: Well-structured database query for batch operations.The query correctly fetches multiple keys based on the workspace ID and provided key IDs, with proper error handling for database failures.
72-77: Nice addition of missing key detection and logging.The code properly identifies and logs keys that weren't found in the database, which helps with debugging and user feedback.
120-128: Secure workspace ID validation in update queries.The addition of workspace ID to the update where clause provides an extra layer of security to prevent accidental cross-workspace writes.
166-169: Consistent return structure for batch operations.The return object with both key IDs and count provides flexibility for different UI feedback scenarios, matching patterns used elsewhere in the codebase.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
nitpick: this doesn't handle plural properly
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-enabled.tsx
Show resolved
Hide resolved
2 participants
What does this PR do?
Adds batch delete, disable/enable and external ID change operations to keys.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Screen.Recording.2025-05-07.at.15.41.46.mov
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Refactor