feat: suggested display of rootkey for filters#3028
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes introduce a new function, Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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! 🙏 |
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
.../app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx (3)
39-42: Simplify the obfuscation logic.The current obfuscation logic is complex and may be confusing. The current calculation for
obfuscatedMiddlechecks if a substring has length > 0, but this will always be true if there are at least 5 characters after the underscore.Consider simplifying the formatting logic:
- const obfuscatedMiddle = - id.substring(id.indexOf("_") + 1, id.indexOf("_") + 5).length > 0 ? "..." : ""; - const nextFour = id.substring(id.indexOf("_") + 1, id.indexOf("_") + 5); - const lastFour = id.substring(id.length - 4); - return rootKey.name ?? prefix + nextFour + obfuscatedMiddle + lastFour; + const underscoreIndex = id.indexOf("_"); + const middlePartStart = underscoreIndex + 1; + const middlePartEnd = id.length - 4; + + // Show "..." only if there are characters between nextFour and lastFour + const shouldShowObfuscation = middlePartEnd - (middlePartStart + 4) > 0; + + const nextFour = id.substring(middlePartStart, Math.min(middlePartStart + 4, id.length)); + const lastFour = id.substring(Math.max(id.length - 4, 0)); + + return rootKey.name ?? prefix + nextFour + (shouldShowObfuscation ? "..." : "") + lastFour;This approach more clearly expresses the intent and handles edge cases better.
43-43: Consider handling empty string names.The current implementation uses the nullish coalescing operator (
??), which will return the right-hand value only ifrootKey.nameisnullorundefined. However, an empty string ("") is considered a valid value, which might not be what you want.If you want to use the formatted ID when the name is an empty string as well:
- return rootKey.name ?? prefix + nextFour + obfuscatedMiddle + lastFour; + return rootKey.name && rootKey.name.trim() !== "" ? + rootKey.name : + prefix + nextFour + obfuscatedMiddle + lastFour;
36-44: Add comments explaining the formatting logic.The function uses a specific formatting pattern for the root key IDs, but there are no comments explaining the intent or expected format of these IDs.
Consider adding explanatory comments:
const getRootKeyLabel = (rootKey: { id: string; name: string | null }) => { + // If the root key has a name, use it; otherwise, format the ID + // Expected format for IDs: "prefix_randomstring" (e.g., "root_a1b2c3d4e5f6g7h8") + // We show: "prefix_abcd...wxyz" where abcd are the first 4 chars after prefix and wxyz are the last 4 const id = rootKey.id; const prefix = id.substring(0, id.indexOf("_") + 1); const obfuscatedMiddle = id.substring(id.indexOf("_") + 1, id.indexOf("_") + 5).length > 0 ? "..." : ""; const nextFour = id.substring(id.indexOf("_") + 1, id.indexOf("_") + 5); const lastFour = id.substring(id.length - 4); return rootKey.name ?? prefix + nextFour + obfuscatedMiddle + lastFour; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx (1)
17-17: Good addition of formatted labels.The change to use a dedicated function for formatting the labels improves consistency and readability. This allows for a more sophisticated presentation of root keys in the filter component.
.../app/(app)/audit/components/controls/components/logs-filters/components/root-keys-filter.tsx
Show resolved
Hide resolved
|
ignore that, we can guarantee they’ll have an underscore |
|
can we fix the toast not vanishing? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx (2)
60-81: Improved UI implementation with checkboxes, but missing loading states.The checkbox-based UI is a more intuitive way to manage role assignments compared to the previous tree structure. However, there's no visual indication or disabled state when a mutation is in progress.
Consider disabling checkboxes during mutation loading states to prevent multiple clicks:
<Checkbox checked={role.isActive} + disabled={connectRole.isLoading || disconnectRole.isLoading} onCheckedChange={(checked) => { if (checked) { connectRole.mutate({ keyId: keyId, roleId: role.id }); } else { disconnectRole.mutate({ keyId: keyId, roleId: role.id }); } }} />
18-85: Consider using optimistic updates for a smoother UX.The current implementation refreshes the entire page after each role connection/disconnection, which may cause a noticeable flicker or delay in the UI.
Consider implementing optimistic updates to provide immediate feedback without waiting for the server response:
export function PermissionList({ roles, keyId }: PermissionTreeProps) { const router = useRouter(); + const [optimisticRoles, setOptimisticRoles] = useState(roles); const connectRole = trpc.rbac.connectRoleToKey.useMutation({ onMutate: ({ roleId }) => { toast.loading("Connecting role to key"); + // Apply optimistic update + setOptimisticRoles(prev => + prev.map(role => role.id === roleId ? {...role, isActive: true} : role) + ); }, // ... rest of the implementation }); // Apply similar changes to disconnectRole mutation return ( // ... existing code <div className="space-y-1"> - {roles.map((role) => ( + {optimisticRoles.map((role) => ( // ... existing render logic ))} </div> ); }This would provide immediate feedback to users while the server operation completes in the background.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
🔇 Additional comments (4)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx (4)
3-6: Good use of necessary imports for the new implementation.The addition of Checkbox, trpc, useRouter, and toast imports aligns well with the component's new functionality for managing role connections through checkboxes with proper user feedback.
10-10: Improved naming with isActive vs. active.Renaming from "active" to "isActive" follows better boolean naming conventions, making the code more readable and self-documenting.
15-15: Necessary addition of keyId parameter.Adding the keyId property to PermissionTreeProps is essential for the role connection/disconnection mutations.
54-56: Clear and concise UI text.The updated card title and description provide better context about the component's purpose.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx
Show resolved
Hide resolved
MichaelUnkey
deleted the
eng-1688-a-root-key-filter-should-show-something-other-than-empty-if
branch
3 participants
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
Documentation