feat(v3): Modal windows (macOS)#4839
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an AttachModal API to Window/WebviewWindow to present modal sheets (macOS). Platform implementations: macOS presents native sheets; Windows sets parent HWND and manages enable/disable; Android/iOS/Linux provide no-op stubs. Docs and examples updated; ShowModal replaced by ShowModaldialog(parent, title). Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Go as WebviewWindow (Go)
participant Impl as Platform Impl
participant Native as Native macOS API
App->>Go: AttachModal(modalWindow)
activate Go
Go->>Go: validate modalWindow, cast to *WebviewWindow
Go->>Go: lock / synchronize
Go->>Impl: impl.attachModal(modalWebviewWindow)
deactivate Go
alt macOS (sheet)
activate Impl
Impl->>Impl: obtain native window pointers
Impl->>Native: createModalWindow(parentPtr, modalPtr)
activate Native
Native->>Native: beginSheet(present modal)
Native-->>Impl: completion
deactivate Native
deactivate Impl
else Windows
activate Impl
Impl->>Impl: set GWLP_HWNDPARENT, disable parent, show modal
Impl-->>Impl: re-enable parent on close/destroy
deactivate Impl
else Other
Impl->>Impl: no-op (unsupported)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/features/windows/multiple.mdx (1)
296-308: Clarify the relationship betweenParentfield andAttachModalmethod.The example shows both setting
Parent: parentinWebviewWindowOptions(line 301) and callingparent.AttachModal(dialog)(line 306). This creates confusion about which approach is correct:
- Is the
Parentfield still necessary ifAttachModalis called?- Should the example demonstrate one approach or the other, not both?
- The function title parameter is declared but never used in the implementation
🔎 Proposed clarification
-func ShowModaldialog(parent *application.WebviewWindow, title string) { +func ShowModalDialog(parent *application.WebviewWindow, title string) { dialog := app.NewWebviewWindowWithOptions(application.WebviewWindowOptions{ Title: title, Width: 400, Height: 200, - Parent: parent, AlwaysOnTop: true, Resizable: false, }) parent.AttachModal(dialog) }This removes the
Parentfield sinceAttachModalestablishes the parent-child relationship. Also note the typo fix:ShowModaldialog→ShowModalDialog.
🧹 Nitpick comments (4)
v3/pkg/application/webview_window.go (2)
1269-1283: Enhance godoc comment to mention platform-specific behavior.The godoc comment states "presenting it as a sheet on macOS" but doesn't explicitly mention that this is a no-op on Windows and Linux. For a cross-platform API with platform-specific behavior, it's important to document this in the godoc so developers don't need to consult external documentation.
Additionally, the silent failure when
modalWindowis not a*WebviewWindow(lines 1275-1278) might be unexpected. Consider whether an error log would be helpful for debugging misuse of the API.🔎 Suggested documentation enhancement
-// AttachModal attaches a modal window to this window, presenting it as a sheet on macOS. +// AttachModal attaches a modal window to this window, presenting it as a sheet on macOS. +// On Windows and Linux, this method has no effect as modal sheets are not supported. +// The modalWindow parameter must be a *WebviewWindow; other Window implementations are ignored. func (w *WebviewWindow) AttachModal(modalWindow Window) {
1275-1278: Consider logging when modal window type assertion fails.The method silently returns when the type assertion
modalWindow.(*WebviewWindow)fails (line 1275). This could make debugging difficult if a developer accidentally passes a differentWindowimplementation.🔎 Optional: Add debug logging
modalWebviewWindow, ok := modalWindow.(*WebviewWindow) if !ok || modalWebviewWindow == nil { + w.Error("AttachModal requires a *WebviewWindow, got %T", modalWindow) return }v3/pkg/application/webview_window_darwin.go (1)
1534-1543: Add parent window validation and consider error reporting.The method silently returns on validation failures, making it difficult to debug issues. Consider adding:
- Validation that the parent window (
w.nsWindow) is not nil before calling the C function- Logging when validation fails to aid debugging
- Error return value to allow callers to handle failures appropriately
🔎 Suggested improvements
-func (w *macosWebviewWindow) attachModal(modalWindow *WebviewWindow) { +func (w *macosWebviewWindow) attachModal(modalWindow *WebviewWindow) error { + if w.nsWindow == nil { + globalApplication.error("attachModal: parent window is nil") + return fmt.Errorf("parent window is nil") + } if modalWindow == nil || modalWindow.impl == nil || modalWindow.isDestroyed() { + globalApplication.error("attachModal: invalid modal window") - return + return fmt.Errorf("invalid modal window") } modalNativeWindow := modalWindow.impl.nativeWindow() if modalNativeWindow == nil { + globalApplication.error("attachModal: modal native window is nil") - return + return fmt.Errorf("modal native window is nil") } C.createModalWindow(w.nsWindow, modalNativeWindow) + return nil }Note: This would require updating the internal webviewWindowImpl interface and the public API in webview_window.go.
docs/src/content/docs/features/windows/multiple.mdx (1)
376-385: Fix function name typo and approve the example pattern.This example demonstrates the correct pattern for modal dialogs using
AttachModal. However, the function name has a typo:ShowModaldialogshould beShowModalDialog(capital 'D' for consistency with Go naming conventions).🔎 Typo fix
-func ShowModaldialog(parent *application.WebviewWindow, title string) { +func ShowModalDialog(parent *application.WebviewWindow, title string) { dialog := app.NewWebviewWindowWithOptions(application.WebviewWindowOptions{ Title: title, Width: 400, Height: 200, }) parent.AttachModal(dialog) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/src/content/docs/features/windows/multiple.mdxdocs/src/content/docs/reference/window.mdxv3/UNRELEASED_CHANGELOG.mdv3/pkg/application/webview_window.gov3/pkg/application/webview_window_android.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_ios.gov3/pkg/application/webview_window_linux.gov3/pkg/application/webview_window_windows.gov3/pkg/application/window.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-17T23:16:11.570Z
Learnt from: Sammy-T
Repo: wailsapp/wails PR: 4570
File: v2/internal/frontend/desktop/linux/window_webkit6.go:97-108
Timestamp: 2025-10-17T23:16:11.570Z
Learning: For webkit_6/GTK4 builds in v2/internal/frontend/desktop/linux/window_webkit6.go, GTK widget creation should not be wrapped in invokeOnMainThread. The activation mechanism (activateWg + onActivate export) already handles thread safety, and additional wrapping would cause issues.
Applied to files:
v3/pkg/application/webview_window_android.gov3/pkg/application/webview_window_windows.gov3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_linux.godocs/src/content/docs/features/windows/multiple.mdx
📚 Learning: 2024-09-20T23:34:29.841Z
Learnt from: nixpare
Repo: wailsapp/wails PR: 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.
Applied to files:
v3/pkg/application/webview_window.godocs/src/content/docs/features/windows/multiple.mdx
📚 Learning: 2025-08-08T09:13:16.916Z
Learnt from: APshenkin
Repo: wailsapp/wails PR: 4480
File: v2/internal/frontend/desktop/darwin/message.h:17-19
Timestamp: 2025-08-08T09:13:16.916Z
Learning: In Wails v2 bindings origin verification, processBindingMessage intentionally has different signatures across platforms: Darwin includes an isMainFrame bool (WKWebKit provides it), Linux uses two params (message, source) as WebKitGTK doesn’t expose main-frame info there, and Windows handles origin checks in Go via WebView2 sender/args without a C bridge. This divergence is acceptable/expected per maintainer (APshenkin).
Applied to files:
v3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_linux.go
📚 Learning: 2025-12-13T19:52:13.812Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 4783
File: v3/pkg/events/events.go:72-100
Timestamp: 2025-12-13T19:52:13.812Z
Learning: In Wails v3, the linux:WindowLoadChanged event was intentionally removed as a breaking change and replaced with four granular WebKit2 load events: linux:WindowLoadStarted, linux:WindowLoadRedirected, linux:WindowLoadCommitted, and linux:WindowLoadFinished. Users should migrate to linux:WindowLoadFinished for detecting when the WebView has finished loading.
Applied to files:
v3/pkg/application/webview_window_linux.go
📚 Learning: 2024-09-30T06:13:46.595Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 3763
File: v3/examples/window/main.go:472-475
Timestamp: 2024-09-30T06:13:46.595Z
Learning: In `v3/examples/window/main.go`, `time.Sleep` is used within a goroutine and does not block the UI thread.
Applied to files:
docs/src/content/docs/features/windows/multiple.mdx
🧬 Code graph analysis (6)
v3/pkg/application/webview_window_android.go (1)
v3/pkg/application/webview_window.go (1)
WebviewWindow(144-178)
v3/pkg/application/webview_window_windows.go (1)
v3/pkg/application/webview_window.go (1)
WebviewWindow(144-178)
v3/pkg/application/window.go (1)
v3/internal/runtime/desktop/@wailsio/runtime/src/index.ts (1)
Window(43-43)
v3/pkg/application/webview_window_ios.go (1)
v3/pkg/application/webview_window.go (1)
WebviewWindow(144-178)
v3/pkg/application/webview_window.go (2)
v3/pkg/application/window.go (1)
Window(9-105)v3/pkg/application/mainthread.go (1)
InvokeSync(23-32)
v3/pkg/application/webview_window_darwin.go (1)
v3/pkg/application/webview_window.go (1)
WebviewWindow(144-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
🔇 Additional comments (3)
v3/pkg/application/webview_window_linux.go (1)
385-387: LGTM!The no-op implementation with clear documentation of lack of support on Linux is appropriate and consistent with other platforms.
v3/pkg/application/webview_window_windows.go (1)
239-241: LGTM!The no-op implementation with clear documentation of lack of support on Windows is appropriate and consistent with other platforms.
v3/pkg/application/webview_window_android.go (1)
299-301: LGTM!The no-op implementation with clear documentation of lack of support on Android is appropriate and consistent with other platforms.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/application/webview_window.go (1)
1251-1265: Consider validating modal window's implementation state.The method correctly validates the parent window's state but doesn't check if the modal window has been initialized or destroyed before passing it to the implementation. Other platform implementations (darwin) do validate this, but consistency at this level would prevent unnecessary cross-thread calls.
🔎 Suggested enhancement
func (w *WebviewWindow) AttachModal(modalWindow Window) { if w.impl == nil || w.isDestroyed() { return } modalWebviewWindow, ok := modalWindow.(*WebviewWindow) - if !ok || modalWebviewWindow == nil { + if !ok || modalWebviewWindow == nil || modalWebviewWindow.impl == nil || modalWebviewWindow.isDestroyed() { return } InvokeSync(func() { w.impl.attachModal(modalWebviewWindow) }) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_linux.gov3/pkg/application/webview_window_windows.gov3/pkg/application/window.go
🚧 Files skipped from review as they are similar to previous changes (3)
- v3/pkg/application/window.go
- v3/pkg/application/webview_window_windows.go
- v3/pkg/application/webview_window_linux.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-29T08:02:15.134Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4839
File: docs/src/content/docs/reference/window.mdx:616-620
Timestamp: 2025-12-29T08:02:15.134Z
Learning: In Wails v3, the correct API for creating windows is `app.Window.New()` and `app.Window.NewWithOptions(...)`, not `app.NewWebviewWindow()` or `app.NewWebviewWindowWithOptions(...)`. The Application struct exposes a Window field of type *WindowManager that provides these methods.
Applied to files:
v3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.go
📚 Learning: 2025-10-17T23:16:11.570Z
Learnt from: Sammy-T
Repo: wailsapp/wails PR: 4570
File: v2/internal/frontend/desktop/linux/window_webkit6.go:97-108
Timestamp: 2025-10-17T23:16:11.570Z
Learning: For webkit_6/GTK4 builds in v2/internal/frontend/desktop/linux/window_webkit6.go, GTK widget creation should not be wrapped in invokeOnMainThread. The activation mechanism (activateWg + onActivate export) already handles thread safety, and additional wrapping would cause issues.
Applied to files:
v3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.go
📚 Learning: 2024-09-20T23:34:29.841Z
Learnt from: nixpare
Repo: wailsapp/wails PR: 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.
Applied to files:
v3/pkg/application/webview_window.go
📚 Learning: 2026-01-04T08:01:00.038Z
Learnt from: symball
Repo: wailsapp/wails PR: 4853
File: v2/internal/system/system.go:128-152
Timestamp: 2026-01-04T08:01:00.038Z
Learning: In v2/internal/system/system.go, shared functions like checkLibrary are defined without build tags but are only invoked from platform-specific files (system_linux.go, system_windows.go, system_darwin.go) that have build constraints. Reviewers should ensure there are no runtime OS checks in system.go and that platform-specific behavior is controlled via build tags. If runtime switches exist, remove them in favor of compile-time platform constraints to reduce overhead and improve correctness.
Applied to files:
v3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.go
📚 Learning: 2025-08-08T09:13:16.916Z
Learnt from: APshenkin
Repo: wailsapp/wails PR: 4480
File: v2/internal/frontend/desktop/darwin/message.h:17-19
Timestamp: 2025-08-08T09:13:16.916Z
Learning: In Wails v2 bindings origin verification, processBindingMessage intentionally has different signatures across platforms: Darwin includes an isMainFrame bool (WKWebKit provides it), Linux uses two params (message, source) as WebKitGTK doesn’t expose main-frame info there, and Windows handles origin checks in Go via WebView2 sender/args without a C bridge. This divergence is acceptable/expected per maintainer (APshenkin).
Applied to files:
v3/pkg/application/webview_window_darwin.go
📚 Learning: 2024-09-21T09:56:48.126Z
Learnt from: nixpare
Repo: wailsapp/wails PR: 3763
File: v3/pkg/application/webview_panel_darwin.go:88-89
Timestamp: 2024-09-21T09:56:48.126Z
Learning: Safety checks for `p.nsPanel` are performed in the `SetFloating` method of `WebviewPanel`, following the `WebviewWindow` and `macosWebviewWindow` implementations and code style.
Applied to files:
v3/pkg/application/webview_window_darwin.go
🧬 Code graph analysis (2)
v3/pkg/application/webview_window.go (2)
v3/pkg/application/window.go (1)
Window(9-105)v3/pkg/application/mainthread.go (1)
InvokeSync(23-32)
v3/pkg/application/webview_window_darwin.go (1)
v3/pkg/application/webview_window.go (1)
WebviewWindow(143-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
🔇 Additional comments (3)
v3/pkg/application/webview_window.go (1)
114-114: LGTM - Interface extension follows established patterns.The new
attachModalmethod correctly follows the internal interface conventions: lowercase name and concrete*WebviewWindowparameter type, consistent with other methods inwebviewWindowImpl.v3/pkg/application/webview_window_darwin.go (2)
333-349: Main thread dispatch correctly implemented.The
dispatch_async(dispatch_get_main_queue(), ...)pattern properly ensures thread safety for thebeginSheet:completionHandler:call. The NULL pointer validation is appropriate.One consideration: the completion handler comment suggests automatic release, but if the modal window needs explicit Go-side cleanup (e.g., updating state, emitting events), this would be the place to add it in a future iteration.
1560-1569: No thread safety issues withw.nsWindowaccess.The call to
attachModalis only ever made from withinInvokeSyncin the publicAttachModalmethod (v3/pkg/application/webview_window.go:1252-1265). There are no direct calls to the privateattachModalmethod outside of this synchronized context, so the hypothetical race condition does not apply to the actual codebase.Likely an incorrect or invalid review comment.
|
This'll take a little time to get to 🙏 |
|
Nice one! Apologies for the delay 🙏 |
* modal window * thread safety * update docs * windows support --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
2 participants
Description
Adds support for modal (sheet) window on macOS. There are docs referencing parent/child relationships. I have been using sheet modals with this functionality exposed through a service and thought it may be a jumping off point to restart parent/child window support if we bring this in upstream. I plan on looking at Windows/Linux support asap.
Added basic Windows support. Deactivates parent window interactivity while modal is open.
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Documentation