Skip to content

fix(v3): implement BrowserWindow.SetScreen to fix -tags server build#5284

Closed
josh-padnick wants to merge 1 commit intowailsapp:masterfrom
josh-padnick:fix/5262-browserwindow-setscreen
Closed

fix(v3): implement BrowserWindow.SetScreen to fix -tags server build#5284
josh-padnick wants to merge 1 commit intowailsapp:masterfrom
josh-padnick:fix/5262-browserwindow-setscreen

Conversation

@josh-padnick
Copy link
Copy Markdown

@josh-padnick josh-padnick commented Apr 29, 2026

Description

PR #5067 added SetScreen(screen *Screen) Window to the Window interface but did not update BrowserWindow (the server-mode adapter in v3/pkg/application/browser_window.go, gated by //go:build server). As a result, *BrowserWindow no longer satisfies Window and any build with -tags server fails to compile, breaking the documented headless / test-automation workflow.

This PR adds a no-op SetScreen implementation on *BrowserWindow that returns the receiver, matching the existing fluent no-op pattern already used by sibling setters like SetTitle, SetMinSize, and SetMaxSize. Moving a browser client to a particular native screen isn't a meaningful operation, so a no-op is the correct behavior here, consistent with how the rest of the server-mode adapter handles native-only window operations.

Fixes #5262.

No new dependencies.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Reproduced the failure on master and verified the fix with:

cd v3/examples/dock
go build -tags server .   # fails before, succeeds after
go build .                # default (non-server) build still succeeds

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.

  • Windows
  • macOS
  • Linux

If you checked Linux, please specify the distro and version.

Test Configuration

 Wails (v3.0.0-alpha.79)  Wails Doctor

# System

┌──────────────────────────────────────────────────┐
| Name          | MacOS                            |
| Version       | 15.7.4                           |
| ID            | 24G517                           |
| Branding      | Sequoia                          |
| Platform      | darwin                           |
| Architecture  | arm64                            |
| Apple Silicon | true                             |
| CPU           | Apple M1 Pro                     |
| CPU 1         | Apple M1 Pro                     |
| CPU 2         | Apple M1 Pro                     |
| GPU           | 16 cores, Metal Support: Metal 3 |
| Memory        | 32 GB                            |
└──────────────────────────────────────────────────┘

# Build Environment

┌────────────────────────────────┐
| Wails CLI    | v3.0.0-alpha.79 |
| Go Version   | go1.25.1        |
| -buildmode   | exe             |
| -compiler    | gc              |
| CGO_CFLAGS   |                 |
| CGO_CPPFLAGS |                 |
| CGO_CXXFLAGS |                 |
| CGO_ENABLED  | 1               |
| CGO_LDFLAGS  |                 |
| GOARCH       | arm64           |
| GOARM64      | v8.0            |
| GOOS         | darwin          |
└────────────────────────────────┘

# Dependencies

┌──────────────────────────────────────────────────────────────────────────────┐
| npm             | 10.9.4                                                     |
| *NSIS           | Not Installed. Install with `brew install makensis`.       |
| Xcode cli tools | 2410                                                       |
| docker          | *Docker version 29.0.1, build eedd969 (daemon not running) |
|                                                                              |
└────────────────────────── * - Optional Dependency ───────────────────────────┘

# Checking for issues

 SUCCESS  No issues found

Checklist:

  • (v2 only) I have updated website/src/pages/changelog.mdx with details of this PR (v3 changelog entries are added automatically)
  • My code follows the general coding style of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (N/A)
  • I have made corresponding changes to the documentation (N/A)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (N/A)
  • New and existing unit tests pass locally with my changes
    • One pre-existing failure in internal/commands (TestGenerateIcon) requires full Xcode.app for actool; unrelated to this change.

Pre-existing failures unrelated to this change

While running the test suite to verify this PR, two pre-existing failures surfaced. Both reproduce on master without the patch applied:

  • TestGenerateIcon in v3/internal/commands — fails with mac asset generation is only supported on macOS despite running on macOS. Appears to require the full Xcode.app bundle (for actool); my local environment only has Xcode CLI tools installed.
  • go vet failure in v3/pkg/application/websocket_server.go:67(*App).error call has arguments but no formatting directives. Only surfaces under -tags server, which suggests another instance of the same server-mode bit-rot that [v3] -tags server build fails (BrowserWindow missing Window.SetScreen) #5262 is fixing. Worth a follow-up PR.

Neither failure is in code touched by this PR, and go test -tags server -vet=off ./... passes cleanly otherwise.

Summary by CodeRabbit

  • New Features
    • Extended browser window API with a new chainable method for screen configuration capabilities.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fd1cfaf0-0e7c-49b1-bf5c-fbdad3d86bfa

📥 Commits

Reviewing files that changed from the base of the PR and between e9082de and e3a5f69.

📒 Files selected for processing (1)
  • v3/pkg/application/browser_window.go

Walkthrough

A single no-op method SetScreen(*Screen) Window is added to the BrowserWindow type to satisfy the Window interface requirement, resolving a compilation failure when building with -tags server.

Changes

Cohort / File(s) Summary
Missing Interface Method
v3/pkg/application/browser_window.go
Added chainable no-op SetScreen method to BrowserWindow to implement the Window interface, following the existing no-op pattern in the file.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested labels

Bug, size:M, lgtm

Suggested reviewers

  • leaanthony
  • atterpac

Poem

🐰 A method was missing, the interface cried,
SetScreen returns self—no secrets to hide,
One little line brings the build back to life,
Server mode blooms without compilation strife! 🌱✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: implementing BrowserWindow.SetScreen to fix server-mode builds with -tags server.
Description check ✅ Passed The description follows the template with all major sections completed: detailed problem explanation, type of change marked, testing documented with reproduction steps, relevant configuration provided, and most checklist items checked.
Linked Issues check ✅ Passed The PR fully addresses issue #5262 by implementing SetScreen on BrowserWindow as a fluent no-op that returns the receiver, exactly matching the suggested fix and resolving the -tags server build failure.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the SetScreen implementation issue; the single-line method addition directly addresses the linked issue with no extraneous modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

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

@leaanthony
Copy link
Copy Markdown
Member

Triaged by Wails PR Reviewer

This PR has been reviewed and accepted. Test sub-issues have been created for all platforms.

Head Ref OID: e3a5f6952ff8834640dd97986927b451684b7521


This comment serves as a signature that this PR has been triaged. Future runs will skip this PR based on the headRefOid.

@leaanthony
Copy link
Copy Markdown
Member

Tested on Linux (Ubuntu 24.04 LTS) — verdict: ✓ PASS

Platform: Ubuntu 24.04.4 LTS / WebKit2GTK v2.50.4 / Go 1.25.0
Commit: e3a5f6952ff8834640dd97986927b451684b7521

Check Result
go build -tags server ./pkg/application/... ✅ PASS
Full ./... build (iOS failure) ⚠️ pre-existing unrelated failure, not caused by this PR
Unit tests (pkg/application core) ✅ 16 pass
Display-dependent tests ⚠️ 5 fail (headless env, pre-existing)
Visual smoke (Dialogs Demo) ✅ window rendered correctly on GNOME

The -tags server build now compiles cleanly on Linux with the SetScreen stub in place. Safe to merge on Linux.

@leaanthony
Copy link
Copy Markdown
Member

My bad! I accidentally fixed this before seeing this 🙏 Thanks for bringing it up.

@leaanthony leaanthony closed this May 2, 2026
@josh-padnick
Copy link
Copy Markdown
Author

No worries! Glad it got fixed, and thanks for the prompt response!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v3] -tags server build fails (BrowserWindow missing Window.SetScreen)

2 participants