Skip to content

fix: Prevent false unsaved-changes prompt after Mode/API change (#8230) #8632

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 unsaved-changes prompt after Mode/API change (#8230) #8632

Show file tree
Hide file tree
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
27 changes: 19 additions & 8 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 @@ -126,6 +126,15 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
const prevApiConfigName = useRef(currentApiConfigName)
const confirmDialogHandler = useRef<() => void>()

// Guard: ignore programmatic syncs during initial mount to prevent false dirty state
const isInitialMountRef = useRef(true)
useEffect(() => {
const id = setTimeout(() => {
isInitialMountRef.current = false
}, 0)
return () => clearTimeout(id)
}, [])

const [cachedState, setCachedState] = useState(() => extensionState)

const {
Expand Down Expand Up @@ -239,15 +248,17 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t

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
// Treat undefined, null, and empty string as uninitialized states
// Only skip change detection for:
// - Automatic initialization (undefined/null/empty -> value)
// - Any non-user programmatic syncs happening during the initial mount of SettingsView
// (prevents dirty state when provider/model defaults auto-sync on open)
const isInitialSync =
!isUserAction &&
(previousValue === undefined || previousValue === "" || previousValue === null) &&
value !== undefined &&
value !== "" &&
value !== null
(!isUserAction &&
(previousValue === undefined || previousValue === "" || previousValue === null) &&
value !== undefined &&
value !== "" &&
value !== null) ||
(!isUserAction && isInitialMountRef.current)

if (!isInitialSync) {
setChangeDetected(true)
Expand Down
12 changes: 6 additions & 6 deletions webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ vi.mock("@src/i18n/TranslationContext", () => ({

// Mock UI components
vi.mock("@src/components/ui", () => ({
AlertDialog: ({ children }: any) => <div>{children}</div>,
AlertDialog: ({ open, children }: any) => (open ? <div data-testid="alert-dialog">{children}</div> : null),
AlertDialogContent: ({ children }: any) => <div>{children}</div>,
AlertDialogTitle: ({ children }: any) => <div>{children}</div>,
AlertDialogDescription: ({ children }: any) => <div>{children}</div>,
Expand Down Expand Up @@ -218,7 +218,7 @@ describe("SettingsView - Unsaved Changes Detection", () => {
// TODO: Fix underlying issue - dialog appears even when no user changes have been made
// This happens because some component is triggering setCachedStateField during initialization
// without properly marking it as a non-user action
it.skip("should not show unsaved changes when settings are automatically initialized", async () => {
it("should not show unsaved changes when settings are automatically initialized", async () => {
const onDone = vi.fn()

render(
Expand Down Expand Up @@ -252,7 +252,7 @@ describe("SettingsView - Unsaved Changes Detection", () => {
})

// TODO: Fix underlying issue - see above
it.skip("should not trigger unsaved changes for automatic model initialization", async () => {
it("should not trigger unsaved changes for automatic model initialization", async () => {
const onDone = vi.fn()

// Mock ApiOptions to simulate ModelPicker initialization
Expand Down Expand Up @@ -351,7 +351,7 @@ describe("SettingsView - Unsaved Changes Detection", () => {
})

// TODO: Fix underlying issue - see above
it.skip("should handle initialization from undefined to value without triggering unsaved changes", async () => {
it("should handle initialization from undefined to value without triggering unsaved changes", async () => {
const onDone = vi.fn()

// Start with undefined apiModelId
Expand Down Expand Up @@ -395,7 +395,7 @@ describe("SettingsView - Unsaved Changes Detection", () => {
})

// TODO: Fix underlying issue - see above
it.skip("should handle initialization from null to value without triggering unsaved changes", async () => {
it("should handle initialization from null to value without triggering unsaved changes", async () => {
const onDone = vi.fn()

// Start with null apiModelId
Expand Down Expand Up @@ -439,7 +439,7 @@ describe("SettingsView - Unsaved Changes Detection", () => {
})

// TODO: Fix underlying issue - see above
it.skip("should not trigger changes when ApiOptions syncs model IDs during mount", async () => {
it("should not trigger changes when ApiOptions syncs model IDs during mount", async () => {
const onDone = vi.fn()

// This specifically tests the bug we fixed where ApiOptions' useEffect
Expand Down
Loading