Add ghostty_config_set_color_scheme C API#39
Conversation
Allows setting the config's conditional theme state before finalization so that conditional theme resolution (e.g. `theme = light:Foo,dark:Bar`) uses the correct scheme during ghostty_config_finalize. Without this, finalization always uses the ConditionalState default (.light), causing apps to load the wrong theme colors until a surface color scheme change triggers re-derivation.
📝 WalkthroughWalkthroughThis PR introduces a new public C API function to set the color scheme on a configuration object. The function accepts an integer parameter mapping to theme states (0 for light, 1 for dark) and validates input with warning logging for invalid values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Greptile SummaryThis PR adds Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Embedder as Embedder (e.g. cmux)
participant CApi as ghostty C API
participant Config as Config._conditional_state
participant Finalize as Config.finalize()
Embedder->>CApi: ghostty_config_new()
CApi-->>Embedder: "config (theme=.light default)"
Embedder->>CApi: ghostty_config_load_default_files(config)
Embedder->>CApi: ghostty_config_set_color_scheme(config, DARK)
CApi->>Config: "_conditional_state.theme = .dark"
Embedder->>CApi: ghostty_config_finalize(config)
CApi->>Finalize: finalize() resolves conditionals using .dark
Note over Finalize: theme = light:Foo,dark:Bar → resolves to Bar
Reviews (1): Last reviewed commit: "Add ghostty_config_set_color_scheme C AP..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export fn ghostty_config_set_color_scheme(self: *Config, scheme_raw: c_int) void { | ||
| self._conditional_state.theme = switch (scheme_raw) { | ||
| 0 => .light, | ||
| 1 => .dark, | ||
| else => { | ||
| log.warn( | ||
| "invalid color scheme to ghostty_config_set_color_scheme value={}", | ||
| .{scheme_raw}, | ||
| ); | ||
| return; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
intToEnum for consistency with existing color-scheme functions
The sibling functions ghostty_app_set_color_scheme and ghostty_surface_set_color_scheme in embedded.zig both use std.meta.intToEnum(apprt.ColorScheme, scheme_raw) for conversion. Using hardcoded 0/1 here diverges from that pattern and means the mapping stays in sync with conditional.State.Theme's field order only by convention. Prefer the same idiom so any future enum evolution is caught at compile time.
| export fn ghostty_config_set_color_scheme(self: *Config, scheme_raw: c_int) void { | |
| self._conditional_state.theme = switch (scheme_raw) { | |
| 0 => .light, | |
| 1 => .dark, | |
| else => { | |
| log.warn( | |
| "invalid color scheme to ghostty_config_set_color_scheme value={}", | |
| .{scheme_raw}, | |
| ); | |
| return; | |
| }, | |
| }; | |
| } | |
| export fn ghostty_config_set_color_scheme(self: *Config, scheme_raw: c_int) void { | |
| const theme = std.meta.intToEnum(conditional.State.Theme, scheme_raw) catch { | |
| log.warn( | |
| "invalid color scheme to ghostty_config_set_color_scheme value={}", | |
| .{scheme_raw}, | |
| ); | |
| return; | |
| }; | |
| self._conditional_state.theme = theme; | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| GHOSTTY_API void ghostty_config_set_color_scheme(ghostty_config_t, | ||
| ghostty_color_scheme_e); |
There was a problem hiding this comment.
The Zig implementation documents that this must be called before ghostty_config_finalize, but the C header has no such note. Embedders working from ghostty.h alone won't see the constraint. If called after finalization the conditional state is updated but the already-resolved theme string is unchanged — a silent no-op that produces wrong colors. A brief comment here would close that gap.
| GHOSTTY_API void ghostty_config_set_color_scheme(ghostty_config_t, | |
| ghostty_color_scheme_e); | |
| /* Must be called before ghostty_config_finalize; sets the conditional | |
| theme state so that theme = light:X,dark:Y resolves correctly. */ | |
| GHOSTTY_API void ghostty_config_set_color_scheme(ghostty_config_t, | |
| ghostty_color_scheme_e); |
There was a problem hiding this comment.
Pull request overview
Adds a new C embedding API to set a config’s conditional color-scheme state prior to finalization, so conditional theme resolution during ghostty_config_finalize uses the correct light/dark branch (needed by cmux reload flow).
Changes:
- Add
ghostty_config_set_color_schemeexported Zig function to updateConfig._conditional_state.themebefore finalize. - Publish the new function in the C header (
include/ghostty.h) as part of the embedding API surface.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/config/CApi.zig | Implements the new exported config API for setting the conditional theme state used during finalize. |
| include/ghostty.h | Exposes the new config API to C embedders via the public header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const state = &@import("../global.zig").state; | ||
| const c = @import("../main_c.zig"); | ||
|
|
||
| const conditional = @import("conditional.zig"); |
There was a problem hiding this comment.
const conditional = @import("conditional.zig"); is unused in this file (no references to conditional.*). Zig treats this as an unused import and fails the build. Remove the import, or use it (e.g., for type-safe conversion) so the file compiles.
| const conditional = @import("conditional.zig"); |
| }, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
This adds a new exported C API that affects conditional theme resolution, but there is no unit test covering the new behavior (unlike other functions in this file which have Zig tests). Add a Zig test that sets the scheme via ghostty_config_set_color_scheme before finalize and asserts a conditional theme resolves according to the selected scheme, to prevent regressions.
| test "ghostty_config_set_color_scheme resolves conditional theme during finalize" { | |
| const config = ghostty_config_new() orelse return error.OutOfMemory; | |
| defer { | |
| config.deinit(); | |
| state.alloc.destroy(config); | |
| } | |
| try edit.set(config, .theme, "light:Foo,dark:Bar"); | |
| ghostty_config_set_color_scheme(config, 1); | |
| try config.finalize(); | |
| try std.testing.expectEqualStrings("Bar", config.theme.?); | |
| } |
![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)
src/config/CApi.zig (1)
93-109: Add regression tests for pre-finalize theme resolution via the new API.Please add coverage for: (1) setting dark before
ghostty_config_finalizeinfluences conditional theme resolution, and (2) invalid scheme values do not mutate an already-set valid theme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/CApi.zig` around lines 93 - 109, Add unit tests that exercise ghostty_config_set_color_scheme and ghostty_config_finalize: (1) create a Config, call ghostty_config_set_color_scheme(self, 1) to set dark before calling ghostty_config_finalize, then check that conditional theme resolution (e.g. resolving a value like "light:Foo,dark:Bar" via the same mechanism used in production tests) yields the dark branch; (2) set a valid scheme first (e.g. ghostty_config_set_color_scheme(self, 0) or 1), then call ghostty_config_set_color_scheme(self, 999) with an invalid scheme_raw and assert Config._conditional_state.theme remains the previously-set valid value and is not mutated by the invalid call; place these checks in your test suite using the existing config construction and finalize helpers and assert expected resolved theme values and retained enum value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/CApi.zig`:
- Line 6: Remove the unused import by deleting the line that declares const
conditional = `@import`("conditional.zig"); in CApi.zig; the file only needs the
Config type and references _conditional_state which uses conditional.State
indirectly via Config.zig, so remove the unused conditional symbol to fix the
compile error caused by an unused import.
---
Nitpick comments:
In `@src/config/CApi.zig`:
- Around line 93-109: Add unit tests that exercise
ghostty_config_set_color_scheme and ghostty_config_finalize: (1) create a
Config, call ghostty_config_set_color_scheme(self, 1) to set dark before calling
ghostty_config_finalize, then check that conditional theme resolution (e.g.
resolving a value like "light:Foo,dark:Bar" via the same mechanism used in
production tests) yields the dark branch; (2) set a valid scheme first (e.g.
ghostty_config_set_color_scheme(self, 0) or 1), then call
ghostty_config_set_color_scheme(self, 999) with an invalid scheme_raw and assert
Config._conditional_state.theme remains the previously-set valid value and is
not mutated by the invalid call; place these checks in your test suite using the
existing config construction and finalize helpers and assert expected resolved
theme values and retained enum value.
🪄 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: 2d45ecd5-c9b5-4d94-98dc-06fafc5543bc
📒 Files selected for processing (2)
include/ghostty.hsrc/config/CApi.zig
| const state = &@import("../global.zig").state; | ||
| const c = @import("../main_c.zig"); | ||
|
|
||
| const conditional = @import("conditional.zig"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether `conditional` is referenced beyond its declaration.
rg -n '\bconditional\b' src/config/CApi.zigRepository: manaflow-ai/ghostty
Length of output: 260
🏁 Script executed:
cat -n src/config/CApi.zig | sed -n '85,115p'Repository: manaflow-ai/ghostty
Length of output: 1216
🏁 Script executed:
# Check full file for any actual usage of conditional (excluding just the import line)
wc -l src/config/CApi.zig && echo "---" && rg -n 'conditional\.' src/config/CApi.zigRepository: manaflow-ai/ghostty
Length of output: 140
🏁 Script executed:
# Search for _conditional_state and conditional type references in Config
rg -n '_conditional_state|ConditionalState' src/config/CApi.zig -A 2 -B 2Repository: manaflow-ai/ghostty
Length of output: 323
🏁 Script executed:
# Search for Config struct definition
rg -n 'struct.*Config|pub.*const.*Config' src/config/CApi.zig -A 5 | head -30Repository: manaflow-ai/ghostty
Length of output: 45
🏁 Script executed:
head -20 src/config/CApi.zigRepository: manaflow-ai/ghostty
Length of output: 708
🏁 Script executed:
rg -n '_conditional_state' src/config/Config.zig -A 1 -B 1 | head -30Repository: manaflow-ai/ghostty
Length of output: 1228
Remove unused conditional import (compile blocker).
Line 6 adds const conditional = @import("conditional.zig");, but this symbol is not referenced anywhere in the file. The _conditional_state field uses the conditional.State type, which is already defined in Config.zig; CApi.zig does not need to directly import this module since it only uses the Config type. In Zig, unused imports cause compilation failures.
💡 Proposed fix
-const conditional = `@import`("conditional.zig");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const conditional = @import("conditional.zig"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/CApi.zig` at line 6, Remove the unused import by deleting the line
that declares const conditional = `@import`("conditional.zig"); in CApi.zig; the
file only needs the Config type and references _conditional_state which uses
conditional.State indirectly via Config.zig, so remove the unused conditional
symbol to fix the compile error caused by an unused import.
![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="src/config/CApi.zig">
<violation number="1" location="src/config/CApi.zig:6">
P1: Added `conditional` import is unused and can fail Zig compilation due to unused top-level const.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const state = &@import("../global.zig").state; | ||
| const c = @import("../main_c.zig"); | ||
|
|
||
| const conditional = @import("conditional.zig"); |
There was a problem hiding this comment.
P1: Added conditional import is unused and can fail Zig compilation due to unused top-level const.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/config/CApi.zig, line 6:
<comment>Added `conditional` import is unused and can fail Zig compilation due to unused top-level const.</comment>
<file context>
@@ -3,6 +3,7 @@ const inputpkg = @import("../input.zig");
const state = &@import("../global.zig").state;
const c = @import("../main_c.zig");
+const conditional = @import("conditional.zig");
const Config = @import("Config.zig");
const c_get = @import("c_get.zig");
</file context>
Companion to cmux #2710 — terminal surfaces losing theme after config reload.
Summary
Adds a new C API,
ghostty_config_set_color_scheme, that lets embedders set the config's conditional theme state before finalization so conditional theme resolution (e.g.theme = light:Foo,dark:Bar) uses the correct scheme duringghostty_config_finalize.Why
Without this, finalization always uses the ConditionalState default (
.light), causing embedded apps (cmux) to load the wrong theme colors until a surface color-scheme change triggers re-derivation. The cmux PR that depends on this API is ghostty-org#2710; merging this first lets the cmux side repoint its ghostty submodule back to manaflow-ai/ghostty#main instead of rod's fork.Test plan
zig buildcleanghostty_config_set_color_schemefrom embedder beforeghostty_config_finalize, confirm conditionaltheme = light:X,dark:Yresolves to the expected schemeSummary by cubic
Add
ghostty_config_set_color_schemeC API to set the config’s color scheme before finalization so conditional themes resolve to the correct light/dark variant. Prevents wrong theme after config reload in embedders likecmux.ghostty_config_set_color_scheme(ghostty_config_t, ghostty_color_scheme_e); call beforeghostty_config_finalizeto set.lightor.darkfor conditional theme resolution. Invalid values are ignored with a warning; default remains.lightif not set.Written for commit e0f6eba. Summary will update on new commits.
Summary by CodeRabbit