Skip to content

Commit 9fbc9e1

Browse files
committed
refactor(shell): apply code review feedback to getTerminalConfig
- Replace TerminalProfiles union with PlatformProfilesMap mapped type so the return type is inferred from the platform key, eliminating explicit type parameters at each call site and making mismatches a compile error - Change || to ?? when falling back to empty profiles object - Add JSDoc explaining the key constraint and inferred return type - Add missing boolean/array non-string defaultProfileName test cases for macOS and Linux to match Windows coverage - Add mutation-resistant tests for macOS and Linux that verify the typeof guard is load-bearing (numeric key matching a real profile entry)
1 parent 81c2f07 commit 9fbc9e1

2 files changed

Lines changed: 119 additions & 9 deletions

File tree

src/utils/__tests__/shell.spec.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -521,6 +521,34 @@ describe("Shell Detection Tests", () => {
521521
expect(getShell()).toBe("/bin/zsh")
522522
})
523523

524+
it("macOS: handles boolean defaultProfileName without TypeError", () => {
525+
Object.defineProperty(process, "platform", { value: "darwin" })
526+
const mockConfig = {
527+
get: vi.fn((key: string) => {
528+
if (key === "defaultProfile.osx") return true
529+
if (key === "profiles.osx") return {}
530+
return undefined
531+
}),
532+
}
533+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
534+
535+
expect(getShell()).toBe("/bin/zsh")
536+
})
537+
538+
it("macOS: handles array defaultProfileName without TypeError", () => {
539+
Object.defineProperty(process, "platform", { value: "darwin" })
540+
const mockConfig = {
541+
get: vi.fn((key: string) => {
542+
if (key === "defaultProfile.osx") return ["zsh"]
543+
if (key === "profiles.osx") return {}
544+
return undefined
545+
}),
546+
}
547+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
548+
549+
expect(getShell()).toBe("/bin/zsh")
550+
})
551+
524552
it("macOS: handles object defaultProfileName without TypeError", () => {
525553
Object.defineProperty(process, "platform", { value: "darwin" })
526554
const mockConfig = {
@@ -535,6 +563,25 @@ describe("Shell Detection Tests", () => {
535563
expect(getShell()).toBe("/bin/zsh")
536564
})
537565

566+
// Mutation-resistant: without the typeof guard, profiles[1] === profiles["1"] in JS,
567+
// so a numeric key that matches a real profile would return its path instead of falling back.
568+
it("macOS: ignores numeric defaultProfileName even when it matches a profile key", () => {
569+
Object.defineProperty(process, "platform", { value: "darwin" })
570+
const mockConfig = {
571+
get: vi.fn((key: string) => {
572+
if (key === "defaultProfile.osx") return 1
573+
// Profile keyed as "1" — would be reached by profiles[1] if the guard were absent
574+
if (key === "profiles.osx") return { "1": { path: "/usr/local/bin/zsh" } }
575+
return undefined
576+
}),
577+
}
578+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
579+
580+
// Guard treats 1 as null → getMacShellFromVSCode returns null → fallback to /bin/zsh
581+
// Without the guard it would return /usr/local/bin/zsh
582+
expect(getShell()).toBe("/bin/zsh")
583+
})
584+
538585
it("Linux: handles numeric defaultProfileName without TypeError", () => {
539586
Object.defineProperty(process, "platform", { value: "linux" })
540587
const mockConfig = {
@@ -549,6 +596,34 @@ describe("Shell Detection Tests", () => {
549596
expect(getShell()).toBe("/bin/bash")
550597
})
551598

599+
it("Linux: handles boolean defaultProfileName without TypeError", () => {
600+
Object.defineProperty(process, "platform", { value: "linux" })
601+
const mockConfig = {
602+
get: vi.fn((key: string) => {
603+
if (key === "defaultProfile.linux") return true
604+
if (key === "profiles.linux") return {}
605+
return undefined
606+
}),
607+
}
608+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
609+
610+
expect(getShell()).toBe("/bin/bash")
611+
})
612+
613+
it("Linux: handles array defaultProfileName without TypeError", () => {
614+
Object.defineProperty(process, "platform", { value: "linux" })
615+
const mockConfig = {
616+
get: vi.fn((key: string) => {
617+
if (key === "defaultProfile.linux") return ["bash"]
618+
if (key === "profiles.linux") return {}
619+
return undefined
620+
}),
621+
}
622+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
623+
624+
expect(getShell()).toBe("/bin/bash")
625+
})
626+
552627
it("Linux: handles object defaultProfileName without TypeError", () => {
553628
Object.defineProperty(process, "platform", { value: "linux" })
554629
const mockConfig = {
@@ -562,6 +637,24 @@ describe("Shell Detection Tests", () => {
562637

563638
expect(getShell()).toBe("/bin/bash")
564639
})
640+
641+
// Mutation-resistant: same pattern as macOS — numeric key matches profile "1" only if unguarded.
642+
it("Linux: ignores numeric defaultProfileName even when it matches a profile key", () => {
643+
Object.defineProperty(process, "platform", { value: "linux" })
644+
const mockConfig = {
645+
get: vi.fn((key: string) => {
646+
if (key === "defaultProfile.linux") return 1
647+
// Profile keyed as "1" — would be reached by profiles[1] if the guard were absent
648+
if (key === "profiles.linux") return { "1": { path: "/usr/bin/fish" } }
649+
return undefined
650+
}),
651+
}
652+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
653+
654+
// Guard treats 1 as null → getLinuxShellFromVSCode returns null → fallback to /bin/bash
655+
// Without the guard it would return /usr/bin/fish
656+
expect(getShell()).toBe("/bin/bash")
657+
})
565658
})
566659

567660
// --------------------------------------------------------------------------

src/utils/shell.ts

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -136,19 +136,36 @@ type LinuxTerminalProfiles = Record<string, LinuxTerminalProfile>
136136
// 1) VS Code Terminal Configuration Helpers
137137
// -----------------------------------------------------
138138

139-
type TerminalProfiles = WindowsTerminalProfiles | MacTerminalProfiles | LinuxTerminalProfiles
139+
type PlatformProfilesMap = {
140+
windows: WindowsTerminalProfiles
141+
osx: MacTerminalProfiles
142+
linux: LinuxTerminalProfiles
143+
}
140144

141-
function getTerminalConfig<T extends TerminalProfiles>(
142-
platformKey: string,
143-
): { defaultProfileName: string | null; profiles: T } {
145+
/**
146+
* Reads the VS Code terminal profile configuration for the given platform.
147+
*
148+
* The key must be one of `"windows"`, `"osx"`, or `"linux"` — the exact strings
149+
* VS Code uses in `terminal.integrated.defaultProfile.<key>` and
150+
* `terminal.integrated.profiles.<key>`. Passing any other value is a compile-time
151+
* error, which prevents silent mismatches such as `"darwin"` returning empty config.
152+
*
153+
* The return type (`defaultProfileName` and `profiles`) is inferred from `K` via
154+
* `PlatformProfilesMap`, so callers don't need an explicit type parameter.
155+
*
156+
* Returns `{ defaultProfileName: null, profiles: {} }` on any VS Code API error.
157+
*/
158+
function getTerminalConfig<K extends keyof PlatformProfilesMap>(
159+
platformKey: K,
160+
): { defaultProfileName: string | null; profiles: PlatformProfilesMap[K] } {
144161
try {
145162
const config = vscode.workspace.getConfiguration("terminal.integrated")
146163
const rawProfileName = config.get<string>(`defaultProfile.${platformKey}`)
147164
const defaultProfileName = typeof rawProfileName === "string" ? rawProfileName : null
148-
const profiles = config.get<T>(`profiles.${platformKey}`) || ({} as T)
165+
const profiles = config.get<PlatformProfilesMap[K]>(`profiles.${platformKey}`) ?? ({} as PlatformProfilesMap[K])
149166
return { defaultProfileName, profiles }
150167
} catch {
151-
return { defaultProfileName: null, profiles: {} as T }
168+
return { defaultProfileName: null, profiles: {} as PlatformProfilesMap[K] }
152169
}
153170
}
154171

@@ -170,7 +187,7 @@ function normalizeShellPath(path: string | string[] | undefined): string | null
170187

171188
/** Attempts to retrieve a shell path from VS Code config on Windows. */
172189
function getWindowsShellFromVSCode(): string | null {
173-
const { defaultProfileName, profiles } = getTerminalConfig<WindowsTerminalProfiles>("windows")
190+
const { defaultProfileName, profiles } = getTerminalConfig("windows")
174191
if (!defaultProfileName) {
175192
// No explicit Windows terminal profile is configured. VS Code auto-detects
176193
// the default on modern Windows and prefers PowerShell 7 (pwsh.exe) when it
@@ -215,7 +232,7 @@ function getWindowsShellFromVSCode(): string | null {
215232

216233
/** Attempts to retrieve a shell path from VS Code config on macOS. */
217234
function getMacShellFromVSCode(): string | null {
218-
const { defaultProfileName, profiles } = getTerminalConfig<MacTerminalProfiles>("osx")
235+
const { defaultProfileName, profiles } = getTerminalConfig("osx")
219236
if (!defaultProfileName) {
220237
return null
221238
}
@@ -226,7 +243,7 @@ function getMacShellFromVSCode(): string | null {
226243

227244
/** Attempts to retrieve a shell path from VS Code config on Linux. */
228245
function getLinuxShellFromVSCode(): string | null {
229-
const { defaultProfileName, profiles } = getTerminalConfig<LinuxTerminalProfiles>("linux")
246+
const { defaultProfileName, profiles } = getTerminalConfig("linux")
230247
if (!defaultProfileName) {
231248
return null
232249
}

0 commit comments

Comments
 (0)