Repository navigation
Delegate profile/command JSON persistence to ktsu.AppDataStorage #93
Description
What's hand-rolled
JsonKeybindingRepository hand-rolls a JSON-file-per-collection store under the application-data folder:
KeybindingManagerFactory.CreateManager()resolves the default location itself:Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), "Keybinding").JsonKeybindingRepositorythen does its own read/write/corruption handling against that directory for three separate files (profiles.json,commands.json,active-profile.json):File.WriteAllTextAsync/File.ReadAllTextAsync(SaveProfileAsync/LoadAllProfilesInternalAsync, similarly for commands at L153-L198 and the active-profile flag at L201-L235), a hand-writtenDirectory.Exists/Directory.CreateDirectoryinit step (L241-L249), andcatch (JsonException)blocks that silently drop back to an empty collection on a corrupt file (e.g. L116-L120, L193-L197, L230-L234) with no backup or recovery attempt.
What ktsu.AppDataStorage provides
AppData<T> is exactly this: a base class for a POCO that is loaded from and saved to a JSON file under %AppData%/<subdirectory>, resolved through ktsu.Semantics.Paths rather than a raw Environment.GetFolderPath + Path.Combine.
public static T LoadOrCreate(RelativeDirectoryPath? subdirectory, FileName? fileName)(AppData.cs:498) loads an existing file or creates and saves a fresh instance — the exact shape ofLoadAllProfilesInternalAsync's "read, or start empty" logic, but atomic:AppData.WriteText(AppData.cs:142) writes to a.tmpfile, backs up the previous file to.bk, then swaps them in, rather than writing the target file directly.- On a corrupt file,
LoadOrCreate(AppData.cs:516-530) deletes the bad file and retries, which letsAppData.ReadText's own fallback (AppData.cs:176-211) recover from the.bkbackup instead of silently returning an empty collection —JsonKeybindingRepositoryhas no backup file at all, so a corruptprofiles.jsoncurrently just loses every profile. public void Save()(AppData.cs:436) serializes the instance and calls the same atomicWriteText.
Three AppData<T> subclasses (e.g. ProfilesData, CommandsData, ActiveProfileData, each wrapping the same DTO shapes JsonKeybindingRepository already defines) would cover the three files, called through AppData<T>.LoadOrCreate(subdirectory) — the static overload that takes an explicit subdirectory, not the cached Get()/InternalState singleton (see Caveats).
Why it's worth it
The win here is the backup/recovery path, not line count: AppData<T>.Save()/LoadOrCreate() keep a .bk copy and a timestamped backup on corruption recovery, so a truncated write or a corrupted file doesn't silently drop a user's profiles the way the current catch (JsonException) { return []; } blocks do. The three near-identical DTO/save/load blocks in JsonKeybindingRepository would also collapse to three small model classes.
Compatibility
- Subject (
Keybinding.Core) targets:net8.0;net9.0;net10.0 ktsu.AppDataStoragetargets:net10.0;net9.0;net8.0;net7.0;netstandard2.0;netstandard2.1- Dependency direction:
ktsu.AppDataStorage'sDirectory.Packages.propsdepends only onktsu.Semantics.Paths,ktsu.Semantics.Strings,ktsu.RoundTripStringJsonConverter, andktsu.CaseConverter— no dependency on this repo, directly or transitively.
Sketch
// before: Keybinding.Core/Services/JsonKeybindingRepository.cs
string profilesPath = Path.Combine(_dataDirectory, ProfilesFileName);
string json = JsonSerializer.Serialize(profileDtos, _jsonOptions);
await File.WriteAllTextAsync(profilesPath, json).ConfigureAwait(false);
// ...and a matching hand-written read+catch(JsonException) path
// after
internal sealed class ProfilesData : AppData<ProfilesData>
{
public List<ProfileDto> Profiles { get; set; } = [];
}
ProfilesData data = AppData<ProfilesData>.LoadOrCreate(RelativeDirectoryPath.Create("Keybinding"));
data.Profiles = profileDtos.ToList();
data.Save();Caveats
AppData<T>'s process-wideGet()/InternalStateaccessor caches a single instance per typeTin astatic Lazy<T>guarded by astatic Lock.JsonKeybindingRepositoryis constructed with an arbitrary_dataDirectoryper instance (the test suite creates a fresh temp directory per test), so adopting this would mean calling theLoadOrCreate(subdirectory, fileName)overload directly and holding the returned instance in the repository object yourself, rather than usingGet()— workable, but theLockobject is still shared across everyProfilesDatainstance in the process regardless of directory, which is a minor contention difference from today's per-repository-instance state.AppData<T>.FilePathis namespaced underAppData.Path(%AppData%/<AppDomain.CurrentDomain.FriendlyName>) by default, joined with thesubdirectoryargument — passingRelativeDirectoryPath.Create("Keybinding")reproduces today's%AppData%/Keybindinglayout, but this is worth double-checking against any existing user installs before changing the file layout.- The JSON options differ:
AppDataalways installsRoundTripStringJsonConverterFactoryandReferenceHandler.Preserve(AppData.cs:36-46) on top ofJsonStringEnumConverter, versus the repository's plainCamelCase+ enum-converter options — the DTOs would need checking against those extra converters, though none of the three DTOs currently hold aSemanticStringor reference cycle thatReferenceHandler.Preservewould change the wire format for.
Activity
matt-edmondson commentedon Sep 15, 2026
Triage
Category: Improvement
Priority: Low
Area:
Keybinding— profile/command JSON persistenceLow: nothing broken, consolidation onto
ktsu.AppDataStorage.Suggested assignment: whoever owns
Keybinding's persistence.Related and worth coordinating:
ktsu-dev/Schema#201(same audit rotation) proposes adoptingktsu.Keybinding.CoreinSchema.Editor, where it is already an unusedPackageReference. That makes this repo's persistence shape a thing a new consumer is about to build against. Not a blocker in either direction, but if both are picked up, doing this one first avoidsSchemaadopting a format that then changes underneath it. Tracked underktsu-dev/.github#15.One caution specific to this adoption.
ktsu.AppDataStoragewrites plain, unencrypted JSON under the user's AppData directory. That is entirely appropriate for keybinding profiles — they aren't secrets — so nothing here is a concern. It is worth being explicit about, though, because three issues in this same rotation (ktsu-dev/OAICLI#42,ktsu-dev/BuildMonitor#278,ktsu-dev/ProjectDirector#411) are High-priority precisely becauseAppDataStoragewas used for credentials. The rule of thumb this audit is converging on:AppDataStoragefor preferences,ktsu.CredentialCachefor anything secret. Keybindings are squarely the former.Open PR covering this: none.
Suggested next step: check whether any existing profile JSON needs migrating, or whether
AppDataStorage's serialization can be pointed at the current on-disk format unchanged — a silent format change that loses a user's configured profiles would be a poor trade for a cleanup.Generated by Claude Code
matt-edmondson commentedon Sep 22, 2026
Triage
Not actionable as specified.
AppData<T>cannot be pointed at an arbitrary absolute directory, and this repo's data directory is public API. Verified againstktsu-dev/AppDataStorageat3571aad, not read off the issue.The blocker
AppData<T>'s root is computed, not injected:Both are read-only expression-bodied properties with no setter. All four
LoadOrCreateoverloads (AppData.cs:498,:538,:545,:552) take only aRelativeDirectoryPath, joined beneathAppData.Path. The one injection point in the library isIFileSystem, via aprivate static FileSystemFactory(:51) — a test-isolation hook, not a relocation hook. There is no way to ask for an absolute root.Why that matters here
The data directory is not an internal detail of
JsonKeybindingRepository; it is a documented parameter on four public entry points, all accepting an arbitrary absolute path:KeybindingManager(string dataDirectory)—KeybindingManager.cs:20IKeybindingManagerFactory.CreateManager(string dataDirectory)—Contracts/IKeybindingManagerFactory.cs:21KeybindingConfiguration.DataDirectory—Models/KeybindingConfiguration.cs:33ServiceCollectionExtensions.AddKeybinding(this IServiceCollection, string? dataDirectory = null)—Extensions/ServiceCollectionExtensions.cs:21, andAddKeybindingScopedat:70Adopting
AppData<T>means none of those can be honored. That is a public API break, not the "minor contention difference" the issue's first caveat describes.It also takes the test suite with it:
KeybindingManagerTestsinjectsPath.Combine(Path.GetTempPath(), Guid.NewGuid().ToString())per test (KeybindingManagerTests.cs:16). UnderAppData<T>every instance of a givenTresolves to the same process-wide location regardless of the directory asked for, so per-test isolation is gone and parallel tests would contend on one file.Two silent-data-loss paths, both confirmed
The first triage asked whether any existing profile JSON needs migrating. It does, on both axes:
%AppData%/Keybinding(ServiceCollectionExtensions.cs:25,KeybindingManagerFactory.cs:48).AppData<T>withsubdirectory: "Keybinding"resolves to%AppData%/<AppDomain.CurrentDomain.FriendlyName>/Keybinding— theFriendlyNamesegment is unconditional. Every existing install's profiles are orphaned, and the new path varies by host process name.AppData.JsonSerializerOptionssetsReferenceHandler.Preserve(AppData.cs:40). That wraps collections as{"$id":"1","$values":[…]}rather than a bare array, soprofiles.jsonandcommands.jsonchange shape. The issue's caveat reasoned this was safe because no DTO holds aSemanticStringor a reference cycle — butPreserverewrites plain collections too, and all three files are collection-rooted.IncludeFields = trueis a further difference from the current options.So the
.bkrecovery path the issue is really after would be bought at the cost of losing every current user's profiles on upgrade.What would unblock it
Any one of:
ktsu.AppDataStorageoverload taking anAbsoluteDirectoryPathroot (or makingAppData.Pathinjectable). This is the clean fix and would also unblock the otherAppDataStorageadoptions in this rotation that inject a directory.JsonKeybindingRepositoryas-is and lifting only the atomic write +.bkrecovery —AppData.WriteText/ReadText(AppData.cs:142,:176) are ~70 lines of tmp/backup/swap that could be reused or mirrored without adopting the path model. This gets the actual win the issue names.%AppData%/Keybinding/*.jsonon first load, rewrite at the new location), which is a breaking-change release rather than a cleanup.The second option looks like the best value-to-risk here, but it is a different change from the one this issue proposes, so it wants an owner's call rather than a drive-by.
Leaving this open and unassigned. The direction is still sound; nothing above argues against consolidating, only against doing it as a straight swap today.
Note: the issue's file links point at
Keybinding.Core/…; the project is nowKeybinding/…(Keybinding/Keybinding.csproj, targetingnet8.0;net9.0;net10.0), presumably from the.Coreretirement work. The line references still line up.Generated by Claude Code
matt-edmondson commentedon Sep 28, 2026
Decision (maintainer, 2026-09-28)
ktsu.AppDataStorage, taking option 1: an upstream overload with anAbsoluteDirectoryPathroot. That keeps the publicdataDirectoryentry points and per-test isolation working. It is filed as Add a LoadOrCreate overload that takes an absolute root directory AppDataStorage#316.ReferenceHandler.Preservewire shape and any location change are fine, but on first load the code must read the legacyprofiles.json,commands.jsonandactive-profile.jsonand rewrite them. No user's profiles should be lost on upgrade.Next reader: blocked on AppDataStorage#316. Once it ships, replace
JsonKeybindingRepository's file I/O with threeAppData<T>models loaded from the configureddataDirectory, and add the legacy-format migration with tests. Coordinate with ktsu-dev/Schema#201 so Schema adopts the new format rather than the old one.Generated by Claude Code
matt-edmondson commentedon Nov 3, 2026
Triage
ktsu.AppDataStorageAbsoluteDirectoryPathroot. The upstream change comes first.InitializeAsync) touch the same persistence code. Land or rebase them before migrating.Re-triaged because the thread changed after the original triage.
Generated by Claude Code