Add support for colored patterns #2062
Conversation
WalkthroughReplaces ad-hoc string pattern/flag fields with a typed cosmetics model (PlayerPattern / PlayerCosmetics), fetches cosmetics (patterns + color palettes) on the client, moves color lookups from Theme to PlayerView, updates decoding/privilege checks for palette-aware patterns, and surfaces palette-aware purchase/preview UI changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Client (Main / Modal)
participant Cosmetics as CosmeticsFetcher
participant WS as Server (Worker)
participant Priv as PrivilegeChecker
participant Game as GameServer
User->>UI: Click "Join" / open pattern modal / purchase
UI->>Cosmetics: fetchCosmetics()
Cosmetics-->>UI: Cosmetics { patterns, colorPalettes }
UI->>UI: getSelectedPatternName(cosmetics) → PlayerPattern?
UI->>WS: WS Join { cosmetics: { flag?, patternName?, patternColorPaletteName? } }
WS->>Priv: isPatternAllowed(flares, name, colorPaletteName)
Priv-->>WS: { allowed: PlayerPattern } / forbidden / unknown
alt allowed
WS->>WS: checkCosmetics → approved PlayerCosmetics
WS->>Game: attach Client(cosmetics)
Game-->>UI: GameStartInfo { players[].cosmetics }
else forbidden/unknown
WS-->>UI: Close WS (invalid cosmetics)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
evanpelle
force-pushed
the
evan-color-pattern
branch
from
adfc4a9 to
539ed48
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/configuration/Config.ts (1)
96-96: RenameattckTroops→attackTroopsin src/core/configuration/Config.tsTypo at src/core/configuration/Config.ts:96 — update the interface so it matches DefaultConfig and callers.
- attckTroops: number, + attackTroops: number,src/core/configuration/PastelTheme.ts (1)
10-14: Fix Map key type: using number keys with Map<string, …>player.id() is numeric; current typing will fail under strict TS.
-type ColorCache = Map<string, Colord>; +type ColorCache = Map<number, Colord>; - private borderColorCache: ColorCache = new Map<string, Colord>(); + private borderColorCache: ColorCache = new Map<number, Colord>();src/client/graphics/layers/UnitLayer.ts (1)
112-136: Guard invalid clicks and ensure only warships are moved
- isOcean(clickRef) may receive undefined; add validity checks.
- Only emit MoveWarshipIntentEvent for warships.
private onMouseUp(event: MouseUpEvent) { @@ - const nearbyWarships = this.findWarshipsNearCell(cell); + const nearbyWarships = this.findWarshipsNearCell(cell); if (this.selectedUnit) { - const clickRef = this.game.ref(cell.x, cell.y); - if (this.game.isOcean(clickRef)) { + const clickRef = this.game.ref(cell.x, cell.y); + if (clickRef !== undefined && + this.selectedUnit.type() === UnitType.Warship && + this.game.isOcean(clickRef)) { this.eventBus.emit( new MoveWarshipIntentEvent(this.selectedUnit.id(), clickRef), ); } // Deselect this.eventBus.emit(new UnitSelectionEvent(this.selectedUnit, false));
🧹 Nitpick comments (40)
src/client/components/PatternButton.ts (4)
118-129: Guard against missing product when showing price.If requiresPurchase is true but product is null/undefined, this will throw.
- (${this.pattern!.product!.price}) + ${this.pattern?.product ? `(${this.pattern.product.price})` : ""}
135-150: Remove stray console.log in render path.Avoid noisy logs in production UIs.
- console.log("renderPatternPreview", pattern);
193-205: Build cache key after normalizing width/height to maximize hits.Today the key uses unnormalized dimensions, so two inputs that round to the same size generate duplicate entries.
- const patternLookupKey = [ - pattern.name, - pattern.colorPalette?.primaryColor ?? "undefined", - pattern.colorPalette?.secondaryColor ?? "undefined", - width, - height, - ].join("-"); - - if (patternCache.has(patternLookupKey)) { - return patternCache.get(patternLookupKey)!; - } + // key computed after decode + normalizationwidth = width === undefined ? scaledWidth : Math.max(1, Math.floor(width / scaledWidth)) * scaledWidth; height = height === undefined ? scaledHeight : Math.max(1, Math.floor(height / scaledHeight)) * scaledHeight; + + const patternLookupKey = [ + pattern.name, + pattern.colorPalette?.primaryColor ?? "undefined", + pattern.colorPalette?.secondaryColor ?? "undefined", + width, + height, + ].join("-"); + if (patternCache.has(patternLookupKey)) { + return patternCache.get(patternLookupKey)!; + }Also applies to: 229-237
248-253: Defensive color parsing.If a palette color is invalid, Colord may yield unexpected RGB. Fall back to defaults.
- const primary = pattern.colorPalette?.primaryColor - ? new Colord(pattern.colorPalette.primaryColor).toRgb() - : DEFAULT_PRIMARY; - const secondary = pattern.colorPalette?.secondaryColor - ? new Colord(pattern.colorPalette.secondaryColor).toRgb() - : DEFAULT_SECONDARY; + const pc = pattern.colorPalette?.primaryColor; + const sc = pattern.colorPalette?.secondaryColor; + const pCol = pc ? new Colord(pc) : null; + const sCol = sc ? new Colord(sc) : null; + const primary = pCol && pCol.isValid() ? pCol.toRgb() : DEFAULT_PRIMARY; + const secondary = sCol && sCol.isValid() ? sCol.toRgb() : DEFAULT_SECONDARY;src/core/CosmeticSchemas.ts (3)
47-51: Validate color strings.Enforce hex to reduce bad inputs.
export const ColorPaletteSchema = z.object({ name: z.string(), - primaryColor: z.string(), - secondaryColor: z.string(), + primaryColor: z.string().regex(/^#([0-9a-fA-F]{6}|[0-9a-fA-F]{3})$/, "Hex color required"), + secondaryColor: z.string().regex(/^#([0-9a-fA-F]{6}|[0-9a-fA-F]{3})$/, "Hex color required"), });
53-66: Harden schemas: reject unknown keys.Prevents stray fields in persisted cosmetics.
-export const PatternSchema = z.object({ +export const PatternSchema = z + .object({ name: PatternNameSchema, pattern: PatternDataSchema, colorPalettes: z .object({ name: z.string(), isArchived: z.boolean().optional(), }) .array() .optional(), affiliateCode: z.string().nullable(), product: ProductSchema.nullable(), -}); + }) + .strict();Optional: also
.strict()onCosmeticsSchematop-level if desired.
24-45: Replace deprecated z.string().base64url() with top-level z.base64url()package.json lists "zod": "^4.0.5" (v4 supports base64url); the instance-method form still works but is deprecated — switch to z.base64url() and keep the .max(1403) + refine(decodePatternData).
Location: src/core/CosmeticSchemas.ts:24-45src/client/graphics/AnimatedSpriteLoader.ts (1)
194-206: Cache key should include colors (and theme tint) to avoid stale sprites.If a player’s palette/theme changes, the cache keeps outdated canvases.
- const spawnHighlightColor = theme.spawnHighlightColor(); - const key = `${fxType}-${owner.id()}`; + const spawnHighlightColor = theme.spawnHighlightColor(); + const key = [ + fxType, + owner.id(), + territoryColor.toHex(), + borderColor.toHex(), + spawnHighlightColor.toHex(), + ].join("-");If cosmetics can change at runtime, also consider a cache clear on cosmetics updates.
src/client/graphics/SpriteLoader.ts (1)
157-166: Cache key misses spawnHighlightColor (possible stale hits).If spawnHighlightColor() changes across themes/sessions, coloredSpriteCache can return a wrong canvas. Include it in the key.
Apply this diff:
-function computeSpriteKey( - unit: UnitView, - territoryColor: Colord, - borderColor: Colord, -): string { +function computeSpriteKey( + unit: UnitView, + territoryColor: Colord, + borderColor: Colord, + highlightColor: Colord, +): string { const owner = unit.owner(); const type = `${unit.type()}-${unit.trainType()}-${unit.isLoaded()}`; - const key = `${type}-${owner.id()}-${territoryColor.toRgbString()}-${borderColor.toRgbString()}`; + const key = `${type}-${owner.id()}-${territoryColor.toRgbString()}-${borderColor.toRgbString()}-${highlightColor.toRgbString()}`; return key; } @@ - const key = computeSpriteKey(unit, territoryColor, borderColor); + const key = computeSpriteKey(unit, territoryColor, borderColor, spawnHighlightColor);Also applies to: 177-179
src/client/graphics/layers/RailroadLayer.ts (2)
119-131: Existing rail tiles aren’t repainted when railType changes.When a tile’s orientation changes but owner stays the same, the canvas won’t update.
Apply this diff:
if (railTile) { - railTile.numOccurence++; - railTile.tile = railRoad; - railTile.lastOwnerId = currentOwner; + const typeChanged = railTile.tile.railType !== railRoad.railType; + railTile.numOccurences++; + railTile.tile = railRoad; + railTile.lastOwnerId = currentOwner; + if (typeChanged) { + this.paintRail(railRoad); + } } else { this.existingRailroads.set(railRoad.tile, { tile: railRoad, - numOccurence: 1, + numOccurences: 1, lastOwnerId: currentOwner, }); this.railTileList.push(railRoad.tile); this.paintRail(railRoad); }Note: pairs with the rename suggested below.
16-19: Spelling: numOccurence → numOccurences.Tidy up naming for clarity (update all references).
Apply this diff:
- numOccurence: number; + numOccurences: number; @@ - railTile.numOccurence++; + railTile.numOccurences++; @@ - if (ref) ref.numOccurence--; + if (ref) ref.numOccurences--; @@ - if (!ref || ref.numOccurence <= 0) { + if (!ref || ref.numOccurences <= 0) {Also applies to: 120-121, 136-139
src/client/graphics/layers/StructureLayer.ts (2)
1-3: Remove unused Theme plumbing.Theme is no longer used in this layer.
Apply this diff:
-import { Theme } from "../../../core/configuration/Config"; @@ - private theme: Theme; @@ - this.theme = game.config().theme();Also applies to: 36-39, 79-80
239-251: Remove unused local variable.color is computed but never used in renderIcon().
Apply this diff:
- ) { - let color = unit.owner().borderColor(); - if (unit.type() === UnitType.Construction) { - color = underConstructionColor; - } + ) {src/client/graphics/layers/StructureIconsLayer.ts (1)
10-10: Drop unused Theme import/field.No theme usage remains here.
Apply this diff:
-import { Theme } from "../../../core/configuration/Config"; @@ - private theme: Theme; @@ - this.theme = game.config().theme();Also applies to: 63-64, 94-95
src/client/Transport.ts (1)
308-317: Preserve message order when flushing buffer.Using pop() reorders buffered messages (LIFO). Use shift() for FIFO.
Apply this diff:
- while (this.buffer.length > 0) { + while (this.buffer.length > 0) { console.log("sending dropped message"); - const msg = this.buffer.pop(); + const msg = this.buffer.shift(); if (msg === undefined) { console.warn("msg is undefined"); continue; } this.socket.send(msg); }src/client/graphics/layers/TerritoryLayer.ts (2)
423-441: Use focused border color and avoid redundant defended checks
- playerIsFocused is computed but unused.
- Call focusedBorderColor() when owner is focused.
- hasUnitNearby() per tile can be heavy; consider caching defended border tiles during the unit-update BFS and pass a boolean, or memoize per tick.
- const playerIsFocused = owner && this.game.focusedPlayer() === owner; + const playerIsFocused = this.game.focusedPlayer() === owner; ... - this.paintTile( - this.imageData, - tile, - owner.borderColor(tile, isDefended), - 255, - ); + this.paintTile( + this.imageData, + tile, + playerIsFocused + ? this.theme.focusedBorderColor() + : owner.borderColor(tile, isDefended), + 255, + );
26-27: Remove unused cache fieldcachedTerritoryPatternsEnabled is no longer used.
- private cachedTerritoryPatternsEnabled: boolean | undefined;src/core/game/UserSettings.ts (3)
117-131: Remove unused paramcosmetics is unused in getDevOnlyPattern().
- getDevOnlyPattern(cosmetics: Cosmetics): PlayerPattern | undefined { + getDevOnlyPattern(): PlayerPattern | undefined {
148-154: Persist palette, not only name (optional)setSelectedPatternName() stores just the name; consider storing palette too (e.g., pattern:NAME:PALETTE) for round‑trip.
132-146: Method name no longer matches return typeIt returns a PlayerPattern, not a name. Consider renaming to getSelectedPattern().
src/client/Main.ts (1)
512-515: Avoid fetching cosmetics inline per joinFetch once, reuse. Reduces latency and network churn.
- this.gameStop = joinLobby( + const cosmetics = await fetchCosmetics(); + this.gameStop = joinLobby( ... - pattern: - this.userSettings.getSelectedPatternName(await fetchCosmetics()) ?? - undefined, + pattern: this.userSettings.getSelectedPatternName(cosmetics) ?? undefined,src/client/graphics/layers/UnitLayer.ts (2)
24-28: Prefer union type over enum for RelationshipKeeps types lightweight and idiomatic TS.
-enum Relationship { - Self, - Ally, - Enemy, -} +type Relationship = "Self" | "Ally" | "Enemy";
546-552: Use sprite.height for heightCurrently width is used twice; non-square sprites would render distorted.
- sprite.width, + sprite.height,src/core/PatternDecoder.ts (3)
10-19: Constructor now couples to PlayerPattern; prefer decoupling.Keep PatternDecoder schema-agnostic. Accept
patternData: stringinstead ofPlayerPattern, and let callers pass the string. This reduces cross‑module coupling and eases reuse.If you keep this API, please confirm all call sites were updated to pass
PlayerPatternobjects.
22-32: Guard negative coordinates; confirm inverted bit is intended.If
x/ycan be negative,%yields negative indices. Also,isPrimaryflips bit logic; verify visuals match expectations.Apply modulo fix:
- const px = (x >> this.scale) % this.width; - const py = (y >> this.scale) % this.height; + const px = ((x >> this.scale) % this.width + this.width) % this.width; + const py = ((y >> this.scale) % this.height + this.height) % this.height;Note: left shifts in scaledWidth/Height are intentional per prior learning; do not change them to multiplication.
43-72: Decoder validation looks solid; small perf tidy.Consider slicing payload once to avoid repeated
3 + byteIndexmath in hot paths:-export function decodePatternData( +export function decodePatternData( b64: string, base64urlDecode: (input: string) => Uint8Array, ): { height: number; width: number; scale: number; bytes: Uint8Array } { const bytes = base64urlDecode(b64); @@ - return { height, width, scale, bytes }; + return { height, width, scale, bytes };Then in the constructor, cache
this.bytesPayload = this.bytes.subarray(3)and read from it inisPrimary. Non‑blocking.src/core/configuration/PastelThemeDark.ts (3)
20-31: Fix noSwitchDeclarations: wrap case block and reuse mag.Declare inside a block to satisfy the linter and avoid leakage across cases. Also reuse
mag.- case TerrainType.Ocean: - case TerrainType.Lake: - const w = this.darkWater.rgba; - if (gm.isShoreline(tile) && gm.isWater(tile)) { - return this.darkShorelineWater; - } - if (gm.magnitude(tile) < 10) { - return colord({ - r: Math.max(w.r + 9 - mag, 0), - g: Math.max(w.g + 9 - mag, 0), - b: Math.max(w.b + 9 - mag, 0), - }); - } - return this.darkWater; + case TerrainType.Ocean: + case TerrainType.Lake: { + const w = this.darkWater.rgba; + if (gm.isShoreline(tile) && gm.isWater(tile)) { + return this.darkShorelineWater; + } + if (mag < 10) { + return colord({ + r: Math.max(w.r + 9 - mag, 0), + g: Math.max(w.g + 9 - mag, 0), + b: Math.max(w.b + 9 - mag, 0), + }); + } + return this.darkWater; + }
12-50: Ensure exhaustive return.If a new TerrainType is added, this can return undefined. Add a default branch or use an exhaustive check.
switch (gm.terrainType(tile)) { @@ - case TerrainType.Mountain: + case TerrainType.Mountain: return colord({ r: 180 + mag / 2, g: 180 + mag / 2, b: 180 + mag / 2, }); + default: + return this.darkWater; }
6-11: Prefer composition over inheritance for themes.Consider making PastelThemeDark a thin wrapper that composes PastelTheme and overrides only
terrainColorvia injected palette/config. Typed unions + data‑driven config are easier to reason about than class hierarchies.src/client/graphics/layers/WinModal.ts (1)
148-155: Use Fisher–Yates instead of sort+random.
sort(() => Math.random() - 0.5)is biased. Use a proper shuffle.- const shuffled = [...purchasablePatterns].sort(() => Math.random() - 0.5); + const shuffled = [...purchasablePatterns]; + for (let i = shuffled.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]; + }src/client/Cosmetics.ts (2)
11-33: Localize user‑visible strings (per PR checklist).Wrap alerts with translateText() and add keys to en.json.
+import { translateText } from "./Utils"; @@ - if (pattern.product === null) { - alert("This pattern is not available for purchase."); + if (pattern.product === null) { + alert(translateText("purchase.not_available")); return; } @@ - if (response.status === 401) { - alert("You are not logged in. Please log in to purchase a pattern."); + if (response.status === 401) { + alert(translateText("purchase.not_logged_in")); } else { - alert("Something went wrong. Please try again later."); + alert(translateText("purchase.generic_error")); }I can add the en.json entries if you want.
74-116: Affiliate filter defaults to block when code is null.
affiliateCode !== pattern.affiliateCodeblocks items ifpattern.affiliateCodeis set and the caller passesnull. Confirm the intended source ofaffiliateCode(e.g., cosmetics root config, query param, or environment) and thread it through callers like WinModal.Consider making
affiliateCodeoptional and treatingundefined/nullas “show all stores” unless explicitly restricted.src/server/Worker.ts (3)
372-382: Close code for policy violations.When cosmetics are forbidden, consider 1008 (Policy Violation) instead of 1002 (Protocol Error):
- ws.close(1002, error); + ws.close(1008, error);
445-445: Avoid console.log in server path.Use the logger to keep logs consistent and filterable.
- console.log("cosmetics", JSON.stringify(cosmetics, replacer)); + log.debug("cosmetics", JSON.stringify(cosmetics, replacer));
430-501: Good: centralized cosmetics check with typed result.Clear allow/forbid path, and structured return fits well. Consider extracting this helper to a module for unit tests.
src/client/SinglePlayerModal.ts (1)
428-433: Good fallback logic; consider prefetching cosmetics.Fetching on click is fine, but you can prefetch/cache cosmetics when the modal opens to avoid a network delay on Start.
src/core/game/GameView.ts (1)
249-253: Guard against invalid dev patterns to avoid constructor throw.
PatternDecoderwill throw on invalid patternData (e.g., dev-only overrides). Wrap in try/catch and fall back gracefully.- this.decoder = - this.cosmetics.pattern === undefined - ? undefined - : new PatternDecoder(this.cosmetics.pattern, base64url.decode); + try { + this.decoder = + this.cosmetics.pattern === undefined + ? undefined + : new PatternDecoder(this.cosmetics.pattern, base64url.decode); + } catch { + this.decoder = undefined; + }src/core/Schemas.ts (1)
364-376: MakeFlagSchemanon-optional at the base to avoid double-optional types leaking intoFlag.Right now
Flag = z.infer<typeof FlagSchema>includesundefined, and you also wrap it in.optional()at use sites, creating nested optionals and confusing types.-export const FlagSchema = z - .string() - .max(128) - .optional() - .refine( +const FlagValueSchema = z + .string() + .max(128) + .refine( (val) => { - if (val === undefined || val === "") return true; + if (val === "") return true; if (val.startsWith("!")) return true; return countryCodes.includes(val); }, { message: "Invalid flag: must be a valid country code or start with !" }, ); +export const FlagSchema = FlagValueSchema.optional();Update usages below to avoid wrapping an already-optional schema in
.optional().-export const PlayerCosmeticRefsSchema = z.object({ - flag: FlagSchema.optional(), +export const PlayerCosmeticRefsSchema = z.object({ + flag: FlagSchema, patternName: PatternNameSchema.optional(), patternColorPaletteName: z.string().optional(), }); export const PlayerCosmeticsSchema = z.object({ - flag: FlagSchema.optional(), + flag: FlagSchema, pattern: PlayerPatternSchema.optional(), });src/client/TerritoryPatternsModal.ts (2)
27-27: InitializeselectedPatternto avoid strict-property errors and undefined flows.This keeps the state predictable before
onUserMe()runs.- @state() private selectedPattern: PlayerPattern | null; + @state() private selectedPattern: PlayerPattern | null = null;
132-143: Persisted key format looks good; tiny nit on clarity.Storing as
pattern:<name>[:<palette>]matchesUserSettings.getSelectedPatternName. Consider extracting a small helper to build/parse this key in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/client/ClientGameRunner.ts(2 hunks)src/client/Cosmetics.ts(3 hunks)src/client/Main.ts(2 hunks)src/client/SinglePlayerModal.ts(3 hunks)src/client/TerritoryPatternsModal.ts(5 hunks)src/client/Transport.ts(1 hunks)src/client/components/PatternButton.ts(7 hunks)src/client/graphics/AnimatedSpriteLoader.ts(1 hunks)src/client/graphics/SpriteLoader.ts(1 hunks)src/client/graphics/layers/RailroadLayer.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(2 hunks)src/client/graphics/layers/StructureLayer.ts(3 hunks)src/client/graphics/layers/TerritoryLayer.ts(1 hunks)src/client/graphics/layers/UILayer.ts(2 hunks)src/client/graphics/layers/UnitLayer.ts(6 hunks)src/client/graphics/layers/WinModal.ts(3 hunks)src/core/CosmeticSchemas.ts(4 hunks)src/core/PatternDecoder.ts(2 hunks)src/core/Schemas.ts(4 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/PastelTheme.ts(2 hunks)src/core/configuration/PastelThemeDark.ts(2 hunks)src/core/game/GameView.ts(4 hunks)src/core/game/UserSettings.ts(2 hunks)src/server/Client.ts(2 hunks)src/server/GameServer.ts(2 hunks)src/server/Privilege.ts(3 hunks)src/server/Worker.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/RailroadLayer.tssrc/client/graphics/SpriteLoader.tssrc/core/configuration/PastelThemeDark.tssrc/core/configuration/PastelTheme.tssrc/core/configuration/Config.tssrc/core/game/GameView.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/UnitLayer.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/core/PatternDecoder.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-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/server/Client.tssrc/server/GameServer.tssrc/core/Schemas.ts
📚 Learning: 2025-06-14T00:56:15.437Z
Learnt from: Aotumuri
PR: openfrontio/OpenFrontIO#786
File: src/core/Cosmetics.ts:1-1
Timestamp: 2025-06-14T00:56:15.437Z
Learning: The `import { base64url } from "jose";` syntax works correctly in ESM environments for the jose library, contrary to potential assumptions about needing specific ESM import paths like "jose/util/base64url".
Applied to files:
src/core/CosmeticSchemas.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-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
🧬 Code graph analysis (22)
src/client/graphics/AnimatedSpriteLoader.ts (2)
src/core/configuration/PastelTheme.ts (2)
territoryColor(43-55)borderColor(62-75)src/core/game/GameView.ts (4)
territoryColor(255-264)owner(105-107)owner(625-627)borderColor(266-278)
src/client/graphics/layers/StructureIconsLayer.ts (3)
src/core/game/GameView.ts (4)
unit(647-649)owner(105-107)owner(625-627)borderColor(266-278)src/client/graphics/layers/StructureLayer.ts (1)
renderIcon(239-269)src/core/configuration/PastelTheme.ts (1)
borderColor(62-75)
src/client/graphics/layers/StructureLayer.ts (1)
src/core/configuration/PastelTheme.ts (1)
borderColor(62-75)
src/client/components/PatternButton.ts (3)
src/core/CosmeticSchemas.ts (3)
ColorPalette(10-10)Pattern(7-7)DefaultPattern(92-96)src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/PatternDecoder.ts (1)
PatternDecoder(3-41)
src/client/ClientGameRunner.ts (1)
src/core/Schemas.ts (1)
PlayerPattern(115-115)
src/core/PatternDecoder.ts (1)
src/core/Schemas.ts (1)
PlayerPattern(115-115)
src/client/graphics/SpriteLoader.ts (1)
src/core/configuration/PastelTheme.ts (1)
borderColor(62-75)
src/core/game/UserSettings.ts (2)
src/core/CosmeticSchemas.ts (1)
Cosmetics(6-6)src/core/Schemas.ts (1)
PlayerPattern(115-115)
src/client/SinglePlayerModal.ts (1)
src/client/Cosmetics.ts (1)
fetchCosmetics(55-72)
src/server/Client.ts (1)
src/core/Schemas.ts (1)
PlayerCosmetics(113-113)
src/client/Main.ts (1)
src/client/Cosmetics.ts (1)
fetchCosmetics(55-72)
src/client/Cosmetics.ts (3)
src/client/components/PatternButton.ts (1)
handlePurchase(61-66)src/core/CosmeticSchemas.ts (4)
Pattern(7-7)ColorPalette(10-10)Cosmetics(6-6)CosmeticsSchema(68-90)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
src/core/configuration/PastelThemeDark.ts (2)
src/core/configuration/PastelTheme.ts (1)
PastelTheme(12-172)src/core/game/GameMap.ts (2)
GameMap(6-51)TileRef(3-3)
src/core/CosmeticSchemas.ts (2)
src/core/PatternDecoder.ts (1)
decodePatternData(43-72)src/core/Schemas.ts (1)
PlayerPattern(115-115)
src/server/Worker.ts (2)
src/core/Schemas.ts (3)
PlayerCosmeticRefs(114-114)PlayerCosmetics(113-113)PlayerPattern(115-115)src/core/Util.ts (2)
replacer(279-281)assertNever(210-212)
src/server/Privilege.ts (2)
src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/PatternDecoder.ts (1)
PatternDecoder(3-41)
src/core/configuration/PastelTheme.ts (1)
src/core/game/GameView.ts (3)
territoryColor(255-264)player(585-591)PlayerView(177-434)
src/core/Schemas.ts (1)
src/core/CosmeticSchemas.ts (3)
PatternNameSchema(19-22)PatternDataSchema(24-45)ColorPaletteSchema(47-51)
src/core/configuration/Config.ts (2)
src/core/game/GameView.ts (2)
PlayerView(177-434)territoryColor(255-264)src/core/configuration/PastelTheme.ts (1)
territoryColor(43-55)
src/core/game/GameView.ts (6)
src/core/configuration/PastelTheme.ts (1)
territoryColor(43-55)src/core/CosmeticSchemas.ts (1)
ColorPalette(10-10)src/core/PatternDecoder.ts (2)
PatternDecoder(3-41)isPrimary(22-32)src/core/game/GameMap.ts (3)
TileRef(3-3)x(122-124)y(126-128)src/core/game/GameImpl.ts (2)
x(780-782)y(783-785)src/core/Schemas.ts (1)
PlayerCosmetics(113-113)
src/client/graphics/layers/WinModal.ts (3)
src/client/Cosmetics.ts (3)
fetchCosmetics(55-72)patternRelationship(74-116)handlePurchase(11-53)src/core/CosmeticSchemas.ts (2)
Pattern(7-7)ColorPalette(10-10)src/client/components/PatternButton.ts (1)
handlePurchase(61-66)
src/client/TerritoryPatternsModal.ts (5)
src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/CosmeticSchemas.ts (3)
Cosmetics(6-6)Pattern(7-7)ColorPalette(10-10)src/core/game/UserSettings.ts (1)
UserSettings(6-155)src/client/Cosmetics.ts (3)
fetchCosmetics(55-72)patternRelationship(74-116)handlePurchase(11-53)src/client/components/PatternButton.ts (2)
handlePurchase(61-66)renderPatternPreview(135-150)
🪛 Biome (2.1.2)
src/core/configuration/PastelThemeDark.ts
[error] 20-20: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (27)
src/client/graphics/AnimatedSpriteLoader.ts (1)
191-193: LGTM: use owner-derived colors.Matches the new composition-first color API on PlayerView.
src/client/graphics/layers/UILayer.ts (2)
146-150: LGTM: border color from owner.Consistent with the new PlayerView color API; keeps Theme out of UI math.
210-215: LGTM: selection color based on owner’s territory color.Simple, readable, and keeps logic close to data.
src/core/configuration/Config.ts (2)
180-191: Verify: defendedBorderColors signature changed to accept Colord — update implementations & call sitesSandbox search skipped files; I couldn't confirm usage. Verify every Theme implementation and every call site now pass a Colord (not PlayerView) — this is a breaking API change. Consider renaming the parameter to baseColor for clarity.
51-51: apiKey() is server-only — verified (take the precautions below)
- Findings: only runtime call is server-side: src/server/Archive.ts calls config.apiKey().
- Notes: client code constructs ServerConfig via src/core/configuration/ConfigLoader.ts; DefaultServerConfig.apiKey() reads process.env.API_KEY (src/core/configuration/DefaultConfig.ts) and DevServerConfig contains a hard-coded dev string (src/core/configuration/DevConfig.ts). webpack.DefinePlugin (webpack.config.js) does not inject API_KEY into the client bundle.
- Actions: do not call config.apiKey() from any client code; do not add API_KEY to DefinePlugin; remove any real secret literals from DevServerConfig or move secret access to a server-only module not included in the client bundle.
src/client/graphics/SpriteLoader.ts (1)
175-176: Switching to owner-based colors is correct.Using unit.owner().territoryColor()/borderColor() keeps this path consistent with the new PlayerView color API.
src/client/graphics/layers/RailroadLayer.ts (1)
156-157: Correct: use recipient.railroadColor().This aligns the layer with PlayerView-derived cosmetics.
src/client/graphics/layers/StructureLayer.ts (1)
193-195: Good move to owner-based colors.Territory and border color resolution via PlayerView is consistent with the new model.
Also applies to: 206-207
src/client/graphics/layers/StructureIconsLayer.ts (1)
346-349: Icon coloring now uses owner.borderColor(): LGTM.Cache key and fill/border derive from PlayerView; matches the rest of the refactor.
Also applies to: 389-396
src/server/GameServer.ts (2)
28-31: GamePhase.Finished + cosmetics in start info: looks good.Players carry cosmetics in the start payload; enum addition is straightforward.
Also applies to: 386-394
476-479: Confirm logging intent for empty games in Dev.The log now appears only outside Dev. If the goal was to reduce Dev noise, great; if we wanted the opposite, flip the condition.
Would you like me to adjust it to only log in Dev builds?
src/server/Client.ts (1)
4-4: Client now holds PlayerCosmetics: OK.Matches server start payload and join flow.
Also applies to: 22-23
src/client/Transport.ts (1)
371-375: Approve: join payload cosmetics match ClientJoinMessage schemaClientJoinMessageSchema uses PlayerCosmeticRefsSchema.optional(); the sent object (flag, patternName, patternColorPaletteName) matches that refs shape — no changes required.
src/client/ClientGameRunner.ts (1)
8-8: Type migration to PlayerPattern looks good.API surface stays stable for callers; Transport adapts the join payload.
Run to find any lingering string-based pattern usage:
Also applies to: 51-51
src/client/graphics/layers/TerritoryLayer.ts (1)
446-447: LGTM: interior tiles now use owner.territoryColor(tile)Consistent with PlayerView color API and palette-aware rendering.
src/core/configuration/PastelTheme.ts (4)
57-59: territorySecondaryColor(): clear and simpleDarken by 0.125 is fine for contrast and keeps palettes cohesive.
77-85: defendedBorderColors() API shift looks goodAccepts a Colord and derives light/dark variants. Matches PlayerView usage.
If Theme is extended elsewhere, confirm all subclasses updated to the new signature.
61-75: Comment “Don’t call directly”Border color is now sourced through PlayerView elsewhere. Good guardrail.
39-55: No change required — assignTeamPlayerColor exists
ColorAllocator.ts defines assignTeamPlayerColor(team: Team, playerId: string) and PastelTheme.ts correctly calls it (src/core/configuration/ColorAllocator.ts:89; src/core/configuration/PastelTheme.ts:46).Likely an incorrect or invalid review comment.
src/client/graphics/layers/UnitLayer.ts (1)
204-208: LGTM: color sourcing via PlayerViewConsistent use of owner().territoryColor()/borderColor() aligns with palette-aware rendering.
Also applies to: 325-333, 373-376, 411-415, 433-435, 456-458
src/client/Main.ts (1)
507-525: Confirm join payload change (pattern vs patternName)Verified — client LobbyConfig expects a PlayerPattern and Main passes a PlayerPattern (via userSettings); Transport converts that to patternName/patternColorPaletteName for the server; Worker accepts patternName and resolves it into a PlayerPattern. No schema mismatch found. (See: src/client/ClientGameRunner.ts, src/client/Transport.ts, src/server/Worker.ts, src/core/Schemas.ts).
src/client/graphics/layers/WinModal.ts (2)
157-168: LGTM: palette‑aware purchase flow is wired correctly.
PatternButtonreceives bothpatternandcolorPalette, andhandlePurchaseis called with both. Looks consistent with Cosmetics.ts.
117-141: Affiliate code passed as null — confirm intendedpatternRelationship(...) (declared in src/client/Cosmetics.ts) is called here with affiliateCode = null:
if ( patternRelationship( pattern, colorPalette, me !== false ? me : null, null, ) === "purchasable" ) { ... }rg shows another call in src/client/TerritoryPatternsModal.ts. If patternRelationship filters by affiliateCode, passing null here will hide affiliate-only patterns — confirm whether this modal should include affiliate patterns or should pass a non-null affiliateCode instead.
src/client/Cosmetics.ts (1)
55-72: Graceful null on errors is good.Returning
nullfromfetchCosmetics()simplifies call sites. Logging looks sufficient.src/server/Worker.ts (1)
142-146: Log gating by env looks fine.Only logs outside Dev. Good for local noise reduction.
src/client/TerritoryPatternsModal.ts (1)
63-86: Pattern grid logic is solid.Iterating (pattern × palette|null) and gating with
patternRelationshipis clean. Passing the resolved palette object to the button is correct.src/client/SinglePlayerModal.ts (1)
445-451: Nested cosmetics payload — confirmed downstream handlers accept the new shape.
Join/host paths emit players[].cosmetics and server/client code (src/client/SinglePlayerModal.ts, src/client/Transport.ts, src/server/Worker.ts, src/server/Client.ts, src/server/GameServer.ts) plus core/Schemas.ts use the cosmetics object; no legacy players[].flag or players[].pattern accesses found.
evanpelle
force-pushed
the
evan-color-pattern
branch
from
539ed48 to
edc7ca5
Compare
evanpelle
force-pushed
the
evan-color-pattern
branch
from
edc7ca5 to
02c9676
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/server/Privilege.ts (1)
28-65: Fix signature mismatch and null-unsafe flare checks (compile error + runtime risk).Accept
flaresas optional, guard all checks, and avoid indexing color palettes with an empty string.- isPatternAllowed( - flares: readonly string[], - name: string, - colorPaletteName: string | null, - ): PatternResult { + isPatternAllowed( + flares: readonly string[] | undefined, + name: string, + colorPaletteName: string | null, + ): PatternResult { + const has = (flare: string) => (flares?.includes(flare) ?? false); // Look for the pattern in the cosmetics.json config const found = this.cosmetics.patterns[name]; if (!found) return { type: "forbidden", reason: "pattern not found" }; @@ - const colorPalette = this.cosmetics.colorPalettes?.[colorPaletteName ?? ""]; + const colorPalette = colorPaletteName + ? this.cosmetics.colorPalettes?.[colorPaletteName] + : undefined; @@ - if (flares.includes("pattern:*")) { + if (has("pattern:*")) { return { type: "allowed", pattern: { name: found.name, patternData: found.pattern, colorPalette }, }; } @@ - if (flares.includes(flareName) || flares.includes("pattern:*")) { + if (has(flareName)) { // Player has a flare for this pattern return { type: "allowed", pattern: { name: found.name, patternData: found.pattern, colorPalette }, }; } else { return { type: "forbidden", reason: "no flares for pattern" }; } }
🧹 Nitpick comments (19)
src/client/graphics/layers/UILayer.ts (2)
143-146: Remove unnecessary theme guard in drawIcon (can skip rendering).drawIcon no longer uses theme; returning early if theme is null can suppress icons. Keep only the context check.
- if (this.context === null || this.theme === null) { + if (this.context === null) { return; }
209-213: Delete stray theme throw in drawSelectionBox.Selection box now resolves colors via unit.owner().territoryColor(); this theme check can crash without providing value.
- if (this.theme === null) throw new Error("missing theme");src/core/configuration/PastelTheme.ts (2)
73-80: Param name mismatch: this takes a border color, not a territory color.PlayerView passes a border color. Rename the parameter for clarity and keep the same behavior.
- defendedBorderColors(territoryColor: Colord): { + defendedBorderColors(borderColor: Colord): { light: Colord; dark: Colord; } { return { - light: territoryColor.darken(0.2), - dark: territoryColor.darken(0.4), + light: borderColor.darken(0.2), + dark: borderColor.darken(0.4), }; }
57-71: Documented “don’t call directly” — consider enforcing at the type level.If possible, move borderColor out of the public Theme interface and route callers through PlayerView to prevent misuse. If not feasible now, add a TSDoc @internal tag.
src/client/Cosmetics.ts (1)
15-18: Localize user-facing alerts.Alerts should go through translateText() and be added to en.json to meet the PR checklist.
@@ -import { getApiBase, getAuthHeader } from "./jwt"; +import { getApiBase, getAuthHeader } from "./jwt"; +import { translateText } from "./Utils"; @@ - if (pattern.product === null) { - alert("This pattern is not available for purchase."); + if (pattern.product === null) { + alert(translateText("cosmetics.pattern_not_for_sale")); return; } @@ - if (response.status === 401) { - alert("You are not logged in. Please log in to purchase a pattern."); - } else { - alert("Something went wrong. Please try again later."); - } + if (response.status === 401) { + alert(translateText("cosmetics.login_to_purchase")); + } else { + alert(translateText("cosmetics.purchase_error_generic")); + }Keys to add in en.json:
- cosmetics.pattern_not_for_sale
- cosmetics.login_to_purchase
- cosmetics.purchase_error_generic
Also applies to: 41-45
src/client/SinglePlayerModal.ts (1)
429-431: Naming nit: getSelectedPatternName returns a pattern.Consider renaming to getSelectedPattern for clarity.
src/core/configuration/PastelThemeDark.ts (4)
18-32: Fix switch-case declaration scope to satisfy lint and avoid accidental leakage.Wrap the Ocean/Lake clause in a block so
const wis scoped to that clause only.- case TerrainType.Ocean: - case TerrainType.Lake: - const w = this.darkWater.rgba; + case TerrainType.Ocean: + case TerrainType.Lake: { + const w = this.darkWater.rgba; @@ - if (gm.magnitude(tile) < 10) { + if (mag < 10) { @@ - return this.darkWater; + return this.darkWater; + }
12-12: Mark method as override for safer API evolution.Add
overrideto catch signature drift at compile time.- terrainColor(gm: GameMap, tile: TileRef): Colord { + override terrainColor(gm: GameMap, tile: TileRef): Colord {
7-10: Make palette fields readonly.These are constants; mark as
readonlyto communicate intent and prevent mutation.- private darkShore = colord({ r: 134, g: 133, b: 88 }); + private readonly darkShore = colord({ r: 134, g: 133, b: 88 }); @@ - private darkWater = colord({ r: 14, g: 11, b: 30 }); - private darkShorelineWater = colord({ r: 50, g: 50, b: 50 }); + private readonly darkWater = colord({ r: 14, g: 11, b: 30 }); + private readonly darkShorelineWater = colord({ r: 50, g: 50, b: 50 });
6-6: Prefer composition over inheritance (optional).Consider extracting a “DarkPalette” value object and composing the base theme with it, instead of subclassing. This keeps the surface area flat and favors typed unions over class hierarchies.
src/core/game/GameView.ts (4)
1-1: Import Colord as a type-only import.Avoids pulling the constructor symbol at runtime; we only need the type.
-import { Colord, colord } from "colord"; +import { colord, type Colord } from "colord";
200-209: Avoid mutating incoming cosmetics; compute a local palette instead.Mutating
pattern.colorPalettein the view layer can surprise other consumers. Derive a localpaletteand use it to compute colors.- const pattern = this.cosmetics.pattern; - if (pattern) { - const territoryColor = this.game.config().theme().territoryColor(this); - pattern.colorPalette ??= { + const pattern = this.cosmetics.pattern; + const derivedPalette = + pattern && pattern.colorPalette === undefined + ? ({ name: "", primaryColor: territoryColor.toHex(), secondaryColor: territoryColor.darken(0.125).toHex(), - } satisfies ColorPalette; - } + } satisfies ColorPalette) + : pattern?.colorPalette;Follow-up: Use
derivedPalette ?? pattern?.colorPalettebelow when choosing_territoryColor/_borderColor.
210-229: Honor team colors; keep the intent explicit.Logic reads well. Please refactor to use the locally derived palette (see above) so team override vs. palette fallback is explicit and side-effect free.
253-264: Cheaper parity check for defended borders.You can avoid two modulo checks by using a single parity expression.
- const x = this.game.x(tile); - const y = this.game.y(tile); - const lightTile = - (x % 2 === 0 && y % 2 === 0) || (y % 2 === 1 && x % 2 === 1); + const x = this.game.x(tile); + const y = this.game.y(tile); + const lightTile = ((x + y) & 1) === 0;src/core/Schemas.ts (1)
364-375: Move .optional() to the end for cleaner validation flow.Refining after
.optional()evaluates the predicate onundefined. Put.optional()last so undefined skips all checks.-export const FlagSchema = z - .string() - .max(128) - .optional() - .refine( +export const FlagSchema = z + .string() + .max(128) + .refine( (val) => { if (val === undefined || val === "") return true; if (val.startsWith("!")) return true; return countryCodes.includes(val); }, { message: "Invalid flag: must be a valid country code or start with !" }, - ); + ) + .optional();src/client/TerritoryPatternsModal.ts (4)
27-28: Initialize state to satisfy strictPropertyInitialization and avoid undefined reads.- @state() private selectedPattern: PlayerPattern | null; + @state() private selectedPattern: PlayerPattern | null = null;
120-124: Eager-load cosmetics on open if not already loaded.Prevents an empty grid when
onUserMehasn’t run yet.public async open(affiliateCode?: string) { this.isActive = true; this.affiliateCode = affiliateCode ?? null; - await this.refresh(); + if (this.cosmetics === null) { + this.cosmetics = await fetchCosmetics(); + this.selectedPattern = + this.userSettings.getSelectedPatternName(this.cosmetics); + } + await this.refresh(); }
63-87: Color palette lookup: avoid indexing with empty string.When
colorPaletteis null (legacy/no-color), passnullthrough without?? ""to prevent accidental lookups.- .colorPalette=${this.cosmetics?.colorPalettes?.[ - colorPalette?.name ?? "" - ] ?? null} + .colorPalette=${colorPalette + ? this.cosmetics?.colorPalettes?.[colorPalette.name] ?? null + : null}
148-150: Preview rendering is fine; minor tidy (optional).
this.selectedPattern ?? nullis redundant; passthis.selectedPattern.- const preview = renderPatternPreview(this.selectedPattern ?? null, 48, 48); + const preview = renderPatternPreview(this.selectedPattern, 48, 48);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/client/ClientGameRunner.ts(2 hunks)src/client/Cosmetics.ts(3 hunks)src/client/Main.ts(2 hunks)src/client/SinglePlayerModal.ts(3 hunks)src/client/TerritoryPatternsModal.ts(5 hunks)src/client/Transport.ts(1 hunks)src/client/components/PatternButton.ts(7 hunks)src/client/graphics/AnimatedSpriteLoader.ts(1 hunks)src/client/graphics/SpriteLoader.ts(1 hunks)src/client/graphics/layers/RailroadLayer.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(2 hunks)src/client/graphics/layers/StructureLayer.ts(3 hunks)src/client/graphics/layers/TerritoryLayer.ts(1 hunks)src/client/graphics/layers/UILayer.ts(2 hunks)src/client/graphics/layers/UnitLayer.ts(6 hunks)src/client/graphics/layers/WinModal.ts(3 hunks)src/core/CosmeticSchemas.ts(4 hunks)src/core/PatternDecoder.ts(2 hunks)src/core/Schemas.ts(4 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/PastelTheme.ts(2 hunks)src/core/configuration/PastelThemeDark.ts(2 hunks)src/core/game/GameView.ts(4 hunks)src/core/game/UserSettings.ts(2 hunks)src/server/Client.ts(2 hunks)src/server/GameServer.ts(2 hunks)src/server/Privilege.ts(3 hunks)src/server/Worker.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- src/client/graphics/layers/StructureLayer.ts
- src/client/graphics/layers/UnitLayer.ts
- src/client/graphics/layers/StructureIconsLayer.ts
- src/client/graphics/layers/TerritoryLayer.ts
- src/client/graphics/AnimatedSpriteLoader.ts
- src/client/ClientGameRunner.ts
- src/server/Worker.ts
- src/client/graphics/layers/RailroadLayer.ts
- src/server/Client.ts
- src/core/PatternDecoder.ts
- src/client/graphics/SpriteLoader.ts
- src/core/game/UserSettings.ts
- src/client/components/PatternButton.ts
- src/core/configuration/Config.ts
- src/server/GameServer.ts
- src/client/graphics/layers/WinModal.ts
- src/client/Main.ts
- src/core/CosmeticSchemas.ts
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/client/graphics/layers/UILayer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/core/configuration/PastelThemeDark.tssrc/core/game/GameView.tssrc/core/configuration/PastelTheme.ts
📚 Learning: 2025-06-12T23:22:55.328Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for `roles`, `flares`, and `pattern` are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
Applied to files:
src/server/Privilege.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.ts
📚 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-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
🧬 Code graph analysis (8)
src/client/Cosmetics.ts (3)
src/client/components/PatternButton.ts (1)
handlePurchase(61-66)src/core/CosmeticSchemas.ts (4)
Pattern(7-7)ColorPalette(10-10)Cosmetics(6-6)CosmeticsSchema(68-90)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
src/core/configuration/PastelThemeDark.ts (2)
src/core/configuration/PastelTheme.ts (1)
PastelTheme(12-158)src/core/game/GameMap.ts (2)
GameMap(6-51)TileRef(3-3)
src/server/Privilege.ts (2)
src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/PatternDecoder.ts (1)
PatternDecoder(3-41)
src/client/SinglePlayerModal.ts (1)
src/client/Cosmetics.ts (1)
fetchCosmetics(55-72)
src/core/game/GameView.ts (6)
src/core/configuration/PastelTheme.ts (1)
territoryColor(43-55)src/core/CosmeticSchemas.ts (1)
ColorPalette(10-10)src/core/PatternDecoder.ts (2)
PatternDecoder(3-41)isPrimary(22-32)src/core/game/GameMap.ts (3)
TileRef(3-3)x(122-124)y(126-128)src/core/game/GameImpl.ts (2)
x(780-782)y(783-785)src/core/Schemas.ts (1)
PlayerCosmetics(113-113)
src/client/TerritoryPatternsModal.ts (3)
src/core/game/UserSettings.ts (1)
UserSettings(6-155)src/client/Cosmetics.ts (3)
fetchCosmetics(55-72)patternRelationship(74-116)handlePurchase(11-53)src/client/components/PatternButton.ts (2)
handlePurchase(61-66)renderPatternPreview(135-150)
src/core/Schemas.ts (1)
src/core/CosmeticSchemas.ts (3)
PatternNameSchema(19-22)PatternDataSchema(24-45)ColorPaletteSchema(47-51)
src/core/configuration/PastelTheme.ts (1)
src/core/game/GameView.ts (3)
territoryColor(242-251)player(568-574)PlayerView(177-417)
🪛 Biome (2.1.2)
src/core/configuration/PastelThemeDark.ts
[error] 20-20: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (8)
src/client/Cosmetics.ts (2)
55-72: Null-returning fetchCosmetics is fine; callers handle it.Good switch from undefined to null and guarded parse/logging.
74-116: patternRelationship rules look good — confirm affiliateCode null/empty semanticsCurrent code uses strict compare (affiliateCode !== pattern.affiliateCode). '' or undefined will not equal null, so an empty string could be blocked while null would pass. If product policy expects “show” when both sides are empty/null, normalize before comparing (e.g. treat ''/undefined as null or compare trimmed strings).
Location: src/client/Cosmetics.ts (patternRelationship).src/client/SinglePlayerModal.ts (2)
445-451: Join-lobby payload cosmetics block — aligned with new model.Nested cosmetics with flag and optional pattern look correct.
424-433: Confirm UserSettings API compatibility — no changes needed.
- getSelectedPatternName(cosmetics: Cosmetics | null): PlayerPattern | null — exists in src/core/game/UserSettings.ts:132 and accepts null.
- getDevOnlyPattern(cosmetics: Cosmetics): PlayerPattern | undefined — exists in src/core/game/UserSettings.ts:118; SinglePlayerModal only calls it when cosmetics is non-null (src/client/SinglePlayerModal.ts:429-432), so signature matches.
- Main passes pattern as getSelectedPatternName(...) ?? undefined (src/client/Main.ts:513-514), so JoinLobbyEvent/GameStartInfo accepts undefined.
src/client/Transport.ts (1)
371-376: Join payload nested cosmetics confirmed
Server handlers and schemas exclusively usecosmetics.flag,cosmetics.patternName, andcosmetics.patternColorPaletteNamewith no leftover root-level fields—shape is fully aligned.src/core/game/GameView.ts (1)
225-229: Confirm UX: focused border color vs. user palette.Current rule uses focused border color only when no secondary color is present. Confirm this matches the design spec.
src/client/TerritoryPatternsModal.ts (1)
132-143: Persisted key composition looks good; keep the scheme stable.The stored key format
pattern:<name>[:<palette>]matches retrieval logic in UserSettings. No changes needed.Please confirm no other code parses this key differently.
src/core/Schemas.ts (1)
500-502: No action needed:checkCosmeticsin src/server/Worker.ts already reads patternName and patternColorPaletteName and returns full PlayerCosmetics before clients are broadcastedLikely an incorrect or invalid review comment.
evanpelle
force-pushed
the
evan-color-pattern
branch
from
02c9676 to
c2f4c1f
Compare
evanpelle
force-pushed
the
evan-color-pattern
branch
from
c2f4c1f to
9e62a35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/server/Privilege.ts (1)
28-32: Fix method signature mismatch and add null guard for flares.The implementation doesn't match the interface signature - the interface allows
undefinedfor flares but the implementation doesn't handle it. This will cause runtime errors whenflaresis undefined.isPatternAllowed( - flares: readonly string[], + flares: readonly string[] | undefined, name: string, colorPaletteName: string | null, ): PatternResult { + const hasFlare = (flare: string) => flares?.includes(flare) ?? false; + // Look for the pattern in the cosmetics.json config const found = this.cosmetics.patterns[name]; if (!found) return { type: "forbidden", reason: "pattern not found" }; try { decodePatternData(found.pattern, this.b64urlDecode); } catch (e) { return { type: "forbidden", reason: "invalid pattern" }; } const colorPalette = this.cosmetics.colorPalettes?.[colorPaletteName ?? ""]; - if (flares.includes("pattern:*")) { + if (hasFlare("pattern:*")) { return { type: "allowed", pattern: { name: found.name, patternData: found.pattern, colorPalette }, }; } const flareName = `pattern:${found.name}` + (colorPaletteName ? `:${colorPaletteName}` : ""); - if (flares.includes(flareName)) { + if (hasFlare(flareName)) { // Player has a flare for this pattern return { type: "allowed", pattern: { name: found.name, patternData: found.pattern, colorPalette }, }; } else { return { type: "forbidden", reason: "no flares for pattern" }; } }src/core/CosmeticSchemas.ts (1)
4-4: Use type-only import to avoid runtime cycles.This import is only for
satisfies. Prevents accidental runtime dependency.-import { PlayerPattern } from "./Schemas"; +import type { PlayerPattern } from "./Schemas";
🧹 Nitpick comments (7)
src/core/configuration/PastelThemeDark.ts (1)
20-20: Fix switch declaration scope issue.The variable
wis declared in a switch case without block scope, which can lead to unexpected behavior if other cases are added later.case TerrainType.Ocean: case TerrainType.Lake: + { const w = this.darkWater.rgba; if (gm.isShoreline(tile) && gm.isWater(tile)) { return this.darkShorelineWater; } if (gm.magnitude(tile) < 10) { return colord({ r: Math.max(w.r + 9 - mag, 0), g: Math.max(w.g + 9 - mag, 0), b: Math.max(w.b + 9 - mag, 0), }); } return this.darkWater; + }src/server/Privilege.ts (1)
142-146: Update FailOpenPrivilegeChecker to match interface signature.The FailOpenPrivilegeChecker implementation also needs to accept
undefinedfor flares to match the interface.isPatternAllowed( - flares: readonly string[], + flares: readonly string[] | undefined, name: string, colorPaletteName: string | null, ): PatternResult { return { type: "unknown" }; }src/core/game/GameView.ts (1)
200-208: Consider extracting default color palette creation.The logic for creating a default color palette could be extracted to improve readability.
+ private createDefaultColorPalette(territoryColor: Colord): ColorPalette { + return { + name: "", + primaryColor: territoryColor.toHex(), + secondaryColor: territoryColor.darken(0.125).toHex(), + }; + } const pattern = this.cosmetics.pattern; if (pattern) { const territoryColor = this.game.config().theme().territoryColor(this); - pattern.colorPalette ??= { - name: "", - primaryColor: territoryColor.toHex(), - secondaryColor: territoryColor.darken(0.125).toHex(), - } satisfies ColorPalette; + pattern.colorPalette ??= this.createDefaultColorPalette(territoryColor); }src/client/graphics/layers/WinModal.ts (1)
117-141: Consider extracting the purchasable patterns logic.The nested loop logic for building purchasable patterns could be extracted to a helper function for better readability and reusability.
Consider extracting to a helper:
+function getPurchasablePatterns( + cosmetics: Cosmetics | null, + userMeResponse: UserMeResponse | null, +): { pattern: Pattern; colorPalette: ColorPalette }[] { + const purchasablePatterns: { + pattern: Pattern; + colorPalette: ColorPalette; + }[] = []; + + for (const pattern of Object.values(cosmetics?.patterns ?? {})) { + for (const colorPalette of pattern.colorPalettes ?? []) { + if ( + patternRelationship( + pattern, + colorPalette, + userMeResponse, + null, + ) === "purchasable" + ) { + const palette = cosmetics?.colorPalettes?.[colorPalette.name]; + if (palette) { + purchasablePatterns.push({ + pattern, + colorPalette: palette, + }); + } + } + } + } + + return purchasablePatterns; +} async loadPatternContent() { const me = await getUserMe(); const patterns = await fetchCosmetics(); - const purchasablePatterns: { - pattern: Pattern; - colorPalette: ColorPalette; - }[] = []; - - for (const pattern of Object.values(patterns?.patterns ?? {})) { - for (const colorPalette of pattern.colorPalettes ?? []) { - if ( - patternRelationship( - pattern, - colorPalette, - me !== false ? me : null, - null, - ) === "purchasable" - ) { - const palette = patterns?.colorPalettes?.[colorPalette.name]; - if (palette) { - purchasablePatterns.push({ - pattern, - colorPalette: palette, - }); - } - } - } - } + const purchasablePatterns = getPurchasablePatterns( + patterns, + me !== false ? me : null, + );src/client/components/PatternButton.ts (2)
37-47: Consider simplifying the translation fallback logic.The
translateCosmeticmethod has complex fallback logic. Consider extracting the name formatting to a separate utility.+private formatPatternName(name: string): string { + return name + .split("_") + .filter((word) => word.length > 0) + .map((word) => word[0].toUpperCase() + word.substring(1)) + .join(" "); +} private translateCosmetic(prefix: string, patternName: string): string { const translation = translateText(`${prefix}.${patternName}`); if (translation.startsWith(prefix)) { - return patternName - .split("_") - .filter((word) => word.length > 0) - .map((word) => word[0].toUpperCase() + word.substring(1)) - .join(" "); + return this.formatPatternName(patternName); } return translation; }
140-140: Remove debug console.log before merge.The console.log statement should be removed from production code.
- console.log("renderPatternPreview", pattern);src/client/TerritoryPatternsModal.ts (1)
61-88: Consider extracting pattern button generation logic.The nested loops and relationship checking could be extracted to improve readability.
+private generatePatternButtons(): TemplateResult[] { + const buttons: TemplateResult[] = []; + + for (const pattern of Object.values(this.cosmetics?.patterns ?? {})) { + const colorPalettes = [...(pattern.colorPalettes ?? []), null]; + for (const colorPalette of colorPalettes) { + const rel = patternRelationship( + pattern, + colorPalette, + this.userMeResponse, + this.affiliateCode, + ); + if (rel === "blocked") { + continue; + } + buttons.push(this.createPatternButton(pattern, colorPalette, rel)); + } + } + + return buttons; +} + +private createPatternButton( + pattern: Pattern, + colorPalette: { name: string; isArchived?: boolean } | null, + relationship: "owned" | "purchasable", +): TemplateResult { + return html` + <pattern-button + .pattern=${pattern} + .colorPalette=${this.cosmetics?.colorPalettes?.[ + colorPalette?.name ?? "" + ] ?? null} + .requiresPurchase=${relationship === "purchasable"} + .onSelect=${(p: PlayerPattern | null) => this.selectPattern(p)} + .onPurchase=${(p: Pattern, colorPalette: ColorPalette | null) => + handlePurchase(p, colorPalette)} + ></pattern-button> + `; +} private renderPatternGrid(): TemplateResult { - const buttons: TemplateResult[] = []; - for (const pattern of Object.values(this.cosmetics?.patterns ?? {})) { - // ... existing loop logic - } + const buttons = this.generatePatternButtons();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
resources/lang/en.json(1 hunks)src/client/ClientGameRunner.ts(2 hunks)src/client/Cosmetics.ts(3 hunks)src/client/Main.ts(2 hunks)src/client/SinglePlayerModal.ts(3 hunks)src/client/TerritoryPatternsModal.ts(5 hunks)src/client/Transport.ts(1 hunks)src/client/components/PatternButton.ts(7 hunks)src/client/graphics/AnimatedSpriteLoader.ts(1 hunks)src/client/graphics/SpriteLoader.ts(1 hunks)src/client/graphics/layers/RailroadLayer.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(2 hunks)src/client/graphics/layers/StructureLayer.ts(3 hunks)src/client/graphics/layers/TerritoryLayer.ts(1 hunks)src/client/graphics/layers/UILayer.ts(2 hunks)src/client/graphics/layers/UnitLayer.ts(6 hunks)src/client/graphics/layers/WinModal.ts(3 hunks)src/core/CosmeticSchemas.ts(4 hunks)src/core/PatternDecoder.ts(2 hunks)src/core/Schemas.ts(4 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/PastelTheme.ts(2 hunks)src/core/configuration/PastelThemeDark.ts(2 hunks)src/core/game/GameView.ts(4 hunks)src/core/game/UserSettings.ts(2 hunks)src/server/Client.ts(2 hunks)src/server/GameServer.ts(1 hunks)src/server/Privilege.ts(3 hunks)src/server/Worker.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (18)
- src/client/ClientGameRunner.ts
- src/client/graphics/SpriteLoader.ts
- src/client/Transport.ts
- src/client/graphics/layers/StructureIconsLayer.ts
- src/client/graphics/layers/UILayer.ts
- src/client/graphics/AnimatedSpriteLoader.ts
- src/server/GameServer.ts
- src/server/Client.ts
- src/core/configuration/PastelTheme.ts
- src/client/graphics/layers/RailroadLayer.ts
- src/core/game/UserSettings.ts
- src/client/graphics/layers/TerritoryLayer.ts
- src/core/configuration/Config.ts
- src/client/SinglePlayerModal.ts
- src/client/Main.ts
- src/core/PatternDecoder.ts
- src/client/graphics/layers/UnitLayer.ts
- src/client/Cosmetics.ts
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/client/graphics/layers/StructureLayer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/core/configuration/PastelThemeDark.tssrc/core/game/GameView.ts
📚 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-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-14T00:56:15.437Z
Learnt from: Aotumuri
PR: openfrontio/OpenFrontIO#786
File: src/core/Cosmetics.ts:1-1
Timestamp: 2025-06-14T00:56:15.437Z
Learning: The `import { base64url } from "jose";` syntax works correctly in ESM environments for the jose library, contrary to potential assumptions about needing specific ESM import paths like "jose/util/base64url".
Applied to files:
src/core/CosmeticSchemas.ts
📚 Learning: 2025-06-12T23:22:55.328Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for `roles`, `flares`, and `pattern` are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
Applied to files:
src/server/Privilege.ts
🧬 Code graph analysis (10)
src/client/graphics/layers/StructureLayer.ts (2)
src/core/game/GameView.ts (1)
borderColor(253-265)src/core/configuration/PastelTheme.ts (1)
borderColor(58-71)
src/client/graphics/layers/WinModal.ts (3)
src/client/Cosmetics.ts (3)
fetchCosmetics(55-72)patternRelationship(74-116)handlePurchase(11-53)src/core/CosmeticSchemas.ts (2)
Pattern(7-7)ColorPalette(10-10)src/client/components/PatternButton.ts (1)
handlePurchase(61-66)
src/client/components/PatternButton.ts (3)
src/core/CosmeticSchemas.ts (3)
ColorPalette(10-10)Pattern(7-7)DefaultPattern(92-96)src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/PatternDecoder.ts (1)
PatternDecoder(3-41)
src/core/configuration/PastelThemeDark.ts (2)
src/core/configuration/PastelTheme.ts (1)
PastelTheme(12-158)src/core/game/GameMap.ts (2)
GameMap(6-51)TileRef(3-3)
src/server/Worker.ts (2)
src/core/Schemas.ts (3)
PlayerCosmeticRefs(114-114)PlayerCosmetics(113-113)PlayerPattern(115-115)src/core/Util.ts (1)
assertNever(210-212)
src/core/Schemas.ts (1)
src/core/CosmeticSchemas.ts (3)
PatternNameSchema(19-22)PatternDataSchema(24-45)ColorPaletteSchema(47-51)
src/core/game/GameView.ts (5)
src/core/configuration/PastelTheme.ts (1)
territoryColor(43-55)src/core/CosmeticSchemas.ts (1)
ColorPalette(10-10)src/core/PatternDecoder.ts (2)
PatternDecoder(3-41)isPrimary(22-32)src/core/game/GameMap.ts (3)
TileRef(3-3)x(122-124)y(126-128)src/core/Schemas.ts (1)
PlayerCosmetics(113-113)
src/core/CosmeticSchemas.ts (2)
src/core/PatternDecoder.ts (1)
decodePatternData(43-72)src/core/Schemas.ts (1)
PlayerPattern(115-115)
src/client/TerritoryPatternsModal.ts (6)
src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/CosmeticSchemas.ts (3)
Cosmetics(6-6)Pattern(7-7)ColorPalette(10-10)src/core/game/UserSettings.ts (1)
UserSettings(6-157)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)src/client/Cosmetics.ts (3)
fetchCosmetics(55-72)patternRelationship(74-116)handlePurchase(11-53)src/client/components/PatternButton.ts (2)
handlePurchase(61-66)renderPatternPreview(135-150)
src/server/Privilege.ts (2)
src/core/Schemas.ts (1)
PlayerPattern(115-115)src/core/PatternDecoder.ts (1)
decodePatternData(43-72)
🪛 Biome (2.1.2)
src/core/configuration/PastelThemeDark.ts
[error] 20-20: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (32)
src/core/configuration/PastelThemeDark.ts (2)
6-6: LGTM! Clean inheritance approach.Good use of inheritance from
PastelThemeto create a dark variant. The approach is clean and follows TypeScript best practices.
7-10: Good color palette definition.The dark color values are well-chosen and properly defined using
colord.src/client/graphics/layers/StructureLayer.ts (3)
193-193: Good migration to owner-based color resolution.The change from
theme.territoryColor(unit.owner())tounit.owner().territoryColor()properly encapsulates color resolution within the owner model.
206-206: Consistent color resolution approach.Good consistency in moving border color resolution to the owner model.
247-247: Maintain consistent color resolution pattern.The change properly follows the same owner-based color resolution pattern used elsewhere in the file.
src/server/Worker.ts (3)
369-379: Good centralized cosmetics validation.The new
checkCosmeticsfunction properly centralizes validation logic and handles errors appropriately.
427-495: Well-structured cosmetics validation function.The
checkCosmeticsfunction is well-organized with clear error handling paths and proper use of theassertNeverpattern for exhaustive switch checking.
460-466: No action required — flares type is consistentPrivilegeChecker.isPatternAllowed expects
flares: readonly string[]and the code normalizesflaresearlier withflares ?? [], so the call is type-safe.src/core/game/GameView.ts (4)
1-1: Good fix for colord import.Properly importing both the factory function and type resolves the previous issue.
242-251: Clean pattern-aware color resolution.The
territoryColormethod properly handles pattern-based coloring with a clean fallback to the default color.
253-265: Well-structured defended border color logic.The
borderColormethod properly handles both regular and defended states with appropriate parity-based color selection.
449-449: Good handling of undefined cosmetics.Using the nullish coalescing operator with an empty object is a clean way to handle undefined cosmetics.
src/client/graphics/layers/WinModal.ts (2)
114-115: Good modernization to the cosmetics API.The switch from
fetchPatternstofetchCosmeticsaligns well with the new palette-aware cosmetics system.
163-167: UI now properly supports color palettes.Good addition of the colorPalette and requiresPurchase props to PatternButton, enabling color-aware pattern selection and purchase flows.
src/client/components/PatternButton.ts (5)
1-11: Good type imports for cosmetics system.The addition of Colord for color manipulation and the cosmetic type imports (ColorPalette, DefaultPattern, Pattern) properly support the new palette-aware pattern system.
49-59: Pattern selection properly handles the new PlayerPattern type.The handleClick method correctly constructs a PlayerPattern object with the optional colorPalette, aligning with the new cosmetics model.
189-192: Good default color constants for pattern rendering.Using white/black as defaults ensures patterns render predictably when no color palette is specified.
198-204: Efficient cache key construction for patterns.The cache key properly includes all relevant pattern attributes (name, colors, dimensions) to prevent incorrect cache hits.
248-253: Clean color extraction with proper fallbacks.The code correctly uses Colord to parse colors from the palette, falling back to defaults when undefined.
src/core/Schemas.ts (6)
4-8: Good modular import structure for cosmetic schemas.Importing the pattern and color schemas from CosmeticSchemas promotes better separation of concerns.
113-116: Well-structured cosmetic type exports.The new PlayerCosmetics, PlayerCosmeticRefs, PlayerPattern, and Flag types provide a clear hierarchy for the cosmetics system.
364-375: FlagSchema validation is comprehensive.The schema properly validates country codes and custom flags (starting with "!"), with a clear error message for invalid values.
377-381: PlayerCosmeticRefsSchema enables efficient reference-based cosmetics.This schema allows the client to send references (names) rather than full cosmetic data, which the server can then resolve.
383-391: Clean cosmetics data model with proper optionality.PlayerPatternSchema and PlayerCosmeticsSchema provide a well-structured model with appropriate optional fields for backward compatibility.
500-501: Good documentation about server-side cosmetic resolution.The comment clearly explains that the server replaces refs with actual cosmetic data, preventing confusion about the data flow.
src/core/CosmeticSchemas.ts (4)
24-45: PatternDataSchema validation is robust.Good use of zod refinement to validate base64url patterns through decodePatternData, with proper error logging.
47-51: Simple and clear ColorPaletteSchema structure.The schema properly captures the essential color palette data (name and two colors).
53-65: PatternSchema properly supports color palette references.The schema includes optional colorPalettes array with archive status, enabling flexible pattern-palette relationships.
92-96: DefaultPattern provides a good fallback.The default pattern constant ensures there's always a valid pattern to render, improving robustness.
src/client/TerritoryPatternsModal.ts (3)
43-55: onUserMe properly initializes cosmetics state.The method correctly fetches cosmetics, loads saved pattern selection, and handles null user responses.
132-146: Pattern selection correctly stores composite keys.The selectPattern method properly constructs pattern: or pattern:: keys for localStorage, supporting the new color palette system.
148-164: Good async handling for modal rendering.The refresh method properly waits for DOM updates before opening the modal and rendering the preview.
Description:
Add support for colored territory patterns/skins
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan