Add Ghostty theme picker to the command palette#1149
Add Ghostty theme picker to the command palette#1149debgotwired wants to merge 5 commits intomanaflow-ai:mainfrom
Conversation
|
@debgotwired is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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 Ghostty Theme submenu to the command palette and implements theme discovery, selection, and config mutation APIs, with unit tests and new localized strings for Ghostty theme UI and loading/empty states. Changes
Sequence DiagramsequenceDiagram
actor User
participant Palette as "Command Palette\n(ContentView)"
participant Ghostty as "GhosttyConfig"
participant FS as "File System"
User->>Palette: Open palette / invoke Ghostty Theme submenu
Palette->>Palette: set submenu = ghosttyThemes
Palette->>Ghostty: discoverThemeNames()
Ghostty->>FS: scan theme search directories
FS-->>Ghostty: return theme files
Ghostty-->>Palette: return theme list
Palette->>User: display theme list
User->>Palette: choose theme "X"
Palette->>Ghostty: applyTheme("X")
Ghostty->>FS: read/update config file(s)
FS-->>Ghostty: confirm write
Ghostty-->>Palette: return applied theme value
Palette->>Palette: clear submenu / dismiss
Palette-->>User: theme applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a Ghostty Theme picker to the command palette: a focused submenu that discovers installed themes from standard Ghostty resource paths, marks the currently active theme with a ✓, and applies a selection by updating the Ghostty config file and reloading.
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CommandPalette as Command Palette (ContentView)
participant Task as Task.detached
participant GhosttyConfig
participant Disk as Config File (Disk)
User->>CommandPalette: Select "Ghostty Theme…"
CommandPalette->>CommandPalette: enterCommandPaletteGhosttyThemeMenu()
CommandPalette->>Task: Task.detached { discoverThemeNames + load() }
Task->>Disk: contentsOfDirectory (theme dirs)
Task->>Disk: read config file (resolveThemeName)
Task-->>CommandPalette: await MainActor.run { update state }
CommandPalette->>CommandPalette: scheduleCommandPaletteResultsRefresh()
CommandPalette-->>User: Show theme list with ✓ on current theme
User->>CommandPalette: Select a theme
CommandPalette->>GhosttyConfig: applyTheme(themeName, colorScheme)
GhosttyConfig->>Disk: read existing config
GhosttyConfig->>GhosttyConfig: updatedThemeValueForSelection()
GhosttyConfig->>Disk: write updated config (atomically)
CommandPalette->>GhosttyConfig: invalidateLoadCache()
CommandPalette->>CommandPalette: reloadConfiguration()
Last reviewed commit: faeb59f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
|
|
||
| for (index, themeName) in themes.enumerated() { | ||
| let isCurrentTheme = currentTheme?.localizedCaseInsensitiveCompare(themeName) == .orderedSame |
There was a problem hiding this comment.
✓ indicator never fires for light/dark variant themes
GhosttyConfig.load().theme returns the raw config value exactly as written in the file (e.g. "light:Nord,dark:Nord Dark"). The comparison against individual theme names like "Nord" will always be false for any user whose config uses the light:X,dark:Y variant syntax, so the ✓ indicator will never show for those users.
The fix is to resolve the raw value through GhosttyConfig.resolveThemeName before comparing:
let resolvedTheme = currentTheme.flatMap { raw in
GhosttyConfig.resolveThemeName(
from: raw,
preferredColorScheme: GhosttyConfig.currentColorSchemePreference()
)
}
let isCurrentTheme = resolvedTheme?.localizedCaseInsensitiveCompare(themeName) == .orderedSame
Sources/ContentView.swift
Outdated
| private func commandPaletteGhosttyThemeCommands() -> [CommandPaletteCommand] { | ||
| let currentTheme = GhosttyConfig.load(useCache: false).theme?.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let themes = GhosttyConfig.discoverThemeNames() |
There was a problem hiding this comment.
Synchronous disk I/O on the main thread
Both GhosttyConfig.load(useCache: false) (which reads config files from disk) and GhosttyConfig.discoverThemeNames() (which walks one or more directories with contentsOfDirectory) are called synchronously here. This function is invoked on the main thread as part of the command palette rendering pass. For users with Ghostty's full bundled theme library (hundreds of files) spread across multiple search directories, or a networked/slow volume, this can produce perceptible UI jank each time the submenu is opened or its search query changes.
Consider loading themes asynchronously (e.g., with a Task) and caching the result for the lifetime of the submenu session, updating the list once the async work completes.
Sources/GhosttyConfig.swift
Outdated
|
|
||
| for child in children { | ||
| let values = try? child.resourceValues(forKeys: [.isRegularFileKey, .isDirectoryKey]) | ||
| guard values?.isRegularFile == true || values?.isDirectory == false else { continue } |
There was a problem hiding this comment.
Guard condition also admits non-regular, non-directory entries
guard values?.isRegularFile == true || values?.isDirectory == false else { continue }The condition isDirectory == false is true for symlinks, sockets, pipes, and other special files in addition to regular files. If a user's theme directory contains a non-file entry (e.g., a named socket left by another tool), it would be added to the discovered list and shown in the palette even though it is not a valid theme.
The intent appears to be "include only regular files", which a simpler guard expresses more directly:
| guard values?.isRegularFile == true || values?.isDirectory == false else { continue } | |
| guard values?.isRegularFile == true else { continue } |
Sources/ContentView.swift
Outdated
| let isCurrentTheme = currentTheme?.localizedCaseInsensitiveCompare(themeName) == .orderedSame | ||
| commands.append( | ||
| CommandPaletteCommand( | ||
| id: "palette.ghosttyTheme.\(themeName.lowercased())", |
There was a problem hiding this comment.
Command ID collision for themes with same lowercased name but different original casing
discoverThemeNames deduplicates using a folded+lowercased normalized key (e.g., both "Nord" and "nord" → key "nord"), so only the first-seen variant survives. However, the command ID here uses only Swift.lowercased(), which is locale-dependent (Turkish-locale "I".lowercased() → "ı"). For the common ASCII case this is benign, but in edge cases the generated ID may not match the normalization key used during dedup, making the pairing fragile.
More importantly, if discoverThemeNames ever emits two themes whose names only differ in non-ASCII characters that lowercased() collapses to the same string (but folding(options:…) kept distinct), the two entries would get identical command IDs, which could cause unexpected palette behaviour. A safer approach is to use a hash or a stable base64 encoding of the original name as the ID suffix.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 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:485">
P2: Restrict theme discovery to regular files only; this condition also allows non-directory special entries, which can add invalid items to the theme list.</violation>
<violation number="2" location="Sources/GhosttyConfig.swift:539">
P2: `upsertConfigValue` replaces the first matching key, which can leave a later duplicate `theme` entry overriding the newly selected theme.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:3908">
P2: Resolve variant theme expressions before comparing to individual theme names; otherwise `light:...,dark:...` values will never mark the active theme correctly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
3928-3945: Reset the submenu when the query leaves command mode.Right now the Ghostty theme submenu is only cleared by Back/dismiss. If the user backspaces the leading
>while inside this submenu, the UI switches to workspace search but the hidden submenu state survives, so typing>again drops them back into Ghostty Themes instead of the root command list. ClearingcommandPaletteCommandSubmenuwhen the prefix is removed would keep the visible mode and internal mode aligned.♻️ Suggested change
.onChange(of: commandPaletteQuery) { _ in + if !commandPaletteQuery.hasPrefix(Self.commandPaletteCommandsPrefix) { + commandPaletteCommandSubmenu = nil + } commandPaletteSelectedResultIndex = 0 commandPaletteSelectionAnchorCommandID = nil commandPaletteHoveredResultIndex = nil commandPaletteScrollTargetIndex = nil commandPaletteScrollTargetAnchor = nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3928 - 3945, The submenu state (commandPaletteCommandSubmenu) isn't cleared when the user backspaces out of the command prefix, leaving the hidden Ghostty submenu active; fix this by clearing commandPaletteCommandSubmenu when the query leaves command mode (i.e., when commandPaletteQuery no longer starts with Self.commandPaletteCommandsPrefix) — either call exitCommandPaletteGhosttyThemeMenu or set commandPaletteCommandSubmenu = nil in the routine that updates commandPaletteQuery (the query-change handler), and then call scheduleCommandPaletteResultsRefresh(...) / resetCommandPaletteSearchFocus() / syncCommandPaletteDebugStateForObservedWindow() as appropriate to keep UI state consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 538-553: The loop in GhosttyConfig.parse(_:) replaces the first
active occurrence of a key (e.g., "theme") instead of the last one, causing
last-write-wins semantics to be violated; change the logic to target the last
active matching line by either iterating lines.indices.reversed() or recording
the matching index (e.g., lastMatchIndex) during the forward pass and performing
the replacement after the loop, then set replaced = true and break/exit
appropriately so the final active key is updated (reference: variables lines,
trimmed, parts, rawKey, indentation and the replacement assignment lines[index]
= ...).
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 3928-3945: The submenu state (commandPaletteCommandSubmenu) isn't
cleared when the user backspaces out of the command prefix, leaving the hidden
Ghostty submenu active; fix this by clearing commandPaletteCommandSubmenu when
the query leaves command mode (i.e., when commandPaletteQuery no longer starts
with Self.commandPaletteCommandsPrefix) — either call
exitCommandPaletteGhosttyThemeMenu or set commandPaletteCommandSubmenu = nil in
the routine that updates commandPaletteQuery (the query-change handler), and
then call scheduleCommandPaletteResultsRefresh(...) /
resetCommandPaletteSearchFocus() /
syncCommandPaletteDebugStateForObservedWindow() as appropriate to keep UI state
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 078f2575-3971-4b22-ab83-fa6a161075a6
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
|
Pushed a follow-up fix pass in What changed:
Tests/verification here:
Attribution note: the variant indicator / regular-file issues were specifically called out by cubic, the variant-overwrite and main-thread I/O risks came from Greptile, and the last-write-wins + submenu-reset issues came from CodeRabbit. @codex review |
@debgotwired I have started the AI code review. It will take a few minutes to complete. |
🧩 Analysis chain🏁 Script executed: Length of output: 24417
✅ All substantive issues addressed
Minor observations (no action required)
Test coverage assessmentThe five new tests cover all the newly introduced logic paths:
The The PR is in good shape. ✅ Actions performedReview triggered.
|
| try fileManager.createDirectory(at: url.deletingLastPathComponent(), withIntermediateDirectories: true) | ||
|
|
||
| let existing = (try? String(contentsOf: url, encoding: .utf8)) ?? "" | ||
| var lines = configLines(from: existing) |
There was a problem hiding this comment.
Silent read failure silently destroys existing config
Both applyTheme (line 562) and upsertConfigValue (line 506) use (try? String(contentsOf: url, encoding: .utf8)) ?? "" to read the existing config. If the file exists but cannot be decoded — for example because it was saved in a non-UTF-8 encoding — the error is silently swallowed, existing becomes "", and the function immediately overwrites the entire file with just the new single-key entry, destroying all other settings.
The safer pattern is to distinguish "file does not exist" (treat as empty) from "file exists but is unreadable" (propagate the error):
let existing: String
if fileManager.fileExists(atPath: path) {
existing = try String(contentsOf: url, encoding: .utf8)
} else {
existing = ""
}The same fix applies in applyTheme at line 562.
| selectedTheme: String, | ||
| preferredColorScheme: ColorSchemePreference | ||
| ) -> String { |
There was a problem hiding this comment.
Theme name containing newline or comma corrupts the config file
nextThemeValue is written verbatim into the config line. A theme filename containing a newline character (\n) would cause "theme = \(nextThemeValue)" to become a multi-line string, injecting extra content into the config. Commas or colons in the theme name (e.g., "Night: Dark") would produce an unparseable variant string like "light:Night: Dark,dark:…".
Because themeName originates from filenames discovered on disk, these characters are legal at the OS level even if unusual. A simple guard before writing would prevent the issue:
guard !themeName.contains(where: { $0.isNewline || $0 == "," }) else {
throw CocoaError(.fileWriteUnknown) // or a dedicated error type
}| static func writableConfigPath( | ||
| fileManager: FileManager = .default, | ||
| searchPaths: [String]? = nil | ||
| ) -> String { | ||
| let resolvedSearchPaths = searchPaths ?? configSearchPaths() | ||
| for path in resolvedSearchPaths where fileManager.fileExists(atPath: path) { | ||
| return path | ||
| } | ||
| return resolvedSearchPaths.first ?? NSString(string: "~/.config/ghostty/config.ghostty").expandingTildeInPath | ||
| } | ||
|
|
||
| @discardableResult |
There was a problem hiding this comment.
writableConfigPath fallback is unreachable dead code
configSearchPaths() always returns a hardcoded list of four non-empty strings, so resolvedSearchPaths can never be empty and resolvedSearchPaths.first will always return a non-nil value. The ?? NSString(string: "~/.config/ghostty/config.ghostty").expandingTildeInPath branch is dead code. Additionally, the fallback path (config.ghostty) differs from what .first would return (config, without the .ghostty extension), which could cause confusion if the reachability assumption ever changes.
return resolvedSearchPaths.first! // safe: configSearchPaths() is always non-emptyOr just inline the first path if you want to be explicit.
![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: faeb59fcdb
ℹ️ 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 resolvedSearchPaths = searchPaths ?? configSearchPaths() | ||
| for path in resolvedSearchPaths where fileManager.fileExists(atPath: path) { | ||
| return path | ||
| } |
There was a problem hiding this comment.
loadFromDisk parses all config paths in order, so later files override earlier ones, but writableConfigPath currently returns the first existing file. In setups where both ~/.config/ghostty/config and ~/.config/ghostty/config.ghostty (or App Support equivalents) exist, theme changes are written to the lower-precedence file and then immediately overridden on reload, so selecting a theme appears to have no effect.
Useful? React with 👍 / 👎.
Sources/ContentView.swift
Outdated
| commandPaletteGhosttyThemeLoadTask = Task.detached(priority: .userInitiated) { | ||
| let preferredColorScheme = GhosttyConfig.currentColorSchemePreference() | ||
| let resolvedCurrentTheme = ( | ||
| GhosttyConfig.load(useCache: false).theme.map { |
There was a problem hiding this comment.
This detached background task calls GhosttyConfig.currentColorSchemePreference(), which reads NSApp?.effectiveAppearance (AppKit UI state), and GhosttyConfig.load(useCache: false) also defaults through that same path when no scheme is passed. Calling AppKit appearance APIs off-main can trigger thread-safety violations and intermittently resolve the wrong active variant; capture the color scheme on the main actor before launching the detached work and pass it through explicitly.
Useful? React with 👍 / 👎.
| contributions.append( | ||
| CommandPaletteCommandContribution( | ||
| commandId: "palette.ghosttyThemeMenu", | ||
| title: constant(String(localized: "command.ghosttyThemeMenu.title", defaultValue: "Ghostty Theme…")), |
There was a problem hiding this comment.
Per /workspace/cmux/AGENTS.md, all user-facing strings must be localized in Resources/Localizable.xcstrings for supported languages, but this commit introduces new keys (for example command.ghosttyThemeMenu.title) without adding catalog entries. I checked the catalog in this commit and these keys are absent, so the new Ghostty theme palette UI falls back to English instead of localized text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 3 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/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:4010">
P2: This detached task reads AppKit appearance state off the main actor. Resolve the color scheme on `MainActor` and pass it into `GhosttyConfig.load` to avoid background-thread AppKit access.</violation>
</file>
<file name="Sources/GhosttyConfig.swift">
<violation number="1" location="Sources/GhosttyConfig.swift:488">
P2: `writableConfigPath` picks the first existing config file, which conflicts with read precedence and can make theme updates ineffective when multiple config files exist.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
3906-3953: Surface an explicit no-themes state.Because the Back row is always present, this function never returns an empty result set after loading, so the Ghostty empty-state copy at Lines 3279-3281 does not render for the blank-query/no-themes case. On a machine with zero discoverable themes, the submenu just shows “Back” with no explanation.
💡 One way to make the zero-themes case explicit
if commandPaletteGhosttyThemesAreLoading && themes.isEmpty { commands.append( CommandPaletteCommand( id: "palette.ghosttyTheme.loading", rank: 1, title: String(localized: "command.ghosttyTheme.loading.title", defaultValue: "Loading Ghostty themes…"), subtitle: String(localized: "command.ghosttyTheme.loading.subtitle", defaultValue: "Ghostty Theme"), shortcutHint: nil, keywords: ["loading", "theme", "ghostty"], dismissOnRun: false, action: {} ) ) return commands } + + if themes.isEmpty { + commands.append( + CommandPaletteCommand( + id: "palette.ghosttyTheme.empty", + rank: 1, + title: String(localized: "command.ghosttyTheme.empty.title", defaultValue: "No Ghostty themes found"), + subtitle: String(localized: "command.ghosttyTheme.empty.subtitle", defaultValue: "Ghostty Theme"), + shortcutHint: nil, + keywords: [], + dismissOnRun: false, + action: {} + ) + ) + return commands + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3906 - 3953, The Back row is always present so when themes is empty after loading the menu shows only "Back"; add an explicit empty-state CommandPaletteCommand when commandPaletteGhosttyThemesAreLoading is false and themes.isEmpty. In the function that builds commands (which uses commandPaletteGhosttyThemesAreLoading, themes, CommandPaletteCommand and commandPaletteGhosttyThemeCommandID), after the loading check add a branch that appends a command with a unique id like "palette.ghosttyTheme.empty", a user-facing title/subtitle indicating "No Ghostty themes found" (localized), appropriate keywords, and a no-op action (or dismissOnRun true) so the submenu displays explanatory copy instead of only Back. Ensure this branch returns commands like the loading branch does.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 4009-4018: The detached task is reading appearance-sensitive
helpers on a background thread; compute the appearance/preferredColorScheme on
the main actor first and pass that value into the detached task so GhosttyConfig
methods won't call NSApp?.effectiveAppearance on a background thread.
Concretely: call await MainActor.run {
GhosttyConfig.currentColorSchemePreference() } (or use Task { `@MainActor` in ...
} ) to obtain preferredColorScheme, then start
commandPaletteGhosttyThemeLoadTask = Task.detached { ... } using that captured
preferredColorScheme when calling GhosttyConfig.load(useCache: false) or
GhosttyConfig.resolveThemeName(...) so no default param falls back to
currentColorSchemePreference() on a background thread; refer to
commandPaletteGhosttyThemeLoadTask,
GhosttyConfig.currentColorSchemePreference(), GhosttyConfig.load(useCache:), and
GhosttyConfig.resolveThemeName(...) to locate and update the code.
In `@Sources/GhosttyConfig.swift`:
- Around line 483-492: writableConfigPath currently returns the first existing
search path, which selects a lower-precedence config file; change it to pick the
highest-precedence config by scanning resolvedSearchPaths in reverse (or using
last(where:)) and returning the first existing path found, and if none exist
return the highest-precedence target path (resolvedSearchPaths.last ?? default
"~/.config/ghostty/config.ghostty" expandingTildeInPath); update the function
writableConfigPath to use reversed iteration or last(where:) so
applyTheme()/upsertConfigValue() writes to the correct (highest-precedence)
file.
- Around line 461-464: The normalization currently folds both diacritics and
case (folding(options: [.diacriticInsensitive, .caseInsensitive], ...)) which
causes "Cafe" and "Café" to be treated as duplicates; change the folding call
used to compute normalized (the variable normalized) to only use
.caseInsensitive (remove .diacriticInsensitive) so the deduplication via
seen.insert(normalized) is case-insensitive only and preserves diacritic
differences.
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 3906-3953: The Back row is always present so when themes is empty
after loading the menu shows only "Back"; add an explicit empty-state
CommandPaletteCommand when commandPaletteGhosttyThemesAreLoading is false and
themes.isEmpty. In the function that builds commands (which uses
commandPaletteGhosttyThemesAreLoading, themes, CommandPaletteCommand and
commandPaletteGhosttyThemeCommandID), after the loading check add a branch that
appends a command with a unique id like "palette.ghosttyTheme.empty", a
user-facing title/subtitle indicating "No Ghostty themes found" (localized),
appropriate keywords, and a no-op action (or dismissOnRun true) so the submenu
displays explanatory copy instead of only Back. Ensure this branch returns
commands like the loading branch does.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb57ea87-4697-497b-86d5-75cc9bd5899d
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Sources/GhosttyConfig.swift (2)
461-464:⚠️ Potential issue | 🟡 MinorKeep theme deduplication case-insensitive only.
This normalization also folds diacritics, so
CafeandCafécollapse to one entry even though this path is only supposed to deduplicate case-insensitively.💡 Suggested fix
- let normalized = name - .folding(options: [.diacriticInsensitive, .caseInsensitive], locale: .current) - .lowercased() + let normalized = name + .folding(options: [.caseInsensitive], locale: .current) + .lowercased()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 461 - 464, The code is over-normalizing theme names by folding diacritics as well as case, collapsing distinct names like "Cafe" and "Café"; change the normalization to be case-insensitive only: replace the folding call with a locale-aware lowercase conversion (e.g. use name.lowercased(with: .current) or name.lowercased()) so that only case differences are normalized before inserting into seen (refer to the variables/functions normalized, name, and seen.insert).
483-491:⚠️ Potential issue | 🟠 MajorWrite to the highest-precedence config file.
loadFromDisk()applies these paths in order, so the last existing file wins. Returning the first existing path here meansapplyTheme()/upsertConfigValue()can update a lower-precedence config and have the change immediately overridden on reload.💡 Suggested fix
let resolvedSearchPaths = searchPaths ?? configSearchPaths() - for path in resolvedSearchPaths where fileManager.fileExists(atPath: path) { + for path in resolvedSearchPaths.reversed() where fileManager.fileExists(atPath: path) { return path } - return resolvedSearchPaths.first ?? NSString(string: "~/.config/ghostty/config.ghostty").expandingTildeInPath + return resolvedSearchPaths.last + ?? NSString(string: "~/.config/ghostty/config.ghostty").expandingTildeInPath🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 483 - 491, writableConfigPath currently returns the first existing path from resolvedSearchPaths, but loadFromDisk treats later paths as higher precedence so you must return the highest-precedence (last existing) file instead; update writableConfigPath to find and return the last existing path (e.g. iterate resolvedSearchPaths.reversed() or use resolvedSearchPaths.last(where: { fileManager.fileExists(atPath: $0) })) and if none exist return the expanded default path as before so applyTheme()/upsertConfigValue() write to the correct (highest-precedence) config file.Sources/ContentView.swift (1)
4007-4018:⚠️ Potential issue | 🟠 MajorKeep the appearance lookup on the main actor.
This detached loader still calls
GhosttyConfig.currentColorSchemePreference()directly, andGhosttyConfig.load(useCache: false)falls back to the same helper whenpreferredColorSchemeis omitted. That meansNSApp?.effectiveAppearanceis still being read off the main thread here.🛠️ Suggested fix
commandPaletteGhosttyThemeLoadTask?.cancel() commandPaletteGhosttyThemesAreLoading = true - commandPaletteGhosttyThemeLoadTask = Task.detached(priority: .userInitiated) { - let preferredColorScheme = GhosttyConfig.currentColorSchemePreference() + let preferredColorScheme = GhosttyConfig.currentColorSchemePreference() + commandPaletteGhosttyThemeLoadTask = Task.detached(priority: .userInitiated) { let resolvedCurrentTheme = ( - GhosttyConfig.load(useCache: false).theme.map { + GhosttyConfig.load( + preferredColorScheme: preferredColorScheme, + useCache: false + ).theme.map { GhosttyConfig.resolveThemeName( from: $0, preferredColorScheme: preferredColorScheme ) }The script below should show the detached call site and the
GhosttyConfig.load→currentColorSchemePreference→NSApp?.effectiveAppearancefallback chain.#!/bin/bash set -euo pipefail sed -n '4007,4018p' Sources/ContentView.swift printf '\n---\n' sed -n '58,73p' Sources/GhosttyConfig.swift printf '\n---\n' sed -n '241,246p' Sources/GhosttyConfig.swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 4007 - 4018, The detached Task is calling GhosttyConfig.currentColorSchemePreference() and GhosttyConfig.load(useCache:) off the main actor, which reads NSApp?.effectiveAppearance; fix by performing the appearance lookup and theme resolution on the main actor before creating commandPaletteGhosttyThemeLoadTask: capture let preferredColorScheme = await MainActor.run { GhosttyConfig.currentColorSchemePreference() } and compute resolvedCurrentTheme = await MainActor.run { /* call GhosttyConfig.load(useCache: false) and resolve/trim theme */ } (or compute both inside a single MainActor.run block), then pass those plain values into Task.detached so the detached work no longer touches AppKit/NSApp; update references to commandPaletteGhosttyThemesAreLoading and commandPaletteGhosttyThemeLoadTask accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 4021-4030: Wrap the async theme load with a request token check
similar to commandPaletteSearchRequestID: generate and store a unique request id
(e.g. commandPaletteGhosttyThemeRequestID) when starting the load, capture it in
the Task, and before applying results (both before awaiting MainActor.run and
inside MainActor.run) verify that the captured id still equals the current
commandPaletteGhosttyThemeRequestID and that commandPaletteCommandSubmenu ==
.ghosttyThemes; only then assign commandPaletteGhosttyThemeNames,
commandPaletteGhosttyResolvedCurrentTheme, and
commandPaletteGhosttyThemesAreLoading, and only clear
commandPaletteGhosttyThemeLoadTask / commandPaletteGhosttyThemeRequestID if the
ids still match so stale tasks cannot overwrite a newer session.
- Around line 3906-3932: The Back and loading sentinel commands
("palette.ghosttyTheme.back" and "palette.ghosttyTheme.loading") are being added
as normal searchable items which allows a non-matching query to hide them and
show an empty Ghostty state; change the builder so these two entries are not
subject to fuzzy filtering: either (A) add the Back command and the loading
sentinel after you run the query filter (i.e., append them to the commands array
post-filter) so they always appear regardless of the search term, or (B) tag
them as non-searchable/alwaysVisible (e.g., set an isSearchable=false or
alwaysVisible flag) and ensure the filtering logic skips items with that flag;
use the existing identifiers commandPaletteGhosttyThemesAreLoading, themes,
exitCommandPaletteGhosttyThemeMenu(), and the command IDs above to locate where
to modify the append/filter logic and ensure the empty-state is suppressed while
commandPaletteGhosttyThemesAreLoading is true.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 4007-4018: The detached Task is calling
GhosttyConfig.currentColorSchemePreference() and GhosttyConfig.load(useCache:)
off the main actor, which reads NSApp?.effectiveAppearance; fix by performing
the appearance lookup and theme resolution on the main actor before creating
commandPaletteGhosttyThemeLoadTask: capture let preferredColorScheme = await
MainActor.run { GhosttyConfig.currentColorSchemePreference() } and compute
resolvedCurrentTheme = await MainActor.run { /* call
GhosttyConfig.load(useCache: false) and resolve/trim theme */ } (or compute both
inside a single MainActor.run block), then pass those plain values into
Task.detached so the detached work no longer touches AppKit/NSApp; update
references to commandPaletteGhosttyThemesAreLoading and
commandPaletteGhosttyThemeLoadTask accordingly.
In `@Sources/GhosttyConfig.swift`:
- Around line 461-464: The code is over-normalizing theme names by folding
diacritics as well as case, collapsing distinct names like "Cafe" and "Café";
change the normalization to be case-insensitive only: replace the folding call
with a locale-aware lowercase conversion (e.g. use name.lowercased(with:
.current) or name.lowercased()) so that only case differences are normalized
before inserting into seen (refer to the variables/functions normalized, name,
and seen.insert).
- Around line 483-491: writableConfigPath currently returns the first existing
path from resolvedSearchPaths, but loadFromDisk treats later paths as higher
precedence so you must return the highest-precedence (last existing) file
instead; update writableConfigPath to find and return the last existing path
(e.g. iterate resolvedSearchPaths.reversed() or use
resolvedSearchPaths.last(where: { fileManager.fileExists(atPath: $0) })) and if
none exist return the expanded default path as before so
applyTheme()/upsertConfigValue() write to the correct (highest-precedence)
config file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f9bf696-15dd-433e-a9f0-9b543a42975c
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
| commands.append( | ||
| CommandPaletteCommand( | ||
| id: "palette.ghosttyTheme.back", | ||
| rank: 0, | ||
| title: String(localized: "command.ghosttyTheme.back.title", defaultValue: "Back"), | ||
| subtitle: String(localized: "command.ghosttyTheme.back.subtitle", defaultValue: "Ghostty Themes"), | ||
| shortcutHint: nil, | ||
| keywords: ["back", "theme", "ghostty"], | ||
| dismissOnRun: false, | ||
| action: { exitCommandPaletteGhosttyThemeMenu() } | ||
| ) | ||
| ) | ||
|
|
||
| if commandPaletteGhosttyThemesAreLoading && themes.isEmpty { | ||
| commands.append( | ||
| CommandPaletteCommand( | ||
| id: "palette.ghosttyTheme.loading", | ||
| rank: 1, | ||
| title: String(localized: "command.ghosttyTheme.loading.title", defaultValue: "Loading Ghostty themes…"), | ||
| subtitle: String(localized: "command.ghosttyTheme.loading.subtitle", defaultValue: "Ghostty Theme"), | ||
| shortcutHint: nil, | ||
| keywords: ["loading", "theme", "ghostty"], | ||
| dismissOnRun: false, | ||
| action: {} | ||
| ) | ||
| ) | ||
| return commands |
There was a problem hiding this comment.
Keep submenu chrome out of the searchable result set.
Because Back and the loading sentinel are emitted as normal commands, typing a non-matching query during discovery can hide both rows and render the Ghostty empty state even though loading is still in progress. Pin those rows outside fuzzy search, or suppress the empty state while commandPaletteGhosttyThemesAreLoading is true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 3906 - 3932, The Back and loading
sentinel commands ("palette.ghosttyTheme.back" and
"palette.ghosttyTheme.loading") are being added as normal searchable items which
allows a non-matching query to hide them and show an empty Ghostty state; change
the builder so these two entries are not subject to fuzzy filtering: either (A)
add the Back command and the loading sentinel after you run the query filter
(i.e., append them to the commands array post-filter) so they always appear
regardless of the search term, or (B) tag them as non-searchable/alwaysVisible
(e.g., set an isSearchable=false or alwaysVisible flag) and ensure the filtering
logic skips items with that flag; use the existing identifiers
commandPaletteGhosttyThemesAreLoading, themes,
exitCommandPaletteGhosttyThemeMenu(), and the command IDs above to locate where
to modify the append/filter logic and ensure the empty-state is suppressed while
commandPaletteGhosttyThemesAreLoading is true.
- writableConfigPath: pick highest-precedence (last) config file, not first, so theme changes aren't overridden by later config files - Move AppKit appearance read (currentColorSchemePreference) to main actor before Task.detached to avoid background-thread AppKit access - Remove .diacriticInsensitive from theme dedup normalization so themes differing only in diacritics (e.g. Café vs Cafe) stay distinct - Replace silent try? config reads with explicit exists-check to avoid destroying config contents when file exists but is unreadable - Validate theme names reject newlines and commas that would corrupt the config file format - Add empty-themes state to submenu when no themes are discovered - Add all new localization keys to Localizable.xcstrings catalog - Add tests for writableConfigPath precedence and theme name validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 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="Resources/Localizable.xcstrings">
<violation number="1" location="Resources/Localizable.xcstrings:72910">
P2: New user-facing localization keys were added with English only; add Japanese entries for these keys to keep supported locale coverage consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/ContentView.swift (2)
4023-4049:⚠️ Potential issue | 🟡 MinorGate async theme loads with a request id.
Cancellation alone still leaves a race here: an older detached load can survive long enough to publish after a newer submenu session starts, and it can clear
commandPaletteGhosttyThemeLoadTaskfor the active request. MirrorcommandPaletteSearchRequestIDwith a dedicated Ghostty theme request id and verify it before applying results or clearing the stored task.Suggested pattern
`@State` private var commandPaletteGhosttyThemeLoadTask: Task<Void, Never>? +@State private var commandPaletteGhosttyThemeRequestID: UInt64 = 0 private func refreshCommandPaletteGhosttyThemeData(force: Bool = false) { if !force, (!commandPaletteGhosttyThemeNames.isEmpty || commandPaletteGhosttyThemesAreLoading) { return } commandPaletteGhosttyThemeLoadTask?.cancel() + commandPaletteGhosttyThemeRequestID &+= 1 + let requestID = commandPaletteGhosttyThemeRequestID commandPaletteGhosttyThemesAreLoading = true let preferredColorScheme = GhosttyConfig.currentColorSchemePreference() commandPaletteGhosttyThemeLoadTask = Task.detached(priority: .userInitiated) { let resolvedCurrentTheme = ( GhosttyConfig.load( @@ )?.trimmingCharacters(in: .whitespacesAndNewlines) let themes = GhosttyConfig.discoverThemeNames() guard !Task.isCancelled else { return } await MainActor.run { - guard commandPaletteCommandSubmenu == .ghosttyThemes else { return } + guard commandPaletteGhosttyThemeRequestID == requestID, + commandPaletteCommandSubmenu == .ghosttyThemes else { return } commandPaletteGhosttyThemeNames = themes commandPaletteGhosttyResolvedCurrentTheme = resolvedCurrentTheme commandPaletteGhosttyThemesAreLoading = false - commandPaletteGhosttyThemeLoadTask = nil + if commandPaletteGhosttyThemeRequestID == requestID { + commandPaletteGhosttyThemeLoadTask = nil + } scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: true) syncCommandPaletteDebugStateForObservedWindow() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 4023 - 4049, Introduce a dedicated request id (e.g., commandPaletteGhosttyThemeRequestID) and increment/set it immediately before creating commandPaletteGhosttyThemeLoadTask; capture that id in the detached Task and, before touching any shared state (setting commandPaletteGhosttyThemeNames, commandPaletteGhosttyResolvedCurrentTheme, commandPaletteGhosttyThemesAreLoading, commandPaletteGhosttyThemeLoadTask, and calling scheduleCommandPaletteResultsRefresh/syncCommandPaletteDebugStateForObservedWindow), verify the captured id still equals the current commandPaletteGhosttyThemeRequestID on the main actor. Only clear commandPaletteGhosttyThemeLoadTask and reset/clear the request id when the ids match; likewise bail early from the Task if Task.isCancelled or the ids mismatch to avoid older tasks overwriting newer submenu sessions. Ensure the id is updated for each new submenu session invocation that starts a theme load.
3906-3948:⚠️ Potential issue | 🟡 MinorKeep submenu chrome out of fuzzy search.
Backand the loading/empty sentinels are still emitted as normal commands, so a non-matching query can filter them out. That makes the Ghostty empty state appear while discovery is still running and can hide the only in-menu way back to the parent command list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3906 - 3948, The back/loading/empty sentinel commands (ids "palette.ghosttyTheme.back", "palette.ghosttyTheme.loading", "palette.ghosttyTheme.empty") are being treated as normal searchable commands and get filtered out by fuzzy queries; change them to be non-searchable chrome so they always appear in the submenu by: add a property/flag on CommandPaletteCommand (e.g. searchable: Bool or isMenuChrome: Bool) and set it to false for these three commands where they are created, then update the command-palette filtering logic to skip fuzzy matching for commands with that flag (still respecting rank/order and dismissOnRun), ensuring exitCommandPaletteGhosttyThemeMenu() remains the action for the back item and the loading/empty sentinels remain no-op actions.
🤖 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/ContentView.swift`:
- Around line 4023-4049: Introduce a dedicated request id (e.g.,
commandPaletteGhosttyThemeRequestID) and increment/set it immediately before
creating commandPaletteGhosttyThemeLoadTask; capture that id in the detached
Task and, before touching any shared state (setting
commandPaletteGhosttyThemeNames, commandPaletteGhosttyResolvedCurrentTheme,
commandPaletteGhosttyThemesAreLoading, commandPaletteGhosttyThemeLoadTask, and
calling
scheduleCommandPaletteResultsRefresh/syncCommandPaletteDebugStateForObservedWindow),
verify the captured id still equals the current
commandPaletteGhosttyThemeRequestID on the main actor. Only clear
commandPaletteGhosttyThemeLoadTask and reset/clear the request id when the ids
match; likewise bail early from the Task if Task.isCancelled or the ids mismatch
to avoid older tasks overwriting newer submenu sessions. Ensure the id is
updated for each new submenu session invocation that starts a theme load.
- Around line 3906-3948: The back/loading/empty sentinel commands (ids
"palette.ghosttyTheme.back", "palette.ghosttyTheme.loading",
"palette.ghosttyTheme.empty") are being treated as normal searchable commands
and get filtered out by fuzzy queries; change them to be non-searchable chrome
so they always appear in the submenu by: add a property/flag on
CommandPaletteCommand (e.g. searchable: Bool or isMenuChrome: Bool) and set it
to false for these three commands where they are created, then update the
command-palette filtering logic to skip fuzzy matching for commands with that
flag (still respecting rank/order and dismissOnRun), ensuring
exitCommandPaletteGhosttyThemeMenu() remains the action for the back item and
the loading/empty sentinels remain no-op actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e460a13-d12d-4a2c-9ba3-fe015fc2046e
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
- Fix task reference leak: always clear loading state and task ref before the guard check so they're reset even when submenu changed - Add early cancellation checks in detached task to avoid wasted disk I/O when user exits the theme menu during loading - Extract resetCommandPaletteGhosttyThemeState() helper to DRY the 5 duplicate state cleanup blocks across onChange, exit, reset, dismiss - Use defer for invalidateLoadCache lock to match cachedLoad pattern and prevent lock leak on unexpected throw - Remove dead `replaced` variable from upsertConfigValue; the if-let/else on lastMatchIndex already handles both branches - Strengthen theme name validation: reject empty names, path traversal (../), and directory separators (/) in addition to newlines and commas - Add tests for path traversal, slash, and empty name rejection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review fixes — completeTwo commits pushed addressing all findings from the 4 AI reviewers (Greptile, Cubic, CodeRabbit, Codex) plus preemptive hardening based on each reviewer's documented patterns and historical behavior on this repo. Commit 1:
|
| Reviewer | Focus | What they check in this repo |
|---|---|---|
| Greptile | Security → Correctness → Architecture → Performance | Traces cross-file code paths, flags redundant I/O/layout, dead code, error swallowing |
| Cubic | Hard-to-find bugs via specialized micro-agents | Thread safety, policy violations (AGENTS.md), lock patterns, unreachable code |
| CodeRabbit | Broad coverage with 30+ integrated linters | Force unwraps, main-thread safety, localization, code duplication, SwiftLint |
| Codex | Only P0/P1 (precision over recall) | AGENTS.md policy compliance, backward compat, provable security issues |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/ContentView.swift (2)
3902-3928:⚠️ Potential issue | 🟡 MinorKeep the submenu chrome out of fuzzy filtering.
Backand the loading row are still emitted as normal searchable commands, so a non-matching query can hide both rows and show the Ghostty empty state even while discovery is still running. These rows should stay visible regardless of the query, or the empty state should be suppressed whilecommandPaletteGhosttyThemesAreLoadingis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3902 - 3928, The Back and Loading rows are being treated as normal searchable commands and get filtered out; update the construction so those two items (CommandPaletteCommand with id "palette.ghosttyTheme.back" and id "palette.ghosttyTheme.loading") are flagged as menu chrome / non-searchable (e.g. add an isMenuChrome or searchable: false property) and update the command-palette filtering logic to skip filtering for commands where searchable is false (or alternatively, when commandPaletteGhosttyThemesAreLoading is true short-circuit and return the commands list before running fuzzy filtering); reference CommandPaletteCommand, commandPaletteGhosttyThemesAreLoading, themes.isEmpty, exitCommandPaletteGhosttyThemeMenu(), and the commands array when making the change.
4023-4055:⚠️ Potential issue | 🟡 MinorGate theme-load completion with a request token.
Cancellation alone still isn't enough here. An older detached load can reach
MainActor.runafter a newer submenu session starts, and this block clearscommandPaletteGhosttyThemesAreLoading/commandPaletteGhosttyThemeLoadTaskbefore it validates freshness. That lets stale work hide the spinner or overwrite the active session's theme list/current mark. Mirror the existingcommandPaletteSearchRequestIDpattern and verify the token before mutating state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 4023 - 4055, Introduce a per-request token for the ghostty-theme load (mirroring commandPaletteSearchRequestID) to ensure stale detached tasks cannot clear or overwrite state: add a commandPaletteGhosttyThemeRequestID (e.g., UUID or incrementing Int) and set it immediately before creating commandPaletteGhosttyThemeLoadTask; capture that token in the Task and, before any MainActor.run mutations (and before clearing commandPaletteGhosttyThemeLoadTask/commandPaletteGhosttyThemesAreLoading), verify the captured token still matches the global commandPaletteGhosttyThemeRequestID—only then update commandPaletteGhosttyThemeNames, commandPaletteGhosttyResolvedCurrentTheme and call scheduleCommandPaletteResultsRefresh/syncCommandPaletteDebugStateForObservedWindow; keep existing cancellation checks (Task.isCancelled) and ensure clearing of the task/loader state is gated by the token match so newer submenu sessions are not affected by stale loads.
🧹 Nitpick comments (1)
Sources/GhosttyConfig.swift (1)
88-95: Consider usingdeferfor consistency withinvalidateLoadCache.This function manually pairs
lock()andunlock()calls, whileinvalidateLoadCachenow usesdefer. While the current code is correct (the dictionary assignment won't throw), usingdeferwould maintain consistency across the caching methods.♻️ Optional refactor for consistency
private static func storeCachedLoad( _ config: GhosttyConfig, for colorScheme: ColorSchemePreference ) { loadCacheLock.lock() + defer { loadCacheLock.unlock() } cachedConfigsByColorScheme[colorScheme] = config - loadCacheLock.unlock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 88 - 95, The storeCachedLoad function currently calls loadCacheLock.lock() and loadCacheLock.unlock() manually; change it to mirror invalidateLoadCache by calling loadCacheLock.lock() then using defer { loadCacheLock.unlock() } immediately after locking so the unlock is guaranteed and consistent across caching helpers (refer to storeCachedLoad and invalidateLoadCache).
🤖 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/ContentView.swift`:
- Around line 3902-3928: The Back and Loading rows are being treated as normal
searchable commands and get filtered out; update the construction so those two
items (CommandPaletteCommand with id "palette.ghosttyTheme.back" and id
"palette.ghosttyTheme.loading") are flagged as menu chrome / non-searchable
(e.g. add an isMenuChrome or searchable: false property) and update the
command-palette filtering logic to skip filtering for commands where searchable
is false (or alternatively, when commandPaletteGhosttyThemesAreLoading is true
short-circuit and return the commands list before running fuzzy filtering);
reference CommandPaletteCommand, commandPaletteGhosttyThemesAreLoading,
themes.isEmpty, exitCommandPaletteGhosttyThemeMenu(), and the commands array
when making the change.
- Around line 4023-4055: Introduce a per-request token for the ghostty-theme
load (mirroring commandPaletteSearchRequestID) to ensure stale detached tasks
cannot clear or overwrite state: add a commandPaletteGhosttyThemeRequestID
(e.g., UUID or incrementing Int) and set it immediately before creating
commandPaletteGhosttyThemeLoadTask; capture that token in the Task and, before
any MainActor.run mutations (and before clearing
commandPaletteGhosttyThemeLoadTask/commandPaletteGhosttyThemesAreLoading),
verify the captured token still matches the global
commandPaletteGhosttyThemeRequestID—only then update
commandPaletteGhosttyThemeNames, commandPaletteGhosttyResolvedCurrentTheme and
call
scheduleCommandPaletteResultsRefresh/syncCommandPaletteDebugStateForObservedWindow;
keep existing cancellation checks (Task.isCancelled) and ensure clearing of the
task/loader state is gated by the token match so newer submenu sessions are not
affected by stale loads.
---
Nitpick comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 88-95: The storeCachedLoad function currently calls
loadCacheLock.lock() and loadCacheLock.unlock() manually; change it to mirror
invalidateLoadCache by calling loadCacheLock.lock() then using defer {
loadCacheLock.unlock() } immediately after locking so the unlock is guaranteed
and consistent across caching helpers (refer to storeCachedLoad and
invalidateLoadCache).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b676149-2f2f-4c30-820d-e1dbc39be35e
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
- Gate async theme load results with a request token (commandPaletteGhosttyThemeLoadRequestID) so a stale detached task from a previous submenu session cannot overwrite the active session's data after an exit→re-enter sequence. Mirrors the existing commandPaletteSearchRequestID pattern used by the search system. - Suppress the empty-state text while themes are still loading so typing a non-matching query during discovery does not flash "No Ghostty themes match your search." before results arrive. - Add Japanese translations for all 12 new localization keys to maintain locale coverage parity with existing command palette entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Final review round — all 24 comments addressedTwo additional commits pushed ( Commit 3:
|
| Reviewer | Comments | All Fixed |
|---|---|---|
| Greptile | 7 inline | ✅ (6 fixed, 1 N/A — nil-coalescing fallback is defensive, not a bug) |
| Cubic | 6 inline | ✅ |
| CodeRabbit | 8 inline | ✅ |
| Codex | 3 inline | ✅ |
Tests added: writableConfigPath precedence, theme name with newline/comma/slash/path-traversal/empty rejection (6 total).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
3906-3932:⚠️ Potential issue | 🟡 MinorKeep the Back/loading rows out of fuzzy filtering.
palette.ghosttyTheme.backandpalette.ghosttyTheme.loadingare still emitted as normal searchable commands, so a non-matching query during discovery can hide both rows and leave the submenu blank while loading. Those sentinel rows should be appended after filtering, or marked always-visible/non-searchable so search only applies to real theme entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3906 - 3932, The back ("palette.ghosttyTheme.back") and loading ("palette.ghosttyTheme.loading") sentinel CommandPaletteCommand rows are being added before fuzzy filtering so they can be hidden by a non-matching query; modify the builder that produces the commands (the block that uses commandPaletteGhosttyThemesAreLoading, themes and exitCommandPaletteGhosttyThemeMenu) so that real theme entries are filtered first, and then append the sentinel back/loading commands afterward (or alternatively set a non-searchable/always-visible flag on those CommandPaletteCommand instances if such a property exists) to ensure they remain visible regardless of the user's query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 3977-3978: The forced refresh path is not seeding the
command-preview synchronously, so
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: true) can still
render an empty/partial commands view; ensure that when forceSearchCorpusRefresh
is true you perform a synchronous seed/update of the command-preview path before
returning. Concretely: in the call-site where
refreshCommandPaletteGhosttyThemeData(force: true) and
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: true) are
invoked, make the seed of the commands preview happen synchronously (either by
calling the sync seeding helper or adding a synchronous code path in
scheduleCommandPaletteResultsRefresh keyed on forceSearchCorpusRefresh) so the
commands preview path is updated immediately (so UI shows Back/Loading or loaded
rows instead of []). Target the functions refreshCommandPaletteGhosttyThemeData
and scheduleCommandPaletteResultsRefresh and ensure forceSearchCorpusRefresh
triggers immediate seeding on the main actor/thread before any async detached
search runs.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 3906-3932: The back ("palette.ghosttyTheme.back") and loading
("palette.ghosttyTheme.loading") sentinel CommandPaletteCommand rows are being
added before fuzzy filtering so they can be hidden by a non-matching query;
modify the builder that produces the commands (the block that uses
commandPaletteGhosttyThemesAreLoading, themes and
exitCommandPaletteGhosttyThemeMenu) so that real theme entries are filtered
first, and then append the sentinel back/loading commands afterward (or
alternatively set a non-searchable/always-visible flag on those
CommandPaletteCommand instances if such a property exists) to ensure they remain
visible regardless of the user's query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff825c56-4526-4b45-a08d-1f11adfc9e79
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/ContentView.swift
| refreshCommandPaletteGhosttyThemeData(force: true) | ||
| scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: true) |
There was a problem hiding this comment.
Honor forceSearchCorpusRefresh with a synchronous seed.
These forced Ghostty submenu refreshes still stay in the .commands scope, so scheduleCommandPaletteResultsRefresh can reuse the previous commands preview path and briefly render [] or only Back before the detached search finishes. That makes the submenu flash blank/partial content instead of immediately showing Back/Loading or the loaded theme rows.
💡 Minimal fix
private func scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: Bool = false) {
refreshCommandPaletteSearchCorpus(force: forceSearchCorpusRefresh)
@@
commandPalettePendingActivation = nil
cancelCommandPaletteSearch()
- if Self.commandPaletteShouldSynchronouslySeedResults(
+ if forceSearchCorpusRefresh || Self.commandPaletteShouldSynchronouslySeedResults(
hasVisibleResultsForScope: commandPaletteVisibleResultsScope == scope
) {
let matches = Self.commandPaletteResolvedSearchMatches(
searchCorpus: searchCorpus,
query: query,Also applies to: 4058-4060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 3977 - 3978, The forced refresh path
is not seeding the command-preview synchronously, so
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: true) can still
render an empty/partial commands view; ensure that when forceSearchCorpusRefresh
is true you perform a synchronous seed/update of the command-preview path before
returning. Concretely: in the call-site where
refreshCommandPaletteGhosttyThemeData(force: true) and
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh: true) are
invoked, make the seed of the commands preview happen synchronously (either by
calling the sync seeding helper or adding a synchronous code path in
scheduleCommandPaletteResultsRefresh keyed on forceSearchCorpusRefresh) so the
commands preview path is updated immediately (so UI shows Back/Loading or loaded
rows instead of []). Target the functions refreshCommandPaletteGhosttyThemeData
and scheduleCommandPaletteResultsRefresh and ensure forceSearchCorpusRefresh
triggers immediate seeding on the main actor/thread before any async detached
search runs.
There was a problem hiding this comment.
Good catch on the UX flash — this is a valid observation. However, I'm intentionally not addressing this in this PR for the following reasons:
-
Scope: The suggested fix modifies
scheduleCommandPaletteResultsRefresh, which is shared infrastructure used by every command palette submenu, not just the Ghostty theme picker. Changing the synchronous seeding logic there to key offforceSearchCorpusRefreshalters the rendering behavior for all submenus that pass that flag — a change with broader blast radius than this feature PR warrants. -
Pre-existing behavior: The "brief flash" pattern is how all existing submenus behave today when entered with
forceSearchCorpusRefresh: true. The theme submenu follows the same established contract. Fixing it in isolation for themes would be inconsistent; fixing it globally should be a deliberate, separately tested change. -
Risk vs. reward: The flash is sub-frame in practice (the sync corpus refresh happens on the same main actor call, so the search corpus is already populated before the detached search kicks off). The actual visible flicker, if any, is masked by the loading state we already display. Adding
forceSearchCorpusRefreshas a sync-seed trigger could cause unintended double-renders in other submenu flows.
Will track this as a follow-up UX polish item. Thanks for the thorough analysis.
There was a problem hiding this comment.
@debgotwired Thanks for the context and rationale — agreed on deferring this from the feature PR given the shared nature of scheduleCommandPaletteResultsRefresh.
If helpful, I can open a follow‑up issue that scopes a global, low‑risk improvement across all submenus. Proposed acceptance criteria:
- On forceSearchCorpusRefresh: true, the command list seeds synchronously with the up‑to‑date corpus before any detached search runs, so the first paint shows stable rows (e.g., Back + Loading or actual items) without a transient empty state.
- No regression to selection anchoring, pending activations, or requestID gating.
- Covered with a test that enters a submenu and asserts the first visible result set is non‑empty when loading is in progress.
Optionally, the solution could be:
- Either a new seed mode in scheduleCommandPaletteResultsRefresh (defaulting to current behavior) to keep backwards compatibility, or
- A small helper that synchronously materializes preview results from the refreshed corpus before kicking off the async search, used by all call sites that do a forced refresh.
Would you like me to file that issue and assign it to you?
✏️ Learnings added
Learnt from: debgotwired
Repo: manaflow-ai/cmux PR: 1149
File: Sources/ContentView.swift:3977-3978
Timestamp: 2026-03-10T09:33:29.814Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) is shared command‑palette infrastructure across all submenus. Do not change its sync‑seeding behavior within feature‑scoped PRs; treat brief initial flashes as consistent with existing submenus. Any UX improvement (e.g., synchronous seeding on forced corpus refresh) should be implemented and tested globally in a dedicated follow‑up PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/ContentView.swift:6055-6057
Timestamp: 2026-03-09T02:09:08.742Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), within CommandPaletteSwitcherSearchIndexer.metadataKeywordsForSearch(_:detail:), when tags are present, only add "tag" and "topic" to contextKeywords; do not append "claude" unconditionally to avoid false-positive switcher matches.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T00:05:51.436Z
Learning: Applies to **/GhosttyTerminalView.swift : `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers such as `Sources/Panels/TerminalPanelView.swift`
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:16.241Z
Learning: For manaflow-ai/cmux PR `#819` (Japanese i18n), keep scope limited to localization changes; UX enhancements like preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels should be handled in a separate follow-up issue.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
1 participant
Summary
Testing
Scope notes
Closes #526
Summary by cubic
Adds a Ghostty Theme picker to the command palette with a focused submenu that discovers installed themes, highlights the current one, and applies the selection by updating config and reloading Ghostty. Implements the command‑palette theme submenu from #526 and hardens async loading, validation, and config handling.
New Features
XDG_DATA_DIRS, and user dirs; case‑insensitive de‑dupe without diacritic folding; ignores non‑regular files. Marks the current theme and applies selection by updating the last active theme line, preserving light/dark variants and fallback; creates the config if missing, invalidates cache, and reloads.Bug Fixes
..,/); avoids clobbering unreadable configs.invalidateLoadCache; minor upsert cleanup. Adds tests for precedence, validation, discovery, and apply flows.Written for commit 3f37356. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests