feat(v3,windows,macos): introduce application and window theme system (#4665)#5042
Open
devmadhava wants to merge 6 commits intowailsapp:v3-alphafrom
Open
feat(v3,windows,macos): introduce application and window theme system (#4665)#5042devmadhava wants to merge 6 commits intowailsapp:v3-alphafrom
devmadhava wants to merge 6 commits intowailsapp:v3-alphafrom
Conversation
…andalone functions
…r events for app.SetTheme() and win.SetTheme()
Contributor
WalkthroughAdds a three-layer theming system (AppTheme, WinTheme, platform-resolved Theme), window-level followApplicationTheme behavior, App/Window Get/Set theme APIs and events, platform-specific resolution/sync logic for Windows and macOS, and a demo app with JS bindings and UI. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as Frontend (JS)
participant WS as WindowService (Go)
participant App as App Instance
participant Platform as Platform Layer
participant OS as Native OS
JS->>WS: SetAppTheme("dark")
WS->>App: SetTheme(AppDark)
activate App
App->>App: validate & update theme
App->>Platform: setTheme(AppDark)
activate Platform
Platform->>Platform: iterate windows -> window.syncTheme()
Platform->>OS: apply native appearance (DWM / NSAppearance)
deactivate Platform
App->>JS: emit ApplicationThemeChanged
deactivate App
sequenceDiagram
participant JS as Frontend (JS)
participant WS as WindowService (Go)
participant W as WebviewWindow
participant Platform as Platform Layer
participant OS as Native OS
JS->>WS: SetWinTheme("light")
WS->>W: SetTheme(WinLight)
activate W
W->>W: update followApplicationTheme/internal state
W->>Platform: setTheme(WinLight)
activate Platform
alt Windows
Platform->>Platform: resolveWindowsEffectiveTheme(win, app)
Platform->>OS: apply resolved numeric Theme
else macOS
Platform->>OS: set NSAppearance explicit override
end
deactivate Platform
W->>JS: emit ThemeChanged
deactivate W
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.3)v3/examples/theme/WindowService.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v3/examples/theme/main.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v3/pkg/application/application.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
v3/pkg/application/application_server.go (1)
329-334: Receiver naming inconsistency.The
setThememethod uses receiver namea, while all otherserverAppmethods in this file useh. Consider aligning for consistency.🔧 Suggested fix for receiver naming
-func (a *serverApp) setTheme(theme AppTheme) { +func (h *serverApp) setTheme(theme AppTheme) { // TODO: Implement theme setting for Server }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/application_server.go` around lines 329 - 334, Receiver name mismatch: change the receiver on the method setTheme from "a" to "h" to match other serverApp methods. Edit the method signature func (a *serverApp) setTheme(theme AppTheme) to use func (h *serverApp) setTheme(theme AppTheme) so it is consistent with other methods on serverApp (e.g., those using receiver h).v3/examples/theme/main.go (1)
41-43: Prefer the exported theme constants in the demo.Because this is the reference example for the new theme API, using raw
"light"/"dark"literals hides the actual enum surface and makes typos easy. Please use the exportedWinThemeconstants here instead of string literals.Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/theme/main.go` around lines 41 - 43, Replace the raw string literals used for the Windows theme with the exported WinTheme constants to show the public API and avoid typos: update the Theme assignments on the WindowsWindow struct (currently using "light"/"dark") to use the corresponding WinTheme constants (e.g., WinThemeLight / WinThemeDark) in the occurrences around the WindowsWindow initializers (including the second occurrence mentioned at lines 59-61).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/examples/theme/assets/style.css`:
- Around line 31-40: The .wrapper CSS rule declares gap twice so the second
declaration (gap: 1px) overrides gap: 50px; remove or correct the unintended
duplicate so the intended spacing is applied — locate the .wrapper selector in
style.css and either delete the erroneous gap: 1px line or replace it with the
intended value (or consolidate into a single gap declaration) to ensure only the
desired gap is present.
In `@v3/examples/theme/main.go`:
- Around line 17-18: The example metadata still says "customEventProcessor Demo"
and its Description references the customEventProcessor feature; update the
metadata fields (the Name and Description entries in the example's metadata
struct in main.go) to correctly describe the theme example (e.g., change Name to
something like "theme Demo" and Description to "A demo of the theme API" or
similar) so logs/metadata reflect the actual feature.
In `@v3/examples/theme/README.md`:
- Line 67: Fix the typo in the README sentence: replace "swicthing" with
"switching" in the line that reads "The macOS implementation handles
light/dark/system swicthing by following the exact same Intent vs. Resolution
flow as Windows, however rather than store the resolved theme, **it infers the
resolved theme from Internal OS Mechanics.**" so the sentence reads "switching"
instead of "swicthing".
- Around line 107-114: Update the README file's file list to fix the typo and
numbering: change "application_linus_gtk4.go" to "application_linux_gtk4.go" and
renumber the "Files Created" list so items are sequential (1 through 5) ensuring
the missing `#3` is inserted or the indices adjusted accordingly; update the
entries `theme_application.go`, `theme_window.go`,
`theme_webview_window_darwin.go`, `theme_webview_window_windows.go`, and any
other listed filenames to reflect the corrected numbering.
In `@v3/examples/theme/WindowService.go`:
- Around line 21-28: SetWinTheme is using s.app.Window.Current() instead of the
caller's window from ctx and GetWinTheme lacks a nil/missing WindowKey check;
change SetWinTheme to retrieve the window via
ctx.Value(application.WindowKey).(application.Window) (the same pattern used in
GetWinTheme) and modify GetWinTheme to safely assert/check
ctx.Value(application.WindowKey) before calling GetTheme (return a safe default
or error behavior when WindowKey is missing) so both methods consistently
operate on the invoking window and avoid panics.
In `@v3/pkg/application/application_darwin.go`:
- Around line 266-277: The setTheme method on macosApp iterates m.parent.windows
without locking, causing a data race; acquire a read lock on the parent’s
windowsLock (use m.parent.windowsLock.RLock() and defer RUnlock()) around the
for loop before iterating m.parent.windows, then call impl.syncTheme() on
matching WebviewWindow/mac osWebviewWindow instances as before; also fix the
comment typos ("Cycle through individual window themes to trigger theme
resolution"). Ensure you reference macosApp.setTheme, m.parent.windows,
m.parent.windowsLock, WebviewWindow, macosWebviewWindow, and impl.syncTheme when
making the change.
In `@v3/pkg/application/application_options.go`:
- Around line 136-137: Update the field comment to start with the exported field
name "Theme" so it matches Go doc conventions; locate the Theme field (type
AppTheme) in the application options struct and change the comment prefix from
"ApplicationTheme" to "Theme" and keep the rest of the description unchanged so
the doc correctly references the exported symbol Theme.
In `@v3/pkg/application/application.go`:
- Around line 186-190: New currently assigns appOptions.Theme directly to
result.theme allowing invalid values; instead validate Options.Theme before
storing by either calling the public setter (App.SetTheme) or reusing the same
validation logic used by SetTheme (e.g., an isValidTheme check) and only assign
when it matches a known AppTheme; if empty or invalid, leave result.theme as
AppSystemDefault. Update the initialization in New to reference the same
validation used by SetTheme so GetTheme never returns an arbitrary string.
In `@v3/pkg/application/theme_application.go`:
- Around line 50-51: The code emits the wrong cross-platform event name
("common:ApplicationThemeChanged") so consumers of the documented shared event
miss updates; change the call to emit the canonical event name
"common:ThemeChanged" instead (update the a.Event.Emit invocation in the theme
change path), ensuring App.SetTheme()/the Application theme-change flow uses the
known_events/common-event map convention "common:ThemeChanged" so listeners
subscribed to the shared event receive the notification.
In `@v3/pkg/application/theme_webview_window_windows.go`:
- Around line 34-80: syncTheme and setTheme mutate w.theme and
w.parent.followApplicationTheme without synchronization while SystemThemeChanged
reads them from another goroutine; add a mutex on windowsWebviewWindow (e.g.,
themeMu sync.RWMutex) and use it to protect all accesses: use
themeMu.Lock()/Unlock() around writes in setTheme and syncTheme (and when
changing parent.followApplicationTheme) and themeMu.RLock()/RUnlock() around
reads in the SystemThemeChanged listener in webview_window_windows.go so the
theme state cannot race across goroutines.
In `@v3/pkg/application/theme_window_windows.go`:
- Around line 3-11: The exported Windows-only theme enum (Theme, SystemDefault,
Dark, Light) is leaking a platform-specific API; make these internal by
unexporting or renaming them to clearly internal/resolved variants (e.g., rename
Theme -> winTheme or resolvedTheme and constants SystemDefault/Dark/Light ->
winSystemDefault/winDark/winLight) and update any references (including the
public WinTheme usage) to the new identifiers so callers no longer see these
Windows-only symbols in pkg/application.
In `@v3/pkg/application/theme_window.go`:
- Around line 34-50: SetTheme currently drops requests when w.impl is nil and
still emits ThemeChanged; add a persistent field on WebviewWindow (e.g.,
requestedTheme WinTheme) that SetTheme assigns even if w.impl == nil, update
GetTheme to return requestedTheme when impl is nil, and in the code path that
creates/initializes the native window (Run()/Show() or where impl is set) call
impl.setTheme(requestedTheme) to apply it; also adjust emission so ThemeChanged
is emitted when requestedTheme actually changes and/or after it's applied to the
impl (avoid emitting for a change that was never stored or applied).
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 1341-1354: The setters setAppearanceByName and clearAppearance
currently dispatch asynchronously which can race with readers; change them to
perform the same synchronous main-thread handoff used by the getters: use the
synchronous dispatch/wait pattern on globalApplication (e.g.,
dispatchOnMainThreadSync or the existing dispatchOnMainThread with a done
channel) so the call blocks until C.windowSetAppearanceTypeByName(w.nsWindow,
C.CString(...)) and C.windowClearAppearanceType(w.nsWindow) have completed
before returning from setAppearanceByName and clearAppearance.
In `@v3/pkg/application/webview_window_options.go`:
- Around line 243-245: Update the comment for the exported Theme field to match
the WinTheme API and actual default behavior: state the accepted values
(application, system, dark, light), remove the outdated "SystemDefault" name,
and clarify that the zero value (default) means "follow application" (i.e.,
inherit the application theme) as implemented in the resolution logic; reference
the Theme field and WinTheme type so readers know which enum values apply.
In `@v3/pkg/application/webview_window_windows.go`:
- Around line 526-535: The SystemThemeChanged handler currently returns early
when w.parent.followApplicationTheme is false, preventing windows with w.theme
== SystemDefault (WinThemeSystem) from following OS changes; modify the guard in
the onApplicationEvent callback so it only checks the window's theme (i.e.,
remove the followApplicationTheme check) and allow the
InvokeAsync(w.updateTheme(...)) to run whenever w.theme == SystemDefault; locate
the handler registered on events.Windows.SystemThemeChanged in
webview_window_windows.go and update the condition that currently references
w.parent.followApplicationTheme and w.theme to only inspect w.theme.
In `@v3/pkg/application/window.go`:
- Around line 106-108: Change GetTheme() to return the typed WinTheme instead of
string to keep the API symmetric with SetTheme(theme WinTheme); update the
interface signature from GetTheme() string to GetTheme() WinTheme and then
update all implementations (methods named GetTheme on structs implementing the
window interface) and all call sites to expect a WinTheme value (or convert
existing strings to WinTheme where appropriate). If you intended a separate
“resolved vs intent” distinction instead, add a new getter with a distinct name
(e.g., GetResolvedTheme() WinTheme) rather than leaving GetTheme as a raw
string, and update implementations/callers accordingly.
---
Nitpick comments:
In `@v3/examples/theme/main.go`:
- Around line 41-43: Replace the raw string literals used for the Windows theme
with the exported WinTheme constants to show the public API and avoid typos:
update the Theme assignments on the WindowsWindow struct (currently using
"light"/"dark") to use the corresponding WinTheme constants (e.g., WinThemeLight
/ WinThemeDark) in the occurrences around the WindowsWindow initializers
(including the second occurrence mentioned at lines 59-61).
In `@v3/pkg/application/application_server.go`:
- Around line 329-334: Receiver name mismatch: change the receiver on the method
setTheme from "a" to "h" to match other serverApp methods. Edit the method
signature func (a *serverApp) setTheme(theme AppTheme) to use func (h
*serverApp) setTheme(theme AppTheme) so it is consistent with other methods on
serverApp (e.g., those using receiver h).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71c752f8-2465-4243-b3b2-26d88568998d
📒 Files selected for processing (28)
v3/examples/theme/README.mdv3/examples/theme/WindowService.gov3/examples/theme/assets/app.jsv3/examples/theme/assets/index.htmlv3/examples/theme/assets/style.cssv3/examples/theme/main.gov3/pkg/application/application.gov3/pkg/application/application_android.gov3/pkg/application/application_android_nocgo.gov3/pkg/application/application_darwin.gov3/pkg/application/application_ios.gov3/pkg/application/application_linux.gov3/pkg/application/application_linux_gtk4.gov3/pkg/application/application_options.gov3/pkg/application/application_server.gov3/pkg/application/application_windows.gov3/pkg/application/events_common_ios.gov3/pkg/application/theme_application.gov3/pkg/application/theme_webview_window_darwin.gov3/pkg/application/theme_webview_window_windows.gov3/pkg/application/theme_window.gov3/pkg/application/theme_window_windows.gov3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_options.gov3/pkg/application/webview_window_options_test.gov3/pkg/application/webview_window_windows.gov3/pkg/application/window.go
v3/examples/theme/WindowService.go
Outdated
Comment on lines
+21
to
+28
| func (s *WindowService) SetWinTheme(ctx context.Context, theme string) { | ||
| win := s.app.Window.Current() | ||
| win.SetTheme((application.WinTheme(theme))) | ||
| } | ||
|
|
||
| func (s *WindowService) GetWinTheme(ctx context.Context) string { | ||
| win := ctx.Value(application.WindowKey).(application.Window) | ||
| return win.GetTheme() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find WindowKey usage to understand how it's populated
rg "WindowKey" -B 2 -A 2Repository: wailsapp/wails
Length of output: 50371
🏁 Script executed:
# Check the messageprocessor that supposedly handles WindowKey
fd "messageprocessor" --type fRepository: wailsapp/wails
Length of output: 754
🏁 Script executed:
# Look at Window.Current() implementation
rg "func.*Current\(\)" -A 5 | grep -A 5 "Window"Repository: wailsapp/wails
Length of output: 694
🏁 Script executed:
# Check other example services to see the pattern they use
fd "Service.go" --type f -path "*/examples/*"Repository: wailsapp/wails
Length of output: 226
🏁 Script executed:
# Read messageprocessor_call.go to understand context handling
cat -n v3/pkg/application/messageprocessor_call.goRepository: wailsapp/wails
Length of output: 4468
🏁 Script executed:
# Read the actual WindowService.go file to see the complete code
cat -n v3/examples/theme/WindowService.goRepository: wailsapp/wails
Length of output: 879
🏁 Script executed:
# Find other service examples to check for pattern consistency
find v3/examples -name "*Service.go" -type fRepository: wailsapp/wails
Length of output: 176
🏁 Script executed:
# Check if there are other similar service examples that access Window
rg "Window.*Current|WindowKey" v3/examples --type go -B 2 -A 2Repository: wailsapp/wails
Length of output: 5828
🏁 Script executed:
# Look at what other bound services do for context handling
rg "ctx.*Value" v3/examples --type go -B 1 -A 1Repository: wailsapp/wails
Length of output: 601
Use the caller's window from ctx for both methods consistently.
SetWinTheme currently themes s.app.Window.Current() instead of the window that invoked the binding. In multi-window scenarios, this applies the theme to whichever window is focused, not the calling window. GetWinTheme also uses ctx correctly but lacks a nil check and will panic if WindowKey is missing. Compare with v3/examples/window-call/main.go, which demonstrates the correct pattern for binding methods that access the calling window.
🛠️ Suggested fix
func (s *WindowService) SetWinTheme(ctx context.Context, theme string) {
- win := s.app.Window.Current()
- win.SetTheme((application.WinTheme(theme)))
+ win, ok := ctx.Value(application.WindowKey).(application.Window)
+ if !ok || win == nil {
+ return
+ }
+ win.SetTheme(application.WinTheme(theme))
}
func (s *WindowService) GetWinTheme(ctx context.Context) string {
- win := ctx.Value(application.WindowKey).(application.Window)
+ win, ok := ctx.Value(application.WindowKey).(application.Window)
+ if !ok || win == nil {
+ return ""
+ }
return win.GetTheme()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *WindowService) SetWinTheme(ctx context.Context, theme string) { | |
| win := s.app.Window.Current() | |
| win.SetTheme((application.WinTheme(theme))) | |
| } | |
| func (s *WindowService) GetWinTheme(ctx context.Context) string { | |
| win := ctx.Value(application.WindowKey).(application.Window) | |
| return win.GetTheme() | |
| func (s *WindowService) SetWinTheme(ctx context.Context, theme string) { | |
| win, ok := ctx.Value(application.WindowKey).(application.Window) | |
| if !ok || win == nil { | |
| return | |
| } | |
| win.SetTheme(application.WinTheme(theme)) | |
| } | |
| func (s *WindowService) GetWinTheme(ctx context.Context) string { | |
| win, ok := ctx.Value(application.WindowKey).(application.Window) | |
| if !ok || win == nil { | |
| return "" | |
| } | |
| return win.GetTheme() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/examples/theme/WindowService.go` around lines 21 - 28, SetWinTheme is
using s.app.Window.Current() instead of the caller's window from ctx and
GetWinTheme lacks a nil/missing WindowKey check; change SetWinTheme to retrieve
the window via ctx.Value(application.WindowKey).(application.Window) (the same
pattern used in GetWinTheme) and modify GetWinTheme to safely assert/check
ctx.Value(application.WindowKey) before calling GetTheme (return a safe default
or error behavior when WindowKey is missing) so both methods consistently
operate on the invoking window and avoid panics.
v3/pkg/application/theme_window.go
Outdated
Comment on lines
+34
to
+50
| func (w *WebviewWindow) GetTheme() string { | ||
| if w.impl == nil { | ||
| return WinThemeApplication.String() | ||
| } | ||
| return w.impl.getTheme().String() | ||
| } | ||
|
|
||
| // SetTheme sets the theme for the window. | ||
| func (w *WebviewWindow) SetTheme(theme WinTheme) { | ||
| if !theme.Valid() { | ||
| return | ||
| } | ||
| if w.impl != nil { | ||
| w.impl.setTheme(theme) | ||
| } | ||
| // Notify listeners of the theme change | ||
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) |
Contributor
There was a problem hiding this comment.
Persist the requested window theme before the native window exists.
SetTheme is a no-op when w.impl is nil, so calling it before Run()/Show() silently drops the requested intent. GetTheme() then still returns "application", and this method even emits common:ThemeChanged for a change that never stuck. Please store the requested WinTheme on WebviewWindow (or its options) and apply it during Run().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/application/theme_window.go` around lines 34 - 50, SetTheme currently
drops requests when w.impl is nil and still emits ThemeChanged; add a persistent
field on WebviewWindow (e.g., requestedTheme WinTheme) that SetTheme assigns
even if w.impl == nil, update GetTheme to return requestedTheme when impl is
nil, and in the code path that creates/initializes the native window
(Run()/Show() or where impl is set) call impl.setTheme(requestedTheme) to apply
it; also adjust emission so ThemeChanged is emitted when requestedTheme actually
changes and/or after it's applied to the impl (avoid emitting for a change that
was never stored or applied).
Comment on lines
+1341
to
+1354
| func (w *macosWebviewWindow) setAppearanceByName(appearanceName MacAppearanceType) { | ||
| // Dispatching on GlobalApplication's main thread to ensure that the window is fully initialized before we try to set the appearance | ||
| // These are utilized in an event listener hence explicitly stating globalApplication | ||
| globalApplication.dispatchOnMainThread(func() { | ||
| C.windowSetAppearanceTypeByName(w.nsWindow, C.CString(string(appearanceName))) | ||
| }) | ||
| } | ||
|
|
||
| // clearAppearance removes any explicit appearance override from the window. | ||
| // Once cleared, the window implicitly follows the system appearance. | ||
| func (w *macosWebviewWindow) clearAppearance() { | ||
| globalApplication.dispatchOnMainThread(func() { | ||
| C.windowClearAppearanceType(w.nsWindow) | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't make the appearance setters fire-and-forget.
The read side waits for the AppKit round-trip to finish, but these two methods only queue the write. That makes SetTheme(...); GetTheme() race on the same window, and run() can first-paint before the queued appearance write runs.
Possible direction (using the same synchronous handoff pattern as the getters)
func (w *macosWebviewWindow) setAppearanceByName(appearanceName MacAppearanceType) {
- globalApplication.dispatchOnMainThread(func() {
- C.windowSetAppearanceTypeByName(w.nsWindow, C.CString(string(appearanceName)))
- })
+ var wg sync.WaitGroup
+ wg.Add(1)
+ globalApplication.dispatchOnMainThread(func() {
+ defer wg.Done()
+ C.windowSetAppearanceTypeByName(w.nsWindow, C.CString(string(appearanceName)))
+ })
+ wg.Wait()
}
// clearAppearance removes any explicit appearance override from the window.
// Once cleared, the window implicitly follows the system appearance.
func (w *macosWebviewWindow) clearAppearance() {
- globalApplication.dispatchOnMainThread(func() {
- C.windowClearAppearanceType(w.nsWindow)
- })
+ var wg sync.WaitGroup
+ wg.Add(1)
+ globalApplication.dispatchOnMainThread(func() {
+ defer wg.Done()
+ C.windowClearAppearanceType(w.nsWindow)
+ })
+ wg.Wait()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/application/webview_window_darwin.go` around lines 1341 - 1354, The
setters setAppearanceByName and clearAppearance currently dispatch
asynchronously which can race with readers; change them to perform the same
synchronous main-thread handoff used by the getters: use the synchronous
dispatch/wait pattern on globalApplication (e.g., dispatchOnMainThreadSync or
the existing dispatchOnMainThread with a done channel) so the call blocks until
C.windowSetAppearanceTypeByName(w.nsWindow, C.CString(...)) and
C.windowClearAppearanceType(w.nsWindow) have completed before returning from
setAppearanceByName and clearAppearance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
v3/pkg/application/theme_window.go (1)
33-51:⚠️ Potential issue | 🟡 MinorSetTheme silently drops requests when called before window initialization.
When
w.implis nil (beforeRun()orShow()),SetThemereturns without storing the requested theme. This means:
- Pre-initialization theme requests are lost
GetTheme()returnsWinAppDefaultregardless of what was requestedConsider storing the requested theme on
WebviewWindowand applying it when the native window is created.🛠️ Suggested approach
+// requestedTheme stores the theme requested before impl is available +// (on WebviewWindow struct, not shown here) + func (w *WebviewWindow) SetTheme(theme WinTheme) { if !theme.Valid() { return } + w.requestedTheme = theme if w.impl != nil { w.impl.setTheme(theme) // Notify listeners of the theme change w.emit(events.WindowEventType(events.Common.ThemeChanged)) } }Then apply
requestedThemeduring window initialization inrun().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/theme_window.go` around lines 33 - 51, SetTheme currently drops requests when w.impl is nil; add a requestedTheme field on WebviewWindow, update SetTheme to store the theme when impl==nil (and still validate via theme.Valid()), and update GetTheme to return requestedTheme when impl==nil; then in the window initialization flow (the run() method or wherever w.impl is created) apply the stored requestedTheme by calling w.impl.setTheme(requestedTheme) and then emit the ThemeChanged event (same emit used in SetTheme) so listeners see the initial theme.v3/examples/theme/WindowService.go (1)
21-27:⚠️ Potential issue | 🟡 MinorType assertion without comma-ok pattern will panic if key is missing.
Lines 22 and 30 use direct type assertion
ctx.Value(application.WindowKey).(application.Window). IfWindowKeyis not present in the context (returnsnil), the type assertion to interface typeapplication.Windowsucceeds but returnsnil. However, if the context value is a different type (notniland notWindow), this will panic.More importantly, this pattern is inconsistent with Go idioms and the past review suggestion to use the comma-ok pattern for safety.
🛠️ Proposed fix using comma-ok pattern
func (s *WindowService) SetWinTheme(ctx context.Context, theme string) { - win := ctx.Value(application.WindowKey).(application.Window) - if win == nil { + win, ok := ctx.Value(application.WindowKey).(application.Window) + if !ok || win == nil { return } - win.SetTheme((application.WinTheme(theme))) + win.SetTheme(application.WinTheme(theme)) } func (s *WindowService) GetWinTheme(ctx context.Context) string { - win := ctx.Value(application.WindowKey).(application.Window) - if win == nil { + win, ok := ctx.Value(application.WindowKey).(application.Window) + if !ok || win == nil { return "" } return win.GetTheme().String() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/theme/WindowService.go` around lines 21 - 27, The SetWinTheme function currently does a direct type assertion on ctx.Value(application.WindowKey).(application.Window) which can panic if the stored value is a different type; change this to use the comma-ok pattern: retrieve the value with v := ctx.Value(application.WindowKey), then do win, ok := v.(application.Window) and return early if !ok or win == nil before calling win.SetTheme(application.WinTheme(theme)); ensure you apply the same safe comma-ok check anywhere else in this file that uses the same assertion.
🧹 Nitpick comments (4)
v3/pkg/application/webview_window_options_test.go (1)
85-98: Tests updated for WinTheme constants.The test correctly verifies the new
WinThemeconstant values. Minor: the error messages reference inconsistent names (e.g., "WinThemeApplication" vs the actual constant name "WinAppDefault").🧹 Optional: Align error messages with constant names
func TestWinTheme_Constants(t *testing.T) { if WinAppDefault != "application" { - t.Error("WinThemeApplication should be application") + t.Error("WinAppDefault should be 'application'") } if WinDark != "dark" { - t.Error("WinThemeDark should be dark") + t.Error("WinDark should be 'dark'") } if WinLight != "light" { - t.Error("WinThemeLight should be light") + t.Error("WinLight should be 'light'") } if WinSystemDefault != "system" { - t.Error("WinThemeSystem should be system") + t.Error("WinSystemDefault should be 'system'") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/webview_window_options_test.go` around lines 85 - 98, Update the test assertions in TestWinTheme_Constants so their error messages reference the actual constant names (WinAppDefault, WinDark, WinLight, WinSystemDefault) instead of the inconsistent labels like "WinThemeApplication" etc.; modify the t.Error strings to e.g. "WinAppDefault should be 'application'", "WinDark should be 'dark'", "WinLight should be 'light'", and "WinSystemDefault should be 'system'" to match the constants being tested.v3/examples/theme/WindowService.go (1)
26-26: Remove redundant parentheses.Line 26 has double parentheses:
win.SetTheme((application.WinTheme(theme)))- the outer pair is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/examples/theme/WindowService.go` at line 26, Remove the redundant outer parentheses in the call to win.SetTheme: change the invocation that currently uses win.SetTheme((application.WinTheme(theme))) to call win.SetTheme with the conversion result directly using application.WinTheme(theme) (referencing the win.SetTheme method and the application.WinTheme conversion of the local variable theme).v3/pkg/application/theme_application.go (1)
36-58: Consider returning an error or logging when an invalid theme is provided.
SetThemesilently ignores invalid themes (lines 37-39), which could make debugging difficult when developers accidentally pass an invalid string. Consider either returning an error or logging a warning.♻️ Proposed enhancement
func (a *App) SetTheme(theme AppTheme) { if !theme.Valid() { + globalApplication.error("Invalid theme provided: %s", theme) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/theme_application.go` around lines 36 - 58, SetTheme currently silently ignores invalid themes; change its signature from SetTheme(theme AppTheme) to SetTheme(theme AppTheme) error and return a descriptive error when theme.Valid() is false (e.g., fmt.Errorf("invalid theme: %v", theme)), keeping the existing behavior for valid themes (update a.theme, call a.impl.setTheme and a.Event.Emit as before); also update all callers to handle the returned error. If you cannot change the API, instead add a warning log before the early return (use the application's logger or a.Event) that includes the invalid theme value to aid debugging.v3/pkg/application/theme_webview_window_darwin.go (1)
48-58: Silently discarding errors may hide issues.Lines 50 and 55 discard the error from
getOppositeMacAppearance. If an unknown appearance is encountered, the code proceeds with the fallback dark appearance without any logging, making debugging difficult.♻️ Proposed fix to log errors
case AppDark: if !currentDark { - appr, _ := getOppositeMacAppearance(currentAppearance) + appr, err := getOppositeMacAppearance(currentAppearance) + if err != nil { + globalApplication.error("syncTheme: %v", err) + } w.setAppearanceByName(appr) } case AppLight: if currentDark { - appr, _ := getOppositeMacAppearance(currentAppearance) + appr, err := getOppositeMacAppearance(currentAppearance) + if err != nil { + globalApplication.error("syncTheme: %v", err) + } w.setAppearanceByName(appr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/theme_webview_window_darwin.go` around lines 48 - 58, The code silently ignores errors returned from getOppositeMacAppearance in the AppDark/AppLight branches; modify the branches that call getOppositeMacAppearance (referencing currentAppearance and setAppearanceByName) to capture the error (appr, err := getOppositeMacAppearance(...)), check if err != nil, and log the error before continuing (e.g., use an existing window logger like w.logger.Errorf or fallback to log.Printf) so failures to resolve an appearance are recorded while still applying the fallback appearance when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/examples/theme/README.md`:
- Around line 56-57: The README's theme mapping text uses incorrect window
intent names; update the second bullet to reference the actual intent constants
used in code: replace mentions of WinThemeSystem, WinThemeDark, WinThemeLight
with WinSystemDefault, WinDark, WinLight (keep the first bullet's AppTheme names
as-is if they match code). Edit the README line that describes the behavior when
followApplicationTheme is false so it refers to WinSystemDefault, WinDark, and
WinLight to match theme_window.go and the resolver that maps those intents to
Theme (0/1/2).
- Around line 82-84: Update the README lines that reference theme constants to
use the actual enum/constant names used in code: replace WinThemeApplication
with WinAppDefault, WinThemeSystem with WinSystemDefault, WinThemeDark with
WinDark, and WinThemeLight with WinLight so the documentation matches the
implementation (references: WinAppDefault, WinSystemDefault, WinDark, WinLight).
- Around line 24-29: Update the README to use the actual enum/constant names
used in code: replace documented `WinThemeApplication` with `WinAppDefault`,
`WinThemeSystem` with `WinSystemDefault`, `WinThemeDark` with `WinDark`, and
`WinThemeLight` with `WinLight`; ensure the documentation sentence referencing
window methods still points to `window.GetTheme()` and `window.SetTheme()` and
confirm the list items match the constants used by the Window options parsing
logic (search for symbols WinAppDefault, WinSystemDefault, WinDark, WinLight to
verify).
In `@v3/pkg/application/theme_webview_window_darwin.go`:
- Around line 9-17: getOppositeMacAppearance currently only handles
"NSAppearanceNameDarkAqua" and returns a dark appearance plus an error for every
other input; update this to explicitly map all common macOS appearance constants
(e.g., "NSAppearanceNameAqua" ↔ "NSAppearanceNameDarkAqua",
"NSAppearanceNameVibrantLight" ↔ "NSAppearanceNameVibrantDark", and any known
high‑contrast/accessible variants to their correct opposites) using a switch or
map in getOppositeMacAppearance (returning the correct MacAppearanceType and nil
error for known variants), and only return an error (with a sensible default)
for truly unknown names so accessibility and opposite-selection work correctly.
In `@v3/pkg/application/theme_webview_window_windows.go`:
- Around line 32-60: The light-theme branches in windowsWebviewWindow.syncTheme
and setTheme need to call w32.AllowDarkModeForWindow(w.hwnd, false) as well as
w.updateTheme(false); update the AppLight branch in syncTheme (and the
corresponding light-path in setTheme) to set w.theme = light, call
AllowDarkModeForWindow with false on w.hwnd, then call w.updateTheme(false) to
mirror the AppDark behavior and ensure consistent Windows API state.
---
Duplicate comments:
In `@v3/examples/theme/WindowService.go`:
- Around line 21-27: The SetWinTheme function currently does a direct type
assertion on ctx.Value(application.WindowKey).(application.Window) which can
panic if the stored value is a different type; change this to use the comma-ok
pattern: retrieve the value with v := ctx.Value(application.WindowKey), then do
win, ok := v.(application.Window) and return early if !ok or win == nil before
calling win.SetTheme(application.WinTheme(theme)); ensure you apply the same
safe comma-ok check anywhere else in this file that uses the same assertion.
In `@v3/pkg/application/theme_window.go`:
- Around line 33-51: SetTheme currently drops requests when w.impl is nil; add a
requestedTheme field on WebviewWindow, update SetTheme to store the theme when
impl==nil (and still validate via theme.Valid()), and update GetTheme to return
requestedTheme when impl==nil; then in the window initialization flow (the run()
method or wherever w.impl is created) apply the stored requestedTheme by calling
w.impl.setTheme(requestedTheme) and then emit the ThemeChanged event (same emit
used in SetTheme) so listeners see the initial theme.
---
Nitpick comments:
In `@v3/examples/theme/WindowService.go`:
- Line 26: Remove the redundant outer parentheses in the call to win.SetTheme:
change the invocation that currently uses
win.SetTheme((application.WinTheme(theme))) to call win.SetTheme with the
conversion result directly using application.WinTheme(theme) (referencing the
win.SetTheme method and the application.WinTheme conversion of the local
variable theme).
In `@v3/pkg/application/theme_application.go`:
- Around line 36-58: SetTheme currently silently ignores invalid themes; change
its signature from SetTheme(theme AppTheme) to SetTheme(theme AppTheme) error
and return a descriptive error when theme.Valid() is false (e.g.,
fmt.Errorf("invalid theme: %v", theme)), keeping the existing behavior for valid
themes (update a.theme, call a.impl.setTheme and a.Event.Emit as before); also
update all callers to handle the returned error. If you cannot change the API,
instead add a warning log before the early return (use the application's logger
or a.Event) that includes the invalid theme value to aid debugging.
In `@v3/pkg/application/theme_webview_window_darwin.go`:
- Around line 48-58: The code silently ignores errors returned from
getOppositeMacAppearance in the AppDark/AppLight branches; modify the branches
that call getOppositeMacAppearance (referencing currentAppearance and
setAppearanceByName) to capture the error (appr, err :=
getOppositeMacAppearance(...)), check if err != nil, and log the error before
continuing (e.g., use an existing window logger like w.logger.Errorf or fallback
to log.Printf) so failures to resolve an appearance are recorded while still
applying the fallback appearance when appropriate.
In `@v3/pkg/application/webview_window_options_test.go`:
- Around line 85-98: Update the test assertions in TestWinTheme_Constants so
their error messages reference the actual constant names (WinAppDefault,
WinDark, WinLight, WinSystemDefault) instead of the inconsistent labels like
"WinThemeApplication" etc.; modify the t.Error strings to e.g. "WinAppDefault
should be 'application'", "WinDark should be 'dark'", "WinLight should be
'light'", and "WinSystemDefault should be 'system'" to match the constants being
tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b8b13a6-febd-4405-b497-7c844465cfe1
📒 Files selected for processing (18)
v3/examples/theme/README.mdv3/examples/theme/WindowService.gov3/examples/theme/assets/style.cssv3/examples/theme/main.gov3/pkg/application/application.gov3/pkg/application/application_darwin.gov3/pkg/application/application_options.gov3/pkg/application/application_server.gov3/pkg/application/theme_application.gov3/pkg/application/theme_webview_window_darwin.gov3/pkg/application/theme_webview_window_windows.gov3/pkg/application/theme_window.gov3/pkg/application/theme_window_windows.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_options.gov3/pkg/application/webview_window_options_test.gov3/pkg/application/webview_window_windows.gov3/pkg/application/window.go
🚧 Files skipped from review as they are similar to previous changes (5)
- v3/pkg/application/theme_window_windows.go
- v3/pkg/application/window.go
- v3/pkg/application/application_options.go
- v3/examples/theme/main.go
- v3/pkg/application/application_darwin.go
Comment on lines
+24
to
+29
| - **Available Values:** | ||
| - `WinThemeApplication` - Inherit the `AppTheme` resolving logic (Default). | ||
| - `WinThemeSystem` - Force the window to follow the OS theme, completely ignoring the `AppTheme`. | ||
| - `WinThemeDark` - Force the window into Dark mode. | ||
| - `WinThemeLight` - Force the window into Light mode. | ||
| - **Behavior:** Configured via window options at creation, and used via `window.GetTheme()` and `window.SetTheme()` methods. This state is purely intent-based and does not directly call OS bindings. |
Contributor
There was a problem hiding this comment.
Documentation references incorrect constant names.
The documented constant names don't match the actual code:
WinThemeApplication→ actual:WinAppDefaultWinThemeSystem→ actual:WinSystemDefaultWinThemeDark→ actual:WinDarkWinThemeLight→ actual:WinLight
This will confuse developers trying to use the API.
📝 Proposed fix
- **Available Values:**
- - `WinThemeApplication` - Inherit the `AppTheme` resolving logic (Default).
- - `WinThemeSystem` - Force the window to follow the OS theme, completely ignoring the `AppTheme`.
- - `WinThemeDark` - Force the window into Dark mode.
- - `WinThemeLight` - Force the window into Light mode.
+ - `WinAppDefault` - Inherit the `AppTheme` resolving logic (Default).
+ - `WinSystemDefault` - Force the window to follow the OS theme, ignoring the `AppTheme`.
+ - `WinDark` - Force the window into Dark mode.
+ - `WinLight` - Force the window into Light mode.🧰 Tools
🪛 LanguageTool
[style] ~26-~26: Consider using a different adverb to strengthen your wording.
Context: ...orce the window to follow the OS theme, completely ignoring the AppTheme. - `WinThemeD...
(COMPLETELY_ENTIRELY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/examples/theme/README.md` around lines 24 - 29, Update the README to use
the actual enum/constant names used in code: replace documented
`WinThemeApplication` with `WinAppDefault`, `WinThemeSystem` with
`WinSystemDefault`, `WinThemeDark` with `WinDark`, and `WinThemeLight` with
`WinLight`; ensure the documentation sentence referencing window methods still
points to `window.GetTheme()` and `window.SetTheme()` and confirm the list items
match the constants used by the Window options parsing logic (search for symbols
WinAppDefault, WinSystemDefault, WinDark, WinLight to verify).
Comment on lines
+56
to
+57
| 1. **Check the Flag:** If `followApplicationTheme` is `true`, the window looks at the global application's `AppTheme` intent (`AppSystemDefault`, `AppDark`, `AppLight`) and maps that intent to the resolved integer `Theme` (`0`, `1`, or `2`). | ||
| 2. **Check the Window Intent:** If `followApplicationTheme` is `false`, the window checks its explicit `WinTheme` intent (`WinThemeSystem`, `WinThemeDark`, or `WinThemeLight`) and maps that to the resolved `Theme`. |
Contributor
There was a problem hiding this comment.
Documentation inconsistency with code.
Line 57 references WinThemeSystem, WinThemeDark, WinThemeLight which should be WinSystemDefault, WinDark, WinLight per the actual code in theme_window.go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/examples/theme/README.md` around lines 56 - 57, The README's theme mapping
text uses incorrect window intent names; update the second bullet to reference
the actual intent constants used in code: replace mentions of WinThemeSystem,
WinThemeDark, WinThemeLight with WinSystemDefault, WinDark, WinLight (keep the
first bullet's AppTheme names as-is if they match code). Edit the README line
that describes the behavior when followApplicationTheme is false so it refers to
WinSystemDefault, WinDark, and WinLight to match theme_window.go and the
resolver that maps those intents to Theme (0/1/2).
Comment on lines
+82
to
+84
| 1. If `followApplicationTheme` is `true`, immediately return `WinThemeApplication`. | ||
| 2. If it is `false`, we check the window's explicit macOS appearance. If it represents an empty string (`""`), it means we are following the system, returning `WinThemeSystem`. | ||
| 3. If the appearance is explicitly populated, we check if that specific macOS Appearance is a dark variant or light variant, and return `WinThemeDark` or `WinThemeLight` accordingly. |
Contributor
There was a problem hiding this comment.
Documentation uses incorrect constant names.
Lines 82-84 reference WinThemeApplication, WinThemeSystem, WinThemeDark, WinThemeLight - these should match the actual code constants: WinAppDefault, WinSystemDefault, WinDark, WinLight.
🧰 Tools
🪛 LanguageTool
[style] ~84-~84: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... system, returning WinThemeSystem. 3. If the appearance is explicitly populated,...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/examples/theme/README.md` around lines 82 - 84, Update the README lines
that reference theme constants to use the actual enum/constant names used in
code: replace WinThemeApplication with WinAppDefault, WinThemeSystem with
WinSystemDefault, WinThemeDark with WinDark, and WinThemeLight with WinLight so
the documentation matches the implementation (references: WinAppDefault,
WinSystemDefault, WinDark, WinLight).
Comment on lines
+9
to
+17
| func getOppositeMacAppearance(name string) (MacAppearanceType, error) { | ||
| if name == "NSAppearanceNameDarkAqua" { | ||
| return "NSAppearanceNameAqua", nil | ||
| } | ||
|
|
||
| // If opposite appearance doesnt match then send the default Dark Appearance | ||
| err := fmt.Errorf("unknown appearance name: %s", name) | ||
| return "NSAppearanceNameDarkAqua", err | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete appearance mapping may cause unexpected behavior.
getOppositeMacAppearance only maps NSAppearanceNameDarkAqua → NSAppearanceNameAqua. For any other appearance (including light variants like NSAppearanceNameAqua, NSAppearanceNameVibrantLight, or high-contrast variants), it returns NSAppearanceNameDarkAqua with an error. This means:
- Calling this with a light appearance returns dark (incorrect opposite)
- High-contrast appearances lose their accessibility semantics
🛠️ Proposed fix to handle all appearance variants
func getOppositeMacAppearance(name string) (MacAppearanceType, error) {
- if name == "NSAppearanceNameDarkAqua" {
- return "NSAppearanceNameAqua", nil
+ switch name {
+ case "NSAppearanceNameDarkAqua":
+ return NSAppearanceNameAqua, nil
+ case "NSAppearanceNameAqua":
+ return NSAppearanceNameDarkAqua, nil
+ case "NSAppearanceNameVibrantDark":
+ return NSAppearanceNameVibrantLight, nil
+ case "NSAppearanceNameVibrantLight":
+ return NSAppearanceNameVibrantDark, nil
+ case "NSAppearanceNameAccessibilityHighContrastDarkAqua":
+ return "NSAppearanceNameAccessibilityHighContrastAqua", nil
+ case "NSAppearanceNameAccessibilityHighContrastAqua":
+ return "NSAppearanceNameAccessibilityHighContrastDarkAqua", nil
+ default:
+ return NSAppearanceNameDarkAqua, fmt.Errorf("unknown appearance name: %s", name)
}
-
- // If opposite appearance doesnt match then send the default Dark Appearance
- err := fmt.Errorf("unknown appearance name: %s", name)
- return "NSAppearanceNameDarkAqua", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func getOppositeMacAppearance(name string) (MacAppearanceType, error) { | |
| if name == "NSAppearanceNameDarkAqua" { | |
| return "NSAppearanceNameAqua", nil | |
| } | |
| // If opposite appearance doesnt match then send the default Dark Appearance | |
| err := fmt.Errorf("unknown appearance name: %s", name) | |
| return "NSAppearanceNameDarkAqua", err | |
| } | |
| func getOppositeMacAppearance(name string) (MacAppearanceType, error) { | |
| switch name { | |
| case "NSAppearanceNameDarkAqua": | |
| return "NSAppearanceNameAqua", nil | |
| case "NSAppearanceNameAqua": | |
| return "NSAppearanceNameDarkAqua", nil | |
| case "NSAppearanceNameVibrantDark": | |
| return "NSAppearanceNameVibrantLight", nil | |
| case "NSAppearanceNameVibrantLight": | |
| return "NSAppearanceNameVibrantDark", nil | |
| case "NSAppearanceNameAccessibilityHighContrastDarkAqua": | |
| return "NSAppearanceNameAccessibilityHighContrastAqua", nil | |
| case "NSAppearanceNameAccessibilityHighContrastAqua": | |
| return "NSAppearanceNameAccessibilityHighContrastDarkAqua", nil | |
| default: | |
| return "NSAppearanceNameDarkAqua", fmt.Errorf("unknown appearance name: %s", name) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/application/theme_webview_window_darwin.go` around lines 9 - 17,
getOppositeMacAppearance currently only handles "NSAppearanceNameDarkAqua" and
returns a dark appearance plus an error for every other input; update this to
explicitly map all common macOS appearance constants (e.g.,
"NSAppearanceNameAqua" ↔ "NSAppearanceNameDarkAqua",
"NSAppearanceNameVibrantLight" ↔ "NSAppearanceNameVibrantDark", and any known
high‑contrast/accessible variants to their correct opposites) using a switch or
map in getOppositeMacAppearance (returning the correct MacAppearanceType and nil
error for known variants), and only return an error (with a sensible default)
for truly unknown names so accessibility and opposite-selection work correctly.
Comment on lines
+32
to
+60
| // syncTheme synchronizes the window's appearance with the application-wide theme, | ||
| // assuming the window is configured to follow the application theme. | ||
| // Theme updates are expected to run on the UI thread. | ||
| // SystemThemeChanged events dispatch via InvokeAsync, ensuring | ||
| // that window theme state is mutated from a single thread. | ||
| // But if required, Mutex can be added to make sure w.theme does not | ||
| // cause any Race condition. | ||
| func (w *windowsWebviewWindow) syncTheme() { | ||
| if !w.parent.followApplicationTheme { | ||
| return | ||
| } | ||
|
|
||
| switch globalApplication.theme { | ||
| case AppSystemDefault: | ||
| w.theme = systemDefault | ||
| w.updateTheme(w32.IsCurrentlyDarkMode()) | ||
| case AppDark: | ||
| if w.theme != dark { | ||
| w.theme = dark | ||
| w32.AllowDarkModeForWindow(w.hwnd, true) | ||
| w.updateTheme(true) | ||
| } | ||
| case AppLight: | ||
| if w.theme != light { | ||
| w.theme = light | ||
| w.updateTheme(false) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if AllowDarkModeForWindow needs to be called with false for light mode
# by examining how updateTheme handles the dark mode flag
rg "AllowDarkModeForWindow" v3/pkg/application/ -B 2 -A 2
# Also check what updateTheme does
rg "func.*updateTheme" v3/pkg/application/webview_window_windows.go -A 20Repository: wailsapp/wails
Length of output: 1351
🏁 Script executed:
#!/bin/bash
# Search for all AllowDarkModeForWindow calls to see the full pattern
rg "AllowDarkModeForWindow" v3/ -B 3 -A 3
# Check for SetTheme calls and understand the relationship
rg "SetTheme" v3/pkg/application/ -B 2 -A 2
# Look for any comments or documentation about dark mode handling
rg "dark.*mode|Dark.*Mode" v3/pkg/application/theme_webview_window_windows.go -B 1 -A 1Repository: wailsapp/wails
Length of output: 4276
🏁 Script executed:
#!/bin/bash
# Get the complete SetTheme function implementation
rg "func.*SetTheme" v3/pkg/w32/theme.go -A 30Repository: wailsapp/wails
Length of output: 1339
🏁 Script executed:
#!/bin/bash
# Search for RefreshTheme and other theme-related functions
rg "RefreshTheme|updateTheme|setTheme" v3/pkg/w32/theme.go -B 2 -A 15
# Also check if AllowDarkModeForWindow is called anywhere else
rg "AllowDarkModeForWindow\(" v3/pkg/w32/ -B 3 -A 3Repository: wailsapp/wails
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Find what function contains the AllowDarkModeForWindow call with useDarkMode
rg "Set dark mode for the window" v3/pkg/w32/theme.go -B 30 | head -40Repository: wailsapp/wails
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Get the complete SetMenuTheme function
rg "func SetMenuTheme" v3/pkg/w32/theme.go -A 40Repository: wailsapp/wails
Length of output: 1086
🏁 Script executed:
#!/bin/bash
# Check what updateTheme calls - specifically if it calls SetMenuTheme
rg "func.*updateTheme" v3/pkg/application/webview_window_windows.go -A 40Repository: wailsapp/wails
Length of output: 1086
🏁 Script executed:
#!/bin/bash
# Search for any issues, comments, or test cases about light mode theme
rg "light.*theme|theme.*light" v3/pkg/application/theme_webview_window_windows.go -i -B 2 -A 2
# Check if there are any race condition or synchronization issues mentioned
rg "Race condition|race|mutex|Mutex" v3/pkg/application/theme_webview_window_windows.go -i
# Look for related code in the setTheme method
rg "func.*setTheme" v3/pkg/application/theme_webview_window_windows.go -A 30Repository: wailsapp/wails
Length of output: 1072
Add AllowDarkModeForWindow(w.hwnd, false) when switching to light theme.
In both syncTheme() and setTheme(), the dark mode path explicitly calls AllowDarkModeForWindow(w.hwnd, true), but the light mode path only calls updateTheme(false) without a corresponding AllowDarkModeForWindow(w.hwnd, false) call. This asymmetry should be corrected by explicitly calling AllowDarkModeForWindow(w.hwnd, false) when switching to light mode to ensure consistent handling of the Windows API across both theme states.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/application/theme_webview_window_windows.go` around lines 32 - 60, The
light-theme branches in windowsWebviewWindow.syncTheme and setTheme need to call
w32.AllowDarkModeForWindow(w.hwnd, false) as well as w.updateTheme(false);
update the AppLight branch in syncTheme (and the corresponding light-path in
setTheme) to set w.theme = light, call AllowDarkModeForWindow with false on
w.hwnd, then call w.updateTheme(false) to mirror the AppDark behavior and ensure
consistent Windows API state.
There was a problem hiding this comment.
Pull request overview
Introduces a foundational theming system for Wails v3, separating application-wide theme intent from per-window theme intent and platform-specific resolution (Windows explicit OS API application; macOS NSAppearance-based behavior), along with a runnable example app.
Changes:
- Added public
AppTheme(application) andWinTheme(window) enums plusApp/Windowtheme getters/setters. - Implemented Windows theme resolution/application and macOS appearance inference/synchronization; added per-window “follow application theme” behavior.
- Added a
v3/examples/themedemo showcasing theme switching and events.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/pkg/application/window.go | Adds theme methods to the public Window interface. |
| v3/pkg/application/webview_window_windows.go | Applies resolved theme at window creation and reacts to OS theme changes. |
| v3/pkg/application/webview_window_options_test.go | Updates tests for new WinTheme string constants/defaults. |
| v3/pkg/application/webview_window_options.go | Replaces Windows Theme int with WinTheme and updates docs. |
| v3/pkg/application/webview_window_darwin.go | Adds C/Go helpers and initialization logic for macOS appearance theme handling. |
| v3/pkg/application/webview_window.go | Extends internal window impl interface; adds followApplicationTheme state. |
| v3/pkg/application/theme_window_windows.go | Adds internal resolved-theme enum (Windows mapping target). |
| v3/pkg/application/theme_window.go | Implements WebviewWindow Get/Set theme APIs and validation. |
| v3/pkg/application/theme_webview_window_windows.go | Windows theme resolution + sync/set/get implementation. |
| v3/pkg/application/theme_webview_window_darwin.go | macOS theme sync/set/get implementation using appearance inference. |
| v3/pkg/application/theme_application.go | Adds AppTheme and App Get/Set theme APIs + event emission. |
| v3/pkg/application/events_common_ios.go | Formatting cleanup for iOS common event forwarding. |
| v3/pkg/application/application_windows.go | Implements app-level theme propagation across open windows (Windows). |
| v3/pkg/application/application_server.go | Adds server stubs and formatting refactor for server implementations. |
| v3/pkg/application/application_options.go | Adds Options.Theme and formatting-only changes in iOS options section. |
| v3/pkg/application/application_linux_gtk4.go | Adds stub setTheme for GTK4 Linux backend. |
| v3/pkg/application/application_linux.go | Adds stub setTheme for Linux backend. |
| v3/pkg/application/application_ios.go | Adds stub setTheme for iOS and minor formatting change. |
| v3/pkg/application/application_darwin.go | Implements app-level theme propagation across open windows (macOS). |
| v3/pkg/application/application_android_nocgo.go | Adds stub setTheme for Android (no-cgo). |
| v3/pkg/application/application_android.go | Adds stub setTheme for Android. |
| v3/pkg/application/application.go | Stores initial app theme in App and extends app impl interface with setTheme. |
| v3/examples/theme/main.go | New demo app wiring app + multiple windows with different theme intents. |
| v3/examples/theme/assets/style.css | Demo styling for theme showcase UI. |
| v3/examples/theme/assets/index.html | Demo UI for toggling app/window themes. |
| v3/examples/theme/assets/app.js | Demo JS bindings + event listeners for theme updates. |
| v3/examples/theme/WindowService.go | Demo service exposing theme getters/setters to frontend. |
| v3/examples/theme/README.md | Documents intent vs resolution architecture and usage. |
Comments suppressed due to low confidence (1)
v3/pkg/application/application_server.go:573
serverWebviewWindowdoesn’t implement the newly requiredgetTheme() WinTheme/setTheme(theme WinTheme)methods onwebviewWindowImpl(seewebview_window.go), soserverbuilds won’t compile. Add no-op implementations that returnWinAppDefaultand ignoreSetTheme, or route throughparentoptions if desired.
// All webviewWindowImpl methods as no-ops for server mode
func (w *serverWebviewWindow) setTitle(title string) {}
func (w *serverWebviewWindow) setSize(width, height int) {}
func (w *serverWebviewWindow) setAlwaysOnTop(alwaysOnTop bool) {}
func (w *serverWebviewWindow) setURL(url string) {}
func (w *serverWebviewWindow) setResizable(resizable bool) {}
func (w *serverWebviewWindow) setMinSize(width, height int) {}
func (w *serverWebviewWindow) setMaxSize(width, height int) {}
func (w *serverWebviewWindow) execJS(js string) {}
func (w *serverWebviewWindow) setBackgroundColour(color RGBA) {}
func (w *serverWebviewWindow) run() {}
func (w *serverWebviewWindow) center() {}
func (w *serverWebviewWindow) size() (int, int) { return 0, 0 }
func (w *serverWebviewWindow) width() int { return 0 }
func (w *serverWebviewWindow) height() int { return 0 }
func (w *serverWebviewWindow) destroy() {}
func (w *serverWebviewWindow) reload() {}
func (w *serverWebviewWindow) forceReload() {}
func (w *serverWebviewWindow) openDevTools() {}
func (w *serverWebviewWindow) zoomReset() {}
func (w *serverWebviewWindow) zoomIn() {}
func (w *serverWebviewWindow) zoomOut() {}
func (w *serverWebviewWindow) getZoom() float64 { return 1.0 }
func (w *serverWebviewWindow) setZoom(zoom float64) {}
func (w *serverWebviewWindow) close() {}
func (w *serverWebviewWindow) zoom() {}
func (w *serverWebviewWindow) setHTML(html string) {}
func (w *serverWebviewWindow) on(eventID uint) {}
func (w *serverWebviewWindow) minimise() {}
func (w *serverWebviewWindow) unminimise() {}
func (w *serverWebviewWindow) maximise() {}
func (w *serverWebviewWindow) unmaximise() {}
func (w *serverWebviewWindow) fullscreen() {}
func (w *serverWebviewWindow) unfullscreen() {}
func (w *serverWebviewWindow) isMinimised() bool { return false }
func (w *serverWebviewWindow) isMaximised() bool { return false }
func (w *serverWebviewWindow) isFullscreen() bool { return false }
func (w *serverWebviewWindow) isNormal() bool { return true }
func (w *serverWebviewWindow) isVisible() bool { return false }
func (w *serverWebviewWindow) isFocused() bool { return false }
func (w *serverWebviewWindow) focus() {}
func (w *serverWebviewWindow) show() {}
func (w *serverWebviewWindow) hide() {}
func (w *serverWebviewWindow) getScreen() (*Screen, error) {
return nil, errors.New("screens not available in server mode")
}
func (w *serverWebviewWindow) setFrameless(frameless bool) {}
func (w *serverWebviewWindow) openContextMenu(menu *Menu, data *ContextMenuData) {}
func (w *serverWebviewWindow) nativeWindow() unsafe.Pointer { return nil }
func (w *serverWebviewWindow) startDrag() error {
return errors.New("drag not available in server mode")
}
func (w *serverWebviewWindow) startResize(border string) error {
return errors.New("resize not available in server mode")
}
func (w *serverWebviewWindow) print() error { return errors.New("print not available in server mode") }
func (w *serverWebviewWindow) setEnabled(enabled bool) {}
func (w *serverWebviewWindow) physicalBounds() Rect { return Rect{} }
func (w *serverWebviewWindow) setPhysicalBounds(bounds Rect) {}
func (w *serverWebviewWindow) bounds() Rect { return Rect{} }
func (w *serverWebviewWindow) setBounds(bounds Rect) {}
func (w *serverWebviewWindow) position() (int, int) { return 0, 0 }
func (w *serverWebviewWindow) setPosition(x int, y int) {}
func (w *serverWebviewWindow) relativePosition() (int, int) { return 0, 0 }
func (w *serverWebviewWindow) setRelativePosition(x int, y int) {}
func (w *serverWebviewWindow) flash(enabled bool) {}
func (w *serverWebviewWindow) handleKeyEvent(acceleratorString string) {}
func (w *serverWebviewWindow) getBorderSizes() *LRTB { return &LRTB{} }
func (w *serverWebviewWindow) setMinimiseButtonState(state ButtonState) {}
func (w *serverWebviewWindow) setMaximiseButtonState(state ButtonState) {}
func (w *serverWebviewWindow) setCloseButtonState(state ButtonState) {}
func (w *serverWebviewWindow) isIgnoreMouseEvents() bool { return false }
func (w *serverWebviewWindow) setIgnoreMouseEvents(ignore bool) {}
func (w *serverWebviewWindow) cut() {}
func (w *serverWebviewWindow) copy() {}
func (w *serverWebviewWindow) paste() {}
func (w *serverWebviewWindow) undo() {}
func (w *serverWebviewWindow) delete() {}
func (w *serverWebviewWindow) selectAll() {}
func (w *serverWebviewWindow) redo() {}
func (w *serverWebviewWindow) showMenuBar() {}
func (w *serverWebviewWindow) hideMenuBar() {}
func (w *serverWebviewWindow) toggleMenuBar() {}
func (w *serverWebviewWindow) setMenu(menu *Menu) {}
func (w *serverWebviewWindow) snapAssist() {}
func (w *serverWebviewWindow) setContentProtection(enabled bool) {}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+117
to
+118
| getTheme() WinTheme | ||
| setTheme(theme WinTheme) |
Comment on lines
+33
to
+50
| // GetTheme returns the current theme of the window. | ||
| func (w *WebviewWindow) GetTheme() WinTheme { | ||
| if w.impl == nil { | ||
| return WinAppDefault | ||
| } | ||
| return w.impl.getTheme() | ||
| } | ||
|
|
||
| // SetTheme sets the theme for the window. | ||
| func (w *WebviewWindow) SetTheme(theme WinTheme) { | ||
| if !theme.Valid() { | ||
| return | ||
| } | ||
| if w.impl != nil { | ||
| w.impl.setTheme(theme) | ||
| // Notify listeners of the theme change | ||
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) | ||
| } |
Comment on lines
+48
to
+49
| // Notify listeners of the theme change | ||
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) |
Comment on lines
+41
to
+51
| // SetTheme sets the theme for the window. | ||
| func (w *WebviewWindow) SetTheme(theme WinTheme) { | ||
| if !theme.Valid() { | ||
| return | ||
| } | ||
| if w.impl != nil { | ||
| w.impl.setTheme(theme) | ||
| // Notify listeners of the theme change | ||
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) | ||
| } | ||
| } |
Comment on lines
+1
to
+3
| package application | ||
|
|
||
| type theme int |
Comment on lines
+5
to
+16
| import "fmt" | ||
|
|
||
| // getOppositeMacAppearance returns the macOS appearance that represents | ||
| // the opposite light/dark variant. | ||
| func getOppositeMacAppearance(name string) (MacAppearanceType, error) { | ||
| if name == "NSAppearanceNameDarkAqua" { | ||
| return "NSAppearanceNameAqua", nil | ||
| } | ||
|
|
||
| // If opposite appearance doesnt match then send the default Dark Appearance | ||
| err := fmt.Errorf("unknown appearance name: %s", name) | ||
| return "NSAppearanceNameDarkAqua", err |
Comment on lines
+130
to
+135
| // Apply theme to all windows | ||
| m.windowMapLock.RLock() | ||
| for _, window := range m.windowMap { | ||
| window.syncTheme() | ||
| } | ||
| m.windowMapLock.RUnlock() |
Comment on lines
+526
to
+530
| // Always listen to OS theme changes but only update the theme if we are following the application theme | ||
| w.parent.onApplicationEvent(events.Windows.SystemThemeChanged, func(*ApplicationEvent) { | ||
| if w.theme != systemDefault { | ||
| return | ||
| } |
Comment on lines
+244
to
+249
| // - WinAppDefault (default): the window follows the application theme | ||
| // - WinSystemDefault: the window follows the operating system theme | ||
| // - WinDark: the window uses dark mode | ||
| // - WinLight: the window uses light mode | ||
| // If not specified, the window defaults to WinAppDefault and inherits | ||
| // the application theme. |
Comment on lines
+7
to
+29
| // resolveWindowsEffectiveTheme determines the realized Theme for the window by resolving | ||
| // application-level and window-level theme settings. It also returns whether the window follows the application theme. | ||
| func resolveWindowsEffectiveTheme(winTheme WinTheme, appTheme AppTheme) (theme, bool) { | ||
| switch winTheme { | ||
| case WinDark: | ||
| return dark, false | ||
| case WinLight: | ||
| return light, false | ||
| case WinSystemDefault: | ||
| return systemDefault, false | ||
| default: | ||
| // For WinThemeApplication and/or Unset values we default to following | ||
| switch appTheme { | ||
| case AppDark: | ||
| return dark, true | ||
| case AppLight: | ||
| return light, true | ||
| case AppSystemDefault: | ||
| return systemDefault, true | ||
| default: | ||
| return systemDefault, true | ||
| } | ||
| } |
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
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.
Description
This PR introduces the foundational application and window theme system for Wails v3 as discussed by Issue #4665. The goal of this change is to establish a clear architecture that separates theme intent (what the developer specifies) from theme resolution (what the OS ultimately receives).
The initial implementation supports:
The system introduces two public enums:
Windows additionally uses an internal resolved Theme enum (System, Dark, Light) to map intent to OS APIs.
Windows explicitly applies resolved themes, while macOS determines the effective theme using the system’s appearance resolution.
The implementation also introduces a followApplicationTheme flag to determine whether a window inherits the application theme or overrides it. This PR also includes small internal refactors to simplify platform-specific theme logic.
Note: An example demonstrating the theme system is included in
v3/examples/theme, along with a README.md explaining the architecture and usage.Fixes #4665
Type of change
Please select the option that is relevant.
How Has This Been Tested?
The implementation was built and tested on the following systems:
The implementation was tested manually with the following scenarios:
Test Configuration
Tested locally during development on:
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit