feat(ui): migrate core components to shadcn primitives (3/3)#593
feat(ui): migrate core components to shadcn primitives (3/3)#593sahilc0 wants to merge 5 commits into
Conversation
Migrate Modal, Checkbox, Select, and Toggle to use shadcn/ui primitives with improved animations and accessibility: Modal: - Replace static overlay with Framer Motion AnimatePresence - Add open/onOpenChange/onExitComplete props for controlled usage - Add enter/exit animations (fade + scale) - Use EASE_OUT_QUINT_TUPLE for consistent easing Checkbox: - Migrate to shadcn Checkbox primitive - Add proper checked state visualization - Maintain haptic feedback on interaction Select: - Migrate to shadcn RadioGroup - Preserve keyboard navigation (arrow keys) - Remove legacy FlexRow wrapper Toggle: - Use shadcn Switch with new lg size variant - Switch gets iOS-like depth with three-layer shadow - 44px minimum tap target for mobile Updated usage sites: - Burn.tsx, Reissue.tsx: Use onExitComplete for async coordination - Backup.tsx, Announcement.tsx: Use controlled Modal props
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
e8ffb38
|
| Status: | 🚫 Build failed. |
Deploying wallet-mutinynet with
|
| Latest commit: |
e8ffb38
|
| Status: | 🚫 Build failed. |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
e8ffb38
|
| Status: | 🚫 Build failed. |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — feat(ui): migrate core components to shadcn primitives (3/3)
Verdict: Request changes — one likely bug, one fragile pattern, a few nits. No protocol-critical concerns (pure UI migration, no signing/VTXO/forfeit logic touched).
🐛 BUG — Announcement close may never fire after "Try it"
src/components/Announcement.tsx:113-114
Old code called close() synchronously in handleTryIt. New code calls setOpen(false) and relies on onExitComplete={close} to eventually fire close.
// Old (guaranteed):
close()
// New (deferred):
setOpen(false) // → animation → onExitComplete → close()If navigate() triggers a re-render that unmounts the Announcement component before the exit animation completes, onExitComplete never fires and close() never runs. The parent still thinks the announcement is active → it reappears when the user returns.
Fix: Call close() directly in handleTryIt (like before), OR use a ref-based pattern like Burn/Reissue to ensure close fires on exit complete while also handling the unmount case. At minimum, add close as a fallback in a cleanup effect:
useEffect(() => {
return () => { if (!open) close() }
}, [open, close])⚠️ FRAGILE — Checkbox dual-handler pattern
src/components/Checkbox.tsx:21-28
The outer <div> has onClick={() => handleChange(!checked)} and the inner <ShadcnCheckbox> has onCheckedChange={handleChange}. The pointer-events-none class on the checkbox prevents mouse-driven double-fire, but:
onCheckedChangeis effectively dead code — it can never fire via pointer events. This is confusing.- Assistive tech (screen readers, Switch Access) may activate the checkbox directly, bypassing
pointer-events-none. If that happens, both handlers fire →onChange()called twice → Reset page'sdisabledstate toggles twice (noop — checkbox looks checked but button stays disabled).
Fix: Remove onCheckedChange from the ShadcnCheckbox since the div's onClick handles everything:
<ShadcnCheckbox
checked={checked}
className='size-6 rounded-md pointer-events-none data-checked:border-primary data-checked:bg-primary data-checked:text-primary-foreground'
/>Or, go the other direction — remove the div's onClick and let the ShadcnCheckbox handle its own state (remove pointer-events-none, use a <label> wrapper).
📝 NITS
1. Modal overlay click-to-dismiss is new behavior
src/components/Modal.tsx:32 — The overlay's onClick={() => onOpenChange?.(false)} lets users dismiss the Burn/Reissue confirmation dialogs by clicking outside. The old Modal didn't support this. Safe (cancels the operation), but intentional? Worth noting for QA.
2. Select: redundant RadioGroupItem
src/components/Select.tsx:42-44 — The RadioGroupItem is visually hidden (opacity-0, absolute, -m-px, size-px). The label's onClick calls event.preventDefault() + handleChange() manually, so the radio never fires its own change event. The RadioGroup's onValueChange also fires from handleChange. This works but the RadioGroupItem is essentially vestigial — consider whether it adds accessibility value (it does provide ARIA semantics, so probably keep it, but worth a comment).
3. Select useEffect deps fix is good
src/components/Select.tsx:29 — Adding options and onChange to the dep array is a legit bugfix 👍. Stale closures were possible before.
4. Button backward compat is clean
src/components/Button.tsx:63-64 — The resolvedVariant fallback from boolean props to CVA variants preserves backward compatibility. Good approach.
✅ Good patterns
- Burn/Reissue
pendingConfirmref pattern (Burn.tsx:33,77-83,Reissue.tsx:34,76-82): Clean way to coordinate animation completion with async operations. No double-fire risk since the ref is reset before callingprocessBurn(). - SheetModal → Drawer migration (
SheetModal.tsx): Clean swap from react-spring-bottom-sheet to vaul. ThehandleOpenChangeonly fires haptic + close on!open— correct. - Switch
lgvariant (switch.tsx): Well-structured with proper thumb positioning and 44px tap target.
Test coverage
Only Checkbox has a new test. Modal animation, Select keyboard nav with RadioGroup, and Toggle Switch migration have no tests. Acceptable for UI-only changes, but the Announcement close bug above would be caught by an integration test.
TL;DR: Fix the Announcement close race condition (real bug), clean up the Checkbox dual-handler (fragile), and verify the overlay click-to-dismiss is intentional. Everything else looks solid.
Restore direct announcement close behavior for the Try action so parent state clears even if navigation unmounts before exit animation completion. Update the migrated checkbox wrapper to use a label-bound control path and guard same-state events, avoiding duplicate onChange calls across pointer and assistive activation paths.
…omponent-migration-20260430 # Conflicts: # bun.lock # src/screens/Apps/Assets/Burn.tsx # src/screens/Apps/Assets/Reissue.tsx
There was a problem hiding this comment.
Follow-up Review — new commits since last review
Reviewed 66da6dc (review fixes), 51dfff8 (merge from base), e8ffb38 (lockfile restore).
✅ Previous findings addressed
1. Announcement close race (BUG) — Fixed correctly. handleTryIt now calls close() directly (line 113), matching the old behavior. Parent state clears immediately regardless of whether the exit animation completes before unmount. Good.
2. Checkbox dual-handler (FRAGILE) — Fixed well. Changed from the awkward div onClick + pointer-events-none combo to a proper <label htmlFor> + <ShadcnCheckbox id> pattern. pointer-events-none removed, so the ShadcnCheckbox is the single source of truth for interaction. The if (nextChecked === checked) return guard at Checkbox.tsx:16 prevents the edge case of duplicate onChange calls. This is the idiomatic Radix/shadcn approach. 👍
⚠️ Note: Cloudflare builds failing
All three Cloudflare Pages deploys are failing. The e8ffb38 commit looks like an attempt to fix this (restores bun.lock), but the checks haven't re-run on that commit yet. Base branch PR #590 builds pass, so the issue is in this PR's build. Confirm builds pass before merging.
No new issues found. Previous concerns resolved. Approving.
1 participant
Stack
wt-shadcn-ui-20260430, notmaster.Summary
Migrate Modal, Checkbox, Select, and Toggle to use shadcn/ui primitives with improved animations and accessibility.
Modal
open,onOpenChange,onExitCompleteprops for controlled usageEASE_OUT_QUINT_TUPLE)Checkbox
Select
Toggle
lgsize variantSwitch (ui primitive)
lgsize variant (24×44px track, 20px thumb)Updated Usage Sites
Burn.tsx,Reissue.tsx: UseonExitCompletefor async operation coordinationBackup.tsx,Announcement.tsx: Use controlled Modal propsTest Plan
bun run test)Files Changed
src/components/Modal.tsxsrc/components/Checkbox.tsxsrc/components/Select.tsxsrc/components/Toggle.tsxsrc/components/ui/switch.tsxsrc/components/ui/dialog.tsxsrc/components/Announcement.tsxsrc/screens/Apps/Assets/Burn.tsxsrc/screens/Apps/Assets/Reissue.tsxsrc/screens/Settings/Backup.tsxsrc/test/components/Checkbox.test.tsx