feat: hold middle-click to continuously upgrade buildings#3821
Open
MoritzCMD wants to merge 1 commit intoopenfrontio:mainfrom
Open
feat: hold middle-click to continuously upgrade buildings#3821MoritzCMD wants to merge 1 commit intoopenfrontio:mainfrom
MoritzCMD wants to merge 1 commit intoopenfrontio:mainfrom
Conversation
- Hold middle-click repeats upgrade every 150ms after 400ms initial delay - Single middle-click still upgrades once (existing behavior preserved) - Toggle and speed (50-500ms) configurable in Settings - preventDefault on middle-click prevents page scroll
|
|
Contributor
WalkthroughThe PR adds a configurable "hold middle-click to auto-upgrade" feature across the input layer, game settings, and UI. Middle-mouse-button holds trigger repeated upgrade events at a user-configurable rate, controlled via toggle and speed slider in the settings modal. ChangesHold Middle-Click Upgrade Feature
Sequence DiagramsequenceDiagram
participant User
participant InputHandler
participant UserSettings
participant Game
User->>InputHandler: Middle-click down
InputHandler->>UserSettings: Check holdMiddleClickUpgrade()
alt Feature enabled
InputHandler->>InputHandler: Start 400ms timeout
Note over InputHandler: Waiting for hold confirmation
InputHandler->>InputHandler: timeout fires → start interval
loop While middle-click held
InputHandler->>InputHandler: Emit AutoUpgradeEvent
InputHandler->>Game: Trigger upgrade
Note over InputHandler: Wait holdMiddleClickUpgradeSpeed() ms
end
end
User->>InputHandler: Middle-click up
InputHandler->>InputHandler: Clear timeout & interval
Note over InputHandler: Ready for next input
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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/InputHandler.ts`:
- Around line 590-598: Create a single helper (e.g. clearHoldMiddleClickTimers)
that clears and nulls this.holdUpgradeTimeout and this.holdUpgradeInterval (use
clearTimeout/clearInterval) to stop AutoUpgradeEvent emission; replace the
existing inline clear logic in the middle-button release path with a call to
this helper and also call it from the blur handler and destroy() so timers
cannot survive focus loss or teardown. Ensure references to
this.userSettings.holdMiddleClickUpgradeSpeed(), this.eventBus.emit, and
AutoUpgradeEvent remain unchanged.
In `@src/core/game/UserSettings.ts`:
- Around line 166-178: Clamp the hold-middle-click upgrade speed to the
documented 50–500ms range on both read and write to prevent negative or NaN
values causing a tight loop: in get method holdMiddleClickUpgradeSpeed() apply a
clamp and NaN fallback after calling
getFloat("settings.holdMiddleClickUpgradeSpeed", 150) so it always returns a
number between 50 and 500, and in setter setHoldMiddleClickUpgradeSpeed(value:
number) sanitize the incoming value (handle NaN) and clamp it to 50–500 before
calling setFloat("settings.holdMiddleClickUpgradeSpeed", value); keep the
boolean toggle method holdMiddleClickUpgrade() unchanged.
🪄 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: 67a6d64a-b07d-4eb4-86ea-3409c7eb3954
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingSlider.tssrc/core/game/UserSettings.ts
Comment on lines
+590
to
+598
| if (this.userSettings.holdMiddleClickUpgrade()) { | ||
| this.holdUpgradeTimeout = setTimeout(() => { | ||
| this.holdUpgradeInterval = setInterval(() => { | ||
| this.eventBus.emit( | ||
| new AutoUpgradeEvent(event.clientX, event.clientY), | ||
| ); | ||
| }, this.userSettings.holdMiddleClickUpgradeSpeed()); | ||
| }, 400); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the middle-click timers outside pointerup too.
If focus changes or the handler is torn down while the button is still held, these timer handles can survive and keep emitting AutoUpgradeEvents in the background. Please route the cleanup through one helper and call it from blur/teardown as well.
🔧 Suggested cleanup helper
+ private clearHoldUpgradeTimers() {
+ clearTimeout(this.holdUpgradeTimeout);
+ clearInterval(this.holdUpgradeInterval);
+ this.holdUpgradeTimeout = undefined;
+ this.holdUpgradeInterval = undefined;
+ }Call it from the middle-button release path, the blur handler, and destroy().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/InputHandler.ts` around lines 590 - 598, Create a single helper
(e.g. clearHoldMiddleClickTimers) that clears and nulls this.holdUpgradeTimeout
and this.holdUpgradeInterval (use clearTimeout/clearInterval) to stop
AutoUpgradeEvent emission; replace the existing inline clear logic in the
middle-button release path with a call to this helper and also call it from the
blur handler and destroy() so timers cannot survive focus loss or teardown.
Ensure references to this.userSettings.holdMiddleClickUpgradeSpeed(),
this.eventBus.emit, and AutoUpgradeEvent remain unchanged.
Contributor
There was a problem hiding this comment.
@evanpelle, here's the fix — a single clearHoldUpgradeTimers() helper called from all three places:
// Add this private helper method to InputHandler class
private clearHoldUpgradeTimers(): void {
clearTimeout(this.holdUpgradeTimeout);
clearInterval(this.holdUpgradeInterval);
this.holdUpgradeTimeout = undefined;
this.holdUpgradeInterval = undefined;
}1. Replace the inline cleanup in onPointerUp:
onPointerUp(event: PointerEvent) {
if (event.button === 1) {
event.preventDefault();
- clearTimeout(this.holdUpgradeTimeout);
- clearInterval(this.holdUpgradeInterval);
- this.holdUpgradeTimeout = undefined;
- this.holdUpgradeInterval = undefined;
+ this.clearHoldUpgradeTimers();
return;
}2. Add cleanup in the blur handler (inside initialize()):
window.addEventListener("blur", () => {
this.activeKeys.clear();
if (this.alternateView) {
this.alternateView = false;
this.eventBus.emit(new AlternateViewEvent(false));
}
this.pointerDown = false;
this.pointers.clear();
+ this.clearHoldUpgradeTimers();
if (this.longPressTimer !== null) {3. Add cleanup in destroy():
destroy() {
if (this.moveInterval !== null) {
clearInterval(this.moveInterval);
}
+ this.clearHoldUpgradeTimers();
this.activeKeys.clear();
}This keeps the logic in one place and makes sure the upgrade loop stops on focus loss or teardown — not just on mouse-up.
🧠 Learnings used
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2946
File: src/client/InputHandler.ts:21-21
Timestamp: 2026-01-18T15:42:50.689Z
Learning: In the OpenFrontIO repository, ensure that InputHandler.ts uses a MOUSE_CODES set that excludes MouseRight. Only MouseLeft and MouseMiddle should be supported for keybind actions; MouseRight should not be included in keybind mappings. Verify by inspecting the MOUSE_CODES definition and any keybind bindings to confirm MouseRight is not present, and add a test or assertion if possible to prevent regression.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: Ensure ghost preservation logic for special units (AtomBomb and HydrogenBomb) is implemented via shouldPreserveGhostAfterBuild(unitType) and that ghost removal is conditional on !shouldPreserveGhostAfterBuild(unitType) within createStructure. Confirm that both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure, and that Enter/NumpadEnter confirmations do not break the locked-nuke multi-placement since the ghost should be preserved by design. Add tests to cover shouldPreserveGhostAfterBuild returning true for AtomBomb/HydrogenBomb and verify that ghost removal only occurs for other units. Applies to files in the repository OpenFrontIO (src/client/InputHandler.ts and src/client/graphics/layers/StructureIconsLayer.ts).
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3549
File: tests/pathfinding/playground/server.ts:246-250
Timestamp: 2026-03-31T17:02:41.798Z
Learning: When migrating/implementing Express 5+ code, use the Express 5 `app.listen(PORT, (error) => { ... })` callback signature: Express 5 passes listen errors (e.g., `EADDRINUSE`) as the first argument to the callback. Handle the error inside that callback instead of relying on Node.js `http.Server`’s `'error'` event semantics.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 3625
File: src/client/ClanApi.ts:164-180
Timestamp: 2026-04-10T00:17:12.810Z
Learning: When reviewing OpenFrontIO TypeScript client code (e.g., src/client/*.ts), do not require runtime Zod validation for response bodies in cases where the client calls an internal endpoint with a guaranteed, fixed response shape. For `joinClan()` in src/client/ClanApi.ts, allow returning the success body using a TypeScript `as { status: "joined" | "requested" }` cast without additional runtime Zod validation, since `/clans/:tag/join` is internal and always returns exactly `{ status: "joined" | "requested" }`.
Learnt from: baculinivan-web
Repo: openfrontio/OpenFrontIO PR: 3688
File: src/client/graphics/layers/GameRightSidebar.ts:235-248
Timestamp: 2026-04-22T08:54:50.345Z
Learning: In the OpenFrontIO/OpenFrontIO repository, do not raise review findings for standard web accessibility (a11y) keyboard/semantic issues that would normally require button semantics (e.g., using clickable `<div>` elements instead of `<button>`, or similar keyboard accessibility/ARIA patterns). This repository intentionally does not enforce those semantic/keyboard-accessibility conventions because gameplay requires touch or mouse input and is not operable via keyboard alone.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Comment on lines
+166
to
+178
| holdMiddleClickUpgradeSpeed(): number { | ||
| return this.getFloat("settings.holdMiddleClickUpgradeSpeed", 150); | ||
| } | ||
|
|
||
| toggleHoldMiddleClickUpgrade() { | ||
| this.setBool( | ||
| "settings.holdMiddleClickUpgrade", | ||
| !this.holdMiddleClickUpgrade(), | ||
| ); | ||
| } | ||
|
|
||
| setHoldMiddleClickUpgradeSpeed(value: number): void { | ||
| this.setFloat("settings.holdMiddleClickUpgradeSpeed", value); |
Contributor
There was a problem hiding this comment.
Clamp the hold speed before persisting it.
A negative or NaN delay from storage/manual edits can turn the upgrade loop into a tight spin. Keep this within the documented 50-500ms window on both read and write.
🛠️ Suggested fix
holdMiddleClickUpgradeSpeed(): number {
- return this.getFloat("settings.holdMiddleClickUpgradeSpeed", 150);
+ const raw = this.getFloat("settings.holdMiddleClickUpgradeSpeed", 150);
+ if (!Number.isFinite(raw)) return 150;
+ return Math.min(500, Math.max(50, Math.round(raw)));
}
@@
setHoldMiddleClickUpgradeSpeed(value: number): void {
- this.setFloat("settings.holdMiddleClickUpgradeSpeed", value);
+ const normalized = Number.isFinite(value) ? Math.round(value) : 150;
+ this.setFloat(
+ "settings.holdMiddleClickUpgradeSpeed",
+ Math.min(500, Math.max(50, normalized)),
+ );
}🤖 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 166 - 178, Clamp the
hold-middle-click upgrade speed to the documented 50–500ms range on both read
and write to prevent negative or NaN values causing a tight loop: in get method
holdMiddleClickUpgradeSpeed() apply a clamp and NaN fallback after calling
getFloat("settings.holdMiddleClickUpgradeSpeed", 150) so it always returns a
number between 50 and 500, and in setter setHoldMiddleClickUpgradeSpeed(value:
number) sanitize the incoming value (handle NaN) and clamp it to 50–500 before
calling setFloat("settings.holdMiddleClickUpgradeSpeed", value); keep the
boolean toggle method holdMiddleClickUpgrade() unchanged.
Contributor
There was a problem hiding this comment.
- Clamp
holdMiddleClickUpgradeSpeed()getter to[50, 500]with aNaN/ non-finite fallback to150. - Clamp
setHoldMiddleClickUpgradeSpeed()setter the same way before persisting.
🧠 Learnings used
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 3292
File: src/core/execution/TrainExecution.ts:70-84
Timestamp: 2026-02-25T02:38:42.961Z
Learning: Preserve the intentional duplication of boundary tiles when constructing train motion plan paths. Specifically, do not deduplicate tiles at station boundaries in train motion plan logic. This affects both server-side tracking (TrainExecution.ts saveTraversedTiles) and client-side processing (GameView.ts train motion plan processing). Reviewers should ensure changes do not remove or consolidate boundary tiles, as duplicates are expected and documented in MotionPlans.ts, and are necessary for correct car spacing and positioning, especially when speed > 1.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3549
File: tests/pathfinding/playground/server.ts:246-250
Timestamp: 2026-03-31T17:02:41.798Z
Learning: When migrating/implementing Express 5+ code, use the Express 5 `app.listen(PORT, (error) => { ... })` callback signature: Express 5 passes listen errors (e.g., `EADDRINUSE`) as the first argument to the callback. Handle the error inside that callback instead of relying on Node.js `http.Server`’s `'error'` event semantics.
Learnt from: baculinivan-web
Repo: openfrontio/OpenFrontIO PR: 3688
File: src/client/graphics/layers/GameRightSidebar.ts:235-248
Timestamp: 2026-04-22T08:54:50.345Z
Learning: In the OpenFrontIO/OpenFrontIO repository, do not raise review findings for standard web accessibility (a11y) keyboard/semantic issues that would normally require button semantics (e.g., using clickable `<div>` elements instead of `<button>`, or similar keyboard accessibility/ARIA patterns). This repository intentionally does not enforce those semantic/keyboard-accessibility conventions because gameplay requires touch or mouse input and is not operable via keyboard alone.
Failed to handle agent chat message. Please try again.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
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:
Adds hold-to-upgrade behavior for the middle mouse button on buildings.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
morizz21