Fix: Apple Terminal contrast by detecting macOS dark mode - #204
Open
prasadus92 wants to merge 2 commits into
Open
Fix: Apple Terminal contrast by detecting macOS dark mode#204prasadus92 wants to merge 2 commits into
prasadus92 wants to merge 2 commits into
Conversation
Apple Terminal was hard-coded as LIGHT background, but since macOS Mojave it follows the system appearance setting. Most modern macOS users run dark mode, causing light-theme colors (dark text) to render on dark backgrounds with near-zero contrast. Instead of assuming LIGHT, Apple Terminal now checks the actual macOS system appearance via `defaults read -g AppleInterfaceStyle`. Falls back to DARK if detection fails (matching the majority of users). Includes 7 new tests covering dark mode, light mode, and error fallback scenarios.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughApple Terminal detection now queries macOS appearance via a new Changes
Sequence DiagramsequenceDiagram
participant Detector as BackgroundDetector
participant Env as Environment Hints
participant Subproc as subprocess.run
participant macOS as macOS System
participant Handler as Error Handler
Detector->>Env: detect_background()
Env->>Env: _check_environment_hints()
Env->>Env: TERM_PROGRAM == "Apple_Terminal" ?
alt Apple Terminal
Env->>Detector: _check_macos_appearance()
Detector->>Subproc: run("defaults read -g AppleInterfaceStyle", timeout=2)
Subproc->>macOS: query AppleInterfaceStyle
alt returns "Dark\n"
macOS-->>Subproc: stdout "Dark\n"
Subproc-->>Detector: CompletedProcess(stdout="Dark\n")
Detector-->>Env: BackgroundType.DARK
else returns "" / key absent
macOS-->>Subproc: stderr indicates key missing / stdout ""
Subproc-->>Detector: CompletedProcess(non-zero / empty)
Detector-->>Env: BackgroundType.LIGHT
else Timeout/FileNotFound/OSError
Subproc-->>Handler: raises exception
Handler-->>Detector: fallback -> BackgroundType.DARK
Detector-->>Env: BackgroundType.DARK
end
else Other terminal
Env-->>Detector: continue other checks
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/claude_monitor/terminal/themes.py`:
- Around line 355-364: The subprocess.run call that checks macOS
AppleInterfaceStyle currently treats any non-zero exit as LIGHT; change the
logic so that after running subprocess.run (used to decide BackgroundType), you
still return BackgroundType.DARK for unexpected non-zero exits: if
result.returncode == 0 and "dark" in result.stdout.strip().lower() return
BackgroundType.DARK; else if result.returncode != 0 inspect result.stderr (or
result.stdout) and return BackgroundType.LIGHT only when the stderr/error text
indicates the key is missing (e.g., contains "does not exist" or "The
domain/default pair"); for any other non-zero exit or unexpected output, return
BackgroundType.DARK to preserve contrast; keep the existing except
(subprocess.TimeoutExpired, FileNotFoundError, OSError) behavior.
In `@src/tests/test_background_detection.py`:
- Around line 53-66: The Apple Terminal tests use `@patch.dict`("os.environ",
{"TERM_PROGRAM": "Apple_Terminal"}, clear=False) which can leak other env vars
and change test behavior; update these tests
(test_apple_terminal_delegates_to_macos_appearance and
test_apple_terminal_light_mode) to use clear=True so only TERM_PROGRAM is
present during BackgroundDetector._check_environment_hints() execution, ensuring
deterministic delegation to BackgroundDetector._check_macos_appearance().
🪄 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: 93cbb548-3dae-46a7-a3ec-ce9352a2a749
📒 Files selected for processing (2)
src/claude_monitor/terminal/themes.pysrc/tests/test_background_detection.py
- Only return LIGHT when defaults stderr confirms the AppleInterfaceStyle key is absent. Other non-zero exits now fall back to DARK for contrast safety. - Use clear=True in test env patches to prevent env var leakage from affecting test determinism. - Add test for unexpected non-zero exit fallback.
5 tasks
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
1 participant
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.
Problem
Apple Terminal (
TERM_PROGRAM=Apple_Terminal) is hard-coded to returnBackgroundType.LIGHTinBackgroundDetector._check_environment_hints(). However, since macOS Mojave (2018), Apple Terminal follows the system appearance — and most modern macOS users run dark mode.This causes light-theme colors (dark foreground text) to be applied on a dark terminal background, resulting in near-zero contrast where labels, values, and progress bars are barely readable.
Affected: Any macOS user running dark mode with the native Terminal.app.
Not affected: iTerm2 users (correctly detected as DARK).
Screenshots
macOS Terminal.app (before fix — barely readable)
iTerm2 (works correctly)
Fix
Instead of assuming
LIGHTfor Apple Terminal, the detector now calls a new_check_macos_appearance()method that queries the actual macOS system appearance:"Dark"→BackgroundType.DARKBackgroundType.LIGHTBackgroundType.DARKThe fallback to DARK matches the reality that most macOS users today have dark mode enabled.
Changes
src/claude_monitor/terminal/themes.py: Added_check_macos_appearance()static method toBackgroundDetector. Updated_check_environment_hints()to call it for Apple Terminal instead of returning hard-codedLIGHT.src/tests/test_background_detection.py: 7 new tests covering dark mode detection, light mode detection, timeout/error fallbacks, and Apple Terminal delegation.Testing
DARKSummary by CodeRabbit
Bug Fixes
Tests