Simplify selector combinators#20089
Conversation
Then during the `.toCss()` process we can re-print whitespace around the combinators in case `minify = false`. The benefit here is that we don't have to keep checking for the `.trim()` value in other spots. We can just rely on combinators as-is.
This is already guaranteed by the `SelectorParser` itself
Now that we automatically minify combinators, we have to take this into account.
Confidence Score: 5/5Safe to merge — the parser now owns combinator normalisation at parse time, removing scattered .trim() guards from call sites without changing observable output. The changes are a tightly-scoped refactor: normalisation logic moves into the parser, the Combinator type enforces the contract statically, and toCss behaviour is unchanged for the default (non-minified) path. The minify=true path is exercised by new tests and used consistently across all call sites in canonicalize-candidates.ts. No files require special attention. Reviews (3): Last reviewed commit: "add tests around printing complex select..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
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 (1)
WalkthroughThis PR hardens the selector parser's type system and extends its CSS serialization capabilities with minification support. It introduces a 🚥 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 |
![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
🤖 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/selector-parser.ts`:
- Around line 277-285: The slice/trim can produce multi-character strings like
">+" which are being force-cast into the Combinator union; instead normalize to
a single valid combinator before calling combinator. In the block that computes
value (the variable value used in target.push(combinator(...)) inside
selector-parser.ts), detect multi-character/mixed tokens: if value contains any
whitespace use the single-space combinator ' ', otherwise pick the first
character from value that is one of '>', '+', or '~' (fallback to ' ' if none
found). Replace the current (value === '' ? ' ' : value) as Combinator with the
normalized single-character combinator and pass that to combinator to ensure AST
nodes always use a valid single-token combinator.
🪄 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: b065a7e4-1451-4bf3-81bb-62ffb7c5ee7d
📒 Files selected for processing (3)
packages/tailwindcss/src/canonicalize-candidates.tspackages/tailwindcss/src/selector-parser.test.tspackages/tailwindcss/src/selector-parser.ts
| let value = input.slice(start, end).trim() | ||
| if ( | ||
| contents.trim() === '' && | ||
| value === '' && | ||
| (target.length === 0 || end >= input.length || input.charCodeAt(end) === COMMA) | ||
| ) { | ||
| break | ||
| } | ||
|
|
||
| let value = contents.trim() | ||
| target.push(combinator(value === '' ? ' ' : value)) | ||
| target.push(combinator((value === '' ? ' ' : value) as Combinator)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm the unsafe cast site and combinator scan logic.
rg -n "slice\\(start, end\\)\\.trim\\(|as Combinator|case GREATER_THAN|case PLUS|case TILDE" packages/tailwindcss/src/selector-parser.ts
# 2) Check whether tests currently cover mixed combinator runs (e.g. '>+', '+~').
rg -n "normalize combinators|>\\+|\\+>|\\+~|~\\+" packages/tailwindcss/src/selector-parser.test.tsRepository: tailwindlabs/tailwindcss
Length of output: 326
🏁 Script executed:
#!/bin/bash
# 1) Get full context of the loop containing lines 277-285
sed -n '240,290p' packages/tailwindcss/src/selector-parser.ts
# 2) Find the Combinator type definition
rg -n "type Combinator|enum Combinator" packages/tailwindcss/src/selector-parser.ts -A 5Repository: tailwindlabs/tailwindcss
Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Get the test implementation starting at line 288
sed -n '288,350p' packages/tailwindcss/src/selector-parser.test.ts
# Also check if there are other references to this normalization logic
rg -n "normalize|combinator.*test|test.*combinator" packages/tailwindcss/src/selector-parser.test.ts | head -20Repository: tailwindlabs/tailwindcss
Length of output: 1696
🏁 Script executed:
#!/bin/bash
# Search for all combinator-related tests
rg -n "combinator|>.*\+|\\+.*~" packages/tailwindcss/src/selector-parser.test.ts | head -30
# Check the combinator() function definition
rg -n "function combinator|const combinator|export.*combinator" packages/tailwindcss/src/selector-parser.ts -A 10Repository: tailwindlabs/tailwindcss
Length of output: 881
as Combinator permits invalid multi-token combinator values from malformed input.
At lines 277–285, the lookahead loop (lines 260–269) intentionally accumulates all contiguous characters matching GREATER_THAN, SPACE, PLUS, TAB, or TILDE. When slice(start, end) extracts this range and .trim() is applied, the result can be a multi-character string like ">+" or "+ " (if mixed combinators appear in the input). The as Combinator cast then forces this invalid string into the Combinator union type (' ' | '>' | '+' | '~'), violating the type contract that combinators must be single-character values. While real CSS wouldn't include mixed combinators, malformed input can trigger this, producing invalid AST nodes.
The existing test "should normalize combinators" (line 288) covers only single combinators and pure whitespace sequences—not mixed combinator cases, leaving this edge case unvalidated.
Suggested fix: normalize to a single explicit combinator token
let value = input.slice(start, end).trim()
if (
value === '' &&
(target.length === 0 || end >= input.length || input.charCodeAt(end) === COMMA)
) {
break
}
- target.push(combinator((value === '' ? ' ' : value) as Combinator))
+ let normalized: Combinator
+ if (value === '') {
+ normalized = ' '
+ } else if (value === '>' || value === '+' || value === '~') {
+ normalized = value
+ } else {
+ // Invalid multi-token combinator: normalize to descendant
+ normalized = ' '
+ }
+ target.push(combinator(normalized))📝 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.
| let value = input.slice(start, end).trim() | |
| if ( | |
| contents.trim() === '' && | |
| value === '' && | |
| (target.length === 0 || end >= input.length || input.charCodeAt(end) === COMMA) | |
| ) { | |
| break | |
| } | |
| let value = contents.trim() | |
| target.push(combinator(value === '' ? ' ' : value)) | |
| target.push(combinator((value === '' ? ' ' : value) as Combinator)) | |
| let value = input.slice(start, end).trim() | |
| if ( | |
| value === '' && | |
| (target.length === 0 || end >= input.length || input.charCodeAt(end) === COMMA) | |
| ) { | |
| break | |
| } | |
| let normalized: Combinator | |
| if (value === '') { | |
| normalized = ' ' | |
| } else if (value === '>' || value === '+' || value === '~') { | |
| normalized = value | |
| } else { | |
| // Invalid multi-token combinator: normalize to descendant | |
| normalized = ' ' | |
| } | |
| target.push(combinator(normalized)) |
🤖 Prompt for 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.
In `@packages/tailwindcss/src/selector-parser.ts` around lines 277 - 285, The
slice/trim can produce multi-character strings like ">+" which are being
force-cast into the Combinator union; instead normalize to a single valid
combinator before calling combinator. In the block that computes value (the
variable value used in target.push(combinator(...)) inside selector-parser.ts),
detect multi-character/mixed tokens: if value contains any whitespace use the
single-space combinator ' ', otherwise pick the first character from value that
is one of '>', '+', or '~' (fallback to ' ' if none found). Replace the current
(value === '' ? ' ' : value) as Combinator with the normalized single-character
combinator and pass that to combinator to ensure AST nodes always use a valid
single-token combinator.
There was a problem hiding this comment.
This behavior is the same as before >+ and can only happen if we start from invalid CSS. If this is an issue in practice, then we can solve it in a follow up PR.
There was a problem hiding this comment.
@RobinMalfait, that's a fair point — the behaviour is pre-existing and the cast is a no-op for any valid CSS input. Happy to defer. Would you like me to open a follow-up issue to track the invalid-combinator normalization, or would you prefer to handle that informally?
🧠 Learnings used
Learnt from: RobinMalfait
Repo: tailwindlabs/tailwindcss PR: 19981
File: packages/tailwindcss/src/utilities.ts:2230-2240
Timestamp: 2026-04-29T15:16:36.864Z
Learning: When reviewing Tailwind CSS v4 code in packages/tailwindcss/src/**, don’t require `var(--tw-*, <fallback>)` inline fallbacks for internal Tailwind CSS variables used in generated CSS declarations. Tailwind v4 relies on `property` (Properties & Values API) across the codebase, where the `initial-value` defined in `property` acts as the fallback. Therefore, bare `var(--tw-*)` without `var(--tw-foo, fallback)` is intentional and consistent with the repo’s pattern—flag only if the relevant `property`/`initial-value` support is missing or the usage is for non-`--tw-*` variables.
Learnt from: RobinMalfait
Repo: tailwindlabs/tailwindcss PR: 19981
File: packages/tailwindcss/src/utilities.ts:2230-2240
Timestamp: 2026-04-29T15:16:36.864Z
Learning: In the tailwindcss codebase, treat Tailwind’s documented browser minimum targets (Safari 16.4, Firefox 128, Chrome 111) as applying to core infrastructure (e.g., foundational color/layout behavior). When adding new utilities that rely on newer CSS features (e.g., scrollbar-related utilities like `scrollbar-color` and `scrollbar-width`), assume they are progressive enhancement: the utility does not need to work in all minimum-target browsers. If a browser lacks support for the underlying CSS property, it should be safely ignored (no layout/page breakage). During review, avoid enforcing hard expectations that these utilities fully work on every minimum-target browser; prefer additive behavior that degrades gracefully when unsupported.
Learnt from: WarningImHack3r
Repo: tailwindlabs/tailwindcss PR: 20028
File: packages/@tailwindcss-node/src/esm-cache.loader.mts:1-7
Timestamp: 2026-05-09T15:01:47.087Z
Learning: When wiring up Node’s ESM `module.registerHooks()` resolve hooks in TypeScript, you can reference the synchronous resolve-hook signature as `ResolveHookSync` imported from `node:module` (provided by `types/node`, for Node 24+). Import it using `import type { ResolveHookSync } from 'node:module'` so it is erased at compile time (no runtime dependency on Node supporting that hook type). Use the `ResolveHookSync` type only to type your hook function/signature; do not rely on it for any runtime behavior on older Node versions.
1 participant
This PR is a follow-up of #20088 to further improve selectors, in particular the
combinator.This PR explicitly types the
combinatoras:This allows us to explicitly test for this pattern in various places, without us having to call
.trim()first to know what the actual combinator was.In the selector parser itself, we already did a
.trim()to know whether we were dealing with a descendant combinator or not. With this PR, we further ensure that there is no whitespace involved aroudn these combinators.This introduces a small problem because we need to be able to re-print a selector's AST. So if we don't track whitespace, we have to re-introduce it. But there are situations where we don't want it at all (during canonicalization).
To solve this, we introduced a
minify = falseoption in theSelectorParser.toCss. If it's false (the default), then we introduce whitespace, otherwise we remove all whitespace.Test plan