refactor(button,card): apply new semantic design tokens#288
refactor(button,card): apply new semantic design tokens#288osohyun0224 wants to merge 6 commits into
Conversation
- Replace hardcoded colors (#FF7C27, gradient, #FE4E07, #000) with
vars.color.accent.{default,hover,subtle} and foreground.onAccent
- Replace primitive color.gray500/600 for disabled with
vars.color.background.muted / vars.color.foreground.subtle
(foreground.muted is reserved for non-readable text per token spec)
- Use vars.color.border.focus for focus-visible outline
- Standardize padding/borderRadius to vars.spacing.component.* and
vars.radius.component.md/lg across all sizes
- Split border shorthand into borderWidth/Style/Color to work around
vanilla-extract shorthand parsing bug with CSS custom properties
- Add opacity 0.9 to fill :active for hover/press visual hierarchy
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Replace primitive color.gray50/100/200 and color.cyan300 with
vars.color.background.{base,subtle}, vars.color.border.{default,strong}
- outline variant border uses border.strong instead of accent.default
to avoid conflating interactive color with divider semantics
- Replace hardcoded borderRadius 12px with vars.radius.component.lg (8px)
and padding 20px with vars.spacing.component.lg (16px) per token spec
("카드 모서리" / "카드 내부 패딩")
- Split border shorthand into individual props to work around
vanilla-extract CSS custom property shorthand parsing bug
- Rewrite tests to inspect stylesheet cssText for var(--side-...) refs
since happy-dom does not resolve CSS variables via getComputedStyle;
helper matches selectorText via comma-split for compound selectors
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5a22c16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR modernizes repository tooling and package publishing, adds package-policy and token-generation automation, updates theme contracts, and expands Accordion, Button, Card, Flex, and Tooltip behavior with corresponding stories and tests. ChangesRepository governance and automation
Component and theme behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 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 @.github/workflows/ci.yaml:
- Around line 18-25: Update the Lint, Test, Type check, and Build steps to pass
github.base_ref through each step’s environment block, then reference that
environment variable in the pnpm --filter expressions instead of directly
interpolating the GitHub context in run commands. Preserve the existing filters
and --if-present behavior.
In @.github/workflows/consistency.yaml:
- Around line 1-6: Define explicit read-only workflow permissions in the
top-level configuration of the “Package consistency” workflow, granting only the
repository contents access required by its jobs. Ensure no broader default token
permissions remain enabled.
In @.github/workflows/tokens.yaml:
- Line 96: Update the stefanzweifel/git-auto-commit-action step to reference a
specific verified commit SHA instead of the mutable `@v5` tag, preserving the
existing action configuration and workflow behavior.
- Line 43: Update the checkout commands in the workflow to pass github.base_ref
and github.event.pull_request.head.sha through step-level env variables, then
reference those variables in the shell commands. Apply this to both checkout
invocations while preserving the existing origin prefix and tokens path.
- Around line 18-25: Add a restrictive top-level permissions block in the
workflow setting the default to none, while preserving each job’s existing
explicit permissions. Add concise rationale comments beside validate’s contents:
write and the corresponding pull-request job’s pull-requests: write entries
describing why those write permissions are required.
- Around line 27-30: Update both actions/checkout@v4 steps in the tokens
workflow, including the checkout in the sync job, to set persist-credentials to
false alongside the existing checkout options. Preserve the current fetch-depth
and ref behavior.
- Line 27: Pin every referenced third-party action in the workflow—both
actions/checkout entries and jdx/mise-action—to immutable commit SHAs instead of
mutable version tags, preserving their current action versions and
configuration.
In @.husky/pre-push:
- Around line 4-6: Update the SPECIAL_REGEX definition to match exactly main and
any branch beginning with dev/, while no longer treating only the exact dev
branch as special. Preserve the existing main-branch exemption and anchor the
pattern so unrelated branch names remain subject to naming rules.
In `@AGENTS.md`:
- Around line 47-48: Insert a blank line between the “Variant enum 패턴 (값 객체 + 타입
유니온):” text and the following TypeScript fenced code block in AGENTS.md,
preserving the block contents.
In `@package.json`:
- Line 15: Update the serve:storybook script to invoke the project-installed
http-server, and add http-server at a pinned version under devDependencies so
local and CI runs use a reproducible dependency instead of unpinned npx
resolution.
In `@packages/accordion/src/Accordion.tsx`:
- Around line 58-68: Memoize the AccordionRoot context value used by
AccordionRootContext.Provider. Wrap onItemToggle in useCallback with its
relevant dependencies, then create the provider value with useMemo so consumers
receive a stable object when type and activeValue are unchanged.
In `@packages/accordion/src/context/AccordionRootContext.tsx`:
- Around line 11-13: Update useAccordionRootContext to check the
useContext(AccordionRootContext) result and throw a clear error when no provider
value is present; otherwise return the context unchanged.
In `@packages/button/src/Button.stories.tsx`:
- Around line 193-210: Add a theme showcase story alongside AllVariantsAndSizes
that wraps Button examples with ThemeProvider and renders the available theme
variants (1st through 4th), while preserving the existing variant and size
coverage.
In `@packages/button/src/Button.test.tsx`:
- Around line 86-112: Remove the duplicated test cases for the Button default
and submit types, retaining one test for each behavior in the Button test suite.
Keep the existing assertions that verify type="button" when omitted and
type="submit" when explicitly provided.
In `@packages/button/src/Button.tsx`:
- Around line 53-55: Mark the decorative icon wrapper spans in the Button render
output as aria-hidden="true" for both leftIcon and rightIcon, while leaving the
Slottable children and existing styling unchanged.
In `@packages/card/src/Card.test.tsx`:
- Line 29: Translate all Korean test descriptions in Card.test.tsx to the
specified English descriptions, including the cases covering children rendering,
default centering, variant defaults, variant-specific tokens, padding, and
radius. Change only the test names and preserve their assertions and test
behavior.
In `@packages/theme/package.json`:
- Around line 26-34: Update the package manifest’s dependency declarations by
adding react and react-dom to a peerDependencies block, while retaining their
devDependencies entries for local development and testing. Use the existing
catalog:react version references and keep the dependency names aligned across
both sections.
In `@packages/tokens/config.js`:
- Around line 113-136: Extract the duplicated grouping and TypeScript
union-emission logic from the primitive and semantic formatters into a shared
emitTokenNamesDts helper. Have the typescript/token-names-dts hook and
typescript/semantic-token-names-dts hook call it with their respective type-name
prefix, union type name, and semantic path filter, preserving the existing
generated output and filtering behavior.
- Around line 268-298: The fallback branch that writes semantic-dark.css must
also generate semantic.d.ts and semantic.d.cts stubs before the token-names
barrel is written. Update the else branch alongside its CSS fallback so the
existing index.d.ts re-exports of ./semantic and SemanticToken references
resolve successfully, using declarations consistent with the generated semantic
type module.
In `@packages/tokens/src/colors/colors.ts`:
- Around line 180-186: Update the text color in the theme5th definition so it
contrasts with the black background; keep background as `#000000` and use the
established light text color convention, such as white, for theme5th.text.
In `@packages/tooltip/src/Tooltip.css.ts`:
- Around line 39-125: Refactor the placement variants in the recipe to use the
relevant placement enum values instead of plain string keys, and introduce a
small helper or generator for the shared selectors['&::after'] structure. Pass
each placement’s unique offset, transform, border width, and border color values
through that helper, reusing the shared tooltip background-color variable rather
than duplicating it across all eight variants.
- Around line 9-38: Update the tooltip recipe’s base styles to import and use
the semantic `@sipe-team/tokens` vars for backgroundColor, color, padding,
borderRadius, fontSize, boxShadow, and zIndex instead of raw literals or
fallback custom properties. Match the token usage established by the migrated
Button and Card styles so Tooltip responds to ThemeProvider theme variants.
In `@packages/tooltip/src/Tooltip.tsx`:
- Around line 105-114: Update the trigger element props near wrapperRef so
aria-describedby preserves and merges any consumer-provided value from rest with
tooltipId when isVisible, rather than overriding it. Keep aria-describedby
undefined only when neither the user value nor the visible tooltip provides an
identifier.
In `@scripts/checkPackageConsistency.ts`:
- Line 12: Align the catalog dependency rule name across the violation flow:
update the reported violation near the isAllowlisted call in the
direct-dependency check to use the canonical name already used by
rulesRequiringFieldPath and allowlistEntrySchema, so isAllowlisted matches
allowlist entries consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eae8caf8-d53f-4435-8de1-ae8fbec9e698
⛔ Files ignored due to path filters (24)
.changeset/accordion-single-mode.mdis excluded by!.changeset/**.changeset/add-card-ghost-variant.mdis excluded by!.changeset/**.changeset/beige-cows-know.mdis excluded by!.changeset/**.changeset/button-v2-redesign.mdis excluded by!.changeset/**.changeset/chip-normalize-package-json.mdis excluded by!.changeset/**.changeset/package-json-divergence-cleanup.mdis excluded by!.changeset/**.changeset/provider-wonju.mdis excluded by!.changeset/**.changeset/semantic-tokens-foundation.mdis excluded by!.changeset/**.changeset/spicy-wasps-clap.mdis excluded by!.changeset/**.changeset/wet-pants-train.mdis excluded by!.changeset/**.templates/component/.storybook/main.tsis excluded by!.templates/**.templates/component/.storybook/preview.tsis excluded by!.templates/**.templates/component/global.d.tsis excluded by!.templates/**,!**/*.d.ts.templates/component/package.jsonis excluded by!.templates/**.templates/component/src/Component.stories.tsxis excluded by!.templates/**.templates/component/src/Component.tsxis excluded by!.templates/**.templates/component/src/Template.css.tsis excluded by!.templates/**.templates/component/src/Template.stories.tsxis excluded by!.templates/**.templates/component/src/Template.test.tsxis excluded by!.templates/**.templates/component/src/Template.tsxis excluded by!.templates/**.templates/component/src/index.tsis excluded by!.templates/**.templates/component/tsup.config.tsis excluded by!.templates/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yamltypes/index.d.tsis excluded by!**/*.d.ts
📒 Files selected for processing (147)
.claude/settings.json.github/CONTRIBUTING.md.github/workflows/ci.yaml.github/workflows/consistency.yaml.github/workflows/release.yaml.github/workflows/tokens.yaml.gitignore.husky/pre-push.mise.toml.storybook/main.ts.storybook/manager.ts.vscode/extensions.jsonAGENTS.mdCLAUDE.mdLICENSEREADME.mdcommitlint.config.tsdocs/introduction.mdxdocs/tokens.mdxdocs/tokens/semantic-proposal.mdpackage-policy.jsonpackage-policy.schema.jsonpackage.jsonpackages/accordion/.storybook/main.tspackages/accordion/.storybook/preview.tspackages/accordion/CHANGELOG.mdpackages/accordion/package.jsonpackages/accordion/src/Accordion.stories.tsxpackages/accordion/src/Accordion.test.tsxpackages/accordion/src/Accordion.tsxpackages/accordion/src/context/AccordionItemContext.tsxpackages/accordion/src/context/AccordionRootContext.tsxpackages/avatar/.storybook/main.tspackages/avatar/.storybook/preview.tspackages/avatar/CHANGELOG.mdpackages/avatar/package.jsonpackages/avatar/vite.config.tspackages/badge/.storybook/main.tspackages/badge/.storybook/preview.tspackages/badge/CHANGELOG.mdpackages/badge/package.jsonpackages/button/.storybook/main.tspackages/button/.storybook/preview.tspackages/button/CHANGELOG.mdpackages/button/package.jsonpackages/button/src/Button.css.tspackages/button/src/Button.stories.tsxpackages/button/src/Button.test.tsxpackages/button/src/Button.tsxpackages/card/.storybook/main.tspackages/card/.storybook/preview.tspackages/card/CHANGELOG.mdpackages/card/package.jsonpackages/card/src/Card.css.tspackages/card/src/Card.stories.tsxpackages/card/src/Card.test.tsxpackages/checkbox/.storybook/main.tspackages/checkbox/.storybook/preview.tspackages/checkbox/CHANGELOG.mdpackages/checkbox/package.jsonpackages/chip/package.jsonpackages/divider/.storybook/main.tspackages/divider/.storybook/preview.tspackages/divider/CHANGELOG.mdpackages/divider/package.jsonpackages/flex/.storybook/main.tspackages/flex/.storybook/preview.tspackages/flex/CHANGELOG.mdpackages/flex/package.jsonpackages/flex/src/Flex.stories.tsxpackages/flex/src/Flex.test.tsxpackages/grid/.storybook/main.tspackages/grid/.storybook/preview.tspackages/grid/CHANGELOG.mdpackages/grid/package.jsonpackages/icon/.storybook/main.tspackages/icon/.storybook/preview.tspackages/icon/package.jsonpackages/input/.storybook/main.tspackages/input/.storybook/preview.tspackages/input/CHANGELOG.mdpackages/input/package.jsonpackages/plugin-figma-codegen/manifest.jsonpackages/plugin-figma-codegen/package.jsonpackages/plugin-figma-codegen/src/index.tspackages/plugin-figma-codegen/tsconfig.jsonpackages/plugin-figma-codegen/tsup.config.tspackages/radio/.storybook/main.tspackages/radio/.storybook/preview.tspackages/radio/CHANGELOG.mdpackages/radio/package.jsonpackages/reset/CHANGELOG.mdpackages/reset/package.jsonpackages/side/CHANGELOG.mdpackages/side/package.jsonpackages/skeleton/.storybook/main.tspackages/skeleton/.storybook/preview.tspackages/skeleton/CHANGELOG.mdpackages/skeleton/package.jsonpackages/switch/.storybook/main.tspackages/switch/.storybook/preview.tspackages/switch/CHANGELOG.mdpackages/switch/package.jsonpackages/theme/.storybook/main.tspackages/theme/.storybook/preview.tspackages/theme/package.jsonpackages/theme/src/ThemeProvider.stories.tsxpackages/theme/src/ThemeProvider.test.tsxpackages/theme/src/ThemeProvider.tsxpackages/tokens/.storybook/main.tspackages/tokens/.storybook/preview.tspackages/tokens/config.jspackages/tokens/package.jsonpackages/tokens/src/colors/colors.tspackages/tokens/src/colors/index.tspackages/tokens/src/effects/border.tspackages/tokens/src/effects/opacity.tspackages/tokens/src/effects/shadows.tspackages/tokens/src/effects/zIndex.tspackages/tokens/src/layout/breakpoints.tspackages/tokens/src/layout/grid.tspackages/tokens/src/layout/responsiveStyle.tspackages/tokens/src/layout/spacing.tspackages/tokens/src/theme/contract.css.tspackages/tokens/src/theme/themes.css.tspackages/tokens/src/theme/utils.test.tspackages/tokens/src/theme/utils.tspackages/tokens/tsconfig.jsonpackages/tooltip/.storybook/main.tspackages/tooltip/.storybook/preview.tspackages/tooltip/CHANGELOG.mdpackages/tooltip/package.jsonpackages/tooltip/src/Tooltip.css.tspackages/tooltip/src/Tooltip.stories.tsxpackages/tooltip/src/Tooltip.test.tsxpackages/tooltip/src/Tooltip.tsxpackages/tooltip/src/hooks/useTooltip/useTooltip.tspackages/tooltip/src/hooks/useTooltip/useTooltip.tsxpackages/typography/.storybook/main.tspackages/typography/.storybook/preview.tspackages/typography/CHANGELOG.mdpackages/typography/package.jsonpnpm-workspace.yamlscripts/checkPackageConsistency.tsscripts/createComponent.tstokens/tokens.jsontsconfig.json
💤 Files with no reviewable changes (44)
- packages/checkbox/.storybook/preview.ts
- packages/checkbox/.storybook/main.ts
- packages/card/.storybook/preview.ts
- packages/divider/.storybook/main.ts
- packages/divider/.storybook/preview.ts
- packages/flex/.storybook/preview.ts
- packages/tokens/.storybook/preview.ts
- packages/tokens/.storybook/main.ts
- packages/avatar/vite.config.ts
- packages/skeleton/.storybook/main.ts
- packages/tooltip/.storybook/preview.ts
- packages/plugin-figma-codegen/tsup.config.ts
- packages/input/.storybook/main.ts
- packages/icon/.storybook/main.ts
- packages/plugin-figma-codegen/manifest.json
- packages/theme/.storybook/main.ts
- packages/button/.storybook/main.ts
- packages/typography/.storybook/preview.ts
- packages/theme/.storybook/preview.ts
- packages/radio/.storybook/preview.ts
- packages/icon/.storybook/preview.ts
- packages/skeleton/.storybook/preview.ts
- packages/plugin-figma-codegen/tsconfig.json
- packages/avatar/.storybook/main.ts
- packages/plugin-figma-codegen/src/index.ts
- packages/switch/.storybook/preview.ts
- packages/badge/.storybook/preview.ts
- packages/card/.storybook/main.ts
- packages/accordion/.storybook/preview.ts
- packages/input/.storybook/preview.ts
- packages/grid/.storybook/preview.ts
- packages/accordion/.storybook/main.ts
- packages/tooltip/.storybook/main.ts
- packages/tooltip/src/hooks/useTooltip/useTooltip.tsx
- packages/flex/.storybook/main.ts
- packages/plugin-figma-codegen/package.json
- packages/button/.storybook/preview.ts
- packages/switch/.storybook/main.ts
- packages/badge/.storybook/main.ts
- packages/avatar/.storybook/preview.ts
- packages/radio/.storybook/main.ts
- packages/theme/src/ThemeProvider.stories.tsx
- packages/typography/.storybook/main.ts
- packages/grid/.storybook/main.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (24)
.github/workflows/ci.yaml (1)
18-25: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Mitigate command injection vulnerability in CI scripts.
Directly interpolating
${{ github.base_ref }}into the inlinerunscripts exposes the workflow to command injection if a maliciously crafted branch name containing shell metacharacters is used. To mitigate this risk, pass the GitHub context variable securely via an environment variable block.🔒️ Proposed fix to securely pass the variable
- name: Lint - run: pnpm --filter="...[origin/${{ github.base_ref }}]" --if-present lint + env: + BASE_REF: ${{ github.base_ref }} + run: pnpm --filter="...[origin/$BASE_REF]" --if-present lint - name: Test - run: pnpm --filter="...[origin/${{ github.base_ref }}]" --if-present test + env: + BASE_REF: ${{ github.base_ref }} + run: pnpm --filter="...[origin/$BASE_REF]" --if-present test - name: Type check - run: pnpm --filter="...[origin/${{ github.base_ref }}]" --if-present typecheck + env: + BASE_REF: ${{ github.base_ref }} + run: pnpm --filter="...[origin/$BASE_REF]" --if-present typecheck - name: Build - run: pnpm --filter="...[origin/${{ github.base_ref }}]" --if-present build + env: + BASE_REF: ${{ github.base_ref }} + run: pnpm --filter="...[origin/$BASE_REF]" --if-present build📝 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.- name: Lint env: BASE_REF: ${{ github.base_ref }} run: pnpm --filter="...[origin/$BASE_REF]" --if-present lint - name: Test env: BASE_REF: ${{ github.base_ref }} run: pnpm --filter="...[origin/$BASE_REF]" --if-present test - name: Type check env: BASE_REF: ${{ github.base_ref }} run: pnpm --filter="...[origin/$BASE_REF]" --if-present typecheck - name: Build env: BASE_REF: ${{ github.base_ref }} run: pnpm --filter="...[origin/$BASE_REF]" --if-present build🧰 Tools
🪛 zizmor (1.26.1)
[error] 19-19: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 21-21: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 23-23: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 25-25: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/ci.yaml around lines 18 - 25, Update the Lint, Test, Type check, and Build steps to pass github.base_ref through each step’s environment block, then reference that environment variable in the pnpm --filter expressions instead of directly interpolating the GitHub context in run commands. Preserve the existing filters and --if-present behavior.Source: Linters/SAST tools
.github/workflows/consistency.yaml (1)
1-6: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Define explicit workflow permissions.
The workflow currently relies on default token permissions, which can be overly broad. It is a security best practice to explicitly define the minimum required permissions (e.g., read-only access to repository contents) to reduce the attack surface.
🔒️ Proposed fix to restrict permissions
name: Package consistency on: pull_request: +permissions: + contents: read + jobs: consistency:📝 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.name: Package consistency on: pull_request: permissions: contents: read jobs:🧰 Tools
🪛 zizmor (1.26.1)
[warning] 1-28: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 3-4: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 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 @.github/workflows/consistency.yaml around lines 1 - 6, Define explicit read-only workflow permissions in the top-level configuration of the “Package consistency” workflow, granting only the repository contents access required by its jobs. Ensure no broader default token permissions remain enabled.Source: Linters/SAST tools
.github/workflows/tokens.yaml (5)
18-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Consider a restrictive top-level
permissions:block plus rationale comments.Both jobs already scope permissions narrowly, but there's no top-level
permissions: {}default, so any future job added without an explicit block would inherit the (potentially broader) repository default. Static analysis also suggests a short comment oncontents: write/pull-requests: writeexplaining why each is needed.🔐 Proposed fix
+permissions: {} + jobs: validate: name: Build & validate runs-on: ubuntu-latest permissions: + contents: write # required to commit transformed tokens back via git-auto-commit-actionAlso applies to: 102-109
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 24-24: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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 @.github/workflows/tokens.yaml around lines 18 - 25, Add a restrictive top-level permissions block in the workflow setting the default to none, while preserving each job’s existing explicit permissions. Add concise rationale comments beside validate’s contents: write and the corresponding pull-request job’s pull-requests: write entries describing why those write permissions are required.Source: Linters/SAST tools
27-27: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin third-party/marketplace actions to a commit SHA.
actions/checkout@v4,jdx/mise-action@v2, and the secondactions/checkout@v4are referenced by mutable tag, not a pinned commit hash. Static analysis flags this as a blanket-policy violation (supply-chain hardening: tags can be moved to point at malicious code).🔒 Proposed fix: pin actions to a commit SHA
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 # v4.x.x + # pin to a specific commit SHA, e.g.: + # uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683Apply the same pinning to
jdx/mise-action@v2(line 32) and the secondactions/checkout@v4(line 112).Also applies to: 32-32, 59-59, 112-112
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 27-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/tokens.yaml at line 27, Pin every referenced third-party action in the workflow—both actions/checkout entries and jdx/mise-action—to immutable commit SHAs instead of mutable version tags, preserving their current action versions and configuration.Source: Linters/SAST tools
27-30: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add
persist-credentials: falseto checkout steps.Both
actions/checkout@v4steps persist theGITHUB_TOKENin the local git config by default, making it available to any subsequent step (including third-party actions run later in the job). Since neither checkout step needs to push using the persisted credential (thesyncjob authenticates separately viaGH_TOKEN, andgit-auto-commit-actionsupplies its own token handling), disabling persistence limits the credential's exposure window.🔒 Proposed fix
- uses: actions/checkout@v4 with: fetch-depth: 0 ref: ${{ github.event.pull_request.head.sha || github.sha }} + persist-credentials: falseApply the same to the
syncjob's checkout (lines 112-115).Also applies to: 112-115
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 27-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/tokens.yaml around lines 27 - 30, Update both actions/checkout@v4 steps in the tokens workflow, including the checkout in the sync job, to set persist-credentials to false alongside the existing checkout options. Preserve the current fetch-depth and ref behavior.Source: Linters/SAST tools
43-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid interpolating
github.*context values directly intorun:shell scripts.
git checkout origin/${{ github.base_ref }} -- tokens(line 43) andgit checkout ${{ github.event.pull_request.head.sha }} -- tokens(line 50) splice GitHub Actions context values directly into the shell script text before it runs — a template-injection vector flagged by zizmor.head.shais a fixed-format commit SHA (lower risk), butbase_refis a branch name and git ref names permit several shell metacharacters, so the safer pattern is to pass these throughenv:and reference them as$VARso the shell treats them as data, not code.🛡️ Proposed fix: use env vars instead of inline template expansion
- name: Build base tokens id: base-build if: github.event_name == 'pull_request' continue-on-error: true + env: + BASE_REF: ${{ github.base_ref }} run: | - git checkout origin/${{ github.base_ref }} -- tokens + git checkout "origin/$BASE_REF" -- tokens pnpm --filter `@sipe-team/tokens` build:tokens mkdir -p /tmp/base-css cp packages/tokens/dist/css/*.css /tmp/base-css/ - name: Restore head tokens if: github.event_name == 'pull_request' - run: git checkout ${{ github.event.pull_request.head.sha }} -- tokens + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: git checkout "$HEAD_SHA" -- tokensAlso applies to: 50-50
🧰 Tools
🪛 zizmor (1.26.1)
[error] 43-43: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/tokens.yaml at line 43, Update the checkout commands in the workflow to pass github.base_ref and github.event.pull_request.head.sha through step-level env variables, then reference those variables in the shell commands. Apply this to both checkout invocations while preserving the existing origin prefix and tokens path.Source: Linters/SAST tools
96-96: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin
stefanzweifel/git-auto-commit-action@v5to a commit SHA.Same unpinned-action concern as the other actions in this workflow, flagged separately since it also writes to the repository (higher-impact if the action were compromised).
🧰 Tools
🪛 zizmor (1.26.1)
[error] 96-96: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 96-96: action functionality is already included by the runner (superfluous-actions): use
git add,git commit, andgit pushin a script step(superfluous-actions)
🤖 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 @.github/workflows/tokens.yaml at line 96, Update the stefanzweifel/git-auto-commit-action step to reference a specific verified commit SHA instead of the mutable `@v5` tag, preserving the existing action configuration and workflow behavior.Source: Linters/SAST tools
.husky/pre-push (1)
4-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update
SPECIAL_REGEXto supportdev/*branches.The current regex only matches exactly
mainanddev. It will incorrectly block pushes for any sub-branches underdev/. Based on learnings, special branchesmainanddev/*are exempt from this rule.🛠️ Proposed fix
# Special branches that skip naming rules -SPECIAL_REGEX='^(main|dev)$' +SPECIAL_REGEX='^(main|dev(/.*)?)$'📝 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.# Special branches that skip naming rules SPECIAL_REGEX='^(main|dev(/.*)?)$'🤖 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 @.husky/pre-push around lines 4 - 6, Update the SPECIAL_REGEX definition to match exactly main and any branch beginning with dev/, while no longer treating only the exact dev branch as special. Preserve the existing main-branch exemption and anchor the pattern so unrelated branch names remain subject to naming rules.Source: Learnings
AGENTS.md (1)
47-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line before the fenced code block (MD031).
The
```tsfence at line 48 immediately follows the preceding text line with no blank line separator.📝 Proposed fix
Variant enum 패턴 (값 객체 + 타입 유니온): + ```ts export const ButtonSize = { sm: 'sm', lg: 'lg' } as const; export type ButtonSize = (typeof ButtonSize)[keyof typeof ButtonSize];</details> <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.23.0)</summary> [warning] 48-48: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) </details> </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@AGENTS.mdaround lines 47 - 48, Insert a blank line between the “Variant
enum 패턴 (값 객체 + 타입 유니온):” text and the following TypeScript fenced code block in
AGENTS.md, preserving the block contents.</details> <!-- fingerprinting:phantom:medusa:beignet --> <!-- cr-indicator-types:potential_issue --> <!-- cr-comment:v1:6797cf0b1eea2629335d6c7f --> _Source: Linters/SAST tools_ <!-- This is an auto-generated comment by CodeRabbit --> </blockquote></details> <details> <summary>package.json (1)</summary><blockquote> 15-15: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Pin `http-server` as a devDependency instead of using unpinned `npx`.** `npx http-server` resolves to whatever is latest at run time, which can silently change behavior/output between runs and isn't reproducible in CI. <details> <summary>♻️ Proposed fix</summary> ```diff - "serve:storybook": "npx http-server storybook-static -p 6006", + "serve:storybook": "http-server storybook-static -p 6006",Add
http-serverwith a pinned version underdevDependencies.🤖 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` at line 15, Update the serve:storybook script to invoke the project-installed http-server, and add http-server at a pinned version under devDependencies so local and CI runs use a reproducible dependency instead of unpinned npx resolution.packages/accordion/src/Accordion.tsx (1)
58-68: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Memoize the root context value.
<AccordionRootContext.Provider value={{ type, activeValue, onItemToggle }}>creates a new object (andonItemToggleisn't wrapped inuseCallback) on every render ofAccordionRoot, forcing every consumingAccordionItemto re-render regardless of whether anything relevant changed.ThemeProvider.tsxin this same PR already usesuseMemofor its context value — follow the same pattern here for consistency and to avoid the re-render cascade.♻️ Proposed fix
- const onItemToggle = (itemValue: string) => { + const onItemToggle = useCallback((itemValue: string) => { const newValue = activeValue === itemValue ? null : itemValue; if (value === undefined) { setInternalValue(newValue); } onValueChange?.(newValue); - }; + }, [activeValue, value, onValueChange]); + + const contextValue = useMemo(() => ({ type, activeValue, onItemToggle }), [type, activeValue, onItemToggle]); const Component = asChild ? Slot : 'div'; return ( - <AccordionRootContext.Provider value={{ type, activeValue, onItemToggle }}> + <AccordionRootContext.Provider value={contextValue}>📝 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 onItemToggle = useCallback((itemValue: string) => { const newValue = activeValue === itemValue ? null : itemValue; if (value === undefined) { setInternalValue(newValue); } onValueChange?.(newValue); }, [activeValue, value, onValueChange]); const contextValue = useMemo(() => ({ type, activeValue, onItemToggle }), [type, activeValue, onItemToggle]); const Component = asChild ? Slot : 'div'; return ( <AccordionRootContext.Provider value={contextValue}>🤖 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 `@packages/accordion/src/Accordion.tsx` around lines 58 - 68, Memoize the AccordionRoot context value used by AccordionRootContext.Provider. Wrap onItemToggle in useCallback with its relevant dependencies, then create the provider value with useMemo so consumers receive a stable object when type and activeValue are unchanged.packages/accordion/src/context/AccordionRootContext.tsx (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a null guard to improve error predictability.
When
useAccordionRootContextis called outside ofAccordionRootContext.Provider, it evaluates tonull. Throwing an explicit error inside the hook helps developers quickly identify missing providers rather than deciphering cryptic downstream destructuring errors.♻️ Proposed refactor to add a guard
export const useAccordionRootContext = () => { - return useContext(AccordionRootContext); + const context = useContext(AccordionRootContext); + if (!context) { + throw new Error('useAccordionRootContext must be used within an AccordionRoot'); + } + return context; };📝 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.export const useAccordionRootContext = () => { const context = useContext(AccordionRootContext); if (!context) { throw new Error('useAccordionRootContext must be used within an AccordionRoot'); } return context; };🤖 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 `@packages/accordion/src/context/AccordionRootContext.tsx` around lines 11 - 13, Update useAccordionRootContext to check the useContext(AccordionRootContext) result and throw a clear error when no provider value is present; otherwise return the context unchanged.packages/button/src/Button.stories.tsx (1)
193-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider adding a theme showcase story.
As per path instructions for story files, it is recommended to demonstrate components with different theme variants (1st-4th) and to integrate the
ThemeProviderfor theme-dependent components. Consider adding a new story that explicitly wraps the button in different available theme configurations to showcase the newly integrated semantic tokens.🤖 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 `@packages/button/src/Button.stories.tsx` around lines 193 - 210, Add a theme showcase story alongside AllVariantsAndSizes that wraps Button examples with ThemeProvider and renders the available theme variants (1st through 4th), while preserving the existing variant and size coverage.Source: Path instructions
packages/button/src/Button.test.tsx (1)
86-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove duplicated tests.
The tests checking for default
type="button"andtype="submit"are duplicated. Lines 86-94 verify the exact same behavior as lines 96-112. Consider removing the duplicate block to keep the test suite clean and maintainable.♻️ Proposed fix
test('type="button" is used as default when type is not provided', () => { render(<Button>Test</Button>); - expect(screen.getByRole('button')).toHaveAttribute('type', 'button'); -}); - -test('type="submit" works correctly', () => { - render(<Button type="submit">Submit</Button>); - expect(screen.getByRole('button')).toHaveAttribute('type', 'submit'); -}); - -test('type="button" is used as default when type is not provided', () => { - render(<Button>Test</Button>); const button = screen.getByRole('button');🤖 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 `@packages/button/src/Button.test.tsx` around lines 86 - 112, Remove the duplicated test cases for the Button default and submit types, retaining one test for each behavior in the Button test suite. Keep the existing assertions that verify type="button" when omitted and type="submit" when explicitly provided.packages/button/src/Button.tsx (1)
53-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider marking decorative icon wrappers as
aria-hidden.
leftIcon/rightIconare visual embellishments alongside the text label; withoutaria-hidden="true"on the wrapping<span>, screen readers may announce icon content redundantly (e.g. if the icon component renders any text/SVG title).♻️ Proposed fix
- {leftIcon && <span className={styles.iconWrapper}>{leftIcon}</span>} + {leftIcon && ( + <span className={styles.iconWrapper} aria-hidden="true"> + {leftIcon} + </span> + )} <Slottable>{children}</Slottable> - {rightIcon && <span className={styles.iconWrapper}>{rightIcon}</span>} + {rightIcon && ( + <span className={styles.iconWrapper} aria-hidden="true"> + {rightIcon} + </span> + )}📝 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.{leftIcon && ( <span className={styles.iconWrapper} aria-hidden="true"> {leftIcon} </span> )} <Slottable>{children}</Slottable> {rightIcon && ( <span className={styles.iconWrapper} aria-hidden="true"> {rightIcon} </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 `@packages/button/src/Button.tsx` around lines 53 - 55, Mark the decorative icon wrapper spans in the Button render output as aria-hidden="true" for both leftIcon and rightIcon, while leaving the Slottable children and existing styling unchanged.packages/card/src/Card.test.tsx (1)
29-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Translate test descriptions to English.
As per path instructions for test files, Korean test descriptions must be converted to English. Please update the test descriptions across these sections.
📝 Proposed translations
test('children으로 넘어간 요소를 반환한다.', ...)→test('returns the element passed as children', ...)test('Card는 default로 요소를 중앙정렬한다.', ...)→test('Card centers elements by default', ...)test('variant는 default로 filled이며 subtle 배경과 default 보더 semantic 토큰을 참조한다.', ...)→test('default variant is filled and references subtle background and default border semantic tokens', ...)test('variant가 outline이면 base 배경과 border.strong semantic 토큰을 참조한다.', ...)→test('outline variant references base background and border.strong semantic tokens', ...)test('variant가 filled일 때 subtle 배경 토큰을 사용한다.', ...)→test('filled variant uses subtle background token', ...)test('variant가 outline일 때 base 배경 토큰을 사용한다.', ...)→test('outline variant uses base background token', ...)test('Card는 semantic 컴포넌트 패딩 토큰을 적용한다.', ...)→test('Card applies semantic component padding token', ...)test('Card는 semantic 컴포넌트 반경 토큰을 적용한다.', ...)→test('Card applies semantic component radius token', ...)Also applies to: 39-39, 73-125
🤖 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 `@packages/card/src/Card.test.tsx` at line 29, Translate all Korean test descriptions in Card.test.tsx to the specified English descriptions, including the cases covering children rendering, default centering, variant defaults, variant-specific tokens, padding, and radius. Change only the test names and preserve their assertions and test behavior.Source: Path instructions
packages/theme/package.json (1)
26-34: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd package.json packages/theme --exec cat -n {}Repository: sipe-team/side
Length of output: 1875
🏁 Script executed:
fd package.json packages -x sh -c 'printf "\n### %s\n" "$1"; cat -n "$1" | sed -n "1,120p"' sh {}Repository: sipe-team/side
Length of output: 43188
Declare
reactandreact-domas peer dependencies —packages/theme/package.jsononly lists them indevDependencies; add apeerDependenciesblock so consumers resolve a single React copy.🤖 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 `@packages/theme/package.json` around lines 26 - 34, Update the package manifest’s dependency declarations by adding react and react-dom to a peerDependencies block, while retaining their devDependencies entries for local development and testing. Use the existing catalog:react version references and keep the dependency names aligned across both sections.Source: Path instructions
packages/tokens/config.js (2)
113-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate dts-generation logic between primitive and semantic formatters.
typescript/token-names-dtsandtypescript/semantic-token-names-dtsimplement nearly identical grouping/type-emission logic, differing only in the type-name prefix and an added path filter. Extracting a shared helper would reduce duplication and prevent the two from silently diverging over time.♻️ Proposed refactor: shared dts-emission helper
function emitTokenNamesDts({ dictionary, typeNamePrefix, unionTypeName, pathFilter }) { const groups = new Map(); for (const token of dictionary.allTokens) { if (pathFilter && !pathFilter(token)) continue; const category = token.path[0]; if (!groups.has(category)) groups.set(category, []); groups.get(category).push(token.name); } const lines = ['/** Auto-generated — do not edit directly. */\n']; const typeNames = []; for (const [category, names] of groups) { const typeName = `${typeNamePrefix}${toPascalCase(category)}Token`; typeNames.push(typeName); lines.push(`export type ${typeName} =`); lines.push(`${names.map((n) => ` | '${n}'`).join('\n')};\n`); } lines.push(`export type ${unionTypeName} =`); lines.push(`${typeNames.map((t) => ` | ${t}`).join('\n')};\n`); return lines.join('\n'); }Then each hook becomes a one-liner calling
emitTokenNamesDts({ dictionary, typeNamePrefix: '', unionTypeName: 'PrimitiveToken' })/emitTokenNamesDts({ dictionary, typeNamePrefix: 'Semantic', unionTypeName: 'SemanticToken', pathFilter: (t) => semanticOnlyPaths.has(t.path.join('.')) }).Also applies to: 209-233
🤖 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 `@packages/tokens/config.js` around lines 113 - 136, Extract the duplicated grouping and TypeScript union-emission logic from the primitive and semantic formatters into a shared emitTokenNamesDts helper. Have the typescript/token-names-dts hook and typescript/semantic-token-names-dts hook call it with their respective type-name prefix, union type name, and semantic path filter, preserving the existing generated output and filtering behavior.
268-298: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and nearby symbols. ast-grep outline packages/tokens/config.js --view expanded || true # Read the relevant section with line numbers. sed -n '180,330p' packages/tokens/config.js | cat -n # Find any semantic declaration outputs or references in the package. rg -n "semantic\.d\.(ts|cts)|export \* from './semantic'|SemanticToken" packages/tokens -S || true # Show the token-names-related source files mentioned in the comment. sed -n '1,260p' packages/tokens/config.js | rg -n "token-names-dts|semantic-token-names-dts|format" -n -C 2 || trueRepository: sipe-team/side
Length of output: 8321
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the exact branch around the fallback and barrel generation. sed -n '240,320p' packages/tokens/config.js | cat -n # Check whether semantic declaration files are generated anywhere else. rg -n "semantic\.d\.ts|semantic\.d\.cts|semantic\.js|semantic\.cjs" packages/tokens -S || true # Look for package export mappings that depend on token-names entrypoints. rg -n "token-names|semantic" packages/tokens/package.json packages/tokens/* -S || trueRepository: sipe-team/side
Length of output: 6613
Fallback branch should also write semantic type stubs
index.d.tsalways re-exports./semanticand referencesSemanticToken, so theelsebranch needs to emitsemantic.d.ts/semantic.d.ctstoo. Add stub declarations there to avoid breaking downstream type resolution.🤖 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 `@packages/tokens/config.js` around lines 268 - 298, The fallback branch that writes semantic-dark.css must also generate semantic.d.ts and semantic.d.cts stubs before the token-names barrel is written. Update the else branch alongside its CSS fallback so the existing index.d.ts re-exports of ./semantic and SemanticToken references resolve successfully, using declarations consistent with the generated semantic type module.packages/tokens/src/colors/colors.ts (1)
180-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
theme5th.textandtheme5th.backgroundare identical (#000000), making text invisible.Any component using this theme with default text-on-background styling will render unreadable content (zero contrast). This looks like a copy-paste value error;
textshould likely be a light color (e.g., white) to contrast the black background.🐛 Proposed fix
export const theme5th: ThemeColor = { primary: '`#FF7C27`', secondary: '`#FE4E07`', background: '`#000000`', - text: '`#000000`', + text: '`#FFFFFF`', gradient: 'linear-gradient(225deg, `#FF4500` 0%, `#FFB24D` 100%)', };📝 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.export const theme5th: ThemeColor = { primary: '`#FF7C27`', secondary: '`#FE4E07`', background: '`#000000`', text: '`#FFFFFF`', gradient: 'linear-gradient(225deg, `#FF4500` 0%, `#FFB24D` 100%)', };🤖 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 `@packages/tokens/src/colors/colors.ts` around lines 180 - 186, Update the text color in the theme5th definition so it contrasts with the black background; keep background as `#000000` and use the established light text color convention, such as white, for theme5th.text.packages/tooltip/src/Tooltip.css.ts (2)
9-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tooltip styles hardcode colors, spacing, radius, shadow and z-index instead of using
@sipe-team/tokens.
backgroundColor: 'var(--tooltip-bg-color,#000000)',color: '#ffffff',padding,borderRadius,fontSize,boxShadow, andzIndex: 1000are all raw literals with novarsimport. This PR just migrated Button and Card to semantic tokens for these exact same properties, so Tooltip is now the odd one out and won't respond toThemeProvidertheme switches (dark/light or 1st–4th theme variants) the way Button/Card now do.🎨 Proposed fix: import and use design tokens
-import { keyframes, style } from '`@vanilla-extract/css`'; +import { keyframes } from '`@vanilla-extract/css`'; import { recipe } from '`@vanilla-extract/recipes`'; +import { vars } from '`@sipe-team/tokens`'; ... base: { position: 'fixed', - backgroundColor: 'var(--tooltip-bg-color, `#000000`)', - color: '`#ffffff`', - padding: '8px 12px', - borderRadius: '8px', - fontSize: '12px', + backgroundColor: `var(--tooltip-bg-color, ${vars.color.background.inverse})`, + color: vars.color.foreground.inverse, + padding: `${vars.spacing.sm} ${vars.spacing.md}`, + borderRadius: vars.radius.md, + fontSize: vars.typography.fontSize.xs, lineHeight: 1.5, whiteSpace: 'normal', wordWrap: 'break-word', maxWidth: '250px', - boxShadow: '0 4px 8px rgba(0, 0, 0, 0.2)', - zIndex: 1000, + boxShadow: vars.shadow.md, + zIndex: vars.zIndex.tooltip,(Adjust token names to match the actual
@sipe-team/tokenscontract.)📝 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.export const tooltip = recipe({ base: { position: 'fixed', backgroundColor: `var(--tooltip-bg-color, ${vars.color.background.inverse})`, color: vars.color.foreground.inverse, padding: `${vars.spacing.sm} ${vars.spacing.md}`, borderRadius: vars.radius.md, fontSize: vars.typography.fontSize.xs, lineHeight: 1.5, whiteSpace: 'normal', wordWrap: 'break-word', maxWidth: '250px', boxShadow: vars.shadow.md, zIndex: vars.zIndex.tooltip, animation: `${fadeIn} 0.15s ease`, '`@media`': { '(prefers-reduced-motion: reduce)': { animation: 'none', }, }, selectors: { '&::after': { content: '""', position: 'absolute', width: 0, height: 0, borderStyle: 'solid', }, }, },🤖 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 `@packages/tooltip/src/Tooltip.css.ts` around lines 9 - 38, Update the tooltip recipe’s base styles to import and use the semantic `@sipe-team/tokens` vars for backgroundColor, color, padding, borderRadius, fontSize, boxShadow, and zIndex instead of raw literals or fallback custom properties. Match the token usage established by the migrated Button and Card styles so Tooltip responds to ThemeProvider theme variants.Source: Path instructions
39-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Placement variants use non-enum string keys and duplicate near-identical blocks 8 times.
Per the repo's coding guideline for
*.css.ts, recipe variant keys should derive from enum values; here they're plain string literals ('top-left','top', …). Each of the 8 variants also repeats the sameselectors['&::after']shape, differing only in offset/border values (including the hardcodedvar(--tooltip-bg-color,#000000)from the base-styles finding above, repeated 8x) — a good candidate for a small generator/helper to reduce duplication.As per coding guidelines, "Vanilla Extract의 recipe()로 variant를 처리하고, variant 키에는 enum 값을 사용하세요."
🤖 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 `@packages/tooltip/src/Tooltip.css.ts` around lines 39 - 125, Refactor the placement variants in the recipe to use the relevant placement enum values instead of plain string keys, and introduce a small helper or generator for the shared selectors['&::after'] structure. Pass each placement’s unique offset, transform, border width, and border color values through that helper, reusing the shared tooltip background-color variable rather than duplicating it across all eight variants.Source: Coding guidelines
packages/tooltip/src/Tooltip.tsx (1)
105-114: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
aria-describedbyoverrides a user-supplied value from...rest.Since
aria-describedbyis set after spreading...rest, anyaria-describedbya consumer passes in is silently discarded.♻️ Proposed fix to merge rather than override
<Component {...rest} ref={wrapperRef} - aria-describedby={isVisible ? tooltipId : undefined} + aria-describedby={ + isVisible ? [rest['aria-describedby'], tooltipId].filter(Boolean).join(' ') : rest['aria-describedby'] + }📝 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.{...rest} ref={wrapperRef} aria-describedby={ isVisible ? [rest['aria-describedby'], tooltipId].filter(Boolean).join(' ') : rest['aria-describedby'] } className={clsx(asChild ? undefined : styles.button, className)} onMouseEnter={composeHandlers(onMouseEnter, triggerHandlers.onMouseEnter)} onMouseLeave={composeHandlers(onMouseLeave, triggerHandlers.onMouseLeave)} onFocus={composeHandlers(onFocus, triggerHandlers.onFocus)} onBlur={composeHandlers(onBlur, triggerHandlers.onBlur)} onMouseDown={composeHandlers(onMouseDown, triggerHandlers.onMouseDown)} onMouseUp={composeHandlers(onMouseUp, triggerHandlers.onMouseUp)}🤖 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 `@packages/tooltip/src/Tooltip.tsx` around lines 105 - 114, Update the trigger element props near wrapperRef so aria-describedby preserves and merges any consumer-provided value from rest with tooltipId when isVisible, rather than overriding it. Keep aria-describedby undefined only when neither the user value nor the visible tooltip provides an identifier.scripts/checkPackageConsistency.ts (1)
12-12: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allowlist rule-name mismatch breaks catalog-dependency exemptions.
isAllowlistedis invoked with rule'directDependency'(Line 205), but the violation actually pushed uses rule'catalogDependency'(Line 209). SinceisAllowlistedmatches on exactentry.rulestring, an allowlist entry copied from the reported violation (rule: 'catalogDependency') will never match, and the exemption silently never takes effect — the violation keeps reappearing regardless of the allowlist entry. TherulesRequiringFieldPathset (Line 12) and theallowlistEntrySchema.superRefinevalidation (Lines 21-36) are also keyed on'directDependency', reinforcing that this was meant to be the canonical name, while the displayed/output rule diverges.Align the two rule names (recommend using
'directDependency'consistently, or updating theisAllowlistedcall/rulesRequiringFieldPathentry to'catalogDependency') so allowlisting actually works for this rule.🐛 Proposed fix to align rule names
-const rulesRequiringFieldPath = new Set(['extraScript', 'directDependency']); +const rulesRequiringFieldPath = new Set(['extraScript', 'catalogDependency']);if (required && spec !== required) { - if (!isAllowlisted(policy.allowlist, name, 'directDependency', `${bucket}.${depName}`)) { + if (!isAllowlisted(policy.allowlist, name, 'catalogDependency', `${bucket}.${depName}`)) { violations.push({Also applies to: 202-215
🤖 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 `@scripts/checkPackageConsistency.ts` at line 12, Align the catalog dependency rule name across the violation flow: update the reported violation near the isAllowlisted call in the direct-dependency check to use the canonical name already used by rulesRequiringFieldPath and allowlistEntrySchema, so isAllowlisted matches allowlist entries consistently.
Align with the team convention that test descriptions be written in English. Test bodies and coverage are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-package changeset keeps each CHANGELOG scoped to its own package description instead of mixing button and card notes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
텍스트 색이랑 버튼 색이랑 좀 대비율좀 높여야 할 것 같아요. 웹접근성 최소대비차: |
| padding: `0 ${vars.spacing.component.sm}`, | ||
| borderRadius: '6px', | ||
| padding: `0 ${vars.spacing.component.md}`, | ||
| borderRadius: vars.radius.component.md, |
There was a problem hiding this comment.
이거 lg 사이즈의 padding 값이랑 borderRadius 값은 의도하신거라 보면 될까요...?
| ':active': { | ||
| borderColor: vars.color.accent.hover, | ||
| color: vars.color.accent.hover, | ||
| }, |
There was a problem hiding this comment.
이거 내용 PR 설명에도 넣어주심 좋을 것 같아요.
| ':active': { | ||
| background: buttonRed, | ||
| backgroundColor: vars.color.accent.hover, | ||
| opacity: 0.9, |
There was a problem hiding this comment.
음....
차라리, opacity 값을 넣는 것보단,
accent.pressed 값을 대비하여, 임시로 backgroundColor 값을 하드코딩하는 건 어떨까요?
(c.c @Yeom-JinHo , @3o14 )
|
|
||
| test('applies semantic component spacing token as padding', () => { | ||
| render(<Card>Card</Card>); | ||
| expect(rulesForElement(screen.getByText('Card'))).toContain('var(--side-spacing-component-lg)'); |
There was a problem hiding this comment.
음?
이러면 실제 값 검증이 되나요?
임시 테스트 코드라면 주석이라도 달면 좋을 것 같습니다.
3 participants
Changes
Sprint 1 새 semantic 토큰(
vars.color.*,vars.spacing.component.*,vars.radius.component.*)을 담당 컴포넌트 Button, Card에 적용했습니다.Button (
packages/button/src/Button.css.ts)vars.color.accent.{default,hover,subtle}+foreground.onAccentcolor.gray500/600disabled →vars.color.background.muted+vars.color.foreground.subtle(foreground.muted는 "읽기용 텍스트 금지" 명시로 배제)vars.color.border.focusvars.spacing.component.*/vars.radius.component.md|lg로 통일:active에opacity: 0.9추가하여 hover와 시각적 계층 구분Card (
packages/card/src/Card.css.ts)color.gray50/100/200,color.cyan300→vars.color.background.{base,subtle},vars.color.border.{default,strong}accent.default대신border.strong사용 (인터랙티브 컬러와 구분선 의미 분리)borderRadius: 12px→vars.radius.component.lg(8px),padding: 20px→vars.spacing.component.lg(16px) — 토큰 스펙(카드 모서리/카드 내부 패딩) 준수공통
bordershorthand는 CSS custom property가 포함될 때 vanilla-extract가 잘못 파싱하는 이슈가 있어borderWidth/Style/Color로 분해테스트 (
packages/card/src/Card.test.tsx)toHaveStyle로 검증 — semantic 토큰 도입 후 stylesheet에var(--side-...)참조만 남아 happy-dom의getComputedStyle이 값을 반환하지 않음document.styleSheets를 순회해 매칭되는 rule의 cssText에서var(--side-...)참조를 확인하는 헬퍼(ruleForClass/rulesForElement)로 재작성Checklist
pnpm --filter @sipe-team/button test— 18 tests passedpnpm --filter @sipe-team/card test— 16 tests passedpnpm --filter @sipe-team/button typecheck/pnpm --filter @sipe-team/card typecheck통과pnpm --filter @sipe-team/button build/pnpm --filter @sipe-team/card build— ESM/CJS/DTS 성공pnpm --filter @sipe-team/button lint/pnpm --filter @sipe-team/card lint— no issuesAdditional Discussion Points
accent.*시리즈에active/pressed단계 토큰이 없어 fill:active가:hover와 동일 토큰을 재사용합니다. 임시로 opacity로 press 피드백을 추가했지만, 장기적으로 토큰 시스템에accent.pressed추가를 제안드립니다.bordershorthand + CSS custom property 조합 파싱 이슈는 다른 컴포넌트에도 영향이 있을 수 있으니 마이그레이션 시 참고 부탁드립니다.Summary by CodeRabbit
New Features
Bug Fixes
Documentation