Fix @scope variant output and add test for scoped variant #19299
Conversation
WalkthroughThe PR updates internal AST and variant handling and adds a test. It adds minor whitespace changes in Pre-merge checks✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (2)
packages/tailwindcss/src/ast.ts (1)
328-336: Prefer a constant Set for at-rule names.The hardcoded string comparisons on line 331 work but are harder to maintain. Consider extracting these into a constant Set at the top of the file for better readability and maintainability.
Apply this diff:
+const CONDITIONAL_AT_RULES = new Set(['@media', '@supports', '@container', '@scope']) + const AT_SIGN = 0x40Then update the condition:
if ( nodes.length === 1 && nodes[0].kind === 'at-rule' && - (nodes[0].name === '@media' || nodes[0].name === '@supports' || nodes[0].name === '@container' || nodes[0].name === '@scope') + CONDITIONAL_AT_RULES.has(nodes[0].name) ) {packages/tailwindcss/src/variants.scope.test.ts (1)
7-42: Consider adding test coverage for other conditional at-rules.The fix in
ast.ts(line 331) handles@media,@supports, and@containerin addition to@scope, but this test only validates@scope. While the core logic is the same, adding test cases for the other at-rules would provide better regression protection and ensure the transformation works correctly for all supported conditional at-rules.Consider adding parallel test cases like:
test('custom variants using @media should wrap correctly', async () => { // Test with addVariant('desktop', '@media (min-width: 1024px) { & }') }) test('custom variants using @supports should wrap correctly', async () => { // Test with addVariant('has-grid', '@supports (display: grid) { & }') }) test('custom variants using @container should wrap correctly', async () => { // Test with addVariant('narrow', '@container (max-width: 400px) { & }') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/tailwindcss/src/ast.ts(1 hunks)packages/tailwindcss/src/variants.scope.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/variants.scope.test.ts (1)
packages/tailwindcss/src/compat/plugin-api.ts (1)
addVariant(120-160)
| // 👇 Move your debug log here | ||
| console.log('\n\n=== GENERATED CSS ===\n', result, '\n====================\n\n') |
There was a problem hiding this comment.
Remove debug console.log.
The console.log statement on line 35 creates noisy test output and should be removed before merging.
Apply this diff:
- // 👇 Move your debug log here
- console.log('\n\n=== GENERATED CSS ===\n', result, '\n====================\n\n')
-📝 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.
| // 👇 Move your debug log here | |
| console.log('\n\n=== GENERATED CSS ===\n', result, '\n====================\n\n') |
🤖 Prompt for AI Agents
In packages/tailwindcss/src/variants.scope.test.ts around lines 34 to 35, there
is a debug console.log that prints generated CSS and creates noisy test output;
remove the console.log statement (and any surrounding leftover blank lines it
introduced) so tests no longer emit the debug output, keeping the rest of the
test intact.
thecrypticace
left a comment
thecrypticace
left a comment
There was a problem hiding this comment.
This implementation may end up modifying user written CSS incorrectly — so definitely don't think we should be doing this in optimizeAst.
A "better" way to do this would be to handle this in variants directly — not sure if we need a marker node other than @slot to notate that things can be wrapped on the outside though as our entire variant implementation was built on nesting inside the class. 🤔
hey @thecrypticace , Thanks for the feedback! I’ve updated the implementation so conditional at-rules like @scope are now handled directly in the variants layer instead of optimizeAst. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/tailwindcss/src/ast.ts(1 hunks)packages/tailwindcss/src/compat/plugin-api.ts(1 hunks)packages/tailwindcss/src/compile.ts(2 hunks)packages/tailwindcss/src/variants.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tailwindcss/src/ast.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/tailwindcss/src/variants.ts (1)
packages/tailwindcss/src/ast.ts (1)
AstNode(68-68)
packages/tailwindcss/src/compile.ts (2)
packages/tailwindcss/src/ast.ts (3)
StyleRule(13-20)AstNode(68-68)rule(87-93)packages/tailwindcss/src/walk.ts (1)
walk(42-53)
| let body = parseVariantValue(variant, r.nodes) | ||
|
|
||
| const isBlock = | ||
| typeof variant === 'string' | ||
| ? variant.trim().endsWith('}') | ||
| : variant.some((v) => v.trim().endsWith('}')) | ||
|
|
||
| if (isBlock && body.length === 1 && body[0].kind === 'at-rule') { | ||
| return body[0] | ||
| } |
There was a problem hiding this comment.
Don’t gate the at-rule collapse on isBlock.
isBlock only returns true when the raw variant string literally ends with }. As soon as someone writes a scoped variant with a trailing comment or other postfix (e.g. addVariant('scoped', '@scope (&) { @slot } /* note */')), the condition fails, we skip the body[0] return, and the compiler falls back to emitting .selector { @scope … }—the exact invalid structure this PR is meant to fix. Please decide purely from the parsed AST (e.g. if (body.length === 1 && body[0].kind === 'at-rule')) so single at-rule variants are collapsed regardless of formatting. This keeps the fix from regressing in common “annotated” plugin variants.
🤖 Prompt for AI Agents
In packages/tailwindcss/src/compat/plugin-api.ts around lines 148 to 157, the
code currently gates collapsing a single at-rule variant on the heuristic
isBlock (which checks whether the raw variant text ends with '}'), causing
variants with trailing comments or other postfixes to miss the collapse and
produce invalid output; change the logic to decide solely from the parsed AST by
removing the isBlock requirement and returning body[0] whenever body.length ===
1 and body[0].kind === 'at-rule' so single at-rule variants are collapsed
regardless of trailing formatting or comments.
2 participants
This pull request fixes the output structure for custom variants that use conditional at-rules such as @scope.
Previously, the generated CSS placed the selector before the at-rule, which resulted in invalid nesting. After this change, the compiler correctly emits the conditional at-rule as the outer wrapper, ensuring valid and expected CSS output.
Changes Made:
file variants.scope.test.ts:
Added a new test case verifying that custom variants using @scope produce correctly structured CSS.
Ensures the at-rule wraps the selector and that theme variables are applied properly.
file ast.ts :
Added a transform in the [optimizeAst]
function that detects when a style rule contains only a single conditional at-rule child (@scope, @media, @supports, @container) and restructures the AST to emit the at-rule as the outer wrapper with the rule inside.
Test Plan:
Verified that the new test case for @scope passes successfully.
Confirmed that all existing test suites continue to pass without regressions.
Validated the generated CSS output structure to ensure correctness and consistency with other conditional at-rules.
Result:
Correctly wraps conditional at-rules like @scope around variant selectors.
Maintains backward compatibility with existing variant behavior.
All tests pass successfully across supported environments.