Repository navigation
v3: replace 11 direct dependencies with internal stdlib implementations - #5508
Merged
Merged
Conversation
Contributor
|
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:
WalkthroughIntroduces internal replacements for multiple external utilities, migrates imports across the codebase to these internal packages, standardizes error wrapping, updates system-tray label parsing, and adds comprehensive tests and go.mod updates. ChangesInternal Utility Package Creation
Import Migration to Internal Packages
Behavioral Changes & Error Handling
Dependency Management
Estimated code review effort: Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces the v3 module’s direct Go dependency surface by replacing several third-party libraries with small internal stdlib-backed implementations, while also adding first-class support for Garble-based obfuscated builds (stable binding method IDs, Taskfile plumbing, and docs). It additionally makes macOS system-tray ANSI label parsing pluggable to avoid hard-wiring an ANSI parsing dependency.
Changes:
- Replaced several direct deps (e.g.
lo,uuid,debounce,browser,tint,semver,gosod,shlex,pkg/errors) with internal packages / stdlib equivalents and updated call sites. - Added obfuscation support: stable binding method ID registration + generator emission of
wails_obfuscated.gen.go, pluswails3 build --obfuscated --garbleargswiring into platform Taskfiles and the cross-build Docker image. - Added/updated docs and changelog entries for obfuscated builds and related CLI flags.
Reviewed changes
Copilot reviewed 68 out of 69 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/UNRELEASED_CHANGELOG.md | Adds unreleased changelog entry describing Garble obfuscation support. |
| v3/tasks/sed/sed.go | Switches from external lo to internal/lo. |
| v3/pkg/services/sqlite/sqlite.go | Replaces pkg/errors wrapping with stdlib fmt.Errorf/errors. |
| v3/pkg/services/notifications/notifications_windows.go | Switches UUID generation to internal/uuid. |
| v3/pkg/services/kvstore/kvstore.go | Replaces pkg/errors wrapping with stdlib fmt.Errorf. |
| v3/pkg/doctor-ng/platform_windows.go | Switches from external lo to internal/lo. |
| v3/pkg/doctor-ng/platform_darwin.go | Switches from external lo to internal/lo. |
| v3/pkg/doctor-ng/doctor.go | Switches from external lo to internal/lo. |
| v3/pkg/application/webview_window.go | Replaces u with internal/optional and updates Enabled/Disabled aliases. |
| v3/pkg/application/webview_window_windows.go | Replaces external debounce with internal/debounce. |
| v3/pkg/application/webview_window_options.go | Replaces u.Bool with optional.Bool in options structs. |
| v3/pkg/application/webview_window_linux.go | Replaces external debounce with internal/debounce. |
| v3/pkg/application/systemtray.go | Introduces SystemTrayLabelPart + pluggable SystemTrayLabelParser with default ANSI stripping. |
| v3/pkg/application/systemtray_darwin.go | Uses SystemTrayLabelParser instead of hard-wired ANSI parser dependency. |
| v3/pkg/application/screenmanager.go | Adds JSON tags to runtime-facing structs for obfuscation-safe wire format. |
| v3/pkg/application/messageprocessor_browser.go | Switches from external browser opener to internal/browser. |
| v3/pkg/application/logger_dev.go | Switches from external tint to internal/tint. |
| v3/pkg/application/logger_dev_windows.go | Switches from external tint to internal/tint. |
| v3/pkg/application/environment.go | Adds JSON tags to EnvironmentInfo fields. |
| v3/pkg/application/browser_manager.go | Switches from external browser opener to internal/browser. |
| v3/pkg/application/bindings.go | Adds global stable binding-method ID registry + uses it when generating bound method IDs. |
| v3/pkg/application/bindings_test.go | Adds tests for registered stable binding method IDs and panic behavior. |
| v3/pkg/application/application_debug.go | Switches from external lo to internal/lo. |
| v3/internal/uuid/uuid.go | New minimal UUID implementation (v4 + name-based SHA-1). |
| v3/internal/tint/tint.go | New internal colorized slog.Handler implementation. |
| v3/internal/templates/templates.go | Switches to internal gosod, internal lo, and stdlib error wrapping. |
| v3/internal/setupwizard/wizard.go | Switches from external browser opener to internal/browser. |
| v3/internal/service/service.go | Switches to internal gosod and internal lo. |
| v3/internal/semver/semver.go | New minimal semver parsing/comparison implementation. |
| v3/internal/s/s.go | Inlines shell-splitting logic (replacing google/shlex). |
| v3/internal/optional/optional.go | New generic optional value implementation replacing leaanthony/u. |
| v3/internal/operatingsystem/os.go | Adds JSON tags to OS info fields. |
| v3/internal/lo/lo.go | New minimal generic helper set replacing samber/lo. |
| v3/internal/gosod/gosod.go | New internal template FS extractor replacing external gosod. |
| v3/internal/go-common-file-dialog/util/util.go | Switches to internal UUID implementation for deterministic GUID creation. |
| v3/internal/go-common-file-dialog/cfd/iFileOpenDialog.go | Switches to internal UUID implementation. |
| v3/internal/github/semver.go | Adapts GitHub semver helpers to new internal semver struct/fields. |
| v3/internal/generator/generate.go | Adds obfuscated bindings aggregation plumbing + main-package dir detection. |
| v3/internal/generator/binding_ids.go | New aggregated wails_obfuscated.gen.go emitter (imports, aliasing, init registrations). |
| v3/internal/generator/binding_ids_test.go | Tests for obfuscated bindings source generation and alias/self-import rules. |
| v3/internal/flags/task_wrapper.go | Adds build flags for --obfuscated and --garbleargs. |
| v3/internal/flags/bindings.go | Adds -obfuscated and -obfuscated-output flags for bindings generation. |
| v3/internal/doctor/doctor.go | Switches from external lo to internal/lo. |
| v3/internal/doctor/doctor_windows.go | Switches from external lo to internal/lo. |
| v3/internal/doctor/doctor_darwin.go | Switches from external lo to internal/lo. |
| v3/internal/debounce/debounce.go | New internal debounce implementation replacing external dep. |
| v3/internal/commands/tool_version.go | Updates version field access to match new internal semver representation. |
| v3/internal/commands/taskfile_obfuscation_test.go | Adds tests asserting Taskfile templates include obfuscation support. |
| v3/internal/commands/task_wrapper.go | Wires --obfuscated / --garbleargs into Taskfile variables. |
| v3/internal/commands/task_wrapper_test.go | Adds test coverage for obfuscation args passed through Build wrapper. |
| v3/internal/commands/syso.go | Replaces pkg/errors wrapping with stdlib fmt.Errorf. |
| v3/internal/commands/build-assets.go | Switches to internal gosod. |
| v3/internal/commands/build_assets/windows/Taskfile.yml | Adds obfuscated build support (garble, tags, env passthrough). |
| v3/internal/commands/build_assets/Taskfile.tmpl.yml | Adds -obfuscated forwarding to wails3 generate bindings. |
| v3/internal/commands/build_assets/linux/Taskfile.yml | Adds obfuscated build support (garble, tags, env passthrough). |
| v3/internal/commands/build_assets/docker/Dockerfile.cross | Bumps Go base image, installs garble, and supports obfuscated builds in build script. |
| v3/internal/commands/build_assets/darwin/Taskfile.yml | Adds obfuscated build support (garble, tags, env passthrough). |
| v3/internal/commands/appimage_testfiles/main.go | Switches from external lo to internal/lo. |
| v3/internal/capabilities/capabilities.go | Adds JSON tags to capabilities struct. |
| v3/internal/buildinfo/buildinfo.go | Switches from external lo to internal/lo. |
| v3/internal/browser/browser.go | New internal URL/file opener replacing pkg/browser. |
| v3/go.sum | Removes checksums for removed direct deps; updates indirect entries. |
| v3/go.mod | Removes 11 direct deps; adjusts some entries to indirect and tweaks x/mod directness. |
| v3/examples/window/main.go | Switches from external lo to internal/lo. |
| v3/cmd/wails3/main.go | Switches from external browser/lo to internal equivalents. |
| docs/src/content/docs/reference/cli.mdx | Documents new wails3 build --obfuscated/--garbleargs and generate bindings -obfuscated flags. |
| docs/src/content/docs/guides/build/obfuscation.mdx | New guide describing Garble obfuscated build workflow and requirements. |
| docs/astro.config.mjs | Adds sidebar link to new obfuscation guide. |
| .github/workflows/build-cross-image.yml | Updates CI Go version used to build the cross-compilation image. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
.github/workflows/build-cross-image.yml (1)
149-149: ⚡ Quick winUse consistent Go versions across CI workflows (build-cross-image.yml sets 1.26 while most others use 1.25)
Go 1.26 is released and can be installed via
actions/setup-go, so this change shouldn’t fail for availability reasons. However,.github/workflows/build-cross-image.ymlis the only workflow usinggo-version: '1.26', while the rest of.github/workflows/(andv3/go.mod) use1.25—either update the other workflows intentionally or switch this one back to1.25for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-cross-image.yml at line 149, The workflow is inconsistent: it sets go-version: '1.26' while other CI workflows and v3/go.mod use 1.25; update the workflow's go-version key from '1.26' to '1.25' (replace the string value for the go-version input) so all workflows use the same Go version, or alternatively update the other workflows and v3/go.mod to '1.26' if you intend to standardize on Go 1.26—ensure the change targets the go-version property in the build-cross-image workflow (the line containing go-version) and keep versions consistent across CI and go.mod.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/internal/commands/syso.go`:
- Line 47: The error assignment that handles iconFile.Close() currently formats
err2 with "%s", losing the original error chain; change the fmt.Errorf call that
sets err (the one referencing err2) to wrap the underlying error using "%w" so
callers can unwrap/inspect the original iconFile.Close() error (i.e., replace
the "%s" formatting of err2 with "%w" when constructing the error).
In `@v3/internal/debounce/debounce.go`:
- Around line 23-30: The current debouncer.add implementation can let stale
callbacks run because time.AfterFunc spawns the callback if Stop returns false;
fix this by adding a generation/guard on the debouncer (e.g., a uint64
generation field) and incrementing it under d.mu before creating the new timer,
then pass a wrapper to time.AfterFunc that captures the generation value and
checks (using either the mutex or an atomic read of d.generation) that the
captured generation still matches the debouncer's current generation before
invoking f; ensure you still Stop the old timer and assign the new timer to
d.timer and update generation atomically/under lock so only the latest
invocation's f executes.
In `@v3/internal/s/s.go`:
- Around line 269-314: splitShell corrupts UTF-8 by indexing s as bytes and can
return an empty slice for whitespace-only input; change iteration to range over
runes (for _, r := range s) so you always work with full runes, replace
WriteByte/WriteRune usage accordingly (use cur.WriteRune for runes and when
consuming an escaped rune), handle backslash escape by peeking/consuming the
next rune from a rune slice or using an index over runes, preserve quote and
escape logic but operating on runes, and at the end return an error (e.g.,
fmt.Errorf("no tokens in input") ) if args is empty or input contains only
whitespace; update function splitShell to use these rune-based operations and
the new empty-input error.
In `@v3/internal/semver/semver.go`:
- Around line 90-100: The prerelease comparison using strings.Compare is
incorrect; instead split v.Prerelease and other.Prerelease on '.' and compare
each dot-separated identifier in order: if both identifiers are numeric (use
strconv.Atoi) compare numerically; if one is numeric and the other is
non-numeric treat the numeric identifier as lower precedence; if both are
non-numeric compare lexically (ASCII); if all compared identifiers are equal the
shorter list has lower precedence. Replace the default branch that returns
strings.Compare(v.Prerelease, other.Prerelease) with this dot-segmented,
numeric-aware comparison using v.Prerelease and other.Prerelease.
In `@v3/internal/tint/tint.go`:
- Around line 110-122: WithAttrs (and similarly WithGroup) currently risks slice
aliasing by doing preformatted: append(h.preformatted, buf.Bytes()...) and
groups: h.groups / groups: append(h.groups, name); fix by explicitly allocating
and copying new slices so derived handlers never share backing arrays: create a
new preformatted slice via append([]byte(nil), h.preformatted...) then append
the new bytes, and set groups to append(append([]string(nil), h.groups...),
name) (or use make+copy) so both preformatted and groups are fresh copies in the
new handler.
In `@v3/pkg/application/systemtray_darwin.go`:
- Around line 250-283: The ANSI-label path leaks C.CString allocations because
partToCStrings() creates C strings that are passed into
createAttributedString/appendAttributedString but never freed; update setLabel
(and partToCStrings usage) to free each C string after it's handed to the native
calls: for the first part, call createAttributedString(...) and then free
label/fg/bg (using C.free(unsafe.Pointer(...)) for each non-nil pointer), and
for each subsequent part call appendAttributedString(...) and then free that
part's label/fg/bg similarly; import the unsafe package if not present. Ensure
you free only after the native call returns (or change the native API to take
ownership and document it) and keep the existing non-ANSI path that relies on
systemTraySetLabel freeing its argument.
In `@v3/pkg/services/sqlite/sqlite.go`:
- Line 473: The current return unconditionally wraps the result of
s.sqlStmt.Close(), causing a non-nil error even when Close() succeeds; change
the logic in the method that calls s.sqlStmt.Close() so you capture err :=
s.sqlStmt.Close(), then if err != nil return fmt.Errorf("error closing prepared
statement: %w", err) else return nil (i.e., only wrap and return when err is
non-nil) to ensure successful closes return nil.
---
Nitpick comments:
In @.github/workflows/build-cross-image.yml:
- Line 149: The workflow is inconsistent: it sets go-version: '1.26' while other
CI workflows and v3/go.mod use 1.25; update the workflow's go-version key from
'1.26' to '1.25' (replace the string value for the go-version input) so all
workflows use the same Go version, or alternatively update the other workflows
and v3/go.mod to '1.26' if you intend to standardize on Go 1.26—ensure the
change targets the go-version property in the build-cross-image workflow (the
line containing go-version) and keep versions consistent across CI and go.mod.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa289d61-fd03-4e16-b0a3-61c30069aaa1
⛔ Files ignored due to path filters (1)
v3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (68)
.github/workflows/build-cross-image.ymldocs/astro.config.mjsdocs/src/content/docs/guides/build/obfuscation.mdxdocs/src/content/docs/reference/cli.mdxv3/UNRELEASED_CHANGELOG.mdv3/cmd/wails3/main.gov3/examples/window/main.gov3/go.modv3/internal/browser/browser.gov3/internal/buildinfo/buildinfo.gov3/internal/capabilities/capabilities.gov3/internal/commands/appimage_testfiles/main.gov3/internal/commands/build-assets.gov3/internal/commands/build_assets/Taskfile.tmpl.ymlv3/internal/commands/build_assets/darwin/Taskfile.ymlv3/internal/commands/build_assets/docker/Dockerfile.crossv3/internal/commands/build_assets/linux/Taskfile.ymlv3/internal/commands/build_assets/windows/Taskfile.ymlv3/internal/commands/syso.gov3/internal/commands/task_wrapper.gov3/internal/commands/task_wrapper_test.gov3/internal/commands/taskfile_obfuscation_test.gov3/internal/commands/tool_version.gov3/internal/debounce/debounce.gov3/internal/doctor/doctor.gov3/internal/doctor/doctor_darwin.gov3/internal/doctor/doctor_windows.gov3/internal/flags/bindings.gov3/internal/flags/task_wrapper.gov3/internal/generator/binding_ids.gov3/internal/generator/binding_ids_test.gov3/internal/generator/generate.gov3/internal/github/semver.gov3/internal/go-common-file-dialog/cfd/iFileOpenDialog.gov3/internal/go-common-file-dialog/util/util.gov3/internal/gosod/gosod.gov3/internal/lo/lo.gov3/internal/operatingsystem/os.gov3/internal/optional/optional.gov3/internal/s/s.gov3/internal/semver/semver.gov3/internal/service/service.gov3/internal/setupwizard/wizard.gov3/internal/templates/templates.gov3/internal/tint/tint.gov3/internal/uuid/uuid.gov3/pkg/application/application_debug.gov3/pkg/application/bindings.gov3/pkg/application/bindings_test.gov3/pkg/application/browser_manager.gov3/pkg/application/environment.gov3/pkg/application/logger_dev.gov3/pkg/application/logger_dev_windows.gov3/pkg/application/messageprocessor_browser.gov3/pkg/application/screenmanager.gov3/pkg/application/systemtray.gov3/pkg/application/systemtray_darwin.gov3/pkg/application/webview_window.gov3/pkg/application/webview_window_linux.gov3/pkg/application/webview_window_options.gov3/pkg/application/webview_window_windows.gov3/pkg/doctor-ng/doctor.gov3/pkg/doctor-ng/platform_darwin.gov3/pkg/doctor-ng/platform_windows.gov3/pkg/services/kvstore/kvstore.gov3/pkg/services/notifications/notifications_windows.gov3/pkg/services/sqlite/sqlite.gov3/tasks/sed/sed.go
Removes the following direct dependencies from go.mod by inlining lightweight stdlib-only replacements: - github.com/Masterminds/semver v1 → internal/semver - github.com/bep/debounce → internal/debounce - github.com/google/shlex → inline splitShell() in internal/s - github.com/google/uuid → internal/uuid - github.com/leaanthony/go-ansi-parser → pluggable SystemTrayLabelParser var - github.com/leaanthony/gosod → internal/gosod - github.com/leaanthony/u → internal/optional - github.com/lmittmann/tint → internal/tint - github.com/pkg/browser → internal/browser - github.com/pkg/errors → fmt.Errorf / stdlib errors - github.com/samber/lo → internal/lo (generics) The SystemTrayLabelParser change makes macOS system tray ANSI label rendering pluggable: the default implementation strips escape codes, and callers can replace the var with a full ANSI parser such as go-ansi-parser or any compatible library. All replacements tested: go build ./..., wails3 init, wails3 generate build-assets, and wails3 service init all pass cleanly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
v3/pkg/services/sqlite/sqlite.go (1)
473-473:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReturn
nilwhenStmt.Close()succeeds.Line 473 wraps and returns
errunconditionally. Successful closes should returnnil, not a wrapped error value.💡 Minimal fix
- return fmt.Errorf("error closing prepared statement: %w", err) + if err != nil { + return fmt.Errorf("error closing prepared statement: %w", err) + } + return nil#!/bin/bash set -euo pipefail # Verify the unconditional wrapped return in Stmt.Close. rg -n -C8 'func \(s \*Stmt\) Close\(\)|return fmt.Errorf\("error closing prepared statement: %w", err\)' v3/pkg/services/sqlite/sqlite.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/services/sqlite/sqlite.go` at line 473, The Stmt.Close() method currently always wraps and returns err even when Close succeeded; update the method on type Stmt so that it checks the result of s.stmt.Close() and only returns fmt.Errorf("error closing prepared statement: %w", err) when err != nil, otherwise return nil (i.e., call s.stmt.Close(), if err != nil wrap and return it, else return nil).v3/pkg/application/systemtray_darwin.go (1)
261-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFree
C.CStringallocations after attributed-string calls.Lines 261-267 allocate C strings for each parsed part, but those pointers are never freed after
C.createAttributedString/C.appendAttributedStringreturn. That leaks native heap on repeated ANSI label updates.💡 Minimal fix
@@ `#include` "Cocoa/Cocoa.h" `#include` "menuitem_darwin.h" `#include` "systemtray_darwin.h" +#include <stdlib.h> @@ func (s *macosSystemTray) setLabel(label string) { @@ cLabel, cFg, cBg := partToCStrings(parts[0]) attr := C.createAttributedString(cLabel, cFg, cBg) + freeCStrings(cLabel, cFg, cBg) for _, p := range parts[1:] { cLabel, cFg, cBg = partToCStrings(p) attr = C.appendAttributedString(attr, cLabel, cFg, cBg) + freeCStrings(cLabel, cFg, cBg) } C.systemTraySetANSILabel(s.nsStatusItem, attr) } @@ func partToCStrings(p SystemTrayLabelPart) (label, fg, bg *C.char) { @@ return } + +func freeCStrings(label, fg, bg *C.char) { + if label != nil { + C.free(unsafe.Pointer(label)) + } + if fg != nil { + C.free(unsafe.Pointer(fg)) + } + if bg != nil { + C.free(unsafe.Pointer(bg)) + } +}#!/bin/bash set -euo pipefail echo "Go-side allocations and frees:" rg -n -C3 'partToCStrings|C\.CString|C\.free|createAttributedString|appendAttributedString' v3/pkg/application/systemtray_darwin.go echo echo "Native-side free() usage for attributed label helpers:" rg -n -C3 'createAttributedString|appendAttributedString|free\(' v3/pkg/application/systemtray_darwin.mAlso applies to: 274-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/systemtray_darwin.go` around lines 261 - 267, The C.CString pointers returned by partToCStrings are never freed, leaking native memory; after calling C.createAttributedString and each C.appendAttributedString, call C.free on the cLabel, cFg, and cBg pointers (or defer free immediately after each allocation) so each trio returned by partToCStrings is released once the native call consumes them; update the code around partToCStrings, C.createAttributedString, C.appendAttributedString and before C.systemTraySetANSILabel to ensure every allocated C string is freed (use C.free on each cLabel/cFg/cBg).v3/internal/tint/tint.go (1)
46-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClone handlers with deep-copied slices and shared lock.
Line 120/131 can alias parent slices via
append, and cloned handlers currently do not share a mutex for the same writer. This can cause cross-handler state bleed and interleaved output under concurrent use.💡 Proposed fix
type handler struct { - mu sync.Mutex + mu *sync.Mutex w io.Writer opts Options timeFormat string preformatted []byte groups []string } @@ - return &handler{w: w, opts: *opts, timeFormat: tf} + return &handler{mu: &sync.Mutex{}, w: w, opts: *opts, timeFormat: tf} } @@ return &handler{ + mu: h.mu, w: h.w, opts: h.opts, timeFormat: h.timeFormat, - preformatted: append(h.preformatted, buf.Bytes()...), - groups: h.groups, + preformatted: append(append([]byte(nil), h.preformatted...), buf.Bytes()...), + groups: append([]string(nil), h.groups...), } } func (h *handler) WithGroup(name string) slog.Handler { return &handler{ + mu: h.mu, w: h.w, opts: h.opts, timeFormat: h.timeFormat, - preformatted: h.preformatted, - groups: append(h.groups, name), + preformatted: append([]byte(nil), h.preformatted...), + groups: append(append([]string(nil), h.groups...), name), } }In Go, can append(...) reuse backing arrays and alias parent/child slices, and for slog.Handler WithAttrs/WithGroup implementations should derived handlers deep-copy slice state and share synchronization for the same writer?Also applies to: 110-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/tint/tint.go` around lines 46 - 53, The cloned handler currently aliases parent slices and uses a separate mutex causing interleaved output; change the handler to share synchronization and deep-copy slice state in the clone code: make the lock a pointer (mu *sync.Mutex) so clones share the same mutex instance for the same writer, and when creating derived handlers in WithAttrs and WithGroup (and any clone/derived creation logic) copy slices with append on a nil slice (e.g., preformatted := append([]byte(nil), h.preformatted...) and groups := append([]string(nil), h.groups...)) so backing arrays are not aliased; ensure other mutable fields (opts) are copied as needed and assign the shared mu pointer to the cloned handler.v3/internal/debounce/debounce.go (1)
23-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Timer.Stop()alone does not enforce “last invocation wins.”On Line 27, stopping the old timer is insufficient for
time.AfterFunc; ifStop()returns false, the old callback may already be running. Add a generation guard so only the newest scheduled callback executes.💡 Proposed fix
type debouncer struct { mu sync.Mutex after time.Duration timer *time.Timer + gen uint64 } func (d *debouncer) add(f func()) { d.mu.Lock() - defer d.mu.Unlock() + d.gen++ + gen := d.gen if d.timer != nil { d.timer.Stop() } - d.timer = time.AfterFunc(d.after, f) + d.timer = time.AfterFunc(d.after, func() { + d.mu.Lock() + if gen != d.gen { + d.mu.Unlock() + return + } + d.mu.Unlock() + f() + }) + d.mu.Unlock() }For Go timers created with time.AfterFunc, does Timer.Stop() guarantee the callback won't run, and what pattern ensures only the latest debounced callback executes?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/debounce/debounce.go` around lines 23 - 30, In debouncer.add, stopping d.timer is not enough because time.AfterFunc callbacks may already be running; introduce a generation/token on the debouncer (e.g., gen or seq) that you increment each time you schedule a new timer, capture the current generation inside the closure passed to time.AfterFunc, and have the closure check that the captured generation still equals the debouncer's latest generation before calling f; use an atomic counter or protect gen with d.mu when reading/updating so only the most recently scheduled callback (the newest generation) runs while older callbacks exit early.v3/internal/semver/semver.go (1)
90-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-release comparison is not SemVer-correct.
On Line 99,
strings.Compare(v.Prerelease, other.Prerelease)misorders numeric/dot-separated prerelease identifiers (e.g.,beta.10vsbeta.2).💡 Proposed fix
default: - return strings.Compare(v.Prerelease, other.Prerelease) + return comparePrerelease(v.Prerelease, other.Prerelease) } } @@ func cmpUint(a, b uint64) int { @@ } + +func comparePrerelease(a, b string) int { + as := strings.Split(a, ".") + bs := strings.Split(b, ".") + for i := 0; i < len(as) && i < len(bs); i++ { + ai, aErr := strconv.ParseUint(as[i], 10, 64) + bi, bErr := strconv.ParseUint(bs[i], 10, 64) + switch { + case aErr == nil && bErr == nil: + if c := cmpUint(ai, bi); c != 0 { + return c + } + case aErr == nil: + return -1 + case bErr == nil: + return 1 + default: + if c := strings.Compare(as[i], bs[i]); c != 0 { + return c + } + } + } + return cmpUint(uint64(len(as)), uint64(len(bs))) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/semver/semver.go` around lines 90 - 100, The current prerelease comparison uses strings.Compare(v.Prerelease, other.Prerelease) which misorders dot-separated and numeric identifiers; replace that fallback with SemVer-compliant logic: split v.Prerelease and other.Prerelease on '.', iterate identifiers pairwise and for each if both are numeric compare as integers, if one is numeric it has lower precedence than the non-numeric, otherwise compare lexically; if all compared identifiers are equal then the shorter identifier list has lower precedence (e.g., "alpha" < "alpha.1"). Update the switch default branch to perform this identifier-by-identifier comparison instead of the single strings.Compare call, using v.Prerelease and other.Prerelease as the sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/internal/browser/browser.go`:
- Around line 19-30: The open function currently returns cmd.Start() directly
which leaves the child process unreaped; instead, after calling cmd.Start() in
open (the function handling runtime.GOOS and creating the exec.Cmd named cmd),
spawn a goroutine that calls cmd.Wait() to reap the child and optionally logs
any Wait() error (so open remains non-blocking), or alternatively call cmd.Run()
if blocking until the opener exits is acceptable; ensure you reference
exec.Cmd.Start and exec.Cmd.Wait on the cmd variable inside the open function so
the started process is always reaped to avoid zombie/resource leaks.
In `@v3/internal/templates/templates.go`:
- Around line 199-203: The fallback branch currently masks the real metadata
read error by returning a "no template" message or the jsonErr even when the
YAML read failed; update the branch that checks errors.Is(yamlErr,
fs.ErrNotExist) to only treat the YAML as missing in that case, and otherwise
return the actual yamlErr (with context) instead of jsonErr so callers see the
real failure; locate the conditional that references yamlErr, jsonErr and result
in templates.go and replace the current returned error with the yamlErr-wrapped
error when yamlErr is not fs.ErrNotExist.
---
Duplicate comments:
In `@v3/internal/debounce/debounce.go`:
- Around line 23-30: In debouncer.add, stopping d.timer is not enough because
time.AfterFunc callbacks may already be running; introduce a generation/token on
the debouncer (e.g., gen or seq) that you increment each time you schedule a new
timer, capture the current generation inside the closure passed to
time.AfterFunc, and have the closure check that the captured generation still
equals the debouncer's latest generation before calling f; use an atomic counter
or protect gen with d.mu when reading/updating so only the most recently
scheduled callback (the newest generation) runs while older callbacks exit
early.
In `@v3/internal/semver/semver.go`:
- Around line 90-100: The current prerelease comparison uses
strings.Compare(v.Prerelease, other.Prerelease) which misorders dot-separated
and numeric identifiers; replace that fallback with SemVer-compliant logic:
split v.Prerelease and other.Prerelease on '.', iterate identifiers pairwise and
for each if both are numeric compare as integers, if one is numeric it has lower
precedence than the non-numeric, otherwise compare lexically; if all compared
identifiers are equal then the shorter identifier list has lower precedence
(e.g., "alpha" < "alpha.1"). Update the switch default branch to perform this
identifier-by-identifier comparison instead of the single strings.Compare call,
using v.Prerelease and other.Prerelease as the sources.
In `@v3/internal/tint/tint.go`:
- Around line 46-53: The cloned handler currently aliases parent slices and uses
a separate mutex causing interleaved output; change the handler to share
synchronization and deep-copy slice state in the clone code: make the lock a
pointer (mu *sync.Mutex) so clones share the same mutex instance for the same
writer, and when creating derived handlers in WithAttrs and WithGroup (and any
clone/derived creation logic) copy slices with append on a nil slice (e.g.,
preformatted := append([]byte(nil), h.preformatted...) and groups :=
append([]string(nil), h.groups...)) so backing arrays are not aliased; ensure
other mutable fields (opts) are copied as needed and assign the shared mu
pointer to the cloned handler.
In `@v3/pkg/application/systemtray_darwin.go`:
- Around line 261-267: The C.CString pointers returned by partToCStrings are
never freed, leaking native memory; after calling C.createAttributedString and
each C.appendAttributedString, call C.free on the cLabel, cFg, and cBg pointers
(or defer free immediately after each allocation) so each trio returned by
partToCStrings is released once the native call consumes them; update the code
around partToCStrings, C.createAttributedString, C.appendAttributedString and
before C.systemTraySetANSILabel to ensure every allocated C string is freed (use
C.free on each cLabel/cFg/cBg).
In `@v3/pkg/services/sqlite/sqlite.go`:
- Line 473: The Stmt.Close() method currently always wraps and returns err even
when Close succeeded; update the method on type Stmt so that it checks the
result of s.stmt.Close() and only returns fmt.Errorf("error closing prepared
statement: %w", err) when err != nil, otherwise return nil (i.e., call
s.stmt.Close(), if err != nil wrap and return it, else return nil).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad1409b6-3286-44fb-ae58-55009fafa381
⛔ Files ignored due to path filters (1)
v3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
v3/cmd/wails3/main.gov3/examples/window/main.gov3/go.modv3/internal/browser/browser.gov3/internal/buildinfo/buildinfo.gov3/internal/commands/appimage_testfiles/main.gov3/internal/commands/build-assets.gov3/internal/commands/syso.gov3/internal/commands/tool_version.gov3/internal/debounce/debounce.gov3/internal/doctor/doctor.gov3/internal/doctor/doctor_darwin.gov3/internal/doctor/doctor_windows.gov3/internal/github/semver.gov3/internal/go-common-file-dialog/cfd/iFileOpenDialog.gov3/internal/go-common-file-dialog/util/util.gov3/internal/gosod/gosod.gov3/internal/lo/lo.gov3/internal/optional/optional.gov3/internal/s/s.gov3/internal/semver/semver.gov3/internal/service/service.gov3/internal/setupwizard/wizard.gov3/internal/templates/templates.gov3/internal/tint/tint.gov3/internal/uuid/uuid.gov3/pkg/application/application_debug.gov3/pkg/application/browser_manager.gov3/pkg/application/logger_dev.gov3/pkg/application/logger_dev_windows.gov3/pkg/application/messageprocessor_browser.gov3/pkg/application/systemtray.gov3/pkg/application/systemtray_darwin.gov3/pkg/application/webview_window.gov3/pkg/application/webview_window_linux.gov3/pkg/application/webview_window_options.gov3/pkg/application/webview_window_windows.gov3/pkg/doctor-ng/doctor.gov3/pkg/doctor-ng/platform_darwin.gov3/pkg/doctor-ng/platform_windows.gov3/pkg/services/kvstore/kvstore.gov3/pkg/services/notifications/notifications_windows.gov3/pkg/services/sqlite/sqlite.gov3/tasks/sed/sed.go
✅ Files skipped from review due to trivial changes (6)
- v3/pkg/application/messageprocessor_browser.go
- v3/internal/setupwizard/wizard.go
- v3/internal/doctor/doctor.go
- v3/pkg/application/webview_window_linux.go
- v3/pkg/doctor-ng/platform_windows.go
- v3/internal/commands/syso.go
- sqlite: guard errors.Wrap replacement so nil err returns nil (critical bug: every successful Stmt.Close was returning a non-nil error) - debounce: use generation counter to drop callbacks from superseded timers that fire after Stop() returns false - semver: implement SemVer 2.0-correct prerelease comparison (numeric identifiers compared numerically, numeric < alphanumeric, dot-split) - tint: make defensive copies in WithAttrs/WithGroup to prevent slice aliasing between parent and child handlers - s/splitShell: operate on []rune to fix UTF-8 multi-byte corruption; track inWord flag to correctly preserve empty quoted arguments - systemtray_darwin: free C strings allocated by partToCStrings after each createAttributedString/appendAttributedString call
- browser: reap child process with go cmd.Wait() to avoid zombies - syso: use %w instead of %s for icon-close error to allow unwrapping - templates: fix error reporting when template.yaml exists but is unreadable — surface the real yamlErr instead of the json not-found message; check both files are absent before reporting "not found"
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v3/internal/s/s.go (1)
318-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an error when no tokens are parsed.
If input is empty/whitespace,
splitShellreturns an empty slice; Line 330 inEXECthen indexesargs[0]and panics.💡 Proposed fix
func splitShell(s string) ([]string, error) { @@ if inWord { args = append(args, cur.String()) } + if len(args) == 0 { + return nil, fmt.Errorf("empty command") + } return args, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/s/s.go` around lines 318 - 320, splitShell currently returns an empty slice for inputs that are empty/whitespace, which causes EXEC to panic when it indexes args[0]; change the contract so callers can handle this: modify splitShell to detect when no tokens were parsed (i.e., args is empty after the final inWord check around cur.String()) and return an error (e.g., ErrNoTokens) instead of an empty slice, and update EXEC to check the error from splitShell before accessing args[0] (or, if you prefer, keep splitShell returning []string but update EXEC to check len(args)==0 and return a descriptive error), referencing the functions splitShell and EXEC and the variables args and cur.String() to locate the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@v3/internal/s/s.go`:
- Around line 318-320: splitShell currently returns an empty slice for inputs
that are empty/whitespace, which causes EXEC to panic when it indexes args[0];
change the contract so callers can handle this: modify splitShell to detect when
no tokens were parsed (i.e., args is empty after the final inWord check around
cur.String()) and return an error (e.g., ErrNoTokens) instead of an empty slice,
and update EXEC to check the error from splitShell before accessing args[0] (or,
if you prefer, keep splitShell returning []string but update EXEC to check
len(args)==0 and return a descriptive error), referencing the functions
splitShell and EXEC and the variables args and cur.String() to locate the
relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 582bb599-923d-4337-90e0-f3697b948f02
📒 Files selected for processing (9)
v3/internal/browser/browser.gov3/internal/commands/syso.gov3/internal/debounce/debounce.gov3/internal/s/s.gov3/internal/semver/semver.gov3/internal/templates/templates.gov3/internal/tint/tint.gov3/pkg/application/systemtray_darwin.gov3/pkg/services/sqlite/sqlite.go
✅ Files skipped from review due to trivial changes (1)
- v3/internal/commands/syso.go
- internal/browser: split open() into per-platform files (darwin/windows/other) so each platform achieves 100% statement coverage; removes unreachable branches that the GOOS switch compiled in on every platform - internal/uuid: drop unreachable error check after crypto/rand.Read; Go 1.20+ calls runtime.fatal internally and never returns an error - Add _test.go files for all 8 new packages: browser(100%), debounce(100%), lo(100%), optional(100%), semver(100%), tint(100%), uuid(100%), gosod(97.9% — two error paths structurally unreachable: filepath.Abs needs deleted CWD, os.File.Close needs concrete-type mock) - Debounce generation-mismatch branch covered deterministically by holding d.mu externally while timer fires then bumping generation before Unlock - SemVer prerelease tests cover all §11.4 comparison branches including the full alpha<alpha.1<alpha.beta<beta<beta.2<beta.11<rc.1<1.0.0 ordering
Add absPath and osCreateFile package-level variables (default to filepath.Abs and os.Create) so tests can substitute failing stubs to cover the two previously unreachable error paths: - Extract: filepath.Abs error branch - processTemplates: w.Close() error branch
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
v3/internal/debounce/debounce_test.go (1)
10-12: ⚡ Quick winReduce timing-flake risk in debounce tests.
These assertions depend on tiny fixed sleeps; on slow CI they can intermittently fail. Prefer deadline-based polling (
Eventually-style loop) over exact millisecond sleeps to make tests deterministic.Also applies to: 46-47, 68-74, 112-113, 126-127, 148-151, 170-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/debounce/debounce_test.go` around lines 10 - 12, Tests in debounce_test.go use tiny fixed sleeps (debounceAfter and waitFor) causing flakes; replace those exact millisecond Sleep-based assertions with deadline-based polling (an "Eventually" loop) that repeatedly checks the expected condition until a timeout. For each assertion that currently does time.Sleep(...) then assert, implement a loop that records deadline := time.Now().Add(waitFor) and in a for loop until time.Now().After(deadline) checks the condition (e.g., output length, call count, or state used in the test) and breaks on success, sleeping a short interval (e.g., 1-5ms) between retries, and fail the test if deadline passes; update all spots referencing debounceAfter and waitFor (and the assertions around them) to use this pattern to reduce CI timing flakiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/internal/browser/browser_test.go`:
- Around line 12-27: Tests TestOpenURL_Success and TestOpenFile_Success
currently launch host handlers via OpenURL and OpenFile; change them so unit
test runs don't trigger OS actions by either (a) converting these tests into
integration tests using a build tag (e.g., add a separate file with a build tag
like //go:build integration that contains TestOpenURL_Success and
TestOpenFile_Success) or (b) refactor OpenURL/OpenFile to accept an opener
interface or function and update tests to inject a stub opener that records
calls instead of launching real apps (update tests to call OpenURL/OpenFile via
the injectable opener or move them to the new integration file). Ensure you
reference the existing symbols OpenURL, OpenFile, TestOpenURL_Success, and
TestOpenFile_Success when applying the change.
In `@v3/internal/gosod/gosod_test.go`:
- Around line 409-430: The test TestExtract_MkdirAllFails relies on Unix
permission semantics; make it Unix-only by moving or duplicating the test into a
file with a Unix build constraint (use //go:build unix and the old +build unix
line) so it only compiles on Unix-like platforms; keep the test body
(referencing TestExtract_MkdirAllFails, td.Extract, os.Mkdir with mode 0555,
t.TempDir) unchanged but ensure the file-level build tag is added at the top to
prevent non-Unix CI runs from executing this platform-specific behavior.
In `@v3/internal/tint/tint_test.go`:
- Around line 137-156: The tests TestHandle_NoColor_ProducesNoEscapeCodes and
TestHandle_Color_ContainsEscapeCodes currently ignore the error returned by
h.Handle; change both calls to check the returned error and fail the test on
non-nil (e.g., if err != nil { t.Fatalf("Handle error: %v", err) }) so real
handler failures don't get masked; locate the h.Handle(context.Background(), r)
invocations in those two test functions and replace the discarded assignment
with proper error handling.
---
Nitpick comments:
In `@v3/internal/debounce/debounce_test.go`:
- Around line 10-12: Tests in debounce_test.go use tiny fixed sleeps
(debounceAfter and waitFor) causing flakes; replace those exact millisecond
Sleep-based assertions with deadline-based polling (an "Eventually" loop) that
repeatedly checks the expected condition until a timeout. For each assertion
that currently does time.Sleep(...) then assert, implement a loop that records
deadline := time.Now().Add(waitFor) and in a for loop until
time.Now().After(deadline) checks the condition (e.g., output length, call
count, or state used in the test) and breaks on success, sleeping a short
interval (e.g., 1-5ms) between retries, and fail the test if deadline passes;
update all spots referencing debounceAfter and waitFor (and the assertions
around them) to use this pattern to reduce CI timing flakiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6409b745-8937-4e4f-8c52-a842ea6f1da6
📒 Files selected for processing (14)
v3/internal/browser/browser.gov3/internal/browser/browser_darwin.gov3/internal/browser/browser_other.gov3/internal/browser/browser_test.gov3/internal/browser/browser_windows.gov3/internal/debounce/debounce_test.gov3/internal/gosod/gosod.gov3/internal/gosod/gosod_test.gov3/internal/lo/lo_test.gov3/internal/optional/optional_test.gov3/internal/semver/semver_test.gov3/internal/tint/tint_test.gov3/internal/uuid/uuid.gov3/internal/uuid/uuid_test.go
💤 Files with no reviewable changes (1)
- v3/internal/browser/browser.go
✅ Files skipped from review due to trivial changes (1)
- v3/internal/browser/browser_other.go
- internal/browser: move OS-launching tests to integration build tag (browser_integration_test.go); add openCmd injectable var to all platform files so unit tests can stub without real side effects; TestOpen_Success stubs with os.Args[0] -test.run=^$ (exits 0, cross-platform) - internal/gosod: move TestExtract_MkdirAllFails to gosod_unix_test.go with //go:build unix to avoid Windows CI failures on chmod semantics - internal/tint: check error return from h.Handle() instead of discarding it with _ in TestHandle_NoColor_ProducesNoEscapeCodes and TestHandle_Color_ContainsEscapeCodes
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes 11 direct Go dependencies from
v3/go.modby replacing them with small internal packages backed entirely by the standard library.github.com/Masterminds/semver v1internal/semver— parse + compare, fields not methodshub.lumenfield.work/bep/debounceinternal/debounce— mutex +time.AfterFunchub.lumenfield.work/google/shlexsplitShell()ininternal/s/s.gohub.lumenfield.work/google/uuidinternal/uuid— v4 (crypto/rand) + v5 (SHA-1)github.com/leaanthony/go-ansi-parserSystemTrayLabelParservar (see below)github.com/leaanthony/gosodinternal/gosod—fs.WalkDir+text/templatehub.lumenfield.work/leaanthony/uinternal/optional— genericVar[T]+Boolaliashub.lumenfield.work/lmittmann/tintinternal/tint—slog.Handlerwith ANSI colourhub.lumenfield.work/pkg/browserinternal/browser—exec.Command(open/xdg-open/rundll32)github.com/pkg/errorsfmt.Errorf("…: %w", err)/errors.Newhub.lumenfield.work/samber/lointernal/lo— 8 generic helpers (Go 1.18+)Some packages still appear as
// indirectin go.mod because transitive dependencies (e.g.atterpac/refresh) still reference them — they are no longer direct dependencies of this module.SystemTrayLabelParser (macOS)
Rather than hard-wiring
go-ansi-parserfor macOS system-tray label rendering, the parser is now a pluggable package-level variable:Test plan
go build ./...— no compilation errorswails3 init -t vanilla— project created, templates substituted correctlywails3 generate build-assets— build assets extracted correctlywails3 service init— service scaffolded correctlySummary by CodeRabbit