镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

SaveAsync without InitializeAsync wipes stored commands and the active profile, leaving the preserved profiles' bindings dead #150

Closed

Description

@matt-edmondson

What's wrong

The #124 fix (PR #139) made KeybindingManager.SaveAsync (Keybinding/KeybindingManager.cs:113-146) keep stored profiles this manager never loaded, and SaveWithoutInitializeTests.SaveAsync_WithoutInitialize_KeepsStoredProfiles pins that. The other two files SaveAsync writes are still overwritten wholesale, though:

  • L118-119: Repository.SaveCommandsAsync(Commands.GetAllCommands()) replaces commands.json with only the commands in memory.
  • L144-145: Repository.SaveActiveProfileAsync(activeProfile?.Id) replaces active-profile.json, writing null when nothing is active.

JsonKeybindingRepository writes both files outright, with no merge.

Failure scenario (reproduced with a throwaway MSTest)

  1. Manager A calls InitializeAsync, registers file.save, calls CreateDefaultProfile(), binds Ctrl+S → file.save, then calls SaveAsync.
  2. Manager B uses the same directory and never calls InitializeAsync. It calls Profiles.CreateProfile("other", "Other"), then SaveAsync.
  3. Manager C calls InitializeAsync.

Result: the profiles are other, default, there are 0 commands, and active is null. ExecuteChord("default", Ctrl+S) returns null.

The profile and its binding survive, as #124 intended, but the command the binding points to is gone. ExecuteChord skips unregistered commands, so the binding is dead, and the user's active profile is lost too. Keeping the profiles was the point of #124, and this makes that only half-effective.

Suggested fix

  • Handle commands the way profiles are handled now. Track the command ids this manager loaded or saved. On save, merge with the stored commands.json, and remove only tracked ids that have since been unregistered.
  • Write active-profile.json only if this manager loaded it or changed the active profile. Otherwise leave it alone.

Acceptance criteria

  • The scenario above ends with file.save still registered, default still active, and Ctrl+S resolving to file.save.
  • A command unregistered by a manager that did load it is still removed on save.
  • Tests extend SaveWithoutInitializeTests to cover both.

Related but distinct: #148 (concurrent saves) and #93 (atomic writes).

Activity

  1. matt-edmondson commented on Sep 29, 2026

    @matt-edmondson
    ContributorAuthor

    Triage


    Generated by Claude Code

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

Metadata

Metadata

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