fix(faq): correct state handling on close and improve button accessibility#113
fix(faq): correct state handling on close and improve button accessibility#113ZinMyoWin wants to merge 1 commit into
Conversation
…ility - Fixed a bug where activeItem state was updated incorrectly when closing a tab - Replaced tabs.find() array search with a direct index lookup for optimization - Removed unnecessary async keyword from handleClick function - Moved onClick handler from the wrapper div to the button element for better web accessibility (a11y)
|
@ZinMyoWin is attempting to deploy a commit to the UI-Layouts OSS Program Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe FAQ accordion component undergoes refactoring to improve type safety and simplify state management. A ChangesFAQ Accordion Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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/ui-layout/registry/components/accordion/faq.tsx (1)
38-38: ⚡ Quick winConsider removing unused
activeItemstate.The
activeItemstate is updated inhandleClickbut never read anywhere in the component. The rendering logic relies entirely onactiveIndexand the derivedisOpenboolean. Unless this state is needed for future features or debugging, it can be safely removed to simplify the component.♻️ Proposed cleanup
- const [activeItem, setActiveItem] = useState<TabItem | undefined>(tabs[0]);Then remove the
setActiveItemcalls fromhandleClick:const handleClick = (index: number) => { const isCurrentActive = activeIndex === index; if (isCurrentActive) { - // If clicking the active tab, close it and clear the active item setActiveIndex(null); - setActiveItem(undefined); } else { - // If clicking a new tab, open it and set the active item cleanly using index matching setActiveIndex(index); - setActiveItem(tabs[index]); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui-layout/registry/components/accordion/faq.tsx` at line 38, Remove the unused state activeItem and its setter setActiveItem from the component: delete the useState line that declares activeItem and remove any setActiveItem(...) calls inside handleClick; ensure rendering still uses activeIndex and the derived isOpen boolean (and existing tabs) so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ui-layout/registry/components/accordion/faq.tsx`:
- Around line 69-79: The accordion toggle button lacks an explicit type and will
default to type="submit" inside forms; update the button element used in the FAQ
accordion (the element that calls handleClick(index) and renders <Plus ... />
and {tab.title}) to include type="button" so it never triggers form submission
unintentionally.
---
Nitpick comments:
In `@apps/ui-layout/registry/components/accordion/faq.tsx`:
- Line 38: Remove the unused state activeItem and its setter setActiveItem from
the component: delete the useState line that declares activeItem and remove any
setActiveItem(...) calls inside handleClick; ensure rendering still uses
activeIndex and the derived isOpen boolean (and existing tabs) so behavior is
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70ab2a5f-391c-4090-89e2-853d1185c4f4
📒 Files selected for processing (1)
apps/ui-layout/registry/components/accordion/faq.tsx
| <button | ||
| className={`p-3 px-2 w-full cursor-pointer sm:text-base text-xs items-center transition-all font-semibold dark:text-white text-black flex gap-2 | ||
| `} | ||
| onClick={() => handleClick(index)} | ||
| className='p-3 px-2 w-full cursor-pointer sm:text-base text-xs items-center transition-all font-semibold dark:text-white text-black flex gap-2' | ||
| > | ||
| <Plus | ||
| className={`${ | ||
| activeIndex === index ? 'rotate-45' : 'rotate-0 ' | ||
| } transition-transform ease-in-out w-5 h-5 dark:text-neutral-200 text-neutral-600`} | ||
| isOpen ? 'rotate-45' : 'rotate-0' | ||
| } transition-transform duration-300 ease-in-out w-5 h-5 dark:text-neutral-200 text-neutral-600`} | ||
| /> | ||
| {tab.title} | ||
| </button> |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submissions.
The button element is missing an explicit type attribute. Without it, the button defaults to type="submit", which can cause unintended form submissions if this accordion is placed inside a <form> element. As per coding guidelines, static analysis correctly flagged this accessibility and correctness issue.
🔘 Proposed fix
<button
+ type='button'
onClick={() => handleClick(index)}
className='p-3 px-2 w-full cursor-pointer sm:text-base text-xs items-center transition-all font-semibold dark:text-white text-black flex gap-2'
>📝 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.
| <button | |
| className={`p-3 px-2 w-full cursor-pointer sm:text-base text-xs items-center transition-all font-semibold dark:text-white text-black flex gap-2 | |
| `} | |
| onClick={() => handleClick(index)} | |
| className='p-3 px-2 w-full cursor-pointer sm:text-base text-xs items-center transition-all font-semibold dark:text-white text-black flex gap-2' | |
| > | |
| <Plus | |
| className={`${ | |
| activeIndex === index ? 'rotate-45' : 'rotate-0 ' | |
| } transition-transform ease-in-out w-5 h-5 dark:text-neutral-200 text-neutral-600`} | |
| isOpen ? 'rotate-45' : 'rotate-0' | |
| } transition-transform duration-300 ease-in-out w-5 h-5 dark:text-neutral-200 text-neutral-600`} | |
| /> | |
| {tab.title} | |
| </button> | |
| <button | |
| type='button' | |
| onClick={() => handleClick(index)} | |
| className='p-3 px-2 w-full cursor-pointer sm:text-base text-xs items-center transition-all font-semibold dark:text-white text-black flex gap-2' | |
| > | |
| <Plus | |
| className={`${ | |
| isOpen ? 'rotate-45' : 'rotate-0' | |
| } transition-transform duration-300 ease-in-out w-5 h-5 dark:text-neutral-200 text-neutral-600`} | |
| /> | |
| {tab.title} | |
| </button> |
🧰 Tools
🪛 Biome (2.4.15)
[error] 69-72: Provide an explicit type prop for the button element.
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ui-layout/registry/components/accordion/faq.tsx` around lines 69 - 79,
The accordion toggle button lacks an explicit type and will default to
type="submit" inside forms; update the button element used in the FAQ accordion
(the element that calls handleClick(index) and renders <Plus ... /> and
{tab.title}) to include type="button" so it never triggers form submission
unintentionally.
1 participant
Summary by CodeRabbit