Skip to content

Commit 4f45c7c

Browse files
refactor(terminal): address review feedback on inline terminal profile (Zoo-Code-Org#119)
- Route profile names through a dedicated allowlisted `requestTerminalProfiles` message instead of the generic `getVSCodeSetting` (which reads any key the webview supplies); the extension reads the profiles and returns only names. - Preserve the profile's `env` (sanitized to string/null; null unsets a var), merged onto the base env in createTerminal. - Clarify the setting copy (en + es) vs the 'Use Inline Terminal' description. - Add tests: updateSettings->setTerminalProfile bridge, resolveWebviewView startup hydration, and profile env preservation/sanitization.
1 parent 8653b3c commit 4f45c7c

10 files changed

Lines changed: 156 additions & 45 deletions

File tree

packages/types/src/vscode-extension-host.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ export interface ExtensionMessage {
6363
| "commandExecutionStatus"
6464
| "mcpExecutionStatus"
6565
| "vsCodeSetting"
66+
| "terminalProfiles"
6667
| "authenticatedUser"
6768
| "condenseTaskContextStarted"
6869
| "condenseTaskContextResponse"
@@ -153,6 +154,8 @@ export interface ExtensionMessage {
153154
error?: string
154155
setting?: string
155156
value?: any // eslint-disable-line @typescript-eslint/no-explicit-any
157+
/** Sanitized VS Code terminal profile names for the `terminalProfiles` message. */
158+
profiles?: string[]
156159
hasContent?: boolean
157160
items?: MarketplaceItem[]
158161
userInfo?: CloudUserInfo
@@ -455,6 +458,7 @@ export interface WebviewMessage {
455458
| "updateVSCodeSetting"
456459
| "getVSCodeSetting"
457460
| "vsCodeSetting"
461+
| "requestTerminalProfiles"
458462
| "updateCondensingPrompt"
459463
| "playSound"
460464
| "playTts"

src/core/webview/__tests__/ClineProvider.spec.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import { Task, TaskOptions } from "../../task/Task"
2424
import { safeWriteJson } from "../../../utils/safeWriteJson"
2525

2626
import { ClineProvider } from "../ClineProvider"
27+
import { Terminal } from "../../../integrations/terminal/Terminal"
2728
import { MessageManager } from "../../message-manager"
2829

2930
// Mock setup must come before imports.
@@ -471,6 +472,20 @@ describe("ClineProvider", () => {
471472
expect(ClineProvider.getVisibleInstance()).toBe(provider)
472473
})
473474

475+
test("resolveWebviewView hydrates the saved terminalProfile into the process-wide Terminal state", async () => {
476+
const setTerminalProfileSpy = vi.spyOn(Terminal, "setTerminalProfile").mockImplementation(() => {})
477+
// Seed the persisted setting so the real getState() returns it during hydration.
478+
await (provider as any).contextProxy.setValue("terminalProfile", "Git Bash")
479+
480+
await provider.resolveWebviewView(mockWebviewView)
481+
// The hydration runs in a getState().then(...) callback, so flush microtasks.
482+
await new Promise((resolve) => setImmediate(resolve))
483+
484+
expect(setTerminalProfileSpy).toHaveBeenCalledWith("Git Bash")
485+
486+
setTerminalProfileSpy.mockRestore()
487+
})
488+
474489
test("resolveWebviewView sets up webview correctly", async () => {
475490
await provider.resolveWebviewView(mockWebviewView)
476491

src/core/webview/__tests__/webviewMessageHandler.spec.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ vi.mock("../../mentions/resolveImageMentions", () => ({
167167
}))
168168

169169
import { resolveImageMentions } from "../../mentions/resolveImageMentions"
170+
import { Terminal } from "../../../integrations/terminal/Terminal"
170171

171172
describe("webviewMessageHandler - requestLmStudioModels", () => {
172173
beforeEach(() => {
@@ -860,6 +861,38 @@ describe("webviewMessageHandler - mcpEnabled", () => {
860861
})
861862
})
862863

864+
describe("webviewMessageHandler - terminalProfile", () => {
865+
beforeEach(() => {
866+
vi.clearAllMocks()
867+
})
868+
869+
it("bridges a saved terminalProfile from updateSettings into the process-wide terminal state", async () => {
870+
const setTerminalProfileSpy = vi.spyOn(Terminal, "setTerminalProfile").mockImplementation(() => {})
871+
872+
await webviewMessageHandler(mockClineProvider, {
873+
type: "updateSettings",
874+
updatedSettings: { terminalProfile: "Git Bash" },
875+
})
876+
877+
expect(setTerminalProfileSpy).toHaveBeenCalledWith("Git Bash")
878+
879+
setTerminalProfileSpy.mockRestore()
880+
})
881+
882+
it("clears the terminal profile when updateSettings sends undefined", async () => {
883+
const setTerminalProfileSpy = vi.spyOn(Terminal, "setTerminalProfile").mockImplementation(() => {})
884+
885+
await webviewMessageHandler(mockClineProvider, {
886+
type: "updateSettings",
887+
updatedSettings: { terminalProfile: undefined },
888+
})
889+
890+
expect(setTerminalProfileSpy).toHaveBeenCalledWith(undefined)
891+
892+
setTerminalProfileSpy.mockRestore()
893+
})
894+
})
895+
863896
describe("webviewMessageHandler - requestCommands", () => {
864897
beforeEach(() => {
865898
vi.clearAllMocks()

src/core/webview/webviewMessageHandler.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1512,6 +1512,38 @@ export const webviewMessageHandler = async (
15121512

15131513
break
15141514

1515+
case "requestTerminalProfiles": {
1516+
// Allowlisted request: read VS Code's terminal profiles server-side and
1517+
// return only the sanitized profile names. The terminal profile dropdown
1518+
// only needs names, so this avoids routing it through the generic
1519+
// `getVSCodeSetting` handler (which reads any key the webview supplies).
1520+
try {
1521+
const names = new Set<string>()
1522+
1523+
for (const platform of ["windows", "osx", "linux"] as const) {
1524+
const profiles = vscode.workspace
1525+
.getConfiguration("terminal.integrated.profiles")
1526+
.get<Record<string, unknown>>(platform)
1527+
1528+
if (profiles && typeof profiles === "object") {
1529+
for (const name of Object.keys(profiles)) {
1530+
names.add(name)
1531+
}
1532+
}
1533+
}
1534+
1535+
await provider.postMessageToWebview({
1536+
type: "terminalProfiles",
1537+
profiles: Array.from(names).sort(),
1538+
})
1539+
} catch (error) {
1540+
console.error("Failed to get terminal profiles:", error)
1541+
await provider.postMessageToWebview({ type: "terminalProfiles", profiles: [] })
1542+
}
1543+
1544+
break
1545+
}
1546+
15151547
case "mode":
15161548
await provider.handleModeSwitch(message.text as Mode)
15171549
break

src/integrations/terminal/Terminal.ts

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export class Terminal extends BaseTerminal {
2424
const options: vscode.TerminalOptions = { cwd, name: "Roo Code", iconPath, env }
2525

2626
// When the user has chosen a specific terminal profile, resolve it to a
27-
// shell path/args so the inline terminal uses that shell (e.g. Git Bash
27+
// shell path/args/env so the inline terminal uses that shell (e.g. Git Bash
2828
// with a UTF-8 charset on Windows). When unset, we leave shellPath/shellArgs
2929
// undefined so VS Code's default terminal behavior is preserved (#119).
3030
const profileShell = Terminal.getProfileShell()
@@ -35,6 +35,12 @@ export class Terminal extends BaseTerminal {
3535
if (profileShell.shellArgs) {
3636
options.shellArgs = profileShell.shellArgs
3737
}
38+
39+
// Merge the profile's own env on top of the base env so profile-specific
40+
// variables (e.g. locale/PATH) are not lost. A `null` value unsets one.
41+
if (profileShell.env) {
42+
options.env = { ...env, ...profileShell.env }
43+
}
3844
}
3945

4046
this.terminal = vscode.window.createTerminal(options)
@@ -243,7 +249,7 @@ export class Terminal extends BaseTerminal {
243249
*/
244250
public static getProfileShell(
245251
platform: NodeJS.Platform = process.platform,
246-
): { shellPath: string; shellArgs?: string[] } | undefined {
252+
): { shellPath: string; shellArgs?: string[]; env?: Record<string, string | null> } | undefined {
247253
const profileName = Terminal.getTerminalProfile()
248254

249255
if (!profileName) {
@@ -257,7 +263,12 @@ export class Terminal extends BaseTerminal {
257263
.get<Record<string, unknown>>(platformKey)
258264

259265
const profile = profiles?.[profileName] as
260-
| { path?: string | string[]; args?: string | string[]; source?: string }
266+
| {
267+
path?: string | string[]
268+
args?: string | string[]
269+
source?: string
270+
env?: Record<string, unknown>
271+
}
261272
| null
262273
| undefined
263274

@@ -285,6 +296,26 @@ export class Terminal extends BaseTerminal {
285296
? [profile.args]
286297
: undefined
287298

288-
return { shellPath: pathValue, shellArgs }
299+
// VS Code profiles may declare their own `env` (e.g. to set a UTF-8 locale or
300+
// a custom PATH). Preserve it so the inline terminal doesn't lose environment
301+
// the user configured on the profile. A `null` value unsets that variable.
302+
// Values come from user `settings.json`, so sanitize to string/null only.
303+
let env: Record<string, string | null> | undefined
304+
305+
if (profile.env && typeof profile.env === "object") {
306+
const sanitized: Record<string, string | null> = {}
307+
308+
for (const [key, val] of Object.entries(profile.env)) {
309+
if (typeof val === "string" || val === null) {
310+
sanitized[key] = val
311+
}
312+
}
313+
314+
if (Object.keys(sanitized).length > 0) {
315+
env = sanitized
316+
}
317+
}
318+
319+
return { shellPath: pathValue, shellArgs, env }
289320
}
290321
}

src/integrations/terminal/__tests__/TerminalProfile.spec.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,26 @@ describe("Terminal inline terminal profile (#119)", () => {
9797
})
9898
})
9999

100+
it("preserves the profile's env and sanitizes non-string/null values", () => {
101+
stubProfiles({
102+
linux: {
103+
"Custom Bash": {
104+
path: "/bin/bash",
105+
env: { LANG: "en_US.UTF-8", UNSET_ME: null, BAD: 123 },
106+
},
107+
},
108+
})
109+
110+
Terminal.setTerminalProfile("Custom Bash")
111+
112+
expect(Terminal.getProfileShell("linux")).toEqual({
113+
shellPath: "/bin/bash",
114+
shellArgs: undefined,
115+
// `null` is preserved (unsets the var); the numeric `BAD` is dropped.
116+
env: { LANG: "en_US.UTF-8", UNSET_ME: null },
117+
})
118+
})
119+
100120
it("uses the first path candidate when path is an array", () => {
101121
stubProfiles({
102122
windows: {

webview-ui/src/components/settings/TerminalSettings.tsx

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,6 @@ type TerminalSettingsProps = HTMLAttributes<HTMLDivElement> & {
4545
// empty-string item value, so we map it to/from `undefined` in the handler.
4646
const DEFAULT_PROFILE_VALUE = "__default__"
4747

48-
// VS Code stores terminal profiles per platform; we request all of them so the
49-
// profile dropdown works regardless of which OS the extension host runs on.
50-
const PROFILE_SETTING_KEYS = [
51-
"terminal.integrated.profiles.windows",
52-
"terminal.integrated.profiles.osx",
53-
"terminal.integrated.profiles.linux",
54-
]
55-
5648
export const TerminalSettings = ({
5749
terminalOutputPreviewSize,
5850
terminalShellIntegrationTimeout,
@@ -75,29 +67,23 @@ export const TerminalSettings = ({
7567

7668
useMount(() => {
7769
vscode.postMessage({ type: "getVSCodeSetting", setting: "terminal.integrated.inheritEnv" })
78-
PROFILE_SETTING_KEYS.forEach((setting) => vscode.postMessage({ type: "getVSCodeSetting", setting }))
70+
// Request the terminal profile names through a dedicated, allowlisted message
71+
// (the extension reads the profiles and returns only sanitized names).
72+
vscode.postMessage({ type: "requestTerminalProfiles" })
7973
})
8074

8175
const onMessage = useCallback((event: MessageEvent) => {
8276
const message: ExtensionMessage = event.data
8377

8478
switch (message.type) {
8579
case "vsCodeSetting":
86-
switch (message.setting) {
87-
case "terminal.integrated.inheritEnv":
88-
setInheritEnv(message.value ?? true)
89-
break
90-
case "terminal.integrated.profiles.windows":
91-
case "terminal.integrated.profiles.osx":
92-
case "terminal.integrated.profiles.linux": {
93-
const names = message.value && typeof message.value === "object" ? Object.keys(message.value) : []
94-
setProfileNames((prev) => Array.from(new Set([...prev, ...names])).sort())
95-
break
96-
}
97-
default:
98-
break
80+
if (message.setting === "terminal.integrated.inheritEnv") {
81+
setInheritEnv(message.value ?? true)
9982
}
10083
break
84+
case "terminalProfiles":
85+
setProfileNames(message.profiles ?? [])
86+
break
10187
default:
10288
break
10389
}

webview-ui/src/components/settings/__tests__/TerminalSettings.profile.spec.tsx

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,7 @@ vi.mock("@/components/ui", () => ({
3333
SelectContent: ({ children }: any) => <div>{children}</div>,
3434
SelectItem: ({ children, value }: any) => <div data-item-value={value}>{children}</div>,
3535
Slider: ({ value, onValueChange }: any) => (
36-
<input
37-
type="range"
38-
value={value?.[0] ?? 0}
39-
onChange={(e) => onValueChange([parseFloat(e.target.value)])}
40-
/>
36+
<input type="range" value={value?.[0] ?? 0} onChange={(e) => onValueChange([parseFloat(e.target.value)])} />
4137
),
4238
}))
4339

@@ -88,13 +84,11 @@ describe("TerminalSettings inline terminal profile (#119)", () => {
8884
return { setCachedStateField }
8985
}
9086

91-
it("requests the VS Code terminal profile lists on mount", () => {
87+
it("requests the terminal profile names on mount via the allowlisted message", () => {
9288
setup()
9389

94-
const requested = postMessageMock.mock.calls.map((c) => c[0]?.setting)
95-
expect(requested).toContain("terminal.integrated.profiles.windows")
96-
expect(requested).toContain("terminal.integrated.profiles.osx")
97-
expect(requested).toContain("terminal.integrated.profiles.linux")
90+
const types = postMessageMock.mock.calls.map((c) => c[0]?.type)
91+
expect(types).toContain("requestTerminalProfiles")
9892
})
9993

10094
it("does not call setCachedStateField on init (only the Default option is shown)", () => {
@@ -104,18 +98,14 @@ describe("TerminalSettings inline terminal profile (#119)", () => {
10498
expect(setCachedStateField).not.toHaveBeenCalled()
10599
})
106100

107-
it("populates the dropdown from received profile lists and selecting one sets the profile name", () => {
101+
it("populates the dropdown from the received profile names and selecting one sets the profile name", () => {
108102
const { setCachedStateField } = setup()
109103

110-
// Simulate the extension responding with a Windows profile list.
104+
// Simulate the extension responding with the sanitized profile names.
111105
act(() => {
112106
window.dispatchEvent(
113107
new MessageEvent("message", {
114-
data: {
115-
type: "vsCodeSetting",
116-
setting: "terminal.integrated.profiles.windows",
117-
value: { "Git Bash": { path: "C:/Program Files/Git/bin/bash.exe" }, PowerShell: {} },
118-
},
108+
data: { type: "terminalProfiles", profiles: ["Git Bash", "PowerShell"] },
119109
}),
120110
)
121111
})

webview-ui/src/i18n/locales/en/settings.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -805,7 +805,7 @@
805805
"profile": {
806806
"label": "Inline terminal profile",
807807
"default": "Default (VS Code default shell)",
808-
"description": "Choose which VS Code terminal profile the inline terminal uses. Pick a UTF-8 shell such as Git Bash if the default shell garbles non-ASCII output (e.g. on Windows with a GBK code page). Leave on Default to keep VS Code's default behavior."
808+
"description": "Pick which shell the Inline Terminal launches, using a VS Code terminal profile's path and arguments. This only changes the shell binary — the Inline Terminal still bypasses shell integration, prompts, and plugins. Useful to choose a UTF-8 shell such as Git Bash when the default garbles non-ASCII output (e.g. on Windows with a GBK code page). Leave on Default to keep VS Code's default shell."
809809
}
810810
},
811811
"advancedSettings": {

webview-ui/src/i18n/locales/es/settings.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)