Fix: Global CSS using hoisting selectors#209
Conversation
🦋 Changeset detectedLatest commit: df91f14 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant globalCss
participant hoistSelectors
participant gStyle
User->>globalCss: Call with selector and rule
globalCss->>globalCss: Wrap rule in selectors object
globalCss->>hoistSelectors: Hoist nested selectors/at-rules
hoistSelectors-->>globalCss: Return flattened selectors and at-rules
loop For each selector
globalCss->>gStyle: Apply style for selector
end
loop For each at-rule
globalCss->>gStyle: Apply style for at-rule/selector
end
Possibly related PRs
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Triggered from #209 by @black7375. Checking if we can fast forward Target branch ( commit 2a6fe6490b7c37743c4f5afe3c75de25feff7e85 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Fri May 9 00:00:00 2025 +0900
Chore: fix security issuesPull request ( commit df91f148cf4c0db6a19f544fb993943cc1f50a70 (pull_request/fix-global-css)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Jun 7 03:32:43 2025 +0900
Fix: Nested Global CSS using hoisting selectorsIt is possible to fast forward |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
vitest.config.ts (1)
3-7: Consider adding common test configuration options.While the minimal configuration is appropriate for a project-based setup, you might want to consider adding common test options that apply across all projects, such as:
export default defineConfig({ test: { projects: ["packages/*/vite.config.ts"], + globals: true, + environment: "happy-dom" } });This ensures consistent test environments across all packages while still allowing individual packages to override these settings in their own
vite.config.tsfiles.packages/css/src/css/index.ts (2)
43-48: Address the TODO: Improve type safetyThe
UnknownObjecttype usingRecord<string, unknown>sacrifices type safety. Consider defining more specific types based on the actual structure of CSS rules and at-rules to catch potential issues at compile time.Would you like me to help create more type-safe definitions based on the expected structure of the transformed CSS objects?
50-102: Consider adding validation and improving type safety in hoistSelectorsThe implementation correctly hoists selectors, but could benefit from:
- Path validation: The function assumes
pathcontains pairs of (atRule, condition), but doesn't validate this assumption- Property collision handling:
Object.assignon line 83 could silently overwrite existing propertiesConsider adding validation:
function processAtRules(obj: UnknownObject, path: string[] = []) { + // Ensure path has even length (pairs of atRule/condition) + if (path.length % 2 !== 0) { + throw new Error('Invalid path structure: expected pairs of atRule/condition'); + } + for (const key in obj) { if (key === "selectors") {For property collisions, consider using a merge strategy or warning when overwriting:
- Object.assign(current, selectors[selector]); + // Warn about property collisions during development + if (process.env.NODE_ENV !== 'production') { + const existingKeys = Object.keys(current); + const newKeys = Object.keys(selectors[selector]); + const collisions = existingKeys.filter(k => newKeys.includes(k)); + if (collisions.length > 0) { + console.warn(`Property collision in hoisted selectors: ${collisions.join(', ')}`); + } + } + Object.assign(current, selectors[selector]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/sharp-ears-wear.md(1 hunks)packages/css/src/css/index.ts(1 hunks)packages/css/src/index.ts(1 hunks)packages/css/src/rules/index.ts(2 hunks)packages/css/src/rules/types.ts(1 hunks)vitest.config.ts(1 hunks)vitest.workspace.ts(0 hunks)
💤 Files with no reviewable changes (1)
- vitest.workspace.ts
🧰 Additional context used
🧠 Learnings (1)
packages/css/src/index.ts (1)
Learnt from: black7375
PR: mincho-js/mincho#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`.
🔇 Additional comments (9)
vitest.config.ts (2)
1-2: LGTM!The import statement correctly uses
defineConfigfrom vitest/config, which is the standard approach for TypeScript-based Vitest configurations.
3-7:Details
✅ Verification successful
Verify that all packages have vite.config.ts files.
The configuration structure looks good and the move from workspace to project-based configuration is a solid architectural decision. However, ensure that all packages in the
packages/*directories actually havevite.config.tsfiles, as missing files could cause configuration errors.
🏁 Script executed:
#!/bin/bash # Description: Verify that all packages have vite.config.ts files as expected by the Vitest configuration. # Expected: All directories in packages/ should have vite.config.ts files. echo "Checking for packages directories:" fd -t d . packages --max-depth 1 echo -e "\nChecking for vite.config.ts files in packages:" fd "vite.config.ts" packages echo -e "\nPackages without vite.config.ts files:" for dir in packages/*/; do if [ -d "$dir" ] && [ ! -f "${dir}vite.config.ts" ]; then echo "Missing: ${dir}vite.config.ts" fi doneLength of output: 1823
All packages have vite.config.ts files as expected
Verified that every directory under
packages/contains avite.config.tsfile, so the Vitestprojects: ["packages/*/vite.config.ts"]pattern is safe. No further changes needed—approving this configuration.packages/css/src/index.ts (1)
39-39: LGTM! Proper type export for public API.The export of
VariantStyletype follows the established pattern and makes the new type available for external usage, which aligns with the type safety improvements mentioned in the changeset..changeset/sharp-ears-wear.md (1)
8-9: Verify completeness of the changeset documentation.The changeset mentions changes to the
globalCssfunction to allow nested selectors, but these changes are not visible in the provided files for review. This could indicate incomplete file coverage in the PR.Please verify that all relevant files mentioned in the changeset are included in the PR, particularly the
globalCssfunction changes referenced inpackages/css/src/css/index.ts.Likely an incorrect or invalid review comment.
packages/css/src/rules/index.ts (2)
29-30: LGTM! Proper import of new types.The addition of
VariantStringMapandVariantStyleimports supports the enhanced type checking implemented in the test cases below.
268-268: Excellent use ofsatisfiesoperator for type safety.The type assertions using
satisfies VariantStyle<...>provide compile-time type checking for variant objects without affecting runtime behavior. This ensures the test data conforms to the expected variant structure and helps catch type mismatches early.Also applies to: 273-273, 277-277
packages/css/src/rules/types.ts (2)
34-39: Well-designed generic type for variant constraints.The
VariantStyletype is properly structured with:
- Generic constraint
VariantNames extends stringfor flexibility- Sensible default
CssRule extends RecipeStyleRule = RecipeStyleRule- Clear mapped type structure mapping variant names to CSS rules
This enhances type safety for variant style definitions across the codebase.
41-41: Helpful clarifying comment.The comment about
VariantDefinitionsbeing similar toVariantMapbut optimized for fast type checking provides valuable context for maintainers.packages/css/src/css/index.ts (1)
18-40: Well-implemented refactoring to support nested selectors!The restructuring of
globalCssto wrap rules in a selectors object and separately handle at-rule styles is a clean approach. This enables the nested selector support mentioned in the PR objectives.
|
/fast-forward |
|
Triggered from #209 (comment) by @black7375. Trying to fast forward Target branch ( commit 2a6fe6490b7c37743c4f5afe3c75de25feff7e85 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Fri May 9 00:00:00 2025 +0900
Chore: fix security issuesPull request ( commit df91f148cf4c0db6a19f544fb993943cc1f50a70 (pull_request/fix-global-css)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Jun 7 03:32:43 2025 +0900
Fix: Nested Global CSS using hoisting selectorsFast forwarding $ git push origin df91f148cf4c0db6a19f544fb993943cc1f50a70:main
To https://github.com/mincho-js/mincho.git
2a6fe64..df91f14 df91f148cf4c0db6a19f544fb993943cc1f50a70 -> main |
1 participant
Description
VariantStyletypeRelated Issue
Summary by CodeRabbit
New Features
Chores
Additional context
Checklist