feat(v3,macos): Add NSWindow tabbing mode configuration for macOS (#4964) #4966
Conversation
- Add MacWindowTabbingMode type and constants to webview_window_options.go - Add TabbingMode field to MacWindow struct - Implement windowSetTabbingMode C function with macOS 10.12+ availability check - Apply tabbing mode in macosWebviewWindow.run() method - Add test for MacWindowTabbingMode constants
|
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:
WalkthroughAdds a macOS NSWindow tabbing option: new MacWindowTabbingMode enum and MacWindow.TabbingMode field, macOS C/Go binding to set NSWindowTabbingMode during window creation, docs updates, tests, and a full mac-window-tabs example with multi-platform build/config files. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant WindowService
participant AppRuntime
participant macOSNative
Frontend->>WindowService: invoke OpenTabbedWindow/OpenNonTabbedWindow
WindowService->>AppRuntime: app.Window.NewWithOptions(opts{Mac.TabbingMode})
AppRuntime->>macOSNative: create NSWindow
AppRuntime->>macOSNative: call windowSetTabbingMode(nsWindow, mode)
macOSNative-->>AppRuntime: tabbing mode applied
AppRuntime-->>WindowService: window created
WindowService-->>Frontend: window opened
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Eriyc
force-pushed
the
v3-alpha-feature/mac-window-tabbing
branch
from
3c14c3e to
210c260
Compare
|
Thanks for this. Please can you add an example in the v3 examples directory? And I agree that it should be disabled by default. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In
`@v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/MainActivity.java`:
- Around line 114-122: The error response returned via the WebResourceResponse
constructor currently uses an empty header map; update the error path that
returns new WebResourceResponse(...) so it includes the same CORS header(s) as
the success path (e.g., add "Access-Control-Allow-Origin":"*" to the headers map
used for the 500/internal error response) to ensure CORS headers are present on
error responses for the /wails/ API calls.
In
`@v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsBridge.java`:
- Around line 155-158: The catch block in WailsBridge.java currently returns
"{\"error\":\"" + e.getMessage() + "\"}" which can produce invalid JSON when
e.getMessage() contains quotes, backslashes or newlines; change the return to
produce a properly escaped JSON string by using a JSON builder/escaping utility
(e.g., org.json.JSONObject.put("error", e.getMessage()).toString() or
JSONObject.quote(e.getMessage())) instead of string concatenation—update the
catch(Exception e) block that calls Log.e(TAG, "Error handling message", e) to
build and return a valid JSON object with the escaped error message.
In `@v3/examples/mac-window-tabs/build/config.yml`:
- Line 62: The comment in config.yml contains a broken placeholder URL
("https://v3.wails.io/noit/done/yet"); update that comment by replacing the
placeholder with the correct Wails v3 file associations/documentation URL or
remove the comment entirely so users aren't misled—search for the exact string
"https://v3.wails.io/noit/done/yet" in config.yml and correct or delete it.
In `@v3/examples/mac-window-tabs/build/ios/Info.plist`:
- Around line 25-29: Remove the unnecessary 32-bit capability from the
UIRequiredDeviceCapabilities array: because MinimumOSVersion is set to 15.0,
delete the "armv7" entry from the UIRequiredDeviceCapabilities key so only
64-bit requirements (e.g., "arm64") remain; update the Info.plist’s
UIRequiredDeviceCapabilities array to no longer list "armv7" to avoid misleading
capability conjunctions during App Store submission.
In `@v3/examples/mac-window-tabs/build/linux/appimage/build.sh`:
- Around line 33-34: The mv invocation is using a quoted glob so the asterisk
won't expand (mv "${APP_NAME}*.AppImage"), causing "No such file" errors; update
the rename to allow shell globbing by removing the quotes around the pattern or
by expanding the glob into a variable before mv (e.g., use mv
${APP_NAME}*.AppImage "${APP_NAME}.AppImage" or assign matched filename from a
glob match into a variable and then mv that variable to "${APP_NAME}.AppImage");
ensure you reference the APP_NAME variable and the mv command in the same script
blocks where the current pattern appears so the change is applied to all
affected AppImage build scripts.
In `@v3/examples/mac-window-tabs/build/linux/nfpm/nfpm.yaml`:
- Around line 34-38: The inline comment "(WebKit 4.0)" is misleading given the
RPM dependency list uses webkit2gtk4.1; update the comment near the rpm:
depends: block to reflect WebKit 4.1 (e.g., change "(WebKit 4.0)" to "(WebKit
4.1)" or remove the version specifier) so the comment matches the dependency
`webkit2gtk4.1`.
In `@v3/examples/mac-window-tabs/frontend/index.html`:
- Line 5: The link tag declaring the favicon has a MIME/type mismatch: it sets
type="image/svg+xml" while href points to "/wails.png"; update the link element
(the favicon <link> with href="/wails.png") to use the correct MIME type (e.g.,
type="image/png") or remove the type attribute so the browser infers it from the
.png file.
In `@v3/examples/mac-window-tabs/go.mod`:
- Line 1: The go.mod currently uses the placeholder module name "changeme";
update the module declaration in go.mod to a proper module path such as
"mac-window-tabs" or the full repository path
"github.com/wailsapp/wails/v3/examples/mac-window-tabs" by replacing the "module
changeme" line with the chosen descriptive module name so builds and imports
resolve correctly.
In `@v3/examples/mac-window-tabs/main.go`:
- Around line 4-5: The file currently imports embed twice (both "embed" and _
"embed"); remove the redundant blank import `_ "embed"` and keep the named
import `"embed"` used by the embed.FS type (e.g., the embedded filesystem
variable using embed.FS). Ensure no other code relies on the blank import and
that the named import remains so embed.FS compiles.
- Around line 37-50: The bug is that NewWindowService(app) is called while app
is still nil inside application.New(...), so WindowService stores a nil app and
openWindow silently returns; to fix, create the app first and only then
add/register the WindowService with the real app reference (or construct
WindowService with a nil app then call a setter to set the app after
application.New returns). Concretely, call application.New(...) with other
services (e.g., application.NewService(&GreetService{})), assign to the variable
app, then register the window service via
app.RegisterService(application.NewService(NewWindowService(app))) or call
windowService.SetApp(app) so WindowService and its openWindow method hold the
valid app.
🧹 Nitpick comments (13)
v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsBridge.java (1)
26-29: Unused fields:context,callbackIdGenerator,pendingAssetCallbacks,pendingMessageCallbacks.These four fields are declared and initialized but never read or used anywhere in this class. They add unnecessary memory overhead and cognitive noise. Consider removing them if they aren't needed, or adding a TODO if they're planned for future use.
♻️ Proposed cleanup
public class WailsBridge { private static final String TAG = "WailsBridge"; static { // Load the native Go library System.loadLibrary("wails"); } - private final Context context; - private final AtomicInteger callbackIdGenerator = new AtomicInteger(0); - private final ConcurrentHashMap<Integer, AssetCallback> pendingAssetCallbacks = new ConcurrentHashMap<>(); - private final ConcurrentHashMap<Integer, MessageCallback> pendingMessageCallbacks = new ConcurrentHashMap<>(); private WebView webView; private volatile boolean initialized = false;v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsJSBridge.java (1)
52-60: Unbounded thread creation for each async call.
new Thread(...).start()on everyinvokeAsynccall can create an unbounded number of threads under load. For this example app it's fine, but if this template is reused in production, consider using anExecutorServicewith a bounded thread pool.♻️ Suggested improvement
+import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + public class WailsJSBridge { private static final String TAG = "WailsJSBridge"; + private static final ExecutorService executor = Executors.newCachedThreadPool(); ... `@JavascriptInterface` public void invokeAsync(final String callbackId, final String message) { Log.d(TAG, "InvokeAsync called: " + message); - new Thread(() -> { + executor.submit(() -> { try { String response = bridge.handleMessage(message); sendCallback(callbackId, response, null); } catch (Exception e) { Log.e(TAG, "Error in async invoke", e); sendCallback(callbackId, null, e.getMessage()); } - }).start(); + }); }v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsPathHandler.java (1)
71-117: Dead code:getMimeTypeis never called.The
handle()method (line 49) delegates MIME type resolution tobridge.getAssetMimeType(path), making this private method unreachable. Remove it to avoid confusion, or switchhandle()to use it if the intent was to avoid the bridge call for MIME types.♻️ Option A: Remove dead method
- /** - * Determine MIME type from file extension - */ - private String getMimeType(String path) { - ... - }♻️ Option B: Use local method instead of bridge call
- String mimeType = bridge.getAssetMimeType(path); + String mimeType = getMimeType(path);v3/examples/mac-window-tabs/frontend/public/style.css (1)
56-59: Duplicate.resultselector.The
.resultclass is defined twice (lines 56–59 and 103–108). The second block supersedes the first. Consider removing the first occurrence to avoid confusion.Also applies to: 103-108
v3/examples/mac-window-tabs/build/ios/Info.dev.plist (1)
9-12: Template placeholder values not updated for this example.
CFBundleName,CFBundleDisplayName,NSHumanReadableCopyright, andCFBundleGetInfoStringstill carry generic template values ("My Product (Dev)", "My Company", "This is a comment"). Consider updating them to match the example name (mac-window-tabs) for consistency and discoverability.v3/examples/mac-window-tabs/build/docker/Dockerfile.cross (1)
34-38: Third-party macOS SDK download — supply chain risk.The macOS SDK is downloaded from
joseluisq/macosx-sdkson GitHub. For an example this is acceptable, but be aware this is an unofficial source. Consider adding a comment noting this dependency and its implications.v3/examples/mac-window-tabs/build/docker/Dockerfile.server (1)
5-5: Consider pinning the Go version in the builder stage.
golang:alpinewill float to whatever the latest Go version is. For reproducibility, considergolang:1.25-alpineto match the project's Go version requirement. Not critical for an example.Proposed fix
-FROM golang:alpine AS builder +FROM golang:1.25-alpine AS builderBased on learnings: Wails v3 will require Go 1.25+ when released.
v3/examples/mac-window-tabs/main.go (1)
65-107: Consider extracting the repeated IIFE pattern into a helper.The same closure pattern for cloning
macBaseand settingTabbingModeis repeated four times. A small helper would reduce noise.Example helper
+ withTabbing := func(mode application.MacWindowTabbingMode) application.MacWindow { + mac := macBase + mac.TabbingMode = mode + return mac + } + app.Window.NewWithOptions(application.WebviewWindowOptions{ Title: "Tabbing Enabled", - Mac: func() application.MacWindow { - mac := macBase - mac.TabbingMode = application.MacWindowTabbingModePreferred - return mac - }(), + Mac: withTabbing(application.MacWindowTabbingModePreferred), BackgroundColour: windowBackground, URL: "/", })v3/examples/mac-window-tabs/build/linux/appimage/build.sh (1)
17-31: Consider deduplicating the linuxdeploy download/run logic.The two branches differ only by the architecture string (
x86_64vsaarch64). A variable would remove the duplication and reduce future maintenance if the URL or invocation changes.♻️ Suggested refactor
-if [[ $(uname -m) == *x86_64* ]]; then - # Download linuxdeploy and make it executable - wget -q -4 -N https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage - chmod +x linuxdeploy-x86_64.AppImage - - # Run linuxdeploy to bundle the application - ./linuxdeploy-x86_64.AppImage --appdir "${APP_DIR}" --output appimage -else - # Download linuxdeploy and make it executable (arm64) - wget -q -4 -N https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-aarch64.AppImage - chmod +x linuxdeploy-aarch64.AppImage - - # Run linuxdeploy to bundle the application (arm64) - ./linuxdeploy-aarch64.AppImage --appdir "${APP_DIR}" --output appimage -fi +if [[ $(uname -m) == *x86_64* ]]; then + DEPLOY_ARCH="x86_64" +else + DEPLOY_ARCH="aarch64" +fi + +wget -q -4 -N "https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-${DEPLOY_ARCH}.AppImage" +chmod +x "linuxdeploy-${DEPLOY_ARCH}.AppImage" +./linuxdeploy-${DEPLOY_ARCH}.AppImage --appdir "${APP_DIR}" --output appimagev3/examples/mac-window-tabs/build/linux/nfpm/nfpm.yaml (1)
27-30: Consider OR dependency for broader Debian/Ubuntu compatibility.The default depends only lists
libwebkit2gtk-4.1-0, which works on Debian 12+ but not on Ubuntu 22.04 (which useslibwebkit2gtk-4.0-37). An OR dependency would broaden compatibility. This is just an example, so low priority.Proposed fix
depends: - libgtk-3-0 - - libwebkit2gtk-4.1-0 + - libwebkit2gtk-4.1-0 | libwebkit2gtk-4.0-37Based on learnings: "For Wails v3 Linux nfpm template, Debian/Ubuntu WebKitGTK runtime package names differ: Debian 12 (Bookworm) uses libwebkit2gtk-4.1-0 while Ubuntu 22.04 (Jammy) uses libwebkit2gtk-4.0-37. Prefer an OR dependency."
v3/examples/mac-window-tabs/build/config.yml (1)
7-14: Example metadata should reflect the mac-window-tabs example, not generic placeholders.Since this is an example in the
v3/examples/directory that users will reference, consider updating the placeholder values to be specific to the tabbing demo (e.g.,productName: "Mac Window Tabs",description: "Demonstrates NSWindow tabbing mode configuration"). Generic values like"A program that does X"reduce the example's usefulness.v3/examples/mac-window-tabs/build/Taskfile.yml (2)
46-79: Puppertino vendor task: in-placesedportability and mutatingindex.htmlat build time.The
sed -E -i''on line 78 and theawkreplacement on lines 73-75 modifyfrontend/index.htmlin-place during a build task. This means running the task twice could produce unexpected results if the replacements aren't idempotent — thegrep -qguard on line 71 protects theawkinsertion, but thesedon line 78 has no guard and will re-run on every invocation (harmless if the class is already replaced, but worth noting).Also,
set -euo pipefail(line 54) is not POSIXsh-compatible on older systems. Since this task can run on Linux where/bin/shmay bedash, consider either specifyingbashas the shell or droppingpipefail.
238-242:PIPESTATUSis bash-specific — will fail if Taskfile uses POSIXsh.
${PIPESTATUS[0]}on line 240 is a bashism. Taskfile defaults tosh -c, and on macOS/bin/shmay bezsh(which does supportPIPESTATUSin recent versions), so this likely works in the iOS-only context. However, it's fragile.A more robust pattern is to avoid piping to
xcprettywith|| trueand instead check forxcprettyavailability first:Proposed alternative
- xcodebuild "${XCB_ARGS[@]}" build | xcpretty || true - # If xcpretty isn't installed, run without it - if [ "${PIPESTATUS[0]}" -ne 0 ]; then - xcodebuild "${XCB_ARGS[@]}" build - fi + if command -v xcpretty >/dev/null 2>&1; then + xcodebuild "${XCB_ARGS[@]}" build | xcpretty + else + xcodebuild "${XCB_ARGS[@]}" build + fi
v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/MainActivity.java
Show resolved
Hide resolved
v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsBridge.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@v3/examples/mac-window-tabs/frontend/src/main.js`:
- Around line 7-16: The doGreet function currently has an empty try block and
never uses the read name; replace the no-op with a real greeting call (or remove
the try/catch). Specifically, inside window.doGreet call the binding method
(e.g. GreetService.Greet) with the retrieved name, await the result, and update
the DOM (e.g. set innerText on a result element) and keep the catch to log
errors; if you prefer simpler cleanup, remove the try/catch and directly perform
the DOM update using the name. Ensure you reference the existing doGreet
function and the document.getElementById('name') value when wiring the call so
the example is functional.
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 1325-1330: The current enum collides with Go's zero value
(MacWindowTabbingModeAutomatic == 0) causing Automatic to be treated as "unset"
and always overridden; fix by introducing an explicit zero-value sentinel and
shifting the iota values (e.g., add MacWindowTabbingModeDefault = 0, then
MacWindowTabbingModeAutomatic = 1, MacWindowTabbingModePreferred = 2,
MacWindowTabbingModeDisallowed = 3), update the enum definition and any checks
that compare macOptions.TabbingMode (including the block that sets Disallowed
and the call to w.setTabbingMode) so that the zero value means "not set" and the
existing default-to-disallowed behavior remains; alternatively, if you prefer
minimal call-site changes, change macOptions.TabbingMode to a pointer
(*MacWindowTabbingMode) so nil represents unset and adjust the check and
w.setTabbingMode invocation accordingly.
In `@v3/pkg/application/webview_window_options_test.go`:
- Around line 264-280: The test TestMacWindowTabbingMode_Constants is
tautological because it compares each constant to itself; change the test to
assert against concrete underlying integer values instead of the same constants.
Update the test cases for MacWindowTabbingModeAutomatic,
MacWindowTabbingModePreferred and MacWindowTabbingModeDisallowed so the expected
field is the known integer value (or a MacWindowTabbingMode cast of that
integer) that each constant should represent, then run the assertions comparing
tt.value to that explicit expected integer value.
🧹 Nitpick comments (2)
v3/pkg/application/webview_window_darwin.go (1)
251-257: Consider castingmodetoNSWindowTabbingModefor type safety in the Objective-C shim.Passing a raw
inttosetTabbingMode:works due to implicit conversion, but an explicit cast documents intent and avoids compiler warnings with stricter settings.Suggested fix
void windowSetTabbingMode(void* nsWindow, int mode) { WebviewWindow* window = (WebviewWindow*)nsWindow; if (`@available`(macOS 10.12, *)) { - [window setTabbingMode:mode]; + [window setTabbingMode:(NSWindowTabbingMode)mode]; } }v3/examples/mac-window-tabs/windowservice.go (1)
10-14:GreetServiceis registered but never called from the frontend.The corresponding
doGreetinmain.jshas an emptytryblock (flagged separately). If this service is intentional for the example, the frontend should be wired up; otherwise, consider removing it to keep the example focused on window tabbing.
Eriyc
force-pushed
the
v3-alpha-feature/mac-window-tabbing
branch
from
99b8f5b to
672c02d
Compare
Eriyc
left a comment
Eriyc
left a comment
There was a problem hiding this comment.
CodeRabbit cleverly pointed out that since the options range from 0 to 2, there's no way to default to Disallowed without completely disabling Automatic. Ergo a string is the best choice in this case.
Refer to https://developer.apple.com/documentation/appkit/nswindow/tabbingmode-swift.enum
…wed, use iota constants
Eriyc
force-pushed
the
v3-alpha-feature/mac-window-tabbing
branch
from
672c02d to
20c715a
Compare
|
Cleaned up the git history — the branch had a messy back-and-forth (iota → string → back to iota) across several fix-up commits. Squashed them all into a single commit on top of the original feature commit. Ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@docs/src/content/docs/features/windows/options.mdx`:
- Around line 852-859: Update the TabbbingMode docs to fix two inaccuracies:
remove the "empty string" wording and state the actual default behavior—when
MacWindowTabbingMode is not set (zero value MacWindowTabbingModeDefault) tabbing
is disallowed; clarify that MacWindowTabbingModeAutomatic lets the system decide
and MacWindowTabbingModeDisallowed disables tabbing; align the text with the
implementation in webview_window_darwin.go (which treats the zero-value sentinel
as disallowed) and ensure MacWindowTabbingModeDefault is mentioned as the
internal zero-value sentinel.
In
`@v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsJSBridge.java`:
- Around line 70-89: The log method in WailsJSBridge calls level.toLowerCase()
which can throw an NPE when JS passes null; update WailsJSBridge.log to guard
against null by normalizing level to a safe default (e.g., treat null or empty
as "debug" or "verbose") before calling toLowerCase(), then proceed with the
switch; reference the log(...) method and the level.toLowerCase() call when
making the change.
In `@v3/examples/mac-window-tabs/build/android/gradlew.bat`:
- Around line 1-93: This macOS-only example accidentally includes Android build
scaffolding; remove the entire build/android directory (including gradlew.bat)
from the mac-window-tabs example so it no longer contains Android/Gradle wrapper
files; ensure no leftover Android artifacts remain (gradlew.bat,
gradle-wrapper.jar, gradle wrapper properties) and update any example metadata
or scripts referencing build/android to point only to the macOS-specific assets.
In `@v3/examples/mac-window-tabs/build/docker/Dockerfile.cross`:
- Around line 1-2: Add the BuildKit syntax directive to the top of the
Dockerfile so the heredoc COPY <<'ZIGWRAP' constructs are supported;
specifically insert the line "# syntax=docker/dockerfile:1" as the first line of
the Dockerfile to satisfy BuildKit requirements and resolve the Hadolint DL1000
warning for the COPY <<'ZIGWRAP' heredocs (seen around the COPY <<'ZIGWRAP'
occurrences).
- Around line 144-148: The linux-arm64|linux-aarch64) case in Dockerfile.cross
sets CC=gcc which yields host-arch binaries when cross-compiling; either switch
this branch to set a proper cross-compiler (e.g., aarch64-linux-gnu-gcc or a
Zig-based cross-wrapper like the Darwin/Windows cases) instead of gcc, or add a
guard in the build script (build.sh) that detects mismatched host vs target arch
and fails if CC is gcc during cross-arch builds; update the Dockerfile.cross
case and/or add the runtime check in build.sh to prevent silent CGO linking to
the host libc.
In `@v3/examples/mac-window-tabs/build/docker/Dockerfile.server`:
- Around line 15-16: The current RUN sed line that removes replace directives
only handles single-line "replace" entries and leaves multi-line replace blocks
in go.mod; update the Dockerfile.server to replace that RUN sed invocation so it
deletes both single-line "replace ..." and multi-line "replace ( ... )" blocks
(e.g., use a tool like perl/awk that can operate across lines to strip
/^replace\b.*$/ and entire /^replace\s*\(/ ... /^\)/ ranges in go.mod), ensuring
the change targets the existing RUN sed -i '/^replace/d' go.mod || true entry
and produces a go.mod without any leftover replace lines before running go mod
tidy.
🧹 Nitpick comments (12)
v3/examples/mac-window-tabs/frontend/public/style.css (2)
56-59: Duplicate.resultselector — consolidate into a single rule.
.resultis declared twice (lines 56–59 and 103–108). The first block is entirely shadowed by the second. Merge them into one rule to avoid confusion.♻️ Suggested consolidation
Remove lines 56–59 and keep only the second, more complete block:
-.result { - height: 20px; - line-height: 20px; -} - body {Also applies to: 103-108
22-28: Font-weight mismatch:Inter-Mediumis typically weight 500, not 400.The
@font-faceloadsInter-Medium.ttfbut registers it asfont-weight: 400. Medium weight is conventionally 500. Since:rootsetsfont-weight: 400, the browser may not match this face correctly, or the rendered weight won't match the filename's intent.♻️ Suggested fix
`@font-face` { font-family: "Inter"; font-style: normal; - font-weight: 400; + font-weight: 500; src: local(""), url("./Inter-Medium.ttf") format("truetype"); }If the intent is to use this as the regular (400) weight, consider shipping
Inter-Regular.ttfinstead.docs/src/content/docs/features/windows/options.mdx (1)
779-779: Example showsMacWindowTabbingModeAutomaticin the general Mac options showcase.This is fine as an explicit configuration example, but given that the PR objective (and maintainer agreement) is that tabbing should be disabled by default, consider using
MacWindowTabbingModeDisallowedhere instead — or omitting it entirely so the default (Disallowed) takes effect. ShowingAutomaticin the primary showcase example may mislead users into thinking that's the default.v3/pkg/application/webview_window_darwin.go (1)
1220-1227: Add a defensive guard against theMacWindowTabbingModeDefaultsentinel value.If
MacWindowTabbingModeDefault(0) is ever passed to this method,mode-1evaluates to-1, which would be an invalidNSWindowTabbingModevalue — potentially causing undefined behavior in AppKit.Currently safe because
run()remaps Default → Disallowed before calling this, but a guard here would prevent future misuse.🛡️ Proposed defensive guard
func (w *macosWebviewWindow) setTabbingMode(mode MacWindowTabbingMode) { + if mode == MacWindowTabbingModeDefault { + return + } // Our iota values are offset by 1 from NSWindowTabbingMode: // MacWindowTabbingModeAutomatic(1) -> NSWindowTabbingModeAutomatic(0) // MacWindowTabbingModePreferred(2) -> NSWindowTabbingModePreferred(1) // MacWindowTabbingModeDisallowed(3) -> NSWindowTabbingModeDisallowed(2) // https://developer.apple.com/documentation/appkit/nswindow/tabbingmode-swift.enum C.windowSetTabbingMode(w.nsWindow, C.int(mode-1)) }v3/examples/mac-window-tabs/build/ios/Info.dev.plist (1)
1-61: Template placeholders remain in the iOS dev plist.This is boilerplate scaffolding, but a few placeholder strings could be updated for the example:
- Line 10/12:
"My Product (Dev)"→ something like"Mac Window Tabs (Dev)"- Line 55:
"© 2026, My Company"→ consider the Wails project copyright- Line 59:
"This is a comment"forCFBundleGetInfoString— either update or removeThese are minor nits for an example directory.
v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsJSBridge.java (1)
48-61: Consider using a thread pool instead of spawning a thread per async call.Each
invokeAsynccall creates a newThread. Under burst traffic from JS, this can exhaust system resources. A sharedExecutorService(e.g.,Executors.newCachedThreadPool()) would be more efficient.Acceptable for an example app, but worth noting if this code is used as a template.
v3/examples/mac-window-tabs/main.go (1)
64-106: Consider simplifying the repeated IIFE pattern.The four window-creation blocks each use an identical IIFE to clone
macBaseand setTabbingMode. A small helper would reduce repetition and improve readability of this example:♻️ Optional helper
+func macWithTabbing(base application.MacWindow, mode application.MacWindowTabbingMode) application.MacWindow { + base.TabbingMode = mode + return base +}Then each window becomes:
app.Window.NewWithOptions(application.WebviewWindowOptions{ Title: "Tabbing Enabled", Mac: macWithTabbing(macBase, application.MacWindowTabbingModePreferred), BackgroundColour: windowBackground, URL: "/", })v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsBridge.java (2)
27-29: Unused fields:callbackIdGenerator,pendingAssetCallbacks,pendingMessageCallbacks.These are declared but never referenced. If they're placeholders for future async support, consider adding a brief comment or removing them to avoid dead code.
191-195:eventDatais injected raw into JS — ensure callers always pass valid JSON.
escapeJsStringis applied toeventNamebuteventDatais inserted verbatim. IfeventDataever contains a value that isn't valid JS (e.g., an unquoted string or a string with unescaped characters), this would produce a JS syntax error or potential injection. This is safe as long as the Go side always serializeseventDataas JSON, but worth a defensive comment or guard.v3/examples/mac-window-tabs/build/docker/Dockerfile.server (1)
25-41: Run as non-root using the distrolessnonrootuser.The
gcr.io/distroless/static-debian12image ships with a built-innonrootuser (UID 65532). Running as root in a container is flagged by Trivy (DS002) and is a security best practice to address.Proposed fix
FROM gcr.io/distroless/static-debian12 +USER nonroot:nonroot + # Copy the binary COPY --from=builder /app/server /serverv3/examples/mac-window-tabs/build/docker/Dockerfile.cross (2)
181-181: Unquoted command substitution is fragile.
$(basename $(pwd))should be double-quoted to prevent word splitting. While/appis safe, this is a correctness nit.Proposed fix
-APP=${APP_NAME:-$(basename $(pwd))} +APP=${APP_NAME:-"$(basename "$(pwd)")"}
46-123: Four nearly identical Zig CC wrapper scripts — consider a parameterized template.The wrapper scripts for darwin-arm64, darwin-amd64, windows-amd64, and windows-arm64 share the same arg-filtering loop; only the
exec zig cctarget and flags differ. A single parameterized wrapper script (e.g.,zcc-wrapperthat reads target info from its own filename or an env var) would reduce duplication and maintenance burden.This is fine to defer; just noting the opportunity.
v3/examples/mac-window-tabs/build/android/app/src/main/java/com/wails/app/WailsJSBridge.java
Show resolved
Hide resolved
…c/wails-fork into v3-alpha-feature/mac-window-tabbing
|
@leaanthony @almas-x ready for review |
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
Code Review: PR #4966 — feat(v3,macos): Add NSWindow tabbing mode configuration
Summary
This PR adds the ability to configure NSWindow.tabbingMode on macOS 10.12+, allowing Wails v3 users to control whether their windows can be grouped as tabs. It introduces a new MacWindowTabbingMode type, a TabbingMode field on MacWindow, the C/Go bridging code, an example app, documentation updates, and a changelog entry.
Verdict: Approve with minor suggestions. The core implementation is clean, correct, and follows established patterns well. The issues are minor.
Findings
CRITICAL — None
HIGH — 1 Issue
H1. Missing #if MAC_OS_X_VERSION_MAX_ALLOWED compile guard in C function
v3/pkg/application/webview_window_darwin.go (CGo block, new windowSetTabbingMode function)
void windowSetTabbingMode(void* nsWindow, int mode) {
WebviewWindow* window = (WebviewWindow*)nsWindow;
if (@available(macOS 10.12, *)) {
[window setTabbingMode:mode];
}
}The runtime @available check is present, but it's missing the compile-time #if MAC_OS_X_VERSION_MAX_ALLOWED >= 101200 guard. Other functions in this file (e.g., windowSetToolbarStyle, fraud warnings) use both the compile-time #if and the runtime @available check together. While the project's minimum deployment target is 10.13 (from -mmacosx-version-min=10.13 in CFLAGS), so 10.12 APIs should always be available, adding the guard would be consistent with existing patterns and defensive against future changes.
Recommendation: Add the compile-time guard for consistency:
#if MAC_OS_X_VERSION_MAX_ALLOWED >= 101200
void windowSetTabbingMode(void* nsWindow, int mode) {
WebviewWindow* window = (WebviewWindow*)nsWindow;
if (@available(macOS 10.12, *)) {
[window setTabbingMode:mode];
}
}
#endifSeverity: Low-High — Functionally correct today but inconsistent with codebase conventions.
MEDIUM — 2 Issues
M1. The mode-1 offset arithmetic in setTabbingMode is fragile
v3/pkg/application/webview_window_darwin.go:1220-1226
func (w *macosWebviewWindow) setTabbingMode(mode MacWindowTabbingMode) {
C.windowSetTabbingMode(w.nsWindow, C.int(mode-1))
}The offset-by-1 design (MacWindowTabbingModeAutomatic=1 maps to NSWindowTabbingModeAutomatic=0) is documented in comments, which is good. However, if MacWindowTabbingModeDefault (0) ever reaches this function, mode-1 becomes -1, which is an invalid NSWindowTabbingMode value. The run() method currently guards against this:
if macOptions.TabbingMode == MacWindowTabbingModeDefault {
macOptions.TabbingMode = MacWindowTabbingModeDisallowed
}
w.setTabbingMode(macOptions.TabbingMode)But setTabbingMode is a method on the struct and could be called from elsewhere. A defensive check would be safer:
func (w *macosWebviewWindow) setTabbingMode(mode MacWindowTabbingMode) {
if mode == MacWindowTabbingModeDefault {
mode = MacWindowTabbingModeDisallowed
}
C.windowSetTabbingMode(w.nsWindow, C.int(mode-1))
}M2. Massive example boilerplate (5,700+ lines added)
95 files changed, 5,735 additions — the vast majority is scaffolding (build/android/, build/ios/, build/windows/, build/docker/, etc.) copied from the default template. This is consistent with how other examples in the repo are structured, but it inflates the PR significantly. Not blocking, but worth noting — the example could potentially reference shared build scaffolding in the future.
LOW — 3 Issues
L1. UNRELEASED_CHANGELOG.md formatting
The added line has no blank line between it and the ## Changed section header:
## Added
- Add macOS window option to configure NSWindow tabbing behavior (#4966)
## ChangedThe original had a blank line. Minor formatting inconsistency.
L2. Documentation says MacWindowTabbingModeDefault "defaults to disallowing tabbing" — could be clearer
The doc says it's a "zero-value sentinel" that "at runtime, defaults to disallowing tabbing." This is accurate but could surprise users who expect the Go zero-value to mean "system default." Consider whether the doc should more explicitly say: "If you don't set TabbingMode, tabbing is disabled. Use MacWindowTabbingModeAutomatic to let macOS decide."
L3. Test only verifies iota values
TestMacWindowTabbingMode_Constants only checks that the constants have the expected integer values. It doesn't test the offset-by-1 mapping or the default-resolution behavior. This is consistent with other tests in the file (they all just verify constant values), so it's fine for now.
Architecture Assessment
The offset-by-1 iota pattern is a well-known Go idiom for distinguishing "not set" from "first enum value." NSWindowTabbingMode uses 0/1/2, so the PR uses 1/2/3 with 0 as sentinel. This trades a tiny bit of arithmetic complexity for a clean zero-value API. The alternative would be a pointer type (*MacWindowTabbingMode) or a separate TabbingModeSet bool field, both of which would be more verbose for users.
Positive Observations
- Follows existing patterns exactly (
CollectionBehaviorwas added recently with the same structure) - Good use of
@availablefor runtime version checking - Clear documentation with examples
- Sensible default (disallowed) — prevents surprise tab-merging behavior
- The example app demonstrates multiple tabbing modes side-by-side
Recommendation
Approve — the core feature is well-implemented. The #if guard (H1) and defensive check in setTabbingMode (M1) are worth requesting as minor changes before merge.
3 participants
Description
Adds support for configuring NSWindow tabbing behavior on macOS 10.12+. macOS allows windows to be grouped as tabs; this feature was not configurable in Wails v3.
Fixes #4964 (issue)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Made the change to a fork, replaced wails3 with the fork -> program ran as expected.
Test Configuration
Not applicable - feature adds new configuration option following established patterns.
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Documentation