fix(settings): widen theme schema to accept custom theme IDs#714
Open
zeshuochen wants to merge 3 commits into
Open
fix(settings): widen theme schema to accept custom theme IDs#714zeshuochen wants to merge 3 commits into
zeshuochen wants to merge 3 commits into
Conversation
CC validates settings.json theme against built-in enum OR "custom:" prefix. Tweakcc custom themes use plain IDs (e.g. "winter") that fail both checks, causing the theme to be silently reset via .catch(void 0) on every startup. Add a new patch that replaces the union validator with z.string() so arbitrary theme IDs round-trip through settings.json without being discarded. Closes Piebald-AI#702
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new patch "settings-theme-schema" that rewrites the settings ChangesSettings Theme Schema Patch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/patches/index.ts (1)
705-713: ⚡ Quick winExtract shared theme-gating condition to one constant.
This condition is duplicated between
themesandsettings-theme-schema.
Centralizing it avoids drift where one patch gets enabled and the other does
not.Suggested refactor
+ const customThemesConfigured = !!( + config.settings.themes && + config.settings.themes.length > 0 && + JSON.stringify(config.settings.themes) !== + JSON.stringify(DEFAULT_SETTINGS.themes) + ); + const patchImplementations: Record<PatchId, PatchImplementation> = { @@ themes: { fn: c => writeThemes(c, config.settings.themes!), - condition: !!( - config.settings.themes && - config.settings.themes.length > 0 && - JSON.stringify(config.settings.themes) !== - JSON.stringify(DEFAULT_SETTINGS.themes) - ), + condition: customThemesConfigured, }, 'settings-theme-schema': { fn: c => writeSettingsTheme(c), - condition: !!( - config.settings.themes && - config.settings.themes.length > 0 && - JSON.stringify(config.settings.themes) !== - JSON.stringify(DEFAULT_SETTINGS.themes) - ), + condition: customThemesConfigured, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/index.ts` around lines 705 - 713, The duplicated gating logic for theme-related patches should be extracted into a single boolean constant (e.g., hasCustomThemes) and reused in both the 'settings-theme-schema' and 'themes' patch entries; locate where the current condition references config.settings.themes, DEFAULT_SETTINGS.themes and writeSettingsTheme, create a descriptive constant (computed once from config.settings.themes.length and JSON.stringify comparison to DEFAULT_SETTINGS.themes) and replace the inline conditions in the 'settings-theme-schema' and 'themes' patch objects with that constant to ensure both patches enable/disable in sync.src/patches/settingsTheme.ts (2)
3-12: ⚡ Quick winRemove the explanatory comment block from this logic file.
Please drop these comments unless explicitly requested by maintainers.
As per coding guidelines, "Do not add comments unless explicitly requested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/settingsTheme.ts` around lines 3 - 12, Remove the explanatory comment block at the top of src/patches/settingsTheme.ts (the multi-line comment that starts with "CC validates the settings.json `theme` field..." and includes the CC 2.1.x diff examples), leaving the actual schema line(s) intact (e.g., the new theme:h.string().optional().catch(void 0) replacement). Ensure you only delete the comment text and do not modify the functional code that widens the schema to accept any string.
15-15: ⚡ Quick winTighten the regex to the exact minified sequence you intend to patch.
The current wildcard parts (
"[^"]*", broad transform shape) make the match
more permissive than needed. Narrowing to the expected minified structure
reduces accidental matches and keeps patch behavior predictable across builds.Based on learnings, "Patch regexes must be strict and match the minified format exactly (ignore formatting differences like whitespace/newlines)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/settingsTheme.ts` at line 15, The regex in the patch (the literal shown starting with /,theme:([$\w]+)\.union\(\[\1\.enum\([$\w$]+\),\1\.string\(\)\.startsWith\("[^"]*"\)\.transform\(\([^)]+\)=>[^)]+\)\]\)\.optional\(\)\.catch\(void 0\)/) is too permissive; tighten it to match the exact minified output by replacing the generic `"[^"]*"` and the broad transform matcher `\([^)]+\)=>[^)]+` with the precise minified string and arrow-function body you observe in production (escape any special chars), and ensure the rest of the sequence (theme:..., .union([...]), .enum(...), .string().startsWith("...").transform(...), .optional().catch(void 0)) is matched verbatim with no loose wildcards so only the intended minified sequence is patched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patches/settingsTheme.ts`:
- Around line 23-26: The check in settingsTheme.ts that treats only patched ===
0 as failure is insufficient because the function expects to replace both schema
occurrences; update the logic around the patched variable (the replacement
count) to treat fewer-than-expected replacements as failure or degraded state
(e.g., require patched === 2 or patched >= EXPECTED_PATCHES), and update the
console/error logging to include the actual patched count and a clear message;
modify the return path of the function (currently returning null on failure) to
also return null or an explicit degraded result when patched < expected so
callers can detect partial rewrites.
---
Nitpick comments:
In `@src/patches/index.ts`:
- Around line 705-713: The duplicated gating logic for theme-related patches
should be extracted into a single boolean constant (e.g., hasCustomThemes) and
reused in both the 'settings-theme-schema' and 'themes' patch entries; locate
where the current condition references config.settings.themes,
DEFAULT_SETTINGS.themes and writeSettingsTheme, create a descriptive constant
(computed once from config.settings.themes.length and JSON.stringify comparison
to DEFAULT_SETTINGS.themes) and replace the inline conditions in the
'settings-theme-schema' and 'themes' patch objects with that constant to ensure
both patches enable/disable in sync.
In `@src/patches/settingsTheme.ts`:
- Around line 3-12: Remove the explanatory comment block at the top of
src/patches/settingsTheme.ts (the multi-line comment that starts with "CC
validates the settings.json `theme` field..." and includes the CC 2.1.x diff
examples), leaving the actual schema line(s) intact (e.g., the new
theme:h.string().optional().catch(void 0) replacement). Ensure you only delete
the comment text and do not modify the functional code that widens the schema to
accept any string.
- Line 15: The regex in the patch (the literal shown starting with
/,theme:([$\w]+)\.union\(\[\1\.enum\([$\w$]+\),\1\.string\(\)\.startsWith\("[^"]*"\)\.transform\(\([^)]+\)=>[^)]+\)\]\)\.optional\(\)\.catch\(void
0\)/) is too permissive; tighten it to match the exact minified output by
replacing the generic `"[^"]*"` and the broad transform matcher
`\([^)]+\)=>[^)]+` with the precise minified string and arrow-function body you
observe in production (escape any special chars), and ensure the rest of the
sequence (theme:..., .union([...]), .enum(...),
.string().startsWith("...").transform(...), .optional().catch(void 0)) is
matched verbatim with no loose wildcards so only the intended minified sequence
is patched.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07d6df88-552a-4399-a453-0a2283916cf8
📒 Files selected for processing (2)
src/patches/index.tssrc/patches/settingsTheme.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/patches/settingsTheme.ts (1)
32-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPartial-rewrite not detected —
patched === 0guard is still insufficient.The PR documents two occurrences;
patched === 1currently returns success while leaving one schema instance unpatched. This issue was raised on the previous commit and remains unaddressed.🐛 Proposed fix
- if (patched === 0) { + if (patched < 2) { console.error('patch: settingsTheme: failed to find theme schema pattern'); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/settingsTheme.ts` around lines 32 - 35, The guard that only fails when patched === 0 is insufficient because there are two schema occurrences to patch; change the validation around the patched counter (variable patched) in the settingsTheme patching routine so that success is only returned when patched === 2, and treat any other value as a failure (log an error mentioning the unexpected patched count and return null); update the success and error messages to reflect the expected two patches and reference the patched variable to locate the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/patches/settingsTheme.ts`:
- Around line 32-35: The guard that only fails when patched === 0 is
insufficient because there are two schema occurrences to patch; change the
validation around the patched counter (variable patched) in the settingsTheme
patching routine so that success is only returned when patched === 2, and treat
any other value as a failure (log an error mentioning the unexpected patched
count and return null); update the success and error messages to reflect the
expected two patches and reference the patched variable to locate the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93b38b6a-ab12-47a2-adaa-c0b64aa4d2c2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/patches/settingsTheme.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a user configures custom tweakcc themes and selects one (e.g.
winter), CC writes the bare theme ID tosettings.json. However, CC's Zod schema for thethemefield only accepts:auto,dark,light,light-daltonized,dark-daltonized,light-ansi,dark-ansi)"custom:"A bare ID like
"winter"fails both checks. The.catch(void 0)on the field silently resets it toundefined, so the theme is discarded on every startup. Only a legacy fallback via~/.claude.jsonkeeps it working — a fragile dependency that may not always be present.Fixes #702.
Solution
Add a new patch
settingsThemethat replaces:This mirrors the approach used in
allowCustomAgentModels(replacing a narrow enum withz.string()). The patch applies to both the user settings and managed settings schema instances. It is gated behind the same condition as thethemespatch — only active when custom themes are configured.CC version tested
Verified against
claude 2.1.126: the regex matches both schema instances (user settings + managed settings) and correctly transforms them.Test plan
tsc --noEmit)claude 2.1.126binary — matches 2 occurrencessettings.jsonSummary by CodeRabbit