fix: complete Google Font setup in brand panel #2867
Conversation
The "Add Google Font" flow was broken - fonts added from the brand panel would apply font-* classes but wouldn't render because the necessary Tailwind config and layout setup was missing. Fixed addFont method to automatically: - Update tailwind.config.ts with fontFamily extension - Add font CSS variable to root layout - Ensure all config files exist Now adding fonts like "Lora" or "Open Sans" works end-to-end: - Brand panel → add font → immediate availability in editor toolbar - Toolbar → select font → applies correctly with proper rendering - Handles fonts with spaces, numbers, and special characters - Idempotent operations prevent duplicate config 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Rate limit exceeded@drfarrell has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughFont addition/removal now validates an active sandbox, ensures font and Tailwind config files exist, updates internal state and search index, updates Tailwind config and root layout, and triggers a full views reload after additions, removals, and synchronization changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Editor UI
participant Store as FontStore.addFont()
participant SB as SandboxManager
participant CFG as ensureConfigFilesExist()
participant FCM as fontConfigManager
participant FSM as fontSearchManager
participant TW as addFontToTailwindConfig()
participant LAY as layoutManager
participant VIEWS as reloadAllViews()
User->>UI: Select/Add Font
UI->>Store: addFont(font)
Store->>SB: getActiveSandbox()
alt No active sandbox
Store-->>UI: return false (logs error)
else Sandbox available
Store->>CFG: ensureConfigFilesExist(sandbox)
Store->>FCM: addFont(font, sandbox)
alt Font added successfully
Store->>FSM: loadBatch([font])
Store->>TW: addFontToTailwindConfig(font, sandbox)
Store->>LAY: addFontVariableToRootLayout(font.id)
Store->>VIEWS: reloadAllViews()
Store-->>UI: success (state updated)
else Add failed
Store-->>UI: return false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
165-183: Make addFont’s success reflect full E2E completion (Tailwind + layout).Both addFontToTailwindConfig(...) and LayoutManager.addFontVariableToRootLayout(...) return Promise; only return true when both succeed and avoid duplicate _fonts entries. Apply to apps/web/client/src/components/store/editor/font/index.ts (around lines ~165–183 and ~392–397). Verified: addFontToTailwindConfig is Promise (tailwind-config.ts) and addFontVariableToRootLayout is Promise (layout-manager.ts).
const success = await this.fontConfigManager.addFont(font); if (success) { // Update the fonts array - this._fonts.push(font); + if (!this._fonts.some((f) => f.id === font.id)) { + this._fonts.push(font); + } // Update font search manager with current fonts this.fontSearchManager.updateFontsList(this._fonts); // Load the new font in the search manager await this.fontSearchManager.loadFontFromBatch([font]); - // Add font to Tailwind config - await addFontToTailwindConfig(font, sandbox); - - // Add font variable to root layout - await this.layoutManager.addFontVariableToRootLayout(font.id); - - return true; + // Add font to Tailwind config + const twOk = await addFontToTailwindConfig(font, sandbox); + // Add font variable to root layout (may be void/CodeDiff/bool) + const layoutResult: unknown = await this.layoutManager.addFontVariableToRootLayout(font.id); + const layoutOk = layoutResult !== false; + + if (!twOk || !layoutOk) { + console.error('Font added to config, but Tailwind/layout update failed', { + tailwind: twOk, + layout: layoutOk, + fontId: font.id, + }); + return false; + } + return true; }
🧹 Nitpick comments (4)
apps/web/client/src/components/store/editor/font/index.ts (4)
168-175: Keep local state idempotent.Guarding the push avoids duplicate entries when re-adding the same font.
Apply this minimal change if you don't adopt the larger diff above:
- this._fonts.push(font); + if (!this._fonts.some((f) => f.id === font.id)) { + this._fonts.push(font); + }
197-214: Remove flow should update Tailwind and root layout immediately (symmetry).Today removals are eventually handled by syncFontsWithConfigs; doing it here gives instant feedback and reduces reliance on file events.
Apply:
async removeFont(font: Font): Promise<CodeDiff | false> { try { - const result = await this.fontConfigManager.removeFont(font); + const result = await this.fontConfigManager.removeFont(font); if (result) { + const sandbox = this.editorEngine.activeSandbox; + if (sandbox) { + await removeFontFromTailwindConfig(font, sandbox); + } else { + console.error('No sandbox session found during removeFont'); + } + await this.layoutManager.removeFontVariableFromRootLayout(font.id); + // Remove from fonts array this._fonts = this._fonts.filter((f) => f.id !== font.id);
387-399: Batch add/remove ops to reduce latency.Sequential awaits can be slow with many fonts. Parallelize with Promise.allSettled.
Apply:
- if (removedFonts.length > 0) { - for (const font of removedFonts) { - await removeFontFromTailwindConfig(font, sandbox); - await this.layoutManager.removeFontVariableFromRootLayout(font.id); - } - } + if (removedFonts.length > 0) { + await Promise.allSettled([ + ...removedFonts.map((font) => removeFontFromTailwindConfig(font, sandbox)), + ...removedFonts.map((font) => this.layoutManager.removeFontVariableFromRootLayout(font.id)), + ]); + } - if (addedFonts.length > 0) { - for (const font of addedFonts) { - await addFontToTailwindConfig(font, sandbox); - await this.layoutManager.addFontVariableToRootLayout(font.id); - } - } + if (addedFonts.length > 0) { + await Promise.allSettled([ + ...addedFonts.map((font) => addFontToTailwindConfig(font, sandbox)), + ...addedFonts.map((font) => this.layoutManager.addFontVariableToRootLayout(font.id)), + ]); + }
91-93: File-event path matching: prefer exact/normalized match to avoid false positives.includes() can match unrelated paths; endsWith() on normalized paths is safer.
Example:
- if (!paths.some((path) => path.includes(fontConfigPath))) { + const norm = (p: string) => p.replace(/\\/g, '/'); + if (!paths.some((p) => norm(p).endsWith(norm(fontConfigPath)))) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/components/store/editor/font/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/font/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/font/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/font/index.ts
🧬 Code graph analysis (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
apps/web/client/src/components/store/editor/font/tailwind-config.ts (1)
addFontToTailwindConfig(39-62)
🔇 Additional comments (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
157-164: Good: early sandbox guard + config bootstrap.Prevents partial writes and clarifies failure modes before mutating state.
After adding/removing fonts, the webview would freeze because new font classes and CSS variables weren't available until manual refresh. Fixed by adding editorEngine.frames.reloadAllViews() calls to: - addFont() - Refreshes after adding new fonts - removeFont() - Refreshes after removing fonts - syncFontsWithConfigs() - Refreshes after automatic font syncing Now users can immediately select elements and apply fonts after adding them from the brand panel. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Font IDs containing hyphens (like "open-sans") were generating invalid JavaScript object keys causing syntax errors in tailwind.config.js. Fixed by using smart key generation: - Valid identifiers (like "inter") → use as-is for clean output - Invalid identifiers (like "open-sans") → wrap in quotes for validity This prevents build errors when adding Google Fonts with spaces or hyphens in their names. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
200-219: Fix double-reload & stop persisting FontConfigManager CodeDiffFontConfigManager.removeFont already writes the font config and deletes font files (apps/web/client/src/components/store/editor/font/font-config-manager.ts — its removeFont writes the file and returns a CodeDiff). FontManager.removeFont currently removes the font from the in-memory list and immediately calls editorEngine.frames.reloadAllViews(), but Tailwind/layout updates are performed later by syncFontsWithConfigs (file watcher) — this causes flicker / a second reload.
- Remove attempting to persist/write the returned CodeDiff in FontManager (font-config-manager already writes it).
- Resolve the double-reload by either:
- Immediately applying Tailwind + layout updates inside FontManager.removeFont (await removeFontFromTailwindConfig(font, sandbox); await this.layoutManager.removeFontVariableFromRootLayout(font.id)) and then call a single editorEngine.frames.reloadAllViews(), or
- Remove the immediate reload and rely on syncFontsWithConfigs to update + reload (less responsive to the user).
- Files/locations: apps/web/client/src/components/store/editor/font/index.ts (removeFont ~lines 200–219 and syncFontsWithConfigs ~lines 375–403), apps/web/client/src/components/store/editor/font/font-config-manager.ts (removeFont implementation).
🧹 Nitpick comments (3)
apps/web/client/src/components/store/editor/font/index.ts (3)
163-164: Minor: redundant sandbox validation.You check for sandbox here and again inside ensureConfigFilesExist(). Consolidate to a single place to reduce noise.
411-414: Debounce/coalesce view reload during sync to prevent duplicate refresh.The watcher-triggered sync will often follow add/remove, causing a second reload. Use the same scheduleReload() helper.
Apply this diff:
- // Reload all views to apply font changes - this.editorEngine.frames.reloadAllViews(); + // Reload once even if multiple changes occurred + this.scheduleReload();
425-436: Propagate ensure failures to callers.ensureConfigFilesExist() logs and returns void, so callers proceed even if creation fails. Return a boolean (or throw) and gate addFont/upload flows.
If both ensure* helpers return booleans, consider:
private async ensureConfigFilesExist(): Promise<boolean> { const sandbox = this.editorEngine.activeSandbox; if (!sandbox) { console.error('No sandbox session found'); return false; } const [fontOk, twOk] = await Promise.all([ this.fontConfigManager.ensureFontConfigFileExists(), ensureTailwindConfigExists(sandbox), ]); return Boolean(fontOk && twOk); }Then check the return in addFont()/uploadFonts() and bail out early on false.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/components/store/editor/font/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/font/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/font/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/font/index.ts
🧬 Code graph analysis (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
apps/web/client/src/components/store/editor/font/tailwind-config.ts (1)
addFontToTailwindConfig(39-62)
🔇 Additional comments (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
157-162: Good early guard for missing sandbox.The null-check prevents downstream FS calls from blowing up.
| // Add font to Tailwind config | ||
| await addFontToTailwindConfig(font, sandbox); | ||
|
|
||
| // Add font variable to root layout | ||
| await this.layoutManager.addFontVariableToRootLayout(font.id); | ||
|
|
||
| // Reload all views to apply new font styles | ||
| this.editorEngine.frames.reloadAllViews(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Handle failures from Tailwind/layout updates and avoid inconsistent state; also coalesce reloads.
addFontToTailwindConfig returns boolean but the result is ignored. If this step fails, you’ll push state, load font, and reload without Tailwind mapping—font won’t render until a later sync (and may never self-heal). Also, this triggers one reload here and likely another from the file watcher.
Suggested fix: fail fast if Tailwind update fails, and switch to a scheduled (coalesced) reload.
Apply this diff in-place:
- // Add font to Tailwind config
- await addFontToTailwindConfig(font, sandbox);
+ // Add font to Tailwind config; abort on failure to avoid inconsistent state
+ const tailwindUpdated = await addFontToTailwindConfig(font, sandbox);
+ if (!tailwindUpdated) {
+ console.error('Failed to update Tailwind config for font:', font.id);
+ return false;
+ }
- // Add font variable to root layout
- await this.layoutManager.addFontVariableToRootLayout(font.id);
+ // Add font variable to root layout (idempotent)
+ await this.layoutManager.addFontVariableToRootLayout(font.id);
- // Reload all views to apply new font styles
- this.editorEngine.frames.reloadAllViews();
+ // Coalesce reloads to avoid double refresh with file watcher
+ this.scheduleReload();Add the helper once in the class (outside the shown range):
// Helper: coalesce multiple reloads in the same tick
private reloadScheduled = false;
private scheduleReload(): void {
if (this.reloadScheduled) return;
this.reloadScheduled = true;
queueMicrotask(() => {
this.reloadScheduled = false;
this.editorEngine.frames.reloadAllViews();
});
}Is LayoutManager.addFontVariableToRootLayout(id) guaranteed to be idempotent and/or return a boolean? If it returns a boolean, also guard and log on failure.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- rg availability ----"
rg --version || true
echo "Searching for addFontToTailwindConfig..."
rg -n --hidden -S "addFontToTailwindConfig" -g '!*node_modules/*' -C3 || true
echo "Searching for addFontVariableToRootLayout..."
rg -n --hidden -S "addFontVariableToRootLayout" -g '!*node_modules/*' -C3 || true
echo "Searching for addFontVariable (broader)..."
rg -n --hidden -S "addFontVariable" -g '!*node_modules/*' -C3 || true
echo "Searching for LayoutManager class or interface..."
rg -n --hidden -S "class .*LayoutManager|interface .*LayoutManager" -g '!*node_modules/*' -C3 || true
echo "Searching for scheduleReload/reloadScheduled/scheduleReload"
rg -n --hidden -S "scheduleReload|reloadScheduled|reloadScheduled" -g '!*node_modules/*' -C3 || true
echo "Printing apps/web/client/src/components/store/editor/font/index.ts (if present)"
if [ -f "apps/web/client/src/components/store/editor/font/index.ts" ]; then
sed -n '1,320p' apps/web/client/src/components/store/editor/font/index.ts
else
echo "File not found: apps/web/client/src/components/store/editor/font/index.ts"
fi
# If any files define addFontToTailwindConfig or addFontVariableToRootLayout, print them
files=$(rg -n --hidden -S -l "addFontToTailwindConfig" -g '!*node_modules/*' || true)
if [ -n "$files" ]; then
for f in $files; do
echo "==== FOUND addFontToTailwindConfig IN: $f ===="
sed -n '1,240p' "$f"
done
else
echo "No files define addFontToTailwindConfig"
fi
files2=$(rg -n --hidden -S -l "addFontVariableToRootLayout" -g '!*node_modules/*' || true)
if [ -n "$files2" ]; then
for f in $files2; do
echo "==== FOUND addFontVariableToRootLayout IN: $f ===="
sed -n '1,240p' "$f"
done
else
echo "No files define addFontVariableToRootLayout"
fi
# Also print any LayoutManager file matches
lm=$(rg -n --hidden -S -g '!*node_modules/*' "class .*LayoutManager|interface .*LayoutManager" || true)
if [ -n "$lm" ]; then
echo "==== LayoutManager matches ===="
echo "$lm"
echo ""
# print the files referenced
for f in $(echo "$lm" | awk -F: '{print $1}' | sort -u); do
echo "---- $f ----"
sed -n '1,240p' "$f"
done
else
echo "No obvious LayoutManager class/interface matches"
fiLength of output: 11920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd /home/jailuser/git || exit 1
DIR="apps/web/client/src/components/store/editor/font"
echo "PWD: $(pwd)"
echo "Checking directory: $DIR"
if [ -d "$DIR" ]; then
ls -la "$DIR"
else
echo "Directory not found: $DIR"
fi
for fname in tailwind-config.ts tailwind-config.tsx layout-manager.ts layout-manager.tsx layout-manager.js; do
fpath="$DIR/$fname"
echo
echo "---- $fpath ----"
if [ -f "$fpath" ]; then
sed -n '1,240p' "$fpath"
else
echo "File not found: $fpath"
fi
done
echo
echo "Searching repository for addFontToTailwindConfig (avoid rg filters)"
find . -type f \( -name '*.ts' -o -name '*.tsx' -o -name '*.js' -o -name '*.jsx' \) -not -path './node_modules/*' -print0 | xargs -0 grep -n --line-number --color=never "addFontToTailwindConfig" || true
echo
echo "Searching repository for addFontVariableToRootLayout"
find . -type f \( -name '*.ts' -o -name '*.tsx' -o -name '*.js' -o -name '*.jsx' \) -not -path './node_modules/*' -print0 | xargs -0 grep -n --line-number --color=never "addFontVariableToRootLayout" || true
echo
echo "Searching for LayoutManager class or file references"
find . -type f \( -name '*.ts' -o -name '*.tsx' -o -name '*.js' \) -not -path './node_modules/*' -print0 | xargs -0 grep -n --line-number --color=never -E "class +LayoutManager|interface +LayoutManager|export +class +LayoutManager" || trueLength of output: 14016
Fail fast on Tailwind update, guard layout update, and coalesce reloads.
addFontToTailwindConfig(...) returns Promise and can fail; LayoutManager.addFontVariableToRootLayout(...) returns Promise (idempotent) but its result is ignored. Abort on Tailwind failure, guard/log layout failure, and replace immediate reload with a coalesced reload to avoid duplicate refreshes (also update the other call at index.ts:402).
Apply this diff in-place:
- // Add font to Tailwind config
- await addFontToTailwindConfig(font, sandbox);
-
- // Add font variable to root layout
- await this.layoutManager.addFontVariableToRootLayout(font.id);
-
- // Reload all views to apply new font styles
- this.editorEngine.frames.reloadAllViews();
+ // Add font to Tailwind config; abort on failure to avoid inconsistent state
+ const tailwindUpdated = await addFontToTailwindConfig(font, sandbox);
+ if (!tailwindUpdated) {
+ console.error('Failed to update Tailwind config for font:', font.id);
+ return false;
+ }
+
+ // Add font variable to root layout (idempotent) and guard failure
+ const layoutUpdated = await this.layoutManager.addFontVariableToRootLayout(font.id);
+ if (!layoutUpdated) {
+ console.error('Failed to add font variable to root layout for font:', font.id);
+ return false;
+ }
+
+ // Coalesce reloads to avoid double refresh with file watcher
+ this.scheduleReload();Add the helper once in the class (outside the shown range):
// Helper: coalesce multiple reloads in the same tick
private reloadScheduled = false;
private scheduleReload(): void {
if (this.reloadScheduled) return;
this.reloadScheduled = true;
queueMicrotask(() => {
this.reloadScheduled = false;
this.editorEngine.frames.reloadAllViews();
});
}File: apps/web/client/src/components/store/editor/font/index.ts (occurrences at ~lines 176–184 and ~402).
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/font/index.ts around lines
176–184 (and also update the other occurrence near line 402), the code currently
awaits addFontToTailwindConfig(...), calls addFontVariableToRootLayout(...) but
ignores its result, and calls editorEngine.frames.reloadAllViews() immediately;
change this so that you first check the boolean result of
addFontToTailwindConfig(...) and abort/throw/return if it fails, then call
LayoutManager.addFontVariableToRootLayout(...) and log a warning if it returns
false (do not throw), and replace the direct reloadAllViews() call with a call
to a class-level coalescing helper (add the provided reloadScheduled and
scheduleReload() helper once in the class outside the shown range) so multiple
reload requests in the same tick are coalesced; also update the other
reloadAllViews() call at ~line 402 to use scheduleReload() instead of calling
reloadAllViews() directly.
This reverts commit 4e6562d.
Summary
Problem
When users added Google Fonts from the brand panel, the fonts would:
font-*fontname*classes to elementsopen-sans)This required manual AI assistance to complete the setup every time.
Solution
Modified
FontManagerto automatically:tailwind.config.ts- ExtendsfontFamilywith new font mappingeditorEngine.frames.reloadAllViews()after font changesTechnical Details
Test Plan
tailwind.config.tscontains exactly one font setup per added fontBefore/After
Before
After
🤖 Generated with Claude Code