Conversation
Setting allow single update
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue with settings reload by changing how user preferences are updated. Instead of replacing the entire Preferences map when an update request is received, the code now merges incoming preferences with existing ones.
Key Changes:
- Modified the
Applymethod to merge preference updates instead of replacing the entire map - Added nil check and default initialization for preferences
- Preserves existing preference values that are not included in the update request
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s.Preferences = req.Preferences | ||
| // Merge preferences instead of replacing them | ||
| if s.Preferences == nil { | ||
| s.Preferences = DefaultPreferences() |
There was a problem hiding this comment.
Initializing with DefaultPreferences() when s.Preferences is nil may not be necessary here, since ensureDefaultPreferences() in the Service layer already handles merging defaults. Consider using an empty map instead (make(map[string]interface{})) to keep the Apply method focused solely on merging the request values without adding default initialization logic.
| s.Preferences = DefaultPreferences() | |
| s.Preferences = make(map[string]interface{}) |
| if req.Preferences != nil { | ||
| s.Preferences = req.Preferences | ||
| // Merge preferences instead of replacing them | ||
| if s.Preferences == nil { | ||
| s.Preferences = DefaultPreferences() | ||
| } | ||
| for key, value := range req.Preferences { | ||
| s.Preferences[key] = value | ||
| } | ||
| } |
There was a problem hiding this comment.
This change introduces inconsistent behavior in the Apply method. All other fields (MemoryConfig, ProfileSettings, AdvancedSettings) use full replacement semantics when non-nil, but Preferences now uses merge semantics. This inconsistency could be confusing for API consumers. Consider documenting this behavior difference in the method's comment or making the behavior consistent across all fields.
Hot fix on settings reload