Properly negate custom variants with @container when used with not-*#20059
Conversation
We parse the condition into a Value AST. This could also be done by the SelectorParser instead of the ValueParser, but the ValueParser is a bit simpler and enough for what we need for now. Once we have the AST, we can check data of the tree to determine what kind of shape we are in. The query of a `@container` looks like `(…)` or `style(…)` for example, and in both cases those are functions, the latter case wasn't handled properly.
Confidence Score: 5/5Safe to merge — the change is a well-scoped fix for a specific @container style(…) negation bug, all existing tests continue to pass, and four new snapshot tests cover every grammar shape. The refactoring from string-character inspection to AST-based matching is correct for all valid CSS @container inputs. The length guards sufficiently protect every indexed AST access, the four structural branches are mutually exclusive and exhaustive for real-world input, and the fallback safely handles any unforeseen edge case. No logic regressions were found. No files require special attention. Reviews (2): Last reviewed commit: "ensure ast contains at least x amount of..." | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR updates Tailwind's variant negation logic to correctly handle 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/tailwindcss/src/variants.ts`:
- Around line 381-418: The parsing code uses ValueParser.parse(condition) into
ast and then indexes ast[0], ast[2], and ast[4] without checking for their
existence; update the branches in the container variant handling (the checks
that read ast[0].kind, ast[2].kind, ast[4].kind) to first guard that the array
has the required length and that ast[index] is defined before accessing
.kind/.value (e.g., check ast[0], ast[2], ast[4] truthiness and ast.length >=
required index+1), and only then perform the existing kind/value comparisons and
splices; if the guards fail, fall back to the safe default behavior (return the
original condition or the existing fallback path) so malformed or empty
conditions like "" or "@container foo" do not throw.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a219861d-0320-4093-9166-1acc4c2ea904
📒 Files selected for processing (2)
packages/tailwindcss/src/variants.test.tspackages/tailwindcss/src/variants.ts
This PR fixes an issue where a custom variant declared with a
@containerwasn't properly negated when using it in combination with thenot-*variant.Given this CSS:
If you then used
not-has-a:flex, then the following CSS was produced:But we expect the
notto be in the correct location:The issue was that we did some string related checks, and we assumed that the
querypart of the@container(style(--a)) had to start with a(character.To fix this, we now parse the value to an AST, and verify the AST shape before manipulating it. This now checks whether the
querypart is a function (both(…)andstyle(…)are considered functions).Also added some additional tests that were already handled, these cases look like:
@container {query}@container not {query}@container {name} not {query}@container {name} {query}Fixes: #20058
Test plan