Skip to content

[BUG] Settings controls persist changes before Save (Save/Discard contract broken) #862

Description

@JunyongParkDev

Describe the bug

Several controls inside SettingsView write their changes to the extension host immediately, instead of buffering them in the local cachedState and waiting for the user to click Save.

Because Discard reverts edits by resetting cachedState back to the currently persisted state (setCachedState(extensionState)), any control that already sent updateSettings has poisoned that source of truth — so clicking Discard can no longer undo the change. The "Save / Discard" promise is silently broken.

This affects five Save-managed values:

  • allowedCommands (Auto Approve → Execute)
  • deniedCommands (Auto Approve → Execute)
  • profileThresholds (Context Management → Condensing Threshold, non-Default profile)
  • includeTaskHistoryInEnhance (Prompts → Enhance)
  • mcpEnabled (MCP → Enable MCP)

All five are part of the SettingsView Save payload (handleSubmit), so per the repository's Settings View Pattern they must bind to local cachedState, not live state.

To Reproduce

Steps to reproduce (using Allowed Commands as the example):

  1. Open Settings and go to Auto Approve.
  2. Enable Execute.
  3. In the allowed-commands input, type npm test and click Add.
  4. Do not click Save. Click Done and choose Discard changes.
  5. Reopen Settings → Auto Approve → Execute.
  6. See that npm test is still there — the discarded edit was persisted anyway.

The same pattern reproduces for each affected value:

  • Denied commands: add/remove a denied command → Discard → entry persists.
  • Profile thresholds: enable Automatically condense context, pick a non-Default profile, move the threshold slider → Discard → value persists. (Selecting Default moves a different value and does not reproduce this.)
  • Include task history: toggle the checkbox in Prompts → Enhance → Discard → toggle persists.
  • MCP enabled: toggle the top-level Enable MCP switch → Discard → toggle persists. (Note: per-server toggles/delete/restart are intentionally immediate and are not part of this bug.)

Expected behavior

Discarding unsaved changes leaves allowedCommands, deniedCommands, profileThresholds, includeTaskHistoryInEnhance, and mcpEnabled unchanged unless Save was clicked. These controls should follow the same Save/Discard contract as the rest of SettingsView:

  • No updateSettings is posted before Save for these Save-managed settings.
  • Save posts the new values once.
  • Discard fully reverts the edits.

Screenshots

N/A (state/behavior bug; see reproduction steps).

Video

N/A.

What version of zoo are you running

3.66.0

Additional context

Root cause (code locations):

  • webview-ui/src/components/settings/SettingsView.tsx — these values are included in the Save payload (handleSubmit) and Discard reverts via setCachedState(extensionState).
  • webview-ui/src/components/settings/AutoApproveSettings.tsx — command add/remove handlers posted updateSettings immediately.
  • webview-ui/src/components/settings/ContextManagementSettings.tsx — the non-default profile branch posted profileThresholds immediately.
  • webview-ui/src/components/settings/PromptsSettings.tsx — the checkbox posted includeTaskHistoryInEnhance immediately.
  • webview-ui/src/components/mcp/McpEnabledToggle.tsx — bound to live extension state and persisted immediately.

The extension host side (src/core/webview/webviewMessageHandler.ts) is correct:
persisting on receipt of updateSettings is expected. The fix belongs on the webview side — stop sending it before Save.

Correct precedent (not a bug): autoApprovalEnabled and the per-server MCP actions (toggleMcpServer, deleteMcpServer, restartMcpServer, updateMcpTimeout) use dedicated messages, are not in the Save payload, and are genuinely immediate — these should stay as they are.

Suggested fix: route all five Save-managed values exclusively through cachedState / setCachedStateField while SettingsView is open (remove the immediate updateSettings calls). For mcpEnabled, which is also read by McpView to gate the server list and was bound to live state, adopt the existing "props first, fall back to context" pattern (as used by PromptsSettings) so SettingsView can pass the buffered value and a cachedState-backed setter.

Suggested tests (place at the lowest layer that would have failed — the webview-ui component/unit layer):

  • AutoApproveSettings — adding/removing allowed & denied commands buffers via setCachedStateField and posts no updateSettings.
  • ContextManagementSettings — changing a non-default profile threshold buffers and posts no updateSettings.
  • PromptsSettings — toggling include-task-history calls the setter and posts no updateSettings.
  • McpEnabledToggle — controlled usage buffers via the setter prop and posts nothing; uncontrolled usage keeps the original immediate behavior.
  • SettingsView — update existing allowed-command tests to assert no pre-Save persistence, and add a Discard regression test.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    Fields

    No fields configured for Bug.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions