Repository navigation
fix: https://github.com/wailsapp/wails/issues/4636 - #4682
Conversation
WalkthroughAdds plist backup-and-merge to UpdateBuildAssets: existing Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant UpdateBuildAssets
participant FS as Filesystem
participant Temp as TempDir
participant PlistWorkflow
Caller->>UpdateBuildAssets: Invoke UpdateBuildAssets(opts)
UpdateBuildAssets->>FS: Locate target assets dir
UpdateBuildAssets->>PlistWorkflow: backupPlistFiles(target)
PlistWorkflow->>FS: Rename `*.plist` -> `*.plist.bak`
UpdateBuildAssets->>Temp: Create temp extraction dir
UpdateBuildAssets->>Temp: Extract archive into temp
UpdateBuildAssets->>Temp: Walk extracted files
alt file is plist
UpdateBuildAssets->>FS: Read extracted plist
UpdateBuildAssets->>PlistWorkflow: mergeBackupPlists(extractedPath)
PlistWorkflow->>FS: Read `*.plist.bak` (if exists)
PlistWorkflow->>PlistWorkflow: mergeMaps(backupMap, extractedMap)
PlistWorkflow->>FS: Write merged plist to target
else file is non-plist
UpdateBuildAssets->>FS: Copy file to target (preserve mode & path)
end
UpdateBuildAssets->>PlistWorkflow: cleanupBackups(target)
PlistWorkflow->>FS: Remove `*.plist.bak` files
UpdateBuildAssets->>Temp: Remove temp extraction dir
UpdateBuildAssets-->>Caller: Return result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/internal/commands/build-assets_test.go (1)
270-352: Consider expanding test coverage for edge cases.The test effectively verifies basic plist merging (updated keys and preserved keys), but additional test cases would strengthen confidence:
- New plist file creation (when target doesn't exist)
- Multiple plist files in nested directories
- Non-plist file copying (files other than .plist)
- Nested dictionary handling
- Array value merging
- Error scenarios (malformed plist, I/O errors)
These additions would help catch regressions and clarify expected behavior for edge cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/internal/commands/build-assets.go(3 hunks)v3/internal/commands/build-assets_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v3/internal/commands/build-assets_test.go (1)
v3/internal/commands/build-assets.go (2)
UpdateBuildAssetsOptions(63-76)UpdateBuildAssets(194-281)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
v3/internal/commands/build-assets.go (5)
16-16: LGTM!The plist library import is appropriate for the new merge functionality.
255-274: Well-structured two-phase extraction workflow.The temporary directory approach cleanly separates extraction from merging, allowing plist files to be merged while other files are copied directly. Error handling and cleanup are properly implemented.
287-324: Verify shallow merge behavior meets requirements.The merge logic performs a shallow (non-recursive) merge where each key from the new plist completely overwrites the corresponding key in the existing plist. If a key's value is a nested dictionary, the entire nested dictionary is replaced, not merged recursively.
Example:
- Existing:
{"LSApplicationQueriesSchemes": ["http", "https", "custom"]}- New:
{"LSApplicationQueriesSchemes": ["http"]}- Result:
{"LSApplicationQueriesSchemes": ["http"]}←"https"and"custom"are lostConfirm this aligns with the intended merge semantics for build assets. If users have custom nested configurations they want to preserve while updating top-level keys, a recursive merge would be needed.
326-362: LGTM!The directory walk, filtering, path computation, and merge invocation are all correctly implemented with proper error handling.
364-400: LGTM!The function correctly copies non-plist files while preserving file modes and directory structure. The inverse filtering logic (.plist files excluded) properly complements
mergePlistFiles.v3/internal/commands/build-assets_test.go (1)
9-9: LGTM!The plist import is necessary for unmarshaling and validating the merged plist content in the test.
github-actions
Bot
added
the
Documentation
|
Previously, the plist merge was shallow - nested dictionaries were completely replaced rather than recursively merged. This caused custom nested configurations to be lost during build asset updates. Now nested dictionaries are recursively merged, preserving custom keys at all levels while still allowing new keys to be added and existing keys to be updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/internal/commands/build-assets.go (1)
287-303: Consider enhancing the documentation.The merge logic is correct, but the comment could be more explicit about the precedence behavior: when types differ or values are not maps, the new value completely replaces the old value.
Consider this enhanced comment:
-// mergeMaps recursively merges src into dst. -// For nested maps, it merges recursively. For other types, src overwrites dst. +// mergeMaps recursively merges src into dst, modifying dst in place. +// - When both dst[key] and src[key] are maps: merges recursively +// - When types differ or values are not maps: src[key] completely replaces dst[key] +// - When key exists only in src: adds src[key] to dst +// - When key exists only in dst: preserves dst[key] func mergeMaps(dst, src map[string]any) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v3/UNRELEASED_CHANGELOG.md(1 hunks)v3/internal/commands/build-assets.go(3 hunks)v3/internal/commands/build-assets_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- v3/UNRELEASED_CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
v3/internal/commands/build-assets_test.go (1)
v3/internal/commands/build-assets.go (2)
UpdateBuildAssetsOptions(63-76)UpdateBuildAssets(194-281)
v3/internal/commands/build-assets.go (1)
v2/pkg/buildassets/buildassets.go (1)
ReadFile(48-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
v3/internal/commands/build-assets.go (5)
255-274: LGTM! Well-structured refactor for plist merging.The refactored flow correctly addresses the issue by:
- Extracting assets to a temporary directory first
- Merging plist files to preserve existing keys
- Copying non-plist files separately
- Using defer to ensure cleanup even on errors
This approach successfully prevents overwriting custom plist modifications.
342-378: LGTM! File walking and merging logic is sound.The function correctly:
- Walks the temp directory recursively
- Filters for .plist files
- Creates target directories as needed
- Merges each plist file into its target location
Note: Line 352 uses a case-sensitive suffix check. This should be fine since macOS conventions use lowercase
.plist, but be aware this won't catch.PLISTor other case variations if they exist.
380-416: LGTM! Non-plist file copying is implemented correctly.The function properly:
- Skips plist files (which are handled by mergePlistFiles)
- Copies all other files from temp to target
- Preserves file permissions using
info.Mode()- Creates target directories as needed
This complements the plist merging workflow well.
16-16: Keep thehowett.net/plistdependency updated and validate plist inputs as untrusted data.The library has no known public CVE vulnerabilities, but like all plist parsers, it should be kept current and used only with validated input. Ensure untrusted plist files are parsed in a controlled context with resource limits to prevent potential DoS vectors, as similar libraries in other ecosystems have had vulnerability history.
305-340: LGTM! Tab indentation is appropriate for plist files.The merge logic correctly handles:
- Parsing new plist content
- Reading existing plist (if present)
- Merging maps to preserve existing keys
- Writing the result back
Line 332 uses
encoder.Indent("\t")for tab indentation, which is valid for plist XML output. Plist indentation is purely cosmetic—Apple's system ignores it entirely, and Xcode has no enforced convention. If your team needs consistent indentation across projects, consider adding.editorconfigrules for*.plistfiles.v3/internal/commands/build-assets_test.go (3)
270-352: LGTM! Comprehensive integration test for plist merging.The test effectively validates the core fix:
- Creates an existing plist with custom keys (NSCameraUsageDescription, NSMicrophoneUsageDescription)
- Runs UpdateBuildAssets with new product information
- Verifies that:
- New values are properly updated (CFBundleName, CFBundleVersion, CFBundleIdentifier)
- Existing custom keys are preserved
This directly tests the bug fix described in issue #4636.
354-480: Excellent test coverage for merge scenarios!The table-driven tests comprehensively cover:
- Simple overwrites
- Key preservation
- Nested dictionary merging
- Deep nesting (multiple levels)
- Type conflicts (map ↔ non-map replacements)
The use of
deepCopyMapto avoid mutation during tests is good practice. The test cases validate that the merge behavior correctly implements the "preserve existing custom keys while updating standard keys" requirement.
482-516: LGTM! Helper functions are implemented correctly.Both
deepCopyMapandmapsEqualproperly handle nested structures:
deepCopyMaprecursively clones nested maps to avoid test mutation issuesmapsEqualrecursively compares maps including type checkingThese helpers enable thorough testing of the merge behavior.
Instead of extracting to a temp directory and copying files over, we now: 1. Rename existing plists to .plist.bak 2. Extract new assets directly to target 3. Merge backup content into newly extracted plists 4. Clean up backup files This is simpler, more crash-safe (backups remain if process crashes), and avoids the overhead of a temp directory and file copying. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@rxliuli thanks for this 🙏 I made a few modifications. LMK if you're good with these and will merge 👍 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
v3/internal/commands/build-assets.go (1)
364-375: Usedeferto ensure file is always closed.The file handle is closed explicitly in both error and success paths, but won't be closed if
encoder.Encodepanics. Usingdeferis more idiomatic and handles all edge cases.Apply this diff:
// Write merged result file, err := os.Create(backup.originalPath) if err != nil { return fmt.Errorf("failed to create merged plist %s: %w", backup.originalPath, err) } + defer file.Close() encoder := plist.NewEncoder(file) encoder.Indent("\t") if err := encoder.Encode(backupDict); err != nil { - file.Close() return fmt.Errorf("failed to encode merged plist %s: %w", backup.originalPath, err) } - file.Close() }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/internal/commands/build-assets.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
v3/internal/commands/build-assets.go (5)
16-16: LGTM: Appropriate library for plist handling.The howett.net/plist library is the standard Go library for working with property list files, and version 1.0.1 is specified in the library context.
255-274: LGTM: Backup-merge-cleanup workflow is well integrated.The workflow correctly implements the backup → extract → merge → cleanup sequence, with appropriate error handling for the critical steps.
287-303: LGTM: Merge semantics correctly preserve custom keys.The recursive merge properly implements the requirement: user-added custom keys (like LSApplicationCategoryType) are preserved, while template values update standard keys (like CFBundleVersion). The type assertions are safe and recursion will terminate at leaf values.
305-309: LGTM: Clear structure for tracking backups.
380-385: LGTM: Acceptable to ignore cleanup errors.For cleanup operations after the main merge has succeeded, ignoring
os.Removeerrors is reasonable. Leftover.plist.bakfiles won't impact functionality.
| // backupPlistFiles finds all .plist files in dir and renames them to .plist.bak | ||
| func backupPlistFiles(dir string) ([]plistBackup, error) { | ||
| var backups []plistBackup | ||
|
|
||
| err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if info.IsDir() || !strings.HasSuffix(path, ".plist") { | ||
| return nil | ||
| } | ||
|
|
||
| backupPath := path + ".bak" | ||
| if err := os.Rename(path, backupPath); err != nil { | ||
| return fmt.Errorf("failed to backup plist %s: %w", path, err) | ||
| } | ||
| backups = append(backups, plistBackup{originalPath: path, backupPath: backupPath}) | ||
| return nil | ||
| }) | ||
|
|
||
| return backups, err | ||
| } |
There was a problem hiding this comment.
Consider partial failure handling.
If os.Rename fails partway through the walk (e.g., permissions issue on one file), some plists will be backed up while others won't, with no rollback mechanism. This could leave the directory in an inconsistent state if the subsequent extraction proceeds.
However, since the error is returned and would prevent the extraction step from running, this is likely acceptable for a build tool.
🤖 Prompt for AI Agents
v3/internal/commands/build-assets.go lines 311-332: the current backupPlistFiles
renames .plist files to .plist.bak but does not undo partial progress if a later
os.Rename fails, leaving a mixed state; change the function to perform
atomic-like behavior by tracking successful backups and, if any rename returns
an error, iterate over the already-backed-up entries to restore them (rename
backupPath back to originalPath), accumulate and return the original error (or a
wrapped error that includes both the rename failure and any restore failures) so
callers see the root cause while the filesystem is left in the original state.
| newContent, err := os.ReadFile(backup.originalPath) | ||
| if err != nil { | ||
| // New file might not exist if template didn't generate one for this path | ||
| continue | ||
| } |
There was a problem hiding this comment.
Critical: User data loss when template doesn't generate a plist file.
If the newly extracted plist doesn't exist (line 349), the function continues without restoring or merging the backup. Since the original file was renamed to .plist.bak during backup, and cleanup will delete all .plist.bak files, the user's original plist file is permanently deleted.
Example scenario:
- User has a custom
CustomSettings.plistfile - Backup renames it to
CustomSettings.plist.bak - New template doesn't generate
CustomSettings.plist - Merge continues (line 352) without restoring the backup
- Cleanup deletes
CustomSettings.plist.bak - User's original file is gone
Apply this fix to restore the backup when the new file doesn't exist:
// Read the newly extracted plist
newContent, err := os.ReadFile(backup.originalPath)
if err != nil {
- // New file might not exist if template didn't generate one for this path
- continue
+ // New file doesn't exist; restore the backup
+ if err := os.Rename(backup.backupPath, backup.originalPath); err != nil {
+ return fmt.Errorf("failed to restore backup %s: %w", backup.backupPath, err)
+ }
+ continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| newContent, err := os.ReadFile(backup.originalPath) | |
| if err != nil { | |
| // New file might not exist if template didn't generate one for this path | |
| continue | |
| } | |
| newContent, err := os.ReadFile(backup.originalPath) | |
| if err != nil { | |
| // New file doesn't exist; restore the backup | |
| if err := os.Rename(backup.backupPath, backup.originalPath); err != nil { | |
| return fmt.Errorf("failed to restore backup %s: %w", backup.backupPath, err) | |
| } | |
| continue | |
| } |
Looks good, please merge it. |
|
|
Thanks! 🙏 |
* fix(v3): fixed update plist, close wailsapp#4636 * chore: update changelog * feat: add recursive merge support for nested plist dictionaries Previously, the plist merge was shallow - nested dictionaries were completely replaced rather than recursively merged. This caused custom nested configurations to be lost during build asset updates. Now nested dictionaries are recursively merged, preserving custom keys at all levels while still allowing new keys to be added and existing keys to be updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor: replace temp directory with backup-based plist merge Instead of extracting to a temp directory and copying files over, we now: 1. Rename existing plists to .plist.bak 2. Extract new assets directly to target 3. Merge backup content into newly extracted plists 4. Clean up backup files This is simpler, more crash-safe (backups remain if process crashes), and avoids the overhead of a temp directory and file copying. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
3 participants
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 #4636
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.