fix: prevent false dirty state when opening Settings after Mode/API change #8232
Conversation
…hange - Added isInitialMount ref to track first mount of SettingsView - Sync extension state on mount without marking as dirty - Simplified setApiConfigurationField to only mark as dirty for user actions - Removed complex initial sync logic that was causing false positives Fixes #8230
dosubot
bot
added
size:S
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| setCachedState(extensionState) | ||
| setChangeDetected(false) | ||
| isInitialMount.current = false | ||
| } |
There was a problem hiding this comment.
Potential race condition: The isInitialMount ref is set to false at the end of the useEffect. If the component re-renders quickly or extensionState changes during the initial mount, there could be edge cases where the dirty state is incorrectly set.
Consider setting isInitialMount.current = false before the state update:
if (isInitialMount.current) {
isInitialMount.current = false
setCachedState(extensionState)
setChangeDetected(false)
}| const apiConfiguration = useMemo(() => cachedState.apiConfiguration ?? {}, [cachedState.apiConfiguration]) | ||
|
|
||
| // Track if this is the initial mount to avoid marking as dirty on first sync | ||
| const isInitialMount = useRef(true) |
There was a problem hiding this comment.
Missing cleanup for isInitialMount: If the component unmounts and remounts (e.g., when navigating away and back to settings), isInitialMount will remain false, potentially causing the dirty state to be set on subsequent mounts.
Consider resetting on unmount or using a different approach to track initial sync.
|
|
||
| const apiConfiguration = useMemo(() => cachedState.apiConfiguration ?? {}, [cachedState.apiConfiguration]) | ||
|
|
||
| // Track if this is the initial mount to avoid marking as dirty on first sync |
There was a problem hiding this comment.
Code comment clarity: This comment could be more specific. Consider:
// Track if this is the component's first mount to prevent marking as dirty during initial state synchronization| } | ||
| }, [settingsImportedAt, extensionState]) | ||
|
|
||
| // Sync with extension state on mount without marking as dirty |
There was a problem hiding this comment.
Consider consolidating initial sync logic: Lines 201-211 and 221-228 both handle initial state synchronization but in slightly different ways. The first handles API config changes, the second handles initial mount. Consider unifying this logic to reduce complexity and potential inconsistencies.
hannesrudolph
added
the
Issue/PR - Triage
This PR fixes Issue #8230 where the Settings dialog incorrectly shows an "unsaved changes" prompt after changing Mode/API in the main interface, even when no settings were actually modified in the dialog.
Problem
After changing the Mode or API from the main interface and then opening Settings and clicking Done, users were seeing an "unsaved changes" prompt despite not making any changes within the Settings dialog.
Solution
isInitialMountref to track the first mount of SettingsViewsetApiConfigurationFieldto only mark as dirty for actual user actions (whenisUserActionis true)Testing
Review Confidence
Code review tool reported 95% confidence with PROCEED recommendation.
Fixes #8230
Important
Fixes false 'unsaved changes' prompt in
SettingsView.tsxby tracking initial mount and marking dirty state only for user actions.SettingsView.tsxafter Mode/API change.isInitialMountref to track first mount and prevent false dirty state.setApiConfigurationFieldto mark dirty only for user actions.SettingsView.spec.tsx).This description was created by
for 8109ac3. You can customize this summary. It will automatically update as commits are pushed.