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
LunarTides commented 2025-11-11 11:39:42 +00:00 (Migrated from github.com)
  • 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 -->
coderabbitai[bot] commented 2025-11-11 11:39:47 +00:00 (Migrated from github.com)

Walkthrough

The card creator now returns and processes multiple abilities per card: getCardAbilities(blueprint) returns Ability[]. 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

Cohort / File(s) Summary
Ability Array Refactor
tools/cardcreator/lib.ts
Renamed getCardAbilitygetCardAbilities, changed return type stringAbility[], added Ability import from @Game/types.ts, and updated function signature and exports.
Parsing & Extraction
tools/cardcreator/lib.ts
Enchantment now returns multiple abilities (EnchantmentSetup, EnchantmentApply, EnchantmentRemove); Minion/Weapon description parsing uses an improved regex to extract multiple abilities or fall back to Ability.Passive; other type cases (Spell/Battlecry/Use/HeroPower) return single-element arrays.
Content Generation & Imports
tools/cardcreator/lib.ts
Creation logic migrated to handle an abilities array: builds per-ability abilitiesTexts, assembles create/ability blocks per ability, conditionally emits blocks, cleans description/keywords, and aggregates dynamic imports (e.g., CardTag, Event, MinionTribe, SpellSchool, EnchantmentPriority). Editor auto-open now triggers on abilities.length > 0.
Flow & Callsite Simplification
tools/cardcreator/lib.ts
Replaced single-ability control flow with a loop over abilities, added handling for Passive and Enchantment-specific abilities, updated stringification/formatting and unit-test hook placeholder.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Creator as Card Creator
    participant Parser as getCardAbilities
    participant Generator as Content Generator

    User->>Creator: Submit card blueprint
    Creator->>Parser: getCardAbilities(blueprint) → Ability[]
    activate Parser
    Parser->>Parser: Detect card type & run regex(es)
    Parser-->>Creator: Return Ability[] (may include multiple or default Passive)
    deactivate Parser

    Creator->>Generator: For each ability → build code block
    activate Generator
    Generator->>Generator: Collect conditional imports
    Generator->>Generator: Assemble create/ability blocks per ability
    Generator-->>Creator: Return assembled code (concatenated)
    deactivate Generator

    Creator-->>User: Open editor if abilities.length > 0 and present code

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay special attention to:
    • Regex edge cases and coverage for ability extraction from freeform descriptions
    • Enchantment multi-ability ordering and correctness (Setup/Apply/Remove)
    • Dynamic import aggregation to ensure required symbols are imported only when used
    • Correct concatenation and formatting of per-ability code blocks to avoid duplicates

Poem

🐇
I parsed the lines where secrets hide,
Turned lone strings into a hopping tide.
Abilities sprout, in arrays they sing,
The card creator leaps—what joy to bring!

Pre-merge checks and finishing touches

Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title accurately summarizes the main change: replacing single-ability support with multi-ability support in the card creator tool.
Linked Issues check Passed The changes implement multi-ability support by converting single string returns to Ability arrays and enabling creation of multiple keyword methods per card [#191].
Out of Scope Changes check Passed All changes are scoped to enabling multiple abilities in the card creator; no unrelated modifications detected.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cc-multiple-abilities

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

<!-- This is an auto-generated comment: summarize by coderabbit.ai --> <!-- walkthrough_start --> ## Walkthrough The card creator now returns and processes multiple abilities per card: `getCardAbilities(blueprint)` returns `Ability[]`. 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 | Cohort / File(s) | Summary | |---|---| | **Ability Array Refactor** <br> `tools/cardcreator/lib.ts` | Renamed `getCardAbility` → `getCardAbilities`, changed return type `string` → `Ability[]`, added `Ability` import from `@Game/types.ts`, and updated function signature and exports. | | **Parsing & Extraction** <br> `tools/cardcreator/lib.ts` | Enchantment now returns multiple abilities (`EnchantmentSetup`, `EnchantmentApply`, `EnchantmentRemove`); Minion/Weapon description parsing uses an improved regex to extract multiple abilities or fall back to `Ability.Passive`; other type cases (Spell/Battlecry/Use/HeroPower) return single-element arrays. | | **Content Generation & Imports** <br> `tools/cardcreator/lib.ts` | Creation logic migrated to handle an abilities array: builds per-ability `abilitiesTexts`, assembles create/ability blocks per ability, conditionally emits blocks, cleans description/keywords, and aggregates dynamic `imports` (e.g., `CardTag`, `Event`, `MinionTribe`, `SpellSchool`, `EnchantmentPriority`). Editor auto-open now triggers on `abilities.length > 0`. | | **Flow & Callsite Simplification** <br> `tools/cardcreator/lib.ts` | Replaced single-ability control flow with a loop over abilities, added handling for Passive and Enchantment-specific abilities, updated stringification/formatting and unit-test hook placeholder. | ## Sequence Diagram(s) ```mermaid sequenceDiagram participant User participant Creator as Card Creator participant Parser as getCardAbilities participant Generator as Content Generator User->>Creator: Submit card blueprint Creator->>Parser: getCardAbilities(blueprint) → Ability[] activate Parser Parser->>Parser: Detect card type & run regex(es) Parser-->>Creator: Return Ability[] (may include multiple or default Passive) deactivate Parser Creator->>Generator: For each ability → build code block activate Generator Generator->>Generator: Collect conditional imports Generator->>Generator: Assemble create/ability blocks per ability Generator-->>Creator: Return assembled code (concatenated) deactivate Generator Creator-->>User: Open editor if abilities.length > 0 and present code ``` ## Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 minutes - Pay special attention to: - Regex edge cases and coverage for ability extraction from freeform descriptions - Enchantment multi-ability ordering and correctness (Setup/Apply/Remove) - Dynamic import aggregation to ensure required symbols are imported only when used - Correct concatenation and formatting of per-ability code blocks to avoid duplicates ## Poem > 🐇 > I parsed the lines where secrets hide, > Turned lone strings into a hopping tide. > Abilities sprout, in arrays they sing, > The card creator leaps—what joy to bring! <!-- walkthrough_end --> <!-- pre_merge_checks_walkthrough_start --> ## Pre-merge checks and finishing touches <details> <summary>✅ Passed checks (5 passed)</summary> | Check name | Status | Explanation | | :------------------------: | :------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | | Title check | ✅ Passed | The title accurately summarizes the main change: replacing single-ability support with multi-ability support in the card creator tool. | | Linked Issues check | ✅ Passed | The changes implement multi-ability support by converting single string returns to Ability arrays and enabling creation of multiple keyword methods per card [#191]. | | Out of Scope Changes check | ✅ Passed | All changes are scoped to enabling multiple abilities in the card creator; no unrelated modifications detected. | | Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. | </details> <!-- pre_merge_checks_walkthrough_end --> <!-- finishing_touch_checkbox_start --> <details> <summary>✨ Finishing touches</summary> - [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> 📝 Generate docstrings <details> <summary>🧪 Generate unit tests (beta)</summary> - [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests - [ ] <!-- {"checkboxId": "07f1e7d6-8a8e-4e23-9900-8731c2c87f58", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Post copyable unit tests in a comment - [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `cc-multiple-abilities` </details> </details> <!-- finishing_touch_checkbox_end --> <!-- tips_start --> --- Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=LunarTides/Hearthstone.js&utm_content=416)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> <sub>Comment `@coderabbitai help` to get the list of available commands and usage tips.</sub> <!-- tips_end --> <!-- internal state start --> <!-- DwQgtGAEAqAWCWBnSTIEMB26CuAXA9mAOYCmGJATmriQCaQDG+Ats2bgFyQAOFk+AIwBWJBrngA3EsgEBPRvlqU0AgfFwA6NPEgQAfACgjoCEYDEZyAAUASpETZWaCrKPR1AGxJcAQiSLwWGi0tIFE9tjc3PgUuJAAZjGQzNge4txe6Goe6vDSKFgMzvQMFCTUSZAGAHKOApRcACwAjABskFUAqjYAMlywuLjciBwA9KMBuLDYAhpMzKM92BjO7kqIowAS5bGwiATkGkIb3Kkeoy3tXYgNkEsrFGv5VQDK+NgUDCSQAlQYDLAuAwGGAUml4BkSGAVPAcuJ8oAkwhgzlIcV+mABXGY2iwr1w1GwI343DIHQMNhIEjyAHdKESABQYfDkSA5fZ0ACUZJ6KhIHiJuFkJK4ZFgGJIbAwuAANPYmMLIAR8B4yQBhMrUOjoTiQABMAAZdQBWMDNZqm5rQM0cADMAE4OI1dQAtIwAEWkpQh4mZHAMUAAYvAAB4/bBERAaf2QVWwUQAazZOoA2sGALqQIOhgThyPR1UefA3ZBIBzfMzNO3NcyWADywlE4ikyHiFBYrMC8a1pew0iMAElEGXIBWq5BACgEkAAgiEIlEYnFEnwwelMjC4XkS4ViowNQQKH6oABRFYCTJTb5FCglPdJAi78o0ZKpVffLuyakxehsKaKZAkvgr3oekHHnWJn3BSEslhXJ8mZdB7DCTIgI5KN/TAQwDBMKAyHofB4hwAhiDIZQaBKFhJR1Xh+AbMRJHyOQFCUKhVHULQdH0LDwCgOBUFQTBCMIUhyCoMiFFYdguCoakIicFwfnkJhmJUNRNG0XQMKMLjTAMJV+VGIDSkfGJRhyWZcBGAwACIbIMCxp37YiRM1egHDk+R8MYMUMFIRAjCgCkMjQL56AAsBBRJRCfMyfYKDCSAylwD4sAkNAPF7ZBqXUWAEOWeB4jyehnCoeREuSyB6SnbJ1FkZM0y5Kk0EgVFVWKKqYPhRBZUiWhqHitKVQiy80GLdBgS/JDZCjKAWra6rOsgJkZOY+jkBXCE13mzcuBeEkPHOHxqFwLxSlkUZOhuLZKHwKx8FpPgyooLBECQqE+Qldh0AoErEAAbkgE8AUwXBKISkgkqetaXw27511g5B6UB7yQfYF5wciWUkeByipyiDxZEx/5kcoilmHwKQOX+gBZQJ4GZUYAHVymiLBuGcG5IHWL1uB9LAsqmBC2xzfYwdIUN7xIYNcCoMQINfaCNzgvh4gGn5gvjRV8EgZN2rhKarBGl6pDTaaH01BJCxk5h4CIUStVbdsmpe6Lvlm69dZqnhqBy+9vNoHIfPQIItvyYq0CmyB+ylNtaGwEKFfh6BJYsoP6FoWQVmthgFClT7DYlM95AEEatXgpQaDELU4c62V30/a8uoS5ZpFlTB6CG/6GC8TAtS5uKebpoIMHoOuv0gP2A6If6epclBmGiWJkCYfbG0HzWCi77AlBjYpoDQIhMakKVZRpjBB+gOL6llXa+Q8F4AXwZVCaBqVKKsOKYk9kbFpIOgtWLm4eEsCEn3iQU2PR8ABGzjPMS9587MDPN8MK1dFJ7m+GefADB4yN2QfNeQlJPoTzpK3Ye68mDD1yMyAa+DrZxCMpqUYKCfiFiwcgZk+NIDUjjFgMoXhUpSg0JAQKHhgqlw8MxKKRAvDQjwV7AW/McpNULPgbg/ApB8GrpuWUgRN6hEDhPeKS5rCG3oqnAGRNsbsDALw2emjpD/WCEoegeVcDhWkHEWAj8NZBS+J48RlBTadG4L1WBydGDdzPj5WuJAPxj2xCsUglESH0DKGLMAACtTJxlrzde8B55tikGNBgHxgoeQIkwtmFBnbhAdswRgO4aBS1NjOIQhIxIFRWCqchNApToCHAXDh95yFFB6ebVW5dGxVxDogQAmATMMwdg5J68VGkgvJAOg6gkjsPkFw0kdjIxeB8gLPQkB9QVUCOyYI/BymSOkUwgECZULRlPkkJc2JBj9VIbFMI+V4AjLXsEVp+xKLICmG2cMni8AVXxAIDYiA2ZfEbqPa8i0YjYhyAALz6syJZeSF5xH3rbfw2KMBPKgEEkJWpThnj+WASW+LOaiBEaJQeXB4jLDovBF6RAVgQ0vN5Ug9BanNXBq1d2eD6Rnl7LwQIOofDpRIDKqUDNsq1gHlQjwHIuDfMDveN2tAPadUlQqpVcqTVxWVaq9VnStXTjwXVMxMCtQ2LEnihcCQ2x1IAAIAHE0BsFGENSMFk0I1mnGkUig9QVazWUoLuzgSVsIIvShcpc+DUpyNndg8N/KQBeDbXlHxvhOtoFwAABuy/4OT9WGtkMa6VFqzUNtlSqqYareZpVtTqogZbIBIgrRy6toq5odU3PWxVjbfDmpbVajtmquC1rqr2wICRYTfDLXpDYhlbwUFMvAcyiAy2CNjJgUgG90rrEgDA+Kj0sBDQ9e2bt69F0ZjbmDTO8U1mVs5RgQRlV+Ra0cd2eeC5y21t7cKstvr/UkEDUKaQGgLJHo5NZWy6FMLYXWaQzyaA8BCRInbciEkpRSTQDJNy2J5KMSUsoViakOKaUw/MWhAB9eAtBEAsbKFSEgtJaAsf2M4OInFtJQFaE6EI9QSA2n1DaGTBoDSNAYL1BgzQ0DxH1L1OgAAOXUDBtP6hIPEG0upmgaUMJhxoNptMCGMyQYuAB2M0jRWiGYc7JtAdobRoFaLqAQrReq0FaCoWgRp9SNAc0Z8zon6m0F1PEVo2nWj6ZM3aRoJB9T6iNEaO0wJGiNAELQBzXxjS6hIHaYIaASDabtNFmLzH1BsY41xykNI6AsdwtFzDvASAsbYBQUgLGHmsIE/icCImADeBgOhWSQLYeVCy6CqgouwW67JaBWTZWlG40ppuQCsogSF4iFtYNsJthI22SC7Zm0gWs6i4ohDIOdlW/Irt7asqEWgNhlhukwS8aWYRECxgTOd6WvZrv7c+99jA7hjokGB1g0HFBwfvahz9z0/deYI/jM9y7EOrIBy7LQQcZZED/YoOdqyAAdDANOqe4Hp4zhnzPgCE/az2Egeg6fM6Z0ziwlhRzNAXbOUCDKjHrSgvsgo9SUX0P3HT2nPOlf0/srDrwCuZyuUiGLpIEvNqjvyCuoCZt5e04wPZD0iBua8wV1TNAXZFRxhlzeYyfB6hZHPFrOX3w9dvhifXb84NPEcZ4JQZ3Ghue86V8APdGAidsaHL2PQVl8ciP2NjikDg0iIHO8mPbHQpsdCL/t4b8Zqgwcp5b63a9scp/z0Xg7+Iko564GDt7xeZv0pEbywelPsf2HjBCEk9AoDLaUDYFS6gZnIAQEQWAYA+F8lkpR+QqAyAqC8LQDQdeO/7bJkoSn1JnCRKIDvjvVlP4BE6dj8vbBKd929L3+vABfCHBf68zdL7fkglO1f8pB2/g3oJs3kjijrvlZF3pgCSr/k7uIHDkUiUjQBwhRs4PABivkGstiEbgKt4GDD4vFNUncrIqLu6gonLPADIh1PICQeBCumssbnLneI/B4NvoATNvvj/lwFZEfk9GEGfsXlZGUOQgVEQEWrjq9mwftpfoEGlDfhXlwXAV4FZC/oAYXufl/vIftj0J2FqCThlF5AAR/vtsAYSKAe3ufpAT3syDAfyqeobvPF4KDOtJQXrHOAytRsyOouIIHIQd8E+retGnalQV9D9GYuvjSoHHLmvJ5L7pAMioHr+CHgBM7trILibPwQ3hwYfsfnwZIYIaIMyCIWIVthIUYRfnFFfrIXGFgt/pTmzsTontIFOP0kOJRMocXq/vXmoQIRoXflwbWFCp5PfCsjGDgUvNUTjnkSYS3oqMjuYQIZYdAVwVOPtF5HYcgM4H4fKFqBLKeJPOQZLtMtLvQTuIwRQP9EyFehgC6lqPvr8v8syMgBMpXFvhkewYoJwftjwSfm8ftkIYUTbMURdqUeAdIdfhMbUVwe8LgLWPEMMSSCej5E0S0YgG0SoV0WUb0Z8VZL9gwE+stuoqAr8Y3gSDMW3nkYsbzJXpgk+kwISWeqgM0JlhoJlgAKScIIAAgoDIAODxAFQMB5ACIwBO5lAACO2A8AZQ7csAZQh2yo9AqABmLJ+orJrBZRWRXB3xuRZRYJVRCYkJkONJAOPkOeL+e2aYqeI0uAtgVemOveXB2mKkDmrmuoNWDmrp8Qdoem3mv8YWwIJA4mumuorQ/mrQOWjQ8QBoDmSgpmdo5QxoNo8Q9maADmxWzQbp2mZ+BOVptgf+lOaA+ozQSZjQ+oXwmW2mBW7mLQRo2m3mpZAgDmVmtADZ/mzQNoDAAgDAnprQ7paALQDAjQRoNowWRonpFZNZwWAZDmWZtARpFqRABJygpAUcNAT0aU/2mo523R+2m6BkxQZxe6B6W5ZRBA+IHgAYg6Ua52uoeR36vMiArasAuJ3aMxuoZpHQz+Bgn5MWPWfWlAg2penGnW+gQAA=== --> <!-- internal state end -->
LunarTides (Migrated from github.com) reviewed 2025-11-11 12:08:15 +00:00
@ -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
 * @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

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
LunarTides (Migrated from github.com) commented 2025-11-11 12:08:11 +00:00
	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

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

⚠️ Potential issue | 🟡 Minor

Fix typo: foundAbilitesfoundAbilities.

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

⚠️ 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

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

⚠️ 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 -->
coderabbitai[bot] (Migrated from github.com) commented 2025-11-11 12:55:41 +00:00

🛠️ 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

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

⚠️ 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.
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!416
No description provided.