Fix Ctrl+Click for Macs, and alphabetize gitignore #2720
Conversation
|
|
WalkthroughAdds persisted, mockable macOS detection and mouse-button helpers; refactors pointer handlers to emit a ContextMenuEvent for Mac Ctrl+Click (preventing click/attack), updates .gitignore layout, and adds a test asserting Mac Ctrl+Click emits ContextMenuEvent and suppresses MouseUpEvent. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as DOM
participant InputHandler
participant EventBus
Note over InputHandler,EventBus: Mac Ctrl+Click flow (changed)
User->>Browser: Ctrl+click (trackpad)
Browser->>InputHandler: pointerup (button, modifiers, coords)
alt InputHandler.isMac() && Control pressed
InputHandler->>EventBus: emit ContextMenuEvent(x,y)
Note right of EventBus: MouseUpEvent NOT emitted (no attack/click)
else normal click / other platforms
InputHandler->>EventBus: emit MouseUpEvent(x,y)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-18T17:54:01.311ZApplied to files:
📚 Learning: 2025-11-07T00:23:48.567ZApplied to files:
🧬 Code graph analysis (1)tests/InputHandler.test.ts (1)
🔇 Additional comments (10)
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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoresrc/client/InputHandler.tstests/InputHandler.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-07T00:23:48.567Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 2408
File: src/client/InputHandler.ts:485-485
Timestamp: 2025-11-07T00:23:48.567Z
Learning: In JavaScript/TypeScript PointerEvent and MouseEvent, the properties event.x and event.y are aliases for event.clientX and event.clientY. They represent the same viewport-relative coordinates and can be used interchangeably.
Applied to files:
src/client/InputHandler.tstests/InputHandler.test.ts
🧬 Code graph analysis (1)
tests/InputHandler.test.ts (1)
src/client/InputHandler.ts (2)
MouseUpEvent(8-13)ContextMenuEvent(52-57)
🔇 Additional comments (11)
.gitignore (1)
1-17: Clean reorganization improves maintainability.The alphabetized sections make the gitignore easier to scan and maintain.
tests/InputHandler.test.ts (2)
4-9: LGTM!The additional imports support the new Mac Ctrl+Click test case.
464-504: Well-structured test validates the Mac Ctrl+Click fix.The test correctly simulates the scenario and verifies that ContextMenuEvent is emitted instead of MouseUpEvent, preventing unintended attacks.
src/client/InputHandler.ts (8)
154-158: Good testable pattern for platform detection.The readonly field with a mockable method enables reliable unit testing while keeping the detection simple.
206-207: Consistent use of the new isMac() method.
455-465: Helper methods improve code clarity.Extracting button checks into named methods makes the intent clear and centralizes the logic.
484-493: LGTM!Consistent button handling with clear comments.
521-526: Helpful comment explains touch event handling.
555-562: Consistent button filtering.
596-601: Comment clarifies ghost structure behavior.Note: When Mac Ctrl+Click occurs, both line 513 and line 601 might emit ContextMenuEvent depending on browser behavior. This is likely fine since the UI should handle duplicate events gracefully, but worth being aware of.
649-655: Clean helper methods centralize button logic.Simple, focused methods that make the code easier to read and maintain.
|
@clounie can you resolve the branch conflicts and I'll look and see what I can do about getting this reviewed! |
DevelopingTom
left a comment
DevelopingTom
left a comment
There was a problem hiding this comment.
Looks good to me but can't try it without a Mac.
…cCtrlClick # Conflicts: # .gitignore # src/client/InputHandler.ts # tests/InputHandler.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoresrc/client/InputHandler.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/client/InputHandler.ts
📚 Learning: 2025-11-07T00:23:48.567Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 2408
File: src/client/InputHandler.ts:485-485
Timestamp: 2025-11-07T00:23:48.567Z
Learning: In JavaScript/TypeScript PointerEvent and MouseEvent, the properties event.x and event.y are aliases for event.clientX and event.clientY. They represent the same viewport-relative coordinates and can be used interchangeably.
Applied to files:
src/client/InputHandler.tstests/InputHandler.test.ts
🧬 Code graph analysis (1)
tests/InputHandler.test.ts (1)
src/client/InputHandler.ts (2)
MouseUpEvent(8-13)ContextMenuEvent(52-57)
🪛 GitHub Actions: 🧪 CI
tests/InputHandler.test.ts
[error] 467-467: TS2708 Cannot use namespace 'jest' as a value.
🪛 GitHub Check: 🔬 Test
tests/InputHandler.test.ts
[failure] 467-467: tests/InputHandler.test.ts > InputHandler AutoUpgrade > Mac Ctrl+Click Context Menu > should create context menu with Ctrl+Click on Mac, but not attack
ReferenceError: jest is not defined
❯ tests/InputHandler.test.ts:467:7
🔇 Additional comments (7)
tests/InputHandler.test.ts (1)
464-504: Test logic looks correct.Once the
jest→viissue is fixed, this test properly verifies that Mac Ctrl+Click emitsContextMenuEventinstead ofMouseUpEvent, preventing accidental attacks on macOS.src/client/InputHandler.ts (6)
156-160: LGTM! Clean Mac detection with test support.The readonly field plus public method pattern makes testing straightforward while keeping the detection logic simple.
665-671: LGTM! Helper methods clarify mouse button logic.These methods replace magic numbers with clear intent.
button === 1for middle-click andbutton > 0for non-left buttons are both correct.
507-515: LGTM! Mac Ctrl+Click fix prevents accidental attacks.The logic correctly emits
ContextMenuEventand returns early to suppressMouseUpEvent(which would trigger an attack). This resolves the reported issue.
454-465: LGTM! Clean middle-click and button filtering.The helper methods make the intent clear: middle-click triggers auto-upgrade, and non-left buttons are filtered early.
596-600: LGTM! Right-click cancels ghost structure placement.This provides clear feedback: when placing a structure, right-click exits placement mode instead of opening the context menu. Good UX.
208-209: LGTM! Platform-aware modifier key selection.Mac users get Cmd (MetaLeft) as the modifier, while others get Ctrl (ControlLeft). This aligns with platform conventions.
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
|
@iiamlewis should be good to go -
|
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
This reverts commit 40eea22.
This reverts commit 40eea22. Commit breaks map dragging ability.
|
@clounie this broke non mac pcs |
I'll have to debug. At first glance this seems to imply the For future me - "Commit breaks map dragging ability." |
5 participants
Resolves #2719
npm run formatnpm run lint:fixnpm testDescription:
What problem(s) was I solving?
On Mac,
Ctrl+Clickis commonly used as a substitute for right-click to open context menus. However, in OpenFront,Ctrl+Clickwas triggering both the context menu, AND causing an attack.This effectively prevented Mac users from being able to ally with bots/nations, as trying to ally would cause you to attack them.
What changes did I make?
Ctrl+Clickon Mac no longer triggers an attack, allowing players to properly interact with the alliance systemHow I implemented it
isMac()method toInputHandlerwhich encapsulates the "is mac" logic, and lets it be mocked during testsonPointerUphandler, added a check: if on Mac andControlLeftis held, emit aContextMenuEvent, and then return (instead of continuing to also create aMouseUpEvent)isMiddleMouseButton,isNonLeftMouseButton) for improved code clarityLast, alphabetized the
.gitignorefile and organized it into "Folders" and "Files" sections to make it easier to read.How to verify it
Manual Testing
Ctrland left-click on another nation - verify the context menu opens (not an attack)Ctrl+Clickcontinue to work as before (modifier key for build menu if configured)Automated Testing
Mac Ctrl+Click Context Menu- verifies thatCtrl+Clickon Mac emitsContextMenuEventinstead ofMouseUpEventDescription for the changelog
Fixed
Ctrl+Clickon Mac to properly open the context menu instead of triggering an attack, restoring the ability for Mac users to form alliances with other nations.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Terekhov