Skip to content

Commit 933ee98

Browse files
fix(settings): buffer Save-managed settings in cachedState until Save (#872)
* fix(settings): buffer Save-managed settings in cachedState until Save Several controls inside SettingsView wrote their changes to the extension host immediately via `updateSettings`, instead of buffering them in local `cachedState` and waiting for the user to click Save. Because Discard reverts edits by resetting `cachedState` to the persisted state, any control that already posted `updateSettings` poisoned that source of truth — so Discard could no longer undo the change, silently breaking the Save/Discard contract. Route all five Save-managed values exclusively through `cachedState` / `setCachedStateField` while SettingsView is open: - allowedCommands / deniedCommands (AutoApproveSettings): drop the immediate updateSettings on add/remove. - profileThresholds (ContextManagementSettings): drop the immediate updateSettings on the non-default profile branch; remove the now-unused vscode import. - includeTaskHistoryInEnhance (PromptsSettings): drop the immediate updateSettings on toggle. - mcpEnabled (McpEnabledToggle / McpView): adopt the existing "props first, fall back to context" pattern so SettingsView can pass the buffered value and a cachedState-backed setter; uncontrolled usage keeps the original immediate behavior. Genuinely-immediate actions (autoApprovalEnabled, per-server MCP toggle/delete/restart/timeout) are not in the Save payload and are left unchanged. The extension host side is correct and untouched. Add/update tests at the webview-ui component layer: buffering assertions for each control, a McpEnabledToggle controlled/uncontrolled regression, and a SettingsView Discard regression. Signed-off-by: JunyongParkDev <jun94.park@samsung.com> * test(settings): cover buffered MCP enablement * test(settings): verify discarded commands are reverted in UI Strengthen the SettingsView regression test to verify that buffered commands disappear from the UI when changes are discarded. Signed-off-by: JunyongParkDev <jun94.park@samsung.com> * fix(settings): restore immediate persistence for uncontrolled includeTaskHistoryInEnhance The previous Save-buffering change dropped the updateSettings post unconditionally, which fixed the controlled path but broke the uncontrolled one: with props omitted the toggle falls back to the context setter, which only updates local state and never persists. Gate the post on whether the component is controlled: - Controlled: setter buffers into cachedState, Save persists — no post here. - Uncontrolled: context setter is local-only, so post updateSettings now. Mirrors the props-first / context-fallback pattern used for McpEnabledToggle. Adds an uncontrolled regression test (updates local state *and* posts), symmetric with the controlled "buffer only" case. Signed-off-by: JunyongParkDev <jun94.park@samsung.com> --------- Signed-off-by: JunyongParkDev <jun94.park@samsung.com>
1 parent 116acfd commit 933ee98

13 files changed

Lines changed: 500 additions & 49 deletions

webview-ui/src/components/mcp/McpEnabledToggle.tsx

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,35 @@ import { useExtensionState } from "@src/context/ExtensionStateContext"
55
import { useAppTranslation } from "@src/i18n/TranslationContext"
66
import { vscode } from "@src/utils/vscode"
77

8-
const McpEnabledToggle = () => {
9-
const { mcpEnabled, setMcpEnabled } = useExtensionState()
8+
interface McpEnabledToggleProps {
9+
mcpEnabled?: boolean
10+
setMcpEnabled?: (value: boolean) => void
11+
}
12+
13+
const McpEnabledToggle = ({
14+
mcpEnabled: propsMcpEnabled,
15+
setMcpEnabled: propsSetMcpEnabled,
16+
}: McpEnabledToggleProps = {}) => {
17+
const { mcpEnabled: contextMcpEnabled, setMcpEnabled: contextSetMcpEnabled } = useExtensionState()
1018
const { t } = useAppTranslation()
1119

20+
// When rendered inside SettingsView the value is buffered in `cachedState` and
21+
// only persisted on Save. Fall back to live extension state when used uncontrolled.
22+
const mcpEnabled = propsMcpEnabled ?? contextMcpEnabled
23+
1224
const handleChange = (e: Event | FormEvent<HTMLElement>) => {
1325
const target = ("target" in e ? e.target : null) as HTMLInputElement | null
1426

1527
if (!target) {
1628
return
1729
}
1830

19-
setMcpEnabled(target.checked)
20-
vscode.postMessage({ type: "updateSettings", updatedSettings: { mcpEnabled: target.checked } })
31+
if (propsSetMcpEnabled) {
32+
propsSetMcpEnabled(target.checked)
33+
} else {
34+
contextSetMcpEnabled(target.checked)
35+
vscode.postMessage({ type: "updateSettings", updatedSettings: { mcpEnabled: target.checked } })
36+
}
2137
}
2238

2339
return (

webview-ui/src/components/mcp/McpView.tsx

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,17 @@ import McpResourceRow from "./McpResourceRow"
2828
import McpEnabledToggle from "./McpEnabledToggle"
2929
import { McpErrorRow } from "./McpErrorRow"
3030

31-
const McpView = () => {
32-
const { mcpServers: servers, alwaysAllowMcp, mcpEnabled } = useExtensionState()
31+
interface McpViewProps {
32+
mcpEnabled?: boolean
33+
setMcpEnabled?: (value: boolean) => void
34+
}
35+
36+
const McpView = ({ mcpEnabled: propsMcpEnabled, setMcpEnabled }: McpViewProps = {}) => {
37+
const { mcpServers: servers, alwaysAllowMcp, mcpEnabled: contextMcpEnabled } = useExtensionState()
38+
39+
// When rendered inside SettingsView the value is buffered in `cachedState` and
40+
// only persisted on Save. Fall back to live extension state when used uncontrolled.
41+
const mcpEnabled = propsMcpEnabled ?? contextMcpEnabled
3342

3443
const { t } = useAppTranslation()
3544
const { isOverThreshold, title, message } = useTooManyTools()
@@ -55,7 +64,7 @@ const McpView = () => {
5564
</Trans>
5665
</div>
5766

58-
<McpEnabledToggle />
67+
<McpEnabledToggle mcpEnabled={mcpEnabled} setMcpEnabled={setMcpEnabled} />
5968

6069
{mcpEnabled && (
6170
<>
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// npx vitest src/components/mcp/__tests__/McpEnabledToggle.spec.tsx
2+
3+
import { render, screen, fireEvent } from "@/utils/test-utils"
4+
5+
import McpEnabledToggle from "../McpEnabledToggle"
6+
import { vscode } from "@src/utils/vscode"
7+
8+
vi.mock("@src/utils/vscode", () => ({
9+
vscode: {
10+
postMessage: vi.fn(),
11+
},
12+
}))
13+
14+
vi.mock("@src/i18n/TranslationContext", () => ({
15+
useAppTranslation: () => ({ t: (key: string) => key }),
16+
}))
17+
18+
const contextSetMcpEnabled = vi.fn()
19+
vi.mock("@src/context/ExtensionStateContext", () => ({
20+
useExtensionState: () => ({
21+
mcpEnabled: true,
22+
setMcpEnabled: contextSetMcpEnabled,
23+
}),
24+
}))
25+
26+
const getCheckbox = () => screen.getByRole("checkbox")
27+
28+
describe("McpEnabledToggle - Save/Discard contract", () => {
29+
beforeEach(() => {
30+
vi.clearAllMocks()
31+
})
32+
33+
// Case 6: controlled (inside SettingsView) must buffer, not persist before Save.
34+
it("buffers via the setter prop without persisting before Save when controlled", () => {
35+
const setMcpEnabled = vi.fn()
36+
render(<McpEnabledToggle mcpEnabled={true} setMcpEnabled={setMcpEnabled} />)
37+
38+
fireEvent.click(getCheckbox())
39+
40+
expect(setMcpEnabled).toHaveBeenCalledWith(false)
41+
expect(vscode.postMessage).not.toHaveBeenCalled()
42+
// Must not touch live extension state either.
43+
expect(contextSetMcpEnabled).not.toHaveBeenCalled()
44+
})
45+
46+
// Regression guard: uncontrolled usage keeps the original immediate behavior.
47+
it("persists immediately via live state when used uncontrolled", () => {
48+
render(<McpEnabledToggle />)
49+
50+
fireEvent.click(getCheckbox())
51+
52+
expect(contextSetMcpEnabled).toHaveBeenCalledWith(false)
53+
expect(vscode.postMessage).toHaveBeenCalledWith({
54+
type: "updateSettings",
55+
updatedSettings: { mcpEnabled: false },
56+
})
57+
})
58+
})
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
import { fireEvent, render, screen } from "@/utils/test-utils"
2+
3+
import McpView from "../McpView"
4+
5+
const setMcpEnabled = vi.fn()
6+
7+
vi.mock("@src/context/ExtensionStateContext", () => ({
8+
useExtensionState: () => ({
9+
mcpServers: [],
10+
alwaysAllowMcp: false,
11+
mcpEnabled: false,
12+
}),
13+
}))
14+
15+
vi.mock("@src/i18n/TranslationContext", () => ({
16+
useAppTranslation: () => ({ t: (key: string) => key }),
17+
}))
18+
19+
vi.mock("@src/hooks/useTooManyTools", () => ({
20+
useTooManyTools: () => ({ isOverThreshold: false, title: "", message: "" }),
21+
}))
22+
23+
vi.mock("@src/components/settings/Section", () => ({
24+
Section: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
25+
}))
26+
27+
vi.mock("@src/components/settings/SectionHeader", () => ({
28+
SectionHeader: ({ children }: { children: React.ReactNode }) => <h2>{children}</h2>,
29+
}))
30+
31+
vi.mock("@src/components/ui", () => ({
32+
Button: ({ children, onClick }: { children: React.ReactNode; onClick: () => void }) => (
33+
<button onClick={onClick}>{children}</button>
34+
),
35+
Dialog: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
36+
DialogContent: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
37+
DialogHeader: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
38+
DialogTitle: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
39+
DialogDescription: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
40+
DialogFooter: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
41+
ToggleSwitch: () => null,
42+
StandardTooltip: ({ children }: { children: React.ReactNode }) => <>{children}</>,
43+
}))
44+
45+
vi.mock("../McpEnabledToggle", () => ({
46+
default: ({
47+
mcpEnabled,
48+
setMcpEnabled: setEnabled,
49+
}: {
50+
mcpEnabled: boolean
51+
setMcpEnabled?: (value: boolean) => void
52+
}) => (
53+
<button data-testid="mcp-enabled-toggle" onClick={() => setEnabled?.(!mcpEnabled)}>
54+
{String(mcpEnabled)}
55+
</button>
56+
),
57+
}))
58+
59+
describe("McpView", () => {
60+
beforeEach(() => {
61+
vi.clearAllMocks()
62+
})
63+
64+
it("uses extension state when no buffered value is provided", () => {
65+
render(<McpView />)
66+
67+
expect(screen.getByTestId("mcp-enabled-toggle")).toHaveTextContent("false")
68+
})
69+
70+
it("uses and updates the buffered value when controlled", () => {
71+
render(<McpView mcpEnabled={true} setMcpEnabled={setMcpEnabled} />)
72+
73+
fireEvent.click(screen.getByTestId("mcp-enabled-toggle"))
74+
75+
expect(setMcpEnabled).toHaveBeenCalledWith(false)
76+
})
77+
})

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

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ export const AutoApproveSettings = ({
8888
const newCommands = [...currentCommands, commandInput]
8989
setCachedStateField("allowedCommands", newCommands)
9090
setCommandInput("")
91-
vscode.postMessage({ type: "updateSettings", updatedSettings: { allowedCommands: newCommands } })
9291
}
9392
}
9493

@@ -99,7 +98,6 @@ export const AutoApproveSettings = ({
9998
const newCommands = [...currentCommands, deniedCommandInput]
10099
setCachedStateField("deniedCommands", newCommands)
101100
setDeniedCommandInput("")
102-
vscode.postMessage({ type: "updateSettings", updatedSettings: { deniedCommands: newCommands } })
103101
}
104102
}
105103

@@ -317,11 +315,6 @@ export const AutoApproveSettings = ({
317315
onClick={() => {
318316
const newCommands = (allowedCommands ?? []).filter((_, i) => i !== index)
319317
setCachedStateField("allowedCommands", newCommands)
320-
321-
vscode.postMessage({
322-
type: "updateSettings",
323-
updatedSettings: { allowedCommands: newCommands },
324-
})
325318
}}>
326319
<div className="flex flex-row items-center gap-1">
327320
<div>{cmd}</div>
@@ -376,11 +369,6 @@ export const AutoApproveSettings = ({
376369
onClick={() => {
377370
const newCommands = (deniedCommands ?? []).filter((_, i) => i !== index)
378371
setCachedStateField("deniedCommands", newCommands)
379-
380-
vscode.postMessage({
381-
type: "updateSettings",
382-
updatedSettings: { deniedCommands: newCommands },
383-
})
384372
}}>
385373
<div className="flex flex-row items-center gap-1">
386374
<div>{cmd}</div>

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import { SetCachedStateField } from "./types"
2424
import { SectionHeader } from "./SectionHeader"
2525
import { Section } from "./Section"
2626
import { SearchableSetting } from "./SearchableSetting"
27-
import { vscode } from "@/utils/vscode"
2827

2928
type ContextManagementSettingsProps = HTMLAttributes<HTMLDivElement> & {
3029
autoCondenseContext: boolean
@@ -139,7 +138,6 @@ export const ContextManagementSettings = ({
139138
}
140139

141140
setCachedStateField("profileThresholds", newThresholds)
142-
vscode.postMessage({ type: "updateSettings", updatedSettings: { profileThresholds: newThresholds } })
143141
}
144142
}
145143
return (

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -209,10 +209,16 @@ const PromptsSettings = ({
209209

210210
setIncludeTaskHistoryInEnhance(target.checked)
211211

212-
vscode.postMessage({
213-
type: "updateSettings",
214-
updatedSettings: { includeTaskHistoryInEnhance: target.checked },
215-
})
212+
// Controlled: setter buffers into cachedState (Save persists).
213+
// Uncontrolled: context setter is local-only, so persist here.
214+
if (!propsSetIncludeTaskHistoryInEnhance) {
215+
vscode.postMessage({
216+
type: "updateSettings",
217+
updatedSettings: {
218+
includeTaskHistoryInEnhance: target.checked,
219+
},
220+
})
221+
}
216222
}}>
217223
<span className="font-medium">
218224
{t("prompts:supportPrompts.enhance.includeTaskHistory")}

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,12 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
901901
{renderTab === "modes" && <ModesView />}
902902

903903
{/* MCP Section */}
904-
{renderTab === "mcp" && <McpView />}
904+
{renderTab === "mcp" && (
905+
<McpView
906+
mcpEnabled={mcpEnabled}
907+
setMcpEnabled={(value) => setCachedStateField("mcpEnabled", value)}
908+
/>
909+
)}
905910

906911
{/* Worktrees Section */}
907912
{renderTab === "worktrees" && <WorktreesView />}
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// npx vitest src/components/settings/__tests__/AutoApproveSettings.spec.tsx
2+
3+
import { render, screen, fireEvent } from "@/utils/test-utils"
4+
5+
import { AutoApproveSettings } from "../AutoApproveSettings"
6+
import { vscode } from "@/utils/vscode"
7+
8+
vi.mock("@/utils/vscode", () => ({
9+
vscode: {
10+
postMessage: vi.fn(),
11+
},
12+
}))
13+
14+
vi.mock("@/i18n/TranslationContext", () => ({
15+
useAppTranslation: () => ({ t: (key: string) => key }),
16+
}))
17+
18+
// AutoApproveSettings reads a couple of live-state values that are genuinely
19+
// immediate actions (autoApprovalEnabled). Those are out of scope for the
20+
// Save/Discard buffering contract, so we just provide inert stand-ins.
21+
vi.mock("@/context/ExtensionStateContext", () => ({
22+
useExtensionState: () => ({
23+
autoApprovalEnabled: false,
24+
setAutoApprovalEnabled: vi.fn(),
25+
}),
26+
}))
27+
28+
vi.mock("@/hooks/useAutoApprovalToggles", () => ({
29+
useAutoApprovalToggles: () => ({}),
30+
}))
31+
32+
vi.mock("@/hooks/useAutoApprovalState", () => ({
33+
useAutoApprovalState: () => ({ effectiveAutoApprovalEnabled: false, hasEnabledOptions: false }),
34+
}))
35+
36+
const renderSettings = (overrides = {}) => {
37+
const setCachedStateField = vi.fn()
38+
const props = {
39+
alwaysAllowExecute: true, // reveal the command list section
40+
allowedCommands: [] as string[],
41+
deniedCommands: [] as string[],
42+
setCachedStateField,
43+
...overrides,
44+
}
45+
render(<AutoApproveSettings {...(props as any)} />)
46+
return { setCachedStateField }
47+
}
48+
49+
// A change is "Save-managed" if it must NOT reach the extension host before Save.
50+
const expectNoImmediateUpdateSettings = () => {
51+
expect(vscode.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "updateSettings" }))
52+
}
53+
54+
describe("AutoApproveSettings - Save/Discard contract", () => {
55+
beforeEach(() => {
56+
vi.clearAllMocks()
57+
})
58+
59+
// Case 1: allowedCommands add
60+
it("buffers an added allowed command without persisting before Save", () => {
61+
const { setCachedStateField } = renderSettings()
62+
63+
fireEvent.change(screen.getByTestId("command-input"), { target: { value: "npm test" } })
64+
fireEvent.click(screen.getByTestId("add-command-button"))
65+
66+
expect(setCachedStateField).toHaveBeenCalledWith("allowedCommands", ["npm test"])
67+
expectNoImmediateUpdateSettings()
68+
})
69+
70+
// Case 2: allowedCommands remove
71+
it("buffers a removed allowed command without persisting before Save", () => {
72+
const { setCachedStateField } = renderSettings({ allowedCommands: ["npm test"] })
73+
74+
fireEvent.click(screen.getByTestId("remove-command-0"))
75+
76+
expect(setCachedStateField).toHaveBeenCalledWith("allowedCommands", [])
77+
expectNoImmediateUpdateSettings()
78+
})
79+
80+
// Case 3a: deniedCommands add
81+
it("buffers an added denied command without persisting before Save", () => {
82+
const { setCachedStateField } = renderSettings()
83+
84+
fireEvent.change(screen.getByTestId("denied-command-input"), { target: { value: "rm -rf" } })
85+
fireEvent.click(screen.getByTestId("add-denied-command-button"))
86+
87+
expect(setCachedStateField).toHaveBeenCalledWith("deniedCommands", ["rm -rf"])
88+
expectNoImmediateUpdateSettings()
89+
})
90+
91+
// Case 3b: deniedCommands remove
92+
it("buffers a removed denied command without persisting before Save", () => {
93+
const { setCachedStateField } = renderSettings({ deniedCommands: ["rm -rf"] })
94+
95+
fireEvent.click(screen.getByTestId("remove-denied-command-0"))
96+
97+
expect(setCachedStateField).toHaveBeenCalledWith("deniedCommands", [])
98+
expectNoImmediateUpdateSettings()
99+
})
100+
})

0 commit comments

Comments
 (0)