Skip to content

fix(vscode): select Git roots with platform-aware paths - #543

Merged
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
LKL-ZREO:codex/fix-vscode-windows-repo-root
Jul 28, 2026
Merged

fix(vscode): select Git roots with platform-aware paths#543
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
LKL-ZREO:codex/fix-vscode-windows-repo-root

Conversation

@LKL-ZREO

@LKL-ZREO LKL-ZREO commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

The VS Code extension selects the repository associated with the first workspace folder. pickRepoRoot previously compared paths using raw string equality and a /-only prefix check.

On Windows, VS Code repository paths normally use \. When a repository subdirectory is opened as the workspace, the ancestor check therefore fails and the extension falls back to the first repository returned by the Git extension, which may be an unrelated or nested repository.

This change uses Node's platform-aware POSIX/Windows path utilities to:

  • recognize exact repository matches using native path semantics;
  • select the deepest repository containing the workspace;
  • avoid treating similarly prefixed sibling paths as ancestors.

The change is limited to repository selection and its focused regression tests.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • corepack yarn test --runInBand src/extension/services/__tests__/gitMap.test.ts - 30 tests passed
  • corepack yarn lint
  • corepack yarn compile

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have added tests that prove the fix is effective
  • The focused unit tests for the changed behavior pass locally
  • Documentation changes are not required for this internal path-resolution fix
  • I have signed the CLA

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

const exact = roots.find((r) => r === workspacePath);
if (exact) return exact;
const candidates = roots.map((root) => {
const pathApi = /^[a-z]:[\\/]/i.test(root) || root.includes('\\') ? path.win32 : path.posix;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The path API detection heuristic only examines root but not workspacePath. If root and workspacePath use different path conventions (e.g., root uses forward slashes from Git output while workspacePath uses OS-native backslashes on Windows), the selected pathApi may compute relative() incorrectly.

Consider also checking workspacePath in the heuristic, or simply using the platform-native path module directly since VSCode extensions run on the host OS and both paths should follow the same OS convention:

const pathApi = process.platform === 'win32' ? path.win32 : path.posix;

Or even simpler, just use path.relative() / path.isAbsolute() without selecting a sub-module, since Node's path module already auto-detects the platform.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated the heuristic to inspect both root and workspacePath, and added a regression test covering a forward-slash UNC root with a native Windows workspace path. The targeted suite now passes 30/30.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 7406391 into alibaba:main Jul 28, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants