space-game KeybindingManager ctor wipes saved localStorage keybindings on every boot (custom binds lost on reload) #826

New Issue

2026-07-24T05:34:10Z

spikerj commented

2026-07-24 05:34:10 +00:00

Summary

KeybindingManager's constructor calls resetToDefaults(), which persists the default keybindings to localStorage (saveToLocalStorage(DEFAULT_KEYBINDINGS) at keybinding-manager.ts:264). Because the service is providedIn: "root" and constructed at app boot, this overwrites any previously-saved custom keybindings with the defaults before loadKeybindings() ever reads them.

Net effect: for a local (not backend-synced) space-game player, custom keybindings do not survive a page reload — they are silently reset to defaults on every startup.

Where

projects/spikersoft/src/app/_components/_games/space-game/keybinding-manager.ts

ctor (L62-65) → resetToDefaults() (L262-272) → saveToLocalStorage(...) (L264) writes localStorage unconditionally.
loadKeybindings() (L78-99) is designed to prefer localStorage → backend → defaults, but the ctor has already clobbered localStorage with defaults, so the localStorage branch always sees defaults.

Repro (unit-level)

localStorage.setItem("spikersoft_keybindings", <custom bindings>)
new KeybindingManager() ← ctor overwrites localStorage with defaults
await mgr.loadKeybindings() → returns { source: "localStorage" } but the values are the DEFAULTS, not the custom bindings.

Expected

The constructor should seed the in-memory map with defaults without persisting them (persist only on an explicit user reset / save), so a saved localStorage set is preserved and picked up by loadKeybindings().

Notes

Found during frontend coverage wave 4 (adding a spec for keybinding-manager.ts). Current behavior is pinned by a characterization test referencing this ticket so the suite stays green and flags the fix.
Suggested fix: split resetToDefaults() into an in-memory applyDefaults() (ctor) vs a persisting resetToDefaults() (user action); or make the ctor only applyKeybindings(defaults) without saveToLocalStorage.

## Summary `KeybindingManager`'s constructor calls `resetToDefaults()`, which **persists the default keybindings to `localStorage`** (`saveToLocalStorage(DEFAULT_KEYBINDINGS)` at `keybinding-manager.ts:264`). Because the service is `providedIn: "root"` and constructed at app boot, this **overwrites any previously-saved custom keybindings with the defaults before `loadKeybindings()` ever reads them**. Net effect: for a local (not backend-synced) space-game player, **custom keybindings do not survive a page reload** — they are silently reset to defaults on every startup. ## Where `projects/spikersoft/src/app/_components/_games/space-game/keybinding-manager.ts` - ctor (L62-65) → `resetToDefaults()` (L262-272) → `saveToLocalStorage(...)` (L264) writes localStorage unconditionally. - `loadKeybindings()` (L78-99) is designed to prefer localStorage → backend → defaults, but the ctor has already clobbered localStorage with defaults, so the localStorage branch always sees defaults. ## Repro (unit-level) 1. `localStorage.setItem("spikersoft_keybindings", <custom bindings>)` 2. `new KeybindingManager()` ← ctor overwrites localStorage with defaults 3. `await mgr.loadKeybindings()` → returns `{ source: "localStorage" }` but the values are the DEFAULTS, not the custom bindings. ## Expected The constructor should seed the in-memory map with defaults **without persisting** them (persist only on an explicit user reset / save), so a saved localStorage set is preserved and picked up by `loadKeybindings()`. ## Notes - Found during frontend coverage wave 4 (adding a spec for `keybinding-manager.ts`). Current behavior is pinned by a characterization test referencing this ticket so the suite stays green and flags the fix. - Suggested fix: split `resetToDefaults()` into an in-memory `applyDefaults()` (ctor) vs a persisting `resetToDefaults()` (user action); or make the ctor only `applyKeybindings(defaults)` without `saveToLocalStorage`.

spikerj commented

2026-07-24 19:27:40 +00:00

Confirmed still present on master, and fixed in spikersoft-angular PR #569 (fix/826-keybinding-ctor-clobber) — awaiting CI/review.

The suggested fix from the ticket is what landed: seedDefaults() applies DEFAULT_KEYBINDINGS to the in-memory map only, the ctor calls that, and resetToDefaults() keeps persisting since it is the explicit user action. The decline-backend path (applyDefaults()) still routes through resetToDefaults(), so an intentional reset persists as before.

The [#826] characterization test is flipped into a regression guard: seed a custom moveForward: "I" into localStorage, construct the manager, and loadKeybindings() now returns { source: "localStorage" } with the custom "i" intact. Also added an assertion that construction alone leaves hasLocalStorageKeybindings() false.

Two things worth recording while I was in here:

The dungeon-crawler KeybindingManager does NOT have this bug — its ctor calls loadFromStorage() rather than a persisting reset, so no fix is needed there. Worth knowing since the two files are near-copies and the next person to read #826 will reasonably wonder.
Blast radius was wider than just reloads. Since the service is providedIn: "root", the clobber happened at app boot for every visitor to the site, not only players who opened the space game — so a player's binds were destroyed even by a visit that never launched the game.

Verified locally: 23 space-game spec files / 194 tests pass.

— Opus 5 Agent

Confirmed still present on master, and fixed in spikersoft-angular **PR #569** (`fix/826-keybinding-ctor-clobber`) — awaiting CI/review. The suggested fix from the ticket is what landed: `seedDefaults()` applies `DEFAULT_KEYBINDINGS` to the in-memory map only, the ctor calls that, and `resetToDefaults()` keeps persisting since it is the explicit user action. The decline-backend path (`applyDefaults()`) still routes through `resetToDefaults()`, so an intentional reset persists as before. The `[#826]` characterization test is flipped into a regression guard: seed a custom `moveForward: "I"` into localStorage, construct the manager, and `loadKeybindings()` now returns `{ source: "localStorage" }` with the custom `"i"` intact. Also added an assertion that construction alone leaves `hasLocalStorageKeybindings()` false. Two things worth recording while I was in here: 1. **The dungeon-crawler `KeybindingManager` does NOT have this bug** — its ctor calls `loadFromStorage()` rather than a persisting reset, so no fix is needed there. Worth knowing since the two files are near-copies and the next person to read #826 will reasonably wonder. 2. **Blast radius was wider than just reloads.** Since the service is `providedIn: "root"`, the clobber happened at app boot for every visitor to the site, not only players who opened the space game — so a player's binds were destroyed even by a visit that never launched the game. Verified locally: 23 space-game spec files / 194 tests pass. — Opus 5 Agent

spikerj commented

2026-07-24 19:58:38 +00:00

Fixed and verified. spikersoft-angular PR #569 merged to master (452b1cc5).

Verified on master: the ctor calls seedDefaults() (in-memory only) and saveToLocalStorage is reached solely through resetToDefaults(), so a saved keybinding set now survives app boot and loadKeybindings() reads it as intended. The full master run after the merge is green (build, e2e-anonymous, e2e-smoke, test-and-lint) and publish shipped it to production.

Closing.

— Opus 5 Agent

Fixed and verified. spikersoft-angular PR #569 merged to master (`452b1cc5`). Verified on master: the ctor calls `seedDefaults()` (in-memory only) and `saveToLocalStorage` is reached solely through `resetToDefaults()`, so a saved keybinding set now survives app boot and `loadKeybindings()` reads it as intended. The full master run after the merge is green (`build`, `e2e-anonymous`, `e2e-smoke`, `test-and-lint`) and `publish` shipped it to production. Closing. — Opus 5 Agent

spikerj closed this issue

2026-07-24 19:58:38 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#826