Rewrite enchantments #400

2025-10-14T12:43:12Z

LunarTides commented

2025-10-14 12:43:12 +00:00

(Migrated from github.com)

Closes #275

Turned out to be harder than I thought, but not quite impossible!

Actually make it work :)
- Fix visual needing an update after adding multiple enchantments to one card.
Add enchantmentSetup in place of setup.
Use the new system in place of the old.
Make sure the card creators work.

The Card.activeEnchantments array currently uses actual cards instead of Blueprints. This isn't great memory-wise, but it will allow enchantments to change over time. If, for example, an enchantment has some weird effect like "+1 Attack. If the card has had this enchantment for over 3 turns, +2 Attack" or something like that, it would be helpful to be able to use the self parameter. Cuz that's the thing. If it's a blueprint, then the self parameter would be unavailable, and therefore you couldn't use self.turn to see if the enchantment has been active for more than 3 turns. Although in this example, the enchantment could do some hack by putting the game's turn in host.storage if it doesn't already exist and using that, something like this:

enchantmentApply(host) {
    // +1 Attack. If the card has had this enchantment for over 3 turns, +2 Attack.
    if (!host.storage.weirdEnchantments) {
        host.storage.weirdEnchantments = {};
    }

    if (!host.storage.weirdEnchantments[host.uuid]) {
        host.storage.weirdEnchantments[host.uuid] = game.turn;
    }

    if (game.turn >= host.storage.weirdEnchantments[host.uuid] + 6) { // The turn counter goes up twice every traditional turn.
        host.addAttack(2);
    } else {
        host.addAttack(1);
    }
}

But if it was an actual card, it would be a lot easier:

enchantmentApply(owner, self, host) {
    // +1 Attack. If the card has had this enchantment for over 3 turns, +2 Attack.
    if (game.turn >= self.turn + 6) {
        host.addAttack(2);
    } else {
        host.addAttack(1);
    }
}

And I'm sure that some things are just straight up impossible without the enchantment being an actual card, so whatever. The game can handle a little more memory usage... probably...
Unless you add like a million enchantments to one card, but I feel like there's other bottlenecks, like game.activeCards, so it's fine...

Summary by CodeRabbit

New Features
- Enchantment introduced as a new card type with priority-based ordering and gameplay hooks.
- Several new enchantment cards and example/test cards added (including 0-cost effects and a Galakrond-themed blessing).
- Card-creation tools now support enchantment priority when creating cards.
Improvements
- Existing cards updated to use the new enchantment system for more consistent cost and effect handling.
- Enchantment application/removal and refreshes now run reliably during game ticks.

Closes #275 Turned out to be harder than I thought, but not *quite* impossible! - [x] Actually make it work :) - [x] Fix visual needing an update after adding multiple enchantments to one card. - [x] Add `enchantmentSetup` in place of `setup`. - [x] Use the new system in place of the old. - [x] Make sure the card creators work. The `Card.activeEnchantments` array currently uses *actual* cards instead of Blueprints. This isn't great memory-wise, but it will allow enchantments to change over time. If, for example, an enchantment has some weird effect like "+1 Attack. If the card has had this enchantment for over 3 turns, +2 Attack" or something like that, it would be helpful to be able to use the `self` parameter. Cuz that's the thing. If it's a blueprint, then the `self` parameter would be unavailable, and therefore you couldn't use `self.turn` to see if the enchantment has been active for more than 3 turns. Although in this example, the enchantment *could* do some hack by putting the game's turn in `host.storage` if it doesn't already exist and using that, something like this: ```ts enchantmentApply(host) { // +1 Attack. If the card has had this enchantment for over 3 turns, +2 Attack. if (!host.storage.weirdEnchantments) { host.storage.weirdEnchantments = {}; } if (!host.storage.weirdEnchantments[host.uuid]) { host.storage.weirdEnchantments[host.uuid] = game.turn; } if (game.turn >= host.storage.weirdEnchantments[host.uuid] + 6) { // The turn counter goes up twice every traditional turn. host.addAttack(2); } else { host.addAttack(1); } } ``` But if it was an *actual* card, it would be a lot easier: ```ts enchantmentApply(owner, self, host) { // +1 Attack. If the card has had this enchantment for over 3 turns, +2 Attack. if (game.turn >= self.turn + 6) { host.addAttack(2); } else { host.addAttack(1); } } ``` And I'm sure that some things are just straight up impossible without the enchantment being an actual card, so whatever. The game can handle a *little* more memory usage... probably... Unless you add like a million enchantments to one card, but I feel like there's other bottlenecks, like `game.activeCards`, so it's *fine*...  ## Summary by CodeRabbit * **New Features** * Enchantment introduced as a new card type with priority-based ordering and gameplay hooks. * Several new enchantment cards and example/test cards added (including 0-cost effects and a Galakrond-themed blessing). * Card-creation tools now support enchantment priority when creating cards. * **Improvements** * Existing cards updated to use the new enchantment system for more consistent cost and effect handling. * Enchantment application/removal and refreshes now run reliably during game ticks.

coderabbitai[bot] commented

2025-10-14 12:43:40 +00:00

(Migrated from github.com)

Walkthrough

Replaces string-based enchantments with card-based Enchantment type, adds ID-based async add/remove APIs, introduces enchantment priority and lifecycle hooks, updates types, core card logic, examples, tests, tooling, and card registry entries.

Changes

Cohort / File(s)	Summary
Core types & enums `src/types/card.ts`	Added `Location`, `EnchantmentPriority`, `EnchantmentDefinition`, `CardBackup`; extended `Type` and `Ability` for Enchantment callbacks; updated Blueprint abilities and signatures to include enchantment callbacks and priority.
Card runtime & lifecycle `src/card.ts`	Replaced string-enchantment model with `activeEnchantments` and `enchantmentPriority`; added async `addEnchantment(enchantmentId, owner)`, `removeEnchantment(enchantmentId, card)`, `enchantmentExists`, and `refreshEnchantments()`; refactored trigger to delegate to `_trigger`.
Game play integration `src/game.ts`, `src/event.ts`	Added `Enchantment` play handler in `playCard.typeSpecific`; `HandTick` now awaits `card.refreshEnchantments()` instead of calling sync apply method.
Utilities & validation `src/functions/card.ts`, `src/functions/util.ts`	Validation updated to require `Ability.EnchantmentApply`/`EnchantmentRemove` for Enchantment type; `getRandomTarget` gains optional filter predicate and `getRandomTargetRelative` helper.
Card registry & IDs `cards/ids.ts`, `cards/.latestId`	Renamed `enchantmentExample_56` → `enchantmentApplyExample_56`; added IDs 142–147; bumped `.latestId` 141 → 147.
New enchantment blueprints `cards/Classes/Neutral/Uncollectible/Enchantments/.../144-fates-blessing.ts`, `cards/Galakrond/Rogue/145-galakronds-blessing.ts`	New Enchantment blueprints implementing `enchantmentSetup`, `enchantmentApply`, `enchantmentRemove` to enforce 0-cost and restore previous cost.
Examples & tests `cards/Examples/4/`, `cards/Examples/Test/`	Added/updated example and test enchantment blueprints (IDs 142–147) demonstrating stat and cost modifications and usage via ID-based add/remove.
Usage updates in cards `cards/.../104-yogg-saron-master-of-fate.ts`, `cards/Galakrond/Rogue/67-hero.ts`	Replaced string-based add/remove with awaited ID-based calls using `game.cardIds.*` constants.
Card creator tooling `tools/cardcreator/lib.ts`, `tools/cardcreator/custom.ts`, `tools/cardcreator/vanilla.ts`	Integrated Enchantment type into creation flow, added `enchantmentPriority` handling, updated generated code to emit enchantment-specific callbacks and imports.
Misc & scripts `scripts/test/crash.ts`	Non-functional comment added.

Sequence Diagram(s)

sequenceDiagram
    participant Player
    participant Card as HostCard
    participant Game
    participant Ench as EnchantmentCard

    rect rgb(240,255,240)
    note right of Game: Applying enchantment (new flow)
    Player->>Game: request addEnchantment(enchantId, owner)
    Game->>Ench: lookup blueprint(enchantId)
    Game->>Card: push activeEnchantment(enchant)
    Game->>Ench: call EnchantmentSetup(owner, host)
    Game->>Ench: call EnchantmentApply(owner, host)
    Card->>Card: effect applied (e.g., cost = 0)
    end

    rect rgb(255,245,240)
    note right of Game: Removing enchantment
    Player->>Game: request removeEnchantment(enchantId, owner)
    Game->>Card: remove from activeEnchantments
    Game->>Ench: call EnchantmentRemove(owner, host)
    Card->>Card: effect reverted
    Game->>Card: await refreshEnchantments() (re-apply remaining by priority)
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Pay special attention to:
- src/card.ts: async lifecycle (refresh ordering, applied flags) and integration with silence/destroy flows.
- src/types/card.ts: new enums/types and alignment with usages across codebase.
- All new enchantment blueprints and example tests for correct async/await and storage/restoration of backups.
- tools/cardcreator/*: generated code patterns for enchantments and imports.
- Call sites updated to await add/remove (e.g., Yogg, Galakrond) to ensure no missing awaits.

Poem

🐰
In burrows of code I nibble and hop,
Strings turned to cards — no more slop!
Priorities set, IDs in a row,
Enchantments apply with a gentle glow.
Hop, refresh, and watch effects grow.

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 'Rewrite enchantments' is concise and accurately summarizes the main objective: replacing the enchantment system with a card-based approach.
Linked Issues check	✅ Passed	The PR successfully implements all key objectives from #275: adds Enchantment type, implements enchantmentApply/enchantmentRemove abilities with host parameter, refactors enchantment management to use card instances, and integrates changes throughout the codebase.
Out of Scope Changes check	✅ Passed	All changes are directly related to the enchantment system rewrite. Minor updates to unrelated files (crash.ts comment, util.ts filter support) provide supporting functionality or documentation without deviating from the core objective.
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 rewrite-enchantments

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.}

## Walkthrough Replaces string-based enchantments with card-based Enchantment type, adds ID-based async add/remove APIs, introduces enchantment priority and lifecycle hooks, updates types, core card logic, examples, tests, tooling, and card registry entries. ## Changes | Cohort / File(s) | Summary | |---|---| | **Core types & enums** `src/types/card.ts` | Added `Location`, `EnchantmentPriority`, `EnchantmentDefinition`, `CardBackup`; extended `Type` and `Ability` for Enchantment callbacks; updated Blueprint abilities and signatures to include enchantment callbacks and priority. | | **Card runtime & lifecycle** `src/card.ts` | Replaced string-enchantment model with `activeEnchantments` and `enchantmentPriority`; added async `addEnchantment(enchantmentId, owner)`, `removeEnchantment(enchantmentId, card)`, `enchantmentExists`, and `refreshEnchantments()`; refactored trigger to delegate to `_trigger`. | | **Game play integration** `src/game.ts`, `src/event.ts` | Added `Enchantment` play handler in `playCard.typeSpecific`; `HandTick` now awaits `card.refreshEnchantments()` instead of calling sync apply method. | | **Utilities & validation** `src/functions/card.ts`, `src/functions/util.ts` | Validation updated to require `Ability.EnchantmentApply`/`EnchantmentRemove` for Enchantment type; `getRandomTarget` gains optional filter predicate and `getRandomTargetRelative` helper. | | **Card registry & IDs** `cards/ids.ts`, `cards/.latestId` | Renamed `enchantmentExample_56` → `enchantmentApplyExample_56`; added IDs 142–147; bumped `.latestId` 141 → 147. | | **New enchantment blueprints** `cards/Classes/Neutral/Uncollectible/Enchantments/.../144-fates-blessing.ts`, `cards/Galakrond/Rogue/145-galakronds-blessing.ts` | New Enchantment blueprints implementing `enchantmentSetup`, `enchantmentApply`, `enchantmentRemove` to enforce 0-cost and restore previous cost. | | **Examples & tests** `cards/Examples/4/*`, `cards/Examples/Test/*` | Added/updated example and test enchantment blueprints (IDs 142–147) demonstrating stat and cost modifications and usage via ID-based add/remove. | | **Usage updates in cards** `cards/.../104-yogg-saron-master-of-fate.ts`, `cards/Galakrond/Rogue/67-hero.ts` | Replaced string-based add/remove with awaited ID-based calls using `game.cardIds.*` constants. | | **Card creator tooling** `tools/cardcreator/lib.ts`, `tools/cardcreator/custom.ts`, `tools/cardcreator/vanilla.ts` | Integrated Enchantment type into creation flow, added `enchantmentPriority` handling, updated generated code to emit enchantment-specific callbacks and imports. | | **Misc & scripts** `scripts/test/crash.ts` | Non-functional comment added. | ## Sequence Diagram(s) ```mermaid sequenceDiagram participant Player participant Card as HostCard participant Game participant Ench as EnchantmentCard rect rgb(240,255,240) note right of Game: Applying enchantment (new flow) Player->>Game: request addEnchantment(enchantId, owner) Game->>Ench: lookup blueprint(enchantId) Game->>Card: push activeEnchantment(enchant) Game->>Ench: call EnchantmentSetup(owner, host) Game->>Ench: call EnchantmentApply(owner, host) Card->>Card: effect applied (e.g., cost = 0) end rect rgb(255,245,240) note right of Game: Removing enchantment Player->>Game: request removeEnchantment(enchantId, owner) Game->>Card: remove from activeEnchantments Game->>Ench: call EnchantmentRemove(owner, host) Card->>Card: effect reverted Game->>Card: await refreshEnchantments() (re-apply remaining by priority) end ``` ## Estimated code review effort 🎯 4 (Complex) | ⏱️ ~50 minutes - Pay special attention to: - src/card.ts: async lifecycle (refresh ordering, applied flags) and integration with silence/destroy flows. - src/types/card.ts: new enums/types and alignment with usages across codebase. - All new enchantment blueprints and example tests for correct async/await and storage/restoration of backups. - tools/cardcreator/*: generated code patterns for enchantments and imports. - Call sites updated to await add/remove (e.g., Yogg, Galakrond) to ensure no missing awaits. ## Poem > 🐰 > In burrows of code I nibble and hop, > Strings turned to cards — no more slop! > Priorities set, IDs in a row, > Enchantments apply with a gentle glow. > Hop, refresh, and watch effects grow.   ## 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 'Rewrite enchantments' is concise and accurately summarizes the main objective: replacing the enchantment system with a card-based approach. | | Linked Issues check | ✅ Passed | The PR successfully implements all key objectives from #275: adds Enchantment type, implements enchantmentApply/enchantmentRemove abilities with host parameter, refactors enchantment management to use card instances, and integrates changes throughout the codebase. | | Out of Scope Changes check | ✅ Passed | All changes are directly related to the enchantment system rewrite. Minor updates to unrelated files (crash.ts comment, util.ts filter support) provide supporting functionality or documentation without deviating from the core objective. | | Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. | </details>   <details> <summary>✨ Finishing touches</summary> - [ ]  📝 Generate docstrings <details> <summary>🧪 Generate unit tests (beta)</summary> - [ ]  Create PR with unit tests - [ ]  Post copyable unit tests in a comment - [ ]  Commit unit tests in branch `rewrite-enchantments` </details> </details>   --- Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=LunarTides/Hearthstone.js&utm_content=400)! 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> Comment `@coderabbitai help` to get the list of available commands and usage tips.

LunarTides commented

2025-10-14 12:44:14 +00:00

(Migrated from github.com)

Issue / PR #400 btw!

LunarTides (Migrated from github.com) reviewed

2025-10-31 09:34:57 +00:00

LunarTides (Migrated from github.com) left a comment

Some changes required :)

src/card.ts

Add this to the EnchantmentPriority type.

Do:

export enum EnchantmentPriority {
    Highest = 2,
    High = 1,
    Normal = 0,
    Low = -1,
    Lowest = -2,
}

Add this to the `EnchantmentPriority` type. Do: ```ts export enum EnchantmentPriority { Highest = 2, High = 1, Normal = 0, Low = -1, Lowest = -2, } ```

Oops! Use sort instead of toSorted you dummy!

Oops! Use `sort` instead of `toSorted` you dummy!

src/functions/card.ts

					
				@ -1,5 +1,6 @@

				import { Card } from "@Game/card.ts";

Remove

src/functions/util.ts

					
				@ -540,7 +542,30 @@ ${mainContent}

							targets.push(...game.player2.board);

						}

Maybe do includeOpposingPlayer and includeOpposingBoard instead.

Maybe do `includeOpposingPlayer` and `includeOpposingBoard` instead.

tools/cardcreator/custom.ts

					
				@ -244,6 +246,26 @@ const cardTypeFunctions: {

						return applyCard(card);

I'm not sure if this would work if I were to change the values in the type definition.

LunarTides commented

2025-11-07 08:51:39 +00:00

(Migrated from github.com)

The hard part is out of the way, I just haven't had time to finish this yet. I'll get there soon™ :)

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

2025-11-07 13:22:56 +00:00

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

Actionable comments posted: 10

🧹 Nitpick comments (4)

src/game.ts (1)
805-809: Consider validating the enchantment play scenario.

The comment indicates uncertainty about when an enchantment would be played directly. Since the new enchantment system applies enchantments via card.addEnchantment() rather than playing them, consider:

Should this case throw an error or warning instead of silently succeeding?

Is there a valid scenario where an enchantment card would be in hand and playable?

Could this be simplified to prevent confusion?

If enchantments should never be played directly, consider making this explicit:
 async Enchantment(card: Card, player: Player): Promise<GamePlayCardReturn> {
-	// I don't *really* know how you would be able to play an enchantment, but ok.
-	// I can't really think if anything specific it should do, so...
-	return GamePlayCardReturn.Success;
+	// Enchantments should not be played directly; they are applied via card.addEnchantment()
+	throw new Error(`Cannot play enchantment card directly: ${card.name} (ID: ${card.id})`);
 }
cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)

35-38: Placeholder test needs implementation.

The test method returns EventListenerMessage.Skip, which means this enchantment card is not tested.

Do you want me to generate a test implementation that validates the cost-setting behavior of this enchantment?

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (2)

14-16: TODO: Verify card name and text.

The comment indicates the name and text may not be accurate. Please verify against the original Hearthstone card or clarify the intended values.

Do you want me to search for the official name and text for this card?

36-39: Placeholder test needs implementation.

The test method returns EventListenerMessage.Skip.

Do you want me to generate a test implementation for this enchantment?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc2ff11675 and 8ae7d6d96f.

📒 Files selected for processing (22)

cards/.latestId (1 hunks)
cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (1 hunks)
cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1 hunks)
cards/Examples/4/2a-enchantments.ts (1 hunks)
cards/Examples/4/2b-enchantment-apply.ts (3 hunks)
cards/Examples/4/lasta-enchantment.ts (1 hunks)
cards/Examples/4/lastb-combined.ts (2 hunks)
cards/Examples/Test/enchantment-apply.ts (1 hunks)
cards/Examples/Test/enchantment.ts (1 hunks)
cards/Galakrond/Rogue/145-galakronds-blessing.ts (1 hunks)
cards/Galakrond/Rogue/67-hero.ts (1 hunks)
cards/ids.ts (2 hunks)
src/card.ts (8 hunks)
src/event.ts (1 hunks)
src/functions/card.ts (2 hunks)
src/functions/util.ts (2 hunks)
src/game.ts (1 hunks)
src/player.ts (1 hunks)
src/types/card.ts (8 hunks)
tools/cardcreator/custom.ts (3 hunks)
tools/cardcreator/lib.ts (9 hunks)
tools/cardcreator/vanilla.ts (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (13)

cards/Examples/4/lasta-enchantment.ts (5)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)

blueprint (13-40)

cards/Examples/4/2a-enchantments.ts (1)

blueprint (13-58)

cards/Examples/4/2b-enchantment-apply.ts (1)

blueprint (12-67)

cards/Examples/4/lastb-combined.ts (1)

blueprint (16-120)

src/types/card.ts (1)

Blueprint (291-393)

cards/Examples/4/2a-enchantments.ts (4)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)

blueprint (13-40)

cards/Examples/4/2b-enchantment-apply.ts (1)

blueprint (12-67)

cards/Examples/4/lasta-enchantment.ts (1)

blueprint (13-40)

src/types/card.ts (1)

Blueprint (291-393)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (3)

cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (1)

blueprint (18-201)

cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)

blueprint (13-39)

src/types/card.ts (1)

Blueprint (291-393)

cards/Examples/Test/enchantment.ts (4)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)

blueprint (13-40)

cards/Examples/4/2a-enchantments.ts (1)

blueprint (13-58)

cards/Examples/Test/enchantment-apply.ts (1)

blueprint (14-72)

src/types/card.ts (1)

Blueprint (291-393)

src/functions/util.ts (2)

src/functions/interact.ts (1)

target (295-317)

src/types/other.ts (1)

Target (108-108)

cards/Galakrond/Rogue/145-galakronds-blessing.ts (3)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)

blueprint (13-40)

cards/Galakrond/Rogue/67-hero.ts (1)

blueprint (13-66)

src/types/card.ts (1)

Blueprint (291-393)

cards/Examples/Test/enchantment-apply.ts (2)

cards/Examples/Test/enchantment.ts (1)

blueprint (13-39)

src/card.ts (1)

Card (39-2083)

src/functions/card.ts (1)

src/types/card.ts (1)

Blueprint (291-393)

tools/cardcreator/custom.ts (3)

src/types/card.ts (1)

Blueprint (291-393)

src/game.ts (1)

input (1251-1261)

src/functions/interact.ts (1)

input (1197-1244)

tools/cardcreator/lib.ts (1)

cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (1)

blueprint (18-201)

src/game.ts (1)

src/card.ts (1)

Card (39-2083)

src/types/card.ts (3)

src/game.ts (1)

Location (787-795)

src/card.ts (1)

Card (39-2083)

src/player.ts (1)

Player (19-1423)

src/card.ts (1)

src/types/card.ts (1)

EnchantmentDefinition (40-44)

🔇 Additional comments (18)

src/player.ts (1)

286-287: LGTM! Documentation example improved.

The example has been simplified and the assignment now correctly uses undefined instead of null, which better aligns with the optional type definition of forceTarget?: Target.

cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (2)

111-111: Good migration to ID-based enchantments.

The change from string-based to ID-based enchantments is correctly implemented with async/await.

115-121: No action required - the code is correct as written.

The removeEnchantment function is designed to safely handle cards without the enchantment by checking if the active enchantment exists before removing it. An existing example in the codebase demonstrates this same pattern with the comment: "You don't need to filter the hand since removeEnchantment only removes the enchantment if they're there."

The event listener correctly removes fatesBlessing_144 from all hand cards at end of turn; cards without this enchantment are safely skipped by the removeEnchantment implementation.

cards/ids.ts (2)

60-60: LGTM! Descriptive renaming.

The rename from enchantmentExample_56 to enchantmentApplyExample_56 better clarifies the example's purpose.

146-151: LGTM! New enchantment IDs properly added.

The new enchantment IDs (142-147) are sequential and align with the updated .latestId file.

cards/.latestId (1)

1-1: LGTM! Latest ID correctly updated.

The increment to 147 correctly reflects the six new enchantment IDs added to the codebase.

tools/cardcreator/vanilla.ts (2)

4-4: LGTM! Required import added.

The EnchantmentPriority import is correctly added to support the new enchantment case.

147-153: LGTM! Enchantment type properly supported.

The new case for Type.Enchantment follows the established pattern and correctly initializes the enchantmentPriority field with a sensible default.

cards/Galakrond/Rogue/67-hero.ts (1)

39-39: Enchantment lifecycle is permanent—verify if this is intentional.

The review's observation is correct. Unlike Fate's Blessing (which is removed at end of turn via Event.EndTurn listener), Galakrond's Blessing has no removal logic:

The enchantment file contains an empty enchantmentRemove method

No event listener is set up in the hero's battlecry to remove it

The card text states only "This card costs (0) mana" with no duration qualifier

This design appears intentional—the cost reduction may persist for the card's lifetime. Confirm that permanent cost reduction is the intended behavior for this card.

cards/Examples/4/2a-enchantments.ts (1)

23-57: LGTM! Excellent documentation and correct implementation.

The enchantment correctly implements both apply and remove hooks with proper cost modification/restoration. The inline documentation clearly explains enchantment priority and refresh behavior, which will help developers understand the system.

cards/Examples/4/lasta-enchantment.ts (1)

13-40: LGTM!

The enchantment correctly reduces cost by 1 and restores it on removal. Using EnchantmentPriority.High is appropriate for this use case.

cards/Examples/4/2b-enchantment-apply.ts (1)

31-67: LGTM!

The migration to ID-based enchantment management is correctly implemented. The tick method properly checks for existing enchantments before adding, and the remove method cleans up enchantments appropriately.

cards/Examples/Test/enchantment.ts (1)

26-29: Known issue flagged: maxHealth not properly set.

The FIXME indicates that addStats doesn't properly set maxHealth. This could cause health to be reduced when the enchantment is removed if max health wasn't increased.

Verify the behavior of host.addStats and host.removeStats to ensure they handle max health correctly. You may need to investigate the implementation in src/card.ts to confirm whether this is still an issue.

src/card.ts (5)

191-201: Excellent documentation on enchantment priority.

The explanation and example clearly illustrate how priority affects the application order. This will help developers correctly set priorities for their enchantments.

1207-1208: LGTM!

Correctly clears all enchantments and refreshes to remove their effects when silencing.

1262-1334: Well-designed refactor for flexibility.

The separation of _trigger (internal, variadic parameters) and trigger (public, typed parameters) provides both flexibility for enchantments and type safety for common use cases. The parameter passing correctly implements the (self, owner, other) signature described in the PR objectives.

1472-1487: Implementation looks correct for the new card-based enchantment system.

Using imperfectCopy() to create actual card instances aligns with the PR's goal of making enchantments stateful. The enchantment instances will be added to game.activeCards, which relates to the unchecked PR objective: "Fix game.activeCards not updating according to the enchantments." Ensure this is addressed before merging.

1497-1501: LGTM!

Correctly checks both enchantment ID and owner to avoid false positives.

**Actionable comments posted: 10** <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/game.ts (1)</summary><blockquote> `805-809`: **Consider validating the enchantment play scenario.** The comment indicates uncertainty about when an enchantment would be played directly. Since the new enchantment system applies enchantments via `card.addEnchantment()` rather than playing them, consider: 1. Should this case throw an error or warning instead of silently succeeding? 2. Is there a valid scenario where an enchantment card would be in hand and playable? 3. Could this be simplified to prevent confusion? If enchantments should never be played directly, consider making this explicit: ```diff async Enchantment(card: Card, player: Player): Promise<GamePlayCardReturn> { - // I don't *really* know how you would be able to play an enchantment, but ok. - // I can't really think if anything specific it should do, so... - return GamePlayCardReturn.Success; + // Enchantments should not be played directly; they are applied via card.addEnchantment() + throw new Error(`Cannot play enchantment card directly: ${card.name} (ID: ${card.id})`); } ``` </blockquote></details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)</summary><blockquote> `35-38`: **Placeholder test needs implementation.** The test method returns `EventListenerMessage.Skip`, which means this enchantment card is not tested. Do you want me to generate a test implementation that validates the cost-setting behavior of this enchantment? </blockquote></details> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (2)</summary><blockquote> `14-16`: **TODO: Verify card name and text.** The comment indicates the name and text may not be accurate. Please verify against the original Hearthstone card or clarify the intended values. Do you want me to search for the official name and text for this card? --- `36-39`: **Placeholder test needs implementation.** The test method returns `EventListenerMessage.Skip`. Do you want me to generate a test implementation for this enchantment? </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 fc2ff116754c55b343ab584d4ff77a31e3e7a0f7 and 8ae7d6d96fdf980c9ce465269cf1b6de8f9246cb. </details> <details> <summary>📒 Files selected for processing (22)</summary> * `cards/.latestId` (1 hunks) * `cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts` (1 hunks) * `cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts` (1 hunks) * `cards/Examples/4/2a-enchantments.ts` (1 hunks) * `cards/Examples/4/2b-enchantment-apply.ts` (3 hunks) * `cards/Examples/4/lasta-enchantment.ts` (1 hunks) * `cards/Examples/4/lastb-combined.ts` (2 hunks) * `cards/Examples/Test/enchantment-apply.ts` (1 hunks) * `cards/Examples/Test/enchantment.ts` (1 hunks) * `cards/Galakrond/Rogue/145-galakronds-blessing.ts` (1 hunks) * `cards/Galakrond/Rogue/67-hero.ts` (1 hunks) * `cards/ids.ts` (2 hunks) * `src/card.ts` (8 hunks) * `src/event.ts` (1 hunks) * `src/functions/card.ts` (2 hunks) * `src/functions/util.ts` (2 hunks) * `src/game.ts` (1 hunks) * `src/player.ts` (1 hunks) * `src/types/card.ts` (8 hunks) * `tools/cardcreator/custom.ts` (3 hunks) * `tools/cardcreator/lib.ts` (9 hunks) * `tools/cardcreator/vanilla.ts` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (13)</summary> <details> <summary>cards/Examples/4/lasta-enchantment.ts (5)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>cards/Examples/4/2a-enchantments.ts (1)</summary> * `blueprint` (13-58) </details> <details> <summary>cards/Examples/4/2b-enchantment-apply.ts (1)</summary> * `blueprint` (12-67) </details> <details> <summary>cards/Examples/4/lastb-combined.ts (1)</summary> * `blueprint` (16-120) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> </blockquote></details> <details> <summary>cards/Examples/4/2a-enchantments.ts (4)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>cards/Examples/4/2b-enchantment-apply.ts (1)</summary> * `blueprint` (12-67) </details> <details> <summary>cards/Examples/4/lasta-enchantment.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> </blockquote></details> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (3)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (1)</summary> * `blueprint` (18-201) </details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)</summary> * `blueprint` (13-39) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> </blockquote></details> <details> <summary>cards/Examples/Test/enchantment.ts (4)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>cards/Examples/4/2a-enchantments.ts (1)</summary> * `blueprint` (13-58) </details> <details> <summary>cards/Examples/Test/enchantment-apply.ts (1)</summary> * `blueprint` (14-72) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> </blockquote></details> <details> <summary>src/functions/util.ts (2)</summary><blockquote> <details> <summary>src/functions/interact.ts (1)</summary> * `target` (295-317) </details> <details> <summary>src/types/other.ts (1)</summary> * `Target` (108-108) </details> </blockquote></details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (3)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>cards/Galakrond/Rogue/67-hero.ts (1)</summary> * `blueprint` (13-66) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> </blockquote></details> <details> <summary>cards/Examples/Test/enchantment-apply.ts (2)</summary><blockquote> <details> <summary>cards/Examples/Test/enchantment.ts (1)</summary> * `blueprint` (13-39) </details> <details> <summary>src/card.ts (1)</summary> * `Card` (39-2083) </details> </blockquote></details> <details> <summary>src/functions/card.ts (1)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (3)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (291-393) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1251-1261) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1197-1244) </details> </blockquote></details> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (1)</summary> * `blueprint` (18-201) </details> </blockquote></details> <details> <summary>src/game.ts (1)</summary><blockquote> <details> <summary>src/card.ts (1)</summary> * `Card` (39-2083) </details> </blockquote></details> <details> <summary>src/types/card.ts (3)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `Location` (787-795) </details> <details> <summary>src/card.ts (1)</summary> * `Card` (39-2083) </details> <details> <summary>src/player.ts (1)</summary> * `Player` (19-1423) </details> </blockquote></details> <details> <summary>src/card.ts (1)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `EnchantmentDefinition` (40-44) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (18)</summary><blockquote> <details> <summary>src/player.ts (1)</summary><blockquote> `286-287`: **LGTM! Documentation example improved.** The example has been simplified and the assignment now correctly uses `undefined` instead of `null`, which better aligns with the optional type definition of `forceTarget?: Target`. </blockquote></details> <details> <summary>cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts (2)</summary><blockquote> `111-111`: **Good migration to ID-based enchantments.** The change from string-based to ID-based enchantments is correctly implemented with async/await. --- `115-121`: **No action required - the code is correct as written.** The `removeEnchantment` function is designed to safely handle cards without the enchantment by checking if the active enchantment exists before removing it. An existing example in the codebase demonstrates this same pattern with the comment: "You don't need to filter the hand since `removeEnchantment` only removes the enchantment if they're there." The event listener correctly removes `fatesBlessing_144` from all hand cards at end of turn; cards without this enchantment are safely skipped by the `removeEnchantment` implementation. </blockquote></details> <details> <summary>cards/ids.ts (2)</summary><blockquote> `60-60`: **LGTM! Descriptive renaming.** The rename from `enchantmentExample_56` to `enchantmentApplyExample_56` better clarifies the example's purpose. --- `146-151`: **LGTM! New enchantment IDs properly added.** The new enchantment IDs (142-147) are sequential and align with the updated `.latestId` file. </blockquote></details> <details> <summary>cards/.latestId (1)</summary><blockquote> `1-1`: **LGTM! Latest ID correctly updated.** The increment to 147 correctly reflects the six new enchantment IDs added to the codebase. </blockquote></details> <details> <summary>tools/cardcreator/vanilla.ts (2)</summary><blockquote> `4-4`: **LGTM! Required import added.** The `EnchantmentPriority` import is correctly added to support the new enchantment case. --- `147-153`: **LGTM! Enchantment type properly supported.** The new case for `Type.Enchantment` follows the established pattern and correctly initializes the `enchantmentPriority` field with a sensible default. </blockquote></details> <details> <summary>cards/Galakrond/Rogue/67-hero.ts (1)</summary><blockquote> `39-39`: **Enchantment lifecycle is permanent—verify if this is intentional.** The review's observation is correct. Unlike Fate's Blessing (which is removed at end of turn via Event.EndTurn listener), Galakrond's Blessing has no removal logic: - The enchantment file contains an empty `enchantmentRemove` method - No event listener is set up in the hero's battlecry to remove it - The card text states only "This card costs (0) mana" with no duration qualifier This design appears intentional—the cost reduction may persist for the card's lifetime. Confirm that permanent cost reduction is the intended behavior for this card. </blockquote></details> <details> <summary>cards/Examples/4/2a-enchantments.ts (1)</summary><blockquote> `23-57`: **LGTM! Excellent documentation and correct implementation.** The enchantment correctly implements both apply and remove hooks with proper cost modification/restoration. The inline documentation clearly explains enchantment priority and refresh behavior, which will help developers understand the system. </blockquote></details> <details> <summary>cards/Examples/4/lasta-enchantment.ts (1)</summary><blockquote> `13-40`: **LGTM!** The enchantment correctly reduces cost by 1 and restores it on removal. Using `EnchantmentPriority.High` is appropriate for this use case. </blockquote></details> <details> <summary>cards/Examples/4/2b-enchantment-apply.ts (1)</summary><blockquote> `31-67`: **LGTM!** The migration to ID-based enchantment management is correctly implemented. The `tick` method properly checks for existing enchantments before adding, and the `remove` method cleans up enchantments appropriately. </blockquote></details> <details> <summary>cards/Examples/Test/enchantment.ts (1)</summary><blockquote> `26-29`: **Known issue flagged: maxHealth not properly set.** The FIXME indicates that `addStats` doesn't properly set `maxHealth`. This could cause health to be reduced when the enchantment is removed if max health wasn't increased. Verify the behavior of `host.addStats` and `host.removeStats` to ensure they handle max health correctly. You may need to investigate the implementation in `src/card.ts` to confirm whether this is still an issue. </blockquote></details> <details> <summary>src/card.ts (5)</summary><blockquote> `191-201`: **Excellent documentation on enchantment priority.** The explanation and example clearly illustrate how priority affects the application order. This will help developers correctly set priorities for their enchantments. --- `1207-1208`: **LGTM!** Correctly clears all enchantments and refreshes to remove their effects when silencing. --- `1262-1334`: **Well-designed refactor for flexibility.** The separation of `_trigger` (internal, variadic parameters) and `trigger` (public, typed parameters) provides both flexibility for enchantments and type safety for common use cases. The parameter passing correctly implements the `(self, owner, other)` signature described in the PR objectives. --- `1472-1487`: **Implementation looks correct for the new card-based enchantment system.** Using `imperfectCopy()` to create actual card instances aligns with the PR's goal of making enchantments stateful. The enchantment instances will be added to `game.activeCards`, which relates to the unchecked PR objective: "Fix `game.activeCards` not updating according to the enchantments." Ensure this is addressed before merging. --- `1497-1501`: **LGTM!** Correctly checks both enchantment ID and owner to avoid false positives. </blockquote></details> </blockquote></details> </details>

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts

					
				@ -0,0 +1,49 @@

				// Created by the Custom Card Creator

⚠️ Potential issue | 🟠 Major

Empty enchantmentRemove does not restore original cost.

Similar to galakronds-blessing.ts, this enchantment's enchantmentRemove is a no-op, leaving the cost at 0 permanently. If this enchantment is temporary (e.g., "costs (0) this turn" as seen in Yogg-Saron usage), the cost should be restored.

Based on the usage in cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts, this enchantment is explicitly removed at end of turn, so it needs to restore the original cost:

 async enchantmentApply(self, owner, host) {
 	// This card costs (0) mana.
+	self.setStorage(self.uuid, `originalCost_${host.uuid}`, host.cost);
 	host.cost = 0;
 },

 async enchantmentRemove(self, owner, host) {
 	// This card costs (0) mana.
+	const originalCost = self.getStorage(self.uuid, `originalCost_${host.uuid}`);
+	if (originalCost !== undefined) {
+		host.cost = originalCost;
+	}
 },

📝 Committable suggestion

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

	async enchantmentApply(self, owner, host) {
		// This card costs (0) mana.
		self.setStorage(self.uuid, `originalCost_${host.uuid}`, host.cost);
		host.cost = 0;
	},

	async enchantmentRemove(self, owner, host) {
		// This card costs (0) mana.
		const originalCost = self.getStorage(self.uuid, `originalCost_${host.uuid}`);
		if (originalCost !== undefined) {
			host.cost = originalCost;
		}
	},

🤖 Prompt for AI Agents

In cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts
around lines 32-34, the enchantmentRemove is a no-op which leaves the target's
cost stuck at 0; modify the enchantment so that when applied it first saves the
host's original cost onto the enchantment instance or host metadata (e.g.,
self._originalCost), then sets cost to 0, and in enchantmentRemove restore the
saved original cost (guarding against missing value) and clean up the stored
metadata; ensure this mirrors the pattern used in galakronds-blessing.ts and
handles undefined/null safely.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🟠 Major_ **Empty `enchantmentRemove` does not restore original cost.** Similar to `galakronds-blessing.ts`, this enchantment's `enchantmentRemove` is a no-op, leaving the cost at 0 permanently. If this enchantment is temporary (e.g., "costs (0) this turn" as seen in Yogg-Saron usage), the cost should be restored. Based on the usage in `cards/Classes/Neutral/Collectible/Minions/10-Cost/104-yogg-saron-master-of-fate.ts`, this enchantment is explicitly removed at end of turn, so it needs to restore the original cost: ```diff async enchantmentApply(self, owner, host) { // This card costs (0) mana. + self.setStorage(self.uuid, `originalCost_${host.uuid}`, host.cost); host.cost = 0; }, async enchantmentRemove(self, owner, host) { // This card costs (0) mana. + const originalCost = self.getStorage(self.uuid, `originalCost_${host.uuid}`); + if (originalCost !== undefined) { + host.cost = originalCost; + } }, ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion async enchantmentApply(self, owner, host) { // This card costs (0) mana. self.setStorage(self.uuid, `originalCost_${host.uuid}`, host.cost); host.cost = 0; }, async enchantmentRemove(self, owner, host) { // This card costs (0) mana. const originalCost = self.getStorage(self.uuid, `originalCost_${host.uuid}`); if (originalCost !== undefined) { host.cost = originalCost; } }, ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts around lines 32-34, the enchantmentRemove is a no-op which leaves the target's cost stuck at 0; modify the enchantment so that when applied it first saves the host's original cost onto the enchantment instance or host metadata (e.g., self._originalCost), then sets cost to 0, and in enchantmentRemove restore the saved original cost (guarding against missing value) and clean up the stored metadata; ensure this mirrors the pattern used in galakronds-blessing.ts and handles undefined/null safely. ``` </details>  ✅ Addressed in commit 5f0b8db

cards/Examples/Test/enchantment-apply.ts

					
				@ -0,0 +68,4 @@

						assert.equal(target?.attack, originalTargetAttack! + 1);

						assert.equal(target?.health, originalTargetHealth! + 2);

					},

⚠️ Potential issue | 🔴 Critical

Critical test assertion bug.

Lines 69-70 contain incorrect variable references in the assertions:

Line 69 compares target?.attack to originalTargetHealth! + 1 (should be originalTargetAttack! + 1)
Line 70 correctly compares health but the pattern is inconsistent

Apply this diff to fix the test assertions:

 	assert.equal(target?.attack, originalTargetAttack);
 	assert.equal(target?.health, originalTargetHealth);

 	owner.forceTarget = target;
 	await self.trigger(Ability.Cast);
 	owner.forceTarget = undefined;

-	assert.equal(target?.attack, originalTargetHealth! + 1);
+	assert.equal(target?.attack, originalTargetAttack! + 1);
 	assert.equal(target?.health, originalTargetHealth! + 2);
 },

📝 Committable suggestion

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

	async test(self, owner) {
		await game.summon(await Card.create(game.cardIds.sheep_1, owner), owner);

		const target = game.functions.util.getRandomTargetRelative(
			false,
			false,
			true,
			false,
			(target) => target instanceof Card && target.type === Type.Minion,
		);
		const originalTargetAttack = target?.attack;
		const originalTargetHealth = target?.health;

		assert.notEqual(originalTargetAttack, undefined);
		assert.notEqual(originalTargetHealth, undefined);

		assert.equal(target?.attack, originalTargetAttack);
		assert.equal(target?.health, originalTargetHealth);

		owner.forceTarget = target;
		await self.trigger(Ability.Cast);
		owner.forceTarget = undefined;

		assert.equal(target?.attack, originalTargetAttack! + 1);
		assert.equal(target?.health, originalTargetHealth! + 2);
	},

🤖 Prompt for AI Agents

In cards/Examples/Test/enchantment-apply.ts around lines 46 to 71, the test
assertions incorrectly compare attack to originalTargetHealth; update the attack
assertion to use originalTargetAttack and keep the health assertion using
originalTargetHealth — specifically change the assertion that currently expects
target?.attack to equal originalTargetHealth! + 1 to expect
originalTargetAttack! + 1, and ensure the health assertion remains
target?.health === originalTargetHealth! + 2.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🔴 Critical_ **Critical test assertion bug.** Lines 69-70 contain incorrect variable references in the assertions: - Line 69 compares `target?.attack` to `originalTargetHealth! + 1` (should be `originalTargetAttack! + 1`) - Line 70 correctly compares health but the pattern is inconsistent Apply this diff to fix the test assertions: ```diff assert.equal(target?.attack, originalTargetAttack); assert.equal(target?.health, originalTargetHealth); owner.forceTarget = target; await self.trigger(Ability.Cast); owner.forceTarget = undefined; - assert.equal(target?.attack, originalTargetHealth! + 1); + assert.equal(target?.attack, originalTargetAttack! + 1); assert.equal(target?.health, originalTargetHealth! + 2); }, ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion async test(self, owner) { await game.summon(await Card.create(game.cardIds.sheep_1, owner), owner); const target = game.functions.util.getRandomTargetRelative( false, false, true, false, (target) => target instanceof Card && target.type === Type.Minion, ); const originalTargetAttack = target?.attack; const originalTargetHealth = target?.health; assert.notEqual(originalTargetAttack, undefined); assert.notEqual(originalTargetHealth, undefined); assert.equal(target?.attack, originalTargetAttack); assert.equal(target?.health, originalTargetHealth); owner.forceTarget = target; await self.trigger(Ability.Cast); owner.forceTarget = undefined; assert.equal(target?.attack, originalTargetAttack! + 1); assert.equal(target?.health, originalTargetHealth! + 2); }, ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In cards/Examples/Test/enchantment-apply.ts around lines 46 to 71, the test assertions incorrectly compare attack to originalTargetHealth; update the attack assertion to use originalTargetAttack and keep the health assertion using originalTargetHealth — specifically change the assertion that currently expects target?.attack to equal originalTargetHealth! + 1 to expect originalTargetAttack! + 1, and ensure the health assertion remains target?.health === originalTargetHealth! + 2. ``` </details>  ✅ Addressed in commit 5f0b8db

cards/Examples/Test/enchantment.ts

					
				@ -0,0 +1,39 @@

				// Created by the Custom Card Creator

⚠️ Potential issue | 🟡 Minor

Parameter order inconsistency.

This test method uses (owner, self) parameter order, while other enchantment test methods in this PR use (self, owner). This inconsistency could indicate a copy-paste error.

Apply this diff to match the standard parameter order:

-async test(owner, self) {
+async test(self, owner) {
 	// TODO: Add proper tests. #325
 	return EventListenerMessage.Skip;
 },

📝 Committable suggestion

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

	async test(self, owner) {
		// TODO: Add proper tests. #325
		return EventListenerMessage.Skip;
	},

🤖 Prompt for AI Agents

In cards/Examples/Test/enchantment.ts around lines 35 to 38 the test method
signature uses the parameters in the order (owner, self) which is inconsistent
with the rest of the PR; change the function signature to use (self, owner)
instead and update any internal references inside the function to use the new
parameter names accordingly so it matches other enchantment test methods.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🟡 Minor_ **Parameter order inconsistency.** This test method uses `(owner, self)` parameter order, while other enchantment test methods in this PR use `(self, owner)`. This inconsistency could indicate a copy-paste error. Apply this diff to match the standard parameter order: ```diff -async test(owner, self) { +async test(self, owner) { // TODO: Add proper tests. #325 return EventListenerMessage.Skip; }, ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion async test(self, owner) { // TODO: Add proper tests. #325 return EventListenerMessage.Skip; }, ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In cards/Examples/Test/enchantment.ts around lines 35 to 38 the test method signature uses the parameters in the order (owner, self) which is inconsistent with the rest of the PR; change the function signature to use (self, owner) instead and update any internal references inside the function to use the new parameter names accordingly so it matches other enchantment test methods. ``` </details>  ✅ Addressed in commit 5f0b8db

cards/Galakrond/Rogue/145-galakronds-blessing.ts

					
				@ -0,0 +1,48 @@

				// Created by the Custom Card Creator

⚠️ Potential issue | 🟠 Major

Empty enchantmentRemove does not restore original cost.

When the enchantment is removed, the host card's cost remains at 0 because enchantmentRemove is a no-op. If the enchantment is temporary, you should restore the cost or rely on refresh logic.

If the enchantment needs to be reversible, consider storing the original cost and restoring it:

 async enchantmentApply(self, owner, host) {
 	// This card costs (0) mana.
+	self.setStorage(self.uuid, `originalCost_${host.uuid}`, host.cost);
 	host.cost = 0;
 },

 async enchantmentRemove(self, owner, host) {
 	// This card costs (0) mana.
+	const originalCost = self.getStorage(self.uuid, `originalCost_${host.uuid}`);
+	if (originalCost !== undefined) {
+		host.cost = originalCost;
+	}
 },

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

🤖 Prompt for AI Agents

In cards/Galakrond/Rogue/145-galakronds-blessing.ts around lines 31 to 33,
enchantmentRemove is currently empty so a temporary enchantment that set the
host card's cost to 0 is never reverted; capture the host's original cost when
applying the enchantment (e.g., save it on the enchantment object or host
metadata), then in enchantmentRemove restore that saved cost back onto the host
(or if your engine supports it, trigger the card refresh/recompute mechanism
instead) and ensure you guard against missing saved value to avoid crashes.

_⚠️ Potential issue_ | _🟠 Major_ **Empty `enchantmentRemove` does not restore original cost.** When the enchantment is removed, the host card's cost remains at 0 because `enchantmentRemove` is a no-op. If the enchantment is temporary, you should restore the cost or rely on refresh logic. If the enchantment needs to be reversible, consider storing the original cost and restoring it: ```diff async enchantmentApply(self, owner, host) { // This card costs (0) mana. + self.setStorage(self.uuid, `originalCost_${host.uuid}`, host.cost); host.cost = 0; }, async enchantmentRemove(self, owner, host) { // This card costs (0) mana. + const originalCost = self.getStorage(self.uuid, `originalCost_${host.uuid}`); + if (originalCost !== undefined) { + host.cost = originalCost; + } }, ``` > Committable suggestion skipped: line range outside the PR's diff. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In cards/Galakrond/Rogue/145-galakronds-blessing.ts around lines 31 to 33, enchantmentRemove is currently empty so a temporary enchantment that set the host card's cost to 0 is never reverted; capture the host's original cost when applying the enchantment (e.g., save it on the enchantment object or host metadata), then in enchantmentRemove restore that saved cost back onto the host (or if your engine supports it, trigger the card refresh/recompute mechanism instead) and ensure you guard against missing saved value to avoid crashes. ``` </details>

src/card.ts

⚠️ Potential issue | 🟡 Minor

Complete the field documentation.

The comment for activeEnchantments is incomplete and contains empty code block markers.

Apply this diff:

-	/**
-	 * The card's active enchantments.
-	 * ```
-	 */
+	/**
+	 * The card's active enchantments.
+	 */

🤖 Prompt for AI Agents

In src/card.ts around line 254, the JSDoc for the activeEnchantments field is
malformed/incomplete (it ends with stray code block markers and an empty
comment). Replace the broken comment block with a proper JSDoc describing the
field—e.g., open /**, add a one-line description "The card's active
enchantments.", then close */—so the field activeEnchantments:
EnchantmentDefinition[] = []; has a valid, complete comment above it.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🟡 Minor_ **Complete the field documentation.** The comment for `activeEnchantments` is incomplete and contains empty code block markers. Apply this diff: ```diff - /** - * The card's active enchantments. - * ``` - */ + /** + * The card's active enchantments. + */ ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/card.ts around line 254, the JSDoc for the activeEnchantments field is malformed/incomplete (it ends with stray code block markers and an empty comment). Replace the broken comment block with a proper JSDoc describing the field—e.g., open /**, add a one-line description "The card's active enchantments.", then close */—so the field activeEnchantments: EnchantmentDefinition[] = []; has a valid, complete comment above it. ``` </details>  ✅ Addressed in commit 5f0b8db

src/card.ts

⚠️ Potential issue | 🔴 Critical

Critical: Sort order is inverted.

The sort order is ascending (priorityA - priorityB), which applies LOWER priority enchantments first. However, the documentation (lines 196-199) states that "A higher priority means that the enchantment will be applied before others."

Your own example confirms this: the "set attack to 0" enchantment (Highest priority) should be applied BEFORE the "+1 attack" enchantment (Normal priority), resulting in a final attack of 1, not 0.

With the current ascending sort:

Order: [Normal=0, Highest=2] → [+1 attack, set to 0]
Result: 3 → 4 → 0 ❌

With descending sort:

Order: [Highest=2, Normal=0] → [set to 0, +1 attack]
Result: 3 → 0 → 1 ✅

Apply this diff:

-				return priorityA - priorityB;
+				return priorityB - priorityA;

📝 Committable suggestion

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

		if (this.activeEnchantments.length > 1) {
			this.activeEnchantments.sort((aeA, aeB) => {
				const priorityA = aeA.enchantment.enchantmentPriority;
				if (priorityA === undefined) {
					throw new Error(
						`Enchantment with id ${aeA.enchantment.id} does not specify a priority.`,
					);
				}

				const priorityB = aeB.enchantment.enchantmentPriority;
				if (priorityB === undefined) {
					throw new Error(
						`Enchantment with id ${aeB.enchantment.id} does not specify a priority.`,
					);
				}

				return priorityB - priorityA;
			});
		}

🤖 Prompt for AI Agents

In src/card.ts around lines 1401 to 1419, the sort currently orders enchantments
ascending (priorityA - priorityB) which applies lower-priority enchantments
first; change the comparator to sort descending so higher priority values are
applied first (e.g., return priorityB - priorityA or swap operands) while
keeping the existing undefined-priority checks and thrown errors unchanged.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🔴 Critical_ **Critical: Sort order is inverted.** The sort order is ascending (`priorityA - priorityB`), which applies LOWER priority enchantments first. However, the documentation (lines 196-199) states that "A higher priority means that the enchantment will be applied before others." Your own example confirms this: the "set attack to 0" enchantment (Highest priority) should be applied BEFORE the "+1 attack" enchantment (Normal priority), resulting in a final attack of 1, not 0. With the current ascending sort: - Order: [Normal=0, Highest=2] → [+1 attack, set to 0] - Result: 3 → 4 → 0 ❌ With descending sort: - Order: [Highest=2, Normal=0] → [set to 0, +1 attack] - Result: 3 → 0 → 1 ✅ Apply this diff: ```diff - return priorityA - priorityB; + return priorityB - priorityA; ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion if (this.activeEnchantments.length > 1) { this.activeEnchantments.sort((aeA, aeB) => { const priorityA = aeA.enchantment.enchantmentPriority; if (priorityA === undefined) { throw new Error( `Enchantment with id ${aeA.enchantment.id} does not specify a priority.`, ); } const priorityB = aeB.enchantment.enchantmentPriority; if (priorityB === undefined) { throw new Error( `Enchantment with id ${aeB.enchantment.id} does not specify a priority.`, ); } return priorityB - priorityA; }); } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/card.ts around lines 1401 to 1419, the sort currently orders enchantments ascending (priorityA - priorityB) which applies lower-priority enchantments first; change the comparator to sort descending so higher priority values are applied first (e.g., return priorityB - priorityA or swap operands) while keeping the existing undefined-priority checks and thrown errors unchanged. ``` </details>  ✅ Addressed in commit 5f0b8db

src/card.ts

⚠️ Potential issue | 🟠 Major

Missing await on async call.

Line 1527 calls removeFromPlay() without awaiting it. Since removeFromPlay() is async and performs important cleanup (silencing, destroying, and removing the enchantment from game.activeCards), failing to await it can cause:

Race conditions with the subsequent refreshEnchantments() call
Incomplete cleanup of the enchantment instance
Issues with game.activeCards not being properly updated (relates to the unchecked PR objective)

Apply this diff:

 		game.functions.util.remove(this.activeEnchantments, activeEnchantment);
-		activeEnchantment.enchantment.removeFromPlay();
+		await activeEnchantment.enchantment.removeFromPlay();
 
 		await this.refreshEnchantments();

📝 Committable suggestion

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

	async removeEnchantment(enchantmentId: number, card: Card): Promise<boolean> {
		const activeEnchantment = this.activeEnchantments.find(
			(c) => c.enchantment.id === enchantmentId && c.owner === card,
		);

		if (!activeEnchantment) {
			return false;
		}

		// Trigger remove.
		await activeEnchantment.enchantment._trigger(
			Ability.EnchantmentRemove,
			this,
		);

		game.functions.util.remove(this.activeEnchantments, activeEnchantment);
		await activeEnchantment.enchantment.removeFromPlay();

		await this.refreshEnchantments();
		return true;
	}

🤖 Prompt for AI Agents

In src/card.ts around lines 1511 to 1531, the call to
activeEnchantment.enchantment.removeFromPlay() is missing an await which can
cause race conditions and incomplete cleanup; change the call to await
activeEnchantment.enchantment.removeFromPlay() so the async cleanup finishes
before refreshEnchantments() and subsequent logic execute, ensuring proper
silencing/destroying and removal from game.activeCards.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🟠 Major_ **Missing await on async call.** Line 1527 calls `removeFromPlay()` without awaiting it. Since `removeFromPlay()` is async and performs important cleanup (silencing, destroying, and removing the enchantment from `game.activeCards`), failing to await it can cause: 1. Race conditions with the subsequent `refreshEnchantments()` call 2. Incomplete cleanup of the enchantment instance 3. Issues with `game.activeCards` not being properly updated (relates to the unchecked PR objective) Apply this diff: ```diff game.functions.util.remove(this.activeEnchantments, activeEnchantment); - activeEnchantment.enchantment.removeFromPlay(); + await activeEnchantment.enchantment.removeFromPlay(); await this.refreshEnchantments(); ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion async removeEnchantment(enchantmentId: number, card: Card): Promise<boolean> { const activeEnchantment = this.activeEnchantments.find( (c) => c.enchantment.id === enchantmentId && c.owner === card, ); if (!activeEnchantment) { return false; } // Trigger remove. await activeEnchantment.enchantment._trigger( Ability.EnchantmentRemove, this, ); game.functions.util.remove(this.activeEnchantments, activeEnchantment); await activeEnchantment.enchantment.removeFromPlay(); await this.refreshEnchantments(); return true; } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/card.ts around lines 1511 to 1531, the call to activeEnchantment.enchantment.removeFromPlay() is missing an await which can cause race conditions and incomplete cleanup; change the call to await activeEnchantment.enchantment.removeFromPlay() so the async cleanup finishes before refreshEnchantments() and subsequent logic execute, ensuring proper silencing/destroying and removal from game.activeCards. ``` </details>  ✅ Addressed in commit 5f0b8db

src/event.ts

				`@ -101,8 +101,8 @@ export const eventManager = {`

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify enchantment refresh is handled elsewhere.

The removal of per-tick enchantment application is a significant change. The commented line and note "Just in case. Remove for performance boost" suggests this may need revisiting. Please confirm:

Where are enchantments refreshed now (if not per-tick)?
Are there scenarios where card state changes (e.g., stat modifications, keyword additions) that require enchantment refresh?
Have you verified enchantments correctly reflect state changes during gameplay?

According to the PR description, this is a known incomplete item: "Fix game.activeCards not updating according to the enchantments" remains unchecked in the PR checklist.

🏁 Script executed:

#!/bin/bash
# Find where refreshEnchantments is called to understand the new refresh strategy
rg -nP --type=ts 'refreshEnchantments|addEnchantment|removeEnchantment' -A2 -B2

# Check if there are other applyEnchantments calls that were missed
rg -nP --type=ts 'applyEnchantments'

Length of output: 5848

is not applicable here as we need repository files.

🏁 Script executed:

#!/bin/bash
# Get more context around the silence method call
rg -nP --type=ts -B10 -A5 'Remove active enchantments' src/card.ts

# Check for state modifications that might not trigger enchantment refresh
rg -nP --type=ts 'this\.stats|this\.keywords' -B2 -A2 src/card.ts | head -50

# Look for where card state might change without explicit enchantment add/remove
rg -nP --type=ts 'applyEnchantments' -B3 -A3

Length of output: 636

🏁 Script executed:

#!/bin/bash
# Get more context around the silence method (line 1200-1210 area)
rg -nP --type=ts -B10 -A5 'Remove active enchantments' git/src/card.ts

# Check the refreshEnchantments implementation
rg -nP --type=ts -B5 -A15 'async refreshEnchantments' git/src/card.ts

# Search for any scenarios where card state (stats/keywords) might change
rg -nP --type=ts 'addKeyword|removeKeyword|modifyStats' -B2 -A2 git/src/

Length of output: 160

🏁 Script executed:

#!/bin/bash
# Find actual file locations
find . -name "card.ts" -o -name "event.ts" | head -10

# Check current directory
pwd
ls -la | head -20

Length of output: 1257

🏁 Script executed:

#!/bin/bash
# Get context around the silence method in card.ts
sed -n '1195,1215p' ./src/card.ts

# Get the full refreshEnchantments method
sed -n '1395,1450p' ./src/card.ts

# Check for any addKeyword or other state modifying methods
rg -nP --type=ts '\.addKeyword|\.removeKeyword|modifyStats|modifyPower|modifyHealth' -B1 -A1

Length of output: 16553

🏁 Script executed:

#!/bin/bash
# Get the complete refreshEnchantments method to understand what it does
sed -n '1395,1550p' ./src/card.ts

# Check if any enchantment actually depends on keywords/stats
grep -r "hasKeyword\|this\.stats\|this\.power\|this\.health" ./src/card.ts | head -20

Length of output: 5335

Confirm enchantment refresh handling for mid-game state changes.

The removal of per-tick enchantment refresh creates a gap: methods that modify card state (addKeyword(), removeKeyword(), setStats()) do not trigger refreshEnchantments(). Enchantment effects are now only re-applied when:

An enchantment is added/removed
A card is silenced

If enchantment effects conditionally depend on card state (keywords, stats), they will not update when that state changes during gameplay through other means. This aligns with the PR checklist item "Fix game.activeCards not updating according to the enchantments" remaining unchecked.

Verify that enchantments do not have state-dependent conditional logic, or add refresh calls to addKeyword(), removeKeyword(), and stat modification methods that could affect active enchantments.

🤖 Prompt for AI Agents

In src/event.ts around lines 104-105, the per-tick refresh of enchantments was
removed but state-changing methods (addKeyword, removeKeyword, setStats and
similar) no longer call refreshEnchantments(), which can leave conditional
enchantments stale; either (A) add calls to card.refreshEnchantments() at the
end of each method that mutates state (addKeyword, removeKeyword, setStats, and
any other setters affecting keywords/stats/flags) so enchantments re-evaluate
immediately, or (B) if you confirm no enchantments depend on dynamic card state,
add a clear code comment/tests asserting enchantments are state-independent and
keep the performance optimization—implement option A unless you can prove option
B with tests.

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🔴 Critical_ <details> <summary>🧩 Analysis chain</summary> **Verify enchantment refresh is handled elsewhere.** The removal of per-tick enchantment application is a significant change. The commented line and note "Just in case. Remove for performance boost" suggests this may need revisiting. Please confirm: 1. Where are enchantments refreshed now (if not per-tick)? 2. Are there scenarios where card state changes (e.g., stat modifications, keyword additions) that require enchantment refresh? 3. Have you verified enchantments correctly reflect state changes during gameplay? According to the PR description, this is a known incomplete item: "Fix `game.activeCards` not updating according to the enchantments" remains unchecked in the PR checklist. --- 🏁 Script executed: ```shell #!/bin/bash # Find where refreshEnchantments is called to understand the new refresh strategy rg -nP --type=ts 'refreshEnchantments|addEnchantment|removeEnchantment' -A2 -B2 # Check if there are other applyEnchantments calls that were missed rg -nP --type=ts 'applyEnchantments' ``` Length of output: 5848 --- is not applicable here as we need repository files. </parameter> </invoke> --- 🏁 Script executed: ```shell #!/bin/bash # Get more context around the silence method call rg -nP --type=ts -B10 -A5 'Remove active enchantments' src/card.ts # Check for state modifications that might not trigger enchantment refresh rg -nP --type=ts 'this\.stats|this\.keywords' -B2 -A2 src/card.ts | head -50 # Look for where card state might change without explicit enchantment add/remove rg -nP --type=ts 'applyEnchantments' -B3 -A3 ``` Length of output: 636 --- </invoke> --- 🏁 Script executed: ```shell #!/bin/bash # Get more context around the silence method (line 1200-1210 area) rg -nP --type=ts -B10 -A5 'Remove active enchantments' git/src/card.ts # Check the refreshEnchantments implementation rg -nP --type=ts -B5 -A15 'async refreshEnchantments' git/src/card.ts # Search for any scenarios where card state (stats/keywords) might change rg -nP --type=ts 'addKeyword|removeKeyword|modifyStats' -B2 -A2 git/src/ ``` Length of output: 160 --- </invoke> --- 🏁 Script executed: ```shell #!/bin/bash # Find actual file locations find . -name "card.ts" -o -name "event.ts" | head -10 # Check current directory pwd ls -la | head -20 ``` Length of output: 1257 --- </invoke> --- 🏁 Script executed: ```shell #!/bin/bash # Get context around the silence method in card.ts sed -n '1195,1215p' ./src/card.ts # Get the full refreshEnchantments method sed -n '1395,1450p' ./src/card.ts # Check for any addKeyword or other state modifying methods rg -nP --type=ts '\.addKeyword|\.removeKeyword|modifyStats|modifyPower|modifyHealth' -B1 -A1 ``` Length of output: 16553 --- </invoke> --- 🏁 Script executed: ```shell #!/bin/bash # Get the complete refreshEnchantments method to understand what it does sed -n '1395,1550p' ./src/card.ts # Check if any enchantment actually depends on keywords/stats grep -r "hasKeyword\|this\.stats\|this\.power\|this\.health" ./src/card.ts | head -20 ``` Length of output: 5335 --- </details> **Confirm enchantment refresh handling for mid-game state changes.** The removal of per-tick enchantment refresh creates a gap: methods that modify card state (`addKeyword()`, `removeKeyword()`, `setStats()`) do not trigger `refreshEnchantments()`. Enchantment effects are now only re-applied when: - An enchantment is added/removed - A card is silenced If enchantment effects conditionally depend on card state (keywords, stats), they will not update when that state changes during gameplay through other means. This aligns with the PR checklist item "Fix `game.activeCards` not updating according to the enchantments" remaining unchecked. Verify that enchantments do not have state-dependent conditional logic, or add refresh calls to `addKeyword()`, `removeKeyword()`, and stat modification methods that could affect active enchantments. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/event.ts around lines 104-105, the per-tick refresh of enchantments was removed but state-changing methods (addKeyword, removeKeyword, setStats and similar) no longer call refreshEnchantments(), which can leave conditional enchantments stale; either (A) add calls to card.refreshEnchantments() at the end of each method that mutates state (addKeyword, removeKeyword, setStats, and any other setters affecting keywords/stats/flags) so enchantments re-evaluate immediately, or (B) if you confirm no enchantments depend on dynamic card state, add a clear code comment/tests asserting enchantments are state-independent and keep the performance optimization—implement option A unless you can prove option B with tests. ``` </details>  ✅ Addressed in commit 5f0b8db

src/functions/card.ts

⚠️ Potential issue | 🔴 Critical

Type mismatch in requiredFieldsTable.

Lines 278 and 279-283 use Ability enum values instead of string keys matching Blueprint property names. The requiredFieldsTable expects (keyof Blueprint)[], but abilities are stored as methods on the blueprint (e.g., heropower, enchantmentApply, enchantmentRemove), not as Ability enum values.

Apply this diff to use the correct property names:

 const requiredFieldsTable: {
 	[x in Type]: (keyof Blueprint)[];
 } = {
 	[Type.Minion]: ["attack", "health", "tribes"],
 	[Type.Spell]: ["spellSchools"],
 	[Type.Weapon]: ["attack", "health"],
 	[Type.Hero]: ["armor", "heropowerId"],
 	[Type.Location]: ["durability", "cooldown"],
-	[Type.HeroPower]: [Ability.HeroPower],
+	[Type.HeroPower]: ["heropower"],
 	[Type.Enchantment]: [
 		"enchantmentPriority",
-		Ability.EnchantmentApply,
-		Ability.EnchantmentRemove,
+		"enchantmentApply",
+		"enchantmentRemove",
 	],
 	[Type.Undefined]: [],
 };

📝 Committable suggestion

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

		const requiredFieldsTable: {
			[x in Type]: (keyof Blueprint)[];
		} = {
			[Type.Minion]: ["attack", "health", "tribes"],
			[Type.Spell]: ["spellSchools"],
			[Type.Weapon]: ["attack", "health"],
			[Type.Hero]: ["armor", "heropowerId"],
			[Type.Location]: ["durability", "cooldown"],
			[Type.HeroPower]: ["heropower"],
			[Type.Enchantment]: [
				"enchantmentPriority",
				"enchantmentApply",
				"enchantmentRemove",
			],
			[Type.Undefined]: [],

🤖 Prompt for AI Agents

In src/functions/card.ts around lines 270 to 284, the requiredFieldsTable uses
Ability enum members for HeroPower and Enchantment entries which causes a type
mismatch because requiredFieldsTable expects (keyof Blueprint)[]; replace
Ability.HeroPower with the Blueprint property name "heropower" and replace
Ability.EnchantmentApply and Ability.EnchantmentRemove with "enchantmentApply"
and "enchantmentRemove" respectively so all entries are keyof Blueprint and the
types align.

_⚠️ Potential issue_ | _🔴 Critical_ **Type mismatch in requiredFieldsTable.** Lines 278 and 279-283 use `Ability` enum values instead of string keys matching `Blueprint` property names. The `requiredFieldsTable` expects `(keyof Blueprint)[]`, but abilities are stored as methods on the blueprint (e.g., `heropower`, `enchantmentApply`, `enchantmentRemove`), not as `Ability` enum values. Apply this diff to use the correct property names: ```diff const requiredFieldsTable: { [x in Type]: (keyof Blueprint)[]; } = { [Type.Minion]: ["attack", "health", "tribes"], [Type.Spell]: ["spellSchools"], [Type.Weapon]: ["attack", "health"], [Type.Hero]: ["armor", "heropowerId"], [Type.Location]: ["durability", "cooldown"], - [Type.HeroPower]: [Ability.HeroPower], + [Type.HeroPower]: ["heropower"], [Type.Enchantment]: [ "enchantmentPriority", - Ability.EnchantmentApply, - Ability.EnchantmentRemove, + "enchantmentApply", + "enchantmentRemove", ], [Type.Undefined]: [], }; ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion const requiredFieldsTable: { [x in Type]: (keyof Blueprint)[]; } = { [Type.Minion]: ["attack", "health", "tribes"], [Type.Spell]: ["spellSchools"], [Type.Weapon]: ["attack", "health"], [Type.Hero]: ["armor", "heropowerId"], [Type.Location]: ["durability", "cooldown"], [Type.HeroPower]: ["heropower"], [Type.Enchantment]: [ "enchantmentPriority", "enchantmentApply", "enchantmentRemove", ], [Type.Undefined]: [], ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/functions/card.ts around lines 270 to 284, the requiredFieldsTable uses Ability enum members for HeroPower and Enchantment entries which causes a type mismatch because requiredFieldsTable expects (keyof Blueprint)[]; replace Ability.HeroPower with the Blueprint property name "heropower" and replace Ability.EnchantmentApply and Ability.EnchantmentRemove with "enchantmentApply" and "enchantmentRemove" respectively so all entries are keyof Blueprint and the types align. ``` </details>

tools/cardcreator/lib.ts

					
				@ -296,27 +317,43 @@ ${runes}${keywords}

						}

⚠️ Potential issue | 🟠 Major

Clamp invalid enchantment priorities before emitting code.

If a creator types an out-of-range value (e.g. 3) the emitted file contains EnchantmentPriority.undefined, which fails to compile and breaks the workflow. Please sanitize the number before stringifying so we always output a real enum member.

-			case "enchantmentPriority":
-				returnValue = `EnchantmentPriority.${EnchantmentPriority[value]}`;
-				break;
+			case "enchantmentPriority": {
+				let priorityValue =
+					typeof value === "number" ? value : Number.parseInt(`${value}`, 10);
+
+				if (
+					Number.isNaN(priorityValue) ||
+					EnchantmentPriority[priorityValue as EnchantmentPriority] === undefined
+				) {
+					priorityValue = EnchantmentPriority.Normal;
+				}
+
+				returnValue = `EnchantmentPriority.${EnchantmentPriority[priorityValue as EnchantmentPriority]}`;
+				break;
+			}

✅ Addressed in commit 5f0b8db

_⚠️ Potential issue_ | _🟠 Major_ **Clamp invalid enchantment priorities before emitting code.** If a creator types an out-of-range value (e.g. `3`) the emitted file contains `EnchantmentPriority.undefined`, which fails to compile and breaks the workflow. Please sanitize the number before stringifying so we always output a real enum member. ```diff - case "enchantmentPriority": - returnValue = `EnchantmentPriority.${EnchantmentPriority[value]}`; - break; + case "enchantmentPriority": { + let priorityValue = + typeof value === "number" ? value : Number.parseInt(`${value}`, 10); + + if ( + Number.isNaN(priorityValue) || + EnchantmentPriority[priorityValue as EnchantmentPriority] === undefined + ) { + priorityValue = EnchantmentPriority.Normal; + } + + returnValue = `EnchantmentPriority.${EnchantmentPriority[priorityValue as EnchantmentPriority]}`; + break; + } ```   ✅ Addressed in commit 5f0b8db

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

2025-11-07 14:01:30 +00:00

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

Actionable comments posted: 0

♻️ Duplicate comments (1)

tools/cardcreator/lib.ts (1)

335-348: Good validation for enchantment priority.

The priority validation correctly handles edge cases:

Converts string inputs to numbers

Checks for NaN values

Verifies the value maps to a valid enum member

Falls back to EnchantmentPriority.Normal for invalid inputs

This addresses the previous review concern about emitting EnchantmentPriority.undefined.

🧹 Nitpick comments (2)

tools/cardcreator/lib.ts (1)

42-44: TODO: Track EnchantmentRemove ability generation.

The comment indicates that EnchantmentRemove should also be generated, but currently only enchantmentApply is returned. Since enchantments typically need both apply and remove hooks for proper cleanup, this should be addressed.

Do you want me to:

Generate a solution that adds both enchantmentApply and enchantmentRemove abilities to the card template?

Open a tracking issue for this enhancement?

Note: The current approach requires manual addition of enchantmentRemove, which could lead to incomplete enchantment implementations.

cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)

26-32: FIXME appears outdated; the priority system addresses this concern.

The FIXME states "Doesn't work since the cost can be changed by other enchantments," but this is precisely what EnchantmentPriority.High is designed to handle. By using host.backups.init.cost (the original base cost) and having high priority, this enchantment correctly sets the cost to 0 before lower-priority enchantments apply their modifications.

For example, with a 5-mana card:

This enchantment (High priority): 5 → 0

"+1 cost" enchantment (Normal priority): 0 → 1

Result: Card costs 1 mana ✓

Consider removing or updating the FIXME to clarify that the current implementation is correct.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ae7d6d96f and 5f0b8dbbf5.

📒 Files selected for processing (10)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1 hunks)
cards/Examples/4/2a-enchantments.ts (1 hunks)
cards/Examples/Test/enchantment-apply.ts (1 hunks)
cards/Examples/Test/enchantment.ts (1 hunks)
cards/Galakrond/Rogue/145-galakronds-blessing.ts (1 hunks)
scripts/test/crash.ts (1 hunks)
src/card.ts (8 hunks)
src/event.ts (1 hunks)
src/types/card.ts (8 hunks)
tools/cardcreator/lib.ts (9 hunks)

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

cards/Examples/Test/enchantment.ts
cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts
cards/Examples/Test/enchantment-apply.ts
src/event.ts

🧰 Additional context used

🧬 Code graph analysis (5)

tools/cardcreator/lib.ts (2)

cards/Examples/Test/enchantment.ts (1)

blueprint (13-39)

cards/Examples/4/1-passive.ts (1)

blueprint (15-91)

cards/Galakrond/Rogue/145-galakronds-blessing.ts (5)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)

blueprint (13-49)

cards/Examples/4/2a-enchantments.ts (1)

blueprint (13-65)

cards/Examples/4/lasta-enchantment.ts (1)

blueprint (13-40)

cards/Galakrond/Rogue/67-hero.ts (1)

blueprint (13-66)

src/types/card.ts (1)

Blueprint (293-395)

src/types/card.ts (3)

tools/cardcreator/custom.ts (1)

Location (224-241)

src/game.ts (1)

Location (787-795)

src/card.ts (1)

Card (39-2083)

cards/Examples/4/2a-enchantments.ts (5)

cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)

blueprint (13-49)

cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)

blueprint (13-48)

cards/Examples/4/lasta-enchantment.ts (1)

blueprint (13-40)

cards/Examples/4/2b-enchantment-apply.ts (1)

blueprint (12-67)

src/types/card.ts (1)

Blueprint (293-395)

src/card.ts (1)

src/types/card.ts (1)

EnchantmentDefinition (40-44)

🔇 Additional comments (11)

scripts/test/crash.ts (1)

1-1: Clarify the instability and add a tracking issue.

The FIXME mentions instability after #400 (this PR), but provides no details about what's unstable or how to reproduce it. Since this is a crash test script, understanding the failure modes is critical.

Please clarify:

What specific instability are you observing?

Is this related to the enchantment system changes?

Should there be a separate issue tracking this?

If the instability is a known limitation of the new system, consider expanding the comment with reproduction steps or expected behavior.

src/types/card.ts (3)

16-51: LGTM! Well-structured foundational types.

The new Location, EnchantmentPriority, EnchantmentDefinition, and CardBackup types provide a solid foundation for the enchantment system. The numeric priority values (Highest=2 to Lowest=-2) make sorting straightforward.

237-277: Excellent type safety for ability callbacks.

The introduction of EnchantmentAbilityCallback with its distinct signature (self, owner, host) and the comprehensive AbilityCallbacks mapping ensures type safety across all ability hooks. This prevents passing incorrect parameters to enchantment lifecycle methods.

384-394: Clear documentation with helpful examples.

The JSDoc for enchantmentPriority includes a concrete example (attack=0 enchantment vs +1 attack) that clearly illustrates why priority matters. This will help card creators understand the ordering semantics.

cards/Examples/4/2a-enchantments.ts (1)

13-65: Excellent educational example.

This example effectively demonstrates:

Proper use of the three enchantment lifecycle hooks

Clear explanation of priority ordering with concrete scenarios

Correct cleanup in enchantmentRemove (cost += 1 undoes cost -= 1)

Appropriate use of EnchantmentPriority.Normal for simple cost reduction

The inline comments (lines 26-37, 40-59) provide valuable guidance for card creators learning the new system.

cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)

39-41: Proper restoration of original cost.

The enchantment correctly restores the saved original cost in enchantmentRemove, which addresses the previous review concern about the no-op removal.

src/card.ts (5)

189-201: Well-documented enchantment priority field.

The field documentation clearly explains the priority system with a concrete example showing how order affects the final result (3→4→0 vs 3→0→1). The default cooldown = 2 is also appropriate for location cards.

1261-1333: Clean refactor of trigger methods.

The separation of _trigger (internal, variadic parameters) and trigger (public, typed parameters) provides both flexibility for internal use and type safety for external callers. The enchantment abilities will use the variadic approach to pass the host parameter.

1394-1462: Robust enchantment refresh implementation.

The refreshEnchantments method correctly:

Sorts by descending priority (line 1416: priorityB - priorityA) ✓

Validates all priorities are defined before processing

Removes all effects (calling EnchantmentSetup for unapplied or EnchantmentRemove for applied)

Reapplies all effects in priority order

Marks all as applied

The two-phase remove-then-apply approach ensures clean state transitions even when priorities change.

Note: Past review comments about inverted sort order have been addressed—the current descending sort is correct.

1472-1487: ID-based enchantment API with instance storage.

The new addEnchantment method takes an enchantment ID and creates an imperfect copy to store in activeEnchantments. This aligns with the PR objective to store actual card instances (not blueprints), enabling stateful enchantments that can track turn counts or reference self.

The method properly:

Validates the enchantment ID exists

Creates an independent copy via imperfectCopy()

Marks as unapplied initially

Triggers a refresh to apply the enchantment

1511-1531: Proper cleanup in removeEnchantment.

The method correctly:

Finds the specific enchantment by ID and owner

Triggers EnchantmentRemove for final cleanup (line 1521-1524)

Removes from the activeEnchantments array (line 1526)

Awaits removeFromPlay() to ensure complete cleanup (line 1527) ✓

Refreshes remaining enchantments

Note: The missing await flagged in past reviews has been fixed.

**Actionable comments posted: 0** <details> <summary>♻️ Duplicate comments (1)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> `335-348`: **Good validation for enchantment priority.** The priority validation correctly handles edge cases: - Converts string inputs to numbers - Checks for NaN values - Verifies the value maps to a valid enum member - Falls back to `EnchantmentPriority.Normal` for invalid inputs This addresses the previous review concern about emitting `EnchantmentPriority.undefined`. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>tools/cardcreator/lib.ts (1)</summary><blockquote> `42-44`: **TODO: Track EnchantmentRemove ability generation.** The comment indicates that `EnchantmentRemove` should also be generated, but currently only `enchantmentApply` is returned. Since enchantments typically need both apply and remove hooks for proper cleanup, this should be addressed. Do you want me to: 1. Generate a solution that adds both `enchantmentApply` and `enchantmentRemove` abilities to the card template? 2. Open a tracking issue for this enhancement? Note: The current approach requires manual addition of `enchantmentRemove`, which could lead to incomplete enchantment implementations. </blockquote></details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)</summary><blockquote> `26-32`: **FIXME appears outdated; the priority system addresses this concern.** The FIXME states "Doesn't work since the cost can be changed by other enchantments," but this is precisely what `EnchantmentPriority.High` is designed to handle. By using `host.backups.init.cost` (the original base cost) and having high priority, this enchantment correctly sets the cost to 0 before lower-priority enchantments apply their modifications. For example, with a 5-mana card: 1. This enchantment (High priority): 5 → 0 2. "+1 cost" enchantment (Normal priority): 0 → 1 Result: Card costs 1 mana ✓ Consider removing or updating the FIXME to clarify that the current implementation is correct. </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 8ae7d6d96fdf980c9ce465269cf1b6de8f9246cb and 5f0b8dbbf543eb845d526919f9d7f4c2acba7f5b. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts` (1 hunks) * `cards/Examples/4/2a-enchantments.ts` (1 hunks) * `cards/Examples/Test/enchantment-apply.ts` (1 hunks) * `cards/Examples/Test/enchantment.ts` (1 hunks) * `cards/Galakrond/Rogue/145-galakronds-blessing.ts` (1 hunks) * `scripts/test/crash.ts` (1 hunks) * `src/card.ts` (8 hunks) * `src/event.ts` (1 hunks) * `src/types/card.ts` (8 hunks) * `tools/cardcreator/lib.ts` (9 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (4)</summary> * cards/Examples/Test/enchantment.ts * cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts * cards/Examples/Test/enchantment-apply.ts * src/event.ts </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (5)</summary> <details> <summary>tools/cardcreator/lib.ts (2)</summary><blockquote> <details> <summary>cards/Examples/Test/enchantment.ts (1)</summary> * `blueprint` (13-39) </details> <details> <summary>cards/Examples/4/1-passive.ts (1)</summary> * `blueprint` (15-91) </details> </blockquote></details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (5)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)</summary> * `blueprint` (13-49) </details> <details> <summary>cards/Examples/4/2a-enchantments.ts (1)</summary> * `blueprint` (13-65) </details> <details> <summary>cards/Examples/4/lasta-enchantment.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>cards/Galakrond/Rogue/67-hero.ts (1)</summary> * `blueprint` (13-66) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (293-395) </details> </blockquote></details> <details> <summary>src/types/card.ts (3)</summary><blockquote> <details> <summary>tools/cardcreator/custom.ts (1)</summary> * `Location` (224-241) </details> <details> <summary>src/game.ts (1)</summary> * `Location` (787-795) </details> <details> <summary>src/card.ts (1)</summary> * `Card` (39-2083) </details> </blockquote></details> <details> <summary>cards/Examples/4/2a-enchantments.ts (5)</summary><blockquote> <details> <summary>cards/Classes/Neutral/Uncollectible/Enchantments/0-Cost/144-fates-blessing.ts (1)</summary> * `blueprint` (13-49) </details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)</summary> * `blueprint` (13-48) </details> <details> <summary>cards/Examples/4/lasta-enchantment.ts (1)</summary> * `blueprint` (13-40) </details> <details> <summary>cards/Examples/4/2b-enchantment-apply.ts (1)</summary> * `blueprint` (12-67) </details> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (293-395) </details> </blockquote></details> <details> <summary>src/card.ts (1)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `EnchantmentDefinition` (40-44) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (11)</summary><blockquote> <details> <summary>scripts/test/crash.ts (1)</summary><blockquote> `1-1`: **Clarify the instability and add a tracking issue.** The FIXME mentions instability after #400 (this PR), but provides no details about what's unstable or how to reproduce it. Since this is a crash test script, understanding the failure modes is critical. Please clarify: 1. What specific instability are you observing? 2. Is this related to the enchantment system changes? 3. Should there be a separate issue tracking this? If the instability is a known limitation of the new system, consider expanding the comment with reproduction steps or expected behavior. </blockquote></details> <details> <summary>src/types/card.ts (3)</summary><blockquote> `16-51`: **LGTM! Well-structured foundational types.** The new Location, EnchantmentPriority, EnchantmentDefinition, and CardBackup types provide a solid foundation for the enchantment system. The numeric priority values (Highest=2 to Lowest=-2) make sorting straightforward. --- `237-277`: **Excellent type safety for ability callbacks.** The introduction of `EnchantmentAbilityCallback` with its distinct signature `(self, owner, host)` and the comprehensive `AbilityCallbacks` mapping ensures type safety across all ability hooks. This prevents passing incorrect parameters to enchantment lifecycle methods. --- `384-394`: **Clear documentation with helpful examples.** The JSDoc for `enchantmentPriority` includes a concrete example (attack=0 enchantment vs +1 attack) that clearly illustrates why priority matters. This will help card creators understand the ordering semantics. </blockquote></details> <details> <summary>cards/Examples/4/2a-enchantments.ts (1)</summary><blockquote> `13-65`: **Excellent educational example.** This example effectively demonstrates: - Proper use of the three enchantment lifecycle hooks - Clear explanation of priority ordering with concrete scenarios - Correct cleanup in `enchantmentRemove` (cost += 1 undoes cost -= 1) - Appropriate use of `EnchantmentPriority.Normal` for simple cost reduction The inline comments (lines 26-37, 40-59) provide valuable guidance for card creators learning the new system. </blockquote></details> <details> <summary>cards/Galakrond/Rogue/145-galakronds-blessing.ts (1)</summary><blockquote> `39-41`: **Proper restoration of original cost.** The enchantment correctly restores the saved original cost in `enchantmentRemove`, which addresses the previous review concern about the no-op removal. </blockquote></details> <details> <summary>src/card.ts (5)</summary><blockquote> `189-201`: **Well-documented enchantment priority field.** The field documentation clearly explains the priority system with a concrete example showing how order affects the final result (3→4→0 vs 3→0→1). The default `cooldown = 2` is also appropriate for location cards. --- `1261-1333`: **Clean refactor of trigger methods.** The separation of `_trigger` (internal, variadic parameters) and `trigger` (public, typed parameters) provides both flexibility for internal use and type safety for external callers. The enchantment abilities will use the variadic approach to pass the `host` parameter. --- `1394-1462`: **Robust enchantment refresh implementation.** The `refreshEnchantments` method correctly: 1. Sorts by descending priority (line 1416: `priorityB - priorityA`) ✓ 2. Validates all priorities are defined before processing 3. Removes all effects (calling `EnchantmentSetup` for unapplied or `EnchantmentRemove` for applied) 4. Reapplies all effects in priority order 5. Marks all as applied The two-phase remove-then-apply approach ensures clean state transitions even when priorities change. Note: Past review comments about inverted sort order have been addressed—the current descending sort is correct. --- `1472-1487`: **ID-based enchantment API with instance storage.** The new `addEnchantment` method takes an enchantment ID and creates an imperfect copy to store in `activeEnchantments`. This aligns with the PR objective to store actual card instances (not blueprints), enabling stateful enchantments that can track turn counts or reference `self`. The method properly: - Validates the enchantment ID exists - Creates an independent copy via `imperfectCopy()` - Marks as unapplied initially - Triggers a refresh to apply the enchantment --- `1511-1531`: **Proper cleanup in removeEnchantment.** The method correctly: 1. Finds the specific enchantment by ID and owner 2. Triggers `EnchantmentRemove` for final cleanup (line 1521-1524) 3. Removes from the activeEnchantments array (line 1526) 4. **Awaits** `removeFromPlay()` to ensure complete cleanup (line 1527) ✓ 5. Refreshes remaining enchantments Note: The missing `await` flagged in past reviews has been fixed. </blockquote></details> </blockquote></details> </details>

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

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

No due date set.

Dependencies

No dependencies set.

Reference

LunarTides/Hearthstone.js!400

				`@ -244,6 +246,26 @@ const cardTypeFunctions: {`
				`return applyCard(card);`

				`@ -296,27 +317,43 @@ ${runes}${keywords}`
				`}`

Rows
Columns