merge v25 into main #1879
merge v25 into main #1879
Conversation
WalkthroughThis PR transitions pattern handling from raw pattern strings to named patterns across client, server, schemas, and config. It updates join/spawn message shapes, alliance request lifecycle (status, expiry, auto-accept), train/trade economics and spawn logic, introduces cosmetics Map lookup, and adds tests. Minor lint and map playlist changes included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant WS as WebSocket
participant PreJoin as PreJoinHandler
participant Priv as PrivilegeChecker
participant Game as Game
Client->>WS: send ClientJoinMessage { patternName, ... }
WS->>PreJoin: handle(clientMsg)
PreJoin->>Priv: isPatternAllowed(patternName, flares)
alt allowed
PreJoin->>Game: join(clientMsg)
Game-->>PreJoin: ok
PreJoin-->>WS: proceed
else unknown
PreJoin-->>WS: close(1002, "backend lookup failed")
else forbidden
PreJoin-->>WS: close(1002, reason)
end
sequenceDiagram
autonumber
actor A as Player A
actor B as Player B
participant G as Game
participant Exec as AllianceRequestExecution
participant AR as AllianceRequest
participant UI as EventsDisplay
A->>G: addExecution(AllianceRequestExecution to B)
G->>Exec: init()
alt B has pending incoming to A
Exec->>AR: accept()
Exec-->>G: deactivate
else can request
Exec->>AR: create (status: pending, createdAt=t)
else not allowed
Exec-->>G: deactivate
end
loop each tick
G->>Exec: tick(now)
alt AR.status in {accepted, rejected}
Exec-->>G: deactivate
else now - createdAt > duration
Exec->>AR: reject()
Exec-->>G: deactivate
end
G->>UI: emit/update events (shouldDelete checks alliance state)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
tests/core/game/TrainStation.test.ts (1)
18-21: Invalid trainGold stub in tests: update to use relation-based signatureThe test in tests/core/game/TrainStation.test.ts is still stubbing trainGold with a boolean parameter, but the API now expects a relation union (“self” | “friendly” | “other”). Please update the mock to match the new signature.
• tests/core/game/TrainStation.test.ts (around lines 18–21)
// Before: config: jest.fn().mockReturnValue({ trainGold: (isFriendly: boolean) => isFriendly ? BigInt(1000) : BigInt(500), }), // After: config: jest.fn().mockReturnValue({ trainGold: (rel: "self" | "friendly" | "other") => rel === "friendly" ? BigInt(1000) : BigInt(500), }),src/core/game/GameView.ts (1)
393-398: Do not reassign a readonly Map; populate it in place to avoid TS error
_cosmeticsis declared as readonly. Reassigning it breaks type safety and will not compile under strict TS.Apply this diff to populate the existing Map instead of reassigning:
- this._cosmetics = new Map( - this._humans.map((h) => [ - h.clientID, - { flag: h.flag, pattern: h.pattern } satisfies PlayerCosmetics, - ]), - ); + for (const h of this._humans) { + this._cosmetics.set(h.clientID, { + flag: h.flag, + pattern: h.pattern, + } as PlayerCosmetics); + }src/client/ClientGameRunner.ts (1)
50-62: Lingering “pattern” in Join Schemas and Single-Player PayloadOur grep shows that while the client now sends
patternNamein LobbyConfig…
- src/client/ClientGameRunner.ts:
patternName: string | undefined- src/client/Main.ts: builds join payload with
patternName…the core join‐message schema (and our single-player modal) still expect or emit the old
patternfield:• Core schema for ClientJoinMessage (src/core/Schemas.ts:489–490)
–pattern: PatternSchema.optional(),
–patternName: z.string().optional(),• SinglePlayerModal.ts (src/client/SinglePlayerModal.ts:449–450)
– building config withpattern: pattern?.pattern,To fully retire the old field and avoid confusion, we should:
• Remove
pattern: PatternSchema.optional()fromClientJoinMessageSchema
• Update SinglePlayerModal to sendpatternNameinstead ofpattern
• Audit server-side handlers (e.g. GameServer) to consumepatternNameonly—Please address these schema and payload discrepancies before approving.
src/client/Transport.ts (1)
317-325: Buffered messages are sent LIFO; this can reorder intents. Fix to FIFO.Using pop() reverses ordering of buffered messages, which can corrupt game state after reconnect.
Apply this diff to send buffered messages in FIFO order:
- while (this.buffer.length > 0) { - console.log("sending dropped message"); - const msg = this.buffer.pop(); - if (msg === undefined) { - console.warn("msg is undefined"); - continue; - } - this.socket.send(msg); - } + while (this.buffer.length > 0) { + console.log("sending dropped message"); + const msg = this.buffer.shift(); + if (msg === undefined) { + console.warn("msg is undefined"); + continue; + } + this.socket.send(msg); + }src/server/worker/websocket/handler/message/PreJoinHandler.ts (1)
240-252: Pass the server-authoritative pattern to Client.Use the validated pattern from privileges, or fall back to the client-provided one only if we did not validate by name.
Apply this diff:
const client = new Client( clientMsg.clientID, persistentId, claims, roles, flares, ip, clientMsg.username, ws, clientMsg.flag, - clientMsg.pattern, + allowedPatternFromServer ?? clientMsg.pattern, );
🧹 Nitpick comments (36)
src/core/Util.ts (1)
292-298: Stabilize sigmoid for extreme inputs (optional) and clarify namingCurrent form can suffer from overflow/underflow when decayRate * |value - midpoint| is large, returning 0/1 abruptly. A numerically stable formulation avoids exp blow-ups and keeps results in (0,1) smoothly.
Also, “decayRate” reads like a negative slope; “steepness” is more standard for the logistic.
Apply this diff to make the function numerically stable:
export function sigmoid( value: number, - decayRate: number, + decayRate: number, midpoint: number, ): number { - return 1 / (1 + Math.exp(-decayRate * (value - midpoint))); + const x = decayRate * (value - midpoint); + // Numerically stable logistic: + // For x >= 0: 1 / (1 + e^-x) + // For x < 0: e^x / (1 + e^x) + if (x >= 0) { + return 1 / (1 + Math.exp(-x)); + } + const ex = Math.exp(x); + return ex / (1 + ex); }src/core/game/Game.ts (1)
1-1: Avoid file-wide lint disable; consider splitting module insteadDisabling max-lines at the file level tends to hide real issues. Prefer breaking this giant module into focused submodules (e.g., map types, unit types/params, diplomacy types) and re-enabling the rule. This improves readability and compilation times.
tests/core/game/TrainStation.test.ts (3)
18-21: Mock trainGold with a union-typed relation to match new Config APITests should mirror the updated API to catch mismatches early and keep types honest. Model trainGold(rel: "self" | "friendly" | "other") explicitly.
- config: jest.fn().mockReturnValue({ - trainGold: (isFriendly: boolean) => - isFriendly ? BigInt(1000) : BigInt(500), - }), + config: jest.fn().mockReturnValue({ + // relation-aware payout: adjust if defaults differ + trainGold: (rel: "self" | "friendly" | "other") => + rel === "friendly" ? 1000n : rel === "self" ? 1000n : 500n, + }),
26-35: Align Player mock with interface (id should be a method, not a field)Right now id is a number, but the interface defines id(): PlayerID. Keeping the shape consistent avoids brittle test doubles if code under test calls id().
- player = { - addGold: jest.fn(), - id: 1, + player = { + addGold: jest.fn(), + id: jest.fn().mockReturnValue("player-1"), canTrade: jest.fn().mockReturnValue(true), isFriendly: jest.fn().mockReturnValue(false), } as any;
57-69: Add a “neutral/other” trade case for coverageYou cover self and friendly. Add a case where isFriendly is false and verify the 500n path so we catch regressions in relation-to-gold mapping.
Do you want me to add the extra test block for “other” relation?
src/core/game/AllianceRequestImpl.ts (1)
31-38: Make accept()/reject() idempotentGuard both methods so they only run once when the request is still “pending.” This prevents duplicate alliances or update events if accept()/reject() is called twice (for example, via a double-click).
• File: src/core/game/AllianceRequestImpl.ts
Lines ~31–38accept(): void { - this.status_ = "accepted"; - this.game.acceptAllianceRequest(this); + if (this.status_ !== "pending") return; + this.status_ = "accepted"; + this.game.acceptAllianceRequest(this); } reject(): void { - this.status_ = "rejected"; - this.game.rejectAllianceRequest(this); + if (this.status_ !== "pending") return; + this.status_ = "rejected"; + this.game.rejectAllianceRequest(this); }src/core/execution/PortExecution.ts (1)
26-28: Use an initialization check that actually guards undefined valuesThese checks compare to null, but these fields are initially undefined. Prefer an undefined-safe guard (or a dedicated initialized flag) to avoid accidental misuse later.
Apply one of the following quick fixes:
- if (this.mg === null || this.random === null || this.checkOffset === null) { + if (this.mg === undefined || this.random === undefined || this.checkOffset === undefined) { throw new Error("Not initialized"); }Alternatively, follow the pattern used in TrainStationExecution by checking for undefined consistently across the codebase.
tests/AllianceRequestExecution.test.ts (1)
62-76: Avoid leaking config mutation across testsOverriding game.config().allianceRequestDuration inline can leak to other tests if order changes. Save and restore the original to keep tests isolated.
Apply this change:
- test("Alliance request expires", () => { - game.config().allianceRequestDuration = () => 5; - game.addExecution(new AllianceRequestExecution(player1, player2.id())); - game.executeNextTick(); - - expect(player1.outgoingAllianceRequests()).toHaveLength(1); - - for (let i = 0; i < 6; i++) { - game.executeNextTick(); - } - - expect(player1.outgoingAllianceRequests()).toHaveLength(0); - expect(player1.isAlliedWith(player2)).toBeFalsy(); - expect(player2.isAlliedWith(player1)).toBeFalsy(); - }); + test("Alliance request expires", () => { + const original = game.config().allianceRequestDuration; + try { + game.config().allianceRequestDuration = () => 5; + game.addExecution(new AllianceRequestExecution(player1, player2.id())); + game.executeNextTick(); + + expect(player1.outgoingAllianceRequests()).toHaveLength(1); + + for (let i = 0; i < 6; i++) { + game.executeNextTick(); + } + + expect(player1.outgoingAllianceRequests()).toHaveLength(0); + expect(player1.isAlliedWith(player2)).toBeFalsy(); + expect(player2.isAlliedWith(player1)).toBeFalsy(); + } finally { + game.config().allianceRequestDuration = original; + } + });src/core/configuration/Config.ts (2)
136-139: Config surface updates are consistent — add brief range notes for clarity
- tradeShipSpawnRate(numTradeShips, numPlayerPorts) and trainSpawnRate(numPlayerFactories) look consistent with the new execution logic.
- trainGold(rel: "self" | "friendly" | "other") is a nice use of a typed union.
Minor clarity nit: consider documenting expected ranges/units for spawn rates (0..1) and typical magnitudes for trainGold to guide implementers of DefaultConfig.
97-101: Typo in parameter name: attckTroopsUnrelated to this PR, but worth fixing for readability.
- attackTilesPerTick( - attckTroops: number, + attackTilesPerTick( + attackTroops: number,src/core/game/TrainStation.ts (2)
46-52: Port stop revenue matches city logic — but random is no longer usedPortStopHandler no longer uses its random field. Remove it to avoid confusion and dead state.
Apply this within the shown ranges:
-class PortStopHandler implements TrainStopHandler { - constructor(private readonly random: PseudoRandom) {} +class PortStopHandler implements TrainStopHandler { onStop( mg: Game, station: TrainStation, trainExecution: TrainExecution, ): void {And update the handlers factory accordingly:
-export function createTrainStopHandlers( - random: PseudoRandom, -): Partial<Record<UnitType, TrainStopHandler>> { +export function createTrainStopHandlers(): Partial<Record<UnitType, TrainStopHandler>> { return { [UnitType.City]: new CityStopHandler(), - [UnitType.Port]: new PortStopHandler(random), + [UnitType.Port]: new PortStopHandler(), [UnitType.Factory]: new FactoryStopHandler(), }; }Additionally, update the TrainStation constructor call (outside the selected range):
// before this.stopHandlers = createTrainStopHandlers(new PseudoRandom(mg.ticks())); // after this.stopHandlers = createTrainStopHandlers();If no other code needs PseudoRandom here, drop its import from this file as well.
61-61: Factory stop handler is a no-op — consider removing until neededIf factories have no stop-side effects, you can omit FactoryStopHandler from the map to reduce indirection, or leave a clear TODO stating the planned behavior.
src/core/execution/alliance/AllianceRequestExecution.ts (2)
34-37: Only auto-accept pending reciprocal requestsBe explicit and only accept if the reciprocal request is still pending. This avoids accidental accepts of already-resolved requests.
- const incoming = recipient - .outgoingAllianceRequests() - .find((r) => r.recipient() === this.requestor); + const incoming = recipient + .outgoingAllianceRequests() + .find( + (r) => r.recipient() === this.requestor && r.status() === "pending", + );
48-63: Short-circuit tick when there’s no request; clarify TTL check
- If
this.reqis null (e.g., recipient missing, or creation failed), we should deactivate immediately instead of waiting for the TTL math with a synthetic createdAt=0.- Optional: consider inclusive expiry (
>=) if TTL is meant to expire at exactly the configured tick delta.tick(ticks: number): void { - if ( - this.req?.status() === "accepted" || - this.req?.status() === "rejected" - ) { + if (!this.req) { + this.active = false; + return; + } + if (this.req.status() === "accepted" || this.req.status() === "rejected") { this.active = false; return; } - if ( - this.mg.ticks() - (this.req?.createdAt() ?? 0) > - this.mg.config().allianceRequestDuration() - ) { - this.req?.reject(); + if ( + this.mg.ticks() - this.req.createdAt() >= + this.mg.config().allianceRequestDuration() + ) { + this.req.reject(); this.active = false; } }src/core/configuration/DefaultConfig.ts (6)
340-344: Comment and formula don’t match (“hyperbolic decay” vs linear)The code is linear:
(numPlayerFactories + 10) * 20. Either update the comment to reflect linear scaling, or implement the intended hyperbolic shape.Option A (update comment only):
- // hyperbolic decay, midpoint at 10 factories - // expected number of trains = numPlayerFactories / trainSpawnRate(numPlayerFactories) + // linear scaling; higher factories => higher spawnRate (lower probability) + // expected trains per tick ≈ numPlayerFactories / trainSpawnRate(numPlayerFactories) return (numPlayerFactories + 10) * 20;Option B (example hyperbolic-ish):
- return (numPlayerFactories + 10) * 20; + const k = 10; // midpoint + const base = 20; + return Math.floor(base * (1 + k / Math.max(1, numPlayerFactories)));
345-354: Rel tiers look consistent; consider documenting rationaleThe mapping is clear. A short doc comment on why self < other < friendly would help avoid regressions later.
No code change required.
374-384: Guard against extreme spawn rates and align comments with math
- If
combinedgets very small,12 / combinedexplodes. Clamp the denominator to a small epsilon and the final value to a reasonable max.- The top-level comment says “Probability of trade ship spawn = 1 / tradeShipSpawnRate”, but internally you’re combining rates as a geometric mean of two dampeners. Consider adding a brief docstring to explain the intended curve.
- const combined = Math.sqrt( + const combined = Math.sqrt( this.tradeShipBaseSpawn(numTradeShips) * this.tradeShipPortMultiplier(numPlayerPorts), ); - - return Math.floor(12 / combined); + const EPS = 1e-6; + const rate = Math.floor(12 / Math.max(combined, EPS)); + return Math.min(rate, 1_000_000); // safety cap
390-396: Ports multiplier comment mismatches implementationComment says “Expected trade ship spawn rate is proportional to numPlayerPorts * multiplier” but
tradeShipSpawnRatedoes not multiply by ports count; it only uses this multiplier as a dampener. Update the comment or rework the formula for consistency.Example comment fix:
- // Expected trade ship spawn rate is proportional to numPlayerPorts * multiplier - // Gradual decay prevents scenario where more ports => fewer ships + // Dampens spawn as ports increase (geometric mean applied in tradeShipSpawnRate). + // Gradual decay prevents pathological spikes as ports scale up.If the intent was “more ports -> more ships (with diminishing returns)”, consider:
// inside tradeShipSpawnRate const portEffect = numPlayerPorts / (numPlayerPorts + 10); const combined = Math.sqrt(this.tradeShipBaseSpawn(numTradeShips) * portEffect);
534-549:costWrapperrefactor: simpler call sites; minor naming nitThe reduction across multiple unit types is neat. Optional: rename
costFn(units)param tocostFn(numUnits)to match usage below.- private costWrapper( - costFn: (units: number) => number, + private costWrapper( + costFn: (numUnits: number) => number, ...types: UnitType[] ): (p: Player) => bigint {
671-706: Defense/attack debuffs: monotone, bounded, and readable
- The “large defender” debuffs use a well-behaved logistic; nice.
- Naming: “Bonus” for factors < 1 is a bit misleading. Consider “Modifier”.
- let largeAttackBonus = 1; + let largeAttackModifier = 1; if (attacker.numTilesOwned() > 100_000) { - largeAttackBonus = Math.sqrt(100_000 / attacker.numTilesOwned()) ** 0.7; + largeAttackModifier = Math.sqrt(100_000 / attacker.numTilesOwned()) ** 0.7; } - let largeAttackerSpeedBonus = 1; + let largeAttackerSpeedModifier = 1; if (attacker.numTilesOwned() > 100_000) { - largeAttackerSpeedBonus = (100_000 / attacker.numTilesOwned()) ** 0.6; + largeAttackerSpeedModifier = (100_000 / attacker.numTilesOwned()) ** 0.6; } … - largeDefenderAttackDebuff * - largeAttackBonus * + largeDefenderAttackDebuff * + largeAttackModifier * … - largeDefenderSpeedDebuff * - largeAttackerSpeedBonus * + largeDefenderSpeedDebuff * + largeAttackerSpeedModifier *src/core/game/GameView.ts (1)
394-398: Guard against null/empty clientID when keying the cosmetics mapIf any human
clientIDcan be null/undefined,Map<string, ...>keying will either fail type check or store an incorrect key. You already handlepu.clientID ?? ""on read. Mirror that here or skip missing ids.Apply this diff to be defensive:
- for (const h of this._humans) { - this._cosmetics.set(h.clientID, { + for (const h of this._humans) { + const key = (h.clientID ?? "") as string; + if (!key) continue; + this._cosmetics.set(key, { flag: h.flag, pattern: h.pattern, } as PlayerCosmetics); }src/core/game/UserSettings.ts (1)
118-124: Param name no longer matches semantics; rename for clarity
base64suggests raw pattern data, but we now store a pattern name.Apply this diff to make the API self-explanatory:
- setSelectedPatternName(base64: string | undefined): void { - if (base64 === undefined) { + setSelectedPatternName(name: string | undefined): void { + if (name === undefined) { localStorage.removeItem(PATTERN_KEY); } else { - localStorage.setItem(PATTERN_KEY, base64); + localStorage.setItem(PATTERN_KEY, name); } }src/client/graphics/layers/EventsDisplay.ts (1)
461-466: Clamp alliance-request duration to avoid negative valuesIf
allianceRequestDuration()< 20, the duration becomes negative and the event will vanish immediately.Apply this diff:
- duration: this.game.config().allianceRequestDuration() - 20, // 2 second buffer + duration: Math.max(0, this.game.config().allianceRequestDuration() - 20), // 2s buffersrc/core/CosmeticSchemas.ts (2)
23-44: Make PatternSchema robust without Zod v4-specific helpersTo keep compatibility and avoid depending on
.base64url(), use a simple Base64URL regex before the decoder refine. Also, avoid logging from schema code unless needed.-export const PatternSchema = z - .string() - .max(1403) - .base64url() - .refine( +export const PatternSchema = z + .string() + .max(1403) + // Base64URL charset: A-Z a-z 0-9 - _ + .regex(/^[A-Za-z0-9_-]+$/) + .refine( (val) => { try { new PatternDecoder(val, base64url.decode); return true; } catch (e) { - if (e instanceof Error) { - console.error(JSON.stringify(e.message, null, 2)); - } else { - console.error(String(e)); - } return false; } }, { message: "Invalid pattern", }, );
46-56: Confirm duplication of pattern name in both key and value is intentional
patternsisz.record(z.string(), PatternInfoSchema)whilePatternInfoSchemaalso has anamefield. If keys are the canonical names, consider droppingnamefrom the value to avoid duplication and drift.Happy to provide a follow-up refactor if you want to key strictly by
PatternNameand removenamefrom the payload.src/client/ClientGameRunner.ts (1)
251-262: Consider unsubscribing EventBus listeners on stop() to avoid leaks.Multiple listeners are registered here but never removed when the game stops. If a user joins multiple lobbies in one session, handlers can accumulate.
If EventBus supports an off/unsubscribe API, store the bound handlers and remove them in stop(). Otherwise, add a lightweight dispose mechanism on EventBus to return an unsubscribe function for each registration.
src/client/Transport.ts (1)
653-663: Gate sendMsg on OPEN; buffer during CONNECTING/CLOSING/CLOSED.Current logic buffers only when CLOSED. Sending during CONNECTING may throw and drop messages.
Apply this diff:
- if (this.socket.readyState === WebSocket.CLOSED) { - // Buffer message - console.warn("socket not ready, closing and trying later"); - this.socket.close(); - this.socket = null; - this.connectRemote(this.onconnect, this.onmessage); - this.buffer.push(str); + if (this.socket.readyState !== WebSocket.OPEN) { + // Buffer message + if (this.socket.readyState !== WebSocket.CONNECTING) { + console.warn("socket not ready, closing and trying later"); + this.socket.close(); + this.socket = null; + this.connectRemote(this.onconnect, this.onmessage); + } + this.buffer.push(str); } else { // Send the message directly this.socket.send(str); }src/client/SinglePlayerModal.ts (2)
15-15: Use consistent import path for translateText.The current path resolves but is odd. Prefer the idiomatic local path for consistency with other files.
-import { translateText } from "../client/Utils"; +import { translateText } from "./Utils";
431-451: Deriving pattern by name via cosmetics is correct; watch the async fetch on start.This awaits network I/O before dispatching the join event, which can delay single-player start if the CDN is slow. Consider caching cosmetics or making the fetch lazy/non-blocking for UX.
You can rely on Cosmetics.getCosmetics() caching (see suggested cache in Cosmetics.ts), or optimistically dispatch with patternName and backfill the concrete pattern later if your renderer supports it.
src/client/Cosmetics.ts (2)
26-31: Avoid mutating cosmetics when stripping product; return copies.Mutating
patternData.productchanges shared state for other consumers. Make shallow copies instead.- if (hasAccess) { - // Remove product info because player already has access. - patternData.product = null; - patterns.set(name, patternData); - } else if (patternData.product !== null) { - // Player doesn't have access, but product is available for purchase. - patterns.set(name, patternData); - } + if (hasAccess) { + // Remove product info because player already has access. + patterns.set(name, { ...patternData, product: null }); + } else if (patternData.product !== null) { + // Player doesn't have access, but product is available for purchase. + patterns.set(name, { ...patternData }); + }
80-96: Exporting getCosmetics is good; add a simple in-memory cache to avoid refetch.This endpoint is read-only and a perfect candidate for caching within a session.
Apply this change inside getCosmetics and add a module-scope cache variable.
Add this near the top-level (outside functions):
let cosmeticsCache: Cosmetics | undefined;Then update getCosmetics:
-export async function getCosmetics(): Promise<Cosmetics | undefined> { +export async function getCosmetics(): Promise<Cosmetics | undefined> { + if (cosmeticsCache !== undefined) return cosmeticsCache; try { const response = await fetch(`${getApiBase()}/cosmetics.json`); if (!response.ok) { console.error(`HTTP error! status: ${response.status}`); return; } const result = CosmeticsSchema.safeParse(await response.json()); if (!result.success) { console.error(`Invalid cosmetics: ${result.error.message}`); return; } - return result.data; + cosmeticsCache = result.data; + return cosmeticsCache; } catch (error) { console.error("Error getting cosmetics:", error); } }src/core/Schemas.ts (1)
489-491: Use PatternNameSchema for stricter validation.Right now
patternNameis a plain string. Prefer the shared schema to validate allowed names and avoid typos.Apply this diff:
-import { PatternSchema } from "./CosmeticSchemas"; +import { PatternSchema, PatternNameSchema } from "./CosmeticSchemas"; @@ pattern: PatternSchema.optional(), - patternName: z.string().optional(), + patternName: PatternNameSchema.optional(),src/server/Privilege.ts (2)
10-13: Rename parameter to reflect semantics (name, not base64).Interface still says
base64, but we pass a pattern name everywhere. Rename for clarity and to match the implementation.Apply this diff:
export type PrivilegeChecker = { isPatternAllowed( - base64: string, + name: string, flares: readonly string[] | undefined, ): PatternResult;
28-50: Optional: cache pattern validation to avoid repeated decoding.
PatternDecoderruns on every join. Since cosmetics rarely change at runtime, cache “valid/invalid” per name to cut CPU in hot paths. Composition via a tiny in-memory map is enough.If you want, I can sketch a small LRU/no-eviction cache around
isPatternAllowed.src/client/TerritoryPatternsModal.ts (2)
14-15: Good: use a single BUTTON_WIDTH constant. Also remove the now-unused field.
BUTTON_WIDTHis clear and shared. ThebuttonWidthinstance field looks unused; consider removing it to avoid confusion.Also applies to: 24-25
246-251: Avoid modal flicker when selecting a pattern.Currently
selectPatterncallsrefresh()(which opens the modal) and thenclose(). Guard opening the modal inrefresh()behindisActive, and close before refresh.Apply these diffs:
private selectPattern(pattern: Pattern | undefined) { this.userSettings.setSelectedPatternName(pattern?.name); this.selectedPattern = pattern; - this.refresh(); - this.close(); + this.close(); + this.refresh(); }- // Now modalEl should be available - if (this.modalEl) { - this.modalEl.open(); - } else { - console.warn("modalEl is still null after updateComplete"); - } + // Now modalEl should be available + if (this.isActive) { + if (this.modalEl) { + this.modalEl.open(); + } else { + console.warn("modalEl is still null after updateComplete"); + } + }Also applies to: 305-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
resources/sprites/trainCarriage.pngis excluded by!**/*.pngresources/sprites/trainCarriageLoaded.pngis excluded by!**/*.pngresources/sprites/trainEngine.pngis excluded by!**/*.png
📒 Files selected for processing (28)
eslint.config.js(0 hunks)src/client/ClientGameRunner.ts(1 hunks)src/client/Cosmetics.ts(3 hunks)src/client/FlagInput.ts(1 hunks)src/client/Main.ts(2 hunks)src/client/SinglePlayerModal.ts(5 hunks)src/client/TerritoryPatternsModal.ts(12 hunks)src/client/Transport.ts(1 hunks)src/client/graphics/layers/EventsDisplay.ts(3 hunks)src/core/CosmeticSchemas.ts(2 hunks)src/core/Schemas.ts(3 hunks)src/core/Util.ts(1 hunks)src/core/configuration/Config.ts(2 hunks)src/core/configuration/DefaultConfig.ts(12 hunks)src/core/execution/PortExecution.ts(1 hunks)src/core/execution/TrainStationExecution.ts(3 hunks)src/core/execution/alliance/AllianceRequestExecution.ts(1 hunks)src/core/game/AllianceRequestImpl.ts(2 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameView.ts(1 hunks)src/core/game/PlayerImpl.ts(1 hunks)src/core/game/TrainStation.ts(5 hunks)src/core/game/UserSettings.ts(1 hunks)src/server/MapPlaylist.ts(0 hunks)src/server/Privilege.ts(2 hunks)src/server/worker/websocket/handler/message/PreJoinHandler.ts(2 hunks)tests/AllianceRequestExecution.test.ts(1 hunks)tests/core/game/TrainStation.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/server/MapPlaylist.ts
- eslint.config.js
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-08-16T16:16:00.414Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1758
File: src/core/ApiSchemas.ts:63-72
Timestamp: 2025-08-16T16:16:00.414Z
Learning: In Zod v4, z.enum() can accept TypeScript native enums directly (e.g., z.enum(MyEnum)). The z.nativeEnum() function was deprecated in v4 and replaced by the overloaded z.enum(). This means z.enum(GameType), z.enum(GameMode), etc. are valid in Zod v4.
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-08-14T10:07:44.588Z
Learnt from: woodydrn
PR: openfrontio/OpenFrontIO#1811
File: src/client/FlagInput.ts:0-0
Timestamp: 2025-08-14T10:07:44.588Z
Learning: The codebase uses FlagSchema from "../core/Schemas" for validating flag values. The validation is done using FlagSchema.safeParse(value).success pattern, which is preferred over custom regex validation for flag input validation.
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/Main.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
PR: openfrontio/OpenFrontIO#977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/client/TerritoryPatternsModal.ts
🧬 Code Graph Analysis (16)
tests/AllianceRequestExecution.test.ts (4)
src/core/game/Game.ts (2)
Game(636-720)Player(500-634)tests/util/Setup.ts (2)
setup(23-87)playerInfo(89-91)src/core/execution/alliance/AllianceRequestExecution.ts (1)
AllianceRequestExecution(9-72)src/core/execution/alliance/AllianceRequestReplyExecution.ts (1)
AllianceRequestReplyExecution(3-56)
src/core/game/AllianceRequestImpl.ts (1)
src/core/game/GameImpl.ts (1)
GameImpl(59-902)
src/core/game/GameView.ts (2)
src/core/Schemas.ts (1)
Player(109-109)src/core/game/Game.ts (1)
Player(500-634)
src/core/game/PlayerImpl.ts (1)
src/core/game/AllianceImpl.ts (1)
other(19-24)
src/core/configuration/Config.ts (1)
src/core/game/Game.ts (2)
Tick(16-16)Gold(17-17)
src/client/Cosmetics.ts (1)
src/core/CosmeticSchemas.ts (2)
Pattern(6-6)Cosmetics(5-5)
src/server/worker/websocket/handler/message/PreJoinHandler.ts (1)
src/core/Util.ts (1)
assertNever(218-220)
src/core/game/TrainStation.ts (1)
src/core/game/Game.ts (1)
Player(500-634)
src/client/graphics/layers/EventsDisplay.ts (1)
src/core/game/GameView.ts (1)
GameView(364-696)
src/core/Schemas.ts (1)
src/core/CosmeticSchemas.ts (1)
PatternSchema(23-44)
src/core/execution/alliance/AllianceRequestExecution.ts (1)
src/core/game/Game.ts (5)
Execution(324-329)AllianceRequest(352-359)Game(636-720)Player(500-634)PlayerID(15-15)
src/client/SinglePlayerModal.ts (1)
src/client/Cosmetics.ts (1)
getCosmetics(80-96)
src/server/Privilege.ts (2)
src/core/CosmeticSchemas.ts (1)
Cosmetics(5-5)src/core/PatternDecoder.ts (1)
PatternDecoder(1-56)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/Game.ts (3)
Gold(17-17)Player(500-634)Tick(16-16)src/core/Util.ts (2)
sigmoid(292-298)within(36-38)
src/client/TerritoryPatternsModal.ts (3)
src/core/CosmeticSchemas.ts (1)
Pattern(6-6)src/client/Cosmetics.ts (1)
patterns(9-35)src/core/PatternDecoder.ts (1)
PatternDecoder(1-56)
src/core/CosmeticSchemas.ts (1)
src/core/PatternDecoder.ts (1)
PatternDecoder(1-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (44)
src/core/game/Game.ts (1)
352-359: AllianceRequest.status() addition looks correct and aligns with lifecycleThe new status(): "pending" | "accepted" | "rejected" is a clean, typed-union accessor that fits the execution flow changes.
tests/core/game/TrainStation.test.ts (1)
64-67: Updated allied trade expectations match new payout logicBoth station owner and train owner receiving 1000n for allied trade is consistent with the new relation-based economics.
src/core/game/PlayerImpl.ts (1)
400-403: Allowing sends when there’s an incoming request is correct (auto-accept path)Restricting the “pending” check to only outgoing requests matches the new auto-accept logic in GameImpl. This prevents soft-deadlocks and improves UX.
If you want extra safety, we can constrain to pending only (ar.status() === "pending"). It should be redundant if allianceRequests only stores pending, but harmless.
src/core/game/AllianceRequestImpl.ts (2)
6-7: Good: explicit status state with a typed unionSimple, readable state machine with "pending" | "accepted" | "rejected". This aligns with idiomatic TypeScript and avoids brittle enums.
15-17: Accessor is fineThe public status() accessor is straightforward and side-effect free.
src/core/execution/PortExecution.ts (1)
81-84: Spawn rate now factors in player-owned ports — looks correctPassing both the global trade ship count and the current player’s port count to config.tradeShipSpawnRate aligns with the new API and the intended gameplay scaling.
src/core/execution/TrainStationExecution.ts (3)
1-2: Imports updated to support new spawn logic — OKAdding UnitType and TrainStation imports is consistent with the refactor.
78-78: Call site updated to new zero-arg shouldSpawnTrain() — goodThe method signature change is correctly propagated here.
51-55: No stale trainSpawnRate call sites foundI ran regex searches across all .ts/.tsx files and confirmed there are no calls with zero or multiple arguments. Every usage now correctly passes a single
numPlayerFactoriesvalue.tests/AllianceRequestExecution.test.ts (1)
37-60: Solid coverage of alliance flowsGood tests for:
- Accepting via explicit reply
- Auto-accept by sending the inverse request
Behavior matches the new executions’ logic.src/core/configuration/Config.ts (1)
124-124: New allianceRequestDuration() config hook — OKThe TTL configuration is now exposed. Tests already exercise this surface.
src/core/game/TrainStation.ts (3)
28-31: Relation-based train revenue — sensible and easy to tuneUsing trainGold(rel(trainOwner, stationOwner)) simplifies the logic and makes tuning by relation straightforward. Clean, idiomatic TS with a typed union. Nice.
216-220: Trade filtering limited to City and Port — good simplificationFiltering to City and Port stations before the tradeability check is clear and keeps logic tight.
236-244: Helper rel() using a typed union — exactly the right toolSimple, explicit relation classification that pairs well with config.trainGold. This is clean, testable, and avoids class hierarchies.
src/core/configuration/DefaultConfig.ts (11)
33-34: Good: pull defense debuff shape into named constantsClear, typed constants make tuning easier and safer. No concerns.
366-372: Trade ship gold: safe BigInt conversion and boundsThis looks fine. If dist can be very large, consider clamping baseGold to a max to avoid overflow to Infinity before BigInt conversion.
Would you confirm the expected max dist in production maps so we can decide whether a clamp is necessary?
385-388: Base spawn shape: good use of logistic with a clear midpointThe “half-life” style tuning via LN2/30 is readable. Nice.
408-411: Warship cost wrapper usage looks correctNew
costWrapper(costFn, ...types)usage is idiomatic and keeps costs bounded. LGTM.
428-433: Port cost coupled with Factory count: verify intentional cross-scalingIncluding
UnitType.Factoryin the Port cost calculation means building either increases the other’s cost. If that coupling is intentional for economy pacing, all good; otherwise, split them.Would you confirm this coupling is desired before it ships? Happy to adjust either way.
441-452: Nuke and MIRV costs: constant mapping reads cleanlyUsing fixed cost functions wrapped in
costWrapperis straightforward. No issues.
466-476: Missile/Defense costs and caps: consistent with the new wrapperThe cap and linear growth are reasonable. No concerns.
493-510: City/Factory exponential growth with caps: good balance; double-check UXExponential until 1,000,000 cap is fine. Just ensure build UI communicates when you’ve hit the cap.
Do we have UI feedback for cost caps? If not, I can help add a hint string.
572-574: Alliance request TTL addedMatches AllianceRequestExecution’s expiry logic. No issues.
794-795: Bot max troops reduced to one-thirdThis should ease late-game bot snowballing. Looks good.
846-849: Bot gold drip cut in halfMatches the broader rebalance. LGTM.
src/core/game/GameView.ts (1)
394-398: GameView pattern mapping is correctVerified that the Zod
PlayerSchemastill defines apatternfield (PatternSchema.optional()), notpatternName. Therefore, usingh.patternin GameView matches the current schema and types. No changes are needed here—if the schema gains apatternNamefield in a future PR, update these sites then.• Schema location: src/core/Schemas.ts:382–385
• GameView mapping: src/core/game/GameView.ts:393–398src/core/game/UserSettings.ts (2)
114-116: Rename approved: getter aligns with patternName migrationMethod rename is clear and consistent with the PR direction. Return type and storage key unchanged.
114-124: All old API calls removedI ran a workspace search and found no uses of
getSelectedPattern()orsetSelectedPattern(). All code now calls the new methodsgetSelectedPatternName()andsetSelectedPatternName(). No further action is needed.src/client/graphics/layers/EventsDisplay.ts (2)
68-69: New shouldDelete hook is clean and composableAdding a per-event predicate avoids ad-hoc onDelete wiring and keeps the type simple.
196-200: AllshouldDeletecallbacks are side-effect freeAfter scanning the code, each
shouldDeletehandler (e.g. inEventsDisplay.ts) only reads state and returns a boolean—no mutations were found. The eviction predicate remains pure and safe.src/client/Main.ts (2)
218-224: Centralizing UI updates via territoryModal.refresh() is a good simplification.Nice cleanup versus manual preview/observer management.
461-468: Join payload includes optional patternNameVerified that:
- Client’s
LobbyConfighaspatternName: string | undefined- Server schema (
ClientJoinMessage) definespatternNameasz.string().optional()This aligns correctly—no changes needed.
src/client/Transport.ts (1)
373-384: Manual check: confirmjoinGameuses onlypatternNameThe
rgscan shows manypattern:fields across schemas, server messages, and UI code—but nopattern:inside any join payload. To be safe, please manually verify:
- Open
src/client/Transport.ts- Inspect the
joinGamemethod’s payload: ensure it only sendspatternName(no straypatternkey).- Search the file for any other
sendMsg({ type: "join", … })calls and confirm none includepattern:.Once you’ve confirmed, this can be approved.
src/server/worker/websocket/handler/message/PreJoinHandler.ts (2)
15-15: Good call using assertNever for exhaustiveness.This keeps the discriminated union handling tight and future-proof.
213-237: Clarify “unknown” policy: fail-open (as the comment says) vs fail-closed (current behavior).Your comment says “allow player to join but disable pattern,” but previously the code closed the socket. The diff above implements fail-open (disable the pattern, continue join). If fail-closed is the intended behavior, keep the close and update the comment accordingly.
If you prefer fail-closed, apply this minimal comment fix instead:
- // Api could be down, so allow player to join but disable pattern. + // API could be down; closing the connection to avoid unauthorized join.src/core/Schemas.ts (2)
1-1: Centralizing PatternSchema import is the right move.Good reuse and single source of truth for cosmetics validation.
385-386: Making Player.pattern optional looks fine; ensure readers handle undefined.Downstream renderers and serializers must accept missing patterns without errors.
Would you like me to scan usages of
Player["pattern"]to flag any unsafe assumptions?src/server/Privilege.ts (2)
4-7: Typed union for pattern checks is a clean, idiomatic TypeScript design.This keeps call sites simple and avoids subclass hierarchies. Nice.
127-132: Align “unknown” semantics with PreJoin.
FailOpenPrivilegeCheckerreturnsunknown, butPreJoinHandlercurrently treatsunknownas a close (unless you take the suggested change). Confirm the intended behavior to avoid surprises during partial outages.Would you like me to update
PreJoinHandlerto consistently fail-open and disable the pattern onunknown?src/client/TerritoryPatternsModal.ts (5)
26-26: Type of selectedPattern updated to Pattern | undefined is correct.Leaning on shared types keeps compile-time safety high.
34-35: Patterns as a Map improves lookups and dedupes.This simplifies selection by name and avoids linear scans. Nice.
52-61: onUserMe flow is sensible (clear selection on null, then restore by name).One minor edge: if the stored name no longer exists,
selectedPatternremains undefined, which is fine and falls back to default.
149-152: LGTM: standardized preview sizing via BUTTON_WIDTH.Consistent button/preview sizing helps alignment and reduces magic numbers.
Also applies to: 213-213
355-369: Solid: preview cache key includes size; decode errors are handled.This prevents cache collisions across different sizes and avoids crashes on bad input. Good use of typed unions + try/catch around PatternDecoder.
Also applies to: 406-406
| disconnectedCallback() { | ||
| super.disconnectedCallback(); | ||
| window.removeEventListener("flag-change", this.updateFlag as EventListener); | ||
| window.addEventListener("flag-change", this.updateFlag as EventListener); | ||
| } |
There was a problem hiding this comment.
Remove re-adding the listener in disconnectedCallback; it leaks handlers
You remove the listener and then immediately re-add it on disconnect, keeping the component subscribed after teardown.
Fix by deleting the re-add:
disconnectedCallback() {
super.disconnectedCallback();
window.removeEventListener("flag-change", this.updateFlag as EventListener);
- window.addEventListener("flag-change", this.updateFlag as EventListener);
}📝 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.
| disconnectedCallback() { | |
| super.disconnectedCallback(); | |
| window.removeEventListener("flag-change", this.updateFlag as EventListener); | |
| window.addEventListener("flag-change", this.updateFlag as EventListener); | |
| } | |
| disconnectedCallback() { | |
| super.disconnectedCallback(); | |
| window.removeEventListener("flag-change", this.updateFlag as EventListener); | |
| } |
🤖 Prompt for AI Agents
In src/client/FlagInput.ts around lines 61-65, disconnectedCallback currently
removes the "flag-change" listener and then immediately re-adds it, which leaks
handlers after teardown; update the method to only call
super.disconnectedCallback() and window.removeEventListener("flag-change",
this.updateFlag as EventListener) and delete the window.addEventListener(...)
line so the component is fully unsubscribed on disconnect.
| import { RequiredPatternSchema } from "./Schemas"; | ||
| import { z } from "zod"; | ||
| import { base64url } from "jose"; | ||
| import { z } from "zod/v4"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid relying on 'zod/v4' and .base64url() unless the toolchain is ready
Subpath import and z.string().base64url() are v4-specific and may not exist in your installed Zod. Many files typically import from "zod".
Run to confirm Zod version and usage:
If v4 is not in use, switch to "zod" and validate Base64URL via regex + refine:
-import { z } from "zod/v4";
+import { z } from "zod";And in PatternSchema (see next comment) remove .base64url() and use a regex guard.
🏁 Script executed:
#!/bin/bash
jq -r '.dependencies.zod // .devDependencies.zod // ""' package.json
rg -nP --type=ts 'from\s+"zod/v4"'
rg -nP --type=ts '\.base64url\('Length of output: 253
Replace subpath import with the main “zod” entrypoint
Confirmed that Zod v4 (v4.0.5) is installed, so .base64url() is supported—but our project convention is to import from "zod", not "zod/v4". In src/core/CosmeticSchemas.ts, update:
- import { z } from "zod/v4";
+ import { z } from "zod";No change needed to the .base64url() call here.
Continue using .base64url() as-is or consider a regex + .refine() in a follow-up if you prefer explicit pattern checks.
📝 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.
| import { z } from "zod/v4"; | |
| import { z } from "zod"; |
🤖 Prompt for AI Agents
In src/core/CosmeticSchemas.ts around line 2, the module is imported from the
subpath "zod/v4" but our project convention (and installed package) uses the
main entrypoint "zod"; change the import statement to import { z } from "zod"
(leave the existing .base64url() usage unchanged).
| private active = true; | ||
| private recipient: Player | null = null; | ||
| private mg: Game; |
There was a problem hiding this comment.
Fix: mg must be definitely assigned
Under strictPropertyInitialization, private mg: Game; will fail compile-time checks. Use a definite assignment assertion.
Apply:
- private mg: Game;
+ private mg!: Game;📝 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.
| private mg: Game; | |
| private mg!: Game; |
🤖 Prompt for AI Agents
In src/core/execution/alliance/AllianceRequestExecution.ts around line 12, the
field declaration "private mg: Game;" fails strictPropertyInitialization; change
it to use a definite assignment assertion (e.g., declare it with a trailing
exclamation mark) and ensure the field is assigned before use (typically
initialized inside the constructor or an initialization method) so TypeScript's
strict checks are satisfied.
| if (!mg.hasPlayer(this.recipientID)) { | ||
| console.warn( | ||
| `AllianceRequestExecution recipient ${this.recipientID} not found`, | ||
| ); | ||
| this.active = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Deactivate when recipient is missing to avoid a “zombie” execution
Currently init returns early without setting active = false, so this execution will keep ticking until TTL logic happens to deactivate it. Set inactive immediately.
if (!mg.hasPlayer(this.recipientID)) {
console.warn(
`AllianceRequestExecution recipient ${this.recipientID} not found`,
);
- return;
+ this.active = false;
+ 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.
| if (!mg.hasPlayer(this.recipientID)) { | |
| console.warn( | |
| `AllianceRequestExecution recipient ${this.recipientID} not found`, | |
| ); | |
| this.active = false; | |
| return; | |
| } | |
| if (!mg.hasPlayer(this.recipientID)) { | |
| console.warn( | |
| `AllianceRequestExecution recipient ${this.recipientID} not found`, | |
| ); | |
| this.active = false; | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/core/execution/alliance/AllianceRequestExecution.ts around lines 21-26,
init currently returns early when the recipient is missing but never sets
this.active = false, which leaves the execution ticking as a “zombie”; update
the early-return branch to set this.active = false before returning (and keep
the existing warning log), ensuring the execution is deactivated immediately
when the recipient cannot be found.
| this.req = this.requestor.createAllianceRequest(recipient); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle createAllianceRequest returning null
Player.createAllianceRequest can return null. If it does, there’s nothing to manage; deactivate to avoid leaking this execution.
- this.req = this.requestor.createAllianceRequest(recipient);
+ this.req = this.requestor.createAllianceRequest(recipient);
+ if (!this.req) {
+ console.warn("createAllianceRequest returned null");
+ this.active = false;
+ }📝 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.
| this.req = this.requestor.createAllianceRequest(recipient); | |
| } | |
| } | |
| this.req = this.requestor.createAllianceRequest(recipient); | |
| if (!this.req) { | |
| console.warn("createAllianceRequest returned null"); | |
| this.active = false; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/core/execution/alliance/AllianceRequestExecution.ts around lines 43 to
45, handle the case where this.requestor.createAllianceRequest(recipient) can
return null by checking the return value; if it is null, immediately deactivate
this execution (e.g., call this.deactivate()) and return to avoid leaking the
execution, optionally log a debug/warn indicating no request was created,
otherwise proceed to assign this.req as before.
| if (clientMsg.patternName !== undefined) { | ||
| const result = privilegeRefresher | ||
| .get() | ||
| .isPatternAllowed(clientMsg.pattern, flares); | ||
| if (allowed !== true) { | ||
| log.warn(`Pattern ${allowed}: ${clientMsg.pattern}`); | ||
| return { | ||
| code: 1002, | ||
| error: `The pattern you have selected is ${allowed}.`, | ||
| reason: `Pattern ${allowed}`, | ||
| success: false, | ||
| }; | ||
| .isPatternAllowed(clientMsg.patternName, flares); | ||
| switch (result.type) { | ||
| case "allowed": | ||
| break; | ||
| case "unknown": | ||
| // Api could be down, so allow player to join but disable pattern. | ||
| log.warn(`Pattern ${clientMsg.patternName} unknown`); | ||
| ws.close( | ||
| 1002, | ||
| "Could not look up pattern, backend may be offline", | ||
| ); | ||
| return; | ||
| case "forbidden": | ||
| log.warn(`Pattern ${clientMsg.patternName}: ${result.reason}`); | ||
| ws.close( | ||
| 1002, | ||
| `Pattern ${clientMsg.patternName}: ${result.reason}`, | ||
| ); | ||
| return; | ||
| default: | ||
| assertNever(result); | ||
| } |
There was a problem hiding this comment.
Security: don’t trust client-supplied pattern data; use the server-authoritative pattern.
Right now, even when the name is validated, we still pass clientMsg.pattern into Client. A malicious client could send an allowed patternName but a different base64 pattern. Capture the allowed pattern from the server and pass that instead.
Apply this diff to capture and use the server-authoritative pattern (see also the follow-up change for the Client constructor):
// Check if the pattern is allowed
if (clientMsg.patternName !== undefined) {
const result = privilegeRefresher
.get()
.isPatternAllowed(clientMsg.patternName, flares);
switch (result.type) {
case "allowed":
- break;
+ allowedPatternFromServer = result.pattern;
+ break;
case "unknown":
- // Api could be down, so allow player to join but disable pattern.
- log.warn(`Pattern ${clientMsg.patternName} unknown`);
- ws.close(
- 1002,
- "Could not look up pattern, backend may be offline",
- );
- return;
+ // API could be down. Allow join but disable the pattern.
+ log.warn(`Pattern ${clientMsg.patternName} unknown`);
+ allowedPatternFromServer = undefined;
+ break;
case "forbidden":
log.warn(`Pattern ${clientMsg.patternName}: ${result.reason}`);
ws.close(
1002,
`Pattern ${clientMsg.patternName}: ${result.reason}`,
);
return;
default:
assertNever(result);
}
}Additionally, declare the storage variable once near the top of handleJoinMessage() so it’s in scope for client creation:
// Place near the start of handleJoinMessage(), before the pattern checks:
let allowedPatternFromServer: string | undefined;
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
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