Repository navigation
Fix theme picker chrome preview sync - #4652
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGhosttyConfig gains recursive ChangesRecursive Config Loading and Background Application
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryFixes the theme picker chrome preview by making window chrome derive from the same disk-resolved
Confidence Score: 5/5Safe to merge; the chrome resolution rerouting and recursive include loading are well-tested and the logic is sound. The core changes — disk-resolved chrome values, per-field parsed/directive tracking, recursive config-file includes with cycle detection, and legacy fallback rules — are all covered by new unit tests and the logic is consistent. The only new code introduced without a corresponding shared abstraction is two pairs of identical helper functions duplicated between GhosttyConfig and GhosttyTerminalView, which creates a maintenance drift risk but has no runtime impact on this change. Sources/GhosttyConfig.swift and Sources/GhosttyTerminalView.swift share duplicate Important Files Changed
Sequence DiagramsequenceDiagram
participant App as GhosttyApp
participant CRT as Ghostty C Runtime
participant DC as Disk Config (GhosttyConfig.load)
participant UI as Chrome/Window
Note over App: reloadConfiguration()
App->>CRT: loadDefaultConfigFilesWithLegacyFallback(newConfig)
CRT-->>App: newConfig (ghostty_config_t)
App->>UI: updateDefaultBackground(from: newConfig) [existing, fires notify]
App->>DC: invalidateLoadCache()
App->>DC: GhosttyConfig.load(useCache: false)
Note over DC: reads config.ghostty → recursive config-file includes → legacy fallback
DC-->>App: "resolved GhosttyConfig (with hasParsed* flags)"
App->>App: resolvedAppearanceValue() merges disk values with C-runtime baseline
App->>UI: applyDefaultBackground(resolved colors) [fires notify again]
App->>CRT: synchronizeGhosttyRuntimeColorScheme()
App->>CRT: ghostty_app_update_config(app, newConfig)
Note over App: config-change callback (GHOSTTY_ACTION_CONFIG_CHANGE)
App->>App: synchronizeGhosttyRuntimeColorScheme() only
Note over App: chrome NOT updated from stale config-change payload
Reviews (8): Last reviewed commit: "fix: align recursive and legacy config l..." | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var recursiveConfigPaths: [String] = [] | ||
|
|
||
| for path in paths.map({ NSString(string: $0).expandingTildeInPath }) { | ||
| loadConfigFile( | ||
| at: path, | ||
| into: &config, | ||
| preferredColorScheme: preferredColorScheme, | ||
| recursiveConfigPaths: &recursiveConfigPaths | ||
| ) | ||
| } | ||
|
|
||
| var loadedRecursivePaths = Set<String>() | ||
| while !recursiveConfigPaths.isEmpty { | ||
| let path = recursiveConfigPaths.removeFirst() | ||
| let resolved = (path as NSString).standardizingPath | ||
| guard !loadedRecursivePaths.contains(resolved) else { continue } | ||
| loadedRecursivePaths.insert(resolved) | ||
|
|
||
| loadConfigFile( | ||
| at: path, | ||
| into: &config, | ||
| preferredColorScheme: preferredColorScheme, | ||
| recursiveConfigPaths: &recursiveConfigPaths | ||
| ) | ||
| } |
There was a problem hiding this comment.
loadedRecursivePaths is initialized after the top-level paths in paths are already loaded. If any top-level config file contains config-file = config.ghostty (self-inclusion) or a chain that circles back to one of the original paths, that top-level file is loaded a second time — the set only guards recursive-to-recursive cycles, not recursive-to-top-level ones. Concretely, if a user has config-file = ~/.config/ghostty/config in their config, the file is parsed twice, so any accumulated settings (e.g. a background color being overridden later in the same file) are applied in the wrong order. Seeding loadedRecursivePaths with the standardized forms of all entries in paths before entering the while loop would close this gap.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 465-477: The loop only dedupes entries already seen in
loadedRecursivePaths, so files that were parsed from the initial paths list can
be re-parsed when encountered via a config-file include; to fix, compute a
resolved-set of the initially loaded paths (e.g., resolve each entry in paths
into a Set<String> before entering the while) and check that set when deciding
to skip a recursive path (i.e., guard !loadedRecursivePaths.contains(resolved)
&& !initialResolvedPaths.contains(resolved)), and when you do load a file via
loadConfigFile(at:into:preferredColorScheme:recursiveConfigPaths:), insert its
resolved path into the same dedupe set(s) so subsequent includes are skipped;
reference loadedRecursivePaths, recursiveConfigPaths, paths and loadConfigFile
to locate where to add this logic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b90eb94-b6cc-40f9-b654-0203e21f4fc4
📒 Files selected for processing (3)
Sources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 236-249: Update the background-hex test case in
testColorParseFlagsOnlyTrackValuesResolvedBySwiftParser to use a hex value that
includes letters (for example "`#aabbcc`") so the test exercises
case-normalization; change the hexColorConfig.parse call (GhosttyConfig.parse)
to "background = `#aabbcc`\n" and update the assertion to expect the normalized
uppercase string via backgroundColor.hexString() == "`#AABBCC`" while keeping the
hasParsedBackgroundColor check unchanged.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3589-3606: The helper
updateDefaultBackgroundFromResolvedGhosttyConfig currently always calls
GhosttyConfig.load(...) and prefers on-disk values over baselineConfig; change
it so callers can avoid reloading disk config when recovering to fallbackConfig:
add a boolean parameter (e.g. useOnDiskResolved: Bool = true) to
updateDefaultBackgroundFromResolvedGhosttyConfig and, when false, set resolved
to a GhosttyConfig-like value derived from baselineConfig (or skip load and use
baseline fields directly) so initialize.fallbackConfig can call it with
useOnDiskResolved: false to short-circuit the on-disk load and preserve baseline
colors/opacity/cursor/selection values. Ensure you update any callers (notably
initialize.fallbackConfig) to pass the flag.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c739530b-b0ad-4ea4-bad5-5d9cb9a4d9c6
📒 Files selected for processing (3)
Sources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 396-447: Summary: The parser currently resets hasParsed* flags to
false on invalid later entries, causing desynchronization between flags (e.g.,
hasParsedBackgroundColor) and retained values (e.g., backgroundColor) across
recursive parse() calls. Fix: remove the else branches that set the per-field
hasParsed* flags to false in the parse() switch cases (examples:
hasParsedBackgroundColor, hasParsedBackgroundOpacity, hasParsedBackgroundBlur,
hasParsedForegroundColor, hasParsedCursorColor, hasParsedCursorTextColor,
hasParsedSelectionBackground, hasParsedSelectionForeground) so that once a flag
is set true by a successful NSColor(hex:) or parseBackgroundBlur() or
Double(value) parse, it is not cleared by later invalid entries; apply this same
change consistently across all similar cases in parse().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8894814c-a715-4cf6-a04b-163fe589f903
📒 Files selected for processing (2)
Sources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d7ac24f. Configure here.
Stale automated review on older commits. The actionable inline threads were resolved or superseded by current tests and the latest Codex review is clean.
This branch was successfully deployed
1 participant
Summary
Testing
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches config loading/parsing and default appearance resolution, which can affect startup theming and how user configs are discovered/applied. Risk is mitigated by extensive new unit coverage, but regressions could impact theme/chrome colors for edge-case configs.
Overview
Unifies chrome appearance with on-disk-resolved Ghostty config.
GhosttyTerminalViewnow computes default background/foreground/cursor/selection colors by re-loadingGhosttyConfigfrom disk (no cache) and merging it with the baseline runtime config so theme picker previews keep terminal + window chrome in sync.Hardens config discovery and parsing.
GhosttyConfigadds recursiveconfig-fileinclude loading with cycle-safe dedup, supports optional/quoted include paths, allows absolute theme file paths, clampsbackground-opacity, and tracks directive present vs Swift-parsed flags so unparseable directives don’t accidentally fall back to stale baselines. Legacyconfigis now only considered whenconfig.ghosttyis missing/empty, and app/surfaceconfig-changecallbacks no longer drive chrome updates.Tests. Expands
GhosttyConfigTeststo cover include recursion, cycles, quoted/optional paths, legacy fallback rules, absolute theme paths, opacity clamping, and parsed-vs-directive tracking.Reviewed by Cursor Bugbot for commit e3cb183. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the theme picker so window chrome previews update with the terminal by resolving chrome from the same on‑disk
GhosttyConfig. Aligns recursive include handling and legacy macOS config fallback so previews don’t use stale defaults.GhosttyConfigat init and reload; fallback chrome keeps its own config; ignore app/surfaceconfig-changepayloads for chrome ownership.config-fileincludes with cycle‑safe dedup; parse relative/absolute paths,~, optional?, and quoted paths (including optional‑quoted); allow absolute theme file paths.config.ghostty; include legacyconfigonly when the new file is missing or empty; ignore legacy baselines when a non‑emptyconfig.ghosttyexists.Written for commit e3cb183. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests