Fix Windows Development Build Issues#1657
Fix Windows Development Build Issues#1657ThisuraGallage wants to merge 2 commits intowso2:release/bi-1.8.xfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced dev-mode CSS/JS URL construction in the webview util to resolve via new URL(..., devHost). Also adjusted tar extraction quoting to use double quotes around archive and extract paths in the CLI download script. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-extension/src/utils/webview-utils.ts (1)
141-141: Consider usingjoin()consistently for the file path.The manual concatenation with
sepworks but is inconsistent with the approach on line 133.♻️ Optional simplification
- const filePath = join((extension.ballerinaExtInstance.context as ExtensionContext).extensionPath, 'resources', 'jslibs') + sep + componentName + '.js'; + const filePath = join((extension.ballerinaExtInstance.context as ExtensionContext).extensionPath, 'resources', 'jslibs', `${componentName}.js`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/webview-utils.ts` at line 141, The filePath construction manually concatenates sep instead of using path.join consistently; update the assignment of filePath (the const named filePath in webview-utils.ts) to build the full path using join(...) for all segments (extension.ballerinaExtInstance.context as ExtensionContext).extensionPath, 'resources', 'jslibs', and componentName + '.js' so it uses join for cross-platform correctness and consistency with the earlier usage on line 133.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@workspaces/ballerina/ballerina-extension/src/utils/webview-utils.ts`:
- Line 141: The filePath construction manually concatenates sep instead of using
path.join consistently; update the assignment of filePath (the const named
filePath in webview-utils.ts) to build the full path using join(...) for all
segments (extension.ballerinaExtInstance.context as
ExtensionContext).extensionPath, 'resources', 'jslibs', and componentName +
'.js' so it uses join for cross-platform correctness and consistency with the
earlier usage on line 133.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a186009-7847-422a-8051-bb5c37295794
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-extension/src/utils/webview-utils.tsworkspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js
There was a problem hiding this comment.
Pull request overview
Windows compatibility fixes for dev setup by preventing Windows path separators/backslash issues in dev URLs and improving tar extraction quoting.
Changes:
- Construct dev-mode resource URLs via template literals instead of
path.jointo avoid\in URLs on Windows. - Quote
tararguments with double quotes to work incmd.exe/PowerShell when paths include spaces.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js | Updates tar extraction command quoting for Windows shell compatibility. |
| workspaces/ballerina/ballerina-extension/src/utils/webview-utils.ts | Replaces join() URL construction with template-literal URLs in dev mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| try { | ||
| if (platform.ext === '.tar.gz') { | ||
| execSync(`tar -xzf '${archivePath}' -C '${tmpExtractDir}'`, { stdio: 'inherit' }); | ||
| execSync(`tar -xzf "${archivePath}" -C "${tmpExtractDir}"`, { stdio: 'inherit' }); |
There was a problem hiding this comment.
Using execSync with interpolated paths can break when archivePath or tmpExtractDir contains double quotes, and it also increases command-injection risk if those values are ever influenced externally. Prefer execFileSync('tar', ['-xzf', archivePath, '-C', tmpExtractDir], { stdio: 'inherit' }) (or spawnSync) to avoid shell parsing/quoting issues entirely.
| const filePath = join((extension.ballerinaExtInstance.context as ExtensionContext).extensionPath, 'resources', 'jslibs', 'themes', 'ballerina-default.min.css'); | ||
| return [ | ||
| (isDevMode && !disableComDebug) ? join(devHost, 'themes', 'ballerina-default.min.css') | ||
| (isDevMode && !disableComDebug) ? `${devHost}/themes/ballerina-default.min.css` |
There was a problem hiding this comment.
If devHost is configured with a trailing slash (e.g., http://localhost:port/), this will produce a double-slash URL (...//themes/...). Consider normalizing devHost (trim trailing /) once, or use new URL('/themes/ballerina-default.min.css', devHost).toString() to build a correct URL robustly.
| const filePath = join((extension.ballerinaExtInstance.context as ExtensionContext).extensionPath, 'resources', 'jslibs') + sep + componentName + '.js'; | ||
| return [ | ||
| (isDevMode && !disableComDebug) ? join(devHost, componentName + '.js') | ||
| (isDevMode && !disableComDebug) ? `${devHost}/${componentName}.js` |
There was a problem hiding this comment.
Same URL-joining issue as above: a trailing slash in devHost will yield ...//<component>.js, and special characters in componentName aren’t URL-encoded. Consider using new URL(\/${componentName}.js`, devHost).toString()(and ensurecomponentName` is a safe path segment) for consistent URL construction.
ThisuraGallage
changed the title
ThisuraGallage
changed the title
|
This change will break on other operating systems. We should use URL join utils instead of adding direct paths. |
Purpose
Windows compatibility fixes for dev setup.
prevent Windows backslash corruption
and PowerShell
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Bug Fixes
Chores