LunarTides/Hearthstone.js
1
1
Fork 0
Code Issues 44 Pull requests 1 Projects Releases 19 Packages Wiki Activity Actions

Begin adding support for multiple abilities in card creator #416

Merged
LunarTides merged 4 commits from cc-multiple-abilities into main 2025-11-11 13:10:26 +00:00
Conversation 4 Commits 4 Files changed 1 +123 -108
LunarTides commented 2025-11-11 11:39:42 +00:00 (Migrated from github.com)
Copy link
  • Fix bugs.

Closes #191

Summary by CodeRabbit

  • Bug Fixes
    • Improved ability detection and description parsing, reducing missed or incorrect abilities and extracting multiple abilities reliably.
  • Refactor
    • Cards now support multiple abilities per card; creation flow and editor now assemble per-ability content and only open when abilities exist.
  • Chores
    • Normalized keywords/text cleaning, minor formatting and import aggregation for more consistent outputs.
- [x] Fix bugs. Closes #191 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved ability detection and description parsing, reducing missed or incorrect abilities and extracting multiple abilities reliably. * **Refactor** * Cards now support multiple abilities per card; creation flow and editor now assemble per-ability content and only open when abilities exist. * **Chores** * Normalized keywords/text cleaning, minor formatting and import aggregation for more consistent outputs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
LunarTides (Migrated from github.com) reviewed 2025-11-11 12:08:15 +00:00
tools/cardcreator/lib.ts
@ -19,49 +20,65 @@ export enum CCType {
* Returns the ability of a card based on its type.
*
* @param blueprint The blueprint of the card
LunarTides (Migrated from github.com) commented 2025-11-11 12:06:19 +00:00
Copy link
 * @returns The abilities of the card.
```suggestion * @returns The abilities of the card. ```
LunarTides (Migrated from github.com) commented 2025-11-11 12:07:13 +00:00
Copy link

It might honestly just be worth it to use a for loop atp

It might honestly just be worth it to use a for loop atp
tools/cardcreator/lib.ts
LunarTides (Migrated from github.com) commented 2025-11-11 12:08:11 +00:00
Copy link
	if (abilities.length > 0 && !debugMode) {
```suggestion if (abilities.length > 0 && !debugMode) { ```
coderabbitai[bot] (Migrated from github.com) reviewed 2025-11-11 12:43:14 +00:00
coderabbitai[bot] (Migrated from github.com) left a comment
Copy link

Actionable comments posted: 2

🧹 Nitpick comments (1)
tools/cardcreator/lib.ts (1)

64-71: Consider simplifying the transformation logic.

The chained .replace() calls could be clearer. As noted in previous review comments, a for loop might improve readability.

-return foundAbilities.map((s) =>
-	s
-		.replace(/:$/, "")
-		.replace(/^\. /, "")
-		.replace(/^<.*?>/, "")
-		.toLowerCase(),
-) as Ability[];
+const abilities: Ability[] = [];
+for (const match of foundAbilities) {
+	let cleaned = match
+		.replace(/:$/, "")      // Remove trailing colon
+		.replace(/^\. /, "")    // Remove leading ". "
+		.replace(/^<.*?>/, "") // Remove leading HTML tags
+		.toLowerCase();
+	
+	// Validate it's a real Ability enum value
+	if (Object.values(Ability).includes(cleaned as Ability)) {
+		abilities.push(cleaned as Ability);
+	}
+}
+return abilities.length > 0 ? abilities : [Ability.Passive];
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c86f59cb2 and 642ddbee30.

📒 Files selected for processing (1)
  • tools/cardcreator/lib.ts (7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/cardcreator/lib.ts (6)
cards/Examples/1/5-battlecry.ts (1)
  • blueprint (13-66)
cards/Examples/Test/enchantment.ts (1)
  • blueprint (12-38)
cards/Examples/4/1-passive.ts (1)
  • blueprint (15-91)
src/types/card.ts (1)
  • BlueprintWithOptional (410-413)
src/card.ts (2)
  • doBlueprint (412-450)
  • i (438-444)
cardcreator/lib.ts (1)
  • create (80-191)
🔇 Additional comments (11)
tools/cardcreator/lib.ts (11)

2-2: LGTM: Function signature updated to support multiple abilities.

The import addition, JSDoc update, and function rename from getCardAbility to getCardAbilities with Ability[] return type correctly implement the PR's goal.

Also applies to: 23-25

28-49: LGTM: Type-specific abilities correctly converted to arrays.

The ability returns for Spell, Hero, Location, HeroPower, and Enchantment types are properly structured as arrays, with Enchantment correctly returning all three lifecycle methods.

178-204: LGTM: Ability extraction and create method generation.

The refactor to collect multiple abilities in an array and generate a create method for cards with text is well-structured.

206-238: LGTM: Ability method generation with appropriate signatures.

The loop correctly generates different method signatures for Passive (with event parameters), Enchantment (with host), and standard abilities. The "ChangeMe" placeholder on line 212 is intentional for developers to customize.

240-243: LGTM: Test method stub generation.

Including a test method stub in generated cards promotes good testing practices.

245-270: LGTM: Dynamic import generation.

Conditionally adding imports based on abilities and card type keeps generated code clean and avoids unused imports.

272-273: LGTM: Clearing processed properties.

Setting runes and keywords to undefined prevents duplication since they're handled separately in the create method.

404-406: LGTM: Conditional CardTag import.

Adding CardTag import only when tags are used maintains clean generated code.

412-424: LGTM: Template content assembly.

The template structure cleanly combines dynamic imports and multiple ability methods with proper formatting.

484-484: LGTM: Editor opening condition updated.

The condition correctly checks abilities.length > 0 && !debugMode, addressing the past review comment.

160-162: Note: TODO for keyword-based ability detection.

The TODO on line 160 references issue #277 for future enhancement to detect keywords and avoid redundant passive abilities. This is documented as a known limitation.

**Actionable comments posted: 2** <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> `64-71`: **Consider simplifying the transformation logic.** The chained `.replace()` calls could be clearer. As noted in previous review comments, a for loop might improve readability. ```diff -return foundAbilities.map((s) => - s - .replace(/:$/, "") - .replace(/^\. /, "") - .replace(/^<.*?>/, "") - .toLowerCase(), -) as Ability[]; +const abilities: Ability[] = []; +for (const match of foundAbilities) { + let cleaned = match + .replace(/:$/, "") // Remove trailing colon + .replace(/^\. /, "") // Remove leading ". " + .replace(/^<.*?>/, "") // Remove leading HTML tags + .toLowerCase(); + + // Validate it's a real Ability enum value + if (Object.values(Ability).includes(cleaned as Ability)) { + abilities.push(cleaned as Ability); + } +} +return abilities.length > 0 ? abilities : [Ability.Passive]; ``` </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 7c86f59cb25146ac51dcf1431eb679b9d0518454 and 642ddbee3033020204cdac1af0daed82c80ef321. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `tools/cardcreator/lib.ts` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tools/cardcreator/lib.ts (6)</summary><blockquote> <details> <summary>cards/Examples/1/5-battlecry.ts (1)</summary> * `blueprint` (13-66) </details> <details> <summary>cards/Examples/Test/enchantment.ts (1)</summary> * `blueprint` (12-38) </details> <details> <summary>cards/Examples/4/1-passive.ts (1)</summary> * `blueprint` (15-91) </details> <details> <summary>src/types/card.ts (1)</summary> * `BlueprintWithOptional` (410-413) </details> <details> <summary>src/card.ts (2)</summary> * `doBlueprint` (412-450) * `i` (438-444) </details> <details> <summary>cardcreator/lib.ts (1)</summary> * `create` (80-191) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (11)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (11)</summary><blockquote> `2-2`: **LGTM: Function signature updated to support multiple abilities.** The import addition, JSDoc update, and function rename from `getCardAbility` to `getCardAbilities` with `Ability[]` return type correctly implement the PR's goal. Also applies to: 23-25 --- `28-49`: **LGTM: Type-specific abilities correctly converted to arrays.** The ability returns for Spell, Hero, Location, HeroPower, and Enchantment types are properly structured as arrays, with Enchantment correctly returning all three lifecycle methods. --- `178-204`: **LGTM: Ability extraction and create method generation.** The refactor to collect multiple abilities in an array and generate a create method for cards with text is well-structured. --- `206-238`: **LGTM: Ability method generation with appropriate signatures.** The loop correctly generates different method signatures for Passive (with event parameters), Enchantment (with host), and standard abilities. The "ChangeMe" placeholder on line 212 is intentional for developers to customize. --- `240-243`: **LGTM: Test method stub generation.** Including a test method stub in generated cards promotes good testing practices. --- `245-270`: **LGTM: Dynamic import generation.** Conditionally adding imports based on abilities and card type keeps generated code clean and avoids unused imports. --- `272-273`: **LGTM: Clearing processed properties.** Setting `runes` and `keywords` to undefined prevents duplication since they're handled separately in the create method. --- `404-406`: **LGTM: Conditional CardTag import.** Adding CardTag import only when tags are used maintains clean generated code. --- `412-424`: **LGTM: Template content assembly.** The template structure cleanly combines dynamic imports and multiple ability methods with proper formatting. --- `484-484`: **LGTM: Editor opening condition updated.** The condition correctly checks `abilities.length > 0 && !debugMode`, addressing the past review comment. --- `160-162`: **Note: TODO for keyword-based ability detection.** The TODO on line 160 references issue #277 for future enhancement to detect keywords and avoid redundant passive abilities. This is documented as a known limitation. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tools/cardcreator/lib.ts
@ -19,49 +20,65 @@ export enum CCType {
* Returns the ability of a card based on its type.
coderabbitai[bot] (Migrated from github.com) commented 2025-11-11 12:43:14 +00:00
Copy link

⚠️ Potential issue | 🟡 Minor

Fix typo: foundAbilites → foundAbilities.

The variable name has a spelling error.

-const foundAbilites = blueprint.text.match(reg);
+const foundAbilities = blueprint.text.match(reg);

And update the reference on line 62:

-if (foundAbilites) {
+if (foundAbilities) {

And line 64:

-return foundAbilites.map((s) =>
+return foundAbilities.map((s) =>

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In tools/cardcreator/lib.ts around lines 55, 62 and 64, rename the misspelled
variable foundAbilites to foundAbilities and update every reference to it on the
indicated lines (and any other occurrences in the file) so the identifier is
consistent; ensure you only change the variable name (preserve matching logic
and scope) and run a quick build/type-check to confirm no remaining references
to the old name remain.

✅ Addressed in commit 438bf3e

_⚠️ Potential issue_ | _🟡 Minor_ **Fix typo: `foundAbilites` → `foundAbilities`.** The variable name has a spelling error. ```diff -const foundAbilites = blueprint.text.match(reg); +const foundAbilities = blueprint.text.match(reg); ``` And update the reference on line 62: ```diff -if (foundAbilites) { +if (foundAbilities) { ``` And line 64: ```diff -return foundAbilites.map((s) => +return foundAbilities.map((s) => ``` > Committable suggestion skipped: line range outside the PR's diff. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/lib.ts around lines 55, 62 and 64, rename the misspelled variable foundAbilites to foundAbilities and update every reference to it on the indicated lines (and any other occurrences in the file) so the identifier is consistent; ensure you only change the variable name (preserve matching logic and scope) and run a quick build/type-check to confirm no remaining references to the old name remain. ``` </details> <!-- This is an auto-generated comment by CodeRabbit --> ✅ Addressed in commit 438bf3e
coderabbitai[bot] (Migrated from github.com) commented 2025-11-11 12:43:14 +00:00
Copy link

⚠️ Potential issue | 🟠 Major

Add validation for extracted ability names.

The type assertion as Ability[] on line 70 is unsafe—extracted strings are not validated against the Ability enum. If card text contains non-standard ability names (e.g., "Some Thing:"), they'll be treated as valid abilities without validation, potentially causing issues in code generation.

Consider validating extracted abilities:

 if (foundAbilities) {
-	return foundAbilities.map((s) =>
-		s
-			.replace(/:$/, "")
-			.replace(/^\. /, "")
-			.replace(/^<.*?>/, "")
-			.toLowerCase(),
-	) as Ability[];
+	const extracted = foundAbilities.map((s) =>
+		s
+			.replace(/:$/, "")
+			.replace(/^\. /, "")
+			.replace(/^<.*?>/, "")
+			.toLowerCase()
+	);
+	
+	// Filter to only valid Ability enum values
+	const validAbilities = extracted.filter((ability) =>
+		Object.values(Ability).includes(ability as Ability)
+	) as Ability[];
+	
+	return validAbilities.length > 0 ? validAbilities : [Ability.Passive];
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In tools/cardcreator/lib.ts around lines 64 to 71, the code blindly casts
extracted strings to Ability[] which is unsafe; update the extraction to
validate each normalized string against the Ability enum (or a lookup set of
valid ability names) and only include those that match, optionally logging or
throwing for unknowns; implement a map that normalizes the string, checks
membership in the Ability enum (using a reverse lookup or Set), filters out
invalid entries, and then returns the validated list typed as Ability[] so the
cast is no longer needed.

✅ Addressed in commit 438bf3e

_⚠️ Potential issue_ | _🟠 Major_ **Add validation for extracted ability names.** The type assertion `as Ability[]` on line 70 is unsafe—extracted strings are not validated against the `Ability` enum. If card text contains non-standard ability names (e.g., "Some Thing:"), they'll be treated as valid abilities without validation, potentially causing issues in code generation. Consider validating extracted abilities: ```diff if (foundAbilities) { - return foundAbilities.map((s) => - s - .replace(/:$/, "") - .replace(/^\. /, "") - .replace(/^<.*?>/, "") - .toLowerCase(), - ) as Ability[]; + const extracted = foundAbilities.map((s) => + s + .replace(/:$/, "") + .replace(/^\. /, "") + .replace(/^<.*?>/, "") + .toLowerCase() + ); + + // Filter to only valid Ability enum values + const validAbilities = extracted.filter((ability) => + Object.values(Ability).includes(ability as Ability) + ) as Ability[]; + + return validAbilities.length > 0 ? validAbilities : [Ability.Passive]; } ``` > Committable suggestion skipped: line range outside the PR's diff. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/lib.ts around lines 64 to 71, the code blindly casts extracted strings to Ability[] which is unsafe; update the extraction to validate each normalized string against the Ability enum (or a lookup set of valid ability names) and only include those that match, optionally logging or throwing for unknowns; implement a map that normalizes the string, checks membership in the Ability enum (using a reverse lookup or Set), filters out invalid entries, and then returns the validated list typed as Ability[] so the cast is no longer needed. ``` </details> <!-- This is an auto-generated comment by CodeRabbit --> ✅ Addressed in commit 438bf3e
coderabbitai[bot] (Migrated from github.com) reviewed 2025-11-11 12:55:42 +00:00
coderabbitai[bot] (Migrated from github.com) left a comment
Copy link

Actionable comments posted: 2

🧹 Nitpick comments (1)
tools/cardcreator/lib.ts (1)

213-224: Consider a more descriptive placeholder for Passive abilities.

The placeholder Event.ChangeMe on line 219 may not be immediately clear to developers. Consider using a more descriptive placeholder or adding a comment with common examples (e.g., Event.PlayCard, Event.TakeDamage). This would improve the developer experience when editing generated cards.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 642ddbee30 and 438bf3eba7.

📒 Files selected for processing (1)
  • tools/cardcreator/lib.ts (7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/cardcreator/lib.ts (5)
cards/Classes/Neutral/Collectible/Minions/1-Cost/110-chaotic-tendril.ts (1)
  • blueprint (14-48)
cards/Examples/Test/enchantment.ts (1)
  • blueprint (12-38)
cards/Examples/4/1-passive.ts (1)
  • blueprint (15-91)
src/types/card.ts (1)
  • BlueprintWithOptional (410-413)
src/card.ts (4)
  • i (438-444)
  • doBlueprint (412-450)
  • func (877-907)
  • activate (865-910)
🔇 Additional comments (2)
tools/cardcreator/lib.ts (2)

185-186: Good refactor to support multiple abilities.

The transition from single ability to multiple abilities is well-implemented. The getCardAbilities function now correctly returns an array, and the create function properly iterates through all abilities to generate the appropriate method stubs.

62-77: Fallback to Passive for unrecognized ability patterns is intentional, existing behavior—verify if this design is acceptable for your data quality standards.

The concern in the original comment is valid: when card text matches the ability pattern (e.g., "SomeThing: description") but the extracted ability isn't recognized, the code silently defaults to [Ability.Passive] rather than logging or returning an empty array.

However, this behavior appears intentional: Ability.Passive is used as a safe fallback in two scenarios (lines 77 and 81), and special handling for Passive elsewhere in the codebase (lines 214, 257) confirms this is by design. The assumption is that cards with descriptive text should always have some ability, and Passive is the safe default for unmatched patterns.

Whether this silent fallback is appropriate depends on your project's data quality expectations. If unrecognized ability patterns should trigger a warning or if such cards should be rejected with an empty array, that would require an intentional refactor.

**Actionable comments posted: 2** <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> `213-224`: **Consider a more descriptive placeholder for Passive abilities.** The placeholder `Event.ChangeMe` on line 219 may not be immediately clear to developers. Consider using a more descriptive placeholder or adding a comment with common examples (e.g., `Event.PlayCard`, `Event.TakeDamage`). This would improve the developer experience when editing generated cards. </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 642ddbee3033020204cdac1af0daed82c80ef321 and 438bf3eba711460e703a93a62b6dad6abd5047ef. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `tools/cardcreator/lib.ts` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tools/cardcreator/lib.ts (5)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Collectible/Minions/1-Cost/110-chaotic-tendril.ts (1)</summary> * `blueprint` (14-48) </details> <details> <summary>cards/Examples/Test/enchantment.ts (1)</summary> * `blueprint` (12-38) </details> <details> <summary>cards/Examples/4/1-passive.ts (1)</summary> * `blueprint` (15-91) </details> <details> <summary>src/types/card.ts (1)</summary> * `BlueprintWithOptional` (410-413) </details> <details> <summary>src/card.ts (4)</summary> * `i` (438-444) * `doBlueprint` (412-450) * `func` (877-907) * `activate` (865-910) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (2)</summary><blockquote> `185-186`: **Good refactor to support multiple abilities.** The transition from single ability to multiple abilities is well-implemented. The `getCardAbilities` function now correctly returns an array, and the `create` function properly iterates through all abilities to generate the appropriate method stubs. --- `62-77`: **Fallback to Passive for unrecognized ability patterns is intentional, existing behavior—verify if this design is acceptable for your data quality standards.** The concern in the original comment is valid: when card text matches the ability pattern (e.g., "SomeThing: description") but the extracted ability isn't recognized, the code silently defaults to `[Ability.Passive]` rather than logging or returning an empty array. However, this behavior appears intentional: `Ability.Passive` is used as a safe fallback in two scenarios (lines 77 and 81), and special handling for Passive elsewhere in the codebase (lines 214, 257) confirms this is by design. The assumption is that cards with descriptive text should always have some ability, and Passive is the safe default for unmatched patterns. Whether this silent fallback is appropriate depends on your project's data quality expectations. If unrecognized ability patterns should trigger a warning or if such cards should be rejected with an empty array, that would require an intentional refactor. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tools/cardcreator/lib.ts
@ -211,3 +200,4 @@
keywords += `\t\tself.addKeyword(Keyword.${keyword.replaceAll(" ", "_")});\n`;
}
// Remove the last newline.
coderabbitai[bot] (Migrated from github.com) commented 2025-11-11 12:55:41 +00:00
Copy link

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify that all card types should have a create method.

The code generates a create ability for any card with text, but examining the example cards in the codebase (e.g., cards/Examples/Test/enchantment.ts, cards/Examples/4/1-passive.ts), none of them include a create method. The create method appears to be used for keyword/rune initialization, but this might not be appropriate for all card types, especially Enchantments which use enchantmentSetup instead.

🏁 Script executed:

#!/bin/bash
# Check existing cards for create methods to verify if they're actually used
ast-grep --pattern $'create($_, $_) {
  $$$
}'

# Also check if there are cards that use create vs those that don't
rg -nP --type=ts 'async create\(' cards/

Length of output: 2192

Let me check the specific example files mentioned in the review comment:

Let me check the specific files mentioned in the review:

🏁 Script executed:

#!/bin/bash
# Check if the specific example files exist and inspect them
find cards/ -name "enchantment.ts" -o -name "1-passive.ts" 2>/dev/null

# Also check what's in the Examples directory structure
ls -la cards/Examples/Test/ 2>/dev/null | head -20
ls -la cards/Examples/ 2>/dev/null | head -20

Length of output: 0


---

🏁 Script executed:

```shell
#!/bin/bash
# Check repository structure and find the files
pwd
ls -la | head -20

# Try to find the example files with simpler approach
find . -type f -name "enchantment.ts" 2>/dev/null
find . -type f -name "*passive*" 2>/dev/null

# List what's available in Examples
find . -path "*/Examples/*" -type f -name "*.ts" 2>/dev/null | head -20

Length of output: 1902


---

🏁 Script executed:

```shell
#!/bin/bash
# Check the contents of the specific files mentioned in the review
echo "=== enchantment.ts ==="
cat ./cards/Examples/Test/enchantment.ts

echo -e "\n=== 1-passive.ts ==="
cat ./cards/Examples/4/1-passive.ts

# Also check a file we know has create method for comparison
echo -e "\n=== Example with create method (divine-shield.ts) ==="
cat ./cards/Examples/Test/divine-shield.ts

Length of output: 5193


---

🏁 Script executed:

```shell
#!/bin/bash
# Check the context of lines 206-211 in tools/cardcreator/lib.ts
echo "=== Context around lines 206-211 in lib.ts ==="
sed -n '180,250p' tools/cardcreator/lib.ts

# Also check if there's any conditional logic based on card type
echo -e "\n=== Searching for type-based conditions ==="
rg -n "Type\." tools/cardcreator/lib.ts | head -20

Length of output: 2170

Conditionally add create method based on card type; Enchantments should not have create.

The code unconditionally generates a create method for any card with text (lines 206-211), but Enchantment-type cards should not have a create method at all. Enchantments use enchantmentSetup/enchantmentApply/enchantmentRemove instead, as seen in cards/Examples/Test/enchantment.ts. The generated code would incorrectly produce both a create method and enchantment-specific methods for the same card.

Add a type check before adding the create method to exclude Enchantments (and any other card types that don't use it).

🤖 Prompt for AI Agents 
In tools/cardcreator/lib.ts around lines 206 to 211, the generator always
appends a `create` method for cards with text, which incorrectly adds `create`
to Enchantment cards; update the conditional to only push the `create` method
when the card's type is one that uses it (e.g., ensure blueprint.type !==
'Enchantment' or check against a whitelist of types that require `create`); keep
the existing text check and only add the extra type check so enchantment cards
will instead rely on their enchantmentSetup/enchantmentApply/enchantmentRemove
methods.
_⚠️ Potential issue_ | _🔴 Critical_ <details> <summary>🧩 Analysis chain</summary> **Verify that all card types should have a `create` method.** The code generates a `create` ability for any card with text, but examining the example cards in the codebase (e.g., `cards/Examples/Test/enchantment.ts`, `cards/Examples/4/1-passive.ts`), none of them include a `create` method. The `create` method appears to be used for keyword/rune initialization, but this might not be appropriate for all card types, especially Enchantments which use `enchantmentSetup` instead. --- 🏁 Script executed: ```shell #!/bin/bash # Check existing cards for create methods to verify if they're actually used ast-grep --pattern $'create($_, $_) { $$$ }' # Also check if there are cards that use create vs those that don't rg -nP --type=ts 'async create\(' cards/ ``` Length of output: 2192 --- Let me check the specific example files mentioned in the review comment: Let me check the specific files mentioned in the review: --- 🏁 Script executed: ```shell #!/bin/bash # Check if the specific example files exist and inspect them find cards/ -name "enchantment.ts" -o -name "1-passive.ts" 2>/dev/null # Also check what's in the Examples directory structure ls -la cards/Examples/Test/ 2>/dev/null | head -20 ls -la cards/Examples/ 2>/dev/null | head -20 ``` Length of output: 0 --- ``` --- 🏁 Script executed: ```shell #!/bin/bash # Check repository structure and find the files pwd ls -la | head -20 # Try to find the example files with simpler approach find . -type f -name "enchantment.ts" 2>/dev/null find . -type f -name "*passive*" 2>/dev/null # List what's available in Examples find . -path "*/Examples/*" -type f -name "*.ts" 2>/dev/null | head -20 ``` Length of output: 1902 --- ``` --- 🏁 Script executed: ```shell #!/bin/bash # Check the contents of the specific files mentioned in the review echo "=== enchantment.ts ===" cat ./cards/Examples/Test/enchantment.ts echo -e "\n=== 1-passive.ts ===" cat ./cards/Examples/4/1-passive.ts # Also check a file we know has create method for comparison echo -e "\n=== Example with create method (divine-shield.ts) ===" cat ./cards/Examples/Test/divine-shield.ts ``` Length of output: 5193 --- ``` --- 🏁 Script executed: ```shell #!/bin/bash # Check the context of lines 206-211 in tools/cardcreator/lib.ts echo "=== Context around lines 206-211 in lib.ts ===" sed -n '180,250p' tools/cardcreator/lib.ts # Also check if there's any conditional logic based on card type echo -e "\n=== Searching for type-based conditions ===" rg -n "Type\." tools/cardcreator/lib.ts | head -20 ``` Length of output: 2170 --- </details> **Conditionally add `create` method based on card type; Enchantments should not have `create`.** The code unconditionally generates a `create` method for any card with text (lines 206-211), but Enchantment-type cards should not have a `create` method at all. Enchantments use `enchantmentSetup`/`enchantmentApply`/`enchantmentRemove` instead, as seen in `cards/Examples/Test/enchantment.ts`. The generated code would incorrectly produce both a `create` method and enchantment-specific methods for the same card. Add a type check before adding the `create` method to exclude Enchantments (and any other card types that don't use it). <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/lib.ts around lines 206 to 211, the generator always appends a `create` method for cards with text, which incorrectly adds `create` to Enchantment cards; update the conditional to only push the `create` method when the card's type is one that uses it (e.g., ensure blueprint.type !== 'Enchantment' or check against a whitelist of types that require `create`); keep the existing text check and only add the extra type check so enchantment cards will instead rely on their enchantmentSetup/enchantmentApply/enchantmentRemove methods. ``` </details> <!-- This is an auto-generated comment by CodeRabbit -->
tools/cardcreator/lib.ts
coderabbitai[bot] (Migrated from github.com) commented 2025-11-11 12:55:41 +00:00
Copy link

🛠️ Refactor suggestion | 🟠 Major

Avoid mutating the input blueprint parameter.

Setting blueprint.runes and blueprint.keywords to undefined mutates the input parameter, which can be surprising to callers if they reuse the blueprint object. Consider either:

  1. Documenting this side effect in the function's JSDoc comment, or
  2. Creating a shallow copy of the blueprint before mutation, or
  3. Using destructuring to extract only the needed properties without mutation

Apply this diff to document the side effect:

 /**
  * Generates a new card based on the provided arguments and saves it to a file.
  *
  * @param creatorType The type of card creator.
- * @param blueprint The blueprint for the card.
+ * @param blueprint The blueprint for the card. Note: runes and keywords properties will be cleared.
  * @param overridePath The override path for the card.
  * @param overrideFilename The override filename for the card.
  * @param debug If true, doesn't save the card, just prints out details about it.
  *
  * @returns The path of the created file.
  */

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In tools/cardcreator/lib.ts around lines 279-280, the code currently sets
blueprint.runes and blueprint.keywords to undefined which mutates the input
object; fix this by creating a shallow copy of the blueprint before mutating
(e.g., const bp = { ...blueprint } and then set bp.runes = undefined and
bp.keywords = undefined and use/return bp), or if mutation is intentional,
document the side effect in the function JSDoc describing that the input
blueprint will be modified; choose one of these two approaches and apply it
consistently where the mutated blueprint is consumed.

✅ Addressed in commit cad2f68

_🛠️ Refactor suggestion_ | _🟠 Major_ **Avoid mutating the input blueprint parameter.** Setting `blueprint.runes` and `blueprint.keywords` to `undefined` mutates the input parameter, which can be surprising to callers if they reuse the blueprint object. Consider either: 1. Documenting this side effect in the function's JSDoc comment, or 2. Creating a shallow copy of the blueprint before mutation, or 3. Using destructuring to extract only the needed properties without mutation Apply this diff to document the side effect: ```diff /** * Generates a new card based on the provided arguments and saves it to a file. * * @param creatorType The type of card creator. - * @param blueprint The blueprint for the card. + * @param blueprint The blueprint for the card. Note: runes and keywords properties will be cleared. * @param overridePath The override path for the card. * @param overrideFilename The override filename for the card. * @param debug If true, doesn't save the card, just prints out details about it. * * @returns The path of the created file. */ ``` > Committable suggestion skipped: line range outside the PR's diff. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/lib.ts around lines 279-280, the code currently sets blueprint.runes and blueprint.keywords to undefined which mutates the input object; fix this by creating a shallow copy of the blueprint before mutating (e.g., const bp = { ...blueprint } and then set bp.runes = undefined and bp.keywords = undefined and use/return bp), or if mutation is intentional, document the side effect in the function JSDoc describing that the input blueprint will be modified; choose one of these two approaches and apply it consistently where the mutated blueprint is consumed. ``` </details> <!-- This is an auto-generated comment by CodeRabbit --> ✅ Addressed in commit cad2f68
coderabbitai[bot] (Migrated from github.com) reviewed 2025-11-11 13:09:14 +00:00
coderabbitai[bot] (Migrated from github.com) left a comment
Copy link

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 438bf3eba7 and cad2f686c8.

📒 Files selected for processing (1)
  • tools/cardcreator/lib.ts (6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/cardcreator/lib.ts (5)
cards/Examples/1/5-battlecry.ts (1)
  • blueprint (13-66)
cards/Examples/Test/enchantment.ts (1)
  • blueprint (12-38)
cards/Examples/4/1-passive.ts (1)
  • blueprint (15-91)
src/types/card.ts (1)
  • BlueprintWithOptional (410-413)
src/card.ts (4)
  • i (438-444)
  • doBlueprint (412-450)
  • func (877-907)
  • activate (865-910)
🔇 Additional comments (10)
tools/cardcreator/lib.ts (10)

1-6: LGTM!

The addition of Ability to the imports is correct and necessary to support the new Ability[] return type.

19-25: LGTM!

The function rename and signature change correctly reflect the transition to supporting multiple abilities per card.

26-49: LGTM!

The simple type cases correctly return ability arrays. The Enchantment case appropriately returns all three enchantment-related abilities.

170-170: LGTM!

Cloning the blueprint before mutation addresses the past review concern about mutating the input parameter. A shallow clone is sufficient here since the properties are being set to undefined rather than mutated.

207-215: LGTM!

The conditional logic correctly excludes Enchantment types from having a create method, addressing the past review concern. Enchantments rely on their enchantmentSetup/enchantmentApply/enchantmentRemove methods instead.

256-281: LGTM!

The imports are built dynamically based on what's actually used in the generated card. The logic correctly adds Keyword, Event, and type-specific imports only when needed.

283-284: LGTM!

Clearing runes and keywords is safe since the blueprint was cloned on line 170. These properties have already been consumed and don't need to appear in the final stringified blueprint.

420-436: Content assembly looks correct, pending test method fix.

The template correctly joins imports and ability texts. Once the test method duplication issue (lines 251-254) is fixed, this assembly will produce the correct output.

495-499: LGTM!

The condition correctly checks for any abilities before opening the editor, consistent with the transition to multiple abilities per card.

51-82: The fallback to Passive is appropriate.

Script output confirms that Minion/Weapon cards without recognized ability keywords in the standard format ("Keyword:") primarily describe passive effects like "At the end of your turn..." These cards correctly default to [Ability.Passive]. Empty text cards are already filtered by the !blueprint.text check before reaching the fallback. The logic is sound.

**Actionable comments posted: 1** <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 438bf3eba711460e703a93a62b6dad6abd5047ef and cad2f686c83294e00559cc44bd7ce252e9adae89. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `tools/cardcreator/lib.ts` (6 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tools/cardcreator/lib.ts (5)</summary><blockquote> <details> <summary>cards/Examples/1/5-battlecry.ts (1)</summary> * `blueprint` (13-66) </details> <details> <summary>cards/Examples/Test/enchantment.ts (1)</summary> * `blueprint` (12-38) </details> <details> <summary>cards/Examples/4/1-passive.ts (1)</summary> * `blueprint` (15-91) </details> <details> <summary>src/types/card.ts (1)</summary> * `BlueprintWithOptional` (410-413) </details> <details> <summary>src/card.ts (4)</summary> * `i` (438-444) * `doBlueprint` (412-450) * `func` (877-907) * `activate` (865-910) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (10)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (10)</summary><blockquote> `1-6`: **LGTM!** The addition of `Ability` to the imports is correct and necessary to support the new `Ability[]` return type. --- `19-25`: **LGTM!** The function rename and signature change correctly reflect the transition to supporting multiple abilities per card. --- `26-49`: **LGTM!** The simple type cases correctly return ability arrays. The Enchantment case appropriately returns all three enchantment-related abilities. --- `170-170`: **LGTM!** Cloning the blueprint before mutation addresses the past review concern about mutating the input parameter. A shallow clone is sufficient here since the properties are being set to `undefined` rather than mutated. --- `207-215`: **LGTM!** The conditional logic correctly excludes Enchantment types from having a `create` method, addressing the past review concern. Enchantments rely on their `enchantmentSetup`/`enchantmentApply`/`enchantmentRemove` methods instead. --- `256-281`: **LGTM!** The imports are built dynamically based on what's actually used in the generated card. The logic correctly adds Keyword, Event, and type-specific imports only when needed. --- `283-284`: **LGTM!** Clearing `runes` and `keywords` is safe since the blueprint was cloned on line 170. These properties have already been consumed and don't need to appear in the final stringified blueprint. --- `420-436`: **Content assembly looks correct, pending test method fix.** The template correctly joins imports and ability texts. Once the test method duplication issue (lines 251-254) is fixed, this assembly will produce the correct output. --- `495-499`: **LGTM!** The condition correctly checks for any abilities before opening the editor, consistent with the transition to multiple abilities per card. --- `51-82`: **The fallback to Passive is appropriate.** Script output confirms that Minion/Weapon cards without recognized ability keywords in the standard format ("Keyword:") primarily describe passive effects like "At the end of your turn..." These cards correctly default to `[Ability.Passive]`. Empty text cards are already filtered by the `!blueprint.text` check before reaching the fallback. The logic is sound. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tools/cardcreator/lib.ts
@ -401,3 +415,1 @@
case Type.Undefined:
typeImport = "";
break;
if (usesTags) {
coderabbitai[bot] (Migrated from github.com) commented 2025-11-11 13:09:13 +00:00
Copy link

⚠️ Potential issue | 🔴 Critical

Move test method outside the abilities loop.

The test method is being added inside the loop (lines 251-254), which means it will be duplicated for each ability. For a card with 3 abilities, the test method will appear 3 times in the generated code, causing issues.

Apply this diff to move the test method outside the loop:

 	for (const ability of abilities) {
 		if (ability === Ability.Passive) {
 			abilitiesTexts.push(`async ${ability}(self, owner, key, value, eventPlayer) {
 		// ${cleanedDescription}
 
 		// Only proceed if the correct event key was broadcast
         if (!game.event.is(key, value, Event.ChangeMe)) {
             return;
         };
 	},`);
 			continue;
 		}
 
 		if (
 			[
 				Ability.EnchantmentSetup,
 				Ability.EnchantmentApply,
 				Ability.EnchantmentRemove,
 			].includes(ability)
 		) {
 			abilitiesTexts.push(`async ${ability}(self, owner, host) {
 		// ${cleanedDescription}
 
 	},`);
 
 			continue;
 		}
 
 		abilitiesTexts.push(`async ${ability}(self, owner) {
 		// ${cleanedDescription}
 
 	},`);
-	}
-
-	abilitiesTexts.push(`async test(self, owner) {
-		// Unit testing
-		return EventListenerMessage.Skip;
-	},`);
+	}
+
+	abilitiesTexts.push(`async test(self, owner) {
+		// Unit testing
+		return EventListenerMessage.Skip;
+	},`);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In tools/cardcreator/lib.ts around lines 217 to 254, the abilitiesTexts.push
call that adds the async test(self, owner) method is currently inside the
abilities loop and will be appended once per ability; move that
abilitiesTexts.push(...) call so it executes after the for (const ability of
abilities) { ... } loop finishes (i.e., outside the loop body) so the test
method is added exactly once to the generated output.
_⚠️ Potential issue_ | _🔴 Critical_ **Move test method outside the abilities loop.** The `test` method is being added inside the loop (lines 251-254), which means it will be duplicated for each ability. For a card with 3 abilities, the test method will appear 3 times in the generated code, causing issues. Apply this diff to move the test method outside the loop: ```diff for (const ability of abilities) { if (ability === Ability.Passive) { abilitiesTexts.push(`async ${ability}(self, owner, key, value, eventPlayer) { // ${cleanedDescription} // Only proceed if the correct event key was broadcast if (!game.event.is(key, value, Event.ChangeMe)) { return; }; },`); continue; } if ( [ Ability.EnchantmentSetup, Ability.EnchantmentApply, Ability.EnchantmentRemove, ].includes(ability) ) { abilitiesTexts.push(`async ${ability}(self, owner, host) { // ${cleanedDescription} },`); continue; } abilitiesTexts.push(`async ${ability}(self, owner) { // ${cleanedDescription} },`); - } - - abilitiesTexts.push(`async test(self, owner) { - // Unit testing - return EventListenerMessage.Skip; - },`); + } + + abilitiesTexts.push(`async test(self, owner) { + // Unit testing + return EventListenerMessage.Skip; + },`); ``` > Committable suggestion skipped: line range outside the PR's diff. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/lib.ts around lines 217 to 254, the abilitiesTexts.push call that adds the async test(self, owner) method is currently inside the abilities loop and will be appended once per ability; move that abilitiesTexts.push(...) call so it executes after the for (const ability of abilities) { ... } loop finishes (i.e., outside the loop body) so the test method is added exactly once to the generated output. ``` </details> <!-- This is an auto-generated comment by CodeRabbit -->
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
breaking change
The issue requires a breaking change.

  
duplicate
This issue or pull request already exists

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
scope: card
The issue is related to 1 or more cards.

  
scope: docs
The issue is related to the docs website.

  
scope: game
The issue is related to the game itself

  
scope: registry
The issue is related to the registry.

  
scope: script
The issue is related to a script (DEPRECATED)

  
scope: tool
The issue is related to a tool

  
scope: universe
The issue is related to the universe.

  
tracker
This issue tracks multiple similar issues.

  
type: bug
Something isn't working

  
type: card request
The issue requests adding one or more cards.

  
type: documentation
Improvements or additions to documentation

  
type: enhancement
New feature or request

  
type: improvement
Improvement to an already existing feature

  
type: parity
Get closer parity with vanilla.

  
wontfix
This will not be worked on

No labels
breaking change
duplicate
good first issue
help wanted
invalid
question
scope: card
scope: docs
scope: game
scope: registry
scope: script
scope: tool
scope: universe
tracker
type: bug
type: card request
type: documentation
type: enhancement
type: improvement
type: parity
wontfix
Milestone
Clear milestone
No items
No milestone
3.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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!416
No description provided.