Skip to content

Conversation

@lambrianmsft
Copy link
Contributor

Updating node dependency to 20 and from net6 to net8, fixed issue where azurite files is sometimes incorrectly put into the workspace

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

Impact of Change

  • Users:
    Users should now see node being downloaded properly.
  • Developers:
  • System:

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:

Contributors

Screenshots/Videos

…re azurite files is sometimes incorrectly put into the workspace
Copilot AI review requested due to automatic review settings January 23, 2026 21:41
@github-actions
Copy link

github-actions bot commented Jan 23, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: fix(vscode): Updating Dependencies and fixing Azurite
  • Issue: Title is generally fine and follows conventional commit style. It's accurate but could be more specific about which dependencies were bumped (Node/.NET) and the key behavioral change (Azurite workspace fix).
  • Recommendation: Consider a slightly more specific title, for example: fix(vscode): bump Node to 20 & .NET to 8; fix Azurite workspace path

Commit Type

  • Properly selected (fix)
  • Only one option selected which is correct.

Risk Level

  • Assessment: The PR body selects Low, but there is no corresponding risk label on the PR (the PR has only needs-pr-update).
  • Issue: Every PR must have a risk label ("risk:low", "risk:medium", or "risk:high") that matches the selection in the PR body. Additionally, based on the code diff (bumping Node from 18 -> 20, .NET from 6 -> 8, runtime config changes, and e2e test setup changes), the impact is non-trivial and I advise risk:medium.
  • Recommendation: Either (a) justify why this is Low risk in the PR body with explicit reasoning, or (b) change the Risk Level selection to Medium in the body and add the risk:medium label to the PR. Given the runtime bumps, I recommend changing to Medium unless you can explain why the upgrade is purely safe and isolated.

What & Why

  • Current: (Missing)
  • Issue: This field is required and currently empty. Reviewers and release managers need a brief context describing what changed and why (e.g., bumping Node/.NET versions to address X, fixing Azurite being stored in workspace unexpectedly, etc.).
  • Recommendation: Add a concise explanation. Example:
    • "Bumped Node from 18 -> 20 and .NET from 6 -> 8 to ensure runtime compatibility with the latest tools. Fixed Azurite activation to prefer a configured location (or fall back to a global default) to prevent Azurite files from being stored in the user's workspace. Updated tests and e2e test setup to reflect Node 20."

⚠️ Impact of Change

  • Issue: The Impact of Change section is sparse. Only Users contains a short note. Developers and System fields are blank. Given runtime upgrades and config changes, impact must be documented.
  • Recommendation:
    • Users: Local development may download Node 20 and .NET 8 when using the extension; workflows may now default to Node 20. If any user-facing behavior changes are likely, list them (e.g., local Functions runtime behavior).
    • Developers: Note changes required for CI/build (update test runner/node versions, Dev Container, GitHub Actions, RBAC), any API changes, and where to update documentation.
    • System: Mention dependency changes, potential effects on e2e test infra, and the need to validate tooling that depends on specific Node/.NET versions.

Test Plan

  • Assessment: Unit tests were updated (see changes in apps/vs-code-designer/src/app/utils/test/binaries.test.ts). Good.
  • Notes & Recommendation:
    • The diff shows added/updated unit tests — this matches the PR body claiming Unit tests added/updated.
    • E2E: The PR modifies e2e/testSetup/BlankLogicApp/logicAppStandard.bicep to change WEBSITE_NODE_DEFAULT_VERSION to '~20' (this is a change to e2e test setup). If this PR relies on or affects E2E tests, please state whether E2E tests were run and their result. If E2E tests were not updated or executed, explain why.
    • Manual testing: The PR body claims manual testing was completed — please add a short list of manual test steps and the environment used (OS, VS Code version, test project) so reviewers can reproduce.

⚠️ Contributors

  • Assessment: Empty.
  • Recommendation: If others contributed (PM, designers, reviewers), list them. If no one else contributed, you may leave blank but a short note like "No additional contributors" helps.

Screenshots/Videos

  • Assessment: Not applicable for this non-visual change.

Summary Table

Section Status Recommendation
Title Consider specifying versions bumped and Azurite fix
Commit Type No change needed
Risk Level Add a risk:* label and update PR body to Medium or justify Low
What & Why Add a brief description of what changed and why (see example above)
Impact of Change ⚠️ Fill out Users/Developers/System with specific impacts and migration notes
Test Plan Add reproduction steps for manual testing and E2E test results or rationale
Contributors ⚠️ Add contributors or a short note if none
Screenshots/Videos N/A

Final message
Please update the PR title/body with the items above and re-submit:

  • Add a short "What & Why" describing the dependency bumps (Node 18 -> 20, .NET 6 -> 8) and the Azurite workspace fix.
  • Add explicit impact bullets for Users/Developers/System.
  • Add/confirm the risk label. I recommend risk:medium given the runtime upgrades — either change the PR risk selection to Medium and add the risk:medium label, or provide a clear justification why this is Low risk.
  • Expand the Test Plan with manual test steps and environment details and confirm whether E2E tests were run (the diff changes e2e setup to Node 20).
  • Add a short contributors list (or note none).

Once those updates are made, re-request review. Thanks — the code diffs and tests look good, but the PR metadata needs completion and the risk should be adjusted/justified.

(Note: advised risk is higher than the one selected in the PR body — advised: risk:medium vs selected: Low.)


Last updated: Sat, 24 Jan 2026 01:31:46 GMT

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the VS Code extension's Node.js and .NET dependencies from legacy versions to current supported versions, and fixes an Azurite configuration issue where files could be incorrectly placed in the workspace folder.

Changes:

  • Updates Node.js dependency from v18 to v20 across runtime configuration, fallback versions, and E2E test infrastructure
  • Updates .NET dependency from v6 to v8 for SDK fallback versions
  • Fixes Azurite location setting to use a fallback default path when the extension setting is undefined, preventing incorrect workspace folder usage
  • Adds telemetry logging for the new Node.js fallback path when no matching version is found

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
e2e/testSetup/BlankLogicApp/logicAppStandard.bicep Updates WEBSITE_NODE_DEFAULT_VERSION from ~18 to ~20 for Azure App Service configuration
apps/vs-code-designer/src/constants.ts Updates workflowappRuntime to node|20, renames dotnet6 to dotnet8 with version 8.0.318, and updates Node.js fallback to 20.18.3
apps/vs-code-designer/src/app/utils/binaries.ts Updates all .NET 6 references to .NET 8, adds fallback logic with telemetry when no matching Node.js version is found
apps/vs-code-designer/src/app/utils/azurite/activateAzurite.ts Fixes Azurite location setting to fall back to default path instead of undefined/empty value
apps/vs-code-designer/src/app/utils/test/binaries.test.ts Updates test expectations from dotnet6 to dotnet8

Comment on lines +241 to +243
context.telemetry.properties.latestNodeJSVersion = 'fallback-no-match';
context.telemetry.properties.errorLatestNodeJsVersion = 'No matching Node JS version found.';
return DependencyVersion.nodeJs;
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new fallback path when no matching Node.js version is found (lines 241-243) is not covered by unit tests. The existing test at line 237 only covers the API failure case (catch block), but doesn't test the scenario where the API succeeds but returns releases that don't match the requested major version. Consider adding a test case that mocks a successful API response with releases that don't match the major version to verify this new fallback behavior works correctly.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant