Fix build assets update #3901
Fix build assets update #3901
Conversation
WalkthroughThe pull request introduces significant updates to the project's changelog, detailing new features, enhancements, and bug fixes. Key additions include new commands for managing build assets and runtime generation, updated window options, and a new configuration type for Wails projects. The changes also encompass various bug fixes across platforms and improvements in asset embedding and configuration handling. Overall, the updates reflect a comprehensive enhancement of the project's functionality and structure. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
v3/internal/commands/build-assets.go (2)
125-136: Consider adding field validation for WailsConfigThe new
WailsConfigtype is well-structured, but consider adding validation for required fields to ensure configuration completeness.Consider implementing a
Validate()method:func (c *WailsConfig) Validate() error { if c.Info.ProductName == "" { return fmt.Errorf("productName is required") } if c.Info.ProductIdentifier == "" { return fmt.Errorf("productIdentifier is required") } // Add other required field validations return nil }
Line range hint
125-170: Consider documenting the configuration precedenceWhile the new configuration system is a good improvement, it would be beneficial to document the precedence rules between command-line options and config file values. Consider adding this information to:
- User documentation
- Code comments
- Help text in CLI
Would you like me to help draft the documentation for this?
mkdocs-website/docs/en/changelog.md (1)
44-44: Enhance the changelog entry with more details.The current entry "Fixed build assets update" is quite brief. Consider expanding it to provide more context about what was fixed and its impact. This helps users understand the significance of the change.
Example format:
-Fixed build assets update by @ansxuman in [#3900](https://github.com/wailsapp/wails/pull/3900) +Fixed build assets update: [brief description of the specific issue and its impact] by @ansxuman in [#3900](https://github.com/wailsapp/wails/pull/3900)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/commands/build-assets.go(2 hunks)
🔇 Additional comments (2)
v3/internal/commands/build-assets.go (1)
169-169: Verify options assignment order
The assignment of options to config occurs after setting individual fields, which could lead to unexpected behavior if there are default values in the options struct.
Let's verify the usage of these options throughout the codebase:
✅ Verification successful
Let me gather more context about the specific code flow in the build-assets.go file.
Options assignment order is correct and safe
The code flow shows that all individual fields from the wailsConfig are assigned to the options struct before the final assignment to config.UpdateBuildAssetsOptions. This ensures that any values read from the config file properly override the defaults, and the final assignment captures all the updated values. There's no risk of unexpected behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to UpdateBuildAssetsOptions fields
rg -t go "UpdateBuildAssetsOptions\." --no-heading
# Search for direct field access
rg -t go "ProductName|ProductIdentifier|ProductCompany" --no-heading
Length of output: 2663
Script:
#!/bin/bash
# Get the full context around the options assignment
rg -t go -B 20 -A 10 "config.UpdateBuildAssetsOptions = \*options" v3/internal/commands/build-assets.go
Length of output: 1044
mkdocs-website/docs/en/changelog.md (1)
Line range hint 1-24: LGTM! The changelog structure follows best practices.
The changelog is well-organized, following the Keep a Changelog format with proper categorization of changes, consistent PR linking, and author attribution.
🧰 Tools
🪛 LanguageTool
[duplication] ~43-~43: Possible typo: you repeated a word
Context: ...ny](https://github.com/leaanthony) ### Fixed - Fixed build assets update by @ansxuman in [#3...
(ENGLISH_WORD_REPEAT_RULE)
| var wailsConfig WailsConfig | ||
| bytes, err := os.ReadFile(options.Config) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = yaml.Unmarshal(bytes, &config) | ||
| err = yaml.Unmarshal(bytes, &wailsConfig) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| options.ProductCompany = wailsConfig.Info.CompanyName | ||
| options.ProductName = wailsConfig.Info.ProductName | ||
| options.ProductIdentifier = wailsConfig.Info.ProductIdentifier | ||
| options.ProductDescription = wailsConfig.Info.Description | ||
| options.ProductCopyright = wailsConfig.Info.Copyright | ||
| options.ProductComments = wailsConfig.Info.Comments | ||
| options.ProductVersion = wailsConfig.Info.Version | ||
| config.FileAssociations = wailsConfig.FileAssociations |
There was a problem hiding this comment.
Add validation and error handling for config loading
Several critical issues need to be addressed in the config loading logic:
- Missing error handling for required fields
- No validation of loaded values
- Potential for silent overwriting of command-line options
Consider applying these improvements:
var wailsConfig WailsConfig
bytes, err := os.ReadFile(options.Config)
if err != nil {
return err
}
err = yaml.Unmarshal(bytes, &wailsConfig)
if err != nil {
return err
}
+// Validate required fields
+if err := wailsConfig.Validate(); err != nil {
+ return fmt.Errorf("invalid config file: %w", err)
+}
+// Log if command-line options are being overridden
+if !options.Silent && options.ProductName != "" &&
+ options.ProductName != wailsConfig.Info.ProductName {
+ fmt.Printf("Warning: Overriding ProductName from '%s' to '%s'\n",
+ options.ProductName, wailsConfig.Info.ProductName)
+}Committable suggestion skipped: line range outside the PR's diff.
|
Thank you! |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
Documentation