settings: Use fallible parsing in edits_for_update to preserve settings#52081
settings: Use fallible parsing in edits_for_update to preserve settings#52081OmChillure wants to merge 4 commits intozed-industries:mainfrom
edits_for_update to preserve settings#52081Conversation
…gs during migration
MrSubidubi
left a comment
There was a problem hiding this comment.
Passing by and am fairly sure that we want a test case for this so that we both know the fix is good and that it will not regress in the future.
Similarly, I think we should probably at least discuss what to do when the parsing failed as oppposed to just falling back to defaults here and then messing around with the user settings anyway despite them having obvious errors.
Sorry, will write the tests too ! For the parsing failure case : and can add a toast if necessary ? What are your suggestions? |
That makes sense, although we probably need a result for that.
Playing around with this might similarly make sense, think that sounds like a good path forward. |
edits_for_update to preserve settings
|
Hello @MrSubidubi I have Updated Added two tests:
Need some help here : For surfacing the parse failure to the user - should we use a |
|
The Result signature change on Also fixed an edge case where empty settings input (e.g. a new settings.json with no content) was incorrectly treated as a parse error. Empty input now falls back to defaults, matching the original behavior. On parse failure, callers now skip editing instead of proceeding with bad data. This is consistent with the PR's goal of not silently corrupting settings. An alternative would be to revert the signature change and handle the error internally (return empty Vec + log), keeping the PR as a single-file change. But that would mean callers have no way to know parsing failed, and silently returning empty edits felt too close to the original bug. Happy to go either way !! Thank you |
## Context `update_settings_file` deletes unrelated settings when the settings file contains deprecated keys that need migration. For example, changing the model from the Agent Panel overwrites the entire `agent` block instead of just updating `default_model`. The root cause is that `edits_for_update` used `parse_json_with_comments` (strict parser), which returns `Err` on deprecated/unknown fields. The error is swallowed by `log_err()`, falling back to `Default::default()` (empty settings). The diff then sees everything as new and replaces the entire block. The fix switches to `parse_json` (the fallible/lenient parser), which returns `Some(parsed_value)` even when deprecated fields are present - the same pattern already used by `parse_and_migrate_zed_settings`. ## Fixes #41344 ## How to Review Single-file change in `settings_store.rs`, focus on `edits_for_update` . Compare with `parse_and_migrate_zed_settings` (line 702) which already uses the same `parse_json` approach. ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Video [Screencast from 2026-03-21 00-36-12.webm](https://github.com/user-attachments/assets/31bd584a-2674-4c91-bdb2-69ed8fa35e88) ### Note : Reopens previous work from closed PR #52081 (fork was deleted) Release Notes: - Fixed settings being overwritten when updating a single setting via UI while the settings file contains deprecated keys.
Context
update_settings_filedeletes unrelated settings when the settings file contains deprecated keys that need migration. For example, changing the model from the Agent Panel overwrites the entireagentblock instead of just updatingdefault_model.The root cause is that
edits_for_updateusedparse_json_with_comments(strict parser), which returnsErron deprecated/unknown fields. The error is swallowed bylog_err(), falling back toDefault::default()(empty settings). The diff then sees everything as new and replaces the entire block.The fix switches to
parse_json(the fallible/lenient parser), which returnsSome(parsed_value)even when deprecated fields are present - the same pattern already used byparse_and_migrate_zed_settings.Fixes #41344
How to Review
Single-file change in
settings_store.rs, focus onedits_for_update. Compare withparse_and_migrate_zed_settings(line 702) which already uses the sameparse_jsonapproach.Self-Review Checklist
Video
Screencast.from.2026-03-21.00-36-12.webm
Release Notes: