Add cmux themes command - #1334
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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:
📝 WalkthroughWalkthroughAdds a new top-level Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as cmux CLI
participant FS as File System
participant Notif as DistributedNotificationCenter
participant App as AppDelegate
participant Ghost as GhosttyApp
User->>CLI: cmux themes set <name|light|dark>
CLI->>FS: validate theme & write managed override file
CLI->>Notif: post reload notification
Notif->>App: deliver reloadConfig notification
App->>Ghost: reloadConfiguration(source: .cmux)
Ghost->>FS: read config files & apply theme
Ghost->>User: terminal appearance updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyConfig.swift">
<violation number="1" location="Sources/GhosttyConfig.swift:128">
P2: Debug-tagged builds can load stale per-tag config instead of the shared release override. This can make app theme behavior diverge from `cmux themes set/clear` output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if hasConfig(currentPaths) { | ||
| return currentPaths | ||
| } | ||
| if SocketControlSettings.isDebugLikeBundleIdentifier(currentBundleIdentifier) { | ||
| return releasePaths | ||
| } |
There was a problem hiding this comment.
P2: Debug-tagged builds can load stale per-tag config instead of the shared release override. This can make app theme behavior diverge from cmux themes set/clear output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyConfig.swift, line 128:
<comment>Debug-tagged builds can load stale per-tag config instead of the shared release override. This can make app theme behavior diverge from `cmux themes set/clear` output.</comment>
<file context>
@@ -87,6 +88,52 @@ struct GhosttyConfig {
+ }
+
+ let currentPaths = paths(for: currentBundleIdentifier)
+ if hasConfig(currentPaths) {
+ return currentPaths
+ }
</file context>
| if hasConfig(currentPaths) { | |
| return currentPaths | |
| } | |
| if SocketControlSettings.isDebugLikeBundleIdentifier(currentBundleIdentifier) { | |
| return releasePaths | |
| } | |
| if SocketControlSettings.isDebugLikeBundleIdentifier(currentBundleIdentifier) { | |
| return releasePaths | |
| } | |
| if hasConfig(currentPaths) { | |
| return currentPaths | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6584a01aef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ghostty_config_load_recursive_files(config) | ||
| loadCmuxAppSupportGhosttyConfigIfNeeded(config) |
There was a problem hiding this comment.
In loadDefaultConfigFilesWithLegacyFallback, ghostty_config_load_recursive_files now runs before loadCmuxAppSupportGhosttyConfigIfNeeded, so any config-file includes declared in the cmux app-support files (.../config or .../config.ghostty) are never expanded. This is a behavior regression from the previous ordering and means users who keep includes in the cmux-managed config chain will silently lose those settings after reload; the recursive pass needs to happen after loading these files (or be run again).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
Sources/GhosttyConfig.swift (1)
91-135: Code duplication withGhosttyTerminalView.cmuxAppSupportConfigURLs().This function duplicates nearly identical logic from
GhosttyTerminalView.cmuxAppSupportConfigURLs()(lines 1232-1272). Both enumerate the same paths (configandconfig.ghosttyundercom.cmuxterm.app) with the same file validation and debug-bundle fallback logic.Consider extracting a shared helper to avoid maintaining parallel implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 91 - 135, cmuxConfigPaths duplicates logic from GhosttyTerminalView.cmuxAppSupportConfigURLs; extract the shared behavior into a single helper (e.g., a new static method like CmuxConfig.helperConfigPaths or extension on FileManager) that encapsulates paths(for:) and hasConfig(_:) logic and the debug-bundle fallback, then have both cmuxConfigPaths and GhosttyTerminalView.cmuxAppSupportConfigURLs call that helper; ensure the helper uses cmuxReleaseBundleIdentifier and SocketControlSettings.isDebugLikeBundleIdentifier and preserves the same validation (attributesOfItem, FileAttributeType.typeRegular, size > 0).Sources/GhosttyTerminalView.swift (1)
1259-1267: Return only existing/non-empty URLs from discovery.At Line 1260/1265 you validate existence with
hasConfig(...), but return the full candidate list; downstream (Line 1322) loads every returned URL, including missing/empty paths.♻️ Proposed refactor
- func hasConfig(_ urls: [URL]) -> Bool { - urls.contains { url in + func existingNonEmptyConfigURLs(_ urls: [URL]) -> [URL] { + urls.filter { url in guard let attrs = try? fileManager.attributesOfItem(atPath: url.path), let type = attrs[.type] as? FileAttributeType, type == .typeRegular, let size = attrs[.size] as? NSNumber else { return false } return size.intValue > 0 } } - let currentURLs = configURLs(for: currentBundleIdentifier) - if hasConfig(currentURLs) { + let currentURLs = existingNonEmptyConfigURLs(configURLs(for: currentBundleIdentifier)) + if !currentURLs.isEmpty { return currentURLs } if SocketControlSettings.isDebugLikeBundleIdentifier(currentBundleIdentifier) { - let releaseURLs = configURLs(for: releaseBundleIdentifier) - if hasConfig(releaseURLs) { + let releaseURLs = existingNonEmptyConfigURLs(configURLs(for: releaseBundleIdentifier)) + if !releaseURLs.isEmpty { return releaseURLs } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1259 - 1267, The function currently returns the full arrays from configURLs(for:) (e.g., currentURLs and releaseURLs) even when hasConfig(...) only checks existence, causing downstream code (which loads every returned URL) to attempt to load missing/empty paths; change the code to return only the existing/non-empty URLs by filtering the result of configURLs(for:) (e.g., filter out nil/empty or non-existent file URLs) before returning in both the currentBundleIdentifier and releaseBundleIdentifier branches (or extract a small helper like filteredConfigURLs(from:)) so that callers only receive valid URLs; keep uses of hasConfig(...) for the quick existence check if desired but ensure the returned array is the filtered set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 5839-5845: The function removingManagedThemeOverride currently
only removes the first regex match because it uses contents.range(of:options:),
so update it to remove all occurrences by performing a global regex replace
(e.g., use contents.replacingOccurrences(of:pattern, with:"",
options:.regularExpression) or an NSRegularExpression replaceMatches approach)
inside removingManagedThemeOverride to ensure every "# cmux themes start" / "#
cmux themes end" block is stripped.
- Around line 5820-5834: The function clearManagedThemeOverride currently
swallows filesystem errors via try? which can report success even when
reads/removes fail; update clearManagedThemeOverride to perform proper throwing
I/O: use try (not try?) when reading the file (String(contentsOf:encoding:)) so
read errors propagate, call try fileManager.removeItem(at:) and propagate that
error instead of ignoring it, and use try when writing the updated contents to
the config URL; keep cmuxThemeOverrideConfigURL() and
removingManagedThemeOverride(_:) calls but ensure all file operations inside
clearManagedThemeOverride either throw up to the caller or are caught and turned
into meaningful errors.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1328-1331: The debug log in GhosttyTerminalView currently calls
Self.initLog(...) inside the `#if` DEBUG block; replace that call with dlog(...)
so the event uses the required debug-only logger. Locate the call to
Self.initLog(...) (the string "loaded cmux app support ghostty config from:
...") and change it to call dlog(...) with the same message, keeping it inside
the existing `#if` DEBUG / `#endif` block to satisfy the debug-only logging
guideline.
- Around line 1232-1236: Tests still call the removed
GhosttyApp.shouldLoadReleaseAppSupportGhosttyConfig; update them to exercise
GhosttyApp.cmuxAppSupportConfigURLs instead by creating temporary App Support
and debug bundle directories (or injecting a mocked FileManager) with the
various config file permutations and asserting the returned [URL] contains the
expected release config URL when only release config exists; specifically,
replace calls to shouldLoadReleaseAppSupportGhosttyConfig with assertions on
cmuxAppSupportConfigURLs(currentBundleIdentifier:appSupportDirectory:fileManager:)
and verify the returned array's contents and order to replicate the previous
fallback logic.
In `@Sources/TerminalController.swift`:
- Line 9667: The help text for the reload_config command currently advertises
only "reload_config [soft]" but the implementation accepts "full" as well;
update the help string that contains "reload_config [soft] - Reload
Ghostty config and refresh terminals" to reflect the supported syntax (e.g.
"reload_config [soft|full]" or "reload_config [soft|full] (alias:
reload_config)") so the documentation matches the implementation that recognizes
the full mode for reload_config; locate and edit the string literal used in the
command help table or the help generation for the reload_config command to make
this change.
- Around line 13839-13842: The socket handler is returning an unconditional
success string instead of the real outcome from
GhosttyApp.reloadConfiguration(soft:source:); change reloadConfiguration to
return a clear result (e.g., an enum or string indicating .skipped, .soft,
.full/fallback) and update the v2MainSync block in TerminalController to capture
that return value from GhosttyApp.shared.reloadConfiguration(soft:source:) and
build the response string from it (e.g., "OK Reloaded config (soft)", "OK
Reloaded config (full)", or "No config loaded, skipped") so callers see the
actual reload outcome.
---
Nitpick comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 91-135: cmuxConfigPaths duplicates logic from
GhosttyTerminalView.cmuxAppSupportConfigURLs; extract the shared behavior into a
single helper (e.g., a new static method like CmuxConfig.helperConfigPaths or
extension on FileManager) that encapsulates paths(for:) and hasConfig(_:) logic
and the debug-bundle fallback, then have both cmuxConfigPaths and
GhosttyTerminalView.cmuxAppSupportConfigURLs call that helper; ensure the helper
uses cmuxReleaseBundleIdentifier and
SocketControlSettings.isDebugLikeBundleIdentifier and preserves the same
validation (attributesOfItem, FileAttributeType.typeRegular, size > 0).
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1259-1267: The function currently returns the full arrays from
configURLs(for:) (e.g., currentURLs and releaseURLs) even when hasConfig(...)
only checks existence, causing downstream code (which loads every returned URL)
to attempt to load missing/empty paths; change the code to return only the
existing/non-empty URLs by filtering the result of configURLs(for:) (e.g.,
filter out nil/empty or non-existent file URLs) before returning in both the
currentBundleIdentifier and releaseBundleIdentifier branches (or extract a small
helper like filteredConfigURLs(from:)) so that callers only receive valid URLs;
keep uses of hasConfig(...) for the quick existence check if desired but ensure
the returned array is the filtered set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc28f4de-feda-4113-bdcc-6d34c2ac367b
📒 Files selected for processing (5)
CLI/cmux.swiftSources/AppDelegate.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/TerminalController.swift
| focus_pane <pane-id|index> - Focus a pane | ||
| focus_surface_by_panel <panel_id> - Focus surface by panel ID | ||
| close_surface [id|idx] - Close surface (collapse split) | ||
| reload_config [soft] - Reload Ghostty config and refresh terminals |
There was a problem hiding this comment.
Advertise the full mode here too.
Line 13831 also accepts full, so reload_config [soft] is currently understating the supported syntax. Either document [soft|full] or drop the alias.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` at line 9667, The help text for the
reload_config command currently advertises only "reload_config [soft]" but the
implementation accepts "full" as well; update the help string that contains
"reload_config [soft] - Reload Ghostty config and refresh terminals"
to reflect the supported syntax (e.g. "reload_config [soft|full]" or
"reload_config [soft|full] (alias: reload_config)") so the documentation matches
the implementation that recognizes the full mode for reload_config; locate and
edit the string literal used in the command help table or the help generation
for the reload_config command to make this change.
| v2MainSync { | ||
| GhosttyApp.shared.reloadConfiguration(soft: soft, source: "socket.reload_config") | ||
| } | ||
| return soft ? "OK Reloaded config (soft)" : "OK Reloaded config" |
There was a problem hiding this comment.
Return the actual reload result instead of unconditional success.
In Sources/GhosttyTerminalView.swift:1383-1427, GhosttyApp.reloadConfiguration(soft:source:) can skip reloading entirely and a soft request falls back to a full reload when no config is loaded yet. This branch still returns "OK Reloaded config" / "OK Reloaded config (soft)" unconditionally, so callers can be told the requested mode succeeded when it did not. Please have reloadConfiguration report its real outcome/mode and build the socket response from that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 13839 - 13842, The socket
handler is returning an unconditional success string instead of the real outcome
from GhosttyApp.reloadConfiguration(soft:source:); change reloadConfiguration to
return a clear result (e.g., an enum or string indicating .skipped, .soft,
.full/fallback) and update the v2MainSync block in TerminalController to capture
that return value from GhosttyApp.shared.reloadConfiguration(soft:source:) and
build the response string from it (e.g., "OK Reloaded config (soft)", "OK
Reloaded config (full)", or "No config loaded, skipped") so callers see the
actual reload outcome.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
CLI/cmux.swift (2)
5961-5975:⚠️ Potential issue | 🟠 MajorDo not swallow filesystem failures in override clearing.
At Line 5964 and Line 5972,
try?suppresses real I/O errors, sothemes clearcan report success without actually clearing the override.Suggested fix
private func clearManagedThemeOverride() throws -> URL { let fileManager = FileManager.default let configURL = try cmuxThemeOverrideConfigURL() - guard let existingContents = try? String(contentsOf: configURL, encoding: .utf8) else { - return configURL - } + let existingContents: String + do { + existingContents = try String(contentsOf: configURL, encoding: .utf8) + } catch { + let nsError = error as NSError + if nsError.domain == NSCocoaErrorDomain && nsError.code == NSFileReadNoSuchFileError { + return configURL + } + throw error + } let strippedContents = removingManagedThemeOverride(from: existingContents) .trimmingCharacters(in: .whitespacesAndNewlines) if strippedContents.isEmpty { - try? fileManager.removeItem(at: configURL) + do { + try fileManager.removeItem(at: configURL) + } catch { + let nsError = error as NSError + if !(nsError.domain == NSCocoaErrorDomain && nsError.code == NSFileNoSuchFileError) { + throw error + } + } } else { try strippedContents.appending("\n").write(to: configURL, atomically: true, encoding: .utf8) } return configURL }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 5961 - 5975, In clearManagedThemeOverride, don't swallow I/O errors: replace the silent `try? String(contentsOf: configURL, ...)` with a do-try-catch that returns configURL only for a "file not found" / no-such-file error and rethrows any other error, and replace the `try? fileManager.removeItem(at: configURL)` and the conditional write path so they use plain `try` (or rethrow on failure) instead of `try?`, allowing failures from FileManager.removeItem(at:) and String.write(to:atomically:encoding:) to propagate; refer to the clearManagedThemeOverride function, cmuxThemeOverrideConfigURL(), FileManager.removeItem(at:), and String.write(to:atomically:encoding:) when making the change.
5980-5985:⚠️ Potential issue | 🟡 MinorRemove all managed theme blocks, not just the first one.
At Line 5982,
range(of:options:)only removes the first# cmux themes start/endblock. Stale blocks can remain and still affect precedence.Suggested hardening
private func removingManagedThemeOverride(from contents: String) -> String { let pattern = #"(?ms)\n?# cmux themes start\n.*?\n# cmux themes end\n?"# - guard let range = contents.range(of: pattern, options: [.regularExpression]) else { - return contents - } - return contents.replacingCharacters(in: range, with: "") + return contents.replacingOccurrences( + of: pattern, + with: "", + options: [.regularExpression] + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 5980 - 5985, The current removingManagedThemeOverride(from:) uses contents.range(of:options:) which only finds the first match; replace all managed theme blocks by running a global regex replacement instead. Inside removingManagedThemeOverride(from:) compile the same pattern into an NSRegularExpression (or use String.replacingOccurrences(of: pattern, options: .regularExpression)) and call replacingMatches / replacingOccurrences to remove all matches (replace with empty string) so every "# cmux themes start" ... "# cmux themes end" block is removed, not just the first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 5961-5975: In clearManagedThemeOverride, don't swallow I/O errors:
replace the silent `try? String(contentsOf: configURL, ...)` with a do-try-catch
that returns configURL only for a "file not found" / no-such-file error and
rethrows any other error, and replace the `try? fileManager.removeItem(at:
configURL)` and the conditional write path so they use plain `try` (or rethrow
on failure) instead of `try?`, allowing failures from
FileManager.removeItem(at:) and String.write(to:atomically:encoding:) to
propagate; refer to the clearManagedThemeOverride function,
cmuxThemeOverrideConfigURL(), FileManager.removeItem(at:), and
String.write(to:atomically:encoding:) when making the change.
- Around line 5980-5985: The current removingManagedThemeOverride(from:) uses
contents.range(of:options:) which only finds the first match; replace all
managed theme blocks by running a global regex replacement instead. Inside
removingManagedThemeOverride(from:) compile the same pattern into an
NSRegularExpression (or use String.replacingOccurrences(of: pattern, options:
.regularExpression)) and call replacingMatches / replacingOccurrences to remove
all matches (replace with empty string) so every "# cmux themes start" ... "#
cmux themes end" block is removed, not just the first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55c8cd83-a7a5-4d63-845b-77729b78ce70
📒 Files selected for processing (4)
CLI/cmux.swiftdocs/ghostty-fork.mdghosttyscripts/reload.sh
✅ Files skipped from review due to trivial changes (1)
- ghostty
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae24dad89f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .appendingPathComponent(Self.cmuxThemeOverrideBundleIdentifier, isDirectory: true) | ||
| .appendingPathComponent("config.ghostty", isDirectory: false) |
There was a problem hiding this comment.
cmuxThemeOverrideConfigURL() always writes to ~/Library/Application Support/com.cmuxterm.app/config.ghostty, so cmux themes set/clear from non-release builds (notably staging) updates the release config instead of the running app’s config. The app reload path uses GhosttyApp.cmuxAppSupportConfigURLs (which prefers the current bundle’s app-support directory and only falls back for debug-like IDs), and SocketControlSettings.isDebugLikeBundleIdentifier excludes staging, so staging users get a successful CLI response with no applied theme change while unintentionally modifying production defaults.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69a286bf57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let type = attrs[.type] as? FileAttributeType, | ||
| type == .typeRegular, | ||
| let size = attrs[.size] as? NSNumber else { |
There was a problem hiding this comment.
The new hasConfig predicate only treats .typeRegular files as valid, so a cmux config managed via symlink (common with dotfile managers) is now treated as missing. That causes the app to skip loading the intended app-support override (or fall back to release config for debug-like bundles), which is a regression from the previous size-based check that accepted symlinked config paths.
Useful? React with 👍 / 👎.
| if commandArgs.isEmpty { | ||
| if shouldUseInteractiveThemePicker(jsonOutput: jsonOutput) { | ||
| try runInteractiveThemes() | ||
| return |
There was a problem hiding this comment.
When cmux themes is run in a TTY with no subcommand, it always attempts the interactive picker and exits on missing helper instead of falling back to a normal theme list. In this commit the helper is only staged via scripts/reload.sh, so build paths that don't run that script can ship without Resources/bin/ghostty, making the default cmux themes command fail for end users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
5978-5994:⚠️ Potential issue | 🟠 MajorDon’t report
themes clearas successful when filesystem work failed.Read/remove still use
try?, so permission errors and other real failures are swallowed while the command printsOKand posts a reload. Non-ENOENT failures should surface here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 5978 - 5994, The clearManagedThemeOverride() function currently swallows IO errors via try? when reading the config and when removing the file, causing commands to report success even on permission or other failures; change the read and remove paths so that reading the config uses try and propagates errors from cmuxThemeOverrideConfigURL() and String(contentsOf:encoding:) (but treat ENOENT/missing file as non-fatal), and replace the try? fileManager.removeItem(at:) with a proper try and error handling that only suppresses a "file not found" error while surfacing other errors to the caller; keep the final write as a throwing write so callers see failures and continue to use removingManagedThemeOverride(from:) to compute strippedContents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 6006-6013: The reloadThemesIfPossible() notification is posted
with a specific object (bundleIdentifier) but the AppDelegate subscriber uses
Bundle.main.bundleIdentifier, causing dropped notifications; update
reloadThemesIfPossible() (and/or currentCmuxAppBundleIdentifier usage) to post
the DistributedNotificationCenter notification with object: nil so subscribers
receive it regardless of bundle, or alternatively implement a fan-out that
iterates supported bundle IDs and posts the same notification for each; ensure
you reference Self.cmuxThemesReloadNotificationName and keep
ThemeReloadStatus(requested:targetBundleIdentifier:) behavior consistent when
changing the object semantics.
- Around line 5958-5975: The current use of `try? String(contentsOf: ...) ?? ""`
in writeManagedThemeOverride silently treats permission/encoding errors as an
empty file and can overwrite unrelated config; replace that with a do/catch that
first attempts to read the file at the URL from cmuxThemeOverrideConfigURL(),
allowing a missing-file error (NSFileReadNoSuchFileError / Cocoa file-not-found)
to fall back to empty, but rethrow or surface any other read error so the caller
won't unknowingly clobber the file; keep the rest of the function
(removingManagedThemeOverride, building the block with Self.cmuxThemesBlockStart
/ cmuxThemesBlockEnd, writing nextContents) unchanged.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 5978-5994: The clearManagedThemeOverride() function currently
swallows IO errors via try? when reading the config and when removing the file,
causing commands to report success even on permission or other failures; change
the read and remove paths so that reading the config uses try and propagates
errors from cmuxThemeOverrideConfigURL() and String(contentsOf:encoding:) (but
treat ENOENT/missing file as non-fatal), and replace the try?
fileManager.removeItem(at:) with a proper try and error handling that only
suppresses a "file not found" error while surfacing other errors to the caller;
keep the final write as a throwing write so callers see failures and continue to
use removingManagedThemeOverride(from:) to compute strippedContents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 946a6238-c241-4e6b-b386-354325886bd1
📒 Files selected for processing (2)
CLI/cmux.swiftghostty
✅ Files skipped from review due to trivial changes (1)
- ghostty
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
1328-1331:⚠️ Potential issue | 🟡 MinorUse
dlog()for this debug event.Line 1329 is a debug event logged with
Self.initLog(...); this should bedlog(...)within the existing#if DEBUGblock.🛠️ Proposed change
`#if` DEBUG - Self.initLog( + dlog( "loaded cmux app support ghostty config from: \(urls.map(\.path).joined(separator: ", "))" ) `#endif`As per coding guidelines
**/*.swift: “All debug events must be logged using thedlog()function ... wrapped in#if DEBUG/#endif.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1328 - 1331, Replace the debug logging call to Self.initLog(...) with dlog(...) inside the existing `#if` DEBUG block so the event uses the project's standard debug logger; modify the call in GhosttyTerminalView (the line invoking Self.initLog with the "loaded cmux app support ghostty config..." message) to call dlog(...) with the same interpolated message string, leaving the surrounding `#if` DEBUG / `#endif` and message content unchanged.
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
1239-1269: Return only existing config files fromcmuxAppSupportConfigURLs.Right now
hasConfigchecks “any file exists,” butloadCmuxAppSupportGhosttyConfigIfNeededthen loads all returned URLs. That can include missing paths and creates avoidable noisy load attempts.💡 Proposed refactor
- func hasConfig(_ urls: [URL]) -> Bool { - urls.contains { url in + func existingConfigURLs(for bundleIdentifier: String) -> [URL] { + configURLs(for: bundleIdentifier).filter { url in guard let attrs = try? fileManager.attributesOfItem(atPath: url.path), let type = attrs[.type] as? FileAttributeType, type == .typeRegular, let size = attrs[.size] as? NSNumber else { return false } return size.intValue > 0 } } - let currentURLs = configURLs(for: currentBundleIdentifier) - if hasConfig(currentURLs) { + let currentURLs = existingConfigURLs(for: currentBundleIdentifier) + if !currentURLs.isEmpty { return currentURLs } if SocketControlSettings.isDebugLikeBundleIdentifier(currentBundleIdentifier) { - let releaseURLs = configURLs(for: releaseBundleIdentifier) - if hasConfig(releaseURLs) { + let releaseURLs = existingConfigURLs(for: releaseBundleIdentifier) + if !releaseURLs.isEmpty { return releaseURLs } }Also applies to: 1322-1325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1239 - 1269, The current logic uses configURLs(for:) + hasConfig(_:) which only checks if any config file exists but then callers (e.g., loadCmuxAppSupportGhosttyConfigIfNeeded) attempt to load all returned URLs, causing noisy attempts for missing files; change to a function (or update configURLs/) that returns only the existing, non-empty file URLs (check FileManager/file attributes like in hasConfig: .type == .typeRegular and .size > 0) and use that filtered array in place of currentURLs/releaseURLs so callers only iterate real files; apply the same change pattern at the other occurrence referenced (around lines 1322-1325).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1328-1331: Replace the debug logging call to Self.initLog(...)
with dlog(...) inside the existing `#if` DEBUG block so the event uses the
project's standard debug logger; modify the call in GhosttyTerminalView (the
line invoking Self.initLog with the "loaded cmux app support ghostty config..."
message) to call dlog(...) with the same interpolated message string, leaving
the surrounding `#if` DEBUG / `#endif` and message content unchanged.
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1239-1269: The current logic uses configURLs(for:) + hasConfig(_:)
which only checks if any config file exists but then callers (e.g.,
loadCmuxAppSupportGhosttyConfigIfNeeded) attempt to load all returned URLs,
causing noisy attempts for missing files; change to a function (or update
configURLs/) that returns only the existing, non-empty file URLs (check
FileManager/file attributes like in hasConfig: .type == .typeRegular and .size >
0) and use that filtered array in place of currentURLs/releaseURLs so callers
only iterate real files; apply the same change pattern at the other occurrence
referenced (around lines 1322-1325).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4d1dbbd-9940-4de6-87ef-6bacc70c2779
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f074f810e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let parts = trimmed.split(separator: "=", maxSplits: 1).map(String.init) | ||
| guard parts.count == 2 else { continue } | ||
| guard parts[0].trimmingCharacters(in: .whitespacesAndNewlines) == "theme" else { continue } | ||
|
|
There was a problem hiding this comment.
cmux themes list and the partial themes set --light/--dark path depend on currentThemeSelection(), but lastThemeDirective only parses direct theme = ... lines and ignores include directives, so any theme defined in a recursively included config is treated as missing. In that setup the command reports incorrect current light/dark values and can overwrite one side without preserving the actual active value, which breaks include-based Ghostty configs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
CLI/cmux.swift (3)
6006-6013:⚠️ Potential issue | 🟠 MajorReload notification scoping can miss the running app
Posting with a specific
objectbundle ID can silently drop reload events when CLI/app bundle IDs diverge (tagged/debug/standalone combinations), while the app observer filters by bundle ID.Suggested fix
DistributedNotificationCenter.default().post( name: Notification.Name(Self.cmuxThemesReloadNotificationName), - object: bundleIdentifier, + object: nil, userInfo: nil )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 6006 - 6013, The reloadThemesIfPossible() method posts a distributed notification using the bundle ID as the object, which can cause the app's observer to miss events when bundle IDs diverge; change the post call to broadcast (object: nil) and include the resolved bundleIdentifier in userInfo (e.g., ["bundleIdentifier": bundleIdentifier]) so receivers can read the intended target while no longer being filtered out by the notification object; update ThemeReloadStatus creation (still using targetBundleIdentifier: bundleIdentifier) so callers keep the existing behavior.
5978-5995:⚠️ Potential issue | 🟠 MajorDo not swallow clear failures in filesystem operations
themes clearcurrently ignores read/remove failures viatry?, so it can print success even when nothing was cleared.Suggested fix
- guard let existingContents = try? String(contentsOf: configURL, encoding: .utf8) else { - return configURL - } + let existingContents: String + do { + existingContents = try String(contentsOf: configURL, encoding: .utf8) + } catch { + let nsError = error as NSError + if nsError.domain == NSCocoaErrorDomain && nsError.code == NSFileReadNoSuchFileError { + return configURL + } + throw error + } @@ - try? fileManager.removeItem(at: configURL) + do { + try fileManager.removeItem(at: configURL) + } catch { + let nsError = error as NSError + if !(nsError.domain == NSCocoaErrorDomain && nsError.code == NSFileNoSuchFileError) { + throw error + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 5978 - 5995, The function clearManagedThemeOverride currently swallows filesystem errors with try? when reading and removing the config, which can report success even when operations fail; change the implementation in clearManagedThemeOverride to perform proper error handling: use try for reading the config URL returned by cmuxThemeOverrideConfigURL (or catch only the "file not found" read error and return configURL in that case), and replace the try? fileManager.removeItem call with a throwing try so removal failures are propagated (or caught and rethrown with context). Ensure writes use try as well and include context in thrown errors; keep removingManagedThemeOverride usage unchanged but stop silencing IO errors.
5958-5975:⚠️ Potential issue | 🟠 MajorPreserve existing config on read errors in managed write path
(try? String(contentsOf: ...)) ?? ""treats unreadable/invalid files as empty and can overwrite unrelated user config.Suggested fix
- let existingContents = (try? String(contentsOf: configURL, encoding: .utf8)) ?? "" + let existingContents: String + do { + existingContents = try String(contentsOf: configURL, encoding: .utf8) + } catch { + let nsError = error as NSError + if nsError.domain == NSCocoaErrorDomain && nsError.code == NSFileReadNoSuchFileError { + existingContents = "" + } else { + throw error + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 5958 - 5975, In writeManagedThemeOverride, avoid treating unreadable/invalid config files as empty (the current "(try? String(contentsOf: ...)) ?? \"\"") because that causes overwrites; instead, perform a proper try when reading the existing contents (use try String(contentsOf: ...) inside writeManagedThemeOverride), and if the read fails propagate/throw the error (or bail without writing) so the existing config is preserved; update the logic around existingContents/strippedContents and the write path accordingly and keep references to cmuxThemeOverrideConfigURL, writeManagedThemeOverride, and removingManagedThemeOverride to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 6006-6013: The reloadThemesIfPossible() method posts a distributed
notification using the bundle ID as the object, which can cause the app's
observer to miss events when bundle IDs diverge; change the post call to
broadcast (object: nil) and include the resolved bundleIdentifier in userInfo
(e.g., ["bundleIdentifier": bundleIdentifier]) so receivers can read the
intended target while no longer being filtered out by the notification object;
update ThemeReloadStatus creation (still using targetBundleIdentifier:
bundleIdentifier) so callers keep the existing behavior.
- Around line 5978-5995: The function clearManagedThemeOverride currently
swallows filesystem errors with try? when reading and removing the config, which
can report success even when operations fail; change the implementation in
clearManagedThemeOverride to perform proper error handling: use try for reading
the config URL returned by cmuxThemeOverrideConfigURL (or catch only the "file
not found" read error and return configURL in that case), and replace the try?
fileManager.removeItem call with a throwing try so removal failures are
propagated (or caught and rethrown with context). Ensure writes use try as well
and include context in thrown errors; keep removingManagedThemeOverride usage
unchanged but stop silencing IO errors.
- Around line 5958-5975: In writeManagedThemeOverride, avoid treating
unreadable/invalid config files as empty (the current "(try? String(contentsOf:
...)) ?? \"\"") because that causes overwrites; instead, perform a proper try
when reading the existing contents (use try String(contentsOf: ...) inside
writeManagedThemeOverride), and if the read fails propagate/throw the error (or
bail without writing) so the existing config is preserved; update the logic
around existingContents/strippedContents and the write path accordingly and keep
references to cmuxThemeOverrideConfigURL, writeManagedThemeOverride, and
removingManagedThemeOverride to locate the change.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:2129">
P2: `object: nil` removes notification scoping, and this observer currently reloads config for every matching distributed notification instead of only the targeted bundle.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| self, | ||
| selector: #selector(handleThemesReloadNotification(_:)), | ||
| name: CmuxThemeNotifications.reloadConfig, | ||
| object: nil, |
There was a problem hiding this comment.
P2: object: nil removes notification scoping, and this observer currently reloads config for every matching distributed notification instead of only the targeted bundle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 2129:
<comment>`object: nil` removes notification scoping, and this observer currently reloads config for every matching distributed notification instead of only the targeted bundle.</comment>
<file context>
@@ -2126,7 +2126,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
selector: #selector(handleThemesReloadNotification(_:)),
name: CmuxThemeNotifications.reloadConfig,
- object: Bundle.main.bundleIdentifier,
+ object: nil,
suspensionBehavior: .deliverImmediately
)
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bd199b625
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @objc func handleThemesReloadNotification(_ notification: Notification) { | ||
| DispatchQueue.main.async { | ||
| GhosttyApp.shared.reloadConfiguration(source: "distributed.cmux.themes") | ||
| } |
There was a problem hiding this comment.
reloadThemesIfPossible() posts a bundleIdentifier in userInfo to target a specific app instance, but handleThemesReloadNotification ignores that payload and reloads unconditionally. When multiple cmux variants are running (e.g., release + tagged/staging), cmux themes set/clear in one instance triggers full config reloads in all of them, defeating the targeting logic and causing unnecessary cross-instance reload churn.
Useful? React with 👍 / 👎.
| guard let attributes = try? fileManager.attributesOfItem(atPath: path), | ||
| let type = attributes[.type] as? FileAttributeType, | ||
| type == .typeRegular, | ||
| let size = attributes[.size] as? NSNumber else { |
There was a problem hiding this comment.
cmuxConfigPaths treats a config as present only when its file type is .typeRegular, so symlinked app-support configs are treated as missing. In debug-like bundles this incorrectly falls back to release paths, causing GhosttyConfig.loadFromDisk to parse the wrong config/theme source even when the current bundle has a valid symlinked config (a common dotfile-manager setup).
Useful? React with 👍 / 👎.
…-command Add cmux themes command
This branch was successfully deployed
1 participant
Follow-up to #1314.
cmux themesto list bundled and discovered Ghostty themescmux themes setandcmux themes clear, including separate light and dark defaults~/Library/Application Support/com.cmuxterm.app/config.ghosttyand reload the running app after updatesVerification:
./scripts/reload.sh --tag feat-cmux-themes-commandCatppuccin LatteandCatppuccin Mocha, and clear the override again/tmp/cmux-debug-feat-cmux-themes-command.logrecordedreload.config.surfaceRefresh source=distributed.cmux.themes count=1after set and clearSummary by cubic
Adds the new
cmux themesCLI to list, set, and clear Ghostty themes with separate light/dark defaults and an interactive picker (TTY) with live preview. Stores a managed override with safe writes and hot-reloads the app so changes apply immediately in both the app and CLI.New Features
cmux themeswithlist,set, andclear(supports--light/--dark) and a no-arg interactive picker in a TTY that previews and applies to light/dark/both using the bundledghosttyhelper.~/Library/Application Support/com.cmuxterm.app/config.ghostty, loaded last in both app and CLI; debug/tagged builds fall back to the release bundle’s override when no local override is present.reload_config(supports soft reload).Bug Fixes
ghostty, improve footer contrast, respect system light/dark, and skip theme detection in cmux mode for faster, conflict‑free startup.Written for commit cd04bb8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests