feat(ui): add shadcn/ui primitives#590
Conversation
WalkthroughAdds shadcn config, Tailwind/theme tokens, TS/Vite path aliases, new dependencies, a mobile-detection hook and a large set of reusable UI primitives (buttons, inputs, navigation, overlays, charts, layout, sidebar, carousel, etc.), plus CSS variable palettes and theming. Shadcn UI primitives + config
Estimated code review effort: Possibly related PRs:
Suggested reviewers:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Deploying wallet-bitcoin with
|
| Latest commit: |
0386dbc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://13a3b14f.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://wt-shadcn-ui-20260430.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
0386dbc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c6277e5d.arkade-wallet.pages.dev |
| Branch Preview URL: | https://wt-shadcn-ui-20260430.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
0386dbc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://57da46ad.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://wt-shadcn-ui-20260430.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Introduces a proper design token system with full color ramps and migrates all opacity-based --darkXX tokens to solid --neutral-XXX colors. New files: - src/tokens.css: Single source of truth for colors, typography, shadows - src/app.css: Tailwind v4 @theme configuration mapping tokens to utilities - src/lib/utils.ts: cn() utility combining clsx + tailwind-merge Token architecture: - Color ramps (50-950): purple, green, red, orange, yellow, neutral - Neutral ramp uses color-mix(in oklab) for automatic light/dark adaptation - Dark mode selector: html.palette-dark (consistent with existing app) - Typography tokens: font families, sizes, line heights, letter spacing - Shadow/elevation tokens: sm, md, lg, xl with three-layer depth Migration: - All --darkXX opacity tokens replaced with --neutral-XXX solid colors - Semantic aliases (--purple, --green, etc.) now reference ramps - Legacy aliases added for backward compat (--background-color, --heading-font) - 50+ component files updated to use new token system Dependencies added: - tailwindcss@^4.2.2, @tailwindcss/vite@^4.2.2 - clsx@^2.1.1, tailwind-merge@^3.5.0, class-variance-authority@^0.7.1
sahilc0
force-pushed
the
wt-design-tokens-tailwind-20260429
branch
from
27759e6 to
f1eccc9
Compare
Add 55 shadcn/ui components for future component migrations. Setup: - Add @/* path alias in tsconfig.json and vite.config.ts - Add shadcn theme tokens to app.css - Add components.json configuration Components include: Button, Input, Dialog, Drawer, Sheet, Select, Checkbox, Switch, Tabs, Card, Table, and 45 more primitives. No existing code modified - primitives are available in src/components/ui/ for future use.
sahilc0
force-pushed
the
wt-shadcn-ui-20260430
branch
from
40e5f47 to
7472fbd
Compare
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(ui): add shadcn/ui primitives
No protocol-critical code touched (no VTXO, signing, forfeit, round, or exit paths). This is purely a UI primitives drop. That said, there are several integration issues that will bite you at runtime.
🔴 Must Fix
1. Dark mode will not work on shadcn components
src/app.css adds two competing dark mode mechanisms:
- Existing (line ~4 of base):
@variant dark (&:where(html.palette-dark, html.palette-dark *)); - New (line ~5 of diff):
@custom-variant dark (&:is(.dark *));
The app toggles dark mode via html.palette-dark class (see src/providers/config.tsx:104-107), but all shadcn CSS tokens are scoped under .dark selector. The :root / .dark token block in app.css defines --background, --foreground, etc. under .dark {} — a class that is never added by the existing theme provider.
Result: Every shadcn component will be permanently stuck in light mode colors. The @custom-variant dark also redefines the Tailwind dark: variant, potentially breaking existing components that rely on the palette-dark selector.
Fix: Either (a) update the theme provider to also toggle .dark class on <html>, or (b) change the .dark {} selector in app.css to html.palette-dark {} to match the existing system.
2. sonner.tsx will crash at runtime
src/components/ui/sonner.tsx imports useTheme from next-themes:
import { useTheme } from 'next-themes'This hook requires a <ThemeProvider> from next-themes wrapping the app. No such provider exists in this codebase. The first time any code renders <Toaster />, it will throw.
Fix: Either wrap the app in a next-themes ThemeProvider, or remove the useTheme dependency and wire it to the existing useConfig() theme state instead.
3. @layer base overrides existing global styles
src/app.css adds:
@layer base {
* { @apply border-border outline-ring/50; }
body { @apply bg-background text-foreground; }
html { @apply font-sans; }
}This applies border-color to every element, overrides the body background/text color with shadcn tokens (which won't match the existing palette in dark mode — see issue #1), and forces font-sans (Geist Variable) on <html>, replacing whatever font the app currently uses.
These are not inert primitives — they are global style overrides that will visually change the existing app.
Fix: Remove the @layer base block entirely. Shadcn components should use their own classes; global style reset should be a separate, deliberate PR with visual QA.
🟡 Should Fix
4. shadcn CLI belongs in devDependencies
package.json lists "shadcn": "^4.6.0" in dependencies. This is a CLI scaffolding tool (like create-react-app). It should be in devDependencies to avoid shipping it in production bundles.
5. recharts exact pin is inconsistent
"recharts": "3.8.0" — every other dependency uses ^ ranges. If this is intentional (known regression in 3.8.1+), add a comment. Otherwise use "^3.8.0".
🟢 Observations (non-blocking)
6. 13 new runtime dependencies for unused code — None of the 55 new components are imported by existing code. This is fine as a staging approach, but the dependency footprint is real: recharts, embla-carousel-react, react-day-picker, cmdk, date-fns, input-otp, react-resizable-panels, vaul, @base-ui/react, @fontsource-variable/geist, sonner, tw-animate-css, next-themes. Consider adding components (and their deps) incrementally as they're actually used.
7. No tests for new components — The 179 passing tests are all pre-existing. Acceptable for vendored primitives, but worth noting.
8. utils.ts change is cosmetic — Import reorder (clsx, type ClassValue → type ClassValue, clsx). No functional change. Fine.
Summary
The core issue is the dark mode integration gap (issues #1, #2, #3). The shadcn token system assumes a .dark class toggle that doesn't exist here, and the @layer base block will visually change the running app today. These need to be resolved before merge — either adapt shadcn's output to the existing palette-dark system, or introduce .dark class toggling alongside it.
Everything else is minor. No security or protocol concerns.
- Dark mode: change .dark selector to html.palette-dark in app.css and chart.tsx - sonner.tsx: replace next-themes useTheme with ConfigContext (fixes crash) - Remove body/html overrides from @layer base (keeps border rule) - Move shadcn CLI to devDependencies - Remove unused next-themes dependency - Use ^3.8.0 for recharts (consistency) - Fix eslint array-index-key warnings in chart.tsx and field.tsx - Fix prettier formatting in 5 shadcn components
|
@arkanaai review latest changes |
There was a problem hiding this comment.
Re-review: all feedback addressed ✅
Checked the changes since my previous review. All five issues are resolved:
| # | Issue | Status |
|---|---|---|
| 1 | Dark mode .dark vs palette-dark mismatch |
✅ Fixed — unified to html.palette-dark in app.css and chart.tsx |
| 2 | sonner.tsx crash (next-themes import) |
✅ Fixed — now uses ConfigContext + Themes from existing codebase |
| 3 | @layer base global style overrides |
✅ Fixed — removed entirely |
| 4 | shadcn CLI in production deps |
✅ Fixed — moved to devDependencies |
| 5 | recharts exact pin |
✅ Fixed — now ^3.8.0 |
Also removed next-themes and @base-ui/react from dependencies. Clean.
No protocol-critical code touched. LGTM for merge (after #582 lands).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
6-45:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
@base-ui/reacttodependencies.
src/components/ui/direction.tsximportsDirectionProvideranduseDirectionfrom@base-ui/react/direction-provider, but this package is not declared inpackage.json. This will cause a module-not-found error at build time or runtime.Suggested fix
"class-variance-authority": "^0.7.1", "clsx": "^2.1.1", + "@base-ui/react": "^1.4.1", "cmdk": "^1.1.1",🤖 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 `@package.json` around lines 6 - 45, The project imports DirectionProvider and useDirection from '@base-ui/react/direction-provider' in src/components/ui/direction.tsx but the package is missing from package.json; add "@base-ui/react" to the dependencies (pick a compatible version or use a careted latest), update package.json accordingly, run your package manager install (npm/yarn/pnpm) to restore modules, and re-run the build to ensure DirectionProvider and useDirection resolve at runtime.
♻️ Duplicate comments (2)
src/components/ui/spinner.tsx (1)
4-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame missing
Reactimport asskeleton.tsx.
React.ComponentProps<'svg'>requiresReactin scope; the same fix applies:+import * as React from 'react' import { cn } from '@/lib/utils'🤖 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 `@src/components/ui/spinner.tsx` around lines 4 - 5, The Spinner component uses React.ComponentProps<'svg'> but React is not imported; add an import for React at the top of the file (e.g., import React from 'react') so Spinner({ className, ...props }: React.ComponentProps<'svg'>) compiles; update the file that defines Spinner (and ensure imports for Loader2Icon and cn remain untouched).src/components/ui/direction.tsx (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
@base-ui/reactis not declared as a dependency.This import will fail at build/runtime since
@base-ui/reactis absent frompackage.json. See the corresponding comment onpackage.jsonfor the fix.🤖 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 `@src/components/ui/direction.tsx` at line 1, The file re-exports DirectionProvider and useDirection from '@base-ui/react' but that package is not declared in package.json; add '@base-ui/react' to dependencies in package.json (using the appropriate version constraint), run npm/yarn install to update lockfile, and then verify the import in src/components/ui/direction.tsx (symbols: DirectionProvider, useDirection) resolves correctly in both dev and build.
🟡 Minor comments (15)
src/components/ui/aspect-ratio.tsx-7-13 (1)
7-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
--ratiowhen caller passesstyleSpreading
...propsafterstyleallowsprops.styleto replace the internal--ratiovariable, which can break sizing.Suggested fix
-function AspectRatio({ ratio, className, ...props }: React.ComponentProps<'div'> & { ratio: number }) { +function AspectRatio({ + ratio, + className, + style, + ...props +}: React.ComponentProps<'div'> & { ratio: number }) { return ( <div data-slot='aspect-ratio' - style={ - { - '--ratio': ratio, - } as React.CSSProperties - } + style={{ ...(style as React.CSSProperties), ['--ratio' as '--ratio']: ratio }} className={cn('relative aspect-(--ratio)', className)} {...props} /> ) }🤖 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 `@src/components/ui/aspect-ratio.tsx` around lines 7 - 13, The component currently sets style={{ '--ratio': ratio } as React.CSSProperties} and then spreads {...props}, allowing props.style to overwrite --ratio; fix by merging styles so the internal CSS variable wins: build the final style as an object that spreads props.style first and then sets '--ratio': ratio (e.g., finalStyle = { ...props.style, '--ratio': ratio } as React.CSSProperties), pass that finalStyle to the element and still spread the rest of props; update references in the AspectRatio component where style and {...props} are used so callers cannot override the internal --ratio.src/components/ui/pagination.tsx-39-43 (1)
39-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
data-active={isActive}rendersdata-active="false"whenisActiveisfalse— attribute remains presentReact serialises boolean
falseas the string"false"fordata-*attributes, so the attribute is present in the DOM. Any CSS[data-active]presence check or Tailwinddata-active:variant will incorrectly match inactive links. Use|| undefinedto omit the attribute when not active:🐛 Proposed fix
- <a aria-current={isActive ? 'page' : undefined} data-slot='pagination-link' data-active={isActive} {...props} /> + <a aria-current={isActive ? 'page' : undefined} data-slot='pagination-link' data-active={isActive || undefined} {...props} />🤖 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 `@src/components/ui/pagination.tsx` around lines 39 - 43, The anchor in the render prop currently sets data-active={isActive}, which produces data-active="false" when isActive is false; change it to omit the attribute when inactive by assigning data-active={isActive || undefined} (or data-active={isActive ? true : undefined}) in the render JSX for the anchor so the data-active attribute is only present when isActive is truthy.src/components/ui/sonner.tsx-24-31 (1)
24-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
React.CSSPropertiesis used without importing theReactnamespace
import { useContext } from 'react'only brings in the named export;React.CSSPropertiesin the type assertion is unresolved and would failtsc --noEmit. Vite/esbuild silently strips the annotation, so the runtime build passes.🛡️ Proposed fix
+'use client' + +import * as React from 'react' import { useContext } from 'react'Or inline the type annotation without the namespace:
- } as React.CSSProperties + } as React.CSSProperties // keep and add namespace import above🤖 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 `@src/components/ui/sonner.tsx` around lines 24 - 31, The style object uses the type assertion React.CSSProperties but the React namespace isn't imported, causing TypeScript errors; fix by either importing the React namespace (add import React from 'react') so React.CSSProperties is resolved, or remove the namespace and import the type directly (import { CSSProperties } from 'react') and change the assertion to CSSProperties on the style object in src/components/ui/sonner.tsx (the inline style block with '--normal-bg', '--normal-text', etc.).src/components/ui/pagination.tsx-72-83 (1)
72-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
sr-onlytext inside anaria-hiddencontainer is never read by assistive technology
aria-hiddenpropagates to all descendants, so the<span className='sr-only'>More pages</span>on Line 81 is invisible to screen readers. Either remove the dead span, or movearia-hiddenonto the icon only so the text is announced:🐛 Option A — remove the unreachable sr-only span
<span aria-hidden data-slot='pagination-ellipsis' className={cn("flex size-8 items-center justify-center [&_svg:not([class*='size-'])]:size-4", className)} {...props} > <MoreHorizontalIcon /> - <span className='sr-only'>More pages</span> </span>🐛 Option B — announce "More pages" to screen readers (move aria-hidden to the icon only)
<span - aria-hidden data-slot='pagination-ellipsis' className={cn("flex size-8 items-center justify-center [&_svg:not([class*='size-'])]:size-4", className)} {...props} > - <MoreHorizontalIcon /> + <MoreHorizontalIcon aria-hidden /> <span className='sr-only'>More pages</span> </span>🤖 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 `@src/components/ui/pagination.tsx` around lines 72 - 83, The wrapper span in PaginationEllipsis currently has aria-hidden which hides its child <span className='sr-only'>More pages</span> from screen readers; remove aria-hidden from the outer span in PaginationEllipsis and instead apply aria-hidden to the visual icon (MoreHorizontalIcon) so the decorative icon is ignored but the sr-only text is announced; keep the existing data-slot, className and props spread on the wrapper and ensure the MoreHorizontalIcon receives aria-hidden (or a surrounding element around the icon) so accessibility is preserved.src/components/ui/empty.tsx-64-74 (1)
64-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
EmptyDescriptionis typed asReact.ComponentProps<'p'>but renders a<div>— element/type mismatchThe prop type advertises
HTMLParagraphElementsemantics (including therefshape), but the rendered element is anHTMLDivElement. Any consumer passing arefwould receive a mistyped DOM node. Semantically, a description/body-copy slot should also be a<p>.🛡️ Proposed fix — render `
` to match the declared type and improve semantics
function EmptyDescription({ className, ...props }: React.ComponentProps<'p'>) { return ( - <div + <p data-slot='empty-description' className={cn( 'text-sm/relaxed text-muted-foreground [&>a]:underline [&>a]:underline-offset-4 [&>a:hover]:text-primary', className, )} {...props} - /> + /> ) }🤖 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 `@src/components/ui/empty.tsx` around lines 64 - 74, The component EmptyDescription is typed as React.ComponentProps<'p'> but renders a <div>, causing an element/type mismatch; change the rendered element in the EmptyDescription function from a div to a p so the DOM node matches the declared type and semantics, keep the data-slot='empty-description', preserve className merging via cn and spread {...props} onto the <p> so refs and paragraph-specific props work correctly with the existing EmptyDescription signature.src/components/ui/input-group.tsx-54-59 (1)
54-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
InputGroupAddonclick-to-focus skips<textarea>controls
querySelector('input')only finds<input>elements. When the group contains anInputGroupTextarea, clicking the addon silently does nothing because noinputelement exists. Extend the selector to covertextareaas well:🐛 Proposed fix
- e.currentTarget.parentElement?.querySelector('input')?.focus() + e.currentTarget.parentElement?.querySelector('input, textarea')?.focus()🤖 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 `@src/components/ui/input-group.tsx` around lines 54 - 59, The click handler on the InputGroupAddon currently focuses only input elements using querySelector('input'), so clicking the addon does nothing when the group contains a textarea; update the selector in the onClick handler (the arrow function bound to onClick in src/components/ui/input-group.tsx / the InputGroupAddon click handler) to target both inputs and textareas (e.g., querySelector('input, textarea')) so clicking the addon focuses the appropriate control.src/components/ui/command.tsx-119-119 (1)
119-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix incorrect SVG descendant selector:
data-selected:*:[svg]:text-foregroundshould use[&_svg]patternThe selector
data-selected:*:[svg]:text-foregrounddoes not target SVG descendants as intended. In Tailwind v4, the*:variant generates> *(matching any direct child), and chaining it with[svg]produces an ineffective selector. The correct pattern used elsewhere in the same component is:Fix
- data-selected:*:[svg]:text-foreground + data-selected:[&_svg]:text-foregroundThe
[&_svg]arbitrary selector pattern is used consistently throughout the codebase for SVG descendants and should be applied here for consistency.🤖 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 `@src/components/ui/command.tsx` at line 119, The class string in src/components/ui/command.tsx contains an incorrect SVG descendant selector token `data-selected:*:[svg]:text-foreground`; replace that token with the correct Tailwind arbitrary descendant pattern `[&_svg]:text-foreground` in the same class string (the one starting with "group/command-item relative flex ...") so SVG descendants receive the intended styles when `data-selected` is present.src/app.css-317-323 (1)
317-323:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRadius scale values diverge significantly between
@themeblocks.The new
@theme inlineblock recomputes radii from--radius: 0.625rem, producing values that contradict the earlier@themedefinitions:
Token Earlier @theme@theme inlineresult--radius-sm0.25rem0.375rem(+50%)--radius-lg1rem0.625rem(−38%)--radius-xl1.5rem0.875rem(−42%)
rounded-lgandrounded-xlutilities will render noticeably tighter corners than the design token intent. Align--radiuswith the existing scale, or remove the duplicate radius definitions from@theme inlineand retain the earlier hardcoded ones.🤖 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 `@src/app.css` around lines 317 - 323, The inline `@theme` block redefines --radius (and thus --radius-sm, --radius-md, --radius-lg, --radius-xl, etc.) producing values that conflict with the original `@theme` tokens; either align the inline --radius to the original token base or remove the duplicated radius calculations. Update the `@theme` inline block to use the same --radius base value as the original theme (so --radius-sm, --radius-lg, --radius-xl compute identically) or delete the inline definitions for --radius, --radius-sm, --radius-md, --radius-lg, --radius-xl, --radius-2xl, --radius-3xl, --radius-4xl and rely on the earlier hardcoded tokens to keep rounded-lg/rounded-xl semantics consistent.src/components/ui/select.tsx-126-153 (1)
126-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScroll arrow buttons are missing a
stickypositioning class.
top-0/bottom-0alone have no visual effect inside theoverflow-y-autopopup — the buttons will scroll out of view with the list content. Addstickyso they stay anchored:🐛 Proposed fix
- "top-0 z-10 flex w-full cursor-default items-center justify-center bg-popover py-1 [&_svg:not([class*='size-'])]:size-4", + "sticky top-0 z-10 flex w-full cursor-default items-center justify-center bg-popover py-1 [&_svg:not([class*='size-'])]:size-4",- "bottom-0 z-10 flex w-full cursor-default items-center justify-center bg-popover py-1 [&_svg:not([class*='size-'])]:size-4", + "sticky bottom-0 z-10 flex w-full cursor-default items-center justify-center bg-popover py-1 [&_svg:not([class*='size-'])]:size-4",🤖 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 `@src/components/ui/select.tsx` around lines 126 - 153, The scroll arrow buttons in SelectScrollUpButton and SelectScrollDownButton use top-0/bottom-0 but lack sticky positioning so they scroll away; update the className strings in both components (SelectPrimitive.ScrollUpArrow and SelectPrimitive.ScrollDownArrow) to include the "sticky" utility alongside the existing "top-0" and "bottom-0" classes so the buttons remain anchored while the popup content scrolls.src/components/ui/field.tsx-104-115 (1)
104-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
FieldTitlesharesdata-slot='field-label'withFieldLabel— confirm intent.Both
FieldLabel(Line 93) andFieldTitle(Line 107) emitdata-slot='field-label'. Any CSS selector targeting[data-slot=field-label]— including*:data-[slot=field-label]:flex-autoinfieldVariants— will apply to both. If this is intentional for layout purposes, a comment explaining the shared slot would help future maintainers. IfFieldTitleis meant to be independently targetable, it should use its own slot name (e.g.,data-slot='field-title').🤖 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 `@src/components/ui/field.tsx` around lines 104 - 115, FieldTitle and FieldLabel both emit data-slot='field-label', which causes shared CSS selectors like the fieldVariants rule (*:data-[slot=field-label]:flex-auto) to hit both; decide whether they should share styling or be distinct: if they should be distinct, change FieldTitle's data-slot to 'field-title' and update any selectors in fieldVariants (and other styles) that should target the new slot name; if sharing is intentional, add a brief inline comment above the FieldTitle component explaining why it uses the same data-slot as FieldLabel to avoid future confusion.src/components/ui/input-otp.tsx-17-17 (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSuspicious
cn-input-otpclass — verify intent.The
cn-input-otpclass on thecontainerClassNameis unusual: thecn-prefix shadows thecn()utility name used throughout this file, and no other component in this PR uses a similar namespace prefix.data-slot='input-otp'already provides a reliable CSS hook for this container. Please confirm whether this is an intentional named class (used somewhere in the project CSS) or a typo.🧹 If unintentional, remove the prefix
- containerClassName={cn('cn-input-otp flex items-center has-disabled:opacity-50', containerClassName)} + containerClassName={cn('flex items-center has-disabled:opacity-50', containerClassName)}🤖 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 `@src/components/ui/input-otp.tsx` at line 17, The class string passed to containerClassName includes a suspicious token 'cn-input-otp' which likely collides with the cn() utility name or is a typo; verify whether 'cn-input-otp' is intentionally referenced in project CSS, and if not remove it from the containerClassName value so the component uses only the intended classes (e.g., 'flex items-center has-disabled:opacity-50' plus any containerClassName prop) and rely on data-slot='input-otp' for styling; update the JSX where containerClassName is set (the cn(...) call in this component) accordingly.src/components/ui/field.tsx-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
data-[slot=checkbox-group]:gap-3never matches — dead selector.
FieldGroupalways renders withdata-slot='field-group', so the conditiondata-[slot=checkbox-group]on itself can never be true. Compare toFieldSetwhich correctly useshas-[>[data-slot=checkbox-group]]:gap-3to detect a child with that slot. The intended behaviour (tighter gap when containing a checkbox group) should use the samehas-pattern.🐛 Proposed fix
- 'group/field-group `@container/field-group` flex w-full flex-col gap-5 data-[slot=checkbox-group]:gap-3 *:data-[slot=field-group]:gap-4', + 'group/field-group `@container/field-group` flex w-full flex-col gap-5 has-[>[data-slot=checkbox-group]]:gap-3 has-[>[data-slot=radio-group]]:gap-3 *:data-[slot=field-group]:gap-4',🤖 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 `@src/components/ui/field.tsx` around lines 41 - 43, The selector 'data-[slot=checkbox-group]:gap-3' in FieldGroup is dead because FieldGroup itself renders with data-slot='field-group'; replace that self-targeting selector with the parent-has-child variant used in FieldSet (e.g. use the has-[>[data-slot=checkbox-group]]:gap-3 pattern) so the container detects a child with data-slot="checkbox-group" and applies the tighter gap; update the class string in the FieldGroup component where className is composed.src/components/ui/navigation-menu.tsx-95-95 (1)
95-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
easing-[ease]is not a valid Tailwind utility — likely a typo forease-[ease].
easing-is not a Tailwind prefix; the correct utility fortransition-timing-functionisease-*. As written, the class is silently ignored, so the intended animation timing for the ending-style state is never applied.🔧 Proposed fix
-<NavigationMenuPrimitive.Popup className='data-[ending-style]:easing-[ease] xs:w-(--popup-width) ...'> +<NavigationMenuPrimitive.Popup className='data-[ending-style]:ease-[ease] xs:w-(--popup-width) ...'>🤖 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 `@src/components/ui/navigation-menu.tsx` at line 95, The tailwind class string on NavigationMenuPrimitive.Popup uses the invalid utility "easing-[ease]" so the ending-style timing isn't applied; update the className for the Popup (NavigationMenuPrimitive.Popup) to replace "easing-[ease]" with the correct Tailwind utility "ease-[ease]" so the transition-timing-function is applied for the data-[ending-style] state.src/components/ui/item.tsx-9-20 (1)
9-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
role="list"onItemGroupwithout a correspondingrole="listitem"onItemchildren.The ARIA
listrole requires its owned elements to haverole="listitem".Itemrenders adiv(or custom element viarender) and theuseRenderstateobject only setsdata-*attributes — it does not injectrole. Screen readers may not associateItemnodes as list members, breaking list navigation.Consider adding
role="listitem"either directly inItem'smergePropscall or as a documented requirement for consumers.🔧 Proposed fix
function Item({ ... }: useRender.ComponentProps<'div'> & VariantProps<typeof itemVariants>) { return useRender({ defaultTagName: 'div', props: mergeProps<'div'>( { + role: 'listitem', className: cn(itemVariants({ variant, size, className })), }, props, ),🤖 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 `@src/components/ui/item.tsx` around lines 9 - 20, The ItemGroup component uses role="list" but Item children lack role="listitem"; update the Item component (the function that uses useRender and mergeProps/render logic) to include role="listitem" in the merged props (e.g., add role: 'listitem' in the mergeProps/returned props or when spreading state.attrs) so every rendered Item element is exposed as a list item to assistive tech; alternatively, if you prefer not to alter props, document in the Item component's API that consumers must pass role="listitem" when rendering inside ItemGroup.src/components/ui/breadcrumb.tsx-69-81 (1)
69-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
sr-onlytext insidearia-hidden="true"is unreachable by screen readers.
aria-hidden="true"on the outer<span>hides the entire subtree from the accessibility tree, including the<span className='sr-only'>More</span>child. The "More" label is never announced. Either remove thesr-onlyspan (if the intent is to fully hide the ellipsis from AT), or removearia-hiddenand keep thesr-onlytext (if "More" should be read aloud).🔧 Proposed fix (remove dead sr-only span)
<span data-slot='breadcrumb-ellipsis' role='presentation' aria-hidden='true' className={cn('flex size-5 items-center justify-center [&>svg]:size-4', className)} {...props} > <MoreHorizontalIcon /> - <span className='sr-only'>More</span> </span>🤖 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 `@src/components/ui/breadcrumb.tsx` around lines 69 - 81, The outer span in BreadcrumbEllipsis has aria-hidden="true" which hides its subtree, making the inner <span className='sr-only'>More</span> unreachable; decide intent and fix accordingly—if the ellipsis is purely decorative, remove the inaccessible sr-only child from the BreadcrumbEllipsis JSX and keep aria-hidden; if the ellipsis should be announced, remove aria-hidden from the outer span and retain the sr-only label so screen readers can read "More". Ensure the change is made inside the BreadcrumbEllipsis component.
🧹 Nitpick comments (9)
src/components/ui/spinner.tsx (1)
5-5: 💤 Low valueSpread
{...props}afterrole/aria-labellets callers silently override accessibility attributes.This is intentional in many component libraries for composability, but worth being explicit about. If the intent is to always enforce
role="status", move those attributes after{...props}:♻️ Proposed fix
- return <Loader2Icon role='status' aria-label='Loading' className={cn('size-4 animate-spin', className)} {...props} /> + return <Loader2Icon {...props} role='status' aria-label='Loading' className={cn('size-4 animate-spin', className)} />🤖 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 `@src/components/ui/spinner.tsx` at line 5, The Spinner component spreads {...props} before setting role and aria-label which allows callers to override those accessibility attributes; to enforce them always, move role='status' and aria-label='Loading' after the {...props} spread in the JSX returned by the Spinner (the line rendering Loader2Icon) so Loader2Icon always receives role and aria-label last and cannot be overridden by props while keeping className composition via cn('size-4 animate-spin', className).src/hooks/use-mobile.ts (1)
9-14: ⚡ Quick winMinor inconsistency between
matchMediaandwindow.innerWidth.
matchMediameasures viewport width including the scrollbar, whilewindow.innerWidthexcludes it. This means the listener fires on breakpoint crossing (correct threshold) butonChangere-reads a slightly different width. For consistency, read frommql.matchesinsideonChangeinstead of re-queryingwindow.innerWidth.♻️ Proposed fix
- const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) - } + const onChange = () => { + setIsMobile(mql.matches) + } mql.addEventListener('change', onChange) - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) + setIsMobile(mql.matches)🤖 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 `@src/hooks/use-mobile.ts` around lines 9 - 14, The onChange handler is inconsistent because it reads window.innerWidth while the listener is attached to mql; update onChange to use mql.matches to determine the breakpoint state and pass that value to setIsMobile so the listener and initial state use the same source (refer to mql, onChange, setIsMobile, and MOBILE_BREAKPOINT).src/components/ui/hover-card.tsx (1)
25-43: 💤 Low value
data-slotonPortalis a no-op and may be misleading.
PreviewCardPrimitive.Portal(like all Base UI portals) renders children directly intodocument.bodywithout a wrapping DOM node, sodata-slot='hover-card-portal'won't appear in the DOM. Compare:SelectContent'sSelectPrimitive.Portal(line 57 inselect.tsx) correctly omitsdata-slot. The meaningfuldata-slotis already on thePopupat line 34.🛠️ Proposed fix
- <PreviewCardPrimitive.Portal data-slot='hover-card-portal'> + <PreviewCardPrimitive.Portal>🤖 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 `@src/components/ui/hover-card.tsx` around lines 25 - 43, Remove the misleading no-op data-slot on PreviewCardPrimitive.Portal: delete the data-slot='hover-card-portal' attribute from the PreviewCardPrimitive.Portal element and leave the meaningful data-slot on PreviewCardPrimitive.Popup (which is the rendered DOM node). This ensures only the actual rendered Popup carries the data-slot; update any related tests/docs if they referenced the portal data-slot.src/components/ui/calendar.tsx (1)
134-167: ⚡ Quick win
getDefaultClassNames()is called on every render of every day button.
CalendarDayButtonis instantiated for each visible day cell (up to ~42 per month). CallinggetDefaultClassNames()fresh on every render is unnecessary since the result is static. Move it to a module-level constant or cache it in the parent:♻️ Proposed fix
+const defaultDayButtonClassNames = getDefaultClassNames() + function CalendarDayButton({ className, day, modifiers, locale, ...props }: React.ComponentProps<typeof DayButton> & { locale?: Partial<Locale> }) { - const defaultClassNames = getDefaultClassNames() - const ref = React.useRef<HTMLButtonElement>(null) ... className={cn( '...', - defaultClassNames.day, + defaultDayButtonClassNames.day, className, )}🤖 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 `@src/components/ui/calendar.tsx` around lines 134 - 167, CalendarDayButton unnecessarily calls getDefaultClassNames() on every render for each day cell; move the static result out of the component to avoid repeated computation by either creating a module-level constant (e.g., DEFAULT_CLASS_NAMES = getDefaultClassNames()) and use that inside CalendarDayButton, or compute it once in the parent and pass it into CalendarDayButton as a prop (or at minimum wrap the call in React.useMemo with an empty dependency array). Update references to defaultClassNames in CalendarDayButton to use the new DEFAULT_CLASS_NAMES (or the prop name) and remove the inline call to getDefaultClassNames().src/components/ui/alert-dialog.tsx (1)
125-142: ⚡ Quick winRedundant
cn(className)calls inAlertDialogActionandAlertDialogCancel.
cn(singleString)returns the string unchanged — it has no effect here. Either passclassNamedirectly or add actual base classes that justify thecn()call.♻️ Proposed fix
function AlertDialogAction({ className, ...props }: React.ComponentProps<typeof Button>) { - return <Button data-slot='alert-dialog-action' className={cn(className)} {...props} /> + return <Button data-slot='alert-dialog-action' className={className} {...props} /> } function AlertDialogCancel({ className, variant = 'outline', size = 'default', ...props }: AlertDialogPrimitive.Close.Props & Pick<React.ComponentProps<typeof Button>, 'variant' | 'size'>) { return ( <AlertDialogPrimitive.Close data-slot='alert-dialog-cancel' - className={cn(className)} + className={className} render={<Button variant={variant} size={size} />} {...props} /> ) }🤖 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 `@src/components/ui/alert-dialog.tsx` around lines 125 - 142, The cn(className) calls in AlertDialogAction and AlertDialogCancel are redundant because cn with a single string returns it unchanged; update these components to either pass className directly to the underlying elements (replace cn(className) with className) or provide actual base classes to cn (e.g., cn('base-classes', className)) so the helper is meaningful; change the return in AlertDialogAction and the className prop on AlertDialogPrimitive.Close (and keep the render prop using Button) accordingly, referencing AlertDialogAction, AlertDialogCancel, Button, AlertDialogPrimitive.Close, and cn.src/components/ui/combobox.tsx (1)
181-195: ⚡ Quick win
ComboboxChipsprops type is inconsistently double-wrapped.Every other component in this file uses only
ComboboxPrimitive.X.Props, butComboboxChipsintersects it withReact.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips>. This creates a redundant intersection that can introduce conflicting or duplicated prop definitions.ComboboxPrimitive.Chips.Propsalone is sufficient and consistent with the rest.♻️ Proposed fix
function ComboboxChips({ className, ...props -}: React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> & ComboboxPrimitive.Chips.Props) { +}: ComboboxPrimitive.Chips.Props) {🤖 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 `@src/components/ui/combobox.tsx` around lines 181 - 195, The ComboboxChips props are unnecessarily typed as an intersection of React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> and ComboboxPrimitive.Chips.Props, causing redundant/Conflicting types; change the component signature to accept only ComboboxPrimitive.Chips.Props (i.e., update the ComboboxChips parameter type to use ComboboxPrimitive.Chips.Props directly) and remove the React.ComponentPropsWithRef<> wrapper so the typing matches the other components in this file.src/components/ui/progress.tsx (1)
1-1: 💤 Low value
'use client'directive is a no-op in this Vite project.The project uses Vite without RSC support, so
'use client'has no effect and is inconsistent with all other newly addedsrc/components/ui/*files, which omit it. It's likely a copy-paste artifact from a Next.js-based shadcn template.🧹 Proposed fix
-'use client' - import { Progress as ProgressPrimitive } from '@base-ui/react/progress'🤖 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 `@src/components/ui/progress.tsx` at line 1, Remove the stray `'use client'` directive from the top of src/components/ui/progress.tsx (it is a no-op in this Vite project and inconsistent with other components); simply delete that line so the Progress component file matches the rest of the ui components and avoids Next.js-specific artifacts.src/components/ui/menubar.tsx (1)
73-76: ⚡ Quick win
MenubarContentis missing close animation classes present onMenubarSubContent.
MenubarSubContent(line 233) includesdata-closed:animate-out data-closed:fade-out-0 data-closed:zoom-out-95, butMenubarContent(line 74) omits them. The menu will appear without an exit animation when dismissed.✨ Proposed fix
className={cn( - 'min-w-36 rounded-lg bg-popover p-1 text-popover-foreground shadow-md ring-1 ring-foreground/10 duration-100 data-[side=bottom]:slide-in-from-top-2 data-[side=inline-end]:slide-in-from-left-2 data-[side=inline-start]:slide-in-from-right-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 data-open:animate-in data-open:fade-in-0 data-open:zoom-in-95', + 'min-w-36 rounded-lg bg-popover p-1 text-popover-foreground shadow-md ring-1 ring-foreground/10 duration-100 data-[side=bottom]:slide-in-from-top-2 data-[side=inline-end]:slide-in-from-left-2 data-[side=inline-start]:slide-in-from-right-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 data-open:animate-in data-open:fade-in-0 data-open:zoom-in-95 data-closed:animate-out data-closed:fade-out-0 data-closed:zoom-out-95', className, )}🤖 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 `@src/components/ui/menubar.tsx` around lines 73 - 76, MenubarContent is missing the exit animation classes that MenubarSubContent uses; update the className in the MenubarContent component to include the same data-closed classes (data-closed:animate-out data-closed:fade-out-0 data-closed:zoom-out-95) so the top-level menu gets a closing animation like MenubarSubContent; locate the className invocation in the MenubarContent JSX and append those data-closed classes to the existing animation-related class list.src/components/ui/sheet.tsx (1)
39-74: ⚡ Quick win
sidebar.tsxhides the close button via CSS instead ofshowCloseButton={false}— fragile.
sidebar.tsx(line 171) suppresses theSheetContentclose button using[&>button]:hidden. This works today becauseSheetPrimitive.Closerenders aButtonas a direct child, but the selector will silently break ifSheetContent's internal DOM structure changes. TheshowCloseButtonprop exists precisely for this use case.In
sidebar.tsx, replace:- className='w-(--sidebar-width) bg-sidebar p-0 text-sidebar-foreground [&>button]:hidden' + className='w-(--sidebar-width) bg-sidebar p-0 text-sidebar-foreground' ... + showCloseButton={false}🤖 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 `@src/components/ui/sheet.tsx` around lines 39 - 74, The sidebar hides the close button via a fragile CSS selector instead of using the SheetContent prop; update the sidebar usage to pass showCloseButton={false} to the SheetContent component and remove the `[&>button]:hidden` rule from the sidebar styles so the visibility is controlled by the SheetContent prop (SheetContent / showCloseButton and SheetPrimitive.Close are the relevant symbols to modify).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f62289a-512b-433c-822b-9b1872ff0c12
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
components.jsonpackage.jsonsrc/app.csssrc/components/ui/accordion.tsxsrc/components/ui/alert-dialog.tsxsrc/components/ui/alert.tsxsrc/components/ui/aspect-ratio.tsxsrc/components/ui/avatar.tsxsrc/components/ui/badge.tsxsrc/components/ui/breadcrumb.tsxsrc/components/ui/button-group.tsxsrc/components/ui/button.tsxsrc/components/ui/calendar.tsxsrc/components/ui/card.tsxsrc/components/ui/carousel.tsxsrc/components/ui/chart.tsxsrc/components/ui/checkbox.tsxsrc/components/ui/collapsible.tsxsrc/components/ui/combobox.tsxsrc/components/ui/command.tsxsrc/components/ui/context-menu.tsxsrc/components/ui/dialog.tsxsrc/components/ui/direction.tsxsrc/components/ui/drawer.tsxsrc/components/ui/dropdown-menu.tsxsrc/components/ui/empty.tsxsrc/components/ui/field.tsxsrc/components/ui/hover-card.tsxsrc/components/ui/input-group.tsxsrc/components/ui/input-otp.tsxsrc/components/ui/input.tsxsrc/components/ui/item.tsxsrc/components/ui/kbd.tsxsrc/components/ui/label.tsxsrc/components/ui/menubar.tsxsrc/components/ui/native-select.tsxsrc/components/ui/navigation-menu.tsxsrc/components/ui/pagination.tsxsrc/components/ui/popover.tsxsrc/components/ui/progress.tsxsrc/components/ui/radio-group.tsxsrc/components/ui/resizable.tsxsrc/components/ui/scroll-area.tsxsrc/components/ui/select.tsxsrc/components/ui/separator.tsxsrc/components/ui/sheet.tsxsrc/components/ui/sidebar.tsxsrc/components/ui/skeleton.tsxsrc/components/ui/slider.tsxsrc/components/ui/sonner.tsxsrc/components/ui/spinner.tsxsrc/components/ui/switch.tsxsrc/components/ui/table.tsxsrc/components/ui/tabs.tsxsrc/components/ui/textarea.tsxsrc/components/ui/toggle-group.tsxsrc/components/ui/toggle.tsxsrc/components/ui/tooltip.tsxsrc/hooks/use-mobile.tssrc/lib/utils.tstsconfig.jsonvite.config.ts
| @theme inline { | ||
| --font-heading: var(--font-sans); | ||
| --font-sans: 'Geist Variable', sans-serif; | ||
| --color-sidebar-ring: var(--sidebar-ring); | ||
| --color-sidebar-border: var(--sidebar-border); | ||
| --color-sidebar-accent-foreground: var(--sidebar-accent-foreground); | ||
| --color-sidebar-accent: var(--sidebar-accent); | ||
| --color-sidebar-primary-foreground: var(--sidebar-primary-foreground); | ||
| --color-sidebar-primary: var(--sidebar-primary); | ||
| --color-sidebar-foreground: var(--sidebar-foreground); | ||
| --color-sidebar: var(--sidebar); | ||
| --color-chart-5: var(--chart-5); | ||
| --color-chart-4: var(--chart-4); | ||
| --color-chart-3: var(--chart-3); | ||
| --color-chart-2: var(--chart-2); | ||
| --color-chart-1: var(--chart-1); | ||
| --color-ring: var(--ring); | ||
| --color-input: var(--input); | ||
| --color-border: var(--border); | ||
| --color-destructive: var(--destructive); | ||
| --color-accent-foreground: var(--accent-foreground); | ||
| --color-accent: var(--accent); | ||
| --color-muted-foreground: var(--muted-foreground); | ||
| --color-muted: var(--muted); | ||
| --color-secondary-foreground: var(--secondary-foreground); | ||
| --color-secondary: var(--secondary); | ||
| --color-primary-foreground: var(--primary-foreground); | ||
| --color-primary: var(--primary); | ||
| --color-popover-foreground: var(--popover-foreground); | ||
| --color-popover: var(--popover); | ||
| --color-card-foreground: var(--card-foreground); | ||
| --color-card: var(--card); | ||
| --color-foreground: var(--foreground); | ||
| --color-background: var(--background); |
There was a problem hiding this comment.
@theme inline silently overrides all color utilities to shadcn grayscale defaults.
In Tailwind v4, a later @theme declaration wins over an earlier one for the same variable name. The @theme inline block redefines every semantic color token—--color-primary, --color-ring, --color-border, --color-destructive, etc.—pointing them at new :root CSS variables (--primary, --ring, …) whose values are shadcn's out-of-the-box oklch grayscale palette:
| Token | Earlier @theme (brand) |
@theme inline (shadcn default) |
|---|---|---|
--color-primary |
var(--purple-700) |
oklch(0.205 0 0) ≈ near-black |
--color-ring |
var(--purple-700) |
oklch(0.708 0 0) grey |
--color-destructive |
var(--red-700) |
oklch(0.577 0.245 27.325) |
Any existing code using bg-primary, text-primary, ring-ring, etc. will silently switch from the brand palette to these greyscale defaults.
To preserve the brand identity, wire the shadcn :root variables to the arkade tokens:
🐛 Proposed fix — map shadcn runtime variables to arkade tokens in :root
:root {
- --background: oklch(1 0 0);
- --foreground: oklch(0.145 0 0);
...
- --primary: oklch(0.205 0 0);
- --primary-foreground: oklch(0.985 0 0);
- --secondary: oklch(0.97 0 0);
- --secondary-foreground: oklch(0.205 0 0);
...
- --ring: oklch(0.708 0 0);
+ --background: var(--bg);
+ --foreground: var(--fg);
...
+ --primary: var(--purple-700);
+ --primary-foreground: `#fff`;
+ --secondary: var(--neutral-100);
+ --secondary-foreground: var(--fg);
...
+ --ring: var(--purple-700);🧰 Tools
🪛 Stylelint (17.10.0)
[error] 283-283: Unexpected unknown at-rule "@theme" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🤖 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 `@src/app.css` around lines 283 - 316, The `@theme` inline block is replacing
your brand colors because shadcn reads its own :root vars (e.g. --primary,
--ring, --border, --destructive, --accent, --muted, --secondary, --popover,
--card, --foreground, --background, --chart-1..5, --sidebar-*, etc.); fix this
by adding a :root mapping that points every shadcn runtime variable to your
arkade semantic tokens (for example set --primary: var(--color-primary); --ring:
var(--color-ring); --border: var(--color-border); --destructive:
var(--color-destructive); --input: var(--color-input); --accent:
var(--color-accent); --muted: var(--color-muted); --secondary:
var(--color-secondary); --popover: var(--color-popover); --card:
var(--color-card); --foreground: var(--color-foreground); --background:
var(--color-background); --chart-1: var(--color-chart-1); … and map the
--sidebar-* names to your --color-sidebar-* tokens) so that when `@theme` inline
defines --color-* values it resolves back to your brand palette rather than the
shadcn grayscale defaults.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
There was a problem hiding this comment.
Re-review (new commit 39b8f05c)
Single commit since my last approval — carousel keyboard nav fix from CodeRabbit suggestion.
src/components/ui/carousel.tsx:78-90 — Adds ArrowUp/ArrowDown support for vertical carousels, correctly guarded by !isHorizontal. orientation properly added to the useCallback dependency array. Clean.
No protocol-critical code touched. Still LGTM.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
There was a problem hiding this comment.
Re-review (new commit 12ac4737)
Single commit since last approval — CodeRabbit co-authored fix to toggle-group.tsx.
src/components/ui/toggle-group.tsx:41-44 — Fixes spacing calculation: --gap now properly computed as calc(var(--spacing) * ${spacing}) instead of raw number, and gap-[var(--gap)] replaces the invalid gap-[--spacing(var(--gap))] syntax. Correct fix.
No protocol-critical code touched. Still LGTM.
There was a problem hiding this comment.
Re-review (new commit 1d94a6ef)
Single commit since last approval — CodeRabbit co-authored fix to tabs.tsx.
src/components/ui/tabs.tsx:9 — Passes orientation prop to TabsPrimitive.Root. Previously only data-orientation (CSS) was set but the actual prop wasn't forwarded, meaning keyboard navigation (arrow key direction) would default to horizontal regardless of the orientation value. Correct fix.
No protocol-critical code. LGTM. ✅
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
There was a problem hiding this comment.
Re-review (new commit 13a4c725)
separator.tsx — Correct fix. data-[orientation=horizontal]: properly targets the data-orientation="horizontal" attribute rendered by @base-ui/react/separator, replacing the shorthand data-horizontal: which only matches boolean data-horizontal attributes.
Consistent with the same pattern applied to toggle-group.tsx and tabs.tsx in the previous commits.
No protocol-critical code. LGTM. ✅
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
There was a problem hiding this comment.
Re-review (new commit 5f6b51f1)
src/components/ui/kbd.tsx:17 — KbdGroup now returns <div> instead of <kbd>, correctly matching its React.ComponentProps<'div'> type. Previous version had a semantic mismatch (wrapper typed as div but rendered as kbd). Clean fix.
No protocol-critical code. LGTM. ✅
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
There was a problem hiding this comment.
Re-review (new commit 94158bbe)
src/components/ui/chart.tsx:88-95 — CSS injection hardening in ChartStyle. Good fix:
CSS.escape(id)on the attribute selector — prevents selector breakoutsafeKey = key.replace(/[^a-zA-Z0-9_-]/g, '')— whitelists CSS custom property name charssafeColor = color?.replace(/[;{}]/g, '')— prevents declaration/block escape
Also correctly removes stale ) from the arrow-to-implicit-return conversion (line 101). File compiles clean.
No protocol-critical code. LGTM. ✅
There was a problem hiding this comment.
Re-review (commits 011111f6..eecd64a1)
Three commits since last approval: carousel cleanup fix, CodeRabbit suggestions batch, lockfile update.
✅ Clean
carousel.tsx:103— Now cleans upreInitlistener in the effect teardown, matching the subscription at line 98. Correct.command.tsx:40-47—DialogHeadermoved insideDialogContent. Required by Radix Dialog —DialogTitle/DialogDescriptionmust be descendants ofDialogContentfor thearia-labelledby/aria-describedbylinkage to work. Correct fix.sidebar.tsx:452—hsl(var(--sidebar-border))→var(--sidebar-border). Matches Tailwind v4 convention where CSS variables already contain the full color value. Correct.slider.tsx:6— Default from[min, max]to[min]. A single-thumb slider should default to one value, not a range. Correct.chart.tsx:104— Missing closing)forChartStylereturn. Syntax fix.
⚠️ Potential duplicate React keys
chart.tsx:178 and chart.tsx:263 — Both ChartTooltipContent and ChartLegendContent now use key={key} where key = nameKey ?? item.dataKey ?? 'value'.
Problem: if a consumer passes nameKey (a single string applied to all items), every item in the .map() gets the same key. The previous code had index as a fallback to guarantee uniqueness. The new code dropped index entirely.
This won't crash but will produce React warnings and potentially incorrect DOM reconciliation when payload items share the same dataKey or when nameKey is provided.
Suggested fix — append index to guarantee uniqueness:
// chart.tsx, tooltip (~line 178)
key={`${key}-${index}`}
// chart.tsx, legend (~line 263) — restore index param too
.map((item, index) => {
...
key={`${key}-${index}`}Low severity since this is UI-only and not yet consumed, but worth fixing before these components get used.
No protocol-critical code touched. Overall LGTM — the key issue is minor but easy to fix now.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/ui/chart.tsx`:
- Around line 169-178: The current tooltip/legend mapping in the payload
iteration uses a derived key string (computed via
nameKey/item.name/item.dataKey) which can collide across items; update the key
generation in the payload.map used for rendering tooltip/legend items (the block
that computes const key = `${nameKey ?? item.name ?? item.dataKey ?? 'value'}`
and calls getPayloadConfigFromPayload) to produce collision-safe keys by
appending a unique discriminator (e.g., include the loop index, item.id,
item.type, or a hash) so each rendered <div> has a stable unique key (for
example change the key to combine the derived key with index or item-specific
id) and apply the same change to the other mapping that renders legend items
(the similar block around lines 254-263).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ec50588-0fd5-47bb-b69d-6ef7ddf2043e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
package.jsonsrc/components/ui/carousel.tsxsrc/components/ui/chart.tsxsrc/components/ui/command.tsxsrc/components/ui/kbd.tsxsrc/components/ui/separator.tsxsrc/components/ui/sidebar.tsxsrc/components/ui/slider.tsxsrc/components/ui/tabs.tsxsrc/components/ui/toggle-group.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/separator.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/ui/command.tsx
- src/components/ui/slider.tsx
- src/components/ui/toggle-group.tsx
- src/components/ui/tabs.tsx
- src/components/ui/sidebar.tsx
- package.json
- src/components/ui/carousel.tsx
| {payload | ||
| .filter((item) => item.type !== 'none') | ||
| .map((item, index) => { | ||
| const key = `${nameKey ?? item.name ?? item.dataKey ?? 'value'}` | ||
| const itemConfig = getPayloadConfigFromPayload(config, item, key) | ||
| const indicatorColor = color ?? item.payload?.fill ?? item.color | ||
|
|
||
| return ( | ||
| <div | ||
| key={key} |
There was a problem hiding this comment.
Use collision-safe React keys for tooltip/legend items.
Line 178 and Line 262 can collide when multiple items resolve to the same derived key, which can cause React reconciliation glitches and warnings.
💡 Proposed fix
@@
- {payload
+ {payload
.filter((item) => item.type !== 'none')
.map((item, index) => {
const key = `${nameKey ?? item.name ?? item.dataKey ?? 'value'}`
+ const reactKey = `${item.dataKey ?? key}-${index}`
const itemConfig = getPayloadConfigFromPayload(config, item, key)
const indicatorColor = color ?? item.payload?.fill ?? item.color
return (
<div
- key={key}
+ key={reactKey}
className={cn(
@@
- {payload
+ {payload
.filter((item) => item.type !== 'none')
- .map((item) => {
+ .map((item, index) => {
const key = `${nameKey ?? item.dataKey ?? 'value'}`
+ const reactKey = `${item.dataKey ?? key}-${index}`
const itemConfig = getPayloadConfigFromPayload(config, item, key)
return (
<div
- key={key}
+ key={reactKey}
className={cn('flex items-center gap-1.5 [&>svg]:h-3 [&>svg]:w-3 [&>svg]:text-muted-foreground')}
>Also applies to: 254-263
🤖 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 `@src/components/ui/chart.tsx` around lines 169 - 178, The current
tooltip/legend mapping in the payload iteration uses a derived key string
(computed via nameKey/item.name/item.dataKey) which can collide across items;
update the key generation in the payload.map used for rendering tooltip/legend
items (the block that computes const key = `${nameKey ?? item.name ??
item.dataKey ?? 'value'}` and calls getPayloadConfigFromPayload) to produce
collision-safe keys by appending a unique discriminator (e.g., include the loop
index, item.id, item.type, or a hash) so each rendered <div> has a stable unique
key (for example change the key to combine the derived key with index or
item-specific id) and apply the same change to the other mapping that renders
legend items (the similar block around lines 254-263).
2 participants
Summary
Add 55 shadcn/ui components as primitives for future component migrations.
Merge order
This PR is part of a chain:
Merge #582 first, then this PR.
Changes
@/*path alias in tsconfig.json and vite.config.tssrc/components/ui/use-mobile.tshookComponents added
Button, Input, Dialog, Drawer, Sheet, Select, Checkbox, Switch, Tabs, Card, Table, Accordion, Alert, Avatar, Badge, Breadcrumb, Calendar, Carousel, Chart, Collapsible, Combobox, Command, ContextMenu, DropdownMenu, HoverCard, InputOTP, Label, Menubar, NavigationMenu, Pagination, Popover, Progress, RadioGroup, Resizable, ScrollArea, Separator, Sidebar, Skeleton, Slider, Sonner, Spinner, Textarea, Toggle, ToggleGroup, Tooltip, and more.
Test plan
Summary by CodeRabbit
New Features
Chores