[codex] Fix font panel globals CSS setup#3097
[codex] Fix font panel globals CSS setup#3097malsomesh9 wants to merge 2 commits intoonlook-dev:mainfrom
Conversation
|
@malsomesh9 is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds CSS Changes
Sequence DiagramsequenceDiagram
participant Manager as Font Manager
participant Engine as EditorEngine
participant Sandbox as SandboxManager
participant FS as FileSystem
participant CSS as CSS Helper
Manager->>Engine: getActiveSandbox()
Engine-->>Manager: sandbox
Manager->>Sandbox: listFiles()
Sandbox-->>Manager: file list
Manager->>Manager: rank/filter globals.css candidates
Manager->>Sandbox: readFile(globalCssPath)
Sandbox->>FS: read operation
FS-->>Manager: cssContent
Manager->>CSS: addFontToCssTheme(font, cssContent)
CSS-->>Manager: updatedCss
alt content changed
Manager->>Sandbox: writeFile(path, updatedCss)
Sandbox->>FS: write operation
FS-->>Manager: success
end
Manager-->>Manager: return boolean result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/client/src/components/store/editor/font/index.ts (2)
357-368:⚠️ Potential issue | 🟠 MajorRemove stale global CSS tokens during config sync removals.
syncFontsWithConfigs()adds global CSS tokens for newly detected fonts, but removed fonts only clear Tailwind config and root layout. If the font config changes outsideremoveFont(), the@themefont token remains inglobals.css.Mirror global CSS cleanup for removed fonts
if (removedFonts.length > 0) { for (const font of removedFonts) { await removeFontFromTailwindConfig(font, sandbox); await removeFontVariableFromRootLayout(font.id, this.editorEngine); + await removeFontFromGlobalCss(font.id, this.editorEngine); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/index.ts` around lines 357 - 368, syncFontsWithConfigs currently only removes fonts from Tailwind config and root layout via removeFontFromTailwindConfig and removeFontVariableFromRootLayout, leaving stale `@theme` tokens in globals.css; mirror the add path by removing global CSS tokens for removed fonts. Add a call to the counterpart removal function (e.g., removeFontFromGlobalCss or similar) inside the removedFonts loop (the same place that calls removeFontFromTailwindConfig and removeFontVariableFromRootLayout) and ensure it accepts the font id and this.editorEngine just like addFontToGlobalCss so globals.css is cleaned when fonts are removed or configs change externally.
107-123:⚠️ Potential issue | 🟠 MajorDo not report success when project config updates fail.
addFontToGlobalCss,addFontToTailwindConfig, and the layout helpers return booleans, but these results are ignored. That allows the UI/config state to say the font was added or removed even whenglobals.csswas not updated, which is the partial-application failure this PR is trying to prevent.Guard the follow-up config writes before mutating visible state
- await Promise.all([ + const [tailwindUpdated, layoutUpdated, globalCssUpdated] = await Promise.all([ addFontToTailwindConfig(font, this.editorEngine.activeSandbox), addFontVariableToRootLayout(font.id, this.editorEngine), addFontToGlobalCss(font, this.editorEngine), ]); + + if (!tailwindUpdated || !layoutUpdated || !globalCssUpdated) { + console.error('Failed to apply font to one or more project config files'); + return false; + }For
removeFont, apply the same pattern before updating_fonts,_defaultFont, and returningresult.Also applies to: 147-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/index.ts` around lines 107 - 123, The current flow ignores the boolean results of addFontToTailwindConfig, addFontVariableToRootLayout, and addFontToGlobalCss so the UI/state (_fonts, _defaultFont) is mutated and a success returned even if config writes fail; update the logic in the add flow (around addFontToTailwindConfig, addFontVariableToRootLayout, addFontToGlobalCss, fontSearchManager.updateFontsList, and fontSearchManager.loadFontFromBatch) to capture each boolean result, only push to this._fonts, call fontSearchManager.updateFontsList, and await loadFontFromBatch if all config helper calls returned true, and return false (without mutating state) when any helper fails; apply the same guarded-check pattern in removeFont so _fonts/_defaultFont are only changed when all config removals succeed.
🧹 Nitpick comments (4)
apps/web/client/src/components/store/editor/font/index.ts (1)
9-9: Use the configured src alias for this new import.This new import maps inside
apps/web/client/src, so it should use the project alias.Proposed import update
-import { addFontToGlobalCss, removeFontFromGlobalCss } from './css-theme'; +import { addFontToGlobalCss, removeFontFromGlobalCss } from '@/components/store/editor/font/css-theme';As per coding guidelines,
apps/web/client/src/**/*.{ts,tsx}: Use path aliases@/* and ~/* for imports mapping to src/*.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/index.ts` at line 9, The import in index.ts uses a relative path "./css-theme" but per project alias rules for files under apps/web/client/src, replace it with the src alias form (e.g., import from "@/components/store/editor/font/css-theme" or using "~/" if preferred) so the symbols addFontToGlobalCss and removeFontFromGlobalCss are imported via the configured alias rather than a relative path.apps/web/client/src/components/store/editor/font/css-theme.ts (3)
3-5: Prefer path aliases over relative imports per coding guidelines.Consider using
~/components/store/editor/engine,~/components/store/editor/sandbox, and~/components/store/editor/sandbox/helpers(or the equivalent@/*mapping) instead of../engine,../sandbox,../sandbox/helpers.As per coding guidelines: "Use path aliases
@/* and ~/* for imports that map to apps/web/client/src/*".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/css-theme.ts` around lines 3 - 5, Update the three imports to use the project's path alias instead of relative paths: replace references to EditorEngine from "../engine", SandboxManager from "../sandbox", and normalizePath from "../sandbox/helpers" with their aliased module paths (e.g. "~/components/store/editor/engine", "~/components/store/editor/sandbox", and "~/components/store/editor/sandbox/helpers" or the equivalent "@/components/..." mapping) so the import specifiers use the configured path alias convention.
51-107: Minor: duplication between add/remove; consider sharing the read/transform/write skeleton.
addFontToGlobalCssandremoveFontFromGlobalCssare identical except for the transform call. A small helper likeupdateGlobalCss(editorEngine, (css) => ...)would remove the duplication and make future error-handling changes (logging, telemetry) single-sourced.♻️ Sketch
const updateGlobalCss = async ( editorEngine: EditorEngine, transform: (css: string) => string, errorLabel: string, ): Promise<boolean> => { try { const sandbox = editorEngine.activeSandbox; const cssPath = await getGlobalCssPath(sandbox); if (!cssPath) return false; const file = await sandbox.readFile(cssPath); if (typeof file !== 'string') return false; const result = transform(file); if (result !== file) await sandbox.writeFile(cssPath, result); return true; } catch (error) { console.error(errorLabel, error); return false; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/css-theme.ts` around lines 51 - 107, Introduce a shared helper (e.g., updateGlobalCss) that encapsulates the read/transform/write skeleton used by addFontToGlobalCss and removeFontFromGlobalCss: it should accept (editorEngine: EditorEngine, transform: (css: string) => string, errorLabel: string) and internally use editorEngine.activeSandbox, getGlobalCssPath(sandbox), sandbox.readFile(cssPath) and sandbox.writeFile(cssPath) and preserve the original return semantics (false on missing path or non-string file, true if unchanged or after successful write), logging errors with the provided errorLabel; then refactor addFontToGlobalCss to call updateGlobalCss(editorEngine, (css) => addFontToCssTheme(font, css), 'Error adding font to global CSS:') and removeFontFromGlobalCss to call updateGlobalCss(editorEngine, (css) => removeFontFromCssTheme(fontId, css), 'Error removing font from global CSS:').
16-21:src/-prefixed basePath fallback is redundant / unreachable.If
routerConfig.basePathstarts with"src/", the only way the second branch matches is whenbasePath === "src"exactly (so thatsrc/globals.cssequals${basePath}/globals.css) — but then the first branch already matched at rank 0. For any deepersrc/...basePath (e.g.src/app),normalizedPath === "src/globals.css"won't be a reasonable candidate anyway unless you actually want to fall back there. Consider dropping thestartsWith('src/')guard so this branch ranks asrc/globals.csssibling regardless of whether basePath isapporsrc/app, or remove it entirely if that's not a supported layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/css-theme.ts` around lines 16 - 21, The second branch that checks routerConfig.basePath.startsWith('src/') before comparing normalizedPath to normalizePath(`src/${GLOBAL_CSS_FILE}`) is redundant/unreachable; update the conditional in the ranking logic to either remove the startsWith('src/') guard so the normalizePath(`src/${GLOBAL_CSS_FILE}`) sibling is considered regardless of basePath, or delete the whole branch if a src/ fallback is not supported—modify the code that references routerConfig.basePath, normalizedPath, normalizePath and GLOBAL_CSS_FILE accordingly to implement the chosen behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fonts/src/helpers/css-theme.ts`:
- Around line 46-71: Limit regex edits to the `@theme` block body and escape font
IDs: update addFontToCssTheme and removeFontFromCssTheme to call
findThemeBlock() and operate only on the block body range (use
block.bodyStart/bodyEnd) instead of the whole file, build their search/replace
against that substring, and then reassemble the full content; add an
escapeRegExp(fontId: string) helper and use it when creating
themeVariable/getThemeFontVariableName-based regexes so any regex metacharacters
in font IDs are escaped; ensure you reference getThemeFontVariableName(font.id)
when creating the escaped pattern and update both existingDeclarationRegex and
declarationRegex accordingly; finally add a test fixture where the same --font-*
property exists outside `@theme` to assert only the `@theme` block is modified.
---
Outside diff comments:
In `@apps/web/client/src/components/store/editor/font/index.ts`:
- Around line 357-368: syncFontsWithConfigs currently only removes fonts from
Tailwind config and root layout via removeFontFromTailwindConfig and
removeFontVariableFromRootLayout, leaving stale `@theme` tokens in globals.css;
mirror the add path by removing global CSS tokens for removed fonts. Add a call
to the counterpart removal function (e.g., removeFontFromGlobalCss or similar)
inside the removedFonts loop (the same place that calls
removeFontFromTailwindConfig and removeFontVariableFromRootLayout) and ensure it
accepts the font id and this.editorEngine just like addFontToGlobalCss so
globals.css is cleaned when fonts are removed or configs change externally.
- Around line 107-123: The current flow ignores the boolean results of
addFontToTailwindConfig, addFontVariableToRootLayout, and addFontToGlobalCss so
the UI/state (_fonts, _defaultFont) is mutated and a success returned even if
config writes fail; update the logic in the add flow (around
addFontToTailwindConfig, addFontVariableToRootLayout, addFontToGlobalCss,
fontSearchManager.updateFontsList, and fontSearchManager.loadFontFromBatch) to
capture each boolean result, only push to this._fonts, call
fontSearchManager.updateFontsList, and await loadFontFromBatch if all config
helper calls returned true, and return false (without mutating state) when any
helper fails; apply the same guarded-check pattern in removeFont so
_fonts/_defaultFont are only changed when all config removals succeed.
---
Nitpick comments:
In `@apps/web/client/src/components/store/editor/font/css-theme.ts`:
- Around line 3-5: Update the three imports to use the project's path alias
instead of relative paths: replace references to EditorEngine from "../engine",
SandboxManager from "../sandbox", and normalizePath from "../sandbox/helpers"
with their aliased module paths (e.g. "~/components/store/editor/engine",
"~/components/store/editor/sandbox", and
"~/components/store/editor/sandbox/helpers" or the equivalent "@/components/..."
mapping) so the import specifiers use the configured path alias convention.
- Around line 51-107: Introduce a shared helper (e.g., updateGlobalCss) that
encapsulates the read/transform/write skeleton used by addFontToGlobalCss and
removeFontFromGlobalCss: it should accept (editorEngine: EditorEngine,
transform: (css: string) => string, errorLabel: string) and internally use
editorEngine.activeSandbox, getGlobalCssPath(sandbox), sandbox.readFile(cssPath)
and sandbox.writeFile(cssPath) and preserve the original return semantics (false
on missing path or non-string file, true if unchanged or after successful
write), logging errors with the provided errorLabel; then refactor
addFontToGlobalCss to call updateGlobalCss(editorEngine, (css) =>
addFontToCssTheme(font, css), 'Error adding font to global CSS:') and
removeFontFromGlobalCss to call updateGlobalCss(editorEngine, (css) =>
removeFontFromCssTheme(fontId, css), 'Error removing font from global CSS:').
- Around line 16-21: The second branch that checks
routerConfig.basePath.startsWith('src/') before comparing normalizedPath to
normalizePath(`src/${GLOBAL_CSS_FILE}`) is redundant/unreachable; update the
conditional in the ranking logic to either remove the startsWith('src/') guard
so the normalizePath(`src/${GLOBAL_CSS_FILE}`) sibling is considered regardless
of basePath, or delete the whole branch if a src/ fallback is not
supported—modify the code that references routerConfig.basePath, normalizedPath,
normalizePath and GLOBAL_CSS_FILE accordingly to implement the chosen behavior.
In `@apps/web/client/src/components/store/editor/font/index.ts`:
- Line 9: The import in index.ts uses a relative path "./css-theme" but per
project alias rules for files under apps/web/client/src, replace it with the src
alias form (e.g., import from "@/components/store/editor/font/css-theme" or
using "~/" if preferred) so the symbols addFontToGlobalCss and
removeFontFromGlobalCss are imported via the configured alias rather than a
relative path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f518a90-a7de-420d-8594-22c1603e249d
📒 Files selected for processing (17)
apps/web/client/src/components/store/editor/font/css-theme.tsapps/web/client/src/components/store/editor/font/index.tspackages/fonts/src/helpers/css-theme.tspackages/fonts/src/helpers/index.tspackages/fonts/test/css-theme.test.tspackages/fonts/test/data/css-theme/add-font-to-css-theme/existing-theme/config.jsonpackages/fonts/test/data/css-theme/add-font-to-css-theme/existing-theme/expected.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/existing-theme/input.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/no-theme/config.jsonpackages/fonts/test/data/css-theme/add-font-to-css-theme/no-theme/expected.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/no-theme/input.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/updates-existing/config.jsonpackages/fonts/test/data/css-theme/add-font-to-css-theme/updates-existing/expected.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/updates-existing/input.csspackages/fonts/test/data/css-theme/remove-font-from-css-theme/removes-font/config.jsonpackages/fonts/test/data/css-theme/remove-font-from-css-theme/removes-font/expected.csspackages/fonts/test/data/css-theme/remove-font-from-css-theme/removes-font/input.css
| export function addFontToCssTheme(font: Font, content: string): string { | ||
| const declaration = getFontThemeDeclaration(font); | ||
| const themeVariable = getThemeFontVariableName(font.id); | ||
| const existingDeclarationRegex = new RegExp(`(^\\s*)${themeVariable}\\s*:[^;]+;`, 'm'); | ||
|
|
||
| if (existingDeclarationRegex.test(content)) { | ||
| return content.replace(existingDeclarationRegex, `$1${declaration}`); | ||
| } | ||
|
|
||
| const block = findThemeBlock(content); | ||
| if (!block) { | ||
| const separator = content.endsWith('\n') ? '\n' : '\n\n'; | ||
| return `${content}${separator}@theme inline {\n ${declaration}\n}\n`; | ||
| } | ||
|
|
||
| const beforeClose = content.slice(0, block.end).replace(/\s*$/, ''); | ||
| const afterClose = content.slice(block.end); | ||
|
|
||
| return `${beforeClose}\n ${declaration}\n${afterClose}`; | ||
| } | ||
|
|
||
| export function removeFontFromCssTheme(fontId: string, content: string): string { | ||
| const themeVariable = getThemeFontVariableName(fontId); | ||
| const declarationRegex = new RegExp(`\\n?\\s*${themeVariable}\\s*:[^;]+;`, 'g'); | ||
|
|
||
| return content.replace(declarationRegex, ''); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find packages/fonts -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: onlook-dev/onlook
Length of output: 1024
🏁 Script executed:
cat -n packages/fonts/src/helpers/css-theme.tsRepository: onlook-dev/onlook
Length of output: 2840
🏁 Script executed:
# Find the findThemeBlock function
rg "findThemeBlock" packages/fonts --type ts -B 3 -A 10Repository: onlook-dev/onlook
Length of output: 2039
🏁 Script executed:
# Find the Font interface definition
rg "interface Font|type Font" packages/fonts --type ts -A 5Repository: onlook-dev/onlook
Length of output: 397
🏁 Script executed:
# Find getThemeFontVariableName function
rg "getThemeFontVariableName" packages/fonts --type ts -B 2 -A 5Repository: onlook-dev/onlook
Length of output: 2208
🏁 Script executed:
# Check for test files for css-theme
find packages/fonts -type f -name "*css-theme*" | grep -E "test|spec"Repository: onlook-dev/onlook
Length of output: 98
🏁 Script executed:
cat -n packages/fonts/test/css-theme.test.tsRepository: onlook-dev/onlook
Length of output: 2135
🏁 Script executed:
# Check the Font interface/type definition
rg "export.*Font" packages -t ts --type-list | grep -E "interface|type" | head -20Repository: onlook-dev/onlook
Length of output: 450
🏁 Script executed:
# Search for Font interface in models
find packages -name "*.ts" -type f | xargs grep -l "interface Font\|type Font\|export.*Font" | head -10Repository: onlook-dev/onlook
Length of output: 560
🏁 Script executed:
find . -path "*/models*" -name "*.ts" -type f | head -20Repository: onlook-dev/onlook
Length of output: 895
🏁 Script executed:
# Search for Font type/interface definition
rg "interface Font|type Font" --type ts -A 10 | head -50Repository: onlook-dev/onlook
Length of output: 2632
🏁 Script executed:
# Look at existing test cases for css-theme
find packages/fonts/test/data/css-theme -type f | head -20Repository: onlook-dev/onlook
Length of output: 1061
🏁 Script executed:
# Check if there are any test cases with special characters in font IDs
find packages/fonts/test/data/css-theme -type f -name "config.json" -exec cat {} \;Repository: onlook-dev/onlook
Length of output: 772
🏁 Script executed:
# Check test cases more carefully - look at input/expected CSS
cat packages/fonts/test/data/css-theme/add-font-to-css-theme/updates-existing/input.cssRepository: onlook-dev/onlook
Length of output: 113
🏁 Script executed:
cat packages/fonts/test/data/css-theme/add-font-to-css-theme/updates-existing/expected.cssRepository: onlook-dev/onlook
Length of output: 123
🏁 Script executed:
cat packages/fonts/test/data/css-theme/remove-font-from-css-theme/removes-font/input.cssRepository: onlook-dev/onlook
Length of output: 148
🏁 Script executed:
cat packages/fonts/test/data/css-theme/remove-font-from-css-theme/removes-font/expected.cssRepository: onlook-dev/onlook
Length of output: 111
🏁 Script executed:
# Check if font IDs can have special regex characters
rg "fontId|font\.id" packages/fonts --type ts -B 2 -A 2 | head -40Repository: onlook-dev/onlook
Length of output: 2698
Scope font declaration edits to @theme block body and escape regex special characters in font IDs.
The regex patterns in both functions scan the entire CSS file. This means :root { --font-inter: ... } or another selector outside @theme can be matched and modified/removed instead of the Tailwind theme token. Additionally, themeVariable is interpolated directly into the regex without escaping, so font IDs with regex metacharacters (e.g., [, ], (, ), *, +, etc.) could cause incorrect matches or throw errors.
The findThemeBlock() function already returns bodyStart, so constrain both operations to the block body range. Add an escapeRegExp() helper to safely handle arbitrary font IDs.
Scope edits to the theme block body and escape special characters
+function escapeRegExp(value: string): string {
+ return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
export function addFontToCssTheme(font: Font, content: string): string {
const declaration = getFontThemeDeclaration(font);
const themeVariable = getThemeFontVariableName(font.id);
- const existingDeclarationRegex = new RegExp(`(^\\s*)${themeVariable}\\s*:[^;]+;`, 'm');
-
- if (existingDeclarationRegex.test(content)) {
- return content.replace(existingDeclarationRegex, `$1${declaration}`);
- }
+ const existingDeclarationRegex = new RegExp(
+ `(^\\s*)${escapeRegExp(themeVariable)}\\s*:[^;]+;`,
+ 'm',
+ );
const block = findThemeBlock(content);
if (!block) {
const separator = content.endsWith('\n') ? '\n' : '\n\n';
return `${content}${separator}@theme inline {\n ${declaration}\n}\n`;
}
- const beforeClose = content.slice(0, block.end).replace(/\s*$/, '');
- const afterClose = content.slice(block.end);
+ const beforeBlockBody = content.slice(0, block.bodyStart);
+ const blockBody = content.slice(block.bodyStart, block.end);
+ const afterBlockBody = content.slice(block.end);
+
+ if (existingDeclarationRegex.test(blockBody)) {
+ return `${beforeBlockBody}${blockBody.replace(existingDeclarationRegex, `$1${declaration}`)}${afterBlockBody}`;
+ }
- return `${beforeClose}\n ${declaration}\n${afterClose}`;
+ return `${beforeBlockBody}${blockBody.replace(/\s*$/, '')}\n ${declaration}\n${afterBlockBody}`;
}
export function removeFontFromCssTheme(fontId: string, content: string): string {
const themeVariable = getThemeFontVariableName(fontId);
- const declarationRegex = new RegExp(`\\n?\\s*${themeVariable}\\s*:[^;]+;`, 'g');
+ const block = findThemeBlock(content);
+ if (!block) {
+ return content;
+ }
+
+ const declarationRegex = new RegExp(
+ `\\n?\\s*${escapeRegExp(themeVariable)}\\s*:[^;]+;`,
+ 'g',
+ );
- return content.replace(declarationRegex, '');
+ const beforeBlockBody = content.slice(0, block.bodyStart);
+ const blockBody = content.slice(block.bodyStart, block.end);
+ const afterBlockBody = content.slice(block.end);
+
+ return `${beforeBlockBody}${blockBody.replace(declarationRegex, '')}${afterBlockBody}`;
}Also add a test fixture where the same --font-* custom property exists outside @theme to verify the helpers only modify the Tailwind theme block.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 48-48: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((^\\s*)${themeVariable}\\s*:[^;]+;, 'm')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 68-68: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\n?\\s*${themeVariable}\\s*:[^;]+;, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fonts/src/helpers/css-theme.ts` around lines 46 - 71, Limit regex
edits to the `@theme` block body and escape font IDs: update addFontToCssTheme and
removeFontFromCssTheme to call findThemeBlock() and operate only on the block
body range (use block.bodyStart/bodyEnd) instead of the whole file, build their
search/replace against that substring, and then reassemble the full content; add
an escapeRegExp(fontId: string) helper and use it when creating
themeVariable/getThemeFontVariableName-based regexes so any regex metacharacters
in font IDs are escaped; ensure you reference getThemeFontVariableName(font.id)
when creating the escaped pattern and update both existingDeclarationRegex and
declarationRegex accordingly; finally add a test fixture where the same --font-*
property exists outside `@theme` to assert only the `@theme` block is modified.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/client/src/components/store/editor/font/layout-manager.ts (1)
103-112:⚠️ Potential issue | 🟠 MajorRemove the layout import even when no className changed.
Line 112 now treats the no-op path as success, but the import cleanup is still gated by
hasUpdated. If the layout has a stale./fontsimport and no matching className token, removal reports success while leaving the import behind.Suggested direction
- if (hasUpdated && ast) { + if (ast) { // Remove the font import if it exists const newContent = removeFontImportFromFile(fontImportPath, fontName, ast); - if (!newContent) { - return false; + if (newContent) { + await editorEngine.activeSandbox.writeFile(layoutPath, newContent); + return true; + } + + if (hasUpdated) { + const { code } = generate(ast); + await editorEngine.activeSandbox.writeFile(layoutPath, code); } - await editorEngine.activeSandbox.writeFile(layoutPath, newContent); - return true; } return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/layout-manager.ts` around lines 103 - 112, The cleanup currently only runs when hasUpdated is true, so stale ./fonts imports can remain; change the logic so that whenever ast is available you call removeFontImportFromFile(fontImportPath, fontName, ast) regardless of hasUpdated, check its return value and if it returns a newContent write it via editorEngine.activeSandbox.writeFile(layoutPath, newContent), and if removeFontImportFromFile signals failure return false—keep returning true for the overall no-op path only when removal succeeded or no AST is present; adjust uses of hasUpdated, ast, removeFontImportFromFile, fontImportPath, fontName, editorEngine.activeSandbox.writeFile, and layoutPath accordingly.apps/web/client/src/components/store/editor/font/index.ts (2)
366-380:⚠️ Potential issue | 🟡 Minor
syncFontsWithConfigssilently ignores failures from each step.The return values of
removeFontFromTailwindConfig,removeFontVariableFromRootLayout,removeFontFromGlobalCss(and their add counterparts) are discarded, so a transient failure on any one step leaves the font partially synced and the loop moves on. After this PR, the same anti-pattern now also covers global CSS. Consider logging per-step failures and at minimum surfacing them throughconsole.errorwith the font id, so a later sync run can notice and retry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/index.ts` around lines 366 - 380, The syncFontsWithConfigs loop ignores failures from each helper so fonts can be partially synced; update syncFontsWithConfigs to handle errors from removeFontFromTailwindConfig, removeFontVariableFromRootLayout, removeFontFromGlobalCss, addFontToTailwindConfig, addFontVariableToRootLayout and addFontToGlobalCss by wrapping each awaited call in a try/catch, log failures with console.error including the font.id and the caught error, and optionally collect failed font ids to return or rethrow after the loop so a later sync can retry; reference these exact helper names and the syncFontsWithConfigs loop when making the change.
105-162:⚠️ Potential issue | 🟠 MajorPartial-failure leaves project in an inconsistent half-configured state.
Both
addFont(lines 106-117) andremoveFont(lines 150-162) perform the font-config mutation first, then run the three side-effect updates (tailwind / root-layout / global CSS) concurrently viaPromise.all. If any subset of the three fails:
addFontreturnsfalseafteraddFontToConfighas already written the config and, potentially, 1–2 of the three downstream files have been updated. The in-memory_fontsarray is not updated, so subsequent operations diverge from disk.removeFontreturnsfalseafterremoveFontFromConfigsucceeded (so the code diff and font-file deletions have already been committed). The_fontsarray is not pruned, and a re-run will not find the font to clean up the remaining side-effects because the config entry is already gone.Consider (a) logging which specific step failed instead of the generic "one or more", (b) still updating the in-memory state on partial success so re-sync can recover, and (c) at minimum, relying on a subsequent
syncFontsWithConfigs()to reconcile. The error message also loses actionable information — pass the individual booleans to a logger.🛠️ Illustrative fix (addFont)
- const [tailwindUpdated, layoutUpdated, globalCssUpdated] = await Promise.all([ - addFontToTailwindConfig(font, this.editorEngine.activeSandbox), - addFontVariableToRootLayout(font.id, this.editorEngine), - addFontToGlobalCss(font, this.editorEngine), - ]); - - if (!tailwindUpdated || !layoutUpdated || !globalCssUpdated) { - console.error('Failed to apply font to one or more project config files'); - return false; - } + const [tailwindUpdated, layoutUpdated, globalCssUpdated] = await Promise.all([ + addFontToTailwindConfig(font, this.editorEngine.activeSandbox), + addFontVariableToRootLayout(font.id, this.editorEngine), + addFontToGlobalCss(font, this.editorEngine), + ]); + + if (!tailwindUpdated || !layoutUpdated || !globalCssUpdated) { + console.error('Failed to fully apply font; partial state on disk', { + fontId: font.id, + tailwindUpdated, + layoutUpdated, + globalCssUpdated, + }); + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/client/src/components/store/editor/font/index.ts` around lines 105 - 162, The addFont/removeFont flows currently mutate the config first via addFontToConfig/removeFontFromConfig then run three concurrent side-effect updates, but on partial failure they return false leaving disk and in-memory (_fonts and fontSearchManager) inconsistent and logs unhelpful; update both functions so that after the config mutation you (1) run the three side-effect updates and capture each boolean result, (2) log which specific updates failed (include the three booleans) instead of a generic message, and (3) always reconcile in-memory state: if addFontToConfig succeeded then push to this._fonts, update fontSearchManager.updateFontsList and await loadFontFromBatch even if some side-effects failed (for removeFont, if removeFontFromConfig succeeded then remove from this._fonts and update fontSearchManager), and finally on any partial failure call or schedule syncFontsWithConfigs() (or return a value that triggers a later resync) so the repo can fully reconcile; locate symbols addFont, removeFont, addFontToConfig, removeFontFromConfig, addFontToTailwindConfig, addFontVariableToRootLayout, addFontToGlobalCss, removeFontFromTailwindConfig, removeFontVariableFromRootLayout, removeFontFromGlobalCss, this._fonts, fontSearchManager.updateFontsList, and syncFontsWithConfigs when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/client/src/components/store/editor/font/css-theme.ts`:
- Around line 48-76: The updateGlobalCss function currently returns false
silently when sandbox.readFile(cssPath) yields a non-string (the "file is not
text" case); modify updateGlobalCss to log this failure before returning by
calling console.error(errorLabel, 'globals.css was not readable as text') (or
similar) right before the existing "return false" that follows the typeof file
!== 'string' check so callers can diagnose encoding/binary read issues;
reference the updateGlobalCss function, the file variable from
sandbox.readFile(cssPath), and the errorLabel parameter when adding the log.
- Around line 27-46: The getGlobalCssPath function currently calls
sandbox.listAllFiles() on every invocation (used by addFont, removeFont, and
per-font in syncFontsWithConfigs), which is expensive and also matches
globals.css in vendor/build dirs; to fix, cache/memoize the file list for the
duration of a single sync operation (or accept an optional files list param) so
listAllFiles() is called once per sync cycle and invalidate that cache on
filesystem events, and tighten the filter in getGlobalCssPath/rankGlobalCssPath
to prefer exact source locations (e.g., check paths like "globals.css",
"src/globals.css", "<basePath>/globals.css") and explicitly exclude paths
containing "node_modules", "dist", "build" or other vendored output before
falling back to other matches.
In `@packages/fonts/src/helpers/css-theme.ts`:
- Around line 5-32: findThemeBlock currently returns only the first `@theme` match
which causes addFontToCssTheme and removeFontFromCssTheme to target the wrong
block; update the logic to enumerate all `@theme` blocks (use
THEME_BLOCK_REGEX.exec in a loop resetting lastIndex) and return their body
ranges instead of a single range, then change addFontToCssTheme to prefer a
block that already contains a --font- declaration or an `@theme` inline block and
otherwise fall back to the last `@theme` block, and change removeFontFromCssTheme
to remove the --font- declaration from every `@theme` body; also make the
brace-depth scan in findThemeBlock robust to comments/strings by stripping or
skipping CSS comments (/* */) and string literals before counting braces.
In
`@packages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/expected.css`:
- Line 3: getThemeFontVariableName currently interpolates raw font IDs into CSS
custom property names causing invalid identifiers (e.g., "."); update
packages/fonts/src/helpers/css-theme.ts to sanitize/escape the font ID before
constructing the custom property (for example replace or encode characters not
allowed in CSS custom property names such as "." with a safe character like "-"
or use a strict whitelist) in the getThemeFontVariableName function, and then
update the regex-font-id fixtures/expected files to reflect the new
escaped/sanitized property names (e.g., --font-font-test).
---
Outside diff comments:
In `@apps/web/client/src/components/store/editor/font/index.ts`:
- Around line 366-380: The syncFontsWithConfigs loop ignores failures from each
helper so fonts can be partially synced; update syncFontsWithConfigs to handle
errors from removeFontFromTailwindConfig, removeFontVariableFromRootLayout,
removeFontFromGlobalCss, addFontToTailwindConfig, addFontVariableToRootLayout
and addFontToGlobalCss by wrapping each awaited call in a try/catch, log
failures with console.error including the font.id and the caught error, and
optionally collect failed font ids to return or rethrow after the loop so a
later sync can retry; reference these exact helper names and the
syncFontsWithConfigs loop when making the change.
- Around line 105-162: The addFont/removeFont flows currently mutate the config
first via addFontToConfig/removeFontFromConfig then run three concurrent
side-effect updates, but on partial failure they return false leaving disk and
in-memory (_fonts and fontSearchManager) inconsistent and logs unhelpful; update
both functions so that after the config mutation you (1) run the three
side-effect updates and capture each boolean result, (2) log which specific
updates failed (include the three booleans) instead of a generic message, and
(3) always reconcile in-memory state: if addFontToConfig succeeded then push to
this._fonts, update fontSearchManager.updateFontsList and await
loadFontFromBatch even if some side-effects failed (for removeFont, if
removeFontFromConfig succeeded then remove from this._fonts and update
fontSearchManager), and finally on any partial failure call or schedule
syncFontsWithConfigs() (or return a value that triggers a later resync) so the
repo can fully reconcile; locate symbols addFont, removeFont, addFontToConfig,
removeFontFromConfig, addFontToTailwindConfig, addFontVariableToRootLayout,
addFontToGlobalCss, removeFontFromTailwindConfig,
removeFontVariableFromRootLayout, removeFontFromGlobalCss, this._fonts,
fontSearchManager.updateFontsList, and syncFontsWithConfigs when making the
changes.
In `@apps/web/client/src/components/store/editor/font/layout-manager.ts`:
- Around line 103-112: The cleanup currently only runs when hasUpdated is true,
so stale ./fonts imports can remain; change the logic so that whenever ast is
available you call removeFontImportFromFile(fontImportPath, fontName, ast)
regardless of hasUpdated, check its return value and if it returns a newContent
write it via editorEngine.activeSandbox.writeFile(layoutPath, newContent), and
if removeFontImportFromFile signals failure return false—keep returning true for
the overall no-op path only when removal succeeded or no AST is present; adjust
uses of hasUpdated, ast, removeFontImportFromFile, fontImportPath, fontName,
editorEngine.activeSandbox.writeFile, and layoutPath accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77ea37b6-c398-4df8-b51b-be163605446e
📒 Files selected for processing (16)
apps/web/client/src/components/store/editor/font/css-theme.tsapps/web/client/src/components/store/editor/font/index.tsapps/web/client/src/components/store/editor/font/layout-manager.tspackages/fonts/src/helpers/css-theme.tspackages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/config.jsonpackages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/expected.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/input.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/config.jsonpackages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/expected.csspackages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/input.csspackages/fonts/test/data/css-theme/remove-font-from-css-theme/outside-theme-token/config.jsonpackages/fonts/test/data/css-theme/remove-font-from-css-theme/outside-theme-token/expected.csspackages/fonts/test/data/css-theme/remove-font-from-css-theme/outside-theme-token/input.csspackages/fonts/test/data/css-theme/remove-font-from-css-theme/regex-font-id/config.jsonpackages/fonts/test/data/css-theme/remove-font-from-css-theme/regex-font-id/expected.csspackages/fonts/test/data/css-theme/remove-font-from-css-theme/regex-font-id/input.css
✅ Files skipped from review due to trivial changes (4)
- packages/fonts/test/data/css-theme/remove-font-from-css-theme/outside-theme-token/config.json
- packages/fonts/test/data/css-theme/remove-font-from-css-theme/regex-font-id/config.json
- packages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/config.json
- packages/fonts/test/data/css-theme/add-font-to-css-theme/outside-theme-token/config.json
| export const getGlobalCssPath = async (sandbox: SandboxManager): Promise<string | null> => { | ||
| const routerConfig = await sandbox.getRouterConfig(); | ||
| if (!routerConfig) { | ||
| return null; | ||
| } | ||
|
|
||
| const files = await sandbox.listAllFiles(); | ||
| const globalCssFiles = files | ||
| .filter( | ||
| (file) => | ||
| file.type === 'file' && | ||
| (file.path === GLOBAL_CSS_FILE || file.path.endsWith(`/${GLOBAL_CSS_FILE}`)), | ||
| ) | ||
| .sort( | ||
| (a, b) => | ||
| rankGlobalCssPath(a.path, routerConfig) - rankGlobalCssPath(b.path, routerConfig), | ||
| ); | ||
|
|
||
| return globalCssFiles[0]?.path ?? null; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C5 '\blistAllFiles\s*\('Repository: onlook-dev/onlook
Length of output: 7377
🏁 Script executed:
#!/bin/bash
# Find the fs implementation used in SandboxManager
rg -nP --type=ts 'this\.fs\s*=' apps/web/client/src/components/store/editor/sandbox/index.ts -A 3Repository: onlook-dev/onlook
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for listAll implementation
rg -nP --type=ts 'listAll\s*\(' apps/web/client/src/components/store/editor/ -A 5 | head -100Repository: onlook-dev/onlook
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Check the full SandboxManager class to understand fs initialization
sed -n '1,100p' apps/web/client/src/components/store/editor/sandbox/index.tsRepository: onlook-dev/onlook
Length of output: 3716
🏁 Script executed:
#!/bin/bash
# Find EXCLUDED_SYNC_PATHS definition
rg -nP --type=ts 'EXCLUDED_SYNC_PATHS' apps/web/client/src/ -B 2 -A 5Repository: onlook-dev/onlook
Length of output: 2688
🏁 Script executed:
#!/bin/bash
# Search for CodeFileSystem definition and listAll method
rg -nP --type=ts 'class CodeFileSystem|interface CodeFileSystem|listAll' --glob='*.ts' --glob='*.tsx'Repository: onlook-dev/onlook
Length of output: 1703
🏁 Script executed:
#!/bin/bash
# Look in packages for file-system module
find . -name 'file-system' -type d 2>/dev/null | grep -E '(packages|node_modules)' | head -5Repository: onlook-dev/onlook
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Check the listAll() implementation in FileSystem
sed -n '536,580p' packages/file-system/src/fs.tsRepository: onlook-dev/onlook
Length of output: 1643
🏁 Script executed:
#!/bin/bash
# Find EXCLUDED_SYNC_PATHS in `@onlook/constants`
find . -path '*/@onlook/constants*' -type f -name '*.ts' 2>/dev/null | head -5Repository: onlook-dev/onlook
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Also check CodeFileSystem implementation
sed -n '31,100p' packages/file-system/src/code-fs.tsRepository: onlook-dev/onlook
Length of output: 2650
getGlobalCssPath rescans the entire sandbox on every call and may select non-source globals.css files.
-
Performance:
sandbox.listAllFiles()walks the entire file tree on every invocation—inaddFont,removeFont, and per-font duringsyncFontsWithConfigs. For larger projects, this is expensive and runs serially in loops. Consider memoizing the result per sync cycle or invalidating on file system events. -
Correctness: The filter
file.path === GLOBAL_CSS_FILE || file.path.endsWith('/globals.css')matches anyglobals.cssanywhere, includingnode_modules/,dist/, or vendored directories. If none of the preferred paths ({basePath}/globals.css,src/globals.css,globals.css) exist, the ranking fallback will select from whateverlistAllFiles()returns first, which could be a dependency's file. Restrict the search to source directories or excludenode_modulesand build output explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/client/src/components/store/editor/font/css-theme.ts` around lines
27 - 46, The getGlobalCssPath function currently calls sandbox.listAllFiles() on
every invocation (used by addFont, removeFont, and per-font in
syncFontsWithConfigs), which is expensive and also matches globals.css in
vendor/build dirs; to fix, cache/memoize the file list for the duration of a
single sync operation (or accept an optional files list param) so listAllFiles()
is called once per sync cycle and invalidate that cache on filesystem events,
and tighten the filter in getGlobalCssPath/rankGlobalCssPath to prefer exact
source locations (e.g., check paths like "globals.css", "src/globals.css",
"<basePath>/globals.css") and explicitly exclude paths containing
"node_modules", "dist", "build" or other vendored output before falling back to
other matches.
| const updateGlobalCss = async ( | ||
| editorEngine: EditorEngine, | ||
| transform: (css: string) => string, | ||
| errorLabel: string, | ||
| ): Promise<boolean> => { | ||
| try { | ||
| const sandbox = editorEngine.activeSandbox; | ||
| const cssPath = await getGlobalCssPath(sandbox); | ||
| if (!cssPath) { | ||
| return false; | ||
| } | ||
|
|
||
| const file = await sandbox.readFile(cssPath); | ||
| if (typeof file !== 'string') { | ||
| return false; | ||
| } | ||
|
|
||
| const result = transform(file); | ||
| if (result === file) { | ||
| return true; | ||
| } | ||
|
|
||
| await sandbox.writeFile(cssPath, result); | ||
| return true; | ||
| } catch (error) { | ||
| console.error(errorLabel, error); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
updateGlobalCss swallows the "file is not text" case silently.
On line 61-63, a non-string readFile result returns false without any log, while all other non-happy paths either log or are benign. If globals.css ever comes back as Uint8Array (e.g., encoding detection glitch), callers will see the font add/remove fail generically with no diagnostic. Add a console.error(errorLabel, 'globals.css was not readable as text') before returning so the failure is attributable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/client/src/components/store/editor/font/css-theme.ts` around lines
48 - 76, The updateGlobalCss function currently returns false silently when
sandbox.readFile(cssPath) yields a non-string (the "file is not text" case);
modify updateGlobalCss to log this failure before returning by calling
console.error(errorLabel, 'globals.css was not readable as text') (or similar)
right before the existing "return false" that follows the typeof file !==
'string' check so callers can diagnose encoding/binary read issues; reference
the updateGlobalCss function, the file variable from sandbox.readFile(cssPath),
and the errorLabel parameter when adding the log.
| function findThemeBlock(content: string): { bodyStart: number; bodyEnd: number } | null { | ||
| const match = THEME_BLOCK_REGEX.exec(content); | ||
| THEME_BLOCK_REGEX.lastIndex = 0; | ||
|
|
||
| if (!match) { | ||
| return null; | ||
| } | ||
|
|
||
| const bodyStart = match.index + match[0].length; | ||
| let depth = 1; | ||
|
|
||
| for (let index = bodyStart; index < content.length; index++) { | ||
| const char = content[index]; | ||
| if (char === '{') { | ||
| depth++; | ||
| } else if (char === '}') { | ||
| depth--; | ||
| if (depth === 0) { | ||
| return { | ||
| bodyStart, | ||
| bodyEnd: index, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4 multiple @theme blocks in same stylesheet
💡 Result:
Yes, Tailwind CSS v4 supports multiple @theme blocks in the same stylesheet. They are merged together, with later blocks overriding values from earlier ones if there are conflicts. This allows organizing theme definitions across multiple blocks, such as separating base theme values from inline or theme-specific ones. For example, from official Tailwind CSS sources and GitHub discussions: @theme { --color-primary: #aab9ff; } @theme inline { --color-secondary: var(--some-var); } This generates utilities for both, combining the definitions. The @theme directive processes all blocks top-level, extending or overriding the theme cumulatively. Official docs emphasize @theme must be top-level (not nested), but multiple sequential blocks are valid and commonly used in practice for modularity, as seen in PR #14095 and discussions like #18560. When importing external CSS files with theme(reference) or theme(inline), they can contain @theme blocks that merge similarly.
Citations:
- 1: https://tailwindcss.com/docs/theme
- 2: https://tailwindcss.com/docs/configuration
- 3: https://tailwindcss.com/docs/functions-and-directives
- 4: Add support for
inlineoption when defining@themevalues tailwindlabs/tailwindcss#14095 - 5: v4 @theme vs. @theme inline tailwindlabs/tailwindcss#18560
- 6: [v4] Files imported with
@import "…" theme(…)must only contain@themeblocks tailwindlabs/tailwindcss#15219
🏁 Script executed:
cat -n packages/fonts/src/helpers/css-theme.tsRepository: onlook-dev/onlook
Length of output: 3788
Only the first @theme block is ever considered — can cause missed removals and wrong-block additions.
Tailwind v4 explicitly supports multiple @theme blocks in one stylesheet (commonly one for colors, another @theme inline { … } dedicated to fonts). findThemeBlock returns only the first match, so:
addFontToCssThemewill insert the font variable into the first@themeblock even if a downstream@theme inline { }is the intended font block — mixing font tokens into the colors block.removeFontFromCssThemewill leave orphaned--font-*declarations in any@themeblock other than the first.
Consider iterating over all @theme blocks: for add, prefer an existing block that already contains a --font-* declaration (or an @theme inline) and fall back to the last block; for remove, strip the declaration from every @theme body.
Additionally, the brace-depth loop doesn't skip CSS comments or strings — /* } */ inside a @theme body would terminate the scan early. Unlikely in practice for generated globals.css, but worth a defensive comment-stripping pass if user-authored input is expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fonts/src/helpers/css-theme.ts` around lines 5 - 32, findThemeBlock
currently returns only the first `@theme` match which causes addFontToCssTheme and
removeFontFromCssTheme to target the wrong block; update the logic to enumerate
all `@theme` blocks (use THEME_BLOCK_REGEX.exec in a loop resetting lastIndex) and
return their body ranges instead of a single range, then change
addFontToCssTheme to prefer a block that already contains a --font- declaration
or an `@theme` inline block and otherwise fall back to the last `@theme` block, and
change removeFontFromCssTheme to remove the --font- declaration from every
`@theme` body; also make the brace-depth scan in findThemeBlock robust to
comments/strings by stripping or skipping CSS comments (/* */) and string
literals before counting braces.
| @@ -0,0 +1,4 @@ | |||
| @theme inline { | |||
| --font-font-test: var(--font-old); | |||
| --font-font.test: var(--font-font-test); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find unescaped dots in generated CSS custom property names and inspect token construction.
# Expectation after the fix: no unescaped-dot custom property declarations in fixtures/helper outputs.
rg -nP -- '^\s*--[A-Za-z0-9_-]*\.[A-Za-z0-9_.-]*\s*:' packages/fonts/test/data/css-theme packages/fonts/src || true
# Inspect the helper implementation for raw font id / fontId interpolation into --font-* tokens.
rg -nP --type=ts -C4 -- '(--font-\$\{|`--font-|font\.id|fontId)' packages/fonts/src/helpers/css-theme.tsRepository: onlook-dev/onlook
Length of output: 1993
Escape font IDs before creating CSS custom property names.
Line 3 expects --font-font.test, but an unescaped . is not valid in a CSS custom property identifier. The helper function getThemeFontVariableName in packages/fonts/src/helpers/css-theme.ts (line 38-39) directly interpolates font IDs without escaping. Sanitize the font ID before constructing the custom property name, then update the regex-font-id fixtures accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/fonts/test/data/css-theme/add-font-to-css-theme/regex-font-id/expected.css`
at line 3, getThemeFontVariableName currently interpolates raw font IDs into CSS
custom property names causing invalid identifiers (e.g., "."); update
packages/fonts/src/helpers/css-theme.ts to sanitize/escape the font ID before
constructing the custom property (for example replace or encode characters not
allowed in CSS custom property names such as "." with a safe character like "-"
or use a strict whitelist) in the getThemeFontVariableName function, and then
update the regex-font-id fixtures/expected files to reflect the new
escaped/sanitized property names (e.g., --font-font-test).
1 participant
Summary
Fixes #2912
Validation
Summary by CodeRabbit
New Features
Tests