Canonicalization: limit arbitrary to bare values conversion#20130
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis change adds MAX_BARE_VALUE_IN_PX (1536) and an isReasonableBareValue helper that resolves spacing multipliers (including rem folding) and rejects conversions whose computed pixel/rem magnitude exceeds the limit. The guard is applied to both spacing-derived bare-value replacement paths. Tests were added to ensure large arbitrary values like left-[5000px], left-[-5000px], calc(...) variants, and z-[9999999] remain as arbitrary candidates rather than being converted. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/canonicalize-candidates.ts (1)
1100-1101: ⚡ Quick winUse absolute pixel magnitude in the reasonableness check.
The guard is intended to cap magnitude, but current comparison uses signed pixels. Using
Math.abs(...)makes the helper semantics match the intent and prevents edge-case regressions if a negative multiplier reaches this helper.Suggested patch
- let bareValueInPixels = value * spacingValue - return bareValueInPixels <= MAX_BARE_VALUE_IN_PX + let bareValueInPixels = Math.abs(value * spacingValue) + return bareValueInPixels <= MAX_BARE_VALUE_IN_PX🤖 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/canonicalize-candidates.ts` around lines 1100 - 1101, The magnitude check currently compares signed bareValueInPixels which allows negative values to bypass the cap; update the guard in canonicalize-candidates.ts (the computation using bareValueInPixels = value * spacingValue and the return that compares against MAX_BARE_VALUE_IN_PX) to use the absolute pixel magnitude (e.g., compare Math.abs(bareValueInPixels) <= MAX_BARE_VALUE_IN_PX) so negative multipliers are correctly clamped.
🤖 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.
Nitpick comments:
In `@packages/tailwindcss/src/canonicalize-candidates.ts`:
- Around line 1100-1101: The magnitude check currently compares signed
bareValueInPixels which allows negative values to bypass the cap; update the
guard in canonicalize-candidates.ts (the computation using bareValueInPixels =
value * spacingValue and the return that compares against MAX_BARE_VALUE_IN_PX)
to use the absolute pixel magnitude (e.g., compare Math.abs(bareValueInPixels)
<= MAX_BARE_VALUE_IN_PX) so negative multipliers are correctly clamped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99ca0375-e99b-45c6-8e6e-36e0a842e067
📒 Files selected for processing (2)
packages/tailwindcss/src/canonicalize-candidates.test.tspackages/tailwindcss/src/canonicalize-candidates.ts
Confidence Score: 5/5Safe to merge — the change narrows an existing conversion path without affecting any other canonicalization logic. The new No files require special attention. Reviews (3): Last reviewed commit: "update changelog" | Re-trigger Greptile |
This PR improves the canonicalization process by limiting the bare values to a certain amount.
Before this PR, whenever we have an arbitrary value, e.g.
left-[6px], then we prefer to use a bare value instead e.g.left-1.5. In most cases, this makes sense.However, there are places where this doesn't really make sense (https://x.com/kettanaito/status/2059987396050268589)
left-[99999px]→left-24999.75The hard part is to figure out why this feels wrong. The
.75could feel wrong, but in theleft-[6px]→left-1.5, the.5makes sense.If we reduce that big number to
left-[99996px]→left-24999, then there is no floating point but it still feels wrong.One possibility I can think of is to analyze the incoming value and see if we find certain patterns. All repeating numbers, fun numbers like
1337, common numbers most programmers know such as720px,1280px, etc.But instead of that, I think it's more reasonable to limit the bare value such that the
pxbased value doesn't exceed a big number. We can improve the logic if there are other cases that don't really make sense.The biggest value we have in our default theme is
--breakpoint-2xl: 96rem, which is equivalent to1536px.So I think any bare value that results in a value
<= 1536pxshould probably be fine.In this case,
left-[99999px]would stay asleft-[99999px], butleft-[6px]is still converted toleft-1.5.Note: this is only happening for arbitrary values being converted to bare values if they use the
--spacingvariable internally.Values such as
z-[99999999999]will still be converted toz-99999999999, since the intent is still clear.Test plan