refactor(terminal): address review feedback on profile picker

Author: edelauna

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

@@ -397,7 +397,7 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
 					terminalZshOhMy,
 					terminalZshP10k,
 					terminalZdotdir,
-					terminalProfile,
+					terminalProfile: terminalProfile ?? "", // "" clears a saved profile; undefined is dropped by JSON.stringify
 					terminalOutputPreviewSize: terminalOutputPreviewSize ?? "medium",
 					mcpEnabled,
 					maxOpenTabsContext: Math.min(Math.max(0, maxOpenTabsContext ?? 20), 500),

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

@@ -1,4 +1,4 @@
-import { HTMLAttributes, useState, useCallback } from "react"
+import { HTMLAttributes, useState, useCallback, useEffect, useId } from "react"
 import { useAppTranslation } from "@/i18n/TranslationContext"
 import { vscode } from "@/utils/vscode"
 import { VSCodeCheckbox, VSCodeLink, VSCodeButton } from "@vscode/webview-ui-toolkit/react"
@@ -66,6 +66,11 @@ export const TerminalSettings = ({
 
 	const [inheritEnv, setInheritEnv] = useState<boolean>(true)
 	const [profileNames, setProfileNames] = useState<string[]>([])
+	const [isProfilesLoaded, setIsProfilesLoaded] = useState(false)
+	const profileModeId = useId()
+	const defaultProfileId = `${profileModeId}-default`
+	const overrideProfileId = `${profileModeId}-override`
+	const isProfileOverrideSelected = !!terminalProfile && (!isProfilesLoaded || profileNames.includes(terminalProfile))
 
 	useMount(() => {
 		vscode.postMessage({ type: "getVSCodeSetting", setting: "terminal.integrated.inheritEnv" })
@@ -85,6 +90,7 @@ export const TerminalSettings = ({
 				break
 			case "terminalProfiles":
 				setProfileNames(message.profiles ?? [])
+				setIsProfilesLoaded(true)
 				break
 			default:
 				break
@@ -93,6 +99,12 @@ export const TerminalSettings = ({
 
 	useEvent("message", onMessage)
 
+	useEffect(() => {
+		if (isProfilesLoaded && terminalProfile && !profileNames.includes(terminalProfile)) {
+			setCachedStateField("terminalProfile", undefined)
+		}
+	}, [isProfilesLoaded, profileNames, setCachedStateField, terminalProfile])
+
 	return (
 		<div className={cn("flex flex-col", className)} {...props}>
 			<SectionHeader>{t("settings:sections.terminal")}</SectionHeader>
@@ -167,13 +179,13 @@ export const TerminalSettings = ({
 								<div className="flex items-center gap-2 mb-2">
 									<input
 										type="radio"
-										id="terminal-profile-default"
-										name="terminal-profile-mode"
-										checked={!terminalProfile}
+										id={defaultProfileId}
+										name={profileModeId}
+										checked={!isProfileOverrideSelected}
 										onChange={() => setCachedStateField("terminalProfile", undefined)}
 										data-testid="terminal-profile-default-radio"
 									/>
-									<label htmlFor="terminal-profile-default" className="cursor-pointer">
+									<label htmlFor={defaultProfileId} className="cursor-pointer">
 										{t("settings:terminal.profile.default")}
 									</label>
 									<VSCodeButton
@@ -191,9 +203,9 @@ export const TerminalSettings = ({
 								<div className="flex items-center gap-2 mb-2">
 									<input
 										type="radio"
-										id="terminal-profile-override"
-										name="terminal-profile-mode"
-										checked={!!terminalProfile}
+										id={overrideProfileId}
+										name={profileModeId}
+										checked={isProfileOverrideSelected}
 										disabled={profileNames.length === 0}
 										onChange={() => {
 											if (!terminalProfile && profileNames.length > 0) {
@@ -203,7 +215,7 @@ export const TerminalSettings = ({
 										data-testid="terminal-profile-override-radio"
 									/>
 									<label
-										htmlFor="terminal-profile-override"
+										htmlFor={overrideProfileId}
 										className={
 											profileNames.length === 0
 												? "cursor-not-allowed text-vscode-disabledForeground"
@@ -220,7 +232,7 @@ export const TerminalSettings = ({
 									)}
 								</div>
 
-								{!!terminalProfile && profileNames.length > 0 && (
+								{isProfileOverrideSelected && profileNames.length > 0 && (
 									<Select
 										value={terminalProfile || DEFAULT_PROFILE_VALUE}
 										data-testid="terminal-profile-dropdown"

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

@@ -117,6 +117,36 @@ describe("TerminalSettings VS Code terminal profile (#277)", () => {
 		expect(screen.getByTestId("terminal-profile-dropdown")).toBeInTheDocument()
 	})
 
+	it("falls back to the default radio and clears an unavailable saved profile after profiles load", () => {
+		const { setCachedStateField } = setup("Git Bash")
+		act(() => {
+			window.dispatchEvent(
+				new MessageEvent("message", {
+					data: { type: "terminalProfiles", profiles: ["Command Prompt"] },
+				}),
+			)
+		})
+
+		expect(screen.getByTestId("terminal-profile-default-radio")).toBeChecked()
+		expect(screen.getByTestId("terminal-profile-override-radio")).not.toBeChecked()
+		expect(screen.queryByTestId("terminal-profile-dropdown")).not.toBeInTheDocument()
+		expect(setCachedStateField).toHaveBeenCalledWith("terminalProfile", undefined)
+	})
+
+	it("uses instance-local radio groups", () => {
+		render(
+			<>
+				<TerminalSettings terminalShellIntegrationDisabled={false} setCachedStateField={vi.fn()} />
+				<TerminalSettings terminalShellIntegrationDisabled={false} setCachedStateField={vi.fn()} />
+			</>,
+		)
+
+		const defaultRadios = screen.getAllByTestId("terminal-profile-default-radio")
+		expect(defaultRadios[0]).toBeChecked()
+		expect(defaultRadios[1]).toBeChecked()
+		expect(defaultRadios[0]).not.toHaveAttribute("name", defaultRadios[1].getAttribute("name"))
+	})
+
 	it("populates the dropdown from received profile names and selecting one sets the profile", () => {
 		const { setCachedStateField } = setup("Git Bash")
 
@@ -128,8 +158,8 @@ describe("TerminalSettings VS Code terminal profile (#277)", () => {
 			)
 		})
 
-		fireEvent.click(screen.getByTestId("option-Git Bash"))
-		expect(setCachedStateField).toHaveBeenCalledWith("terminalProfile", "Git Bash")
+		fireEvent.click(screen.getByTestId("option-zsh"))
+		expect(setCachedStateField).toHaveBeenCalledWith("terminalProfile", "zsh")
 	})
 
 	it("clicking default radio sets terminalProfile to undefined", () => {

webview-ui/src/context/ExtensionStateContext.tsx

@@ -85,7 +85,6 @@ export interface ExtensionStateContextType extends ExtensionState {
 	terminalZdotdir?: boolean
 	setTerminalZdotdir: (value: boolean) => void
 	terminalProfile?: string
-	setTerminalProfile: (value: string | undefined) => void
 	setTtsEnabled: (value: boolean) => void
 	setTtsSpeed: (value: number) => void
 	setEnableCheckpoints: (value: boolean) => void
@@ -536,7 +535,6 @@ export const ExtensionStateContextProvider: React.FC<{ children: React.ReactNode
 		setTerminalShellIntegrationDisabled: (value) =>
 			setState((prevState) => ({ ...prevState, terminalShellIntegrationDisabled: value })),
 		setTerminalZdotdir: (value) => setState((prevState) => ({ ...prevState, terminalZdotdir: value })),
-		setTerminalProfile: (value) => setState((prevState) => ({ ...prevState, terminalProfile: value })),
 		setMcpEnabled: (value) => setState((prevState) => ({ ...prevState, mcpEnabled: value })),
 		setTaskSyncEnabled: (value) => setState((prevState) => ({ ...prevState, taskSyncEnabled: value }) as any),
 		setCurrentApiConfigName: (value) => setState((prevState) => ({ ...prevState, currentApiConfigName: value })),