fix: support local variable fonts with weight ranges#794
Conversation
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLocal font provider parsing now recognizes and normalizes weight ranges (e.g., "100-900") and prefers range extraction when generating slugs; single-weight extraction remains a fallback. Slug suffixes are sanitized by replacing spaces with dashes. Documentation was updated with an explicit variable-font filename example ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/providers/local.ts (1)
142-142:WEIGHT_RANGE_REmay false-match non-weight digit sequences in filenames.The regex
([1-9]00)-([1-9]00)has no boundary assertions, so a filename likeMyFont-2005-300.woff2(where200and300are substring matches within2005-300) would still match200)-（300— actually wait,2005-300would match500-300? Let me re-check:[1-9]00matches exactly 3 digits, and the regex engine will try every position. In2005-300, it would find200at position 0, then require-, but position 3 is5. It would then try005which fails[1-9]. Moving on,05-fails. So5-3fails. It would eventually not match2005-300.However, consider
font-v200-300dpi.woff2— the regex would match200-300within it, incorrectly treating it as a weight range. Adding word-boundary or non-digit assertions would make this more robust.Proposed hardening
-const WEIGHT_RANGE_RE = /(?<weightRange>([1-9]00)-([1-9]00))/i; +const WEIGHT_RANGE_RE = /(?<![0-9])(?<weightRange>([1-9]00)-([1-9]00))(?![0-9])/This uses lookbehind/lookahead to ensure the weight digits aren't part of a larger number.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/local.ts` at line 142, The current WEIGHT_RANGE_RE (const WEIGHT_RANGE_RE) can match weight-like substrings embedded inside larger numbers in filenames; update the regex to require non-digit boundaries around the weight range by adding a negative lookbehind (?<!\d) before and a negative lookahead (?!\d) after the named group (?<weightRange>([1-9]00)-([1-9]00)) so only standalone three-digit weight ranges like "200-300" are captured (adjust the pattern in src/providers/local.ts where WEIGHT_RANGE_RE is declared).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/1.get-started/4.providers.md`:
- Line 14: The documentation example for font filenames is incorrect: update the
example `comic-sans-ms-100-900.woff2 (100 900/italic/cyrillic)` to accurately
reflect defaults by changing the annotation to `comic-sans-ms-100-900.woff2 (100
900/normal/latin)` or alternatively change the filename to include explicit
suffixes like `comic-sans-ms-100-900-italic-cyrillic.woff2 (100
900/italic/cyrillic)` so the stated style/subset matches the filename; also fix
the grammar by replacing the phrase "will lookup" with "will look up" where the
behavior of `font-family` lookup is described.
In `@src/providers/local.ts`:
- Around line 38-40: The slug construction in src/providers/local.ts has
stylistic lint errors: remove the unnecessary parentheses around the single
arrow parameter in the map callback, fix indentation to align the map callback
and join call consistently (use 4 spaces per indent level), and remove the
unexpected trailing comma after the map array element. Locate the slug
assignment that uses fontFamilyToSlug(family) and suffixes.map(...) and update
the map callback to use a single unparenthesized parameter (e.g., suffix =>
...), adjust indentation to match surrounding code, and ensure no trailing comma
remains before the closing bracket/join.
- Line 142: Remove the unnecessary case-insensitive flag and the trailing
semicolon on the WEIGHT_RANGE_RE declaration: update the const WEIGHT_RANGE_RE
regex (symbol: WEIGHT_RANGE_RE) to omit the `i` flag (digits/hyphens are
case-invariant) and drop the semicolon to satisfy the project's no-semicolons
lint rule.
- Around line 161-170: Remove all stray semicolons and fix brace style in the
weight parsing block so the closing brace of the if sits on its own line and the
else begins on the next line; update the code around WEIGHT_RANGE_RE,
weightRange and weight to follow project style (no semicolons, proper newline
before else). Also eliminate the no-op replace call (.replace(/[\s._]+/, '-'))
or adjust WEIGHT_RANGE_RE to allow alternate separators if you intended to
accept spaces/dots/underscores, and ensure SINGLE_WEIGHT_RE usage remains
unchanged for the fallback.
In `@test/providers/local.test.ts`:
- Around line 158-192: The test has ESLint style issues: in
test/providers/local.test.ts update the promise then callback to use an
unparenthesized single parameter (change .then((r) => r.fonts) to .then(r =>
r.fonts)) and remove the trailing semicolon at the end of the expect assertion;
locate the call to provider.resolveFont in the test and adjust the .then
callback and final line terminator accordingly so the file conforms to project
lint rules.
---
Nitpick comments:
In `@src/providers/local.ts`:
- Line 142: The current WEIGHT_RANGE_RE (const WEIGHT_RANGE_RE) can match
weight-like substrings embedded inside larger numbers in filenames; update the
regex to require non-digit boundaries around the weight range by adding a
negative lookbehind (?<!\d) before and a negative lookahead (?!\d) after the
named group (?<weightRange>([1-9]00)-([1-9]00)) so only standalone three-digit
weight ranges like "200-300" are captured (adjust the pattern in
src/providers/local.ts where WEIGHT_RANGE_RE is declared).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/content/1.get-started/4.providers.mdsrc/providers/local.tstest/providers/local.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/content/1.get-started/4.providers.md (1)
14-14:⚠️ Potential issue | 🟡 Minor"will lookup" → "will look up"
The grammar issue flagged in the previous review remains unaddressed. "lookup" is a noun; the verb form is "look up".
✏️ Proposed fix
-`font-family` values with spaces and/or caps will lookup hyphenated lowercase filenames as well +`font-family` values with spaces and/or caps will look up hyphenated lowercase filenames as well🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/1.get-started/4.providers.md` at line 14, The sentence uses the incorrect verb "will lookup"; change the phrase "will lookup hyphenated lowercase filenames as well" to the correct verb form "will look up hyphenated lowercase filenames as well" in the paragraph describing filename lookup behavior (the sentence mentioning `font-family: 'Comic Sans MS'` and `comic-sans-ms.woff2`) so the documentation uses proper grammar.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/content/1.get-started/4.providers.md`:
- Line 14: The sentence uses the incorrect verb "will lookup"; change the phrase
"will lookup hyphenated lowercase filenames as well" to the correct verb form
"will look up hyphenated lowercase filenames as well" in the paragraph
describing filename lookup behavior (the sentence mentioning `font-family:
'Comic Sans MS'` and `comic-sans-ms.woff2`) so the documentation uses proper
grammar.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
+ Coverage 68.47% 69.30% +0.83%
==========================================
Files 9 9
Lines 295 303 +8
Branches 67 70 +3
==========================================
+ Hits 202 210 +8
Misses 75 75
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I feel like that coverage complaint is mostly on lines predating my additions. But I was a bit worried about support for weights like |
2 participants
🔗 Linked issue
❓ Type of change
📚 Description
Currently local variable fonts are not supported unless a
srcis supplied.Weights with ranges (e.g.
["100 900"]) and file names with rangesmy-variable-font-100-900.woff2are not supported. This means @nuxt/fonts can't connect the family configuration with the font files, which means it can't produce the font face declaration.The current workaround is manually adding a
srcto the family configuration.This PR fixes the font file detection when a variable weight range is specified.
It also expands unit tests and updates documentation.