Skip to content

feat(v3,windows,macos): introduce application and window theme system#5458

Open
taliesin-ai wants to merge 1 commit into
wailsapp:masterfrom
taliesin-ai:agent/engineer-windows/f77f055f-theme-fix
Open

feat(v3,windows,macos): introduce application and window theme system#5458
taliesin-ai wants to merge 1 commit into
wailsapp:masterfrom
taliesin-ai:agent/engineer-windows/f77f055f-theme-fix

Conversation

@taliesin-ai
Copy link
Copy Markdown
Collaborator

@taliesin-ai taliesin-ai commented May 15, 2026

Adds AppTheme / WinTheme string-enum APIs for app-wide and per-window dark/light theme control on Windows and macOS.

Supersedes #5042.

Summary

  • New types: AppTheme (AppSystemDefault/AppDark/AppLight) and WinTheme (WinAppDefault/WinSystemDefault/WinDark/WinLight)
  • New methods: (*App).SetTheme(), (*App).GetTheme(), (*WebviewWindow).SetTheme(), (*WebviewWindow).GetTheme()
  • New event: common:ApplicationThemeChanged
  • Windows: DWM dark-title-bar via AllowDarkModeForWindow; sync.RWMutex for race-free OS-event handling
  • macOS: NSAppearance-based theme via syncTheme()
  • All platforms compile: stubs for Linux, Android, iOS, Server
  • SetTheme wrapped in InvokeSync; intent stored before nil-check
  • 10 table-driven unit tests for all WinTheme/AppTheme combinations

Test results

GOOS=windows GOARCH=amd64 go build ./v3/pkg/application/ -> exit 0
GOOS=windows GOARCH=amd64 go test ./v3/pkg/application/ -run TestResolveWindowsEffectiveTheme -> PASS (10/10)

CC @leaanthony

Summary by CodeRabbit

New Features

  • Application-level theming with system, dark, and light mode options
  • Per-window theme configuration with ability to follow application theme or set explicit preferences
  • Theme change event notifications for application and window-level updates
  • New example application demonstrating theme control and platform-specific behavior

Review Change Stack

Implements AppTheme/WinTheme APIs for Windows and macOS.
Includes review fixes: compilation stubs, thread-safety, InvokeSync marshaling,
AllowDarkModeForWindow symmetry, unit tests.

Closes wailsapp#4665
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Walkthrough

Introduces 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.

Changes

Theme System Feature

Layer / File(s) Summary
Theme Type System and Validation
v3/pkg/application/theme_application.go, v3/pkg/application/theme_window.go, v3/pkg/application/theme_window_windows.go
AppTheme and WinTheme types with constants (system/dark/light and application/dark/light/system defaults) and String()/Valid() validation methods. Windows-only internal theme type with unexported constants.
Application-Level Theme API and Storage
v3/pkg/application/application_options.go, v3/pkg/application/application.go, v3/pkg/application/theme_application.go
Options.Theme field, App initialization to set default AppSystemDefault theme, platformApp.setTheme() interface method, App.theme storage field, App.GetTheme() reader, App.SetTheme() with validation, state persistence, platform delegation, and ApplicationThemeChanged event emission.
Window-Level Theme API and Interface
v3/pkg/application/webview_window.go, v3/pkg/application/window.go, v3/pkg/application/theme_window.go
webviewWindowImpl interface extended with getTheme()/setTheme() theme accessors, WebviewWindow.followApplicationTheme tracking flag, Window interface with GetTheme()/SetTheme() declarations, WebviewWindow.GetTheme() delegating to implementation or options, WebviewWindow.SetTheme() with validation and ThemeChanged event emission.
Windows Theme Resolution and Synchronization
v3/pkg/application/theme_webview_window_windows.go, v3/pkg/application/webview_window_windows.go, v3/pkg/application/theme_webview_window_windows_test.go
resolveWindowsEffectiveTheme() computes realized theme from WinTheme/AppTheme and follow-app flag; syncTheme() conditionally applies global theme via w32.AllowDarkModeForWindow and updateTheme(); setTheme() toggles follow-app or applies explicit dark/light; getTheme() returns stored theme or WinAppDefault. Window initialization via resolveWindowsEffectiveTheme() and system theme change handler. Test validates resolution logic across override/default/empty scenarios.
macOS Theme Integration and Appearance Synchronization
v3/pkg/application/application_darwin.go, v3/pkg/application/theme_webview_window_darwin.go, v3/pkg/application/webview_window_darwin.go
getOppositeMacAppearance() and isMacAppearanceDark() helpers map appearance names; macosApp.setTheme() broadcasts theme to all windows; syncTheme() aligns window appearance with app theme when follow-app enabled; setTheme() configures explicit/system/app-default appearance; getTheme() returns effective theme. C helpers retrieve/clear explicit/effective appearance; Go wrappers dispatch on main thread. Window init applies theme-derived DarkAqua/Aqua or explicit appearance.
Platform Stubs and Server Mode
v3/pkg/application/application_android.go, v3/pkg/application/application_android_nocgo.go, v3/pkg/application/application_ios.go, v3/pkg/application/application_linux.go, v3/pkg/application/application_linux_gtk4.go, v3/pkg/application/application_server.go
Stub setTheme() implementations for Android (both cgo/nocgo), iOS, Linux (base and GTK4), and server mode. Server window stubs include getTheme()/setTheme() returning defaults or no-op.
Cross-Platform Window Implementations
v3/pkg/application/webview_window_android.go, v3/pkg/application/webview_window_ios.go, v3/pkg/application/webview_window_linux.go
Android/iOS/Linux webview windows implement getTheme() returning WinAppDefault and setTheme(_ WinTheme) stubs, replacing the prior Theme type.
Tests and Window Options Updates
v3/pkg/application/webview_window_options.go, v3/pkg/application/webview_window_options_test.go
WindowsWindow.Theme migrated from local Theme enum to WinTheme type with updated documentation. Old Theme type and SystemDefault/Dark/Light constants removed. New TestWinTheme_Constants validates WinAppDefault/WinDark/WinLight/WinSystemDefault values. TestWindowsWindow_Defaults updated to expect empty string default. iOS options formatting adjusted.
Example Application Demonstrating Theme Usage
v3/examples/theme/main.go, v3/examples/theme/WindowService.go, v3/examples/theme/assets/app.js, v3/examples/theme/assets/index.html, v3/examples/theme/assets/style.css
Complete example: main.go initializes Wails app with dark theme and three webview windows with mixed theme configurations; WindowService wraps app/window theme getters/setters accessible from JS; app.js wires button clicks to backend setters and subscribes to ApplicationThemeChanged/ThemeChanged events; index.html provides labeled application and window theme controls; style.css defines theme variables, dark override, and responsive flex layout.
Theme Architecture Documentation
v3/examples/theme/README.md
Documents intent vs resolution model, AppTheme/WinTheme/Windows resolved theme roles, macOS dynamic inference from NSAppearance, followApplicationTheme flag behavior, and lists modified/created files.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related issues

  • wailsapp/wails#4665: This PR directly implements the requested window theme API with GetTheme/SetTheme methods, WinTheme/AppTheme type abstractions, platform-specific sync for Windows/macOS, and theme change events.

Possibly related PRs

  • wailsapp/wails#4492: Both PRs modify windowsWebviewWindow.run() initialization path, with this PR adding theme resolution/sync and the referenced PR adjusting runtime background-color handling—overlapping edits in the same function.
  • wailsapp/wails#4471: Both PRs extend the application.Window interface, with this PR adding GetTheme()/SetTheme() while the referenced PR refactors interface call sites broadly.

Suggested labels

Enhancement, Documentation, Windows, MacOS, v3, go, runtime, Implemented in v3, size:XXL

Suggested reviewers

  • leaanthony

Poem

🐰 A theme so fine, now dark or light divine,
Windows and Mac in harmony align,
With intent and resolution, a dance so clear,
The Wails application grows ever dear!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description covers the main changes (new types, methods, events), platform implementations, and test results. However, it does not follow the provided template structure and is missing required sections like issue reference, type of change checkboxes, testing details, and checklist completion. Reformat the PR description to follow the template: add issue reference (Fixes #), select type of change, document testing approach and configuration, and complete all checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary change: introducing a theme system for both application and window levels on Windows and macOS platforms.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Update the file inventory lists to avoid stale architecture docs.

The Files Updated / Files Created sections 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 win

Inconsistent change-detection for AppSystemDefault.

The AppDark and AppLight cases (lines 46-67) check whether w.theme has changed before calling w32.AllowDarkModeForWindow and w.updateTheme. However, the AppSystemDefault case updates w.theme and calls w.updateTheme unconditionally, 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 win

Missing HWND validation before OS calls.

setTheme calls w32.AllowDarkModeForWindow(w.hwnd, ...) (lines 86, 92) without verifying that w.hwnd is initialized. If setTheme is invoked before windowsWebviewWindow.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 tradeoff

Consider synchronization consistency with Window.SetTheme.

a.impl.setTheme(theme) at line 47 is called without synchronization, while WebviewWindow.SetTheme wraps the equivalent call in InvokeSync (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

📥 Commits

Reviewing files that changed from the base of the PR and between 99617cc and 11a3dd2.

📒 Files selected for processing (31)
  • v3/examples/theme/README.md
  • v3/examples/theme/WindowService.go
  • v3/examples/theme/assets/app.js
  • v3/examples/theme/assets/index.html
  • v3/examples/theme/assets/style.css
  • v3/examples/theme/main.go
  • v3/pkg/application/application.go
  • v3/pkg/application/application_android.go
  • v3/pkg/application/application_android_nocgo.go
  • v3/pkg/application/application_darwin.go
  • v3/pkg/application/application_ios.go
  • v3/pkg/application/application_linux.go
  • v3/pkg/application/application_linux_gtk4.go
  • v3/pkg/application/application_options.go
  • v3/pkg/application/application_server.go
  • v3/pkg/application/application_windows.go
  • v3/pkg/application/theme_application.go
  • v3/pkg/application/theme_webview_window_darwin.go
  • v3/pkg/application/theme_webview_window_windows.go
  • v3/pkg/application/theme_webview_window_windows_test.go
  • v3/pkg/application/theme_window.go
  • v3/pkg/application/theme_window_windows.go
  • v3/pkg/application/webview_window.go
  • v3/pkg/application/webview_window_android.go
  • v3/pkg/application/webview_window_darwin.go
  • v3/pkg/application/webview_window_ios.go
  • v3/pkg/application/webview_window_linux.go
  • v3/pkg/application/webview_window_options.go
  • v3/pkg/application/webview_window_options_test.go
  • v3/pkg/application/webview_window_windows.go
  • v3/pkg/application/window.go

Comment on lines +36 to +38
gap: 1px;

background-color: var(--secondary-color);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Comment on lines +57 to +59
func (a *App) setTheme(theme AppTheme) {
// TODO: Implement theme setting for Android
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

Suggested change
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.go

Repository: 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 -30

Repository: 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.go

Repository: 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 -100

Repository: 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.go

Repository: 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 15

Repository: 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.go

Repository: 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.go

Repository: 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 5

Repository: 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.go

Repository: 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 15

Repository: 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 2

Repository: 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 -60

Repository: 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 5

Repository: 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 -50

Repository: 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.go

Repository: 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: linuxApp has its own theme field that is never initialized from parent.theme, and setTheme() is an unimplemented stub (TODO). The isDarkMode() method reads a local field that defaults to empty, ignoring the configured preference.
  • Windows/macOS: isDarkMode() detects the system theme directly, bypassing the configured a.theme value.
  • 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().

Comment on lines +29 to +32
// GetTheme returns the current application-level theme setting.
func (a *App) GetTheme() AppTheme {
return a.theme
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

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.RWMutex

Then 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.

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
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

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
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.

Comment on lines +49 to +51
if !currentDark {
appr, _ := getOppositeMacAppearance(currentAppearance)
w.setAppearanceByName(appr)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Comment on lines +35 to +38
func (w *windowsWebviewWindow) syncTheme() {
if !w.parent.followApplicationTheme {
return
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for all writes to followApplicationTheme to verify thread-safety model
rg -nC3 'followApplicationTheme\s*=' --type=go

Repository: wailsapp/wails

Length of output: 3863


🏁 Script executed:

# Find all calls to setTheme to understand the threading model
rg 'setTheme\(' --type=go -B2 -A2

Repository: 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.go

Repository: wailsapp/wails

Length of output: 457


🏁 Script executed:

# Look for InvokeSync usage context
rg 'InvokeSync.*setTheme|setTheme.*InvokeSync' --type=go -B2 -A2

Repository: 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 -A1

Repository: 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=go

Repository: 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 -A1

Repository: 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 -A1

Repository: 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.go

Repository: 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's setTheme() calls window.syncTheme() without InvokeSync, allowing it to execute outside the UI thread
  • Meanwhile, window-level setTheme() (called via InvokeSync in theme_window.go) writes followApplicationTheme
  • Since followApplicationTheme is 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.

Comment on lines +42 to +54
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))
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

Comment on lines +1409 to +1415
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)))
})
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

Comment on lines +85 to 98
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")
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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 WinAppDefault but says "WinThemeApplication"
  • Line 90: Tests WinDark but says "WinThemeDark"
  • Line 93: Tests WinLight but says "WinThemeLight"
  • Line 96: Tests WinSystemDefault but 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.

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant