[Feature] Test a skin

[ryanbarlow97]

Description:

It lets users test skins :)

They are added to Iceland Map no bots, no nations, max troops, max speed, auto attack wilderness, win victory at 99%, and then the testing complete modal appears with the skin they tested is present (rating system not developed as OOS)

Game does not save, the username of the player is the name of the skin, the ID of the game is the name of the skin too.

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:

w.o.n

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/client/TerritoryPatternsModal.ts`:
- Around line 210-211: The startTestGame method in TerritoryPatternsModal
currently does a silent early return when this.userMeResponse is falsy; change
this so the user receives feedback: either show an in-UI warning/toast/modal
(e.g., call a helper like showNotLoggedInToast or this.showWarning) before
returning, and/or disable the "Test" button in the component render logic by
binding its disabled state to the same this.userMeResponse check; update both
startTestGame (to show feedback) and the render/button code (to prevent the
action when not logged in) so behavior matches the store's "NOT LOGGED IN"
badge.
- Around line 250-278: The config object in TerritoryPatternsModal (the object
literal named config) is missing a game speed setting despite the PR claiming
"maximum speed"; add a gameSpeed property (e.g., gameSpeed: GameSpeed.Maximum or
the appropriate enum/value used elsewhere) to the config so the modal actually
configures the game to max speed, and import/reference the GameSpeed enum if not
already present; alternatively, if no global gameSpeed exists, add/rename the
correct global speed property used by the game engine to match the PR
description.

In `@src/core/execution/TestSkinExecution.ts`:
- Around line 107-122: initialAttack currently reschedules itself indefinitely
when playerByClientID returns null; add a bounded retry counter on the instance
(e.g., initialAttackRetries and MAX_INITIAL_ATTACK_RETRIES) and increment it
each time initialAttack schedules another attempt via
scheduleInitialAttack(100), stop reseting/clearing the counter and any pending
timeout; if retries exceed the max, stop scheduling and log or bail out. Update
initialAttack, scheduleInitialAttack and stop to reference these symbols
(initialAttackRetries, MAX_INITIAL_ATTACK_RETRIES, scheduleInitialAttack, stop,
isRunnerActive, myPlayer, gameView.playerByClientID, clientID) so the retry is
explicit and cancelled when stopped.

🧹 Nitpick comments (5)

src/client/TerritoryPatternsModal.ts (1)

222-230: Translation-miss detection relies on the key being returned verbatim — this is fragile.

The check translation.startsWith("territory_patterns.pattern.") assumes the i18n library returns the raw key when a translation is missing. If the library ever changes behavior (e.g., returns empty string, throws, or wraps the key), this fallback silently breaks and the user sees a raw key or garbage.

A safer approach: check equality against the key you passed in.

💡 Suggested tweak

-    const translation = translateText(
-      `territory_patterns.pattern.${pattern.name}`,
-    );
-    const displayName = translation.startsWith("territory_patterns.pattern.")
+    const translationKey = `territory_patterns.pattern.${pattern.name}`;
+    const translation = translateText(translationKey);
+    const displayName = translation === translationKey
       ? pattern.name
           .split("_")
           .map((w) => w.charAt(0).toUpperCase() + w.slice(1))
           .join(" ")
       : translation;

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

36-49: Extract magic numbers into named constants.

100 (initial attack delay) and 120000 (modal delay / 2 minutes) are hard-coded. Pulling them into private static readonly constants at the top of the class makes intent clear and avoids mistakes if either value needs to change.

Suggested fix

 export class TestSkinExecution implements Execution {
+  private static readonly INITIAL_ATTACK_DELAY_MS = 100;
+  private static readonly MODAL_DELAY_MS = 120_000;
+
   private myPlayer: PlayerView | null = null;
   ...

   public start() {
-    this.scheduleInitialAttack(100);
+    this.scheduleInitialAttack(TestSkinExecution.INITIAL_ATTACK_DELAY_MS);
     ...
     this.modalTimeoutId = setTimeout(() => {
       ...
-    }, 120000);
+    }, TestSkinExecution.MODAL_DELAY_MS);
   }

---

64-72: showModal() calls stop() before resolving the player — silent swallow of onShowModalRequested errors.

Two things to note here:

1. The catch block on lines 67-69 swallows any error from onShowModalRequested() without logging. If this callback ever throws unexpectedly, debugging will be painful. At least log a warning.
2. stop() on line 72 sets active = false before attempting to resolve the player. This is fine if intentional (no further ticks after modal), but it means a failed player lookup leaves the execution permanently stopped with no modal shown and no way to recover. Just flagging for awareness.

Suggested minimal improvement

     try {
       this.onShowModalRequested();
     } catch (e) {
-      // ignore
+      console.warn("onShowModalRequested threw", e);
     }

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

97-98: Use Lit's nothing sentinel instead of empty html\``.

When there is nothing to render, Lit provides nothing (imported from lit) which avoids creating an empty template result. Small thing, but more idiomatic:

-import { LitElement, html } from "lit";
+import { LitElement, html, nothing } from "lit";
 ...
-    if (!this.isVisible) return html``;
+    if (!this.isVisible) return nothing;

---

189-201: Inline <style> in render() lives in the global scope.

Since createRenderRoot returns this (no Shadow DOM), this <style> block is injected into the light DOM. The animate-fadeIn class name could collide with other global styles or future Tailwind config. Consider either:

• Moving this animation to your global CSS / Tailwind config, or
• Using a more specific class name like skin-test-animate-fadeIn.

This is minor since the keyframe name fadeIn and class animate-fadeIn are simple enough to collide with other components doing the same.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/client/ClientGameRunner.ts`:
- Around line 365-368: Replace the incorrect use of this.lobby.clientID
(LobbyConfig has no clientID) with the class field this.clientID when
constructing TestSkinExecution; locate the TestSkinExecution instantiation in
ClientGameRunner (the one passing this.gameView and this.lobby) and change the
second argument to this.clientID so the call matches the actual class field
initialized in the constructor and compiles against the LobbyConfig type.

[ryanbarlow97]

@iiamlewis