Nordic keyboard fix - #27
Swaggermuffin64 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughVimRaceEditor now supports international keyboard layouts by adding a ChangesInternational Keyboard Telemetry Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
frontend/src/components/VimRaceEditor.tsx (1)
413-421: 💤 Low valueMinor:
dtMssource differs between the two telemetry paths.
handleEscapeKeyusese.timeStamp(the actual keydown time), whilehandleBeforeInputusesperformance.now()(the time the input event is processed). Since you already snapshottimeStampintotelemetryLastModsRef, using it here would keep dt semantics consistent and avoid a small skew on slow frames.♻️ Use the captured event timestamp
- dtMs: Math.max(0, Math.floor(performance.now())), + dtMs: Math.max(0, Math.floor(telemetryLastModsRef.current.timeStamp)),🤖 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 `@frontend/src/components/VimRaceEditor.tsx` around lines 413 - 421, The dtMs value in the onKeyStrokeRef invocation should use the captured event timestamp from telemetryLastModsRef to match the semantics used in handleEscapeKey and avoid skew; update the dtMs calculation in the onKeyStrokeRef.current call to derive from telemetryLastModsRef.current.timeStamp (preserving the Math.max(0, Math.floor(...)) wrapper) instead of performance.now(), keeping references to onKeyStrokeRef, handleBeforeInput, handleEscapeKey, and telemetryLastModsRef consistent.
🤖 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 `@frontend/src/components/VimRaceEditor.tsx`:
- Line 411: The telemetry suppression ref can persist and drop later genuine
keypresses; update the logic that sets telemetrySuppressNextKeyRef.current in
handleBeforeInput to start a short timer (≈220ms) that clears
telemetrySuppressNextKeyRef.current when it fires, also clear that timer on
component unmount, and change the keydown handler (the function that currently
compares e.key to telemetrySuppressNextKeyRef.current) to clear
telemetrySuppressNextKeyRef.current on any keydown regardless of key match;
ensure you cancel/clear the timer whenever the ref is cleared to avoid leaks.
- Around line 398-422: The handleBeforeInput handler currently treats both
'insertText' and 'insertCompositionText' the same, causing repeated IME
composition strings to be recorded; update the conditional in handleBeforeInput
to either drop 'insertCompositionText' entirely or, preferably, only accept it
when e.isComposing is false and data.length === 1 (i.e. treat insertText as
before but guard insertCompositionText), keeping the existing uses of
telemetryPendingIntlRef, telemetrySuppressNextKeyRef and onKeyStrokeRef
unchanged.
---
Nitpick comments:
In `@frontend/src/components/VimRaceEditor.tsx`:
- Around line 413-421: The dtMs value in the onKeyStrokeRef invocation should
use the captured event timestamp from telemetryLastModsRef to match the
semantics used in handleEscapeKey and avoid skew; update the dtMs calculation in
the onKeyStrokeRef.current call to derive from
telemetryLastModsRef.current.timeStamp (preserving the Math.max(0,
Math.floor(...)) wrapper) instead of performance.now(), keeping references to
onKeyStrokeRef, handleBeforeInput, handleEscapeKey, and telemetryLastModsRef
consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7eed10b-f9e2-45a3-8724-be608b83bc9e
📒 Files selected for processing (3)
frontend/src/components/VimRaceEditor.tsxfrontend/src/utils/keyFormatting.test.tsfrontend/src/utils/keyFormatting.ts
| const handleBeforeInput = (e: InputEvent) => { | ||
| if (!telemetryPendingIntlRef.current) return; | ||
| if ( | ||
| e.inputType !== 'insertText' && | ||
| e.inputType !== 'insertCompositionText' | ||
| ) { | ||
| return; | ||
| } | ||
| const data = e.data; | ||
| if (!data) return; | ||
|
|
||
| telemetryPendingIntlRef.current = false; | ||
| clearTelemetryIntlTimer(); | ||
| telemetrySuppressNextKeyRef.current = data; | ||
|
|
||
| onKeyStrokeRef.current?.({ | ||
| key: data, | ||
| altKey: telemetryLastModsRef.current.altKey, | ||
| ctrlKey: telemetryLastModsRef.current.ctrlKey, | ||
| metaKey: telemetryLastModsRef.current.metaKey, | ||
| shiftKey: telemetryLastModsRef.current.shiftKey, | ||
| repeat: false, | ||
| dtMs: Math.max(0, Math.floor(performance.now())), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/src/components/VimRaceEditor.tsx | sed -n '380,440p'Repository: Swaggermuffin64/VIM_GYM
Length of output: 2687
🏁 Script executed:
cd frontend/src/components && wc -l VimRaceEditor.tsxRepository: Swaggermuffin64/VIM_GYM
Length of output: 89
🏁 Script executed:
rg -n 'telemetryPendingIntlRef' frontend/src/components/VimRaceEditor.tsx -B 3 -A 3Repository: Swaggermuffin64/VIM_GYM
Length of output: 1732
🏁 Script executed:
rg -n 'insertCompositionText|isComposing' frontend/src/components/VimRaceEditor.tsx -B 2 -A 2Repository: Swaggermuffin64/VIM_GYM
Length of output: 222
🏁 Script executed:
rg -n 'Process|Dead' frontend/src/components/VimRaceEditor.tsx | head -20Repository: Swaggermuffin64/VIM_GYM
Length of output: 419
🏁 Script executed:
rg -n 'isComposing' frontend/src/components/VimRaceEditor.tsxRepository: Swaggermuffin64/VIM_GYM
Length of output: 49
🏁 Script executed:
rg -n 'IME|composition|AltGr' frontend/src/components/VimRaceEditor.tsx -i | head -15Repository: Swaggermuffin64/VIM_GYM
Length of output: 942
🏁 Script executed:
sed -n '440,480p' frontend/src/components/VimRaceEditor.tsxRepository: Swaggermuffin64/VIM_GYM
Length of output: 1310
Guard insertCompositionText to avoid recording partial IME composition strings.
The handler accepts both insertText and insertCompositionText events when telemetryPendingIntlRef is set by Process/Dead keydowns. However, insertCompositionText fires repeatedly during active IME composition (CJK, Vietnamese, etc.) with partial/intermediate multi-character strings that change on each event. Recording these as keystroke telemetry inflates the data.
Since the codebase is narrowly scoped to Nordic AltGr (which produces a single insertText event per committed glyph), consider either:
- Removing
insertCompositionTextentirely, or - Guarding it with
data.length === 1and/ore.isComposing === falseif full IME support is intended.
🤖 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 `@frontend/src/components/VimRaceEditor.tsx` around lines 398 - 422, The
handleBeforeInput handler currently treats both 'insertText' and
'insertCompositionText' the same, causing repeated IME composition strings to be
recorded; update the conditional in handleBeforeInput to either drop
'insertCompositionText' entirely or, preferably, only accept it when
e.isComposing is false and data.length === 1 (i.e. treat insertText as before
but guard insertCompositionText), keeping the existing uses of
telemetryPendingIntlRef, telemetrySuppressNextKeyRef and onKeyStrokeRef
unchanged.
|
|
||
| telemetryPendingIntlRef.current = false; | ||
| clearTelemetryIntlTimer(); | ||
| telemetrySuppressNextKeyRef.current = data; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On Windows AltGr (Nordic) layouts in Chrome and Firefox, after a "Process" keydown and a beforeinput with the composed glyph, does the browser always emit a follow-up keydown whose KeyboardEvent.key equals the inserted character?
💡 Result:
No. Browsers do not consistently guarantee that after a Windows AltGr (Nordic) “Process” keydown and the subsequent beforeinput that inserts the composed glyph, they will always emit a follow-up keydown where KeyboardEvent.key equals the inserted character. What the sources do support: 1) The inserted character is carried by input-before DOM events (beforeinput/input) rather than being reliable in keydown/keyup. The beforeinput event is the one that reports the inserted text via InputEvent.data (“returns a string with the inserted characters”) [1]. keydown is layout/character-dependent only via KeyboardEvent.key, while it is not described as the event that must correspond 1:1 to produced text in all cases [2]. 2) AltGr on Windows is special-cased and differs across engines; it can show up as AltGraph/getModifierState("AltGraph") and can produce multiple underlying key events (e.g., extra ControlLeft/AltRight artifacts under Windows in some browsers). Mozilla’s tracking bug describes AltRight keydown/keyup sequencing and notes that browser behavior is mapped differently, not that a deterministic “keydown with key==inserted glyph” always follows [3]. Community issue reports also describe extra events/stream artifacts for AltGr under Windows that can interfere with assumptions like “one keydown equals the composed character” [4]. Therefore, your requested guarantee (always emitting a follow-up keydown whose KeyboardEvent.key equals the composed glyph) is not something that Chrome/Firefox document or promise, and AltGr’s platform/browser-specific event stream characteristics make such a strict guarantee unlikely. Practical implication: if you need the inserted character, observe beforeinput (InputEvent.data) / input, not a follow-up keydown [1]. If you need to detect AltGraph specifically, check KeyboardEvent.key values and/or getModifierState("AltGraph"), since key behavior for the right Alt/AltGr is special-cased in Firefox and Chrome [5][3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Events/beforeinput
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Element/keydown_event
- 3: https://bugzilla.mozilla.org/show_bug.cgi?id=900750
- 4: AltGr-Key under Windows copy/v86#1127
- 5: https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values
telemetrySuppressNextKeyRef can leak across keystrokes and silently swallow a later keypress.
telemetrySuppressNextKeyRef.current is set inside handleBeforeInput but is only ever cleared when a subsequent keydown arrives whose e.key matches the stored data. However, for AltGr (Nordic) and IME scenarios, browsers do not guarantee that such a follow-up keydown will arrive with matching KeyboardEvent.key. The inserted character is delivered via the beforeinput event itself (InputEvent.data), not by a follow-up keydown event. If the matching follow-up keydown never fires—whether due to focus changes, IME composition behavior, or platform-specific key event streams—the suppression ref retains its value indefinitely. The next time the user genuinely types that glyph, it will be silently dropped from telemetry.
Clear it on a short timer (e.g., 220ms, consistent with the Intl pending fallback), or clear on any keydown regardless of key match.
🛡️ Sketch of a timer-based clear
+ const telemetrySuppressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const clearTelemetrySuppress = () => {
+ telemetrySuppressNextKeyRef.current = null;
+ if (telemetrySuppressTimerRef.current !== null) {
+ clearTimeout(telemetrySuppressTimerRef.current);
+ telemetrySuppressTimerRef.current = null;
+ }
+ };
const handleBeforeInput = (e: InputEvent) => {
...
- telemetrySuppressNextKeyRef.current = data;
+ telemetrySuppressNextKeyRef.current = data;
+ if (telemetrySuppressTimerRef.current !== null) {
+ clearTimeout(telemetrySuppressTimerRef.current);
+ }
+ telemetrySuppressTimerRef.current = setTimeout(clearTelemetrySuppress, 220);…and clear the timer on unmount alongside the existing Intl timer.
Also applies to: 445-451
🤖 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 `@frontend/src/components/VimRaceEditor.tsx` at line 411, The telemetry
suppression ref can persist and drop later genuine keypresses; update the logic
that sets telemetrySuppressNextKeyRef.current in handleBeforeInput to start a
short timer (≈220ms) that clears telemetrySuppressNextKeyRef.current when it
fires, also clear that timer on component unmount, and change the keydown
handler (the function that currently compares e.key to
telemetrySuppressNextKeyRef.current) to clear
telemetrySuppressNextKeyRef.current on any keydown regardless of key match;
ensure you cancel/clear the timer whenever the ref is cleared to avoid leaks.
1 participant
@bro-lauritz