Skip to content

KeybindingConfiguration's AutoSave, AutoSaveIntervalMilliseconds and DefaultProfileId/Name have no effect: nothing in the library reads IKeybindingConfiguration #145

Description

@matt-edmondson

What's wrong

IKeybindingConfiguration (Keybinding/Contracts/IKeybindingConfiguration.cs) and its implementation KeybindingConfiguration (Keybinding/Models/KeybindingConfiguration.cs) are public API that promise behaviour:

  • AutoSave - "whether to auto-save changes"
  • AutoSaveIntervalMilliseconds - "the auto-save interval in milliseconds (0 to disable)"
  • DefaultProfileId - "the default profile ID to use when no profile is active"
  • DefaultProfileName

No code in the library consumes any of them. grep -rn "AutoSave\|IKeybindingConfiguration\|DefaultProfileId" Keybinding/ finds only the interface and the class itself:

  • KeybindingManager has no constructor or property that takes an IKeybindingConfiguration; it only ever saves when the caller invokes SaveAsync().
  • KeybindingManager.CreateDefaultProfile hard-codes "default" / "Default" as parameter defaults rather than reading the configuration.
  • ServiceCollectionExtensions.AddKeybinding* never registers or resolves IKeybindingConfiguration.
  • The default constructor and the constructor both default AutoSave to true, which suggests saving happens automatically.

Failure scenario

Following USAGE_EXAMPLES.md ("Using Configuration Objects" and "Configuration with Options Pattern"):

var config = new KeybindingConfiguration("./data", autoSave: true, autoSaveIntervalMilliseconds: 3000);
var manager = new KeybindingManager(new CommandRegistry(), new ProfileManager(), new JsonKeybindingRepository(config.DataDirectory));
await manager.InitializeAsync();
manager.CreateDefaultProfile();
manager.Keybindings.BindChord("file.save", Chord.Parse("Ctrl+S"));
// app exits without an explicit SaveAsync()

The caller reasonably expects the binding to have been auto-saved within 3 seconds. Nothing is written to disk, so every change made since start-up is lost on exit. Likewise, a DefaultProfileId of "my-default" is never used: CreateDefaultProfile() still creates "default", and nothing falls back to the configured profile when none is active.

Suggested fix

Pick one, and make the docs match:

  1. Wire it up: give KeybindingManager a constructor (and DI registration) that accepts IKeybindingConfiguration; use DefaultProfileId/DefaultProfileName in CreateDefaultProfile when no arguments are passed; when AutoSave is true and the interval is > 0, save after mutations on a debounced timer (disposed in Dispose), or
  2. Retire it: mark AutoSave, AutoSaveIntervalMilliseconds, DefaultProfileId and DefaultProfileName [Obsolete] (or remove them in a major release) and drop the auto-save examples from USAGE_EXAMPLES.md.

Acceptance criteria

  • Either a configured AutoSave/interval demonstrably persists changes without an explicit SaveAsync() call (covered by a test), and CreateDefaultProfile() honours DefaultProfileId/DefaultProfileName; or those members are obsolete/removed and no documentation implies they do anything.

Activity

  1. matt-edmondson commented on Sep 28, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    • Category: Bug (public API and USAGE_EXAMPLES.md promise behaviour that nothing implements)
    • Priority: Medium. A caller who trusts AutoSave = true (the default) loses every binding change made since start-up on exit, and gets no error. The workaround is easy (call SaveAsync()), and any app that already saves explicitly is unaffected, so it isn't High.
    • Area / suggested assignment: Keybinding/Contracts/IKeybindingConfiguration.cs, Keybinding/Models/KeybindingConfiguration.cs, KeybindingManager (CreateDefaultProfile), ServiceCollectionExtensions, and USAGE_EXAMPLES.md.
    • Duplicates: none found. Related to Delegate profile/command JSON persistence to ktsu.AppDataStorage #93 (delegating persistence to ktsu.AppDataStorage). If that lands, it changes where an auto-save timer would live.
    • In progress: no. The open Keybinding PR Lock a profile's chords so concurrent binds cannot corrupt them [minor] #142 locks profile chords against concurrent binds. That's a different change, but a debounced auto-save timer would also need that lock.

    Suggested next step: choose between wiring it up and retiring it before anyone writes code. Given #93, retiring AutoSave/AutoSaveIntervalMilliseconds with [Obsolete] now and fixing the docs is the cheaper, safer option. DefaultProfileId/DefaultProfileName are simple to honour in CreateDefaultProfile() either way. Wiring up auto-save should wait until #142 merges, so the timer's save runs under the same lock as mutations.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions