Conversation
Refactored helper functions for clarity and consistency, including renaming and parameter changes. Added support for downloading and installing websocat, updated artifact URLs and extraction logic, and improved architecture-specific handling for optional tools. Enhanced output messages and error handling throughout the script.
Reviewer's GuideRefactors installer script by standardizing helper functions (renaming parameters and generalizing extraction routines), introduces websocat support with per-architecture conditional download/extract, updates artifact URLs and handling, and enhances logging and error handling. Sequence diagram for architecture-specific download and install including websocatsequenceDiagram
participant Script
participant "Python"
participant "curl"
participant "YARA" as YARA
participant "WAMR" as WAMR
participant "websocat" as websocat
participant "artifacts" as artifacts
Script->>"Python": Download-File
Script->>"curl": Download-File
alt If YARAUrl exists
Script->>YARA: Download-File
else
Script->>YARA: Skip download
end
alt If WamrUrl exists
Script->>WAMR: Download-File
else
Script->>WAMR: Skip download
end
alt If WebsocatUrl exists
Script->>websocat: Download-File
else
Script->>websocat: Skip download
end
alt If ArtifactsUrl exists
Script->>artifacts: Download-File
else
Script->>artifacts: Skip download
end
Script->>"Python": Extract-CompressedFile
Script->>"curl": Extract-CompressedFile
alt If YARAUrl exists
Script->>YARA: Extract-CompressedFile
else
Script->>YARA: Skip install
end
alt If WamrUrl exists
Script->>WAMR: Extract-TarGzArchive
else
Script->>WAMR: Skip install
end
alt If WebsocatUrl exists
Script->>websocat: Extract-CompressedFile
else
Script->>websocat: Skip install
end
alt If ArtifactsUrl exists
Script->>artifacts: Extract-CompressedFile
else
Script->>artifacts: Skip install
end
Class diagram for refactored helper functions in install scriptclassDiagram
class Ensure_EmptyDirectory {
+Path: string
+Ensure directory exists
+Remove file if exists
}
class Download_File {
+Url: string
+DestinationPath: string
+Download file with error handling
}
class Extract_CompressedFile {
+CompressedPath: string
+DestinationDirectory: string
+Extract ZIP/Archive
+Flatten single root directory
+Move multiple entries
}
class Extract_TarGzArchive {
+ArchivePath: string
+DestinationDirectory: string
+Extract TAR.GZ using tar
}
Ensure_EmptyDirectory <|-- Extract_CompressedFile
Ensure_EmptyDirectory <|-- Extract_TarGzArchive
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@gnh1201 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRenames extraction helpers and multiple parameters in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Installer as afterInstall.ps1
participant FS as Filesystem
participant Net as Downloader
participant WS as lib/websocket.js
participant SYS as lib/system.js
User->>Installer: run installer
Installer->>FS: Ensure-EmptyDirectory(DestinationDirectory)
Installer->>Net: Download-File(Url, DestinationPath)
Net-->>Installer: file/archive
alt compressed archive (zip)
Installer->>Installer: Extract-CompressedFile(CompressedPath, DestinationDirectory)
else tar.gz
Installer->>Installer: Extract-TarGzArchive(ArchivePath, DestinationDirectory)
end
alt optional component supported by arch
Installer->>Net: Download optional component
Net-->>Installer: optional archive
Installer->>Installer: Extract -> DestinationDirectory
else not supported by arch
Installer->>User: log "skipping optional component for this architecture"
end
Installer->>FS: cleanup temp extraction dirs
WS->>FS: check app-data for websocat.exe (via SYS.getAppDataDir)
alt websocat exists
WS->>Installer: setBinPath(app-data websocat.exe)
else not exists
WS->>Installer: pick bundled 64/32-bit binary
end
SYS->>FS: return AppDataDir (note: path casing changed)
Installer->>User: "Installation completed successfully."
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The repeated pattern of checking optional URLs and invoking Download-File/Extract could be refactored into a data-driven loop or helper to reduce boilerplate and ensure consistent skip messages.
- Correct the typo in the FATAL download error message (“faled” → “failed”) to prevent confusion when diagnosing failures.
- Consider validating each download URL before starting the download phase to catch misconfigurations early and surface clear errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated pattern of checking optional URLs and invoking Download-File/Extract could be refactored into a data-driven loop or helper to reduce boilerplate and ensure consistent skip messages.
- Correct the typo in the FATAL download error message (“faled” → “failed”) to prevent confusion when diagnosing failures.
- Consider validating each download URL before starting the download phase to catch misconfigurations early and surface clear errors.
## Individual Comments
### Comment 1
<location> `afterInstall.ps1:322` </location>
<code_context>
}
catch {
- Write-Host "[FATAL] Download phase failed."
+ Write-Host "[FATAL] Download phase faled."
+ Write-Host $_.Exception.Message
exit 1
</code_context>
<issue_to_address>
**issue (typo):** Typo in error message: 'faled' should be 'failed'.
Please update the error message to use the correct spelling.
```suggestion
Write-Host "[FATAL] Download phase failed."
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
afterInstall.ps1 (1)
271-271: Clarify the ordering requirement or remove the comment.The comments at lines 271 and 329 state "websocat before artifacts" but no explicit dependency between these components is evident in the code. If there's a specific reason websocat must be downloaded/installed before artifacts, please document it. Otherwise, consider removing these comments as they may cause confusion.
Also applies to: 329-329
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
afterInstall.ps1(5 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
afterInstall.ps1 (4)
34-176: LGTM! Well-executed refactoring of helper functions.The function renames and parameter name changes significantly improve code clarity and follow PowerShell naming conventions more closely. All usages have been updated consistently throughout the script.
331-374: LGTM! Extraction and installation logic is well-structured.The conditional extraction logic for optional components is clear and consistent with the download phase. All function calls correctly use the refactored function names and parameters.
229-229: No issues found. The URL is valid and accessible.The curl URL at line 229 returns an HTTP 302 redirect with proper headers, confirming the file exists at SourceForge. The date "2025-11-09" in the filename is not in the future—relative to the current system date of November 21, 2025, it is in the past. The original review comment's concern about the date was based on an incorrect assumption. The code is correct as written.
192-208: All URLs verified as accessible and valid.The verification confirms all download URLs return successful responses: Python, curl, YARA, WAMR, websocat, and artifacts URLs are all accessible with appropriate HTTP 200 or 302 responses. Content-Length headers confirm file availability for Azure blob storage resources. No issues detected.
Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
afterInstall.ps1 (1)
322-322: Fix typo in error message.
🧹 Nitpick comments (2)
afterInstall.ps1 (2)
287-319: Consider abstracting repetitive conditional download logic.The conditional download pattern is repeated for YARA, WAMR, websocat, and artifacts. While the current implementation is correct and clear, you could reduce duplication with a helper function.
Example approach:
function Download-Optional { param( [Parameter(Mandatory = $true)] [string]$Name, [Parameter(Mandatory = $false)] [string]$Url, [Parameter(Mandatory = $true)] [string]$DestinationPath ) if ($Url) { Download-File -Url $Url -DestinationPath $DestinationPath } else { Write-Host "[*] $Name download skipped on this architecture." } }Then use it as:
Download-Optional -Name "YARA" -Url $YaraUrl -DestinationPath $YaraCompressed Download-Optional -Name "WAMR" -Url $WamrUrl -DestinationPath $WamrArchive Download-Optional -Name "websocat" -Url $WebsocatUrl -DestinationPath $WebsocatCompressed Download-Optional -Name "artifacts" -Url $ArtifactsUrl -DestinationPath $ArtifactsCompressed
338-368: Conditional extraction logic is correct; consider similar abstraction.The extraction phase correctly mirrors the download phase with proper conditional logic. Similar to the download phase, this could benefit from the same abstraction pattern if you choose to refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
afterInstall.ps1(5 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
afterInstall.ps1 (2)
34-176: Helper function refactoring looks good.The renaming and parameter standardization improves clarity and consistency. The extraction logic correctly handles both single-root and multi-root archives, and the
-Forceflag at line 119 ensures hidden files are extracted.
380-380: Improved success message is more accurate.The updated message "Installation completed successfully." is more appropriate than "All tools installed successfully." since some optional tools may be skipped based on architecture.
| # WAMR (x64 only) | ||
| $WamrUrl = "https://github.com/bytecodealliance/wasm-micro-runtime/releases/download/WAMR-2.4.3/iwasm-2.4.3-x86_64-windows-2022.tar.gz" | ||
| # websocat (x64) | ||
| $WebsocatUrl = "https://catswords.blob.core.windows.net/welsonjs/websocat-1.14.0.x86_64-pc-windows-gnu.zip" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify websocat download URLs exist and are accessible.
The websocat URLs point to Azure Blob Storage. Ensure these URLs are valid and publicly accessible.
Also applies to: 238-238
🏁 Script executed:
#!/bin/bash
# Description: Verify websocat download URLs are accessible
echo "Testing x64 websocat URL..."
curl -I "https://catswords.blob.core.windows.net/welsonjs/websocat-1.14.0.x86_64-pc-windows-gnu.zip" 2>&1 | head -n 1
echo ""
echo "Testing x86 websocat URL..."
curl -I "https://catswords.blob.core.windows.net/welsonjs/websocat-1.14.0.i686-pc-windows-gnu.zip" 2>&1 | head -n 1Length of output: 216
Replace custom Azure Blob Storage URLs with official GitHub release URLs for improved reliability.
The websocat binaries are being downloaded from a custom Azure Blob Storage account (catswords.blob.core.windows.net) that cannot be verified to exist or be publicly accessible. Official websocat 1.14.0 binaries are available on GitHub releases:
- x64:
https://github.com/vi/websocat/releases/download/v1.14.0/websocat.x86_64-pc-windows-gnu.exe - x86:
https://github.com/vi/websocat/releases/download/v1.14.0/websocat.i686-pc-windows-gnu.exe
Consider updating lines 204 and 238 to use the official GitHub URLs for better reliability and maintainability, or document why the custom blob storage is necessary.
🤖 Prompt for AI Agents
In afterInstall.ps1 around lines 204 and 238, the script downloads websocat from
a custom Azure Blob Storage URL which is not verifiable; replace those blob URLs
with the official GitHub release URLs for websocat v1.14.0 (use the x64 GitHub
URL for the x86_64 target and the i686 GitHub URL for the x86 target) or, if the
custom blob is required, add a comment explaining why and ensure the blob is
publicly accessible and trustworthy; update the two lines accordingly so they
point to the official GitHub release assets (or include the documented
justification).
| catch { | ||
| Write-Host "[FATAL] Download phase failed." | ||
| Write-Host "[FATAL] Download phase faled." | ||
| Write-Host $_.Exception.Message |
There was a problem hiding this comment.
Suggestion: Fix typo in error message
| catch { | |
| Write-Host "[FATAL] Download phase failed." | |
| Write-Host "[FATAL] Download phase faled." | |
| Write-Host $_.Exception.Message | |
| catch { | |
| Write-Host "[FATAL] Download phase failed." | |
| Write-Host $_.Exception.Message |
Enhanced error reporting by checking if caught errors are System.Exception and printing the appropriate message. This provides clearer output for both exception and non-exception error types during file download, extraction, and main script execution.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
afterInstall.ps1 (2)
81-87: Function naming: "Extract-CompressedFile" is too generic for ZIP-specific functionality.The function is renamed from
Extract-ZiptoExtract-CompressedFile, but it specifically usesExpand-Archive, which only works with ZIP files in PowerShell. The generic name "CompressedFile" suggests it could handle other formats (e.g., RAR, 7z), but it cannot.Consider either:
- Reverting to
Extract-Zipor usingExtract-ZipArchivefor consistency withExtract-TarGzArchive.- If a generic name is desired, document in comments that only ZIP format is supported.
378-384: Clarify artifacts extraction to bin/ directory.Unlike other components that extract to their own subdirectories (e.g.,
python/,curl/,yara/), artifacts extract directly to thebin/directory. This suggests the artifacts.zip contains executable binaries that should be available in a common bin path.Consider adding a comment explaining that artifacts contain shared binaries to make the directory choice clear.
# artifacts if ($ArtifactsUrl) { + # Extract WelsonJS binary artifacts to shared bin/ directory Extract-CompressedFile -CompressedPath $ArtifactsCompressed -DestinationDirectory (Join-Path $TargetDir "bin")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
afterInstall.ps1(4 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
afterInstall.ps1 (6)
72-76: LGTM: Improved error handling with exception type check.The error handling now checks whether
$_is aSystem.Exceptionbefore accessing.Exception.Message, preventing potential errors if$_is not an exception object.
127-127: LGTM: Hidden files are now included in extraction.The addition of the
-Forceparameter toGet-ChildItemensures hidden files are extracted from the archive.
292-332: LGTM: Clean conditional download logic with consistent skip messaging.The download phase correctly handles optional components with architecture-aware conditional logic and provides clear skip messages when components are not available for the current architecture.
400-400: LGTM: Updated success message is more accurate.The message "Installation completed successfully." is more appropriate than "All tools installed successfully." since optional components may be skipped based on architecture.
219-219: Artifacts.zip URL is publicly accessible and operational.Verification confirms the Azure Blob Storage URL returns HTTP 200 OK with a valid blob file (37.5 MB). The artifacts download for both architectures is currently functional.
241-241: The curl URL is correct—no changes needed.The latest curl for Windows x86 on SourceForge is curl 8.17.0, which was uploaded on 2025-11-09. The date in the URL is not a typo; it reflects the actual upload date of the current latest version.
Refactored default WebSocket executable path selection to check for a user-specific installation before falling back to architecture-based defaults. Updated comments for clarity and incremented version to 0.2.4.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/websocket.js (1)
23-39: Default path casing must match afterInstall.ps1 configuration.The enhanced logic is sound, but verification reveals a path casing inconsistency. afterInstall.ps1 installs websocat to
%APPDATA%\welsonjs\websocat, while websocket.js expects%APPDATA%\WelsonJS\websocat(as returned by SYS.getAppDataDir()). While Windows resolves this due to case-insensitivity, the inconsistent casing creates a maintenance issue.Fix the casing inconsistency:
Either update afterInstall.ps1 to use
"WelsonJS"(to match SYS.getAppDataDir()), or update SYS.getAppDataDir() to use lowercase"welsonjs". Recommend standardizing on"WelsonJS"since it's already hardcoded in SYS.getAppDataDir().Also apply the modernization improvements:
- Use
constinstead ofvarfordefault_websocat_pathandarch(lines 27, 31)- Use template literals for Windows paths to improve readability
🧹 Nitpick comments (1)
lib/websocket.js (1)
16-16: Prefer const for module imports.The FILE module import should use
constinstead ofvarfor consistency with modern JavaScript best practices, as module imports are immutable constants.Apply this diff:
-var FILE = require("lib/file"); +const FILE = require("lib/file");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/websocket.js(3 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
lib/websocket.js
[failure] 27-27: Unexpected var, use let or const instead.
[warning] 27-27: String.raw should be used to avoid escaping \.
⏰ 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
lib/websocket.js (2)
1-13: Documentation improvements enhance clarity.The updated header comments add a helpful compatibility note directing users to the WelsonJS Launcher for more fine-grained WebSocket control, while clarifying this file exists for backward compatibility. The references provide useful context.
79-79: Appropriate version bump for new functionality.The version increment from 0.2.3 to 0.2.4 correctly reflects the enhanced setBinPath functionality. The minor text adjustments improve consistency.
Changed the app data directory from 'WelsonJS' to 'welsonjs' for consistency. Bumped VERSIONINFO to 0.1.7 to reflect the update.
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)
lib/system.js(2 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
lib/system.js (1)
227-227: LGTM!The version bump from 0.1.6 to 0.1.7 is appropriate for this release.
|
|
||
| function getAppDataDir() { | ||
| return getEnvString("APPDATA") + "\\WelsonJS"; | ||
| return getEnvString("APPDATA") + "\\welsonjs"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify migration strategy for existing installations.
The AppData path case change from "\\WelsonJS" to "\\welsonjs" will cause the application to use a different directory. On Windows, although the filesystem is case-insensitive for lookups, it is case-preserving, meaning existing installations with data in %APPDATA%\WelsonJS will not automatically be found at %APPDATA%\welsonjs. This could lead to:
- Loss of user data, configurations, or cached files
- Breaking existing installations without a clear migration path
- Silent failures if the code expects to find previously stored data
Ensure that a migration strategy is in place (e.g., checking for the old directory and copying/moving data to the new location) or document this as a breaking change requiring manual user action.
Run the following script to verify what data is stored in this directory and if migration logic exists:
🏁 Script executed:
#!/bin/bash
# Description: Check usage of getAppDataDir and verify if migration logic exists
# Search for all usages of getAppDataDir to understand what data is stored
echo "=== Searching for getAppDataDir usage ==="
rg -n "getAppDataDir" -A 3 -B 1
# Search for any migration logic or references to the old "WelsonJS" path
echo -e "\n=== Searching for references to old WelsonJS path ==="
rg -n "WelsonJS" --type js
# Search for any code that might handle directory migration
echo -e "\n=== Searching for potential migration logic ==="
rg -n -i "(migrate|migration|old.*path|rename.*dir)" --type jsLength of output: 301835
Fix path case inconsistency and provide migration guidance for AppData directory change.
The verification confirms a critical inconsistency: lib/chrome.js:114 contains a hardcoded reference to the OLD path %APPDATA%\WelsonJS\, while lib/system.js:41 changes the path to lowercase \welsonjs. This creates mixed-case behavior where:
- Direct inconsistency: Modules using
getAppDataDir()(http.js, ovftool.js, python3.js, wamr.js, websocket.js) will use the new path\welsonjs, but chrome.js bypasses this by hardcoding the old uppercase path - No migration logic: The script found no code handling the path transition for existing installations
- Breaking change: On Windows case-preserving filesystems, existing data in
%APPDATA%\WelsonJSwill not be found
Required fixes:
- lib/chrome.js:114: Update hardcoded path from
"\\WelsonJS\\"to useSYS.getAppDataDir() + "\\"for consistency - Add migration logic (e.g., in system initialization): Check if old path exists, and if new path doesn't, copy/move data from
%APPDATA%\WelsonJSto%APPDATA%\welsonjs - Document this as a breaking change in release notes if no migration is provided
🤖 Prompt for AI Agents
In lib/system.js around line 41 and lib/chrome.js at line 114, there is a case
mismatch between the new AppData path "\\welsonjs" and the hardcoded
"\\WelsonJS\\" in chrome.js which breaks installs on case-preserving Windows
filesystems; update chrome.js to stop hardcoding the path and call
SYS.getAppDataDir() + "\\" instead, and in system initialization add migration
logic that checks if the old path (%APPDATA%\WelsonJS) exists and the new path
(%APPDATA%\welsonjs) does not, then atomically move or copy the contents
(preserving permissions and handling errors) to the new directory with safe
retries and logging; if you choose not to implement automatic migration, add an
explicit release-note entry marking this as a breaking change and instruct users
how to manually move their data.
Replaced usage of SYS.getEnvString("APPDATA") with SYS.getAppDataDir() for determining the user data directory. Updated VERSIONINFO to 0.5.4 to reflect the change.
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)
lib/chrome.js(2 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
lib/chrome.js (1)
1501-1501: LGTM! Version bump is appropriate.The version increment from 0.5.3 to 0.5.4 is appropriate given the functional changes to the user data directory management.
| this.userDataDir = dirname; | ||
| } else { | ||
| this.userDataDir = SYS.getEnvString("APPDATA") + "\\WelsonJS\\" + publisherName.get() + "_user_profile"; | ||
| this.userDataDir = SYS.getAppDataDir() + "\\" + publisherName.get() + "_user_profile"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the SYS.getAppDataDir() implementation and assess backward compatibility.
The change from a hardcoded environment variable path to SYS.getAppDataDir() centralizes path management, which improves maintainability. However, please verify:
- That
SYS.getAppDataDir()is correctly implemented inlib/system.jsand returns the expected path - Whether this creates a breaking change for existing users with profiles in the old location
- If migration is needed, ensure users are informed or an automatic migration path exists
Run the following script to verify the implementation:
🏁 Script executed:
#!/bin/bash
# Verify SYS.getAppDataDir() implementation and usage
# Check if getAppDataDir is defined in system.js
echo "Checking for getAppDataDir implementation:"
rg -n -A 5 "getAppDataDir\s*[:=]?\s*function" --type=js
# Check all usages of getAppDataDir in the codebase
echo -e "\nChecking all usages of getAppDataDir:"
rg -n "getAppDataDir\s*\(\)" --type=js -C 2Length of output: 1955
🏁 Script executed:
#!/bin/bash
# Verify SYS object export and getAppDataDir membership
# Check system.js exports and SYS object definition
echo "Checking system.js for SYS object definition and exports:"
rg -n "module\.exports|exports\[|SYS\s*[=:]|SYS\." lib/system.js -A 2 -B 2
# Check how chrome.js imports/uses SYS
echo -e "\nChecking chrome.js imports and SYS usage:"
head -30 lib/chrome.js | rg -n "require|SYS|system"
# Check if there's any migration or backward compatibility code
echo -e "\nSearching for migration or backward compatibility logic:"
rg -n "migration|backward|compat|profile.*path|userData" --type=js -i | head -20Length of output: 1938
🏁 Script executed:
#!/bin/bash
# Verify SYS object export and getAppDataDir membership
# Check system.js exports and SYS object definition
echo "Checking system.js for SYS object definition and exports:"
rg -n "module\.exports|exports\[|SYS\s*[=:]|function.*getAppDataDir" lib/system.js -A 2 -B 1
# Check how chrome.js imports/uses SYS and first 50 lines
echo -e "\nChecking chrome.js imports and initial setup:"
head -50 lib/chrome.js | rg -n "require|SYS"
# Check if there's old userDataDir logic or environment variable fallback
echo -e "\nSearching for old userDataDir patterns or LOCALAPPDATA:"
rg -n "LOCALAPPDATA|USERPROFILE|userDataDir" lib/chrome.js -i -B 2 -A 2Length of output: 6613
🏁 Script executed:
#!/bin/bash
# Verify getAppDataDir export and check previous implementation
# Check system.js module.exports
echo "Checking system.js module.exports:"
rg -n "module\.exports\s*[={]" lib/system.js -A 20
# Check if getAppDataDir is explicitly exported
echo -e "\nSearching for getAppDataDir in exports:"
rg -n "getAppDataDir" lib/system.js
# Get git diff to see what line 114 was before
echo -e "\nSearching git history for chrome.js changes:"
git log --oneline -n 5 -- lib/chrome.js 2>/dev/null || echo "Git history not available"
# Check the actual git diff for the specific line
echo -e "\nChecking git diff for lib/chrome.js:"
git diff HEAD~1 lib/chrome.js 2>/dev/null | head -100 || echo "Git diff not available"Length of output: 1468
Fix path case-sensitivity to maintain backward compatibility.
The verification confirms SYS.getAppDataDir() is properly implemented and exported. However, the change introduces a breaking change: the git diff reveals the previous implementation used "\\WelsonJS\\" (capital letters), while getAppDataDir() returns "\\welsonjs" (lowercase). This creates two different paths:
- Old:
APPDATA\WelsonJS\<publisherName>_user_profile - New:
APPDATA\welsonjs\<publisherName>_user_profile
Existing user profiles will not be found by the new code. Either align the case in getAppDataDir() to return "\\WelsonJS\\", or add migration logic to move profiles from the old location to the new location.
🤖 Prompt for AI Agents
In lib/chrome.js around line 114, the assignment of userDataDir uses
SYS.getAppDataDir() which returns a lowercase path ("\\welsonjs") but older
installs used a capitalized "\\WelsonJS\\", causing user profiles to be missed;
fix by either (A) normalizing to the legacy casing: call a helper that returns
the legacy-cased base (or adjust getAppDataDir() to return "\\WelsonJS\\") so
userDataDir continues to point to the original folder, or (B) implement
migration: detect existence of the old-path
(APPDATA\WelsonJS\<publisher>_user_profile) and if present move/rename its
contents to the new-path returned by getAppDataDir(), ensuring permissions and
atomicity, and log the migration outcome; pick one approach and apply it
consistently across startup code.
Changed the registry command for opening scripts to reference WelsonJS.Launcher.exe in the 'bin' subdirectory instead of the root application directory. This ensures the correct executable path is used after installation.
|
Refactored helper functions for clarity and consistency, including renaming and parameter changes. Added support for downloading and installing websocat, updated artifact URLs and extraction logic, and improved architecture-specific handling for optional tools. Enhanced output messages and error handling throughout the script.
Summary by Sourcery
Refactor the PowerShell install script for consistency and clarity, enhance logging and error handling, update per-architecture artifact management, and add support for downloading and installing websocat.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Refactor
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.