feat: Complete App API restructuring with organized manager pattern #4359
Conversation
dosubot
bot
added
the
size:XXL
WalkthroughThis update refactors the core application API by introducing dedicated manager structs for handling windows, events, menus, dialogs, environment, clipboard, key bindings, context menus, browser, screens, and system tray functionalities. Direct methods and fields on the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Windows
participant Events
participant Menus
participant Dialogs
participant Clipboard
participant KeyBindings
participant ContextMenus
participant Browser
participant Env
participant Screens
participant SystemTray
App->>Windows: Create/Retrieve/Manage windows
App->>Events: Emit/Subscribe to custom/app events
App->>Menus: Create/Set application menus
App->>Dialogs: Show dialogs (info, question, etc.)
App->>Clipboard: Set/Get clipboard text
App->>KeyBindings: Register/Remove key bindings
App->>ContextMenus: Create/Add context menus
App->>Browser: Open URL or file in browser
App->>Env: Query environment info, dark mode, open file manager
App->>Screens: Get screen info
App->>SystemTray: Create/manage system tray icons
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Documentation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -65,7 +66,7 @@ | |||
| m.Error("Window ID not parsable:", "id", windowID, "error", err) | |||
| return nil, windowID | |||
| } | |||
| targetWindow := globalApplication.getWindowForID(uint(wID)) | |||
| targetWindow, _ := globalApplication.Windows.GetByID(uint(wID)) | |||
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the issue, we need to ensure that the value parsed from windowID is within the valid range for the uint type before performing the conversion. This can be achieved by adding an upper bound check against math.MaxUint32 (the maximum value for uint on most platforms). If the value exceeds this limit, the function should handle the error gracefully, such as by logging the issue and returning nil.
Changes required:
- Add an upper bound check for
wIDafter parsing it withstrconv.ParseUint. - Use the
mathpackage to accessmath.MaxUint32for the comparison. - Handle cases where the value is out of range by logging an error and returning
nil.
| @@ -11,2 +11,3 @@ | ||
| "sync" | ||
| "math" | ||
| ) | ||
| @@ -67,2 +68,7 @@ | ||
| return nil, windowID | ||
| } | ||
| // Check if wID is within the valid range for uint | ||
| if wID > math.MaxUint32 { | ||
| m.Error("Window ID out of range for uint:", "id", wID) | ||
| return nil, windowID | ||
| } |
Deploying wails with
|
| Latest commit: |
609a331
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0b6a703.wails.pages.dev |
| Branch Preview URL: | https://v3-alpha-chore-refactor-appl.wails.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (9)
v3/internal/commands/appimage_testfiles/main.go (1)
53-59: 🛠️ Refactor suggestionMixed window-creation APIs – finish the migration
app.Windows.New()is correctly used here, but the same file still contains multiple calls to the old helpers (app.NewWebviewWindowWithOptions) further down (e.g. lines 86-110, 115-198). Leaving a mixture of both APIs undermines the goal of a clean break and will mislead developers studying the example.- app.NewWebviewWindowWithOptions(application.WebviewWindowOptions{ ... }) + app.Windows.NewWithOptions(application.WebviewWindowOptions{ ... })Please update every remaining occurrence in this file.
v3/examples/plain/main.go (1)
27-36:⚠️ Potential issueReturn value from
NewWithOptionsis silently ignored
app.Windows.NewWithOptions(...)returns the newly-created*application.WebviewWindow(and in many helpers also anerror). Swallowing these values loses an opportunity to:
- handle window-creation errors early, and
- keep a handle to the window for subsequent API calls.
-window := app.Windows.NewWithOptions(application.WebviewWindowOptions{ ... }) +window, err := app.Windows.NewWithOptions(application.WebviewWindowOptions{ ... }) +if err != nil { + log.Fatal(err) +} +_ = window // or store it if you need it laterv3/examples/clipboard/main.go (1)
23-30:⚠️ Potential issue
app.NewMenu()was removed – use the new manager APIThe PR deprecates direct constructors; menu creation should go through the
Menusmanager:-menu := app.NewMenu() +menu := app.Menus.New()Compilation will fail otherwise.
docs/src/content/docs/learn/systray.mdx (1)
110-118: 🛠️ Refactor suggestionUpdate window-creation example to the new Manager API
The snippet still calls the removed
app.NewWebviewWindow()helper.
To compile after the API break, replace it with the Windows manager variant:-// Create a window -window := app.NewWebviewWindow() +// Create a window +window := app.Windows.New()v3/examples/systray-menu/main.go (1)
55-60:⚠️ Potential issueSwitch to
app.Menus.New()–app.NewMenu()no longer exists after the refactorThe new API groups menu functionality under the
Menusmanager. Using the old constructor will not compile.-myMenu := app.NewMenu() +myMenu := app.Menus.New()v3/examples/dialogs/main.go (1)
28-34: 🛠️ Refactor suggestionMenu construction uses deprecated helper
app.NewMenu()was removed; use the manager instead to prevent a build break:-// Create a custom menu -menu := app.NewMenu() +// Create a custom menu +menu := app.Menus.New()v3/examples/build/main.go (1)
49-56: 🛠️ Refactor suggestionStill using deprecated
app.NewMenu()helperAll other changes moved to manager accessors (
app.Menus.*). For consistency and to avoid removal-time breakage, replace withapp.Menus.New().- menu := app.NewMenu() + menu := app.Menus.New()docs/src/content/docs/guides/menus.mdx (1)
291-299:⚠️ Potential issueOut-of-date window API in example
app.NewWebviewWindowWithOptionspredates the manager refactor.
Replace with:window, _ := app.Windows.NewWithOptions(application.WebviewWindowOptions{ Title: "Custom Menu Window", Windows: application.WindowsWindow{ Menu: customMenu, }, })Ensures all examples consistently use the new
Windowsmanager.v3/examples/window/main.go (1)
30-38:⚠️ Potential issuePossible nil-pointer dereference
application.Get().Windows.Current()can returnnilif no window is focused; the subsequentPosition()call would panic.
Either reuse thecurrentWindowhelper or add a guard:win := application.Get().Windows.Current() if win == nil { return }
♻️ Duplicate comments (8)
v3/internal/generator/testcases/out_of_tree/main.go (1)
39-39: Same unused return value issue as flagged invariable_single_from_function/main.goThis line has the identical problem – see earlier comment for details and recommended fix.
v3/internal/generator/testcases/function_from_imported_package/main.go (1)
43-43: Unused return fromWindows.New()Replicates the compilation issue already pointed out; apply the
_ = …or variable assignment fix here as well.v3/internal/generator/testcases/struct_literal_multiple_files/main.go (1)
18-18: Unused return fromWindows.New()Same concern as in the previous files – update accordingly.
v3/internal/generator/testcases/struct_literal_single/main.go (1)
197-197: Unused return fromWindows.New()Duplicate of the issue already raised; please correct here as well to keep the test-cases compiling.
v3/examples/badge-custom/main.go (1)
77-83: Ditto for"set:badge"listenerApply the same pattern as above.
v3/examples/badge/main.go (2)
63-69: Store unsubscribe for"remove:badge"listenerSame recommendation as in the custom-badge example.
70-76: Store unsubscribe for"set:badge"listenerSame recommendation as above.
docs/src/content/docs/learn/screens.mdx (1)
365-368: Same shadowing issue inisValidHTTPURLhelperRename the parameter or alias the import to avoid
url.Parseambiguity, mirroring the correction proposed for the browser guide.
🧹 Nitpick comments (65)
v3/internal/generator/testcases/variable_single/main.go (1)
29-29: *Capture or explicitly discard the returned Window to avoid linter noise
Windows.New()returns a*application.Window. Most static-analysis tools (e.g.staticcheck,errcheck) will flag the ignored result.
Either capture it for later use or assign it to_with an explanatory comment.- app.Windows.New() + // Initialise the main window – discard handle if not needed. + _ = app.Windows.New() // nolint:errcheck,unusedv3/internal/generator/testcases/function_from_nested_imported_package/main.go (1)
42-42: Same note: don’t silently drop the*Windowreturned byWindows.New()Consider:
- app.Windows.New() + _ = app.Windows.New() // keep for future use or document intentional discardv3/internal/generator/testcases/embedded_interface/main.go (1)
43-43: Window handle is ignoredIf the window instance isn’t required, discard it explicitly to keep linters quiet; otherwise store it for later interaction.
- app.Windows.New() + _ = app.Windows.New()v3/internal/generator/testcases/function_single_internal/main.go (1)
53-53: Explicitly handle the value returned fromWindows.New()- app.Windows.New() + _ = app.Windows.New() // or `mainWin := …` if needed laterv3/internal/generator/testcases/struct_literal_multiple_other/main.go (1)
41-41: Avoid dropping the returned window without comment- app.Windows.New() + _ = app.Windows.New() // intentional ignorev3/internal/generator/testcases/complex_json/main.go (1)
116-116: Unused window instanceIf the created window instance is needed later (e.g., to manipulate size, title, etc.), keep a reference:
- app.Windows.New() + win := app.Windows.New() + _ = win // remove or use laterIf truly unused (e.g., tests that only need a default window), ignore this suggestion.
v3/internal/generator/testcases/enum/main.go (1)
66-66: Ensure method chaining isn’t requiredPrevious API allowed chaining (
app.NewWebviewWindow().On...).
If any following code expected the returned window for further configuration, capture it after switching to the manager API.v3/examples/events-bug/main.go (1)
31-40: API migration correct; consider naming returned windowsMigration to
app.Windows.NewWithOptions()is accurate.
Optional: assign the returned*WebviewWindowto a variable (e.g.,win1 := …) if you later need to reference it (events, state, etc.).
Purely a readability nit – ignore if not required in this demo.v3/examples/html-dnd-api/main.go (1)
26-33: Consider handling the returned window / error
app.Windows.NewWithOptions()returns aWindowand possibly anerror. In an example it’s fine to ignore them, but in production it’s safer to:
- capture the window if you later need to manipulate it;
- check the error to avoid a silent failure.
-window, _ := app.Windows.NewWithOptions(application.WebviewWindowOptions{ … }) +if wnd, err := app.Windows.NewWithOptions(application.WebviewWindowOptions{ … }); err != nil { + log.Fatal(err) +} else { + _ = wnd // use if needed +}v3/examples/raw-message/main.go (1)
29-37: Same note on ignored return values
For consistency with the new manager API, capture and check theerrorreturned byNewWithOptions. Skipping it risks masking runtime issues (e.g. invalid options on some platforms).v3/pkg/application/dialogs_linux.go (2)
4-8: Minor redundancy afterGetByIDchange
Windows.GetByID()already tells you whether a window was found via the secondboolresult. Immediately checkingwindow != nilduplicates that information. If you keep the nil-check, drop the unused bool; otherwise:-window, _ := globalApplication.Windows.GetByID(a.getCurrentWindowID()) -if window != nil { +if window, ok := globalApplication.Windows.GetByID(a.getCurrentWindowID()); ok { parent, _ = window.(*WebviewWindow).NativeWindowHandle() }
26-31: Apply the same simplification here
Replicate theokpattern inlinuxDialog.show()to keep the codebase consistent.v3/examples/window-api/main.go (1)
26-35: HandleNewWithOptionsresult
Same remark as in the other examples: capture the returned(Window, error)to surface construction failures.v3/internal/generator/testcases/map_keys/main.go (1)
299-299: Consider capturing the returned window for clarity
app.Windows.New()returns a*application.WebviewWindow.
Even if you don’t need it immediately, assigning it to a variable (or the blank identifier) documents intent and avoids the impression of an ignored result.- app.Windows.New() + _ = app.Windows.New() // returned window currently unusedv3/internal/generator/testcases/function_single/main.go (1)
32-32: Capture or intentionally discard the returned windowSimilar to the earlier test case,
app.Windows.New()returns a window pointer. Assigning it (or using_) makes the ignored value explicit:- app.Windows.New() + _ = app.Windows.New()v3/internal/commands/appimage_testfiles/main.go (1)
63-71: Minor: avoid hidden slice growthAppending to
hiddenWindowsinside theWindowClosinghook without bounds or deduplication may leak memory if users repeatedly open/close windows. Consider using amap[*application.WebviewWindow]struct{}or checkingcontainsbefore appending.v3/examples/services/main.go (1)
58-61: Window pointer is discarded – consider retaining it for later use
app.Windows.NewWithOptions(...)returns a*application.WebviewWindow.
Although not strictly required for this example, keeping the pointer enables future window customisation (title updates, event hooks, etc.) and avoids creating an unreachable object.-app.Windows.NewWithOptions(application.WebviewWindowOptions{ +mainWindow := app.Windows.NewWithOptions(application.WebviewWindowOptions{ Width: 1024, Height: 768, }) +_ = mainWindow // remove or use as neededv3/examples/dialogs-basic/main.go (1)
26-33: Mixed old/new menu creation style – consider uniform Manager APIYou’ve migrated window creation (
app.Windows.NewWithOptions) but still rely on the convenience methodapp.NewMenu().
For full consistency and discoverability, use theMenusmanager:-// Create main menu -menu := app.NewMenu() +// Create main menu +menu := app.Menus.New()(Assumes
Menus.New()is exposed; if not, keep as-is.)v3/examples/gin-example/main.go (1)
96-100: Leaking event handler – store and defer the unsubscribe function
Events.Onreturns anunsubscribecallback.
Ignoring it means the handler is never removed and will stay alive for the whole app lifetime, which is fine for this tiny sample but is a bad pattern for real-world code or long-running apps.-app.Events.On("gin-button-clicked", func(event *application.CustomEvent) { +off := app.Events.On("gin-button-clicked", func(event *application.CustomEvent) { log.Printf("Received event from frontend: %v", event.Data) }) +defer off() // unregister on shutdownv3/examples/window-menu/main.go (1)
40-58: Propagate window-creation error
NewWithOptionsmay fail; make the sample robust:- app.Windows.NewWithOptions(application.WebviewWindowOptions{ … }) + if _, err := app.Windows.NewWithOptions(application.WebviewWindowOptions{ … }); err != nil { + log.Fatal(err) + }v3/examples/badge-custom/main.go (3)
59-68: Check result ofNewWithOptionsSame concern as other examples—handle possible errors.
70-76: Unsubscribe handle ignored for"remove:badge"Store the
offfunction so the listener can be removed if the window is closed/recreated.
88-93: Potential goroutine leak – stop the ticker on app quitThe infinite loop keeps running after all windows are closed.
Listen forapp.OnQuit(or similar) or use acontext.Contextto cancel.v3/examples/badge/main.go (2)
52-61: Handle window creation errorPropagate or log the error returned by
NewWithOptions.
81-85: Gracefully stop the broadcaster goroutineConsider cancelling the loop on application shutdown to avoid stray goroutines.
v3/examples/systray-basic/main.go (1)
36-40: Prefer a symbolic key-constant instead of the raw"F12"stringHard-coding
"F12"makes the binding less self-documenting and more error-prone if the key name ever changes. If the framework already exposes key constants (e.g.application.KeyF12or similar), swap the literal for the constant to improve discoverability and avoid typos.v3/examples/contextmenus/main.go (1)
38-45: Consistent embed patterns increase readabilityEarlier examples embed all assets via
//go:embed assets/*, whereas this file uses//go:embed assets. Both are valid, but adopting one style across examples avoids confusion for newcomers skimming the repo.v3/examples/menu/main.go (3)
59-65: Cache the current window instead of callingapp.Windows.Current()twiceCalling
app.Windows.Current()twice inside the same callback costs an extra bridge hop and risks returningnilif the active window changes in-between (highly unlikely but possible when menus trigger window focus changes). Grab the pointer once for clarity and safety.- if app.Windows.Current().Resizable() { - app.Windows.Current().SetResizable(false) + w := app.Windows.Current() + if w == nil { + return // no window – nothing to resize + } + if w.Resizable() { + w.SetResizable(false) ... - app.Windows.Current().SetResizable(true) + w.SetResizable(true)
147-147: Preferapp.Menus.Apply()overSet()for clarity
Menus.Set()mutates global state; a more descriptive name such asApply(used in internal API docs) avoids confusion with assigning a menu instance. If both names exist, consider switching for consistency or deprecating one of them.
149-149: Naming nit – unused variable warning in linters
window := app.Windows.New()is only used for the fluent call; the local variable isn’t referenced afterwards except to callSetMenu. Either inline the expression or use_to silence “unused variable” warnings in stricter linters.-window := app.Windows.New().SetBackgroundColour(application.NewRGB(33, 37, 41)) -window.SetMenu(menu) +app.Windows. + New(). + SetBackgroundColour(application.NewRGB(33, 37, 41)). + SetMenu(menu)v3/examples/notifications/main.go (1)
71-72: Handle possible emit failureSame concern as above: ignore result only if you deliberately don’t care. Otherwise:
- app.Events.Emit("notification:action", result.Response) + if _, err := app.Events.Emit("notification:action", result.Response); err != nil { + app.Logger.Error("emit failed", "err", err) + }v3/examples/systray-custom/main.go (2)
19-30: Double-checkHidden:trueduring window creationCreating the systray window with
Hidden: truethen immediately callingwindow.Show()a few lines below is fine, but settingHiddentofalseand dropping the extraShow()call removes one IPC round-trip.No change required if you want the explicit call – mentioning for potential micro-optimisation.
53-53: Consider naming the tray variabletrayinstead ofsystemTrayMinor readability suggestion – the type already conveys it’s a system tray; shorter names make subsequent chained calls less verbose.
-systemTray := app.SystemTray.New() +tray := app.SystemTray.New()v3/examples/video/main.go (1)
30-39: Capture or intentionally discard the returned window
app.Windows.NewWithOptions(...)returns a*WebviewWindow.
If the example intends to manipulate the window later (show/hide, emit events, etc.), keep the handle:win := app.Windows.NewWithOptions(application.WebviewWindowOptions{ ... }) _ = win // remove if usedIf no interaction is needed, explicitly ignore the value (
_) to document the intent.v3/examples/events/main.go (1)
36-41: Busy-loop goroutine may block shutdownInside the
ApplicationStartedhandler an infinitefor { ... Sleep }loop is spawned.
Because the goroutine is untracked and never terminates,app.Quit()will hang until the program exits forcibly on some platforms. Consider replacing it with atime.Tickerand stopping the ticker in a shutdown hook.- for { - app.Events.Emit("myevent", "hello") - time.Sleep(10 * time.Second) - } + ticker := time.NewTicker(10 * time.Second) + go func() { + for range ticker.C { + app.Events.Emit("myevent", "hello") + } + }() + app.OnShutdown(func() { ticker.Stop() })v3/pkg/application/linux_cgo.go (4)
1282-1288: Prefer checking theokflag returned byGetByIDwindow, ok := globalApplication.Windows.GetByID(uint(we.id)) if !ok { return }Explicitly using the boolean avoids ambiguity if
GetByIDever returns(nil, true)or a non-nil stub.
1293-1315: Sameok-flag handling applies hereReplicate the
okcheck to avoid unnecessary type assertions when the window isn’t found.
1474-1481: Minor: early-exit pattern simplifies control flow-window, _ := globalApplication.Windows.GetByID(windowId) -if window == nil { - return C.gboolean(0) -} +window, ok := globalApplication.Windows.GetByID(windowId) +if !ok { + return C.gboolean(0) +}
1511-1518: Repeat of the ok-flag suggestionConsistency across handlers makes the code easier to reason about and marginally faster.
v3/examples/gin-service/services/gin_service.go (2)
32-35:EventDatastruct appears unusedThe new
EventDatatype isn’t referenced anywhere in the service. If it’s only here for future use, consider documenting that intent; otherwise remove it to avoid dead-code noise.
150-161: Email validation is extremely naive
strings.Contains(email, "@")will accept many invalid addresses (e.g."@","foo@"). If correctness matters, switch tonet/mail.ParseAddressor a dedicated validator package.-import "strings" +import ( + "strings" + "net/mail" +) ... - if !strings.Contains(newUser.Email, "@") { + if _, err := mail.ParseAddress(newUser.Email); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid email format"}) return }v3/pkg/application/browser_manager.go (1)
19-27: Add basic argument validation & context to errors
browser.OpenURL("")/OpenFile("")silently fails on some platforms. Guard against empty strings and provide clearer error context.-import ( - "github.com/pkg/browser" -) +import ( + "fmt" + "github.com/pkg/browser" +) ... -func (bm *BrowserManager) OpenURL(url string) error { - return browser.OpenURL(url) +func (bm *BrowserManager) OpenURL(url string) error { + if url == "" { + return fmt.Errorf("OpenURL: empty url") + } + return browser.OpenURL(url) } ... -func (bm *BrowserManager) OpenFile(path string) error { - return browser.OpenFile(path) +func (bm *BrowserManager) OpenFile(path string) error { + if path == "" { + return fmt.Errorf("OpenFile: empty path") + } + return browser.OpenFile(path) }v3/examples/badge/frontend/bindings/image/color/models.ts (1)
42-45: Consider guarding JSON parsing errors
JSON.parsewill throw on invalid input, crashing the caller. Wrapping it intry/catchand returning a typed error (orundefined) would be safer for consumer code.docs/src/content/docs/learn/dialogs.mdx (2)
17-22: Nitpick – prefixing withInfo()already returns a fully-configured dialogBecause
app.Dialogs.Info()returns a builder that already carries the correct dialog type, naming the local variabledialogis fine; however, some examples inside the docs use more explicit names (infoDialog,questionDialog, …). Sticking to one convention improves readability for people copy-pasting snippets.
114-117: Minor API consistency –AttachToWindowaccepts a window, not a managerThe example calls
dialog.AttachToWindow(app.Windows.Current())which is correct, but a short sentence clarifying that
app.Windows.Current()returns a Window (not another manager) would prevent confusion for newcomers reading the snippet in isolation.v3/pkg/application/environment_manager.go (2)
22-33:EnvironmentManager.Info()duplicates expensive OS queries
operatingsystem.Info()andem.app.platformEnvironment()might each probe the system (file I/O, shell exec). IfInfo()is called frequently you can cache the result:type EnvironmentManager struct { app *App + cachedInfo *EnvironmentInfo } ... func (em *EnvironmentManager) Info() EnvironmentInfo { - info, _ := operatingsystem.Info() - ... - return result + if em.cachedInfo != nil { + return *em.cachedInfo + } + info, _ := operatingsystem.Info() + ... + em.cachedInfo = &result + return result }This keeps the API unchanged while avoiding redundant work.
36-41: Nil-impl guard good – but document behaviour
IsDarkMode()silently returnsfalsewhenapp.implisnil. Add a short comment or godoc sentence so users know afalseresult can also mean “not initialised yet”.v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/badge/models.ts (1)
40-54: Generated helper loses type-safety for unknown keys
Object.assign(this, $$source)blindly copies all properties, so typos slip through unnoticed. For generated models you can enforce strictness:for (const key of Object.keys($$source)) { if (!(key in this)) { throw new Error(`Unknown property '${key}' supplied to Options`); } } Object.assign(this, $$source);Not critical for generated code but improves DX when developers hand-craft JSON.
docs/src/content/docs/guides/menus.mdx (1)
417-420: Context-menu creation deviates from new patternAfter the refactor, creating a context menu without immediately registering it can confuse readers.
Consider showing:contextMenu := app.ContextMenus.New() // or .New("imageMenu") app.ContextMenus.Add("imageMenu", contextMenu)Aligns with the “manager owns creation” philosophy introduced elsewhere.
docs/src/content/docs/learn/context-menu.mdx (3)
13-17: Mismatch between recommendation and code sampleThe text states “use the
Addmethod from the ContextMenus manager”, yet the snippet still instantiates the menu withapplication.NewContextMenu()instead of the new, shorter helperapp.ContextMenus.New().
Usingapp.ContextMenus.New()keeps the sample consistent with the manager-based guidance and avoids pulling in the (still valid but now secondary)applicationpackage function.-contextMenu := app.ContextMenus.Add("menu-id", application.NewContextMenu()) +contextMenu := app.ContextMenus.Add("menu-id", app.ContextMenus.New())
26-39: Prefer the manager helper to emphasise the new patternThroughout the doc you alternate between
application.NewContextMenu()andapp.ContextMenus.New(). Sticking to one style (the manager helper) reduces cognitive load and shows the intended migration path.Same applies to the registration comment – the two-step flow (
NewthenAdd) is clearer when both calls originate fromapp.ContextMenus.
112-117: Mixed creation styles within one document
application.NewContextMenu()resurfaces here; consider swapping to the manager helper for consistency and to reinforce the new API.-contextMenu := application.NewContextMenu() +contextMenu := app.ContextMenus.New()v3/pkg/application/application_darwin.go (2)
238-242: Unused cancel-function fromOnApplicationEvent
m.parent.Events.OnApplicationEventreturns a cancel function; ignoring it makes it impossible to detach the listener in long-running apps and may leak closures.
Store it (even if only to call indestroy()later) to avoid silent leaks.- m.parent.Events.OnApplicationEvent(events.Mac.ApplicationDidFinishLaunching, func(*ApplicationEvent) { + cancel := m.parent.Events.OnApplicationEvent(events.Mac.ApplicationDidFinishLaunching, func(*ApplicationEvent) { // … }) // TODO: call `cancel()` during shutdown if required
293-295: Repeated dark-mode lookup could be expensive
globalApplication.Env.IsDarkMode()may hit Objective-C each time the event fires.
If this event is frequent, cache the bool inside the event context (or Env) before broadcasting.v3/examples/window/main.go (1)
670-676: Screen API now returns zero value on failure
app.Screens.GetPrimary()andGetAll()no longer surface an error. Consider checking for a zero‐initialisedScreen(empty ID) before using the struct to prevent empty-dialog spam on headless setups.docs/src/content/docs/learn/events.mdx (1)
431-439: Duplicated word & small grammar nitThe comment
// Emit from a specific windowsits immediately abovewindow.EmitEvent– fine – but static analysis flags a repeated word (“window window” if rendered after line wrap).
Also consider mirroring the Manager API by usingwindow.Events.Emitonce the window manager exposes it, to keep messaging symmetrical.-// Emit from a specific window +// Emit from a specific window🧰 Tools
🪛 LanguageTool
[duplication] ~437-~437: Possible typo: you repeated a word.
Context: ...ent", "hello") // Emit from a specific window window.EmitEvent("windowevent", "window specif...(ENGLISH_WORD_REPEAT_RULE)
v3/examples/dev/go.mod (1)
8-15: New indirects: double-check necessitySeveral new indirect dependencies (
mergo,go-crypto,xdg,circl,filepath-securejoin) balloon the example’s module graph.
If they’re brought in only via the rootwailsupgrade you can prune them withgo mod tidy -compat=1.22once the upstream settles.v3/pkg/application/key_binding_manager.go (1)
48-61: Expose immutable data fromGetAll
GetAllreturns a slice of pointers to the internally stored callbacks.
External packages can mutate the pointed-to struct fields, bypassing the lock and corrupting state.-func (kbm *KeyBindingManager) GetAll() []*KeyBinding { +func (kbm *KeyBindingManager) GetAll() []KeyBinding { … - result = append(result, &KeyBinding{ … }) + result = append(result, KeyBinding{ … }) // value copydocs/src/content/docs/learn/keybindings.mdx (1)
116-128: Normalise accelerator strings or document case-sensitivityAccelerators are shown with mixed-case (“Cmd+S”, “Ctrl+S”).
KeyBindingManager.Adddoes not enforce normalisation, so"ctrl+s"and"Ctrl+S"register two distinct bindings. Either:
- Internally canonicalise to a lower-case format, or
- Explicitly state in the docs that accelerators are case-sensitive.
This avoids subtle duplicate or missed-binding bugs.
v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/badge/service.ts (2)
11-15: Remove unused import to avoid dead-code noise
$Createis imported but never referenced.
Even though the// @ts-ignorecomment suppresses the TypeScript error, shipping unused symbols clutters the bundle and defeats tree-shaking.-import { Call as $Call, CancellablePromise as $CancellablePromise, Create as $Create } from "@wailsio/runtime"; +import { Call as $Call, CancellablePromise as $CancellablePromise } from "@wailsio/runtime";
31-33: Missing JSDoc for newly added API
SetCustomBadgelacks the descriptive block that the other exported functions have. Generated bindings are part of the public surface; keep them self-documented for IDE tooltips and website generation.+/** + * SetCustomBadge sets a badge with extended styling options. + * @param label The text to display inside the badge. + * @param options Additional styling options (colour, font, etc.). + */ export function SetCustomBadge(label: string, options: $models.Options): $CancellablePromise<void> {docs/src/content/docs/learn/browser.mdx (2)
167-170: Compile-time name collision in example codeIn
isValidURL, the parameterurlshadows the importednet/urlpackage;url.Parsetherefore fails to compile.-func isValidURL(url string) bool { - parsed, err := url.Parse(url) +func isValidURL(rawURL string) bool { + parsed, err := neturl.Parse(rawURL) // import net/url as neturlSmall detail, but examples copied verbatim should build.
505-505: Minor grammar – missing determiner“showing them in a dialog” – add “the”/“a” as appropriate to improve readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~505-~505: You might be missing the article “the” here.
Context: ...perations fail, such as copying URLs to clipboard or showing them in a dialog for manual ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
v3/pkg/application/event_manager.go (1)
33-41: Method names are fine – consider documenting cancellation semantics
On,Off,Resetwork well but callers might miss that callbacks can callevent.Cancel(). A short comment or example would aid discoverability.v3/pkg/application/application.go (1)
311-313: Minor naming confusion
EventListener.callback func(app *ApplicationEvent)uses parameter nameapp, which shadows theAppconcept and can mislead readers. Recommend renaming toevtoreventin a follow-up patch.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
v3/examples/badge-custom/frontend/package-lock.jsonis excluded by!**/package-lock.jsonv3/examples/badge/frontend/dist/assets/index-edhLCYCH.jsis excluded by!**/dist/**v3/examples/badge/frontend/dist/index.htmlis excluded by!**/dist/**v3/examples/badge/frontend/package-lock.jsonis excluded by!**/package-lock.jsonv3/examples/dev/go.sumis excluded by!**/*.sum
📒 Files selected for processing (107)
docs/src/content/docs/guides/menus.mdx(9 hunks)docs/src/content/docs/learn/application-menu.mdx(8 hunks)docs/src/content/docs/learn/browser.mdx(1 hunks)docs/src/content/docs/learn/clipboard.mdx(3 hunks)docs/src/content/docs/learn/context-menu.mdx(7 hunks)docs/src/content/docs/learn/dialogs.mdx(10 hunks)docs/src/content/docs/learn/environment.mdx(1 hunks)docs/src/content/docs/learn/events.mdx(2 hunks)docs/src/content/docs/learn/keybindings.mdx(1 hunks)docs/src/content/docs/learn/manager-api.mdx(1 hunks)docs/src/content/docs/learn/notifications.mdx(1 hunks)docs/src/content/docs/learn/screens.mdx(1 hunks)docs/src/content/docs/learn/systray.mdx(3 hunks)docs/src/content/docs/learn/windows.mdx(1 hunks)v3/examples/badge-custom/main.go(3 hunks)v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/badge/index.ts(1 hunks)v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/badge/models.ts(1 hunks)v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/badge/service.ts(1 hunks)v3/examples/badge/frontend/bindings/image/color/index.ts(1 hunks)v3/examples/badge/frontend/bindings/image/color/models.ts(1 hunks)v3/examples/badge/main.go(3 hunks)v3/examples/binding/main.go(1 hunks)v3/examples/build/main.go(8 hunks)v3/examples/cancel-async/main.go(1 hunks)v3/examples/cancel-chaining/main.go(1 hunks)v3/examples/clipboard/main.go(2 hunks)v3/examples/contextmenus/main.go(3 hunks)v3/examples/dev/go.mod(1 hunks)v3/examples/dev/main.go(1 hunks)v3/examples/dialogs-basic/main.go(2 hunks)v3/examples/dialogs/main.go(5 hunks)v3/examples/drag-n-drop/main.go(2 hunks)v3/examples/environment/main.go(1 hunks)v3/examples/events-bug/main.go(2 hunks)v3/examples/events/main.go(3 hunks)v3/examples/file-association/main.go(3 hunks)v3/examples/frameless/main.go(1 hunks)v3/examples/gin-example/main.go(1 hunks)v3/examples/gin-routing/main.go(1 hunks)v3/examples/gin-service/main.go(1 hunks)v3/examples/gin-service/services/gin_service.go(3 hunks)v3/examples/hide-window/main.go(2 hunks)v3/examples/html-dnd-api/main.go(1 hunks)v3/examples/ignore-mouse/main.go(1 hunks)v3/examples/keybindings/main.go(2 hunks)v3/examples/menu/main.go(2 hunks)v3/examples/notifications/main.go(2 hunks)v3/examples/panic-handling/main.go(1 hunks)v3/examples/plain/main.go(2 hunks)v3/examples/raw-message/main.go(1 hunks)v3/examples/screen/main.go(1 hunks)v3/examples/screen/screens.go(2 hunks)v3/examples/services/main.go(1 hunks)v3/examples/show-macos-toolbar/main.go(2 hunks)v3/examples/single-instance/main.go(1 hunks)v3/examples/systray-basic/main.go(1 hunks)v3/examples/systray-custom/main.go(2 hunks)v3/examples/systray-menu/main.go(1 hunks)v3/examples/video/main.go(1 hunks)v3/examples/window-api/main.go(1 hunks)v3/examples/window-call/main.go(2 hunks)v3/examples/window-menu/main.go(1 hunks)v3/examples/window/main.go(30 hunks)v3/examples/wml/main.go(2 hunks)v3/internal/commands/appimage_testfiles/main.go(3 hunks)v3/internal/generator/testcases/aliases/main.go(1 hunks)v3/internal/generator/testcases/complex_expressions/main.go(1 hunks)v3/internal/generator/testcases/complex_instantiations/main.go(1 hunks)v3/internal/generator/testcases/complex_json/main.go(1 hunks)v3/internal/generator/testcases/complex_method/main.go(1 hunks)v3/internal/generator/testcases/cyclic_imports/main.go(1 hunks)v3/internal/generator/testcases/cyclic_types/main.go(1 hunks)v3/internal/generator/testcases/directives/main.go(1 hunks)v3/internal/generator/testcases/embedded_interface/main.go(1 hunks)v3/internal/generator/testcases/enum/main.go(1 hunks)v3/internal/generator/testcases/enum_from_imported_package/main.go(1 hunks)v3/internal/generator/testcases/function_from_imported_package/main.go(1 hunks)v3/internal/generator/testcases/function_from_nested_imported_package/main.go(1 hunks)v3/internal/generator/testcases/function_multiple_files/main.go(1 hunks)v3/internal/generator/testcases/function_single/main.go(1 hunks)v3/internal/generator/testcases/function_single_context/main.go(1 hunks)v3/internal/generator/testcases/function_single_internal/main.go(1 hunks)v3/internal/generator/testcases/map_keys/main.go(1 hunks)v3/internal/generator/testcases/marshalers/main.go(1 hunks)v3/internal/generator/testcases/out_of_tree/main.go(1 hunks)v3/internal/generator/testcases/struct_literal_multiple/main.go(1 hunks)v3/internal/generator/testcases/struct_literal_multiple_files/main.go(1 hunks)v3/internal/generator/testcases/struct_literal_multiple_other/main.go(1 hunks)v3/internal/generator/testcases/struct_literal_non_pointer_single/main.go(1 hunks)v3/internal/generator/testcases/struct_literal_single/main.go(1 hunks)v3/internal/generator/testcases/variable_single/main.go(1 hunks)v3/internal/generator/testcases/variable_single_from_function/main.go(1 hunks)v3/internal/generator/testcases/variable_single_from_other_function/main.go(1 hunks)v3/pkg/application/application.go(7 hunks)v3/pkg/application/application_darwin.go(3 hunks)v3/pkg/application/application_linux.go(1 hunks)v3/pkg/application/application_windows.go(1 hunks)v3/pkg/application/browser_manager.go(1 hunks)v3/pkg/application/clipboard_manager.go(1 hunks)v3/pkg/application/context_menu_manager.go(1 hunks)v3/pkg/application/dialog_manager.go(1 hunks)v3/pkg/application/dialogs_linux.go(2 hunks)v3/pkg/application/environment_manager.go(1 hunks)v3/pkg/application/event_manager.go(1 hunks)v3/pkg/application/events_common_darwin.go(1 hunks)v3/pkg/application/key_binding_manager.go(1 hunks)v3/pkg/application/linux_cgo.go(5 hunks)
⛔ Files not processed due to max files limit (17)
- v3/pkg/application/linux_purego.go
- v3/pkg/application/menu.go
- v3/pkg/application/menu_manager.go
- v3/pkg/application/menuitem_dev.go
- v3/pkg/application/menuitem_roles.go
- v3/pkg/application/messageprocessor.go
- v3/pkg/application/messageprocessor_clipboard.go
- v3/pkg/application/messageprocessor_dialog.go
- v3/pkg/application/messageprocessor_screens.go
- v3/pkg/application/messageprocessor_system.go
- v3/pkg/application/roles.go
- v3/pkg/application/screen_windows.go
- v3/pkg/application/screenmanager.go
- v3/pkg/application/system_tray_manager.go
- v3/pkg/application/systemtray.go
- v3/pkg/application/webview_window.go
- v3/pkg/application/window_manager.go
🧰 Additional context used
🧠 Learnings (1)
v3/examples/keybindings/main.go (1)
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/examples/keybindings/main.go:16-17
Timestamp: 2024-09-20T23:34:29.841Z
Learning: In the codebase, `application.Options.KeyBindings` uses the `application.Window` type, whereas `application.WebviewWindowOptions.KeyBindings` uses `*application.WebviewWindow`. This is intentional and acceptable.
🧬 Code Graph Analysis (19)
v3/internal/generator/testcases/complex_json/main.go (1)
v3/pkg/events/events.go (1)
Windows(364-364)
v3/internal/generator/testcases/complex_method/main.go (1)
v3/pkg/events/events.go (1)
Windows(364-364)
v3/examples/panic-handling/main.go (2)
v3/pkg/events/events.go (1)
Windows(364-364)v3/pkg/application/application.go (1)
New(49-168)
v3/internal/generator/testcases/function_single_internal/main.go (1)
v3/pkg/events/events.go (1)
Windows(364-364)
v3/examples/cancel-chaining/main.go (1)
v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/internal/generator/testcases/variable_single/main.go (1)
v3/pkg/events/events.go (1)
Windows(364-364)
v3/examples/environment/main.go (1)
v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/screen/main.go (1)
v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/binding/main.go (1)
v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/gin-example/main.go (3)
v3/internal/runtime/desktop/@wailsio/runtime/src/events.ts (1)
On(96-98)v3/pkg/application/events.go (1)
CustomEvent(52-58)v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/dialogs/main.go (2)
v3/pkg/events/events.go (1)
Windows(364-364)v3/pkg/application/application.go (1)
New(49-168)
v3/internal/generator/testcases/function_single_context/main.go (1)
v3/pkg/events/events.go (1)
Windows(364-364)
v3/examples/contextmenus/main.go (3)
v3/pkg/events/events.go (1)
Windows(364-364)v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)v3/pkg/application/application.go (1)
New(49-168)
v3/examples/systray-custom/main.go (3)
v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)v3/pkg/application/systemtray.go (1)
SystemTray(44-66)v3/pkg/application/application.go (1)
New(49-168)
v3/examples/video/main.go (3)
v3/pkg/events/events.go (2)
Mac(90-90)Windows(364-364)v3/pkg/application/events.go (1)
ApplicationEvent(11-16)v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/keybindings/main.go (2)
v3/pkg/events/events.go (1)
Windows(364-364)v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/gin-routing/main.go (2)
v3/pkg/application/events.go (1)
CustomEvent(52-58)v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/show-macos-toolbar/main.go (1)
v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
v3/examples/single-instance/main.go (2)
v3/pkg/events/events.go (1)
Windows(364-364)v3/pkg/application/webview_window_options.go (1)
WebviewWindowOptions(32-142)
🪛 LanguageTool
docs/src/content/docs/learn/windows.mdx
[style] ~266-~266: Consider an alternative adverb to strengthen your wording.
Context: ... }) ``` 2. Resource Management: Properly clean up windows when they're no longer...
(PROPERLY_THOROUGHLY)
docs/src/content/docs/learn/browser.mdx
[uncategorized] ~505-~505: You might be missing the article “the” here.
Context: ...perations fail, such as copying URLs to clipboard or showing them in a dialog for manual ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
docs/src/content/docs/learn/events.mdx
[duplication] ~437-~437: Possible typo: you repeated a word.
Context: ...ent", "hello") // Emit from a specific window window.EmitEvent("windowevent", "window specif...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (71)
v3/internal/generator/testcases/complex_expressions/main.go (1)
46-46: API refactor applied correctly
app.Windows.New()is the correct replacement for the oldapp.NewWebviewWindow()call under the new Manager API. No other changes needed here.v3/internal/generator/testcases/enum_from_imported_package/main.go (1)
30-30: Window creation migrated to the Windows managerCall updated to the new pattern – looks good and keeps the example aligned with the refactor.
v3/internal/generator/testcases/directives/main.go (1)
52-52: Consistent use of the new Manager APIThe change is correct and consistent with the project-wide shift to
app.Windows.New().v3/internal/generator/testcases/complex_method/main.go (1)
35-35: Refactor compliance verified
app.Windows.New()correctly replaces the deprecated call.v3/internal/generator/testcases/function_single_context/main.go (1)
38-38: Updated to new window-manager callMatches the new API; no further action required.
v3/internal/generator/testcases/complex_instantiations/main.go (1)
52-52: Confirm returned value fromWindows.New()is intentionally ignored
app.Windows.New()likely returns a*application.Window(or similar).
If the caller does not need the handle, ignoring it is fine, but double-check that the function does not return an additional boolean/error; otherwise compilation will fail.v3/internal/generator/testcases/cyclic_imports/main.go (1)
47-47: Validate signature change forWindows.New()Same note as other files: ensure the new manager API’s
New()method returns a single value; if it returns two, the current call will not compile.v3/internal/generator/testcases/variable_single_from_other_function/main.go (1)
45-45: Aligned with new manager-based API – looks goodThe switch to
app.Windows.New()matches the new modular architecture. No additional adjustments needed here.v3/internal/generator/testcases/marshalers/main.go (1)
201-201: Change correctly adoptsWindowsmanagerThe update keeps this test-case in sync with the new API surface. ✔️
v3/internal/generator/testcases/aliases/main.go (1)
129-129: Consistent with the refactorUsing
app.Windows.New()here maintains consistency across all generator test-cases.v3/examples/gin-service/main.go (1)
33-37: Correct migration toNewWithOptions
app.Windows.NewWithOptions(...)is the proper replacement for the oldNewWebviewWindowWithOptionscall; the suppliedapplication.WebviewWindowOptionsstruct remains valid. Looks good.v3/internal/generator/testcases/cyclic_types/main.go (1)
38-38: Window creation updated as expectedThe call now routes through the
Windowsmanager, matching the new API contract.v3/examples/panic-handling/main.go (1)
51-53: Adopts newWindowsmanager cleanly – looks goodThe call chain now uses
app.Windows.New()and method chaining still reads clearly. No functional or stylistic issues spotted.v3/examples/events-bug/main.go (1)
42-51: Second window call mirrors first – consistentSame comment as above; change is correct and consistent with the new API.
v3/internal/generator/testcases/struct_literal_multiple/main.go (1)
33-34: Update aligns with manager pattern
app.Windows.New()replaces the deprecated constructor as expected. No further action needed.v3/internal/generator/testcases/struct_literal_non_pointer_single/main.go (1)
197-198: Correct switch toWindowsmanagerThe example now follows the new API. All good.
v3/examples/badge/frontend/bindings/image/color/index.ts (1)
1-6: Verify module-resolution for./models.jsThe re-export targets
./models.js, while the source file in the repo appears to bemodels.ts.
With TypeScript’s Node16/Next resolution this can work only if:
- A compiled
models.jswill exist at runtime, andtscis configured to allow imports with .js extension pointing to .ts sources (moduleResolution=node16orbundler, orallowImportingTsExtensions).If neither is true the build will fail to resolve the module. Please double-check the tsconfig or emit pipeline.
v3/examples/window-call/main.go (2)
66-66: Inconsistent menu API usage
You’ve migratedSettoapp.Menus, but the menu is still created withapp.NewMenu().
If the old helper is deprecated, switch to the manager for symmetry:-menu := app.NewMenu() +menu := app.Menus.New()Confirm that
Menus.New()exists in the refactor; otherwise keep as-is.
59-63: 🛠️ Refactor suggestionDitto for windows created from the menu action
Failing fast inside the click handler avoids leaving the app in an inconsistent state.- app.Windows.New(). + if wnd, err := app.Windows.New(); err == nil { wnd.SetTitle("WebviewWindow …").SetRelativePosition(…).Show() } else { log.Println("failed to open window:", err) }Likely an incorrect or invalid review comment.
v3/examples/badge/frontend/bindings/github.com/wailsapp/wails/v3/pkg/services/badge/index.ts (1)
8-11: Verify.jsimport path works under the TypeScript compilerThe file is a TypeScript source but imports
Optionsfrom"./models.js".
Depending ontsconfig(allowJs,resolveJsonModule,esModuleInterop), this path can cause “cannot find module” errors when emitting declaration files.If
models.tsexists, prefer the TypeScript extension:-} from "./models.js"; +} from "./models.js"; // ✅ runtime +// If you emit declarations, consider "./models.js" ➜ "./models"Double-check the build passes on strict CI.
v3/examples/screen/main.go (1)
62-71: LGTM – call updated to new Window managerThe switch to
app.Windows.NewWithOptionsaligns with the new manager-based API and preserves the original options.
No additional changes required.v3/examples/environment/main.go (1)
27-36: LGTM – window creation updated correctlyThe code now uses
app.Windows.NewWithOptions, matching the refactored API. Everything else remains intact.v3/examples/single-instance/main.go (1)
66-71: API-migration looks correct – no further actionSwapping
app.NewWebviewWindowWithOptionsforapp.Windows.NewWithOptionsmatches the new manager API. Signature and option usage remain intact, so this hunk needs no additional changes.v3/examples/keybindings/main.go (2)
23-33: Correct use of theWindowsmanagerThe example now instantiates the first window via
app.Windows.NewWithOptions, fully aligning with the new API.
34-43: Ditto for second window – LGTMv3/pkg/application/application_windows.go (1)
133-134: Encapsulation approvedAssigning the menu to the unexported
applicationMenufield reinforces the newMenusmanager encapsulation. No issues spotted.docs/src/content/docs/learn/notifications.mdx (1)
123-124: Documentation correctly reflectsEventsmanagerThe snippet now uses
app.Events.Emit, matching the refactor. Good catch.v3/examples/cancel-chaining/main.go (1)
26-29: Switched to manager API correctly – looks goodThe example now creates its window via
app.Windows.NewWithOptions, matching the new modular API. No further changes needed here.v3/examples/dev/main.go (1)
26-36: Correct adoption ofWindowsmanagerThe call to
app.Windows.NewWithOptions(plus the unchanged options struct) is consistent with the new design. Nice straightforward update.v3/examples/frameless/main.go (1)
27-30: Manager-based window creation acknowledgedThe frameless-window example is now in line with the new API. Implementation remains functionally identical – all good.
v3/examples/cancel-async/main.go (1)
26-29: Conformant change toapp.WindowsThe migration to
app.Windows.NewWithOptionsis correct and compiles cleanly with the updated package. Nothing further.v3/examples/ignore-mouse/main.go (1)
19-25: Window variable retained, API call updated – goodThe example keeps the returned
*WebviewWindowinwindowand moves toapp.Windows.NewWithOptions. Alignment with the new API is accurate.v3/pkg/application/events_common_darwin.go (1)
16-19: Good adaptation to the newEventsmanagerSwitching to
m.parent.Events.OnApplicationEventaligns with the manager-based API and keeps event registration encapsulated.
No further action required.v3/pkg/application/application_linux.go (1)
90-92: Assignment now targets the private field – looks correctUpdating to
globalApplication.applicationMenumatches the recent visibility change and prevents accidental external mutation.
Implementation is consistent with the newMenusmanager. Good catch.v3/examples/binding/main.go (1)
26-29: API call updated correctlyThe example now uses
app.Windows.NewWithOptions, matching the new manager pattern. No issues spotted.v3/examples/dialogs-basic/main.go (1)
37-38:app.Menus.Setcall looks goodThe menu is now set via the manager, matching the refactor. 👍
v3/examples/screen/screens.go (1)
16-17: Loss of error-path visibility –GetAll()does not expose failuresThe old
GetScreens()returned(screens []*Screen, err error), letting callers react to runtime failures.
After the refactor the call is replaced byScreens.GetAll()which returns only the slice, silently swallowing any potential backend error.If the new contract genuinely guarantees a non-nil/always-valid slice, please annotate that fact in code comments or docs.
Otherwise propagate an error (orbool ok) so callers can fail fast instead of continuing with an invalid slice.v3/examples/gin-example/main.go (1)
101-108: Missing error handling for window creation
Windows.NewWithOptionscan return an error (e.g. invalid options, platform failure).
Bubble it up instead of discarding:-win := app.Windows.NewWithOptions(application.WebviewWindowOptions{ … }) +win, err := app.Windows.NewWithOptions(application.WebviewWindowOptions{ … }) +if err != nil { + log.Fatal(err) +} +_ = winv3/examples/plain/main.go (1)
49-56: Verify thatWindows.NewWithOptionsis goroutine-safeThe goroutine executes after
app.Run()starts, meaning window creation happens concurrently with the main UI thread. Double-check that the window manager is designed for concurrent use; otherwise, marshal the call back onto the main thread (e.g., viaapp.Events.Dispatch(...)or a dedicated UI scheduler).v3/examples/clipboard/main.go (1)
68-70: Nice usage ofapp.Menus.Set, but update the matching constructorOnce the constructor above is fixed, this call remains valid.
v3/examples/wml/main.go (1)
38-43: LGTM – event wiring matches the newEventsmanagerThe switch to
app.Events.On(...)is correct and consistent with the new API.v3/examples/drag-n-drop/main.go (1)
40-41:❌ Incorrect review comment
Check the error/ok result of
app.Events.Emit
Events.Emitreturns(bool, error)(per manager API) so silently discarding it loses delivery failures and makes debugging harder.- app.Events.Emit("files", files) + if ok, err := app.Events.Emit("files", files); err != nil { + app.Logger.Error("failed emitting 'files' event", "error", err) + } else if !ok { + app.Logger.Warn("'files' event had no listeners") + }
🏁 Script executed:
#!/bin/bash # Find the definition of the Emit method to confirm its return signature. rg -R "^func.*Emit" -n .Length of output: 431
🏁 Script executed:
#!/bin/bash # Locate all Emit method definitions and the Events type to verify return signatures. grep -rn --include="*.go" "func .*Emit" . grep -rn --include="*.go" "type Events" .Length of output: 1010
🏁 Script executed:
#!/bin/bash # Show the signature and any return statements for EventManager.Emit sed -n '1,200p' v3/pkg/application/event_manager.go | sed -n '1,50p'Length of output: 1406
Ignore error/ok checks on
Emit– it has no return values
Theapp.Events.Emitmethod in v3’sEventManageris defined as returningvoid, so there is no(bool, error)to inspect. The proposed diff isn’t applicable and can be safely disregarded.Likely an incorrect or invalid review comment.
v3/examples/show-macos-toolbar/main.go (1)
20-30: Store window pointers if future interaction is requiredBoth windows are created and the pointers discarded. Fine for a static demo, but if you later need to modify them (title, size, etc.) you’ll need the references. Keeping them in variables now avoids refactors later.
No action needed if the demo deliberately keeps them fire-and-forget.
v3/examples/dialogs/main.go (1)
60-66:ShowAboutalready shows a dialog – no need for a submenu click handler?
app.Menus.ShowAbout()immediately displays the standard About dialog.
Invoking it inside a menu item click is correct but consider whether the separate submenu is still required or if the item should directly use the built-in About role to keep platform behaviour consistent.docs/src/content/docs/learn/application-menu.mdx (4)
13-17: Documentation correctly reflects the Manager APIThe example correctly demonstrates menu creation via
app.Menus.New()which matches the new API.
No issues spotted.
26-30: macOS example looks good
app.Menus.Set(menu)is the correct replacement for the old direct method.
39-50: Windows/Linux snippets are consistentUsing
app.Windows.Current().SetMenu(menu)aligns with the new Window manager.
192-194: Nice clarificationCalling
app.Windows.Current()inside handlers is a helpful hint.v3/examples/hide-window/main.go (1)
46-49: Good migration toEventsmanager
app.Events.OnApplicationEventis the correct replacement for the oldapp.OnApplicationEvent.v3/examples/file-association/main.go (3)
51-60: Window creation migration LGTMUsing
app.Windows.NewWithOptionsis correct and the options struct is unchanged.
63-66: Application-level event subscription looks correct
app.Events.OnApplicationEvent(events.Common.ApplicationOpenedWithFile, …)matches the new API.
79-80: Custom event emission migrated
app.Events.Emit("time", now)is the right call.v3/examples/gin-routing/main.go (2)
97-99: Event handler migration LGTM
app.Events.On("gin-button-clicked", …)matches the newEventsmanager.
102-107: Window creation migration LGTM
app.Windows.NewWithOptionsis used consistently.v3/pkg/application/linux_cgo.go (1)
1603-1610: Good defensive check on window lookupThe inline
okguard avoids a potential nil-dereference – nice.docs/src/content/docs/learn/clipboard.mdx (1)
14-15: Docs align with new manager API – looks goodv3/examples/build/main.go (1)
263-266: Initial window not shown
app.Windows.New()returns a builder that requires.Show()(unlessNew()side-effects changed). Without it the demo starts with no visible window.- app.Windows.New() + app.Windows.New().Show()docs/src/content/docs/guides/menus.mdx (1)
268-281: Macro roles example ok – but remind users to callmenu.Update()After mutating the menu (adding roles/submenus) it must be
Update()-ed on Windows/Linux. Adding a one-liner avoids subtle bugs where menu appears unchanged.docs/src/content/docs/learn/context-menu.mdx (2)
50-59: Out-of-date wording on menu-id referenceThe surrounding paragraph later says the CSS id must match that used in
NewContextMenu, but in the new flow the id comes fromAdd(...).
Update the prose accordingly, otherwise beginners may hunt for a non-existent parameter.
198-212: Window creation code not yet migratedThe example switches to
app.ContextMenus.*but still callsapp.NewWebviewWindow().
For consistency with the rest of the PR, useapp.Windows.New()(or.NewWithOptions()).v3/pkg/application/application_darwin.go (1)
318-325: Potential race onglobalApplication.Windows.GetByID
processURLRequestis executed from C callbacks that may run on non-Go threads; ensureGetByIDis safe for concurrent access.
If the manager already guards with a mutex you’re fine; otherwise wrap this lookup with the manager’s lock.v3/examples/dev/go.mod (1)
22-22: Suspicious pseudo-version timestamp
github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8references a commit from November 2024, which is in the future relative to Go’s module proxy mirrors.
Resolve to an existing commit or you riskgo mod downloadfailures.docs/src/content/docs/learn/manager-api.mdx (1)
9-11: “Full backward-compatibility” claim conflicts with the PR descriptionThe introduction states that the Manager API is “maintaining full backward compatibility”, yet the PR summary explicitly calls this a breaking change that requires migration. Please revisit the wording to avoid misleading readers and to set correct expectations for developers upgrading to v3.
docs/src/content/docs/learn/windows.mdx (2)
116-124: VerifyOnCreatecallback signatureThe snippet shows
app.Windows.OnCreate(func(window application.Window) { … })In the new manager code the concrete callback type is likely
func(window Window)orfunc(window *application.WebviewWindow). Double-check the exported signature to keep examples compile-ready.
126-138: ConfirmRemove/RemoveByNameAPI availability
app.Windows.Remove(id)andRemoveByName(name)are demonstrated here, but they are not mentioned in the Manager API overview. If they are truly exported, add them to the reference section; otherwise, update the docs to the correct call (e.g.Close()on the window, or an internal manager helper).v3/pkg/application/key_binding_manager.go (1)
30-38: Race window between lookup and callback invocationThe read lock is released before invoking
callback.
A concurrentRemove()could nil-out the map entry, leading to a callback that is no longer registered being executed.
Consider holding the read lock for the duration of the call or documenting this behaviour clearly. A safer pattern is to copy the function pointer under lock and execute it afterwards only if it is still equal to the looked-up value.v3/pkg/application/dialog_manager.go (1)
21-25: Nil-options guard
OpenFileWithOptionsandSaveFileWithOptionsdereferenceoptionsviaSetOptionswithout a nil check.
If callers passnil(easy to do given the pointer type), you risk a panic unlessSetOptionshandles it internally. Clarify the contract or add a guard.if options != nil { result.SetOptions(options) }v3/pkg/application/application.go (4)
79-80: Potential nil-pointer – verify manager initialisation order
result.customEventProcessor = NewWailsEventProcessor(result.Events.dispatch)is executed insideNew().
Ifresult.Eventsis set after this line (i.e.init()hasn’t run yet),dispatchwill panic at runtime.Please double-check that
newApplication()invokes(*App).init()(or otherwise setsEvents) before reaching this assignment. If not, swap the lines or callresult.init()first.
273-285: Nice encapsulation with managersThe embedded managers dramatically improve discoverability and separation of concerns. 👍
415-427: Concurrent safety of manager pointersAll managers are created without guarding
a.runLock. That’s fine during initialisation, but if any manager stores references to shared maps (likeEventsdoes) ensure they always respect the parentApplocks when accessing those maps; otherwise the race detector will still complain even after fixingdispatch().
557-558: Menus channel consumer renamed – looks goodRouting
menuItemClickedtoa.Menus.handleMenuItemClickedkeeps responsibilities local to the manager. No issues spotted.
| @@ -30,7 +30,7 @@ func main() { | |||
| }, | |||
| }) | |||
|
|
|||
| app.NewWebviewWindow() | |||
| app.Windows.New() | |||
There was a problem hiding this comment.
Return value from app.Windows.New() is ignored – compilation will fail
Windows.New() returns at least one value (*application.Window, possibly a (win *application.Window, ok bool) tuple based on the new manager-API rules).
Calling it as a stand-alone statement causes “value not used” compilation errors in Go.
- app.Windows.New()
+ _ = app.Windows.New() // discard
+ // or: win := app.Windows.New() // keep for laterAdjust all occurrences (examples below) or change the API to return void if that is the intent.
📝 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.
| app.Windows.New() | |
| _ = app.Windows.New() // discard | |
| // or: win := app.Windows.New() // keep for later |
🤖 Prompt for AI Agents
In v3/internal/generator/testcases/variable_single_from_function/main.go at line
33, the call to app.Windows.New() returns a value that is currently ignored,
causing a compilation error. Modify the code to capture the returned value(s)
from app.Windows.New() by assigning them to appropriate variables or use the
blank identifier (_) if the values are not needed. This will resolve the "value
not used" error without changing the API.
| @@ -14,7 +14,7 @@ func main() { | |||
| }, | |||
| }) | |||
|
|
|||
| app.NewWebviewWindow() | |||
| app.Windows.New() | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Compile-time check for new API
Quick search script to surface any remaining NewWebviewWindow( calls:
Run it to ensure migration is complete.
🏁 Script executed:
#!/bin/bash
rg -n "NewWebviewWindow\\(" || echo "All occurrences migrated."Length of output: 413
Update remaining NewWebviewWindow references in documentation
The migration isn’t complete—these MDX files still reference the old API and need to be updated to the new app.Windows.New() call:
• docs/src/content/docs/learn/application-menu.mdx:251
• docs/src/content/docs/learn/bindings.mdx:186
• docs/src/content/docs/learn/clipboard.mdx:113
• docs/src/content/docs/learn/context-menu.mdx:213
• docs/src/content/docs/learn/systray.mdx:115
Please replace each app.NewWebviewWindow() occurrence with the new API and re-run:
rg -n "NewWebviewWindow\(" || echo "All occurrences migrated."to confirm that no references remain.
🤖 Prompt for AI Agents
In v3/internal/generator/testcases/function_multiple_files/main.go at line 17,
the code correctly uses the new API call app.Windows.New(). However, the review
points out that there are still references to the old API app.NewWebviewWindow()
in several documentation MDX files. To fix this, search for all occurrences of
app.NewWebviewWindow() in the listed MDX files and replace them with
app.Windows.New(). After updating, run the provided rg command to confirm no old
API references remain.
| app.Windows.New(). | ||
| SetTitle("WebviewWindow 1"). | ||
| Show() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error ignored on initial window creation
app.Windows.New() also returns (Window, error). Consider at least logging the error:
-wnd := app.Windows.New()
-if err != nil { … }
-…
+if wnd, err := app.Windows.New(); err != nil {
+ log.Fatal(err)
+} else {
+ wnd.SetTitle("WebviewWindow 1").Show()
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In v3/examples/window-call/main.go around lines 41 to 43, the call to
app.Windows.New() returns a Window and an error, but the error is currently
ignored. Modify the code to capture both the Window and error return values,
check if the error is non-nil, and log the error appropriately before proceeding
to set the title and show the window.
| screen := app.GetPrimaryScreen() | ||
| msg := fmt.Sprintf("Screen: %+v", screen) | ||
| application.InfoDialog().SetTitle("Primary Screen").SetMessage(msg).Show() | ||
| }) | ||
| stateMenu.Add("Get Screens").OnClick(func(ctx *application.Context) { | ||
| screens, err := app.GetScreens() | ||
| if err != nil { | ||
| application.ErrorDialog().SetTitle("Error").SetMessage(err.Error()).Show() | ||
| return | ||
| } | ||
| screens := app.GetScreens() | ||
| for _, screen := range screens { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Screen API not yet migrated
app.GetPrimaryScreen() / app.GetScreens() should be routed through the new Screens manager for consistency:
- screen := app.GetPrimaryScreen()
+ screen := app.Screens.GetPrimary()- screens := app.GetScreens()
+ screens := app.Screens.GetAll()(Adjust return-type handling if the new methods differ.)
📝 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.
| screen := app.GetPrimaryScreen() | |
| msg := fmt.Sprintf("Screen: %+v", screen) | |
| application.InfoDialog().SetTitle("Primary Screen").SetMessage(msg).Show() | |
| }) | |
| stateMenu.Add("Get Screens").OnClick(func(ctx *application.Context) { | |
| screens, err := app.GetScreens() | |
| if err != nil { | |
| application.ErrorDialog().SetTitle("Error").SetMessage(err.Error()).Show() | |
| return | |
| } | |
| screens := app.GetScreens() | |
| for _, screen := range screens { | |
| screen := app.Screens.GetPrimary() | |
| msg := fmt.Sprintf("Screen: %+v", screen) | |
| application.InfoDialog().SetTitle("Primary Screen").SetMessage(msg).Show() | |
| }) | |
| stateMenu.Add("Get Screens").OnClick(func(ctx *application.Context) { | |
| screens := app.Screens.GetAll() | |
| for _, screen := range screens { |
🤖 Prompt for AI Agents
In v3/internal/commands/appimage_testfiles/main.go around lines 343 to 349,
replace calls to app.GetPrimaryScreen() and app.GetScreens() with the
corresponding methods from the new Screens manager to maintain consistency.
Review the return types of the new Screens manager methods and adjust the code
accordingly to handle any differences in return values or data structures.
| s.screenManager.LayoutScreens(screens) | ||
| return s.screenManager.Screens() | ||
| return s.screenManager.GetAll() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return value now depends on internal state – guard against a nil screenManager
LayoutScreens() and GetAll() both dereference s.screenManager.
If the service is instantiated without an injected ScreenManager this will panic.
Consider a defensive check:
if s.screenManager == nil {
- return s.screenManager.GetAll()
+ return nil // or log/return an error
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In v3/examples/screen/screens.go around lines 54 to 56, the code calls
LayoutScreens and GetAll on s.screenManager without checking if s.screenManager
is nil, which can cause a panic if it is not initialized. Add a defensive check
to verify s.screenManager is not nil before calling these methods, and handle
the nil case appropriately to prevent runtime panics.
| kbm.app.keyBindingsLock.Lock() | ||
| defer kbm.app.keyBindingsLock.Unlock() | ||
| kbm.app.keyBindings[accelerator] = callback | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nil-map safety & duplicate detection
Add assumes app.keyBindings is already initialised.
If an external caller instantiates App without the internal initialiser, this will panic.
func (kbm *KeyBindingManager) Add(accelerator string, callback func(window *WebviewWindow)) {
- kbm.app.keyBindingsLock.Lock()
- defer kbm.app.keyBindingsLock.Unlock()
- kbm.app.keyBindings[accelerator] = callback
+ if accelerator == "" || callback == nil {
+ return // silently ignore invalid input
+ }
+ kbm.app.keyBindingsLock.Lock()
+ defer kbm.app.keyBindingsLock.Unlock()
+ if kbm.app.keyBindings == nil {
+ kbm.app.keyBindings = make(map[string]func(*WebviewWindow))
+ }
+ kbm.app.keyBindings[accelerator] = callback
}🤖 Prompt for AI Agents
In v3/pkg/application/key_binding_manager.go around lines 17 to 20, the Add
method assumes that app.keyBindings map is already initialized, which can cause
a panic if it is nil. To fix this, add a nil check before using the map and
initialize it if nil. This ensures nil-map safety and prevents runtime panics
when adding key bindings.
| func (cmm *ContextMenuManager) Get(name string) (*ContextMenu, bool) { | ||
| cmm.app.contextMenusLock.Lock() | ||
| defer cmm.app.contextMenusLock.Unlock() | ||
| menu, exists := cmm.app.contextMenus[name] | ||
| return menu, exists | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use RLock for read-only access to maximise parallelism
Get only reads the map; using Lock blocks every other goroutine—even readers—while the map is consulted.
- cmm.app.contextMenusLock.Lock()
- defer cmm.app.contextMenusLock.Unlock()
+ cmm.app.contextMenusLock.RLock()
+ defer cmm.app.contextMenusLock.RUnlock()Apply the same pattern to GetAll.
📝 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 (cmm *ContextMenuManager) Get(name string) (*ContextMenu, bool) { | |
| cmm.app.contextMenusLock.Lock() | |
| defer cmm.app.contextMenusLock.Unlock() | |
| menu, exists := cmm.app.contextMenus[name] | |
| return menu, exists | |
| } | |
| func (cmm *ContextMenuManager) Get(name string) (*ContextMenu, bool) { | |
| cmm.app.contextMenusLock.RLock() | |
| defer cmm.app.contextMenusLock.RUnlock() | |
| menu, exists := cmm.app.contextMenus[name] | |
| return menu, exists | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/context_menu_manager.go around lines 37 to 42, the Get
method uses a full Lock for reading the contextMenus map, which unnecessarily
blocks other readers. Change the Lock to RLock and the Unlock to RUnlock to
allow multiple concurrent readers. Apply the same change to the GetAll method to
maximize parallelism during read-only access.
| func (cmm *ContextMenuManager) GetAll() []*ContextMenu { | ||
| cmm.app.contextMenusLock.Lock() | ||
| defer cmm.app.contextMenusLock.Unlock() | ||
|
|
||
| result := make([]*ContextMenu, 0, len(cmm.app.contextMenus)) | ||
| for _, menu := range cmm.app.contextMenus { | ||
| result = append(result, menu) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inefficient slice pre-allocation with write lock
GetAll currently (a) grabs a full write lock and (b) iterates twice (length + range).
Switch to RLock and append without pre-allocating—the map size is already known but the micro-optimisation isn’t worth the exclusive lock.
- cmm.app.contextMenusLock.Lock()
- defer cmm.app.contextMenusLock.Unlock()
-
- result := make([]*ContextMenu, 0, len(cmm.app.contextMenus))
- for _, menu := range cmm.app.contextMenus {
- result = append(result, menu)
- }
+ cmm.app.contextMenusLock.RLock()
+ defer cmm.app.contextMenusLock.RUnlock()
+
+ result := make([]*ContextMenu, 0, len(cmm.app.contextMenus))
+ for _, menu := range cmm.app.contextMenus {
+ result = append(result, menu)
+ }📝 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 (cmm *ContextMenuManager) GetAll() []*ContextMenu { | |
| cmm.app.contextMenusLock.Lock() | |
| defer cmm.app.contextMenusLock.Unlock() | |
| result := make([]*ContextMenu, 0, len(cmm.app.contextMenus)) | |
| for _, menu := range cmm.app.contextMenus { | |
| result = append(result, menu) | |
| } | |
| return result | |
| } | |
| func (cmm *ContextMenuManager) GetAll() []*ContextMenu { | |
| cmm.app.contextMenusLock.RLock() | |
| defer cmm.app.contextMenusLock.RUnlock() | |
| result := make([]*ContextMenu, 0, len(cmm.app.contextMenus)) | |
| for _, menu := range cmm.app.contextMenus { | |
| result = append(result, menu) | |
| } | |
| return result | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/context_menu_manager.go lines 45 to 54, replace the full
write lock with a read lock by using RLock and RUnlock instead of Lock and
Unlock. Remove the pre-allocation of the result slice and simply append each
menu item during iteration. This avoids holding an exclusive lock and eliminates
the unnecessary micro-optimization of pre-allocating the slice capacity.
| func (em *EventManager) OnApplicationEvent(eventType events.ApplicationEventType, callback func(event *ApplicationEvent)) func() { | ||
| eventID := uint(eventType) | ||
| em.app.applicationEventListenersLock.Lock() | ||
| defer em.app.applicationEventListenersLock.Unlock() | ||
| listener := &EventListener{ | ||
| callback: callback, | ||
| } | ||
| em.app.applicationEventListeners[eventID] = append(em.app.applicationEventListeners[eventID], listener) | ||
| if em.app.impl != nil { | ||
| go func() { | ||
| defer handlePanic() | ||
| em.app.impl.on(eventID) | ||
| }() | ||
| } | ||
|
|
||
| return func() { | ||
| // lock the map | ||
| em.app.applicationEventListenersLock.Lock() | ||
| defer em.app.applicationEventListenersLock.Unlock() | ||
| // Remove listener | ||
| em.app.applicationEventListeners[eventID] = lo.Without(em.app.applicationEventListeners[eventID], listener) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Copy listener slice while locked to avoid future races
handleApplicationEvent copies listeners after releasing the RW-lock. If another goroutine adds/removes listeners the underlying array can still be mutated, leading to races. Copy the slice while the lock is held:
em.app.applicationEventListenersLock.RLock()
listeners := append([]*EventListener(nil), em.app.applicationEventListeners[event.Id]...)
em.app.applicationEventListenersLock.RUnlock()Same applies to hooks.
🤖 Prompt for AI Agents
In v3/pkg/application/event_manager.go around lines 54 to 76, the listener slice
is accessed without copying it while holding the lock, which can cause race
conditions if the underlying array is mutated concurrently. To fix this, modify
the code that reads the listeners to copy the slice inside a read lock by
acquiring the RLock, copying the slice with append to a new slice, then
releasing the RLock before using the copy. Apply the same pattern for hooks to
ensure safe concurrent access.
| func (em *EventManager) dispatch(event *CustomEvent) { | ||
| listeners := em.app.wailsEventListeners | ||
|
|
||
| for _, window := range em.app.windows { | ||
| if event.IsCancelled() { | ||
| return | ||
| } | ||
| window.DispatchWailsEvent(event) | ||
| } | ||
|
|
||
| for _, listener := range listeners { | ||
| if event.IsCancelled() { | ||
| return | ||
| } | ||
| listener.DispatchWailsEvent(event) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsynchronised map & slice access can trigger data races
dispatch() walks em.app.windows and em.app.wailsEventListeners without acquiring the matching read-locks (windowsLock, wailsEventListenerLock).
While other goroutines (window creation/close, listener registration) mutate those collections, this will race under -race.
func (em *EventManager) dispatch(event *CustomEvent) {
- listeners := em.app.wailsEventListeners
-
- for _, window := range em.app.windows {
+ em.app.windowsLock.RLock()
+ for _, window := range em.app.windows {
if event.IsCancelled() {
- return
+ em.app.windowsLock.RUnlock()
+ return
}
window.DispatchWailsEvent(event)
}
- for _, listener := range listeners {
+ em.app.windowsLock.RUnlock()
+
+ em.app.wailsEventListenerLock.Lock()
+ listeners := slices.Clone(em.app.wailsEventListeners)
+ em.app.wailsEventListenerLock.Unlock()
+
+ for _, listener := range listeners {
...
}
}Please wrap the reads in the appropriate locks or copy the collections while locked.
Failing tests under the race detector will surface otherwise.
📝 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 (em *EventManager) dispatch(event *CustomEvent) { | |
| listeners := em.app.wailsEventListeners | |
| for _, window := range em.app.windows { | |
| if event.IsCancelled() { | |
| return | |
| } | |
| window.DispatchWailsEvent(event) | |
| } | |
| for _, listener := range listeners { | |
| if event.IsCancelled() { | |
| return | |
| } | |
| listener.DispatchWailsEvent(event) | |
| } | |
| } | |
| func (em *EventManager) dispatch(event *CustomEvent) { | |
| // Snapshot windows under RLock | |
| em.app.windowsLock.RLock() | |
| for _, window := range em.app.windows { | |
| if event.IsCancelled() { | |
| em.app.windowsLock.RUnlock() | |
| return | |
| } | |
| window.DispatchWailsEvent(event) | |
| } | |
| em.app.windowsLock.RUnlock() | |
| // Snapshot listeners under Lock | |
| em.app.wailsEventListenerLock.Lock() | |
| listeners := slices.Clone(em.app.wailsEventListeners) | |
| em.app.wailsEventListenerLock.Unlock() | |
| for _, listener := range listeners { | |
| if event.IsCancelled() { | |
| return | |
| } | |
| listener.DispatchWailsEvent(event) | |
| } | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/event_manager.go around lines 96 to 112, the dispatch
method accesses em.app.windows and em.app.wailsEventListeners without acquiring
the corresponding read locks (windowsLock and wailsEventListenerLock), causing
potential data races. To fix this, acquire the appropriate read locks before
accessing these collections, either by locking windowsLock before reading
em.app.windows and wailsEventListenerLock before reading
em.app.wailsEventListeners, or by copying the slices/maps while holding the
locks and then iterating over the copies after releasing the locks.
|
Really love this. Would window management under |
Yeah I did wonder this myself and the more I think about it, the more it makes sense..... e.g. Window.Get() |
|
🚀 Major API Improvement: Complete App Restructuring
This PR completes the comprehensive restructuring of the Wails v3 App API, transforming it from a monolithic interface into a clean, organized manager pattern that dramatically improves developer experience and maintainability.
✨ High-Level Benefits
🎯 Improved Developer Experience
app.Windows.*for window operations,app.Events.*for events)🏗️ Better Architecture
🔧 Enhanced Maintainability
🔄 Migration Guide
This is a breaking change that requires updating your application code. However, the migration is straightforward and provides immediate benefits.
Window Management
Event Management
System Tray Management
Application Menu Management
Quick Migration Script
For most cases, you can use these find-and-replace patterns:
📋 Detailed Changes
Manager Organization
The App struct now provides access to 11 focused managers:
Windows- Window creation, management, and current window accessEvents- Custom events and application event handlingMenus- Application menu management and menu creationDialogs- File dialogs and message dialogsContextMenus- Context menu registration and managementKeyBindings- Keyboard shortcut registration and handlingSystemTray- System tray creation and managementScreens- Multi-monitor support and screen informationBrowser- Browser-related functionality and controlsEnv- Environment information and system integrationClipboard- Clipboard text operationsAPI Consistency Improvements
Unified Naming Conventions
Get()- Retrieve single items by name/identifierGetAll()- Retrieve collections of itemsGetByID()- Retrieve items by IDAdd()/Remove()- Manage collectionsNew()/NewWithOptions()- Create new instancesConsistent Return Patterns
(item, bool)tuplesEnhanced Manager APIs
GetByID(), improvedCurrent()methodShow*prefixes for all dialog methods for clarityKeyBindingobjects instead of raw mapsGetAll()Removed Deprecated Methods
App.NewWebviewWindow()→App.Windows.New()App.NewWebviewWindowWithOptions()→App.Windows.NewWithOptions()App.CurrentWindow()→App.Windows.Current()App.OnApplicationEvent()→App.Events.OnApplicationEvent()App.RegisterApplicationEventHook()→App.Events.RegisterApplicationEventHook()App.NewSystemTray()→App.SystemTray.New()WindowManager.GetCurrent()→WindowManager.Current()(removed duplicate)Private Implementation Details
ApplicationMenufield private with proper manager accessNew*Manager()→new*Manager())Updated Examples and Tests
🧪 Testing
This PR maintains 100% functional compatibility while improving the developer interface:
🔍 Review Focus Areas
🎉 Future Benefits
This restructuring provides a solid foundation for:
This change represents a significant step forward in Wails v3's API maturity, providing a clean, discoverable, and maintainable interface that will serve developers well as the framework continues to evolve.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Chores