Skip to content

CmdPal: Extract persistence services from SettingsModel and AppStateModel#46312

Merged
michaeljolley merged 8 commits intomainfrom
dev/mjolley/settings
Mar 20, 2026
Merged

CmdPal: Extract persistence services from SettingsModel and AppStateModel#46312
michaeljolley merged 8 commits intomainfrom
dev/mjolley/settings

Conversation

@michaeljolley
Copy link
Copy Markdown
Contributor

@michaeljolley michaeljolley commented Mar 20, 2026

Summary of the Pull Request

Extracts persistence (load/save) logic from SettingsModel and AppStateModel into dedicated service classes, following the single-responsibility principle. Consumers now interact with ISettingsService and IAppStateService instead of receiving raw model objects through DI.

New services introduced:

  • IPersistenceService / PersistenceService — generic Load<T> / Save<T> with AOT-compatible JsonTypeInfo<T>, ensures target directory exists before writing
  • ISettingsService / SettingsService — loads settings on construction, runs migrations, exposes Settings property and SettingsChanged event
  • IAppStateService / AppStateService — loads state on construction, exposes State property and StateChanged event

Key changes:

  • SettingsModel and AppStateModel are now pure data models — all file I/O, migration, and directory management removed
  • Raw SettingsModel and AppStateModel removed from DI container; consumers receive the appropriate service
  • IApplicationInfoService.ConfigDirectory injected into services for config path resolution (no more hardcoded Utilities.BaseSettingsPath)
  • ~30 consumer files updated across Microsoft.CmdPal.UI.ViewModels and Microsoft.CmdPal.UI projects
  • All #pragma warning disable SA1300 suppressions removed — convenience accessors replaced with direct _settingsService.Settings / _appStateService.State access
  • Namespace prefixes (Services.ISettingsService) replaced with proper using directives

PR Checklist

  • Communication: I've discussed this with core contributors already.
  • Tests: Added/updated and all pass
  • Localization: N/A — no end-user-facing strings changed
  • Dev docs: N/A — internal refactor, no public API changes
  • New binaries: N/A — no new binaries introduced

Detailed Description of the Pull Request / Additional comments

Architecture

Services are registered as singletons in App.xaml.cs:

services.AddSingleton<IPersistenceService, PersistenceService>();
services.AddSingleton<ISettingsService, SettingsService>();
services.AddSingleton<IAppStateService, AppStateService>();

PersistenceService.Save<T> writes the serialized model directly to disk, creating the target directory if it doesn't exist. It also does not attempt to merge existing and new settings/state. SettingsService runs hotkey migrations on load and raises SettingsChanged after saves. AppStateService always raises StateChanged after saves.

Files changed (41 files, +1169/−660)

Area Files What changed
New services Services/IPersistenceService.cs, PersistenceService.cs, ISettingsService.cs, SettingsService.cs, IAppStateService.cs, AppStateService.cs New service interfaces and implementations
Models SettingsModel.cs, AppStateModel.cs Stripped to pure data bags
DI App.xaml.cs Service registration, removed raw model DI
ViewModels 12 files Constructor injection of services
UI 10 files Service injection replacing model access
Settings DockSettings.cs Colors.Transparent replaced with struct literal to avoid WinUI3 COM dependency
Tests PersistenceServiceTests.cs, SettingsServiceTests.cs, AppStateServiceTests.cs 38 unit tests covering all three services
Config .gitignore Added .squad/, .github/agents/ exclusions

Validation Steps Performed

  • Built Microsoft.CmdPal.UI with MSBuild (x64/Debug) — exit code 0, clean build
  • Ran 38 unit tests via vstest.console.exe — all passing
  • Verified no remaining #pragma warning disable SA1300 blocks
  • Verified no remaining Services. namespace prefixes

michaeljolley and others added 3 commits March 19, 2026 16:50
- Created orchestration logs for Snake Eyes (Phases 1-4 implementation)
- Created orchestration logs for Duke (code review, APPROVED verdict)
- Created orchestration logs for Hawk (23 unit tests, 14/14 passing)
- Written session log documenting complete persistence service extraction
- Merged decision inbox files into decisions.md (2 decisions recorded)
- Updated agent history files (snake-eyes, duke, hawk) with session learnings
- Cleaned up decision inbox (3 files processed and removed)

This session completed the persistence service architecture for CmdPal:
- Extracted logic from SettingsModel/AppStateModel into service layer
- Created IPersistenceService, ISettingsService, IAppStateService
- Updated ~20 consumer files with proper DI injection
- Verified migration logic preserved, contracts stable, no ABI breaks
- Established test patterns for future service development

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove raw model DI registrations
- Inject IApplicationInfoService into persistence services
- Update 42+ consumer locations across ~26 files
- Build passing, 43/43 tests passing
- Code review approved

Orchestration logs created for Snake Eyes and Duke.
Decision record merged and deduped.
… AppStateModel

- Create IPersistenceService, ISettingsService, and IAppStateService interfaces
- Implement PersistenceService with generic Load<T>/Save<T> and shallow-merge
- Implement SettingsService with migration support and hot-reload events
- Implement AppStateService with load/save lifecycle and state change events
- Strip all persistence logic from SettingsModel and AppStateModel (pure data models)
- Inject IApplicationInfoService for config directory resolution
- Remove raw model DI registrations; all consumers use service interfaces
- Update 31 consumer files from direct model access to service-based access
- Replace Colors.Transparent with struct literal to remove WinUI3 COM dependency
- Add 23 unit tests (PersistenceService, SettingsService, AppStateService)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

michaeljolley and others added 3 commits March 19, 2026 21:12
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace relative namespace references (Services.ISettingsService,
Services.IAppStateService) with proper using directives across
7 ViewModel files for consistency with the rest of the codebase.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace underscore-prefixed convenience properties (_settings,
_settingsModel, _appStateModel, _dockSettings) with direct service
property access (_settingsService.Settings, _appStateService.State).

Removes all #pragma warning disable SA1300 suppressions (14 blocks
across 12 files, ~192 inline replacements).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@michaeljolley michaeljolley changed the title refactor(cmdpal): extract persistence services from SettingsModel and AppStateModel CmdPal: Extract persistence services from SettingsModel and AppStateModel Mar 20, 2026
@michaeljolley michaeljolley self-assigned this Mar 20, 2026
@michaeljolley michaeljolley added the Product-Command Palette Refers to the Command Palette utility label Mar 20, 2026
@michaeljolley michaeljolley marked this pull request as ready for review March 20, 2026 18:55
michaeljolley and others added 2 commits March 20, 2026 16:11
PersistenceService.Save now calls Directory.CreateDirectory before
File.WriteAllText to prevent DirectoryNotFoundException on first run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nd Reload

- Remove shallow-merge strategy from PersistenceService.Save (direct
  write replaces read-merge-write)
- Remove postProcessMerge callback parameter from IPersistenceService
- Remove Reload() from ISettingsService and SettingsService
- Consolidate Directory.CreateDirectory into PersistenceService.Save
  only, removing redundant calls from service path helpers
- Update test mocks to match new 3-arg Save signature
- Remove tests for dropped features (shallow merge, postProcessMerge,
  Reload)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@michaeljolley michaeljolley changed the title CmdPal: Extract persistence services from SettingsModel and AppStateModel refactor(cmdpal): extract persistence services from SettingsModel and AppStateModel Mar 20, 2026
@michaeljolley michaeljolley changed the title refactor(cmdpal): extract persistence services from SettingsModel and AppStateModel CmdPal: Extract persistence services from SettingsModel and AppStateModel Mar 20, 2026
Copy link
Copy Markdown
Collaborator

@jiripolasek jiripolasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Image

@jiripolasek jiripolasek added this to the PowerToys 0.99 milestone Mar 20, 2026
@michaeljolley michaeljolley merged commit 86115a5 into main Mar 20, 2026
15 checks passed
@michaeljolley michaeljolley deleted the dev/mjolley/settings branch March 20, 2026 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Product-Command Palette Refers to the Command Palette utility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants