Fix hotkeys triggering text search on firefox#3620
Open
akaltar wants to merge 2 commits intoopenfrontio:mainfrom
Open
Fix hotkeys triggering text search on firefox#3620akaltar wants to merge 2 commits intoopenfrontio:mainfrom
akaltar wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
|
|
Contributor
WalkthroughAdds Firefox detection to Platform and uses it in InputHandler to call preventDefault() for plain text/number key presses on Firefox, preventing the browser's page text search. Tests updated to cover Platform.isFirefox and the new InputHandler behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🤖 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/Platform.ts`:
- Line 53: The module reads navigator.userAgent into the const isFirefox at
top-level which can throw in non-browser environments; change the initialization
to guard against missing browser globals by using the existing isBrowser check
(or typeof navigator !== 'undefined') before testing the userAgent, e.g. compute
isFirefox as a boolean expression that short-circuits when not in a browser (use
isBrowser && /Firefox/i.test(navigator.userAgent) or similar) so module
initialization never accesses navigator in non-browser contexts.
In `@tests/InputHandler.test.ts`:
- Around line 755-756: The test is incorrectly using vi.spyOn(Platform,
"isFirefox", "get") on a plain data property; replace the spy call with direct
property assignment or a temporary defineProperty override on Platform.isFirefox
in the test (e.g., save the original value, set Platform.isFirefox = true for
the test, then restore the original in afterEach) so the mock does not throw at
runtime—target the Platform.isFirefox symbol in InputHandler.test.ts and ensure
the original value is restored after the 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: de351d94-7ad2-4e38-a61d-e378f5ed8ff8
📒 Files selected for processing (4)
src/client/InputHandler.tssrc/client/Platform.tstests/InputHandler.test.tstests/client/Platform.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
|
@akaltar Hi, thanks for this. I'm not a reviewer, but have you been able to check CodeRabbit's feedback? It makes valid points quite a lot |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/InputHandler.test.ts (1)
767-769:⚠️ Potential issue | 🟡 MinorRestore the original
Platform.isFirefoxvalue, not a hardcodedfalse.At Line 768, hardcoding
falsecan leak incorrect global state across tests. Save the previous value and restore it in teardown.Suggested fix
describe("preventDefault is called on firefox keybinds", () => { + let previousIsFirefox: boolean; + + beforeAll(() => { + previousIsFirefox = Platform.isFirefox; + }); + beforeEach(() => { @@ Platform.isFirefox = true; @@ afterAll(() => { - Platform.isFirefox = false; + Platform.isFirefox = previousIsFirefox; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InputHandler.test.ts` around lines 767 - 769, The teardown currently overwrites Platform.isFirefox with a hardcoded false; instead capture the original value (e.g., const originalIsFirefox = Platform.isFirefox) before tests run (in the containing describe or beforeAll) and restore that saved value in afterAll using Platform.isFirefox = originalIsFirefox so global state is returned to its prior value rather than always false.
🧹 Nitpick comments (1)
tests/InputHandler.test.ts (1)
744-797: Add a non-Firefox assertion for plain text keydown.Current coverage only verifies Firefox mode. Add one test where
Platform.isFirefox = falseandDigit1does not callpreventDefault, so “Firefox-only” behavior is protected.Suggested test addition
test("should preventDefault on simple button press", () => { @@ expect(event.defaultPrevented).toBeTruthy(); }); + test("should not preventDefault on simple button press outside Firefox", () => { + Platform.isFirefox = false; + const event = new KeyboardEvent("keydown", { + code: "Digit1", + key: "1", + cancelable: true, + }); + + window.dispatchEvent(event); + + expect(event.defaultPrevented).toBeFalsy(); + }); + test("Shouldn't call preventDefault for browser shortcut like zoom", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InputHandler.test.ts` around lines 744 - 797, Add a mirror test for non-Firefox behavior: create a test that sets Platform.isFirefox = false, initializes InputHandler (using the same setup as the Firefox tests or reusing the existing beforeEach), dispatches a cancelable KeyboardEvent with code "Digit1"/key "1" and then asserts event.defaultPrevented is false; ensure Platform.isFirefox is restored/reset the same way other tests do and reuse InputHandler/initialize from the existing test setup so the new test protects the Firefox-only behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/InputHandler.test.ts`:
- Around line 767-769: The teardown currently overwrites Platform.isFirefox with
a hardcoded false; instead capture the original value (e.g., const
originalIsFirefox = Platform.isFirefox) before tests run (in the containing
describe or beforeAll) and restore that saved value in afterAll using
Platform.isFirefox = originalIsFirefox so global state is returned to its prior
value rather than always false.
---
Nitpick comments:
In `@tests/InputHandler.test.ts`:
- Around line 744-797: Add a mirror test for non-Firefox behavior: create a test
that sets Platform.isFirefox = false, initializes InputHandler (using the same
setup as the Firefox tests or reusing the existing beforeEach), dispatches a
cancelable KeyboardEvent with code "Digit1"/key "1" and then asserts
event.defaultPrevented is false; ensure Platform.isFirefox is restored/reset the
same way other tests do and reuse InputHandler/initialize from the existing test
setup so the new test protects the Firefox-only behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddf79c44-92b5-4315-a8f8-57033242662a
📒 Files selected for processing (2)
src/client/Platform.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Platform.ts
4 tasks
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
3 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.
Description:
On firefox desktop, typing anything starts searching on the page by default , similar to ctrl + F shortcut on other browsers.
This causes the active keys to get stuck and most of the hotkeys to not work.
Here is how that looked like when pressing "a" to scroll left:
To fix I've added:
preventDefaulton text-like keydowns that are commonly used in the game, and only on firefoxpreventDefaultbeing called at the correct timesMaybe a more ideal fix could be to check if there is a bound key that matches the current key pressed, and only
preventDefaultthen. I'm open to implementing any suggested approach if a different one is preferred.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Discord username: akaltar
But I'm more likely to respond quickly to being tagged on github