Draft build screen - #6670
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughLimited deck building now uses explicit land metadata across draft models and generated packs, separates lands from spells in deck displays and mana curves, supports localized deck-list copying, and restricts unlimited basics to the five core basic lands. ChangesLimited deck and land metadata flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
actor Drafter
participant LimitedDeckBuilder
participant Clipboard
Drafter->>LimitedDeckBuilder: Select Copy list
LimitedDeckBuilder->>Clipboard: Write formatted deck and sideboard text
Clipboard-->>LimitedDeckBuilder: Resolve clipboard write
LimitedDeckBuilder-->>Drafter: Display copied confirmation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/components/draft/LimitedDeckBuilder.tsx (1)
179-257: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMove deck composition and curve classification into the engine.
These components parse
type_lineand derive land/spell/creature state locally, so UI behavior can diverge from engine rules and adapters. Expose engine-calculated deck composition and curve buckets, then wire them through every frontend transport.
client/src/components/draft/LimitedDeckBuilder.tsx#L179-L257: consume engine-provided land/spell classification and counts instead of parsingtype_line.client/src/components/draft/ManaCurve.tsx#L23-L54: consume engine-provided total and creature CMC buckets instead of inferring creature status and CMC categories.As per path instructions, “The frontend is a display layer, never a logic layer.”
🤖 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/draft/LimitedDeckBuilder.tsx` around lines 179 - 257, The deck builder and mana curve currently derive card classifications in the frontend instead of consuming engine results. Update client/src/components/draft/LimitedDeckBuilder.tsx lines 179-257 to use engine-provided land/spell composition and counts, removing local type_line parsing; update client/src/components/draft/ManaCurve.tsx lines 23-54 to use engine-provided total and creature CMC buckets rather than inferring creature status or CMC categories, and wire these values through every frontend transport.Sources: Coding guidelines, Path instructions
🤖 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/draft/LimitedDeckBuilder.tsx`:
- Around line 419-429: Add a disabled attribute to the button rendering the
copyDeckList action, using the same mainDeck.length === 0 condition as
menuButtonClass so the control is non-interactive when the deck is empty.
- Around line 271-289: Update the toLines helper inside copyDeckList to merge
extra counts into the existing countMap before formatting lines. Ensure names
present in both mainDeck and landCounts produce one line with their summed
count, while preserving separate positive entries for names found only in extra.
---
Outside diff comments:
In `@client/src/components/draft/LimitedDeckBuilder.tsx`:
- Around line 179-257: The deck builder and mana curve currently derive card
classifications in the frontend instead of consuming engine results. Update
client/src/components/draft/LimitedDeckBuilder.tsx lines 179-257 to use
engine-provided land/spell composition and counts, removing local type_line
parsing; update client/src/components/draft/ManaCurve.tsx lines 23-54 to use
engine-provided total and creature CMC buckets rather than inferring creature
status or CMC categories, and wire these values through every frontend
transport.
🪄 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: 704d421e-40b0-4a6a-9c26-85e1ccd8bde5
📒 Files selected for processing (10)
client/src/components/draft/LimitedDeckBuilder.tsxclient/src/components/draft/ManaCurve.tsxclient/src/i18n/locales/de/draft.jsonclient/src/i18n/locales/en/draft.jsonclient/src/i18n/locales/es/draft.jsonclient/src/i18n/locales/fr/draft.jsonclient/src/i18n/locales/it/draft.jsonclient/src/i18n/locales/pl/draft.jsonclient/src/i18n/locales/pt/draft.jsoncrates/draft-core/src/validation.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — verified against current head bdf9a9519d292dfd945b6715f0e0a151d7399499.
I independently checked the current-head source below. Any matching CodeRabbit comments are corroboration only, not the basis for these findings.
[HIGH] Deck classification is derived in the display layer rather than provided by the draft domain. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:179-182 parses type_line to classify lands and :242-257 uses that result to compute spell/land accounting; client/src/components/draft/ManaCurve.tsx:23-54 separately derives creature classification from type_line. Why it matters: duplicated frontend inference can disagree with draft validation and adapters as card/type representations evolve, violating the engine/domain-to-display ownership boundary. Suggested fix: have draft-core expose the needed land/spell/creature classification/accounting in the draft view (threaded through the adapter), then render that supplied state in both components.
[MED] Copy-list aggregation emits duplicate card names when a name is present in both mainDeck and landCounts. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:271-284 builds a count map from names and then independently appends every extra entry. Why it matters: exported deck lists can contain two lines for the same name and no longer represent a single total. Suggested fix: merge landCounts into the same count map before formatting lines, and add a case with the same card name in both sources.
[MED] The empty-main-deck Copy List control is visually disabled but remains actionable. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:419-430 passes a disabled style to menuButtonClass but omits native disabled, while the Submit button at :432-441 supplies disabled={!deckValid}. Why it matters: keyboard/pointer users can still copy an empty deck despite the disabled affordance. Suggested fix: add disabled={mainDeck.length === 0} and a behavior test that verifies the clipboard action is not invoked for an empty main deck.
[MED] The PR adds distinct accounting, curve, copy, and validation behavior without UI coverage that distinguishes those branches. Evidence: the current-head changed-file list contains the two components, locale files, and crates/draft-core/src/validation.rs, but no frontend test file; the new paths are in LimitedDeckBuilder.tsx:242-284,419-441 and ManaCurve.tsx:23-54. Why it matters: type-checking cannot show that the visible counts, curve buckets, copied list, and disabled/submission states remain correct. Suggested fix: add focused UI tests for land-versus-spell accounting, creature curve count, merged copy-list totals, empty-copy disablement, and valid/invalid submit state.
Next step: move classification/accounting ownership to draft-core/its adapter, update the components to render that view data, fix copy aggregation and native disablement, then add the targeted UI tests above before requesting re-review.
fix disabled
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head ec48f04.
[HIGH] Deck composition remains derived in the display layer. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:179-182 parses type_line, and :242-257 converts that local parse into spell/land accounting and the ManaCurve input. Why it matters: the draft domain and its adapters do not own the classification that controls visible deck counts, so this frontend-only inference can diverge from validation and card representation. Suggested fix: expose composition/curve data from draft-core through DraftPlayerView, then render it here.
[MED] Copy-list aggregation still emits duplicate names. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:271-288 writes mainDeck counts and landCounts as separate lists; the new test at client/src/components/draft/tests/LimitedDeckBuilder.test.tsx:121-149 exercises only disjoint names. Why it matters: a card present in both inputs exports as two lines rather than one aggregate quantity. Suggested fix: merge positive landCounts into countMap before rendering and test an overlapping name.
[MED] Copy List is still actionable with an empty main deck. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:419-429 has neither a native disabled attribute nor a length guard, and the new test never exercises the empty deck path. Why it matters: users can invoke a control that should be unavailable and copy a blank deck list. Suggested fix: disable the button using the same empty-deck condition and add a negative clipboard assertion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/draft/__tests__/LimitedDeckBuilder.test.tsx`:
- Around line 124-145: Add a regression test in the LimitedDeckBuilder copy-list
tests using an overlapping “Island” entry in both mainDeck and landCounts, then
assert the copied deck list contains a single merged Island line with the
combined count rather than duplicate entries. Keep the existing disjoint-entry
coverage intact and ensure the new assertion exercises copyDeckList.
🪄 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: 26b8c949-2b59-4e15-8864-9db8dfc4d9d5
📒 Files selected for processing (1)
client/src/components/draft/__tests__/LimitedDeckBuilder.test.tsx
|
I will fix land/spell classification but the last is not an issue:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/components/draft/LimitedDeckBuilder.tsx (2)
236-263: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep deck accounting out of the React layer.
landNameSet,mainDeckSpells,deckLandCount,basicLands,totalCards, anddeckValidderive game state insideclient/src. This creates a second rules implementation, and the name-based set cannot represent per-instance classifications. Expose spell/land counts and validity from engine/state, then render those values here.As per path instructions, the React frontend must only render engine-provided state and dispatch actions.
🤖 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/draft/LimitedDeckBuilder.tsx` around lines 236 - 263, Remove the deck-accounting calculations from LimitedDeckBuilder, including landNameSet, mainDeckSpells, deckLandCount, basicLands, totalCards, and deckValid. Expose the required spell/land counts and deck validity through the engine or state layer, preserving per-instance classifications, then consume those engine-provided values in the component while keeping it limited to rendering and dispatching actions.Source: Path instructions
265-299: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle clipboard failures here.
navigator.clipboard.writeTextcan reject in insecure contexts or when permission is denied, and this path currently drops that failure silently. Add a guard/catch and a fallback message so the copy action gives feedback instead of failing invisibly.🤖 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/draft/LimitedDeckBuilder.tsx` around lines 265 - 299, The copyDeckList clipboard operation currently ignores write failures. Update navigator.clipboard.writeText handling in copyDeckList to catch rejected writes, preserve the existing success feedback, and provide a user-visible fallback message when copying is unavailable or denied.
🤖 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 `@crates/draft-core/src/types.rs`:
- Around line 229-231: The is_land field on DraftCardInstance must not
deserialize omitted legacy metadata as false. Update its
migration/deserialization path to derive the value from type_line for older
payloads, or version and rebuild those payloads, while preserving explicit
values from current data; add a regression test covering legacy land cards.
- Around line 229-231: Replace the boolean is_land field in the Rust card type
with a shared serializable card-kind enum, defining both domain categories for
exhaustive handling and preserving serde defaults as needed. In
client/src/adapter/draft-adapter.ts:14, mirror the same card-kind enum in the
frontend contract and update consumers to use the typed classification instead
of inferring the complementary category.
---
Outside diff comments:
In `@client/src/components/draft/LimitedDeckBuilder.tsx`:
- Around line 236-263: Remove the deck-accounting calculations from
LimitedDeckBuilder, including landNameSet, mainDeckSpells, deckLandCount,
basicLands, totalCards, and deckValid. Expose the required spell/land counts and
deck validity through the engine or state layer, preserving per-instance
classifications, then consume those engine-provided values in the component
while keeping it limited to rendering and dispatching actions.
- Around line 265-299: The copyDeckList clipboard operation currently ignores
write failures. Update navigator.clipboard.writeText handling in copyDeckList to
catch rejected writes, preserve the existing success feedback, and provide a
user-visible fallback message when copying is unavailable or denied.
🪄 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: 8a335b2c-2b65-435b-be32-73e8c757b344
📒 Files selected for processing (13)
client/src/adapter/draft-adapter.tsclient/src/components/draft/LimitedDeckBuilder.tsxclient/src/components/draft/__tests__/LimitedDeckBuilder.test.tsxclient/src/components/draft/__tests__/PackDisplay.pod.test.tsxclient/src/network/__tests__/draftProtocol.test.tsclient/src/stores/__tests__/multiplayerDraftStore.test.tscrates/draft-core/src/cube.rscrates/draft-core/src/pack_generator.rscrates/draft-core/src/pack_source.rscrates/draft-core/src/session.rscrates/draft-core/src/types.rscrates/draft-core/src/view.rscrates/draft-wasm/src/suggest.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/components/draft/tests/LimitedDeckBuilder.test.tsx
| /// Domain-owned classification for deckbuilding views. | ||
| #[serde(default)] | ||
| pub is_land: bool, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not default legacy land cards to false.
#[serde(default)] makes pre-change DraftCardInstance payloads deserialize successfully while classifying omitted land metadata as a spell. The downstream suggestion and deck-builder code now trusts this field, so reloading an older pool can misclassify lands and corrupt deck composition. Derive the value from type_line during migration/deserialization, or version and rebuild legacy payloads; add a regression test.
🤖 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/draft-core/src/types.rs` around lines 229 - 231, The is_land field on
DraftCardInstance must not deserialize omitted legacy metadata as false. Update
its migration/deserialization path to derive the value from type_line for older
payloads, or version and rebuild those payloads, while preserving explicit
values from current data; add a regression test covering legacy land cards.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Carry typed card classification end to end.
The new domain state is represented as a boolean in both layers, preventing exhaustive handling and forcing each consumer to infer the complementary card category.
crates/draft-core/src/types.rs#L229-L231: replaceis_land: boolwith a shared serializable card-kind enum.client/src/adapter/draft-adapter.ts#L14-L14: mirror that enum in the frontend contract rather than reintroducing a boolean.
As per path instructions, known domain state must use typed enums across the engine/frontend boundary.
📍 Affects 2 files
crates/draft-core/src/types.rs#L229-L231(this comment)client/src/adapter/draft-adapter.ts#L14-L14
🤖 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/draft-core/src/types.rs` around lines 229 - 231, Replace the boolean
is_land field in the Rust card type with a shared serializable card-kind enum,
defining both domain categories for exhaustive handling and preserving serde
defaults as needed. In client/src/adapter/draft-adapter.ts:14, mirror the same
card-kind enum in the frontend contract and update consumers to use the typed
classification instead of inferring the complementary category.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 543b415.
[HIGH] The type-line parsing itself has moved to draft-core, but selected-deck composition and the curve remain derived in the display layer. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:235-253 builds a land-name set from the pool, filters mainDeck into mainDeckSpells, and calculates deckLandCount; it then passes that reconstructed list to ManaCurve. client/src/components/draft/ManaCurve.tsx:20-43 independently maps selected names to CMC buckets. Why it matters: DraftPlayerView exposes only individual card facts, so the browser—not the draft domain/adapter—still owns the composition and curve projection that controls deckbuilder output. This violates the engine-to-display boundary and leaves multiple frontend computations to keep consistent. Suggested fix: expose an engine/draft-core projection for the active selected deck (composition and curve buckets), thread it through the adapter/store, and render those supplied values.
Reconciliation: the current head fixes the duplicate copy-list aggregation and covers the overlap case; the old disabled-control mismatch is no longer present because Copy List is no longer styled disabled. The remaining issue above is independently verified against this head.
|
Additional verified current-head finding for 543b415: [HIGH] The new is_land serde default misclassifies legacy persisted sessions. Evidence: crates/draft-core/src/types.rs:229-231 marks is_land with #[serde(default)], which supplies false when an older DraftCardInstance payload omits the field. crates/draft-wasm/src/lib.rs:642-652 restores persisted DraftSession JSON directly with serde_json::from_str; the client persists those full snapshots for crash/reload recovery. Downstream deck suggestions and the deck builder now trust is_land, so resumed older snapshots treat every omitted-field land as a nonland. Suggested fix: add a versioned import migration (or equivalent custom deserialization) that derives the missing value from type_line while preserving explicitly serialized values, and cover import of a legacy land snapshot. |
|
#6670 (review) #6670 (comment) |
2 participants
Summary
Fix land accounting on limited deck build screen.
Add copy deck button.
Files changed
Track
Developer
LLM
Model: gpt-5.5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable - no engine related changes
CR references
MTR 7.2 replaced CR 100.2a for limited event
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
<exact command or CI check>—Gate A
Gate A PASS head=bdf9a9519d292dfd945b6715f0e0a151d7399499 base=b9238c8c8d488288aad13fba1dae239acced84f9
Final review-impl
[LOW] Copy export can emit duplicate lines for the same card. Evidence: client/src/components/draft/LimitedDeckBuilder.tsx:271. Why it matters: if a card name appears in both mainDeck and landCounts, the copied list may contain two separate Plains or custom-addable lines instead of the submitted aggregate count. Suggested fix: merge extra counts into the same count map before rendering lines.
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit