fix: Auditlogs Filters now can be clicked on #2639
Conversation
BadriVishalPadhy
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@BadriVishalPadhy is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes in the pull request focus on the Changes
Assessment against linked issues
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! 🙏 |
BadriVishalPadhy
changed the title
|
@chronark plz do check |
![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
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (1)
75-103: Consider refactoring duplicate state update logic.The state update logic is duplicated between
onMouseDownCaptureandonSelecthandlers. Consider extracting this into a shared function to improve maintainability.Here's a suggested refactor:
+ const toggleSelection = () => { + const next = isSelected + ? selected.filter((v) => v !== option.value) + : Array.from(new Set([...selected, option.value])); + setSelected(next); + }; <div key={option.value} - onMouseDownCapture={() => { - const next = isSelected - ? selected.filter((v) => v !== option.value) - : Array.from(new Set([...selected, option.value])); - setSelected(next); - }} + onMouseDownCapture={toggleSelection} > <CommandItem key={option.value} - onSelect={() => { - const next = isSelected - ? selected.filter((v) => v !== option.value) - : Array.from(new Set([...selected, option.value])); - setSelected(next); - }} + onSelect={toggleSelection} >🧰 Tools
🪛 Biome
[error] 75-83: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx
[error] 75-83: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (2)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (2)
93-98: LGTM! Well-structured styling implementation.
The conditional styling provides clear visual feedback and follows best practices:
- Uses utility function for class merging
- Proper state-based styling
- Accessible implementation with proper icon visibility
75-103: Implementation successfully addresses the clicking issue.
The changes effectively solve the reported bug by:
- Adding mouse interaction through
onMouseDownCapture - Preserving existing keyboard navigation
- Maintaining proper visual feedback for selected state
This implementation aligns well with the PR objectives from issue #2391.
🧰 Tools
🪛 Biome
[error] 75-83: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (1)
75-104: Consider refactoring duplicate state update logic.The state update logic is duplicated between
onMouseDownCaptureandonSelecthandlers. Consider extracting this into a shared function to improve maintainability.+ const toggleOption = (option: { value: string }) => { + const next = selected.includes(option.value) + ? selected.filter((v) => v !== option.value) + : Array.from(new Set([...selected, option.value])); + setSelected(next); + }; <div key={option.value} - onMouseDownCapture={() => { - const next = isSelected - ? selected.filter((v) => v !== option.value) - : Array.from(new Set([...selected, option.value])); - setSelected(next); - }} + onMouseDownCapture={() => toggleOption(option)} > <CommandItem key={option.value} - onSelect={() => { - const next = isSelected - ? selected.filter((v) => v !== option.value) - : Array.from(new Set([...selected, option.value])); - setSelected(next); - }} + onSelect={() => toggleOption(option)} >🧰 Tools
🪛 Biome
[error] 75-82: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx
[error] 75-82: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (2)
apps/dashboard/app/(app)/audit/[bucket]/filter.tsx (2)
92-103: LGTM! Clear visual feedback for selection state.
The implementation provides excellent visual feedback with the checkbox-like behavior and proper state management. This successfully addresses the original issue of filters not being clickable.
75-104: Verify the fix resolves the original issue.
The implementation looks good and directly addresses the reported issue in #2391. Please verify:
- All filter options are now clickable with mouse
- Keyboard navigation still works as before
- Visual feedback is clear when selecting/deselecting options
✅ Verification successful
Mouse interaction implementation is correctly isolated and non-conflicting
The implementation looks clean with only one mouse event handler (onMouseDownCapture) in the filters component, ensuring no conflicting mouse interactions. The dual-handling with both onMouseDownCapture and onSelect provides robust interaction support for both mouse and keyboard users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no other mouse event handlers that might interfere
rg -A 2 "onMouse" apps/dashboard/app/
Length of output: 353
🧰 Tools
🪛 Biome
[error] 75-82: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
| <div | ||
| onMouseDownCapture={() => { | ||
| const next = isSelected | ||
| ? selected.filter((v) => v !== option.value) | ||
| : Array.from(new Set([...selected, option.value])); | ||
|
|
||
| setSelected(next); | ||
| // params.set(param, next); | ||
| }} | ||
| > |
There was a problem hiding this comment.
Add key prop to the wrapper div.
The wrapper div in the iteration is missing a key prop, which is required for React's reconciliation process.
Apply this change:
<div
+ key={option.value}
onMouseDownCapture={() => {📝 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.
| <div | |
| onMouseDownCapture={() => { | |
| const next = isSelected | |
| ? selected.filter((v) => v !== option.value) | |
| : Array.from(new Set([...selected, option.value])); | |
| setSelected(next); | |
| // params.set(param, next); | |
| }} | |
| > | |
| <div | |
| key={option.value} | |
| onMouseDownCapture={() => { | |
| const next = isSelected | |
| ? selected.filter((v) => v !== option.value) | |
| : Array.from(new Set([...selected, option.value])); | |
| setSelected(next); | |
| }} | |
| > |
🧰 Tools
🪛 Biome
[error] 75-82: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
|
@chronark any issue? |
What does this PR do?
This PR fixes - on clicking any of the filter option in the audit log page except the system one, all events users and root keys are not clickable, only way to select them is via the keyboard
Fixes #2391
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?
Go to sidebar -> audit logs-> in the events tab drop down try clicking on options
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
issue-https://preview.screen.studio/share/zCAzNHHP
after resolvoing -
https://github.com/user-attachments/assets/67995c85-0929-41c0-b69c-86c19b41618c
Summary by CodeRabbit
New Features
Bug Fixes
Refactor