WIP#3735
Draft
VariableVince wants to merge 5 commits intomainfrom
Draft
Conversation
VariableVince
added
Refactor
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors keybind handling to use a typed Changes
Sequence DiagramsequenceDiagram
actor User as User
participant IH as InputHandler
participant US as UserSettings
participant Utils as Utils
participant Nav as Navigator.keyboard
User->>IH: press key (maybe Shift)
IH->>US: read keybinds (KeybindAction → bind string)
IH->>IH: parseKeybind(bindString) / keybindMatchesEvent(event)
alt match found
IH->>IH: execute action (game behavior)
else no match
IH-->>User: no action
end
Note over Utils,Nav: key formatting/caching
Utils->>Nav: getLayoutMap() [once]
Nav-->>Utils: Map<string,string>
Utils->>Utils: cache map
IH->>Utils: formatKeyForDisplay(code or "Shift+code")
Utils-->>IH: human-friendly label
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/client/graphics/layers/UnitDisplay.ts (1)
129-219:⚠️ Potential issue | 🟡 MinorUse the shared key formatter for unit hotkeys.
The unit bar still formats hotkeys with
.replace(...).toUpperCase(), so new values likeShift+BracketLeftor layout-mapped keys can render asSHIFT+BRACKETLEFTinstead of the same label shown in settings/help.🐛 Proposed fix
-import { renderNumber, translateText } from "../../Utils"; +import { formatKeyForDisplay, renderNumber, translateText } from "../../Utils"; @@ - const displayHotkey = hotkey - .replace("Digit", "") - .replace("Key", "") - .toUpperCase(); + const displayHotkey = formatKeyForDisplay(hotkey);Based on learnings:
displayKeyvalues are rendered throughformatKeyForDisplay(), which already handles fallback capitalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitDisplay.ts` around lines 129 - 219, The hotkey string in renderUnitItem is being manually transformed with .replace(...).toUpperCase(), which misformats complex keys; replace that logic by using the shared formatter function formatKeyForDisplay(hotkey) (or the existing displayKey helper used elsewhere) to compute displayHotkey so unit hotkeys match the labels shown in settings/help and handle combos like Shift+BracketLeft correctly; update references in renderUnitItem where displayHotkey is computed and used to call formatKeyForDisplay instead of the chained replace/toUpperCase.src/client/components/baseComponents/setting/SettingKeybind.ts (1)
95-106:⚠️ Potential issue | 🟡 MinorFinish reserved-key handling before accepting binds.
Enter and arrow keys can still be saved, but
InputHandleralways uses them for ghost confirm and camera movement. That can create double actions. Please reserve/block them here or model them in a typed reserved-key list; also remove the debug log.I can help generate the reserved-key guard if you want to track it as a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/baseComponents/setting/SettingKeybind.ts` around lines 95 - 106, Add a reserved-key guard in SettingKeybind's keydown handler so Enter and arrow keys (ArrowUp, ArrowDown, ArrowLeft, ArrowRight, "Enter") are treated like Tab/Escape: don't accept them as binds and cancel listening when appropriate (use the existing this.listening flow and requestUpdate()), or reference a typed reserved-key enum in UserSettings/InputHandler if one exists; also remove the debug console.log("Keydown event:", e) line. Ensure the handler prevents saving these reserved keys while still allowing other keys to be bound, and keep compatibility with InputHandler's existing ghost-confirm and camera movement behavior.src/client/InputHandler.ts (1)
222-276:⚠️ Potential issue | 🟠 MajorSupport or block Shift+ binds for hold actions.
Settings now allow
Shift+key, but movement/zoom tracking still compares rawe.codeand Line 226 exits whenever Shift is held. SoShift+Wfor move/zoom-style actions is accepted by the UI but never works. Either block Shift+ for these hold actions in settings, or parse Shift-aware active keys here.Also applies to: 328-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/InputHandler.ts` around lines 222 - 276, The movement/zoom loop (moveInterval) currently returns early whenever ShiftLeft/ShiftRight are down, which prevents Shift+key binds from working; remove that early-return and change the key checks to be Shift-aware by checking both the raw code and a Shift-prefixed variant when Shift is held (e.g. if Shift active treat a bind like "W" as matching either "KeyW" or "Shift+KeyW"); update the comparisons that reference this.activeKeys, this.keybinds.moveUp/moveDown/moveLeft/moveRight and this.keybinds.zoomIn/zoomOut so they test both the base code and the Shift+code (or detect bind strings that include "Shift+"), then emit DragEvent/ZoomEvent as before using PAN_SPEED/ZOOM_SPEED and eventBus.emit.
🧹 Nitpick comments (8)
src/client/Utils.ts (1)
333-335: Remove temporary key-format logging.
formatKeyForDisplay()is called by settings/help rendering, so this test log will spam the console for normal UI use.🧹 Proposed cleanup
export function formatKeyForDisplay(value: string): string { - // TODO remove after testing - console.info("formatKeyForDisplay recieved: " + value); - // Handle empty stringI can help remove the temporary debug path before merge if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Utils.ts` around lines 333 - 335, Remove the temporary debug logging from formatKeyForDisplay: delete the console.info("formatKeyForDisplay recieved: " + value); line (and the "TODO remove after testing" comment) in the formatKeyForDisplay function in Utils.ts so the function no longer spams the console during normal UI rendering; leave the function body to perform its intended formatting and return the formatted string.src/client/vite-env.d.ts (1)
41-44: UseReadonlyMap<string, string>instead of mutableMap.The browser's
KeyboardLayoutMapis read-only and provides:get(),has(),size,forEach(),entries(),keys(),values(), and iterator support. Typing asPromise<Map<string, string>>is wider than the actual API and misleads callers into thinking they can use mutable methods likeset/delete/clear. UseReadonlyMap<string, string>to match the actual API and follow idiomatic TypeScript.♻️ Proposed type correction
interface Navigator { keyboard?: { - getLayoutMap(): Promise<Map<string, string>>; + getLayoutMap(): Promise<ReadonlyMap<string, string>>; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/vite-env.d.ts` around lines 41 - 44, The Navigator.keyboard.getLayoutMap() signature currently returns Promise<Map<string, string>> but the browser API is read-only; change the return type to Promise<ReadonlyMap<string, string>> so callers can't assume mutating methods (set/delete/clear) exist. Update the declaration for the Navigator interface's keyboard property and its getLayoutMap() method to use ReadonlyMap<string, string> (and import/ensure ReadonlyMap is available in the ambient types) so the type matches KeyboardLayoutMap.src/client/UserSettingModal.ts (2)
100-113: Small readability nit:actionis shadowed inside the loop.The outer
actionfrome.detailis shadowed by the loop variablefor (const [action, codeAndKey] of Object.entries(...)). It happens to work because the outeractionis used again only after the loop (on line 112 / 156), but the shadowing makes the code harder to follow. Renaming the loop variable keeps things obvious and avoids future bugs if anyone adds code inside the loop that expects the outeraction.♻️ Suggested rename
const activeKeybinds = { ...this.defaultKeybinds }; - for (const [action, codeAndKey] of Object.entries(this.userKeybinds)) { - const normalizedCode = codeAndKey.value; + for (const [boundAction, codeAndKey] of Object.entries(this.userKeybinds)) { + const normalizedCode = codeAndKey.value; if (normalizedCode === KeyUnbound) { - delete activeKeybinds[action]; + delete activeKeybinds[boundAction]; } else { - activeKeybinds[action] = normalizedCode; + activeKeybinds[boundAction] = normalizedCode; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/UserSettingModal.ts` around lines 100 - 113, The loop variable shadows the outer action (from e.detail), so rename the loop binding in the for-of that iterates Object.entries(this.userKeybinds) (currently "for (const [action, codeAndKey] ...)") to a non-conflicting name like "keybindAction" or "userAction" and update its usages inside the loop (and any related references such as codeAndKey and normalizedCode) so the outer action remains unshadowed; ensure this change is made where activeKeybinds is constructed and where codes is computed so behavior is unchanged.
51-79: Unknown action names from localStorage will silently land inuserKeybinds.
validated[action]usesactionstraight fromObject.entries(parsed)— any leftover/typo action from an older version of the app will be kept as aPartial<Record<KeybindAction, ...>>key. It will never match any render site and will never be cleaned up. Consider filtering unknown actions so the storage stays in sync with the enum:♻️ Suggested filter
for (const [action, entry] of Object.entries(parsed)) { + if (!(action in this.defaultKeybinds)) continue; // drop unknown actions if (typeof entry === "string") { validated[action] = { value: entry, key: entry }; } else if (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/UserSettingModal.ts` around lines 51 - 79, During validation in the loop over Object.entries(parsed) you currently assign validated[action] directly which preserves unknown/typo keys; change the loop so you first verify that action is a known KeybindAction (e.g. check it exists in the KeybindAction enum or in your canonical keybind map like defaultKeybinds) and only then populate validated[action]; discard any entries where the action is not recognized so userKeybinds only contains valid KeybindAction keys.src/core/game/UserSettings.ts (3)
4-10: Typos/clarity in the reserved-keys comment, and theKeyUnboundsentinel could use a docstring.A couple of small cleanups to help non-native readers and future maintainers:
- Line 5: "Shift+left quick" → "Shift+left click".
- Line 6: "kindbinds" → "keybinds".
- Line 7: "alltogether" → "altogether".
KeyUnbound = "Null"is a string that travels throughJSON.stringifyinto localStorage and is then compared with===. That is fine, but the capital-N "Null" is very easy to confuse withnull. A short JSDoc on the constant explaining "sentinel stored in JSON to mark an explicitly unbound action; do NOT usenull" would save the next reader a trip through the call sites.📝 Suggested edits
-// ADD Reserved keys here or in Utils.ts: -// (maybe also comment about Shift+left quick being reserved for future devs, see HelpModal, code in onPointerUp in InputHandler: when leftClickOpensMenu is false, Shift+left click is hardcoded to be attack. So it should not be used elsewhere where modifier+click is expected) -// Shift+D for performance overlay, Alt+R for reset gfx (NO not reserved anymore, can be changed with fallback removed), Escape for menu close and cancel ghost build, Enter for confirm Ghost build -// But also browser combos: Ctrl+Shift+I for dev tools, etc. Shift+Tab for backwards tabbing through fields. Alt+N for new browser screen, etc. If we won't just suppress Alt/Ctrl etc alltogether or if used in combination (but then confusing things could still happen) -// These keys won't be available for binding in UserSettingsModal. +// Reserved keys (add new ones here or in Utils.ts): +// - Shift+left click: reserved for future use. See HelpModal and onPointerUp in InputHandler: +// when leftClickOpensMenu is false, Shift+left click is hardcoded as "attack", so it must +// not be reused for anything else where modifier+click is expected. +// - Shift+D: performance overlay. +// - Alt+R: reset gfx (not strictly reserved anymore; can be changed if the fallback is removed). +// - Escape: close menu / cancel ghost build. +// - Enter: confirm ghost build. +// Browser combos are also effectively reserved: Ctrl+Shift+I (dev tools), Shift+Tab +// (reverse focus), Alt+N (new window), etc. Unless we suppress Alt/Ctrl entirely, binding +// these can still produce confusing behavior. +// These keys are not selectable in UserSettingsModal. -export const KeyUnbound = "Null"; +/** + * Sentinel stored (as a plain string) in the keybinds JSON to mark an action as + * explicitly unbound by the user. Compared with `===` on the `code` string. + * Note: this is the string "Null", not the JavaScript `null` — the value must + * survive `JSON.stringify`/`JSON.parse` round-trips. + */ +export const KeyUnbound = "Null";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 4 - 10, Fix the typos and clarify the reserved-keys comment (change "Shift+left quick" → "Shift+left click", "kindbinds" → "keybinds", "alltogether" → "altogether", and tidy phrasing about browser combos/reservations) and add a short JSDoc above the exported KeyUnbound constant (export const KeyUnbound = "Null") explaining that this is a JSON/string sentinel used to mark an explicitly unbound action (do NOT use null, it is intentionally the string "Null" and is compared with ===), so future readers aren’t confused by the capitalized "Null".
406-421:for...inovermergedKeybinds— small nit.
for (const action in mergedKeybinds)iterates string keys and typescript loses theKeybindActiontyping onaction.Object.entrieskeeps the code idiomatic and the types tighter:♻️ Suggested tweak
- for (const action in mergedKeybinds) { - if (mergedKeybinds[action] === KeyUnbound) { - delete mergedKeybinds[action]; - } - } + for (const [action, code] of Object.entries(mergedKeybinds)) { + if (code === KeyUnbound) { + delete mergedKeybinds[action as KeybindAction]; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 406 - 421, The loop in keybinds(isMac) uses for...in which yields string-typed keys and loses the KeybindAction typing; replace that loop with Object.entries(mergedKeybinds) to iterate [action, key] pairs, type action as KeybindAction (or cast each entry to [KeybindAction, string]), and remove entries where key === KeyUnbound (or build a new Record by filtering entries) so mergedKeybinds retains proper typing and the same unbind behavior; refer to functions getDefaultKeybinds, normalizedUserKeybinds, and the constant KeyUnbound to locate the code to change.
368-404: Return types are a bit optimistic — consider filtering to known actions.
parsedUserKeybinds()claimsPartial<Record<KeybindAction, any>>andnormalizedUserKeybinds()claimsRecord<KeybindAction, string>, but both pass whatever keys happened to be in the stored JSON straight through. An old/typo action from a previous version of the app will be typed asKeybindActionto every caller, which is a small type lie that can hide bugs.Filtering to
KeybindActionmembers once at the boundary keeps the rest of the code honestly typed:♻️ Suggested guard
parsedUserKeybinds(): Partial<Record<KeybindAction, any>> { const raw = this.getString(KEYBINDS_KEY, "{}"); try { const parsed = JSON.parse(raw); if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { - return parsed; + const known = new Set<string>(Object.values(KeybindAction)); + return Object.fromEntries( + Object.entries(parsed).filter(([k]) => known.has(k)), + ) as Partial<Record<KeybindAction, any>>; } } catch (e) { console.warn("Invalid keybinds JSON:", e); } return {}; }This also makes
UserSettingModal.loadKeybindsFromStorage()safe without a duplicate guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 368 - 404, parsedUserKeybinds() and normalizedUserKeybinds() currently accept any JSON keys and pretend they are KeybindAction which can hide bad keys; update them to filter incoming entries to only known KeybindAction members (use whatever canonical source exists for valid actions — e.g., the KeybindAction enum/const array or a helper isValidKeybindAction) before returning. Specifically, in parsedUserKeybinds() validate/filter Object.keys(parsed) to only include keys that pass isValidKeybindAction and return a Partial<Record<KeybindAction, any>> containing only those entries; in normalizedUserKeybinds() operate on that filtered parsed object and build Record<KeybindAction, string> by mapping only valid KeybindAction keys and extracting string codes as you already do. Ensure you reference parsedUserKeybinds and normalizedUserKeybinds and the KeybindAction symbol when adding the guard so callers get correctly typed results.tests/InputHandler.test.ts (1)
524-537: Reaching intoUserSettings.cachefrom the test — fine here, but fragile.
(UserSettings as any).cache.delete(KEYBINDS_KEY)pokes a private static field. It works, but if the cache ever becomes per-instance or changes type, this silently breaks. Consider exposing a tiny test helper (e.g. aclearCache()inUserSettings) or reusingremoveUserKeybinds(false)before thelocalStorage.setItem("not a json")line so the parse-failure path is what the test is really about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InputHandler.test.ts` around lines 524 - 537, The test directly mutates the private static cache via (UserSettings as any).cache.delete(KEYBINDS_KEY) which is fragile; change the test to clear existing keybinds through the public API instead—either call the existing UserSettings.removeUserKeybinds(false) before localStorage.setItem(KEYBINDS_KEY, "not a json") or add a small test-only helper like UserSettings.clearCache() and call that; then run inputHandler.initialize() and assert console.warn was called and (inputHandler as any).keybinds.moveUp remains "KeyW". Ensure you reference KEYBINDS_KEY, UserSettings.removeUserKeybinds (or new clearCache helper), inputHandler.initialize, and keybinds.moveUp in the updated test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/InputHandler.ts`:
- Around line 389-392: The handler for the reset-graphics shortcut currently
hard-codes e.altKey but help text uses the configured emoji/menu modifier;
update the InputHandler so the reset branch uses the same configured-modifier
check as keybindMatchesEvent by calling a shared isMenuModifierPressed that
accepts both KeyboardEvent and PointerEvent (widen its signature to
KeyboardEvent | PointerEvent and update its logic to check the configured
modifier), remove the direct e.altKey check, and replace it with
isMenuModifierPressed(e) when handling this.keybinds.resetGfx (also apply the
same change to the other similar handlers mentioned around the other block).
In `@src/client/UserSettingModal.ts`:
- Around line 90-99: Remove the debug console.info call in UserSettingModal (the
line calling console.info inside the handleKeybindChange flow that logs
"handleKeybindChange recieved value...") so keybind changes no longer leak user
key data to the console; locate the block where let { action, value, key,
prevValue } = e.detail; is declared and delete or disable the console.info call
(or alternatively gate it behind an explicit debug flag) and keep the subsequent
normalization of key (key === "Dead" || key === "Unidentified" ? "" : key).
In `@src/client/Utils.ts`:
- Around line 389-394: The mapping has a typo: the key "Metat" should be "Meta",
so replace the incorrect property name Metat with Meta (keeping the value
Platform.isMac ? "⌘" : "⊞") so the platform-specific symbol is used instead of
falling back to a generic "Meta"; verify consistency with MetaLeft and MetaRight
entries and remove any accidental duplicate keys if present.
---
Outside diff comments:
In `@src/client/components/baseComponents/setting/SettingKeybind.ts`:
- Around line 95-106: Add a reserved-key guard in SettingKeybind's keydown
handler so Enter and arrow keys (ArrowUp, ArrowDown, ArrowLeft, ArrowRight,
"Enter") are treated like Tab/Escape: don't accept them as binds and cancel
listening when appropriate (use the existing this.listening flow and
requestUpdate()), or reference a typed reserved-key enum in
UserSettings/InputHandler if one exists; also remove the debug
console.log("Keydown event:", e) line. Ensure the handler prevents saving these
reserved keys while still allowing other keys to be bound, and keep
compatibility with InputHandler's existing ghost-confirm and camera movement
behavior.
In `@src/client/graphics/layers/UnitDisplay.ts`:
- Around line 129-219: The hotkey string in renderUnitItem is being manually
transformed with .replace(...).toUpperCase(), which misformats complex keys;
replace that logic by using the shared formatter function
formatKeyForDisplay(hotkey) (or the existing displayKey helper used elsewhere)
to compute displayHotkey so unit hotkeys match the labels shown in settings/help
and handle combos like Shift+BracketLeft correctly; update references in
renderUnitItem where displayHotkey is computed and used to call
formatKeyForDisplay instead of the chained replace/toUpperCase.
In `@src/client/InputHandler.ts`:
- Around line 222-276: The movement/zoom loop (moveInterval) currently returns
early whenever ShiftLeft/ShiftRight are down, which prevents Shift+key binds
from working; remove that early-return and change the key checks to be
Shift-aware by checking both the raw code and a Shift-prefixed variant when
Shift is held (e.g. if Shift active treat a bind like "W" as matching either
"KeyW" or "Shift+KeyW"); update the comparisons that reference this.activeKeys,
this.keybinds.moveUp/moveDown/moveLeft/moveRight and
this.keybinds.zoomIn/zoomOut so they test both the base code and the Shift+code
(or detect bind strings that include "Shift+"), then emit DragEvent/ZoomEvent as
before using PAN_SPEED/ZOOM_SPEED and eventBus.emit.
---
Nitpick comments:
In `@src/client/UserSettingModal.ts`:
- Around line 100-113: The loop variable shadows the outer action (from
e.detail), so rename the loop binding in the for-of that iterates
Object.entries(this.userKeybinds) (currently "for (const [action, codeAndKey]
...)") to a non-conflicting name like "keybindAction" or "userAction" and update
its usages inside the loop (and any related references such as codeAndKey and
normalizedCode) so the outer action remains unshadowed; ensure this change is
made where activeKeybinds is constructed and where codes is computed so behavior
is unchanged.
- Around line 51-79: During validation in the loop over Object.entries(parsed)
you currently assign validated[action] directly which preserves unknown/typo
keys; change the loop so you first verify that action is a known KeybindAction
(e.g. check it exists in the KeybindAction enum or in your canonical keybind map
like defaultKeybinds) and only then populate validated[action]; discard any
entries where the action is not recognized so userKeybinds only contains valid
KeybindAction keys.
In `@src/client/Utils.ts`:
- Around line 333-335: Remove the temporary debug logging from
formatKeyForDisplay: delete the console.info("formatKeyForDisplay recieved: " +
value); line (and the "TODO remove after testing" comment) in the
formatKeyForDisplay function in Utils.ts so the function no longer spams the
console during normal UI rendering; leave the function body to perform its
intended formatting and return the formatted string.
In `@src/client/vite-env.d.ts`:
- Around line 41-44: The Navigator.keyboard.getLayoutMap() signature currently
returns Promise<Map<string, string>> but the browser API is read-only; change
the return type to Promise<ReadonlyMap<string, string>> so callers can't assume
mutating methods (set/delete/clear) exist. Update the declaration for the
Navigator interface's keyboard property and its getLayoutMap() method to use
ReadonlyMap<string, string> (and import/ensure ReadonlyMap is available in the
ambient types) so the type matches KeyboardLayoutMap.
In `@src/core/game/UserSettings.ts`:
- Around line 4-10: Fix the typos and clarify the reserved-keys comment (change
"Shift+left quick" → "Shift+left click", "kindbinds" → "keybinds", "alltogether"
→ "altogether", and tidy phrasing about browser combos/reservations) and add a
short JSDoc above the exported KeyUnbound constant (export const KeyUnbound =
"Null") explaining that this is a JSON/string sentinel used to mark an
explicitly unbound action (do NOT use null, it is intentionally the string
"Null" and is compared with ===), so future readers aren’t confused by the
capitalized "Null".
- Around line 406-421: The loop in keybinds(isMac) uses for...in which yields
string-typed keys and loses the KeybindAction typing; replace that loop with
Object.entries(mergedKeybinds) to iterate [action, key] pairs, type action as
KeybindAction (or cast each entry to [KeybindAction, string]), and remove
entries where key === KeyUnbound (or build a new Record by filtering entries) so
mergedKeybinds retains proper typing and the same unbind behavior; refer to
functions getDefaultKeybinds, normalizedUserKeybinds, and the constant
KeyUnbound to locate the code to change.
- Around line 368-404: parsedUserKeybinds() and normalizedUserKeybinds()
currently accept any JSON keys and pretend they are KeybindAction which can hide
bad keys; update them to filter incoming entries to only known KeybindAction
members (use whatever canonical source exists for valid actions — e.g., the
KeybindAction enum/const array or a helper isValidKeybindAction) before
returning. Specifically, in parsedUserKeybinds() validate/filter
Object.keys(parsed) to only include keys that pass isValidKeybindAction and
return a Partial<Record<KeybindAction, any>> containing only those entries; in
normalizedUserKeybinds() operate on that filtered parsed object and build
Record<KeybindAction, string> by mapping only valid KeybindAction keys and
extracting string codes as you already do. Ensure you reference
parsedUserKeybinds and normalizedUserKeybinds and the KeybindAction symbol when
adding the guard so callers get correctly typed results.
In `@tests/InputHandler.test.ts`:
- Around line 524-537: The test directly mutates the private static cache via
(UserSettings as any).cache.delete(KEYBINDS_KEY) which is fragile; change the
test to clear existing keybinds through the public API instead—either call the
existing UserSettings.removeUserKeybinds(false) before
localStorage.setItem(KEYBINDS_KEY, "not a json") or add a small test-only helper
like UserSettings.clearCache() and call that; then run inputHandler.initialize()
and assert console.warn was called and (inputHandler as any).keybinds.moveUp
remains "KeyW". Ensure you reference KEYBINDS_KEY,
UserSettings.removeUserKeybinds (or new clearCache helper),
inputHandler.initialize, and keybinds.moveUp in the updated test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a158aa08-8a5a-4471-a371-bf0d29349e3d
📒 Files selected for processing (11)
resources/lang/en.jsonsrc/client/HelpModal.tssrc/client/InputHandler.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/Utils.tssrc/client/components/baseComponents/setting/SettingKeybind.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/vite-env.d.tssrc/core/game/UserSettings.tstests/InputHandler.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
Author
|
@CodeRabbit pause reviews |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/InputHandler.test.ts (1)
874-876:⚠️ Potential issue | 🔴 CriticalFile does not parse: the outer
describe("InputHandler AutoUpgrade")is never closed.The Shift+ describe closes at line 874, but the outer AutoUpgrade
describeopened on line 45 has no matching});before the new top-leveldescribe("Warship box selection (Shift+drag)")on line 876. That is exactly what Prettier is complaining about ('}' expected.at 1059:1). Add the missing});between the two blocks.🛠️ Proposed fix
expect(uiState.ghostStructure).toBe(UnitType.City); }); }); +}); describe("Warship box selection (Shift+drag)", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InputHandler.test.ts` around lines 874 - 876, The test file is missing the closing of the outer describe("InputHandler AutoUpgrade") block, causing a parse error; add the missing closing sequence (});) immediately before the new top-level describe("Warship box selection (Shift+drag)") declaration so that the InputHandler AutoUpgrade describe is properly terminated and the subsequent describe can start at top level.src/client/UserSettingModal.ts (1)
682-700:⚠️ Potential issue | 🔴 Critical
requestAlliance/breakAllianceentries were missed by the enum migration.Every other
<setting-keybind>now passes aKeybindAction.*value andthis.defaultKeybinds.*, but these two still use raw strings and a hard-codeddefaultKey="KeyK"/"KeyL". Two concrete problems:
breakAllianceis already in the enum, so it should beKeybindAction.breakAlliancewiththis.defaultKeybinds.breakAlliancefor consistency.requestAllianceis referenced bygetDefaultKeybindsinUserSettings.ts([KeybindAction.requestAlliance]: "KeyK") but is not declared in theKeybindActionenum — that file will not compile. Decide whetherrequestAllianceis an action or not, add it to the enum if yes, and switch this modal row to the enum + default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/UserSettingModal.ts` around lines 682 - 700, The two setting-keybind rows use raw strings and hard-coded defaultKey values; update them to use the KeybindAction enum and the defaultKeybinds map: replace action="requestAlliance"/"breakAlliance" and defaultKey="KeyK"/"KeyL" with action={KeybindAction.requestAlliance|breakAlliance} and .value/.display sourced from this.defaultKeybinds (i.e. .value=${this.defaultKeybinds.requestAlliance} / .value=${this.defaultKeybinds.breakAlliance}), and ensure KeybindAction includes requestAlliance (add KeybindAction.requestAlliance to the enum if missing to match getDefaultKeybinds in UserSettings.ts) so the file compiles and the modal is consistent with other keybind entries.
🧹 Nitpick comments (2)
src/client/UserSettingModal.ts (1)
633-636: Small polish on theattackRatioUpdisplay binding.Two tiny nits flagged by ESLint and the type system:
KeybindAction.attackRatioUp as KeybindAction— the cast is redundant;attackRatioUpis already aKeybindAction.|| ""should be?? ""to avoid swallowing legitimately-empty strings (ESLint rule).Also worth asking: why is only
attackRatioUpwrapped informatKeyForDisplay, while every other row relies onthis.getKeyChar(...)? If this is intentional, a quick inline comment would help; if not, the rows are now inconsistent.✨ Suggested change
- .display=${formatKeyForDisplay( - this.getKeyValue(KeybindAction.attackRatioUp as KeybindAction) || "", - )} + .display=${formatKeyForDisplay( + this.getKeyValue(KeybindAction.attackRatioUp) ?? "", + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/UserSettingModal.ts` around lines 633 - 636, The display binding for attackRatioUp should remove the redundant cast and use the nullish coalescing operator; update the call using getKeyValue(KeybindAction.attackRatioUp) (no "as KeybindAction") and replace "|| ''" with "?? ''". Also make the row consistent with other rows: either wrap it with formatKeyForDisplay intentionally and add a brief inline comment next to formatKeyForDisplay to explain why it differs, or switch to using this.getKeyChar(this.getKeyValue(KeybindAction.attackRatioUp) ?? "") like the others (adjust whichever matches the surrounding rows in UserSettingModal.ts).src/core/game/UserSettings.ts (1)
387-427: Return types claim "full" records but the maps can be partial.Both
normalizedUserKeybinds()(line 388) andkeybinds(isMac)(line 412) are typed asRecord<KeybindAction, string>, but:
normalizedUserKeybindsonly surfaces entries the user has customized — every other action is missing.keybinds(isMac)activelydeletes entries whose value equalsKeyUnbound.Downstream consumers (e.g.
HelpModaldoingkeybinds.buildMenuModifier) therefore getundefinedat runtime while the types promisestring. PreferPartial<Record<KeybindAction, string>>so callers must handle the unbound case explicitly.A secondary nit:
setUserKeybinds(value: Record<string, any>)at line 429 throws away the type information the callers already have. Tightening it toPartial<Record<KeybindAction, { value: string; key: string } | string>>(or similar) would keep the API honest with the rest of the refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 387 - 427, Change the declared return types from Record<KeybindAction, string> to Partial<Record<KeybindAction, string>> for normalizedUserKeybinds() and keybinds(isMac) so the signatures reflect that some actions may be missing/unbound; update any related local typings if needed (e.g., the mergedKeybinds variable) so deletes and absent entries are allowed. Also tighten the setUserKeybinds(...) parameter type from Record<string, any> to something like Partial<Record<KeybindAction, { value: string; key: string } | string>> so callers' types are preserved and match the parsing logic used by normalizedUserKeybinds(). Ensure all referenced symbols (normalizedUserKeybinds, keybinds, setUserKeybinds, KeyUnbound) compile after the change and update any call sites that assumed a full Record to handle possibly undefined values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/UserSettings.ts`:
- Around line 12-47: The KeybindAction enum contains a duplicated member
swapDirection causing a redeclaration error and is missing requestAlliance
referenced by getDefaultKeybinds and UserSettingModal.ts; fix by removing the
duplicate swapDirection entry from the KeybindAction enum and add a new member
requestAlliance = "requestAlliance" to the same enum so getDefaultKeybinds
(which uses [KeybindAction.requestAlliance]) and UserSettingModal can reference
the enum member instead of the raw string; ensure no other duplicates were
introduced when editing the enum.
---
Outside diff comments:
In `@src/client/UserSettingModal.ts`:
- Around line 682-700: The two setting-keybind rows use raw strings and
hard-coded defaultKey values; update them to use the KeybindAction enum and the
defaultKeybinds map: replace action="requestAlliance"/"breakAlliance" and
defaultKey="KeyK"/"KeyL" with
action={KeybindAction.requestAlliance|breakAlliance} and .value/.display sourced
from this.defaultKeybinds (i.e. .value=${this.defaultKeybinds.requestAlliance} /
.value=${this.defaultKeybinds.breakAlliance}), and ensure KeybindAction includes
requestAlliance (add KeybindAction.requestAlliance to the enum if missing to
match getDefaultKeybinds in UserSettings.ts) so the file compiles and the modal
is consistent with other keybind entries.
In `@tests/InputHandler.test.ts`:
- Around line 874-876: The test file is missing the closing of the outer
describe("InputHandler AutoUpgrade") block, causing a parse error; add the
missing closing sequence (});) immediately before the new top-level
describe("Warship box selection (Shift+drag)") declaration so that the
InputHandler AutoUpgrade describe is properly terminated and the subsequent
describe can start at top level.
---
Nitpick comments:
In `@src/client/UserSettingModal.ts`:
- Around line 633-636: The display binding for attackRatioUp should remove the
redundant cast and use the nullish coalescing operator; update the call using
getKeyValue(KeybindAction.attackRatioUp) (no "as KeybindAction") and replace "||
''" with "?? ''". Also make the row consistent with other rows: either wrap it
with formatKeyForDisplay intentionally and add a brief inline comment next to
formatKeyForDisplay to explain why it differs, or switch to using
this.getKeyChar(this.getKeyValue(KeybindAction.attackRatioUp) ?? "") like the
others (adjust whichever matches the surrounding rows in UserSettingModal.ts).
In `@src/core/game/UserSettings.ts`:
- Around line 387-427: Change the declared return types from
Record<KeybindAction, string> to Partial<Record<KeybindAction, string>> for
normalizedUserKeybinds() and keybinds(isMac) so the signatures reflect that some
actions may be missing/unbound; update any related local typings if needed
(e.g., the mergedKeybinds variable) so deletes and absent entries are allowed.
Also tighten the setUserKeybinds(...) parameter type from Record<string, any> to
something like Partial<Record<KeybindAction, { value: string; key: string } |
string>> so callers' types are preserved and match the parsing logic used by
normalizedUserKeybinds(). Ensure all referenced symbols (normalizedUserKeybinds,
keybinds, setUserKeybinds, KeyUnbound) compile after the change and update any
call sites that assumed a full Record to handle possibly undefined values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 762ddfbc-f358-4248-aab9-f649855316fe
📒 Files selected for processing (8)
resources/lang/en.jsonsrc/client/HelpModal.tssrc/client/InputHandler.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/core/game/UserSettings.tstests/InputHandler.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/client/Main.ts
- resources/lang/en.json
- src/client/InputHandler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/UnitDisplay.ts
Contributor
|
🧠 Learnings used✅ Actions performedReviews paused. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
WIP DRAFT
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME