Don't emit utilities that resolve a theme value when an unsupported modifier is used - #20419
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously reported changelog placeholder has been replaced with the correct PR link. Reviews (5): Last reviewed commit: "update CHANGELOG" | Re-trigger Greptile |
WalkthroughFunctional utilities now reject unsupported modifiers on resolved theme values unless the value is a fraction. Stroke-width resolution applies the same validation. Drop-shadow, text-shadow, shadow, and inset-shadow utilities reject invalid opacity modifiers across default, arbitrary, named, and theme-backed values. Tests cover these cases, and the changelog documents the fix. Merge Risk: ⚪ Minimal · up to The change prevents unsupported modifiers from emitting utilities. One arbitrary text-shadow case could use an additional regression test, but no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7681f73-63e3-4892-ad83-9c6455b8d6d7
📒 Files selected for processing (3)
CHANGELOG.mdpackages/tailwindcss/src/utilities.test.tspackages/tailwindcss/src/utilities.ts
e58dd03 to
d979c6d
Compare
…orted modifier is used Modifiers on functional utilities that resolve a named theme value (e.g. `rounded-sm/[5]`) were silently dropped instead of invalidating the candidate, unlike every sibling code path which rejects them. The same applied to the shadow family (`shadow-sm/foo`, `inset-shadow-sm/foo`, `text-shadow/foo`, `drop-shadow/foo`), where the `if (candidate.modifier && !alpha) return` guard existed only in `drop-shadow`'s named-size and arbitrary-value branches.
d979c6d to
0f5aa7a
Compare
RobinMalfait
left a comment
There was a problem hiding this comment.
Thanks!
I updated the tests to make them part of the existing section where we expect the tests to fail.
I also added a missing case for the stroke-* utilities, because stroke-2/foo would also compiler otherwise.
Last but not least, I also split up your commit, just so I could easily verify that the tests were indeed failing before applying the fix.
Thanks again!
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 537d3b07-3e72-41f4-99c0-e4cbb8a3d5fb
📒 Files selected for processing (3)
CHANGELOG.mdpackages/tailwindcss/src/utilities.test.tspackages/tailwindcss/src/utilities.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- packages/tailwindcss/src/utilities.ts
| '-text-shadow-[var(--value)]', | ||
| 'text-shadow/foo', | ||
| 'text-shadow-sm/foo', | ||
| ], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test an arbitrary text-shadow value with a slash modifier.
-text-shadow-[var(--value)] only tests negation. It does not execute the arbitrary-value branch with candidate.modifier set. Add text-shadow-[var(--value)]/foo so this test fails if unsupported modifiers are accepted again.
Proposed test case
'-text-shadow-[var(--value)]',
'text-shadow/foo',
'text-shadow-sm/foo',
+ 'text-shadow-[var(--value)]/foo',📝 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.
| '-text-shadow-[var(--value)]', | |
| 'text-shadow/foo', | |
| 'text-shadow-sm/foo', | |
| ], | |
| '-text-shadow-[var(--value)]', | |
| 'text-shadow/foo', | |
| 'text-shadow-sm/foo', | |
| 'text-shadow-[var(--value)]/foo', | |
| ], |
|
Thank you @RobinMalfait for the quick review and merge, and for tidying up the test organization noted for next time. This was my first contribution to Tailwind CSS and the experience was great. |
Summary
Modifiers on functional utilities that resolve a named theme value are silently
dropped instead of invalidating the candidate. For example, with the default theme:
rounded-sm/[5]emits the same CSS asrounded-sm(the[5]is ignored)shadow-sm/foo,inset-shadow-sm/foo,text-shadow/foo, anddrop-shadow/fooemit the full shadow CSS with the invalid
foomodifier ignoredThis is inconsistent with how every sibling code path behaves:
rounded/foo,rounded-[4px]/foo,rounded-sm/5, anddrop-shadow-xl/fooall correctlyproduce no output, because those paths check
candidate.modifier.This PR adds the missing guards:
functionalUtilityhandler, a candidate whose named valueresolves from the theme now rejects modifiers (except fractions like
w-1/2,where the modifier is part of the resolved value).
shadow,inset-shadow, andtext-shadowutilities now apply the sameif (candidate.modifier && !alpha) returnguard in their default-value,arbitrary-value, and named-size branches that
drop-shadowalready applies,and
drop-shadowgets it in its default-value branch too.Existing tests asserting candidates like
drop-shadow/fooproduce no output werepassing for the wrong reason: they run without a theme, so the theme lookup fails
before the modifier is ever considered. The new tests provide a theme so the
invalid modifier is what invalidates the candidate.
Test plan
rounded,filter,shadow,inset-shadow, andtext-shadowtests that compile candidates with invalid modifiers against atheme that defines the relevant values, and expect no output. All of them fail
without the fix.
pnpm vitest run packages/tailwindcss/src/utilities.test.ts— 398 passed.