-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent false unsaved changes notification when mode/API changes externally #7418
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
… externally - Added logic to detect and handle external changes (from main interface) - External changes to mode or API configuration no longer mark settings as dirty - Only changes made within the Settings dialog itself trigger unsaved changes warning - Fixes #7417
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 because apparently I trust no one, not even myself.
| extensionState.mode !== cachedState.mode || | ||
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) | ||
|
|
||
| if (isExternalChange && !isChangeDetected) { |
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 condition isExternalChange && !isChangeDetected could lead to race conditions if internal changes happen simultaneously with external changes. Consider using a more robust state management approach or a ref to track the source of changes.
| // by comparing the extension state with our cached state | ||
| const isExternalChange = | ||
| extensionState.mode !== cachedState.mode || | ||
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) |
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.
Using JSON.stringify for deep comparison on every render could be expensive for large API configurations. Could we use a more efficient deep comparison utility like lodash's isEqual or memoize this comparison?
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) | |
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) |
| // Check if the changes are coming from outside the settings dialog | ||
| // by comparing the extension state with our cached state | ||
| const isExternalChange = | ||
| extensionState.mode !== cachedState.mode || |
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 code only checks for mode and apiConfiguration changes, but other fields could also change externally (e.g., language, experiments). Should we expand this to cover all fields that could change externally, or is there a specific reason to limit it to these two?
| setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) | ||
| prevApiConfigName.current = currentApiConfigName | ||
| // Reset change detection when API config changes externally | ||
| setChangeDetected(false) |
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.
There are now three separate places handling similar logic for setChangeDetected(false) (lines 200, 208, and within the new effect). Could we consolidate this logic into a single helper function or custom hook to improve maintainability?
|
Closing this PR as it addresses the symptoms but not the root cause. The real issue appears to be that something in the configuration object (particularly with OpenRouter APIs) is changing unexpectedly or not being saved properly, which triggers the dirty state incorrectly. We need to investigate the actual root cause instead of masking the problem. |
Description
This PR fixes issue #7417 where users were incorrectly prompted about unsaved changes when:
Problem
The Settings dialog was incorrectly detecting external changes (mode/API config changes from the main interface) as internal changes, causing a false positive "unsaved changes" warning.
Solution
Added logic to detect and handle external changes separately from internal settings changes:
useEffecthook that monitors for external state changes and syncs them without triggering change detectionTesting
Related Issue
Fixes #7417
Important
Fixes false unsaved changes notification in
SettingsView.tsxby correctly handling external mode/API changes.SettingsView.tsxwhen mode/API changes externally.useEffecthook inSettingsView.tsxto monitor and sync external state changes without triggering change detection.This description was created by
for f9cf6e5. You can customize this summary. It will automatically update as commits are pushed.