fix(client): show keybind labels using the user's actual keyboard layout#3993
Open
vansszh wants to merge 3 commits into
Open
fix(client): show keybind labels using the user's actual keyboard layout#3993vansszh wants to merge 3 commits into
vansszh wants to merge 3 commits into
Conversation
On AZERTY, Dvorak, Russian and other non-QWERTY layouts the game currently shows the QWERTY letter for default keybinds even though detection already uses `e.code` (physical position). For a French user the moveUp keybind correctly fires on the top-left letter key but the settings UI, help modal, and build-menu HUD all label it "W" instead of "Z" (the character actually printed on that key). Wire all keybind displays through the new Keyboard Layout Map API: - New utility `src/client/utilities/KeyboardLayout.ts` — wraps `navigator.keyboard.getLayoutMap()`, caches the result, listens for the `layoutchange` event for OS-level layout switches, and exposes a small synchronous reader plus a subscribe helper. - `formatKeyForDisplay` in `Utils.ts` now consults the layout map first and falls through to the existing QWERTY-style code stripping when the API is unavailable (Firefox, Safari) or while the map is still loading. - `HelpModal.getKeyLabel` follows the same pattern, preserving the existing UI-specific labels for special keys (Shift, arrows, etc.) and only consulting the layout map for letter and digit codes. - `UnitDisplay` (build-menu HUD) gets a `hotkeyLabel` helper that prefers the user's saved character when they have rebound the action and otherwise translates the default code through the current layout. Subscribes to `layoutchange` so the HUD re-renders without a reload. - `SettingKeybind` subscribes to layout changes too, so the in-settings keybind labels update in lockstep. - `Main.ts` preloads the layout map at bootstrap so the first paint is already layout-aware in supported browsers. Tests cover the new utility (load, dedupe, error handling, subscribe/unsubscribe, layoutchange invalidation), the layout-aware branch of `formatKeyForDisplay`, and `HelpModal.getKeyLabel`. 33 new tests, full suite at 1279 + 65 = 1344 passing. Resolves openfrontio#1071
|
|
Contributor
WalkthroughAdds keyboard layout awareness for displayed key labels: a new KeyboardLayout utility loads/caches the browser Layout Map, components subscribe to layout changes and re-render, formatters prefer mapped characters, bootstrap preloads the map, and tests cover loader, subscriptions, and formatting. ChangesKeyboard Layout Support for Display Labels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/utilities/KeyboardLayout.ts`:
- Around line 64-68: onLayoutChange currently clears cachedLayout and
pendingLoad and notifies subscribers but does not start a new load, which can
leave getKeyForCode returning null; modify onLayoutChange to immediately kick
off a new load by calling the layout-loading function (e.g., loadLayout or
whatever async loader is present) after clearing cachedLayout/pendingLoad,
assign its Promise to pendingLoad and handle errors (log or set cachedLayout to
a safe value), then call notifySubscribers so subscribers get the refreshed map;
reference: onLayoutChange, cachedLayout, pendingLoad, getKeyForCode,
notifySubscribers.
🪄 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: 8dcaa47a-520f-4517-a346-e1bb2305b3ae
📒 Files selected for processing (9)
src/client/HelpModal.tssrc/client/Main.tssrc/client/Utils.tssrc/client/components/baseComponents/setting/SettingKeybind.tssrc/client/hud/layers/UnitDisplay.tssrc/client/utilities/KeyboardLayout.tstests/client/HelpModal.getKeyLabel.test.tstests/client/KeyboardLayout.test.tstests/client/formatKeyForDisplay.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Per CodeRabbit feedback on openfrontio#3993 (and on reflection it's correct): `onLayoutChange` cleared the cache and notified subscribers but did not kick off a fresh load, so `getKeyForCode` would return null indefinitely after an OS layout switch — components would re-render with the QWERTY fallback and stay there until something else triggered a load. Now `onLayoutChange` does both: it notifies subscribers immediately (forcing a re-render with the fallback, so labels visibly update rather than holding stale chars from the old layout), then calls `loadKeyboardLayout` which fetches the new map and notifies subscribers again from its `.finally` hook. End result: components flicker through fallback for one paint, then settle on the new layout's characters. Tests: replaced the cache-invalidation test (which relied on a manual follow-up `loadKeyboardLayout` call) with one assertion for the synchronous fallback state and a new test that awaits the auto-triggered reload via a subscriber callback and verifies both the post-reload map contents and the call count on the underlying API.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/client/KeyboardLayout.test.ts (1)
177-208: ⚡ Quick winAdd one overlap test for
layoutchangeduring an in-flight initial loadThis suite covers post-load behavior well, but it does not cover the overlap case where
layoutchangefires before the firstgetLayoutMap()resolves. A deferred-first-call test would lock in correct behavior and catch stale-write races.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/client/KeyboardLayout.test.ts` around lines 177 - 208, Add a test that simulates a layoutchange firing while the initial loadKeyboardLayout() is still pending to ensure no stale-write race: use installFakeKeyboard to return a deferred Promise from fake.getLayoutMap() for the first call, call loadKeyboardLayout() (but do not resolve yet), then emitLayoutChange() to trigger the "cache cleared" notification and assert subscribers see the cleared state (getKeyForCode returns null), then resolve the deferred first getLayoutMap, wait for the subsequent notification and verify the final layout is the resolved value; also assert fake.getLayoutMap was called the expected number of times and subscribeToLayoutChange is used to observe the two notifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/utilities/KeyboardLayout.ts`:
- Around line 64-73: The current onLayoutChange -> loadKeyboardLayout flow can
allow an earlier performLoad() to overwrite cachedLayout/pendingLoad after a
newer load started; add a generation guard: introduce a
monotonically-incremented loadGeneration (module-scope), increment it in
onLayoutChange before calling loadKeyboardLayout(), capture the current
generation inside performLoad()/the promise chain (e.g., localGen =
loadGeneration) and only commit results to cachedLayout and clear pendingLoad
when localGen === loadGeneration; ensure loadKeyboardLayout, performLoad,
cachedLayout and pendingLoad are referenced so the check prevents stale writes
from older loads.
---
Nitpick comments:
In `@tests/client/KeyboardLayout.test.ts`:
- Around line 177-208: Add a test that simulates a layoutchange firing while the
initial loadKeyboardLayout() is still pending to ensure no stale-write race: use
installFakeKeyboard to return a deferred Promise from fake.getLayoutMap() for
the first call, call loadKeyboardLayout() (but do not resolve yet), then
emitLayoutChange() to trigger the "cache cleared" notification and assert
subscribers see the cleared state (getKeyForCode returns null), then resolve the
deferred first getLayoutMap, wait for the subsequent notification and verify the
final layout is the resolved value; also assert fake.getLayoutMap was called the
expected number of times and subscribeToLayoutChange is used to observe the two
notifications.
🪄 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: ac9eabd2-8bf9-45e2-9995-51efb1a2fab7
📒 Files selected for processing (2)
src/client/utilities/KeyboardLayout.tstests/client/KeyboardLayout.test.ts
Per CodeRabbit's follow-up on openfrontio#3993: the previous version started a new `loadKeyboardLayout` in `onLayoutChange` but did nothing to prevent an earlier in-flight `performLoad` from later resolving and writing its stale map back into `cachedLayout`, briefly corrupting the cache before the newer load eventually overwrote it. Add a module-scope `loadGeneration` counter that bumps on every `layoutchange`. `performLoad` captures the generation at start and skips committing its result (or its error-clear) if the counter has advanced while it was awaiting `getLayoutMap`. Net effect: only the newest load can ever publish to `cachedLayout`. Tests: new case stages two deferred `getLayoutMap` calls, kicks off load openfrontio#1, fires `layoutchange` (which starts load openfrontio#2), resolves load openfrontio#1 with a marker "stale" map, and asserts the cache rejects it. After load openfrontio#2 resolves, the cache holds the correct fresh map. Sanity-checked that removing the guard makes this test fail with `expected 'stale' not to be 'stale'`.
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
2 participants
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.
Resolves #1071
Description:
On AZERTY, Dvorak, Russian and other non-QWERTY layouts the game currently shows the QWERTY letter for default keybinds even though detection already uses
e.code(physical position). A French user'smoveUpkeybind correctly fires on the top-left letter key, but the settings UI, help modal, and build-menu HUDall label it
Winstead ofZ(the character actually printed on that key).This PR wires every keybind display through the Keyboard Layout Map API so labels match the
user's physical keyboard. On browsers without the API (Firefox, Safari) the existing QWERTY-style fallback runs, so behavior is unchanged for them.
What changed
src/client/utilities/KeyboardLayout.tswrapsnavigator.keyboard.getLayoutMap(), caches the result, listens for thelayoutchangeevent forOS-level layout switches, and exposes
loadKeyboardLayout(),getKeyForCode(code), andsubscribeToLayoutChange(cb).formatKeyForDisplayinUtils.tsconsults the layout map first and falls through to the existing code-stripping when the API is unavailable or the map isstill loading.
HelpModal.getKeyLabeluses the layout map for letter and digit codes; the existing special-label table (Shift, arrows, Return, etc.) takes precedence.UnitDisplay(build-menu HUD) gets a smallhotkeyLabelhelper that prefers the user's saved character when they have rebound the action and otherwisetranslates the default code through the current layout.
SettingKeybindsubscribes tolayoutchangeso the in-settings keybind labels stay in sync if the user switches layouts mid-session.Main.tspreloads the layout map at bootstrap so the first paint is already layout-aware in supported browsers.What I tested
npm test- full suite at 1281 + 65 = 1346 passing, including 35 new tests for the layout-aware behavior and the overlap race.npm run build-prod- succeeds (tsc + vite).npx eslint- clean.npx prettier --checkon the touched files - clean.Compatibility notes
navigator.keyboardis undefined;getKeyForCodereturnsnulland every caller falls through to the existing QWERTY-style codestripping. Zero behavior change for those users.
layoutchangeevent has been removed from some Chromium versions for fingerprinting reasons; we listen defensively (try/catch) and degrade gracefully.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
@vansszh