-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix: Addresses overeager 'there are unsaved changes' dialog in settings #8410
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
| // This prevents the dirty state when the component initializes and auto-syncs values | ||
| const isInitialSync = !isUserAction && previousValue === undefined && value !== undefined | ||
| // Treat undefined, null, and empty string as uninitialized states | ||
| const isInitialSync = |
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.
Nice improvement: treating undefined, null, and empty string as uninitialized prevents false change detection. Consider extracting this check into a helper for reuse and clarity.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
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.
I found some issues that need attention. See inline comments for details.
| setApiConfigurationField("apiModelId", selectedModelId) | ||
| // Pass false as third parameter to indicate this is not a user action | ||
| // This is an internal sync, not a user-initiated change | ||
| setApiConfigurationField("apiModelId", selectedModelId, 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.
[P2] Internal sync path correctly marks isUserAction=false. To avoid future drift, audit similar internal sync sites (provider/model defaults, migrations) and consider centralizing via a small helper to ensure they always pass false.
| const isInitialSync = !isUserAction && previousValue === undefined && value !== undefined | ||
| // Treat undefined, null, and empty string as uninitialized states | ||
| const isInitialSync = | ||
| !isUserAction && |
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.
[P3] The isInitialSync detection is duplicated logic you’ll likely want consistent across fields. Consider extracting a small helper (e.g., isInitializingValue(prev, next, isUserAction)) and unit-testing it so future edits don’t regress this behavior.
* origin/main: fix: Addresses overeager 'there are unsaved changes' dialog in settings (#8410) Add structured data to the homepage (#8427) fix: show send button when only images are selected in chat textarea (#8423) Include reasoning messages in cloud tasks (#8401) chore: Remove unsupported Gemini 2.5 Flash Image Preview free model (#8359) A couple more sonnet 4.5 fixes (#8421) Changeset version bump (#8414) chore: add changeset for v3.28.14 (#8413) feat: add GLM-4.6 model support for z.ai provider (#8408)
* chore(deps): update dependency @changesets/cli to v2.29.6 (RooCodeInc#7376) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency nock to v14.0.10 (RooCodeInc#6465) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency eslint-plugin-turbo to v2.5.6 (RooCodeInc#7764) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * refactor(utils): simplify shell detection logic * Merge remote-tracking branch 'upstream/main' into roo-to-main * test: update shell detection test expectation and mocks * test: add mock cleanup in shell detection tests * test: update shell detection tests to prioritize PowerShell 7 * fix(workspace-event-monitor): increase max retries and improve retry logic * refactor(codebase): improve error handling and workspace validation * Revert PR RooCodeInc#7188 - Restore temperature parameter to fix TabbyApi/ExLlamaV2 crashes (RooCodeInc#7594) * fix: reduce CodeBlock button z-index to prevent overlap with popovers (RooCodeInc#7783) Fixes RooCodeInc#7703 - CodeBlock language dropdown and copy button were appearing above popovers due to z-index: 100. Reduced to z-index: 40 to maintain proper layering hierarchy while keeping buttons functional. * Make ollama models info transport work like lmstudio (RooCodeInc#7679) * feat: add click-to-edit, ESC-to-cancel, and fix padding consistency for chat messages (RooCodeInc#7790) * feat: add click-to-edit, ESC-to-cancel, and fix padding consistency - Enable click-to-edit for past messages by making message text clickable - Add ESC key handler to cancel edit mode in ChatTextArea - Fix padding consistency between past and queued message editors - Adjust right padding for edit mode to accommodate cancel button Fixes RooCodeInc#7788 * fix: adjust padding and layout for ChatTextArea in edit mode * refactor: replace hardcoded pr-[72px] with standard Tailwind pr-20 class --------- Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: daniel-lxs <[email protected]> * Let people paste in the auth redirect url (RooCodeInc#7805) Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com> Co-authored-by: Bruno Bergher <[email protected]> * test: change console.error to console.warn in tests * fix: resolve chat message edit/delete duplication issues (RooCodeInc#7793) * fix: add GIT_EDITOR env var to merge-resolver mode for non-interactive rebase (RooCodeInc#7819) * UI: Render reasoning as plain italic (match <thinking>) (RooCodeInc#7752) Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: daniel-lxs <[email protected]> * Add taskSyncEnabled to userSettingsConfigSchema (RooCodeInc#7827) feat: add taskSyncEnabled to userSettingsConfigSchema Co-authored-by: Roo Code <[email protected]> * Release: v1.75.0 (RooCodeInc#7829) chore: bump version to v1.75.0 * fix: prevent negative cost values and improve label visibility in evals chart (RooCodeInc#7830) Co-authored-by: Roo Code <[email protected]> * Fix Groq context window display (RooCodeInc#7839) * test: enhance vscode mock implementations and error handling * feat(chat): replace edit button with copy functionality * refactor(core): enhance binary file detection and encoding handling * separate task sync roomote control (RooCodeInc#7799) * feat: separate Task Sync and Roomote Control settings - Add new taskSyncEnabled setting to control task content syncing - Keep remoteControlEnabled for Roomote Control functionality - Task Sync controls whether task content is sent to cloud - Roomote Control controls whether cloud can send instructions back - Roomote Control now depends on Task Sync being enabled - Usage metrics (tokens, cost) always reported regardless of settings - Update UI with two separate toggles and clear descriptions - Add info text explaining usage metrics are always reported * feat: add missing translations for Task Sync and Roomote Control settings - Added taskSync, taskSyncDescription, remoteControlRequiresTaskSync, and usageMetricsAlwaysReported keys to all non-English cloud.json files - Updated cloudBenefit keys to match English structure - Ensured all languages have consistent translation keys for the new Task Sync and Roomote Control features * Cloud: cleanup taskSyncEnabled additions * fix: correct indentation localization files --------- Co-authored-by: Roo Code <[email protected]> * feat: In-extension dismissible upsells for Roo Code Cloud (RooCodeInc#7850) * First pass at separate upsell dialog * Revert PR RooCodeInc#7188 - Restore temperature parameter to fix TabbyApi/ExLlamaV2 crashes (RooCodeInc#7594) * fix: reduce CodeBlock button z-index to prevent overlap with popovers (RooCodeInc#7783) Fixes RooCodeInc#7703 - CodeBlock language dropdown and copy button were appearing above popovers due to z-index: 100. Reduced to z-index: 40 to maintain proper layering hierarchy while keeping buttons functional. * Make ollama models info transport work like lmstudio (RooCodeInc#7679) * feat: add click-to-edit, ESC-to-cancel, and fix padding consistency for chat messages (RooCodeInc#7790) * feat: add click-to-edit, ESC-to-cancel, and fix padding consistency - Enable click-to-edit for past messages by making message text clickable - Add ESC key handler to cancel edit mode in ChatTextArea - Fix padding consistency between past and queued message editors - Adjust right padding for edit mode to accommodate cancel button Fixes RooCodeInc#7788 * fix: adjust padding and layout for ChatTextArea in edit mode * refactor: replace hardcoded pr-[72px] with standard Tailwind pr-20 class --------- Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: daniel-lxs <[email protected]> * Let people paste in the auth redirect url (RooCodeInc#7805) Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com> Co-authored-by: Bruno Bergher <[email protected]> * fix: resolve chat message edit/delete duplication issues (RooCodeInc#7793) * fix: add GIT_EDITOR env var to merge-resolver mode for non-interactive rebase (RooCodeInc#7819) * UI: Render reasoning as plain italic (match <thinking>) (RooCodeInc#7752) Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: daniel-lxs <[email protected]> * Add taskSyncEnabled to userSettingsConfigSchema (RooCodeInc#7827) feat: add taskSyncEnabled to userSettingsConfigSchema Co-authored-by: Roo Code <[email protected]> * Release: v1.75.0 (RooCodeInc#7829) chore: bump version to v1.75.0 * fix: prevent negative cost values and improve label visibility in evals chart (RooCodeInc#7830) Co-authored-by: Roo Code <[email protected]> * Fix Groq context window display (
Description
Up to now, the "Done" button in settings would always warn about unsaved changes, even when there weren't any.
This fixes the behavior of one of the panels, which was flipping a dirty bit unnecessarily.
Test Procedure
There are automated tests, but to manually check it:
Important
Fixes overeager unsaved changes dialog in settings by refining change detection logic in
SettingsView.tsxandApiOptions.tsx.SettingsView.tsxby refining change detection logic.setApiConfigurationFieldinApiOptions.tsxto passfalsefor non-user actions.SettingsView.change-detection.spec.tsxto test change detection logic.SettingsView.unsaved-changes.spec.tsxto verify unsaved changes dialog behavior.setCachedStatelogic inSettingsView.tsxto prevent unnecessary change detection.This description was created by
for 4e54cf9. You can customize this summary. It will automatically update as commits are pushed.