Repository navigation
Fix terminal surfaces losing theme after config reload - #2710
Closed
rodchristiansen wants to merge 2082 commits into
Closed
rodchristiansen wants to merge 2082 commits into
rodchristiansen wants to merge 2082 commits into
Conversation
* Add cmd-click fallback for bare filenames in terminal output When cmd-clicking text that ghostty's built-in URL/path regex doesn't match (e.g. bare filenames from `ls` like README.md, src, config.json), fall back to checking if the word under cursor is a valid file or directory in the terminal panel's CWD. Uses the existing ghostty_surface_quicklook_word API to extract the word, then resolves it against the panel's working directory and opens it if it exists. * Add pointing-hand cursor on Cmd-hover over bare filenames When holding Cmd and hovering over a word that resolves to an existing file/directory in the terminal's CWD, show the pointing-hand cursor. Hooks into mouseMoved and flagsChanged so the cursor updates both when moving the mouse with Cmd held and when pressing/releasing Cmd while the mouse is stationary. * Address PR review comments - Refresh ghostty mouse position before quicklook_word in mouseUp and flagsChanged so stale coordinates don't resolve the wrong word - Use failable String(bytes:encoding:.utf8) instead of lossy decoding - Skip absolute-path words (already handled by ghostty's regex) - Guard against remote terminal sessions (local fileExists would be wrong) - Use invalidateCursorRects instead of forcing iBeam on hover deactivation to avoid overwriting ghostty/AppKit's cursor state * Add preferred editor setting for cmd-click file opens New "Open Files With" picker in Settings > App lets users choose which editor opens when cmd-clicking bare filenames. Options: System Default, Cursor, VS Code, Windsurf, Zed, Sublime Text, Xcode. Reuses the existing TerminalDirectoryOpenTarget app detection infrastructure. Defaults to system default (NSWorkspace default handler). * Replace editor picker with free-form command field, respect $VISUAL/$EDITOR The "Open Files With" setting is now a text field where users can type any command (code, zed, subl, open -a Xcode, etc.). Resolution order: 1. User-configured command from settings 2. $VISUAL environment variable 3. $EDITOR environment variable 4. System default (NSWorkspace) Removes the fixed PreferredEditor enum in favor of flexibility. * Fix stuck pointing-hand cursor using NSCursor push/pop invalidateCursorRects did nothing since the view has no cursor rects. Use NSCursor push/pop stack instead so the previous cursor is properly restored when the hover deactivates. * Remove $VISUAL/$EDITOR fallback, use system default when empty $EDITOR/$VISUAL are typically terminal editors (vim, nano) that can't launch as GUI subprocesses. Empty field now falls back to system default (opens in Finder/default app) which is the expected behavior. * Address PR review comments (round 2) - Use broader CWD fallback chain (panelDirectories → requestedWorkingDirectory → workspace currentDirectory) matching Workspace split creation logic - Pop cursor stack in viewDidMoveToWindow to balance push if view is removed while hover is active - Reset preferredEditorCommand in resetAllSettings() - Fall back to NSWorkspace.open when the custom editor command exits non-zero (e.g. command not found exits 127 but /bin/sh itself succeeds) * Clear cursor on mouse exit to prevent stuck pointing-hand --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
* Simplify R2 upload to appcast-only (keep DMGs on GitHub) DMGs are immutable per-build on GitHub Releases (unique filenames, no overwrite), so there's no race condition for them. Only the appcast.xml needs atomic replacement, which R2 PutObject provides. Upload the original appcast.xml as-is (GitHub Release DMG URLs) to R2. No sed URL rewriting, no DMG uploads, less storage/bandwidth. * Move R2 appcast upload after GitHub Release publish The R2 appcast references GitHub Release DMG URLs, so it must be uploaded after the DMGs exist on GitHub. Previously the R2 upload ran before the publish step, creating a brief window where the appcast pointed to a not-yet-existing DMG. * Add semver guard to stable R2 appcast upload Prevents a backport tag (e.g. v0.62.1 pushed after v0.63.1) from overwriting the stable appcast with an older version. Uses sort -V to compare all non-prerelease tags and only uploads if the current tag is the highest. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
* Relicense from AGPL-3.0 to GPL-3.0 (keep dual-license with commercial option) AGPL's network-use clause is irrelevant for a desktop app, but triggers blanket corporate bans. GPL-3.0 still requires forks to stay open source (preventing proprietary commercial forks) while being accepted by most corporate policies for desktop software. Changes: - LICENSE: Replace AGPL-3.0 text with GPL-3.0 text - Update dual-license header (AGPL → GPL) - Update all README translations, CONTRIBUTING.md, package.json files - Historical changelog/project entries left as-is * Fix French and Italian grammar in license section AGPL starts with a vowel so "l'AGPL" / "all'AGPL" were correct. GPL starts with a consonant, so use "la GPL" / "alla GPL" instead. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
Point cmux NIGHTLY's SUFeedURL to files.cmux.com/nightly/appcast.xml (Cloudflare R2) instead of the GitHub Release asset. R2 uses atomic PutObject for replacement, eliminating the transient SUDownloadError 2001 that occurs when GitHub Release assets are being overwritten during a nightly publish. The isNightly detection (checks for "/nightly/" in the URL) still works with the new R2 URL. DMGs continue to be served from GitHub Releases. Only the appcast feed URL changes. Stable builds are unchanged (still use GitHub Releases). Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
Three issues caused the Bonsplit horizontal tab bar to be hidden when entering fullscreen with minimal mode enabled: 1. ignoresSafeArea(.container, edges: .top) was applied unconditionally in minimal mode, pushing content behind the fullscreen menu bar area. Now gated on !isFullScreen. 2. effectiveTitlebarPadding returned -titlebarPadding in minimal mode regardless of fullscreen state. In fullscreen there is no native titlebar to compensate for, so the negative offset pushed content off the top of the screen. Now returns 0 in fullscreen. 3. Traffic light leading inset (80px) was applied in fullscreen minimal mode even though there are no traffic light buttons. Now gated on !isFullScreen, and syncTrafficLightInset is called on fullscreen enter/exit. Closes manaflow-ai#2317 Based on manaflow-ai#2341 Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
* Add hover background to split action buttons Split buttons (terminal, browser, split right/down) now show a subtle rounded-rect background highlight on hover. Matches standard macOS toolbar button behavior. * Prevent fade overlays from bleeding into bottom separator * Render bottom separator above fade overlays to prevent bleed * Exclude drop zone from scroll fade threshold * Revert button hover, fix fade threshold with 32pt buffer * Fix right fade threshold: subtract drop zone, 4pt tolerance * Add leading padding to split buttons, fix fade threshold * Rework tab bar: full-width scroll with floating split buttons * Use ultraThinMaterial blur for floating split buttons * Make split buttons group full height * Full-height split button blur background * Inset split buttons from bottom separator * Fix blur overlapping separator * Use matching tab bar bg for split buttons, clear separator * Use regularMaterial blur for split buttons * Try thickMaterial for split buttons * Fade gradient + solid barFill for floating split buttons * Use extended right fade as split buttons backdrop * Add 5 debug styles for split button background * Add Split Button Style debug window to Debug menu * Clean up: no-bg floating buttons, alphabetical debug menu - Split buttons float with no background (tabBarBackground covers the area, scroll padding prevents tabs from appearing behind buttons) - Default splitButtonsWidth to 120 so first render has correct padding - Remove split button style debug window and debug styles - Alphabetize Debug Windows menu entries, remove dividers * Revert to HStack sibling layout, add debug menu docs to CLAUDE.md - Split buttons are HStack siblings of the ScrollView, not overlays. Single .background() on parent, no compositing mismatch. - Alphabetize Debug Windows menu, remove dividers. - Document Debug menu in CLAUDE.md. * Add Split Button Layout debug window with 5 switchable approaches * Fix fade gradient color to match tab bar background * Add fade color debug window with 6 color options * Use mask for scroll fades, fixes color mismatch * Hide scroll fades in minimal mode unless hovering * Remove split buttons from layout when hidden in minimal mode * Always show fades, overlay buttons to prevent scroll jump * Add hover-only mask fade behind split buttons * Reduce button mask area to 90pt * Animate button mask smoothly * Fade entire button group together via opacity * Add blur behind buttons with fade mask * Use theme barBackground for button backdrop * Blur + theme tint for button backdrop * More tint (0.85), less blur * Tint 0.2, clear bottom border * Use terminal bg color, add scroll trailing padding for buttons * Less blur, paneBackground at 0.75 opacity * paneBackground at 0.9 opacity * 0.97 opacity for button backdrop * Test: fully opaque paneBackground * Test: solid red backdrop * Gradient + solid paneBackground backdrop, no mask * Force opaque paneBackground for button backdrop * Use barBackground for button backdrop * Use terminal bg (paneBackground forced opaque) * Pre-composite backdrop color for exact match * Add 6 switchable backdrop styles in debug window * Mask-based button area hiding, no backdrop color needed --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
* Add React Grab inject button to browser toolbar Adds a toolbar button (cursor click icon) that injects the react-grab script (unpkg.com/react-grab/dist/index.global.js) into the current page. Hover over React elements and Cmd+C to copy component context (file, component name, line number) for AI agents. Button highlights when active, resets on navigation. * Auto-activate selection mode on React Grab inject First click: injects the script and auto-activates selection mode via the react-grab:init event. Subsequent clicks toggle selection mode on/off via window.__REACT_GRAB__.toggle(). * Bridge React Grab state back to Swift via WKScriptMessageHandler Register a cmux-bridge plugin after injecting react-grab that posts state changes back to Swift via webkit.messageHandlers. The button now highlights accent color only when selection mode is actually active (not just when the script is loaded), and deactivates when the user exits selection mode via Escape or the react-grab toolbar. * Fetch react-grab script via URLSession to bypass CSP Sites like vercel.com block loading external scripts via CSP headers. Fetch the script with URLSession (not subject to page CSP), cache it, and inject inline via evaluateJavaScript. Also guard against duplicate injection on repeated clicks. * Prefetch react-grab script on first browser panel init Kick off a low-priority background fetch of the react-grab script when the first BrowserPanel is created. The script is cached statically so clicking the button is instant. * Eliminate react-grab button and callback lag Three changes: 1. Fire-and-forget: use evaluateJavaScript with completionHandler instead of await, so button taps return immediately. 2. Single JS payload: combine bootstrap listener + script source into one evaluateJavaScript call (one IPC round-trip, not two). 3. Dedupe state callbacks: only post webkit message when isActive actually changes, not on every hover/drag state update. * Fix duplicate state callback on react-grab toggle toggleReactGrab was sending an explicit postMessage AND the plugin's onStateChange hook was firing too, causing two @published updates per toggle. Remove the explicit postMessage since the plugin hook handles it. Also add dlog instrumentation for debugging. * Add Cmd+Shift+G shortcut for React Grab (configurable) - Add toggleReactGrab to KeyboardShortcutSettings with Cmd+Shift+G default - Add View menu item with customizable shortcut - Add command palette entry (searchable as "react grab" or "inspect element") - Simplify button to use toggleOrInjectReactGrab, remove local state tracking * Fix Codex review findings: pin version, verify hash, fix retry and state 1. Pin react-grab to exact version (0.1.29) with SHA-256 integrity check. Script is verified before evaluation to prevent supply-chain attacks via compromised CDN responses. 2. Clear prefetchTask on failure so subsequent attempts retry the download instead of reusing a permanently failed task. 3. Remove premature isReactGrabActive=true. State is now only set by the onStateChange message handler callback after confirmed initialization, or explicitly reset on evaluation error. * Extract React Grab into own file, make version configurable Move all react-grab logic (settings, script loader, message handler, BrowserPanel extension) into Sources/Panels/ReactGrab.swift. Add a "React Grab Version" text field in Settings > Browser that lets the user pin which npm version is fetched. Only versions with a known SHA-256 integrity hash in ReactGrabSettings.knownHashes are accepted. The cache invalidates when the configured version changes. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
…-enter-tmux Map Shift+Enter to raw newline in Ghostty
) * Fix transparent background flash during sidebar toggle Move terminal background rendering from the Metal GPU pass to a CALayer (backgroundView). The GPU bg_color pass is disabled via a new Ghostty config flag (macos-background-from-layer). The CALayer resizes instantly with its parent NSView, eliminating the 3-5 frame gap where the desktop was visible through the transparent window during sidebar toggles and layout transitions. Also simplifies the titlebar and sidebar opacity formulas since there is now a single background layer instead of two stacked semi-transparent layers. * Document macos-background-from-layer fork change * Pin GhosttyKit checksum for macos-background-from-layer * Address review feedback: fix fallback config path, inline identity wrapper - Inject macos-background-from-layer in the fallback config path too, preventing alpha double-stacking when user config is invalid - Inline panelBackgroundFillColor (now an identity function) at its two call sites and remove the wrapper * Address adversarial review: skip fullscreen bg draw call explicitly The bg_color uniform alpha is still zeroed for cell compositing (so transparent cells pass through to the CALayer), but the fullscreen background fill draw step is now explicitly skipped instead of relying on alpha=0 as a no-op. * Pin GhosttyKit checksum for bg draw-call skip --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
…and-palette Fix Ctrl+K in the command palette
…-animated-workspace-rerender Keep cmd+p stable during animated workspace title updates
After ghostty_app_update_config, each surface re-derives its config using its conditional state (light/dark). However, ghostty_surface_set_color_scheme has an internal dedup that skips when the scheme hasn't changed, so surfaces could end up with stale theme colors (e.g. light foreground on dark background) after a config reload or appearance toggle. Fix by calling ghostty_surface_update_config on each surface in refreshTerminalSurfacesAfterGhosttyConfigReload, which forces a full config re-derive with the surface's current conditional state, bypassing the color scheme dedup. Also re-apply the color scheme from the current macOS appearance to keep Swift-side tracking in sync.
|
@rodchristiansen is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughApp-level appearance observation and theme synchronization were added; Ghostty config now receives the app color-scheme before finalization; terminal surfaces explicitly re-derive and reapply their color scheme/config after Ghostty config reloads. The Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
Contributor
Greptile SummaryThis PR fixes stale theme colors on terminal surfaces after Confidence Score: 5/5Safe to merge; the fix correctly addresses the dedup bypass and all remaining findings are P2 suggestions. The core logic is sound: calling ghostty_surface_update_config before applySurfaceColorScheme ensures the config is re-derived even when the scheme is unchanged, which is exactly the dedup bypass needed. The only finding is a P2 suggestion to split the early-return guard so ghostty_surface_update_config still fires for surfaces without an attached view. Sources/GhosttyTerminalView.swift — minor guard ordering in reapplyColorSchemeAndConfig
|
| Filename | Overview |
|---|---|
| Sources/GhosttyTerminalView.swift | Adds reapplyColorSchemeAndConfig() to TerminalSurface and widens applySurfaceColorScheme to fileprivate; early-return guard skips ghostty_surface_update_config for surfaces with no attached view. |
| Sources/AppDelegate.swift | Calls reapplyColorSchemeAndConfig() after forceRefresh in the config-reload surface refresh loop — straightforward integration, no issues. |
Sequence Diagram
sequenceDiagram
participant AD as AppDelegate
participant TS as TerminalSurface
participant GNV as GhosttyNSView
participant G as "Ghostty C layer"
AD->>AD: "refreshTerminalSurfacesAfterGhosttyConfigReload()"
loop "each terminalPanel"
AD->>TS: "forceRefresh(reason:)"
TS->>GNV: "forceRefreshSurface()"
TS->>G: "ghostty_surface_refresh()"
AD->>TS: "reapplyColorSchemeAndConfig()"
TS->>G: "ghostty_surface_update_config(surface, config)"
TS->>GNV: "applySurfaceColorScheme(force: true)"
GNV->>G: "ghostty_surface_set_color_scheme(surface, scheme)"
end
Reviews (1): Last reviewed commit: "Fix terminal surfaces losing theme after..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4448
to
+4460
| func reapplyColorSchemeAndConfig() { | ||
| guard let surface, let view = attachedView else { return } | ||
| // Force the surface to re-derive its config with its current conditional state. | ||
| // ghostty_surface_set_color_scheme has an internal dedup that skips when the | ||
| // scheme hasn't changed, but after a config reload the underlying theme data | ||
| // may have changed. ghostty_surface_update_config bypasses that dedup. | ||
| if let config = GhosttyApp.shared.config { | ||
| ghostty_surface_update_config(surface, config) | ||
| } | ||
| // Re-apply color scheme to ensure the surface's conditional state matches | ||
| // the current macOS appearance, in case it drifted. | ||
| view.applySurfaceColorScheme(force: true) | ||
| } |
Contributor
There was a problem hiding this comment.
attachedView is nil
The combined guard guard let surface, let view = attachedView else { return } means that if a panel's surface exists but has no attached view (e.g. a background panel that was never rendered), ghostty_surface_update_config is also skipped. That surface will carry stale theme data until it eventually gets an attached view and goes through the normal setup path — but nothing in the attach path calls ghostty_surface_update_config again, so a subsequent config reload may never reach it.
Consider splitting the guard so the config update always runs when a surface is present:
Suggested change
| func reapplyColorSchemeAndConfig() { | |
| guard let surface, let view = attachedView else { return } | |
| // Force the surface to re-derive its config with its current conditional state. | |
| // ghostty_surface_set_color_scheme has an internal dedup that skips when the | |
| // scheme hasn't changed, but after a config reload the underlying theme data | |
| // may have changed. ghostty_surface_update_config bypasses that dedup. | |
| if let config = GhosttyApp.shared.config { | |
| ghostty_surface_update_config(surface, config) | |
| } | |
| // Re-apply color scheme to ensure the surface's conditional state matches | |
| // the current macOS appearance, in case it drifted. | |
| view.applySurfaceColorScheme(force: true) | |
| } | |
| func reapplyColorSchemeAndConfig() { | |
| guard let surface else { return } | |
| // Force the surface to re-derive its config with its current conditional state. | |
| // ghostty_surface_set_color_scheme has an internal dedup that skips when the | |
| // scheme hasn't changed, but after a config reload the underlying theme data | |
| // may have changed. ghostty_surface_update_config bypasses that dedup. | |
| if let config = GhosttyApp.shared.config { | |
| ghostty_surface_update_config(surface, config) | |
| } | |
| // Re-apply color scheme to ensure the surface's conditional state matches | |
| // the current macOS appearance, in case it drifted. | |
| guard let view = attachedView else { return } | |
| view.applySurfaceColorScheme(force: true) | |
| } |
Author
There was a problem hiding this comment.
Fixed in d97ba42 — split the guard so ghostty_surface_update_config runs even when attachedView is nil (background panels now get the reload too). View-side applySurfaceColorScheme(force: true) still gates on attachedView.
There was a problem hiding this comment.
Pull request overview
Fixes a macOS theme/color desync where terminal surfaces could retain stale derived theme colors after ghostty_app_update_config (notably with conditional theme = light:X,dark:Y), by forcing each surface to re-derive config and re-assert the current appearance-driven color scheme.
Changes:
- Add
TerminalSurface.reapplyColorSchemeAndConfig()to callghostty_surface_update_configand then force re-apply the Swift-side appearance color scheme. - Invoke the new per-surface reapply step for every terminal panel after a Ghostty config reload.
- Relax
GhosttyNSView.applySurfaceColorSchemeaccess fromprivate→fileprivateto allow same-file coordination.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Sources/GhosttyTerminalView.swift | Adds a post-reload surface re-derive + scheme re-assert helper; exposes applySurfaceColorScheme within the file to support it. |
| Sources/AppDelegate.swift | Calls the new helper for each terminal surface during the existing post-reload refresh pass. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4449-4456: Replace the direct use of the local surface variable
with the stale-safe accessor: call liveSurfaceForGhosttyAccess(reason:) to
obtain the current live surface (and attached view) before invoking Ghostty C
APIs; then, if that returns a valid surface and GhosttyApp.shared.config exists,
call ghostty_surface_update_config(surface, config). Update the guard that
references `surface`/`attachedView` to use the result of
liveSurfaceForGhosttyAccess(reason:) so you never pass a potentially stale
pointer into ghostty_surface_update_config.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eba8c066-1f93-4851-b0b0-5dd8eb24a415
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 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/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:4449">
P2: The combined `guard` causes `ghostty_surface_update_config` to be skipped when `attachedView` is nil, even though the config update only needs the surface. Split the guard so that the config update always runs when a surface is present, and only gate the `applySurfaceColorScheme` call on `attachedView`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The previous fix only handled the config reload path. If macOS auto- switched dark/light while cmux was in the background (e.g. overnight via the auto appearance schedule), per-view viewDidChangeEffectiveAppearance callbacks did not always fire reliably for hidden views, leaving the surfaces stuck in the wrong theme. The root cause runs deeper: Ghostty's ConditionalState.theme defaults to .light, so ghostty_config_finalize always resolved conditional themes (e.g. `theme = light:Foo,dark:Bar`) with the light variant even on systems running dark mode. This made defaultBackgroundColor and the surface-level config wrong from the start. Fix: 1. New ghostty C API: ghostty_config_set_color_scheme, called before ghostty_config_finalize in loadDefaultConfigFilesWithLegacyFallback so the config is finalized with the correct theme variant from the beginning. (Requires ghostty submodule bump with the new export.) 2. Deferred ghostty_app_set_color_scheme after ghostty_app_new so app.config_conditional_state matches the current appearance. New surfaces then inherit the right scheme instead of .light. Deferred via DispatchQueue.main.async to avoid re-entrant reloads during GhosttyApp initialization. 3. ghostty_app_set_color_scheme in synchronizeThemeWithAppearance to keep the app-level state in sync with the new appearance before the config reload runs. 4. KVO observer on NSApp.effectiveAppearance so appearance changes are detected even when no terminal view receives viewDidChangeEffectiveAppearance. 5. synchronizeThemeWithAppearance call in applicationDidBecomeActive as a backstop for when cmux regains focus after macOS switched appearance modes while it was inactive. .gitmodules temporarily points ghostty at the rodchristiansen fork while the upstream ghostty C API PR (manaflow-ai/ghostty#37) is in review. Revert to manaflow-ai/ghostty once that merges.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
4507-4518:⚠️ Potential issue | 🔴 CriticalUse
liveSurfaceForGhosttyAccess(...)before touching Ghostty C APIs.
surfacecan be stale during portal/reparent churn, so callingghostty_surface_update_config(...)on the raw stored pointer can dereference freed native state. Please resolve the live surface through the quarantine helper first.🐛 Proposed fix
+ `@MainActor` func reapplyColorSchemeAndConfig() { - guard let surface, let view = attachedView else { return } + guard let view = attachedView, + let surface = liveSurfaceForGhosttyAccess(reason: "surface.reapplyColorSchemeAndConfig") else { return } // Force the surface to re-derive its config with its current conditional state. // ghostty_surface_set_color_scheme has an internal dedup that skips when the // scheme hasn't changed, but after a config reload the underlying theme data // may have changed. ghostty_surface_update_config bypasses that dedup. if let config = GhosttyApp.shared.config { ghostty_surface_update_config(surface, config) } // Re-apply color scheme to ensure the surface's conditional state matches // the current macOS appearance, in case it drifted. view.applySurfaceColorScheme(force: true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 4507 - 4518, The stored raw `surface` pointer may be stale; before calling Ghostty C APIs use the quarantine helper to resolve a live pointer. Replace direct uses of `surface` in `reapplyColorSchemeAndConfig()` with a resolved pointer from `liveSurfaceForGhosttyAccess(surface)` (guarding early if it returns nil), then call `ghostty_surface_update_config(liveSurface, config)` and otherwise proceed; keep `view.applySurfaceColorScheme(force: true)` as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4507-4518: The stored raw `surface` pointer may be stale; before
calling Ghostty C APIs use the quarantine helper to resolve a live pointer.
Replace direct uses of `surface` in `reapplyColorSchemeAndConfig()` with a
resolved pointer from `liveSurfaceForGhosttyAccess(surface)` (guarding early if
it returns nil), then call `ghostty_surface_update_config(liveSurface, config)`
and otherwise proceed; keep `view.applySurfaceColorScheme(force: true)` as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f3ebdcf-166f-4ec3-ab23-43d75f7cbe11
📒 Files selected for processing (3)
.gitmodulesSources/GhosttyTerminalView.swiftghostty
✅ Files skipped from review due to trivial changes (2)
- .gitmodules
- ghostty
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| url = https://github.com/manaflow-ai/ghostty.git | ||
| branch = main | ||
| url = https://github.com/rodchristiansen/ghostty.git | ||
| branch = feat/config-set-color-scheme-api |
There was a problem hiding this comment.
Submodule points to personal fork, breaking CI
High Severity
The ghostty submodule URL was changed from manaflow-ai/ghostty (the organization fork) to rodchristiansen/ghostty on branch feat/config-set-color-scheme-api. This contradicts the documented workflow in CLAUDE.md which states changes must go to manaflow-ai/ghostty. CI workflows in build-ghosttykit.yml, test-depot.yml, and test-e2e.yml all hardcode manaflow-ai/ghostty for release downloads, so merging this will break builds since the submodule SHA won't have a matching release at the expected location.
Reviewed by Cursor Bugbot for commit dc7fe67. Configure here.
Author
There was a problem hiding this comment.
Tracked by companion PR manaflow-ai/ghostty#39 (Add ghostty_config_set_color_scheme C API). Once that merges, .gitmodules will be repointed back to manaflow-ai/ghostty#main and the submodule SHA bumped so CI workflows keep pulling from the org fork.
3 tasks
Previously this function took the raw `self.surface` pointer and handed it to ghostty_surface_update_config. During portal reparent / teardown churn that pointer can already be freed, which would crash inside Ghostty. Route through liveSurfaceForGhosttyAccess so a stale pointer is quarantined instead. Also: - Run the config update independently of `attachedView`. Background panels (no view attached yet) were being skipped by the combined guard and would keep the pre-reload theme data. - Mark the method `@MainActor` so callers don't cross isolation boundaries with the surface pointer. Addresses bot review on PR manaflow-ai#2710 (coderabbitai critical, greptile/cubic P2).
rodchristiansen
added a commit
to rodchristiansen/cmux
that referenced
this pull request
Apr 14, 2026
Previously this function took the raw `self.surface` pointer and handed it to ghostty_surface_update_config. During portal reparent / teardown churn that pointer can already be freed, which would crash inside Ghostty. Route through liveSurfaceForGhosttyAccess so a stale pointer is quarantined instead. Also: - Run the config update independently of `attachedView`. Background panels (no view attached yet) were being skipped by the combined guard and would keep the pre-reload theme data. - Mark the method `@MainActor` so callers don't cross isolation boundaries with the surface pointer. Addresses bot review on PR manaflow-ai#2710 (coderabbitai critical, greptile/cubic P2).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/GhosttyTerminalView.swift`:
- Around line 1777-1784: The fallback branch finalizes the config before
applying the system-derived color scheme, so in invalid-user-config paths the
config can end up using the wrong light/dark state; update the fallback path to
compute isDark the same way and call ghostty_config_set_color_scheme(config,
isDark ? GHOSTTY_COLOR_SCHEME_DARK : GHOSTTY_COLOR_SCHEME_LIGHT) before invoking
ghostty_config_finalize(config) (the same sequence used in the non-fallback
path), referencing the existing isDark logic and the functions
ghostty_config_set_color_scheme and ghostty_config_finalize to locate and fix
the code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b92f459b-05c9-4bef-b7e8-f3f8b889e231
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d97ba42. Configure here.
The primary config path set `ghostty_config_set_color_scheme` before `ghostty_config_finalize`, but the fallback path (when primary config could not be loaded) called finalize without seeding the conditional theme state. Conditional themes in the fallback config would always resolve as light until a later surface color-scheme event triggered re-derivation. Apply the same pre-finalize seed on the fallback path so both code paths land at the correct initial theme for dark-mode boots.
rodchristiansen
force-pushed
the
fix/theme-sync-clean
branch
from
September 2, 2026 00:50
586921b to
e36c290
Compare
Contributor
|
Too many files changed for review (3000 files, 100 file limit). |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
Contributor
|
Thanks for the earlier theme synchronization investigation. I withdrew my attempted Swift reload fix in #14474 after tracing the protocol one layer deeper: current Ghostty reports from Termio's copied configuration state, which can remain light even when the surface and renderer are dark. The existing protocol fix in #11507 and its dependency manaflow-ai/ghostty#209 address that state ownership. I posted the detailed call path on #11507. — CinderPlum pending |
Collaborator
|
This is shipped on current main in the live terminal configuration reload path ( |
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
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.
Summary
ghostty_app_update_config, surfaces can end up with stale theme colors (e.g. light foreground on dark background) becauseghostty_surface_set_color_scheme's internal dedup skips re-derivation when the scheme hasn't changedghostty_surface_update_configon each surface after config reload, which forces a full config re-derive with the surface's current light/dark conditional state, bypassing the dedupTest plan
theme = light:X,dark:Yconditional themes configuredtheme = X)Note
Medium Risk
Touches terminal rendering/theme synchronization and updates the
ghosttysubmodule/API usage, so regressions could affect colors or config reload behavior across all panes.Overview
Fixes panes ending up with mismatched light/dark theme colors after
ghosttyconfig reloads by forcing each terminal surface to re-derive its config and then re-apply the current appearance-driven color scheme.Config loading now seeds Ghostty’s conditional color-scheme state before
ghostty_config_finalize, and the app keeps its internal scheme synchronized viaghostty_app_set_color_schemeon init, on app-activation, and via KVO ofNSApp.effectiveAppearance.Updates the
ghosttysubmodule to a fork/branch that provides the new color-scheme setter API used by these changes.Reviewed by Cursor Bugbot for commit 586921b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes panes showing the wrong theme after config reloads or when macOS switches appearance while the app is inactive. Surfaces and the app now set and sync the color scheme so light/dark themes stay correct.
Bug Fixes
ghostty_surface_update_config, routed throughliveSurfaceForGhosttyAccess, working even without attached views; then re-apply the view's scheme (@MainActor).ghostty_config_finalizeviaghostty_config_set_color_schemefor both primary and fallback config paths, ensuring correct variants on dark-mode boots.ghostty_app_set_color_schemeon init (deferred), via KVO onNSApp.effectiveAppearance, and when the app becomes active.Dependencies
ghosttysubmodule torodchristiansen/ghosttyonfeat/config-set-color-scheme-apito use the new color-scheme C APIs.Written for commit e36c290. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
Chores