Split the common macos asset-gen error message to specific error messages#5258
Open
barremian wants to merge 3 commits intowailsapp:masterfrom
Open
Split the common macos asset-gen error message to specific error messages#5258barremian wants to merge 3 commits intowailsapp:masterfrom
barremian wants to merge 3 commits intowailsapp:masterfrom
Conversation
Contributor
WalkthroughReplaces exported sentinel Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
barremian
changed the title
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v3/internal/commands/icons.go (1)
23-31: Preserve the underlying error cause insidemacAssetNotSupportedError.The new typed error only carries a flat
messagestring, so the underlying causes fromoperatingsystem.Info()(Line 176),plist.Unmarshal(Line 199) andcmd.Output()(Line 193) are dropped on the floor. That makes diagnosing field issues like#5083strictly harder than before — users hit a generic "failed to determine macOS version" with no hint of why. Consider carrying a wrapped cause and exposingUnwrap()soerrors.Is/Asand%wformatting still work end-to-end.♻️ Proposed refactor
// macAssetNotSupportedError is returned by generateMacAsset when mac asset generation // is not supported in the current environment. type macAssetNotSupportedError struct { message string + cause error } func (e *macAssetNotSupportedError) Error() string { + if e.cause != nil { + return e.message + ": " + e.cause.Error() + } return e.message } + +func (e *macAssetNotSupportedError) Unwrap() error { + return e.cause +}Then at the call sites that currently swallow the error:
- info, err := operatingsystem.Info() - if err != nil { - return &macAssetNotSupportedError{message: "failed to determine macOS version"} - } + info, err := operatingsystem.Info() + if err != nil { + return &macAssetNotSupportedError{message: "failed to determine macOS version", cause: err} + } @@ - versionPlist, err := cmd.Output() - if err != nil { - return &macAssetNotSupportedError{message: "actool 26+ is required (install Xcode 26+)"} - } + versionPlist, err := cmd.Output() + if err != nil { + return &macAssetNotSupportedError{message: "actool 26+ is required (install Xcode 26+)", cause: err} + } @@ - if _, err := plist.Unmarshal(versionPlist, &plistData); err != nil { - return &macAssetNotSupportedError{message: "failed to parse actool version output"} - } + if _, err := plist.Unmarshal(versionPlist, &plistData); err != nil { + return &macAssetNotSupportedError{message: "failed to parse actool version output", cause: err} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/icons.go` around lines 23 - 31, The macAssetNotSupportedError type currently only holds a message and drops the original error; change its definition to include a cause field (err error), implement Unwrap() error { return e.cause } and update Error() to include both the message and the cause (so formatting and %w work); then at the sites that return macAssetNotSupportedError (calls around operatingsystem.Info(), cmd.Output(), and plist.Unmarshal in the generateMacAsset flow) wrap the original error into the new macAssetNotSupportedError (preserve the underlying error when constructing the value) so errors.Is/As and %w chaining continue to work.v3/internal/commands/icons_test.go (1)
173-188: Substring assertion is currently ambiguous between two error sites.
"mac asset generation requires macOS 26 or later"matches both the non-darwin branch (Line 171 oficons.go) and themajor < 26branch (Line 187 oficons.go). Since this test is gated byrequireNonDarwin, you'd ideally want a substring that only the non-darwin branch can produce, so a future refactor that swaps the two messages can't silently pass. This ties into the suggestion left on Line 171 oficons.goto differentiate the wording.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/icons_test.go` around lines 173 - 188, The test's wantErrContains is ambiguous because the substring "mac asset generation requires macOS 26 or later" matches two different error branches; update the test case (the table entry that builds an IconsOptions with IconComposerInput and MacAssetDir and is gated by requireNonDarwin) to assert a unique message produced only by the non-darwin branch — e.g., change wantErrContains to a phrase like "icon composer assets are unsupported on non-macOS hosts" or "icon composer assets are only supported on macOS" so the assertion can only pass when the non-darwin error path in icons.go is hit. Ensure the new substring matches the exact error text emitted by the non-darwin branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/internal/commands/icons.go`:
- Around line 190-194: The current actool invocation in the icons generation
(`exec.Command("/usr/bin/actool", "--version")` and `cmd.Output()`) always
returns a macAssetNotSupportedError claiming Xcode 26+ is required; instead
surface the real failure and distinguish missing binary vs execution error by
inspecting the error from cmd.Output(): if errors.Is(err, exec.ErrNotFound) or
os.IsNotExist(err) return a macAssetNotSupportedError whose message indicates
actool not found and include the original error as the cause; if the error is an
*exec.ExitError or other execution error, return a macAssetNotSupportedError
that includes the stderr/err text (wrap or format the underlying error) so
callers see the real reason rather than always blaming Xcode version; keep the
existing macAssetNotSupportedError type but populate its message/cause with the
underlying error details.
- Around line 170-171: Change the non-darwin error text so it reflects
unsupported platform rather than a macOS version mismatch: inside the
runtime.GOOS != "darwin" branch (the check that currently returns
&macAssetNotSupportedError{message: "mac asset generation requires macOS 26 or
later"}), update the message to something like "mac asset generation is only
supported on macOS" and adjust the corresponding test in icons_test.go to assert
the updated substring instead of the macOS version phrasing; keep the separate
version-check branch (the major < 26 branch) unchanged so the two errors are
distinct.
---
Nitpick comments:
In `@v3/internal/commands/icons_test.go`:
- Around line 173-188: The test's wantErrContains is ambiguous because the
substring "mac asset generation requires macOS 26 or later" matches two
different error branches; update the test case (the table entry that builds an
IconsOptions with IconComposerInput and MacAssetDir and is gated by
requireNonDarwin) to assert a unique message produced only by the non-darwin
branch — e.g., change wantErrContains to a phrase like "icon composer assets are
unsupported on non-macOS hosts" or "icon composer assets are only supported on
macOS" so the assertion can only pass when the non-darwin error path in icons.go
is hit. Ensure the new substring matches the exact error text emitted by the
non-darwin branch.
In `@v3/internal/commands/icons.go`:
- Around line 23-31: The macAssetNotSupportedError type currently only holds a
message and drops the original error; change its definition to include a cause
field (err error), implement Unwrap() error { return e.cause } and update
Error() to include both the message and the cause (so formatting and %w work);
then at the sites that return macAssetNotSupportedError (calls around
operatingsystem.Info(), cmd.Output(), and plist.Unmarshal in the
generateMacAsset flow) wrap the original error into the new
macAssetNotSupportedError (preserve the underlying error when constructing the
value) so errors.Is/As and %w chaining continue to work.
🪄 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: 64eb4f09-bf5f-47fa-b136-89305500fdc4
📒 Files selected for processing (2)
v3/internal/commands/icons.gov3/internal/commands/icons_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v3/internal/commands/icons.go (1)
174-200:⚠️ Potential issue | 🟡 MinorGenuine macOS toolchain failures now silently fall back to image-based generation.
Several new typed errors here cover real runtime failures on macOS, not unsupported environments:
- Line 176:
operatingsystem.Info()failed- Line 193:
/usr/bin/actool --versionexec failed (binary missing,xcode-selectmis-pointed, permission, sandbox, etc.)- Lines 199, 205, 210: plist parsing/structure unexpected
- Lines 180, 184, 217: version-string parse failures
Because
GenerateIcons(Lines 78–87) treats every*macAssetNotSupportedErroras fallback-eligible whenoptions.Inputis set, a macOS user with a broken Xcode/CLT setup now silently gets an image-based.icnswith noAssets.car/icons.icnsand no warning. Pre-PR these would have been wrappedfmt.Errorfpaths that propagated out as hard errors.Suggested options:
- Restrict
*macAssetNotSupportedErrorto the truly unsupported-environment branches (non-darwin,major < 26,actoolMajor < 26) and return wrappederrors for everything else, so they bubble up.- Or have
GenerateIconslog a warning before falling back so the user knows the Icon Composer path actually failed.This also subsumes the previously raised, still-unaddressed concern that an
actool --versionexec failure on Lines 192–194 is unconditionally blamed on Xcode 26+ — at minimum, include the underlyingerr(or distinguish "binary missing" from "exec error") in the message so the failure is diagnosable in either direction.♻️ Sketch: keep typed error only for unsupported environments
info, err := operatingsystem.Info() if err != nil { - return &macAssetNotSupportedError{message: "failed to determine macOS version"} + return fmt.Errorf("failed to determine macOS version: %w", err) } @@ cmd := exec.Command("/usr/bin/actool", "--version") versionPlist, err := cmd.Output() if err != nil { - return &macAssetNotSupportedError{message: "actool 26+ is required (install Xcode 26+)"} + return fmt.Errorf("failed to query actool --version (Xcode 26+ required): %w", err) } @@ if _, err := plist.Unmarshal(versionPlist, &plistData); err != nil { - return &macAssetNotSupportedError{message: "failed to parse actool version output"} + return fmt.Errorf("failed to parse actool version output: %w", err) }(version-tier branches at Lines 187 and 221 stay as
*macAssetNotSupportedError.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/icons.go` around lines 174 - 200, The code currently returns *macAssetNotSupportedError for both unsupported environments and runtime/tooling failures; change it so only genuine unsupported-environment checks return *macAssetNotSupportedError (e.g. non-darwin, parsed major < 26, actool reported major < 26), while all runtime failures (operatingsystem.Info() error, exec.Command("/usr/bin/actool").Output() error, plist.Unmarshal failures, strconv.Atoi/strings.Cut parse errors) return wrapped errors (fmt.Errorf with %w) that include the underlying err and a clear message; update the actool exec failure to include the underlying err (or distinguish binary-missing vs exec error) and adjust callers like GenerateIcons to keep fallback behavior unchanged for the true unsupported error type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@v3/internal/commands/icons.go`:
- Around line 174-200: The code currently returns *macAssetNotSupportedError for
both unsupported environments and runtime/tooling failures; change it so only
genuine unsupported-environment checks return *macAssetNotSupportedError (e.g.
non-darwin, parsed major < 26, actool reported major < 26), while all runtime
failures (operatingsystem.Info() error, exec.Command("/usr/bin/actool").Output()
error, plist.Unmarshal failures, strconv.Atoi/strings.Cut parse errors) return
wrapped errors (fmt.Errorf with %w) that include the underlying err and a clear
message; update the actool exec failure to include the underlying err (or
distinguish binary-missing vs exec error) and adjust callers like GenerateIcons
to keep fallback behavior unchanged for the true unsupported error type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c7eb367-9c1d-4303-988f-50d542d855a8
📒 Files selected for processing (2)
v3/internal/commands/icons.gov3/internal/commands/icons_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/internal/commands/icons.go (1)
23-31: Consider preserving the underlying cause onmacAssetNotSupportedError.Several branches (Lines 184, 197, 203, 221) discard a real error (
strconv.Atoi,cmd.Output,plist.Unmarshal) and flatten it into a static string. Adding acausefield plusUnwrap()keeps the typed-error behavior used byerrors.Aswhile retaining diagnostics for users hitting environment-specific failures — which is exactly the kind of opaque-error situation#5083was filed about.♻️ Suggested change
type macAssetNotSupportedError struct { message string + cause error } func (e *macAssetNotSupportedError) Error() string { + if e.cause != nil { + return fmt.Sprintf("%s: %v", e.message, e.cause) + } return e.message } + +func (e *macAssetNotSupportedError) Unwrap() error { + return e.cause +}…and populate
causeat sites that have one, e.g.:- major, err := strconv.Atoi(majorStr) - if err != nil { - return &macAssetNotSupportedError{message: fmt.Sprintf("failed to parse macOS version %q", info.Version)} - } + major, err := strconv.Atoi(majorStr) + if err != nil { + return &macAssetNotSupportedError{message: fmt.Sprintf("failed to parse macOS version %q", info.Version), cause: err} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/icons.go` around lines 23 - 31, Replace the current macAssetNotSupportedError struct with one that preserves an underlying cause: add a field `cause error` to macAssetNotSupportedError, implement an `Unwrap() error` method that returns `cause`, and update all sites that construct this error (e.g. in generateMacAsset where strconv.Atoi, cmd.Output, and plist.Unmarshal errors are currently discarded) to pass the original error into the new `cause` field so callers can use errors.As / errors.Is while retaining diagnostic details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/internal/commands/icons.go`:
- Around line 23-31: Replace the current macAssetNotSupportedError struct with
one that preserves an underlying cause: add a field `cause error` to
macAssetNotSupportedError, implement an `Unwrap() error` method that returns
`cause`, and update all sites that construct this error (e.g. in
generateMacAsset where strconv.Atoi, cmd.Output, and plist.Unmarshal errors are
currently discarded) to pass the original error into the new `cause` field so
callers can use errors.As / errors.Is while retaining diagnostic details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c8967c0-fe1f-418b-862f-a1281d298045
📒 Files selected for processing (2)
v3/internal/commands/icons.gov3/internal/commands/icons_test.go
barremian
force-pushed
the
v3-alpha-feat/5083-asset-gen-macos
branch
from
1cf914d to
52a3d9b
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
v3/internal/commands/icons_test.go (2)
197-211: Assert typed error in the non-Darwin unsupported-path test.This case only checks the message substring, so a future non-typed error with the same text would still pass. Since the table now supports typed assertions, set
wantUnsupported: truehere too.Suggested patch
{ name: "should return a descriptive error when icon composer assets are unsupported without fallback input", requireNonDarwin: true, setup: func(t *testing.T) *IconsOptions { // Get the directory of this file _, thisFile, _, _ := runtime.Caller(1) localDir := filepath.Dir(thisFile) exampleIcon := filepath.Join(localDir, "build_assets", "appicon.icon") return &IconsOptions{ IconComposerInput: exampleIcon, MacAssetDir: t.TempDir(), } }, wantErr: true, wantErrContains: "macOS 26 or later", + wantUnsupported: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/icons_test.go` around lines 197 - 211, The test case creating an IconsOptions with IconComposerInput and MacAssetDir currently only asserts the error message substring; update the table entry to also assert the typed unsupported error by adding wantUnsupported: true to that case so the runner checks for the specific unsupported error type (refer to the test table entry for the case name "should return a descriptive error when icon composer assets are unsupported without fallback input" and the IconsOptions struct used in setup).
75-89: Remove duplicateappicon.icocleanup in the same test.The file is already removed via
defer; removing it again in the body is redundant.Suggested patch
if f.Size() == 0 { return fmt.Errorf("appicon.ico is empty") } - // Remove the file - err = os.Remove("appicon.ico") - if err != nil { - return err - } return nil }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/icons_test.go` around lines 75 - 89, The test currently removes "appicon.ico" twice: once in the defer func() { _ = os.Remove("appicon.ico") } and again later with err = os.Remove("appicon.ico") / if err != nil { return err }. Remove the explicit removal block (the lines referencing err = os.Remove("appicon.ico") and its error-check) and rely on the existing defer cleanup; ensure any code path that previously returned after the second removal still returns appropriately without referencing err. This touches the test function using the variable f and the defer cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/internal/commands/icons_test.go`:
- Around line 197-211: The test case creating an IconsOptions with
IconComposerInput and MacAssetDir currently only asserts the error message
substring; update the table entry to also assert the typed unsupported error by
adding wantUnsupported: true to that case so the runner checks for the specific
unsupported error type (refer to the test table entry for the case name "should
return a descriptive error when icon composer assets are unsupported without
fallback input" and the IconsOptions struct used in setup).
- Around line 75-89: The test currently removes "appicon.ico" twice: once in the
defer func() { _ = os.Remove("appicon.ico") } and again later with err =
os.Remove("appicon.ico") / if err != nil { return err }. Remove the explicit
removal block (the lines referencing err = os.Remove("appicon.ico") and its
error-check) and rely on the existing defer cleanup; ensure any code path that
previously returned after the second removal still returns appropriately without
referencing err. This touches the test function using the variable f and the
defer cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cf25a57-da7b-47e8-b614-7c277d12953e
📒 Files selected for processing (2)
v3/internal/commands/icons.gov3/internal/commands/icons_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/internal/commands/icons.go
…detailed error messages
…nts to be more specific
…generating icon composer based icon assets in macos
barremian
force-pushed
the
v3-alpha-feat/5083-asset-gen-macos
branch
from
52a3d9b to
b7cb559
Compare
Member
Triaged ✓ — labeled Ready For Testing.
|
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
2 participants
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.
Description
Running
wails3 generate icons -iconcomposerinput <icon-file> -macassetdir <output-folder>on macOS < v26 throws the misleading error message "mac asset generation is only supported on macOS". This PR splits the common error message to specific error messages like "mac asset generation requires macOS 26 or later".Fixes #5083
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Tested locally in macOS 15 by building wails3 locally and running icon composer file based asset-gen command.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Bug Fixes
Tests