fix: Improve unbound keybind handling #2840
Conversation
- Update InputHandler to preserve 'Null' values for unbound keys instead of filtering them out. - Update SettingKeybind component to explicitly use 'Null' for unbound states and display appropriate placeholder. - Update tests to reflect the change in unbound keybind behavior.
|
antigrid seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis PR preserves the Changes
Sequence Diagram(s)(omitted — changes are focused on parsing/UI state and tests, not a new multi-component control flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/components/baseComponents/setting/SettingKeybind.ts (1)
62-69: Consider simplifying the displayKey fallback logic.Line 68 returns
"Press a key"whenkey.lengthis 0, but this branch appears unreachable since line 63 already handles empty strings by returning the "none" translation. The fallback might never execute.♻️ Simplified displayKey implementation
private displayKey(key: string): string { if (key === "Null" || key === "") return translateText("common.none"); if (key === " ") return "Space"; if (key.startsWith("Key") && key.length === 4) { return key.slice(3); } - return key.length ? key.charAt(0).toUpperCase() + key.slice(1) : "Press a key"; + return key.charAt(0).toUpperCase() + key.slice(1); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/InputHandler.tssrc/client/components/baseComponents/setting/SettingKeybind.tstests/InputHandler.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-09T06:10:44.499Z
Learnt from: plazmaezio
Repo: openfrontio/OpenFrontIO PR: 2716
File: tests/AllianceAcceptNukes.test.ts:85-87
Timestamp: 2026-01-09T06:10:44.499Z
Learning: In tests under tests/**, objects may stack on the same tile coordinates (e.g., MissileSilo). When writing tests that involve building/placing structures or cooldowns, assume multiple items can share coordinates; adjust assertions to account for stacking, and explicitly manage or reset the stack state where necessary.
Applied to files:
tests/InputHandler.test.ts
📚 Learning: 2025-08-28T15:14:40.664Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1956
File: src/client/InputHandler.ts:430-449
Timestamp: 2025-08-28T15:14:40.664Z
Learning: On trackpads and touchscreens, users don't expect purely vertical two-finger swipes to trigger zoom. Pan detection using deltaX !== 0 is appropriate since any horizontal movement indicates pan intent, and zoom should be handled via pinch gestures instead.
Applied to files:
src/client/InputHandler.ts
🧬 Code graph analysis (1)
src/client/components/baseComponents/setting/SettingKeybind.ts (1)
src/client/LangSelector.ts (1)
translateText(224-244)
🔇 Additional comments (6)
tests/InputHandler.test.ts (2)
419-419: LGTM - formatting cleanup.The single-line formatting is clearer.
426-438: LGTM - test correctly validates "Null" preservation.The updated test name and expectations align with the new behavior where "Null" (as a string) indicates an unbound keybind and should be preserved rather than filtered out.
src/client/InputHandler.ts (2)
172-184: Critical fix: "Null" preservation enables persistent unbound keybinds.The removal of the filter condition that excluded
"Null"values is the core change that allows unbound keybinds to persist across page refreshes. Previously, "Null" would be filtered out and the default would be restored.One consideration: when a keybind is set to
"Null"and the code checksthis.activeKeys.has(this.keybinds.moveLeft), it will check if the literal string"Null"is pressed, which will never be true. This is correct behavior—unbound keys shouldn't trigger any actions.
246-268: LGTM - formatting improvements with no functional changes.Single-line conditionals improve readability without altering logic.
src/client/components/baseComponents/setting/SettingKeybind.ts (2)
33-41: LGTM - unbound state visualization works correctly.The changes properly handle the unbound state:
- Adds visual "unbound" class when value is "Null"
- Displays appropriate text for unbound keys
The ternary on line 40 ensures "Null" is passed to
displayKey()when the keybind is unbound, which then renders as "None" (per line 63).
107-117: Verify the necessity of includingkey: "Null"in the event detail.When unbinding, the event detail includes both
value: "Null"andkey: "Null". ThehandleKeydownmethod (line 86) includeskey: e.keybecause it captures the actual keyboard event, but for unbind, the additionalkeyfield seems redundant since no physical key was pressed.Could you clarify the purpose of including
key: "Null"in the unbind event detail (line 111)? If it's not consumed by event listeners, consider removing it for consistency:private unbindKey() { this.value = "Null"; this.dispatchEvent( new CustomEvent("change", { - detail: { action: this.action, value: "Null", key: "Null" }, + detail: { action: this.action, value: "Null" }, bubbles: true, composed: true, }), ); this.requestUpdate(); }If the
keyfield is required by event handlers, this comment can be disregarded.
|
This is fixed in my PR, it will cause issues if this gets merged |
Refined displayKey to only return "None" for explicitly "Null" values. Empty strings will now correctly fall through to the default "Press a key" placeholder, improving the UX during key assignment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/components/baseComponents/setting/SettingKeybind.ts (1)
106-115: Event detail structure is inconsistent across methods.The
unbindKeyandhandleKeydownmethods include akeyproperty in their event details, butresetToDefaultdoes not. This inconsistency may require event handlers to handle both cases.🔧 Proposed fix for consistency
private resetToDefault() { this.value = this.defaultKey; this.dispatchEvent( new CustomEvent("change", { - detail: { action: this.action, value: this.defaultKey }, + detail: { action: this.action, value: this.defaultKey, key: this.defaultKey }, bubbles: true, composed: true, }), ); }Also applies to: 117-127
🧹 Nitpick comments (2)
src/client/components/baseComponents/setting/SettingKeybind.ts (2)
44-48: Consider simplifying the display logic.The
displayKeyfunction already handles "Null" on line 71, so you can simplify this to:♻️ Proposed simplification
- ${this.displayKey( - this.value === "Null" - ? "Null" - : this.value || this.defaultKey, - )} + ${this.displayKey(this.value || this.defaultKey)}
6-12: Consider using a typed union for better type safety.The component uses the string
"Null"as a sentinel value for unbound keybinds. While this works, a union type would make the intent clearer and provide better type safety.♻️ Proposed type safety improvement
+type KeybindValue = string | "Null"; + @customElement("setting-keybind") export class SettingKeybind extends LitElement { @property() label = "Setting"; @property() description = ""; @property({ type: String, reflect: true }) action = ""; - @property({ type: String }) defaultKey = ""; - @property({ type: String }) value = ""; + @property({ type: String }) defaultKey: string = ""; + @property({ type: String }) value: KeybindValue = ""; @property({ type: Boolean }) easter = false;This makes it clear that
"Null"is a valid and expected value.Also applies to: 70-79, 117-127
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/components/baseComponents/setting/SettingKeybind.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/components/baseComponents/setting/SettingKeybind.ts (1)
src/client/LangSelector.ts (1)
translateText(224-244)
🔇 Additional comments (3)
src/client/components/baseComponents/setting/SettingKeybind.ts (3)
37-39: LGTM!The conditional "unbound" class provides a clear visual indicator for unbound keybinds.
71-71: The translation key"common.none"exists inresources/lang/en.jsonand is properly defined with the value"None". The code at line 71 is correct.
86-104: Conflict detection is implemented in the parent component.UserSettingModal's
handleKeybindChangemethod (lines 215–227) validates keybinds before saving. It checks if the new key is already assigned to another action, shows a popup warning, and reverts the value on conflict. Only "Null" (unbind) is allowed on multiple actions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/InputHandler.ts (1)
169-189: Logic change looks correct; consider typed union for type safety.The removal of the
v !== 'Null'filter correctly preserves unbound keybinds. The parsing logic handles both object and string formats well.However, using the string
"Null"as a sentinel value is less type-safe than using an actual typed union. Consider refactoring to useRecord<string, string | null>and actualnullvalues instead of the string"Null". This would:
- Make the unbound state explicit in the type system
- Prevent accidental string comparisons
- Follow TypeScript best practices for typed unions (as per coding guidelines)
♻️ Example refactor using typed union
- private keybinds: Record<string, string> = {}; + private keybinds: Record<string, string | null> = {};Then update the filter to convert string "Null" to actual
null:saved = Object.fromEntries( Object.entries(parsed) .map(([k, v]) => { if (v && typeof v === "object" && "value" in (v as any)) { - return [k, (v as any).value as string]; + const value = (v as any).value; + return [k, value === "Null" ? null : value]; } - if (typeof v === "string") return [k, v]; + if (typeof v === "string") return [k, v === "Null" ? null : v]; return [k, undefined]; }) - .filter(([, v]) => typeof v === "string"), + .filter(([, v]) => typeof v === "string" || v === null), - ) as Record<string, string>; + ) as Record<string, string | null>;Runtime checks would need updating to handle
null:if (this.keybinds.moveUp && this.activeKeys.has(this.keybinds.moveUp)) { // ... }Note: This would require coordinating changes with
SettingKeybind.tsto use actualnullinstead of string"Null".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/InputHandler.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/InputHandler.test.ts
🔇 Additional comments (1)
src/client/InputHandler.ts (1)
169-189: No evidence of a PR conflict with ryanbarlow97 exists in the codebase or git history. The git log shows no related work from this author on InputHandler.ts, and no code comments or repository references mention this conflict. Since this claim cannot be substantiated through accessible evidence, it should be disregarded.Likely an incorrect or invalid review comment.
|
@ryanbarlow97 could you link the PR you’re referring to? The changes here are minimal, so any conflicts should be straightforward to resolve if they come up. |
|
I fixed it already in this |
|
It’s hard to argue this is a problem when this PR is
Never mind — your PR was merged. But there are a few follow-ups to address; I’ll reach out on Discord.
You linked to the wrong change: reset logic fix commit is unrelated. If you have concerns or suggestions regarding my implementation, please comment directly on the relevant lines. The changes are minimal. |
4 participants
Resolves #2652 #2291
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
webdev.js