Begin adding support for multiple abilities in card creator #416
No reviewers
Labels
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
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
LunarTides/Hearthstone.js!416
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "cc-multiple-abilities"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #191
Summary by CodeRabbit
Walkthrough
The card creator now returns and processes multiple abilities per card:
getCardAbilities(blueprint)returnsAbility[]. Parsing for Enchantment, Minion, Weapon, and other types extracts arrays of abilities; create flow, imports, and editor behavior were updated to assemble per-ability code blocks and open the editor only when abilities exist.Changes
tools/cardcreator/lib.tsgetCardAbility→getCardAbilities, changed return typestring→Ability[], addedAbilityimport from@Game/types.ts, and updated function signature and exports.tools/cardcreator/lib.tsEnchantmentSetup,EnchantmentApply,EnchantmentRemove); Minion/Weapon description parsing uses an improved regex to extract multiple abilities or fall back toAbility.Passive; other type cases (Spell/Battlecry/Use/HeroPower) return single-element arrays.tools/cardcreator/lib.tsabilitiesTexts, assembles create/ability blocks per ability, conditionally emits blocks, cleans description/keywords, and aggregates dynamicimports(e.g.,CardTag,Event,MinionTribe,SpellSchool,EnchantmentPriority). Editor auto-open now triggers onabilities.length > 0.tools/cardcreator/lib.tsSequence Diagram(s)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches
✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
cc-multiple-abilitiesThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
Comment
@coderabbitai helpto get the list of available commands and usage tips.@ -19,49 +20,65 @@ export enum CCType {* Returns the ability of a card based on its type.** @param blueprint The blueprint of the cardIt might honestly just be worth it to use a for loop atp
Actionable comments posted: 2
🧹 Nitpick comments (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
7c86f59cb2and642ddbee30.📒 Files selected for processing (1)
tools/cardcreator/lib.ts(7 hunks)🧰 Additional context used
🧬 Code graph analysis (1)
tools/cardcreator/lib.ts (6)
🔇 Additional comments (11)
@ -19,49 +20,65 @@ export enum CCType {* Returns the ability of a card based on its type.⚠️ Potential issue | 🟡 Minor
Fix typo:
foundAbilites→foundAbilities.The variable name has a spelling error.
And update the reference on line 62:
And line 64:
🤖 Prompt for AI Agents
✅ 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 theAbilityenum. 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:
🤖 Prompt for AI Agents
✅ Addressed in commit
438bf3eActionable comments posted: 2
🧹 Nitpick comments (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
642ddbee30and438bf3eba7.📒 Files selected for processing (1)
tools/cardcreator/lib.ts(7 hunks)🧰 Additional context used
🧬 Code graph analysis (1)
tools/cardcreator/lib.ts (5)
🔇 Additional comments (2)
@ -211,3 +200,4 @@keywords += `\t\tself.addKeyword(Keyword.${keyword.replaceAll(" ", "_")});\n`;}// Remove the last newline.⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
Verify that all card types should have a
createmethod.The code generates a
createability 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 acreatemethod. Thecreatemethod appears to be used for keyword/rune initialization, but this might not be appropriate for all card types, especially Enchantments which useenchantmentSetupinstead.🏁 Script executed:
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:
Length of output: 0
Length of output: 1902
Length of output: 5193
Length of output: 2170
Conditionally add
createmethod based on card type; Enchantments should not havecreate.The code unconditionally generates a
createmethod for any card with text (lines 206-211), but Enchantment-type cards should not have acreatemethod at all. Enchantments useenchantmentSetup/enchantmentApply/enchantmentRemoveinstead, as seen incards/Examples/Test/enchantment.ts. The generated code would incorrectly produce both acreatemethod and enchantment-specific methods for the same card.Add a type check before adding the
createmethod to exclude Enchantments (and any other card types that don't use it).🤖 Prompt for AI Agents
🛠️ Refactor suggestion | 🟠 Major
Avoid mutating the input blueprint parameter.
Setting
blueprint.runesandblueprint.keywordstoundefinedmutates the input parameter, which can be surprising to callers if they reuse the blueprint object. Consider either:Apply this diff to document the side effect:
🤖 Prompt for AI Agents
✅ Addressed in commit
cad2f68Actionable 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
438bf3eba7andcad2f686c8.📒 Files selected for processing (1)
tools/cardcreator/lib.ts(6 hunks)🧰 Additional context used
🧬 Code graph analysis (1)
tools/cardcreator/lib.ts (5)
🔇 Additional comments (10)
@ -401,3 +415,1 @@case Type.Undefined:typeImport = "";break;if (usesTags) {⚠️ Potential issue | 🔴 Critical
Move test method outside the abilities loop.
The
testmethod 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:
🤖 Prompt for AI Agents