Fix split-divider-color Ghostty option in cmux#758
Fix split-divider-color Ghostty option in cmux#758lawrencecchen wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces optional split divider color support across the workspace chrome theming flow and centralizes color parsing. The chrome resolver now accepts an optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes the Ghostty Key improvements:
Implementation details:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: bcfa7b8 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
492-505: Add a regression for the runtime apply path, not just color resolution.Lines 492-505 validate
resolvedChromeColors, but the runtime fix in this PR is the no-op/apply behavior inWorkspace.applyGhosttyChrome. Please add a test wherebackgroundHexstays the same and onlysplitDividerColorchanges, then assertborderHexis applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 492 - 505, Add a regression test that exercises the runtime apply path by using Workspace.applyGhosttyChrome rather than only Workspace.resolvedChromeColors: create initial colors where backgroundHex is "#272822" and splitDividerColor is some value, create a Workspace (or the object that has applyGhosttyChrome) with those initial chrome values, then call applyGhosttyChrome with a new chrome where backgroundHex remains "#272822" but splitDividerColor changes (e.g., "#45475A"), and finally assert that the workspace's applied borderHex (or equivalent property) equals the new splitDividerColor while the backgroundHex remains unchanged; reference Workspace.applyGhosttyChrome and Workspace.resolvedChromeColors to locate related code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 492-505: Add a regression test that exercises the runtime apply
path by using Workspace.applyGhosttyChrome rather than only
Workspace.resolvedChromeColors: create initial colors where backgroundHex is
"#272822" and splitDividerColor is some value, create a Workspace (or the object
that has applyGhosttyChrome) with those initial chrome values, then call
applyGhosttyChrome with a new chrome where backgroundHex remains "#272822" but
splitDividerColor changes (e.g., "#45475A"), and finally assert that the
workspace's applied borderHex (or equivalent property) equals the new
splitDividerColor while the backgroundHex remains unchanged; reference
Workspace.applyGhosttyChrome and Workspace.resolvedChromeColors to locate
related code.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/GhosttyConfig.swift (1)
495-1281: Consider moving the generated X11 map out ofGhosttyConfig.swift.This is correct functionally, but extracting
GhosttyX11ColorMapto a dedicated generated file (or resource-backed loader) would keep parsing logic focused and reduce churn in this core config file.Possible lightweight refactor
- // Generated from ghostty/src/terminal/res/rgb.txt. - private enum GhosttyX11ColorMap { - static let hexByName: [String: String] = [ ... ] - } + // Keep in a dedicated generated file: + // Sources/Generated/GhosttyX11ColorMap.swift + private enum GhosttyX11ColorMap { + static let hexByName: [String: String] = [ ... ] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 495 - 1281, The large generated GhosttyX11ColorMap enum (type GhosttyX11ColorMap with static hexByName) should be extracted into its own generated source (e.g., GhosttyX11ColorMap.generated.swift) or a resource-backed loader; move the enum and its hexByName dictionary out of GhosttyConfig.swift, keep the same type name and access level, update any references in GhosttyConfig to use GhosttyX11ColorMap.hexByName, and ensure the new file is included in the build so compilation and visibility remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/GhosttyConfig.swift`:
- Around line 495-1281: The large generated GhosttyX11ColorMap enum (type
GhosttyX11ColorMap with static hexByName) should be extracted into its own
generated source (e.g., GhosttyX11ColorMap.generated.swift) or a resource-backed
loader; move the enum and its hexByName dictionary out of GhosttyConfig.swift,
keep the same type name and access level, update any references in GhosttyConfig
to use GhosttyX11ColorMap.hexByName, and ensure the new file is included in the
build so compilation and visibility remain identical.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/GhosttyConfig.swiftcmuxTests/GhosttyConfigTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/GhosttyConfigTests.swift
![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: 3e5e0185d9
ℹ️ 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".
| if let hex = NSColor(hex: normalized) { | ||
| return hex |
There was a problem hiding this comment.
parseColor tries NSColor(hex:) before looking up GhosttyX11ColorMap, but NSColor(hex:) currently accepts partial scans as long as the input length is 6. That means valid X11 names like bisque, brown1, or coral1 are interpreted as tiny hex values (e.g. bisque becomes #00000B) and never reach the name map, so the new named-color support silently returns incorrect colors for a non-trivial subset of names.
Useful? React with 👍 / 👎.
1 participant
Summary
split-divider-colorinto Bonsplitappearance.chromeColors.borderHexbackgroundHexandborderHex, so divider-color-only changes are not skippedTesting
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/GhosttyConfigTests test(passed)xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' build(passed)Issues
Summary by CodeRabbit
New Features
Tests