fix(gui-client): don't reset favourites when settings change#9211
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
thomaseizinger
enabled auto-merge
May 23, 2025 02:04
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR separates user-facing settings (favourites and Internet resource status) from developer-focused advanced settings to prevent overwriting favourites on saving advanced settings. It introduces a GeneralSettings struct, renames the original AdvancedSettings to AdvancedSettingsLegacy, and implements a migration path on startup.
- Split settings into
GeneralSettings(favourites, Internet resource) andAdvancedSettings(URLs, log filter) - Renamed original struct to
AdvancedSettingsLegacyand created newAdvancedSettings - Added
migrate_legacy_settingslogic and updated load/save calls across modules
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| settings.rs | Refactored settings structs, added migration, new load/save functions, paths for general and advanced settings |
| gui.rs | Updated run signature to accept legacy settings and invoke migration; replaced calls to save with save_advanced |
| controller.rs | Added general_settings field, updated constructor and event handlers to use GeneralSettings and save_general |
Comments suppressed due to low confidence (3)
rust/gui-client/src-tauri/src/settings.rs:96
- [nitpick] The method name
internet_resource_enabledconflicts with the field of the same name. Renaming it tois_internet_resource_enabledwould improve clarity.
pub fn internet_resource_enabled(&self) -> bool {
rust/gui-client/src-tauri/src/settings.rs:190
- [nitpick] The generic
load_advanced_settings<T>may be confusing since it always reads from the advanced settings path. Consider restricting it to a concrete return type or renaming to clarify its use.
pub fn load_advanced_settings<T>() -> Result<T>
rust/gui-client/src-tauri/src/settings.rs:210
- There are no tests covering
migrate_legacy_settings. Adding unit tests for both migration success and failure paths would ensure future changes don’t regress this logic.
#[cfg(test)]
Member
Author
|
Tested in the Windows VM and works. |
jamilbk
approved these changes
May 23, 2025
thomaseizinger
pushed a commit
that referenced
this pull request
May 18, 2026
A user who skipped every GUI release between #9211 (May 2025, when `auth_base_url` was renamed to `auth_url`) and this PR would otherwise have their custom self-hosted portal URL silently reset to the default when the new `AdvancedSettings` struct failed to parse the old field name. Add a serde alias so the old field name still deserializes, with a regression test covering the pre-9211 on-disk shape. Favourites and internet-resource state from that era still get dropped (they're recoverable from the UI), but URL configuration is preserved.
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.
The GUI client currently has a bug that resets the favourites and the status of the Internet Resource every time the advanced settings are saved. This happens because those fields are annotated with
#[serde(default)]and are thus initialised to their default value when the struct is deserialised from the frontend.To mitigate this, we introduce a new
GeneralSettingsstruct that holds the status of the Internet Resource and the list of favourites. When a client starts up, it will try to migrate the existing advanced settings into the new split of general and advanced settings.