Fix: CSS - More strict required check css.with() #226#230
Fix: CSS - More strict required check css.with() #226#230github-actions[bot] merged 1 commit intomainfrom
css.with() #226#230Conversation
|
WalkthroughType-level enforcement tightened by making required CSSRule keys non-nullable. Corresponding TypeScript tests for css.with were expanded and reorganized: variable rename, added helper types, additional ts-expect-error cases, and raw() validations. No runtime/production logic changed; no exported APIs beyond the type alias semantics were altered. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Triggered from #230 by @black7375. Checking if we can fast forward Target branch ( commit 60ebee56170b3b683b72eb721fdd26bfc46ce338 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Wed Jun 11 00:00:00 2025 +0900
Feat: CSS - `css.with()` API support #226Pull request ( commit 216f6e113a5e357d6fd8a0abe7f02f058d2a62d5 (pull_request/css-with)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Mon Jul 21 00:00:00 2025 +0900
Fix: CSS - More strict required check `css.with()` #226It 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 (2)
packages/css/src/css/index.ts (2)
355-360: Clarify comments: “values” vs “properties”The inline comments read like property-level allowances, but they’re actually about value domains.
Apply this diff to improve clarity:
- color: "red", // Allow all properties - background: "blue", // Only some properties are allowed + color: "red", // Allow any value for 'color' + background: "blue", // Only some values are allowed for 'background'
366-374: Add explicit undefined/null cases to validate NonNullable enforcementSince required keys marked with true are now NonNullable, add explicit checks for undefined and null to prevent regressions.
Apply this diff to extend coverage:
// @ts-expect-error: color is required myCss1({ background: "blue" }); // @ts-expect-error: background is required myCss1({ color: "red" }); + // @ts-expect-error: color cannot be undefined when marked as required + myCss1({ + color: undefined, + background: "blue" + }); + // @ts-expect-error: color cannot be null when marked as required + myCss1({ + // null isn't a valid CSS value here and is excluded by NonNullable + color: null, + background: "blue" + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/css/src/css/index.ts(1 hunks)packages/css/src/css/types.ts(1 hunks)
🔇 Additional comments (4)
packages/css/src/css/types.ts (1)
12-12: NonNullable for required CSSRule values is the right tighteningUsing NonNullable<CSSRule[K]> for keys marked with true closes the gap where a required property could still accept undefined/null. This aligns perfectly with the PR goal and keeps user-specified unions (like "blue" | "grey") intact elsewhere.
packages/css/src/css/index.ts (3)
349-354: Good expansion of type-restriction coverageDefining myCss1 with mixed sentinel booleans and literal unions is a clear, readable way to exercise the RestrictCSSRule behavior for allowed, denied, and required keys.
361-365: LGTM: negative test for disallowed value domainThe ts-expect-error for background: "red" correctly verifies that only "blue" | "grey" are permitted.
375-378: Nice: verifies requiredness without callback pathThe myCss2 case is a solid assertion that requiredness is preserved even when no callback is supplied (identity path), and radius remains optional.
|
/fast-forward |
|
Triggered from #230 (comment) by @black7375. Trying to fast forward Target branch ( commit 60ebee56170b3b683b72eb721fdd26bfc46ce338 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Wed Jun 11 00:00:00 2025 +0900
Feat: CSS - `css.with()` API support #226Pull request ( commit 216f6e113a5e357d6fd8a0abe7f02f058d2a62d5 (pull_request/css-with)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Mon Jul 21 00:00:00 2025 +0900
Fix: CSS - More strict required check `css.with()` #226Fast forwarding $ git push origin 216f6e113a5e357d6fd8a0abe7f02f058d2a62d5:main
To https://github.com/mincho-js/mincho.git
60ebee5..216f6e1 216f6e113a5e357d6fd8a0abe7f02f058d2a62d5 -> main |
1 participant
Description
More strict type check for required CSSRule type for
css.with()API.Related Issue
Summary by CodeRabbit
Additional context
Checklist