Repository navigation
Fix DECRPM mode 2031 reporting wrong color scheme - #20
Conversation
When changeConditionalState returns null (no theme-conditional config rules in the user's config), the Termio DerivedConfig inherits its conditional_state from the app-level config, which defaults to .light and is never updated. This caused colorSchemeReportLocked to always report light mode even when the surface's own color scheme was dark. Fix: after creating the Termio DerivedConfig, override its conditional_state with the surface's own config_conditional_state. This ensures the DECRPM mode 2031 response matches the actual surface color scheme.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request adds cmux theme picker integration to the interactive theme lister, establishes build-step ordering for the CLI helper artifact, ensures conditional state consistency in surface configuration updates, and implements buffered stdout output with explicit flushing in the main executable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ 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's stated goal — fixing DECRPM mode 2031 reporting the wrong color scheme — is addressed cleanly by a single line in However, the PR also includes ~400 lines of new functionality for a "cmux" (third-party terminal multiplexer) theme-picker integration in
Memory ownership and error-handling in the new code look sound overall. Two minor correctness concerns are noted inline: a stale Confidence Score: 4/5The core one-line fix is correct and safe; the large undescribed cmux feature addition warrants additional review before merging. All findings are P2, which would normally yield 5/5. However, ~400 lines of new functionality (cmux integration, file I/O, ObjC notifications, new build step) are entirely absent from the PR description, making independent validation harder. The double-restore path can trigger a redundant macOS NSDistributedNotification, and the stale applied-state tracking causes unnecessary repeated work on every keystroke in an edge case. These are not blocking bugs, but the scope mismatch and lack of description warrant a 4/5 to prompt a conversation before merging. src/cli/list_themes.zig — the cmux integration code is large, undocumented in the PR, and contains two minor logic issues worth addressing. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App (config_conditional_state=.light default)
participant Surface as Surface.updateConfig()
participant TernioDerived as Termio.DerivedConfig.init()
participant TermioThread as Termio Thread (DECRPM 2031)
Note over App,TermioThread: Bug path (before fix)
App->>Surface: updateConfig(config)
Surface->>TernioDerived: init(alloc, config)
Note right of TernioDerived: conditional_state = config._conditional_state (app-level default = .light)
TernioDerived-->>Surface: DerivedConfig{conditional_state=.light}
Surface->>TermioThread: change_config(DerivedConfig)
TermioThread-->>TermioThread: DECRPM 2031 reports light ❌
Note over App,TermioThread: Fixed path (after fix)
App->>Surface: updateConfig(config)
Surface->>TernioDerived: init(alloc, config)
TernioDerived-->>Surface: DerivedConfig{conditional_state=.light (app default)}
Surface->>Surface: override conditional_state = self.config_conditional_state
Note right of Surface: surface-level state (.dark or .light)
Surface->>TermioThread: change_config(DerivedConfig)
TermioThread-->>TermioThread: DECRPM 2031 reports correct scheme ✅
Reviews (1): Last reviewed commit: "Fix DECRPM mode 2031 reporting wrong col..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try self.syncCmuxPreview(cmux); | ||
| } | ||
|
|
||
| fn syncCmuxPreview(self: *Preview, cmux: CmuxThemePicker) !void { | ||
| if (eqlOptionalTheme(self.cmux_preview_light, self.cmux_applied_light) and | ||
| eqlOptionalTheme(self.cmux_preview_dark, self.cmux_applied_dark)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (eqlOptionalTheme(self.cmux_preview_light, cmux.initial_light) and | ||
| eqlOptionalTheme(self.cmux_preview_dark, cmux.initial_dark)) | ||
| { | ||
| try restoreCmuxThemeOverride(&cmux); | ||
| } else { | ||
| const raw_theme_value = (try encodeCmuxThemeValue( | ||
| self.allocator, | ||
| self.cmux_preview_light, | ||
| self.cmux_preview_dark, | ||
| )) orelse return; | ||
| defer self.allocator.free(raw_theme_value); | ||
| try writeCmuxThemeOverride(self.allocator, &cmux, raw_theme_value); | ||
| } | ||
|
|
||
| try postCmuxReloadNotification(self.allocator, cmux.bundle_id); | ||
| self.cmux_applied_light = self.cmux_preview_light; | ||
| self.cmux_applied_dark = self.cmux_preview_dark; |
There was a problem hiding this comment.
cmux_applied_* update
In syncCmuxPreview, when encodeCmuxThemeValue returns null (both cmux_preview_light and cmux_preview_dark are null), the function returns early via orelse return after the early-equality guard has already passed — meaning the preview state genuinely differs from the applied state, but cmux_applied_light/cmux_applied_dark are never updated to reflect that. Every subsequent navigation key press will call syncCmuxPreview again, hit the same path, and return early again rather than short-circuiting on the equality check at the top.
While harmless in practice (no file is written and no notification is sent), the applied-state tracking stays permanently stale for this edge case. Consider syncing the applied fields before returning:
const raw_theme_value = try encodeCmuxThemeValue(
self.allocator,
self.cmux_preview_light,
self.cmux_preview_dark,
) orelse {
// Both themes are null — nothing to write; sync applied state so we
// don't keep re-entering this branch on every event.
self.cmux_applied_light = self.cmux_preview_light;
self.cmux_applied_dark = self.cmux_preview_dark;
return;
};| errdefer self.restoreCmuxOriginal() catch {}; | ||
| defer if (self.outcome == .cancel) { | ||
| self.restoreCmuxOriginal() catch {}; |
There was a problem hiding this comment.
In run(), if an error propagates out while self.outcome is still .cancel (the initial value), both errdefer and the conditional defer will call restoreCmuxOriginal(). Writing the same file twice is idempotent and harmless, but it sends an unnecessary postCmuxReloadNotification on the second call, which could cause the host application to reload its config twice for no reason.
Consider folding both into a single defer that checks outcome != .apply:
defer {
if (self.outcome != .apply) {
self.restoreCmuxOriginal() catch {};
}
}This naturally handles the error path since outcome is initialised to .cancel.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…ners Re-apply CI fixes for GitHub-hosted runners
1 participant
Fixes DECRPM mode 2031 reporting light when surface is dark. The Termio DerivedConfig inherited conditional_state from the app level (default .light) instead of the surface's own state.
Summary by cubic
Fixes DECRPM mode 2031 so each surface reports the correct light/dark scheme. Adds a cmux-aware theme picker with live preview and safe config overrides.
Bug Fixes
.lightdefault after reloads).New Features
list_themes: reads env for config/targets, live-applies selections, reverts on cancel, applies on Enter, and cycles target (both/light/dark) with “t”.objc; uses atomic writes and managed blocks for overrides; adds acli-helperbuild step and flushes helper usage output.Written for commit c4cef10. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes