proto #2688
proto #2688
Conversation
WalkthroughA comprehensive refactoring moves type definitions (UnitType, PlayerType, MessageType) from the Game module to a new GameUpdates module, introduces a Protocol Buffers schema for game state serialization, and restructures update payloads to use nested objects instead of flat fields throughout the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (2 warnings, 1 inconclusive)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/client/graphics/layers/WinModal.ts (1)
320-345: Inconsistent handling ofwu.winner— treated as both string and array.The code calls
JSON.parse(wu.winner)on lines 327 and 345, which meanswu.winneris a JSON string. However, the code also accesseswu.winner[0](line 325) andwu.winner[1](lines 329, 334, 340) as if it were already an array.If
wu.winneris a JSON string like'["team", "red"]':
wu.winner[0]returns'['(first character), not"team"- The condition
wu.winner[0] === "team"will always be falsewu.winner[1]returns'"', not the team nameYou need to parse the winner first, then use the parsed result for comparisons.
🔎 Proposed fix
winUpdates.forEach((update) => { const wu = update.win!; + const parsedWinner = wu.winner !== undefined ? JSON.parse(wu.winner) : undefined; + const parsedStats = JSON.parse(wu.stats); if (wu.winner === undefined) { // ... - } else if (wu.winner[0] === "team") { - this.eventBus.emit( - new SendWinnerEvent(JSON.parse(wu.winner), JSON.parse(wu.stats)), - ); - if (wu.winner[1] === this.game.myPlayer()?.team()) { + } else if (parsedWinner[0] === "team") { + this.eventBus.emit( + new SendWinnerEvent(parsedWinner, parsedStats), + ); + if (parsedWinner[1] === this.game.myPlayer()?.team()) { this._title = translateText("win_modal.your_team"); this.isWin = true; } else { this._title = translateText("win_modal.other_team", { - team: wu.winner[1], + team: parsedWinner[1], }); this.isWin = false; } this.show(); } else { - const winner = this.game.playerByClientID(wu.winner[1]); + const winner = this.game.playerByClientID(parsedWinner[1]); if (!winner?.isPlayer()) return; const winnerClient = winner.clientID(); if (winnerClient !== null) { this.eventBus.emit( - new SendWinnerEvent(["player", winnerClient], JSON.parse(wu.stats)), + new SendWinnerEvent(["player", winnerClient], parsedStats), ); }src/core/GameRunner.ts (1)
94-147: WireplayerNameViewDatainto updates and resetisExecutingon errorsTwo issues here:
this.playerViewDatais updated but never copied into the tick payload.GameView.updateexpectsgu.playerNameViewData[pu.id]to be populated for name placement, so names and territory overlays will be wrong or missing.In the
catchblock,this.isExecutingstaystrue, so any exception during a tick permanently blocks further ticks.You can fix both with a small change:
Suggested patch
public executeNextTick() { @@ - this.isExecuting = true; + this.isExecuting = true; @@ - try { - updates = this.game.executeNextTick(); - } catch (error: unknown) { + try { + updates = this.game.executeNextTick(); + } catch (error: unknown) { if (error instanceof Error) { console.error("Game tick error:", error.message); console.error("Stack trace:", error.stack); this.callBack({ errMsg: error.message, stack: error.stack, } as ErrorUpdate); } else { console.error("Game tick error:", error); } - return; + this.isExecuting = false; + return; } @@ - if (this.game.ticks() < 3 || this.game.ticks() % 30 === 0) { + if (this.game.ticks() < 3 || this.game.ticks() % 30 === 0) { this.game.players().forEach((p) => { this.playerViewData[p.id()] = placeName(this.game, p); }); } @@ - this.callBack(updates); - this.isExecuting = false; + // Attach computed name view data to the tick payload. + updates.playerNameViewData = this.playerViewData; + + this.callBack(updates); + this.isExecuting = false; }Also applies to: 153-173
src/client/graphics/layers/EventsDisplay.ts (2)
358-388: Add a finite number check before converting goldAmount to BigIntThe code checks if
goldAmount !== undefined, but does not guard against non-finite values likeNaNorInfinity. If these values reach theBigInt()call on line 369, the code will throw and break the message display layer.Add a simple check to skip invalid values:
if (event.goldAmount !== undefined) { + if (!Number.isFinite(event.goldAmount)) { + return; + } const hasChanged = this.latestGoldAmount !== BigInt(event.goldAmount); this.latestGoldAmount = BigInt(event.goldAmount);This makes the UI robust even if a corrupt or unexpectedly large value is received.
172-188: Pass unwrappedGameUpdatepayloads to event handlers
game.updatesSinceLastTick()now returns a map ofGameUpdateType → GameUpdates, whereGameUpdates.updatesis an array ofGameUpdatewrappers (each with nested fields likedisplayMessage,emoji,allianceRequest, etc.).In
tick()at lines 224–229, you currently pass these wrappers directly to handlers:for (const [ut, fn] of this.updateMap) { updates[ut]?.updates.forEach(fn as (event: unknown) => void); }But handlers like
onDisplayMessageEvent(event: DisplayMessageUpdate)expect the unwrapped payload. When you pass the wrapper, fields likeevent.playerId,update.recipientId, andevent.goldAmountareundefined, so:
- display message and chat events fail the player check and are silently dropped
- alliance requests, target, emoji, and incoming unit events never show up
- the
as (event: unknown)cast hides this type mismatchUnwrap each
GameUpdatebefore calling the handler. Check other layers likeChatDisplay.tsandAlertFrame.tsfor the correct pattern:updates[GameUpdateType.DisplayEvent]?.updates.forEach((u) => { if (u.displayMessage) this.onDisplayMessageEvent(u.displayMessage); });Apply this to all nine entries in
updateMap.src/core/game/PlayerImpl.ts (1)
579-597: Fix emoji cooldown for direct recipientsIn
canSendEmoji, the filter usingmsg.allPlayers ?? msg.recipientId === recipientIDis broken. SinceallPlayersis always a boolean (nevernull/undefined), the??operator never evaluates the right side. WhenallPlayersisfalsefor 1:1 emojis, the filter returnsfalseand skips those messages entirely—meaning the cooldown check never runs for direct recipients.The fix is to branch on the recipient type instead of using
??:const recipientID = - recipient === AllPlayers ? AllPlayers : recipient.smallID(); - const prevMsgs = this.outgoingEmojis_.filter((msg) => { - return msg.allPlayers ?? msg.recipientId === recipientID; - }); + recipient === AllPlayers ? AllPlayers : recipient.smallID(); + const prevMsgs = this.outgoingEmojis_.filter((msg) => + recipient === AllPlayers + ? msg.allPlayers + : msg.recipientId === recipientID, + );This restores the intended per-recipient cooldown behavior.
src/core/game/GameImpl.ts (2)
740-761: OmitgoldAmountfrom the update when it's undefinedThe current code sends
Number(undefined)which serializes toNaNfor thegold_amountfield:displayMessage: { message: message, messageType: type, playerId: id ?? undefined, goldAmount: Number(goldAmount), params: params ?? {}, },This causes the proto encoder to emit
gold_amountwith a nonsensical value even when there is no gold amount. The proto schema marks this field asoptional int64 gold_amount, so it should be omitted entirely when not provided.Update to only set
goldAmountwhen present:displayMessage: { message: message, messageType: type, playerId: id ?? undefined, goldAmount: goldAmount !== undefined ? Number(goldAmount) : undefined, params: params ?? {}, },
82-88: SetgameViewData.tickand convert helpers to function declarationsThree issues need fixing:
tick always stays 0:
gameViewData.tickdefaults to 0 and is never updated before being returned inexecuteNextTick(). SinceGameViewreadslastUpdate.tickto determine the current tick, clients will always see tick 0, breaking spawn-phase logic and any tick-based UI timing.goldAmount converts undefined to NaN: When
displayMessage()is called without agoldAmountparameter (8 calls across UnitImpl, PlayerImpl, and AttackExecution), the code doesNumber(goldAmount)which convertsundefinedtoNaN. This sends invalid data to the client.TDZ warning:
private gameViewData = createGameUpdateViewData();at line 82 referencescreateGameUpdateViewData, which is declared as a const at line 1023. This triggers thenoInvalidUseBeforeDeclarationlinter.Fix all three by converting the helper functions to declarations (which are hoisted) and setting the tick explicitly before incrementing:
- private gameViewData = createGameUpdateViewData(); + private gameViewData = createGameUpdateViewData(); executeNextTick(): GameUpdateViewData { this.gameViewData = createGameUpdateViewData(); // ... executions ... if (this.ticks() % 10 === 0) { this.addUpdate({ type: GameUpdateType.Hash, hash: { tick: this.ticks(), hash: this.hash(), }, }); } + // Export the tick number associated with these updates. + this.gameViewData.tick = this._ticks; + this._ticks++; return this.gameViewData; } -const createGameUpdatesMap = (): Record<GameUpdateType, GameUpdates> => { - const map = {} as Record<GameUpdateType, GameUpdates>; - Object.values(GameUpdateType) - .filter((key) => !isNaN(Number(key))) - .forEach((key) => { - map[key as GameUpdateType] = { - type: key as GameUpdateType, - updates: [], - }; - }); - return map; -}; - -const createGameUpdateViewData = (): GameUpdateViewData => { - return { - tick: 0, - updates: createGameUpdatesMap(), - tileUpdates: [], - playerNameViewData: {}, - tickExecutionDuration: undefined, - }; -}; +function createGameUpdatesMap(): Record<GameUpdateType, GameUpdates> { + const map = {} as Record<GameUpdateType, GameUpdates>; + Object.values(GameUpdateType) + .filter((key) => !isNaN(Number(key))) + .forEach((key) => { + map[key as GameUpdateType] = { + type: key as GameUpdateType, + updates: [], + }; + }); + return map; +} + +function createGameUpdateViewData(): GameUpdateViewData { + return { + tick: 0, + updates: createGameUpdatesMap(), + tileUpdates: [], + playerNameViewData: {}, + tickExecutionDuration: undefined, + }; +}
🧹 Nitpick comments (3)
tests/client/graphics/UILayer.test.ts (1)
54-54: Consider creating the event object using theUnitSelectionEventconstructor instead of a plain object.The mock at lines 46-52 is a plain object
{ isSelected: true, unit }, butUnitSelectionEventis a class. The double castas unknown as UnitSelectionEventis needed because TypeScript treats class instances differently from plain objects. You can avoid this cast by instantiating the class:new UnitSelectionEvent(unit, true)instead of a plain object. This is clearer and removes the type safety bypass.src/client/graphics/layers/StructureLayer.ts (1)
101-108: Minor: Redundant config lookup.Line 105 looks up
this.unitConfigs[unitType]!butconfigfrom line 103 already holds the same value.Suggested simplification
private loadIconData() { AllUnitTypes.forEach((unitType) => { const config = this.unitConfigs[unitType]; if (config) { - this.loadIcon(unitType, this.unitConfigs[unitType]!); + this.loadIcon(unitType, config); } }); }src/core/game/Game.ts (1)
33-36: Restore type link betweenbuildUnit’stypeandparamsRight now
buildUnitacceptsparams: UnitParams<keyof UnitParamsMap>, which is a union of all param shapes. This gives up compile‑time safety that the params match the selected unit type.You can keep runtime behavior the same but recover stronger typing like this:
Suggested type tightening
-export type UnitParams<T extends keyof UnitParamsMap> = UnitParamsMap[T]; +export type UnitParams<T extends keyof UnitParamsMap> = UnitParamsMap[T]; @@ - buildUnit<T extends UnitType>( - type: T, + buildUnit<T extends keyof UnitParamsMap>( + type: T, spawnTile: TileRef, - params: UnitParams<keyof UnitParamsMap>, + params: UnitParams<T>, ): Unit;This keeps the API simple but ensures, for example, that
TransportShipbuilds cannot accidentally passWarshipparams.Also applies to: 268-272, 549-553
| if (gu.updates[GameUpdateType.Win]?.updates.length > 0) { | ||
| this.saveGame(gu.updates[GameUpdateType.Win][0]); | ||
| } |
There was a problem hiding this comment.
Bug: Missing .updates before array access.
Line 334 accesses gu.updates[GameUpdateType.Win][0] but should be gu.updates[GameUpdateType.Win].updates[0] based on the pattern used elsewhere (e.g., line 317).
Fix
if (gu.updates[GameUpdateType.Win]?.updates.length > 0) {
- this.saveGame(gu.updates[GameUpdateType.Win][0]);
+ this.saveGame(gu.updates[GameUpdateType.Win].updates[0]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (gu.updates[GameUpdateType.Win]?.updates.length > 0) { | |
| this.saveGame(gu.updates[GameUpdateType.Win][0]); | |
| } | |
| if (gu.updates[GameUpdateType.Win]?.updates.length > 0) { | |
| this.saveGame(gu.updates[GameUpdateType.Win].updates[0]); | |
| } |
🤖 Prompt for AI Agents
In src/client/ClientGameRunner.ts around lines 333 to 335, the code incorrectly
indexes gu.updates[GameUpdateType.Win][0] but the structure uses a nested
.updates array (as checked on line 333); change the access to
gu.updates[GameUpdateType.Win].updates[0] so you retrieve the first update
object correctly, keeping the existing guard that checks .updates.length before
calling saveGame.
| for (const update of updates?.[GameUpdateType.DisplayEvent]?.updates ?? | ||
| []) { | ||
| const msg = update.displayMessage!; | ||
| if (msg.messageType === MessageType.CHAT) { | ||
| const myPlayer = this.game.myPlayer(); | ||
| if ( | ||
| msg.playerId !== null && | ||
| (!myPlayer || myPlayer.smallID() !== msg.playerId) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| this.chatEvents = [ | ||
| ...this.chatEvents, | ||
| { | ||
| description: msg.message, | ||
| unsafeDescription: true, | ||
| createdAt: this.game.ticks(), | ||
| }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Non-null assertion on displayMessage could throw at runtime.
Line 81 uses update.displayMessage! but if the update object lacks this field, you get a crash. Consider a guard or optional chaining:
Suggested fix
for (const update of updates?.[GameUpdateType.DisplayEvent]?.updates ??
[]) {
- const msg = update.displayMessage!;
+ const msg = update.displayMessage;
+ if (!msg) continue;
if (msg.messageType === MessageType.CHAT) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const update of updates?.[GameUpdateType.DisplayEvent]?.updates ?? | |
| []) { | |
| const msg = update.displayMessage!; | |
| if (msg.messageType === MessageType.CHAT) { | |
| const myPlayer = this.game.myPlayer(); | |
| if ( | |
| msg.playerId !== null && | |
| (!myPlayer || myPlayer.smallID() !== msg.playerId) | |
| ) { | |
| continue; | |
| } | |
| this.chatEvents = [ | |
| ...this.chatEvents, | |
| { | |
| description: msg.message, | |
| unsafeDescription: true, | |
| createdAt: this.game.ticks(), | |
| }, | |
| ]; | |
| } | |
| for (const update of updates?.[GameUpdateType.DisplayEvent]?.updates ?? | |
| []) { | |
| const msg = update.displayMessage; | |
| if (!msg) continue; | |
| if (msg.messageType === MessageType.CHAT) { | |
| const myPlayer = this.game.myPlayer(); | |
| if ( | |
| msg.playerId !== null && | |
| (!myPlayer || myPlayer.smallID() !== msg.playerId) | |
| ) { | |
| continue; | |
| } | |
| this.chatEvents = [ | |
| ...this.chatEvents, | |
| { | |
| description: msg.message, | |
| unsafeDescription: true, | |
| createdAt: this.game.ticks(), | |
| }, | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In src/client/graphics/layers/ChatDisplay.ts around lines 79 to 99, the code
uses a non-null assertion update.displayMessage! which can throw if
displayMessage is missing; remove the non-null assertion and add a guard (e.g.,
if (!update.displayMessage) continue) or use optional chaining to skip updates
without displayMessage before accessing properties like messageType and
playerId, then proceed to push chatEvents only when displayMessage is present
and of type CHAT.
| const winUpdates = | ||
| updates !== null ? updates[GameUpdateType.Win].updates : []; |
There was a problem hiding this comment.
Missing optional chaining causes potential runtime error.
If updates[GameUpdateType.Win] is undefined (likely for most ticks without a win), accessing .updates will throw TypeError: Cannot read property 'updates' of undefined.
🔎 Proposed fix
- const winUpdates =
- updates !== null ? updates[GameUpdateType.Win].updates : [];
+ const winUpdates =
+ updates !== null ? (updates[GameUpdateType.Win]?.updates ?? []) : [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const winUpdates = | |
| updates !== null ? updates[GameUpdateType.Win].updates : []; | |
| const winUpdates = | |
| updates !== null ? (updates[GameUpdateType.Win]?.updates ?? []) : []; |
🤖 Prompt for AI Agents
In src/client/graphics/layers/WinModal.ts around lines 318 to 319, the code
accesses updates[GameUpdateType.Win].updates directly which will throw if
updates[GameUpdateType.Win] is undefined; change the expression to safely access
the nested property (e.g., use optional chaining or a fallback): compute
winUpdates from updates?.[GameUpdateType.Win]?.updates or from (updates &&
updates[GameUpdateType.Win] && updates[GameUpdateType.Win].updates) and default
to an empty array so winUpdates is always defined.
| const winUpdates = | ||
| updates !== null ? updates[GameUpdateType.Win].updates : []; | ||
| winUpdates.forEach((update) => { | ||
| const wu = update.win!; |
There was a problem hiding this comment.
Non-null assertion on update.win may cause runtime error.
The ! operator assumes update.win is always defined. If an update lacks the win property, this will throw. Consider adding a guard or using optional chaining with an early return.
🔎 Proposed fix
winUpdates.forEach((update) => {
- const wu = update.win!;
+ const wu = update.win;
+ if (wu === undefined) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wu = update.win!; | |
| winUpdates.forEach((update) => { | |
| const wu = update.win; | |
| if (wu === undefined) return; |
🤖 Prompt for AI Agents
In src/client/graphics/layers/WinModal.ts around line 321, the code uses a
non-null assertion const wu = update.win! which can throw if update.win is
undefined; instead, add a guard or early return that checks for update.win (e.g.
if (!update.win) return; or handle the missing case) and then assign without the
bang (const wu = update.win), or use optional chaining where appropriate so the
code only accesses wu when it exists.
| (emoji) => | ||
| emoji.recipientID === AllPlayers || | ||
| emoji.recipientID === myPlayer?.smallID(), | ||
| emoji.allPlayers ?? emoji.recipientId === myPlayer?.smallID(), |
There was a problem hiding this comment.
Check operator precedence in recipient filter.
The nullish coalescing operator ?? has lower precedence than ===, so this parses as:
emoji.allPlayers ?? (emoji.recipientId === myPlayer?.smallID())If emoji.allPlayers is explicitly false (not null or undefined), the recipientId comparison won't run. Consider:
🔎 Suggested fix
- emoji.allPlayers ?? emoji.recipientId === myPlayer?.smallID(),
+ emoji.allPlayers || emoji.recipientId === myPlayer?.smallID(),Or if allPlayers can be nullable:
- emoji.allPlayers ?? emoji.recipientId === myPlayer?.smallID(),
+ (emoji.allPlayers ?? false) || emoji.recipientId === myPlayer?.smallID(),🤖 Prompt for AI Agents
In src/client/graphics/PlayerIcons.ts around line 117, the expression uses ??
and === without parentheses so precedence makes it parse as emoji.allPlayers ??
(emoji.recipientId === myPlayer?.smallID()); disambiguate the intent by adding
parentheses: if you want to treat a defined false/true in allPlayers as
authoritative use (emoji.allPlayers ?? (emoji.recipientId ===
myPlayer?.smallID())); if you want allPlayers null/undefined to fall through to
the recipient check but treat false as a fallthrough value use (emoji.allPlayers
?? false) || emoji.recipientId === myPlayer?.smallID(); update the expression
accordingly to make the precedence explicit.
| } from "../../../src/client/graphics/layers/RadialMenuElements"; | ||
| import { UnitType } from "../../../src/core/game/Game"; | ||
| import { TileRef } from "../../../src/core/game/GameMap"; | ||
| import { UnitType } from "../../../src/core/game/GameUpdates"; |
There was a problem hiding this comment.
Import mismatch in mock setup.
Line 13 imports UnitType from GameUpdates, but line 22's mock still loads it from the Game module. If UnitType has been moved to GameUpdates, this mock will fail or use the wrong type.
🔎 Proposed fix
jest.mock("../../../src/client/graphics/layers/BuildMenu", () => {
- const { UnitType } = jest.requireActual("../../../src/core/game/Game");
+ const { UnitType } = jest.requireActual("../../../src/core/game/GameUpdates");
return {Also applies to: 22-22
🤖 Prompt for AI Agents
In tests/client/graphics/RadialMenuElements.test.ts around line 13 (and the mock
at line 22), the test imports UnitType from "../../../src/core/game/GameUpdates"
but the mock still references UnitType from the old Game module; update the mock
to import or reference UnitType from the same
"../../../src/core/game/GameUpdates" location so both the test and the mock use
the same source, and scan the file for any other imports/mocks referencing the
old Game path and change them to the GameUpdates path to keep types consistent.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME