[v3] Fix Linux Desktop Template & Windows NSIS Template Added #4510
Conversation
WalkthroughAdds tests for build-assets generation and update, adjusts Linux desktop template to use top-level Changes
Sequence Diagram(s)sequenceDiagram
participant NSIS as NSIS Installer Script
participant REG as Windows Registry
rect #F0F9FF
NSIS->>REG: Delete HKCU\Software\Classes\{scheme} (cleanup)
NSIS->>REG: Write HKCU\Software\Classes\{scheme}\(Default)=Description
NSIS->>REG: Write HKCU\Software\Classes\{scheme}\URL Protocol=""
NSIS->>REG: Write HKCU\Software\Classes\{scheme}\DefaultIcon=(exe,0)
NSIS->>REG: Write HKCU\Software\Classes\{scheme}\shell\open\command="\"exe\" \"%1\""
end
Note over NSIS,REG: Repeat for each protocol in `.Protocols`
NSIS->>REG: Delete HKCU\Software\Classes\{scheme} on uninstall
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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)
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 |
dosubot
bot
added
the
size:L
dosubot
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
v3/internal/commands/build-assets_test.go (4)
13-18: Prefer t.TempDir() over manual temp dir managementUse t.TempDir() for automatic cleanup and simpler code.
Apply this diff:
- // Create a temporary directory for testing - tempDir, err := os.MkdirTemp("", "wails-build-assets-test-*") - if err != nil { - t.Fatalf("Failed to create temp directory: %v", err) - } - defer os.RemoveAll(tempDir) + // Create a temporary directory for testing + tempDir := t.TempDir()
119-131: Expected file names may be template-dependent; assert only invariant assets or introspect via fsHardcoding "config.yml" and "Taskfile.yml" can be brittle if templates change (e.g., YAML vs YML, or taskfile naming). Consider asserting a smaller invariant subset (e.g., appicon.png) and/or verifying presence using a contains-any approach.
If you want stronger verification, parse the generated config file (if present) to assert defaults (like ProductIdentifier) rather than asserting its exact filename.
133-139: Strengthen assertion: verify the derived ProductIdentifier from generated artifactsYou compute expectedIdentifier but don’t verify it. If the generator writes a config with ProductIdentifier, parse and assert it. Otherwise, factor out the defaulting logic so tests can assert without relying on file outputs.
I can propose a small refactor to expose/return the computed config for assertion—say, by returning the config from GenerateBuildAssets—or by adding a helper that applies defaults to BuildAssetsOptions for testing. Want me to draft that?
146-152: Use t.TempDir() here as wellMirror the earlier suggestion in TestUpdateBuildAssets.
Apply this diff:
- // Create a temporary directory for testing - tempDir, err := os.MkdirTemp("", "wails-update-assets-test-*") - if err != nil { - t.Fatalf("Failed to create temp directory: %v", err) - } - defer os.RemoveAll(tempDir) + // Create a temporary directory for testing + tempDir := t.TempDir()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
v3/internal/commands/build-assets_test.go(1 hunks)v3/internal/commands/updatable_build_assets/linux/desktop.tmpl(1 hunks)v3/internal/commands/updatable_build_assets/windows/nsis/wails_tools.nsh.tmpl(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
v3/internal/commands/build-assets_test.go (1)
v3/internal/commands/build-assets.go (6)
BuildAssetsOptions(33-52)GenerateBuildAssets(78-158)WailsConfig(178-190)ProtocolConfig(25-30)UpdateBuildAssetsOptions(62-75)UpdateBuildAssets(193-264)
⏰ 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). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
v3/internal/commands/updatable_build_assets/linux/desktop.tmpl (1)
14-15: Correct fix: reference top-level.Protocolsin desktop entrySwitching to
.Protocolsaligns with UpdateConfig/WailsConfig and resolves the reported template error during asset generation.v3/internal/commands/build-assets_test.go (1)
207-245: Good coverage of config/no-config and error pathThese cases validate both config-driven population and default-only updates, plus the error path for missing config files. Nice.
| !macro CUSTOM_PROTOCOL_ASSOCIATE PROTOCOL DESCRIPTION ICON COMMAND | ||
| DeleteRegKey SHELL_CONTEXT "Software\Classes\${PROTOCOL}" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "URL Protocol" "" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell" "" "" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open" "" "" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}" | ||
| !macroend | ||
|
|
||
| !macro CUSTOM_PROTOCOL_UNASSOCIATE PROTOCOL | ||
| DeleteRegKey SHELL_CONTEXT "Software\Classes\${PROTOCOL}" | ||
| !macroend | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use SHCTX for registry root (or define SHELL_CONTEXT) to ensure correct per-user/all-users context
NSIS supports the SHCTX root, which respects SetShellVarContext. Using unrecognized roots can break builds. This file’s earlier macros also use SHELL_CONTEXT; if that isn’t defined elsewhere, builds may fail. Two options:
- Define SHELL_CONTEXT as SHCTX once near the top (preferred for consistency).
- Or change these new macros to use SHCTX directly.
Minimal global alias (outside the selected range; add near the top after includes):
!ifndef SHELL_CONTEXT
!define SHELL_CONTEXT SHCTX
!endif
Alternatively, change within this block:
- DeleteRegKey SHELL_CONTEXT "Software\Classes\${PROTOCOL}"
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}"
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "URL Protocol" ""
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}"
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell" "" ""
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open" "" ""
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}"
+ DeleteRegKey SHCTX "Software\Classes\${PROTOCOL}"
+ WriteRegStr SHCTX "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}"
+ WriteRegStr SHCTX "Software\Classes\${PROTOCOL}" "URL Protocol" ""
+ WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}"
+ WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\shell" "" ""
+ WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\shell\open" "" ""
+ WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}"📝 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.
| !macro CUSTOM_PROTOCOL_ASSOCIATE PROTOCOL DESCRIPTION ICON COMMAND | |
| DeleteRegKey SHELL_CONTEXT "Software\Classes\${PROTOCOL}" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "URL Protocol" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell" "" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open" "" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}" | |
| !macroend | |
| !macro CUSTOM_PROTOCOL_UNASSOCIATE PROTOCOL | |
| DeleteRegKey SHELL_CONTEXT "Software\Classes\${PROTOCOL}" | |
| !macroend | |
| !macro CUSTOM_PROTOCOL_ASSOCIATE PROTOCOL DESCRIPTION ICON COMMAND | |
| DeleteRegKey SHCTX "Software\Classes\${PROTOCOL}" | |
| WriteRegStr SHCTX "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}" | |
| WriteRegStr SHCTX "Software\Classes\${PROTOCOL}" "URL Protocol" "" | |
| WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}" | |
| WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\shell" "" "" | |
| WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\shell\open" "" "" | |
| WriteRegStr SHCTX "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}" | |
| !macroend | |
| !macro CUSTOM_PROTOCOL_UNASSOCIATE PROTOCOL | |
| DeleteRegKey SHCTX "Software\Classes\${PROTOCOL}" | |
| !macroend |
🤖 Prompt for AI Agents
v3/internal/commands/updatable_build_assets/windows/nsis/wails_tools.nsh.tmpl
around lines 220-233: the new CUSTOM_PROTOCOL_* macros use SHELL_CONTEXT which
may be undefined and NSIS expects SHCTX (or a defined alias) for correct
per-user/all-users registry root; fix by defining SHELL_CONTEXT as SHCTX near
the top of the file (after includes) using a conditional !ifndef/!define block,
or alternatively replace SHELL_CONTEXT in these macros with SHCTX directly so
the registry root respects SetShellVarContext.
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "URL Protocol" "" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell" "" "" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open" "" "" | ||
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}" | ||
| !macroend |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote macro parameters with backticks to safely handle spaces and special chars
The existing APP_ASSOCIATE macro uses backticks when writing ${DESCRIPTION}, ${ICON}, ${COMMAND}. Mirror that here to avoid issues with spaces/quotes in descriptions or paths.
Apply this diff:
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}"
+ WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" `${DESCRIPTION}`
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}"
+ WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" `${ICON}`
- WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}"
+ WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" `${COMMAND}`If you adopt SHCTX per the previous comment, replace SHELL_CONTEXT with SHCTX accordingly in the same diff.
📝 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.
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" "${DESCRIPTION}" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "URL Protocol" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" "${ICON}" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell" "" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open" "" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" "${COMMAND}" | |
| !macroend | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "" `${DESCRIPTION}` | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}" "URL Protocol" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\DefaultIcon" "" `${ICON}` | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell" "" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open" "" "" | |
| WriteRegStr SHELL_CONTEXT "Software\Classes\${PROTOCOL}\shell\open\command" "" `${COMMAND}` | |
| !macroend |
🤖 Prompt for AI Agents
In v3/internal/commands/updatable_build_assets/windows/nsis/wails_tools.nsh.tmpl
around lines 222 to 228, the registry WriteRegStr calls use unquoted macro
parameters which can break on spaces/special characters; update each WriteRegStr
to quote the macro parameters with backticks (e.g. replace ${DESCRIPTION},
${ICON}, ${COMMAND} with `${DESCRIPTION}`, `${ICON}`, `${COMMAND}`) and, if you
adopted the SHCTX name from the previous comment, also replace SHELL_CONTEXT
with SHCTX consistently in these lines.
v3/internal/commands/updatable_build_assets/windows/nsis/wails_tools.nsh.tmpl
Show resolved
Hide resolved
|
Thanks 🙏 Please could you add an entry to the changelog located at |
github-actions
bot
added
the
Documentation
Absolutely, updated and pushed. Please let me know if is used correctly or not. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v3/UNRELEASED_CHANGELOG.md (2)
19-20: Align with changelog guidelines: use present tense and clarify scopePer the file’s own guidelines (Line 11), use present tense. Also, minor clarity/capitalization tweaks and optional PR reference.
Apply this diff:
- - Added NSIS Protocol template for Windows - - Added tests for build-assets + - Add NSIS protocol template for Windows to restore custom protocol support (#4510) + - Add tests for build-assets
26-26: Fix capitalization and accurately describe the template changeCapitalize Linux, use present tense, and correct the field names from singular to plural. “Protocol range” is unclear—be explicit about changing the iteration target.
Apply this diff:
- - Fixed linux desktop.tmpl protocol range, by removing `<.Info.Protocol>` to `<.Protocol>` + - Fix Linux desktop.tmpl protocols iteration: change `<.Info.Protocols>` to `<.Protocols>` (#4510)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
v3/UNRELEASED_CHANGELOG.md
[grammar] ~25-~25: There might be a mistake here.
Context: ... in existing functionality --> ## Fixed - Fixed linux desktop.tmpl protocol range, by r...
(QB_NEW_EN)
⏰ 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). (1)
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/internal/commands/build_assets/windows/nsis/project.nsi.tmpl (1)
100-112: Optional: unassociate protocols and file types before deleting files/shortcutsSlightly safer uninstall order is to unassociate first, then remove shortcuts and files. If unassociation fails (permissions), you avoid removing the binaries before the operation that references them. Functional impact is minor, hence optional.
Apply this diff within the uninstall section:
Section "uninstall" !insertmacro wails.setShellContext RMDir /r "$AppData\${PRODUCT_EXECUTABLE}" # Remove the WebView2 DataPath - RMDir /r $INSTDIR - - Delete "$SMPROGRAMS\${INFO_PRODUCTNAME}.lnk" - Delete "$DESKTOP\${INFO_PRODUCTNAME}.lnk" - - !insertmacro wails.unassociateFiles - !insertmacro wails.unassociateCustomProtocols + !insertmacro wails.unassociateCustomProtocols + !insertmacro wails.unassociateFiles + + Delete "$SMPROGRAMS\${INFO_PRODUCTNAME}.lnk" + Delete "$DESKTOP\${INFO_PRODUCTNAME}.lnk" + + RMDir /r $INSTDIR !insertmacro wails.deleteUninstaller SectionEnd
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v3/internal/commands/build_assets/windows/nsis/project.nsi.tmpl(2 hunks)
🔇 Additional comments (1)
v3/internal/commands/build_assets/windows/nsis/project.nsi.tmpl (1)
95-96: Protocol association macros verified — safe no-ops on empty.Protocolsand correct%1quoting
- The
wails.associateCustomProtocolsandwails.unassociateCustomProtocolsmacros are defined in
v3/internal/commands/updatable_build_assets/windows/nsis/wails_tools.nsh.tmpl.- They iterate over the top-level
.Protocolsfield, so when no protocols are specified the macros produce no registration calls.- The protocol launch command is correctly wrapped as
"$INSTDIR\${PRODUCT_EXECUTABLE} $\"%1$\""
which ensures the%1argument is properly quoted.No further changes required.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v3/UNRELEASED_CHANGELOG.md (2)
19-20: Use present tense and reference PR; clarify Windows NSIS entry.Align with the guideline “Use present tense (‘Add’ not ‘Added’)” and add the PR reference. Also clarify what the NSIS template does.
- - Added NSIS Protocol template for Windows - - Added tests for build-assets + - Add Windows NSIS template for protocol registration/unregistration (#4510) + - Add tests for build-assets (GenerateBuildAssets, UpdateBuildAssets) (#4510)
26-26: Fix capitalization/grammar and correct template identifiers.Use “Linux,” present tense, and the correct template fields (.Protocols vs .Info.Protocols). “Removing X to Y” should be “use X instead of Y.”
- - Fixed linux desktop.tmpl protocol range, by removing `<.Info.Protocol>` to `<.Protocol>` + - Fix Linux desktop.tmpl to use top-level `.Protocols` instead of `.Info.Protocols` (#4510)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
v3/UNRELEASED_CHANGELOG.md
[grammar] ~25-~25: There might be a mistake here.
Context: ... in existing functionality --> ## Fixed - Fixed linux desktop.tmpl protocol range, by r...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ange, by removing <.Info.Protocol> to <.Protocol> - Fixed redefinition error for liquid glas...
(QB_NEW_EN)
|
|
Thanks for this! |
2 participants
Description
Protocols is currently not working for Windows, since is missing the template in NSIS.
Linux desktop.tmpl is also broken, since it expects .Info.Protocols but Protocols is in top root.
Type of change
How Has This Been Tested?
Created new test in build-assets.
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation