fix(deckbuilder): gate copy-limit affordances on engine canonical counts - #6712
Conversation
Expose canonical_deck_count_key via WASM so the deck builder aggregates alias spellings into one bucket, and disable CardGrid search-add when canIncrement says the ceiling is reached. Fixes phase-rs#6659
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe deck builder now canonicalizes card identities for copy-limit counting and passes increment eligibility into search-result card tiles. Tiles disable additions and show copy-limit messaging when limits are reached, with localized strings and component tests added. ChangesDeck builder copy-limit gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/deck-builder/useDeckBuilder.ts`:
- Around line 342-360: Remove the React-side copy-limit derivation in
combinedCopyCounts and canIncrement, and expose an engine/WASM eligibility
result for the current deck and candidate card using the authoritative
copy_limit_violations logic. Update the frontend to render that engine-provided
result and dispatch the existing add action without aggregating deck zones or
applying its own limits.
- Around line 388-414: Update the copy-limit loading flow in the Promise.all
request and its surrounding state logic to fail closed: clear or invalidate
copyLimits before each format/name request, and make canIncrement return false
when an entry is unresolved or absent. Preserve cancellation protection for
stale responses, and retry the request after transient WASM-load failures
instead of leaving additions enabled indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 079ec60d-d1da-49d4-b2db-ba9492017f22
⛔ Files ignored due to path filters (1)
client/src/wasm/engine_wasm.d.tsis excluded by!client/src/wasm/**,!**/*.d.ts
📒 Files selected for processing (15)
client/src/components/deck-builder/CardGrid.tsxclient/src/components/deck-builder/DeckBuilder.tsxclient/src/components/deck-builder/__tests__/CardGrid.test.tsxclient/src/components/deck-builder/useDeckBuilder.tsclient/src/i18n/locales/de/deck-builder.jsonclient/src/i18n/locales/en/deck-builder.jsonclient/src/i18n/locales/es/deck-builder.jsonclient/src/i18n/locales/fr/deck-builder.jsonclient/src/i18n/locales/it/deck-builder.jsonclient/src/i18n/locales/pl/deck-builder.jsonclient/src/i18n/locales/pt/deck-builder.jsonclient/src/services/engineRuntime.tscrates/engine-wasm/src/lib.rscrates/engine/src/game/deck_validation.rscrates/engine/src/game/mod.rs
| // Counts are keyed by the engine's canonical name so alias spellings share a | ||
| // bucket (#6659) — never fold accents/case/DFC forms in the display layer. | ||
| const [canonicalKeys, setCanonicalKeys] = useState<Map<string, string>>( | ||
| () => new Map(), | ||
| ); | ||
|
|
||
| const combinedCopyCounts = useMemo(() => { | ||
| const counts = new Map<string, number>(); | ||
| const add = (name: string, n: number) => | ||
| counts.set(name, (counts.get(name) ?? 0) + n); | ||
| const add = (name: string, n: number) => { | ||
| const key = canonicalKeys.get(name) ?? name; | ||
| counts.set(key, (counts.get(key) ?? 0) + n); | ||
| }; | ||
| for (const entry of deck.main) add(entry.name, entry.count); | ||
| for (const entry of deck.sideboard) add(entry.name, entry.count); | ||
| for (const name of commanders) add(name, 1); | ||
| for (const name of deck.signature_spell ?? []) add(name, 1); | ||
| if (deck.companion) add(deck.companion, 1); | ||
| return counts; | ||
| }, [deck, commanders]); | ||
| }, [deck, commanders, canonicalKeys]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Move copy-limit derivation back into the engine.
combinedCopyCounts and canIncrement reconstruct game-rule state in React by aggregating deck zones and comparing the result with an engine limit. Even with canonicalDeckCountKey, this can drift from the engine’s authoritative copy_limit_violations behavior for special formats, zones, or future rule changes. Expose an engine/WASM eligibility result for the current deck and candidate card; the frontend should only render that result and dispatch the add action.
As per coding guidelines and path instructions, the frontend must only render engine-provided game state and the engine owns copy-limit logic.
Also applies to: 421-428
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/components/deck-builder/useDeckBuilder.ts` around lines 342 - 360,
Remove the React-side copy-limit derivation in combinedCopyCounts and
canIncrement, and expose an engine/WASM eligibility result for the current deck
and candidate card using the authoritative copy_limit_violations logic. Update
the frontend to render that engine-provided result and dispatch the existing add
action without aggregating deck zones or applying its own limits.
Sources: Coding guidelines, Path instructions
| setCanonicalKeys(new Map()); | ||
| return; | ||
| } | ||
| let cancelled = false; | ||
| Promise.all( | ||
| names.map(async (name) => [name, await maxDeckCopies(name, format)] as const), | ||
| names.map(async (name) => { | ||
| const [limit, canonical] = await Promise.all([ | ||
| maxDeckCopies(name, format), | ||
| canonicalDeckCountKey(name), | ||
| ]); | ||
| return [name, limit, canonical] as const; | ||
| }), | ||
| ) | ||
| .then((results) => { | ||
| if (!cancelled) setCopyLimits(new Map(results)); | ||
| if (cancelled) return; | ||
| setCopyLimits(new Map(results.map(([name, limit]) => [name, limit]))); | ||
| setCanonicalKeys( | ||
| new Map(results.map(([name, , canonical]) => [name, canonical])), | ||
| ); | ||
| }) | ||
| .catch(() => { | ||
| // WASM may not be loaded yet; an empty map leaves increments open and | ||
| // the engine's compatibility warnings still flag any real violation. | ||
| if (!cancelled) setCopyLimits(new Map()); | ||
| if (!cancelled) { | ||
| setCopyLimits(new Map()); | ||
| setCanonicalKeys(new Map()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed while copy-limit data is stale or unavailable.
A new request does not clear the previous copyLimits, so changing formats can temporarily use the old format’s ceiling. For new names or rejected WASM requests, copyLimits.get(name) is undefined and canIncrement returns true, re-enabling additions while eligibility is unknown. Clear or version the state before each request, return false for unresolved entries, and retry after transient engine-load failures.
Suggested direction
useEffect(() => {
+ setCopyLimits(new Map());
+ setCanonicalKeys(new Map());
...
}, [copyLimitKey, format]);
const limit = copyLimits.get(name);
- if (!limit || limit.type === "Unlimited") return true;
+ if (!limit) return false;
+ if (limit.type === "Unlimited") return true;Also applies to: 421-428
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/components/deck-builder/useDeckBuilder.ts` around lines 388 - 414,
Update the copy-limit loading flow in the Promise.all request and its
surrounding state logic to fail closed: clear or invalidate copyLimits before
each format/name request, and make canIncrement return false when an entry is
unresolved or absent. Preserve cancellation protection for stale responses, and
retry the request after transient WASM-load failures instead of leaving
additions enabled indefinitely.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed current head 26695aa6ed852ac492b6e38dc684919096499112.
-
The required frontend job is red. Its lint step passed, but TypeScript fails in the new
CardGrid.test.tsx:legalityFormat="modern"is not aBrowserLegalityFilter(the type requires"Modern"), and the partial fixture is not structurally compatible withScryfallCard. Vitest was consequently skipped. Please correct the typed test setup and provide a green required frontend run. -
Copy-limit eligibility needs to remain engine-authoritative.
useDeckBuildernow rebuilds the relevant deck-zone counts in React (combinedCopyCounts) and compares them to separate WASM results (maxDeckCopiesandcanonicalDeckCountKey). That duplicates the decision already represented by enginecopy_limit_violations, and can drift when the validation's zones or format-specific rules evolve. Expose an engine/WASM eligibility result for the deck plus candidate card and have the grid render that result rather than re-deriving eligibility in React. -
Do not leave the new affordance fail-open. While requests are pending, when a new result has no map entry, or after the catch path empties the maps,
canIncrementreturnstruefor!limit; a format change can also retain the prior format's map until the new request settles. The search-grid path callshandleAddCard, which does not independently callcanIncrement. Make unresolved or stale eligibility unavailable (fail closed) until the authoritative response for the current deck/format/candidate is ready.
Please add a discriminating runtime test through the engine → WASM → deck-builder search-grid path. It should demonstrate that alias spellings sharing a canonical key reach the ceiling, and cover the unresolved/stale response behavior. The present CardGrid unit tests only verify a supplied boolean and would not catch a broken canonicalization/binding/eligibility path.
Evidence: failed required frontend job; the two unresolved CodeRabbit threads at the current head independently identify the engine-boundary and fail-open defects.
Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Superseded by a fresh one-shot PR that includes the CardGrid.test.tsx type-check fix (CI failure on this head). |
2 participants
Summary
Fixes #6659: two copy-limit affordance gaps. (1) Client
combinedCopyCountskeyed on raw entry names, so alias spellings (Nazgul/Nazgûl, case, DFC forms) each got their own ceiling. Expose the engine’s existingcanonical_deck_count_keyvia WASM and aggregate on that key. (2) SearchCardGridhad nocanAddCardgate (unlikeDeckStack); wirecanIncrementso at-ceiling search results disable visibly.Neither gap can produce an illegal playable deck —
copy_limit_violationsalready canonicalizes — these are stale-affordance fixes.Files changed
crates/engine/src/game/deck_validation.rs—pub fn canonical_deck_count_key+ unit testcrates/engine/src/game/mod.rs— re-exportcrates/engine-wasm/src/lib.rs—canonicalDeckCountKeyWASM bindingclient/src/services/engineRuntime.ts/client/src/wasm/engine_wasm.d.ts— TS surfaceclient/src/components/deck-builder/useDeckBuilder.ts— canonical aggregation; include search names in limit fetchclient/src/components/deck-builder/CardGrid.tsx+DeckBuilder.tsx—canAddCardaffordanceclient/src/components/deck-builder/__tests__/CardGrid.test.tsx— disabled/enabled coverageclient/src/i18n/locales/*/deck-builder.json— copy-limit stringsTrack
Developer
LLM
Model: Composer
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — exposes the existing
deck_validation::canonical_deck_count_keyauthority for deck-builder affordance gating; no rules/resolver/parser behavior change. Legality still enforced solely bycopy_limit_violations.CR references
max_deck_copies/combined_copy_counts)canonical_deck_count_key)Verification
cargo test/ frontend vitest deferred — no Rust toolchain here; CI will run engine +check-frontend/test-frontend.scripts/check-parser-combinators.sh origin/main— Gate A PASS (no parser files touched).Gate A
Gate A PASS head=26695aa6ed852ac492b6e38dc684919096499112 base=e9a268a4d5ef4db00dd903ed2e1c1f6ceebcd95a
Anchored on
crates/engine/src/game/deck_validation.rs—combined_copy_counts/max_deck_copiesalready usecanonical_deck_count_key; WASM now exposes that same keyclient/src/components/deck-builder/DeckStack.tsx— existingcanAddCard={canIncrement}pattern mirrored ontoCardGridFinal review-impl
Final review-impl PASS head=26695aa6ed852ac492b6e38dc684919096499112 — no name folding in the client; ceilings and keys come from the engine; search grid uses the same
canIncrementgate as deck rows.Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests