Repository navigation
Fix browser pane dark-mode leak on light pages - #2339
austinywang wants to merge 2 commits into
Conversation
|
@austinywang is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRefactored browser theme application to centralize WebKit appearance switching via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 removes the browser-pane theme-override system that was forcing Key changes:
Confidence Score: 4/5Safe to merge after deleting the orphaned test class and test method that reference the deleted BrowserThemeSettings/BrowserThemeMode types — the unit-test target will not compile as-is All three production-code files are clean and the change is self-consistent. The sole blocking issue is that cmuxTests/BrowserConfigTests.swift still contains BrowserThemeSettingsTests and testBrowserPanelThemeModeUpdatesWebViewAppearance, which reference types and methods that no longer exist, causing a compile error in the unit-test target. Once those are deleted the PR is ready to merge. cmuxTests/BrowserConfigTests.swift — BrowserThemeSettingsTests class (lines 1049–1090) and testBrowserPanelThemeModeUpdatesWebViewAppearance (lines 1170–1181) reference deleted symbols Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BrowserPanel init / makeWebView] --> B[Set underPageBackgroundColor\nfrom GhosttyBackgroundTheme]
A --> C[Set customUserAgent]
B --> D[WKWebView loads page]
D --> E{Website controls\nits own theme}
E -->|light site| F[Renders in light mode]
E -->|dark site| G[Renders in dark mode]
E -->|respects prefers-color-scheme| H[Follows macOS system appearance]
subgraph REMOVED ["Removed (was: BrowserThemeMode override)"]
R1[Set webView.appearance\n.aqua / .darkAqua / nil]
R2[Inject color-scheme CSS\nvia evaluateJavaScript]
R3[Theme toolbar button\nin BrowserPanelView]
R4[Browser Theme picker\nin SettingsView]
end
style REMOVED fill:#fee2e2,stroke:#ef4444
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/BrowserPanel.swift (1)
2396-2458:⚠️ Potential issue | 🔴 CriticalUpdate the removed theme API tests in the same PR.
This change removes the browser theme surface from
BrowserPanel, butcmuxTests/BrowserConfigTests.swiftstill referencesBrowserThemeSettings,BrowserThemeMode, andBrowserPanel.setBrowserThemeMode(_:). As-is, the test target will stop compiling. Please delete or rewrite those tests to cover the new contract instead (no forcedappearance/ no injected color-scheme override).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 2396 - 2458, Tests in cmuxTests/BrowserConfigTests.swift still reference the removed theme APIs (BrowserThemeSettings, BrowserThemeMode, BrowserPanel.setBrowserThemeMode(_:)) and must be updated; remove those references and either delete the obsolete tests or rewrite them to assert the new contract: verify BrowserPanel.makeWebView(profileID:websiteDataStore:) / configureWebViewConfiguration(...) no longer forces WKWebView appearance or injects color-scheme CSS and that underPageBackgroundColor is set to GhosttyBackgroundTheme.currentColor(), and ensure no calls remain to BrowserPanel.setBrowserThemeMode(_:), BrowserThemeSettings, or BrowserThemeMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2396-2458: Tests in cmuxTests/BrowserConfigTests.swift still
reference the removed theme APIs (BrowserThemeSettings, BrowserThemeMode,
BrowserPanel.setBrowserThemeMode(_:)) and must be updated; remove those
references and either delete the obsolete tests or rewrite them to assert the
new contract: verify BrowserPanel.makeWebView(profileID:websiteDataStore:) /
configureWebViewConfiguration(...) no longer forces WKWebView appearance or
injects color-scheme CSS and that underPageBackgroundColor is set to
GhosttyBackgroundTheme.currentColor(), and ensure no calls remain to
BrowserPanel.setBrowserThemeMode(_:), BrowserThemeSettings, or BrowserThemeMode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d344422b-db61-4528-bf18-1da761f294cb
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swift
💤 Files with no reviewable changes (2)
- Sources/Panels/BrowserPanelView.swift
- Sources/cmuxApp.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 214-223: BrowserThemeSettings.apply(_:to:) currently forces
webView.appearance for .light/.dark which contradicts the "do not force page
appearance" policy; change apply(_ mode: BrowserThemeMode, to webView:
WKWebView) to always set webView.appearance = nil (remove the .aqua/.darkAqua
branches) and stop calling applyBrowserThemeModeIfNeeded(...) for both main and
popup web views so no overrides are reapplied; also update the migration/loading
of browserThemeMode (where saved UserDefaults are read) to either clear the
legacy browserThemeMode preference during migration or ignore persisted values
and default to .system so upgraded users aren’t stuck with a forced 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: 67d18d99-47f6-4845-8e5e-07333fc31ba9
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swift
| static func apply(_ mode: BrowserThemeMode, to webView: WKWebView) { | ||
| switch mode { | ||
| case .system: | ||
| webView.appearance = nil | ||
| case .light: | ||
| webView.appearance = NSAppearance(named: .aqua) | ||
| case .dark: | ||
| webView.appearance = NSAppearance(named: .darkAqua) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && sed -n '214,223p' Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 119
🏁 Script executed:
cd /tmp/repo && sed -n '4944,4949p' Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 119
🏁 Script executed:
cd /tmp/repo && sed -n '5451,5453p' Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 119
🏁 Script executed:
cd /tmp/repo && sed -n '2486,2489p' Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 119
🏁 Script executed:
cd /tmp/repo && sed -n '2309,2311p' Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 119
🏁 Script executed:
pwd && git ls-files Sources/Panels/BrowserPanel.swift | head -5Repository: manaflow-ai/cmux
Length of output: 112
🏁 Script executed:
wc -l /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 118
🏁 Script executed:
sed -n '214,223p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 394
🏁 Script executed:
sed -n '4944,4949p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 292
🏁 Script executed:
sed -n '5451,5453p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 174
🏁 Script executed:
grep -n "browserThemeMode" /home/jailuser/git/Sources/Panels/BrowserPanel.swift | head -20Repository: manaflow-ai/cmux
Length of output: 366
🏁 Script executed:
grep -n "var browserThemeMode" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 115
🏁 Script executed:
sed -n '2309,2311p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 142
🏁 Script executed:
grep -n "BrowserThemeSettings.mode()" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 124
🏁 Script executed:
grep -n "class BrowserThemeSettings" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
sed -n '185,230p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 1640
🏁 Script executed:
grep -n "static func mode()" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
sed -n '2600,2620p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 1146
🏁 Script executed:
sed -n '2605,2620p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 795
🏁 Script executed:
sed -n '2614,2630p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 870
🏁 Script executed:
grep -n "applyBrowserThemeModeIfNeeded\|BrowserThemeSettings.apply" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 403
🏁 Script executed:
sed -n '2700,2710p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 484
🏁 Script executed:
sed -n '2860,2870p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 487
🏁 Script executed:
sed -n '2480,2495p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 797
🏁 Script executed:
grep -n "makeWebView\|Do not force page appearance" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 394
🏁 Script executed:
sed -n '2481,2495p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 796
🏁 Script executed:
sed -n '185,230p' /home/jailuser/git/Sources/Panels/BrowserPanel.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 1962
🏁 Script executed:
grep -n "BrowserThemeMode\|enum BrowserThemeMode" /home/jailuser/git/Sources/Panels/BrowserPanel.swift | head -10Repository: manaflow-ai/cmux
Length of output: 782
🏁 Script executed:
sed -n '156,165p' /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 272
🏁 Script executed:
rg "BrowserThemeMode\.(system|light|dark)" /home/jailuser/git/Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 42
BrowserThemeSettings.apply() still overrides webView.appearance, contradicting the "do not force page appearance" design.
The apply() method at lines 217–221 still maps .light → .aqua and .dark → .darkAqua, then reapplies that override through applyBrowserThemeModeIfNeeded() at lines 4946 and 5452 for both main and popup views. Meanwhile, the makeWebView() comment (lines 2487–2488) explicitly states "Do not force page appearance or inject color-scheme CSS; websites must keep control of their own theme" — but this intent is undermined by the active override.
Since BrowserPanel loads browserThemeMode from persisted UserDefaults (line 2607) and the migration logic (lines 206–207) preserves legacy dark-mode settings, upgraded users can remain stuck with a forced appearance indefinitely after the UI for that preference disappears.
Remove the appearance override entirely: set webView.appearance = nil unconditionally in apply(), and either clear the stale browserThemeMode preference during migration or ignore persisted values to reset to .system (the intended default).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanel.swift` around lines 214 - 223,
BrowserThemeSettings.apply(_:to:) currently forces webView.appearance for
.light/.dark which contradicts the "do not force page appearance" policy; change
apply(_ mode: BrowserThemeMode, to webView: WKWebView) to always set
webView.appearance = nil (remove the .aqua/.darkAqua branches) and stop calling
applyBrowserThemeModeIfNeeded(...) for both main and popup web views so no
overrides are reapplied; also update the migration/loading of browserThemeMode
(where saved UserDefaults are read) to either clear the legacy browserThemeMode
preference during migration or ignore persisted values and default to .system so
upgraded users aren’t stuck with a forced appearance.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
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="Sources/Panels/BrowserPanelView.swift">
<violation number="1">
P1: Applying `normalizedMode` here reintroduces forced `WKWebView.appearance` overrides for light/dark modes, which can cause embedded pages to lose control of their own color scheme again.</violation>
</file>
<file name="Sources/Panels/BrowserPopupWindowController.swift">
<violation number="1" location="Sources/Panels/BrowserPopupWindowController.swift:114">
P2: This line forces popup WKWebView appearance to light/dark when a stored mode is set, which overrides embedded pages’ own color-scheme choices and can reintroduce the dark-mode bleed this PR is trying to remove. Consider leaving the appearance unset for popups as well.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| webView.underPageBackgroundColor = GhosttyBackgroundTheme.currentColor() | ||
| webView.customUserAgent = BrowserUserAgentSettings.safariUserAgent | ||
| BrowserThemeSettings.apply(openerPanel?.currentBrowserThemeMode ?? BrowserThemeSettings.mode(), to: webView) |
There was a problem hiding this comment.
P2: This line forces popup WKWebView appearance to light/dark when a stored mode is set, which overrides embedded pages’ own color-scheme choices and can reintroduce the dark-mode bleed this PR is trying to remove. Consider leaving the appearance unset for popups as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPopupWindowController.swift, line 114:
<comment>This line forces popup WKWebView appearance to light/dark when a stored mode is set, which overrides embedded pages’ own color-scheme choices and can reintroduce the dark-mode bleed this PR is trying to remove. Consider leaving the appearance unset for popups as well.</comment>
<file context>
@@ -111,6 +111,7 @@ final class BrowserPopupWindowController: NSObject, NSWindowDelegate {
}
webView.underPageBackgroundColor = GhosttyBackgroundTheme.currentColor()
webView.customUserAgent = BrowserUserAgentSettings.safariUserAgent
+ BrowserThemeSettings.apply(openerPanel?.currentBrowserThemeMode ?? BrowserThemeSettings.mode(), to: webView)
self.webView = webView
</file context>
|
Closing this fork-based PR in favor of #2346, which is opened from manaflow-ai/cmux per repo workflow. |
This branch was successfully deployed
1 participant
Summary
color-schemeoverrides or DOM theme metadata into browser panesWKWebViewappearance so the selected light/dark/system mode stays consistentunderPageBackgroundColoronly for the unpainted/loading regionTesting
./scripts/reload.sh --tag browser-dark-mode-leak --launch./scripts/reload.sh --tag browser-dark-mode-leak./scripts/reload.sh --tag browser-dark-mode-leakCloses #2337
Related to #2083
Summary by CodeRabbit