Skip to content

fix: prevent false dirty state when opening Settings after Mode/API change #8232

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: prevent false dirty state when opening Settings after Mode/API change #8232

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions webview-ui/src/components/settings/SettingsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t

const apiConfiguration = useMemo(() => cachedState.apiConfiguration ?? {}, [cachedState.apiConfiguration])

// Track if this is the initial mount to avoid marking as dirty on first sync
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const isInitialMount = useRef(true)
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


useEffect(() => {
// Update only when currentApiConfigName is changed.
// Expected to be triggered by loadApiConfiguration/upsertApiConfiguration.
Expand All @@ -215,6 +218,15 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
}
}, [settingsImportedAt, extensionState])

// Sync with extension state on mount without marking as dirty
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

useEffect(() => {
if (isInitialMount.current) {
setCachedState(extensionState)
setChangeDetected(false)
isInitialMount.current = false
}
Copy link
Contributor Author

@roomote roomote bot Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

}, [extensionState])

const setCachedStateField: SetCachedStateField<keyof ExtensionStateContextType> = useCallback((field, value) => {
setCachedState((prevState) => {
if (prevState[field] === value) {
Expand All @@ -233,13 +245,9 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
return prevState
}

const previousValue = prevState.apiConfiguration?.[field]

// Only skip change detection for automatic initialization (not user actions)
// This prevents the dirty state when the component initializes and auto-syncs values
const isInitialSync = !isUserAction && previousValue === undefined && value !== undefined

if (!isInitialSync) {
// Only mark as changed if this is a user action
// Don't mark as changed for programmatic updates (like initial sync)
if (isUserAction) {
setChangeDetected(true)
}
return { ...prevState, apiConfiguration: { ...prevState.apiConfiguration, [field]: value } }
Expand Down
Loading