Conversation
Contributor
WalkthroughReorganizes the bottom HUD into a responsive three-column grid, moves Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
🧹 Nitpick comments (2)
src/client/graphics/layers/ControlPanel.ts (2)
430-432: Missing resize listener for mobile/desktop switch.The
isMobilecheck atwindow.innerWidth < 1024runs only at render time. If a user resizes their browser across the 1024px boundary, the component won't automatically switch between mobile and desktop layouts until a state change triggers a re-render.This matches the
lg:breakpoint (1024px) used inindex.htmlfor the HUD layout, so the behavior is at least consistent. If dynamic resizing is not a priority for this draft PR, this can be addressed later.♻️ Optional: Add resize listener for responsive switching
+ private _isMobile = false; + private _resizeHandler: (() => void) | null = null; + + connectedCallback() { + super.connectedCallback(); + this._resizeHandler = () => { + const isMobile = window.innerWidth < 1024; + if (this._isMobile !== isMobile) { + this._isMobile = isMobile; + this.requestUpdate(); + } + }; + window.addEventListener("resize", this._resizeHandler); + this._resizeHandler(); + } + + disconnectedCallback() { + super.disconnectedCallback(); + if (this._resizeHandler) { + window.removeEventListener("resize", this._resizeHandler); + } + } render() { - const isMobile = window.innerWidth < 1024; return html` <div class="relative pointer-events-auto ${this._isVisible ? "relative w-full text-sm bg-gray-800/70 px-2 py-1.5 shadow-lg rounded-lg backdrop-blur-xs" : "hidden"}" `@contextmenu`=${(e: MouseEvent) => e.preventDefault()} > - ${isMobile ? this.renderMobile() : this.renderDesktop()} + ${this._isMobile ? this.renderMobile() : this.renderDesktop()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 430 - 432, The render() method in ControlPanel uses a one-time isMobile = window.innerWidth < 1024 check, so add a resize observer to update component state when crossing the 1024px breakpoint: in the component (ControlPanel) add a reactive property (e.g., isMobile) and initialize it from window.innerWidth in connectedCallback, attach a window.resize listener or window.matchMedia('(min-width:1024px)') listener that updates the isMobile property (thus forcing re-render), and remove the listener in disconnectedCallback; ensure the render() method reads this.isMobile instead of a local variable so the component switches layouts on resize.
406-426: Duplicate touch overlay code with color variations.The touch dragging overlay appears in both
renderMobile()(lines 406-426, blue) andrender()for desktop (lines 440-460, red). The structure is identical except for color classes.Consider extracting to a helper method with a color parameter to reduce duplication:
♻️ Optional: Extract overlay to helper method
+ private renderTouchOverlay(color: "blue" | "red") { + const textColor = color === "blue" ? "text-blue-400" : "text-red-400"; + const bgColor = color === "blue" ? "bg-blue-500" : "bg-red-500"; + return html` + <div + class="absolute bottom-full right-0 flex flex-col items-center pointer-events-auto z-[10000] bg-gray-800/70 backdrop-blur-xs rounded-tl-lg sm:rounded-lg p-2 w-12" + style="height: 50vh;" + `@touchstart`=${(e: TouchEvent) => this.handleBarTouch(e)} + > + <span class="${textColor} text-sm font-bold mb-1" translate="no" + >${(this.attackRatio * 100).toFixed(0)}%</span + > + <div + class="attack-drag-bar flex-1 w-3 bg-white/20 rounded-full relative overflow-hidden" + > + <div + class="absolute bottom-0 w-full ${bgColor} rounded-full" + style="height: ${this.attackRatio * 100}%" + ></div> + </div> + </div> + `; + }Then use
${this._touchDragging ? this.renderTouchOverlay("blue") : ""}in renderMobile() and${!isMobile && this._touchDragging ? this.renderTouchOverlay("red") : ""}in render().Also applies to: 440-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 406 - 426, The touch-dragging overlay HTML is duplicated between renderMobile() and render() (guarded by this._touchDragging) with only color-class differences; extract this into a helper method (e.g., renderTouchOverlay(color: string) or renderTouchOverlay(colorClass: string)) that returns the shared template and accepts a color modifier for the differing classes, then replace the duplicated blocks in renderMobile() and render() with calls like this._touchDragging ? this.renderTouchOverlay("blue") : "" and !isMobile && this._touchDragging ? this.renderTouchOverlay("red") : "" respectively, ensuring you reuse existing symbols (_touchDragging, handleBarTouch, attackRatio) inside the new helper.
🤖 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/ControlPanel.ts`:
- Around line 430-432: The render() method in ControlPanel uses a one-time
isMobile = window.innerWidth < 1024 check, so add a resize observer to update
component state when crossing the 1024px breakpoint: in the component
(ControlPanel) add a reactive property (e.g., isMobile) and initialize it from
window.innerWidth in connectedCallback, attach a window.resize listener or
window.matchMedia('(min-width:1024px)') listener that updates the isMobile
property (thus forcing re-render), and remove the listener in
disconnectedCallback; ensure the render() method reads this.isMobile instead of
a local variable so the component switches layouts on resize.
- Around line 406-426: The touch-dragging overlay HTML is duplicated between
renderMobile() and render() (guarded by this._touchDragging) with only
color-class differences; extract this into a helper method (e.g.,
renderTouchOverlay(color: string) or renderTouchOverlay(colorClass: string))
that returns the shared template and accepts a color modifier for the differing
classes, then replace the duplicated blocks in renderMobile() and render() with
calls like this._touchDragging ? this.renderTouchOverlay("blue") : "" and
!isMobile && this._touchDragging ? this.renderTouchOverlay("red") : ""
respectively, ensuring you reuse existing symbols (_touchDragging,
handleBarTouch, attackRatio) inside the new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af53af0d-5806-4fad-b461-b7b74d60165e
📒 Files selected for processing (4)
index.htmlsrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/UnitDisplay.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ControlPanel.ts`:
- Around line 408-423: The touchstart handler is on the outer wrapper, causing
touches on the percent label/padding to call handleBarTouch which then uses
setRatioFromTouch (which measures .attack-drag-bar) and snaps ratio incorrectly;
move the `@touchstart` handler from the outer div to the element with class
"attack-drag-bar" (and replicate the same change in the desktop popover), or
alternatively have handleBarTouch early-return if the touch is outside the
bounding rect of .attack-drag-bar by checking event.target or hit-testing
against the .attack-drag-bar DOMRect before calling setRatioFromTouch.
- Around line 368-370: render() switches to renderMobile() by width but the
attack control only listens for touchstart (handleAttackTouchStart), leaving
resized desktops without a way to change uiState.attackRatio and causing
PlayerActionHandler to keep the old ratio; add a non-touch fallback by wiring a
pointer/click event (e.g., `@click` or `@pointerdown`) to the same logic or a new
handler (handleAttackClick/handleAttackPointer) that reuses
handleAttackTouchStart's behavior to open the mobile attack UI and update
uiState.attackRatio, and apply the same change to the analogous mobile controls
around the 430-440 block so both touch and mouse/pointer input paths update the
attack slider/state.
In `@src/client/graphics/layers/UnitDisplay.ts`:
- Around line 196-201: The hover toggle logic is missing a MIRV-specific branch
so MIRV currently highlights itself instead of its real dependency
(MissileSilo); update the switch that dispatches ToggleStructureEvent to add a
case for UnitType.MIRV that calls ToggleStructureEvent([UnitType.MissileSilo])
(the same dependency checked by canBuild()), keeping existing behavior for other
unit types and using the same event path used for other structure highlight
cases; locate this logic near renderUnitItem usages and the hover switch
handling ToggleStructureEvent to add the MIRV branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff6376b5-ed5a-4b4c-81fe-4eb7fac30ad1
📒 Files selected for processing (5)
index.htmlsrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/UnitDisplay.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/AttacksDisplay.ts
Comment on lines
+196
to
+201
| ${this.renderUnitItem( | ||
| mirvIcon, | ||
| null, | ||
| UnitType.MIRV, | ||
| "mirv", | ||
| this.keybinds["buildMIRV"]?.key ?? "0", |
Contributor
There was a problem hiding this comment.
MIRV still needs the launcher highlight path.
Adding MIRV here is incomplete unless the hover ToggleStructureEvent switch below also gets a MIRV branch. Right now MIRV falls back to ToggleStructureEvent([UnitType.MIRV]), even though canBuild() says its real dependency is UnitType.MissileSilo, so the hover hint points at the wrong thing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/UnitDisplay.ts` around lines 196 - 201, The hover
toggle logic is missing a MIRV-specific branch so MIRV currently highlights
itself instead of its real dependency (MissileSilo); update the switch that
dispatches ToggleStructureEvent to add a case for UnitType.MIRV that calls
ToggleStructureEvent([UnitType.MissileSilo]) (the same dependency checked by
canBuild()), keeping existing behavior for other unit types and using the same
event path used for other structure highlight cases; locate this logic near
renderUnitItem usages and the hover switch handling ToggleStructureEvent to add
the MIRV branch.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/client/graphics/layers/UnitDisplay.ts (1)
276-291:⚠️ Potential issue | 🟡 MinorMIRV still needs a hover highlight case for MissileSilo.
The switch statement handles AtomBomb and HydrogenBomb together (highlighting MissileSilo + SAMLauncher), but MIRV falls through to the default case. Since MIRV also requires a MissileSilo to build (see
canBuild()at lines 82-87), it should highlight the same structures as the other nukes.Proposed fix
switch (unitType) { case UnitType.AtomBomb: case UnitType.HydrogenBomb: + case UnitType.MIRV: this.eventBus?.emit( new ToggleStructureEvent([ UnitType.MissileSilo, UnitType.SAMLauncher, ]), ); break; case UnitType.Warship:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitDisplay.ts` around lines 276 - 291, The switch in UnitDisplay handling hover highlights omits MIRV so it falls to default; add a case for UnitType.MIRV alongside UnitType.AtomBomb and UnitType.HydrogenBomb so the handler emits the same ToggleStructureEvent([UnitType.MissileSilo, UnitType.SAMLauncher]) as the other nukes (modify the switch that checks unitType in UnitDisplay.ts to include UnitType.MIRV in the same branch that calls this.eventBus?.emit(new ToggleStructureEvent([UnitType.MissileSilo, UnitType.SAMLauncher]))).src/client/graphics/layers/ControlPanel.ts (1)
453-462:⚠️ Potential issue | 🟠 MajorDesktop path still needs a mouse/click fallback for the attack ratio control.
Line 454 switches to
renderMobile()purely based on width (window.innerWidth < 1024), but the desktop attack ratio control (lines 346-364) only has@touchstart. On a resized desktop browser below 1024px, users with a mouse cannot change the attack ratio.Consider using
window.matchMedia("(pointer: coarse)").matchesto detect actual touch capability, or add a@clickhandler alongside@touchstart.Proposed approach
- const isMobile = window.innerWidth < 1024; + const isCompactLayout = window.innerWidth < 1024; + const useTouchUi = isCompactLayout && window.matchMedia("(pointer: coarse)").matches; return html` ... - ${isMobile ? this.renderMobile() : this.renderDesktop()} + ${useTouchUi ? this.renderMobile() : this.renderDesktop()}Alternatively, wire a
@clickhandler to open the attack bar for mouse users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 453 - 462, The desktop branch uses renderDesktop() selected by window.innerWidth but the attack-ratio control only listens for `@touchstart` (so mouse users on small/resized screens cannot open it); update the logic in render()/renderDesktop() to detect input capability (e.g., window.matchMedia("(pointer: coarse)").matches) or add a mouse fallback by attaching a `@click` handler alongside the existing `@touchstart` on the attack-ratio control; ensure the same handler/function that opens/manages the attack bar (the callback used by the `@touchstart`) is reused for `@click` so behavior is identical and no duplicate logic is introduced.
🤖 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/ControlPanel.ts`:
- Around line 453-462: The desktop branch uses renderDesktop() selected by
window.innerWidth but the attack-ratio control only listens for `@touchstart` (so
mouse users on small/resized screens cannot open it); update the logic in
render()/renderDesktop() to detect input capability (e.g.,
window.matchMedia("(pointer: coarse)").matches) or add a mouse fallback by
attaching a `@click` handler alongside the existing `@touchstart` on the
attack-ratio control; ensure the same handler/function that opens/manages the
attack bar (the callback used by the `@touchstart`) is reused for `@click` so
behavior is identical and no duplicate logic is introduced.
In `@src/client/graphics/layers/UnitDisplay.ts`:
- Around line 276-291: The switch in UnitDisplay handling hover highlights omits
MIRV so it falls to default; add a case for UnitType.MIRV alongside
UnitType.AtomBomb and UnitType.HydrogenBomb so the handler emits the same
ToggleStructureEvent([UnitType.MissileSilo, UnitType.SAMLauncher]) as the other
nukes (modify the switch that checks unitType in UnitDisplay.ts to include
UnitType.MIRV in the same branch that calls this.eventBus?.emit(new
ToggleStructureEvent([UnitType.MissileSilo, UnitType.SAMLauncher]))).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24a06b08-ae9f-4670-950d-20037831cc44
📒 Files selected for processing (5)
index.htmlsrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/UnitDisplay.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/graphics/layers/ControlPanel.ts (1)
127-131: Consider persisting attackRatio changes to localStorage.
init()reads fromlocalStorage.getItem("settings.attackRatio"), buthandleRatioSliderInput()never writes the updated value back. User-adjusted attack ratios won't persist across sessions.Optional fix to persist changes
private handleRatioSliderInput(e: Event) { const value = Number((e.target as HTMLInputElement).value); this.attackRatio = value / 100; this.onAttackRatioChange(this.attackRatio); + localStorage.setItem("settings.attackRatio", String(this.attackRatio)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 127 - 131, The slider handler handleRatioSliderInput updates this.attackRatio and calls onAttackRatioChange but doesn't persist the new value; modify handleRatioSliderInput to also write the updated value into localStorage (use the same key read in init(), e.g. "settings.attackRatio")—store a string (either the ratio or percentage consistently with init's parsing) so the value persists across sessions and will be picked up by init() on next load.index.html (1)
269-269: Minor: Tailwind v4 supports direct z-index syntax.
z-[200]works, but v4 allowsz-200without brackets. Not urgent - bracket notation remains valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` at line 269, Replace the bracketed z-index token in the element's class attribute to use Tailwind v4's direct syntax: change the token "z-[200]" inside the class string "fixed bottom-0 left-0 w-full z-[200] flex flex-col pointer-events-none sm:flex-row sm:items-end lg:grid lg:grid-cols-[1fr_460px_1fr] lg:items-end min-[1200px]:bottom-4 min-[1200px]:px-4" to "z-200" so the component uses the newer shorthand while preserving the rest of the class list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@index.html`:
- Line 269: Replace the bracketed z-index token in the element's class attribute
to use Tailwind v4's direct syntax: change the token "z-[200]" inside the class
string "fixed bottom-0 left-0 w-full z-[200] flex flex-col pointer-events-none
sm:flex-row sm:items-end lg:grid lg:grid-cols-[1fr_460px_1fr] lg:items-end
min-[1200px]:bottom-4 min-[1200px]:px-4" to "z-200" so the component uses the
newer shorthand while preserving the rest of the class list.
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 127-131: The slider handler handleRatioSliderInput updates
this.attackRatio and calls onAttackRatioChange but doesn't persist the new
value; modify handleRatioSliderInput to also write the updated value into
localStorage (use the same key read in init(), e.g.
"settings.attackRatio")—store a string (either the ratio or percentage
consistently with init's parsing) so the value persists across sessions and will
be picked up by init() on next load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05e65fb3-097a-4353-8fa0-ca14bab02b9a
📒 Files selected for processing (5)
index.htmlsrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/UnitDisplay.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/layers/AttacksDisplay.ts
- src/client/graphics/layers/EventsDisplay.ts
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
1 participant
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.
Relates to #2260
Description:
Inspired by #3359
This PR centers the control panel and combines it with the units display. The reasoning is that the control panel contains the most critical info so it should be in the center of the screen. Combining it with the units display reduces the number of UI components on screen.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan
Co-authored-by: hkio120 111693579+hkio120@users.noreply.github.com