Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Panda CSS-based design system, new Ark UI primitives (Button, Group, Loader, Spinner, AbsoluteCenter), extensive theme tokens/recipes, TypeScript path aliases, tooling/config, and package manifest updates; refactors Counter to use the new Button and applies a root Panda CSS class at app startup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Counter
participant Button
participant Loader
participant Spinner
User->>Counter: click increment control
Counter->>Button: call onClick (wrap(() => countAtom.increment()))
Button->>Button: merge props with ButtonPropsContext
alt loading state enabled
Button->>Loader: render loading UI (spinner/text)
Loader->>Spinner: render Spinner (size="inherit")
Spinner-->>Loader: spinner visible
Loader-->>Button: loading UI rendered
else not loading
Button-->>Counter: normal click handled
end
Button-->>User: visual update (disabled/data-loading as needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR integrates Park UI with a custom Panda CSS theme, introduces a reusable UI component set (Button, Loader, etc.), and wires up CI/CD with coverage-aware Storybook tests and GitHub Pages deployment.
Changes:
- Add a Park UI–style Panda theme (tokens, recipes, global styles) and wire it into
panda.config.tsand TypeScript path aliases. - Introduce shared UI primitives (Button, ButtonGroup, Group, Spinner, Loader, AbsoluteCenter) and migrate the Counter to use them.
- Configure Vitest + Playwright Storybook tests with coverage thresholds, adjust Vite/Storybook bases for GitHub Pages, and add CI/CD workflows for tests and deployment.
Reviewed changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.ts |
Enables v8 coverage with thresholds and configures the Storybook browser project (Playwright, timeouts, CI-friendly Chromium args). |
vite.config.ts |
Adds isCI flag and sets Vite base to /modern-stack/ for CI builds to support GitHub Pages. |
tsconfig.node.json |
Expands includes to cover panda.config.ts and theme so Node-side tooling (Panda, configs) is type-checked. |
tsconfig.json |
Adds baseUrl and a root #* path alias to support top-level TS path resolution across subprojects. |
tsconfig.base.json |
Centralizes baseUrl and defines generic #* and #theme/* path aliases for shared theme access. |
tsconfig.app.json |
Ensures app TS config sees .storybook and theme (albeit with a duplicated theme include entry). |
theme/tokens/z-index.ts |
Defines z-index tokens for various UI layers (overlay, modal, tooltip, etc.). |
theme/tokens/shadows.ts |
Adds semantic shadow tokens with light/dark variants for multiple elevation levels. |
theme/tokens/durations.ts |
Introduces named animation duration tokens used by recipes like Spinner. |
theme/tokens/colors.ts |
Adds base black and white color tokens including alpha variants. |
theme/text-styles.ts |
Defines text styles for headings, body sizes, and a label style. |
theme/recipes/spinner.ts |
Adds a Spinner recipe that uses CSS vars for size and Panda keyframe/duration tokens. |
theme/recipes/index.ts |
Aggregates button, group, absolute-center, and spinner recipes into recipes/slotRecipes. |
theme/recipes/group.ts |
Adds a Group recipe with orientation, attached, and grow variants for grouped controls. |
theme/recipes/button.ts |
Implements the Park-style Button recipe with variants (solid, surface, subtle, outline, plain) and size scales. |
theme/recipes/absolute-center.ts |
Provides an AbsoluteCenter recipe to flex-center content along horizontal/vertical/both axes. |
theme/layer-styles.ts |
Adds a disabled layer style (cursor, opacity, grayscale) used by button _disabled. |
theme/keyframes.ts |
Declares keyframe tokens for collapse, fade, slide, scale, and positional animations. |
theme/global-css.ts |
Sets global CSS variables, default color palette (gray), and body background/foreground tokens. |
theme/conditions.ts |
Defines custom conditions for light mode, invalid/hover/active/checked/on/pinned states. |
theme/colors/red.ts |
Provides semantic tokens for the red palette, including solid/subtle/surface/outline/plain variants. |
theme/colors/orange.ts |
Provides semantic tokens for the orange palette similar to red. |
theme/colors/olive.ts |
Defines neutral/gray-like semantic tokens for solid/subtle/surface/outline/plain variants. |
theme/colors/green.ts |
Defines semantic tokens for the green palette, including button-friendly variants. |
theme/animation-styles.ts |
Adds reusable animation styles (slide-fade, scale-fade) based on placement or transform origin. |
src/shared/components/ui/spinner.tsx |
Exposes a Spinner component by styling ark.span with the Spinner recipe. |
src/shared/components/ui/loader.tsx |
Implements a Loader component that composes Spinner and AbsoluteCenter around arbitrary children and text. |
src/shared/components/ui/index.ts |
Central barrel export for AbsoluteCenter, Button, ButtonGroup, Group, Loader, and Spinner. |
src/shared/components/ui/group.tsx |
Wraps ark.div with the Group recipe, exporting a Group component and its props type. |
src/shared/components/ui/button.tsx |
Adds Button and ButtonGroup components, integrating loading state (via Loader) and recipe-based variant propagation through context. |
src/shared/components/ui/absolute-center.tsx |
Wraps ark.div with the AbsoluteCenter recipe to create an AbsoluteCenter component. |
src/reatom.init.ts |
Keeps the clearStack() call and adds a strong warning comment about not removing it. |
src/main.tsx |
Applies a global color palette class via css({ colorPalette: 'orang' }) and wires up the App root. |
src/counter/Counter.tsx |
Refactors Counter to use the new shared Button component instead of inline-styled <button> elements. |
panda.config.ts |
Integrates the new theme (tokens, recipes, conditions, globalCss), sets jsxFramework: 'react', and configures a plugin to strip Panda’s default colors, but points outdir to src/sstyled-system (which doesn’t match existing #styled-system aliases). |
package.json |
Adds imports for #theme/*, new dependencies (@ark-ui/react, lucide-react), and testing scripts including test:coverage. |
mise.toml |
Pins Bun via mise (bun = "1.3.6") for consistent runtime/tooling in CI. |
components.json |
Introduces Park UI CLI configuration, linking to panda.config.ts and defining alias names for components, hooks, theme, and UI. |
bun.lock |
Updates lockfile to include new dependencies (Ark UI, lucide-react, Floating UI, Zag JS, etc.). |
README.md |
Documents live app/Storybook URLs and adds sections describing the CI test workflow and GitHub Pages deployment. |
.storybook/main.ts |
Configures Storybook’s Vite builder to use different base paths in CI production (for GitHub Pages /modern-stack/storybook/). |
.github/workflows/test.yml |
Adds a GitHub Actions workflow that installs Bun via mise, runs typecheck/format/lint, executes Storybook Vitest tests with Chromium, and uploads coverage (with a JSON summary in the job output). |
.github/workflows/storybook-deploy.yml |
Adds a GitHub Actions workflow that builds the app and Storybook, nests Storybook under /storybook, and deploys the combined artifact to GitHub Pages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // The output directory for your css system | ||
| outdir: 'src/shared/styled-system', | ||
| outdir: 'src/sstyled-system', |
There was a problem hiding this comment.
The Panda CSS outdir points to src/sstyled-system, but all existing path aliases (e.g. #styled-system/* in tsconfig.app.json and package.json imports) target src/shared/styled-system. This mismatch will cause generated styles to be emitted into a different folder than the one your runtime and TypeScript configuration use, breaking imports like #styled-system/css; please align outdir with the configured aliases.
| outdir: 'src/sstyled-system', | |
| outdir: 'src/shared/styled-system', |
|
|
||
| const root = document.getElementById('root') | ||
| assert(root, 'Root element not found') | ||
| root.className = css({ colorPalette: 'orang' }) |
There was a problem hiding this comment.
The colorPalette value 'orang' appears to be a typo and does not match the defined color palettes (e.g. orange), so this class assignment is likely a no-op or will select an unintended palette. Consider updating it to a valid palette name so the root element gets the expected theme colors.
| root.className = css({ colorPalette: 'orang' }) | |
| root.className = css({ colorPalette: 'orange' }) |
| } | ||
| }, | ||
| "include": ["src", ".storybook/**/*"] | ||
| "include": ["src", ".storybook/**/*", "theme", "theme"] |
There was a problem hiding this comment.
"theme" is listed twice in the include array, which is redundant and can be confusing when maintaining the config. It would be clearer to keep each entry unique (e.g. remove the duplicate).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 472eb617b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // The output directory for your css system | ||
| outdir: 'src/shared/styled-system', | ||
| outdir: 'src/sstyled-system', |
There was a problem hiding this comment.
In panda.config.ts, outdir is set to src/sstyled-system, but the project’s #styled-system/* imports are mapped to src/shared/styled-system (see package.json/tsconfig). Since prepare runs panda codegen, the generated files land in src/sstyled-system, leaving #styled-system/* unresolved and causing module-not-found errors during typecheck/build in CI and local builds. The output directory needs to match the alias target.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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)
53-53: Using beta version of Vite.
vite@^8.0.0-beta.10is a pre-release version. Beta versions may contain breaking changes or stability issues. Consider whether this is intentional for early adoption or if the stable version (currentlyvite@7.3.1) would be more appropriate for production readiness.
🤖 Fix all issues with AI agents
In `@panda.config.ts`:
- Line 111: The outdir in panda.config.ts is incorrect ('src/sstyled-system')
causing build outputs not to match the path alias; update the outdir value to
'src/shared/styled-system' so the built files align with the
tsconfig.app.json/package.json alias `#styled-system/*` and imports like
`#styled-system/jsx` resolve to `./src/shared/styled-system/jsx`; ensure you
modify the outdir property in panda.config.ts (the one currently named outdir)
to this exact path.
In `@README.md`:
- Around line 43-47: The README's "Live" section contains bare URLs (the App and
Storybook links) that trigger markdownlint MD034; update the two lines under the
"Live" heading to use proper markdown link syntax by replacing the bare URLs
with descriptive inline links (e.g., change "- App:
https://guria.github.io/modern-stack/" and "- Storybook:
https://guria.github.io/modern-stack/storybook/" to "- App: [Modern Stack
App](https://guria.github.io/modern-stack/)" and "- Storybook: [Modern Stack
Storybook](https://guria.github.io/modern-stack/storybook/)") so the links are
not bare while preserving the same text and targets.
In `@src/counter/Counter.tsx`:
- Line 12: Type error for Button children is due to missing PandaCSS
styled-system artifacts; run the Panda codegen to generate the
./src/shared/styled-system/ module so TypeScript can resolve styled() types
(affecting BaseButton and ButtonProps). Run the project prepare script (panda
codegen) or execute `panda codegen` manually, then rebuild/TypeScript-compile so
Button, BaseButton and ButtonProps types used by wrap(() =>
countAtom.decrement()) resolve correctly.
In `@src/main.tsx`:
- Line 12: The root element's className is being set with an invalid color
palette string; update the css call on root.className = css({ colorPalette:
'orang' }) to use the correct palette name 'orange' so it reads css({
colorPalette: 'orange' }) ensuring the valid color palette is applied.
- Line 8: The import "import { css } from '#styled-system/css'" fails because
panda.config.ts wrongly sets outdir to "src/sstyled-system" (typo with double
's') so the generated files are placed in the wrong directory; open
panda.config.ts and change the outdir value from "src/sstyled-system" to
"styled-system" so the generated output matches the #* path alias resolution and
the import in main.tsx (and any uses of css) can find styled-system/css.
In `@src/shared/components/ui/absolute-center.tsx`:
- Around line 1-9: The Panda CSS output directory is misconfigured causing
imports used by AbsoluteCenter (the styled import and absoluteCenter recipe
referenced in src/shared/components/ui/absolute-center.tsx) to fail; open
panda.config.ts and correct the outdir value from 'src/sstyled-system' to
'src/shared/styled-system' so the generated files match the tsconfig.app.json
alias and the imports for styled and absoluteCenter resolve correctly.
In `@theme/tokens/shadows.ts`:
- Around line 3-46: The shadows use undefined gray tokens (e.g.,
{colors.gray.a4}, {colors.gray.a6}, {colors.gray.a7}, {colors.gray.a8}) used by
the defineSemanticTokens.shadows export; add a gray color object to
theme/tokens/colors.ts that defines the numeric keys (1..12 as needed) and alpha
keys (a2, a4, a6, a7, a8) referenced, or alternatively change all shadow entries
in shadows (xs, sm, md, lg, xl, 2xl, inset) to use existing tokens (like
colors.black.*); also update theme/colors/olive.ts to use the same gray token
names (or replace them with the new gray tokens) so all references
({colors.gray.1}, {colors.gray.12}, {colors.gray.a2}, etc.) resolve.
In `@tsconfig.app.json`:
- Line 16: The tsconfig.app.json "include" array contains a duplicate entry
"theme"; edit the "include" array to remove the redundant "theme" so each path
appears only once (update the value for "include" in tsconfig.app.json to have a
single "theme" entry), ensuring the JSON remains valid.
In `@tsconfig.base.json`:
- Around line 25-30: The tsconfig option "erasableSyntaxOnly" requires
TypeScript >=5.8 but package.json currently pins TypeScript to "^5"; either
update package.json's TypeScript entry (devDependencies/peerDependencies) to
"^5.8" to guarantee compatibility, or remove the "erasableSyntaxOnly" key from
tsconfig.base.json so the project works with older 5.x releases; make the change
in package.json or tsconfig.base.json respectively and keep the existing path
aliases ("#theme/*" and "#*") as-is.
In `@tsconfig.json`:
- Around line 4-8: The root tsconfig.json's path alias "#*" currently maps to
the repository root ("./*") and must be changed to map to the src folder (e.g.,
"#*": ["./src/*"]) so it matches package.json and tsconfig.app.json; update the
"#*" entry in tsconfig.json to point to "./src/*" and verify that any other
config that defines the same alias (package.json, tsconfig.app.json,
vite.config.ts) uses the identical "#*" -> "./src/*" mapping to ensure
consistent module resolution across tools.
♻️ Duplicate comments (1)
src/shared/components/ui/button.tsx (1)
6-7: Type errors are cascading from module resolution failure.The pipeline errors about missing
children,disabled, andasChildproperties are cascading from the unresolved#styled-system/*modules. Once theoutdirtypo inpanda.config.tsis fixed, these types should resolve correctly asBaseButtonPropswill properly inherit from the styled button component.
🧹 Nitpick comments (8)
src/reatom.init.ts (1)
3-4: Consider adding rationale to the comment.While the warning serves its purpose, the comment would be more helpful if it explained why the
clearStack()call is critical. For example, mentioning that it prevents memory leaks, clears previous state, or is required for proper initialization would help future maintainers understand the necessity rather than just being told not to remove it.📝 Suggested improvement
-// Don't dare to remove this line! +// Critical: clearStack() must be called during initialization to clear Reatom's +// internal stack and prevent potential memory leaks or stale state issues. clearStack()tsconfig.node.json (1)
8-8: Minor: Redundant include pattern.
"panda.config.ts"is already matched by the"*.config.ts"glob pattern. Consider removing the explicit entry to reduce redundancy.- "include": ["vite.config.ts", "*.config.ts", ".storybook/main.ts", "panda.config.ts", "theme"] + "include": ["vite.config.ts", "*.config.ts", ".storybook/main.ts", "theme"]theme/keyframes.ts (1)
65-97: Minor inconsistency: mixingfromand0%notation.The partial slide keyframes use
'0%'while other keyframes use'from'. Both are valid CSS, but consider using consistent notation throughout for maintainability.♻️ Suggested consistency fix
'slide-from-top': { - '0%': { translate: '0 -0.5rem' }, + from: { translate: '0 -0.5rem' }, to: { translate: '0' }, }, 'slide-from-bottom': { - '0%': { translate: '0 0.5rem' }, + from: { translate: '0 0.5rem' }, to: { translate: '0' }, }, // ... apply same pattern to other partial slide keyframesvitest.config.ts (1)
7-7: Broaden CI detection beyondCI="true".Some CI systems set
CI=1, so the strict check can miss CI mode.♻️ Proposed tweak
-const isCI = process.env['CI'] === 'true' +const isCI = process.env['CI'] === 'true' || process.env['CI'] === '1'.storybook/main.ts (1)
12-18: Avoid hard-coding the repo name in the base path.Derive it from
GITHUB_REPOSITORYso forks/renames keep working without config edits.♻️ Proposed refactor
-viteFinal: (config, { configType }) => { - if (process.env['CI'] === 'true' && configType === 'PRODUCTION') { - config.base = '/modern-stack/storybook/' - } else { - config.base = '/' - } - return config -}, +viteFinal: (config, { configType }) => { + const repo = process.env['GITHUB_REPOSITORY']?.split('/')[1] + if (process.env['CI'] === 'true' && configType === 'PRODUCTION' && repo) { + config.base = `/${repo}/storybook/` + } else { + config.base = '/' + } + return config +},.github/workflows/storybook-deploy.yml (1)
19-43: Consider addingactions/configure-pagesbefore the build.This aligns with GitHub Pages guidance and can set
base_pathfor frameworks if you decide to consume it later.♻️ Suggested step
- name: Checkout code uses: actions/checkout@v4 + + - name: Configure GitHub Pages + uses: actions/configure-pages@v5 - name: Setup mise uses: jdx/mise-action@v2.github/workflows/test.yml (1)
26-41: Cache Playwright browsers to cut CI time.Playwright downloads are large; caching
~/.cache/ms-playwrightusually speeds up runs.♻️ Suggested cache step
- name: Cache Bun dependencies uses: actions/cache@v4 with: path: | ~/.bun/install/cache node_modules/.vite key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock') }} restore-keys: | ${{ runner.os }}-bun- + - name: Cache Playwright browsers + uses: actions/cache@v4 + with: + path: ~/.cache/ms-playwright + key: ${{ runner.os }}-playwright-${{ hashFiles('bun.lock') }} + restore-keys: | + ${{ runner.os }}-playwright- + - name: Install dependencies run: bun install --frozen-lockfile - name: Install Playwright browsers run: bunx playwright install --with-deps chromiumtheme/colors/red.ts (1)
33-33: Consider using token references for consistency.The
solid.fguses literal'white'whileolive.tsuses'{colors.white}'token references. Using token references is more maintainable and allows centralized theme customization.♻️ Suggested change
- fg: { DEFAULT: { value: { _light: 'white', _dark: 'white' } } }, + fg: { DEFAULT: { value: { _light: '{colors.white}', _dark: '{colors.white}' } } },
| ## Live | ||
|
|
||
| - App: https://guria.github.io/modern-stack/ | ||
| - Storybook: https://guria.github.io/modern-stack/storybook/ | ||
|
|
There was a problem hiding this comment.
Replace bare URLs to satisfy MD034.
markdownlint flags Line 45 and Line 46. Convert to proper markdown links.
💡 Suggested fix
- - App: https://guria.github.io/modern-stack/
- - Storybook: https://guria.github.io/modern-stack/storybook/
+ - App: [https://guria.github.io/modern-stack/](https://guria.github.io/modern-stack/)
+ - Storybook: [https://guria.github.io/modern-stack/storybook/](https://guria.github.io/modern-stack/storybook/)📝 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.
| ## Live | |
| - App: https://guria.github.io/modern-stack/ | |
| - Storybook: https://guria.github.io/modern-stack/storybook/ | |
| ## Live | |
| - App: [https://guria.github.io/modern-stack/](https://guria.github.io/modern-stack/) | |
| - Storybook: [https://guria.github.io/modern-stack/storybook/](https://guria.github.io/modern-stack/storybook/) | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
45-45: Bare URL used
(MD034, no-bare-urls)
46-46: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@README.md` around lines 43 - 47, The README's "Live" section contains bare
URLs (the App and Storybook links) that trigger markdownlint MD034; update the
two lines under the "Live" heading to use proper markdown link syntax by
replacing the bare URLs with descriptive inline links (e.g., change "- App:
https://guria.github.io/modern-stack/" and "- Storybook:
https://guria.github.io/modern-stack/storybook/" to "- App: [Modern Stack
App](https://guria.github.io/modern-stack/)" and "- Storybook: [Modern Stack
Storybook](https://guria.github.io/modern-stack/storybook/)") so the links are
not bare while preserving the same text and targets.
| export const shadows = defineSemanticTokens.shadows({ | ||
| xs: { | ||
| value: { | ||
| _light: '0px 1px 2px {colors.gray.a6}, 0px 0px 1px {colors.gray.a7}', | ||
| _dark: '0px 1px 1px {colors.black.a8}, 0px 0px 1px inset {colors.gray.a8}', | ||
| }, | ||
| }, | ||
| sm: { | ||
| value: { | ||
| _light: '0px 2px 4px {colors.gray.a4}, 0px 0px 1px {colors.gray.a4}', | ||
| _dark: '0px 2px 4px {colors.black.a8}, 0px 0px 1px inset {colors.gray.a8}', | ||
| }, | ||
| }, | ||
| md: { | ||
| value: { | ||
| _light: '0px 4px 8px {colors.gray.a4}, 0px 0px 1px {colors.gray.a4}', | ||
| _dark: '0px 4px 8px {colors.black.a8}, 0px 0px 1px inset {colors.gray.a8}', | ||
| }, | ||
| }, | ||
| lg: { | ||
| value: { | ||
| _light: '0px 8px 16px {colors.gray.a4}, 0px 0px 1px {colors.gray.a4}', | ||
| _dark: '0px 8px 16px {colors.black.a8}, 0px 0px 1px inset {colors.gray.a8}', | ||
| }, | ||
| }, | ||
| xl: { | ||
| value: { | ||
| _light: '0px 16px 24px {colors.gray.a4}, 0px 0px 1px {colors.gray.a4}', | ||
| _dark: '0px 16px 24px {colors.black.a8}, 0px 0px 1px inset {colors.gray.a8}', | ||
| }, | ||
| }, | ||
| '2xl': { | ||
| value: { | ||
| _light: '0px 24px 40px {colors.gray.a4}, 0px 0px 1px {colors.gray.a4}', | ||
| _dark: '0px 24px 40px {colors.black.a8}, 0px 0px 1px inset {colors.gray.a8}', | ||
| }, | ||
| }, | ||
| inset: { | ||
| value: { | ||
| _light: 'inset 8px 0 12px -8px {colors.gray.a4}', | ||
| _dark: 'inset 8px 0 12px -8px {colors.black.a6}', | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find color-related files in the theme directory
echo "=== Finding theme structure ==="
fd -t f -e ts . theme/ | head -30Repository: Guria/modern-stack
Length of output: 533
🏁 Script executed:
#!/bin/bash
# Check the colors token file
echo "=== Content of theme/tokens/colors.ts ==="
cat -n theme/tokens/colors.tsRepository: Guria/modern-stack
Length of output: 1573
🏁 Script executed:
#!/bin/bash
# Check if gray is defined anywhere in the theme
echo "=== Searching for 'gray' definition in theme ==="
rg -n "gray" theme/
echo ""
echo "=== Checking theme/colors directory ==="
ls -la theme/colors/Repository: Guria/modern-stack
Length of output: 3856
Define missing gray color tokens. The shadow token definitions reference {colors.gray.a4}, {colors.gray.a6}, {colors.gray.a7}, and {colors.gray.a8} which are not defined anywhere in the codebase. The theme/tokens/colors.ts file only exports black and white colors. Add a gray color definition to theme/tokens/colors.ts or update the shadow token references to use existing color tokens (e.g., {colors.black.*}).
Additionally, theme/colors/olive.ts also references undefined gray color tokens like {colors.gray.1}, {colors.gray.12}, {colors.gray.a2}, and others, suggesting a broader missing color definition.
🤖 Prompt for AI Agents
In `@theme/tokens/shadows.ts` around lines 3 - 46, The shadows use undefined gray
tokens (e.g., {colors.gray.a4}, {colors.gray.a6}, {colors.gray.a7},
{colors.gray.a8}) used by the defineSemanticTokens.shadows export; add a gray
color object to theme/tokens/colors.ts that defines the numeric keys (1..12 as
needed) and alpha keys (a2, a4, a6, a7, a8) referenced, or alternatively change
all shadow entries in shadows (xs, sm, md, lg, xl, 2xl, inset) to use existing
tokens (like colors.black.*); also update theme/colors/olive.ts to use the same
gray token names (or replace them with the new gray tokens) so all references
({colors.gray.1}, {colors.gray.12}, {colors.gray.a2}, etc.) resolve.
There was a problem hiding this comment.
@coderabbitai
gray is mapped in panda config:
orange: orange,
gray: olive,
red: red,
green: green,
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the gray → olive mapping in panda.config.ts
echo "=== Checking panda.config.ts for gray mapping ==="
rg -n -A 5 -B 5 "gray.*olive" panda.config.ts
Length of output: 235
@Guria, thank you for clarifying! You're absolutely right. I can confirm that Line 86 of panda.config.ts maps gray: olive, which means all the {colors.gray.*} references in the shadow tokens will correctly resolve to the corresponding olive tokens defined in theme/colors/olive.ts. Since olive defines a1 through a12, all the alpha values used in the shadows (a4, a6, a7, a8) are covered.
I apologize for the confusion in my initial review—I didn't account for the semantic token aliasing. The token references are correctly set up! 👍
| } | ||
| }, | ||
| "include": ["src", ".storybook/**/*"] | ||
| "include": ["src", ".storybook/**/*", "theme", "theme"] |
There was a problem hiding this comment.
Duplicate "theme" entry in the include array.
The "theme" value appears twice in the include array, which is redundant.
Proposed fix
- "include": ["src", ".storybook/**/*", "theme", "theme"]
+ "include": ["src", ".storybook/**/*", "theme"]🤖 Prompt for AI Agents
In `@tsconfig.app.json` at line 16, The tsconfig.app.json "include" array contains
a duplicate entry "theme"; edit the "include" array to remove the redundant
"theme" so each path appears only once (update the value for "include" in
tsconfig.app.json to have a single "theme" entry), ensuring the JSON remains
valid.
Guria
force-pushed
the
park-ui
branch
2 times, most recently
from
5c3ce28 to
acbd37b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/shared/components/ui/button.tsx`:
- Around line 4-26: The ButtonLoadingProps type references React.ReactNode but
React isn't imported; import the type ReactNode from 'react' (e.g., add "import
{ type ReactNode } from 'react'") and update the ButtonLoadingProps properties
(loadingText and spinner) to use ReactNode instead of React.ReactNode so
TypeScript compiles cleanly; locate the ButtonLoadingProps interface and the
declarations for loadingText and spinner to apply the change.
- Around line 45-53: The button component currently spreads disabled onto
whatever element asChild may render, which doesn't disable non-button elements;
update the render logic in the component that destructures buttonProps
(variables: loading, loadingText, children, spinner, spinnerPlacement, rest) and
the BaseButton usage to: always add aria-disabled={loading || rest.disabled} and
data-disabled={loading || rest.disabled}, set disabled={loading ||
rest.disabled} only when the underlying element is a native button (detect via a
prop like rest.as or a helper that checks HTMLElement type), and keep
data-loading as before; also add/update CSS rules targeting [data-disabled] to
block pointer-events and apply disabled styling for non-button elements.
♻️ Duplicate comments (1)
tsconfig.json (1)
3-8: Verify#*alias consistency across configs.
Roottsconfig.jsonmaps#*to./*. Iftsconfig.app.json,package.jsonimports, or Vite aliases map#*to./src/*, this can cause divergent module resolution. Please confirm they align and update if needed.#!/bin/bash # Show all alias mappings for #* across configs rg -n '"#\*"' -g 'tsconfig*.json' -g 'package.json' -g 'vite.config.*' -C2
🧹 Nitpick comments (1)
src/shared/components/ui/loader.tsx (1)
44-44: Ref is not forwarded whenvisibleis false.When
visibleisfalse, the component returnschildrendirectly without attaching theref. This means callers using the ref will receivenullwhen the loader is not visible, which could cause issues if they expect a consistent DOM reference.Consider wrapping the children in a span even when not visible, or document this behavior:
♻️ Option: Maintain ref consistency
- if (!visible) return children + if (!visible) { + return ( + <Span ref={ref} display="contents" {...rest}> + {children} + </Span> + ) + }
| import { type ComponentProps, forwardRef, useMemo } from 'react' | ||
|
|
||
| import { styled } from '#styled-system/jsx' | ||
| import { type ButtonVariantProps, button } from '#styled-system/recipes' | ||
|
|
||
| import { Group, type GroupProps } from './group' | ||
| import { Loader } from './loader' | ||
|
|
||
| interface ButtonLoadingProps { | ||
| /** | ||
| * If `true`, the button will show a loading spinner. | ||
| * @default false | ||
| */ | ||
| loading?: boolean | undefined | ||
| /** | ||
| * The text to show while loading. | ||
| */ | ||
| loadingText?: React.ReactNode | undefined | ||
| /** | ||
| * The spinner to show while loading. | ||
| */ | ||
| spinner?: React.ReactNode | undefined | ||
| /** |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/shared/components/ui/button.tsxRepository: Guria/modern-stack
Length of output: 2972
Import ReactNode type (React namespace is missing).
React.ReactNode is used on lines 21 and 25, but React is not imported as a namespace, causing a TypeScript compilation error. Import ReactNode directly instead.
🔧 Proposed fix
-import { type ComponentProps, forwardRef, useMemo } from 'react'
+import { type ComponentProps, type ReactNode, forwardRef, useMemo } from 'react'Then replace React.ReactNode with ReactNode on lines 21 and 25.
🤖 Prompt for AI Agents
In `@src/shared/components/ui/button.tsx` around lines 4 - 26, The
ButtonLoadingProps type references React.ReactNode but React isn't imported;
import the type ReactNode from 'react' (e.g., add "import { type ReactNode }
from 'react'") and update the ButtonLoadingProps properties (loadingText and
spinner) to use ReactNode instead of React.ReactNode so TypeScript compiles
cleanly; locate the ButtonLoadingProps interface and the declarations for
loadingText and spinner to apply the change.
| const { loading, loadingText, children, spinner, spinnerPlacement, ...rest } = buttonProps | ||
| return ( | ||
| <BaseButton | ||
| type="button" | ||
| ref={ref} | ||
| {...rest} | ||
| data-loading={loading ? '' : undefined} | ||
| disabled={loading || rest.disabled} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the button component file
cat -n src/shared/components/ui/button.tsx | head -80Repository: Guria/modern-stack
Length of output: 2896
🏁 Script executed:
# Search for Button usage with asChild to non-button elements
rg -n --type=tsx -C3 '<Button[^>]*\basChild\b'Repository: Guria/modern-stack
Length of output: 89
🏁 Script executed:
# Also search for asChild patterns more broadly
rg -n 'asChild' src/shared/components/ui/button.tsxRepository: Guria/modern-stack
Length of output: 98
🏁 Script executed:
# Search for Button usage with asChild
rg -n 'Button.*asChild'Repository: Guria/modern-stack
Length of output: 44
🏁 Script executed:
# Search for non-button elements using asChild pattern
rg -n 'asChild.*=.*{true|true.*asChild}' --type=tsxRepository: Guria/modern-stack
Length of output: 189
🏁 Script executed:
# Check if there are any click handlers or event handlers in button component
rg -n 'onClick|onPointerDown|onKeyDown' src/shared/components/ui/button.tsxRepository: Guria/modern-stack
Length of output: 44
🏁 Script executed:
# Check BaseButton and ark.button for any event handling or disabled logic
rg -n 'ark\.button|BaseButton' src/shared/components/ui/ -A 5 | head -50Repository: Guria/modern-stack
Length of output: 1531
🏁 Script executed:
# Check ButtonProps interface and what it extends
rg -n 'interface ButtonProps|interface BaseButtonProps' src/shared/components/ui/button.tsx -A 2Repository: Guria/modern-stack
Length of output: 242
🏁 Script executed:
# Check ComponentProps<typeof BaseButton> to see what properties are available
rg -n 'ComponentProps|ark\.button' --type=ts -A 3 | head -40Repository: Guria/modern-stack
Length of output: 2132
🏁 Script executed:
# Search for any Ark UI documentation or asChild references in the project
find . -name "*.md" -o -name "*.json" | xargs grep -l "asChild\|ark-ui" 2>/dev/null | head -5Repository: Guria/modern-stack
Length of output: 94
🏁 Script executed:
# Check Ark UI version in package.json
cat package.json | jq '.dependencies | .["@ark-ui/react"]'Repository: Guria/modern-stack
Length of output: 71
🏁 Script executed:
# Also check components.json
cat components.jsonRepository: Guria/modern-stack
Length of output: 383
Prevent clicks on non-button elements when using asChild with loading or disabled state.
When asChild renders a non-button element (e.g., <a>, <div>), the disabled attribute has no effect. Add aria-disabled, data-disabled attributes and conditionally apply disabled only to native buttons:
🔧 Suggested adjustment
const { loading, loadingText, children, spinner, spinnerPlacement, ...rest } = buttonProps
+const isDisabled = loading || rest.disabled
return (
<BaseButton
type="button"
ref={ref}
{...rest}
data-loading={loading ? '' : undefined}
- disabled={loading || rest.disabled}
+ data-disabled={isDisabled ? '' : undefined}
+ aria-disabled={isDisabled || undefined}
+ disabled={!rest.asChild ? isDisabled : undefined}
>Apply corresponding CSS for [data-disabled] to block pointer events and style the disabled state for non-button elements.
📝 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.
| const { loading, loadingText, children, spinner, spinnerPlacement, ...rest } = buttonProps | |
| return ( | |
| <BaseButton | |
| type="button" | |
| ref={ref} | |
| {...rest} | |
| data-loading={loading ? '' : undefined} | |
| disabled={loading || rest.disabled} | |
| > | |
| const { loading, loadingText, children, spinner, spinnerPlacement, ...rest } = buttonProps | |
| const isDisabled = loading || rest.disabled | |
| return ( | |
| <BaseButton | |
| type="button" | |
| ref={ref} | |
| {...rest} | |
| data-loading={loading ? '' : undefined} | |
| data-disabled={isDisabled ? '' : undefined} | |
| aria-disabled={isDisabled || undefined} | |
| disabled={!rest.asChild ? isDisabled : undefined} | |
| > |
🤖 Prompt for AI Agents
In `@src/shared/components/ui/button.tsx` around lines 45 - 53, The button
component currently spreads disabled onto whatever element asChild may render,
which doesn't disable non-button elements; update the render logic in the
component that destructures buttonProps (variables: loading, loadingText,
children, spinner, spinnerPlacement, rest) and the BaseButton usage to: always
add aria-disabled={loading || rest.disabled} and data-disabled={loading ||
rest.disabled}, set disabled={loading || rest.disabled} only when the underlying
element is a native button (detect via a prop like rest.as or a helper that
checks HTMLElement type), and keep data-loading as before; also add/update CSS
rules targeting [data-disabled] to block pointer-events and apply disabled
styling for non-button elements.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main.tsx`:
- Line 8: tsconfig.json currently has a generic "#*" → "./src/*" path mapping
which incorrectly resolves imports like "#styled-system/css"; update
tsconfig.json's compilerOptions.paths to add an explicit mapping for
"#styled-system/*" pointing to "./src/shared/styled-system/*" while keeping the
generic "#*" mapping, so imports such as "#styled-system/css" resolve the same
way as the package.json and panda.config.ts outputs.
In `@theme/colors/olive.ts`:
- Around line 28-69: The semantic groups in olive.ts (solid, subtle, surface,
outline, plain) incorrectly reference {colors.gray.*}; update those token
references to use the olive palette (replace occurrences like
'{colors.gray.12}', '{colors.gray.a3}', '{colors.gray.2}', '{colors.gray.3}',
'{colors.gray.6}', '{colors.gray.7}', '{colors.gray.a2}', '{colors.gray.a4}',
etc.) with the corresponding '{colors.olive.*}' tokens (e.g.,
'{colors.olive.12}', '{colors.olive.a3}', '{colors.olive.2}',
'{colors.olive.3}', '{colors.olive.6}', '{colors.olive.7}', '{colors.olive.a2}',
'{colors.olive.a4}') in the semantic groups (solid, subtle, surface, outline,
plain); leave the existing _light/_dark white/black values untouched.
♻️ Duplicate comments (2)
src/shared/components/ui/button.tsx (2)
21-25: ImportReactNodetype (React namespace is missing).
React.ReactNodeis used on lines 21 and 25, butReactis not imported as a namespace. This will cause a TypeScript compilation error.🔧 Proposed fix
-import { type ComponentProps, forwardRef, useMemo } from 'react' +import { type ComponentProps, type ReactNode, forwardRef, useMemo } from 'react'Then update the interface:
- loadingText?: React.ReactNode | undefined + loadingText?: ReactNode | undefined /** * The spinner to show while loading. */ - spinner?: React.ReactNode | undefined + spinner?: ReactNode | undefined
45-53: Disabled attribute doesn't work for non-button elements withasChild.When
asChildrenders a non-button element (e.g.,<a>,<div>), the nativedisabledattribute has no effect. Consider addingaria-disabledanddata-disabledfor accessibility and styling hooks.🔧 Suggested adjustment
const { loading, loadingText, children, spinner, spinnerPlacement, ...rest } = buttonProps +const isDisabled = loading || rest.disabled return ( <BaseButton type="button" ref={ref} {...rest} data-loading={loading ? '' : undefined} - disabled={loading || rest.disabled} + data-disabled={isDisabled ? '' : undefined} + aria-disabled={isDisabled || undefined} + disabled={!rest.asChild ? isDisabled : undefined} >
🧹 Nitpick comments (6)
tsconfig.node.json (1)
8-8: Minor:panda.config.tsis redundant with*.config.tsglob.The
*.config.tspattern already matchespanda.config.ts, making the explicit entry unnecessary. Consider removing it for cleaner configuration.Proposed fix
- "include": ["vite.config.ts", "*.config.ts", ".storybook/main.ts", "panda.config.ts", "theme"] + "include": ["vite.config.ts", "*.config.ts", ".storybook/main.ts", "theme"]panda.config.ts (3)
33-43: Consider using ES6 object shorthand for cleaner syntax.Multiple properties repeat the same identifier for both key and value. Using shorthand would reduce repetition and improve readability.
✨ Suggested refactor
extend: { - animationStyles: animationStyles, - recipes: recipes, - slotRecipes: slotRecipes, - keyframes: keyframes, - layerStyles: layerStyles, - textStyles: textStyles, + animationStyles, + recipes, + slotRecipes, + keyframes, + layerStyles, + textStyles, tokens: { - colors: colors, - durations: durations, - zIndex: zIndex, + colors, + durations, + zIndex, },
85-88: Clarify thegray: olivesemantic token alias.Assigning the
olivecolor scale to thegraysemantic token may confuse future maintainers who expect gray to reference actual gray tones. If this is intentional theming (e.g., using olive-tinted neutrals), consider adding a brief comment explaining the choice.+ // Using olive-tinted neutrals as the primary gray scale orange: orange, gray: olive,
113-114: Use ES6 shorthand here as well.Same shorthand opportunity applies to
globalCssandconditions.✨ Suggested refactor
- globalCss: globalCss, - conditions: conditions, + globalCss, + conditions,src/shared/components/ui/loader.tsx (1)
44-44: Ref is not forwarded whenvisibleis false.When
visibleis false, returningchildrendirectly means the forwardedrefis not attached to any element. This could surprise consumers who expectref.currentto always reference the Loader's DOM node.If this is intentional, consider documenting this behavior in the JSDoc. Otherwise, wrap the return:
♻️ Optional: maintain ref attachment when not visible
- if (!visible) return children + if (!visible) { + return ( + <Span ref={ref} display="contents" {...rest}> + {children} + </Span> + ) + }src/shared/components/ui/button.tsx (1)
54-60: Use mergedbuttonProps.asChildinstead ofprops.asChildfor consistency.The condition checks
props.asChilddirectly, butbuttonPropsis the merged result of context and props. WhileasChildis unlikely to come from context, using the merged value ensures consistent behavior.♻️ Suggested fix
- {!props.asChild && loading ? ( + {!rest.asChild && loading ? ( <Loader spinner={spinner} text={loadingText} spinnerPlacement={spinnerPlacement}> {children} </Loader>
Guria
force-pushed
the
park-ui
branch
3 times, most recently
from
65d0f27 to
39a0308
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 37 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| spinner?: React.ReactNode | undefined | ||
| /** | ||
| * The placement of the spinner | ||
| * @default "start" | ||
| */ | ||
| spinnerPlacement?: 'start' | 'end' | undefined | ||
| /** | ||
| * The text to display when loading | ||
| */ | ||
| text?: React.ReactNode | undefined |
There was a problem hiding this comment.
This file references React.ReactNode in the prop types but does not import the React namespace or the ReactNode type, which will cause TypeScript errors (Cannot find namespace 'React') under the current jsx and strict settings. Consider importing the appropriate types from react (for example import type { ReactNode } from 'react') and updating these annotations accordingly.
| loadingText?: React.ReactNode | undefined | ||
| /** | ||
| * The spinner to show while loading. | ||
| */ | ||
| spinner?: React.ReactNode | undefined |
There was a problem hiding this comment.
ButtonLoadingProps uses React.ReactNode for loadingText and spinner, but this file does not import the React namespace or the corresponding type, which will fail type-checking (Cannot find namespace 'React'). Please import the relevant type(s) from react and update these annotations so the file compiles cleanly.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.