fix: Key settings is missing from keys/keyAuthId/keyId#4093
Conversation
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughAdds a new KeySettingsDialog React component and updates the API ID navbar to fetch key data via TRPC and conditionally render key settings, copy ID, or key-creation UI based on key context and fetched keyData. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Navbar as API ID Navbar
participant TRPC as TRPC Client
participant Dialog as KeySettingsDialog
participant Popover as TableActionPopover
User->>Navbar: Open API page
Navbar->>TRPC: fetchKey(keyId)
alt keyData available
TRPC-->>Navbar: keyData
Navbar->>Dialog: render Dialog(keyData)
Dialog->>Popover: build items via getKeysTableActionItems(keyData, utils)
Popover-->>User: show Settings actions
else key context present, no data
Navbar-->>User: show Settings button + CopyableIDButton
else no key context
Navbar-->>User: show CreateKeyDialog or disabled Create button
end
sequenceDiagram
participant Dialog as KeySettingsDialog
participant Utils as trpc.useUtils()
participant Actions as getKeysTableActionItems
participant UI as TableActionPopover / NavbarActionButton
Dialog->>Utils: obtain TRPC utils
Dialog->>Actions: compute items(keyData, utils)
Actions-->>Dialog: return action items[]
Dialog->>UI: render Popover trigger "Settings" with items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_components/key-settings-dialog.tsx (1)
8-8: Consider simplifying the deeply nested import path.The import path contains
_components/components/table/components/actions, with multiplecomponentssegments. While functional, this suggests the directory structure could be flattened or reorganized for better maintainability.Example improvement (if feasible):
// Current import { getKeysTableActionItems } from "../keys/[keyAuthId]/_components/components/table/components/actions/keys-table-action.popover.constants"; // Potential alternative import { getKeysTableActionItems } from "../keys/[keyAuthId]/_components/table-actions/keys-table-action.popover.constants";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_components/key-settings-dialog.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_components/key-settings-dialog.tsx (4)
apps/dashboard/lib/trpc/routers/api/keys/query-api-keys/schema.ts (1)
KeyDetails(52-52)apps/dashboard/components/logs/table-action.popover.tsx (1)
TableActionPopover(37-156)apps/dashboard/components/navigation/action-button.tsx (1)
NavbarActionButton(33-70)internal/icons/src/icons/gear.tsx (1)
Gear(16-205)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (3)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_components/key-settings-dialog.tsx (1)
KeySettingsDialog(14-26)apps/dashboard/components/navigation/copyable-id-button.tsx (1)
CopyableIDButton(10-91)apps/dashboard/components/navigation/action-button.tsx (1)
NavbarActionButton(33-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (1)
175-202: LGTM: Conditional rendering correctly handles all key states.The conditional logic properly handles three scenarios:
- Key context exists with loaded data → shows KeySettingsDialog
- Key context exists but data is loading → shows disabled Settings button
- No key context → shows CreateKeyDialog or disabled Create button
The addition of
CopyableIDButtonin key view modes improves UX.apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_components/key-settings-dialog.tsx (1)
14-26: LGTM: Component correctly wires up key settings.The component properly:
- Retrieves TRPC utilities for action handlers
- Generates action items specific to the key
- Wraps the popover trigger with appropriate styling via
NavbarActionButton- Uses the
Gearicon consistently with the design system
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (3)
109-128: Significant efficiency improvement over previous implementation!The query now uses server-side filtering with the
keyIdsparameter andlimit: 1, which is much more efficient than the previous approach of fetching 1000 keys and filtering client-side. This addresses the past review concern.However, consider creating a dedicated
trpc.api.keys.getendpoint for better semantics and to avoid the empty string fallbacks required by the.listendpoint signature.
116-119: Empty string fallbacks are a code smell.Using empty strings to "silence TS errors" is not ideal. While this is safe because the query is guarded by the
enabledcondition, consider:
- Restructuring the query parameters to properly handle optional values
- Using a dedicated endpoint that accepts optional parameters with proper types
134-134: Redundant client-side filtering.Since the query now filters server-side to return exactly one key (with
keyIdsfilter andlimit: 1), the.find()operation is redundant. Consider simplifying to direct array access:-const specificKey = keyData?.keys.find((key) => key.id === keyId); +const specificKey = keyData?.keys[0];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/api-id-navbar.tsx (4)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_components/key-settings-dialog.tsx (1)
KeySettingsDialog(14-26)apps/dashboard/components/navigation/copyable-id-button.tsx (1)
CopyableIDButton(10-91)apps/dashboard/components/navigation/action-button.tsx (1)
NavbarActionButton(33-70)internal/icons/src/icons/gear.tsx (1)
Gear(16-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
2 participants
What does this PR do?
Puts the Settings button back on the Key specific page.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated