Improve runes #418

2025-11-18T08:05:02Z

LunarTides commented

2025-11-18 08:05:02 +00:00

(Migrated from github.com)

Use a new Rune enum rather than a string using characters.

Closes #417

Summary by CodeRabbit

New Features
- Introduced a typed rune system with three rune types (Blood, Frost, Unholy) and array-based rune assignment for cards and decks.
Bug Fixes
- Made rune handling consistent across cards, player logic, deck import/export and creators; added stronger validation and clearer error behavior for invalid rune input.
Tests
- Updated rune-related tests to use the typed rune format and verify new validation/serialization behavior.

Use a new `Rune` enum rather than a string using characters. Closes #417  ## Summary by CodeRabbit * **New Features** * Introduced a typed rune system with three rune types (Blood, Frost, Unholy) and array-based rune assignment for cards and decks. * **Bug Fixes** * Made rune handling consistent across cards, player logic, deck import/export and creators; added stronger validation and clearer error behavior for invalid rune input. * **Tests** * Updated rune-related tests to use the typed rune format and verify new validation/serialization behavior.

coderabbitai[bot] (Migrated from github.com) reviewed

2025-11-18 08:10:03 +00:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/functions/deckcode.ts (1)
731-774: Fix array-to-string concatenation that breaks rune parsing at line 739

The bug is confirmed. At line 739, runes += card.runes.map((r) => r[0]) concatenates an array with a string, which produces a comma-separated value (e.g., "B,B,F" instead of "BBF"). This malformed string fails both rune regex patterns at lines 63 and 66, causing the rune header to be misparsed as part of the hero name, which then fails validation and invalidates the deck.

The fix is correct—use .join("") to concatenate the rune characters:
-				runes += card.runes.map((r) => r[0]);
+				runes += card.runes.map((r) => r[0]).join("");

🧹 Nitpick comments (5)

tools/cardcreator/custom.ts (1)
126-143: Remove redundant filter after error throw.

The code throws an error on line 137 if any rune is undefined, making the filter on line 140 unreachable for undefined values. While the filter is defensive, it's redundant given the preceding throw.

Apply this diff to remove the redundant filter:
 		const runesArray = runesString
 			.split("")
 			.map((char) =>
 				Object.values(Rune).find((rune) => rune.startsWith(char)),
 			);
 		if (runesArray.some((rune) => rune === undefined)) {
 			throw new Error("Invalid rune found.");
 		}
 
-		runes = runesArray.filter((rune) => rune !== undefined);
+		runes = runesArray as Rune[];
Since we've already verified no undefined values exist, we can safely cast the array to Rune[].
tools/deckcreator.ts (1)
97-119: Simplify class assignment - unnecessary enum lookup.

On line 112, the expression Class[cl as Class] is redundant. For string enums like Class, when cl is "Mage", Class["Mage"] simply returns "Mage" again. Since cl is already validated as a member of the classes array (which contains Class values), you can assign it directly.

Apply this diff to simplify:
-	if (!cl) {
-		await game.pause("\n<red>Invalid class.</red>");
-		return askClass();
-	}
-
-	heroClass = Class[cl as Class];
+	if (!cl) {
+		await game.pause("\n<red>Invalid class.</red>");
+		return askClass();
+	}
+
+	heroClass = cl;
The subsequent undefined check (lines 115-119) ensures type safety, so this simplification is safe.
src/player.ts (1)
15-18: Rune[] integration is consistent; consider empty‑requirement semantics and a helper for counts

The switch to a strongly typed Rune import plus runes: Rune[] = [] and the new testRunes(runes: Rune[]) implementation looks aligned with the enum‑based design and preserves the old “count runes by type” behavior.

Two small follow‑ups to consider:
If testRunes([]) is ever called for cards with no rune requirement, the guard if (runes.length <= 0 || this.runes.length <= 0) forces it to return false. If call sites don’t already skip rune‑less cards, this could incorrectly reject otherwise valid cards. A safer pattern is e.g.:
testRunes(runes: Rune[]): boolean {
	if (runes.length === 0) {
		return true;
	}
	if (this.runes.length === 0) {
		return false;
	}
	// existing counting logic...
}
You recompute counts via filter six times. A small helper like countRunes(runes: Rune[]) returning { blood, frost, unholy } would keep the logic DRY and make future rune additions less error‑prone.
Also applies to: 275-277, 981-983, 1031-1052
src/functions/deckcode.ts (2)
1-12: Make Rune import + char→Rune mapping type‑safe

Importing Rune and mapping from characters to enum values is the right direction, but this block has a couple of type/robustness sharp edges:
const actualRunes = runes
	.split("")
	.map((char) =>
		Object.values(Rune).find((rune) => rune.startsWith(char)),
	)
	// TODO: Maybe throw an error if this fails?
	.filter((r) => r !== undefined);

player.runes = actualRunes;
Object.values(Rune) will include non‑string members if Rune is a numeric enum; calling .startsWith on those will throw at runtime. This is safe only if Rune is a string enum.

filter((r) => r !== undefined) does not narrow the type for TypeScript, so actualRunes is still seen as (Rune | undefined)[], which doesn’t strictly match player.runes: Rune[].

A more robust version that keeps runtime behavior and tightens types:
const actualRunes = runes
	.split("")
	.map((char) =>
		Object.values(Rune)
			.filter((v): v is string => typeof v === "string")
			.find((rune) => rune.startsWith(char)),
	)
	.filter((r): r is Rune => r !== undefined);

player.runes = actualRunes;
This both guards against non‑string enum members and gives you a clean Rune[] type.

Also applies to: 90-101

321-323: Exporting runes from Rune[] – tighten handling of empty arrays and update docs

You’ve updated the export API to accept Rune[]:
/**
 * @param runes The runes of the deck. Example: "BFU"
 */
export(
	deck: CardLike[],
	heroClass: string,
	runes: Rune[],
): { code: string; error: FunctionsExportDeckError } {
	…
	if (runes) {
		deckcode +=
			new Set(runes).size === 1
				? `[3${runes[0][0]}] `
				: `[${runes.map((r) => r[0]).join("")}] `;
	}
Two things to clean up:

The JSDoc still shows runes as the old "BFU" string representation. It should now show a Rune[] example, e.g. [Rune.Blood, Rune.Frost, Rune.Unholy], and maybe note that the deckcode still uses initials.

if (runes) is always true for an array, even when it’s empty. If any call site passes [] for non‑Death Knight decks, this will render [] into the deckcode, which won’t match the import regex and will yield an invalid header.

A small adjustment makes the behavior safer and clearer:
/**
 * @param runes The runes of the deck. Example: [Rune.Blood, Rune.Frost, Rune.Unholy]
 */
export(
	deck: CardLike[],
	heroClass: string,
	runes: Rune[],
): { code: string; error: FunctionsExportDeckError } {
	…
	if (runes.length) {
		const runeChars = runes.map((r) => r[0]);
		deckcode +=
			new Set(runeChars).size === 1
				? `[3${runeChars[0]}] `
				: `[${runeChars.join("")}] `;
	}
This preserves the existing encoding (single‑type [3X] or mixed [XYZ]) while avoiding invalid [] output.

Also applies to: 325-329, 355-363

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99b306e0b3 and a105ec6aa2.

📒 Files selected for processing (10)

cards/Examples/2/4-runes.ts (2 hunks)
cards/Examples/2/lastb-main.ts (2 hunks)
src/card.ts (2 hunks)
src/functions/deckcode.ts (6 hunks)
src/player.ts (3 hunks)
src/types/card.ts (2 hunks)
test/src/player.test.ts (2 hunks)
tools/cardcreator/custom.ts (2 hunks)
tools/cardcreator/lib.ts (2 hunks)
tools/deckcreator.ts (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

tools/deckcreator.ts (1)

src/functions/interact.ts (1)

watermark (858-922)

tools/cardcreator/custom.ts (2)

src/game.ts (1)

input (1252-1262)

src/functions/interact.ts (1)

input (1203-1250)

tools/cardcreator/lib.ts (1)

cards/Examples/2/4-runes.ts (1)

blueprint (13-37)

🔇 Additional comments (8)

src/types/card.ts (2)

188-195: LGTM! Clean enum definition.

The new Rune enum is well-defined with clear, self-documenting values. The string enum pattern is consistent with other enums in the file (e.g., Class, Rarity, SpellSchool).

420-420: Type change verification complete—all rune usages properly updated.

Codebase search confirms zero string-based rune assignments. All usages across the codebase (deckcreator, deckcode, player logic, card types, tests, and examples) consistently use the Rune enum, confirming the breaking change has been fully migrated.

cards/Examples/2/lastb-main.ts (1)

10-10: LGTM! Example updated correctly.

The import of Rune is properly added, and the rune assignment correctly uses the new array-based syntax with enum values.

Also applies to: 30-30

src/card.ts (1)

20-20: LGTM! Core type change correctly implemented.

The Rune type import and the field type change from string to Rune[] are correctly implemented. This is the fundamental change that enables typed rune handling throughout the codebase.

Also applies to: 304-304

test/src/player.test.ts (1)

13-13: Excellent test coverage for the Rune[] migration.

The tests comprehensively validate the new Rune[] behavior:

Order independence (e.g., [Rune.Blood, Rune.Unholy] vs [Rune.Unholy, Rune.Blood])

Repeated rune handling (e.g., [Rune.Blood, Rune.Blood, Rune.Blood])

Subset validation (ensuring cards requiring 2 Frost runes fail when player has only 1 Frost)

This thorough testing gives confidence that the migration is correct.

Also applies to: 638-684

cards/Examples/2/4-runes.ts (1)

9-9: LGTM! Rune assignment correctly updated.

The import and array-based rune assignment correctly reflect the requirement stated in the comment (2 Frost runes and 1 Blood rune).

Also applies to: 30-30

tools/cardcreator/lib.ts (1)

192-194: LGTM! Card generation correctly updated for Rune[].

The rune serialization correctly generates array syntax with enum values ([Rune.Blood, Rune.Frost]), and the conditional import ensures Rune is only imported when needed.

Also applies to: 261-263

tools/deckcreator.ts (1)

123-148: LGTM! Rune collection logic correctly implemented.

The rune collection loop properly:

Collects up to 3 runes from user input

Maps single-character input to Rune enum values by matching the first character

Validates and skips invalid input

Assigns the final array to player1.runes

**Actionable comments posted: 0** > [!CAUTION] > Some comments are outside the diff and can’t be posted inline due to platform limitations. > > > > <details> > <summary>⚠️ Outside diff range comments (1)</summary><blockquote> > > <details> > <summary>src/functions/deckcode.ts (1)</summary><blockquote> > > `731-774`: **Fix array-to-string concatenation that breaks rune parsing at line 739** > > The bug is confirmed. At line 739, `runes += card.runes.map((r) => r[0])` concatenates an array with a string, which produces a comma-separated value (e.g., `"B,B,F"` instead of `"BBF"`). This malformed string fails both rune regex patterns at lines 63 and 66, causing the rune header to be misparsed as part of the hero name, which then fails validation and invalidates the deck. > > The fix is correct—use `.join("")` to concatenate the rune characters: > > ```diff > - runes += card.runes.map((r) => r[0]); > + runes += card.runes.map((r) => r[0]).join(""); > ``` > > </blockquote></details> > > </blockquote></details> <details> <summary>🧹 Nitpick comments (5)</summary><blockquote> <details> <summary>tools/cardcreator/custom.ts (1)</summary><blockquote> `126-143`: **Remove redundant filter after error throw.** The code throws an error on line 137 if any rune is undefined, making the filter on line 140 unreachable for undefined values. While the filter is defensive, it's redundant given the preceding throw. Apply this diff to remove the redundant filter: ```diff const runesArray = runesString .split("") .map((char) => Object.values(Rune).find((rune) => rune.startsWith(char)), ); if (runesArray.some((rune) => rune === undefined)) { throw new Error("Invalid rune found."); } - runes = runesArray.filter((rune) => rune !== undefined); + runes = runesArray as Rune[]; ``` Since we've already verified no undefined values exist, we can safely cast the array to `Rune[]`. </blockquote></details> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> `97-119`: **Simplify class assignment - unnecessary enum lookup.** On line 112, the expression `Class[cl as Class]` is redundant. For string enums like `Class`, when `cl` is `"Mage"`, `Class["Mage"]` simply returns `"Mage"` again. Since `cl` is already validated as a member of the `classes` array (which contains `Class` values), you can assign it directly. Apply this diff to simplify: ```diff - if (!cl) { - await game.pause("\n<red>Invalid class.</red>"); - return askClass(); - } - - heroClass = Class[cl as Class]; + if (!cl) { + await game.pause("\n<red>Invalid class.</red>"); + return askClass(); + } + + heroClass = cl; ``` The subsequent undefined check (lines 115-119) ensures type safety, so this simplification is safe. </blockquote></details> <details> <summary>src/player.ts (1)</summary><blockquote> `15-18`: **Rune[] integration is consistent; consider empty‑requirement semantics and a helper for counts** The switch to a strongly typed `Rune` import plus `runes: Rune[] = []` and the new `testRunes(runes: Rune[])` implementation looks aligned with the enum‑based design and preserves the old “count runes by type” behavior. Two small follow‑ups to consider: - If `testRunes([])` is ever called for cards with no rune requirement, the guard `if (runes.length <= 0 || this.runes.length <= 0)` forces it to return `false`. If call sites don’t already skip rune‑less cards, this could incorrectly reject otherwise valid cards. A safer pattern is e.g.: ```ts testRunes(runes: Rune[]): boolean { if (runes.length === 0) { return true; } if (this.runes.length === 0) { return false; } // existing counting logic... } ``` - You recompute counts via `filter` six times. A small helper like `countRunes(runes: Rune[])` returning `{ blood, frost, unholy }` would keep the logic DRY and make future rune additions less error‑prone. Also applies to: 275-277, 981-983, 1031-1052 </blockquote></details> <details> <summary>src/functions/deckcode.ts (2)</summary><blockquote> `1-12`: **Make Rune import + char→Rune mapping type‑safe** Importing `Rune` and mapping from characters to enum values is the right direction, but this block has a couple of type/robustness sharp edges: ```ts const actualRunes = runes .split("") .map((char) => Object.values(Rune).find((rune) => rune.startsWith(char)), ) // TODO: Maybe throw an error if this fails? .filter((r) => r !== undefined); player.runes = actualRunes; ``` - `Object.values(Rune)` will include non‑string members if `Rune` is a numeric enum; calling `.startsWith` on those will throw at runtime. This is safe only if `Rune` is a string enum. - `filter((r) => r !== undefined)` does not narrow the type for TypeScript, so `actualRunes` is still seen as `(Rune | undefined)[]`, which doesn’t strictly match `player.runes: Rune[]`. A more robust version that keeps runtime behavior and tightens types: ```ts const actualRunes = runes .split("") .map((char) => Object.values(Rune) .filter((v): v is string => typeof v === "string") .find((rune) => rune.startsWith(char)), ) .filter((r): r is Rune => r !== undefined); player.runes = actualRunes; ``` This both guards against non‑string enum members and gives you a clean `Rune[]` type. Also applies to: 90-101 --- `321-323`: **Exporting runes from Rune[] – tighten handling of empty arrays and update docs** You’ve updated the export API to accept `Rune[]`: ```ts /** * @param runes The runes of the deck. Example: "BFU" */ export( deck: CardLike[], heroClass: string, runes: Rune[], ): { code: string; error: FunctionsExportDeckError } { … if (runes) { deckcode += new Set(runes).size === 1 ? `[3${runes[0][0]}] ` : `[${runes.map((r) => r[0]).join("")}] `; } ``` Two things to clean up: - The JSDoc still shows `runes` as the old `"BFU"` string representation. It should now show a Rune[] example, e.g. `[Rune.Blood, Rune.Frost, Rune.Unholy]`, and maybe note that the deckcode still uses initials. - `if (runes)` is always true for an array, even when it’s empty. If any call site passes `[]` for non‑Death Knight decks, this will render `[] ` into the deckcode, which won’t match the import regex and will yield an invalid header. A small adjustment makes the behavior safer and clearer: ```ts /** * @param runes The runes of the deck. Example: [Rune.Blood, Rune.Frost, Rune.Unholy] */ export( deck: CardLike[], heroClass: string, runes: Rune[], ): { code: string; error: FunctionsExportDeckError } { … if (runes.length) { const runeChars = runes.map((r) => r[0]); deckcode += new Set(runeChars).size === 1 ? `[3${runeChars[0]}] ` : `[${runeChars.join("")}] `; } ``` This preserves the existing encoding (single‑type `[3X]` or mixed `[XYZ]`) while avoiding invalid `[]` output. Also applies to: 325-329, 355-363 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 99b306e0b3cae9e2be076ee7430dfecd44e8f753 and a105ec6aa2ebd7d2c65b3a78bde0c4d903902f40. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `cards/Examples/2/4-runes.ts` (2 hunks) * `cards/Examples/2/lastb-main.ts` (2 hunks) * `src/card.ts` (2 hunks) * `src/functions/deckcode.ts` (6 hunks) * `src/player.ts` (3 hunks) * `src/types/card.ts` (2 hunks) * `test/src/player.test.ts` (2 hunks) * `tools/cardcreator/custom.ts` (2 hunks) * `tools/cardcreator/lib.ts` (2 hunks) * `tools/deckcreator.ts` (3 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> <details> <summary>src/functions/interact.ts (1)</summary> * `watermark` (858-922) </details> </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (2)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1203-1250) </details> </blockquote></details> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> <details> <summary>cards/Examples/2/4-runes.ts (1)</summary> * `blueprint` (13-37) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>src/types/card.ts (2)</summary><blockquote> `188-195`: **LGTM! Clean enum definition.** The new `Rune` enum is well-defined with clear, self-documenting values. The string enum pattern is consistent with other enums in the file (e.g., `Class`, `Rarity`, `SpellSchool`). --- `420-420`: **Type change verification complete—all rune usages properly updated.** Codebase search confirms zero string-based rune assignments. All usages across the codebase (deckcreator, deckcode, player logic, card types, tests, and examples) consistently use the Rune enum, confirming the breaking change has been fully migrated. </blockquote></details> <details> <summary>cards/Examples/2/lastb-main.ts (1)</summary><blockquote> `10-10`: **LGTM! Example updated correctly.** The import of `Rune` is properly added, and the rune assignment correctly uses the new array-based syntax with enum values. Also applies to: 30-30 </blockquote></details> <details> <summary>src/card.ts (1)</summary><blockquote> `20-20`: **LGTM! Core type change correctly implemented.** The `Rune` type import and the field type change from `string` to `Rune[]` are correctly implemented. This is the fundamental change that enables typed rune handling throughout the codebase. Also applies to: 304-304 </blockquote></details> <details> <summary>test/src/player.test.ts (1)</summary><blockquote> `13-13`: **Excellent test coverage for the Rune[] migration.** The tests comprehensively validate the new Rune[] behavior: - Order independence (e.g., `[Rune.Blood, Rune.Unholy]` vs `[Rune.Unholy, Rune.Blood]`) - Repeated rune handling (e.g., `[Rune.Blood, Rune.Blood, Rune.Blood]`) - Subset validation (ensuring cards requiring 2 Frost runes fail when player has only 1 Frost) This thorough testing gives confidence that the migration is correct. Also applies to: 638-684 </blockquote></details> <details> <summary>cards/Examples/2/4-runes.ts (1)</summary><blockquote> `9-9`: **LGTM! Rune assignment correctly updated.** The import and array-based rune assignment correctly reflect the requirement stated in the comment (2 Frost runes and 1 Blood rune). Also applies to: 30-30 </blockquote></details> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> `192-194`: **LGTM! Card generation correctly updated for Rune[].** The rune serialization correctly generates array syntax with enum values (`[Rune.Blood, Rune.Frost]`), and the conditional import ensures `Rune` is only imported when needed. Also applies to: 261-263 </blockquote></details> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> `123-148`: **LGTM! Rune collection logic correctly implemented.** The rune collection loop properly: - Collects up to 3 runes from user input - Maps single-character input to Rune enum values by matching the first character - Validates and skips invalid input - Assigns the final array to `player1.runes` </blockquote></details> </blockquote></details> </details>

coderabbitai[bot] (Migrated from github.com) reviewed

2025-11-18 10:36:09 +00:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/functions/deckcode.ts (3)
90-101: Make addRunes char→Rune mapping type‑safe and explicit

The new mapping from characters to Rune has a couple of rough edges:

Object.values(Rune).find(...) returns Rune | undefined, and .filter((r) => r !== undefined) does not narrow the type, so actualRunes is still typed as (Rune | undefined)[] and may not assign cleanly to player.runes under strict TypeScript settings.

Any unexpected character silently drops out of actualRunes, potentially leaving the player with fewer than 3 runes without a clear error (the TODO hints at this).

You can address both in a small, local tweak:
const actualRunes = runes
  .split("")
  .map((char) =>
    Object.values(Rune).find((rune) => rune.startsWith(char)),
  )
-  // TODO: Maybe throw an error if this fails?
-  .filter((r) => r !== undefined);
+  // TODO: Maybe throw an error if this fails?
+  .filter((r): r is Rune => r !== undefined);
If you do want to fail hard on bad input, you could also add a length check right after this and reuse panic("INVALIDRUNES") or throw.

321-363: Clarify and constrain Rune[] usage in export to avoid malformed codes

The new export signature and serialization logic look reasonable, but there are a couple of edge cases worth tightening up:

new Set(runes).size === 1 will encode [Rune.Blood] or [Rune.Blood, Rune.Blood] as [3B] even though the rune array is not length 3. If you expect exactly three runes whenever the array is non‑empty, it’s safer to enforce that invariant:
-    if (runes.length > 0) {
+    if (runes.length > 0) {
       // If the runes is 3 of one type, write, for example, 3B instead of BBB
-      deckcode +=
-        new Set(runes).size === 1
+      deckcode +=
+        runes.length === 3 && new Set(runes).size === 1
          ? `[3${runes[0][0]}] `
          : `[${runes.map((r) => r[0]).join("")}] `;
    }
Since this function is a public API and now requires runes: Rune[], you might also consider giving runes a default of [] (or an overload) so non‑rune classes don’t have to plumb an array everywhere, unless you explicitly want this to remain a strict breaking change.

731-741: Avoid duplicating Rune→character logic and relying on r[0]

fromVanilla now turns card.runes: Rune[] into a string via:
runes += card.runes.map((r) => r[0]).join("");
This implicitly relies on Rune’s string representation always starting with the desired deckcode character (B/F/U). The same assumption appears in export (runes[0][0], runes.map((r) => r[0])).

To keep this robust if the enum ever changes, consider centralizing the mapping:
const runeToChar = (r: Rune): string => r[0]; // or a switch/map if enum values change
and then use runeToChar both here and in export. That removes the magic indexing and keeps all rune encoding logic in one place.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a105ec6aa2 and b831562550698d267197eb9e3ed76a2acf18947e.

📒 Files selected for processing (3)

src/functions/deckcode.ts (6 hunks)
tools/cardcreator/custom.ts (2 hunks)
tools/deckcreator.ts (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

tools/deckcreator.ts

🧰 Additional context used

🧬 Code graph analysis (2)

tools/cardcreator/custom.ts (2)

src/game.ts (1)

input (1252-1262)

src/functions/interact.ts (1)

input (1203-1250)

src/functions/deckcode.ts (3)

tools/deckcreator.ts (2)

deck (1142-1147)

deckcode (1104-1115)

src/types/other.ts (1)

CardLike (104-104)

src/functions/interact.ts (1)

deckcode (62-116)

🔇 Additional comments (2)

tools/cardcreator/custom.ts (2)

9-9: LGTM!

The Rune import is correctly added to support the new typed rune system.

141-141: Verify the early-break behavior for multi-class cards.

The loop breaks after capturing runes for the first rune-capable class. If a card belongs to multiple classes that support runes, it will only prompt once and use those runes for all classes.

This appears intentional, but please confirm this aligns with Hearthstone's rune rules—specifically, whether multi-class cards should have a single set of runes or separate runes per class.

**Actionable comments posted: 1** <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>src/functions/deckcode.ts (3)</summary><blockquote> `90-101`: **Make `addRunes` char→Rune mapping type‑safe and explicit** The new mapping from characters to `Rune` has a couple of rough edges: - `Object.values(Rune).find(...)` returns `Rune | undefined`, and `.filter((r) => r !== undefined)` does not narrow the type, so `actualRunes` is still typed as `(Rune | undefined)[]` and may not assign cleanly to `player.runes` under strict TypeScript settings. - Any unexpected character silently drops out of `actualRunes`, potentially leaving the player with fewer than 3 runes without a clear error (the TODO hints at this). You can address both in a small, local tweak: ```diff const actualRunes = runes .split("") .map((char) => Object.values(Rune).find((rune) => rune.startsWith(char)), ) - // TODO: Maybe throw an error if this fails? - .filter((r) => r !== undefined); + // TODO: Maybe throw an error if this fails? + .filter((r): r is Rune => r !== undefined); ``` If you do want to fail hard on bad input, you could also add a length check right after this and reuse `panic("INVALIDRUNES")` or throw. --- `321-363`: **Clarify and constrain Rune[] usage in `export` to avoid malformed codes** The new `export` signature and serialization logic look reasonable, but there are a couple of edge cases worth tightening up: - `new Set(runes).size === 1` will encode `[Rune.Blood]` or `[Rune.Blood, Rune.Blood]` as `[3B]` even though the rune array is not length 3. If you expect exactly three runes whenever the array is non‑empty, it’s safer to enforce that invariant: ```diff - if (runes.length > 0) { + if (runes.length > 0) { // If the runes is 3 of one type, write, for example, 3B instead of BBB - deckcode += - new Set(runes).size === 1 + deckcode += + runes.length === 3 && new Set(runes).size === 1 ? `[3${runes[0][0]}] ` : `[${runes.map((r) => r[0]).join("")}] `; } ``` - Since this function is a public API and now requires `runes: Rune[]`, you might also consider giving `runes` a default of `[]` (or an overload) so non‑rune classes don’t have to plumb an array everywhere, unless you explicitly want this to remain a strict breaking change. --- `731-741`: **Avoid duplicating Rune→character logic and relying on `r[0]`** `fromVanilla` now turns `card.runes: Rune[]` into a string via: ```ts runes += card.runes.map((r) => r[0]).join(""); ``` This implicitly relies on `Rune`’s string representation always starting with the desired deckcode character (B/F/U). The same assumption appears in `export` (`runes[0][0]`, `runes.map((r) => r[0])`). To keep this robust if the enum ever changes, consider centralizing the mapping: ```ts const runeToChar = (r: Rune): string => r[0]; // or a switch/map if enum values change ``` and then use `runeToChar` both here and in `export`. That removes the magic indexing and keeps all rune encoding logic in one place. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a105ec6aa2ebd7d2c65b3a78bde0c4d903902f40 and b831562550698d267197eb9e3ed76a2acf18947e. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `src/functions/deckcode.ts` (6 hunks) * `tools/cardcreator/custom.ts` (2 hunks) * `tools/deckcreator.ts` (3 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * tools/deckcreator.ts </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>tools/cardcreator/custom.ts (2)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1203-1250) </details> </blockquote></details> <details> <summary>src/functions/deckcode.ts (3)</summary><blockquote> <details> <summary>tools/deckcreator.ts (2)</summary> * `deck` (1142-1147) * `deckcode` (1104-1115) </details> <details> <summary>src/types/other.ts (1)</summary> * `CardLike` (104-104) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `deckcode` (62-116) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>tools/cardcreator/custom.ts (2)</summary><blockquote> `9-9`: **LGTM!** The `Rune` import is correctly added to support the new typed rune system. --- `141-141`: **Verify the early-break behavior for multi-class cards.** The loop breaks after capturing runes for the first rune-capable class. If a card belongs to multiple classes that support runes, it will only prompt once and use those runes for all classes. This appears intentional, but please confirm this aligns with Hearthstone's rune rules—specifically, whether multi-class cards should have a single set of runes or separate runes per class. </blockquote></details> </blockquote></details> </details>

tools/cardcreator/custom.ts

					
				@ -117,12 +118,27 @@ async function common(): Promise<BlueprintWithOptional> {

							.map((k) => game.lodash.startCase(k) as Keyword);

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Fix case-insensitive matching and potential ambiguity in rune selection.

The current implementation has two critical issues:

Case sensitivity: rune.startsWith(char) is case-sensitive. If users enter lowercase letters (e.g., "f" for Frost) but the enum values are capitalized, the match will fail and throw "Invalid rune found."
Ambiguous matching: Using .find() with .startsWith() returns only the first matching rune. If multiple runes start with the same letter (e.g., "Frost" and "Fire" both start with "F"), users cannot select the second one.

Apply this diff to fix both issues by matching case-insensitively and using a map for unambiguous lookup:

-		const runesString = await input(
-			`Runes (${Object.values(Rune)
-				.map((rune) => rune[0])
-				.join("")}): `,
-		);
-		const runesArray = runesString
-			.split("")
-			.map((char) =>
-				Object.values(Rune).find((rune) => rune.startsWith(char)),
-			);
+		const runeInitials = Object.values(Rune).map((rune) => rune[0].toUpperCase());
+		const runesString = await input(
+			`Runes (${runeInitials.join("")}): `,
+		);
+		const runeMap = new Map(
+			Object.values(Rune).map((rune) => [rune[0].toUpperCase(), rune])
+		);
+		const runesArray = runesString
+			.toUpperCase()
+			.split("")
+			.map((char) => runeMap.get(char));
 		if (runesArray.some((rune) => rune === undefined)) {
-			throw new Error("Invalid rune found.");
+			const validInitials = Array.from(runeMap.keys()).join(", ");
+			throw new Error(`Invalid rune character(s) in input. Valid initials: ${validInitials}`);
 		}
 
 		runes = runesArray as Rune[];

This change:

Normalizes input and enum values to uppercase for case-insensitive comparison
Uses a Map for O(1) lookup and unambiguous matching
Improves the error message to show valid options

Run the following script to verify the Rune enum definition and check for initial collisions:

🏁 Script executed:

#!/bin/bash
# Description: Verify Rune enum values and check for collisions in first characters

# Find the Rune enum definition
ast-grep --pattern 'enum Rune {
  $$$
}'

# Also search for any Rune type definitions
rg -nP --type=ts -A 10 'enum Rune'

Length of output: 654

Fix case-sensitivity in rune selection logic.

The case-sensitivity issue is valid—users entering lowercase letters will be rejected even though the prompt only shows uppercase initials. However, verification shows the Rune enum has unique first characters (B, F, U), so there is no collision/ambiguity concern.

A simpler fix addresses only the actual issue:

-		const runesArray = runesString
-			.split("")
-			.map((char) =>
-				Object.values(Rune).find((rune) => rune.startsWith(char)),
-			);
+		const runesArray = runesString
+			.toUpperCase()
+			.split("")
+			.map((char) =>
+				Object.values(Rune).find((rune) => rune.startsWith(char)),
+			);

This normalizes input to uppercase, making matching case-insensitive without overcomplicating the logic.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

			const runesString = await input(
				`Runes (${Object.values(Rune)
					.map((rune) => rune[0])
					.join("")}): `,
			);
			const runesArray = runesString
				.toUpperCase()
				.split("")
				.map((char) =>
					Object.values(Rune).find((rune) => rune.startsWith(char)),
				);
			if (runesArray.some((rune) => rune === undefined)) {
				throw new Error("Invalid rune found.");
			}

			runes = runesArray as Rune[];

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 126 to 140, the rune-matching logic
is case-sensitive causing lowercase user input to be rejected; convert the user
input string to uppercase before splitting and matching (i.e., normalize
runesString to uppercase), then perform the same .split("").map(...) using
Object.values(Rune).find(rune => rune.startsWith(char)) and keep the existing
undefined check and cast to Rune[].

_⚠️ Potential issue_ | _🟠 Major_ <details> <summary>🧩 Analysis chain</summary> **Fix case-insensitive matching and potential ambiguity in rune selection.** The current implementation has two critical issues: 1. **Case sensitivity**: `rune.startsWith(char)` is case-sensitive. If users enter lowercase letters (e.g., "f" for Frost) but the enum values are capitalized, the match will fail and throw "Invalid rune found." 2. **Ambiguous matching**: Using `.find()` with `.startsWith()` returns only the first matching rune. If multiple runes start with the same letter (e.g., "Frost" and "Fire" both start with "F"), users cannot select the second one. Apply this diff to fix both issues by matching case-insensitively and using a map for unambiguous lookup: ```diff - const runesString = await input( - `Runes (${Object.values(Rune) - .map((rune) => rune[0]) - .join("")}): `, - ); - const runesArray = runesString - .split("") - .map((char) => - Object.values(Rune).find((rune) => rune.startsWith(char)), - ); + const runeInitials = Object.values(Rune).map((rune) => rune[0].toUpperCase()); + const runesString = await input( + `Runes (${runeInitials.join("")}): `, + ); + const runeMap = new Map( + Object.values(Rune).map((rune) => [rune[0].toUpperCase(), rune]) + ); + const runesArray = runesString + .toUpperCase() + .split("") + .map((char) => runeMap.get(char)); if (runesArray.some((rune) => rune === undefined)) { - throw new Error("Invalid rune found."); + const validInitials = Array.from(runeMap.keys()).join(", "); + throw new Error(`Invalid rune character(s) in input. Valid initials: ${validInitials}`); } runes = runesArray as Rune[]; ``` This change: - Normalizes input and enum values to uppercase for case-insensitive comparison - Uses a `Map` for O(1) lookup and unambiguous matching - Improves the error message to show valid options Run the following script to verify the Rune enum definition and check for initial collisions: --- 🏁 Script executed: ```shell #!/bin/bash # Description: Verify Rune enum values and check for collisions in first characters # Find the Rune enum definition ast-grep --pattern 'enum Rune { $$$ }' # Also search for any Rune type definitions rg -nP --type=ts -A 10 'enum Rune' ``` Length of output: 654 --- </details> **Fix case-sensitivity in rune selection logic.** The case-sensitivity issue is valid—users entering lowercase letters will be rejected even though the prompt only shows uppercase initials. However, verification shows the Rune enum has unique first characters (B, F, U), so there is no collision/ambiguity concern. A simpler fix addresses only the actual issue: ```diff - const runesArray = runesString - .split("") - .map((char) => - Object.values(Rune).find((rune) => rune.startsWith(char)), - ); + const runesArray = runesString + .toUpperCase() + .split("") + .map((char) => + Object.values(Rune).find((rune) => rune.startsWith(char)), + ); ``` This normalizes input to uppercase, making matching case-insensitive without overcomplicating the logic.  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion const runesString = await input( `Runes (${Object.values(Rune) .map((rune) => rune[0]) .join("")}): `, ); const runesArray = runesString .toUpperCase() .split("") .map((char) => Object.values(Rune).find((rune) => rune.startsWith(char)), ); if (runesArray.some((rune) => rune === undefined)) { throw new Error("Invalid rune found."); } runes = runesArray as Rune[]; ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 126 to 140, the rune-matching logic is case-sensitive causing lowercase user input to be rejected; convert the user input string to uppercase before splitting and matching (i.e., normalize runesString to uppercase), then perform the same .split("").map(...) using Object.values(Rune).find(rune => rune.startsWith(char)) and keep the existing undefined check and cast to Rune[]. ``` </details>

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

LunarTides/Hearthstone.js!418

				`@ -117,12 +118,27 @@ async function common(): Promise<BlueprintWithOptional> {`
				`.map((k) => game.lodash.startCase(k) as Keyword);`

Rows
Columns