UI overhaul: split/classic layouts, control panel, events, attacks…#3359
Open
hkio120 wants to merge 2 commits intoopenfrontio:mainfrom
Open
UI overhaul: split/classic layouts, control panel, events, attacks…#3359hkio120 wants to merge 2 commits intoopenfrontio:mainfrom
hkio120 wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
hkio120
changed the title
Contributor
WalkthroughAdds a configurable Classic vs Split in-game layout, new DOM panels and persistent UI toggles, expanded localization, and multiple graphics-layer changes (events mode filtering, attacks anchors, control panel income/regen logic, hover overlays), plus related renderer and configuration adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/lang/en.json (1)
8-25:⚠️ Potential issue | 🟠 MajorRestore removed
commonkeys still used by UI.
common.pasteandcommon.enabledare still referenced in join-lobby flows, so their removal from thecommonblock causes missing translation output at runtime.🩹 Suggested key restoration
"common": { "close": "Close", "copy": "Copy", + "paste": "Paste", "back": "Back", "available": "Available", @@ "target_dead_note": "You can't send resources to an eliminated player.", "none": "None", + "enabled": "Enabled", "copied": "Copied!", "click_to_copy": "Click to copy" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/en.json` around lines 8 - 25, Restore the missing translation keys under the common block by re-adding "paste" and "enabled" (i.e., common.paste and common.enabled) with appropriate English strings (for example "Paste" and "Enabled") so join-lobby flows can resolve translations; update the resources/lang/en.json common object to include these keys alongside existing entries and ensure JSON syntax remains valid (commas, quoting) after insertion.
🧹 Nitpick comments (4)
src/client/graphics/layers/UnitDisplay.ts (1)
281-287: Remove dead template output and duplicate class token.Line 281 renders an empty template (
hovered ? html\` : null) with no visible effect, androunded-sm` is duplicated in the class string on Lines 285 and 287.♻️ Proposed cleanup
- ${hovered ? html`` : null} <div class="${this.canBuild(unitType) ? "" - : "opacity-40"} border border-slate-500 rounded-sm pr-1 pb-0.5 flex items-center gap-1 cursor-pointer + : "opacity-40"} border border-slate-500 pr-1 pb-0.5 flex items-center gap-1 cursor-pointer ${selected ? "hover:bg-gray-400/10" : "hover:bg-gray-800"} rounded-sm text-white text-xs ${selected ? "bg-slate-400/20" : ""}"🤖 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 281 - 287, In UnitDisplay.ts (in the render template where hovered, selected, and canBuild(unitType) are used) remove the dead template expression "hovered ? html`` : null" and deduplicate the "rounded-sm" class token in the big class string (used alongside the conditional from canBuild(unitType) and selected) so the markup no longer outputs an empty template and the class attribute contains "rounded-sm" only once; keep the rest of the conditional class logic (opacity-40, hover:bg-*, bg-slate-400/20, text-white, text-xs, border classes) intact.src/client/graphics/layers/PlayerInfoOverlay.ts (1)
113-123: Gate reactive cursor updates to when compact hover is actually used.Lines 114-115 update reactive state on every mouse move before the throttle branch, which can cause unnecessary re-renders on a very hot path.
⚡ Suggested gating
private onMouseEvent(event: MouseMoveEvent) { - this.hoverScreenX = event.x; - this.hoverScreenY = event.y; + const shouldTrackPointer = + this._isInfoVisible && + this.player !== null && + this.unit === null && + this.isDesktopViewport() && + this.isMiniHoverOverlayEnabled(); + if (shouldTrackPointer) { + this.hoverScreenX = event.x; + this.hoverScreenY = event.y; + } const now = Date.now(); if (now - this.lastMouseUpdate < 100) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PlayerInfoOverlay.ts` around lines 113 - 123, The code updates hoverScreenX/hoverScreenY every mouse move before throttling; change onMouseEvent so it first checks whether compact hover mode is active (e.g. a flag/method like this.isCompactHoverEnabled or this.compactHoverEnabled) and only then performs the reactive assignments and throttling logic; keep the Date.now() throttle using lastMouseUpdate and call this.maybeShow(x,y) as before, but move hoverScreenX/hoverScreenY assignments to occur inside the compact-hover branch after the throttle check so non-compact cursor movement does not trigger reactive updates.src/client/graphics/layers/ControlPanel.ts (2)
624-644: Extract touch drag overlay into a helper method.This block (lines 624-644) is nearly identical to lines 732-752 in
render(). Extracting it to a private method likerenderTouchDragOverlay()would reduce duplication and simplify both render paths.♻️ Proposed extraction
+ private renderTouchDragOverlay() { + if (!this._touchDragging) return ""; + 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="text-red-400 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 bg-red-500 rounded-full" + style="height: ${this.attackRatio * 100}%" + ></div> + </div> + </div> + `; + }Then replace both occurrences with
${this.renderTouchDragOverlay()}.🤖 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 624 - 644, The touch-drag overlay HTML is duplicated in render(); extract it into a private helper like renderTouchDragOverlay() that returns the same template and uses the same bindings (_touchDragging, attackRatio, handleBarTouch), then replace both duplicated inline blocks in render() with ${this.renderTouchDragOverlay()} so both paths reuse the single method and remove the duplicated markup.
52-52: Unused state propertyregenPct.This property is declared but never read or written anywhere in the file. Remove it or wire it up if it was intended for something.
🧹 Proposed fix
- `@state`() private regenPct: number = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` at line 52, The property decorated as `@state`() private regenPct: number = 0 in the ControlPanel class is unused; remove this unused state field or implement its intended usage. Locate the ControlPanel class in ControlPanel.ts, delete the regenPct declaration if not needed, or if it should reflect some UI state, connect it to the corresponding update logic (e.g., update it inside methods that compute regen percentage or bind it to the template/render function) and ensure any references use this.regenPct consistently; also remove any unused imports triggered by this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 384-401: The three control buttons (ids layout-switch-button,
mini-hover-toggle-button, control-panel-style-button) currently hardcode English
labels in HTML and are later updated in JS (around the code that updates them at
~417-453); change both the static HTML text and all JS updates to use the app's
established i18n mechanism (the same translation function/utility used
elsewhere) and reference new translation keys (e.g., controls.layout.split,
controls.miniHover.on, controls.panel.split) instead of literal strings so the
labels render localized text for all locales; ensure keys are added to locale
files and the JS that toggles/changes labels calls the translation function
rather than setting raw English text.
In `@resources/lang/en.json`:
- Around line 869-871: The locale entries "slider_tooltip", "aria_slider", and
"capacity_note" use double-brace placeholders (e.g., "{{percent}}") which is
inconsistent with the project's single-brace convention; update those keys to
use single braces (e.g., "{percent}") and also change the other occurrence
referenced at line 877 to single-brace placeholders so all locale interpolations
use the same "{...}" format.
In `@resources/lang/fr.json`:
- Around line 721-722: Revert the direct edits to resources/lang/fr.json for the
keys "received_gold_from_conquest" and "conquered_no_gold" (remove those two
added/changed entries) and instead add these translation updates via the
dedicated translation workflow: create or include them in a translation PR
titled "mls" authored by Aotumori (or submit the source strings in en.json if
missing) so that only the dedicated translation PR updates non-English locale
files.
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 955-957: The static mode headers "Alliance Requests" and "Trade
Information" in the EventsDisplay component are hardcoded; replace them with the
component's i18n translation calls (use the same translate function used
elsewhere in this file, e.g., t(...) or i18n.t(...)) and reference new keys like
events.allianceRequests and events.tradeInformation so the headers are localized
consistently with the rest of the component (update both occurrences that render
those header divs).
---
Outside diff comments:
In `@resources/lang/en.json`:
- Around line 8-25: Restore the missing translation keys under the common block
by re-adding "paste" and "enabled" (i.e., common.paste and common.enabled) with
appropriate English strings (for example "Paste" and "Enabled") so join-lobby
flows can resolve translations; update the resources/lang/en.json common object
to include these keys alongside existing entries and ensure JSON syntax remains
valid (commas, quoting) after insertion.
---
Nitpick comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 624-644: The touch-drag overlay HTML is duplicated in render();
extract it into a private helper like renderTouchDragOverlay() that returns the
same template and uses the same bindings (_touchDragging, attackRatio,
handleBarTouch), then replace both duplicated inline blocks in render() with
${this.renderTouchDragOverlay()} so both paths reuse the single method and
remove the duplicated markup.
- Line 52: The property decorated as `@state`() private regenPct: number = 0 in
the ControlPanel class is unused; remove this unused state field or implement
its intended usage. Locate the ControlPanel class in ControlPanel.ts, delete the
regenPct declaration if not needed, or if it should reflect some UI state,
connect it to the corresponding update logic (e.g., update it inside methods
that compute regen percentage or bind it to the template/render function) and
ensure any references use this.regenPct consistently; also remove any unused
imports triggered by this change.
In `@src/client/graphics/layers/PlayerInfoOverlay.ts`:
- Around line 113-123: The code updates hoverScreenX/hoverScreenY every mouse
move before throttling; change onMouseEvent so it first checks whether compact
hover mode is active (e.g. a flag/method like this.isCompactHoverEnabled or
this.compactHoverEnabled) and only then performs the reactive assignments and
throttling logic; keep the Date.now() throttle using lastMouseUpdate and call
this.maybeShow(x,y) as before, but move hoverScreenX/hoverScreenY assignments to
occur inside the compact-hover branch after the throttle check so non-compact
cursor movement does not trigger reactive updates.
In `@src/client/graphics/layers/UnitDisplay.ts`:
- Around line 281-287: In UnitDisplay.ts (in the render template where hovered,
selected, and canBuild(unitType) are used) remove the dead template expression
"hovered ? html`` : null" and deduplicate the "rounded-sm" class token in the
big class string (used alongside the conditional from canBuild(unitType) and
selected) so the markup no longer outputs an empty template and the class
attribute contains "rounded-sm" only once; keep the rest of the conditional
class logic (opacity-40, hover:bg-*, bg-slate-400/20, text-white, text-xs,
border classes) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b53ab94-67d1-4b0d-bda1-55c5fd5c5a64
📒 Files selected for processing (9)
index.htmlresources/lang/en.jsonresources/lang/fr.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/UnitDisplay.ts
Comment on lines
+384
to
+401
| <button | ||
| id="layout-switch-button" | ||
| class="px-3 py-1.5 rounded-md bg-gray-900/80 border border-white/20 text-white text-xs lg:text-sm hover:bg-gray-800/90 transition-colors" | ||
| > | ||
| Layout: Split | ||
| </button> | ||
| <button | ||
| id="mini-hover-toggle-button" | ||
| class="px-3 py-1.5 rounded-md bg-gray-900/80 border border-white/20 text-white text-xs lg:text-sm hover:bg-gray-800/90 transition-colors" | ||
| > | ||
| Mini Hover: On | ||
| </button> | ||
| <button | ||
| id="control-panel-style-button" | ||
| class="px-3 py-1.5 rounded-md bg-gray-900/80 border border-white/20 text-white text-xs lg:text-sm hover:bg-gray-800/90 transition-colors" | ||
| > | ||
| Panel: Split | ||
| </button> |
Contributor
There was a problem hiding this comment.
New layout-control labels are not localized.
Texts set on Lines 384-401 and updated in JS on Lines 417-453 are hardcoded in English. In other locales, these controls will still show English strings. Please route them through translation keys like the rest of the UI.
Also applies to: 417-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 384 - 401, The three control buttons (ids
layout-switch-button, mini-hover-toggle-button, control-panel-style-button)
currently hardcode English labels in HTML and are later updated in JS (around
the code that updates them at ~417-453); change both the static HTML text and
all JS updates to use the app's established i18n mechanism (the same translation
function/utility used elsewhere) and reference new translation keys (e.g.,
controls.layout.split, controls.miniHover.on, controls.panel.split) instead of
literal strings so the labels render localized text for all locales; ensure keys
are added to locale files and the JS that toggles/changes labels calls the
translation function rather than setting raw English text.
Comment on lines
+869
to
+871
| "slider_tooltip": "{{percent}}% • {{amount}}", | ||
| "aria_slider": "Troops slider", | ||
| "capacity_note": "Receiver can accept only {amount} right now." | ||
| "capacity_note": "Receiver can accept only {{amount}} right now." |
Contributor
There was a problem hiding this comment.
Use consistent single-brace placeholders in tooltips.
Lines 869-871 and 877 use {{...}}, but existing locale placeholders use {...}. This inconsistency can break interpolation and show raw braces to users.
🩹 Placeholder consistency fix
- "slider_tooltip": "{{percent}}% • {{amount}}",
+ "slider_tooltip": "{percent}% • {amount}",
@@
- "capacity_note": "Receiver can accept only {{amount}} right now."
+ "capacity_note": "Receiver can accept only {amount} right now."
@@
- "slider_tooltip": "{{percent}}% • {{amount}}"
+ "slider_tooltip": "{percent}% • {amount}"Also applies to: 877-877
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/en.json` around lines 869 - 871, The locale entries
"slider_tooltip", "aria_slider", and "capacity_note" use double-brace
placeholders (e.g., "{{percent}}") which is inconsistent with the project's
single-brace convention; update those keys to use single braces (e.g.,
"{percent}") and also change the other occurrence referenced at line 877 to
single-brace placeholders so all locale interpolations use the same "{...}"
format.
Comment on lines
+721
to
+722
| "received_gold_from_conquest": "{gold} ors reçu en conquérant {name}", | ||
| "conquered_no_gold": "{name} a été conquis, mais il n'y avait aucun or à récupérer", |
Contributor
There was a problem hiding this comment.
Move fr.json updates to the dedicated translation flow.
Adding keys in resources/lang/fr.json on Lines 721-722 should be done in the dedicated translation PR process, not in a regular feature PR.
Based on learnings: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/fr.json` around lines 721 - 722, Revert the direct edits to
resources/lang/fr.json for the keys "received_gold_from_conquest" and
"conquered_no_gold" (remove those two added/changed entries) and instead add
these translation updates via the dedicated translation workflow: create or
include them in a translation PR titled "mls" authored by Aotumori (or submit
the source strings in en.json if missing) so that only the dedicated translation
PR updates non-English locale files.
Comment on lines
+955
to
+957
| <div class="text-[11px] uppercase tracking-wide text-gray-300 mb-1"> | ||
| Alliance Requests | ||
| </div> |
Contributor
There was a problem hiding this comment.
Mode headers are hardcoded instead of translated.
Alliance Requests and Trade Information (Lines 955-957, 1041-1049) should use translation keys to match the rest of this component’s i18n behavior.
Also applies to: 1041-1049
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/EventsDisplay.ts` around lines 955 - 957, The
static mode headers "Alliance Requests" and "Trade Information" in the
EventsDisplay component are hardcoded; replace them with the component's i18n
translation calls (use the same translate function used elsewhere in this file,
e.g., t(...) or i18n.t(...)) and reference new keys like events.allianceRequests
and events.tradeInformation so the headers are localized consistently with the
rest of the component (update both occurrences that render those header divs).
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
added a commit
that referenced
this pull request
Mar 7, 2026
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. Also made the attack ratio bar persistent on mobile <img width="618" height="216" alt="Screenshot 2026-03-06 at 2 06 34 PM" src="https://github.com/user-attachments/assets/34b041c1-d78b-46b5-a7ab-f2a44145a7e2" /> <img width="941" height="343" alt="Screenshot 2026-03-06 at 2 06 55 PM" src="https://github.com/user-attachments/assets/1e3b026c-8eb2-407c-be38-0e71e1ae426c" /> <img width="562" height="228" alt="Screenshot 2026-03-06 at 4 11 20 PM" src="https://github.com/user-attachments/assets/56eac49f-c8a4-4ac1-a60a-f1bcb2fad2d0" /> <img width="939" height="357" alt="Screenshot 2026-03-06 at 4 11 32 PM" src="https://github.com/user-attachments/assets/eb5591d5-3cc2-4182-944b-3a4b0b76852a" /> ## 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 ## 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/configuration/DefaultConfig.ts (1)
630-644: Caching and formula changes look correct.Good optimization to cache
attackerTilesOwnedinstead of callingattacker.numTilesOwned()multiple times. The math for bothlargeAttackBonusandlargeAttackerSpeedBonusis sound:
largeAttackBonusdecreases as attacker grows larger (penalizes large attackers)largeAttackerSpeedBonusdecreases asattackerSpeedgrows (slows expansion for large attackers)- Division by
attackerSpeedis safe since it's always >= 1Consider extracting the threshold
100_000into a named constant (like the existingDEFENSE_DEBUFF_MIDPOINT) for clarity and easier tuning.🔧 Optional: Extract magic number to named constant
const DEFENSE_DEBUFF_MIDPOINT = 150_000; const DEFENSE_DEBUFF_DECAY_RATE = Math.LN2 / 50000; const DEFAULT_SPAWN_IMMUNITY_TICKS = 5 * 10; +const LARGE_ATTACKER_THRESHOLD = 100_000;Then replace
100_000references withLARGE_ATTACKER_THRESHOLD.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 630 - 644, The code repeats the magic number 100_000 when computing largeAttackBonus and largeAttackerSpeedBonus; extract that value into a named constant (e.g., LARGE_ATTACKER_THRESHOLD) and replace the hardcoded 100_000 uses in the attackerTilesOwned check and the attackerSpeed formula; update any related documentation/comments and preferably colocate the constant near DEFENSE_DEBUFF_MIDPOINT so tuning is easier, while leaving the logic in the functions/variables attacker.numTilesOwned(), attackerTilesOwned, largeAttackBonus, and largeAttackerSpeedBonus unchanged.src/client/graphics/layers/ControlPanel.ts (1)
145-148: Typo in method name:setVisibileshould besetVisible.The method name has a typo (double 'i'). Consider renaming to
setVisiblefor clarity, though this would require updating callers.🔤 Suggested fix
- setVisibile(isVisible: boolean) { + setVisible(isVisible: boolean) { this._isVisible = isVisible; this.requestUpdate(); }And update the callers at lines 101 and 106.
🤖 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 145 - 148, Rename the misnamed method setVisibile to setVisible in the ControlPanel class and update all its callers to the new name; inside the method keep the same behavior (assign this._isVisible = isVisible; this.requestUpdate();) and ensure any imports/usages referencing setVisibile are changed to setVisible so callers continue to compile and the visibility property (_isVisible) and requestUpdate usage remain intact.src/client/graphics/layers/PlayerInfoOverlay.ts (1)
503-557: Duplicated troop optimization logic between PlayerInfoOverlay and ControlPanel.The methods
predictedTroopIncreaseRate,getOptimalRegenPoint,lerpColor, andtroopFillColorare nearly identical in both files. Consider extracting these to a shared utility module to avoid drift and reduce maintenance burden.Also note: ControlPanel caches the optimal regen point (
_cachedOptimalMaxTroops, etc.), but PlayerInfoOverlay recalculates on every call. SincemaxTroopschanges infrequently, caching would improve performance here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PlayerInfoOverlay.ts` around lines 503 - 557, The four nearly-identical functions predictedTroopIncreaseRate, getOptimalRegenPoint, lerpColor, and troopFillColor should be extracted into a shared utility (e.g., troopUtils) and PlayerInfoOverlay should call those helpers instead of duplicating logic; move the implementations to the new module and import them here, and add a caching mechanism similar to ControlPanel's (_cachedOptimalMaxTroops / cached value + lastMaxTroops) so getOptimalRegenPoint is not recomputed every render when maxTroops is unchanged. Ensure you preserve the same function signatures (predictedTroopIncreaseRate(troops,maxTroops), getOptimalRegenPoint(maxTroops), lerpColor(from,to,t), troopFillColor(troops,maxTroops)) and update all references in PlayerInfoOverlay to use the shared exports.
🤖 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/AttacksDisplay.ts`:
- Around line 161-189: resolveOutgoingAttackAnchor starts an async
attackAveragePosition lookup and may write anchors after the attack has been
removed by refreshOutgoingAttackAnchors, causing stale entries; after the await
(before setting outgoingAttackAnchors) check that the attack is still active
(e.g. via the same source refreshOutgoingAttackAnchors uses—call/game lookup
like this.game.attackById(attack.id) or this.attacks.has(attack.id)) and only
write the anchor if that check passes; keep the existing
pendingOutgoingAnchorLookups removal and requestUpdate in finally.
---
Nitpick comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 145-148: Rename the misnamed method setVisibile to setVisible in
the ControlPanel class and update all its callers to the new name; inside the
method keep the same behavior (assign this._isVisible = isVisible;
this.requestUpdate();) and ensure any imports/usages referencing setVisibile are
changed to setVisible so callers continue to compile and the visibility property
(_isVisible) and requestUpdate usage remain intact.
In `@src/client/graphics/layers/PlayerInfoOverlay.ts`:
- Around line 503-557: The four nearly-identical functions
predictedTroopIncreaseRate, getOptimalRegenPoint, lerpColor, and troopFillColor
should be extracted into a shared utility (e.g., troopUtils) and
PlayerInfoOverlay should call those helpers instead of duplicating logic; move
the implementations to the new module and import them here, and add a caching
mechanism similar to ControlPanel's (_cachedOptimalMaxTroops / cached value +
lastMaxTroops) so getOptimalRegenPoint is not recomputed every render when
maxTroops is unchanged. Ensure you preserve the same function signatures
(predictedTroopIncreaseRate(troops,maxTroops), getOptimalRegenPoint(maxTroops),
lerpColor(from,to,t), troopFillColor(troops,maxTroops)) and update all
references in PlayerInfoOverlay to use the shared exports.
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 630-644: The code repeats the magic number 100_000 when computing
largeAttackBonus and largeAttackerSpeedBonus; extract that value into a named
constant (e.g., LARGE_ATTACKER_THRESHOLD) and replace the hardcoded 100_000 uses
in the attackerTilesOwned check and the attackerSpeed formula; update any
related documentation/comments and preferably colocate the constant near
DEFENSE_DEBUFF_MIDPOINT so tuning is easier, while leaving the logic in the
functions/variables attacker.numTilesOwned(), attackerTilesOwned,
largeAttackBonus, and largeAttackerSpeedBonus 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: efb17300-bd4f-40cd-939f-3587f7ca5de4
📒 Files selected for processing (5)
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/configuration/DefaultConfig.ts
Comment on lines
+161
to
+189
| private async resolveOutgoingAttackAnchor(attack: AttackUpdate) { | ||
| this.pendingOutgoingAnchorLookups.add(attack.id); | ||
| try { | ||
| const attacker = this.game.playerBySmallID(attack.attackerID); | ||
| if (!(attacker instanceof PlayerView)) { | ||
| this.outgoingAttackAnchors.set(attack.id, null); | ||
| return; | ||
| } | ||
|
|
||
| const averagePosition = await attacker.attackAveragePosition( | ||
| attack.attackerID, | ||
| attack.id, | ||
| ); | ||
|
|
||
| if (averagePosition === null) { | ||
| this.outgoingAttackAnchors.set(attack.id, null); | ||
| } else { | ||
| this.outgoingAttackAnchors.set(attack.id, { | ||
| x: averagePosition.x, | ||
| y: averagePosition.y, | ||
| }); | ||
| } | ||
| } catch { | ||
| this.outgoingAttackAnchors.set(attack.id, null); | ||
| } finally { | ||
| this.pendingOutgoingAnchorLookups.delete(attack.id); | ||
| this.requestUpdate(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Async anchor resolution can leave stale data if attacks end mid-flight.
The resolveOutgoingAttackAnchor method starts an async lookup, but if the attack ends before attackAveragePosition returns, the anchor is still written to outgoingAttackAnchors. The cleanup in refreshOutgoingAttackAnchors runs before the async call completes, so the next tick will re-add the stale entry to the map.
Consider checking if the attack is still active before writing to the map:
🛠️ Suggested fix
const averagePosition = await attacker.attackAveragePosition(
attack.attackerID,
attack.id,
);
+ // Check if attack still exists before storing anchor
+ const stillActive = this.outgoingAttacks.some((a) => a.id === attack.id);
+ if (!stillActive) {
+ return;
+ }
+
if (averagePosition === null) {
this.outgoingAttackAnchors.set(attack.id, null);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 161 - 189,
resolveOutgoingAttackAnchor starts an async attackAveragePosition lookup and may
write anchors after the attack has been removed by refreshOutgoingAttackAnchors,
causing stale entries; after the await (before setting outgoingAttackAnchors)
check that the attack is still active (e.g. via the same source
refreshOutgoingAttackAnchors uses—call/game lookup like
this.game.attackById(attack.id) or this.attacks.has(attack.id)) and only write
the anchor if that check passes; keep the existing pendingOutgoingAnchorLookups
removal and requestUpdate in finally.
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.
Summary
This PR introduces a full in-game UI rework focused on layout flexibility, clearer information hierarchy, and improved readability during combat-heavy gameplay.
What’s New (vs main)
• Added a switchable in-game layout:
◦ Layout: Split
◦ Layout: Classic
• Added persistent layout setting.
• Updated panel placement logic (control panel, attacks display, side event panels) for each layout.
Files:
• index.html
• src/client/graphics/GameRenderer.ts
• Added a second toggle to switch control panel style independently of layout:
◦ Panel: Split
◦ Panel: Classic
• Added persistent panel-style setting.
• Enables mixed combinations (e.g. classic layout + split panel).
Files:
• index.html
• src/client/graphics/layers/ControlPanel.ts
• Reworked control panel rendering for both styles.
• Improved spacing/alignment for troops, attack ratio, gold, and economy info.
• Added/adjusted gold-per-minute visibility where needed by mode.
Files:
• src/client/graphics/layers/ControlPanel.ts
• Better integration of unit display with panel layout.
• Size/spacing refinements for unit icons and fit behavior.
Files:
• src/client/graphics/layers/UnitDisplay.ts
• index.html
• Added mode-based event display routing (trades, right, alliance_requests, etc.).
• Improved filtering to reduce noisy messages.
• Separated alliance request flow from general event stream where relevant.
Files:
• src/client/graphics/layers/EventsDisplay.ts
• index.html
• Made attacks display more compact/readable.
• Improved vertical positioning relative to control panel, especially in classic layout.
Files:
• src/client/graphics/layers/AttacksDisplay.ts
• index.html
• Added mini-hover overlay toggle and behavior refinements.
Files:
• src/client/graphics/layers/PlayerInfoOverlay.ts
• index.html
• Added missing i18n entries used by new/updated event messages (including conquest gold messages) to avoid raw key rendering.
Files:
• resources/lang/en.json
• resources/lang/fr.json
Why
These changes improve:
• player customization of UI setup,
• combat readability,
• event signal-to-noise ratio,
• consistency across in-game overlays.
Testing
Manual in-game verification performed for:
• all Layout x Panel combinations,
• control panel / attacks display alignment,
• events panel routing and filtering,
• mini-hover toggle and rendering,
• translation rendering for updated event messages.
I DIDNT TESTED ON MOBILE !
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
HulKiora