-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix: Prevent unnecessary context condensing when toggling settings #7958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Fixed dirty state tracking in SettingsView to properly detect when settings are reverted to original values - Modified all setter functions to check entire state against original extension state - Only send settings updates to backend when values actually differ from saved state - Added comprehensive tests for dirty state management - This prevents unnecessary context condensing triggers during active tasks Fixes issue where Save button remained enabled after toggling settings back to original values, which could trigger unwanted context condensing even when no actual changes were made.
| const isSettingValid = !errorMessage | ||
|
|
||
| // Helper function to check if a value has changed | ||
| const hasChanged = (cachedValue: any, originalValue: any): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the inline helper function 'hasChanged' to a shared utility module so its deep‐equality logic can be reused elsewhere. This follows our best practices for utility functions (irule_tTqpIuNs8DV0QFGj).
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| import { setPendingTodoList } from "../tools/updateTodoListTool" | ||
|
|
||
| // Cache for condense-related settings to prevent unnecessary re-evaluation | ||
| const condenseSettingsCache = new Map<string, any>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condenseSettingsCache Map grows indefinitely without cleanup. Should we clear it when tasks are disposed or the provider is reset? This could become a memory leak in long-running sessions.
| await provider.contextProxy.setValue(key, value) | ||
|
|
||
| // Helper function to check if a condense-related setting has actually changed | ||
| const hasCondenseSettingChanged = (key: string, value: any): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The hasCondenseSettingChanged helper function lacks test coverage. Consider adding tests to ensure the caching logic works correctly, especially for edge cases like undefined values.
| ) | ||
|
|
||
| const setApiConfigurationField = useCallback( | ||
| <K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K], isUserAction: boolean = true) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isUserAction parameter defaults to true but isn't used consistently throughout the codebase. Could this cause issues distinguishing between automatic initialization and user changes?
| const isSettingValid = !errorMessage | ||
|
|
||
| // Helper function to check if a value has changed | ||
| const hasChanged = (cachedValue: any, originalValue: any): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this hasChanged helper to a utility module for reusability and easier testing. It could be useful in other components that need deep equality checks.
| const hasChanged = (cachedValue: any, originalValue: any): boolean => { | ||
| // Handle objects and arrays with deep comparison | ||
| if (typeof cachedValue === "object" && cachedValue !== null) { | ||
| return JSON.stringify(cachedValue) !== JSON.stringify(originalValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change detection uses JSON.stringify for deep comparison multiple times. Have you considered using a more efficient deep equality check library like fast-deep-equal? It could improve performance for complex settings objects.
Fixes #4430
Problem
Based on the GitHub comment analysis, the Settings Save action was posting all settings even when nothing changed, triggering unnecessary context condensing during active tasks. The issue had two parts:
Root Cause
The
setCachedStateFieldand related setter functions inSettingsView.tsxwere always settingsetChangeDetected(true)whenever any field changed, even if the user toggled back to the original value. This kept the Save button enabled and allowed unchanged settings to be sent to the backend, which could trigger context condensing.Solution
Modified all setter functions in
SettingsView.tsxto:setChangeDetected) when there are actual differences from the saved stateChanges Made
SettingsView.tsxto properly detect when settings are reverted to original valuessetCachedStateField,setApiConfigurationField,setExperimentEnabled,setTelemetrySetting,setOpenRouterImageApiKey,setImageGenerationSelectedModel,setCustomSupportPromptsField) to check entire state against original extension statehasCondenseSettingChangedhelper to prevent unnecessary condensingTesting
Impact
This fix prevents unnecessary context condensing triggers during active tasks, which was causing unexpected conversation summarization even when users were just toggling settings back and forth without making actual changes.
Fixes the issue where the Save button remained enabled after toggling settings back to original values.
Important
Optimizes settings management by enabling Save only on actual changes, preventing unnecessary context condensing.
SettingsView.tsxto only enable the Save button when settings differ from the original state.webviewMessageHandler.tsto prevent unnecessary context condensing by caching condense-related settings and checking for changes before updating.SettingsView.tsx(setCachedStateField,setApiConfigurationField, etc.) to compare new state with original state and set the dirty flag only if changes exist.hasCondenseSettingChangedinwebviewMessageHandler.tsto check if condense-related settings have changed before updating.SettingsView.spec.tsxto ensure the Save button is only enabled when there are actual changes and that no settings are sent to the backend if unchanged.This description was created by
for b18721b. You can customize this summary. It will automatically update as commits are pushed.