fix(deckbuilder): gate copy-limit affordances on engine canonical counts - #6713
fix(deckbuilder): gate copy-limit affordances on engine canonical counts#6713wondercreatemaster wants to merge 1 commit into
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. CardGrid tests use a full ScryfallCard fixture and BrowserLegalityFilter "Modern". Fixes phase-rs#6659
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe deck builder now uses engine-canonical card keys for copy-limit counting and passes eligibility into search-card tiles. Disabled tiles show copy-limit messaging, localized strings support the new states, and tests cover enabled and disabled add behavior. ChangesDeck builder copy-limit affordances
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeckBuilder
participant CardGrid
participant useDeckBuilder
participant EngineWASM
DeckBuilder->>CardGrid: pass canIncrement as canAddCard
CardGrid->>useDeckBuilder: evaluate card name
useDeckBuilder->>EngineWASM: request canonical deck-count key
EngineWASM-->>useDeckBuilder: return canonical key
useDeckBuilder-->>CardGrid: return copy-limit eligibility
CardGrid-->>DeckBuilder: add card only when legal and eligible
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: 3
🤖 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 348-360: Remove the local combinedCopyCounts aggregation from the
deck-builder hook and use the engine’s add-eligibility/count result for the
current deck partition and candidate instead. Update the consuming logic to rely
on the engine’s copy-limit evaluation, keeping canonicalization and counted-zone
selection centralized in the engine so it remains consistent with
copy_limit_violations.
In `@crates/engine/src/game/deck_validation.rs`:
- Around line 2622-2625: Add a verified Comprehensive Rules citation in the
documentation for canonical_deck_count_key, using the required “CR <number>:
<description>” format. Describe how that rule supports canonicalizing card
identities for copy-limit aggregation and shared UI affordance validation,
without changing the helper’s behavior.
- Around line 4000-4013: Expand canonical_deck_count_key_merges_alias_spellings
to cover a double-faced card fixture, asserting both its combined name and
front-face name resolve to the documented canonical key. Keep the existing
Nazgul spelling and case checks, and exercise the reusable canonicalization
behavior across the relevant DFC name forms rather than only comparing two
returned values.
🪄 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: 0e05be70-13c0-4e31-ae9d-ab29f53507c2
⛔ 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
| 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.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep copy-limit aggregation and eligibility in the engine.
This hook independently selects counted zones and derives copy-limit state. Expose an engine add-eligibility/count result for the deck partition and candidate instead; otherwise this UI logic can drift from copy_limit_violations as format rules evolve.
🤖 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 348 - 360,
Remove the local combinedCopyCounts aggregation from the deck-builder hook and
use the engine’s add-eligibility/count result for the current deck partition and
candidate instead. Update the consuming logic to rely on the engine’s copy-limit
evaluation, keeping canonicalization and counted-zone selection centralized in
the engine so it remains consistent with copy_limit_violations.
Sources: Coding guidelines, Path instructions
| /// | ||
| /// Public so the deck-builder WASM surface can share the same authority the | ||
| /// legality checker uses (affordance gating must not re-derive folding in JS). | ||
| pub fn canonical_deck_count_key(db: &CardDatabase, name: &str) -> String { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the required verified Comprehensive Rules citation.
This public helper now defines card identity for copy-limit validation and UI affordances, but its documentation has no verified CR <number>: <description> citation. Add the applicable rule citation and describe how it supports canonical copy aggregation.
As per path instructions, rules-touching code in crates/engine/** must carry a verified CR <number>: <description> annotation.
🤖 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 `@crates/engine/src/game/deck_validation.rs` around lines 2622 - 2625, Add a
verified Comprehensive Rules citation in the documentation for
canonical_deck_count_key, using the required “CR <number>: <description>”
format. Describe how that rule supports canonicalizing card identities for
copy-limit aggregation and shared UI affordance validation, without changing the
helper’s behavior.
Source: Path instructions
| #[test] | ||
| fn canonical_deck_count_key_merges_alias_spellings() { | ||
| // Public surface for the deck-builder affordance (#6659): alias | ||
| // spellings must share one key so client-side counts match validation. | ||
| let db = CardDatabase::from_json_str(&test_db_json()).unwrap(); | ||
| assert_eq!( | ||
| canonical_deck_count_key(&db, "Nazgul"), | ||
| canonical_deck_count_key(&db, "Nazgûl") | ||
| ); | ||
| assert_eq!( | ||
| canonical_deck_count_key(&db, "Nazgul"), | ||
| canonical_deck_count_key(&db, "nazgûl") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Cover DFC/front-face canonicalization in the regression test.
This test only exercises the Nazgul spelling and case variants. The exposed contract also promises that double-faced combined names and front-face names share a key; add a DFC fixture and assert the expected canonical key, not only equality between two calls.
As per path instructions, reusable building blocks must be tested across their parameter range rather than through a single card case.
🤖 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 `@crates/engine/src/game/deck_validation.rs` around lines 4000 - 4013, Expand
canonical_deck_count_key_merges_alias_spellings to cover a double-faced card
fixture, asserting both its combined name and front-face name resolve to the
documented canonical key. Keep the existing Nazgul spelling and case checks, and
exercise the reusable canonicalization behavior across the relevant DFC name
forms rather than only comparing two returned values.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed current head fb30c845c8ff826f0c9fa853fffb2fd61f8fde1d.
This superseding PR corrects the CardGrid fixture types, but it retains the two material #6712 defects.
-
Keep copy-limit eligibility and counted-zone selection in the engine.
useDeckBuilderreconstructs the count in React and compares it with separatemaxDeckCopies/canonicalDeckCountKeycalls. That has already diverged from the validator: the hook counts the dedicateddeck.companion, while engineconstruction_deck_cardsdeliberately excludes it because it is outside the starting deck. It also counts sideboard entries for Commander/Brawl variants, while the engine strips those entries before copy-limit validation because they are not in the loaded deck. Please expose an engine/WASM candidate eligibility result derived from the sameDeckCompatibilityRequestandcopy_limit_violationsauthority, and have the UI render that result. -
Do not fail open while eligibility is stale or unavailable. The effect retains the prior maps while a format/search request is pending, clears them on rejection, and
canIncrementreturnstruewhen no entry exists. A newly returned search card (including a canonical alias) can therefore be added before its request resolves; a failed request leaves it addable.handleAddCardhas no independent eligibility guard. Make unresolved/stale eligibility unavailable until the authoritative response for the current deck, format, and candidate is available. -
Add a discriminating runtime test through the engine → WASM → deck-builder search-grid path. It must fail if the eligibility binding or canonicalization is removed, cover the alias ceiling and the companion/Commander-sideboard zone selection, and cover an unresolved or rejected request. The present CardGrid tests only verify an injected boolean. The public canonical-key test should also cover the documented DFC combined-name/front-face contract.
The frontend CI is green, but Rust/card-data checks are still pending and this branch is behind current main. CodeRabbit independently raised the engine-authority and missing-DFC-coverage findings. Its CR-annotation thread is not actionable: the existing helper already has the project's allowed compound CR 201.3 + CR 100.2a annotation.
Parse changes introduced by this PR✓ No card-parse changes detected. |
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.Supersedes closed #6712 (same change plus CardGrid test typing that failed CI type-check).
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 coverage (fullScryfallCard+Modernfilter)client/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
pnpm run type-check— passvitest run src/components/deck-builder/__tests__/CardGrid.test.tsx— 2 passedscripts/check-parser-combinators.sh origin/main— Gate A PASS (no parser files touched).Gate A
Gate A PASS head=fb30c845c8ff826f0c9fa853fffb2fd61f8fde1d 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=fb30c845c8ff826f0c9fa853fffb2fd61f8fde1d — no name folding in the client; ceilings and keys come from the engine; search grid uses the same
canIncrementgate as deck rows; CardGrid test fixtures matchScryfallCard/BrowserLegalityFilter.Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Made with Cursor
Summary by CodeRabbit
New Features
Tests