fix: clarify replacement modal choices#6612
Conversation
|
Warning Review limit reached
Next review available in: 30 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 (5)
📝 WalkthroughWalkthroughThe client validates brace-delimited mana shards before rendering symbols and formats TapLandForMana choices from selected mana. The engine centralizes optional replacement labels, adds commander destination descriptions, expands cost labels, and adds coverage for these outputs. ChangesMana label rendering
Replacement choice labels
Estimated code review effort: 4 (Complex) | ~45 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: 3
🧹 Nitpick comments (1)
crates/engine/src/game/replacement.rs (1)
11636-11667: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name promises both destinations but only exercises
Library.The
Zone::Handarm at Line 1028 is uncovered, so a wrong/typo'd "Put into hand" label ships green. Parameterizing over both zones covers the full destination axis of the new labeler.♻️ Proposed refactor: drive both destinations
#[test] fn commander_library_replacement_labels_both_destinations() { let commander = ObjectId(21); - let mut state = test_state_with_object(commander, Zone::Battlefield, vec![]); - state.pending_replacement = Some(PendingReplacement { - proposed: ProposedEvent::zone_change(commander, Zone::Battlefield, Zone::Library, None), - sacrifice_provenance: None, - candidates: vec![commander_hand_or_library_return_replacement_id(commander)], - search_found_candidates: Vec::new(), - depth: 0, - is_optional: true, - library_placement: None, - excess_recipient: None, - lifelink_bonus: 0, - may_cost_paid: false, - may_cost_remaining: None, - }); - - let WaitingFor::ReplacementChoice { candidates, .. } = - replacement_choice_waiting_for(PlayerId(0), &state) - else { - panic!("expected commander replacement choice"); - }; - - assert_eq!( - candidates - .iter() - .map(|candidate| candidate.description.as_str()) - .collect::<Vec<_>>(), - vec!["Move to command zone", "Put into library"], - "CR 903.9b choices must name the resulting zone, not generic accept/decline" - ); + for (destination, decline_label) in [ + (Zone::Hand, "Put into hand"), + (Zone::Library, "Put into library"), + ] { + let mut state = test_state_with_object(commander, Zone::Battlefield, vec![]); + state.pending_replacement = Some(PendingReplacement { + proposed: ProposedEvent::zone_change( + commander, + Zone::Battlefield, + destination, + None, + ), + sacrifice_provenance: None, + candidates: vec![commander_hand_or_library_return_replacement_id(commander)], + search_found_candidates: Vec::new(), + depth: 0, + is_optional: true, + library_placement: None, + excess_recipient: None, + lifelink_bonus: 0, + may_cost_paid: false, + may_cost_remaining: None, + }); + + let WaitingFor::ReplacementChoice { candidates, .. } = + replacement_choice_waiting_for(PlayerId(0), &state) + else { + panic!("expected commander replacement choice for {destination:?}"); + }; + + assert_eq!( + candidates + .iter() + .map(|candidate| candidate.description.as_str()) + .collect::<Vec<_>>(), + vec!["Move to command zone", decline_label], + "CR 903.9b choices must name the resulting zone, not generic accept/decline" + ); + } }🤖 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/replacement.rs` around lines 11636 - 11667, Update commander_library_replacement_labels_both_destinations to exercise both Zone::Hand and Zone::Library destinations, preferably by parameterizing the test over those zones and using the selected zone in the proposed event. Preserve the expected command-zone choice and assert the destination-specific label, including “Put into hand” for Hand and “Put into library” for Library.
🤖 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/mana/__tests__/ManaSymbol.test.tsx`:
- Around line 13-17: Update the assertion in the “keeps non-mana brace content
as text” test to match the rendered span’s full text content, using the complete
“Pay Fixed { value: 2 } life” string while preserving the existing assertion
that no image role is rendered.
In `@client/src/components/mana/ManaSymbol.tsx`:
- Around line 22-23: Update isManaSymbolShard in ManaSymbol.tsx to accept only
the explicitly supported mana-shard composite codes, rejecting combinations
containing unsupported tokens such as X while preserving valid numeric and
symbol composites. Add a regression test covering unsupported inputs like W/X
and 2/X and confirming they use the fallback behavior.
In `@crates/engine/src/game/replacement.rs`:
- Around line 1044-1046: Update the ReplacementMode::MayCost arm to retain its
decline payload and derive the decline label from the decline ability's
description, matching the Optional arm's behavior; preserve "Decline" as the
fallback when no decline ability is present.
---
Nitpick comments:
In `@crates/engine/src/game/replacement.rs`:
- Around line 11636-11667: Update
commander_library_replacement_labels_both_destinations to exercise both
Zone::Hand and Zone::Library destinations, preferably by parameterizing the test
over those zones and using the selected zone in the proposed event. Preserve the
expected command-zone choice and assert the destination-specific label,
including “Put into hand” for Hand and “Put into library” for Library.
🪄 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: f7f6bc34-8c69-4fa5-9ffd-f437c2ff8cc1
📒 Files selected for processing (6)
client/src/components/mana/ManaSymbol.tsxclient/src/components/mana/RichLabel.tsxclient/src/components/mana/__tests__/ManaSymbol.test.tsxclient/src/viewmodel/__tests__/cardActionChoice.test.tsclient/src/viewmodel/costLabel.tscrates/engine/src/game/replacement.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests