Repository navigation
Send initial color scheme report when mode 2031 is enabled - #21
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.
Per the mode 2031 spec, the terminal must send an initial DSR 997 report when the mode is enabled via DECSET. Without this, clients that enable mode 2031 never receive the current color scheme.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
![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 5 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/cli/list_themes.zig">
<violation number="1" location="src/cli/list_themes.zig:847">
P2: `kp_down` is handled by both the single-step and page-down branches, causing double movement and duplicate preview updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) | ||
| try self.applyCmuxSelectionForCurrentTheme(); | ||
| } | ||
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) { |
There was a problem hiding this comment.
P2: kp_down is handled by both the single-step and page-down branches, causing double movement and duplicate preview updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/list_themes.zig, line 847:
<comment>`kp_down` is handled by both the single-step and page-down branches, causing double movement and duplicate preview updates.</comment>
<file context>
@@ -432,40 +803,71 @@ const Preview = struct {
- if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{}))
+ try self.applyCmuxSelectionForCurrentTheme();
+ }
+ if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) {
self.down(20);
- if (key.matchesAny(&.{ 'k', '-', vaxis.Key.up, vaxis.Key.kp_up, vaxis.Key.kp_subtract }, .{}))
</file context>
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) { | |
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_page_down }, .{})) { |
Greptile SummaryThis PR delivers two distinct changes: (1) the core terminal emulation fix — sending an initial DSR 997 color-scheme report when DECSET 2031 is enabled, and ensuring the termio Key changes:
Issues found:
Confidence Score: 5/5Safe to merge; the terminal emulation fixes are correct and all remaining findings are P2 style/robustness suggestions in the cmux integration code. The two core changes (stream_handler.zig and Surface.zig) are correct, well-scoped, and follow established patterns. The cmux integration in list_themes.zig is additive and only active when CMUX_THEME_PICKER_CONFIG is set, so it does not regress existing Ghostty behavior. All identified issues are P2 (dead code, minor robustness edge case, copy inefficiency, and a doubled kp_down side-effect from a pre-existing key binding collision). src/cli/list_themes.zig — the kp_down double-binding and the removeManagedThemeOverride orphan-marker edge case are the most actionable items. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as TerminalApp
participant SH as StreamHandler
participant TM as TermioThread
participant S as Surface
App->>SH: DECSET 2031 enable mode
SH->>SH: modes.set report_color_scheme true
SH->>TM: color_scheme_report force=true
TM->>TM: colorSchemeReportLocked reads conditional_state.theme
TM->>App: ESC response dark or light
S->>S: DerivedConfig.init from config
S->>S: override conditional_state with surface state
S->>TM: queueIo change_config
TM->>TM: changeConfig with surface conditional_state
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn initialTheme(self: *const CmuxThemePicker) ?[]const u8 { | ||
| return switch (self.target_mode) { | ||
| .both => if (eqlOptionalTheme(self.initial_light, self.initial_dark)) | ||
| self.initial_light orelse self.initial_dark | ||
| else | ||
| self.initial_dark orelse self.initial_light, | ||
| .light => self.initial_light orelse self.initial_dark, | ||
| .dark => self.initial_dark orelse self.initial_light, |
There was a problem hiding this comment.
initialTheme appears to be dead code
CmuxThemePicker.initialTheme() is defined but never called anywhere in list_themes.zig. The Preview.init function initializes cmux_preview_light/dark directly from value.initial_light/dark rather than going through this helper. If this method isn't used externally, it should be removed to avoid confusion.
| if (key.matchesAny(&.{ 'j', '+', vaxis.Key.down, vaxis.Key.kp_down, vaxis.Key.kp_add }, .{})) { | ||
| self.down(1); | ||
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) | ||
| try self.applyCmuxSelectionForCurrentTheme(); | ||
| } | ||
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) { | ||
| self.down(20); | ||
| if (key.matchesAny(&.{ 'k', '-', vaxis.Key.up, vaxis.Key.kp_up, vaxis.Key.kp_subtract }, .{})) | ||
| try self.applyCmuxSelectionForCurrentTheme(); | ||
| } |
There was a problem hiding this comment.
vaxis.Key.kp_down double-binding causes two file writes per keypress in cmux mode
vaxis.Key.kp_down appears in both the j/+/down/kp_down/kp_add match (move 1) and the page_down/kp_down match (move 20). This is a pre-existing double-binding, but the PR makes it worse in cmux mode: a single kp_down keypress now triggers two separate applyCmuxSelectionForCurrentTheme() calls, leading to two config file writes and two NSDistributedNotification posts in rapid succession.
The page_down binding likely intended vaxis.Key.kp_page_down rather than vaxis.Key.kp_down:
| if (key.matchesAny(&.{ 'j', '+', vaxis.Key.down, vaxis.Key.kp_down, vaxis.Key.kp_add }, .{})) { | |
| self.down(1); | |
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) | |
| try self.applyCmuxSelectionForCurrentTheme(); | |
| } | |
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) { | |
| self.down(20); | |
| if (key.matchesAny(&.{ 'k', '-', vaxis.Key.up, vaxis.Key.kp_up, vaxis.Key.kp_subtract }, .{})) | |
| try self.applyCmuxSelectionForCurrentTheme(); | |
| } | |
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_page_down }, .{})) { | |
| self.down(20); | |
| try self.applyCmuxSelectionForCurrentTheme(); | |
| } |
* Add cmux theme picker helper hooks * Fix cmux theme picker preview writes * Improve cmux theme picker footer contrast * Respect system theme in cmux picker * Skip theme detection in cmux picker * Match Ghostty theme picker startup * Harden cmux theme override writes * Fix DECRPM mode 2031 reporting wrong color scheme after config reload 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. * Send initial color scheme report when mode 2031 is enabled Per the mode 2031 spec, the terminal must send an initial DSR 997 report when the mode is enabled via DECSET. Without this, clients that enable mode 2031 never receive the current color scheme. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
* Add cmux theme picker helper hooks * Fix cmux theme picker preview writes * Improve cmux theme picker footer contrast * Respect system theme in cmux picker * Skip theme detection in cmux picker * Match Ghostty theme picker startup * Harden cmux theme override writes * Fix DECRPM mode 2031 reporting wrong color scheme after config reload 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. * Send initial color scheme report when mode 2031 is enabled Per the mode 2031 spec, the terminal must send an initial DSR 997 report when the mode is enabled via DECSET. Without this, clients that enable mode 2031 never receive the current color scheme. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
1 participant
Adds initial DSR 997 report when DECSET 2031 is received. Also ensures Termio DerivedConfig uses surface-level conditional state during config reloads.
Summary by cubic
Send the initial DSR 997 when
DECSET 2031is enabled and fix incorrect color scheme reporting after config reloads. Also adds cmux theme picker integration to thelist_themesCLI for live preview and apply.Bug Fixes
New Features
DECSET 2031(report color scheme) is enabled.list_themes:CMUX_THEME_PICKER_*env vars, write a managedthemeoverride block, and restore on cancel.NSDistributedNotificationCenterreload notice tocom.cmuxterm.themes.reload-config.cli-helperbuild step to build the Ghostty CLI helper.Written for commit 41eeaaf. Summary will update on new commits.