Skip to content

Fix null font options in legacy theme resolution - #20590

Closed
jiwoo1222 wants to merge 1 commit into
tailwindlabs:mainfrom
jiwoo1222:fix/null-font-theme-options
Closed

jiwoo1222 wants to merge 1 commit into
tailwindlabs:mainfrom
jiwoo1222:fix/null-font-theme-options

Conversation

@jiwoo1222

@jiwoo1222 jiwoo1222 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fix the null check in resolveThemeValue: typeof value[1] !== null always evaluates to true, so null is incorrectly treated as a font-options object.

If a JS config extends an existing default sans or mono font with a null second array element, resolving the default font settings throws while reading fontFeatureSettings. Check the value itself before accessing its properties.

Add regression tests for both font namespaces. This is defensive handling of malformed options, not a new config format.

Test plan

  • The two regression cases fail with the original implementation and pass with this fix.
  • pnpm exec vitest run --project tailwindcss packages/tailwindcss/src/compat/apply-config-to-theme.test.ts — 9 passed.
  • Prettier check on both changed files — passed.
  • pnpm exec vitest run --project tailwindcss — 5,143 passed, 3 failed, 1 skipped. The same three snapshot failures reproduce on unmodified upstream f7f47f8 (CSS mixins, rounded utilities, and theme string indexing), which has 5,141 passing tests.
  • Package typecheck does not pass in this partial workspace install due to missing workspace dependencies and cross-project type errors. Rust tests and the full monorepo build have not been run.

@jiwoo1222
jiwoo1222 marked this pull request as ready for review October 19, 2026 01:24
@jiwoo1222
jiwoo1222 requested a review from a team as a code owner October 19, 2026 01:24
@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 608f7375-842a-4e17-9841-41b7f3df87b7
📥 Commits

Reviewing files that changed from the base of the PR and between f7f47f8 and 3de46c5.

📒 Files selected for processing: 2
  • packages/tailwindcss/src/compat/apply-config-to-theme.test.ts
  • packages/tailwindcss/src/compat/apply-config-to-theme.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Walkthrough

The theme resolver now checks whether a tuple’s second element is non-null. Regression tests cover sans and mono font configurations with a null option and verify that applying the configuration does not throw.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 3de46

Null font-option tuples are handled defensively, with regression tests for both font namespaces. No actionable merge-blocking risk is identified.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the main change: fixing null font options during legacy theme resolution.
Description check Passed The description directly explains the bug, the implementation fix, the regression tests, and the reported test results.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Reviews (1) · Last reviewed commit: "Fix null font options in legacy theme re..." · Reviewed by Greptile

@officialmail5531-alt

This comment was marked as spam.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@officialmail5531-alt, you quoted the review summary but did not include a request. What would you like me to do?

You are interacting with an AI system.

@officialmail5531-alt officialmail5531-alt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aprove

@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@RobinMalfait, your comment contains only the quoted review summary. What would you like me to do?

You are interacting with an AI system.

@RobinMalfait

Copy link
Copy Markdown
Contributor

Thanks for the PR!

You are right that typeof value[1] !== null is always true. But ['Inter', null] is not a valid fontFamily value, and Tailwind CSS v3 also renders it as invalid CSS (font-family: Inter,;).

With this change, the build does not throw anymore, but the output is still wrong: --default-font-family becomes Inter, (with a trailing comma), and the font-sans utility ignores the config value. So the change replaces an error with invalid CSS, for a config that is already invalid.

Because of this, I am going to close this PR.

Sent by an AI assistant on behalf of @RobinMalfait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants