fix: Lint error using onClick on a div #2756 #2778
Conversation
GARY121github
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@GARY121github is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request addresses a lint error in the Changes
Assessment against linked issues
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 comments (1)
apps/dashboard/app/(app)/audit/components/filters/filter.tsx (1)
Based on the search results, I can now provide a definitive response. Looking at other implementations in the codebase, I can see that
CommandItemis typically used directly without being wrapped in aButtoncomponent. The other examples show proper usage patterns whereCommandItemhandles its own interactivity through theonSelectprop.Remove the outer Button wrapper to avoid nested interactive elements.
The
CommandItemcomponent already provides:
- Keyboard navigation (through the Command component)
- Click handling via
onSelect- Proper styling and focus management
Instead of the current nested structure, you should:
- Move the selection logic to the
CommandItem'sonSelectprop (which is already implemented correctly)- Remove the wrapping
Buttoncomponent entirely🔗 Analysis chain
Line range hint
84-116: Avoid nesting interactive elements for better accessibility.Nesting a Button (interactive element) around a CommandItem (also interactive) can create accessibility and UX issues. Screen readers and keyboard navigation might misinterpret overlapping interactivity. Consider replacing the CommandItem or the Button so only one component manages clicks and focus.
Let me verify if the
CommandItemcomponent is indeed an interactive element by checking its implementation in the codebase.
Based on the search results, I can see that
CommandItemis indeed an interactive element. It's built on top ofCommandPrimitive.Itemand includes interactive styles likecursor-pointerand keyboard navigation attributes (aria-selected). This confirms that we have a nested interactive element issue as mentioned in the review comment.Let me verify if this is a common pattern in the codebase or if there are better examples of implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for CommandItem component definition ast-grep --pattern 'const CommandItem = $_' # Search for any CommandItem import statements rg 'import.*CommandItem' -A 2 # Search for CommandItem usage patterns ast-grep --pattern 'CommandItem $$$'Length of output: 7880
Script:
#!/bin/bash # Search for other CommandItem implementations to see if there are better patterns rg -B 5 -A 5 '<CommandItem'Length of output: 11218
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/audit/components/filters/filter.tsx (2)
84-85: Potential duplication of event handling logic.You're already calling “handleSelection” inside the CommandItem’s onSelect. Wrapping the CommandItem with a Button that also calls “handleSelection” in onClick may cause confusion or lead to double triggers in some cases. Consider consolidating the logic by relying on a single, consistent event handler to simplify future maintenance and avoid potential side effects.
96-96: Consider removing redundant onSelect logic if using onClick event from the Button.If the wrapping Button's onClick event is the primary way to toggle selection, evaluate if the onSelect inside the CommandItem is still needed. Removing duplicates helps keep the code DRY and easier to maintain.
What does this PR do?
Fixes #2756
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