fix: prevent unnecessary MCP server refresh on settings save (#6772)

[hannesrudolph]

Summary

Fixes #6772 - MCP servers no longer refresh unnecessarily when saving unrelated settings.

Problem

MCP servers were incorrectly refreshing every time any settings were saved, not just when MCP-related settings changed. This caused unnecessary reconnections and confusing notifications for users.

Solution

Added a state comparison check in the mcpEnabled message handler to only trigger MCP server refresh when the value actually changes. The handler now:

1. Retrieves the current mcpEnabled state
2. Compares it with the new value
3. Only calls handleMcpEnabledChange() if the values differ
4. Always updates the state to ensure consistency

Changes

Modified Files

• src/core/webview/webviewMessageHandler.ts - Added state comparison logic to mcpEnabled handler
• src/core/webview/__tests__/webviewMessageHandler.spec.ts - Added comprehensive test suite for the fix

Key Changes

// Before: Always refreshed MCP servers
await mcpHubInstance.handleMcpEnabledChange(mcpEnabled)

// After: Only refresh when value changes
if (currentMcpEnabled !== mcpEnabled) {
    await mcpHubInstance.handleMcpEnabledChange(mcpEnabled)
}

Testing

Automated Tests

✅ Added 8 comprehensive test cases covering:

• No refresh when value doesn't change (true → true, false → false)
• Refresh when value changes (true → false, false → true)
• Handling of undefined values with defaults
• Graceful handling when McpHub is unavailable
• State consistency verification

Manual Testing Performed

✅ Scenario 1: Changed non-MCP settings → No refresh notification
✅ Scenario 2: Toggled MCP enabled → Refresh notification appears
✅ Scenario 3: Multiple saves without changes → No spurious notifications

Test Results

✓ webviewMessageHandler - mcpEnabled (8 tests)
  ✓ should not refresh MCP servers when value does not change (true to true)
  ✓ should not refresh MCP servers when value does not change (false to false)
  ✓ should refresh MCP servers when value changes from true to false
  ✓ should refresh MCP servers when value changes from false to true
  ✓ should handle undefined values with defaults correctly
  ✓ should handle when mcpEnabled changes from undefined to false
  ✓ should not call handleMcpEnabledChange when McpHub is not available
  ✓ should always update state even when value doesn't change

Test Suites: 1 passed, 1 total
Tests:       21 passed, 21 total
Time:        2.89s

Impact

• Bug Fixed: Eliminates unnecessary MCP server refreshes when saving unrelated settings
• Performance: Reduces unnecessary reconnection overhead
• User Experience: Removes confusing notifications that appeared when they shouldn't
• Backward Compatibility: ✅ No breaking changes

Checklist

• Code follows project style guidelines
• Tests have been added/updated
• All tests pass
• No linting errors
• Changes are focused on the issue (no scope creep)
• No debug logs or commented code
• Manual testing completed
• Backward compatibility maintained

Related

• Fixes Bug: Settings save incorrectly triggers MCP server refresh notifications #6772
• Related PR: fix: prevent unnecessary MCP server refresh on settings save #6773 (community submission with similar approach)

Notes for Reviewers

• The fix is minimal and focused - only 6 lines of production code changed
• The state comparison pattern is unique to this handler as other handlers don't trigger expensive operations like server refreshes
• Test coverage is comprehensive to ensure the fix works correctly in all scenarios

---

Important

Fixes unnecessary MCP server refreshes by adding a state comparison in mcpEnabled handler to only refresh when the value changes.

• Behavior:
  • mcpEnabled handler in webviewMessageHandler.ts now compares current and new mcpEnabled values before refreshing MCP servers.
  • Refresh only occurs if the mcpEnabled value changes, reducing unnecessary operations.
• Testing:
  • Added tests in webviewMessageHandler.spec.ts to cover scenarios where mcpEnabled does not change, changes from true to false, false to true, and handles undefined values.
  • Tests ensure no refresh occurs when mcpEnabled value remains the same and verify state consistency.
• Impact:
  • Fixes unnecessary MCP server refreshes, improving performance and user experience by eliminating confusing notifications.

^{This description was created by for 3b92231. You can customize this summary. It will automatically update as commits are pushed.}

[Copilot]

Pull Request Overview

This PR fixes an issue where MCP servers were unnecessarily refreshing whenever any settings were saved, even if MCP-related settings hadn't changed. The solution adds state comparison logic to only trigger server refreshes when the mcpEnabled value actually changes.

• Added state comparison check to prevent unnecessary MCP server refreshes
• Only triggers handleMcpEnabledChange() when the value actually differs from current state
• Maintains state consistency by always updating the stored value

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/core/webview/webviewMessageHandler.ts	Added comparison logic to check current vs new mcpEnabled state before triggering refresh
src/core/webview/tests/webviewMessageHandler.spec.ts	Added comprehensive test suite covering state comparison scenarios and edge cases

[Copilot]

[nitpick] The comment should use consistent indentation style with tabs instead of spaces mixed with tabs.

[roomote]

Great work on this fix! The implementation successfully addresses the issue where MCP servers were unnecessarily refreshing when saving unrelated settings. The approach is clean, minimal, and well-tested.

Strengths:
✅ Correctly identifies and fixes the root cause
✅ Maintains state consistency
✅ Comprehensive test coverage (8 test cases)
✅ Minimal, focused changes (only 6 lines of production code)
✅ Fully backward compatible

The PR is ready to merge. I've left a couple of minor suggestions for future improvements, but they're not blockers.

[daniel-lxs]

LGTM