Add new MSI dialog to select workspaces folder path #182
Conversation
WalkthroughThe installer configuration has been updated to support user-defined workspace selection. A new property Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
🪧 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
CodeRabbit Configuration File (
|
subCode321
changed the title
subCode321
force-pushed
the
windows-installer
branch
from
e7c091d to
f16fbb6
Compare
subCode321
changed the title
| <!-- Custom Action: Write WORKSPACES_FOLDER to a text file --> | ||
| <CustomAction Id="WriteWorkspacePath" Directory="INSTALLFOLDER" Execute="deferred" Impersonate="no" ExeCommand="cmd.exe /c echo [WORKSPACES_FOLDER] > "[AppSubFolder]workspace_path.txt"" Return="ignore" /> | ||
There was a problem hiding this comment.
This i need to change to write to settings.json and update workspaces_dir key @t0mdavid-m
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🔇 Additional comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
412-439: New Workspace Selection Dialog IntegrationThe new
ChooseWorkspacesDlgis well integrated into the installer’s UI flow. The dialog provides clear instructions and contains the necessary controls—aPathEditbound to theWORKSPACES_FOLDERproperty and a "Change" button that triggers the browser dialog (BrowseDlg). Please verify that:
- The indirect binding (
Indirect="yes") for thePathEditcontrol properly reflects updates to the workspace folder.- The event sequence for the "Change" control (publishing the property and spawning
BrowseDlg) functions as expected.- The navigation between dialogs (via the publish events) seamlessly reflects the user’s selections in the final configuration.
These validations are essential to ensure the dialog behaves as intended.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 439-439: trailing spaces
(trailing-spaces)
362-362:Details
❓ Verification inconclusive
New Workspace Property Definition
The addition of the
<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />correctly defines a property for the workspace folder. Please ensure that the default value ("AppSubFolder") aligns with the intended default installation path and that it’s consistently used throughout the installer. Additionally, note that according to the PR objectives, the workspace selection should eventually update theworkspaces_dirkey insettings.json—consider outlining a follow-up task to integrate this update.
Workspace Property Consistency & Follow-up Task
The addition of
<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />in the workflow file appears correct. Please verify that:
- The default value "AppSubFolder" is indeed aligned with the intended default installation path.
- This value is used consistently throughout the installer.
- A follow-up task is outlined to update the
workspaces_dirkey insettings.jsonaccording to the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/build-windows-executable-app.yaml (3)
362-362: Workspace Folder Property DeclarationThe property
WORKSPACES_FOLDERis declared with the value"AppSubFolder". Please confirm that this value correctly represents the default workspaces folder and that it aligns with the later UI and custom action expectations. Once the user selects a new workspace, consideration should be given to updating this property accordingly.
404-404: Custom Action Execution OrderThe
<Custom Action="WriteWorkspacePath" Before="InstallFinalize">NOT Installed</Custom>entry schedules the custom action at the finalization stage. If you modify this action to updatesettings.json, please verify that its execution order continues to allow file accessibility and that no timing issues occur (e.g., the configuration file should be writable by this stage).
412-439: Workspace Selection Dialog Integration and Action FlowThe new dialog
ChooseWorkspacesDlgis well integrated into the installer’s UI, providing clear instructions for selecting a workspace folder. It binds thePathEditcontrol to theWORKSPACES_FOLDERproperty correctly. However, the PR objective stipulates that the selected workspace must update thesettings.jsonfile'sworkspaces_dirkey. Consider enhancing the workflow so that when the user confirms their choice:
- The installation process either calls an updated custom action or a separate post-installation script to modify
settings.jsonaccordingly.- This change is clearly tested and verified.
Do let me know if you want guidance or a sample implementation on how to perform this file update via a PowerShell script or through a direct custom action modification.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 439-439: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: build-openms
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
396-397: Remove Trailing Whitespace
Static analysis has flagged trailing spaces on these lines. Removing any extraneous whitespace will help maintain consistency with style guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
405-406: Custom Action Scheduling in InstallExecuteSequence
Placing<Custom Action="WriteWorkspacePath" Before="InstallFinalize">NOT Installed</Custom>appears appropriate. Please test to ensure that this action runs reliably before finalization and that any potential errors in updatingsettings.jsonare handled gracefully without adversely affecting the installation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: build-openms
🔇 Additional comments (4)
.github/workflows/build-windows-executable-app.yaml (4)
413-424: New Workspace Selection Dialog UI
The newly added dialogChooseWorkspacesDlgappears well structured. The controls—including thePathEditcontrol bound to theWORKSPACES_FOLDERproperty—should allow users to choose a workspace location easily. Verify that the default instructions and control placements match your UX guidelines and that the dialog handles both default and custom paths as expected.
425-438: Dialog Navigation and Publish Events
The publish events for transitioning between dialogs (e.g., fromLicenseAgreementDlgtoChooseWorkspacesDlg, and then toInstallDirDlg) are set up correctly. Ensure that the "Change" button properly triggers the built-inBrowseDlgvia the event:<Publish Dialog="ChooseWorkspacesDlg" Control="ChangeWorkspacesFolder" Property="_BrowseProperty" Value="[WORKSPACES_FOLDER]" Order="2">1</Publish> <Publish Dialog="ChooseWorkspacesDlg" Control="ChangeWorkspacesFolder" Event="SpawnDialog" Value="BrowseDlg" Order="2">1</Publish>This should enable the user to select an alternate workspace folder. Testing in the UI flow is recommended to confirm the expected behavior.
395-398:Details
❓ Verification inconclusive
Custom Action to Update settings.json
The custom actionWriteWorkspacePathnow uses a PowerShell command to read the existingsettings.json, update itsworkspaces_dirkey, and write it back. Please verify that:
- The path
[AppSubFolder]settings.jsoncorrectly resolves to the intended file location.- The property
[WORKSPACES_FOLDER]is suitably replaced with the user's selection at runtime.
Additionally, consider adding error handling or logging within the PowerShell command to assist with troubleshooting during installation failures.
Custom Action for Updating settings.json:
The updated custom actionWriteWorkspacePathnow uses a PowerShell command to modify theworkspaces_dirkey insettings.json. Please verify that the placeholder[AppSubFolder]settings.jsoncorrectly resolves to the intended file location and that[WORKSPACES_FOLDER]is properly replaced with the user's runtime selection.Consider enhancing the command by adding error handling and logging. This extra layer would capture JSON parsing or file write failures, making troubleshooting during installation more straightforward.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
362-363:Details
❓ Verification inconclusive
Workspace Property Declaration
The<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />element is now declared. Please verify that "AppSubFolder" is the correct default value for the workspace folder. This property is used both in the UI and when updatingsettings.json; ensure that when the user selects a custom workspace, the property is properly updated.
I'll start by running a shell script to search the repository for references to both
"AppSubFolder"and"WORKSPACES_FOLDER", as well as any connections tosettings.json. This will help us verify whether"AppSubFolder"is consistently used as the default workspace folder and that it's properly referenced in the UI and configuration updates.
🏁 Script executed:
#!/bin/bash echo "Searching for occurrences of 'AppSubFolder'..." rg "AppSubFolder" -n . echo -e "\nSearching for occurrences of 'WORKSPACES_FOLDER'..." rg "WORKSPACES_FOLDER" -n . echo -e "\nSearching for references to 'settings.json'..." rg "settings\.json" -n .Length of output: 1975
Workspace Default Value Verification
The workflow now declares
<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />as intended. I searched the repository and found no conflicting references for either"AppSubFolder"or"WORKSPACES_FOLDER", and thesettings.jsonusages appear consistent elsewhere. However, please verify that:
"AppSubFolder"is indeed the desired default value for the workspace folder.- The UI components and settings update logic correctly propagate custom workspace selections when a user changes the default.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 363-363: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
395-398: Review Custom Action for Updating settings.json
The custom action uses an inline PowerShell command to update theworkspaces_dirkey insettings.json. Ensure that:
- The file path
'[AppSubFolder]settings.json'is correctly resolved and properly quoted (especially if the path includes spaces).- Consider adding error handling (e.g., verifying that
settings.jsonexists before attempting to update it) to prevent installation errors.
This approach aligns with the PR objective to updatesettings.jsondirectly.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
396-397: Remove Trailing Spaces
Static analysis has flagged trailing spaces on these lines. Please remove the extra spaces to maintain clean YAML formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: build-openms
🔇 Additional comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
362-362: Ensure Correct Default Value for WORKSPACES_FOLDER Property
The<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />line sets the default workspace folder subdirectory. Please confirm that "AppSubFolder" is intentionally chosen as the default value and that it integrates correctly with the dialog and custom action logic downstream.
413-440: Workspace Selection Dialog Implementation
The newChooseWorkspacesDlgis well-structured with clear instructions and controls for selecting the workspace folder. ThePathEditcontrol correctly binds to theWORKSPACES_FOLDERproperty, and the associated publish events (e.g., for the "Change" button) are appropriate. Please verify that changes made through this dialog are properly propagated to the custom action that updatessettings.json.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 440-440: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build-windows-executable-app.yaml (1)
395-398: Inline Custom Action to Update settings.json
This custom action uses a PowerShell command to read the existingsettings.json, update itsworkspaces_dirkey using the[WORKSPACES_FOLDER]property, and then write the file back. This inline approach meets the PR objective of updatingsettings.jsondirectly rather than writing to a separate text file.A few points to consider:
- Error Handling: If
settings.jsondoes not exist, theGet-Contentcall will fail and cause the installer action to crash. It might be beneficial to add a check (e.g., viaTest-Path) or a try/catch block to handle the missing file gracefully.- Trailing Spaces: Static analysis noted trailing spaces on lines 396 and 397. Removing these will improve code cleanliness.
- Variable Expansion: Verify that MSI substitutes
[AppSubFolder]and[WORKSPACES_FOLDER]as expected so that the$settingsPathis correctly set.Below is a diff suggestion that includes an existence check for
settings.jsonand removes extra trailing whitespace:- ExeCommand="powershell.exe -NoProfile -ExecutionPolicy Bypass -Command "&{$settingsPath='[AppSubFolder]settings.json'; $settings = Get-Content -Raw $settingsPath | ConvertFrom-Json; $settings.workspaces_dir = '[WORKSPACES_FOLDER]'; $settings | ConvertTo-Json -Depth 100 | Set-Content $settingsPath}"" + ExeCommand="powershell.exe -NoProfile -ExecutionPolicy Bypass -Command "&{ + if (!(Test-Path '[AppSubFolder]settings.json')) { + Write-Error 'settings.json not found'; + exit 1; + } + $settingsPath='[AppSubFolder]settings.json'; + $settings = Get-Content -Raw $settingsPath | ConvertFrom-Json; + $settings.workspaces_dir = '[WORKSPACES_FOLDER]'; + $settings | ConvertTo-Json -Depth 100 | Set-Content $settingsPath + }""This enhanced command ensures that if the settings file is missing, the action fails with a clear error message rather than crashing silently.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-openms
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
🔇 Additional comments (3)
.github/workflows/build-windows-executable-app.yaml (3)
362-362: Workspace Property Definition
The new property<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />correctly sets the default value for the workspace folder. Ensure that this value is appropriately updated by the user’s selection in the dialog.
405-405: Custom Action Scheduling in InstallExecuteSequence
The<Custom Action="WriteWorkspacePath" Before="InstallFinalize">NOT Installed</Custom>scheduling places the custom action at the appropriate point during installation. The conditionNOT Installedis standard for new installs, ensuring the update is only performed on first install.
413-440: Integration of the Workspace Selector Dialog
The newChooseWorkspacesDlgdialog is well integrated into the WiX UI flow. The dialog’s controls—including thePathEditcontrol bound indirectly to theWORKSPACES_FOLDERproperty and the navigation buttons with appropriate publish events—are correctly set up. This effectively supports the PR objective of letting the user specify their workspace folder. Just verify that the dialog’s navigation events (e.g., transitions toLicenseAgreementDlgandInstallDirDlg) are working as intended in all installation scenarios.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 440-440: trailing spaces
(trailing-spaces)
@Arslan-Siraj What is your opinion on this? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
395-399: Custom Action for Updating settings.json
The custom actionWriteWorkspacePathnow attempts to update theworkspaces_dirkey insettings.jsonusing a PowerShell one-liner. Please verify that the file[AppSubFolder]settings.jsonexists at the expected location during installation. For clarity and maintainability, consider spacing out the PowerShell script block inside the command string. For example, adding spaces inside the script block (after the&{and before the closing}) can help readability and prevent misinterpretation of the variables.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
396-397: Remove Trailing Spaces
Static analysis has flagged trailing spaces on these lines. Removing these will help maintain code cleanliness and avoid potential formatting issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
362-362: New Workspace Property Declaration
The new<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />is declared correctly to set the default workspace folder. Ensure that “AppSubFolder” is intended to be the default folder name throughout the installer logic.
413-440: New Workspace Selection Dialog Integration
The new dialogChooseWorkspacesDlgis integrated with the proper controls (e.g., thePathEditcontrol forWORKSPACES_FOLDER) and navigation events to allow users to select the workspace location. The publish events correctly link this dialog into the installer flow. Ensure that the conditions and control IDs (e.g.,ChangeWorkspacesFolder) are consistent with your installer’s UX design.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 440-440: trailing spaces
(trailing-spaces)
I'm trying out an inline approach. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build-windows-executable-app.yaml (1)
395-398: 🛠️ Refactor suggestion
⚠️ Potential issueFix PowerShell Command Escaping in Custom Action
The custom actionWriteWorkspacePathis intended to updatesettings.jsonwith the selected workspace. However, the PowerShell command is missing a proper script block (e.g. using&{ ... }), which can cause variable resolution issues (as seen in previous error logs). Please update the command as follows to ensure the variables are preserved and executed correctly:- ExeCommand="powershell.exe -NoProfile -ExecutionPolicy Bypass -Command "`$settingsPath='[AppSubFolder]settings.json'; `$settings = Get-Content -Raw `$settingsPath | ConvertFrom-Json; `$settings.workspaces_dir = '[WORKSPACES_FOLDER]'; `$settings | ConvertTo-Json -Depth 100 | Set-Content `$settingsPath"" + ExeCommand="powershell.exe -NoProfile -ExecutionPolicy Bypass -Command "&{$settingsPath='[AppSubFolder]settings.json'; $settings = Get-Content -Raw $settingsPath | ConvertFrom-Json; $settings.workspaces_dir = '[WORKSPACES_FOLDER]'; $settings | ConvertTo-Json -Depth 100 | Set-Content $settingsPath}""This change wraps the commands in a script block, ensuring that
$settingsPathand other variables are correctly set and used.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
362-362: Verify Default WORKSPACES_FOLDER Property
The new WiX property<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />sets a default value for the workspace folder. Please verify that “AppSubFolder” is the appropriate and clearly understandable default for end users. If the intent is to have an absolute or more descriptive path for the workspaces location, consider updating it accordingly.
396-397: Remove Trailing Spaces in Custom Action
Static analysis indicates there are trailing spaces on lines 396 and 397. Please remove these extraneous spaces to improve readability and adhere to coding style guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 397-397: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-simple-app
- GitHub Check: build-openms
🔇 Additional comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
405-405: Review Custom Action Scheduling
The<Custom Action="WriteWorkspacePath" Before="InstallFinalize">NOT Installed</Custom>line schedules the custom action based on the conditionNOT Installed. Please verify that this condition is appropriate for both fresh installations and upgrade scenarios. In some cases, you might need to adjust the condition to ensure thatsettings.jsonis updated as expected during reinstallation or upgrades.
413-440: New Workspace Selection Dialog Integration
The newly added dialogChooseWorkspacesDlg(along with its associated controls and publish events) provides users with an interface to select a workspace location. The configuration includes:
- A text description and title explaining the dialog’s purpose.
- A
PathEditcontrol bound via theWORKSPACES_FOLDERproperty for capturing the user’s choice.- A “Change” button that spawns the standard Browse dialog for folder selection.
- Navigation buttons with publish events to control dialog flow.
Please ensure that:
- The text guiding the user is clear and that all instructions are properly localized if necessary.
- The publish events (e.g. transitioning from the LicenseAgreementDlg to the new dialog and then back to InstallDirDlg) follow the intended installation workflow.
- Testing confirms that the selected path is correctly propagated to the
WORKSPACES_FOLDERproperty for later use by the custom action.Overall, the dialog integration appears solid; just verify the end-to-end behavior during installation.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 440-440: trailing spaces
(trailing-spaces)
|
the issue I'm facing is that my custom action is getting converted to the following where |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build-windows-executable-app.yaml (1)
400-412: PowerShell Inline Script in WriteWorkspacePath Custom Action
This custom action uses an inline PowerShell command to update thesettings.jsonfile. The logic to split[CustomActionData], derive the settings file path, and update theworkspaces_dirlooks sound. However, please note the following:
- Trailing Spaces: Static analysis flags trailing spaces on lines 407–411. Cleaning these up will improve maintainability and avoid any potential parsing issues.
- Readability: The inline script spans multiple lines within the attribute value. Consider reformatting or adding inline comments in the XML for clarity if maintainability becomes an issue.
A suggested diff to remove trailing spaces could be:
- $props = @{}; ('[CustomActionData]' -split ';') | - ForEach-Object { $k,$v = $_ -split '=',2; $props[$k]=$v }; - $settingsPath = Join-Path $props.AppSubFolder 'settings.json'; - $settings = Get-Content -Raw -Path $settingsPath | ConvertFrom-Json; - $settings.workspaces_dir = $props.WORKSPACES_FOLDER; + $props = @{}; ('[CustomActionData]' -split ';') | + ForEach-Object { $k, $v = $_ -split '=', 2; $props[$k] = $v }; + $settingsPath = Join-Path $props.AppSubFolder 'settings.json'; + $settings = Get-Content -Raw -Path $settingsPath | ConvertFrom-Json; + $settings.workspaces_dir = $props.WORKSPACES_FOLDER;Ensure thorough testing of this custom action to verify that the JSON update works as intended.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-openms
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
🔇 Additional comments (3)
.github/workflows/build-windows-executable-app.yaml (3)
362-362: New Property Declaration: WORKSPACES_FOLDER
The property<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />is added to support the workspace selection feature. Ensure that using “AppSubFolder” as the folder name is consistent throughout the installer configuration.
396-399: Custom Action Data Passing Setup
The custom actionSetWriteWorkspacePathDatais correctly setting up the property data for the subsequent PowerShell action by passing:Value="AppSubFolder=[AppSubFolder];WORKSPACES_FOLDER=[WORKSPACES_FOLDER]"Make sure that both
[AppSubFolder]and[WORKSPACES_FOLDER]are being resolved as expected at runtime.
429-456: New Dialog for Workspace Selection (ChooseWorkspacesDlg)
The new dialogChooseWorkspacesDlgis integrated to allow users to select a workspace folder. The dialog layout—with controls for instructions, a path editor linked to theWORKSPACES_FOLDERproperty, and a "Change" button to invoke folder browsing—appears to meet the PR objectives. Please ensure thorough testing of the dialog’s navigation (Publish events) and the indirect property binding for the path edit control.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 456-456: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build-windows-executable-app.yaml (1)
420-421:⚠️ Potential issueSyntax Correction in InstallExecuteSequence
There is a syntax error in the InstallExecuteSequence where theWriteWorkspacePathcustom action is scheduled. The attribute value forBeforehas an extra double quote:- <Custom Action="WriteWorkspacePath" Before="InstallFinalize"">NOT Installed</Custom> + <Custom Action="WriteWorkspacePath" Before="InstallFinalize">NOT Installed</Custom>Removing the extra quote is necessary for proper MSI execution.
🧹 Nitpick comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
400-412: Inline PowerShell Command for settings.json Update
This custom action invokes PowerShell to updatesettings.jsonusing data from[CustomActionData]. The overall logic is sound; however, note that the inline command spans multiple lines and static analysis has flagged trailing whitespace issues (see lines 407–411). It is recommended to remove any extraneous spaces to ensure cleaner command execution.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
407-411: Remove Trailing Whitespace in PowerShell Command
Static analysis reported trailing spaces on these lines within the PowerShell command. Removing these extra spaces can improve clarity and help avoid subtle formatting issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-simple-app
- GitHub Check: build-openms
🔇 Additional comments (3)
.github/workflows/build-windows-executable-app.yaml (3)
362-362: New Workspace Folder Property Declaration
The added property<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />establishes the default workspace folder value and makes it available to other custom actions. Please verify that the directory with Id "AppSubFolder" is properly defined in the Directory tree so that the property resolves as intended.
395-399: CustomActionData Setup for Workspace Path Update
The custom actionSetWriteWorkspacePathDatapasses the valuesAppSubFolder=[AppSubFolder];WORKSPACES_FOLDER=[WORKSPACES_FOLDER]to the deferredWriteWorkspacePathaction. This setup is key for ensuring that the PowerShell script later receives the correct folder paths. Double-check that both[AppSubFolder]and[WORKSPACES_FOLDER]are resolving correctly at runtime.
429-456: Workspace Selection Dialog Integration
The newly added dialogChooseWorkspacesDlgintroduces a UI for selecting the workspace folder. The controls—including the PathEdit (bound viaIndirect="yes"to theWORKSPACES_FOLDERproperty) and the accompanying navigation buttons—appear to be correctly implemented. Please verify that all Publish events (for navigation and property setting) function as expected during installation.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 456-456: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
400-412: Implement PowerShell Inline Script with Custom ActionData
The inline PowerShell script in the<CustomAction Id="WriteWorkspacePath" ... />block uses CustomActionData to retrieve the folder paths and update the JSON settings. The usage of the following snippet:ExeCommand="powershell.exe -NoProfile -ExecutionPolicy Bypass -Command " $props = @{}; ('[CustomActionData]' -split ';') | ForEach-Object { $k,$v = $_ -split '=',2; $props[$k]=$v }; $settingsPath = Join-Path $props.AppSubFolder 'settings.json'; $settings = Get-Content -Raw -Path $settingsPath | ConvertFrom-Json; $settings.workspaces_dir = $props.WORKSPACES_FOLDER; $settings | ConvertTo-Json -Depth 100 | Set-Content -Path $settingsPath""appears logically sound. A few points to consider:
- Trailing Whitespace: Static analysis reports trailing spaces on lines 407–411. Removing these extraneous spaces can help avoid subtle issues.
- Error Handling: It might be beneficial to incorporate additional error capture (for example, exiting with a non-zero code or writing to the log) if JSON conversion fails.
- Verification of Data: Ensure that the values passed via CustomActionData (i.e.
AppSubFolderandWORKSPACES_FOLDER) are correctly set prior to execution.Suggested Diff to Remove Trailing Whitespace:
- $props = @{}; ('[CustomActionData]' -split ';') |␣ - ForEach-Object { $k,$v = $_ -split '=',2; $props[$k]=$v };␣ - $settingsPath = Join-Path $props.AppSubFolder 'settings.json';␣ - $settings = Get-Content -Raw -Path $settingsPath | ConvertFrom-Json;␣ - $settings.workspaces_dir = $props.WORKSPACES_FOLDER;␣ + $props = @{}; ('[CustomActionData]' -split ';') | + ForEach-Object { $k,$v = $_ -split '=',2; $props[$k]=$v }; + $settingsPath = Join-Path $props.AppSubFolder 'settings.json'; + $settings = Get-Content -Raw -Path $settingsPath | ConvertFrom-Json; + $settings.workspaces_dir = $props.WORKSPACES_FOLDER;🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
429-456: New Workspace Selection Dialog Integration
The newly introduced dialog,ChooseWorkspacesDlg, enhances user experience by allowing selection of the workspace location. Key observations:
- The dialog provides clear instructions with the WorkspacesFolderLabel and includes a
<PathEdit>control bound (viaIndirect="yes") to theWORKSPACES_FOLDERproperty.- The publish events properly manage navigation between dialogs (for example, spawning the BrowseDlg on button click).
- Verify that the default value for the workspaces path (i.e. "AppSubFolder") is correctly resolved to the installation folder path during runtime.
Overall, this integration looks promising; ensure that the BrowseDlg referenced in the publish event is correctly defined elsewhere in your WiX project.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 456-456: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build-windows-executable-app.yaml
[error] 407-407: trailing spaces
(trailing-spaces)
[error] 408-408: trailing spaces
(trailing-spaces)
[error] 409-409: trailing spaces
(trailing-spaces)
[error] 410-410: trailing spaces
(trailing-spaces)
[error] 411-411: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-simple-app
- GitHub Check: build-openms
🔇 Additional comments (3)
.github/workflows/build-windows-executable-app.yaml (3)
362-362: Define Workspace Property Consistently
The addition of the property<Property Id="WORKSPACES_FOLDER" Value="AppSubFolder" />ensures that the workspace folder default is centralized. Please verify that the value "AppSubFolder" is aligned in all references in your installer configuration.
395-398: Pass Custom Action Data Correctly
The new custom action element:<CustomAction Id="SetWriteWorkspacePathData" Property="WriteWorkspacePath" Value="AppSubFolder=[AppSubFolder];WORKSPACES_FOLDER=[WORKSPACES_FOLDER]" />passes required variables using the CustomActionData mechanism. Verify that both placeholders resolve as expected during installation.
420-422: Custom Action Sequencing in InstallExecuteSequence
The scheduling of custom actions:<Custom Action="SetWriteWorkspacePathData" Before="WriteWorkspacePath">NOT Installed</Custom> <Custom Action="WriteWorkspacePath" Before="InstallFinalize">NOT Installed</Custom>ensures that the custom action data is set before executing the PowerShell script and that the action runs before installation finalization. Double-check that the condition
NOT Installedis appropriate for your installation scenarios.
|
@Arslan-Siraj , awaiting your response |
|
@Arslan-Siraj I would need your help on this! |
2 participants
Fixes #173
Description
Currently, we store the workspaces location inside
workspaces_dirinsidesettings.jsonand this new workspaces selector window will ask user forworkspaceslocation and update theworkspaces_dirkey insidesettings.jsonFix
C:\Program Files (x86)\OpenMS-StreamlitTemplateApp\OpenMS-StreamlitTemplateAppworkspaces_dir. Currently, I'm writing it toC:\Program Files (x86)\OpenMS-StreamlitTemplateApp\OpenMS-StreamlitTemplateApp\workspaces_path.txt.@t0mdavid-m .... I've read a lot of blogs and it's being suggested everywhere that to update a
JSONfile I'll have to create a new.exeor.ps1that needs to be packaged with the.msiinstaller that will run once workspace is selected and the script will read the content ofsettings.jsonand updated theworkspaces_dirkey. Is this the way you would want to take this forward or you have some other suggestions. Also, the current.msiinstaller is working and you can also give it a try.Untitled.video.-.Made.with.Clipchamp.8.mp4
Summary by CodeRabbit
Summary by CodeRabbit