fix(draft): put suggested nonbasic lands on main_deck - #6711
fix(draft): put suggested nonbasic lands on main_deck#6711wondercreatemaster wants to merge 1 commit into
Conversation
Drafted duals were stuffed into `lands`, which the limited deckbuilder only renders for addable basics — so they stayed visible in the pool, got added again, and submit double-counted them as ExceedsPoolCount. Also surface per-card ValidationFailed details in the error Display and submit UI. Fixes phase-rs#6562
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughSuggested deck construction now places drafted nonbasic fixing lands in ChangesLimited deck flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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/draft/LimitedDeckBuilder.tsx`:
- Around line 366-379: Update the submission handler in LimitedDeckBuilder’s
onClick flow to guard against an in-flight submitDeck call, disable the
associated button while submission is active, and clear the guard in a finally
block. Preserve the existing error extraction and setSubmitError behavior, while
ensuring rapid clicks cannot start concurrent submissions.
In `@crates/draft-wasm/src/suggest.rs`:
- Around line 121-128: Update the suggested-deck contract around SuggestedDeck
so main_deck remains usable by existing spell-only consumers: expose a separate
engine-provided spell count or otherwise provide distinct spell and land data in
the shared contract, and update consumers to use that field. Ensure
LimitedDeckBuilder and other frontend code render the engine-provided values
without calculating or filtering card data in React.
🪄 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: d175a86d-ba50-44a3-9c39-72475874b139
📒 Files selected for processing (3)
client/src/components/draft/LimitedDeckBuilder.tsxcrates/draft-core/src/types.rscrates/draft-wasm/src/suggest.rs
| onClick={async () => { | ||
| setSubmitError(null); | ||
| try { | ||
| await submitDeck(); | ||
| } catch (err) { | ||
| const message = | ||
| err instanceof Error | ||
| ? err.message | ||
| : typeof err === "string" | ||
| ? err | ||
| : String(err); | ||
| setSubmitError(message); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent concurrent deck submissions.
This handler allows rapid clicks to invoke submitDeck() multiple times concurrently. Because each completion writes submitError, an older failed request can overwrite the result of a newer successful submission. Add an in-flight guard, disable the button while submitting, and clear the guard in finally.
As per path instructions, async races in frontend submission flows must be handled explicitly.
Suggested guard
+ const [isSubmitting, setIsSubmitting] = useState(false);
+
onClick={async () => {
+ if (isSubmitting) return;
+ setIsSubmitting(true);
setSubmitError(null);
try {
await submitDeck();
} catch (err) {
const message =
err instanceof Error
? err.message
: typeof err === "string"
? err
: String(err);
setSubmitError(message);
+ } finally {
+ setIsSubmitting(false);
}
}}
- disabled={!deckValid}
+ disabled={!deckValid || isSubmitting}📝 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.
| onClick={async () => { | |
| setSubmitError(null); | |
| try { | |
| await submitDeck(); | |
| } catch (err) { | |
| const message = | |
| err instanceof Error | |
| ? err.message | |
| : typeof err === "string" | |
| ? err | |
| : String(err); | |
| setSubmitError(message); | |
| } | |
| }} | |
| const [isSubmitting, setIsSubmitting] = useState(false); | |
| onClick={async () => { | |
| if (isSubmitting) return; | |
| setIsSubmitting(true); | |
| setSubmitError(null); | |
| try { | |
| await submitDeck(); | |
| } catch (err) { | |
| const message = | |
| err instanceof Error | |
| ? err.message | |
| : typeof err === "string" | |
| ? err | |
| : String(err); | |
| setSubmitError(message); | |
| } finally { | |
| setIsSubmitting(false); | |
| } | |
| }} |
🤖 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 366 - 379,
Update the submission handler in LimitedDeckBuilder’s onClick flow to guard
against an in-flight submitDeck call, disable the associated button while
submission is active, and clear the guard in a finally block. Preserve the
existing error extraction and setSubmitError behavior, while ensuring rapid
clicks cannot start concurrent submissions.
Source: Path instructions
| let mut main_deck = spell_names; | ||
| for (name, count) in nonbasic_lands { | ||
| *lands.entry(name).or_insert(0) += count; | ||
| for _ in 0..count { | ||
| main_deck.push(name.clone()); | ||
| } | ||
| } | ||
|
|
||
| SuggestedDeck { | ||
| main_deck: spell_names, | ||
| lands, | ||
| } | ||
| SuggestedDeck { main_deck, lands } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the main_deck consumer contract.
main_deck now includes nonbasic lands, but client/src/components/draft/LimitedDeckBuilder.tsx, Line 259 still passes mainDeck.length as the spells count. Every suggested dual will therefore inflate the UI’s spell count (and any other spell-only consumer). Expose a separate engine-provided spell count or update the shared deck contract; do not filter card data in React.
As per path instructions, the frontend must only render engine-provided state and must not calculate or filter game data.
🤖 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-wasm/src/suggest.rs` around lines 121 - 128, Update the
suggested-deck contract around SuggestedDeck so main_deck remains usable by
existing spell-only consumers: expose a separate engine-provided spell count or
otherwise provide distinct spell and land data in the shared contract, and
update consumers to use that field. Ensure LimitedDeckBuilder and other frontend
code render the engine-provided values without calculating or filtering card
data in React.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Keep main_deck's consumer contract coherent after admitting nonbasic lands. Evidence: suggest.rs:121-128 now inserts drafted duals into main_deck, while LimitedDeckBuilder.tsx:259 still passes mainDeck.length to the spells field rendered by DeckStatus as a spell count. Why it matters: every suggested nonbasic is shown as a spell, so the deck status misreports the constructed deck and any other spell-only consumer now receives a mixed card list. Suggested fix: make the shared display contract explicit—either expose the engine-owned spell count or change this UI path to render main_deck as mixed deck cards without reclassifying them in React; add a suggested-dual UI regression.
[MED] Prevent concurrent deck submissions and test the new error surface. Evidence: LimitedDeckBuilder.tsx:366-398 starts an unguarded async submitDeck for every click and writes submitError on completion; client/src/components/draft/__tests__/LimitedDeckBuilder.test.tsx has no submit rejection or in-flight test. Why it matters: rapid clicks can submit the same deck more than once and an older rejection can overwrite the state from a newer request; the PR's claimed in-app validation message is also unproven. Suggested fix: guard/disable while the promise is pending, clear it in finally, and add a component test that rejects onSubmitDeck and awaits the alert text.
2 participants
Summary
Fixes #6562: suggested drafted duals were placed in
lands, but the limited deckbuilder only exposeslandsfor addable basics andcomputeRemainingPoolonly subtractsmainDeck. Duals stayed visible in the pool, players could add them again, and submit expanded both maps →ExceedsPoolCount(often as a silent “deck validation failed”). Drafted nonbasics now go onmain_deck;landsstays basics-only.ValidationFailedDisplay includes per-card errors, and the submit button surfaces that message in-app.Files changed
crates/draft-wasm/src/suggest.rs— admit fixing lands ontomain_deck; update unit testscrates/draft-core/src/types.rs— includeLimitedDeckErrors inValidationFailedDisplay + regression testclient/src/components/draft/LimitedDeckBuilder.tsx— show submit validation errors in-appTrack
Developer
LLM
Model: Composer
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — draft-wasm / draft-core / limited deckbuilder UI, not
crates/engine/game rules. No/engine-implementerpath.CR references
None — Limited deck construction / tournament UI, not MTG Comprehensive Rules game logic.
Verification
cargo testdeferred — no Rust toolchain in this environment; CI will rundraft-wasm/draft-coretests.scripts/check-parser-combinators.sh origin/main— Gate A PASS (no parser files touched).Gate A
Gate A PASS head=6f2f0093ab36e2330a21da7a701abf66fdda18f2 base=e9a268a4d5ef4db00dd903ed2e1c1f6ceebcd95a
Anchored on
crates/draft-wasm/src/suggest.rs— existing comment that consumers concatenatemain_deck+lands(double-count hazard); nonbasics belong with drafted cards inmain_deckcrates/draft-core/src/validation.rs—LimitedDeckErroralready carries per-card Display text;ValidationFailednow forwards it the same way otherDraftErrorvariants surface detailFinal review-impl
Final review-impl PASS head=6f2f0093ab36e2330a21da7a701abf66fdda18f2 — non-engine draft suggest/validation Display fix; tests updated for main_deck placement and Display content.
Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Made with Cursor
Summary by CodeRabbit