fix: pass prevValue in resetToDefault so conflict rejection correctly reverts keybind UI

[berkelmali]

Fixes #4285

What changed and why

resetToDefault() in SettingKeybind.ts dispatched a change event without
prevValue or key. The parent handler in UserSettingModal.ts uses:

element.value = prevValue ?? this.defaultKeybinds[action] ?? "";

With prevValue missing, the fallback was always defaultKey  exact key the reset was trying to assign. So even when the conflict check correctly rejected the reset and returned early, the UI element was set to defaultKey anyway, making it appear as if the reset succeeded. The actual binding in localStorage was untouched, silently diverging from what the UI showed.

A second consequence: once the element shows defaultKey, canReset becomes false (this.value !== this.defaultKey → false), permanently disabling the Reset button. The user has no way to recover without manually reassigning the key.

Changes
src/client/components/baseComponents/setting/SettingKeybind.ts
resetToDefault() now saves prevValue before updating this.value and forwards both prevValue and key in the dispatched event/matching the contract that handleKeydown already follows. If the parent rejects the reset, element.value is correctly restored to the action's actual previous binding.

src/client/UserSettingModal.ts
Two supporting improvements in handleKeybindChange:
1.Conflict check: skip the action's own current user override when building activeKeybinds. The values filter already excludes the action from the conflict check, but excluding it from the override loop too makes the intent explicit and avoids any future confusion.
2.Clean save: if the accepted value equals the action's default, remove the override entry from userKeybinds entirely instead of writing a redundant { value: "Digit8", key: "Digit8" }. The keybinds() merge in UserSettings already applies defaults; storing an identical override just adds noise to localStorage.
Testing
All 1517 existing tests pass (npx vitest run).
No unit tests exist for UserSettingModal or SettingKeybind. The fix was verified manually by reproducing the exact steps from #4285.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b9dacdd4-3abf-4900-bf6c-a0a17b621fd9

📥 Commits

Reviewing files that changed from the base of the PR and between f7ce58a and 97b9bda.

📒 Files selected for processing (2)

• src/client/UserSettingModal.ts
• src/client/components/baseComponents/setting/SettingKeybind.ts

---

Walkthrough

Two keybind bugs are fixed. SettingKeybind.resetToDefault() now captures the prior value and adds key and prevValue to the emitted change event detail. UserSettingModal.handleKeybindChange now excludes the action being updated when building the conflict-check map and removes override entries when the new value matches the default.

Changes

Keybind reset and conflict detection fix

Layer / File(s)	Summary
resetToDefault event payload expansion src/client/components/baseComponents/setting/SettingKeybind.ts	resetToDefault captures prevValue before resetting and emits key and prevValue in the change event detail so the parent handler can restore the correct UI state when a reset is rejected due to a conflict.
Conflict detection exclusion and override cleanup src/client/UserSettingModal.ts	handleKeybindChange now builds activeKeybinds by skipping the action currently being updated (so its own previous override does not trigger a false conflict), and removes the override entry from userKeybinds when the new value matches the default instead of writing a redundant override.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• openfrontio/OpenFrontIO#4175: Modifies UserSettingModal keybind handling for build/emoji menu modifier actions, directly overlapping with the handleKeybindChange logic changed in this PR.

Suggested reviewers

• evanpelle

Poem

🎹 Reset was a trap, a silent betrayal,
The UI said "done" but the store told a tale.
Now prevValue rides along in the event,
And defaults clean up — no stale overrides meant.
Conflicts caught clean, the keybinds align! ✅

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: passing prevValue in resetToDefault to correct UI reversion on conflict rejection.
Description check	✅ Passed	The description thoroughly explains the bug, root cause, changes made to both files, and testing approach—all directly related to the changeset.
Linked Issues check	✅ Passed	Changes directly address all coding requirements from #4285: capturing prevValue in resetToDefault, including it in event dispatch, and improving conflict detection logic.
Out of Scope Changes check	✅ Passed	All changes in both files focus on fixing the keybind reset conflict detection bug described in #4285 with no unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed due to a network error.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}