Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion client/src/components/draft/LimitedDeckBuilder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,8 @@ export function LimitedDeckBuilder({

const [hoveredCard, setHoveredCard] = useState<CardHoverInfo | null>(null);

const [submitError, setSubmitError] = useState<string | null>(null);

const pool = useMemo(() => view?.pool ?? [], [view?.pool]);

const remainingPool = useMemo(
Expand Down Expand Up @@ -361,7 +363,20 @@ export function LimitedDeckBuilder({

<button
type="button"
onClick={submitDeck}
onClick={async () => {
setSubmitError(null);
try {
await submitDeck();
} catch (err) {
const message =
err instanceof Error
? err.message
: typeof err === "string"
? err
: String(err);
setSubmitError(message);
}
}}
Comment on lines +366 to +379

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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

disabled={!deckValid}
className={menuButtonClass({
tone: "emerald",
Expand All @@ -372,6 +387,14 @@ export function LimitedDeckBuilder({
>
{t("limitedDeck.submitDeck")}
</button>
{submitError ? (
<p
role="alert"
className="rounded-md border border-red-500/40 bg-red-950/40 px-2 py-1.5 text-xs text-red-200"
>
{submitError}
</p>
) : null}
</section>
</div>
</div>
Expand Down
34 changes: 33 additions & 1 deletion crates/draft-core/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,14 @@ pub enum DraftError {
CardNotInPack { card_instance_id: String },
#[error("seat {seat} has no pending pack")]
NoPendingPack { seat: u8 },
#[error("deck validation failed")]
#[error(
"deck validation failed: {}",
errors
.iter()
.map(ToString::to_string)
.collect::<Vec<_>>()
.join("; ")
)]
ValidationFailed { errors: Vec<LimitedDeckError> },
#[error("pairing not found: {match_id}")]
PairingNotFound { match_id: String },
Expand Down Expand Up @@ -716,4 +723,29 @@ mod tests {
let config: DraftConfig = serde_json::from_str(json).unwrap();
assert_eq!(config.spectator_visibility, SpectatorVisibility::Public);
}

#[test]
fn validation_failed_display_includes_per_card_errors() {
let err = DraftError::ValidationFailed {
errors: vec![
LimitedDeckError::ExceedsPoolCount {
name: "Hell's Kitchen".to_string(),
requested: 2,
available: 1,
},
LimitedDeckError::NotInPool {
name: "Ghost Card".to_string(),
},
],
};
let message = err.to_string();
assert!(
message.contains("Hell's Kitchen") && message.contains("Ghost Card"),
"Display must name offending cards, got {message}"
);
assert!(
message.contains("used 2 times") || message.contains("not in the drafted pool"),
"Display must include reasons, got {message}"
);
}
}
46 changes: 33 additions & 13 deletions crates/draft-wasm/src/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,17 @@ pub fn suggest_deck(
}
}

// `main_deck` holds the non-land spells only; `lands` carries the land
// distribution separately. Consumers (the deckbuilder store, `get_bot_deck`)
// concatenate the two — appending lands here as well would double-count them
// (e.g. 23 spells + 17 lands in `main_deck`, then +17 lands again = 57).
// `main_deck` holds drafted cards (spells + nonbasic fixing lands); `lands`
// carries only the always-addable basic fill. Consumers (the deckbuilder
// store, `get_bot_deck`) concatenate the two — never put basics into
// `main_deck` or they'd double-count (e.g. 23 spells + 17 lands in
// `main_deck`, then +17 lands again = 57).
//
// Drafted nonbasics must live in `main_deck`, not `lands`: the limited
// deckbuilder UI only exposes `lands` for `addable_cards` (basics), and
// `computeRemainingPool` only subtracts `mainDeck`. Putting duals in
// `lands` left them visible in the pool so players could add them again,
// then submit expanded both maps → ExceedsPoolCount (#6562).
let spell_names: Vec<String> = spells.iter().map(|c| c.name.clone()).collect();
let land_total = min_deck_size.saturating_sub(spell_names.len()) as u8;

Expand All @@ -109,15 +116,16 @@ pub fn suggest_deck(
};
let nonbasic_count: u8 = nonbasic_lands.values().copied().sum();
let basics_total = land_total.saturating_sub(nonbasic_count);
let mut lands = suggest_addable_cards(&spell_names, pool, basics_total, addable_cards);
let lands = suggest_addable_cards(&spell_names, pool, basics_total, addable_cards);

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 }
Comment on lines +121 to +128

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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

}

/// On-color drafted nonbasic fixing lands as a `name -> copy-count` map, capped at
Expand Down Expand Up @@ -480,10 +488,20 @@ mod tests {
&DeckAddableCards::standard_basics(),
);
assert!(
deck.lands.contains_key("On Color Dual"),
"on-color (W/U) fixing land should be admitted to the manabase, got {:?}",
deck.main_deck.iter().any(|n| n == "On Color Dual"),
"on-color (W/U) fixing land should be admitted onto main_deck, got {:?}",
deck.main_deck
);
assert!(
!deck.lands.contains_key("On Color Dual"),
"drafted nonbasics must not go in lands (basics-only map), got {:?}",
deck.lands
);
assert!(
!deck.main_deck.iter().any(|n| n == "Off Color Dual"),
"off-color (B/R) fixing land must not be admitted, got {:?}",
deck.main_deck
);
assert!(
!deck.lands.contains_key("Off Color Dual"),
"off-color (B/R) fixing land must not be admitted, got {:?}",
Expand All @@ -506,7 +524,8 @@ mod tests {
assert_eq!(
deck.main_deck.len() as u32 + land_count,
8,
"spells + lands must equal min_deck_size; lands = {:?}",
"main_deck + basics must equal min_deck_size; main={:?} lands={:?}",
deck.main_deck,
deck.lands
);
}
Expand All @@ -522,6 +541,7 @@ mod tests {
8,
&DeckAddableCards::standard_basics(),
);
assert!(!deck.main_deck.iter().any(|n| n == "On Color Dual"));
assert!(!deck.lands.contains_key("On Color Dual"));
}
}
Loading