feat: add territory hover highlight setting#3701
Open
luctrate wants to merge 3 commits intoopenfrontio:mainfrom
Open
feat: add territory hover highlight setting#3701luctrate wants to merge 3 commits intoopenfrontio:mainfrom
luctrate wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
Add an opt-in setting to highlight the territory of the player under the cursor, making it easier to identify ownership in large games where colors repeat or look similar. Three modes: Always, On Key Press (default H, rebindable), and Never. When active, hovered player's territory renders at alpha 230 vs the normal 150, giving an immediate visual distinction. Closes openfrontio#1307 Relates to openfrontio#3125, openfrontio#2549 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
Contributor
WalkthroughAdded a territory highlighting feature: new English UI strings, a user setting (never/always/onKeyPress) with default H keybind, input events to emit hold/release, UI controls in settings, and rendering changes to toggle per-tile alpha for highlighted territories. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Input as InputHandler
participant Layer as TerritoryLayer
participant Settings as UserSettings
participant Render as Renderer
User->>Input: Press/Hold highlight key (H)
Input->>Input: set highlightKeyHeld = true
Input->>Layer: Emit TerritoryHighlightKeyEvent(true)
Layer->>Layer: update highlightKeyHeld & compute shouldHighlight
Layer->>Layer: updateHighlightAlpha(oldPlayer, newPlayer)
Layer->>Render: Trigger redraw
User->>Input: Release key / Window blur
Input->>Layer: Emit TerritoryHighlightKeyEvent(false)
Layer->>Layer: clear highlightKeyHeld & clear highlightedTerritory
Layer->>Layer: updateHighlightAlpha(reset)
Layer->>Render: Trigger redraw
User->>Settings: Change territory highlight mode
Settings->>Layer: Broadcast USER_SETTINGS_CHANGED_EVENT:TERRITORY_HIGHLIGHT_KEY
Layer->>Layer: update highlightMode & recompute shouldHighlight
Layer->>Render: Trigger redraw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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.
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/TerritoryLayer.ts (1)
337-344:⚠️ Potential issue | 🟡 MinorToggling alternate view won't refresh the highlight immediately.
shouldHighlightconsidersthis.alternativeView, but theAlternateViewEventhandler only sets the flag — it does not callupdateHighlightedTerritory(). Result: pressing/releasing the alt-view key (withhighlightMode: "never") does not add or clear the highlight until the nextMouseOverEvent. A one-liner to callthis.updateHighlightedTerritory()in theAlternateViewEventhandler keeps state and visuals in sync.🧹 Proposed fix
this.eventBus.on(AlternateViewEvent, (e) => { this.alternativeView = e.alternateView; + this.updateHighlightedTerritory(); });Also applies to: 371-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 337 - 344, The AlternateViewEvent handler mutates this.alternativeView but doesn't refresh visuals, so when shouldHighlight (which checks this.alternativeView) changes the UI won't update until next MouseOverEvent; modify the AlternateViewEvent listener in TerritoryLayer (the eventBus.on(AlternateViewEvent, ...) callback) to call this.updateHighlightedTerritory() immediately after setting this.alternativeView so the highlight is recomputed and rendered; also apply the same fix for the analogous handler referenced around the 371-384 region to keep state and visuals in sync.
🧹 Nitpick comments (3)
src/core/game/UserSettings.ts (1)
226-234: Mirror the getter's union on the setter.The getter already narrows to
"always" | "onKeyPress" | "never", but the setter still takes an arbitrarystring. Tightening the setter's type matches the getter and lets TypeScript catch typos at the call site (e.g., inUserSettingModal.changeTerritoryHighlight).🧹 Suggested change
- setTerritoryHighlight(value: string) { - this.setString(TERRITORY_HIGHLIGHT_KEY, value); - } + setTerritoryHighlight(value: "always" | "onKeyPress" | "never") { + this.setString(TERRITORY_HIGHLIGHT_KEY, value); + }You can also extract a
TerritoryHighlightModetype alias and reuse it inUserSettingModal.tsandTerritoryLayer.tsto keep the three call sites in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 226 - 234, The setter setTerritoryHighlight currently accepts an arbitrary string while the getter territoryHighlight narrows to the union "always" | "onKeyPress" | "never"; change setTerritoryHighlight's signature to accept the same union (or extract a shared type alias like TerritoryHighlightMode and use it for territoryHighlight, setTerritoryHighlight and call sites such as UserSettingModal.changeTerritoryHighlight and TerritoryLayer) and update usages to match so TypeScript can enforce valid values; keep the implementation using this.setString(TERRITORY_HIGHLIGHT_KEY, value).src/client/graphics/layers/TerritoryLayer.ts (1)
350-361: Settings listener has no matchingremoveEventListener.The
globalThislistener added ininit()is never detached. It's unlikely to leak in the common path sinceTerritoryLayerlives as long as the game, but ifinit()is ever re-entered (map reload, dev hot-reload, re-queue) you'll accumulate duplicate handlers and each setting change will trigger anupdateHighlightedTerritoryper copy. Consider storing the handler on the instance and cleaning it up in a disposal hook, mirroring the pattern used inPerformanceOverlay.ts(disconnectedCallback withremoveEventListener).🧹 Suggested shape
+ private onTerritoryHighlightSettingChanged = (e: Event) => { + const value = (e as CustomEvent).detail; + this.highlightMode = + value === "always" || value === "onKeyPress" ? value : "never"; + this.updateHighlightedTerritory(); + }; ... - globalThis.addEventListener?.( - `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}`, - ((e: CustomEvent) => { ... }) as EventListener, - ); + globalThis.addEventListener?.( + `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}`, + this.onTerritoryHighlightSettingChanged, + );(…plus a matching
removeEventListenerwherever the layer is torn down.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 350 - 361, The globalThis listener added in init() for `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}` is never removed; store the EventListener callback on the TerritoryLayer instance (e.g. this._settingsListener) when registering it in init(), reuse that named function to set highlightMode and call updateHighlightedTerritory(), and remove it in the layer teardown (disconnectedCallback or the layer's dispose/cleanup method) using globalThis.removeEventListener with the same event name and the stored handler, mirroring the pattern in PerformanceOverlay.ts to prevent duplicate handlers on re-init.src/client/UserSettingModal.ts (1)
312-317: Narrow the accepted value to the three known modes.
e.detail.valuecan in theory be any string here, which then flows through tosetTerritoryHighlight. Validating it against the known modes keeps bad values out oflocalStorageand makes the intent explicit. A small typed union fits here nicely.🧹 Suggested tightening
- private changeTerritoryHighlight(e: CustomEvent<{ value: number | string }>) { - const value = e.detail?.value; - if (typeof value !== "string") return; - this.userSettings.setTerritoryHighlight(value); - this.requestUpdate(); - } + private changeTerritoryHighlight(e: CustomEvent<{ value: number | string }>) { + const value = e.detail?.value; + if (value !== "never" && value !== "always" && value !== "onKeyPress") { + return; + } + this.userSettings.setTerritoryHighlight(value); + this.requestUpdate(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/UserSettingModal.ts` around lines 312 - 317, changeTerritoryHighlight currently accepts any string and forwards it into userSettings.setTerritoryHighlight; restrict accepted values to the three known territory highlight modes by defining a tight typed union (e.g. type TerritoryMode = 'modeA' | 'modeB' | 'modeC') and validate e.detail.value against that set before calling this.userSettings.setTerritoryHighlight. Inside changeTerritoryHighlight, ensure the value is a string, narrow it to the union (or check membership in an allowedModes array), and only call setTerritoryHighlight and requestUpdate when it matches; otherwise ignore or handle invalid input gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/lang/en.json`:
- Line 576: The string "territory_highlight_on_key" currently hard-codes
"default H" which becomes inaccurate after key rebinding; update this resource
by either removing the hard-coded key hint (e.g., change to "On Key Press
(customizable via key settings)") or converting it to an ICU-style string with a
placeholder like "{key}" and ensure the code that renders the label supplies the
current binding for highlightTerritory; locate the "territory_highlight_on_key"
entry in the locale JSON and change it accordingly and update the UI code that
reads this key to pass the current key binding if you choose the placeholder
approach.
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 412-432: updateHighlightAlpha currently scans every tile via
forEachTile on each hover causing lag; instead maintain a synchronous per-player
tile cache updated only when ownership changes (e.g., update cache inside
conquest/relinquish handlers or in methods that modify ownership), then change
updateHighlightAlpha to iterate only the oldPlayer/newPlayer tile lists from
that cache (use PlayerView.id() as the key) and update imageData.data[offset+3]
for those tiles; as an alternative interim, throttle pointer hover updates to
reduce frequency until the cache is implemented.
---
Outside diff comments:
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 337-344: The AlternateViewEvent handler mutates
this.alternativeView but doesn't refresh visuals, so when shouldHighlight (which
checks this.alternativeView) changes the UI won't update until next
MouseOverEvent; modify the AlternateViewEvent listener in TerritoryLayer (the
eventBus.on(AlternateViewEvent, ...) callback) to call
this.updateHighlightedTerritory() immediately after setting this.alternativeView
so the highlight is recomputed and rendered; also apply the same fix for the
analogous handler referenced around the 371-384 region to keep state and visuals
in sync.
---
Nitpick comments:
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 350-361: The globalThis listener added in init() for
`${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}` is never removed;
store the EventListener callback on the TerritoryLayer instance (e.g.
this._settingsListener) when registering it in init(), reuse that named function
to set highlightMode and call updateHighlightedTerritory(), and remove it in the
layer teardown (disconnectedCallback or the layer's dispose/cleanup method)
using globalThis.removeEventListener with the same event name and the stored
handler, mirroring the pattern in PerformanceOverlay.ts to prevent duplicate
handlers on re-init.
In `@src/client/UserSettingModal.ts`:
- Around line 312-317: changeTerritoryHighlight currently accepts any string and
forwards it into userSettings.setTerritoryHighlight; restrict accepted values to
the three known territory highlight modes by defining a tight typed union (e.g.
type TerritoryMode = 'modeA' | 'modeB' | 'modeC') and validate e.detail.value
against that set before calling this.userSettings.setTerritoryHighlight. Inside
changeTerritoryHighlight, ensure the value is a string, narrow it to the union
(or check membership in an allowedModes array), and only call
setTerritoryHighlight and requestUpdate when it matches; otherwise ignore or
handle invalid input gracefully.
In `@src/core/game/UserSettings.ts`:
- Around line 226-234: The setter setTerritoryHighlight currently accepts an
arbitrary string while the getter territoryHighlight narrows to the union
"always" | "onKeyPress" | "never"; change setTerritoryHighlight's signature to
accept the same union (or extract a shared type alias like
TerritoryHighlightMode and use it for territoryHighlight, setTerritoryHighlight
and call sites such as UserSettingModal.changeTerritoryHighlight and
TerritoryLayer) and update usages to match so TypeScript can enforce valid
values; keep the implementation using this.setString(TERRITORY_HIGHLIGHT_KEY,
value).
🪄 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: e9d53f57-d72c-4d6f-ac2a-98e62bfcbf9c
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/graphics/layers/TerritoryLayer.tssrc/core/game/UserSettings.ts
| "territory_highlight_label": "Territory Highlight", | ||
| "territory_highlight_desc": "Highlight the territory of the player under your cursor", | ||
| "territory_highlight_always": "Always", | ||
| "territory_highlight_on_key": "On Key Press (default H, customizable via key settings)", |
Contributor
There was a problem hiding this comment.
The "default H" hint will become stale after rebinding.
This label literally says "default H", but the keybind is user-configurable. If someone rebinds highlightTerritory to another key, the select option will keep advertising H, which is a small but real source of confusion. Consider dropping the key hint here (the keybinds tab is the source of truth) or passing the current key in as an ICU argument.
✏️ Possible simplification
- "territory_highlight_on_key": "On Key Press (default H, customizable via key settings)",
+ "territory_highlight_on_key": "On Key Press",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/en.json` at line 576, The string "territory_highlight_on_key"
currently hard-codes "default H" which becomes inaccurate after key rebinding;
update this resource by either removing the hard-coded key hint (e.g., change to
"On Key Press (customizable via key settings)") or converting it to an ICU-style
string with a placeholder like "{key}" and ensure the code that renders the
label supplies the current binding for highlightTerritory; locate the
"territory_highlight_on_key" entry in the locale JSON and change it accordingly
and update the UI code that reads this key to pass the current key binding if
you choose the placeholder approach.
Author
There was a problem hiding this comment.
Intentionally keeping the "default H" hint. The benefit is that a user can see the default key right in the dropdown without having to navigate to the keybinds tab first. If they rebind it, they already know where to find keybind settings — but for the majority of users who never rebind, this tells them exactly which key to press at a glance.
Contributor
There was a problem hiding this comment.
@luctrate, that's a fair point — showing the default key inline reduces friction for the majority of users who never touch keybind settings. The trade-off (slightly stale label for the small group who rebind) is reasonable given that those users already know where keybinds live.
Makes sense, thanks for the context!
🧠 Learnings used
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/zh_cn.json:527-539
Timestamp: 2025-07-12T08:42:02.109Z
Learning: In OpenFrontIO project, the territory pattern key "embelem" is the correct spelling used in all language files, not "emblem". This is the technical identifier that appears in en.json and must be consistent across all localization files.
Learnt from: baculinivan-web
Repo: openfrontio/OpenFrontIO PR: 3679
File: src/client/components/baseComponents/setting/SettingKeybind.ts:120-121
Timestamp: 2026-04-15T17:29:56.021Z
Learning: In OpenFrontIO's SettingKeybind.ts, the `displayKey` field in the `change` event detail (e.g., `e.key` or `Shift+${e.key.toUpperCase()}`) does not need extra casing normalization because all callers (e.g., UserSettingModal.ts) pass the value through `formatKeyForDisplay()` before rendering, which already applies fallback capitalization (`value.charAt(0).toUpperCase() + value.slice(1)`). Do not flag display casing on `detail.key` as an issue.
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Comment on lines
+412
to
+432
| private updateHighlightAlpha( | ||
| oldPlayer: PlayerView | null, | ||
| newPlayer: PlayerView | null, | ||
| ) { | ||
| const data = this.imageData.data; | ||
| const oldID = oldPlayer?.id(); | ||
| const newID = newPlayer?.id(); | ||
| this.game.forEachTile((tile) => { | ||
| const offset = tile * 4; | ||
| const alpha = data[offset + 3]; | ||
| // Only update non-border territory fill tiles (alpha 150 or 230) | ||
| if (alpha !== 150 && alpha !== 230) return; | ||
| if (!this.game.hasOwner(tile)) return; | ||
| const ownerID = (this.game.owner(tile) as PlayerView).id(); | ||
| if (newID !== undefined && ownerID === newID) { | ||
| data[offset + 3] = 230; | ||
| } else if (alpha === 230 && oldID !== undefined && ownerID === oldID) { | ||
| data[offset + 3] = 150; | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for tile-iteration methods on PlayerView and related classes.
rg -nP --type=ts -C2 '\b(tiles|ownedTiles|allTiles|borderTiles)\s*\(' -g 'src/core/game/**'
ast-grep --pattern $'class PlayerView {
$$$
}'Repository: openfrontio/OpenFrontIO
Length of output: 25984
🏁 Script executed:
cd src/core/game && grep -n "^\s*\(tiles\|ownedTiles\|allTiles\)(" GameView.ts | head -20Repository: openfrontio/OpenFrontIO
Length of output: 49
updateHighlightAlpha scans the entire map on every player hover change — performance concern is valid, but the proposed solution is not available.
forEachTile() iterates every tile each time the pointer moves between player territories. On large maps (especially giantworldmap) this causes noticeable lag during crowded matches where hover events fire frequently.
However, PlayerView does not expose a synchronous per-player tiles accessor—only async borderTiles(), which cannot be used in the synchronous highlight update flow.
To address this, consider:
- Caching per-player tile lists incrementally as they change (update on conquest/relinquish only, not per-hover)
- Exposing a synchronous
tiles()method onPlayerView(requires core API expansion) - Deferring highlight updates to a throttled pass rather than per-mousemove
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 412 - 432,
updateHighlightAlpha currently scans every tile via forEachTile on each hover
causing lag; instead maintain a synchronous per-player tile cache updated only
when ownership changes (e.g., update cache inside conquest/relinquish handlers
or in methods that modify ownership), then change updateHighlightAlpha to
iterate only the oldPlayer/newPlayer tile lists from that cache (use
PlayerView.id() as the key) and update imageData.data[offset+3] for those tiles;
as an alternative interim, throttle pointer hover updates to reduce frequency
until the cache is implemented.
Author
There was a problem hiding this comment.
Addressed in 6b19661. The main bottleneck was calling owner(tile).id() in the inner loop — this does a Map.get() lookup + PlayerView creation + string comparison per tile.
Replaced with ownerID(tile) (direct Uint16Array read + bitmask) compared against smallID() integers. Combined with the existing alpha filter (skips ~60% of tiles with alpha != 150/230), the inner loop is now just: read byte → skip most tiles → read uint16 → compare two ints. No allocations, no Map lookups, no string ops.
The forEachTile scan remains O(n) over all tiles, but the per-tile cost is now negligible (a few nanoseconds). A per-player tile cache would eliminate the scan entirely but would require core API changes — can revisit if profiling shows this is still a bottleneck on giant maps.
Contributor
There was a problem hiding this comment.
@luctrate, that's a clean fix. The updated implementation in <file_content> confirms it:
const oldSmallID = oldPlayer?.smallID() ?? -1;
const newSmallID = newPlayer?.smallID() ?? -1;
this.game.forEachTile((tile) => {
const offset = tile * 4;
const alpha = data[offset + 3];
if (alpha !== 150 && alpha !== 230) return; // skips ~60% early
const tileOwner = this.game.ownerID(tile); // Uint16Array read + bitmask
if (tileOwner === newSmallID) { ... } // int comparison
else if (alpha === 230 && tileOwner === oldSmallID) { ... }
});The per-tile cost is now just a byte read, an early return for most tiles, a Uint16Array read + bitmask, and two integer comparisons — no allocations, no Map lookups, no string ops. That's a solid pragmatic fix without needing core API changes. The per-player tile cache can be revisited later if profiling on giant maps shows it's still needed.
(\_/)
(•ᴗ•)
/>🎉
🧠 Learnings used
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3540
File: src/client/graphics/layers/NameLayer.ts:84-86
Timestamp: 2026-03-30T18:41:35.016Z
Learning: In graphics layers, `redraw()` should be intentionally a no-op (or have an empty/limited body) when the layer is DOM-based and does not depend on Canvas/WebGL context (e.g., it does not require reinitializing GPU resources after context loss). The `redraw()` hook should only contain reinitialization logic for Canvas/WebGL layers that must restore GPU resources on context restoration.
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3604
File: src/core/game/GameImpl.ts:747-765
Timestamp: 2026-04-07T12:24:45.143Z
Learning: In `src/core/game/GameImpl.ts`, the flush loop for `_pendingWaterTiles` intentionally re-checks live ownership (`isLand && !hasOwner`) rather than snapshotting at enqueue time. NukeExecution always calls `relinquish(tile)` before `queueWaterConversion(tile)`, so every queued tile is unowned when enqueued. If another execution reconquers the tile during the same tick, the conversion is skipped and the land survives — this is deliberate "last writer wins" behaviour consistent with the rest of the engine. A dedicated test ("queueWaterConversion skips tiles conquered before flush") covers this case. Do not flag this as a race condition or suggest snapshotting.
Learnt from: bibizu
Repo: openfrontio/OpenFrontIO PR: 2944
File: src/client/graphics/layers/SAMRadiusLayer.ts:283-286
Timestamp: 2026-01-26T00:20:17.081Z
Learning: In src/client/graphics/layers/SAMRadiusLayer.ts, the lineColorStressed variable currently uses this.theme.allyColor() as placeholder code. A dynamic color method for stressed SAMs is planned for a future PR.
Learnt from: andybellenie
Repo: openfrontio/OpenFrontIO PR: 3509
File: src/server/GameServer.ts:667-680
Timestamp: 2026-03-24T23:21:38.781Z
Learning: In openfrontio/OpenFrontIO, the clan-tag privacy feature in `src/server/GameServer.ts` (`gameInfoForClient()`, PR `#3509`) is intentionally a partial mitigation against tag sniping, not a complete fix. A larger clan system rework is in development. The bypass via rejoin with a different clanTag (identityUpdate path in Worker.ts) is a known, accepted limitation. Do not flag this as a critical issue in future reviews of this file.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 0
File: :0-0
Timestamp: 2026-02-01T16:36:52.217Z
Learning: In this TypeScript game codebase, UI/layer updates should be wall-clock throttled (independent of simulation speed). Do not use tick-modulus gating like `if (game.ticks()%N===0)` inside `layer.tick()` methods. Instead, layers should expose a `getTickIntervalMs()` method and let the renderer schedule ticks based on wall-clock time.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3604
File: src/core/game/WaterManager.ts:219-225
Timestamp: 2026-04-07T21:30:00.497Z
Learning: In `src/core/game/WaterManager.ts`, the `oceanQueue.length === 0` fallback in `finalizeWaterChanges` intentionally promotes all converted tiles to ocean even when no converted tile is adjacent to existing ocean. This ensures water-nuke craters are always navigable (ships/ports can use them) regardless of their inland position. Do not flag this as incorrect ocean promotion — it is deliberate design by FloPinguin.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: Ensure ghost preservation logic for special units (AtomBomb and HydrogenBomb) is implemented via shouldPreserveGhostAfterBuild(unitType) and that ghost removal is conditional on !shouldPreserveGhostAfterBuild(unitType) within createStructure. Confirm that both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure, and that Enter/NumpadEnter confirmations do not break the locked-nuke multi-placement since the ghost should be preserved by design. Add tests to cover shouldPreserveGhostAfterBuild returning true for AtomBomb/HydrogenBomb and verify that ghost removal only occurs for other units. Applies to files in the repository OpenFrontIO (src/client/InputHandler.ts and src/client/graphics/layers/StructureIconsLayer.ts).
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3549
File: tests/pathfinding/playground/server.ts:246-250
Timestamp: 2026-03-31T17:02:41.798Z
Learning: When migrating/implementing Express 5+ code, use the Express 5 `app.listen(PORT, (error) => { ... })` callback signature: Express 5 passes listen errors (e.g., `EADDRINUSE`) as the first argument to the callback. Handle the error inside that callback instead of relying on Node.js `http.Server`’s `'error'` event semantics.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
- Add updateHighlightedTerritory() call in AlternateViewEvent handler so highlight refreshes immediately on spacebar toggle - Extract TerritoryHighlightMode type alias to eliminate duplicated union literals across UserSettings and TerritoryLayer - Tighten setTerritoryHighlight setter and changeTerritoryHighlight handler to only accept valid mode values - Store settings listener as instance field for future cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
Replace expensive owner(tile).id() string lookups with cheap ownerID(tile) integer comparison in updateHighlightAlpha and paintTerritory. ownerID is a direct Uint16Array read + bitmask vs owner() which does a Map lookup + PlayerView creation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/TerritoryLayer.ts (1)
374-413:⚠️ Potential issue | 🟡 MinorFix highlight visibility in alternate view mode.
The
updateHighlightAlphamethod (lines 415–434) only updatesthis.imageData, neverthis.alternativeImageData. When alternate view is active, the code setshighlightedTerritorybased onshouldHighlight(line 378, which includes|| this.alternativeView), but the brightness change (alpha 230 vs 150 on non-border tiles) is only written tothis.imageData. SincerenderLayerdisplaysthis.alternativeImageDatain alternate view mode (line 537), the highlight has no visible effect.Choose one fix:
- Apply highlight-aware alpha/color to borders in
this.alternativeImageData(similar to line 624), or- Remove
|| this.alternativeViewfromshouldHighlightif highlighting was never intended for alternate view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 374 - 413, The highlight logic in updateHighlightedTerritory uses this.alternativeView but updateHighlightAlpha only writes to this.imageData, so highlights don’t show when renderLayer displays this.alternativeImageData; fix by either (A) updating updateHighlightAlpha (or adding a new helper) so it also applies the same alpha/brightness changes to this.alternativeImageData (respecting border vs non-border rules used elsewhere, e.g., similar to the border handling around line 624), or (B) if alternate view should not show highlights, remove the `|| this.alternativeView` term from the shouldHighlight calculation in updateHighlightedTerritory so highlights are not enabled for alternative views; reference updateHighlightedTerritory, updateHighlightAlpha, this.alternativeImageData, this.alternativeView and renderLayer to locate the code.
🤖 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/TerritoryLayer.ts`:
- Around line 361-364: The event listener added via globalThis.addEventListener
for `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}` in init() is
never removed — add cleanup to avoid handler accumulation: implement a destroy()
(or teardown) method on TerritoryLayer that calls globalThis.removeEventListener
with the same event string and the onHighlightSettingChanged handler, and ensure
init() guards against double-init (e.g., a boolean _inited or check/remove
existing listener before adding) so re-creating the layer or HMR won’t leave
dangling listeners.
---
Outside diff comments:
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 374-413: The highlight logic in updateHighlightedTerritory uses
this.alternativeView but updateHighlightAlpha only writes to this.imageData, so
highlights don’t show when renderLayer displays this.alternativeImageData; fix
by either (A) updating updateHighlightAlpha (or adding a new helper) so it also
applies the same alpha/brightness changes to this.alternativeImageData
(respecting border vs non-border rules used elsewhere, e.g., similar to the
border handling around line 624), or (B) if alternate view should not show
highlights, remove the `|| this.alternativeView` term from the shouldHighlight
calculation in updateHighlightedTerritory so highlights are not enabled for
alternative views; reference updateHighlightedTerritory, updateHighlightAlpha,
this.alternativeImageData, this.alternativeView and renderLayer to locate the
code.
🪄 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: 9634c8dd-bba9-4d9d-b3d4-ab94fe8acf96
📒 Files selected for processing (1)
src/client/graphics/layers/TerritoryLayer.ts
Comment on lines
+361
to
+364
| globalThis.addEventListener?.( | ||
| `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}`, | ||
| this.onHighlightSettingChanged, | ||
| ); |
Contributor
There was a problem hiding this comment.
Settings listener added on globalThis is never removed.
The listener attached in init() has no matching removeEventListener. If init() ever runs twice (e.g., layer re-created across games, HMR in dev) the handler accumulates and will keep a reference to this TerritoryLayer alive, so old layers keep reacting to setting changes.
A small fix is to keep a destroy() / teardown path and unregister there, and/or guard against double-init.
🧹 Suggested teardown
+ destroy() {
+ globalThis.removeEventListener?.(
+ `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}`,
+ this.onHighlightSettingChanged,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 361 - 364, The
event listener added via globalThis.addEventListener for
`${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}` in init() is never
removed — add cleanup to avoid handler accumulation: implement a destroy() (or
teardown) method on TerritoryLayer that calls globalThis.removeEventListener
with the same event string and the onHighlightSettingChanged handler, and ensure
init() guards against double-init (e.g., a boolean _inited or check/remove
existing listener before adding) so re-creating the layer or HMR won’t leave
dangling listeners.
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #1307
Relates to #3125, #2549
Description:
Adds an opt-in Territory Highlight setting (Never / Always / On Key Press) that brightens the hovered player's territory (alpha 230 vs 150), making it easy to identify ownership in large games where colors repeat or look similar.
TerritoryLayerhighlight detection (which was computed but never used) and extends it to work outside alternate viewFiles changed
UserSettings.ts— newTerritoryHighlightModetype,territoryHighlightsetting (getter/setter/localStorage key) +highlightTerritory: "KeyH"default keybindInputHandler.ts— newTerritoryHighlightKeyEvent(hold-to-activate, with blur cleanup)TerritoryLayer.ts— highlight mode state + settings listener; replaced alternate-view-only gate with 3-way check;updateHighlightAlpha()for immediate full-territory alpha update on hover change;paintTerritory()now usesisHighlightedfor alphaUserSettingModal.ts—<setting-select>for highlight mode in Basic tab; conditional<setting-keybind>in Keybinds tab (disabled with info message when not "On Key Press")en.json— 8 new translation keysPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
luctrate_98321
🤖 Generated with Claude Code