Fix: prevent full settings reset on schema mismatch#1263
Merged
Conversation
github-actions
Bot
added
bug
Contributor
Rust Backend Coverage ReportCoverage Details |
Contributor
There was a problem hiding this comment.
Pull request overview
Improves robustness of persisted Tauri/Rust settings loading so app updates or schema/value mismatches don’t trigger a full settings reset, while also backfilling new/defaulted fields.
Changes:
- Adds
#[serde(default)]+Defaultimpls to settings-related structs to allow deserialization with missing fields. - Implements a fallback “field-level recovery” path when full
Settingsdeserialization fails. - Adds unit tests covering missing-field deserialization and fallback recovery behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src-tauri/src/services/settings_service.rs | Refactors settings file read, adds fallback merge-based recovery, and persists settings on app version change. |
| src-tauri/src/models/settings.rs | Adds serde defaults + Default impls for nested settings structs and adds tests for compatibility/recovery. |
| src-tauri/src/models/hardware_archive.rs | Adds serde defaults + Default impl for HardwareArchiveSettings to support missing-field deserialization. |
…e error logging in settings service
| ); | ||
| Err(format!("Failed to read settings file: {e}")) | ||
| // Fall back to field-by-field recovery | ||
| self.merge_from_json_str(&input) |
There was a problem hiding this comment.
When full deserialization fails but merge_from_json_str succeeds, the recovered settings are not persisted unless settings.version != current_version. If the file contains a permanently invalid value (e.g. invalid enum string) but the version already matches, startup will log an error and fall back on every run. Consider writing the recovered/defaulted settings back to disk after a successful merge (possibly with a best-effort write_file() + log on failure).
Suggested change
| self.merge_from_json_str(&input) | |
| match self.merge_from_json_str(&input) { | |
| Ok(()) => { | |
| // Best-effort: try to persist the recovered/defaulted settings so that | |
| // subsequent startups don't repeatedly hit the recovery path. | |
| if let Err(write_err) = self.write_file() { | |
| log_error!( | |
| "Recovered settings but failed to persist updated settings file", | |
| "read_file", | |
| Some(write_err) | |
| ); | |
| } | |
| Ok(()) | |
| } | |
| Err(recover_err) => Err(recover_err), | |
| } |
Comment on lines
112
to
116
| log_error!( | ||
| "Failed to deserialize settings", | ||
| "Failed to deserialize settings, attempting field-level recovery", | ||
| "read_file", | ||
| Some(e.to_string()) | ||
| ); |
There was a problem hiding this comment.
This branch logs an error even though the code can recover and continue successfully via field-level merge. Consider logging this as warn (and reserving error for cases where recovery also fails) to avoid misleading error-level logs for recoverable schema drift/corruption.
Comment on lines
+27
to
+32
| impl Default for HardwareArchiveSettings { | ||
| fn default() -> Self { | ||
| Self { | ||
| enabled: true, | ||
| refresh_interval_days: 30, | ||
| scheduled_data_deletion: true, |
There was a problem hiding this comment.
Now that HardwareArchiveSettings implements Default and is used for #[serde(default)] deserialization, there are multiple hard-coded copies of these same default values elsewhere (e.g. Settings::default and settings tests). To avoid future divergence, consider switching those call sites to HardwareArchiveSettings::default() as the single source of truth for defaults.
Suggested change
| impl Default for HardwareArchiveSettings { | |
| fn default() -> Self { | |
| Self { | |
| enabled: true, | |
| refresh_interval_days: 30, | |
| scheduled_data_deletion: true, | |
| impl HardwareArchiveSettings { | |
| /// Default value for whether hardware archiving is enabled. | |
| pub const DEFAULT_ENABLED: bool = true; | |
| /// Default value for whether scheduled data deletion is enabled. | |
| pub const DEFAULT_SCHEDULED_DATA_DELETION: bool = true; | |
| /// Default refresh interval in days for hardware archiving. | |
| pub const DEFAULT_REFRESH_INTERVAL_DAYS: u32 = 30; | |
| } | |
| impl Default for HardwareArchiveSettings { | |
| fn default() -> Self { | |
| Self { | |
| enabled: Self::DEFAULT_ENABLED, | |
| scheduled_data_deletion: Self::DEFAULT_SCHEDULED_DATA_DELETION, | |
| refresh_interval_days: Self::DEFAULT_REFRESH_INTERVAL_DAYS, |
Comment on lines
+16
to
+23
| impl Default for LineGraphColorSettings { | ||
| fn default() -> Self { | ||
| Self { | ||
| cpu: [75, 192, 192], | ||
| memory: [255, 99, 132], | ||
| gpu: [255, 206, 86], | ||
| } | ||
| } |
There was a problem hiding this comment.
LineGraphColorSettings now has a Default impl (and #[serde(default)] depends on it), but the same default RGB values are still duplicated in Settings::default and some tests. Consider using LineGraphColorSettings::default() at those call sites to keep defaults consistent and reduce maintenance overhead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request improves the robustness and backward compatibility of settings deserialization and management. It ensures that missing fields in
settings.jsonor related structs are safely filled with default values, and adds logic to recover from partial or invalid settings files. Additionally, it updates the settings version automatically and persists new fields after app upgrades. The most important changes are grouped below:Settings deserialization and recovery
#[serde(default)]toSettings,LineGraphColorSettings,BurnInShiftOptions, andHardwareArchiveSettingsstructs, and implemented theirDefaulttrait to ensure missing fields are filled with sensible defaults during deserialization. [1] [2] [3]read_filemethod inSettingsServiceto attempt field-by-field recovery if full deserialization fails, logging errors and preserving valid fields while falling back to defaults for invalid or missing fields.merge_from_json_strmethod toSettingsfor granular field recovery from a JSON object, with detailed error logging for each failed field.Version management and persistence
Settings::newmethod to automatically update the settings version and persist new fields added by app updates, ensuring that the settings file stays current.Testing and validation