Add Appearance settings tab - #7000
Closed
austinywang wants to merge 28 commits into
Closed
austinywang wants to merge 28 commits into
austinywang wants to merge 28 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 an Appearance settings section that centralizes terminal font, UI sizing, theme, sidebar matching, and workspace color controls. It also extends Ghostty config parsing/writing, updates navigation/search metadata, and removes the migrated controls from older settings sections. ChangesAppearance settings
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AppearanceSection
participant HostSettingsActions
participant CmuxGhosttyConfigSettingEditor
participant GhosttyApp
AppearanceSection->>HostSettingsActions: setTerminalFontFamily(...) / setTerminalFontSize(...)
HostSettingsActions->>CmuxGhosttyConfigSettingEditor: writeEditableSetting(...)
HostSettingsActions->>GhosttyApp: reloadConfiguration(reloadSource)
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
added 2 commits
June 26, 2026 16:48
…earance-settings-tab-like-warp-term # Conflicts: # .github/swift-file-length-budget.tsv
Contributor
Greptile SummaryThis PR introduces a dedicated Appearance settings tab that consolidates app theme, terminal font family/size, UI font scale, sidebar background, and workspace color controls from several disparate sections into one pane, backed by new Ghostty config read/write logic and live reload.
Confidence Score: 5/5Safe to merge. The previously reported compile errors and fallback font-family data-loss path are both resolved in the current HEAD. All three issues flagged in prior review rounds — dangling identifiers in AppSection, missing updatedTerminalFontFamilyContents, and silent overwrite of fallback font-family chains — are addressed and covered by new tests. The GhosttyConfig parsing changes are correct and localised. The one remaining concern runs off the main actor and does not affect correctness. MonospacedFontFamilyCatalog.swift — the per-family CoreText scan is worth revisiting for caching if users on font-heavy machines report a slow first open of the Appearance pane. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as AppearanceSection (@MainActor)
participant HA as HostSettingsActions
participant FCW as FontConfigWriter (actor)
participant CE as CmuxGhosttyConfigSettingEditor
participant GC as GhosttyConfig (cached)
UI->>HA: terminalFontFamily() [sync, init]
HA->>GC: load().fontFamily
GC-->>HA: "Menlo" (cached)
HA-->>UI: "Menlo"
UI->>UI: Task.detached → MonospacedFontFamilyCatalog().families()
UI-->>UI: terminalFontFamilies populated (off-main)
UI->>HA: setTerminalFontFamily("SF Mono") [async]
HA->>FCW: write(key: "font-family", value: "SF Mono")
FCW->>CE: writeEditableSetting(key:value:to:)
CE->>CE: updatedTerminalFontFamilyContents (preserve fallbacks)
CE-->>FCW: updated config string
FCW-->>HA: true
HA->>HA: reload open windows
HA-->>UI: true
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as AppearanceSection (@MainActor)
participant HA as HostSettingsActions
participant FCW as FontConfigWriter (actor)
participant CE as CmuxGhosttyConfigSettingEditor
participant GC as GhosttyConfig (cached)
UI->>HA: terminalFontFamily() [sync, init]
HA->>GC: load().fontFamily
GC-->>HA: "Menlo" (cached)
HA-->>UI: "Menlo"
UI->>UI: Task.detached → MonospacedFontFamilyCatalog().families()
UI-->>UI: terminalFontFamilies populated (off-main)
UI->>HA: setTerminalFontFamily("SF Mono") [async]
HA->>FCW: write(key: "font-family", value: "SF Mono")
FCW->>CE: writeEditableSetting(key:value:to:)
CE->>CE: updatedTerminalFontFamilyContents (preserve fallbacks)
CE-->>FCW: updated config string
FCW-->>HA: true
HA->>HA: reload open windows
HA-->>UI: true
Reviews (21): Last reviewed commit: "Avoid constructing fonts during monospac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SettingsNavigation.swift (1)
478-483: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the missing
app.globalFontMagnificationanchor mapping.The new
.appearancerow exists, and the package-side curated entry already declarespaths: ["app.globalFontMagnification"], but this dictionary never maps that path. In the app target,anchorID(forSettingsPath: "app.globalFontMagnification")will returnnil, so deep-link/highlight navigation for that row breaks.🩹 Suggested fix
"app.language": settingID(for: .app, idSuffix: "language"), "app.appearance": settingID(for: .appearance, idSuffix: "appearance"), + "app.globalFontMagnification": settingID(for: .appearance, idSuffix: "global-font-magnification"), "app.appIcon": settingID(for: .app, idSuffix: "app-icon"),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/SettingsNavigation.swift` around lines 478 - 483, The settings anchor map in SettingsNavigation.settingsPathAnchorIDs is missing the app.globalFontMagnification entry, so anchorID(forSettingsPath:) cannot resolve that settings row. Add a mapping for "app.globalFontMagnification" using the same settingID(for:idSuffix:) pattern as the other app anchors, and make sure it points to the .appearance section so deep-link and highlight navigation works for the new row.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppearanceSection.swift`:
- Around line 120-157: The font save helpers in AppearanceSection are only
canceling the wrapper Task, so in-flight hostActions.set… writes can still
complete out of order and overwrite newer values. Update saveTerminalFontFamily,
saveTerminalFontSize, saveSidebarFontSize, and saveSurfaceTabBarFontSize to use
a single serialized/coalescing save path so only the latest font-setting change
is persisted. Keep the existing cancel logic only if it truly prevents the
underlying write; otherwise ensure the actual save operation is queued or
awaited sequentially rather than overlapping.
- Around line 247-255: The Appearance rows for app.globalFontMagnification,
sidebarAppearance.matchTerminalBackground, and workspaceColors.* are missing
matching search anchors even though
SettingsSearchIndex.anchorID(forSettingsPath:) now points to them. Update the
affected SettingsCardRow declarations in AppearanceSection so each row publishes
the corresponding searchAnchorID (for the setting:appearance:* anchors) and keep
the anchor mapping aligned with the path-to-anchor logic used by
SettingsSearchIndex.
---
Outside diff comments:
In `@Sources/SettingsNavigation.swift`:
- Around line 478-483: The settings anchor map in
SettingsNavigation.settingsPathAnchorIDs is missing the
app.globalFontMagnification entry, so anchorID(forSettingsPath:) cannot resolve
that settings row. Add a mapping for "app.globalFontMagnification" using the
same settingID(for:idSuffix:) pattern as the other app anchors, and make sure it
points to the .appearance section so deep-link and highlight navigation works
for the new row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b54cd8f-2f3b-4c04-917c-fb284aad8946
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (18)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigPaths/CmuxGhosttyConfigSettingEditor.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/TerminalFontConfigEditorTests.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/SettingsHostActions.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSectionID.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppearanceSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/WorkspaceColorsSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsSearchIndexTests.swiftResources/Localizable.xcstringsSources/HostSettingsActions.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftcmuxTests/SettingsSearchIndexTests.swift
💤 Files with no reviewable changes (1)
- Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift
added 5 commits
June 28, 2026 20:26
…earance-settings-tab-like-warp-term # Conflicts: # .github/swift-file-length-budget.tsv # Resources/Localizable.xcstrings
…earance-settings-tab-like-warp-term # Conflicts: # Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/WorkspaceColorsSection.swift
…earance-settings-tab-like-warp-term # Conflicts: # .github/swift-file-length-budget.tsv # Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swift # Resources/Localizable.xcstrings
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigPaths/CmuxGhosttyConfigSettingEditor.swift (1)
188-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate read/write boilerplate between
writeSettingandwriteTerminalFontFamily.Both methods perform the identical resolve-write-URL → read-existing-contents → create-directory → write-back sequence, differing only in which
updated*Contentstransform is applied. Consider extracting a shared private helper that takes the content-transform closure.♻️ Proposed refactor: extract shared write helper
+ private func writeTransformedContents( + to url: URL, + fileManager: FileManager, + transform: (String) -> String + ) throws { + let writeURL = configWriteURL(for: url, fileManager: fileManager) + let contents = (try? String(contentsOf: writeURL, encoding: .utf8)) + ?? (try? String(contentsOf: url, encoding: .utf8)) + ?? "" + let updated = transform(contents) + try fileManager.createDirectory( + at: writeURL.deletingLastPathComponent(), + withIntermediateDirectories: true, + attributes: nil + ) + try updated.write(to: writeURL, atomically: true, encoding: .utf8) + } + public func writeSetting( key: String, value: String, to url: URL, fileManager: FileManager = .default ) throws { - let writeURL = configWriteURL(for: url, fileManager: fileManager) - let contents = (try? String(contentsOf: writeURL, encoding: .utf8)) - ?? (try? String(contentsOf: url, encoding: .utf8)) - ?? "" - let updated = updatedContents(contents, setting: key, value: value) - try fileManager.createDirectory( - at: writeURL.deletingLastPathComponent(), - withIntermediateDirectories: true, - attributes: nil - ) - try updated.write(to: writeURL, atomically: true, encoding: .utf8) + try writeTransformedContents(to: url, fileManager: fileManager) { + updatedContents($0, setting: key, value: value) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigPaths/CmuxGhosttyConfigSettingEditor.swift` around lines 188 - 240, The write flow is duplicated between writeSetting and writeTerminalFontFamily, both resolving the write URL, loading existing contents, creating the target directory, and writing the updated text back. Extract that shared sequence into a private helper in CmuxGhosttyConfigSettingEditor that accepts a transform closure, then have writeSetting and writeTerminalFontFamily pass updatedContents and updatedTerminalFontFamilyContents respectively while keeping their existing key-specific behavior in writeEditableSetting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigPaths/CmuxGhosttyConfigSettingEditor.swift`:
- Around line 188-240: The write flow is duplicated between writeSetting and
writeTerminalFontFamily, both resolving the write URL, loading existing
contents, creating the target directory, and writing the updated text back.
Extract that shared sequence into a private helper in
CmuxGhosttyConfigSettingEditor that accepts a transform closure, then have
writeSetting and writeTerminalFontFamily pass updatedContents and
updatedTerminalFontFamilyContents respectively while keeping their existing
key-specific behavior in writeEditableSetting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 885e7540-2ec8-462c-9d58-1f230201bbdd
📒 Files selected for processing (5)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigPaths/CmuxGhosttyConfigSettingEditor.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/TerminalFontConfigEditorTests.swiftPackages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Config/GhosttyConfig.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/GhosttyConfigFontFamilyTests.swiftSources/Settings/ConfigSource.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppearanceSection.swift (1)
197-250: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSave race is still reachable; also consolidate the four near-identical debounce helpers.
saveTerminalFontFamily/saveTerminalFontSize/saveSidebarFontSize/saveSurfaceTabBarFontSizecancel the previous wrapperTaskand bump a generation counter, but the guard is only checked before theawait hostActions.set…call. If a prior task has already passed its guard and is awaiting the real (I/O-bound) host write when a newer edit fires, cancelling it does not stop that in-flight write — it only suppresses the stale task's failure-flag update. Two overlappingsetTerminalFontSize/setTerminalFontFamilycalls can still complete out of order and let a stale value clobber the newer one on disk. This is the same class of issue flagged on a previous commit (marked "Addressed"), but the underlying pattern here does not actually serialize the writes.Since all four methods share this exact structure, extracting one generic serialized/coalescing save helper (e.g.
debouncedSave<T>(_ value: T, generation: inout Int, task: inout Task<Void, Never>?, failed: inout Bool, write:@escaping(T) async -> Bool)) would both remove the duplication and give you a single place to add real serialization (e.g. await the previous task before starting the write, or use an actor-owned queue) instead of four independent copies to fix.Based on learnings, "for optimistic UI or CLI updates, keep one mutation path, record pending state with a request id or previous snapshot, reconcile from the authoritative result" — this generation-counter pattern only reconciles the UI flag, not the actual persisted value order.
♻️ Sketch of a consolidated helper
- private func saveTerminalFontFamily(_ family: String) { - terminalFontFamilySaveGeneration += 1 - let generation = terminalFontFamilySaveGeneration - terminalFontFamilySaveTask?.cancel() - terminalFontFamilySaveTask = Task { - await Task.yield() - guard !Task.isCancelled, generation == terminalFontFamilySaveGeneration else { return } - let saved = await hostActions.setTerminalFontFamily(family) - if !Task.isCancelled, generation == terminalFontFamilySaveGeneration { terminalFontFamilySaveFailed = !saved } - } - } - - private func saveTerminalFontSize(_ points: Double) { - terminalFontSizeSaveGeneration += 1 - let generation = terminalFontSizeSaveGeneration - terminalFontSizeSaveTask?.cancel() - terminalFontSizeSaveTask = Task { - await Task.yield() - guard !Task.isCancelled, generation == terminalFontSizeSaveGeneration else { return } - let saved = await hostActions.setTerminalFontSize(points) - if !Task.isCancelled, generation == terminalFontSizeSaveGeneration { terminalFontSizeSaveFailed = !saved } - } - } + private func save<Value>( + _ value: Value, + generation: inout Int, + task: inout Task<Void, Never>?, + failed: inout Bool, + write: `@escaping` (Value) async -> Bool + ) { + generation += 1 + let expected = generation + let previous = task + task = Task { + await previous?.value // wait for the prior write to actually finish first + guard expected == generation else { return } + let saved = await write(value) + guard expected == generation else { return } + failed = !saved + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppearanceSection.swift` around lines 197 - 250, The four save helpers in AppearanceSection are still vulnerable to out-of-order persistence because cancellation and the generation guard only protect the post-write failure flag, not the actual host write. Update saveTerminalFontFamily, saveTerminalFontSize, saveSidebarFontSize, and saveSurfaceTabBarFontSize so only one write path can run at a time and stale tasks cannot complete after a newer request; use a shared serialized/coalescing helper around hostActions.setTerminalFontFamily, setTerminalFontSize, setSidebarFontSize, and setSurfaceTabBarFontSize. While fixing this, consolidate the duplicated debounce logic into one generic helper so the serialization and generation handling live in a single place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppearanceSection.swift`:
- Around line 197-250: The four save helpers in AppearanceSection are still
vulnerable to out-of-order persistence because cancellation and the generation
guard only protect the post-write failure flag, not the actual host write.
Update saveTerminalFontFamily, saveTerminalFontSize, saveSidebarFontSize, and
saveSurfaceTabBarFontSize so only one write path can run at a time and stale
tasks cannot complete after a newer request; use a shared serialized/coalescing
helper around hostActions.setTerminalFontFamily, setTerminalFontSize,
setSidebarFontSize, and setSurfaceTabBarFontSize. While fixing this, consolidate
the duplicated debounce logic into one generic helper so the serialization and
generation handling live in a single place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3b2a1cf-0ccc-4675-b22e-cd9c48351a3e
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/SettingsHostActions.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSectionID.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppearanceSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MonospacedFontFamilyCatalog.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swift
…earance-settings-tab-like-warp-term
…earance-settings-tab-like-warp-term
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5811
Summary
nonisolatedand completing Appearance string catalog entries for every supported locale.Testing
swift testinPackages/macOS/CmuxFoundationswift testinPackages/macOS/CmuxSettingsUIgit diff --checkpython3 scripts/check-package-resolved-policy.pypython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvpython3 -m json.tool Resources/Localizable.xcstrings >/dev/nullResources/Localizable.xcstringslocalesNo dev app build, reload script, or bare
xcodebuildwas run per issue instructions.Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization