Conversation
WalkthroughMigrates font handling to the google_fonts package: adds dependency and re-exports it, switches default text styles to GoogleFonts.geist/geistMono, makes theme Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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. 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 |
Deploying shadcn-ui-docs with
|
| Latest commit: |
972b853
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d3c0fce5.flutter-shadcn-ui.pages.dev |
| Branch Preview URL: | https://refactor-use-geist-from-goog.flutter-shadcn-ui.pages.dev |
Deploying shadcn-ui-playground with
|
| Latest commit: |
e852a1e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://50b60137.shadcn-ui-playground.pages.dev |
| Branch Preview URL: | https://refactor-use-geist-from-goog.shadcn-ui-playground.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/src/content/docs/packages.md`:
- Around line 35-38: Change the package entry heading "## [google_fonts](...)"
to a level-3 heading (prefix with ###) so it matches the other package entries,
and fix the description sentence by capitalizing "Google" in "fonts.google.com"
(e.g., "any of the fonts from fonts.google.com" → "any of the fonts from Google
Fonts (fonts.google.com)" or at minimum capitalize "Google"). Ensure you update
the heading token "## [google_fonts]" and the description line that mentions
"fonts.google.com".
In `@example/lib/pages/typography.dart`:
- Line 10: The getter myCustomStyle currently returns a nullable TextStyle?
(TextStyle? get myCustomStyle => custom['myCustomStyle'];) but is used without
null-check in the Text widget; change the getter to return a non-nullable
TextStyle by force-unwrapping the map value (TextStyle get myCustomStyle =>
custom['myCustomStyle']!;) so callers like
ShadTheme.of(context).textTheme.myCustomStyle can pass it directly to
Text(style: ...) without nullable handling.
In `@lib/src/theme/text_theme/theme.dart`:
- Around line 200-210: The code currently prefixes package even when family is
null, producing 'packages/<pkg>/null'; modify the logic around effectiveFamily
and package so prefixing only happens when a non-null, non-empty family is
present: keep the existing googleFontBuilder nullification (googleFontBuilder,
family) but change the package prefix block to check effectiveFamily != null &&
effectiveFamily != '' (or equivalent) before setting effectiveFamily =
'packages/$package/$effectiveFamily'; reference symbols: googleFontBuilder,
family, package, effectiveFamily.
In `@pubspec.yaml`:
- Line 26: Update the Dart SDK constraint in the pubspec environment to match
google_fonts 7.1.0 requirements: locate the environment sdk entry (currently
allowing ">=3.8.0") and change it to enforce ">=3.9.0 <4.0.0" so the project
Dart SDK minimum meets google_fonts 7.1.0's requirement; ensure the env block's
sdk string is updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.pubignore:
- Line 11: Remove the entry "test/fonts" from .pubignore or alternatively remove
the matching asset and font declarations from pubspec.yaml (the flutter.assets
and flutter.fonts sections) so published packages don’t reference assets that
are excluded; specifically either delete "test/fonts" from .pubignore to ensure
the font files declared in pubspec.yaml are included in the package, or remove
the test font asset lines under flutter.assets and the corresponding
flutter.fonts entries in pubspec.yaml so no missing-asset references remain.
♻️ Duplicate comments (2)
pubspec.yaml (1)
26-26: Align Dart SDK constraint with google_fonts requirements.If
google_fonts ^7.1.0requires Dart ≥ 3.9, the currentsdk: ">=3.8.0 <4.0.0"allows unsupported versions and will fail pub resolution for users on 3.8.x. Please confirm the dependency’s SDK constraint and bump the project’s minimum Dart version if needed.🔧 Proposed fix (if google_fonts requires Dart ≥ 3.9)
environment: - sdk: ">=3.8.0 <4.0.0" + sdk: ">=3.9.0 <4.0.0" flutter: ">=3.35.0"google_fonts 7.1.0 pubspec environment sdk constraintlib/src/theme/text_theme/theme.dart (1)
203-209: Avoid prefixing package whenfamilyis null.
effectiveFamily != ''is true wheneffectiveFamilyis null, which yieldspackages/<pkg>/null. Guard against null or empty before prefixing.🐛 Proposed fix
- if (package != null && effectiveFamily != '') { + if (package != null && + package.isNotEmpty && + (effectiveFamily?.isNotEmpty ?? false)) { effectiveFamily = 'packages/$package/$effectiveFamily'; }
🧹 Nitpick comments (1)
lib/src/theme/text_theme/theme.dart (1)
258-287: ApplyfontFamilytopstyle for consistency.All other styles copy the resolved
fontFamily, butpdoesn’t. If the defaultpstyle has no weight/style, it may keep a null family. Consider applying the same copyWith as the others.♻️ Suggested tweak
- final p = GoogleFontTextStyle( + final p = GoogleFontTextStyle( effectiveTextTheme.p.omitFamilyAndPackage, builder: fontBuilder, - ); + ).copyWith(fontFamily: fontFamily);
| web | ||
| linux | ||
| fonts.zip | ||
| test/fonts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and display pubspec.yaml content
find . -name "pubspec.yaml" -type f | head -5Repository: nank1ro/flutter-shadcn-ui
Length of output: 151
🏁 Script executed:
# Also check .pubignore file
find . -name ".pubignore" -type fRepository: nank1ro/flutter-shadcn-ui
Length of output: 81
🏁 Script executed:
cat -n ./pubspec.yamlRepository: nank1ro/flutter-shadcn-ui
Length of output: 3149
🏁 Script executed:
cat -n ./.pubignoreRepository: nank1ro/flutter-shadcn-ui
Length of output: 220
Don't ignore assets that are declared in pubspec.yaml.
test/fonts is listed under flutter.assets and the font files are declared in flutter.fonts in pubspec.yaml (lines 45-87). Adding it to .pubignore will exclude these font files from the published package, but the font declarations will remain. This causes missing-asset failures for consumers at runtime.
Either keep the fonts in the published package or remove both the asset declarations and font definitions from pubspec.yaml if they're only for internal testing.
💡 Proposed fix (keep fonts published)
- test/fonts📝 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.
| test/fonts |
🤖 Prompt for AI Agents
In @.pubignore at line 11, Remove the entry "test/fonts" from .pubignore or
alternatively remove the matching asset and font declarations from pubspec.yaml
(the flutter.assets and flutter.fonts sections) so published packages don’t
reference assets that are excluded; specifically either delete "test/fonts" from
.pubignore to ensure the font files declared in pubspec.yaml are included in the
package, or remove the test font asset lines under flutter.assets and the
corresponding flutter.fonts entries in pubspec.yaml so no missing-asset
references remain.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pubspec.yaml`:
- Around line 47-87: pubspec.yaml currently declares font families "Geist" and
"GeistMono" with assets under test/fonts/, which are excluded from publication
and will point to missing files in published packages; remove the entire fonts
block for the "Geist" and "GeistMono" declarations (the entries under fonts:
referencing test/fonts/*) so published packages no longer reference non-existent
assets, or alternatively stop excluding test/fonts in .pubignore if you
intentionally want to publish them (recommended: delete the font family
declarations).
| fonts: | ||
| - family: Geist | ||
| fonts: | ||
| - asset: fonts/Geist-Thin.otf | ||
| - asset: test/fonts/Geist-Thin.otf | ||
| weight: 100 | ||
| - asset: fonts/Geist-UltraLight.otf | ||
| - asset: test/fonts/Geist-UltraLight.otf | ||
| weight: 200 | ||
| - asset: fonts/Geist-Light.otf | ||
| - asset: test/fonts/Geist-Light.otf | ||
| weight: 300 | ||
| - asset: fonts/Geist-Regular.otf | ||
| - asset: test/fonts/Geist-Regular.otf | ||
| weight: 400 | ||
| - asset: fonts/Geist-Medium.otf | ||
| - asset: test/fonts/Geist-Medium.otf | ||
| weight: 500 | ||
| - asset: fonts/Geist-SemiBold.otf | ||
| - asset: test/fonts/Geist-SemiBold.otf | ||
| weight: 600 | ||
| - asset: fonts/Geist-Bold.otf | ||
| - asset: test/fonts/Geist-Bold.otf | ||
| weight: 700 | ||
| - asset: fonts/Geist-Black.otf | ||
| - asset: test/fonts/Geist-Black.otf | ||
| weight: 800 | ||
| - asset: fonts/Geist-UltraBlack.otf | ||
| - asset: test/fonts/Geist-UltraBlack.otf | ||
| weight: 900 | ||
| - family: GeistMono | ||
| fonts: | ||
| - asset: fonts/GeistMono-Thin.otf | ||
| - asset: test/fonts/GeistMono-Thin.otf | ||
| weight: 100 | ||
| - asset: fonts/GeistMono-UltraLight.otf | ||
| - asset: test/fonts/GeistMono-UltraLight.otf | ||
| weight: 200 | ||
| - asset: fonts/GeistMono-Light.otf | ||
| - asset: test/fonts/GeistMono-Light.otf | ||
| weight: 300 | ||
| - asset: fonts/GeistMono-Regular.otf | ||
| - asset: test/fonts/GeistMono-Regular.otf | ||
| weight: 400 | ||
| - asset: fonts/GeistMono-Medium.otf | ||
| - asset: test/fonts/GeistMono-Medium.otf | ||
| weight: 500 | ||
| - asset: fonts/GeistMono-SemiBold.otf | ||
| - asset: test/fonts/GeistMono-SemiBold.otf | ||
| weight: 600 | ||
| - asset: fonts/GeistMono-Bold.otf | ||
| - asset: test/fonts/GeistMono-Bold.otf | ||
| weight: 700 | ||
| - asset: fonts/GeistMono-Black.otf | ||
| - asset: test/fonts/GeistMono-Black.otf | ||
| weight: 800 | ||
| - asset: fonts/GeistMono-UltraBlack.otf | ||
| - asset: test/fonts/GeistMono-UltraBlack.otf | ||
| weight: 900 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if .pubignore exists and whether test/ is excluded
echo "=== Checking .pubignore ==="
if [ -f ".pubignore" ]; then
cat .pubignore
else
echo ".pubignore not found"
fi
echo ""
echo "=== Checking .gitignore for test/ exclusion ==="
if [ -f ".gitignore" ]; then
grep -E "^test/?|test/" .gitignore || echo "test/ not excluded in .gitignore"
else
echo ".gitignore not found"
fi
echo ""
echo "=== Checking if font files exist in test/fonts/ ==="
if [ -d "test/fonts" ]; then
echo "test/fonts/ directory exists"
find test/fonts -type f \( -name "*.otf" -o -name "*.ttf" \) | wc -l | xargs echo "Font file count:"
find test/fonts -type f \( -name "*.otf" -o -name "*.ttf" \) | head -5
else
echo "test/fonts/ directory does not exist"
fi
echo ""
echo "=== Checking pubspec.yaml structure ==="
head -50 pubspec.yamlRepository: nank1ro/flutter-shadcn-ui
Length of output: 1761
Remove font declarations from pubspec.yaml or keep test/fonts in published package.
The test/fonts/ directory is already excluded from pub.dev publication via .pubignore, but pubspec.yaml still declares these fonts with assets pointing to test/fonts/. This creates a mismatch: published packages will have font declarations referencing non-existent files, potentially causing Flutter warnings during initialization.
Since production uses GoogleFonts.geist() for runtime fetching, the bundled fonts in pubspec.yaml (lines 47–87) are only needed for tests. Either:
- Recommended: Remove the font family declarations from
pubspec.yaml(they're unnecessary for published packages), or - Remove
test/fontsfrom.pubignoreif the fonts should be included in the published package (not recommended due to ~1–2MB added size).
🤖 Prompt for AI Agents
In `@pubspec.yaml` around lines 47 - 87, pubspec.yaml currently declares font
families "Geist" and "GeistMono" with assets under test/fonts/, which are
excluded from publication and will point to missing files in published packages;
remove the entire fonts block for the "Geist" and "GeistMono" declarations (the
entries under fonts: referencing test/fonts/*) so published packages no longer
reference non-existent assets, or alternatively stop excluding test/fonts in
.pubignore if you intentionally want to publish them (recommended: delete the
font family declarations).
2 participants
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
Testing this PR
To try this branch, add the following to your
pubspec.yaml:Pre-launch Checklist
///).0.18.0and you introduced a breaking change or a new feature, bump it to0.19.0, if you just added a fix or a chore bump it to0.18.1.CHANGELOG.mdfile with a summary of changes made following the format already used.If you need help, consider asking for advice on Discord.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.