feat(ui): add new theme "auto" that automatically switches between da…#241
feat(ui): add new theme "auto" that automatically switches between da…#241m4r1vs wants to merge 1 commit intomodem-dev:mainfrom
Conversation
…rk and light themes
| deps: { spawnSync: typeof Bun.spawnSync } = { spawnSync: Bun.spawnSync }, | ||
| ): "light" | "dark" | undefined { | ||
| if (process.platform !== "darwin") { | ||
| return undefined; |
There was a problem hiding this comment.
I don't know why the pager/piped flow works on Linux but it somehow does lol
Greptile SummaryIntroduces an
Confidence Score: 4/5Safe to merge; existing theme names are unaffected and the new auto-detection path is gated behind a feature flag. The core detection and menu logic work correctly. The main concern is that 'auto' is now advertised in the public HunkDiffView type and docs but always silently resolves to graphite in that component since themeMode is hardcoded to null. src/opentui/HunkDiffView.tsx and src/ui/themes.ts — the former advertises auto as a valid theme without a way to supply themeMode, and the latter silently falls to the dark palette whenever themeMode is null. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App starts] --> B{usesPipedPatchInput?}
B -- yes --> C[detectSystemThemeMode]
C -- darwin --> D[spawnSync defaults read]
D -- stdout == Dark --> E[initialThemeMode = dark]
D -- other/empty --> F[initialThemeMode = light]
D -- throws --> F
C -- non-darwin --> G[initialThemeMode = undefined]
B -- no --> H[initialThemeMode = undefined]
E & F & G & H --> I[App.useState: systemThemeMode = initialThemeMode ?? renderer.themeMode]
I --> J[renderer emits theme_mode]
J --> K[setSystemThemeMode]
K --> L{themeId === auto?}
I --> L
L -- yes --> M{systemThemeMode === light?}
M -- yes --> N[resolveTheme to themeLight]
M -- no/null --> O[resolveTheme to themeDark]
L -- no --> P[resolveTheme to exact match or graphite]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isDarwin = process.platform === "darwin"; | ||
|
|
||
| test("detects dark mode on macOS", () => { | ||
| if (!isDarwin) return; | ||
|
|
||
| const spawnSync = ((args: string[]) => { | ||
| expect(args).toEqual(["defaults", "read", "-g", "AppleInterfaceStyle"]); | ||
| return { stdout: Buffer.from("Dark\n") }; | ||
| }) as typeof Bun.spawnSync; | ||
|
|
||
| expect(detectSystemThemeMode({ spawnSync })).toBe("dark"); | ||
| }); | ||
|
|
||
| test("detects light mode on macOS", () => { | ||
| if (!isDarwin) return; | ||
|
|
There was a problem hiding this comment.
The three macOS detection tests each open with if (!isDarwin) return, which causes them to silently pass on Linux CI without executing any assertions. Because detectSystemThemeMode accepts an injected spawnSync, the tests can actually run on any platform without needing a real defaults binary. Only the final test ("returns undefined on non-macOS platforms") genuinely requires the real platform branch. The first three tests would be more reliable as unconditional tests that always inject the mock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/terminal.test.ts
Line: 120-135
Comment:
**macOS-specific tests are skipped on non-macOS CI despite using injectable deps**
The three macOS detection tests each open with `if (!isDarwin) return`, which causes them to silently pass on Linux CI without executing any assertions. Because `detectSystemThemeMode` accepts an injected `spawnSync`, the tests can actually run on any platform without needing a real `defaults` binary. Only the final test ("returns undefined on non-macOS platforms") genuinely requires the real platform branch. The first three tests would be more reliable as unconditional tests that always inject the mock.
How can I resolve this? If you propose a fix, please make it concise.| if (requested === "auto" || !requested) { | ||
| const preferred = themeMode === "light" ? themeLight : themeDark; | ||
| return THEMES.find((theme) => theme.id === preferred) ?? THEMES[0]!; |
There was a problem hiding this comment.
null themeMode always routes "auto" to the dark theme with no signal
resolveTheme uses themeMode === "light" to pick between themeLight and themeDark. When themeMode is null (renderer cannot report terminal color scheme), the condition is always false, so "auto" invariably resolves to themeDark. A user on a light terminal whose color-scheme detection is unsupported will permanently receive the dark palette with no indication or warning. The old code resolved the same way (to graphite), but only because themeMode was never read; now that the parameter is meaningful, a null value effectively forces dark mode in a way that may surprise light-terminal users who enable "Follow system".
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/themes.ts
Line: 296-298
Comment:
**`null` themeMode always routes "auto" to the dark theme with no signal**
`resolveTheme` uses `themeMode === "light"` to pick between `themeLight` and `themeDark`. When `themeMode` is `null` (renderer cannot report terminal color scheme), the condition is always `false`, so "auto" invariably resolves to `themeDark`. A user on a light terminal whose color-scheme detection is unsupported will permanently receive the dark palette with no indication or warning. The old code resolved the same way (to graphite), but only because `themeMode` was never read; now that the parameter is meaningful, a `null` value effectively forces dark mode in a way that may surprise light-terminal users who enable "Follow system".
How can I resolve this? If you propose a fix, please make it concise.
|
Ran into an issue where it would use the dark theme when opening it immediately after switching from dark to light in system preferences on MacOS. I'll undraft when I've fixed it. |
| try { | ||
| // defaults read -g AppleInterfaceStyle returns "Dark" in dark mode. | ||
| // It exits with code 1 if the key doesn't exist (which means light mode). | ||
| const proc = deps.spawnSync(["defaults", "read", "-g", "AppleInterfaceStyle"]); |
There was a problem hiding this comment.
I find this idea a bit tricky. Some people use macOS in a light theme but still want a dark theme for the terminal. To me, the terminal should be the main source of information to keep things clear. For instance, you could use CSI to notify the tool when the colour palette changes. OpenTUI support it via the theme mode.
There was a problem hiding this comment.
yeah, but then you could just not use the auto theme, no?
The issue with the OpenTUI theme mode is that it does not work when Hunk is used as a pager because stdin is redirected
There was a problem hiding this comment.
Fair enough. It is just odd that Hunk will not follow the terminal when I change the light/dark themes outside of the OS general settings.
2 participants
fixes #238
autothemetheme_lightandtheme_darkbased on the detected terminal or system mode (with support for pipe-mode, e.g. whengit diff | hunk patch -is used.Technical details
detectSystemThemeModeto fall back to OS-level settings (starting with macOS support viadefaults read -g AppleInterfaceStyle) when terminal-based detection is inconclusive (e.g., in piped/pager flows).ThemeModelisteners in the ReactAppshell to respond to live system theme changes.README.mdand component documentation to reflect the new theme options.