fix: replace unsaved changes dialog with auto-save system #7601
Conversation
- Remove disruptive "Unsaved Changes" confirmation dialog - Implement automatic saving with 500ms debouncing - Add visual status indicators (orange for saving, green for saved, red for errors) - Prevent profile switching during active save operations - Update translation strings to reflect new behavior - Simplify checkUnsaveChanges to work with auto-save - Background saves continue even when closing settings panel Fixes #7599
- Remove intrusive unsaved changes dialog that blocked profile switching - Implement debounced auto-save (500ms delay) for all provider settings - Add visual save status indicator (three dots animation during save) - Prevent profile switching while save is in progress - Add error handling with toast notifications for save failures - Update tests to reflect new auto-save behavior Fixes #7599
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| setSaveStatus("saved") | ||
| // Reset to idle after showing saved status for a bit | ||
| setTimeout(() => { | ||
| setSaveStatus("idle") |
There was a problem hiding this comment.
Is this intentional? The setTimeout here could accumulate if users make rapid changes, potentially causing a memory leak. Consider storing the timeout reference and clearing it before setting a new one:
| setSaveStatus("idle") | |
| const timeoutRef = useRef<NodeJS.Timeout>(); | |
| // ... | |
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | |
| timeoutRef.current = setTimeout(() => { | |
| setSaveStatus("idle") | |
| }, 2000) |
| // If we're currently saving, wait for it to complete | ||
| if (saveStatus === "saving") { | ||
| // Wait a bit for save to complete, then execute | ||
| setTimeout(then, 100) |
There was a problem hiding this comment.
The 100ms timeout here seems arbitrary. What happens if the save operation takes longer than 100ms? Could we use a more robust approach like a Promise or event-based system to ensure the action only executes after save completes?
| }, 2000) | ||
| } catch (error) { | ||
| setSaveStatus("error") | ||
| setSaveError(error instanceof Error ? error.message : "Failed to save settings") |
There was a problem hiding this comment.
When saveStatus is 'error', users have no way to retry the save except by making another change. Should we consider adding a manual retry button or automatically retry after a delay?
| <div className="flex items-center gap-1"> | ||
| {saveStatus === "saving" && ( | ||
| <StandardTooltip content={t("settings:status.saving")}> | ||
| <Circle className="w-3 h-3 fill-orange-500 text-orange-500" /> |
There was a problem hiding this comment.
These status indicators rely solely on color to convey meaning, which could be problematic for users with color vision deficiencies. Consider adding aria-label attributes or visible text labels for better accessibility:
| <Circle className="w-3 h-3 fill-orange-500 text-orange-500" /> | |
| <Circle className="w-3 h-3 fill-orange-500 text-orange-500" aria-label="Saving changes" /> |
| // Volume slider should not be visible when sound is disabled | ||
| expect(screen.queryByTestId("sound-volume-slider")).not.toBeInTheDocument() | ||
| }) | ||
|
|
There was a problem hiding this comment.
The tests cover the happy path well, but we're missing coverage for error scenarios. Could we add tests for:
- Save failures and error status display
- Race conditions during rapid changes
- Profile switching while save is in progress
| profileThresholds, | ||
| openRouterImageApiKey, | ||
| openRouterImageGenerationSelectedModel, | ||
| ]) |
There was a problem hiding this comment.
This dependency array with 70+ items is quite unwieldy. Could we simplify this by either:
- Using a single state object to reduce dependencies
- Extracting this to a custom hook like useAutoSave
- Using a ref for values that don't need to trigger re-execution?
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Fixes #7599
Summary
This PR replaces the problematic "Unsaved Changes" dialog with a modern auto-save system that provides a better user experience.
Problem
Solution
Important
Replaces unsaved changes dialog with an auto-save system in
SettingsView.tsx, adding debounced saving, visual indicators, and error handling.SettingsView.tsx.SettingsView.spec.tsxto reflect auto-save behavior.settings.jsonfor new save status messages.This description was created by
for 7587a2f. You can customize this summary. It will automatically update as commits are pushed.