Skip to content

Commit d647011

Browse files
author
Bertan Ari
committed
address PR #538 review: scope=machine for rootResolution, use Package.name, test mock cleanup + coverage, drop scratch design doc
1 parent d21399c commit d647011

4 files changed

Lines changed: 31 additions & 149 deletions

File tree

‎docs/design/workspace-root-resolution.md‎

Lines changed: 0 additions & 140 deletions
This file was deleted.

‎src/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,7 @@
426426
"%settings.workspace.rootResolution.firstFolder.description%"
427427
],
428428
"default": "activeEditor",
429+
"scope": "machine",
429430
"description": "%settings.workspace.rootResolution.description%",
430431
"markdownDescription": "%settings.workspace.rootResolution.description%"
431432
}

‎src/utils/__tests__/path.spec.ts‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,19 +54,25 @@ function withWorkspaceMock(opts: {
5454
folders?: Array<{ uri: { fsPath: string }; name: string; index: number }> | undefined
5555
getWorkspaceFolder?: (...args: unknown[]) => { uri: { fsPath: string } } | null | undefined
5656
activeEditor?: { document: { uri: { fsPath: string } } } | null
57+
getConfiguration?: (section?: string) => { get: (key: string, defaultValue?: unknown) => unknown }
5758
}): () => void {
5859
const previousFolders = mockWorkspace.workspaceFolders
5960
const previousGetWorkspaceFolder = mockWorkspace.getWorkspaceFolder
6061
const previousActiveEditor = mockWindow.activeTextEditor
62+
// Capture getConfiguration too: tests that mutate it must not leak a broken
63+
// mock into sibling tests if the outer beforeEach is ever changed or moved.
64+
const previousGetConfiguration = mockWorkspace.getConfiguration
6165

6266
if ("folders" in opts) mockWorkspace.workspaceFolders = opts.folders
6367
if (opts.getWorkspaceFolder) mockWorkspace.getWorkspaceFolder = opts.getWorkspaceFolder
6468
if ("activeEditor" in opts) mockWindow.activeTextEditor = opts.activeEditor ?? null
69+
if (opts.getConfiguration) mockWorkspace.getConfiguration = opts.getConfiguration
6570

6671
return () => {
6772
mockWorkspace.workspaceFolders = previousFolders
6873
mockWorkspace.getWorkspaceFolder = previousGetWorkspaceFolder
6974
mockWindow.activeTextEditor = previousActiveEditor
75+
mockWorkspace.getConfiguration = previousGetConfiguration
7076
}
7177
}
7278

@@ -107,8 +113,6 @@ describe("Path Utilities", () => {
107113
expect(getWorkspacePath(workspacePath)).toBe("/Users/test/project")
108114
})
109115

110-
it("should return undefined when outside a workspace", () => {})
111-
112116
describe("rootResolution = 'activeEditor' (default)", () => {
113117
it("prefers the workspace folder containing the active editor", () => {
114118
setRootResolution("activeEditor")
@@ -203,13 +207,15 @@ describe("Path Utilities", () => {
203207
})
204208

205209
it("falls back to default behavior when reading the setting throws", () => {
206-
mockWorkspace.getConfiguration = () => {
207-
throw new Error("not available in this context")
208-
}
210+
// Route the throwing config mock through withWorkspaceMock so it is
211+
// restored by the cleanup callback and cannot leak into sibling tests.
209212
const restore = withWorkspaceMock({
210213
folders: [{ uri: { fsPath: "/test/workspace" }, name: "test", index: 0 }],
211214
activeEditor: { document: { uri: { fsPath: "/test/workspaceFolder/file.ts" } } },
212215
getWorkspaceFolder: () => ({ uri: { fsPath: "/test/workspaceFolder" } }),
216+
getConfiguration: () => {
217+
throw new Error("not available in this context")
218+
},
213219
})
214220
try {
215221
// Should not throw and should resolve via active-editor logic.
@@ -241,6 +247,22 @@ describe("Path Utilities", () => {
241247
}
242248
})
243249

250+
it("(activeEditor) falls back to getWorkspacePath when the context path has no workspace folder", () => {
251+
setRootResolution("activeEditor")
252+
// contextPath is provided but getWorkspaceFolder returns null, exercising
253+
// the console.debug fallback branch in getWorkspacePathForContext.
254+
const restore = withWorkspaceMock({
255+
folders: [{ uri: { fsPath: "/test/workspace" }, name: "test", index: 0 }],
256+
activeEditor: null,
257+
getWorkspaceFolder: () => null,
258+
})
259+
try {
260+
expect(getWorkspacePathForContext("/unknown/path/file.ts")).toBe("/test/workspace")
261+
} finally {
262+
restore()
263+
}
264+
})
265+
244266
it("(firstFolder) ignores the context path and returns workspaceFolders[0]", () => {
245267
setRootResolution("firstFolder")
246268
const spy = vi.fn(() => ({ uri: { fsPath: "/some/other/workspace" } }))

‎src/utils/path.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import * as path from "path"
22
import os from "os"
33
import * as vscode from "vscode"
44

5+
import { Package } from "../shared/package"
6+
57
/*
68
The Node.js 'path' module resolves and normalizes paths differently depending on the platform:
79
- On Windows, it uses backslashes (\) as the default path separator.
@@ -138,11 +140,8 @@ const DEFAULT_ROOT_RESOLUTION: WorkspaceRootResolution = "activeEditor"
138140
*/
139141
function getRootResolutionStrategy(): WorkspaceRootResolution {
140142
try {
141-
// Read directly from the `zoo-code` section to avoid importing
142-
// `Package` here (path.ts is a low-level utility and we want to keep
143-
// it free of circular dependencies on shared/* and package.json).
144143
const value = vscode.workspace
145-
.getConfiguration("zoo-code")
144+
.getConfiguration(Package.name)
146145
.get<string>("workspace.rootResolution", DEFAULT_ROOT_RESOLUTION)
147146
return value === "firstFolder" ? "firstFolder" : DEFAULT_ROOT_RESOLUTION
148147
} catch {

0 commit comments

Comments
 (0)