feat(v3,windows,macos): introduce application and window theme system#5458
feat(v3,windows,macos): introduce application and window theme system#5458taliesin-ai wants to merge 1 commit into
Conversation
Implements AppTheme/WinTheme APIs for Windows and macOS. Includes review fixes: compilation stubs, thread-safety, InvokeSync marshaling, AllowDarkModeForWindow symmetry, unit tests. Closes wailsapp#4665
WalkthroughIntroduces a two-tier theming system separating application-level intent (AppTheme: system/dark/light) from window-level intent (WinTheme) and platform-specific resolution. Implements GetTheme/SetTheme on both App and WebviewWindow with event emission, adds Windows theme resolution logic with mutex-protected state, implements macOS appearance synchronization via NSAppearance names, provides platform stubs for other OSes, updates window options to use WinTheme, and includes a complete working example application. ChangesTheme System Feature
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend (JS)
participant WindowService
participant App
participant Window as WebviewWindow
participant Platform as Platform Implementation
User->>Frontend: Click theme button
Frontend->>WindowService: SetAppTheme("dark")
WindowService->>App: SetTheme(AppDark)
App->>App: Validate and store theme
App->>Platform: impl.setTheme(AppDark)
Platform->>Window: syncTheme()
Window->>Platform: Update OS appearance
App->>Frontend: Emit ApplicationThemeChanged
Frontend->>WindowService: GetAppTheme()
WindowService->>App: GetTheme()
App-->>Frontend: "dark"
Frontend->>User: Display updated theme
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/examples/theme/README.md (1)
98-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the file inventory lists to avoid stale architecture docs.
The
Files Updated/Files Createdsections are incomplete versus this PR’s actual theme stack, so readers may infer the wrong change surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/examples/theme/README.md` around lines 98 - 115, The README's "Files Updated" and "Files Created" lists are incomplete and must be brought in sync with the PR; update the inventory under the headings (the lists currently referencing files like webview_window_options_test.go, application.go, etc.) to include every file actually changed or added in this theme stack (for example ensure theme_application.go, theme_window.go, theme_webview_window_darwin.go, theme_webview_window_windows.go are listed and add any missing platform-specific files such as application_*.go variants and application_server.go), and remove any entries that are no longer touched so the documentation accurately reflects the PR surface.
🧹 Nitpick comments (3)
v3/pkg/application/theme_webview_window_windows.go (2)
40-45: ⚡ Quick winInconsistent change-detection for AppSystemDefault.
The
AppDarkandAppLightcases (lines 46-67) check whetherw.themehas changed before callingw32.AllowDarkModeForWindowandw.updateTheme. However, theAppSystemDefaultcase updatesw.themeand callsw.updateThemeunconditionally, which may trigger redundant OS calls if the theme hasn't actually changed.♻️ Apply change-detection to AppSystemDefault
case AppSystemDefault: w.themeMu.Lock() + changed := w.theme != systemDefault + if changed { w.theme = systemDefault + } w.themeMu.Unlock() - w.updateTheme(w32.IsCurrentlyDarkMode()) + if changed { + w.updateTheme(w32.IsCurrentlyDarkMode()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/theme_webview_window_windows.go` around lines 40 - 45, The AppSystemDefault branch currently unconditionally sets w.theme and calls w.updateTheme, causing redundant work; modify the AppSystemDefault case to first compute the desired value (e.g., desired := systemDefault and desiredDark := w32.IsCurrentlyDarkMode()), take w.themeMu, compare desired to w.theme, and only if different assign w.theme, call w32.AllowDarkModeForWindow with desiredDark and then call w.updateTheme(desiredDark); ensure you reference the existing symbols: globalApplication.theme, AppSystemDefault, w.themeMu, w.theme, w.updateTheme, and w32.IsCurrentlyDarkMode (and w32.AllowDarkModeForWindow) when making the change.
82-98: ⚡ Quick winMissing HWND validation before OS calls.
setThemecallsw32.AllowDarkModeForWindow(w.hwnd, ...)(lines 86, 92) without verifying thatw.hwndis initialized. IfsetThemeis invoked beforewindowsWebviewWindow.run()completes window creation, this could fail silently or cause undefined behavior.🛡️ Add HWND validation
func (w *windowsWebviewWindow) setTheme(theme WinTheme) { + if w.hwnd == 0 { + // Window not yet created; theme will be applied during initialization + return + } + if theme == WinAppDefault { w.parent.followApplicationTheme = true w.syncTheme()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/theme_webview_window_windows.go` around lines 82 - 98, The OS calls w32.AllowDarkModeForWindow(w.hwnd, ...) are made without validating that w.hwnd is initialized; update the setTheme switch (cases WinDark and WinLight) to check that w.hwnd is non-zero/valid before invoking w32.AllowDarkModeForWindow, and if hwnd is zero skip the OS call but still set w.theme under w.themeMu and call w.updateTheme(...) (for WinSystemDefault, leave w.updateTheme(w32.IsCurrentlyDarkMode()) guarded by an hwnd check as well or use a safe default when hwnd is not ready); reference w.hwnd, w.themeMu, w.theme, w.updateTheme, w32.AllowDarkModeForWindow and w32.IsCurrentlyDarkMode to locate the changes.v3/pkg/application/theme_application.go (1)
36-58: ⚖️ Poor tradeoffConsider synchronization consistency with Window.SetTheme.
a.impl.setTheme(theme)at line 47 is called without synchronization, whileWebviewWindow.SetThemewraps the equivalent call inInvokeSync(theme_window.go:48-50). For consistency and safety, wrap the call:if a.impl != nil { - a.impl.setTheme(theme) + InvokeSync(func() { + a.impl.setTheme(theme) + }) // Notify listeners of the theme change🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/theme_application.go` around lines 36 - 58, The call to a.impl.setTheme(theme) in App.SetTheme is not synchronized like WebviewWindow.SetTheme which uses InvokeSync; to fix, invoke the implementation's setTheme via its InvokeSync wrapper (e.g., a.impl.InvokeSync(func(){ a.impl.setTheme(theme) })) so the theme change runs on the implementation thread consistently with WebviewWindow.SetTheme; update App.SetTheme to call InvokeSync around a.impl.setTheme(theme) and keep the subsequent a.Event.Emit("common:ApplicationThemeChanged") after the synchronized call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/examples/theme/assets/style.css`:
- Around line 36-38: There are Stylelint declaration-empty-line-before
violations caused by blank lines preceding background-color properties; edit the
CSS in style.css to remove the empty line(s) immediately before any
background-color declarations (e.g., the block where "gap: 1px;" is followed by
"background-color: var(--secondary-color);") and do the same for the other
occurrence (around lines with "background-color") so no empty line exists before
those declarations.
In `@v3/pkg/application/application_android_nocgo.go`:
- Around line 57-59: The method receiver for setTheme is declared on *App but
this Android no-cgo file’s implementations use *androidApp; change the receiver
from (a *App) to (a *androidApp) so the method binds to the androidApp type and
satisfies platform-specific dispatch; update the function signature for setTheme
to use *androidApp (keeping the AppTheme parameter) so it matches other android
implementations like methods on androidApp.
In `@v3/pkg/application/application.go`:
- Line 230: The platform implementations never receive the initial configured
theme (a.theme) at construction, so update the creation/initialization flow to
apply that theme: when New()/Run() constructs a.impl, call setTheme(a.theme) on
the platform implementation (or pass the theme into its constructor) so the
implementation sees the initial preference; specifically initialize
linuxApp.theme from parent a.theme and implement linuxApp.setTheme to store that
value (and have linuxApp.isDarkMode consult the stored theme when set), and
update other platforms' setTheme/isDarkMode stubs (Windows/macOS/iOS/Android) to
respect a.theme instead of always querying the OS. Ensure the unique entry
points affected are New(), Run(), setTheme(), linuxApp, and isDarkMode().
In `@v3/pkg/application/theme_application.go`:
- Around line 29-32: GetTheme and SetTheme access the App.theme field without
synchronization which can race; add a sync.RWMutex (e.g. themeLock) to the App
struct and wrap reads in GetTheme with themeLock.RLock()/RUnlock() and writes in
SetTheme with themeLock.Lock()/Unlock(), updating references to a.theme
accordingly (or if you intend no locking, add clear thread-safety documentation
on GetTheme/SetTheme and callers instead). Ensure you update all places that
read/write a.theme to use the new lock-protected accessors.
In `@v3/pkg/application/theme_webview_window_darwin.go`:
- Around line 49-51: The call to getOppositeMacAppearance currently ignores its
error, so if it returns an error (e.g., for accessibility variants) we still
call w.setAppearanceByName with an incorrect fallback; update both call sites
(the branch where !currentDark and the other identical block) to capture the
returned error from getOppositeMacAppearance and handle it the same way setTheme
does: if err != nil log the error (using the same logger used in setTheme) and
avoid or adjust the call to w.setAppearanceByName accordingly so the window
appearance isn't changed based on an errored value; locate the calls to
getOppositeMacAppearance and w.setAppearanceByName to apply this fix.
- Around line 9-17: The getOppositeMacAppearance function currently only maps
"NSAppearanceNameDarkAqua" to "NSAppearanceNameAqua" and treats
"NSAppearanceNameAqua" as unknown; update getOppositeMacAppearance (return type
MacAppearanceType, error) to explicitly handle both directions by returning
"NSAppearanceNameAqua" with nil error when given "NSAppearanceNameDarkAqua" and
returning "NSAppearanceNameDarkAqua" with nil error when given
"NSAppearanceNameAqua"; keep the existing fmt.Errorf and default return for any
other unknown name.
In `@v3/pkg/application/theme_webview_window_windows.go`:
- Around line 35-38: syncTheme() reads w.parent.followApplicationTheme without
synchronization which can race with writes in setTheme(); protect access by
making followApplicationTheme concurrency-safe: either add a mutex on the parent
(e.g. a field like parent.themeMu) and acquire it for reads/writes in
windowsWebviewWindow.syncTheme() and the various setTheme() implementations, or
change followApplicationTheme to an atomic.Bool and replace direct reads/writes
with atomic.Load/Store calls; update windowsWebviewWindow.syncTheme(), the
window-level setTheme(), and application-level setTheme() to use the chosen
synchronization (lock/unlock around reads/writes or atomic.LoadBool/StoreBool)
so all accesses to followApplicationTheme are safe.
In `@v3/pkg/application/theme_window.go`:
- Around line 42-54: The write to w.options.Windows.Theme in
WebviewWindow.SetTheme occurs before the synchronization via InvokeSync, causing
a potential data race; move the mutation of w.options.Windows.Theme inside the
InvokeSync closure so the option update, the call to w.impl.setTheme and the
subsequent w.emit(events.WindowEventType(events.Common.ThemeChanged)) all occur
under the same synchronized context (i.e., inside the InvokeSync(func(){ ... })
block), or alternatively document in SetTheme that callers must only call it
from a single goroutine if you intentionally keep the current ordering.
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 1409-1415: The C.CString allocation is done outside the dispatched
closure causing a lifetime/ownership ambiguity; move the C.CString creation into
the closure passed to globalApplication.dispatchOnMainThread so the UTF-8 C
string is allocated on the main thread immediately before calling
C.windowSetAppearanceTypeByName. Specifically, update setAppearanceByName so the
closure performs: create the C string from appearanceName with
C.CString(string(appearanceName)) and then call
C.windowSetAppearanceTypeByName(w.nsWindow, <that C string>) (the C function
will free it), ensuring the pointer is not captured by the outer goroutine.
In `@v3/pkg/application/webview_window_options_test.go`:
- Around line 85-98: The test TestWinTheme_Constants has mismatched error
messages; update the t.Error strings to reference the actual constant names
being asserted (WinAppDefault, WinDark, WinLight, WinSystemDefault) so failures
are clear—locate the assertions in TestWinTheme_Constants and replace the
messages like
"WinThemeApplication"/"WinThemeDark"/"WinThemeLight"/"WinThemeSystem" with
"WinAppDefault"/"WinDark"/"WinLight"/"WinSystemDefault" respectively.
---
Outside diff comments:
In `@v3/examples/theme/README.md`:
- Around line 98-115: The README's "Files Updated" and "Files Created" lists are
incomplete and must be brought in sync with the PR; update the inventory under
the headings (the lists currently referencing files like
webview_window_options_test.go, application.go, etc.) to include every file
actually changed or added in this theme stack (for example ensure
theme_application.go, theme_window.go, theme_webview_window_darwin.go,
theme_webview_window_windows.go are listed and add any missing platform-specific
files such as application_*.go variants and application_server.go), and remove
any entries that are no longer touched so the documentation accurately reflects
the PR surface.
---
Nitpick comments:
In `@v3/pkg/application/theme_application.go`:
- Around line 36-58: The call to a.impl.setTheme(theme) in App.SetTheme is not
synchronized like WebviewWindow.SetTheme which uses InvokeSync; to fix, invoke
the implementation's setTheme via its InvokeSync wrapper (e.g.,
a.impl.InvokeSync(func(){ a.impl.setTheme(theme) })) so the theme change runs on
the implementation thread consistently with WebviewWindow.SetTheme; update
App.SetTheme to call InvokeSync around a.impl.setTheme(theme) and keep the
subsequent a.Event.Emit("common:ApplicationThemeChanged") after the synchronized
call.
In `@v3/pkg/application/theme_webview_window_windows.go`:
- Around line 40-45: The AppSystemDefault branch currently unconditionally sets
w.theme and calls w.updateTheme, causing redundant work; modify the
AppSystemDefault case to first compute the desired value (e.g., desired :=
systemDefault and desiredDark := w32.IsCurrentlyDarkMode()), take w.themeMu,
compare desired to w.theme, and only if different assign w.theme, call
w32.AllowDarkModeForWindow with desiredDark and then call
w.updateTheme(desiredDark); ensure you reference the existing symbols:
globalApplication.theme, AppSystemDefault, w.themeMu, w.theme, w.updateTheme,
and w32.IsCurrentlyDarkMode (and w32.AllowDarkModeForWindow) when making the
change.
- Around line 82-98: The OS calls w32.AllowDarkModeForWindow(w.hwnd, ...) are
made without validating that w.hwnd is initialized; update the setTheme switch
(cases WinDark and WinLight) to check that w.hwnd is non-zero/valid before
invoking w32.AllowDarkModeForWindow, and if hwnd is zero skip the OS call but
still set w.theme under w.themeMu and call w.updateTheme(...) (for
WinSystemDefault, leave w.updateTheme(w32.IsCurrentlyDarkMode()) guarded by an
hwnd check as well or use a safe default when hwnd is not ready); reference
w.hwnd, w.themeMu, w.theme, w.updateTheme, w32.AllowDarkModeForWindow and
w32.IsCurrentlyDarkMode to locate the changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b42e99c6-af24-4707-a6e3-2b1c81ec818e
📒 Files selected for processing (31)
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/theme_application.gov3/pkg/application/theme_webview_window_darwin.gov3/pkg/application/theme_webview_window_windows.gov3/pkg/application/theme_webview_window_windows_test.gov3/pkg/application/theme_window.gov3/pkg/application/theme_window_windows.gov3/pkg/application/webview_window.gov3/pkg/application/webview_window_android.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_ios.gov3/pkg/application/webview_window_linux.gov3/pkg/application/webview_window_options.gov3/pkg/application/webview_window_options_test.gov3/pkg/application/webview_window_windows.gov3/pkg/application/window.go
| gap: 1px; | ||
|
|
||
| background-color: var(--secondary-color); |
There was a problem hiding this comment.
Fix Stylelint declaration-empty-line-before violations.
Remove the blank lines before background-color declarations to satisfy the configured rule.
Suggested patch
.wrapper {
display: flex;
flex-direction: row;
width: 100vw;
height: 100vh;
gap: 1px;
-
background-color: var(--secondary-color);
}
@@
.container {
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
gap: 20px;
padding: 20px;
width: 50%;
-
background-color: var(--main-color);
}Also applies to: 49-50
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 38-38: Expected no empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/examples/theme/assets/style.css` around lines 36 - 38, There are Stylelint
declaration-empty-line-before violations caused by blank lines preceding
background-color properties; edit the CSS in style.css to remove the empty
line(s) immediately before any background-color declarations (e.g., the block
where "gap: 1px;" is followed by "background-color: var(--secondary-color);")
and do the same for the other occurrence (around lines with "background-color")
so no empty line exists before those declarations.
| func (a *App) setTheme(theme AppTheme) { | ||
| // TODO: Implement theme setting for Android | ||
| } |
There was a problem hiding this comment.
Fix setTheme receiver to *androidApp for platform conformance.
Line 57 defines setTheme on *App, but this platform file’s implementation methods are on *androidApp. This likely breaks interface/dispatch consistency for Android !cgo builds.
Proposed fix
-func (a *App) setTheme(theme AppTheme) {
+func (a *androidApp) setTheme(_ AppTheme) {
// TODO: Implement theme setting for Android
}📝 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.
| func (a *App) setTheme(theme AppTheme) { | |
| // TODO: Implement theme setting for Android | |
| } | |
| func (a *androidApp) setTheme(_ AppTheme) { | |
| // TODO: Implement theme setting for Android | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/application_android_nocgo.go` around lines 57 - 59, The
method receiver for setTheme is declared on *App but this Android no-cgo file’s
implementations use *androidApp; change the receiver from (a *App) to (a
*androidApp) so the method binds to the androidApp type and satisfies
platform-specific dispatch; update the function signature for setTheme to use
*androidApp (keeping the AppTheme parameter) so it matches other android
implementations like methods on androidApp.
| isOnMainThread() bool | ||
| isDarkMode() bool | ||
| getAccentColor() string | ||
| setTheme(theme AppTheme) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find platform app constructors and check if they read the theme field
rg -n -A 20 'func newPlatformApp' --type go
# Also search for direct theme field access in platform files
rg -n 'app\.theme|a\.theme' \
--glob '*_windows.go' \
--glob '*_darwin.go' \
--glob '*_linux.go' \
--glob '*_android.go' \
--glob '*_ios.go'Repository: wailsapp/wails
Length of output: 12338
🏁 Script executed:
# Check the New() function to see where a.theme is set (lines 191-194)
sed -n '180,210p' v3/pkg/application/application.go
# Check the setTheme method (line 230)
sed -n '225,240p' v3/pkg/application/application.go
# Check the Run() function (line 573)
sed -n '565,585p' v3/pkg/application/application.goRepository: wailsapp/wails
Length of output: 1623
🏁 Script executed:
# Search for when/where theme is applied to platform implementations
rg -n 'setTheme|applyTheme|updateTheme' v3/pkg/application/ --type go
# Search for platform method calls during initialization/run
rg -n '\.impl\.' v3/pkg/application/application.go | head -30Repository: wailsapp/wails
Length of output: 3976
🏁 Script executed:
# Check if platform implementations have theme-related methods
rg -n 'func.*theme|Theme' v3/pkg/application/application_linux.go
rg -n 'func.*theme|Theme' v3/pkg/application/application_windows.go
rg -n 'func.*theme|Theme' v3/pkg/application/application_darwin.goRepository: wailsapp/wails
Length of output: 939
🏁 Script executed:
# Check the full Run() method to see if theme is applied to impl after creation
sed -n '560,700p' v3/pkg/application/application.go | head -100Repository: wailsapp/wails
Length of output: 2264
🏁 Script executed:
# Check theme_application.go to understand when setTheme is called
cat -n v3/pkg/application/theme_application.goRepository: wailsapp/wails
Length of output: 2251
🏁 Script executed:
# Search for preRun() implementation to see if theme is applied there
rg -n 'func.*preRun' v3/pkg/application/ --type go -A 15Repository: wailsapp/wails
Length of output: 1667
🏁 Script executed:
# Get more of Run() to see if setTheme is called after line 700
sed -n '700,800p' v3/pkg/application/application.goRepository: wailsapp/wails
Length of output: 2857
🏁 Script executed:
# Search all of application.go for explicit setTheme calls on impl
rg -n 'impl.setTheme|\.setTheme' v3/pkg/application/application.goRepository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Check if platform implementations read a.theme in isDarkMode or similar methods
rg -n 'isDarkMode|a\.theme|app\.theme' v3/pkg/application/application_linux.go -B 2 -A 5Repository: wailsapp/wails
Length of output: 575
🏁 Script executed:
# Check if windows apply app theme during creation
rg -n 'newWebviewWindow|createWindow' v3/pkg/application/ --type go -A 10 | grep -A 10 'theme\|Theme'Repository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Check Windows platform impl to see how it initializes theme
sed -n '417,500p' v3/pkg/application/application_windows.goRepository: wailsapp/wails
Length of output: 1430
🏁 Script executed:
# Check the linuxApp struct definition to see if it has its own theme field
rg -n 'type linuxApp struct' v3/pkg/application/ -A 15Repository: wailsapp/wails
Length of output: 2061
🏁 Script executed:
# Check if linuxApp.theme is ever initialized from parent.theme
rg -n 'a\.theme\s*=' v3/pkg/application/application_linux.go -B 2 -A 2Repository: wailsapp/wails
Length of output: 240
🏁 Script executed:
# Check how windows are created and if they apply initial theme
rg -n 'NewWebviewWindow|createWindow|newWebviewWindow' v3/pkg/application/application_linux.go -A 20 | head -60Repository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Check Windows platform to see how it handles theme
rg -n 'isDarkMode' v3/pkg/application/application_windows.go -B 2 -A 5Repository: wailsapp/wails
Length of output: 850
🏁 Script executed:
# Check macOS platform theme handling
rg -n 'isDarkMode|theme' v3/pkg/application/application_darwin.go -B 1 -A 3 | head -50Repository: wailsapp/wails
Length of output: 959
🏁 Script executed:
# Check if setTheme is supposed to initialize the platform's internal theme field
sed -n '226,240p' v3/pkg/application/application_linux.goRepository: wailsapp/wails
Length of output: 529
Apply initial theme to platform implementations during construction.
The theme is stored in a.theme at lines 191-194 (in New()), but a.impl is created later in Run() at line 573 without reading the configured theme. Platform implementations have no access to the initial theme preference at construction time:
- Linux:
linuxApphas its ownthemefield that is never initialized fromparent.theme, andsetTheme()is an unimplemented stub (TODO). TheisDarkMode()method reads a local field that defaults to empty, ignoring the configured preference. - Windows/macOS:
isDarkMode()detects the system theme directly, bypassing the configureda.themevalue. - iOS/Android:
setTheme()methods are empty stubs.
This causes the stored theme preference to be unused at startup, creating an inconsistency between the application's configured theme (a.theme) and the active platform theme.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/application.go` at line 230, The platform implementations
never receive the initial configured theme (a.theme) at construction, so update
the creation/initialization flow to apply that theme: when New()/Run()
constructs a.impl, call setTheme(a.theme) on the platform implementation (or
pass the theme into its constructor) so the implementation sees the initial
preference; specifically initialize linuxApp.theme from parent a.theme and
implement linuxApp.setTheme to store that value (and have linuxApp.isDarkMode
consult the stored theme when set), and update other platforms'
setTheme/isDarkMode stubs (Windows/macOS/iOS/Android) to respect a.theme instead
of always querying the OS. Ensure the unique entry points affected are New(),
Run(), setTheme(), linuxApp, and isDarkMode().
| // GetTheme returns the current application-level theme setting. | ||
| func (a *App) GetTheme() AppTheme { | ||
| return a.theme | ||
| } |
There was a problem hiding this comment.
Potential race on a.theme field.
GetTheme() and SetTheme() access a.theme without mutex protection. Concurrent calls from multiple goroutines could race. Compare with a.windows (application.go:362-363), which uses windowsLock.
If theme changes are meant to be rare and coordination is external, document the thread-safety expectations. Otherwise, add a mutex:
🔒 Proposed fix: Add mutex protection
In application.go, add a lock field near line 432:
theme AppTheme
themeLock sync.RWMutexThen wrap access:
func (a *App) GetTheme() AppTheme {
+ a.themeLock.RLock()
+ defer a.themeLock.RUnlock()
return a.theme
}
func (a *App) SetTheme(theme AppTheme) {
if !theme.Valid() {
return
}
+ a.themeLock.Lock()
if theme == a.theme {
+ a.themeLock.Unlock()
return
}
a.theme = theme
+ a.themeLock.Unlock()
if a.impl != nil {Also applies to: 36-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/theme_application.go` around lines 29 - 32, GetTheme and
SetTheme access the App.theme field without synchronization which can race; add
a sync.RWMutex (e.g. themeLock) to the App struct and wrap reads in GetTheme
with themeLock.RLock()/RUnlock() and writes in SetTheme with
themeLock.Lock()/Unlock(), updating references to a.theme accordingly (or if you
intend no locking, add clear thread-safety documentation on GetTheme/SetTheme
and callers instead). Ensure you update all places that read/write a.theme to
use the new lock-protected accessors.
| 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 | ||
| } |
There was a problem hiding this comment.
Handle both dark→light and light→dark appearance mapping.
The function only explicitly maps NSAppearanceNameDarkAqua to NSAppearanceNameAqua. When the input is NSAppearanceNameAqua (light), the function returns an error and defaults to dark appearance, which is not the opposite. This breaks the stated contract of returning the "opposite light/dark variant."
🔄 Proposed fix to handle both directions
func getOppositeMacAppearance(name string) (MacAppearanceType, error) {
- if name == "NSAppearanceNameDarkAqua" {
+ switch name {
+ case "NSAppearanceNameDarkAqua":
return "NSAppearanceNameAqua", nil
+ case "NSAppearanceNameAqua":
+ return "NSAppearanceNameDarkAqua", 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.
| 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 | |
| default: | |
| return "NSAppearanceNameDarkAqua", fmt.Errorf("unknown appearance name: %s", name) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/theme_webview_window_darwin.go` around lines 9 - 17, The
getOppositeMacAppearance function currently only maps "NSAppearanceNameDarkAqua"
to "NSAppearanceNameAqua" and treats "NSAppearanceNameAqua" as unknown; update
getOppositeMacAppearance (return type MacAppearanceType, error) to explicitly
handle both directions by returning "NSAppearanceNameAqua" with nil error when
given "NSAppearanceNameDarkAqua" and returning "NSAppearanceNameDarkAqua" with
nil error when given "NSAppearanceNameAqua"; keep the existing fmt.Errorf and
default return for any other unknown name.
| if !currentDark { | ||
| appr, _ := getOppositeMacAppearance(currentAppearance) | ||
| w.setAppearanceByName(appr) |
There was a problem hiding this comment.
Handle errors from getOppositeMacAppearance.
Errors returned by getOppositeMacAppearance are silently discarded. If the current appearance is unrecognized (e.g., one of the accessibility variants), the function returns an error but the window appearance is still updated with the fallback value. This could lead to unexpected theme behavior.
🛡️ Proposed fix to log errors
case AppDark:
if !currentDark {
- appr, _ := getOppositeMacAppearance(currentAppearance)
+ appr, err := getOppositeMacAppearance(currentAppearance)
+ if err != nil {
+ globalApplication.debug("Failed to get opposite appearance", "current", currentAppearance, "error", err)
+ }
w.setAppearanceByName(appr)
}
case AppLight:
if currentDark {
- appr, _ := getOppositeMacAppearance(currentAppearance)
+ appr, err := getOppositeMacAppearance(currentAppearance)
+ if err != nil {
+ globalApplication.debug("Failed to get opposite appearance", "current", currentAppearance, "error", err)
+ }
w.setAppearanceByName(appr)
}Apply the same pattern in the setTheme method at lines 82 and 87.
Also applies to: 54-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/theme_webview_window_darwin.go` around lines 49 - 51, The
call to getOppositeMacAppearance currently ignores its error, so if it returns
an error (e.g., for accessibility variants) we still call w.setAppearanceByName
with an incorrect fallback; update both call sites (the branch where
!currentDark and the other identical block) to capture the returned error from
getOppositeMacAppearance and handle it the same way setTheme does: if err != nil
log the error (using the same logger used in setTheme) and avoid or adjust the
call to w.setAppearanceByName accordingly so the window appearance isn't changed
based on an errored value; locate the calls to getOppositeMacAppearance and
w.setAppearanceByName to apply this fix.
| func (w *windowsWebviewWindow) syncTheme() { | ||
| if !w.parent.followApplicationTheme { | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all writes to followApplicationTheme to verify thread-safety model
rg -nC3 'followApplicationTheme\s*=' --type=goRepository: wailsapp/wails
Length of output: 3863
🏁 Script executed:
# Find all calls to setTheme to understand the threading model
rg 'setTheme\(' --type=go -B2 -A2Repository: wailsapp/wails
Length of output: 7010
🏁 Script executed:
# Check if followApplicationTheme has mutex protection or is part of a protected structure
rg -nC5 'followApplicationTheme' v3/pkg/application/webview_window.goRepository: wailsapp/wails
Length of output: 457
🏁 Script executed:
# Look for InvokeSync usage context
rg 'InvokeSync.*setTheme|setTheme.*InvokeSync' --type=go -B2 -A2Repository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Check the WebviewWindow struct definition to see mutex patterns
ast-grep --pattern 'type WebviewWindow struct {
$$$
}'Repository: wailsapp/wails
Length of output: 2560
🏁 Script executed:
rg 'syncTheme\(' --type=go -B3 -A1Repository: wailsapp/wails
Length of output: 2438
🏁 Script executed:
# Also check for any callbacks or goroutines that might call syncTheme indirectly
rg 'go\s+.*syncTheme|goroutine.*syncTheme' --type=goRepository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Check where followApplicationTheme is read to understand the scope of the issue
rg 'followApplicationTheme' --type=go -B1 -A1Repository: wailsapp/wails
Length of output: 3265
🏁 Script executed:
# Get the full setTheme implementation in application_windows.go
sed -n '1,150p' v3/pkg/application/application_windows.go | grep -A 20 'func (m \*windowsApp) setTheme'Repository: wailsapp/wails
Length of output: 588
🏁 Script executed:
# Check where application-level setTheme is called from
rg 'a\.impl\.setTheme|a\.setTheme' --type=go -B3 -A1Repository: wailsapp/wails
Length of output: 363
🏁 Script executed:
# Look at theme_window.go to understand InvokeSync context
rg -B5 -A5 'InvokeSync.*setTheme' v3/pkg/application/theme_window.goRepository: wailsapp/wails
Length of output: 40
Verify concurrent access to followApplicationTheme.
syncTheme() reads w.parent.followApplicationTheme (line 36) without mutex protection. This can race with writes in setTheme() (lines 75, 80). Specifically:
application_windows.go'ssetTheme()callswindow.syncTheme()withoutInvokeSync, allowing it to execute outside the UI thread- Meanwhile, window-level
setTheme()(called viaInvokeSyncintheme_window.go) writesfollowApplicationTheme - Since
followApplicationThemeis a plain bool with no mutex, concurrent reads and writes are unsafe
Protect followApplicationTheme with a mutex or use atomic.Bool.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/theme_webview_window_windows.go` around lines 35 - 38,
syncTheme() reads w.parent.followApplicationTheme without synchronization which
can race with writes in setTheme(); protect access by making
followApplicationTheme concurrency-safe: either add a mutex on the parent (e.g.
a field like parent.themeMu) and acquire it for reads/writes in
windowsWebviewWindow.syncTheme() and the various setTheme() implementations, or
change followApplicationTheme to an atomic.Bool and replace direct reads/writes
with atomic.Load/Store calls; update windowsWebviewWindow.syncTheme(), the
window-level setTheme(), and application-level setTheme() to use the chosen
synchronization (lock/unlock around reads/writes or atomic.LoadBool/StoreBool)
so all accesses to followApplicationTheme are safe.
| func (w *WebviewWindow) SetTheme(theme WinTheme) { | ||
| if !theme.Valid() { | ||
| return | ||
| } | ||
| w.options.Windows.Theme = theme | ||
| if w.impl != nil { | ||
| InvokeSync(func() { | ||
| w.impl.setTheme(theme) | ||
| }) | ||
| // Notify listeners of the theme change | ||
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Options update is outside synchronized block.
Line 46 updates w.options.Windows.Theme before entering InvokeSync at line 48. If multiple goroutines call SetTheme() concurrently, the write to w.options can race.
Move the options update inside InvokeSync or document that SetTheme must be called from a single goroutine:
♻️ Proposed fix: Move options update into sync block
func (w *WebviewWindow) SetTheme(theme WinTheme) {
if !theme.Valid() {
return
}
- w.options.Windows.Theme = theme
if w.impl != nil {
InvokeSync(func() {
+ w.options.Windows.Theme = theme
w.impl.setTheme(theme)
})
// Notify listeners of the theme change
w.emit(events.WindowEventType(events.Common.ThemeChanged))
}
}📝 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.
| func (w *WebviewWindow) SetTheme(theme WinTheme) { | |
| if !theme.Valid() { | |
| return | |
| } | |
| w.options.Windows.Theme = theme | |
| if w.impl != nil { | |
| InvokeSync(func() { | |
| w.impl.setTheme(theme) | |
| }) | |
| // Notify listeners of the theme change | |
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) | |
| } | |
| } | |
| func (w *WebviewWindow) SetTheme(theme WinTheme) { | |
| if !theme.Valid() { | |
| return | |
| } | |
| if w.impl != nil { | |
| InvokeSync(func() { | |
| w.options.Windows.Theme = theme | |
| w.impl.setTheme(theme) | |
| }) | |
| // Notify listeners of the theme change | |
| w.emit(events.WindowEventType(events.Common.ThemeChanged)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/theme_window.go` around lines 42 - 54, The write to
w.options.Windows.Theme in WebviewWindow.SetTheme occurs before the
synchronization via InvokeSync, causing a potential data race; move the mutation
of w.options.Windows.Theme inside the InvokeSync closure so the option update,
the call to w.impl.setTheme and the subsequent
w.emit(events.WindowEventType(events.Common.ThemeChanged)) all occur under the
same synchronized context (i.e., inside the InvokeSync(func(){ ... }) block), or
alternatively document in SetTheme that callers must only call it from a single
goroutine if you intentionally keep the current ordering.
| 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))) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Memory leak: C.CString allocation is not freed.
Line 1413 allocates a C string via C.CString(string(appearanceName)) and passes it to windowSetAppearanceTypeByName, which already frees the string (line 582 in the C code). However, since the Go code dispatches this call asynchronously to the main thread, the lifetime becomes ambiguous. The C function at line 575 expects to take ownership and free the string, but the async dispatch means the Go-allocated memory might not be freed correctly if the closure captures it.
🐛 Proposed fix to ensure proper memory management
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
+ // Convert to C string before dispatch to ensure proper memory lifecycle
+ cName := C.CString(string(appearanceName))
globalApplication.dispatchOnMainThread(func() {
- C.windowSetAppearanceTypeByName(w.nsWindow, C.CString(string(appearanceName)))
+ C.windowSetAppearanceTypeByName(w.nsWindow, cName)
})
}This ensures the C string is allocated in the caller's context and freed by the C function, regardless of async dispatch timing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/webview_window_darwin.go` around lines 1409 - 1415, The
C.CString allocation is done outside the dispatched closure causing a
lifetime/ownership ambiguity; move the C.CString creation into the closure
passed to globalApplication.dispatchOnMainThread so the UTF-8 C string is
allocated on the main thread immediately before calling
C.windowSetAppearanceTypeByName. Specifically, update setAppearanceByName so the
closure performs: create the C string from appearanceName with
C.CString(string(appearanceName)) and then call
C.windowSetAppearanceTypeByName(w.nsWindow, <that C string>) (the C function
will free it), ensuring the pointer is not captured by the outer goroutine.
| func TestWinTheme_Constants(t *testing.T) { | ||
| if WinAppDefault != "application" { | ||
| t.Error("WinThemeApplication should be application") | ||
| } | ||
| if Dark != 1 { | ||
| t.Error("Dark should be 1") | ||
| if WinDark != "dark" { | ||
| t.Error("WinThemeDark should be dark") | ||
| } | ||
| if Light != 2 { | ||
| t.Error("Light should be 2") | ||
| if WinLight != "light" { | ||
| t.Error("WinThemeLight should be light") | ||
| } | ||
| if WinSystemDefault != "system" { | ||
| t.Error("WinThemeSystem should be system") | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct the error messages to match constant names.
The error messages don't match the constant names being tested, which would cause confusion during test failures:
- Line 87: Tests
WinAppDefaultbut says "WinThemeApplication" - Line 90: Tests
WinDarkbut says "WinThemeDark" - Line 93: Tests
WinLightbut says "WinThemeLight" - Line 96: Tests
WinSystemDefaultbut says "WinThemeSystem"
📝 Proposed fix for error messages
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")
}
}📝 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.
| func TestWinTheme_Constants(t *testing.T) { | |
| if WinAppDefault != "application" { | |
| t.Error("WinThemeApplication should be application") | |
| } | |
| if Dark != 1 { | |
| t.Error("Dark should be 1") | |
| if WinDark != "dark" { | |
| t.Error("WinThemeDark should be dark") | |
| } | |
| if Light != 2 { | |
| t.Error("Light should be 2") | |
| if WinLight != "light" { | |
| t.Error("WinThemeLight should be light") | |
| } | |
| if WinSystemDefault != "system" { | |
| t.Error("WinThemeSystem should be system") | |
| } | |
| } | |
| func TestWinTheme_Constants(t *testing.T) { | |
| if WinAppDefault != "application" { | |
| t.Error("WinAppDefault should be application") | |
| } | |
| if WinDark != "dark" { | |
| t.Error("WinDark should be dark") | |
| } | |
| if WinLight != "light" { | |
| t.Error("WinLight should be light") | |
| } | |
| if WinSystemDefault != "system" { | |
| t.Error("WinSystemDefault should be system") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/pkg/application/webview_window_options_test.go` around lines 85 - 98, The
test TestWinTheme_Constants has mismatched error messages; update the t.Error
strings to reference the actual constant names being asserted (WinAppDefault,
WinDark, WinLight, WinSystemDefault) so failures are clear—locate the assertions
in TestWinTheme_Constants and replace the messages like
"WinThemeApplication"/"WinThemeDark"/"WinThemeLight"/"WinThemeSystem" with
"WinAppDefault"/"WinDark"/"WinLight"/"WinSystemDefault" respectively.
Adds
AppTheme/WinThemestring-enum APIs for app-wide and per-window dark/light theme control on Windows and macOS.Supersedes #5042.
Summary
AppTheme(AppSystemDefault/AppDark/AppLight) andWinTheme(WinAppDefault/WinSystemDefault/WinDark/WinLight)(*App).SetTheme(),(*App).GetTheme(),(*WebviewWindow).SetTheme(),(*WebviewWindow).GetTheme()common:ApplicationThemeChangedAllowDarkModeForWindow;sync.RWMutexfor race-free OS-event handlingsyncTheme()SetThemewrapped inInvokeSync; intent stored before nil-checkWinTheme/AppThemecombinationsTest results
CC @leaanthony
Summary by CodeRabbit
New Features