fix: Allows triggers on dropdown and fixes filtering #2748
Conversation
Remove asChild to allow trigger from buttons
Removing as child to allow for correct triggers
Fix filter to work as expected
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to various components within the dashboard application, particularly focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/menu.tsx (1)
Line range hint
23-42: Architectural Improvement: Consistent Dropdown ImplementationThe systematic removal of
asChildprop across dropdown triggers represents a positive architectural change that:
- Simplifies the component hierarchy
- Better aligns with Radix UI's recommended patterns
- Provides consistent behavior across different contexts (data tables, menus, filters)
This change should resolve the dropdown trigger issues mentioned in the PR objectives while maintaining proper functionality.
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (2)
Line range hint
75-83: Remove duplicate event handler logicThe
onClickhandler on thedivduplicates the logic in theCommandItem'sonSelecthandler. This could potentially cause the selection logic to execute twice.Remove the redundant
onClickhandler:- <div - onClick={() => { - const next = isSelected - ? selected.filter((v) => v !== option.value) - : Array.from(new Set([...selected, option.value])); - setSelected(next); - }} - > + <div>
Line range hint
84-91: Add error handling for state updatesThe selection state update logic should include error handling to gracefully handle any potential failures.
Consider wrapping the state update in a try-catch block:
<CommandItem key={option.value} onSelect={() => { + try { const next = isSelected ? selected.filter((v) => v !== option.value) : Array.from(new Set([...selected, option.value])); setSelected(next); + } catch (error) { + console.error('Failed to update selection:', error); + // Consider showing a user-friendly error message + } }} >apps/dashboard/components/dashboard/root-key-table/index.tsx (1)
Line range hint
158-162: Consider adding rate limiting for key deletion operationsWhile the deletion confirmation dialog provides a good warning, additional security measures could help prevent accidental or malicious bulk deletions.
Consider implementing:
- Rate limiting for deletion operations
- A cool-down period between bulk deletions
- Additional confirmation for deleting multiple keys simultaneously
apps/dashboard/components/dashboard/api-key-table/table.tsx (1)
Line range hint
108-112: Remove @ts-ignore and fix type safetyThe use of @ts-ignore masks potential type safety issues that should be addressed properly.
Consider fixing the type safety issue:
- // @ts-ignore - .rows.map((row) => row.original.id as string); + .rows.map((row) => { + if (!row.original || typeof row.original !== 'object' || !('id' in row.original)) { + throw new Error('Invalid row data structure'); + } + return row.original.id as string; + });apps/dashboard/components/dashboard/api-key-table/index.tsx (1)
Line range hint
243-256: Good improvement in dropdown menu item handlingThe addition of
onSelecthandlers withpreventDefault()is a good practice that prevents unintended navigation when selecting menu items. TheasChildprop onDialogTriggeris correctly used to maintain proper DOM structure.Consider extracting the
onSelecthandler to avoid repetition:+ const preventDefaultSelect = (e: Event) => e.preventDefault(); <DropdownMenuItem - onSelect={(e) => { - e.preventDefault(); - }} + onSelect={preventDefaultSelect} >apps/dashboard/app/(app)/settings/user/update-user-email.tsx (2)
127-129: Add aria-label to improve accessibility.The Button with MoreHorizontal icon should have an aria-label to improve screen reader experience.
-<Button variant="ghost"> +<Button variant="ghost" aria-label="Email options"> <MoreHorizontal className="w-4 h-4" /> </Button>
Line range hint
126-238: Consider refactoring error handling and loading states.The dropdown menu actions contain repeated error handling patterns and multiple loading states that could be consolidated for better maintainability.
Consider these improvements:
- Create a reusable error handler:
const handleError = (error: unknown, customMessage?: string) => { const message = error instanceof Error ? error.message : customMessage ?? 'An error occurred'; toast.error(message); };
- Use a single loading state manager:
type LoadingAction = 'verification' | 'promotion' | 'removal'; const [loadingAction, setLoadingAction] = useState<LoadingAction | null>(null); const isLoading = (action: LoadingAction) => loadingAction === action;This would simplify the action handlers and make the code more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/menu.tsx(1 hunks)apps/dashboard/app/(app)/settings/user/update-user-email.tsx(1 hunks)apps/dashboard/components/dashboard/api-key-table/index.tsx(1 hunks)apps/dashboard/components/dashboard/api-key-table/table.tsx(1 hunks)apps/dashboard/components/dashboard/root-key-table/index.tsx(1 hunks)apps/dashboard/components/dashboard/root-key-table/table.tsx(1 hunks)apps/dashboard/components/data-table/column-header.tsx(1 hunks)apps/dashboard/components/data-table/view-options.tsx(1 hunks)
🔇 Additional comments (8)
apps/dashboard/components/data-table/view-options.tsx (1)
Line range hint 23-28: LGTM! Dropdown trigger implementation follows Radix UI best practices.
The removal of asChild prop is correct here as the Button is properly wrapped by DropdownMenuTrigger, ensuring proper event handling and accessibility.
Let's verify this component's integration:
✅ Verification successful
Both implementations are valid for their specific use cases
The chart component uses asChild with ghost buttons that only contain icons, while our view-options component correctly implements a standard dropdown trigger with a labeled button. Both approaches follow Radix UI patterns:
- With
asChild: When the trigger is a simple icon button (like in chart.tsx) - Without
asChild: When the trigger is a fully styled button with text and icon (like in our view-options)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent dropdown trigger implementation across the codebase
rg -l "DropdownMenuTrigger.*asChild" --type tsx
Length of output: 77
Script:
#!/bin/bash
# Check for consistent dropdown trigger implementation across the codebase
# Using ripgrep without file type restriction first to find all instances
rg -l "DropdownMenuTrigger.*asChild"
# Also search for general usage of DropdownMenuTrigger for comparison
rg -l "DropdownMenuTrigger"
Length of output: 1064
Script:
#!/bin/bash
# Let's check the implementation in the one file that uses asChild
rg "DropdownMenuTrigger.*asChild" -A 3 "apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/chart.tsx"
# Also check the implementation in our component under review for comparison
rg "DropdownMenuTrigger" -A 3 "apps/dashboard/components/data-table/view-options.tsx"
Length of output: 1110
apps/dashboard/components/data-table/column-header.tsx (1)
Line range hint 30-42: LGTM! Consistent dropdown implementation with proper sort state handling.
The dropdown trigger implementation correctly wraps the Button component while maintaining the dynamic sort indicator rendering.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/menu.tsx (1)
Line range hint 32-36: LGTM! Dropdown trigger implementation aligns with PR objectives.
The dropdown trigger correctly wraps the Button component, and the implementation properly supports the filtering functionality mentioned in the PR objectives.
Let's verify the filtering functionality:
✅ Verification successful
Let me also check the menu component implementation:
And let's verify the query state management with nuqs:
LGTM! Filtering functionality is properly implemented in the menu component
The menu component correctly implements the filtering functionality using nuqs query state management:
- Uses
useQueryStatehook with proper parsing and default options - Implements "Filter for identifier" action that sets the identifier filter through
setIdentifier([identifier]) - Integrates with the broader filtering system as evidenced by the shared query state usage across related components (filter.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent filter implementation across rate limit logs
rg -A 5 "setIdentifier.*identifier" --type tsx
Length of output: 76
Script:
#!/bin/bash
# Check for filtering implementation in rate limits logs
rg -t ts -t tsx "setIdentifier|identifier.*filter" apps/
Length of output: 86
Script:
#!/bin/bash
# Check the menu component implementation
ast-grep --pattern $'const Menu = () => {
$$$
}' apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/menu.tsx
Length of output: 117
Script:
#!/bin/bash
# Check query state management
rg -t ts -t tsx "useQueryState" apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/
Length of output: 117
Script:
#!/bin/bash
# Let's try without file type specifications
rg "useQueryState|setIdentifier|identifier.*filter" apps/dashboard/app/
# And check the menu component content
cat apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/menu.tsx
Length of output: 5595
apps/dashboard/components/dashboard/api-key-table/table.tsx (1)
Line range hint 136-140: LGTM: Dropdown trigger implementation looks correct
The implementation aligns with the PR's objective of fixing dropdown triggers.
apps/dashboard/components/dashboard/root-key-table/table.tsx (1)
136-139: Verify dropdown trigger accessibility
The removal of asChild prop from DropdownMenuTrigger changes how the trigger is rendered in the DOM. While this fixes the dropdown trigger issues, we should verify that it maintains proper accessibility.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx (1)
Line range hint 89-93: LGTM! Changes align with fixing dropdown triggers
The removal of asChild prop from DropdownMenuTrigger is consistent with the PR's objective, and the filter functionality remains intact with proper state management.
apps/dashboard/components/dashboard/api-key-table/index.tsx (1)
Line range hint 238-242: LGTM! Dropdown trigger changes are consistent
The removal of asChild prop from DropdownMenuTrigger is consistent with the changes in other files, maintaining a uniform approach to fixing dropdown triggers.
apps/dashboard/app/(app)/settings/user/update-user-email.tsx (1)
126-129: LGTM! Improved dropdown trigger implementation.
The direct wrapping of the Button component within DropdownMenuTrigger improves the dropdown's behavior while maintaining proper keyboard accessibility.
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
Bug Fixes
Documentation
Chores