[2/12] feat: core primitives — atomic JSON, DPAPI secrets, single-instance, window placement - #11
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Not ready to approve
The single-instance mutex acquisition can block/hang secondary launches and mishandles mutex ownership, and the DPI conversion fallback incorrectly assumes uniform X/Y scaling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds foundational Windows-native “core primitives” under windows-native/Muesli.Windows/Services/ to support future app-shell wiring (atomic JSON persistence, secret storage, single-instance activation, and window/work-area placement helpers).
Changes:
- Introduces
AtomicJsonFilefor recoverable, atomic JSON persistence (temp write + replace, backup/quarantine handling). - Adds Windows secrets storage (
ISecretStore+ Credential Manager-backed implementation) and a simple in-memory fallback. - Adds primitives for single-instance coordination (named mutex + activation pipe), cross-process file locking, and DPI-aware work-area/window placement.
File summaries
| File | Description |
|---|---|
| windows-native/Muesli.Windows/Services/AtomicJsonFile.cs | Atomic JSON read/write with backup/quarantine recovery and privacy-sensitive purge mode. |
| windows-native/Muesli.Windows/Services/CrossProcessFileLock.cs | Async cross-process lock acquisition via exclusive lock file. |
| windows-native/Muesli.Windows/Services/MuesliPathService.cs | User profile path resolution helper with validation. |
| windows-native/Muesli.Windows/Services/SecretStore.cs | Secret store abstraction plus Windows Credential Manager and in-memory implementations. |
| windows-native/Muesli.Windows/Services/SingleInstanceCoordinator.cs | Named mutex + named pipe activation signaling for single-instance behavior. |
| windows-native/Muesli.Windows/Services/WindowPlacementService.cs | Monitor/work-area helpers and pixel↔DIP conversions for window placement. |
Review details
Suppressed comments (1)
windows-native/Muesli.Windows/Services/WindowPlacementService.cs:134
- This DPI transform fallback assumes uniform scaling by using only
DpiScaleXand applying it to both axes. IfDpiScaleXandDpiScaleYdiffer, work-area conversion will be wrong in Y. Build the matrix with separate X/Y scale values (with a safe fallback).
var dpi = VisualTreeHelper.GetDpi(window);
var scale = dpi.DpiScaleX > 0 ? dpi.DpiScaleX : Math.Max(1d, GetDpiForWindow(handle) / 96d);
return new Matrix(scale, 0, 0, scale, 0, 0);
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| public static SingleInstanceCoordinator AcquireForCurrentUser() | ||
| { | ||
| var identity = WindowsIdentity.GetCurrent().User?.Value ?? Environment.UserName; | ||
| var suffix = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(identity)))[..20]; | ||
| var mutex = new Mutex(initiallyOwned: true, $"Local\\Muesli.Windows.UI.{suffix}", out var createdNew); | ||
| return new SingleInstanceCoordinator(mutex, $"Muesli.Windows.Activation.{suffix}", createdNew); | ||
| } |
| private static Matrix GetTransformFromDevice(Window window, IntPtr handle) | ||
| { | ||
| var source = GetHwndSource(window, handle); | ||
| if (source?.CompositionTarget is not null) return source.CompositionTarget.TransformFromDevice; | ||
| var dpi = VisualTreeHelper.GetDpi(window); | ||
| var scale = dpi.DpiScaleX > 0 ? dpi.DpiScaleX : Math.Max(1d, GetDpiForWindow(handle) / 96d); | ||
| return new Matrix(1 / scale, 0, 0, 1 / scale, 0, 0); | ||
| } |
| } | ||
| catch (IOException exception) | ||
| { | ||
| throw new TimeoutException($"Timed out waiting for the model setup lock '{Path.GetFileName(path)}'.", exception); |
eda67f4 to
b84e056
Compare
|
Claude finished @Mvkd108's task in 4m 6s —— View job Review: core primitives (AtomicJsonFile, CrossProcessFileLock, MuesliPathService, SecretStore, SingleInstanceCoordinator, WindowPlacementService)
Reviewed against Findings1.
2.
Side note: this class is Windows Credential Manager backed ( 3. I looked into this since it was flagged as blocking. Per Win32 One smaller, real observation in the same file: the pipe listener's fallback catch block ( 4. Confirmed: both 5. Agreed with Copilot here — this is a generic file-lock helper (used generically per its name), but the Not flagged
|
…vice Introduces AtomicJsonFile for crash-safe JSON writes, CrossProcessFileLock for coordinating access across app instances, and MuesliPathService as the single source of truth for app data locations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stores API keys encrypted at rest with the current user's Windows DPAPI key rather than in plaintext settings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ensures only one Muesli instance owns the hotkeys and capture devices, handing activation to the running instance instead of starting a second. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Centralises monitor-aware window positioning and restores placement across DPI and display-topology changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b84e056 to
7b12d57
Compare
This PR (2/12): core primitives
Commits:
437c712feat: atomic JSON persistence, cross-process locking and path service —Services/AtomicJsonFile.cs,Services/CrossProcessFileLock.cs,Services/MuesliPathService.csd6a7664feat: DPAPI-backed secret store —Services/SecretStore.csa8a677afeat: single-instance coordinator —Services/SingleInstanceCoordinator.cseda67f4feat: window placement service —Services/WindowPlacementService.cs6 files, +800. All paths under
windows-native/Muesli.Windows/Services/.Review focus
AtomicJsonFile+CrossProcessFileLock— write protocol (temp file + atomic replace), lock scope and contention behavior; corruption recovery on malformed JSON.SecretStore— DPAPI scope (CurrentUser), entropy handling, failure modes when DPAPI is unavailable.SingleInstanceCoordinator— second-instance handoff protocol and race windows at startup.WindowPlacementService— off-screen/monitor-removed rescue logic, DPI awareness of saved coordinates.Expected — do not flag: these primitives have almost no callers yet; they are wired into the app in PR 9 (
feat/rebuild-app-shell). Forward references are intentional.Test evidence
dotnet build windows-native\Muesli.Windows\Muesli.Windows.csproj --no-restoreclean;dotnet test windows-native\Muesli.Windows.Tests\Muesli.Windows.Tests.csproj --no-restore— 484/484 passing.AtomicPersistenceTests,SecretsAndSettingsTests,ModelAndSingleInstanceTests.Previous: #10 | Next: #12