Feat: cx function from clsx #285#286
Conversation
|
📝 WalkthroughWalkthroughAdds a clsx-based Changes
Sequence Diagram(s)(omitted — changes are local library additions without multi-component sequential interactions) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Triggered from #286 by @black7375. Checking if we can fast forward Target branch ( commit 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 18 00:00:00 2025 +0900
Fix: `theme` - composite valuePull request ( commit 02092733b74affb4c920317ab8bfff59dd9ce239 (pull_request/cx-fn)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Nov 22 00:00:00 2025 +0900
Feat: `cx` function from clsx #285Can't fast forward * 02092733b74affb4c920317ab8bfff59dd9ce239 Feat: `cx` function from clsx #285
| * 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff Fix: `theme` - composite value
| * 60c2a59bc5cd079ce5bf980b6cc397714bc2a09e Fix: Theme - `alias` value
| * 26546b1a08cecbbaba4a4895d09657bdc7e7a633 Chore: update eslint config as consist
|/
* 1bb50106844d162d26f1f470ef0cae0565461708 Refactor: restrict `rules()` to function-style compoundVariants #245
commit 1bb50106844d162d26f1f470ef0cae0565461708
Author: JeongJun <rgfdds98@gmail.com>
Date: Wed Oct 15 00:00:00 2025 +0900
Refactor: restrict `rules()` to function-style compoundVariants #245Rebase locally, and then force push to |
|
Triggered from #286 by @black7375. Checking if we can fast forward Target branch ( commit 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 18 00:00:00 2025 +0900
Fix: `theme` - composite valuePull request ( commit ecd8576c32ccd2327072f662eecdd525a909b0f6 (pull_request/cx-fn)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Nov 22 00:00:00 2025 +0900
Feat: `cx` function from clsx #285Can't fast forward * ecd8576c32ccd2327072f662eecdd525a909b0f6 Feat: `cx` function from clsx #285
| * 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff Fix: `theme` - composite value
| * 60c2a59bc5cd079ce5bf980b6cc397714bc2a09e Fix: Theme - `alias` value
| * 26546b1a08cecbbaba4a4895d09657bdc7e7a633 Chore: update eslint config as consist
|/
* 1bb50106844d162d26f1f470ef0cae0565461708 Refactor: restrict `rules()` to function-style compoundVariants #245
commit 1bb50106844d162d26f1f470ef0cae0565461708
Author: JeongJun <rgfdds98@gmail.com>
Date: Wed Oct 15 00:00:00 2025 +0900
Refactor: restrict `rules()` to function-style compoundVariants #245Rebase locally, and then force push to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/css/src/classname/cx.ts:
- Around line 31-129: The test block opened by the conditional if
(import.meta.vitest) { in the cx tests is missing its closing brace; fix by
adding a matching } (and optional newline) at the end of the block to properly
terminate the if (import.meta.vitest) { ... } scope so the file parses and CI
passes.
In @packages/css/src/classname/types.ts:
- Around line 17-22: The doc/type mismatch: update the ClassDictionary type to
enforce boolean values by replacing export type ClassDictionary = Record<string,
unknown>; with export type ClassDictionary = Record<string, boolean>; and adjust
the JSDoc/example to reflect boolean conditions (e.g., { 'bg-blue-500': true,
'text-white': false }) so the type and comment are consistent; if you prefer to
allow truthy/falsy values instead, alternatively change the JSDoc wording to
“truthy/falsey conditions” rather than tightening the type.
🧹 Nitpick comments (1)
packages/css/src/classname/cx.ts (1)
31-129: Consider moving in-source tests to a*.test.tsfile to avoid shipping them in library output.
Theimport.meta.vitestgate prevents execution, but the test code still tends to end up in published artifacts unless your build strips it.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/css/package.jsonpackages/css/src/classname/cx.tspackages/css/src/classname/index.tspackages/css/src/classname/types.tspackages/css/src/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-09T13:00:38.449Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
Applied to files:
packages/css/src/index.ts
🪛 Biome (2.1.2)
packages/css/src/classname/cx.ts
[error] 129-129: expected } but instead the file ends
the file ends here
(parse)
🪛 GitHub Actions: CI
packages/css/src/classname/cx.ts
[error] 130-130: TypeScript error TS1005: '}' expected.
🔇 Additional comments (4)
packages/css/src/index.ts (1)
61-66: Public API export looks good; please ensure the build actually emitsclassname/index.{mjs,cjs,d.ts}as referenced.
Main risk is packaging/emit configuration, not TS syntax here.packages/css/src/classname/index.ts (1)
1-3: Clean re-export surface forclassname.packages/css/src/classname/cx.ts (1)
1-30: No action needed onclsximport. Theimport { clsx }syntax is correct—clsx v2.1.1 exports both a default export and a namedclsxexport, properly typed for TypeScript.Likely an incorrect or invalid review comment.
packages/css/package.json (1)
85-89: No issues found. Theclsxv2.1.1 version is compatible with your ESM+CJS build setup. The codebase correctly uses the named import (import { clsx } from "clsx"), which is the recommended approach for v2 and works cleanly for both CommonJS and ES module consumers without requiringesModuleInterop. The package'sexportsmap properly handles both output formats.
| /** | ||
| * Object with class names as keys and boolean conditions as values | ||
| * @example { 'bg-blue-500': true, 'text-white': isActive } | ||
| */ | ||
| export type ClassDictionary = Record<string, unknown>; | ||
|
|
There was a problem hiding this comment.
Docs/type mismatch: ClassDictionary values aren’t necessarily boolean.
Either tighten the type to booleans (and maybe ClassValue) or adjust the doc to “truthy/falsey conditions”.
Proposed doc fix (minimal)
/**
- * Object with class names as keys and boolean conditions as values
+ * Object with class names as keys and truthy/falsey conditions as values
* @example { 'bg-blue-500': true, 'text-white': isActive }
*/
export type ClassDictionary = Record<string, unknown>;📝 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.
| /** | |
| * Object with class names as keys and boolean conditions as values | |
| * @example { 'bg-blue-500': true, 'text-white': isActive } | |
| */ | |
| export type ClassDictionary = Record<string, unknown>; | |
| /** | |
| * Object with class names as keys and truthy/falsey conditions as values | |
| * @example { 'bg-blue-500': true, 'text-white': isActive } | |
| */ | |
| export type ClassDictionary = Record<string, unknown>; |
🤖 Prompt for AI Agents
In @packages/css/src/classname/types.ts around lines 17 - 22, The doc/type
mismatch: update the ClassDictionary type to enforce boolean values by replacing
export type ClassDictionary = Record<string, unknown>; with export type
ClassDictionary = Record<string, boolean>; and adjust the JSDoc/example to
reflect boolean conditions (e.g., { 'bg-blue-500': true, 'text-white': false })
so the type and comment are consistent; if you prefer to allow truthy/falsy
values instead, alternatively change the JSDoc wording to “truthy/falsey
conditions” rather than tightening the type.
|
Triggered from #286 by @black7375. Checking if we can fast forward Target branch ( commit 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 18 00:00:00 2025 +0900
Fix: `theme` - composite valuePull request ( commit d7cf9b54ceed3bbb251661ee542372082074e044 (pull_request/cx-fn)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Nov 22 00:00:00 2025 +0900
Feat: `cx` function from clsx #285It is possible to fast forward |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/css/src/classname/cx.ts (1)
3-28: Clarify "duplicates preserved" and add test coverage.The JSDoc states that the function returns "merged class name string with duplicates preserved" (line 7). While technically accurate (clsx doesn't deduplicate), this phrasing may confuse readers about whether duplicate handling is intentional behavior.
Additionally, the test suite doesn't include a case that explicitly verifies duplicate preservation, such as:
expect(cx("foo", "foo", "bar")).toBe("foo foo bar");Consider either:
- Clarifying the JSDoc to explicitly state that duplicates are not removed (e.g., "without deduplication")
- Adding a test case that demonstrates this behavior
📝 Suggested improvements
JSDoc clarification:
- * @returns Merged class name string with duplicates preserved + * @returns Merged class name string (duplicates are not removed)Additional test case:
it("preserves duplicate class names", () => { expect(cx("foo", "foo", "bar")).toBe("foo foo bar"); expect(cx("foo", { foo: true })).toBe("foo foo"); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/css/package.jsonpackages/css/src/classname/cx.tspackages/css/src/classname/index.tspackages/css/src/classname/types.tspackages/css/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/css/src/index.ts
- packages/css/src/classname/index.ts
- packages/css/src/classname/types.ts
- packages/css/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-03T11:16:32.528Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 134
File: packages/transform-to-vanilla/src/transform.ts:49-51
Timestamp: 2024-12-03T11:16:32.528Z
Learning: In projects that target both CommonJS and ES modules, adjusting the TypeScript `module` option may not be feasible. Therefore, using `ts-ignore` comments to suppress TypeScript errors related to `import.meta` is acceptable.
Applied to files:
packages/css/src/classname/cx.ts
🧬 Code graph analysis (1)
packages/css/src/classname/cx.ts (2)
packages/css/src/classname/index.ts (1)
cx(1-1)packages/css/src/index.ts (1)
cx(61-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: typecheck
🔇 Additional comments (3)
packages/css/src/classname/cx.ts (3)
29-29: Clean re-export pattern.The re-export pattern
export const cx = clsx;is idiomatic and will correctly inherit types from the clsx library. This approach keeps the API surface simple while maintaining full type safety.
31-130: Comprehensive test coverage with appropriate use of ts-ignore.The test suite is thorough and well-structured:
- Covers all major use cases (strings, objects, arrays, mixed inputs, edge cases)
- Uses type assertions to verify type safety
- Aligns with JSDoc examples for consistency
- Employs
describe.concurrentfor efficient parallel executionThe
@ts-ignorecomments forimport.meta.vitestare acceptable based on project learnings, as this is a known issue in dual CommonJS/ES module environments.Based on learnings: Using
ts-ignorecomments forimport.metais acceptable in this project's build configuration.
1-1: The import ofclsxis secure and appropriately versioned. Version 2.1.1 is the latest release with no known security vulnerabilities or compatibility concerns.
|
/fast-forward |
|
Triggered from #286 (comment) by @black7375. Trying to fast forward Target branch ( commit 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 18 00:00:00 2025 +0900
Fix: `theme` - composite valuePull request ( commit d7cf9b54ceed3bbb251661ee542372082074e044 (pull_request/cx-fn)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Nov 22 00:00:00 2025 +0900
Feat: `cx` function from clsx #285Fast forwarding $ git push origin d7cf9b54ceed3bbb251661ee542372082074e044:main
To https://github.com/mincho-js/mincho.git
236463b..d7cf9b5 d7cf9b54ceed3bbb251661ee542372082074e044 -> main |
1 participant
Description
clsxfunction tocxRelated Issue
cxclassname function #285Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Additional context
Checklist