merge v25 into main

[evanpelle]

Description:

Merge v25 into main

Please complete the following:

• I have added screenshots for all UI updates
• I process any text displayed to the user through translateText() and I've added it to the en.json file
• I have added relevant tests to the test directory
• I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

evan

[coderabbitai]

Walkthrough

Refactors pattern handling to use patternName and new cosmetics schemas; updates client join/spawn payloads, privilege checks, and UI modals. Adds alliance request duration/status with revised execution and event cleanup. Overhauls trade/train spawn and rewards with new config APIs and sigmoid utility. Removes a map, tweaks tests, and updates lint rule.

Changes

Cohort / File(s)	Summary
Lint config eslint.config.js	Remove rule sort-imports.
Cosmetics schemas and pattern-by-name refactor src/core/CosmeticSchemas.ts, src/core/Schemas.ts, src/core/game/UserSettings.ts, src/client/Cosmetics.ts, src/client/Main.ts, src/client/ClientGameRunner.ts, src/client/Transport.ts, src/server/Privilege.ts, src/server/worker/websocket/handler/message/PreJoinHandler.ts, src/client/TerritoryPatternsModal.ts, src/client/SinglePlayerModal.ts, src/core/game/GameView.ts	Introduce PatternName/PatternInfo schemas and base64url validation; move PatternSchema; rename APIs to patternName; change cosmetics patterns to Map and export getCosmetics; switch join to use patternName; restructure privilege check to name-based PatternResult; adjust PreJoin flow; modal and UI updated to Pattern objects; minor rename _hunans→_humans.
Alliance request lifecycle src/core/configuration/Config.ts, src/core/configuration/DefaultConfig.ts, src/core/execution/alliance/AllianceRequestExecution.ts, src/core/game/AllianceRequestImpl.ts, src/core/game/Game.ts, src/client/graphics/layers/EventsDisplay.ts, src/core/game/PlayerImpl.ts, tests/AllianceRequestExecution.test.ts	Add allianceRequestDuration; track request status; create/expire requests via init/tick using duration; UI events add shouldDelete; narrow pending-request condition; add tests for reply, reciprocal create, and expiry.
Trade ships and trains src/core/configuration/DefaultConfig.ts, src/core/execution/PortExecution.ts, src/core/execution/TrainStationExecution.ts, src/core/game/TrainStation.ts, tests/core/game/TrainStation.test.ts	New tradeShipSpawnRate(numTradeShips,numPlayerPorts); PortExecution uses ports; trainSpawnRate now uses numPlayerFactories; TrainStation rewards use relation-based gold; test expectations updated.
Utility src/core/Util.ts	Add exported sigmoid(value, decayRate, midpoint).
Map playlist src/server/MapPlaylist.ts	Remove Yenisei from frequency pool.
UI lifecycle tweak src/client/FlagInput.ts	disconnectedCallback now re-adds flag-change listener after removal.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Client as Client (Main/Transport)
  participant Server as WS PreJoinHandler
  participant Priv as PrivilegeChecker
  participant Store as Cosmetics (by name)

  User->>Client: Click Join Lobby
  Client->>Client: Read userSettings.getSelectedPatternName()
  Client->>Server: ClientJoin{ patternName, ... }
  Server->>Priv: isPatternAllowed(name, flares)
  alt allowed
    Priv-->>Server: {type:"allowed", pattern}
    Server->>Store: Resolve pattern by name
    Store-->>Server: PatternInfo
    Server-->>Client: Join OK
  else forbidden
    Priv-->>Server: {type:"forbidden", reason}
    Server-->>Client: Close 1002 (reason)
  else unknown
    Priv-->>Server: {type:"unknown"}
    Server-->>Client: Close 1002 (backend lookup)
  end

Loading

sequenceDiagram
  autonumber
  participant P1 as Player A
  participant P2 as Player B
  participant Exec as AllianceRequestExecution
  participant Game as Game (config/time)
  participant UI as EventsDisplay

  P1->>Exec: Add execution (request A→B)
  Exec->>Game: init(mg)
  alt B already requested A
    Exec->>Game: accept(existing)
    Exec-->>P1: deactivate
  else can send
    Exec->>Game: create request(A→B)
  else cannot send
    Exec-->>P1: deactivate
  end

  loop each tick
    Exec->>Game: check status/time
    alt status accepted/rejected
      Exec-->>P1: deactivate
    else expired (ticks > allianceRequestDuration)
      Exec->>Game: reject(request)
      Exec-->>P1: deactivate
    end
  end

  UI->>UI: Filter events
  UI->>Game: event.shouldDelete?(game)
  note right of UI: Deletes when players become allied

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• store & reference pattern by name #1766 — Broad pattern→patternName refactor across client/server and cosmetics; overlaps with this PR’s schema and transport changes.
• Improve alliance UX, prevent hung alliance requests #1868 — Alliance request duration, status, UI cleanup, and execution flow; matches the alliance lifecycle updates here.
• v25 meta update #1872 — Adds sigmoid and updates config/spawn logic; aligns with DefaultConfig, PortExecution, and train changes.

Suggested labels

Feature - New, Feature - Simulation, Feature - Frontend, Feature - Backend, Feature - Test, Feature - Map

Suggested reviewers

• scottanderson
• drillskibo

Poem

Patterns by name, not by code we claim,
Trains roll on curves of sigmoid flame.
Alliances bloom, then fade on time,
Ships heed ports in spawning rhyme.
Yenisei rests; the maps rearrange—
Tick by tick, we script the change.

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

• Visit our Status Page to check the current availability of CodeRabbit.
• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

src/client/ClientGameRunner.ts (1)

50-62: Migrate all pattern fields in join payloads and handlers to patternName

Several places still use pattern: in the wire format or handler logic, which will be ignored by the server now that we’re only looking at patternName. Please update these to ensure the client’s selection makes it all the way through:

• src/core/Schemas.ts (ClientJoinMessageSchema around line 488)

  export const ClientJoinMessageSchema = z.object({
    clientID: ID,
    flag: FlagSchema,
-     pattern: PatternSchema.optional(),
    patternName: z.string().optional(),
    token: TokenSchema,
    type: z.literal("join"),
  });

• src/server/GameServer.ts (around line 302)
Replace the outgoing join-complete payload’s pattern: c.pattern with patternName: c.patternName.

• src/client/SinglePlayerModal.ts (around line 450)
When dispatching the “join” event for single-player, send patternName instead of pattern:

    payload: {
      flag: flagInput.getCurrentFlag(),
-       pattern: pattern?.pattern,
+       patternName,
    },

• src/server/Client.ts
Rename the public readonly pattern: string | undefined property to patternName (and update all its usages).

• src/core/game/GameView.ts (around line 396)
Change { flag: h.flag, pattern: h.pattern } to { flag: h.flag, patternName: h.patternName } so the view schema aligns.

After these changes, verify that:

1. All pattern: occurrences in join messages/schema are removed.
2. All ClientJoinMessage handlers read from clientMsg.patternName only.
3. No client-to-server or server-to-client payloads still reference the old pattern field.

This ensures the chosen pattern flows end-to-end.

src/client/Transport.ts (1)

373-384: Ensure PreJoinHandler uses patternName instead of pattern

We still see one remaining reference to clientMsg.pattern in the server’s join flow. Update this to consume the new patternName field so that the server honors the client’s selected pattern rather than falling back to the old property.

• Location to fix:

• src/server/worker/websocket/handler/message/PreJoinHandler.ts around line 255
  • Change this snippet:

// before
clientMsg.flag,
clientMsg.pattern,
);

// after
clientMsg.flag,
clientMsg.patternName,
);

• Confirm downstream logic uses the correct pattern value (pulled from patternName). Update any other references or model types as needed to avoid having two different sources for the same data.

src/core/Schemas.ts (1)

483-494: Ensure patternName is strongly typed and mutually exclusive with pattern

The PatternNameSchema is already exported from src/core/CosmeticSchemas.ts (line 18), so we can strengthen typing and add a guard to prevent sending both fields at once.

Please update src/core/Schemas.ts as follows:

• Import PatternNameSchema alongside PatternSchema.
• Replace the plain-string patternName with PatternNameSchema.optional().
• Add a .refine() call to enforce that only one of pattern or patternName may be present.

--- a/src/core/Schemas.ts
+++ b/src/core/Schemas.ts
@@ -1,5 +1,5 @@
-import { PatternSchema } from "./CosmeticSchemas";
+import { PatternSchema, PatternNameSchema } from "./CosmeticSchemas";
 
 export const ClientJoinMessageSchema = z.object({
   clientID: ID,
@@ -490,8 +490,8 @@ export const ClientJoinMessageSchema = z.object({
   lastTurn: z.number(), // The last turn the client saw.
-  pattern: PatternSchema.optional(),
-  patternName: z.string().optional(),
+  pattern: PatternSchema.optional(),
+  patternName: PatternNameSchema.optional(),
   token: TokenSchema, // WARNING: PII
   type: z.literal("join"),
   username: UsernameSchema,
@@ -494,4 +494,9 @@ export const ClientJoinMessageSchema = z.object({
 });
+  
+// Ensure only one of `pattern` or `patternName` is provided.
+export const ClientJoinMessageSchema = ClientJoinMessageSchema.refine(
+  (v) => !(v.pattern && v.patternName),
+  { message: "Provide either pattern or patternName, not both" }
+);

src/core/CosmeticSchemas.ts (1)

52-76: Enforce key/name consistency to avoid drift.

Right now, the key (record key) and PatternInfoSchema.name can diverge. Add a top-level superRefine to validate equality.

-export const CosmeticsSchema = z.object({
+export const CosmeticsSchema = z.object({
   /* eslint-disable sort-keys */
-  patterns: z.record(z.string(), PatternInfoSchema),
+  patterns: z.record(PatternNameSchema, PatternInfoSchema),
   flag: z
     .object({
       layers: z.record(
         z.string(),
         z.object({
           name: z.string(),
           flares: z.string().array().optional(),
         }),
       ),
       color: z.record(
         z.string(),
         z.object({
           color: z.string(),
           name: z.string(),
           flares: z.string().array().optional(),
         }),
       ),
     })
     .optional(),
   /* eslint-enable sort-keys */
-});
+}).superRefine((data, ctx) => {
+  for (const [key, info] of Object.entries(data.patterns)) {
+    if (info.name !== key) {
+      ctx.addIssue({
+        code: "custom",
+        message: `patterns["${key}"].name must equal "${key}"`,
+        path: ["patterns", key, "name"],
+      });
+    }
+  }
+});

src/client/TerritoryPatternsModal.ts (1)

101-107: Tooltip shows for unlocked patterns (null vs undefined bug).

product is Product | null, never undefined. This condition is always true for null, so the tooltip appears when it shouldn’t.

-  if (this.hoveredPattern && this.hoveredPattern.product !== undefined) {
+  if (this.hoveredPattern && this.hoveredPattern.product !== null) {

src/core/game/TrainStation.ts (1)

28-34: Compare players by id(), not by object identity.

Object identity can differ across references to the same player. Use stable PlayerID for equality in both revenue sharing and rel().

Apply this diff:

-    const goldBonus = mg.config().trainGold(rel(trainOwner, stationOwner));
-    // Share revenue with the station owner if it's not the current player
-    if (trainOwner !== stationOwner) {
+    const goldBonus = mg.config().trainGold(rel(trainOwner, stationOwner));
+    // Share revenue with the station owner if it's not the current player
+    if (trainOwner.id() !== stationOwner.id()) {
       stationOwner.addGold(goldBonus, station.tile());
     }
     trainOwner.addGold(goldBonus, station.tile());

-    const goldBonus = mg.config().trainGold(rel(trainOwner, stationOwner));
+    const goldBonus = mg.config().trainGold(rel(trainOwner, stationOwner));
 
-    trainOwner.addGold(goldBonus, station.tile());
-    // Share revenue with the station owner if it's not the current player
-    if (trainOwner !== stationOwner) {
+    trainOwner.addGold(goldBonus, station.tile());
+    // Share revenue with the station owner if it's not the current player
+    if (trainOwner.id() !== stationOwner.id()) {
       stationOwner.addGold(goldBonus, station.tile());
     }

-function rel(player: Player, other: Player): "self" | "friendly" | "other" {
-  if (player === other) {
+function rel(player: Player, other: Player): "self" | "friendly" | "other" {
+  if (player.id() === other.id()) {
     return "self";
   }
   if (player.isFriendly(other)) {
     return "friendly";
   }
   return "other";
 }

Also applies to: 46-53, 236-244

🧹 Nitpick comments (38)

src/client/ClientGameRunner.ts (1)

50-56: Rename to patternName aligns with the wider schema changes; prefer an optional field over string | undefined.

Using an optional property reads cleaner in TypeScript and better reflects intent.

Apply this diff:

 export type LobbyConfig = {
   serverConfig: ServerConfig;
-  patternName: string | undefined;
+  patternName?: string;
   flag: string;
   playerName: string;

src/client/FlagInput.ts (1)

46-52: Harden event parsing to avoid crashes if detail is missing.

If someone dispatches flag-change without detail.flag, safeParse(e.detail.flag) will throw due to e.detail being undefined.

Apply this diff:

-  private readonly updateFlag = (ev: Event) => {
-    const e = ev as CustomEvent<{ flag: string }>;
-    if (!FlagSchema.safeParse(e.detail.flag).success) return;
-    if (this.flag !== e.detail.flag) {
-      this.flag = e.detail.flag;
-    }
-  };
+  private readonly updateFlag = (ev: Event) => {
+    const e = ev as Partial<CustomEvent<{ flag: string }>>;
+    const candidate = e?.detail?.flag;
+    if (!FlagSchema.safeParse(candidate).success) return;
+    if (this.flag !== candidate) {
+      this.flag = candidate;
+    }
+  };

src/core/game/UserSettings.ts (2)

114-116: Consider migrating old stored values to the new name-based format.

Older clients may have base64 pattern data under territoryPattern. Returning it as a “name” will fail lookups. Clear or migrate obviously non-name values.

Apply this diff:

   getSelectedPatternName(): string | undefined {
-    return localStorage.getItem(PATTERN_KEY) ?? undefined;
+    const raw = localStorage.getItem(PATTERN_KEY) ?? undefined;
+    // Naive migration: if it looks like legacy base64/blob (too long or contains '=' or '/'),
+    // drop it so the UI falls back to "no selection".
+    if (raw && (raw.length > 64 || /[=\/]/.test(raw))) {
+      localStorage.removeItem(PATTERN_KEY);
+      return undefined;
+    }
+    return raw;
   }

---

118-124: Rename the parameter to reflect the new semantics (it’s a name, not base64).

Also avoid leaking legacy wording into code.

Apply this diff:

-  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/Cosmetics.ts (2)

21-31: Avoid mutating the cosmetics catalog when the player has access.

patternData.product = null changes shared data. Prefer a shallow copy.

Apply this diff:

-    const patternData = cosmetics.patterns[name];
+    const patternData = cosmetics.patterns[name];
     const hasAccess = playerFlares.has(`pattern:${name}`);
     if (hasAccess) {
       // Remove product info because player already has access.
-      patternData.product = null;
-      patterns.set(name, patternData);
+      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);
+      patterns.set(name, { ...patternData });
     }

---

80-96: Exporting getCosmetics() is fine; consider caching to cut network chatter.

Low-priority: a simple in-memory cache would avoid repeated fetch+parse when multiple views open.

Example (outside this hunk):

let cosmeticsCache: Cosmetics | undefined;
export async function getCosmetics(): Promise<Cosmetics | undefined> {
  if (cosmeticsCache) return cosmeticsCache;
  try {
    // ...fetch/parse...
    cosmeticsCache = result.data;
    return cosmeticsCache;
  } catch (e) {
    return undefined;
  }
}

src/client/Main.ts (1)

432-439: Localize user-facing alerts for purchase flow.

These strings should go through translateText() per PR checklist.

Apply this diff:

-        alertAndStrip("purchase succeeded");
+        alertAndStrip(translateText("purchase.succeeded"));
         return;
       } else if (params.get("purchase-completed") === "false") {
-        alertAndStrip("purchase failed");
+        alertAndStrip(translateText("purchase.failed"));
         return;
       }

src/client/SinglePlayerModal.ts (2)

5-16: Imports look good; keep the API surface small and typed.

• Nice: using translateText for UI strings and typed imports (TeamCountConfig).
• Consider importing a PatternName type (if exported) for local variables where you handle patternName to make intent explicit and help refactors later.

---

407-477: Async pattern resolution is correct; add resilience and prefetch to cut latency.

• If cosmetics fetch fails, pattern is undefined (good), but there’s no user feedback. Optional: log and proceed is fine for Singleplayer.
• To reduce click-to-start latency, prefetch cosmetics on modal open and reuse them.

Example patch:

   public open() {
     this.modalEl?.open();
     this.useRandomMap = false;
+    // Warm-up cosmetics so startGame avoids a network hop.
+    void getCosmetics();
   }

Optionally, cache the result in a field to avoid multiple fetches.

src/server/Privilege.ts (1)

9-18: Rename the parameter to reflect new semantics.

The API now takes a pattern name, not base64. Rename for clarity to avoid misuse at call sites.

 export type PrivilegeChecker = {
   isPatternAllowed(
-    base64: string,
+    name: string,
     flares: readonly string[] | undefined,
   ): PatternResult;

src/core/CosmeticSchemas.ts (4)

18-21: PatternName should forbid empty strings.

Add a minimal length to prevent "" from passing validation.

 export const PatternNameSchema = z
   .string()
   .regex(/^[a-z0-9_]+$/)
-  .max(32);
+  .min(1)
+  .max(32);

---

23-44: Avoid noisy logging inside schema validation and remove the “magic number”.

• Validation should not console.error on expected bad input; it’ll spam logs client- and server-side.
• The hardcoded 1403 is a magic number; make it a named constant with a short rationale.

Apply this diff to quiet validation and reference a named constant:

 export const PatternSchema = z
   .string()
-  .max(1403)
-  .base64url()
+  .max(MAX_PATTERN_B64URL_LEN, { message: "Pattern too large" })
+  // If your Zod version supports it, `.base64url()` can be kept in addition to the refine.
   .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",
     },
   );

Add this constant near the top of the file (outside the shown range):

// Upper bound to cap decode/preview work. 3 header bytes + up to 128x128 bits (scaled) ≈ 1403 chars base64url.
// Keep aligned with PatternDecoder expectations and UI preview limits.
export const MAX_PATTERN_B64URL_LEN = 1403 as const;

---

55-55: Use PatternNameSchema for record keys (stronger typing).

Keys are pattern names; reflect that in the schema.

-  patterns: z.record(z.string(), PatternInfoSchema),
+  patterns: z.record(PatternNameSchema, PatternInfoSchema),

---

1-3: Use the standard Zod import

I’ve confirmed that your project is already on Zod v4 (package.json shows ^4.0.5), so there’s no build-break risk from using v3. Also, I didn’t find any calls to a chained .base64url() on Zod schemas—your code imports base64url from jose instead.

To keep imports consistent across the repo, please update:

• In src/core/CosmeticSchemas.ts, line 2

- import { z } from "zod/v4";
+ import { z } from "zod";

No other changes are needed here.

src/client/TerritoryPatternsModal.ts (4)

14-25: Single source of truth for button width.

You introduced BUTTON_WIDTH (nice), but buttonWidth remains a redundant instance field. Either remove it or have it reference the constant to avoid divergence.

-  public previewButton: HTMLElement | null = null;
-  public buttonWidth = 150;
+  public previewButton: HTMLElement | null = null;
+  public buttonWidth = BUTTON_WIDTH; // keep for backwards compatibility, but delegate to constant

---

176-179: Iterate Map values directly for clarity.

Small readability win; avoids the unused _.

-    for (const [_, pattern] of this.patterns) {
+    for (const pattern of this.patterns.values()) {
       const { name } = pattern;
       if (!this.showChocoPattern && name === "choco") continue;

---

355-369: Fallback to a safe default preview on decode errors.

Returning an empty string yields a broken image icon. Decode the DEFAULT pattern instead so the UI stays clean.

-  let decoder: PatternDecoder;
-  try {
-    decoder = new PatternDecoder(pattern, base64url.decode);
-  } catch (e) {
-    console.error("Error decoding pattern", e);
-    return "";
-  }
+  let decoder: PatternDecoder;
+  try {
+    decoder = new PatternDecoder(pattern, base64url.decode);
+  } catch (e) {
+    console.warn("Error decoding pattern; falling back to default", e);
+    try {
+      decoder = new PatternDecoder(DEFAULT_PATTERN_B64, base64url.decode);
+      pattern = DEFAULT_PATTERN_B64; // keep cache key consistent with actual preview
+    } catch {
+      return "";
+    }
+  }

---

345-407: Bound the preview cache to prevent unbounded growth.

patternCache is global and unbounded. If patterns or sizes vary a lot, memory can grow without limit. Add a simple cap (e.g., 100 entries) and evict oldest.

Example helper outside the function:

function setCacheEntry(cache: Map<string, string>, key: string, value: string, max = 100) {
  if (cache.size >= max) {
    const firstKey = cache.keys().next().value;
    cache.delete(firstKey);
  }
  cache.set(key, value);
}

Then replace the setter:

-  patternCache.set(patternLookupKey, dataUrl);
+  setCacheEntry(patternCache, patternLookupKey, dataUrl, 128);

src/core/game/GameView.ts (1)

388-399: No remaining _hunans usages; rename is complete

I ran a full search for the old _hunans identifier and found zero hits—your rename to _humans is clean and final.

Regarding the null-guard on clientID:

• The Zod PlayerSchema enforces clientID as a non-null ID (a safe string) when you load gameStart.players .
• In GameView you only map over those human players, so h.clientID cannot be null or undefined at runtime.

Optional hardening: if you ever extend PlayerInfo to allow bots or missing IDs, you could apply your .filter(...) + .map(...) diff in src/core/game/GameView.ts (around lines 392–397). Otherwise, no changes are strictly needed here.

src/core/Util.ts (1)

292-298: Sigmoid is correct; document inputs and handle edge cases (decayRate = 0, non-finite)

Implementation is the standard logistic. Add minimal guards + JSDoc for clarity and robustness.

Apply this diff:

+/**
+ * Logistic/sigmoid helper.
+ * value: x-axis input.
+ * decayRate: steepness (> 0 recommended). If 0, returns 0.5.
+ * midpoint: x where output is 0.5.
+ * Returns a number in [0, 1].
+ */
 export function sigmoid(
   value: number,
   decayRate: number,
   midpoint: number,
 ): number {
-  return 1 / (1 + Math.exp(-decayRate * (value - midpoint)));
+  if (!Number.isFinite(value) || !Number.isFinite(decayRate) || !Number.isFinite(midpoint)) {
+    return 0.5;
+  }
+  if (decayRate === 0) return 0.5;
+  const z = -decayRate * (value - midpoint);
+  const out = 1 / (1 + Math.exp(z));
+  // Clamp to [0,1] to avoid tiny FP drift.
+  return Math.min(1, Math.max(0, out));
 }

src/core/game/Game.ts (2)

1-1: Avoid disabling max-lines; split types and enums into focused modules

Large files are harder to review and maintain. Prefer composition and smaller modules (e.g., move enums and type aliases into dedicated files).

Happy to suggest a cut plan if you want it.

---

352-359: Use a named union for AllianceRequest status
To make the status type clear and easy to reuse, replace the inline string-literal union with a shared alias. I found two spots still using the literal union and no existing alias in the codebase:

• src/core/game/Game.ts (line 358)
• src/core/game/AllianceRequestImpl.ts (line 15)

Please:

1. Add this alias near your other game/diplomacy types (for example, above the AllianceRequest interface in Game.ts):

   export type AllianceRequestStatus = "pending" | "accepted" | "rejected";

2. Update the interface in Game.ts:

   export type AllianceRequest = {
   ── status(): "pending" | "accepted" | "rejected";
   ++ status(): AllianceRequestStatus;
   };

3. Update the implementation in AllianceRequestImpl.ts:

   export class AllianceRequestImpl implements AllianceRequest {
   ── status(): "pending" | "accepted" | "rejected" {
   ++ status(): AllianceRequestStatus {
       return this._status;
   }
   }

This change is optional but will improve type clarity and make future refactors or tests easier.

src/client/graphics/layers/EventsDisplay.ts (2)

196-200: Minor: evaluate shouldDelete first to drop stale events early

Functionally correct. For efficiency and intent, check shouldDelete before duration. It avoids needless tick math and reads clearer.

Apply this diff:

-    let remainingEvents = this.events.filter((event) => {
-      const shouldKeep =
-        this.game.ticks() - event.createdAt < (event.duration ?? 600) &&
-        !event.shouldDelete?.(this.game);
+    let remainingEvents = this.events.filter((event) => {
+      const shouldKeep =
+        !(event.shouldDelete?.(this.game) ?? false) &&
+        this.game.ticks() - event.createdAt < (event.duration ?? 600);

---

461-466: Clamp alliance request duration; avoid magic tick constant

Good use of config(). To be safe against misconfig, clamp to non-negative. Also, the -20 assumes “10 ticks = 1s”. Prefer a helper if available; otherwise clamp now.

Apply this diff:

-      duration: this.game.config().allianceRequestDuration() - 20, // 2 second buffer
+      duration: Math.max(0, this.game.config().allianceRequestDuration() - 20), // 2s buffer; clamp

If you want, I can add a small util like secondsToTicks(s: number): Tick to remove the magic 20/10s from UI code.

src/core/configuration/Config.ts (1)

124-139: Add explicit JSDoc for timing and spawn‐rate methods

Please enrich the new configuration signatures with clear JSDoc so callers know exactly what units and semantics they’re working with:

• allianceRequestDuration(): Tick
– How many game ticks an alliance request stays actionable before expiring.

• tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number
– Chance of spawning a trade ship per tick (e.g. expected spawns/tick, normalized per port, bounded [0,1]).

• trainGold(rel: "self" | "friendly" | "other"): Gold
– Amount of gold awarded per train spawn or per delivery (specify which).

• trainSpawnRate(numPlayerFactories: number): number
– Expected number of train spawns per tick (describe cap or decay behavior).

Adding these comments in src/core/configuration/Config.ts above each signature will prevent magic constants and misuse in callers.

tests/core/game/TrainStation.test.ts (1)

64-67: Future-proof the config mock to match the new relation-based API.

The production API is now trainGold(rel: "self" | "friendly" | "other"), but the mock still accepts a boolean. This passes today because strings are truthy, but it can hide bugs and weakens type-safety.

Suggested mock for clarity:

config: jest.fn().mockReturnValue({
  trainGold: (rel: "self" | "friendly" | "other") =>
    rel === "friendly" || rel === "self" ? 1000n : 500n,
}),

If desired, I can add a small test that verifies self/friendly/other all call the mock with the right rel.

src/core/game/AllianceRequestImpl.ts (1)

6-7: Good move: use a typed-union status on the request.

This is clean, composable, and easy to reason about. Consider whether you also need to represent “expired” in the lifecycle, or ensure expired requests are removed so clients never see stale "pending". If clients need to render status, consider adding it to the update payloads (only if the wire type supports it).

Would you like me to scan usages to confirm whether expiry is handled purely by removal vs. state?

Also applies to: 15-17

src/core/execution/PortExecution.ts (1)

81-85: Confirm whether tradeShipSpawnRate expects count-of-ports or port-level capacity.

unitCount(UnitType.Port) sums levels, not the number of port units. If the config function expects a simple count, use this.player.units(UnitType.Port).length. If it expects “capacity”, keeping sum-of-levels is fine but please rename the variable accordingly to avoid confusion.

Two options:

• If config expects number of ports (recommended for clarity):

-    const numPlayerPorts = this.player.unitCount(UnitType.Port);
+    const numPlayerPorts = this.player.units(UnitType.Port).length;

• If config expects a capacity-like value (sum of port levels), just rename:

-    const numPlayerPorts = this.player.unitCount(UnitType.Port);
+    const playerPortCapacity = this.player.unitCount(UnitType.Port);
-    const spawnRate = this.mg
-      .config()
-      .tradeShipSpawnRate(numTradeShips, numPlayerPorts);
+    const spawnRate = this.mg
+      .config()
+      .tradeShipSpawnRate(numTradeShips, playerPortCapacity);

I can grep DefaultConfig to verify the intent and update this spot plus its docstring.

src/core/execution/TrainStationExecution.ts (2)

51-55: Double-check whether trainSpawnRate expects factory count vs. capacity (sum of levels).

unitCount(UnitType.Factory) returns the sum of factory levels. If the config treats the parameter as “number of factories,” prefer a plain count:

-    const spawnRate = this.mg
-      .config()
-      .trainSpawnRate(this.unit.owner().unitCount(UnitType.Factory));
+    const numFactories = this.unit.owner().units(UnitType.Factory).length;
+    const spawnRate = this.mg.config().trainSpawnRate(numFactories);

If capacity is intended, consider documenting it or renaming a local variable for clarity in both Train and Port logic to keep semantics consistent.

---

78-78: Spawn decision now ignores cluster size — confirm balance impact.

Previously, cluster size influenced spawn probability. With the new config-driven approach, you still gate on availableForTrade.size > 0, which is good. If pacing feels off during playtests, consider feeding a small cluster factor into the config function rather than re-introducing complex logic here (composition over inheritance, keep config as the policy).

Happy to prototype a trainSpawnRate(capacity: number, clusterSize: number) signature if needed.

src/core/game/TrainStation.ts (3)

38-38: Remove unused PseudoRandom from PortStopHandler and handler factory.

random is not used anymore; keep API simple and avoid dead params/imports.

Apply this diff:

-import { PseudoRandom } from "../PseudoRandom";
+// PseudoRandom no longer needed here

-class PortStopHandler implements TrainStopHandler {
-  constructor(private readonly random: PseudoRandom) {}
+class PortStopHandler implements TrainStopHandler {
   onStop(

-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(),
   };
 }

-  constructor(
-    private readonly mg: Game,
-    public unit: Unit,
-  ) {
-    this.stopHandlers = createTrainStopHandlers(new PseudoRandom(mg.ticks()));
-  }
+  constructor(private readonly mg: Game, public unit: Unit) {
+    this.stopHandlers = createTrainStopHandlers();
+  }

Also applies to: 64-72, 80-86

---

61-61: FactoryStopHandler is a no-op; ensure this is deliberate.

If factories are supposed to have stop effects later, leave a TODO with context to avoid regressions.

---

28-34: Deduplicate stop award logic.

City and Port handlers do the same thing; prefer a tiny helper to keep logic in one place and reduce bugs.

Example:

+function awardStopGold(
+  mg: Game,
+  station: TrainStation,
+  trainOwner: Player,
+  stationOwner: Player,
+): void {
+  const amount = mg.config().trainGold(rel(trainOwner, stationOwner));
+  trainOwner.addGold(amount, station.tile());
+  if (trainOwner.id() !== stationOwner.id()) {
+    stationOwner.addGold(amount, station.tile());
+  }
+}

Also applies to: 46-53

src/core/execution/alliance/AllianceRequestExecution.ts (1)

48-62: Optional: use ticks param for testability, or document choice.

You ignore the ticks param and read mg.ticks(). That's okay, but pick one source and document, so executions behave uniformly.

tests/AllianceRequestExecution.test.ts (1)

62-76: Good expiry test; add two edge cases for robustness.

Consider adding:

• Missing recipient deactivates immediately (should not linger).
• Cannot send (cooldown or already allied) keeps requests at 0.

I can draft tests if helpful. Example skeletons:

test("Missing recipient deactivates immediately", () => {
  // use an invalid id
  game.addExecution(new AllianceRequestExecution(player1, "does-not-exist"));
  game.executeNextTick();
  expect(player1.outgoingAllianceRequests()).toHaveLength(0);
});

test("Cannot send when already allied or on cooldown", () => {
  // Pre-create alliance or simulate cooldown; then:
  game.addExecution(new AllianceRequestExecution(player1, player2.id()));
  game.executeNextTick();
  expect(player1.outgoingAllianceRequests()).toHaveLength(0);
});

src/core/configuration/DefaultConfig.ts (3)

340-344: trainSpawnRate comment mismatches the math.

trainSpawnRate returns a “rate” used as 1/p; the comment about expected number is misleading. Clarify units.

-    // hyperbolic decay, midpoint at 10 factories
-    // expected number of trains = numPlayerFactories  / trainSpawnRate(numPlayerFactories)
+    // Hyperbolic-like decay, midpoint ~10 factories.
+    // Probability of spawning a train each tick is 1 / trainSpawnRate(numPlayerFactories).
     return (numPlayerFactories + 10) * 20;

---

671-706: Attack debuffs: extract magic numbers and document rationale.

Values like 100_000 tiles, exponents 0.7/0.6 are important tuning knobs. Consider top-level named constants to keep balance intent explicit.

---

1-1: Style: prefer composition over small handler classes where possible.

DefaultConfig remains monolithic; for stop handlers elsewhere, consider a simple Record<UnitType, (ctx) => void> instead of tiny classes to reduce boilerplate and improve readability.

Also applies to: 214-222

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between c55c656 and 12a1fc5.

⛔ Files ignored due to path filters (3)

• resources/sprites/trainCarriage.png is excluded by !**/*.png
• resources/sprites/trainCarriageLoaded.png is excluded by !**/*.png
• resources/sprites/trainEngine.png is 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 (10)

📚 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/Main.ts
• src/client/SinglePlayerModal.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-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 (13)

src/core/game/GameView.ts (2)

src/core/game/Game.ts (1)

• Player (500-634)

src/core/Schemas.ts (1)

• Player (109-109)

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/Game.ts (1)

• Player (500-634)

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 (1)

• Pattern (6-6)

src/client/graphics/layers/EventsDisplay.ts (3)

src/core/game/GameView.ts (1)

• GameView (364-696)

src/core/game/AllianceRequestImpl.ts (2)

• requestor (19-21)
• recipient (23-25)

src/core/game/AllianceImpl.ts (2)

• requestor (26-28)
• recipient (30-32)

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/core/CosmeticSchemas.ts (1)

src/core/PatternDecoder.ts (1)

• PatternDecoder (1-56)

src/core/game/TrainStation.ts (1)

src/core/game/Game.ts (1)

• Player (500-634)

src/client/SinglePlayerModal.ts (1)

src/client/Cosmetics.ts (1)

• getCosmetics (80-96)

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/server/Privilege.ts (1)

src/core/PatternDecoder.ts (1)

• PatternDecoder (1-56)

🔇 Additional comments (27)

src/core/game/UserSettings.ts (1)

114-124: No old API usages found

I ran the Ripgrep search for both getSelectedPattern( and setSelectedPattern( and found no matches. All legacy calls have been removed—no further action needed.

src/client/Cosmetics.ts (1)

9-16: All call sites now use Map methods; safe to approve

I checked the two consumer files mentioned and found no array-style calls (.find, .map, .filter) on the result of patterns(). Both TerritoryPatternsModal and any SinglePlayerModal usages either aren’t present or correctly handle the Map<string, Pattern> return (e.g., via .get, .has, or iteration).

Great work—this change stands as is!

src/client/Main.ts (2)

221-224: Switch to territoryModal.refresh() looks good; it simplifies layout sync.

Assuming refresh() encapsulates preview sizing + data reload, this is a clean improvement over the previous two-step flow.

---

461-473: patternName is already optional—no change needed

The call this.userSettings.getSelectedPatternName() returns string | undefined (see UserSettings.getSelectedPatternName(): localStorage.getItem(...) ?? undefined) and the transport schema declares patternName: z.string().optional() in core/Schemas.ts – it already accepts an undefined value. The server handler in PreJoinHandler.ts checks clientMsg.patternName !== undefined before using it. At runtime JSON.stringify will omit keys with undefined, so adding ?? undefined has no effect.

No diff required here—please leave the code as is.

src/server/worker/websocket/handler/message/PreJoinHandler.ts (2)

15-15: Solid: assertNever import for exhaustiveness.

Good defensive addition to force handling of all PatternResult variants in the switch.

---

245-256: Apply resolved pattern in Client constructor

The search shows that clientMsg.pattern only appears here, so updating this spot covers all uses.

• File: src/server/worker/websocket/handler/message/PreJoinHandler.ts
  Lines 253–256: pass the locally computed pattern instead of clientMsg.pattern

     const client = new Client(
       clientMsg.clientID,
       persistentId,
       claims,
       roles,
       flares,
       ip,
       clientMsg.username,
       ws,
       clientMsg.flag,
-      clientMsg.pattern,
+      pattern,
     );

src/core/Schemas.ts (2)

1-1: Good: centralizing PatternSchema in CosmeticSchemas.

This reduces duplicate validation logic and keeps pattern rules in one place.

---

383-387: Player.pattern optional is correct for fail-open and singleplayer paths.

This aligns with letting players join without a resolved pattern and avoids over-validating.

src/server/Privilege.ts (3)

27-34: Minor: early “not found” return is fine; consider distinguishing unlisted vs. unknown at source.

Current “pattern not found” maps to forbidden here; PreJoinHandler maps it to “Pattern unlisted” in its return. That’s consistent. No code change needed.

---

126-132: Fail-open checker returns unknown, which pairs well with the PreJoinHandler fail-open change.

After adopting the PreJoinHandler change to allow join without pattern on “unknown”, this path behaves as intended during backend outages.

---

41-49: Incorrect flare check suggestion

The cosmetics patterns in resources/cosmetics/cosmetics.json all include a "name" field (e.g. "ABMIVVU": { "name": "stripes_v" }), so found.name is defined and matches the requested name. Swapping found.name for the local name variable is a no-op in practice and isn’t needed.

No changes required here—please ignore this comment.

Likely an incorrect or invalid review comment.

src/core/CosmeticSchemas.ts (1)

46-50: Schema looks good. Ensure product is strictly nullable, not undefined.

This matches downstream usage that checks product !== null. Good alignment.

src/client/TerritoryPatternsModal.ts (1)

116-128: Purchase/button gating looks correct.

Good: selection is blocked via both styling and a click guard when product !== null. Matches Product being nullable in the schema.

Also applies to: 155-167

src/client/graphics/layers/EventsDisplay.ts (1)

68-69: New shouldDelete hook is a clean, composable addition

This keeps event lifecycle logic local to the event and avoids side-effects. Good move.

tests/core/game/TrainStation.test.ts (1)

64-67: Allied gold expectations updated correctly to 1000n.

The assertions now reflect the new config. Looks good.

src/core/game/PlayerImpl.ts (1)

400-403: No change needed: outgoingAllianceRequests only returns pending requests

The game’s request list (mg.allianceRequests) is pruned on both accept and reject, so non-pending requests are removed immediately. That means:

• In GameImpl.acceptAllianceRequest(request) and rejectAllianceRequest(request), the request is filtered out of allianceRequests
• outgoingAllianceRequests() simply returns the remaining items, which are all still “pending”

Because of this, the extra ar.status() === "pending" check is redundant.

src/core/execution/TrainStationExecution.ts (1)

1-2: Imports look correct and minimal.

Brings in UnitType and TrainStation needed by the new spawn logic. No issues.

src/core/game/TrainStation.ts (2)

28-34: Confirm intent: both trainOwner and stationOwner get full trainGold().

With current config, a friendly stop injects 2 × trainGold("friendly") per stop. If trainGold is meant as total-per-stop, this doubles economy. If it is per-recipient, all good. Please confirm balance and update name/docs if needed (e.g., trainGoldPerRecipient).

Also applies to: 46-53

---

216-220: Trading eligibility now excludes Factory stations.

Filtering to City/Port only changes reachable trade graph. If UI/AI rely on factory-as-trade-node, this is a breaking change. Validate downstream behavior.

tests/AllianceRequestExecution.test.ts (1)

37-49: Nice coverage for reply and reciprocal flows.

The “reply” and “send back” paths both look solid and readable.

src/core/configuration/DefaultConfig.ts (7)

345-354: Relation-based trainGold looks good.

Clear and typed union; values are BigInt and consistent.

---

366-372: tradeShipGold: confirm behavior at 0 ports.

With numPorts=0, bonus drops to 0.5× baseGold. If trade ships can only exist with ≥1 port, fine; otherwise this halves earnings unexpectedly. Verify call sites.

---

408-433: costWrapper multi-type counting is sensible; watch cost growth across types.

Counting the min(owned, constructed) across multiple types (e.g., Port with Factory; Factory with Port) can accelerate costs compared to previous single-type curves. Make sure this is intended and reflected in balance docs.

Also applies to: 466-476, 493-510

---

534-549: costWrapper: readable and safe.

The function is side-effect free and clamps nicely using min(owned, constructed).

---

572-574: allianceRequestDuration addition: nice.

Good central knob for diplomacy timing.

---

845-850: Bot goldAdditionRate increased; ensure tests cover bot economy.

A quick regression test around bot pacing would help catch unexpected snowballing.

---

23-23: API Changes Fully Applied

All call sites for the updated trainSpawnRate and trainGold signatures have been updated—no leftover boolean arguments or mismatched parameters remain.

• trainGold(rel)
– No calls with true/false were found. Both calls in src/core/game/TrainStation.ts now pass a "self" | "friendly" | "other" union via rel(trainOwner, stationOwner).
• trainSpawnRate(numPlayerFactories)
– The call in src/core/execution/TrainStationExecution.ts passes a number (unit count) as expected.
• Configuration signatures in src/core/configuration/Config.ts and their implementations in src/core/configuration/DefaultConfig.ts match the new method shapes.

No further updates are required.

[coderabbitai]

⚠️ Potential issue

Crash when userMe is null: new Set(undefined) throws.

Guard with a fallback empty array.

Apply this diff:

-  const playerFlares = new Set(userMe?.player.flares);
+  const playerFlares = new Set(userMe?.player.flares ?? []);

📝 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.

Suggested change

	const patterns: Map<string, Pattern> = new Map();
	const playerFlares = new Set(userMe?.player.flares);
	for (const name in cosmetics.patterns) {
	const patterns: Map<string, Pattern> = new Map();
	const playerFlares = new Set(userMe?.player.flares ?? []);
	for (const name in cosmetics.patterns) {

🤖 Prompt for AI Agents

In src/client/Cosmetics.ts around lines 18 to 21, constructing new
Set(userMe?.player.flares) can throw when userMe or player.flares is
null/undefined; change to use a safe fallback (e.g. new
Set(userMe?.player?.flares ?? [])) so the set is created from an empty array
when flares are missing.

[coderabbitai]

⚠️ Potential issue

Bug: re-adding the listener in disconnectedCallback leaks handlers and duplicates updates.

disconnectedCallback should only remove listeners.

Apply this diff:

   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.

Suggested change

	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 listener then immediately re-adds it, causing event handler leaks
and duplicate updates; modify the method so it only removes the listener (delete
the window.addEventListener("flag-change", this.updateFlag as EventListener)
line) and keep the existing window.removeEventListener call using the same
this.updateFlag reference so the handler is correctly detached.

[coderabbitai]

🛠️ Refactor suggestion

Don’t auto-open the modal from refresh(); it causes flicker and unintended opens.

refresh() calls this.modalEl.open() unconditionally. It runs in open(), selectPattern(), and onUserMe(). This re-opens the modal when selecting a pattern and when user data loads, causing UX glitches.

• Make refresh(openModal = false) and only open the modal when explicitly requested.
• Call await this.refresh(true) from open().
• Call await this.refresh(false) from onUserMe() and selectPattern().

Apply these diffs:

-  public async open() {
+  public async open() {
     this.isActive = true;
-    await this.refresh();
+    await this.refresh(true);
     window.addEventListener("keydown", this.handleKeyDown);
   }

-  private selectPattern(pattern: Pattern | undefined) {
+  private async selectPattern(pattern: Pattern | undefined) {
     this.userSettings.setSelectedPatternName(pattern?.name);
     this.selectedPattern = pattern;
-    this.refresh();
+    await this.refresh(false);
     this.close();
   }

-  public async refresh() {
+  public async refresh(openModal = false) {
     const preview = this.renderPatternPreview(
       this.selectedPattern?.pattern,
       48,
       48,
     );
     this.requestUpdate();

     // Wait for the DOM to be updated and the o-modal element to be available
     await this.updateComplete;

-    // Now modalEl should be available
-    if (this.modalEl) {
-      this.modalEl.open();
-    } else {
-      console.warn("modalEl is still null after updateComplete");
-    }
+    if (openModal) {
+      if (this.modalEl) {
+        this.modalEl.open();
+      } else {
+        console.warn("modalEl is still null after updateComplete");
+      }
+    }
     if (this.previewButton === null) return;
     render(preview, this.previewButton);
     this.requestUpdate();
   }

   async onUserMe(userMeResponse: UserMeResponse | null) {
     if (userMeResponse === null) {
       this.userSettings.setSelectedPatternName(undefined);
       this.selectedPattern = undefined;
     }
     this.patterns = await patterns(userMeResponse);
     const storedPatternName = this.userSettings.getSelectedPatternName();
     if (storedPatternName) {
       this.selectedPattern = this.patterns.get(storedPatternName);
     }
-    this.refresh();
+    await this.refresh(false);
   }

Also applies to: 246-251, 305-325, 51-62

🤖 Prompt for AI Agents

In src/client/TerritoryPatternsModal.ts around lines 51-62, 234-238, 246-251 and
305-325, change refresh() to accept a boolean flag (e.g., refresh(openModal =
false)) and remove the unconditional this.modalEl.open() so the modal is only
opened when openModal is true; then update calls accordingly: in open() call
await this.refresh(true) and in onUserMe() and selectPattern() call await
this.refresh(false) (or just await this.refresh()), and ensure any other
internal calls use the default false unless they explicitly should open the
modal.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Guard against divide-by-zero and revisit port multiplier wiring.

• For large numTradeShips, tradeShipBaseSpawn() can be exactly 0 (sigmoid→1), making combined=0 and rate=Infinity. Clamp to a small epsilon to avoid Infinity.
• The comment says “expected spawn rate ∝ numPlayerPorts × multiplier,” but you only multiply by multiplier. As ports increase, multiplier decreases, which increases rate (since rate = 12 / combined) and lowers spawn probability — opposite of intuition. Consider incorporating a saturating portFactor = numPlayerPorts × multiplier into combined.

Minimal fix (safety):

-    const combined = Math.sqrt(
-      this.tradeShipBaseSpawn(numTradeShips) *
-        this.tradeShipPortMultiplier(numPlayerPorts),
-    );
-
-    return Math.floor(12 / combined);
+    const base = this.tradeShipBaseSpawn(numTradeShips);
+    const mult = this.tradeShipPortMultiplier(numPlayerPorts);
+    let combined = Math.sqrt(base * mult);
+    combined = Math.max(combined, 1e-6); // avoid division by 0/Infinity
+    return Math.floor(12 / combined);

Optional follow-up (make more ports never reduce spawn probability):

-    const combined = Math.sqrt(
-      this.tradeShipBaseSpawn(numTradeShips) *
-        this.tradeShipPortMultiplier(numPlayerPorts),
-    );
+    const base = this.tradeShipBaseSpawn(numTradeShips);
+    const portFactor = numPlayerPorts * this.tradeShipPortMultiplier(numPlayerPorts); // increases and saturates
+    let combined = Math.sqrt(base * (1 + portFactor));

To validate monotonicity quickly:

Also applies to: 385-396

---

🏁 Script executed:

#!/bin/bash
# Explore tradeShipSpawnRate monotonicity vs ports and ships
python - << 'PY'
import math
def sigmoid(value, decayRate, midpoint):
    return 1/(1+math.exp(-decayRate*(value-midpoint)))
def base(numTradeShips):
    return 1 - sigmoid(numTradeShips, math.log(2)/30, 100)
def mult(numPorts):
    return 1/(1 + (1/10)*numPorts)
def rate(numShips, numPorts):
    combined = math.sqrt(max(base(numShips)*mult(numPorts), 1e-12))
    return math.floor(12/combined)
for ships in (0, 25, 50, 100, 200):
    row = [ships]
    for ports in (0,1,2,5,10,20,50):
        row.append(rate(ships, ports))
    print("ships=",ships," rates=",row[1:],"(ports=[0,1,2,5,10,20,50])")
PY

Length of output: 381

---

Guard against divide-by-zero and correct port multiplier behavior in tradeShipSpawnRate

The current implementation can underflow to zero (when base * mult becomes 0), causing tradeShipSpawnRate to return Infinity. It also inverts the intended port effect: as you add ports, the spawn rate increases (spawn probability decreases).

• File: src/core/configuration/DefaultConfig.ts, method tradeShipSpawnRate
– Clamp combined to a small ε before dividing
– Adjust multiplier logic if you want more ports to raise spawn probability

Minimal fix (prevent Infinity):

@@ src/core/configuration/DefaultConfig.ts:374
-    const combined = Math.sqrt(
-      this.tradeShipBaseSpawn(numTradeShips) *
-        this.tradeShipPortMultiplier(numPlayerPorts),
-    );
-    return Math.floor(12 / combined);
+    const base = this.tradeShipBaseSpawn(numTradeShips);
+    const mult = this.tradeShipPortMultiplier(numPlayerPorts);
+    let combined = Math.sqrt(base * mult);
+    combined = Math.max(combined, 1e-6); // avoid division by zero/Infinity
+    return Math.floor(12 / combined);

Optional refactor (make more ports boost spawn probability):

+    const portFactor = numPlayerPorts * this.tradeShipPortMultiplier(numPlayerPorts);
+    combined = Math.sqrt(base * (1 + portFactor));

📝 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.

Suggested change

	// Probability of trade ship spawn = 1 / tradeShipSpawnRate
	tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number {
	// Geometric mean of base spawn rate and port multiplier
	const combined = Math.sqrt(
	this.tradeShipBaseSpawn(numTradeShips) *
	this.tradeShipPortMultiplier(numPlayerPorts),
	);
	let totalMultiplier = 1;
	for (let i = 0; i < numPorts; i++) {
	totalMultiplier += basePortBonus * Math.pow(diminishingFactor, i);
	}
	return Math.floor(12 / combined);
	}
	// Probability of trade ship spawn = 1 / tradeShipSpawnRate
	tradeShipSpawnRate(numTradeShips: number, numPlayerPorts: number): number {
	// Geometric mean of base spawn rate and port multiplier
	const base = this.tradeShipBaseSpawn(numTradeShips);
	const mult = this.tradeShipPortMultiplier(numPlayerPorts);
	let combined = Math.sqrt(base * mult);
	combined = Math.max(combined, 1e-6); // avoid division by zero/Infinity
	return Math.floor(12 / combined);
	}

🤖 Prompt for AI Agents

In src/core/configuration/DefaultConfig.ts around lines 374 to 383, the computed
combined value can underflow to zero and produce Infinity on the division, and
the current port multiplier inverts the intended effect (more ports should
increase spawn probability); fix this by clamping combined to a small epsilon
before dividing (e.g., combined = Math.max(combined, 1e-6)) and update the port
multiplier logic so it increases spawn probability as numPlayerPorts grows (for
example make tradeShipPortMultiplier return a value >=1 that grows with
numPlayerPorts such as 1 + k * numPlayerPorts or return the reciprocal if
current implementation decreases with ports), then compute spawn rate using the
clamped combined value.

[coderabbitai]

🛠️ Refactor suggestion

Fix strict property initialization for mg.

Mark mg as definitely assigned to satisfy TS when strictPropertyInitialization is on.

-  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.

Suggested change

	private mg: Game;
	private mg!: Game;

🤖 Prompt for AI Agents

In src/core/execution/alliance/AllianceRequestExecution.ts around line 12, the
property "mg" lacks a definite assignment and fails TS
strictPropertyInitialization; mark it as definitely assigned by changing the
declaration to use the definite assignment assertion (e.g., declare it as
private mg!: Game;) or ensure it is assigned in the class constructor before any
use (assign this.mg = <Game instance>), so the compiler knows it will be
initialized.

[coderabbitai]

⚠️ Potential issue

Do not linger when recipient is missing; deactivate immediately.

Otherwise this execution stays active until duration elapses, wasting ticks.

Apply this diff:

     if (!mg.hasPlayer(this.recipientID)) {
       console.warn(
         `AllianceRequestExecution recipient ${this.recipientID} not found`,
       );
-      return;
+      this.active = false;
+      return;
     }

🤖 Prompt for AI Agents

In src/core/execution/alliance/AllianceRequestExecution.ts around lines 21-27,
the execution currently just logs and returns when the recipient is missing, but
it should be deactivated immediately to avoid wasting ticks; update the branch
so that after detecting !mg.hasPlayer(this.recipientID) you call the execution
deactivation routine (e.g., this.deactivate() or the project’s canonical method
to mark executions finished/removed from the manager) and then return, ensuring
the execution is no longer active and will not consume further ticks.

[coderabbitai]

🛠️ Refactor suggestion

Handle createAllianceRequest() possibly returning null.

When it returns null, this exec will idle until expiry. Fail fast and log.

-      } else {
-        this.req = this.requestor.createAllianceRequest(recipient);
-      }
+      } else {
+        this.req = this.requestor.createAllianceRequest(recipient);
+        if (this.req === null) {
+          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.

Suggested change

	const incoming = recipient
	.outgoingAllianceRequests()
	.find((r) => r.recipient() === this.requestor);
	if (incoming) {
	// If the recipient already has pending alliance request,
	// then accept it instead of creating a new one.
	this.active = false;
	incoming.accept();
	} else {
	this.req = this.requestor.createAllianceRequest(recipient);
	}
	}
	const incoming = recipient
	.outgoingAllianceRequests()
	.find((r) => r.recipient() === this.requestor);
	if (incoming) {
	// If the recipient already has pending alliance request,
	// then accept it instead of creating a new one.
	this.active = false;
	incoming.accept();
	} else {
	this.req = this.requestor.createAllianceRequest(recipient);
	if (this.req === null) {
	console.warn("createAllianceRequest returned null");
	this.active = false;
	}
	}
	}

🤖 Prompt for AI Agents

In src/core/execution/alliance/AllianceRequestExecution.ts around lines 34 to
45, createAllianceRequest(...) may return null which currently leaves this
execution idling until expiry; update the code to handle a null return
immediately by logging an error with context (requestor and recipient ids),
marking this.active = false and setting an explicit failure state (or throwing a
controlled error) instead of assigning null to this.req so the exec fails fast
and does not hang.

[coderabbitai]

🛠️ Refactor suggestion

Keep relation bumps consistent on auto-accept path.

AllianceRequestReplyExecution adds +100 relation on accept; the auto-accept path here does not. Align behaviors to avoid divergent diplomacy states.

       if (incoming) {
         // If the recipient already has pending alliance request,
         // then accept it instead of creating a new one.
         this.active = false;
-        incoming.accept();
+        incoming.accept();
+        this.requestor.updateRelation(recipient, 100);
+        recipient.updateRelation(this.requestor, 100);
       } else {

📝 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.

Suggested change

	// If the recipient already has pending alliance request,
	// then accept it instead of creating a new one.
	this.active = false;
	incoming.accept();
	} else {
	// If the recipient already has pending alliance request,
	// then accept it instead of creating a new one.
	this.active = false;
	incoming.accept();
	this.requestor.updateRelation(recipient, 100);
	recipient.updateRelation(this.requestor, 100);
	} else {

🤖 Prompt for AI Agents

In src/core/execution/alliance/AllianceRequestExecution.ts around lines 38–42
the auto-accept branch marks the request inactive and calls incoming.accept()
but does not apply the +100 relation bump that AllianceRequestReplyExecution
applies on accept; update this auto-accept path to call the same relation-bump
logic (or the shared helper) used by AllianceRequestReplyExecution so the +100
relation is applied, preserve this.active = false and incoming.accept(), and
ensure any persistence/events triggered by the reply path are also executed so
diplomacy state remains consistent.

[coderabbitai]

🛠️ Refactor suggestion

Make accept/reject idempotent to avoid double-processing.

Right now, repeated calls re-invoke game handlers. Guarding on "pending" prevents accidental re-entry and aligns with a simple state machine.

Apply this diff:

   accept(): 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);
   }

I can also add a tiny unit test to assert idempotency on accept/reject.

📝 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.

Suggested change

	accept(): void {
	this.status_ = "accepted";
	this.game.acceptAllianceRequest(this);
	}
	reject(): void {
	this.status_ = "rejected";
	this.game.rejectAllianceRequest(this);
	}
	accept(): void {
	if (this.status_ !== "pending") return;
	this.status_ = "accepted";
	this.game.acceptAllianceRequest(this);
	}
	reject(): void {
	if (this.status_ !== "pending") return;
	this.status_ = "rejected";
	this.game.rejectAllianceRequest(this);
	}

🤖 Prompt for AI Agents

In src/core/game/AllianceRequestImpl.ts around lines 31 to 38, accept() and
reject() are not idempotent and re-invoke game handlers on repeated calls;
modify both methods to first check that this.status_ === "pending" and only then
set the status_ ("accepted" or "rejected") and call
this.game.acceptAllianceRequest(this) / this.game.rejectAllianceRequest(this),
otherwise return/do nothing so repeated calls have no effect.

[coderabbitai]

⚠️ Potential issue

Pattern handling flow: unknown path closes the socket; mismatch with comment and inconsistent error contract.

• The comment says “allow player to join but disable pattern,” yet the code closes the connection on “unknown”.
• Elsewhere in this handler you surface join failures by returning a { success:false, code, reason, error } object; here you bypass that and call ws.close() directly. This breaks observability and consistency.
• Recommended: fail-open on “unknown” (let join continue with no pattern) and map “forbidden” cases to the existing reason enums via a returned failure, not a direct close.

Apply:

-      const result = privilegeRefresher
+      const check = privilegeRefresher
         .get()
-        .isPatternAllowed(clientMsg.patternName, flares);
-      switch (result.type) {
+        .isPatternAllowed(clientMsg.patternName, flares);
+      switch (check.type) {
         case "allowed": {
-          const { pattern: resultPattern } = result;
+          const { pattern: resultPattern } = check;
           pattern = resultPattern;
           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;
+        case "unknown": {
+          // Fail-open: let player in without a pattern.
+          log.warn(`Pattern ${clientMsg.patternName} unknown; allowing join without pattern`);
+          pattern = undefined;
+          break;
+        }
+        case "forbidden": {
+          log.warn(`Pattern ${clientMsg.patternName}: ${check.reason}`);
+          const reason =
+            check.reason === "invalid pattern"
+              ? "Pattern invalid"
+              : check.reason === "pattern not found"
+                ? "Pattern unlisted"
+                : "Pattern restricted";
+          return {
+            success: false,
+            code: 1002,
+            reason,
+            error: `Pattern ${clientMsg.patternName}: ${check.reason}`,
+          };
+        }
         default:
-          assertNever(result);
+          assertNever(check);
       }

📝 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.

Suggested change

	let pattern: string | undefined;
	// Check if the pattern is allowed
	if (clientMsg.pattern !== undefined) {
	const allowed = privilegeRefresher
	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": {
	const { pattern: resultPattern } = result;
	pattern = resultPattern;
	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);
	}
	let pattern: string | undefined;
	// Check if the pattern is allowed
	if (clientMsg.patternName !== undefined) {
	const check = privilegeRefresher
	.get()
	.isPatternAllowed(clientMsg.patternName, flares);
	switch (check.type) {
	case "allowed": {
	const { pattern: resultPattern } = check;
	pattern = resultPattern;
	break;
	}
	case "unknown": {
	// Fail-open: let player in without a pattern.
	log.warn(`Pattern ${clientMsg.patternName} unknown; allowing join without pattern`);
	pattern = undefined;
	break;
	}
	case "forbidden": {
	log.warn(`Pattern ${clientMsg.patternName}: ${check.reason}`);
	const reason =
	check.reason === "invalid pattern"
	? "Pattern invalid"
	: check.reason === "pattern not found"
	? "Pattern unlisted"
	: "Pattern restricted";
	return {
	success: false,
	code: 1002,
	reason,
	error: `Pattern ${clientMsg.patternName}: ${check.reason}`,
	};
	}
	default:
	assertNever(check);
	}
	}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
15 out of 20 committers have signed the CLA.

✅ Kipstz
✅ TLS15
✅ djmrFunnyMan
✅ DevelopingTom
✅ TylerHanavan
✅ pierrebrtr
✅ Aotumuri
✅ TheGiraffe3
✅ woodydrn
✅ evanpelle
✅ ElMelchizedek
✅ camclark
✅ abdallahbahrawi1
✅ fantom845
✅ Killersoren
❌ VariableVince
❌ scottanderson
❌ dependabot[bot]
❌ Baguette8ot
❌ aaa4xu
_{You have signed the CLA already but the status is still pending? Let us recheck it.}