Create new interaction interface #438

2025-12-09T20:48:29Z

LunarTides commented

2025-12-09 20:48:29 +00:00

(Migrated from github.com)

Closes #436
Closes #384
Closes #15

HUB
Go through the interact module
- Deckcode
- Choose One
- Choose From List
- Yes No
- Target
- Use Location
- Mulligan
- Dredge
- Discover
Commands
- Titan
- Give
Deckcode to vanilla
Adapt
Replace Which card do you want to play?
Custom card creator
Vanilla card creator (Search)
- Was not able to find the class of this card.\nWhat is the class of this card?
Class creator
Deck creator
- Ask Class
- Main
...

Summary by CodeRabbit

Bug Fixes
- Minor README spelling fixes and small copy tweaks.
New Features
- App-wide menu-driven prompts and richer selection dialogs.
- Interactive card/class/vanilla-card creation workflows.
- New prompt utilities for configuring arrays, enums and objects.
- Config flag to toggle legacy vs. new gameloop UI.
Refactoring
- Replaced many text-input flows with unified prompt-based UI.
- Standardized numeric parsing and tightened selection UX.
Tests
- Updated test TODOs for the new prompt helpers and APIs.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Closes #436 Closes #384 Closes #15 - [x] HUB - [x] Go through the `interact` module - [x] Deckcode - [x] Choose One - [x] Choose From List - [x] Yes No - [x] Target - [x] Use Location - [x] Mulligan - [x] Dredge - [x] Discover - [x] Commands - [x] Titan - [x] Give - [x] Deckcode to vanilla - [x] Adapt - [x] Replace `Which card do you want to play?` - [x] Custom card creator - [x] Vanilla card creator (Search) - [x] `Was not able to find the class of this card.\nWhat is the class of this card?` - [x] Class creator - [x] Deck creator - [x] Ask Class - [x] Main - [x] ...  ## Summary by CodeRabbit * **Bug Fixes** * Minor README spelling fixes and small copy tweaks. * **New Features** * App-wide menu-driven prompts and richer selection dialogs. * Interactive card/class/vanilla-card creation workflows. * New prompt utilities for configuring arrays, enums and objects. * Config flag to toggle legacy vs. new gameloop UI. * **Refactoring** * Replaced many text-input flows with unified prompt-based UI. * Standardized numeric parsing and tightened selection UX. * **Tests** * Updated test TODOs for the new prompt helpers and APIs. ✏️ Tip: You can customize this high-level summary in your review settings.

coderabbitai[bot] commented

2025-12-09 20:48:45 +00:00

(Migrated from github.com)

Warning

Rate limit exceeded

@LunarTides has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 19 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 5b5bc74dfea3c19ec9f6d3018a5c2ed9e282e103 and 4ca601bce4.

📒 Files selected for processing (3)

src/functions/deckcode.ts (6 hunks)

tools/cardcreator/custom.ts (2 hunks)

tools/deckcreator.ts (7 hunks)

Walkthrough

Replaced many string-based console prompts with Inquirer.js-driven menus across core and tooling, updated several public APIs and types (AI, Card, Player, GameConfig), adjusted test input tokens to new prompt tokens, and refactored tools (card/deck/packager) and hub to interactive flows; also guarded script main lifecycles.

Changes

Cohort / File(s)	Summary
Test input token updates `cards/Classes/Neutral/Uncollectible/Spells/0-Cost/reign-of-chaos-2ef76c34.ts`, `cards/Examples/3/1-manathirst.ts`, `cards/Lackeys/goblin-4cde4d98.ts`, `cards/Lackeys/kobold-bf5c86d3.ts`, `cards/Lackeys/witchy-39591f26.ts`, `cards/StartingHeroes/Mage/heropower-4b71b3e6.ts`, `cards/StartingHeroes/Priest/heropower-7b3ef914.ts`	Updated test inputQueue tokens (e.g., `"1"`, `"face"`, `"y"`) to new token identifiers (e.g., `"e0"`, `"f0"`, `"pe"`, `"pf"`) to match new prompt semantics; no gameplay logic changes.
Config & types `config.ts`, `src/types/other.ts`	Removed `debug.hideRedundantInformation`; added `advanced.gameloopUseOldUserInterface` to GameConfig.
AI, Card & Player API changes `src/ai.ts`, `src/card.ts`, `src/player.ts`	Renamed `AI.question` → `AI.chooseFromList` (returns string
Game class cleanup `src/game.ts`	Removed translation/input wrapper methods: `logTranslate`, `inputTranslate`, `inputTranslateWithOptions`, `pauseTranslate`.
Prompt & interaction system `src/functions/interact.ts`, `src/functions/card.ts`	Added prompt utilities (`customSelect`, `customSelectEnum`, `configureArray`, `configureArrayEnum`, `configureObject`), extended target APIs with `shouldBeDisabled` callbacks, migrated mulligan/dredge/discover/choose flows to prompt-driven UX, and added `readables()` for card displays.
Command handling `src/commands.ts`	Exported `helpColumns`/`helpDebugColumns`; replaced numeric/string prompts with `customSelect` flows (e.g., Titan ability selection); adjusted separators and pause behavior.
Parsing & utility normalization `src/functions/deckcode.ts`, `src/functions/info.ts`, `src/functions/util.ts`	Replaced lodash parseInt usages with native `parseInt(..., 10)` across parsing sites; standardized multi-match selection flows to use `customSelect`.
Event text tweak `src/event.ts`	Adjusted quest-removal pause message formatting and added a no-op console.log() before pause.
Hub & script lifecycle `hub.ts`, `scripts/test/cards.ts`, `scripts/test/crash.ts`, `scripts/vanilla/generate.ts`	Reworked hub to menu-driven select loops (added `cardCreator()`); guarded top-level game initialization and exported `main()` functions; removed unconditional process.exit() calls and conditionalized script runs on `import.meta.main`.
Card creator modernization `tools/cardcreator/class.ts`, `tools/cardcreator/custom.ts`, `tools/cardcreator/vanilla.ts`, `tools/cardcreator/lib.ts`	Replaced linear Q/A with interactive Inquirer menus, dirty-tracking, confirm/cancel flows; tightened runes codegen; set `blueprint.id` on create and immediately push blueprint + regenerate IDs.
Deck creator & packager refactors `tools/deckcreator.ts`, `tools/packager.ts`	Simplified deck creator state and UI to prompt flows; `askClass()` may return `undefined`; added `rules()` command; packager migrated to `game.prompt.customSelect`, increased pageSize, and replaced metadata helpers with prompt configuration APIs.
Function utilities & card helpers `src/functions/card.ts`, `src/functions/deckcode.ts`, `src/functions/interact.ts`, `src/functions/util.ts`	Added `readables()` mapping for card displays; standardized numeric parsing; replaced many console prompts with prompt APIs; minor formatting/TODO updates.
Tests updated `test/src/functions/card.test.ts`, `test/src/functions/interact.test.ts`, `test/src/game.test.ts`, `test/src/player.test.ts`	Added/updated `test.todo` entries to reflect new prompt APIs and removed translation-related todos; added todos for `readables`, prompt utilities, `canActuallyAttack`, and `inputQueueNext`.
README `README.md`	Minor typographical fix: "seperate" → "separate".

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Areas that will need careful review:

src/functions/interact.ts — large refactor adding many prompt APIs and changing target signatures.
src/player.ts — mulligan signature changes, new inputQueueNext(), and canActuallyAttack() usage.
src/card.ts — doBlueprint/adapt signature and adapt selection changes.
hub.ts — entry-point control-flow refactor to menu-based architecture.
tools/cardcreator/* and tools/deckcreator.ts — user-facing workflow rewrites and persistence/dirtiness behavior.
src/ai.ts & its callers — API rename and return-type changes require propagation.

LunarTides/Hearthstone.js#395 — modifies Card class and related event handling; potentially related to card/blueprint changes in this PR.

Suggested labels

type: improvement

Poem

🐰 I hop through menus, bright and neat,

Prompts now guide each playful feat.
Cards and creators dance in tune,
Tests follow tokens like the moon.
A rabbit cheers — interactive soon!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Create new interaction interface' is vague and does not clearly convey the primary changes, which involve integrating Inquirer.js throughout the codebase.	Consider a more specific title like 'Integrate Inquirer.js for interactive prompts across game and tools' or 'Switch to prompt-driven user interface with Inquirer.js'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The PR successfully addresses all primary coding objectives from linked issues: Inquirer.js integration [#384, #436], custom card creator refactoring with visual representation [#15], and prompt-driven interactions throughout.
Out of Scope Changes check	✅ Passed	All changes align with the migration to Inquirer.js and prompt-driven interfaces. Minor textual corrections (README, config) and internal refactoring are supporting changes that facilitate the main objective.

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

> [!WARNING] > ## Rate limit exceeded > > @LunarTides has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait **5 minutes and 19 seconds** before requesting another review. > > <details> > <summary>⌛ How to resolve this issue?</summary> > > After the wait time has elapsed, a review can be triggered using the `@coderabbitai review` command as a PR comment. Alternatively, push new commits to this PR. > > We recommend that you space out your commits to avoid hitting the rate limit. > > </details> > > > <details> > <summary>🚦 How do rate limits work?</summary> > > CodeRabbit enforces hourly rate limits for each developer per organization. > > Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. > > Please see our [FAQ](https://docs.coderabbit.ai/faq) for further information. > > </details> > > <details> > <summary>📥 Commits</summary> > > Reviewing files that changed from the base of the PR and between 5b5bc74dfea3c19ec9f6d3018a5c2ed9e282e103 and 4ca601bce4ae7a57e3bf556c91260a35cab5121a. > > </details> > > <details> > <summary>📒 Files selected for processing (3)</summary> > > * `src/functions/deckcode.ts` (6 hunks) > * `tools/cardcreator/custom.ts` (2 hunks) > * `tools/deckcreator.ts` (7 hunks) > > </details>   ## Walkthrough Replaced many string-based console prompts with Inquirer.js-driven menus across core and tooling, updated several public APIs and types (AI, Card, Player, GameConfig), adjusted test input tokens to new prompt tokens, and refactored tools (card/deck/packager) and hub to interactive flows; also guarded script main lifecycles. ## Changes | Cohort / File(s) | Summary | |---|---| | **Test input token updates** `cards/Classes/Neutral/Uncollectible/Spells/0-Cost/reign-of-chaos-2ef76c34.ts`, `cards/Examples/3/1-manathirst.ts`, `cards/Lackeys/goblin-4cde4d98.ts`, `cards/Lackeys/kobold-bf5c86d3.ts`, `cards/Lackeys/witchy-39591f26.ts`, `cards/StartingHeroes/Mage/heropower-4b71b3e6.ts`, `cards/StartingHeroes/Priest/heropower-7b3ef914.ts` | Updated test inputQueue tokens (e.g., `"1"`, `"face"`, `"y"`) to new token identifiers (e.g., `"e0"`, `"f0"`, `"pe"`, `"pf"`) to match new prompt semantics; no gameplay logic changes. | | **Config & types** `config.ts`, `src/types/other.ts` | Removed `debug.hideRedundantInformation`; added `advanced.gameloopUseOldUserInterface` to GameConfig. | | **AI, Card & Player API changes** `src/ai.ts`, `src/card.ts`, `src/player.ts` | Renamed `AI.question` → `AI.chooseFromList` (returns string|undefined); `AI.mulligan()` now returns `Card[]`; `Card.doBlueprint()` gained `forceUsingOwnBlueprint?` param; `Card.adapt()` signature simplified; `Player.mulligan()` now takes `Card[]`/returns `Card[]`; added `canActuallyAttack()` and `inputQueueNext()`. | | **Game class cleanup** `src/game.ts` | Removed translation/input wrapper methods: `logTranslate`, `inputTranslate`, `inputTranslateWithOptions`, `pauseTranslate`. | | **Prompt & interaction system** `src/functions/interact.ts`, `src/functions/card.ts` | Added prompt utilities (`customSelect`, `customSelectEnum`, `configureArray`, `configureArrayEnum`, `configureObject`), extended target APIs with `shouldBeDisabled` callbacks, migrated mulligan/dredge/discover/choose flows to prompt-driven UX, and added `readables()` for card displays. | | **Command handling** `src/commands.ts` | Exported `helpColumns`/`helpDebugColumns`; replaced numeric/string prompts with `customSelect` flows (e.g., Titan ability selection); adjusted separators and pause behavior. | | **Parsing & utility normalization** `src/functions/deckcode.ts`, `src/functions/info.ts`, `src/functions/util.ts` | Replaced lodash parseInt usages with native `parseInt(..., 10)` across parsing sites; standardized multi-match selection flows to use `customSelect`. | | **Event text tweak** `src/event.ts` | Adjusted quest-removal pause message formatting and added a no-op console.log() before pause. | | **Hub & script lifecycle** `hub.ts`, `scripts/test/cards.ts`, `scripts/test/crash.ts`, `scripts/vanilla/generate.ts` | Reworked hub to menu-driven select loops (added `cardCreator()`); guarded top-level game initialization and exported `main()` functions; removed unconditional process.exit() calls and conditionalized script runs on `import.meta.main`. | | **Card creator modernization** `tools/cardcreator/class.ts`, `tools/cardcreator/custom.ts`, `tools/cardcreator/vanilla.ts`, `tools/cardcreator/lib.ts` | Replaced linear Q/A with interactive Inquirer menus, dirty-tracking, confirm/cancel flows; tightened runes codegen; set `blueprint.id` on create and immediately push blueprint + regenerate IDs. | | **Deck creator & packager refactors** `tools/deckcreator.ts`, `tools/packager.ts` | Simplified deck creator state and UI to prompt flows; `askClass()` may return `undefined`; added `rules()` command; packager migrated to `game.prompt.customSelect`, increased pageSize, and replaced metadata helpers with prompt configuration APIs. | | **Function utilities & card helpers** `src/functions/card.ts`, `src/functions/deckcode.ts`, `src/functions/interact.ts`, `src/functions/util.ts` | Added `readables()` mapping for card displays; standardized numeric parsing; replaced many console prompts with prompt APIs; minor formatting/TODO updates. | | **Tests updated** `test/src/functions/card.test.ts`, `test/src/functions/interact.test.ts`, `test/src/game.test.ts`, `test/src/player.test.ts` | Added/updated `test.todo` entries to reflect new prompt APIs and removed translation-related todos; added todos for `readables`, prompt utilities, `canActuallyAttack`, and `inputQueueNext`. | | **README** `README.md` | Minor typographical fix: "seperate" → "separate". | ## Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Areas that will need careful review: - src/functions/interact.ts — large refactor adding many prompt APIs and changing target signatures. - src/player.ts — mulligan signature changes, new inputQueueNext(), and canActuallyAttack() usage. - src/card.ts — doBlueprint/adapt signature and adapt selection changes. - hub.ts — entry-point control-flow refactor to menu-based architecture. - tools/cardcreator/* and tools/deckcreator.ts — user-facing workflow rewrites and persistence/dirtiness behavior. - src/ai.ts & its callers — API rename and return-type changes require propagation. ## Possibly related PRs - LunarTides/Hearthstone.js#395 — modifies Card class and related event handling; potentially related to card/blueprint changes in this PR. ## Suggested labels `type: improvement` ## Poem > 🐰 I hop through menus, bright and neat, > > Prompts now guide each playful feat. > > Cards and creators dance in tune, > > Tests follow tokens like the moon. > > A rabbit cheers — interactive soon!   ## Pre-merge checks and finishing touches <details> <summary>❌ Failed checks (1 warning, 1 inconclusive)</summary> | Check name | Status | Explanation | Resolution | | :----------------: | :------------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Docstring Coverage | ⚠️ Warning | Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%. | You can run `@coderabbitai generate docstrings` to improve docstring coverage. | | Title check | ❓ Inconclusive | The title 'Create new interaction interface' is vague and does not clearly convey the primary changes, which involve integrating Inquirer.js throughout the codebase. | Consider a more specific title like 'Integrate Inquirer.js for interactive prompts across game and tools' or 'Switch to prompt-driven user interface with Inquirer.js'. | </details> <details> <summary>✅ Passed checks (3 passed)</summary> | Check name | Status | Explanation | | :------------------------: | :------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | | Linked Issues check | ✅ Passed | The PR successfully addresses all primary coding objectives from linked issues: Inquirer.js integration [#384, #436], custom card creator refactoring with visual representation [#15], and prompt-driven interactions throughout. | | Out of Scope Changes check | ✅ Passed | All changes align with the migration to Inquirer.js and prompt-driven interfaces. Minor textual corrections (README, config) and internal refactoring are supporting changes that facilitate the main objective. | </details>   --- Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=LunarTides/Hearthstone.js&utm_content=438)! 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 (Migrated from github.com) reviewed

2025-12-10 06:04:52 +00:00

tools/cardcreator/custom.ts

					
				@ -1,290 +1,331 @@

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

Only change the what was changed by the user. Do card[key] = value where we do blueprint[key] = value. Add special case for keywords.

Only change the what was changed by the user. Do `card[key] = value` where we do `blueprint[key] = value`. Add special case for keywords.

Rename to separator with two a's.
Add separator after tags instead of all this.

1. Rename to separator with two a's. 2. Add separator after `tags` instead of all this.

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

2025-12-11 18:09:30 +00:00

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

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/deckcreator.ts (1)
67-80: Ensure deck import always restores config.decks.validate using try/finally

The import function temporarily disables deck validation (lines 366-368):
game.config.decks.validate = false;
let newDeck = await game.functions.deckcode.import(player, deckcode);
game.config.decks.validate = true;
If deckcode.import throws, validate remains false for the rest of the session, silently weakening deck checks elsewhere.

Wrap this in a try/finally block to guarantee restoration:
 game.config.decks.validate = false;
- let newDeck = await game.functions.deckcode.import(player, deckcode);
- game.config.decks.validate = true;
+ let newDeck: Card[] | undefined;
+ try {
+ newDeck = await game.functions.deckcode.import(player, deckcode);
+ } finally {
+ game.config.decks.validate = true;
+ }
src/functions/deckcode.ts (1)
150-171: Handle customSelect "Back" result and guard indices in toVanilla

The numeric parsing cleanups are fine, but the multi‑match selection path can currently crash:

customSelect returns "Back" as a sentinel option in addition to numeric indices.

At line 589, if the user chooses "Back", parseInt(chosen, 10) becomes NaN, so matches[NaN] yields undefined, and accessing match.dbfId on line 590 will throw.

You can tighten this up by treating "Back" (or any invalid index) explicitly, e.g.:
-			const chosen = await game.prompt.customSelect(
-				"Multiple cards with the name '</yellow>%s<yellow>' detected! Please choose one.",
-				matches.map((m) => JSON.stringify(m)),
-			);
-
-			match = matches[parseInt(chosen, 10)];
+			const chosen = await game.prompt.customSelect(
+				"Multiple cards with the name '</yellow>%s<yellow>' detected! Please choose one.",
+				matches.map((m) => JSON.stringify(m)),
+			);
+
+			// Treat "Back" (or any invalid index) as "pick the first match" to avoid crashes.
+			let matchIndex = 0;
+			if (chosen !== "Back") {
+				const parsed = parseInt(chosen, 10);
+				if (!Number.isNaN(parsed) && parsed >= 0 && parsed < matches.length) {
+					matchIndex = parsed;
+				}
+			}
+
+			match = matches[matchIndex];
That keeps the flow non‑interactive (no extra loops) and avoids runtime errors even if customSelect adds or changes sentinel values.

🧹 Nitpick comments (13)

config.ts (1)

394-398: Minor typos in new fun facts

The new 4.0 fun facts read well, but there are a couple of small text issues:

“Prievously” → “Previously”.

“enemy secion” → “enemy section”.

Fixing these keeps the in-game copy polished.

Also applies to: 448-449

src/functions/card.ts (2)

12-12: cardFunctions.readables helper is reasonable; watch for double tag parsing

The new readables(cards) helper is a good central place for “indexed readable + tag parsing”. Depending on whether Card.readable already calls parseTags, this may re-parse an already-colored string. That’s harmless but slightly redundant.

If Card.readable now intentionally returns raw tag strings, this is perfect; otherwise, consider either:

Documenting that readables is the canonical entrypoint for tagged output, and having Card.readable avoid parseTags, or

Dropping the parseTags call here and relying on Card.readable alone.

Not a blocker, just worth aligning on one convention.

Also applies to: 247-254

339-355: generateIdsFile blueprint-based generation looks correct

Switching to game.blueprints and using camelCase(blueprint.name) plus a numeric prefix for name-starting-with-digit is a sane way to generate valid identifiers. Sorting by blueprint.id keeps output deterministic.

One minor consideration: game.blueprints.sort(...) mutates the global array. If any runtime code depends on the original order, consider sorting a shallow copy instead, but for a one-off generator this is acceptable.

src/ai.ts (1)

510-535: chooseFromList semantics match prior behavior; confirm all call sites updated

The new chooseFromList(prompt, options) mirrors the old scoring logic but now returns the chosen option string instead of a 1-based index. History logging is also updated to record the actual choice string.

As long as all call sites now treat the result as an option value (or an index string they then parseInt themselves), this is fine. It may be worth giving bestChoice an explicit type (string | undefined) instead of starting from null to avoid accidental any inference.

tools/deckcreator.ts (1)

213-311: Search-based deck UI is a nice UX upgrade with coherent command routing

The new main loop:

Shows current validity and deckcode when available.

Uses a search prompt with top-level “Deck” and “Commands” entries plus card options rendered via parseTags(await c.readable() …).

Toggles between library and deck view with the “Deck” pseudo-option.

Routes user to a structured commands menu instead of free-form text.

Add/remove behavior wired to deck and undoableCommandHistory is consistent and should give a good experience.

Only nit: the “Commands” menu is currently TODO-backed but already prints a clear list of supported commands and wires them through handleCmds, so this is safe to ship and iterate on.

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

655-655: Test coverage reminder for new Player methods.

The test.todo stubs for canActuallyAttack and inputQueueNext align with the new public methods added to the Player class in this PR. Consider implementing these tests to ensure the new API surface is properly validated.

Would you like me to generate skeleton test implementations for these new methods?
src/event.ts (1)
211-213: Consider consolidating newline handling.

The empty console.log() on Line 211 followed by a pause message with a trailing newline could be simplified by including the leading newline in the pause message itself (reverting to the original format). This would reduce the number of function calls and make the intent clearer.

Apply this diff to consolidate:
-			console.log();
 			await game.pause(
-				`You triggered the opponents's '${quest.card.name}'.\n`,
+				`\nYou triggered the opponents's '${quest.card.name}'.\n`,
 			);
src/player.ts (1)
1384-1405: Consider adding explicit return type annotation.

The method returns string | undefined based on the implementation, but there's no explicit return type. Adding it would improve type safety and documentation:
 /**
  * @returns The next element in the input queue.
  */
-inputQueueNext() {
+inputQueueNext(): string | undefined {
   if (this.inputQueue) {
     if (typeof this.inputQueue === "string") {
       return this.inputQueue;
     }
tools/packager.ts (1)

467-491: Clarification: The ||= behavior comment is actually correct.

The comment at lines 472-473 notes that dirty ||= await game.prompt... wouldn't work because if dirty is true, the right side won't evaluate. However, looking at the actual code, you're using separate assignment (changed = await...) followed by dirty ||= changed, which correctly evaluates the async call first. The comment might confuse future readers - consider simplifying or removing it since the current pattern works correctly.
tools/cardcreator/class.ts (1)
66-70: Consider adding a validation error message for better UX.

When the class name contains a space, the validation fails silently. Users may not understand why their input is rejected.
 			const className = (await input({
 				message: "Set the name of the class.",
 				default: heroBlueprint.classes.at(0) ?? "ChangeMe",
-				validate: (value) => !value.includes(" "),
+				validate: (value) =>
+					!value.includes(" ") || "Class name cannot contain spaces.",
 			})) as Class;
tools/cardcreator/custom.ts (2)
70-72: Consider moving dirty declaration before set() for clarity.

The set() function references dirty, which is declared after it. While this works because set() is only called in the loop, moving dirty before set() would make the code flow clearer.

288-303: Misleading variable name newRarity for boolean selection.

This handler is for boolean fields, not rarity. The variable name appears to be copy-pasted from another handler.
 		if (typeof value === "boolean") {
-			const newRarity = await game.prompt.customSelect(
+			const newBoolValue = await game.prompt.customSelect(
 				message,
 				["True", "False"],
 				{
 					arrayTransform: async (i, element) => ({
 						name: element,
 						value: element.toLowerCase(),
 					}),
 					hideBack: false,
 				},
 			);
 
-			set(key, Boolean(newRarity));
+			set(key, newBoolValue === "true");
 			continue;
 		}
src/card.ts (1)

2114-2117: Consider addressing the TODO or creating a tracking issue.

The TODO suggests formatting DivineShield as Divine Shield for readability. This could be addressed by using game.lodash.startCase(keyword) in the map function.

Would you like me to open an issue to track this improvement?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8508a839ac and fd2e5266cd.

📒 Files selected for processing (35)

README.md (1 hunks)
cards/Classes/Neutral/Uncollectible/Spells/0-Cost/reign-of-chaos-2ef76c34.ts (1 hunks)
cards/Examples/3/1-manathirst.ts (1 hunks)
cards/Lackeys/goblin-4cde4d98.ts (1 hunks)
cards/Lackeys/kobold-bf5c86d3.ts (1 hunks)
cards/Lackeys/witchy-39591f26.ts (1 hunks)
cards/StartingHeroes/Mage/heropower-4b71b3e6.ts (1 hunks)
cards/StartingHeroes/Priest/heropower-7b3ef914.ts (1 hunks)
config.ts (3 hunks)
hub.ts (2 hunks)
scripts/test/cards.ts (3 hunks)
scripts/test/crash.ts (2 hunks)
scripts/vanilla/generate.ts (3 hunks)
src/ai.ts (4 hunks)
src/card.ts (6 hunks)
src/commands.ts (8 hunks)
src/event.ts (1 hunks)
src/functions/card.ts (4 hunks)
src/functions/deckcode.ts (6 hunks)
src/functions/info.ts (1 hunks)
src/functions/interact.ts (29 hunks)
src/functions/util.ts (1 hunks)
src/game.ts (0 hunks)
src/player.ts (4 hunks)
src/types/other.ts (1 hunks)
test/src/functions/card.test.ts (1 hunks)
test/src/functions/interact.test.ts (1 hunks)
test/src/game.test.ts (0 hunks)
test/src/player.test.ts (2 hunks)
tools/cardcreator/class.ts (2 hunks)
tools/cardcreator/custom.ts (2 hunks)
tools/cardcreator/lib.ts (3 hunks)
tools/cardcreator/vanilla.ts (3 hunks)
tools/deckcreator.ts (7 hunks)
tools/packager.ts (7 hunks)

💤 Files with no reviewable changes (2)

test/src/game.test.ts
src/game.ts

🧰 Additional context used

🧬 Code graph analysis (14)

hub.ts (6)

scripts/test/crash.ts (1)

main (21-107)

tools/cardcreator/vanilla.ts (1)

main (179-280)

scripts/test/cards.ts (1)

main (22-135)

scripts/vanilla/generate.ts (1)

main (45-80)

tools/packager.ts (1)

main (528-559)

tools/cardcreator/class.ts (1)

main (145-225)

tools/cardcreator/class.ts (3)

src/types/card.ts (1)

Blueprint (320-420)

src/functions/interact.ts (1)

input (1648-1680)

src/game.ts (1)

input (1252-1262)

tools/deckcreator.ts (1)

src/types/other.ts (2)

GameConfig (137-220)

CommandList (108-114)

tools/cardcreator/vanilla.ts (3)

src/functions/interact.ts (1)

input (1648-1680)

src/game.ts (1)

input (1252-1262)

tools/cardcreator/lib.ts (1)

create (176-496)

tools/cardcreator/lib.ts (3)

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

blueprint (12-45)

cards/Examples/1/5-battlecry.ts (1)

blueprint (13-66)

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

blueprint (13-37)

src/functions/util.ts (2)

src/functions/info.ts (1)

version (5-15)

src/commands.ts (1)

version (281-365)

scripts/vanilla/generate.ts (3)

src/game.ts (1)

createGame (1803-1815)

scripts/test/crash.ts (1)

main (21-107)

hub.ts (1)

main (200-250)

src/functions/card.ts (1)

src/card.ts (1)

Card (40-2121)

src/commands.ts (2)

src/functions/interact.ts (1)

target (713-736)

src/card.ts (1)

Card (40-2121)

scripts/test/crash.ts (4)

src/game.ts (1)

createGame (1803-1815)

hub.ts (1)

main (200-250)

scripts/test/cards.ts (1)

main (22-135)

scripts/vanilla/generate.ts (1)

main (45-80)

src/player.ts (3)

src/card.ts (1)

Card (40-2121)

src/functions/interact.ts (1)

mulligan (972-1008)

src/ai.ts (1)

mulligan (599-627)

scripts/test/cards.ts (6)

src/game.ts (1)

createGame (1803-1815)

scripts/test/crash.ts (1)

main (21-107)

tools/cardcreator/vanilla.ts (1)

main (179-280)

hub.ts (1)

main (200-250)

scripts/vanilla/generate.ts (1)

main (45-80)

tools/cardcreator/class.ts (1)

main (145-225)

tools/cardcreator/custom.ts (3)

src/types/card.ts (1)

BlueprintWithOptional (422-425)

src/card.ts (1)

Card (40-2121)

src/game.ts (1)

input (1252-1262)

src/functions/interact.ts (4)

src/game.ts (1)

input (1252-1262)

src/player.ts (1)

Player (21-1453)

src/types/other.ts (2)

TargetFlags (88-93)

Target (124-124)

src/commands.ts (2)

helpColumns (18-32)

helpDebugColumns (38-49)

🔇 Additional comments (60)

README.md (1)

50-51: Spelling/wording fix looks good

The updated sentence reads naturally and accurately describes the HUB behavior; no further changes needed here.

cards/Classes/Neutral/Uncollectible/Spells/0-Cost/reign-of-chaos-2ef76c34.ts (1)

59-61: Test target token aligns with enemy targeting

Using owner.inputQueue = ["e0"] matches the enemy-aligned target selection in this test and is consistent with the new input token pattern introduced elsewhere in the PR.

cards/Lackeys/goblin-4cde4d98.ts (1)

63-65: Friendly target token matches test intent

Setting owner.inputQueue = ["f0"] correctly models selecting the friendly sheep as the Battlecry target under the new interaction scheme.

cards/StartingHeroes/Mage/heropower-4b71b3e6.ts (1)

35-41: Hero-power target token matches face-damage assertion

The ["pe"] input queue cleanly represents targeting the enemy hero for 1 damage and keeps the test aligned with the new prompt/token model.

cards/Examples/3/1-manathirst.ts (1)

88-100: Consistent enemy-target tokens for both Manathirst cases

Using owner.inputQueue = ["e0"] in both the low-mana and Manathirst(6) branches keeps the test deterministic and correctly targets the enemy sheep in each run.

cards/Lackeys/witchy-39591f26.ts (1)

96-99: Friendly transform target token is consistent

owner.inputQueue = ["f0"] correctly encodes selecting the friendly sheep as the Battlecry transform target and matches the token pattern used in other Lackey tests.

cards/StartingHeroes/Priest/heropower-7b3ef914.ts (1)

34-45: Self-heal target token correctly represents friendly hero

Using owner.inputQueue = ["pf"] for both invocations cleanly drives the hero power to target the friendly hero, matching the intended 1→3 and 29→30 health assertions.

src/types/other.ts (1)

190-199: New advanced flag is well placed and clearly named

gameloopUseOldUserInterface: boolean fits naturally under advanced and the naming makes its purpose obvious; assuming config defaults/docs were updated in config.ts/README, this type change looks good.

config.ts (1)

251-256: New gameloopUseOldUserInterface flag is consistent with config structure

The new advanced flag is wired in the right place and matches the updated GameConfig shape. Behavior will depend on callers, but there’s nothing structurally unsafe here.

src/functions/card.ts (1)

232-245: getClasses TODO is fine but semantics differ from Object.values(Class)

The TODO to “Replace this with Object.values(Class)” would change semantics: the current implementation only returns classes that have a Tag.StartingHero blueprint, while Object.values(Class) would include any defined class enum, even if there is no starting hero.

If you ever act on the TODO, double‑check all call sites to ensure they truly want “all defined classes” instead of “implemented starting hero classes only”.

test/src/functions/card.test.ts (1)

41-44: readables test TODO added appropriately

Adding a test.todo("readables") keeps the new helper on the testing radar and matches the existing pattern in this file. No issues here.

src/ai.ts (1)

599-627: Card-based mulligan() return type lines up with the rest of the refactor

Returning Card[] from mulligan() and recording [toMulligan, scores] in history is a clean improvement over the previous index-string approach, and the threshold logic is unchanged.

This should integrate well with a Player.mulligan implementation that takes cards instead of indices; just ensure all AI callers expect the new return type.

hub.ts (3)

5-15: Menu-based cardCreator flow looks solid; double-check ccc.main({}) signature

The new “Create a Card” menu with select + Separator is much clearer than the prior ad‑hoc input handling, and the vanilla‑card path correctly forces vanilla.getAll() up front so missing data is surfaced early.

One thing to verify: ccc.main({}) assumes the custom card creator’s main takes an options object. If its first parameter is still a boolean (e.g. a debug flag), this call changes behavior relative to main()/main(false). Worth confirming against tools/cardcreator/custom.ts.

Also applies to: 70-113

118-198: devmode menu wiring is coherent and keeps responsibilities separated

The developer menu cleanly delegates to:

cardCreator for card flows,

clc.main() for class creation,

card/crash tests, and

vanilla generation + packager.

The confirmation prompt before querying the vanilla API is a good addition, and looping with select until “Back” is chosen matches the overall UX of the new hub.

No functional issues spotted here.

200-250: Top-level main loop using select simplifies navigation

The main hub menu (“Play / Create a Deck / Developer Options / Exit”) is straightforward and uses the same select/Separator pattern as the other flows. Delegation to src.main() and dc.main() preserves existing responsibilities, with interest logs around each action.

Looks good from a control-flow perspective.

tools/deckcreator.ts (2)

94-166: generateDeckcode + initial setup are correct; vanilla conversion guard is sensible

The new generateDeckcode(parseVanillaOnPseudo) correctly:

delegates to deckcode.export(deck, player.heroClass, player.runes),

emits human-readable warnings based on error.msg, and

only converts to vanilla when either there’s no error or the error is recoverable (and parseVanillaOnPseudo is true).

Initial state (deck: Card[] = [], deckcodeFormat = JS, viewingClass = player.heroClass) looks sane, and the TODO about rune-aware filtering is clearly marked. No correctness issues here.

Also applies to: 196-212

314-349: Command handlers are generally solid; undo/neutral/rules behavior looks correct

Highlights:

neutral correctly toggles between player.heroClass and Class.Neutral for filtering.

undo sensibly interprets undoableCommandHistory entries (add/remove) and replays the inverse action against the shared cards list.

format simply flips between JS and Vanilla deckcode output, which plays nicely with generateDeckcode.

rules prints out validation state and deck constraints in a user-friendly way and pauses before returning.

These pieces together make the deck creator much easier to reason about compared to the previous ad‑hoc flow.

Also applies to: 383-401, 405-474

src/commands.ts (6)

18-32: Exporting helpColumns/helpDebugColumns is fine and keeps help text centralized

Making the help column arrays exported is a clean way to share command docs elsewhere while keeping the definitions in one place. The updated wording (e.g., for end, history, help) reads clearly.

No behavioral concerns here.

Also applies to: 38-49

141-149: Titan ability selection via customSelect and readables is well-structured

The new Titan flow:

Uses targetCard with a shouldBeDisabled predicate to ensure only friendly Titans with attackTimes > 0 can be picked.

Materializes Titan abilities as temporary Card instances and feeds them through game.functions.card.readables into customSelect.

Correctly handles "Back" by early-returning.

Uses the parsed index to pick the chosen ability and then splices the corresponding Titan id from titanIds.

This is a nice improvement over manual numeric input, and the bookkeeping (splicing titanIds, removing the keyword when empty, exhausting otherwise) remains correct.

Also applies to: 160-184

196-251: Help command formatting and pause behavior look good

Reusing the exported helpColumns/helpDebugColumns, aligning columns, and then adding a trailing console.log() plus await game.pause("Press enter to continue...\n"); makes the help output easier to read and keeps it consistent with other commands.

No functional issues here.

367-541: History command’s added blank line + pause improve UX without changing semantics

The structural logic of history is unchanged; the main behavioral change is:

Printing a blank line after the history text (console.log();), and

Pausing for input unless flags.echo === false.

This matches the pause pattern used elsewhere (AI info, reload, etc.) and doesn’t affect the string returned when called programmatically.

548-577: Debug give now uses customSelect and readables correctly

For multiple matching cards:

You call customSelect with readables(cards), so the user sees fully formatted card descriptions.

"Back" is honored by returning false.

Otherwise, the numeric choice is parsed and used as an index into cards.

This is consistent with other customSelect usages and is a clear UX upgrade over raw index prompts.

674-677: RL and AI debug outputs: extra newlines + explicit pause are fine

Both rl and ai now:

Print a blank line before their final pause message.

Use a consistent “Press enter to continue…” prompt.

These are cosmetic improvements only and don’t alter the underlying reload or AI-history behavior.

Also applies to: 681-684, 759-762

src/functions/info.ts (1)

8-8: LGTM: Lodash parseInt replaced with native parsing.

The change to parseInt(..., 10) is correct and aligns with the broader codebase shift to explicit radix parsing.

src/functions/util.ts (1)

274-276: LGTM: Consistent migration to native parseInt.

The replacement of lodash parseInt with parseInt(..., 10) for version and logVersion parsing is correct and consistent with the broader refactoring across the codebase.

test/src/functions/interact.test.ts (1)

6-10: Test scaffolding expanded for new prompt helpers.

The addition of test.todo stubs for customSelect, customSelectEnum, configureArray, configureArrayEnum, and configureObject aligns with the new Inquirer.js-based prompt utilities introduced in this PR. These placeholders ensure the expanded prompt API surface is tracked for future test coverage.

cards/Lackeys/kobold-bf5c86d3.ts (1)

47-47: LGTM: Test input simplified for new prompt system.

The change from ["face", "y"] to ["pe"] simplifies the test input sequence by removing the interactive confirmation step. This aligns with the broader test suite migration to the new Inquirer.js-based prompt system, where target selection uses predefined shorthand tokens.
tools/cardcreator/lib.ts (2)
137-140: LGTM: Runes condition properly guards against empty arrays.

The tightened condition blueprint.runes && blueprint.runes.length > 0 correctly prevents generating empty rune code when an empty array is present. This is more precise than a truthy check alone.

484-486: Blueprint registration now runs in debug mode—verify if intentional.

The code now pushes the blueprint to game.blueprints and regenerates the IDs file regardless of debug mode. Previously, this only happened in non-debug mode. If debug mode is intended to be side-effect-free (for testing/validation without modifying game state), this change introduces a breaking side effect.

Please confirm whether debug mode should modify game.blueprints and regenerate the IDs file. If debug mode should remain side-effect-free, apply this diff:
-	// Update the ids so that `game.cardIds` is updated immediately.
-	game.blueprints.push(blueprint);
-	await game.functions.card.generateIdsFile();
+	if (!debugMode) {
+		// Update the ids so that `game.cardIds` is updated immediately.
+		game.blueprints.push(blueprint);
+		await game.functions.card.generateIdsFile();
+	}
scripts/vanilla/generate.ts (1)

15-17: LGTM: Guarded initialization and exported main improve testability.

The changes introduce a consistent pattern across script files:

Game initialization is guarded under import.meta.main to prevent side effects during imports.

The main() function is now exported, making it reusable when the script is imported as a module.

Exit behavior is conditional: exits when run as a script, pauses when imported.

This pattern aligns with similar refactoring in scripts/test/crash.ts and scripts/test/cards.ts, improving script testability and composability.

Also applies to: 45-45, 74-78, 82-84

scripts/test/cards.ts (4)

39-39: Good enhancement: Card ID now shown in progress output.

Including card.id in the progress message improves debuggability when a test hangs or fails.

129-134: LGTM: Conditional exit/pause pattern is consistent.

The import.meta.main guard for exit vs. pause aligns with the pattern used in scripts/test/crash.ts (lines 102-106) and scripts/vanilla/generate.ts (lines 72-76).

137-139: LGTM: Guarded main invocation.

Conditional execution of main() when run as the entry point is consistent with the refactored script patterns across the PR.

11-13: [Your rewritten review comment text here]
[Exactly ONE classification tag]

scripts/test/crash.ts (5)

6-8: Same concern as cards.ts: Module-level code depends on game.

Lines 10 and 12 access game.blueprints and Card.all(true) at module level. When imported as a module (not main), createGame() is skipped, potentially causing failures if game isn't initialized elsewhere first.

This is the same pattern issue noted in scripts/test/cards.ts.

15-16: Good simplification: Native parseInt replaces lodash.

Using native parseInt(gamesEnv, 10) with Number.isNaN check is cleaner and avoids the lodash dependency for this simple operation.

21-21: LGTM: Exporting main() enables module reuse.

Exporting the function allows other modules to invoke the crash test programmatically.

101-106: LGTM: Conditional exit/pause pattern.

Consistent with the refactored lifecycle pattern across test scripts.

109-111: LGTM: Guarded main invocation.

Matches the pattern in other refactored scripts.

tools/cardcreator/vanilla.ts (3)

185-194: LGTM: Type filter prompt with back navigation.

The customSelectEnum usage for card type filtering with "Back" handling is clean and consistent with the new prompt-driven UI patterns.

214-252: LGTM: Search-based card selection.

The search prompt with filtering by name, cost, and type provides a good UX. The special entries for manual input (-2) and back (-1) are well-structured.

256-272: LGTM: Manual name/dbfId entry with validation.

The validation ensures the card exists before proceeding, making the non-null assertion at line 271 safe.

src/player.ts (3)

998-1003: LGTM: New canActuallyAttack() predicate.

This method cleanly encapsulates the attack eligibility check, consolidating canAttack, frozen, and attack > 0 conditions. This is a good abstraction that can be reused throughout the codebase.

1093-1119: LGTM: Mulligan refactored to accept Card[].

The signature change from string-based input to Card[] aligns well with:

AI.mulligan() returning Card[] (src/ai.ts:598-626)

interact.mulligan() building Card[] from checkbox selection (src/functions/interact.ts:971-1007)

The suppression of DiscardCard, AddCardToDeck, and DrawCard events during mulligan is appropriate since these are internal shuffle operations, not gameplay actions.

296-299: LGTM: Documentation updated for inputQueue behavior.

The doc comments now accurately reflect the undefined state when the queue is exhausted.

tools/packager.ts (3)

148-187: LGTM: Pack import with update detection.

The arrayTransform callback elegantly handles showing update status and disabling already-imported packs. The async transform for reading metadata per-pack is well-structured.

528-558: LGTM: Main menu refactored to prompt-based loop.

The while loop with select prompt is cleaner than the previous manual input approach. The "Back" option correctly breaks the loop.

235-243: LGTM: Export pack selection.

Using arrayTransform: undefined explicitly documents that no transformation is needed for this simpler selection.

tools/cardcreator/class.ts (2)

97-104: LGTM!

The number prompt with required: true ensures the cost is always set. The configuration loop with dirty tracking is well-implemented.

145-225: LGTM!

The main function has a clean flow: initialize blueprints → configure interactively → create files → guide user. The early return on cancel is handled correctly.

tools/cardcreator/custom.ts (1)

344-366: LGTM!

The main function cleanly integrates with the new configure() flow and properly handles the cancellation case.

src/card.ts (3)

566-574: LGTM!

The new forceUsingOwnBlueprint parameter cleanly enables the card creator to use custom blueprints while maintaining backward compatibility with the default behavior.

635-637: LGTM!

The simplified hasKeyword implementation using Object.keys().includes() is cleaner and functionally equivalent.

1824-1849: LGTM!

The adapt method now uses the new prompt system cleanly. The random selection via sampleSize and the customSelect prompt provide a better user experience.

src/functions/interact.ts (6)

83-171: LGTM!

The customSelect utility is well-designed with flexible options for array transformation, separators, and disabled states. The pattern of allowing false in otherChoices for conditional inclusion is elegant.

713-916: LGTM!

The target function refactoring is well-implemented. The shouldBeDisabled callback provides flexibility for context-specific target filtering while maintaining clean separation between choice building and post-selection validation.

972-1008: LGTM!

The mulligan function now uses the checkbox prompt for intuitive multi-selection. The exclusion of The Coin from mulligan options is correctly handled.

1079-1118: LGTM!

The discover function is cleanly refactored to use the new prompt system while preserving AI behavior and correctly returning a perfectCopy() of the selected card.

1850-1940: LGTM!

The new gameloop UI is well-designed with:

Backward compatibility via gameloopUseOldUserInterface config option

Organized command menu with proper debug command handling

"Type in" option for users who prefer the legacy interface

The separation between card selection and command execution is clean.

930-950: LGTM!

The useLocation function properly uses the shouldBeDisabled callback to prevent selection of non-locations and locations on cooldown. The post-selection validation is good defensive programming.

**Actionable comments posted: 9** > [!CAUTION] > Some comments are outside the diff and can’t be posted inline due to platform limitations. > > > > <details> > <summary>⚠️ Outside diff range comments (2)</summary><blockquote> > > <details> > <summary>tools/deckcreator.ts (1)</summary><blockquote> > > `67-80`: **Ensure deck import always restores `config.decks.validate` using try/finally** > > The `import` function temporarily disables deck validation (lines 366-368): > > ```ts > game.config.decks.validate = false; > let newDeck = await game.functions.deckcode.import(player, deckcode); > game.config.decks.validate = true; > ``` > > If `deckcode.import` throws, `validate` remains `false` for the rest of the session, silently weakening deck checks elsewhere. > > Wrap this in a try/finally block to guarantee restoration: > > ```diff > game.config.decks.validate = false; > - let newDeck = await game.functions.deckcode.import(player, deckcode); > - game.config.decks.validate = true; > + let newDeck: Card[] | undefined; > + try { > + newDeck = await game.functions.deckcode.import(player, deckcode); > + } finally { > + game.config.decks.validate = true; > + } > ``` > > </blockquote></details> > <details> > <summary>src/functions/deckcode.ts (1)</summary><blockquote> > > `150-171`: **Handle `customSelect` "Back" result and guard indices in toVanilla** > > The numeric parsing cleanups are fine, but the multi‑match selection path can currently crash: > > - `customSelect` returns `"Back"` as a sentinel option in addition to numeric indices. > - At line 589, if the user chooses "Back", `parseInt(chosen, 10)` becomes `NaN`, so `matches[NaN]` yields `undefined`, and accessing `match.dbfId` on line 590 will throw. > > You can tighten this up by treating "Back" (or any invalid index) explicitly, e.g.: > > ```diff > - const chosen = await game.prompt.customSelect( > - "Multiple cards with the name '</yellow>%s<yellow>' detected! Please choose one.", > - matches.map((m) => JSON.stringify(m)), > - ); > - > - match = matches[parseInt(chosen, 10)]; > + const chosen = await game.prompt.customSelect( > + "Multiple cards with the name '</yellow>%s<yellow>' detected! Please choose one.", > + matches.map((m) => JSON.stringify(m)), > + ); > + > + // Treat "Back" (or any invalid index) as "pick the first match" to avoid crashes. > + let matchIndex = 0; > + if (chosen !== "Back") { > + const parsed = parseInt(chosen, 10); > + if (!Number.isNaN(parsed) && parsed >= 0 && parsed < matches.length) { > + matchIndex = parsed; > + } > + } > + > + match = matches[matchIndex]; > ``` > > That keeps the flow non‑interactive (no extra loops) and avoids runtime errors even if `customSelect` adds or changes sentinel values. > > </blockquote></details> > > </blockquote></details> <details> <summary>🧹 Nitpick comments (13)</summary><blockquote> <details> <summary>config.ts (1)</summary><blockquote> `394-398`: **Minor typos in new fun facts** The new 4.0 fun facts read well, but there are a couple of small text issues: - “Prievously” → “Previously”. - “enemy secion” → “enemy section”. Fixing these keeps the in-game copy polished. Also applies to: 448-449 </blockquote></details> <details> <summary>src/functions/card.ts (2)</summary><blockquote> `12-12`: **`cardFunctions.readables` helper is reasonable; watch for double tag parsing** The new `readables(cards)` helper is a good central place for “indexed readable + tag parsing”. Depending on whether `Card.readable` already calls `parseTags`, this may re-parse an already-colored string. That’s harmless but slightly redundant. If `Card.readable` now intentionally returns raw tag strings, this is perfect; otherwise, consider either: - Documenting that `readables` is the canonical entrypoint for tagged output, and having `Card.readable` avoid `parseTags`, or - Dropping the `parseTags` call here and relying on `Card.readable` alone. Not a blocker, just worth aligning on one convention. Also applies to: 247-254 --- `339-355`: **`generateIdsFile` blueprint-based generation looks correct** Switching to `game.blueprints` and using `camelCase(blueprint.name)` plus a numeric prefix for name-starting-with-digit is a sane way to generate valid identifiers. Sorting by `blueprint.id` keeps output deterministic. One minor consideration: `game.blueprints.sort(...)` mutates the global array. If any runtime code depends on the original order, consider sorting a shallow copy instead, but for a one-off generator this is acceptable. </blockquote></details> <details> <summary>src/ai.ts (1)</summary><blockquote> `510-535`: **`chooseFromList` semantics match prior behavior; confirm all call sites updated** The new `chooseFromList(prompt, options)` mirrors the old scoring logic but now returns the chosen option string instead of a 1-based index. History logging is also updated to record the actual choice string. As long as all call sites now treat the result as an option value (or an index string they then `parseInt` themselves), this is fine. It may be worth giving `bestChoice` an explicit type (`string | undefined`) instead of starting from `null` to avoid accidental `any` inference. </blockquote></details> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> `213-311`: **Search-based deck UI is a nice UX upgrade with coherent command routing** The new main loop: - Shows current validity and deckcode when available. - Uses a `search` prompt with top-level “Deck” and “Commands” entries plus card options rendered via `parseTags(await c.readable() …)`. - Toggles between library and deck view with the “Deck” pseudo-option. - Routes user to a structured commands menu instead of free-form text. Add/remove behavior wired to `deck` and `undoableCommandHistory` is consistent and should give a good experience. Only nit: the “Commands” menu is currently TODO-backed but already prints a clear list of supported commands and wires them through `handleCmds`, so this is safe to ship and iterate on. </blockquote></details> <details> <summary>test/src/player.test.ts (1)</summary><blockquote> `655-655`: **Test coverage reminder for new Player methods.** The test.todo stubs for `canActuallyAttack` and `inputQueueNext` align with the new public methods added to the Player class in this PR. Consider implementing these tests to ensure the new API surface is properly validated. Would you like me to generate skeleton test implementations for these new methods? </blockquote></details> <details> <summary>src/event.ts (1)</summary><blockquote> `211-213`: **Consider consolidating newline handling.** The empty `console.log()` on Line 211 followed by a pause message with a trailing newline could be simplified by including the leading newline in the pause message itself (reverting to the original format). This would reduce the number of function calls and make the intent clearer. Apply this diff to consolidate: ```diff - console.log(); await game.pause( - `You triggered the opponents's '${quest.card.name}'.\n`, + `\nYou triggered the opponents's '${quest.card.name}'.\n`, ); ``` </blockquote></details> <details> <summary>src/player.ts (1)</summary><blockquote> `1384-1405`: **Consider adding explicit return type annotation.** The method returns `string | undefined` based on the implementation, but there's no explicit return type. Adding it would improve type safety and documentation: ```diff /** * @returns The next element in the input queue. */ -inputQueueNext() { +inputQueueNext(): string | undefined { if (this.inputQueue) { if (typeof this.inputQueue === "string") { return this.inputQueue; } ``` </blockquote></details> <details> <summary>tools/packager.ts (1)</summary><blockquote> `467-491`: **Clarification: The `||=` behavior comment is actually correct.** The comment at lines 472-473 notes that `dirty ||= await game.prompt...` wouldn't work because if `dirty` is `true`, the right side won't evaluate. However, looking at the actual code, you're using separate assignment (`changed = await...`) followed by `dirty ||= changed`, which correctly evaluates the async call first. The comment might confuse future readers - consider simplifying or removing it since the current pattern works correctly. </blockquote></details> <details> <summary>tools/cardcreator/class.ts (1)</summary><blockquote> `66-70`: **Consider adding a validation error message for better UX.** When the class name contains a space, the validation fails silently. Users may not understand why their input is rejected. ```diff const className = (await input({ message: "Set the name of the class.", default: heroBlueprint.classes.at(0) ?? "ChangeMe", - validate: (value) => !value.includes(" "), + validate: (value) => + !value.includes(" ") || "Class name cannot contain spaces.", })) as Class; ``` </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (2)</summary><blockquote> `70-72`: **Consider moving `dirty` declaration before `set()` for clarity.** The `set()` function references `dirty`, which is declared after it. While this works because `set()` is only called in the loop, moving `dirty` before `set()` would make the code flow clearer. --- `288-303`: **Misleading variable name `newRarity` for boolean selection.** This handler is for boolean fields, not rarity. The variable name appears to be copy-pasted from another handler. ```diff if (typeof value === "boolean") { - const newRarity = await game.prompt.customSelect( + const newBoolValue = await game.prompt.customSelect( message, ["True", "False"], { arrayTransform: async (i, element) => ({ name: element, value: element.toLowerCase(), }), hideBack: false, }, ); - set(key, Boolean(newRarity)); + set(key, newBoolValue === "true"); continue; } ``` </blockquote></details> <details> <summary>src/card.ts (1)</summary><blockquote> `2114-2117`: **Consider addressing the TODO or creating a tracking issue.** The TODO suggests formatting `DivineShield` as `Divine Shield` for readability. This could be addressed by using `game.lodash.startCase(keyword)` in the map function. Would you like me to open an issue to track this improvement? </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 8508a839ace12eb00f35147cb466eb1dcdab085e and fd2e5266cd4796f0683f093b09fa3b5f8e2a7e84. </details> <details> <summary>📒 Files selected for processing (35)</summary> * `README.md` (1 hunks) * `cards/Classes/Neutral/Uncollectible/Spells/0-Cost/reign-of-chaos-2ef76c34.ts` (1 hunks) * `cards/Examples/3/1-manathirst.ts` (1 hunks) * `cards/Lackeys/goblin-4cde4d98.ts` (1 hunks) * `cards/Lackeys/kobold-bf5c86d3.ts` (1 hunks) * `cards/Lackeys/witchy-39591f26.ts` (1 hunks) * `cards/StartingHeroes/Mage/heropower-4b71b3e6.ts` (1 hunks) * `cards/StartingHeroes/Priest/heropower-7b3ef914.ts` (1 hunks) * `config.ts` (3 hunks) * `hub.ts` (2 hunks) * `scripts/test/cards.ts` (3 hunks) * `scripts/test/crash.ts` (2 hunks) * `scripts/vanilla/generate.ts` (3 hunks) * `src/ai.ts` (4 hunks) * `src/card.ts` (6 hunks) * `src/commands.ts` (8 hunks) * `src/event.ts` (1 hunks) * `src/functions/card.ts` (4 hunks) * `src/functions/deckcode.ts` (6 hunks) * `src/functions/info.ts` (1 hunks) * `src/functions/interact.ts` (29 hunks) * `src/functions/util.ts` (1 hunks) * `src/game.ts` (0 hunks) * `src/player.ts` (4 hunks) * `src/types/other.ts` (1 hunks) * `test/src/functions/card.test.ts` (1 hunks) * `test/src/functions/interact.test.ts` (1 hunks) * `test/src/game.test.ts` (0 hunks) * `test/src/player.test.ts` (2 hunks) * `tools/cardcreator/class.ts` (2 hunks) * `tools/cardcreator/custom.ts` (2 hunks) * `tools/cardcreator/lib.ts` (3 hunks) * `tools/cardcreator/vanilla.ts` (3 hunks) * `tools/deckcreator.ts` (7 hunks) * `tools/packager.ts` (7 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * test/src/game.test.ts * src/game.ts </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (14)</summary> <details> <summary>hub.ts (6)</summary><blockquote> <details> <summary>scripts/test/crash.ts (1)</summary> * `main` (21-107) </details> <details> <summary>tools/cardcreator/vanilla.ts (1)</summary> * `main` (179-280) </details> <details> <summary>scripts/test/cards.ts (1)</summary> * `main` (22-135) </details> <details> <summary>scripts/vanilla/generate.ts (1)</summary> * `main` (45-80) </details> <details> <summary>tools/packager.ts (1)</summary> * `main` (528-559) </details> <details> <summary>tools/cardcreator/class.ts (1)</summary> * `main` (145-225) </details> </blockquote></details> <details> <summary>tools/cardcreator/class.ts (3)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (320-420) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1648-1680) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> </blockquote></details> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> <details> <summary>src/types/other.ts (2)</summary> * `GameConfig` (137-220) * `CommandList` (108-114) </details> </blockquote></details> <details> <summary>tools/cardcreator/vanilla.ts (3)</summary><blockquote> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1648-1680) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> <details> <summary>tools/cardcreator/lib.ts (1)</summary> * `create` (176-496) </details> </blockquote></details> <details> <summary>tools/cardcreator/lib.ts (3)</summary><blockquote> <details> <summary>cards/Examples/1/4-taunt.ts (1)</summary> * `blueprint` (12-45) </details> <details> <summary>cards/Examples/1/5-battlecry.ts (1)</summary> * `blueprint` (13-66) </details> <details> <summary>cards/Examples/2/4-runes.ts (1)</summary> * `blueprint` (13-37) </details> </blockquote></details> <details> <summary>src/functions/util.ts (2)</summary><blockquote> <details> <summary>src/functions/info.ts (1)</summary> * `version` (5-15) </details> <details> <summary>src/commands.ts (1)</summary> * `version` (281-365) </details> </blockquote></details> <details> <summary>scripts/vanilla/generate.ts (3)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `createGame` (1803-1815) </details> <details> <summary>scripts/test/crash.ts (1)</summary> * `main` (21-107) </details> <details> <summary>hub.ts (1)</summary> * `main` (200-250) </details> </blockquote></details> <details> <summary>src/functions/card.ts (1)</summary><blockquote> <details> <summary>src/card.ts (1)</summary> * `Card` (40-2121) </details> </blockquote></details> <details> <summary>src/commands.ts (2)</summary><blockquote> <details> <summary>src/functions/interact.ts (1)</summary> * `target` (713-736) </details> <details> <summary>src/card.ts (1)</summary> * `Card` (40-2121) </details> </blockquote></details> <details> <summary>scripts/test/crash.ts (4)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `createGame` (1803-1815) </details> <details> <summary>hub.ts (1)</summary> * `main` (200-250) </details> <details> <summary>scripts/test/cards.ts (1)</summary> * `main` (22-135) </details> <details> <summary>scripts/vanilla/generate.ts (1)</summary> * `main` (45-80) </details> </blockquote></details> <details> <summary>src/player.ts (3)</summary><blockquote> <details> <summary>src/card.ts (1)</summary> * `Card` (40-2121) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `mulligan` (972-1008) </details> <details> <summary>src/ai.ts (1)</summary> * `mulligan` (599-627) </details> </blockquote></details> <details> <summary>scripts/test/cards.ts (6)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `createGame` (1803-1815) </details> <details> <summary>scripts/test/crash.ts (1)</summary> * `main` (21-107) </details> <details> <summary>tools/cardcreator/vanilla.ts (1)</summary> * `main` (179-280) </details> <details> <summary>hub.ts (1)</summary> * `main` (200-250) </details> <details> <summary>scripts/vanilla/generate.ts (1)</summary> * `main` (45-80) </details> <details> <summary>tools/cardcreator/class.ts (1)</summary> * `main` (145-225) </details> </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (3)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `BlueprintWithOptional` (422-425) </details> <details> <summary>src/card.ts (1)</summary> * `Card` (40-2121) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> </blockquote></details> <details> <summary>src/functions/interact.ts (4)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> <details> <summary>src/player.ts (1)</summary> * `Player` (21-1453) </details> <details> <summary>src/types/other.ts (2)</summary> * `TargetFlags` (88-93) * `Target` (124-124) </details> <details> <summary>src/commands.ts (2)</summary> * `helpColumns` (18-32) * `helpDebugColumns` (38-49) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (60)</summary><blockquote> <details> <summary>README.md (1)</summary><blockquote> `50-51`: **Spelling/wording fix looks good** The updated sentence reads naturally and accurately describes the HUB behavior; no further changes needed here. </blockquote></details> <details> <summary>cards/Classes/Neutral/Uncollectible/Spells/0-Cost/reign-of-chaos-2ef76c34.ts (1)</summary><blockquote> `59-61`: **Test target token aligns with enemy targeting** Using `owner.inputQueue = ["e0"]` matches the enemy-aligned target selection in this test and is consistent with the new input token pattern introduced elsewhere in the PR. </blockquote></details> <details> <summary>cards/Lackeys/goblin-4cde4d98.ts (1)</summary><blockquote> `63-65`: **Friendly target token matches test intent** Setting `owner.inputQueue = ["f0"]` correctly models selecting the friendly sheep as the Battlecry target under the new interaction scheme. </blockquote></details> <details> <summary>cards/StartingHeroes/Mage/heropower-4b71b3e6.ts (1)</summary><blockquote> `35-41`: **Hero-power target token matches face-damage assertion** The `["pe"]` input queue cleanly represents targeting the enemy hero for 1 damage and keeps the test aligned with the new prompt/token model. </blockquote></details> <details> <summary>cards/Examples/3/1-manathirst.ts (1)</summary><blockquote> `88-100`: **Consistent enemy-target tokens for both Manathirst cases** Using `owner.inputQueue = ["e0"]` in both the low-mana and Manathirst(6) branches keeps the test deterministic and correctly targets the enemy sheep in each run. </blockquote></details> <details> <summary>cards/Lackeys/witchy-39591f26.ts (1)</summary><blockquote> `96-99`: **Friendly transform target token is consistent** `owner.inputQueue = ["f0"]` correctly encodes selecting the friendly sheep as the Battlecry transform target and matches the token pattern used in other Lackey tests. </blockquote></details> <details> <summary>cards/StartingHeroes/Priest/heropower-7b3ef914.ts (1)</summary><blockquote> `34-45`: **Self-heal target token correctly represents friendly hero** Using `owner.inputQueue = ["pf"]` for both invocations cleanly drives the hero power to target the friendly hero, matching the intended 1→3 and 29→30 health assertions. </blockquote></details> <details> <summary>src/types/other.ts (1)</summary><blockquote> `190-199`: **New advanced flag is well placed and clearly named** `gameloopUseOldUserInterface: boolean` fits naturally under `advanced` and the naming makes its purpose obvious; assuming config defaults/docs were updated in config.ts/README, this type change looks good. </blockquote></details> <details> <summary>config.ts (1)</summary><blockquote> `251-256`: **New `gameloopUseOldUserInterface` flag is consistent with config structure** The new advanced flag is wired in the right place and matches the updated `GameConfig` shape. Behavior will depend on callers, but there’s nothing structurally unsafe here. </blockquote></details> <details> <summary>src/functions/card.ts (1)</summary><blockquote> `232-245`: **`getClasses` TODO is fine but semantics differ from `Object.values(Class)`** The TODO to “Replace this with `Object.values(Class)`” would change semantics: the current implementation only returns classes that have a `Tag.StartingHero` blueprint, while `Object.values(Class)` would include any defined class enum, even if there is no starting hero. If you ever act on the TODO, double‑check all call sites to ensure they truly want “all defined classes” instead of “implemented starting hero classes only”. </blockquote></details> <details> <summary>test/src/functions/card.test.ts (1)</summary><blockquote> `41-44`: **`readables` test TODO added appropriately** Adding a `test.todo("readables")` keeps the new helper on the testing radar and matches the existing pattern in this file. No issues here. </blockquote></details> <details> <summary>src/ai.ts (1)</summary><blockquote> `599-627`: **Card-based `mulligan()` return type lines up with the rest of the refactor** Returning `Card[]` from `mulligan()` and recording `[toMulligan, scores]` in history is a clean improvement over the previous index-string approach, and the threshold logic is unchanged. This should integrate well with a `Player.mulligan` implementation that takes cards instead of indices; just ensure all AI callers expect the new return type. </blockquote></details> <details> <summary>hub.ts (3)</summary><blockquote> `5-15`: **Menu-based `cardCreator` flow looks solid; double-check `ccc.main({})` signature** The new “Create a Card” menu with `select` + `Separator` is much clearer than the prior ad‑hoc input handling, and the vanilla‑card path correctly forces `vanilla.getAll()` up front so missing data is surfaced early. One thing to verify: `ccc.main({})` assumes the custom card creator’s `main` takes an options object. If its first parameter is still a boolean (e.g. a `debug` flag), this call changes behavior relative to `main()`/`main(false)`. Worth confirming against `tools/cardcreator/custom.ts`. Also applies to: 70-113 --- `118-198`: **`devmode` menu wiring is coherent and keeps responsibilities separated** The developer menu cleanly delegates to: - `cardCreator` for card flows, - `clc.main()` for class creation, - card/crash tests, and - vanilla generation + packager. The confirmation prompt before querying the vanilla API is a good addition, and looping with `select` until “Back” is chosen matches the overall UX of the new hub. No functional issues spotted here. --- `200-250`: **Top-level `main` loop using `select` simplifies navigation** The main hub menu (“Play / Create a Deck / Developer Options / Exit”) is straightforward and uses the same `select`/`Separator` pattern as the other flows. Delegation to `src.main()` and `dc.main()` preserves existing responsibilities, with interest logs around each action. Looks good from a control-flow perspective. </blockquote></details> <details> <summary>tools/deckcreator.ts (2)</summary><blockquote> `94-166`: **`generateDeckcode` + initial setup are correct; vanilla conversion guard is sensible** The new `generateDeckcode(parseVanillaOnPseudo)` correctly: - delegates to `deckcode.export(deck, player.heroClass, player.runes)`, - emits human-readable warnings based on `error.msg`, and - only converts to vanilla when either there’s no error or the error is recoverable (and `parseVanillaOnPseudo` is true). Initial state (`deck: Card[] = []`, `deckcodeFormat = JS`, `viewingClass = player.heroClass`) looks sane, and the TODO about rune-aware filtering is clearly marked. No correctness issues here. Also applies to: 196-212 --- `314-349`: **Command handlers are generally solid; undo/neutral/rules behavior looks correct** Highlights: - `neutral` correctly toggles between `player.heroClass` and `Class.Neutral` for filtering. - `undo` sensibly interprets `undoableCommandHistory` entries (`add`/`remove`) and replays the inverse action against the shared `cards` list. - `format` simply flips between JS and Vanilla deckcode output, which plays nicely with `generateDeckcode`. - `rules` prints out validation state and deck constraints in a user-friendly way and pauses before returning. These pieces together make the deck creator much easier to reason about compared to the previous ad‑hoc flow. Also applies to: 383-401, 405-474 </blockquote></details> <details> <summary>src/commands.ts (6)</summary><blockquote> `18-32`: **Exporting `helpColumns`/`helpDebugColumns` is fine and keeps help text centralized** Making the help column arrays exported is a clean way to share command docs elsewhere while keeping the definitions in one place. The updated wording (e.g., for `end`, `history`, `help`) reads clearly. No behavioral concerns here. Also applies to: 38-49 --- `141-149`: **Titan ability selection via `customSelect` and `readables` is well-structured** The new Titan flow: - Uses `targetCard` with a `shouldBeDisabled` predicate to ensure only friendly Titans with `attackTimes > 0` can be picked. - Materializes Titan abilities as temporary `Card` instances and feeds them through `game.functions.card.readables` into `customSelect`. - Correctly handles `"Back"` by early-returning. - Uses the parsed index to pick the chosen ability and then splices the corresponding Titan id from `titanIds`. This is a nice improvement over manual numeric input, and the bookkeeping (splicing `titanIds`, removing the keyword when empty, exhausting otherwise) remains correct. Also applies to: 160-184 --- `196-251`: **Help command formatting and pause behavior look good** Reusing the exported `helpColumns`/`helpDebugColumns`, aligning columns, and then adding a trailing `console.log()` plus `await game.pause("Press enter to continue...\n");` makes the help output easier to read and keeps it consistent with other commands. No functional issues here. --- `367-541`: **History command’s added blank line + pause improve UX without changing semantics** The structural logic of `history` is unchanged; the main behavioral change is: - Printing a blank line after the history text (`console.log();`), and - Pausing for input unless `flags.echo === false`. This matches the pause pattern used elsewhere (AI info, reload, etc.) and doesn’t affect the string returned when called programmatically. --- `548-577`: **Debug `give` now uses `customSelect` and `readables` correctly** For multiple matching cards: - You call `customSelect` with `readables(cards)`, so the user sees fully formatted card descriptions. - `"Back"` is honored by returning `false`. - Otherwise, the numeric choice is parsed and used as an index into `cards`. This is consistent with other `customSelect` usages and is a clear UX upgrade over raw index prompts. --- `674-677`: **RL and AI debug outputs: extra newlines + explicit pause are fine** Both `rl` and `ai` now: - Print a blank line before their final pause message. - Use a consistent “Press enter to continue…” prompt. These are cosmetic improvements only and don’t alter the underlying reload or AI-history behavior. Also applies to: 681-684, 759-762 </blockquote></details> <details> <summary>src/functions/info.ts (1)</summary><blockquote> `8-8`: **LGTM: Lodash parseInt replaced with native parsing.** The change to `parseInt(..., 10)` is correct and aligns with the broader codebase shift to explicit radix parsing. </blockquote></details> <details> <summary>src/functions/util.ts (1)</summary><blockquote> `274-276`: **LGTM: Consistent migration to native parseInt.** The replacement of lodash parseInt with `parseInt(..., 10)` for version and logVersion parsing is correct and consistent with the broader refactoring across the codebase. </blockquote></details> <details> <summary>test/src/functions/interact.test.ts (1)</summary><blockquote> `6-10`: **Test scaffolding expanded for new prompt helpers.** The addition of test.todo stubs for `customSelect`, `customSelectEnum`, `configureArray`, `configureArrayEnum`, and `configureObject` aligns with the new Inquirer.js-based prompt utilities introduced in this PR. These placeholders ensure the expanded prompt API surface is tracked for future test coverage. </blockquote></details> <details> <summary>cards/Lackeys/kobold-bf5c86d3.ts (1)</summary><blockquote> `47-47`: **LGTM: Test input simplified for new prompt system.** The change from `["face", "y"]` to `["pe"]` simplifies the test input sequence by removing the interactive confirmation step. This aligns with the broader test suite migration to the new Inquirer.js-based prompt system, where target selection uses predefined shorthand tokens. </blockquote></details> <details> <summary>tools/cardcreator/lib.ts (2)</summary><blockquote> `137-140`: **LGTM: Runes condition properly guards against empty arrays.** The tightened condition `blueprint.runes && blueprint.runes.length > 0` correctly prevents generating empty rune code when an empty array is present. This is more precise than a truthy check alone. --- `484-486`: **Blueprint registration now runs in debug mode—verify if intentional.** The code now pushes the blueprint to `game.blueprints` and regenerates the IDs file regardless of debug mode. Previously, this only happened in non-debug mode. If debug mode is intended to be side-effect-free (for testing/validation without modifying game state), this change introduces a breaking side effect. Please confirm whether debug mode should modify `game.blueprints` and regenerate the IDs file. If debug mode should remain side-effect-free, apply this diff: ```diff - // Update the ids so that `game.cardIds` is updated immediately. - game.blueprints.push(blueprint); - await game.functions.card.generateIdsFile(); + if (!debugMode) { + // Update the ids so that `game.cardIds` is updated immediately. + game.blueprints.push(blueprint); + await game.functions.card.generateIdsFile(); + } ``` </blockquote></details> <details> <summary>scripts/vanilla/generate.ts (1)</summary><blockquote> `15-17`: **LGTM: Guarded initialization and exported main improve testability.** The changes introduce a consistent pattern across script files: 1. Game initialization is guarded under `import.meta.main` to prevent side effects during imports. 2. The `main()` function is now exported, making it reusable when the script is imported as a module. 3. Exit behavior is conditional: exits when run as a script, pauses when imported. This pattern aligns with similar refactoring in `scripts/test/crash.ts` and `scripts/test/cards.ts`, improving script testability and composability. Also applies to: 45-45, 74-78, 82-84 </blockquote></details> <details> <summary>scripts/test/cards.ts (4)</summary><blockquote> `39-39`: **Good enhancement: Card ID now shown in progress output.** Including `card.id` in the progress message improves debuggability when a test hangs or fails. --- `129-134`: **LGTM: Conditional exit/pause pattern is consistent.** The `import.meta.main` guard for exit vs. pause aligns with the pattern used in `scripts/test/crash.ts` (lines 102-106) and `scripts/vanilla/generate.ts` (lines 72-76). --- `137-139`: **LGTM: Guarded main invocation.** Conditional execution of `main()` when run as the entry point is consistent with the refactored script patterns across the PR. --- `11-13`: [Your rewritten review comment text here] [Exactly ONE classification tag] </blockquote></details> <details> <summary>scripts/test/crash.ts (5)</summary><blockquote> `6-8`: **Same concern as cards.ts: Module-level code depends on `game`.** Lines 10 and 12 access `game.blueprints` and `Card.all(true)` at module level. When imported as a module (not main), `createGame()` is skipped, potentially causing failures if `game` isn't initialized elsewhere first. This is the same pattern issue noted in `scripts/test/cards.ts`. --- `15-16`: **Good simplification: Native parseInt replaces lodash.** Using native `parseInt(gamesEnv, 10)` with `Number.isNaN` check is cleaner and avoids the lodash dependency for this simple operation. --- `21-21`: **LGTM: Exporting `main()` enables module reuse.** Exporting the function allows other modules to invoke the crash test programmatically. --- `101-106`: **LGTM: Conditional exit/pause pattern.** Consistent with the refactored lifecycle pattern across test scripts. --- `109-111`: **LGTM: Guarded main invocation.** Matches the pattern in other refactored scripts. </blockquote></details> <details> <summary>tools/cardcreator/vanilla.ts (3)</summary><blockquote> `185-194`: **LGTM: Type filter prompt with back navigation.** The `customSelectEnum` usage for card type filtering with "Back" handling is clean and consistent with the new prompt-driven UI patterns. --- `214-252`: **LGTM: Search-based card selection.** The search prompt with filtering by name, cost, and type provides a good UX. The special entries for manual input (-2) and back (-1) are well-structured. --- `256-272`: **LGTM: Manual name/dbfId entry with validation.** The validation ensures the card exists before proceeding, making the non-null assertion at line 271 safe. </blockquote></details> <details> <summary>src/player.ts (3)</summary><blockquote> `998-1003`: **LGTM: New `canActuallyAttack()` predicate.** This method cleanly encapsulates the attack eligibility check, consolidating `canAttack`, `frozen`, and `attack > 0` conditions. This is a good abstraction that can be reused throughout the codebase. --- `1093-1119`: **LGTM: Mulligan refactored to accept `Card[]`.** The signature change from string-based input to `Card[]` aligns well with: - `AI.mulligan()` returning `Card[]` (src/ai.ts:598-626) - `interact.mulligan()` building `Card[]` from checkbox selection (src/functions/interact.ts:971-1007) The suppression of `DiscardCard`, `AddCardToDeck`, and `DrawCard` events during mulligan is appropriate since these are internal shuffle operations, not gameplay actions. --- `296-299`: **LGTM: Documentation updated for inputQueue behavior.** The doc comments now accurately reflect the `undefined` state when the queue is exhausted. </blockquote></details> <details> <summary>tools/packager.ts (3)</summary><blockquote> `148-187`: **LGTM: Pack import with update detection.** The `arrayTransform` callback elegantly handles showing update status and disabling already-imported packs. The async transform for reading metadata per-pack is well-structured. --- `528-558`: **LGTM: Main menu refactored to prompt-based loop.** The while loop with `select` prompt is cleaner than the previous manual input approach. The "Back" option correctly breaks the loop. --- `235-243`: **LGTM: Export pack selection.** Using `arrayTransform: undefined` explicitly documents that no transformation is needed for this simpler selection. </blockquote></details> <details> <summary>tools/cardcreator/class.ts (2)</summary><blockquote> `97-104`: **LGTM!** The number prompt with `required: true` ensures the cost is always set. The configuration loop with dirty tracking is well-implemented. --- `145-225`: **LGTM!** The main function has a clean flow: initialize blueprints → configure interactively → create files → guide user. The early return on cancel is handled correctly. </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (1)</summary><blockquote> `344-366`: **LGTM!** The main function cleanly integrates with the new `configure()` flow and properly handles the cancellation case. </blockquote></details> <details> <summary>src/card.ts (3)</summary><blockquote> `566-574`: **LGTM!** The new `forceUsingOwnBlueprint` parameter cleanly enables the card creator to use custom blueprints while maintaining backward compatibility with the default behavior. --- `635-637`: **LGTM!** The simplified `hasKeyword` implementation using `Object.keys().includes()` is cleaner and functionally equivalent. --- `1824-1849`: **LGTM!** The adapt method now uses the new prompt system cleanly. The random selection via `sampleSize` and the `customSelect` prompt provide a better user experience. </blockquote></details> <details> <summary>src/functions/interact.ts (6)</summary><blockquote> `83-171`: **LGTM!** The `customSelect` utility is well-designed with flexible options for array transformation, separators, and disabled states. The pattern of allowing `false` in `otherChoices` for conditional inclusion is elegant. --- `713-916`: **LGTM!** The target function refactoring is well-implemented. The `shouldBeDisabled` callback provides flexibility for context-specific target filtering while maintaining clean separation between choice building and post-selection validation. --- `972-1008`: **LGTM!** The mulligan function now uses the checkbox prompt for intuitive multi-selection. The exclusion of The Coin from mulligan options is correctly handled. --- `1079-1118`: **LGTM!** The discover function is cleanly refactored to use the new prompt system while preserving AI behavior and correctly returning a `perfectCopy()` of the selected card. --- `1850-1940`: **LGTM!** The new gameloop UI is well-designed with: - Backward compatibility via `gameloopUseOldUserInterface` config option - Organized command menu with proper debug command handling - "Type in" option for users who prefer the legacy interface The separation between card selection and command execution is clean. --- `930-950`: **LGTM!** The `useLocation` function properly uses the `shouldBeDisabled` callback to prevent selection of non-locations and locations on cooldown. The post-selection validation is good defensive programming. </blockquote></details> </blockquote></details> </details>

src/functions/interact.ts

					
				@ -69,0 +301,4 @@

								new Separator(),

								"Done",

							);

⚠️ Potential issue | 🟡 Minor

Missing await on async onLoop callback.

Unlike configureArray (line 203) and configureObject (line 388) which correctly use await onLoop?.(), this call is missing the await, which could cause timing issues with watermark rendering.

 		while (true) {
-			onLoop?.();
+			await onLoop?.();
 			console.log(JSON.stringify(array, null, 4));
 			console.log();

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

		while (true) {
			await onLoop?.();
			console.log(JSON.stringify(array, null, 4));
			console.log();

🤖 Prompt for AI Agents

In src/functions/interact.ts around lines 273-276, the onLoop callback is
invoked without awaiting, which can cause timing issues; change the call to
await onLoop?.() (same pattern used in configureArray and configureObject) so
the loop waits for any async work to complete before continuing; ensure the
enclosing function is async (or already is) so the await is valid.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟡 Minor_ **Missing `await` on async `onLoop` callback.** Unlike `configureArray` (line 203) and `configureObject` (line 388) which correctly use `await onLoop?.()`, this call is missing the `await`, which could cause timing issues with watermark rendering. ```diff while (true) { - onLoop?.(); + await onLoop?.(); console.log(JSON.stringify(array, null, 4)); console.log(); ```  <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 while (true) { await onLoop?.(); console.log(JSON.stringify(array, null, 4)); console.log(); ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/functions/interact.ts around lines 273-276, the onLoop callback is invoked without awaiting, which can cause timing issues; change the call to await onLoop?.() (same pattern used in configureArray and configureObject) so the loop waits for any async work to complete before continuing; ensure the enclosing function is async (or already is) so the await is valid. ``` </details>   ✅ Addressed in commit 837c778

tools/cardcreator/class.ts

⚠️ Potential issue | 🟡 Minor

Unclosed color tag in warning message.

The closing tag uses <yellow> instead of </yellow>, which may cause unintended color rendering.

-				message = parseTags(
-					"<yellow>You haven't changed the class name. Continue anyway?<yellow>",
-				);
+				message = parseTags(
+					"<yellow>You haven't changed the class name. Continue anyway?</yellow>",
+				);

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

				message = parseTags(
					"<yellow>You haven't changed the class name. Continue anyway?</yellow>",
				);
			}

🤖 Prompt for AI Agents

In tools/cardcreator/class.ts around lines 124 to 127, the warning string uses
an opening <yellow> tag but closes with <yellow> again; update the string to use
a proper closing tag (</yellow>) so the color tag is balanced — replace the
trailing "<yellow>" with "</yellow>" in the parseTags call.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟡 Minor_ **Unclosed color tag in warning message.** The closing tag uses `<yellow>` instead of `</yellow>`, which may cause unintended color rendering. ```diff - message = parseTags( - "<yellow>You haven't changed the class name. Continue anyway?<yellow>", - ); + message = parseTags( + "<yellow>You haven't changed the class name. Continue anyway?</yellow>", + ); ```  <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 message = parseTags( "<yellow>You haven't changed the class name. Continue anyway?</yellow>", ); } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/class.ts around lines 124 to 127, the warning string uses an opening <yellow> tag but closes with <yellow> again; update the string to use a proper closing tag (</yellow>) so the color tag is balanced — replace the trailing "<yellow>" with "</yellow>" in the parseTags call. ``` </details>   ✅ Addressed in commit 837c778

tools/cardcreator/custom.ts

					
				@ -35,0 +52,4 @@

					const card = await Card.create(

						game.cardIds.sheep_668b9054_7ca9_49af_9dd9_4f0126c6894c,

						game.player,

					);

⚠️ Potential issue | 🟠 Major

Logic errors in the set() helper function.

Two issues:

Line 51: (card.keywords as any)[key] when key === "keywords" sets card.keywords["keywords"] to undefined, which doesn't seem intentional.
Line 55: Array.isArray(typeof (card as any)[key]) is always false because typeof returns a string (e.g., "object"), not an array. The condition should check the value directly.

 	const set = (key: keyof typeof blueprint, value: any) => {
 		(blueprint as any)[key] = value;
 		dirty = true;
 
 		if (key === "keywords") {
-			(card.keywords as any)[key] = undefined;
+			// Clear keywords on card to force re-apply from blueprint
+			card.keywords = {};
 			return;
 		}
 
-		if (Array.isArray(typeof (card as any)[key])) {
+		if (Array.isArray((card as any)[key])) {
 			(card as any)[key].push(value);
 			return;
 		}
 
 		(card as any)[key] = value;
 	};

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 46 to 61, the set() helper has two
logic bugs: when key === "keywords" it currently does (card.keywords as
any)[key] = undefined which sets card.keywords["keywords"] instead of
clearing/replacing the keywords field, and the array check uses
Array.isArray(typeof (card as any)[key]) which is always false. Fix by assigning
the keywords field directly (e.g., (card as any).keywords = undefined or set to
the incoming value per intended behavior) and change the array check to
Array.isArray((card as any)[key]); when pushing, ensure the array exists
(initialize it to [] if undefined) before calling push.

_⚠️ Potential issue_ | _🟠 Major_ **Logic errors in the `set()` helper function.** Two issues: 1. **Line 51**: `(card.keywords as any)[key]` when `key === "keywords"` sets `card.keywords["keywords"]` to undefined, which doesn't seem intentional. 2. **Line 55**: `Array.isArray(typeof (card as any)[key])` is always `false` because `typeof` returns a string (e.g., `"object"`), not an array. The condition should check the value directly. ```diff const set = (key: keyof typeof blueprint, value: any) => { (blueprint as any)[key] = value; dirty = true; if (key === "keywords") { - (card.keywords as any)[key] = undefined; + // Clear keywords on card to force re-apply from blueprint + card.keywords = {}; return; } - if (Array.isArray(typeof (card as any)[key])) { + if (Array.isArray((card as any)[key])) { (card as any)[key].push(value); return; } (card as any)[key] = value; }; ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 46 to 61, the set() helper has two logic bugs: when key === "keywords" it currently does (card.keywords as any)[key] = undefined which sets card.keywords["keywords"] instead of clearing/replacing the keywords field, and the array check uses Array.isArray(typeof (card as any)[key]) which is always false. Fix by assigning the keywords field directly (e.g., (card as any).keywords = undefined or set to the incoming value per intended behavior) and change the array check to Array.isArray((card as any)[key]); when pushing, ensure the array exists (initialize it to [] if undefined) before calling push. ``` </details>

tools/cardcreator/custom.ts

					
				@ -160,0 +321,4 @@

						}

						delete blueprint[key];

					}

⚠️ Potential issue | 🟡 Minor

Unclosed color tag in warning message.

Same issue as in class.ts - the closing tag uses <yellow> instead of </yellow>.

-			if (!blueprint.name || blueprint.name === "CHANGE ME") {
-				message = parseTags(
-					"<yellow>You haven't changed the name. The game doesn't support cards with empty names. Continue anyway?<yellow>",
-				);
-			}
+			if (!blueprint.name || blueprint.name === "CHANGE ME") {
+				message = parseTags(
+					"<yellow>You haven't changed the name. The game doesn't support cards with empty names. Continue anyway?</yellow>",
+				);
+			}

📝 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 (!blueprint.name || blueprint.name === "CHANGE ME") {
				message = parseTags(
					"<yellow>You haven't changed the name. The game doesn't support cards with empty names. Continue anyway?</yellow>",
				);
			}

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 170 to 174, the warning message uses
an opening color tag "<yellow>" but mistakenly repeats "<yellow>" instead of the
proper closing tag "</yellow>"; update the string passed to parseTags to replace
the second "<yellow>" with "</yellow>" so the color tag is correctly closed.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟡 Minor_ **Unclosed color tag in warning message.** Same issue as in `class.ts` - the closing tag uses `<yellow>` instead of `</yellow>`. ```diff - if (!blueprint.name || blueprint.name === "CHANGE ME") { - message = parseTags( - "<yellow>You haven't changed the name. The game doesn't support cards with empty names. Continue anyway?<yellow>", - ); - } + if (!blueprint.name || blueprint.name === "CHANGE ME") { + message = parseTags( + "<yellow>You haven't changed the name. The game doesn't support cards with empty names. Continue anyway?</yellow>", + ); + } ```  <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 (!blueprint.name || blueprint.name === "CHANGE ME") { message = parseTags( "<yellow>You haven't changed the name. The game doesn't support cards with empty names. Continue anyway?</yellow>", ); } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 170 to 174, the warning message uses an opening color tag "<yellow>" but mistakenly repeats "<yellow>" instead of the proper closing tag "</yellow>"; update the string passed to parseTags to replace the second "<yellow>" with "</yellow>" so the color tag is correctly closed. ``` </details>   ✅ Addressed in commit 837c778

tools/cardcreator/custom.ts

⚠️ Potential issue | 🟡 Minor

Missing await on async doBlueprint call.

doBlueprint is an async function. Without await, the blueprint reset may not complete before the loop continues, potentially displaying stale card state.

 			// Force reset.
-			card.doBlueprint(false, true);
+			await card.doBlueprint(false, true);

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

			// Force reset.
			await card.doBlueprint(false, true);

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 240 to 242, the call to
card.doBlueprint(false, true) is missing an await so the async blueprint reset
may not finish before the loop continues; change the call to await
card.doBlueprint(false, true) and ensure the enclosing function is declared
async (or otherwise handle the returned promise) so the reset completes before
proceeding.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟡 Minor_ **Missing `await` on async `doBlueprint` call.** `doBlueprint` is an async function. Without `await`, the blueprint reset may not complete before the loop continues, potentially displaying stale card state. ```diff // Force reset. - card.doBlueprint(false, true); + await card.doBlueprint(false, true); ```  <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 // Force reset. await card.doBlueprint(false, true); ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 240 to 242, the call to card.doBlueprint(false, true) is missing an await so the async blueprint reset may not finish before the loop continues; change the call to await card.doBlueprint(false, true) and ensure the enclosing function is declared async (or otherwise handle the returned promise) so the reset completes before proceeding. ``` </details>   ✅ Addressed in commit 837c778

tools/cardcreator/vanilla.ts

⚠️ Potential issue | 🟡 Minor

Chalk tags won't render in inquirer prompt message.

The input() function from @inquirer/prompts doesn't automatically parse chalk tags like <red>...</red>. The message will display the raw tags.

Wrap the message with parseTags():

 cardClass = game.lodash.startCase(
   await input({
-    message:
-      "<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>",
+    message: parseTags(
+      "<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>",
+    ),
   }),
 ) as Class;

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

			await input({
				message: parseTags(
					"<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>",
				),
			}),
		) as Class;

🤖 Prompt for AI Agents

In tools/cardcreator/vanilla.ts around lines 71-75, the inquirer prompt message
uses raw chalk-like tags which won't be rendered; wrap the message string with
parseTags(...) when passing it to input() (e.g. message: parseTags("<red>Was not
able to find the class of this card.\nWhat is the class of this card? </red>")),
and add the appropriate import for parseTags from the project's tag-parsing
utility or library you use for chalk-style tags so the tags are converted to
ANSI before showing the prompt.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟡 Minor_ **Chalk tags won't render in inquirer prompt message.** The `input()` function from `@inquirer/prompts` doesn't automatically parse chalk tags like `<red>...</red>`. The message will display the raw tags. Wrap the message with `parseTags()`: ```diff cardClass = game.lodash.startCase( await input({ - message: - "<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>", + message: parseTags( + "<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>", + ), }), ) as Class; ```  <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 await input({ message: parseTags( "<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>", ), }), ) as Class; ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/vanilla.ts around lines 71-75, the inquirer prompt message uses raw chalk-like tags which won't be rendered; wrap the message string with parseTags(...) when passing it to input() (e.g. message: parseTags("<red>Was not able to find the class of this card.\nWhat is the class of this card? </red>")), and add the appropriate import for parseTags from the project's tag-parsing utility or library you use for chalk-style tags so the tags are converted to ANSI before showing the prompt. ``` </details>   ✅ Addressed in commit 837c778

tools/cardcreator/vanilla.ts

					
				@ -208,0 +244,4 @@

								return cards;

							},

							pageSize: 15,

						});

⚠️ Potential issue | 🟡 Minor

cardCost may be NaN when user types "unknown".

The validation at line 207 allows value.toLowerCase() === "unknown" to pass, but then parseInt at line 203 returns NaN. This NaN is assigned to cardCost and used in the filter at lines 222-223, where c.cost === cardCost will always be false when cardCost is NaN.

If "unknown" is meant to skip cost filtering, set cardCost to undefined instead:

 if (answer === "0") {
-  cardCost = parseInt(
-    await input({
-      message: "How much does the card cost?",
-      validate: (value) =>
-        value.toLowerCase() === "unknown" ||
-        !Number.isNaN(parseInt(value, 10)),
-    }),
-    10,
-  );
+  const costInput = await input({
+    message: "How much does the card cost?",
+    validate: (value) =>
+      value.toLowerCase() === "unknown" ||
+      !Number.isNaN(parseInt(value, 10)),
+  });
+  
+  if (costInput.toLowerCase() !== "unknown") {
+    cardCost = parseInt(costInput, 10);
+  }
 }

📝 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 answer = await game.prompt.customSelect(
			"Do you want to filter by cost?",
			["Type in cost", "Unknown (Slow)"],
		);
		let cardCost: number | undefined;

		if (answer === "0") {
			const costInput = await input({
				message: "How much does the card cost?",
				validate: (value) =>
					value.toLowerCase() === "unknown" ||
					!Number.isNaN(parseInt(value, 10)),
			});
			
			if (costInput.toLowerCase() !== "unknown") {
				cardCost = parseInt(costInput, 10);
			}
		}

🤖 Prompt for AI Agents

In tools/cardcreator/vanilla.ts around lines 196 to 212, the code parses the
user's input even when they type "unknown", which yields NaN and breaks the
later equality check; change the logic so that after prompting the input you
check if the trimmed/lowercased value === "unknown" and in that case assign
cardCost = undefined, otherwise parseInt the value (with base 10) and assign the
resulting number, and guard/convert NaN to undefined as a final fallback so cost
filtering is skipped when the user enters "unknown".

_⚠️ Potential issue_ | _🟡 Minor_ **`cardCost` may be `NaN` when user types "unknown".** The validation at line 207 allows `value.toLowerCase() === "unknown"` to pass, but then `parseInt` at line 203 returns `NaN`. This `NaN` is assigned to `cardCost` and used in the filter at lines 222-223, where `c.cost === cardCost` will always be `false` when `cardCost` is `NaN`. If "unknown" is meant to skip cost filtering, set `cardCost` to `undefined` instead: ```diff if (answer === "0") { - cardCost = parseInt( - await input({ - message: "How much does the card cost?", - validate: (value) => - value.toLowerCase() === "unknown" || - !Number.isNaN(parseInt(value, 10)), - }), - 10, - ); + const costInput = await input({ + message: "How much does the card cost?", + validate: (value) => + value.toLowerCase() === "unknown" || + !Number.isNaN(parseInt(value, 10)), + }); + + if (costInput.toLowerCase() !== "unknown") { + cardCost = parseInt(costInput, 10); + } } ```  <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 answer = await game.prompt.customSelect( "Do you want to filter by cost?", ["Type in cost", "Unknown (Slow)"], ); let cardCost: number | undefined; if (answer === "0") { const costInput = await input({ message: "How much does the card cost?", validate: (value) => value.toLowerCase() === "unknown" || !Number.isNaN(parseInt(value, 10)), }); if (costInput.toLowerCase() !== "unknown") { cardCost = parseInt(costInput, 10); } } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/vanilla.ts around lines 196 to 212, the code parses the user's input even when they type "unknown", which yields NaN and breaks the later equality check; change the logic so that after prompting the input you check if the trimmed/lowercased value === "unknown" and in that case assign cardCost = undefined, otherwise parseInt the value (with base 10) and assign the resulting number, and guard/convert NaN to undefined as a final fallback so cost filtering is skipped when the user enters "unknown". ``` </details>

tools/deckcreator.ts

					
				@ -680,4 +90,3 @@

				/**

				 * Generates a deckcode from the current deck.

				 *

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the deckcreator.ts file
fd -t f "deckcreator.ts" 

# Then read the askClass function around lines 35-62

Repository: LunarTides/Hearthstone.js

Length of output: 89

🏁 Script executed:

# Find deckcreator.ts
find . -name "deckcreator.ts" -type f

# Get file size first
wc -l tools/deckcreator.ts 2>/dev/null || echo "File not found at tools/deckcreator.ts"

Repository: LunarTides/Hearthstone.js

Length of output: 116

🏁 Script executed:

# List TypeScript files in tools directory
ls -la tools/*.ts 2>/dev/null | head -20

Repository: LunarTides/Hearthstone.js

Length of output: 207

🏁 Script executed:

# Find all TypeScript files named deckcreator
find . -name "*deckcreator*" -type f

Repository: LunarTides/Hearthstone.js

Length of output: 91

🏁 Script executed:

# Search for askClass function across TypeScript files
rg -t ts "askClass" --max-count 5

Repository: LunarTides/Hearthstone.js

Length of output: 262

🏁 Script executed:

# Also search for the specific pattern with customSelectEnum
rg -t ts "customSelectEnum" -B 2 -A 5 | head -50

Repository: LunarTides/Hearthstone.js

Length of output: 2755

🏁 Script executed:

# Read the full askClass function to see the complete context
sed -n '35,70p' tools/deckcreator.ts

Repository: LunarTides/Hearthstone.js

Length of output: 783

🏁 Script executed:

# Also check if there are other similar patterns in the file
rg -t ts "player\.(heroClass|.*) = await.*customSelectEnum" -A 3 tools/deckcreator.ts

Repository: LunarTides/Hearthstone.js

Length of output: 233

Avoid persisting "Back" as a hero class when cancelling askClass

The askClass function assigns directly to player.heroClass from customSelectEnum before checking for the "Back" option:

player.heroClass = await game.prompt.customSelectEnum<Class>(
	`What class do you want to choose?`,
	Object.values(Class).filter((c) => c !== Class.Neutral),
);
if (player.heroClass === ("Back" as Class)) {
	return undefined;
}

When a user selects "Back", player.heroClass is left as the invalid string "Back" instead of reverting to its previous value. This breaks the invariant that player.heroClass is always a valid Class enum value and can leak into downstream logic.

Use an intermediate variable to validate before assignment:

+	const chosen = await game.prompt.customSelectEnum<Class>(
-	player.heroClass = await game.prompt.customSelectEnum<Class>(
		`What class do you want to choose?`,
		Object.values(Class).filter((c) => c !== Class.Neutral),
	);
-	if (player.heroClass === ("Back" as Class)) {
+	if (chosen === ("Back" as Class)) {
		return undefined;
	}
+
+	player.heroClass = chosen;

🤖 Prompt for AI Agents

In tools/deckcreator.ts around lines 19 to 31, assigning the result of
customSelectEnum directly into player.heroClass allows the sentinel "Back"
string to be persisted as the hero class; instead, read the selection into a
temporary variable, check if it equals the "Back" option and return/cancel if
so, and only then assign the validated enum value to player.heroClass so the
player.heroClass invariant remains intact.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟠 Major_ <details> <summary>🧩 Analysis chain</summary> 🏁 Script executed: ```shell # First, let's find and examine the deckcreator.ts file fd -t f "deckcreator.ts" # Then read the askClass function around lines 35-62 ``` Repository: LunarTides/Hearthstone.js Length of output: 89 --- 🏁 Script executed: ```shell # Find deckcreator.ts find . -name "deckcreator.ts" -type f # Get file size first wc -l tools/deckcreator.ts 2>/dev/null || echo "File not found at tools/deckcreator.ts" ``` Repository: LunarTides/Hearthstone.js Length of output: 116 --- 🏁 Script executed: ```shell # List TypeScript files in tools directory ls -la tools/*.ts 2>/dev/null | head -20 ``` Repository: LunarTides/Hearthstone.js Length of output: 207 --- 🏁 Script executed: ```shell # Find all TypeScript files named deckcreator find . -name "*deckcreator*" -type f ``` Repository: LunarTides/Hearthstone.js Length of output: 91 --- 🏁 Script executed: ```shell # Search for askClass function across TypeScript files rg -t ts "askClass" --max-count 5 ``` Repository: LunarTides/Hearthstone.js Length of output: 262 --- 🏁 Script executed: ```shell # Also search for the specific pattern with customSelectEnum rg -t ts "customSelectEnum" -B 2 -A 5 | head -50 ``` Repository: LunarTides/Hearthstone.js Length of output: 2755 --- 🏁 Script executed: ```shell # Read the full askClass function to see the complete context sed -n '35,70p' tools/deckcreator.ts ``` Repository: LunarTides/Hearthstone.js Length of output: 783 --- 🏁 Script executed: ```shell # Also check if there are other similar patterns in the file rg -t ts "player\.(heroClass|.*) = await.*customSelectEnum" -A 3 tools/deckcreator.ts ``` Repository: LunarTides/Hearthstone.js Length of output: 233 --- </details> **Avoid persisting `"Back"` as a hero class when cancelling `askClass`** The `askClass` function assigns directly to `player.heroClass` from `customSelectEnum` before checking for the "Back" option: ```ts player.heroClass = await game.prompt.customSelectEnum<Class>( `What class do you want to choose?`, Object.values(Class).filter((c) => c !== Class.Neutral), ); if (player.heroClass === ("Back" as Class)) { return undefined; } ``` When a user selects "Back", `player.heroClass` is left as the invalid string `"Back"` instead of reverting to its previous value. This breaks the invariant that `player.heroClass` is always a valid `Class` enum value and can leak into downstream logic. Use an intermediate variable to validate before assignment: ```diff + const chosen = await game.prompt.customSelectEnum<Class>( - player.heroClass = await game.prompt.customSelectEnum<Class>( `What class do you want to choose?`, Object.values(Class).filter((c) => c !== Class.Neutral), ); - if (player.heroClass === ("Back" as Class)) { + if (chosen === ("Back" as Class)) { return undefined; } + + player.heroClass = chosen; ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/deckcreator.ts around lines 19 to 31, assigning the result of customSelectEnum directly into player.heroClass allows the sentinel "Back" string to be persisted as the hero class; instead, read the selection into a temporary variable, check if it equals the "Back" option and return/cancel if so, and only then assign the validated enum value to player.heroClass so the player.heroClass invariant remains intact. ``` </details>   ✅ Addressed in commit 837c778

tools/packager.ts

					
				@ -305,7 +260,7 @@ async function exportPack() {

							if (

⚠️ Potential issue | 🟡 Minor

Missing await on game.pause().

game.pause() is an async function, but await is missing. This could cause the continue to execute before the user sees the message.

-game.pause(
+await game.pause(
   "<yellow>That pack doesn't have a 'meta.jsonc' file.</yellow>",
 );

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

			await game.pause(
				"<yellow>That pack doesn't have a 'meta.jsonc' file.</yellow>",
			);
			continue;

🤖 Prompt for AI Agents

In tools/packager.ts around lines 263-266, game.pause(...) is called without
awaiting it, so the function may continue (and hit continue) before the pause
completes; change the call to await game.pause("<yellow>That pack doesn't have a
'meta.jsonc' file.</yellow>"); ensuring the enclosing function is async (or make
it async if not) so the await is valid.

✅ Addressed in commit 837c778

_⚠️ Potential issue_ | _🟡 Minor_ **Missing `await` on `game.pause()`.** `game.pause()` is an async function, but `await` is missing. This could cause the `continue` to execute before the user sees the message. ```diff -game.pause( +await game.pause( "<yellow>That pack doesn't have a 'meta.jsonc' file.</yellow>", ); ```  <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 await game.pause( "<yellow>That pack doesn't have a 'meta.jsonc' file.</yellow>", ); continue; ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/packager.ts around lines 263-266, game.pause(...) is called without awaiting it, so the function may continue (and hit continue) before the pause completes; change the call to await game.pause("<yellow>That pack doesn't have a 'meta.jsonc' file.</yellow>"); ensuring the enclosing function is async (or make it async if not) so the await is valid. ``` </details>   ✅ Addressed in commit 837c778

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

2025-12-11 20:32:21 +00:00

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

Actionable comments posted: 3

♻️ Duplicate comments (4)

tools/cardcreator/vanilla.ts (1)

203-207: Past issue resolved: number() prompt handles the cost filter correctly.

The @inquirer/prompts number() function returns number | undefined, so when the user enters nothing or an invalid value, cardCost will be undefined. The filter at lines 216-218 correctly handles this with cardCost === undefined to skip cost filtering.

tools/deckcreator.ts (1)

38-46: Past issue resolved: askClass now validates before assignment.

The function now correctly stores the selection in an intermediate variable chosen, checks for "Back", and only assigns to player.heroClass after validation. This prevents the invalid "Back" string from being persisted.

src/functions/interact.ts (1)

273-276: Past issue resolved: onLoop is now properly awaited.

The callback is now correctly awaited with await onLoop?.(), consistent with other similar patterns in the file.

tools/cardcreator/class.ts (1)

124-127: Past issue resolved: Color tag is now properly closed.

The closing tag now correctly uses </yellow> instead of <yellow>.

🧹 Nitpick comments (11)

tools/cardcreator/vanilla.ts (1)
262-266: Non-null assertion on find result may throw if card not found.

If the user enters a name/dbfId that passes validation but the subsequent find returns undefined (edge case with concurrent modifications or validation bug), the ! assertion will cause a runtime error.

Consider adding a safeguard:
-			dbfId = vanillaCards.find(
-				(c) =>
-					c.name.toLowerCase() === name.toLowerCase() ||
-					c.dbfId === parseInt(name, 10),
-			)!.dbfId;
+			const foundCard = vanillaCards.find(
+				(c) =>
+					c.name.toLowerCase() === name.toLowerCase() ||
+					c.dbfId === parseInt(name, 10),
+			);
+			if (!foundCard) {
+				continue;
+			}
+			dbfId = foundCard.dbfId;
tools/deckcreator.ts (3)
75-78: Unsafe any cast when applying deck settings.

The cast value as any bypasses type safety. Consider using a more specific type or adding runtime validation.
 for (const setting of Object.entries(card.deckSettings)) {
 	const [key, value] = setting;
-	game.config[key as keyof GameConfig] = value as any;
+	// Consider validating that key exists and value type matches
+	(game.config as Record<string, unknown>)[key] = value;
 }
209-212: TODO comment for rune filtering.

There's a TODO to add rune filtering in filterCards. Consider tracking this as a follow-up task.

Would you like me to open an issue to track implementing rune-based card filtering?

262-263: TODO comment for commands implementation.

Line 263 has a // TODO: Do. comment that seems incomplete.

Would you like me to help implement the missing functionality or clarify what needs to be done here?
src/card.ts (1)

2114-2120: TODO for keyword display formatting.

The comment notes that DivineShield should display as Divine Shield. This is a minor UX improvement.

Would you like me to implement the keyword name formatting to convert camelCase/PascalCase to space-separated words (e.g., DivineShield → Divine Shield)?
src/functions/interact.ts (2)
665-673: Unsafe as any cast for shouldBeDisabled callback.

The type casting shouldBeDisabled as any loses type safety. Consider using generics or union types to properly type the callback.
 async targetPlayer(
 	prompt: string,
 	card: Card | undefined,
 	flags: TargetFlags,
-	shouldBeDisabled?: (target: Player) => Promise<boolean>,
+	shouldBeDisabled?: (target: Target) => Promise<boolean>,
 ): Promise<Player | null> {
 	return (await this.target(
 		prompt,
 		card,
 		{
 			...flags,
 			targetType: TargetType.Player,
 		},
-		shouldBeDisabled as any,
+		shouldBeDisabled,
 	)) as Player | null;
 }
Also applies to: 687-695

776-833: Async shouldBeDisabled calls in choice construction may impact performance.

Each choice construction awaits shouldBeDisabled, which could slow down rendering if there are many targets. Consider batching these checks or using Promise.all for parallel execution.
+	// Pre-compute disabled states in parallel
+	const disabledStates = new Map<Target, boolean>();
+	const targets: Target[] = [];
+	
+	if (flags.alignment === undefined || flags.alignment === Alignment.Friendly) {
+		if (flags.targetType === undefined || flags.targetType === TargetType.Player) {
+			targets.push(game.player);
+		}
+		if (!flags.targetType || flags.targetType === TargetType.Card) {
+			targets.push(...game.player.board);
+		}
+	}
+	// ... similar for enemy side
+	
+	await Promise.all(
+		targets.map(async (t) => {
+			disabledStates.set(t, (await shouldBeDisabled?.(t)) ?? false);
+		})
+	);
tools/cardcreator/custom.ts (1)
312-312: Redundant assignment.

dirty = true is unnecessary here since set() already sets dirty = true on line 49.
-	dirty = true;
tools/packager.ts (3)

148-183: Import menu: customSelect + arrayTransform wiring looks correct

The customSelect call and arrayTransform correctly:

Surface already-imported packs as disabled with clear update text.

Use the index (i.toString()) as the value, which matches the subsequent parseInt usage.

Only minor thought: arrayTransform re‑reads each pack’s meta.jsonc on every render. That’s totally fine at current scale (and consistent with the file’s “not performant” note), but if you ever expect a lot of packs, pre-reading metadata into a map before calling customSelect would avoid repeated disk I/O.

235-243: Export menu + awaited pause look good; consider making sentinel options more explicit

The export customSelect call mirrors the import flow and is straightforward to follow.

Adding await to game.pause when meta.jsonc is missing ensures the user actually sees the warning before the loop continues, addressing the earlier async issue.

For readability, instead of relying on parseInt(answer, 10) yielding NaN for non-index answers (e.g., the “New” option), you might:

Treat sentinel options (“New”, “Refresh”, “Done”) via explicit values in customSelect (if supported) and branch on those values directly, then only parseInt when you know the answer is an index string.

That would make the “create new pack vs. use existing pack” path a bit more self-documenting without changing behavior.

Also applies to: 260-267

391-392: Metadata configuration: prompt options and dirty handling are solid (small DRY opportunity)

Bumping pageSize to 15 on the metadata and license selects is consistent with the rest of the new UI and should improve navigation for longer lists.

The new configureArray / configureObject calls for authors, links, and requires correctly capture the changed flag and then update dirty with dirty ||= changed, avoiding the short‑circuiting pitfall you noted with dirty ||= await ....

The proprietary‑license warning wrapped in parseTags is a nice UX improvement and doesn’t alter control flow.

Optional refinements:

The changed + dirty ||= changed pattern repeats three times; if it grows further, a tiny helper (e.g., async function markDirty(fn: () => Promise<boolean>) { if (await fn()) dirty = true; }) could reduce duplication.

Since dirty is module‑level state, configureMetadata is somewhat coupled to its caller. It’s fine for this CLI-style tool, but if you ever need to reuse configureMetadata elsewhere, consider scoping dirty inside the function (or returning a “was changed” flag) to make it more self-contained.

Not blockers; current code is correct and maintainable.

Also applies to: 455-456, 467-491, 510-514

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd2e5266cd and 837c7780ef.

📒 Files selected for processing (7)

src/card.ts (6 hunks)
src/functions/interact.ts (29 hunks)
tools/cardcreator/class.ts (2 hunks)
tools/cardcreator/custom.ts (2 hunks)
tools/cardcreator/vanilla.ts (3 hunks)
tools/deckcreator.ts (7 hunks)
tools/packager.ts (8 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

tools/cardcreator/vanilla.ts (2)

src/functions/interact.ts (1)

input (1648-1680)

src/game.ts (1)

input (1252-1262)

tools/deckcreator.ts (4)

src/types/other.ts (2)

GameConfig (137-220)

CommandList (108-114)

src/functions/interact.ts (1)

input (1648-1680)

src/commands.ts (1)

commands (51-545)

src/game.ts (1)

input (1252-1262)

tools/cardcreator/class.ts (1)

src/types/card.ts (1)

Blueprint (320-420)

tools/cardcreator/custom.ts (4)

src/types/card.ts (1)

BlueprintWithOptional (422-425)

src/card.ts (1)

Card (40-2124)

src/functions/interact.ts (1)

input (1648-1680)

src/game.ts (1)

input (1252-1262)

src/functions/interact.ts (2)

src/game.ts (1)

input (1252-1262)

src/types/other.ts (2)

TargetFlags (88-93)

Target (124-124)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: build

🔇 Additional comments (15)

tools/cardcreator/vanilla.ts (2)

71-76: Past issue resolved: parseTags now wraps the prompt message.

The chalk tags are now correctly wrapped with parseTags(), which will properly render the <red> tags in the inquirer prompt.

209-247: Well-structured search prompt implementation.

The search-based card selection flow is clean, with proper filtering by name, cost, and type. The use of parseTags for formatting card names and the inclusion of "Type name / dbfId" and "Back" options provides good UX.

tools/deckcreator.ts (1)

439-475: Well-implemented rules command with clear formatting.

The rules command provides useful information about deck validation settings and constraints. Good use of color formatting for readability.

src/card.ts (3)

566-574: Clean addition of forceUsingOwnBlueprint parameter.

The new parameter provides flexibility for blueprint resolution while maintaining backward compatibility with the default false value. The logic correctly falls back to this.blueprint when the blueprint isn't found in game.blueprints.

635-637: Simplified hasKeyword implementation is correct.

Using Object.keys(this.keywords).includes(keyword) correctly checks for keyword presence. This is functionally equivalent to the previous implementation.

1840-1849: Clean refactor of adapt to use customSelect.

The adapt method now uses the prompt-driven UI consistently with the rest of the codebase. The "Back" handling returning Card.REFUND correctly maintains the cancellation flow.

src/functions/interact.ts (3)

83-171: Well-designed customSelect prompt utility.

The function provides flexible array transformation, separator support, and a consistent "Back" option. The handling of various choice types (string, object, Separator, false) is comprehensive.

972-1008: Clean mulligan refactor with proper coin filtering.

The mulligan function now correctly filters out The Coin from being mulliganed (lines 984-988), uses checkbox for multi-select, and returns the array of mulliganed cards. The AI path is properly handled.

1853-1939: New gameloop UI provides improved user experience.

The new select-based interface with card list, commands menu, and type-in fallback is well-structured. The handling of debug commands with proper prefix prepending is correct.

tools/cardcreator/class.ts (2)

7-139: Well-structured interactive configuration flow.

The configure function provides a clean menu-driven interface with:

Dirty tracking for change detection

Confirmation prompts for cancel/done actions

Proper validation (e.g., no spaces in class name at line 69)

Warning when class name isn't changed

145-179: Clean integration of configuration flow with lib.create.

The main function properly initializes blueprints with placeholder values, delegates to configure, and only proceeds with lib.create if configuration completes successfully. The early return on !done prevents partial creation.

tools/cardcreator/custom.ts (3)

19-44: Clean blueprint initialization with sensible defaults.

The structure is well-organized with all type-specific fields included upfront, allowing the blacklist logic to filter them appropriately. The TODO comment for heropower support is noted.

46-50: Simplified set() helper addresses previous logic bugs.

The streamlined implementation correctly updates both the blueprint and card state. The previous issues with keyword handling and array type checking have been eliminated by this simplification.

333-355: Clean simplified entry point.

The main() function signature simplification and direct use of the configure flow aligns well with the PR objectives. The early return on cancel is handled correctly.

tools/packager.ts (1)

529-558: New main loop integrates packager cleanly into the HUB flow

The main function’s while (true) loop with a select-based menu and explicit “Back” option matches the new HUB-style interaction pattern:

hub.watermark() per iteration keeps the UI consistent with other tools.

Branching on "export" | "import" | "back" is clear, and both paths correctly await the async pack operations before re-rendering the menu.

No issues from a correctness or UX standpoint.

**Actionable comments posted: 3** <details> <summary>♻️ Duplicate comments (4)</summary><blockquote> <details> <summary>tools/cardcreator/vanilla.ts (1)</summary><blockquote> `203-207`: **Past issue resolved: `number()` prompt handles the cost filter correctly.** The `@inquirer/prompts` `number()` function returns `number | undefined`, so when the user enters nothing or an invalid value, `cardCost` will be `undefined`. The filter at lines 216-218 correctly handles this with `cardCost === undefined` to skip cost filtering. </blockquote></details> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> `38-46`: **Past issue resolved: `askClass` now validates before assignment.** The function now correctly stores the selection in an intermediate variable `chosen`, checks for "Back", and only assigns to `player.heroClass` after validation. This prevents the invalid "Back" string from being persisted. </blockquote></details> <details> <summary>src/functions/interact.ts (1)</summary><blockquote> `273-276`: **Past issue resolved: `onLoop` is now properly awaited.** The callback is now correctly awaited with `await onLoop?.()`, consistent with other similar patterns in the file. </blockquote></details> <details> <summary>tools/cardcreator/class.ts (1)</summary><blockquote> `124-127`: **Past issue resolved: Color tag is now properly closed.** The closing tag now correctly uses `</yellow>` instead of `<yellow>`. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (11)</summary><blockquote> <details> <summary>tools/cardcreator/vanilla.ts (1)</summary><blockquote> `262-266`: **Non-null assertion on `find` result may throw if card not found.** If the user enters a name/dbfId that passes validation but the subsequent `find` returns `undefined` (edge case with concurrent modifications or validation bug), the `!` assertion will cause a runtime error. Consider adding a safeguard: ```diff - dbfId = vanillaCards.find( - (c) => - c.name.toLowerCase() === name.toLowerCase() || - c.dbfId === parseInt(name, 10), - )!.dbfId; + const foundCard = vanillaCards.find( + (c) => + c.name.toLowerCase() === name.toLowerCase() || + c.dbfId === parseInt(name, 10), + ); + if (!foundCard) { + continue; + } + dbfId = foundCard.dbfId; ``` </blockquote></details> <details> <summary>tools/deckcreator.ts (3)</summary><blockquote> `75-78`: **Unsafe `any` cast when applying deck settings.** The cast `value as any` bypasses type safety. Consider using a more specific type or adding runtime validation. ```diff for (const setting of Object.entries(card.deckSettings)) { const [key, value] = setting; - game.config[key as keyof GameConfig] = value as any; + // Consider validating that key exists and value type matches + (game.config as Record<string, unknown>)[key] = value; } ``` --- `209-212`: **TODO comment for rune filtering.** There's a TODO to add rune filtering in `filterCards`. Consider tracking this as a follow-up task. Would you like me to open an issue to track implementing rune-based card filtering? --- `262-263`: **TODO comment for commands implementation.** Line 263 has a `// TODO: Do.` comment that seems incomplete. Would you like me to help implement the missing functionality or clarify what needs to be done here? </blockquote></details> <details> <summary>src/card.ts (1)</summary><blockquote> `2114-2120`: **TODO for keyword display formatting.** The comment notes that `DivineShield` should display as `Divine Shield`. This is a minor UX improvement. Would you like me to implement the keyword name formatting to convert camelCase/PascalCase to space-separated words (e.g., `DivineShield` → `Divine Shield`)? </blockquote></details> <details> <summary>src/functions/interact.ts (2)</summary><blockquote> `665-673`: **Unsafe `as any` cast for `shouldBeDisabled` callback.** The type casting `shouldBeDisabled as any` loses type safety. Consider using generics or union types to properly type the callback. ```diff async targetPlayer( prompt: string, card: Card | undefined, flags: TargetFlags, - shouldBeDisabled?: (target: Player) => Promise<boolean>, + shouldBeDisabled?: (target: Target) => Promise<boolean>, ): Promise<Player | null> { return (await this.target( prompt, card, { ...flags, targetType: TargetType.Player, }, - shouldBeDisabled as any, + shouldBeDisabled, )) as Player | null; } ``` Also applies to: 687-695 --- `776-833`: **Async `shouldBeDisabled` calls in choice construction may impact performance.** Each choice construction awaits `shouldBeDisabled`, which could slow down rendering if there are many targets. Consider batching these checks or using `Promise.all` for parallel execution. ```diff + // Pre-compute disabled states in parallel + const disabledStates = new Map<Target, boolean>(); + const targets: Target[] = []; + + if (flags.alignment === undefined || flags.alignment === Alignment.Friendly) { + if (flags.targetType === undefined || flags.targetType === TargetType.Player) { + targets.push(game.player); + } + if (!flags.targetType || flags.targetType === TargetType.Card) { + targets.push(...game.player.board); + } + } + // ... similar for enemy side + + await Promise.all( + targets.map(async (t) => { + disabledStates.set(t, (await shouldBeDisabled?.(t)) ?? false); + }) + ); ``` </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (1)</summary><blockquote> `312-312`: **Redundant assignment.** `dirty = true` is unnecessary here since `set()` already sets `dirty = true` on line 49. ```diff - dirty = true; ``` </blockquote></details> <details> <summary>tools/packager.ts (3)</summary><blockquote> `148-183`: **Import menu: customSelect + arrayTransform wiring looks correct** The `customSelect` call and `arrayTransform` correctly: - Surface already-imported packs as disabled with clear update text. - Use the index (`i.toString()`) as the value, which matches the subsequent `parseInt` usage. Only minor thought: `arrayTransform` re‑reads each pack’s `meta.jsonc` on every render. That’s totally fine at current scale (and consistent with the file’s “not performant” note), but if you ever expect a lot of packs, pre-reading metadata into a map before calling `customSelect` would avoid repeated disk I/O. --- `235-243`: **Export menu + awaited pause look good; consider making sentinel options more explicit** - The export `customSelect` call mirrors the import flow and is straightforward to follow. - Adding `await` to `game.pause` when `meta.jsonc` is missing ensures the user actually sees the warning before the loop continues, addressing the earlier async issue. For readability, instead of relying on `parseInt(answer, 10)` yielding `NaN` for non-index answers (e.g., the “New” option), you might: - Treat sentinel options (“New”, “Refresh”, “Done”) via explicit values in `customSelect` (if supported) and branch on those values directly, then only `parseInt` when you know the answer is an index string. That would make the “create new pack vs. use existing pack” path a bit more self-documenting without changing behavior. Also applies to: 260-267 --- `391-392`: **Metadata configuration: prompt options and `dirty` handling are solid (small DRY opportunity)** - Bumping `pageSize` to 15 on the metadata and license selects is consistent with the rest of the new UI and should improve navigation for longer lists. - The new `configureArray` / `configureObject` calls for `authors`, `links`, and `requires` correctly capture the `changed` flag and then update `dirty` with `dirty ||= changed`, avoiding the short‑circuiting pitfall you noted with `dirty ||= await ...`. - The proprietary‑license warning wrapped in `parseTags` is a nice UX improvement and doesn’t alter control flow. Optional refinements: - The `changed` + `dirty ||= changed` pattern repeats three times; if it grows further, a tiny helper (e.g., `async function markDirty(fn: () => Promise<boolean>) { if (await fn()) dirty = true; }`) could reduce duplication. - Since `dirty` is module‑level state, `configureMetadata` is somewhat coupled to its caller. It’s fine for this CLI-style tool, but if you ever need to reuse `configureMetadata` elsewhere, consider scoping `dirty` inside the function (or returning a “was changed” flag) to make it more self-contained. Not blockers; current code is correct and maintainable. Also applies to: 455-456, 467-491, 510-514 </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 fd2e5266cd4796f0683f093b09fa3b5f8e2a7e84 and 837c7780ef505d258eba071fee5bbdbc79784af2. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `src/card.ts` (6 hunks) * `src/functions/interact.ts` (29 hunks) * `tools/cardcreator/class.ts` (2 hunks) * `tools/cardcreator/custom.ts` (2 hunks) * `tools/cardcreator/vanilla.ts` (3 hunks) * `tools/deckcreator.ts` (7 hunks) * `tools/packager.ts` (8 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (5)</summary> <details> <summary>tools/cardcreator/vanilla.ts (2)</summary><blockquote> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1648-1680) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> </blockquote></details> <details> <summary>tools/deckcreator.ts (4)</summary><blockquote> <details> <summary>src/types/other.ts (2)</summary> * `GameConfig` (137-220) * `CommandList` (108-114) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1648-1680) </details> <details> <summary>src/commands.ts (1)</summary> * `commands` (51-545) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> </blockquote></details> <details> <summary>tools/cardcreator/class.ts (1)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `Blueprint` (320-420) </details> </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (4)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `BlueprintWithOptional` (422-425) </details> <details> <summary>src/card.ts (1)</summary> * `Card` (40-2124) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1648-1680) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> </blockquote></details> <details> <summary>src/functions/interact.ts (2)</summary><blockquote> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> <details> <summary>src/types/other.ts (2)</summary> * `TargetFlags` (88-93) * `Target` (124-124) </details> </blockquote></details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: build </details> <details> <summary>🔇 Additional comments (15)</summary><blockquote> <details> <summary>tools/cardcreator/vanilla.ts (2)</summary><blockquote> `71-76`: **Past issue resolved: `parseTags` now wraps the prompt message.** The chalk tags are now correctly wrapped with `parseTags()`, which will properly render the `<red>` tags in the inquirer prompt. --- `209-247`: **Well-structured search prompt implementation.** The search-based card selection flow is clean, with proper filtering by name, cost, and type. The use of `parseTags` for formatting card names and the inclusion of "Type name / dbfId" and "Back" options provides good UX. </blockquote></details> <details> <summary>tools/deckcreator.ts (1)</summary><blockquote> `439-475`: **Well-implemented `rules` command with clear formatting.** The rules command provides useful information about deck validation settings and constraints. Good use of color formatting for readability. </blockquote></details> <details> <summary>src/card.ts (3)</summary><blockquote> `566-574`: **Clean addition of `forceUsingOwnBlueprint` parameter.** The new parameter provides flexibility for blueprint resolution while maintaining backward compatibility with the default `false` value. The logic correctly falls back to `this.blueprint` when the blueprint isn't found in `game.blueprints`. --- `635-637`: **Simplified `hasKeyword` implementation is correct.** Using `Object.keys(this.keywords).includes(keyword)` correctly checks for keyword presence. This is functionally equivalent to the previous implementation. --- `1840-1849`: **Clean refactor of `adapt` to use `customSelect`.** The adapt method now uses the prompt-driven UI consistently with the rest of the codebase. The "Back" handling returning `Card.REFUND` correctly maintains the cancellation flow. </blockquote></details> <details> <summary>src/functions/interact.ts (3)</summary><blockquote> `83-171`: **Well-designed `customSelect` prompt utility.** The function provides flexible array transformation, separator support, and a consistent "Back" option. The handling of various choice types (string, object, Separator, false) is comprehensive. --- `972-1008`: **Clean mulligan refactor with proper coin filtering.** The mulligan function now correctly filters out The Coin from being mulliganed (lines 984-988), uses checkbox for multi-select, and returns the array of mulliganed cards. The AI path is properly handled. --- `1853-1939`: **New gameloop UI provides improved user experience.** The new select-based interface with card list, commands menu, and type-in fallback is well-structured. The handling of debug commands with proper prefix prepending is correct. </blockquote></details> <details> <summary>tools/cardcreator/class.ts (2)</summary><blockquote> `7-139`: **Well-structured interactive configuration flow.** The `configure` function provides a clean menu-driven interface with: - Dirty tracking for change detection - Confirmation prompts for cancel/done actions - Proper validation (e.g., no spaces in class name at line 69) - Warning when class name isn't changed --- `145-179`: **Clean integration of configuration flow with lib.create.** The main function properly initializes blueprints with placeholder values, delegates to `configure`, and only proceeds with `lib.create` if configuration completes successfully. The early return on `!done` prevents partial creation. </blockquote></details> <details> <summary>tools/cardcreator/custom.ts (3)</summary><blockquote> `19-44`: **Clean blueprint initialization with sensible defaults.** The structure is well-organized with all type-specific fields included upfront, allowing the blacklist logic to filter them appropriately. The TODO comment for heropower support is noted. --- `46-50`: **Simplified set() helper addresses previous logic bugs.** The streamlined implementation correctly updates both the blueprint and card state. The previous issues with keyword handling and array type checking have been eliminated by this simplification. --- `333-355`: **Clean simplified entry point.** The `main()` function signature simplification and direct use of the configure flow aligns well with the PR objectives. The early return on cancel is handled correctly. </blockquote></details> <details> <summary>tools/packager.ts (1)</summary><blockquote> `529-558`: **New `main` loop integrates packager cleanly into the HUB flow** The `main` function’s `while (true)` loop with a `select`-based menu and explicit “Back” option matches the new HUB-style interaction pattern: - `hub.watermark()` per iteration keeps the UI consistent with other tools. - Branching on `"export" | "import" | "back"` is clear, and both paths correctly await the async pack operations before re-rendering the menu. No issues from a correctness or UX standpoint. </blockquote></details> </blockquote></details> </details>

tools/cardcreator/class.ts

					
				@ -5,0 +131,4 @@

								default: false,

							});

							if (done) {

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

@inquirer/prompts number function required option TypeScript types

💡 Result:

Short answer — the number prompt options are typed roughly like this:

type NumberValidate = (value: number | undefined) => boolean | string | Promise<boolean | string>;

interface NumberOptions {
message: string;
default?: number;
min?: number;
max?: number;
step?: number | 'any';
required?: boolean; // defaults to false
validate?: NumberValidate;
theme?: unknown;
}

Usage example:

import { number } from '@inquirer/prompts';

const answer = await number({
message: 'Enter a value',
required: true, // prevents empty (undefined) answers
validate: (v) => (v === undefined ? 'Required' : v >= 0 || 'Must be >= 0'),
});

Sources: @inquirer/number/@inquirer/prompts docs (options table, including required and validate signatures). [1][2]

🌐 Web query:

@inquirer/prompts number function return type Promise signature

💡 Result:

Signature (TypeScript, concise):

number(options?: NumberOptions): Promise<number | undefined> & { cancel(): void }.

Notes: the prompt returns a cancelable Promise (callable .cancel() to reject/cleanup). Source: @inquirer/prompts / @inquirer/number docs. [1][2]

Sources
[1] @inquirer/number (npm) — options/validate docs.
[2] @inquirer/prompts (package README / docs) — “All prompt functions are returning a cancelable promise.”

number() still returns number | undefined even with required: true.

The @inquirer/prompts number() function returns Promise<number | undefined> regardless of the required option. The required: true setting only prevents users from submitting empty values in the UI but does not narrow the TypeScript return type. The assignment to heropowerBlueprint.cost could therefore receive undefined, creating a type safety issue.

🤖 Prompt for AI Agents

In tools/cardcreator/class.ts around lines 97 to 102, the call to number(...) is
typed as Promise<number | undefined> even with required: true, so assigning it
directly to heropowerBlueprint.cost can introduce undefined; capture the result
in a local const, check if it is undefined and handle that case (re-prompt,
throw a clear error or exit), and only assign the confirmed number to
heropowerBlueprint.cost (alternatively use a non-null assertion only if you are
certain the prompt cannot return undefined at runtime).

_⚠️ Potential issue_ | _🔴 Critical_ <details> <summary>🧩 Analysis chain</summary> 🌐 Web query: `@inquirer/prompts number function required option TypeScript types` 💡 Result: Short answer — the number prompt options are typed roughly like this: type NumberValidate = (value: number | undefined) => boolean | string | Promise<boolean | string>; interface NumberOptions { message: string; default?: number; min?: number; max?: number; step?: number | 'any'; required?: boolean; // defaults to false validate?: NumberValidate; theme?: unknown; } Usage example: import { number } from '@inquirer/prompts'; const answer = await number({ message: 'Enter a value', required: true, // prevents empty (undefined) answers validate: (v) => (v === undefined ? 'Required' : v >= 0 || 'Must be >= 0'), }); Sources: @inquirer/number/@inquirer/prompts docs (options table, including required and validate signatures). [1][2] --- 🌐 Web query: `@inquirer/prompts number function return type Promise signature` 💡 Result: Signature (TypeScript, concise): - number(options?: NumberOptions): Promise<number | undefined> & { cancel(): void }. Notes: the prompt returns a cancelable Promise (callable .cancel() to reject/cleanup). Source: @inquirer/prompts / @inquirer/number docs. [1][2] Sources [1] @inquirer/number (npm) — options/validate docs. [2] @inquirer/prompts (package README / docs) — “All prompt functions are returning a cancelable promise.” --- </details> **`number()` still returns `number | undefined` even with `required: true`.** The `@inquirer/prompts` `number()` function returns `Promise<number | undefined>` regardless of the `required` option. The `required: true` setting only prevents users from submitting empty values in the UI but does not narrow the TypeScript return type. The assignment to `heropowerBlueprint.cost` could therefore receive `undefined`, creating a type safety issue. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/class.ts around lines 97 to 102, the call to number(...) is typed as Promise<number | undefined> even with required: true, so assigning it directly to heropowerBlueprint.cost can introduce undefined; capture the result in a local const, check if it is undefined and handle that case (re-prompt, throw a clear error or exit), and only assign the confirmed number to heropowerBlueprint.cost (alternatively use a non-null assertion only if you are certain the prompt cannot return undefined at runtime). ``` </details>

tools/cardcreator/custom.ts

⚠️ Potential issue | 🔴 Critical

Critical bug: Boolean("false") always returns true.

The variable newRarity is a string ("true" or "false"), and Boolean("false") evaluates to true because any non-empty string is truthy. This means selecting "False" will incorrectly set the value to true.

-		const newRarity = await game.prompt.customSelect(
+		const booleanChoice = await game.prompt.customSelect(
 			message,
 			["True", "False"],
 			{
 				arrayTransform: async (i, element) => ({
 					name: element,
 					value: element.toLowerCase(),
 				}),
 				hideBack: false,
 			},
 		);

-		set(key, Boolean(newRarity));
+		set(key, booleanChoice === "true");
 		continue;

📝 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 (typeof value === "boolean") {
			const booleanChoice = await game.prompt.customSelect(
				message,
				["True", "False"],
				{
					arrayTransform: async (i, element) => ({
						name: element,
						value: element.toLowerCase(),
					}),
					hideBack: false,
				},
			);

			set(key, booleanChoice === "true");
			continue;
		}

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 277-292, the code uses
Boolean(newRarity) which converts any non-empty string (including "false") to
true; change the assignment to evaluate the string explicitly (e.g., set(key,
newRarity === "true")) so selecting "False" stores false; keep the
arrayTransform producing "true"/"false" strings or adjust it to return a boolean
directly and use that boolean when calling set.

✅ Addressed in commit 5b5bc74

_⚠️ Potential issue_ | _🔴 Critical_ **Critical bug: `Boolean("false")` always returns `true`.** The variable `newRarity` is a string ("true" or "false"), and `Boolean("false")` evaluates to `true` because any non-empty string is truthy. This means selecting "False" will incorrectly set the value to `true`. ```diff - const newRarity = await game.prompt.customSelect( + const booleanChoice = await game.prompt.customSelect( message, ["True", "False"], { arrayTransform: async (i, element) => ({ name: element, value: element.toLowerCase(), }), hideBack: false, }, ); - set(key, Boolean(newRarity)); + set(key, booleanChoice === "true"); continue; ```  <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 (typeof value === "boolean") { const booleanChoice = await game.prompt.customSelect( message, ["True", "False"], { arrayTransform: async (i, element) => ({ name: element, value: element.toLowerCase(), }), hideBack: false, }, ); set(key, booleanChoice === "true"); continue; } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 277-292, the code uses Boolean(newRarity) which converts any non-empty string (including "false") to true; change the assignment to evaluate the string explicitly (e.g., set(key, newRarity === "true")) so selecting "False" stores false; keep the arrayTransform producing "true"/"false" strings or adjust it to return a boolean directly and use that boolean when calling set. ``` </details>   ✅ Addressed in commit 5b5bc74

tools/cardcreator/custom.ts

⚠️ Potential issue | 🟡 Minor

Validation function and JSON.parse have issues.

Line 298: typeof value === "number" is always false since input() returns a string. The validation never triggers.
Line 309: JSON.parse(newValue) can throw on malformed input with no error handling.

 	const newValue = await input({
 		message: "What will you change this value to?",
 		default: value?.toString(),
 		validate: (value) => {
-			if (typeof value === "number") {
-				return !Number.isNaN(parseInt(value, 10));
+			if (typeof blueprint[key] === "number") {
+				const parsed = parseInt(value, 10);
+				if (Number.isNaN(parsed)) {
+					return "Please enter a valid number";
+				}
 			}
 
 			return true;
 		},
 	});

 	if (typeof value === "string") {
 		set(key, newValue);
 	} else {
-		set(key, JSON.parse(newValue));
+		try {
+			set(key, JSON.parse(newValue));
+		} catch {
+			// For numbers, parseInt is sufficient
+			set(key, parseInt(newValue, 10));
+		}
 	}

📝 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 newValue = await input({
			message: "What will you change this value to?",
			default: value?.toString(),
			validate: (value) => {
				if (typeof blueprint[key] === "number") {
					const parsed = parseInt(value, 10);
					if (Number.isNaN(parsed)) {
						return "Please enter a valid number";
					}
				}

				return true;
			},
		});

		if (typeof value === "string") {
			set(key, newValue);
		} else {
			try {
				set(key, JSON.parse(newValue));
			} catch {
				// For numbers, parseInt is sufficient
				set(key, parseInt(newValue, 10));
			}
		}

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 294 to 310, the input validation
incorrectly checks typeof value === "number" (always false because input()
returns a string) and JSON.parse(newValue) can throw on invalid JSON; change the
validate callback to inspect the original value (the outer variable) and, if
that original value is a number, validate the entered string/newValue is a valid
numeric string (e.g., !Number.isNaN(Number(newValue)) or a regex) so the
validation actually runs, and surround the JSON.parse usage with try/catch to
handle malformed JSON (e.g., show an error and re-prompt or avoid crashing by
falling back or returning) so parsing errors are handled gracefully.

_⚠️ Potential issue_ | _🟡 Minor_ **Validation function and JSON.parse have issues.** 1. Line 298: `typeof value === "number"` is always false since `input()` returns a string. The validation never triggers. 2. Line 309: `JSON.parse(newValue)` can throw on malformed input with no error handling. ```diff const newValue = await input({ message: "What will you change this value to?", default: value?.toString(), validate: (value) => { - if (typeof value === "number") { - return !Number.isNaN(parseInt(value, 10)); + if (typeof blueprint[key] === "number") { + const parsed = parseInt(value, 10); + if (Number.isNaN(parsed)) { + return "Please enter a valid number"; + } } return true; }, }); if (typeof value === "string") { set(key, newValue); } else { - set(key, JSON.parse(newValue)); + try { + set(key, JSON.parse(newValue)); + } catch { + // For numbers, parseInt is sufficient + set(key, parseInt(newValue, 10)); + } } ```  <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 newValue = await input({ message: "What will you change this value to?", default: value?.toString(), validate: (value) => { if (typeof blueprint[key] === "number") { const parsed = parseInt(value, 10); if (Number.isNaN(parsed)) { return "Please enter a valid number"; } } return true; }, }); if (typeof value === "string") { set(key, newValue); } else { try { set(key, JSON.parse(newValue)); } catch { // For numbers, parseInt is sufficient set(key, parseInt(newValue, 10)); } } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 294 to 310, the input validation incorrectly checks typeof value === "number" (always false because input() returns a string) and JSON.parse(newValue) can throw on invalid JSON; change the validate callback to inspect the original value (the outer variable) and, if that original value is a number, validate the entered string/newValue is a valid numeric string (e.g., !Number.isNaN(Number(newValue)) or a regex) so the validation actually runs, and surround the JSON.parse usage with try/catch to handle malformed JSON (e.g., show an error and re-prompt or avoid crashing by falling back or returning) so parsing errors are handled gracefully. ``` </details>

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

2025-12-11 20:48:18 +00:00

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 837c7780ef and 5b5bc74dfea3c19ec9f6d3018a5c2ed9e282e103.

📒 Files selected for processing (1)

tools/cardcreator/custom.ts (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tools/cardcreator/custom.ts (3)

src/types/card.ts (1)

BlueprintWithOptional (422-425)

src/functions/interact.ts (1)

input (1648-1680)

src/game.ts (1)

input (1252-1262)

🔇 Additional comments (6)

tools/cardcreator/custom.ts (6)

46-50: LGTM - Simplified helper function.

The set() helper has been simplified from the previous version, removing the problematic keywords and array handling logic that was flagged in earlier reviews. The current implementation correctly updates both the blueprint and card objects.

52-63: LGTM - Card initialization with proper async handling.

The await on doBlueprint is now correctly applied (addressing a previous review concern). The unconventional direct blueprint assignment is acknowledged with a comment.

159-163: LGTM - Color tag properly closed.

The yellow warning message now correctly uses </yellow> as the closing tag, addressing the issue from a previous review.

186-234: LGTM - Array field handling with proper configuration.

The array handling correctly maps each field to its enum type with appropriate constraints (e.g., runes limited to 3 with duplicates allowed). The await on doBlueprint is correct, and the dirty ||= changed pattern properly tracks modifications.

277-292: LGTM - Boolean comparison fixed.

The previous bug where Boolean("false") would evaluate to true has been fixed. The code now correctly uses booleanChoice === "true" for proper boolean conversion.

337-359: LGTM - Simplified main function.

The main() function has been streamlined with a cleaner signature and proper integration with the new configure() flow. Error handling for user cancellation is correct.

**Actionable comments posted: 1** <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 837c7780ef505d258eba071fee5bbdbc79784af2 and 5b5bc74dfea3c19ec9f6d3018a5c2ed9e282e103. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `tools/cardcreator/custom.ts` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tools/cardcreator/custom.ts (3)</summary><blockquote> <details> <summary>src/types/card.ts (1)</summary> * `BlueprintWithOptional` (422-425) </details> <details> <summary>src/functions/interact.ts (1)</summary> * `input` (1648-1680) </details> <details> <summary>src/game.ts (1)</summary> * `input` (1252-1262) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (6)</summary><blockquote> <details> <summary>tools/cardcreator/custom.ts (6)</summary><blockquote> `46-50`: **LGTM - Simplified helper function.** The `set()` helper has been simplified from the previous version, removing the problematic keywords and array handling logic that was flagged in earlier reviews. The current implementation correctly updates both the blueprint and card objects. --- `52-63`: **LGTM - Card initialization with proper async handling.** The `await` on `doBlueprint` is now correctly applied (addressing a previous review concern). The unconventional direct blueprint assignment is acknowledged with a comment. --- `159-163`: **LGTM - Color tag properly closed.** The yellow warning message now correctly uses `</yellow>` as the closing tag, addressing the issue from a previous review. --- `186-234`: **LGTM - Array field handling with proper configuration.** The array handling correctly maps each field to its enum type with appropriate constraints (e.g., runes limited to 3 with duplicates allowed). The `await` on `doBlueprint` is correct, and the `dirty ||= changed` pattern properly tracks modifications. --- `277-292`: **LGTM - Boolean comparison fixed.** The previous bug where `Boolean("false")` would evaluate to `true` has been fixed. The code now correctly uses `booleanChoice === "true"` for proper boolean conversion. --- `337-359`: **LGTM - Simplified main function.** The `main()` function has been streamlined with a cleaner signature and proper integration with the new `configure()` flow. Error handling for user cancellation is correct. </blockquote></details> </blockquote></details> </details>

tools/cardcreator/custom.ts

⚠️ Potential issue | 🟡 Minor

Unhandled JSON.parse exception and redundant assignment.

Line 313: JSON.parse(newValue) can throw if the user enters malformed JSON. Since validation only checks for valid numbers when blueprint[key] is a number, other types could receive invalid JSON input.
Line 316: dirty = true is redundant since set() already sets dirty = true on line 49.

 	if (typeof value === "string") {
 		set(key, newValue);
 	} else {
-		set(key, JSON.parse(newValue));
+		try {
+			set(key, JSON.parse(newValue));
+		} catch {
+			// Fallback for numbers if JSON parsing fails
+			const parsed = parseInt(newValue, 10);
+			if (!Number.isNaN(parsed)) {
+				set(key, parsed);
+			}
+			// If parsing fails and it's not a number, the value remains unchanged
+			continue;
+		}
 	}
-
-	dirty = true;

📝 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 (typeof value === "string") {
			set(key, newValue);
		} else {
			try {
				set(key, JSON.parse(newValue));
			} catch {
				// Fallback for numbers if JSON parsing fails
				const parsed = parseInt(newValue, 10);
				if (!Number.isNaN(parsed)) {
					set(key, parsed);
				}
				// If parsing fails and it's not a number, the value remains unchanged
				continue;
			}
		}
	}

🤖 Prompt for AI Agents

In tools/cardcreator/custom.ts around lines 310 to 317, JSON.parse(newValue) can
throw on malformed input and the subsequent dirty = true is redundant; wrap the
parse in a try/catch (or validate newValue first) and handle parse errors by
showing/returning a validation error or falling back to a safe value instead of
letting the exception propagate, then remove the redundant dirty = true
assignment because set(...) already marks dirty; ensure set(key, parsed) is only
called with a successfully parsed value.

_⚠️ Potential issue_ | _🟡 Minor_ **Unhandled `JSON.parse` exception and redundant assignment.** 1. **Line 313**: `JSON.parse(newValue)` can throw if the user enters malformed JSON. Since validation only checks for valid numbers when `blueprint[key]` is a number, other types could receive invalid JSON input. 2. **Line 316**: `dirty = true` is redundant since `set()` already sets `dirty = true` on line 49. ```diff if (typeof value === "string") { set(key, newValue); } else { - set(key, JSON.parse(newValue)); + try { + set(key, JSON.parse(newValue)); + } catch { + // Fallback for numbers if JSON parsing fails + const parsed = parseInt(newValue, 10); + if (!Number.isNaN(parsed)) { + set(key, parsed); + } + // If parsing fails and it's not a number, the value remains unchanged + continue; + } } - - dirty = true; ```  <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 (typeof value === "string") { set(key, newValue); } else { try { set(key, JSON.parse(newValue)); } catch { // Fallback for numbers if JSON parsing fails const parsed = parseInt(newValue, 10); if (!Number.isNaN(parsed)) { set(key, parsed); } // If parsing fails and it's not a number, the value remains unchanged continue; } } } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In tools/cardcreator/custom.ts around lines 310 to 317, JSON.parse(newValue) can throw on malformed input and the subsequent dirty = true is redundant; wrap the parse in a try/catch (or validate newValue first) and handle parse errors by showing/returning a validation error or falling back to a safe value instead of letting the exception propagate, then remove the redundant dirty = true assignment because set(...) already marks dirty; ensure set(key, parsed) is only called with a successfully parsed value. ``` </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!438

Rows
Columns