feat: multi-warship selection with Shift+drag box#3677
feat: multi-warship selection with Shift+drag box#3677baculinivan-web wants to merge 13 commits intoopenfrontio:mainfrom
Conversation
Resolves openfrontio#3666 - Shift+drag draws a pixel-dashed selection rectangle in player color - All player-owned warships inside the box get selected at once - Click on water sends all selected warships to that location - Same pulsing selection outline as single-warship selection - MoveIndicatorUI chevron animation shown on target tile - Shift keydown discards active ghost structure (build mode) - Crosshair cursor on Shift hold, resets on release/blur - Warship toolbar tooltip includes Shift hint (en.json) - Tests: WarshipMultiSelection.test.ts + InputHandler selection box tests
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Shift/long-press drag multi-selection for warships: input events and cursor handling, selection-box rendering layer, multi-selection visuals and controls, transport/schema/execution support for batch move intents, localization/help additions, and tests. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant IH as InputHandler
participant SBL as SelectionBoxLayer
participant UL as UnitLayer
participant T as Transport
participant Server as Server
User->>IH: Press Shift / long-press + Drag
IH->>IH: mark selectionBoxActive\nset cursor = "crosshair"
IH->>SBL: WarshipSelectionBoxUpdateEvent(start,end)
SBL->>SBL: render dashed selection rect
User->>IH: Release Pointer
IH->>IH: compute drag distance
alt distance >= threshold
IH->>UL: WarshipSelectionBoxCompleteEvent(start,end)
UL->>UL: filter player-owned warships in rect
UL->>UL: emit WarshipMultiSelectionEvent(units)
else
IH->>UL: WarshipSelectionBoxCancelEvent
UL->>UL: emit WarshipMultiSelectionEvent([])
end
User->>UL: Click water tile (with multi-selection)
UL->>T: MoveMultipleWarshipsIntentEvent(unitIds, tile)
T->>Server: send intent {type: "move_multiple_warships", unitIds, tile}
Server->>Server: ExecutionManager -> create MoveMultipleWarshipsExecution -> spawn per-unit MoveWarshipExecution
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/UnitLayer.ts (1)
1-702:⚠️ Potential issue | 🟡 MinorFormatting issue flagged by CI pipeline.
The Prettier check reported a formatting/style issue for this file. Please run your formatter to fix it before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitLayer.ts` around lines 1 - 702, The file UnitLayer.ts fails the Prettier/style check; run your formatter (e.g., npm run format or npx prettier --write) on UnitLayer.ts (the class UnitLayer and its methods like onMouseUp, onUnitSelectionChange, drawSprite, paintCell, etc.) and commit the formatted file so the CI Prettier check passes—do not change logic, only apply formatting and stage the updated file for the PR.
🤖 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/graphics/layers/UILayer.ts`:
- Around line 211-225: onMultiSelection currently only clears
multiSelectionBoxCenters, so any existing single-selection state (selectedUnit
and lastSelectionBoxCenter) continues animating; update onMultiSelection to also
clear the single-selection outline by calling
clearSelectionBox(lastSelectionBoxCenter.x, lastSelectionBoxCenter.y,
lastSelectionBoxCenter.size) if lastSelectionBoxCenter is set, then null out
selectedUnit and lastSelectionBoxCenter before proceeding to set
multiSelectedWarships and drawSelectionBoxMulti for active units.
In `@src/client/Transport.ts`:
- Around line 637-645: The onMoveMultipleWarshipsEvent currently loops and calls
sendIntent repeatedly, which can split group moves if the socket drops; change
it to send a single batched intent (e.g. sendIntent with type
"move_multiple_warships" and payload { unitIds: event.unitIds, tile: event.tile
}) so the client emits one atomic intent and let the server/local executor (or
existing intent handler) fan out into individual move_warship actions; update
any handlers expecting multiple individual intents to accept and expand the new
batched intent (references: onMoveMultipleWarshipsEvent, sendIntent,
MoveMultipleWarshipsIntentEvent).
---
Outside diff comments:
In `@src/client/graphics/layers/UnitLayer.ts`:
- Around line 1-702: The file UnitLayer.ts fails the Prettier/style check; run
your formatter (e.g., npm run format or npx prettier --write) on UnitLayer.ts
(the class UnitLayer and its methods like onMouseUp, onUnitSelectionChange,
drawSprite, paintCell, etc.) and commit the formatted file so the CI Prettier
check passes—do not change logic, only apply formatting and stage the updated
file for the PR.
🪄 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: 108a19e0-761b-426a-96c9-9d1c20bc3fda
📒 Files selected for processing (11)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/Transport.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/DynamicUILayer.tssrc/client/graphics/layers/SelectionBoxLayer.tssrc/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/UnitLayer.tstests/InputHandler.test.tstests/WarshipMultiSelection.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
- Replace hardcoded ShiftLeft/ShiftRight with this.keybinds.shiftKey - Add warship multi-select row to HelpModal hotkeys table - Add help_modal.action_warship_multiselect and drag keys to en.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/UILayer.ts (1)
230-271: Extract the shared selection-box painter.
drawSelectionBoxMulti()now copies almost all ofdrawSelectionBox(). One helper for the pulse/color/border drawing would keep single- and multi-select visuals from drifting on the next style change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UILayer.ts` around lines 230 - 271, Extract the duplicated selection-box drawing from drawSelectionBoxMulti() and drawSelectionBox() into a single helper (e.g., paintSelectionBox or drawSelectionBoxAt) that accepts centerX, centerY, size, ownerColor (or a computed selectionColor), and uses this.selectionAnimTime / SELECTION_BOX_SIZE to compute pulse opacity and paint the border cells via this.paintCell; update both drawSelectionBoxMulti and drawSelectionBox to call the new helper, keep clearSelectionBox and multiSelectionBoxCenters logic in the callers (only the painting/pulse/color loop moves to the helper) and reuse this.SELECTION_BOX_SIZE, this.paintCell, and this.selectionAnimTime symbols to preserve behavior.
🤖 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/graphics/layers/UnitLayer.ts`:
- Around line 159-169: The current multi-move branch emits
MoveMultipleWarshipsIntentEvent using this.selectedWarships without filtering
out ships that may have been destroyed or deactivated; before emitting, filter
this.selectedWarships to only include currently active/player-owned warships
(e.g., check each warship.isActive() and warship.ownerId === currentPlayerId or
equivalent), then map to ids and emit MoveMultipleWarshipsIntentEvent with that
filtered id list; if the filtered list is empty, do not emit the move intent but
still clear this.selectedWarships and emit WarshipMultiSelectionEvent([]).
---
Nitpick comments:
In `@src/client/graphics/layers/UILayer.ts`:
- Around line 230-271: Extract the duplicated selection-box drawing from
drawSelectionBoxMulti() and drawSelectionBox() into a single helper (e.g.,
paintSelectionBox or drawSelectionBoxAt) that accepts centerX, centerY, size,
ownerColor (or a computed selectionColor), and uses this.selectionAnimTime /
SELECTION_BOX_SIZE to compute pulse opacity and paint the border cells via
this.paintCell; update both drawSelectionBoxMulti and drawSelectionBox to call
the new helper, keep clearSelectionBox and multiSelectionBoxCenters logic in the
callers (only the painting/pulse/color loop moves to the helper) and reuse
this.SELECTION_BOX_SIZE, this.paintCell, and this.selectionAnimTime symbols to
preserve behavior.
🪄 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: c08444b4-2d2b-4217-a30c-2f7b0d74560e
📒 Files selected for processing (5)
src/client/graphics/layers/DynamicUILayer.tssrc/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitLayer.tstests/InputHandler.test.tstests/WarshipMultiSelection.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/graphics/layers/DynamicUILayer.ts
- tests/WarshipMultiSelection.test.ts
- tests/InputHandler.test.ts
onMultiSelection now clears selectedUnit and lastSelectionBoxCenter before applying multi-selection, preventing ghost outlines
…onBoxAt helper - UnitLayer: filter selectedWarships to active+owned before emitting MoveMultipleWarshipsIntentEvent; skip emit if none remain active - UILayer: extract shared paintSelectionBoxAt() helper used by both drawSelectionBox and drawSelectionBoxMulti to remove duplication
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/UILayer.ts (1)
238-279: Extract one shared selection-box painter.
drawSelectionBoxMulti()now copies most ofdrawSelectionBox(). Keeping two copies of the same pulse/border logic will drift fast. Please move the shared paint logic into one helper and keep only the per-mode state tracking separate.Small refactor sketch
+ private drawAnimatedSelectionBox( + centerX: number, + centerY: number, + selectionSize: number, + selectionColor: Colord, + ) { + const baseOpacity = 200; + const pulseAmount = 55; + const opacity = + baseOpacity + Math.sin(this.selectionAnimTime * 0.1) * pulseAmount; + + for (let x = centerX - selectionSize; x <= centerX + selectionSize; x++) { + for (let y = centerY - selectionSize; y <= centerY + selectionSize; y++) { + if ( + (x === centerX - selectionSize || + x === centerX + selectionSize || + y === centerY - selectionSize || + y === centerY + selectionSize) && + (x + y) % 2 === 0 + ) { + this.paintCell(x, y, selectionColor, opacity); + } + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UILayer.ts` around lines 238 - 279, drawSelectionBoxMulti duplicates the pulse/border painting logic from drawSelectionBox; extract that shared logic into a new helper (e.g., paintSelectionBox or paintSelectionBorder) that accepts centerX, centerY, selectionSize, selectionColor and opacity and performs the nested x/y loop and paintCell calls. In drawSelectionBoxMulti and drawSelectionBox compute mode-specific state (selectionSize, centerX/centerY, selectionColor, opacity) and then call the new helper; keep each function's per-mode state tracking (multiSelectionBoxCenters vs the single-selection center/clear logic) unchanged. Ensure helper references the same this.paintCell and this.selectionAnimTime/SELECTION_BOX_SIZE/theme checks as needed and update callers to remove duplicated logic.
🤖 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/graphics/layers/UILayer.ts`:
- Around line 220-225: Update UILayer to clear multi-selection state when a
single unit is selected: in the UILayer.onUnitSelectionChange handler (the
method that processes UnitSelectionEvent), detect when the event represents a
single unit selection and then clear this.multiSelectedWarships and
this.multiSelectionBoxCenters (and remove any rendered boxes via the same
clearSelectionBox logic used in the WarshipMultiSelectionEvent handling). This
mirrors the WarshipMultiSelectionEvent flow (which clears boxes and centers) so
ensure you call the same clearing logic before setting single selection state.
---
Nitpick comments:
In `@src/client/graphics/layers/UILayer.ts`:
- Around line 238-279: drawSelectionBoxMulti duplicates the pulse/border
painting logic from drawSelectionBox; extract that shared logic into a new
helper (e.g., paintSelectionBox or paintSelectionBorder) that accepts centerX,
centerY, selectionSize, selectionColor and opacity and performs the nested x/y
loop and paintCell calls. In drawSelectionBoxMulti and drawSelectionBox compute
mode-specific state (selectionSize, centerX/centerY, selectionColor, opacity)
and then call the new helper; keep each function's per-mode state tracking
(multiSelectionBoxCenters vs the single-selection center/clear logic) unchanged.
Ensure helper references the same this.paintCell and
this.selectionAnimTime/SELECTION_BOX_SIZE/theme checks as needed and update
callers to remove duplicated logic.
🪄 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: ca104888-b7c1-4c1f-be67-ab596db346e5
📒 Files selected for processing (1)
src/client/graphics/layers/UILayer.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/graphics/layers/UILayer.ts (1)
189-205:⚠️ Potential issue | 🟠 MajorClear box-selection state on normal single selection.
A single
UnitSelectionEventsetsselectedUnit, but it does not clearmultiSelectedWarshipsor the already painted multi-selection boxes. After box-selecting warships, a normal click can therefore leave stale group outlines on screen until some laterWarshipMultiSelectionEvent([])happens.Suggested fix
private onUnitSelection(event: UnitSelectionEvent) { if (event.isSelected) { + for (const [, center] of this.multiSelectionBoxCenters) { + this.clearSelectionBox(center.x, center.y, center.size); + } + this.multiSelectionBoxCenters.clear(); + this.multiSelectedWarships = []; + this.selectedUnit = event.unit; if (event.unit && event.unit.type() === UnitType.Warship) { this.drawSelectionBox(event.unit); } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UILayer.ts` around lines 189 - 205, When handling a normal single selection in onUnitSelection, you must also clear any prior multi-selection state and visuals so stale group outlines aren't left on screen; update onUnitSelection to reset this.multiSelectedWarships (set to empty array) and remove any painted multi-selection boxes — either call an existing clearMultiSelectionBoxes() helper if present or iterate the stored multi-selection centers and call clearSelectionBox(x,y,size) for each before setting this.selectedUnit; ensure any stored multi-selection center/state is nulled after clearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/graphics/layers/UILayer.ts`:
- Around line 189-205: When handling a normal single selection in
onUnitSelection, you must also clear any prior multi-selection state and visuals
so stale group outlines aren't left on screen; update onUnitSelection to reset
this.multiSelectedWarships (set to empty array) and remove any painted
multi-selection boxes — either call an existing clearMultiSelectionBoxes()
helper if present or iterate the stored multi-selection centers and call
clearSelectionBox(x,y,size) for each before setting this.selectedUnit; ensure
any stored multi-selection center/state is nulled after clearing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f032545b-bc27-4586-a3ec-ceeb986e9cb3
📒 Files selected for processing (3)
src/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitLayer.tstests/InputHandler.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/graphics/layers/UnitLayer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/InputHandler.test.ts
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Nice. Have you thought about how the mobile equivalent would work? The goal is to have mobile on par with desktop as much as possible, even if hotkeys etc aren't possible. Maybe in a follow-up PR if you'd be up for it and this one gets merged. Perhaps something like this (but water instead of ground in this case ofc) 
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/UILayer.ts (1)
271-293: Nice extraction of shared logic.
paintSelectionBoxAtconsolidates the duplicated box-painting code. Clean and readable.One small note: the nested loop iterates all cells in the box but only paints border pixels. You could iterate just the border for a tiny speedup, but with
SELECTION_BOX_SIZE = 6(13×13 = 169 iterations), this is fine as-is.♻️ Optional: iterate only border cells
private paintSelectionBoxAt( centerX: number, centerY: number, selectionColor: Colord, ) { const size = this.SELECTION_BOX_SIZE; const opacity = 200 + Math.sin(this.selectionAnimTime * 0.1) * 55; - for (let x = centerX - size; x <= centerX + size; x++) { - for (let y = centerY - size; y <= centerY + size; y++) { - if ( - x === centerX - size || - x === centerX + size || - y === centerY - size || - y === centerY + size - ) { - if ((x + y) % 2 === 0) { - this.paintCell(x, y, selectionColor, opacity); - } - } - } - } + // Top and bottom edges + for (let x = centerX - size; x <= centerX + size; x++) { + if ((x + centerY - size) % 2 === 0) { + this.paintCell(x, centerY - size, selectionColor, opacity); + } + if ((x + centerY + size) % 2 === 0) { + this.paintCell(x, centerY + size, selectionColor, opacity); + } + } + // Left and right edges (skip corners, already painted) + for (let y = centerY - size + 1; y < centerY + size; y++) { + if ((centerX - size + y) % 2 === 0) { + this.paintCell(centerX - size, y, selectionColor, opacity); + } + if ((centerX + size + y) % 2 === 0) { + this.paintCell(centerX + size, y, selectionColor, opacity); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UILayer.ts` around lines 271 - 293, paintSelectionBoxAt currently loops over the entire square but only paints border cells; refactor it to iterate only the four border ranges to avoid unnecessary iterations: compute size = this.SELECTION_BOX_SIZE and opacity as before, then loop x from centerX - size to centerX + size for the top (y = centerY - size) and bottom (y = centerY + size) edges and loop y from centerY - size + 1 to centerY + size - 1 for the left (x = centerX - size) and right (x = centerX + size) edges, calling this.paintCell(x,y, selectionColor, opacity); keep using the same function name paintSelectionBoxAt and the same opacity calculation so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/UILayer.ts`:
- Around line 271-293: paintSelectionBoxAt currently loops over the entire
square but only paints border cells; refactor it to iterate only the four border
ranges to avoid unnecessary iterations: compute size = this.SELECTION_BOX_SIZE
and opacity as before, then loop x from centerX - size to centerX + size for the
top (y = centerY - size) and bottom (y = centerY + size) edges and loop y from
centerY - size + 1 to centerY + size - 1 for the left (x = centerX - size) and
right (x = centerX + size) edges, calling this.paintCell(x,y, selectionColor,
opacity); keep using the same function name paintSelectionBoxAt and the same
opacity calculation so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09065808-86a5-4bd0-b271-c5b5bf224c63
📒 Files selected for processing (3)
src/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitLayer.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/InputHandler.test.ts
- src/client/graphics/layers/UnitLayer.ts
- Add MoveMultipleWarshipsIntentSchema to Schemas.ts - Add MoveMultipleWarshipsExecution that fans out to individual MoveWarshipExecutions - Transport now sends a single batched intent instead of N individual ones - Prevents split group moves if socket drops mid-loop
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/Transport.ts (1)
637-643: Consider cloningunitIdsbefore sending to keep payload immutable.At Line 640, passing the same array reference works today, but a defensive copy avoids accidental mutation side effects in future listeners/refactors.
🛡️ Small defensive tweak
private onMoveMultipleWarshipsEvent(event: MoveMultipleWarshipsIntentEvent) { this.sendIntent({ type: "move_multiple_warships", - unitIds: event.unitIds, + unitIds: [...event.unitIds], tile: event.tile, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Transport.ts` around lines 637 - 643, The onMoveMultipleWarshipsEvent handler passes the original MoveMultipleWarshipsIntentEvent.unitIds array to sendIntent, risking accidental mutation of the payload; update onMoveMultipleWarshipsEvent to send a shallow copy of unitIds (e.g., via slice() or spread) when calling sendIntent so the intent payload is immutable — locate the onMoveMultipleWarshipsEvent function and change the unitIds argument to a cloned array before sending.src/core/Schemas.ts (1)
413-417: ConstrainunitIdsin the schema to avoid malformed or oversized intents.At Line 415,
z.array(z.number())allows empty lists, duplicates, and non-integer IDs. Tightening this will prevent no-op/duplicate fan-out and reduce abuse risk from oversized payloads.♻️ Suggested schema hardening
export const MoveMultipleWarshipsIntentSchema = z.object({ type: z.literal("move_multiple_warships"), - unitIds: z.array(z.number()), + unitIds: z + .array(z.number().int().nonnegative()) + .min(1) + // pick a game-appropriate cap constant + .max(1000) + .refine((ids) => new Set(ids).size === ids.length, { + message: "unitIds must be unique", + }), tile: z.number(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Schemas.ts` around lines 413 - 417, The MoveMultipleWarshipsIntentSchema's unitIds currently allows empty, non-integer, duplicate or overly large arrays; update the unitIds definition to be an array of integers (use z.number().int()), require at least one entry (.nonempty()), enforce a sensible maximum length to prevent oversized payloads (e.g., .max(N) with N chosen by domain limits), and enforce uniqueness (either via .refine(...) or .superRefine(...) checking new Set(size) equals array length). Keep the schema name MoveMultipleWarshipsIntentSchema and field unitIds when making these validations so downstream code still matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/Transport.ts`:
- Around line 637-643: The onMoveMultipleWarshipsEvent handler passes the
original MoveMultipleWarshipsIntentEvent.unitIds array to sendIntent, risking
accidental mutation of the payload; update onMoveMultipleWarshipsEvent to send a
shallow copy of unitIds (e.g., via slice() or spread) when calling sendIntent so
the intent payload is immutable — locate the onMoveMultipleWarshipsEvent
function and change the unitIds argument to a cloned array before sending.
In `@src/core/Schemas.ts`:
- Around line 413-417: The MoveMultipleWarshipsIntentSchema's unitIds currently
allows empty, non-integer, duplicate or overly large arrays; update the unitIds
definition to be an array of integers (use z.number().int()), require at least
one entry (.nonempty()), enforce a sensible maximum length to prevent oversized
payloads (e.g., .max(N) with N chosen by domain limits), and enforce uniqueness
(either via .refine(...) or .superRefine(...) checking new Set(size) equals
array length). Keep the schema name MoveMultipleWarshipsIntentSchema and field
unitIds when making these validations so downstream code still matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0354f116-d367-446a-bc09-71a34d361b4b
📒 Files selected for processing (4)
src/client/Transport.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tssrc/core/execution/MoveMultipleWarshipsExecution.ts
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
@VariableVince Is mobile support nessesary to. be merged? idk how such a complex gesture can be implemented for mobile. two finger drag might work, but on a small screen gust selecting by hand would probably be simpler for the end user |
|
@VariableVince i looked at how mobile interactions are handled (first time playing on touchscreen....) and figured Hold + Drag would work. flowchart TD
A[User holds finger on map for ~1 second] --> B[Cross target appears under finger]
B --> C[User drags]
C --> D[Selection box is created]
D --> E[User lets go]
E --> F[Warships are selected]
F --> G[User taps]
G --> H[Warships go to tapped area]
|
|
@VariableVince did it. loved it. wanna do more. works perfectly on ipad mini. IMG_2237.2.MOV |
- Hold finger ~800ms to activate selection mode - Drag to draw selection box (same pixel-dashed style as desktop) - Release to select warships, tap water to send them - Long-press cancelled if finger moves >10px before timer fires - Single SelectionBoxLayer handles both Shift+drag and touch long-press
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/InputHandler.ts (2)
593-612: Inconsistent use ofevent.xvsevent.clientX.Line 597 uses
event.xfor distance calculation, but line 604-605 usesevent.clientX/event.clientYfor the event coordinates. While these are typically equivalent, using them inconsistently can be confusing. Pick one and stick with it.💡 Suggested fix
if (this.selectionBoxActive) { this.selectionBoxActive = false; const dist = - Math.abs(event.x - this.lastPointerDownX) + - Math.abs(event.y - this.lastPointerDownY); + Math.abs(event.clientX - this.lastPointerDownX) + + Math.abs(event.clientY - this.lastPointerDownY); if (dist >= 10) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/InputHandler.ts` around lines 593 - 612, The distance calculation uses event.x/event.y while the emitted WarshipSelectionBoxCompleteEvent passes event.clientX/event.clientY, causing inconsistency; update the distance computation in the this.selectionBoxActive block (where lastPointerDownX and lastPointerDownY are used) to reference event.clientX and event.clientY so the same coordinate properties are used throughout, leaving WarshipSelectionBoxCompleteEvent and WarshipSelectionBoxCancelEvent usage unchanged.
264-278: Consider resettingselectionBoxActiveon blur.The blur handler clears
longPressActiveand the timer, butselectionBoxActiveremains set. If the user tabs away mid-selection, returning focus could leave the state inconsistent.💡 Suggested fix
if (this.longPressTimer !== null) { clearTimeout(this.longPressTimer); this.longPressTimer = null; } this.longPressActive = false; + if (this.selectionBoxActive) { + this.selectionBoxActive = false; + this.eventBus.emit(new WarshipSelectionBoxCancelEvent()); + } this.canvas.style.cursor = "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/InputHandler.ts` around lines 264 - 278, The blur handler registered in window.addEventListener("blur") fails to reset selectionBoxActive, leaving selection state inconsistent when focus is lost; update that handler to set this.selectionBoxActive = false (and clear any selection box state such as this.selectionBox coordinates or visual indicators) and, if you have a SelectionBoxEvent or similar, emit an event via this.eventBus to notify others of the change; ensure this runs alongside the existing resets (this.longPressActive, this.longPressTimer, this.pointers, this.pointerDown, this.alternateView) so returning focus starts from a clean state.
🤖 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 519-523: The current Shift-release handler in InputHandler
(checking this.keybinds.shiftKey and this.selectionBoxActive) can clear the
crosshair too early; update the handler so it only clears the cursor when
neither a selection box nor an ongoing multi-selection is active — e.g., add a
guard that checks a multi-selection flag or whether warships remain selected
(use whatever existing state you have, or introduce this.multiSelectionActive)
before setting this.canvas.style.cursor = ""; alternatively remove this
cursor-reset here and let WarshipSelectionBoxCompleteEvent /
WarshipMultiSelectionEvent handlers manage cursor state so the crosshair set by
WarshipMultiSelectionEvent isn't immediately cleared.
---
Nitpick comments:
In `@src/client/InputHandler.ts`:
- Around line 593-612: The distance calculation uses event.x/event.y while the
emitted WarshipSelectionBoxCompleteEvent passes event.clientX/event.clientY,
causing inconsistency; update the distance computation in the
this.selectionBoxActive block (where lastPointerDownX and lastPointerDownY are
used) to reference event.clientX and event.clientY so the same coordinate
properties are used throughout, leaving WarshipSelectionBoxCompleteEvent and
WarshipSelectionBoxCancelEvent usage unchanged.
- Around line 264-278: The blur handler registered in
window.addEventListener("blur") fails to reset selectionBoxActive, leaving
selection state inconsistent when focus is lost; update that handler to set
this.selectionBoxActive = false (and clear any selection box state such as
this.selectionBox coordinates or visual indicators) and, if you have a
SelectionBoxEvent or similar, emit an event via this.eventBus to notify others
of the change; ensure this runs alongside the existing resets
(this.longPressActive, this.longPressTimer, this.pointers, this.pointerDown,
this.alternateView) so returning focus starts from a clean state.
🪄 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: c42308cc-4bcd-4f19-aad2-fb5049314df9
📒 Files selected for processing (2)
src/client/InputHandler.tssrc/client/graphics/layers/SelectionBoxLayer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/SelectionBoxLayer.ts
- Keep crosshair after Shift release while multi-selection is active - Use event.clientX/Y consistently in selection box distance check - Reset selectionBoxActive and emit cancel on window blur
Wow way to go! You asked if it was necessary to merge, maybe not but an Issue would surely have been created for someone to built it later on. There are other things that could use 'getting more on par' like nuke trajectory preview, which could work with single tap, drag and double tap for launch or something. We probably do need an Issue to have improved instructions for mobile control in the HelpModal. |
- New keybind selectAllWarships (default KeyF) in UserSettings - SelectAllWarshipsEvent emitted on keyup - UnitLayer selects all active player-owned warships - HelpModal row + en.json translation key
Oh for sure, but i am already a bit overwhelmed by 6 of my PRs being bombarded by coderabbit.... I would probably take this after a few of them would be merged @evanpelle :) 
|
|
@baculinivan-web thought about it shortly yesterday and was like 'nah enough for now' but low and behold you have addressed keyboard+mouse and touch screen, but some use touchpad/trackpad :p #3666 (comment). Follow-up PR? I think touchpad is especially tricky to handle correctly without messing up other gestures, if possible at all in a good way btw. |
Touchpad works well. It has same inputs as mouse. Tested. Also tasted with Apple Magic Mouse |
|
⁷> > @baculinivan-web thought about it shortly yesterday and was like 'nah enough for now' but low and behold you have addressed keyboard+mouse and touch screen, but some use touchpad/trackpad :p #3666 (comment). Follow-up PR? I think touchpad is especially tricky to handle correctly without messing up other gestures, if possible at all in a good way btw.
It reminded of issues with zooming/panning, each one never working nicely when it was tried to have both (#1956 was reverted later on). This one apparently didn't have such issues, good to know |
## Description: This PR adds support for `Shift+<key>` keybind combinations across the entire keybind system. Previously, keybinds only supported a single key (e.g. `KeyB` for boat attack). Now any keybind can be configured as `Shift+KeyB`, which will only trigger when Shift is held down simultaneously. Enables to use Shift + A for "select all" feature from #3677 **Changes:** - `InputHandler.ts`: Added `parseKeybind()` helper that parses `"Shift+KeyB"` → `{ shift: true, code: "KeyB" }`. Added `keybindMatchesEvent()` for consistent matching across all keyup/keydown handlers. Updated `resolveBuildKeybind()` and all keybind comparisons to respect the shift modifier. - `SettingKeybind.ts`: When recording a keybind, lone modifier keys (Shift, Ctrl, etc.) are skipped — the component waits for the actual key. If Shift is held when the key is pressed, the value is stored as `"Shift+<code>"`. - `Utils.ts`: `formatKeyForDisplay()` now handles the `Shift+` prefix, displaying e.g. `"Shift+B"`. - `tests/InputHandler.test.ts`: Added 6 tests covering Shift+ keybind matching, negative cases (plain key not triggering Shift-bound action), coexistence of `Digit1` and `Shift+Digit1` on different actions, and Numpad alias support with Shift. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## UI changes: <img width="2255" height="2070" alt="CleanShot 2026-04-15 at 20 23 25@2x" src="https://github.com/user-attachments/assets/96c19fc3-6294-40b7-82eb-3fde52b71618" /> ## Please put your Discord username so you can be contacted if a bug or regression is found: fghjk_60845
|
@evanpelle Review, please. |
| export const MoveMultipleWarshipsIntentSchema = z.object({ | ||
| type: z.literal("move_multiple_warships"), | ||
| unitIds: z.array(z.number()), | ||
| tile: z.number(), | ||
| }); |
There was a problem hiding this comment.
could we reuse movewarshipintent?
|
|
||
| /** Emitted when the selection box is cancelled (e.g. Escape or no drag) */ | ||
| export class WarshipSelectionBoxCancelEvent implements GameEvent {} | ||
|
|
There was a problem hiding this comment.
could this share the same event with single warship selection cancelled?
| /** Emitted when multiple warships are selected via box selection */ | ||
| export class WarshipMultiSelectionEvent implements GameEvent { | ||
| constructor(public readonly units: UnitView[]) {} | ||
| } |
There was a problem hiding this comment.
would we reuse the warship selected event, just have it be an array instead?
| export class MoveMultipleWarshipsIntentEvent implements GameEvent { | ||
| constructor( | ||
| public readonly unitIds: number[], | ||
| public readonly tile: number, | ||
| ) {} | ||
| } |
There was a problem hiding this comment.
same here, just reuse the existing move warship but have it be an array instead
There was a problem hiding this comment.
Same here, remove this file and just have MmoveWarshipExecution handle it
There was a problem hiding this comment.
so i basically forgot warships can move already.... fix incoming
| * Renders the shift+drag / touch long-press warship selection rectangle | ||
| * in world-space, using the same pixel-dashed style as UILayer. | ||
| */ | ||
| export class SelectionBoxLayer implements Layer { |
There was a problem hiding this comment.
I don't think this should implement Layer since it's not registered by the GameRenderer
3 participants
Resolves #3666
Description:
Adds RTS-style box selection for warships. Hold Shift and drag (desktop) or long-press and drag (touch/mobile) to draw a selection rectangle — all player-owned warships inside get selected at once. A subsequent click/tap on water sends them all to that location.
SelectionBoxLayer— pixel-dashed rectangle in world-space, player territory color; shared between desktop and touchUILayer— same pulsing selection outline on each box-selected warship; clears correctly when switching between single/multi selectionUnitLayer— finds warships in screen rect, filters inactive ships before sending; touch support includedInputHandler— Shift+drag and touch long-press+drag both emit selection box events; cursor becomes crosshair on Shift; discards active ghost structure on Shift press; configurable viashiftKeykeybindTransport— single atomicmove_multiple_warshipsintent (no split on socket drop)Schemas+ExecutionManager+MoveMultipleWarshipsExecution— server fans out atomic intent into individualMoveWarshipExecutionper shipDynamicUILayer—MoveIndicatorUIchevron animation on target tile for both single and multi moveUnitDisplay— warship tooltip Shift hint viatranslateTextHelpModal— new hotkey row: Shift + drag → select multiple warshipsPlease complete the following:
UI update
Mouse + Keyboard
CleanShot.2026-04-15.at.14.48.02.mp4
Touch
IMG_2237.2.MOV
Please put your Discord username so you can be contacted if a bug or regression is found:
fghjk_60845