cr733(p2): journal cost-settlement consume-before-priority occurrences#6558
Conversation
📝 WalkthroughWalkthroughCost settlement now resolves consumed-before-priority trigger occurrences through ChangesConsumed-trigger settlement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CostSettlement
participant TriggerResolver
participant ResolvedRulesJournal
CostSettlement->>TriggerResolver: submit ConsumeBeforePriority occurrences
TriggerResolver->>ResolvedRulesJournal: record TriggerCollection command
TriggerResolver-->>CostSettlement: apply collected triggers
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
crates/engine/src/game/casting_costs.rs (1)
2387-2400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the consumed-occurrence builder into one shared helper. Both cost-settlement sites build
Vec<ConsumedTriggerEventOccurrence>with identical prefix-count semantics; keeping two copies risks divergence of the dedup index. A single helper (e.g.crate::game::triggers::consumed_occurrences(events: &[GameEvent], start: usize, end: usize)) covers both slice shapes.
crates/engine/src/game/casting_costs.rs#L2387-L2400: replace the inline loop with a call passingcurrent_start/current_end.crates/engine/src/game/mana_abilities.rs#L2494-L2507: replace theskip(current_start)inline loop with the same helper passingcurrent_startandevents.len().🤖 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/casting_costs.rs` around lines 2387 - 2400, Extract the shared prefix-count construction into a helper such as consumed_occurrences in the triggers module, accepting events, start, and end and returning Vec<ConsumedTriggerEventOccurrence>. In crates/engine/src/game/casting_costs.rs:2387-2400, replace the inline builder with the helper using current_start and current_end; in crates/engine/src/game/mana_abilities.rs:2494-2507, replace the skip-based builder with the same helper using current_start and events.len().Source: Coding guidelines
🤖 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/engine/tests/integration/cr733_resolved_trigger_collection.rs`:
- Around line 93-105: Update current_action_occurrences and its fixtures so
occurrence indices match the full events prefix used by both settle_* call
sites, including events before current_start. Add a genuinely repeated GameEvent
to the fixture and ensure the expected occurrences assert a non-zero dedup
index, exercising ConsumeBeforePriority behavior.
---
Nitpick comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 2387-2400: Extract the shared prefix-count construction into a
helper such as consumed_occurrences in the triggers module, accepting events,
start, and end and returning Vec<ConsumedTriggerEventOccurrence>. In
crates/engine/src/game/casting_costs.rs:2387-2400, replace the inline builder
with the helper using current_start and current_end; in
crates/engine/src/game/mana_abilities.rs:2494-2507, replace the skip-based
builder with the same helper using current_start and events.len().
🪄 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: 19484ce8-ea28-42d7-a130-f068fb1a4408
📒 Files selected for processing (3)
crates/engine/src/game/casting_costs.rscrates/engine/src/game/mana_abilities.rscrates/engine/tests/integration/cr733_resolved_trigger_collection.rs
| fn current_action_occurrences(events: &[GameEvent]) -> Vec<ConsumedTriggerEventOccurrence> { | ||
| events | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(index, event)| ConsumedTriggerEventOccurrence { | ||
| event: event.clone(), | ||
| occurrence: events[..index] | ||
| .iter() | ||
| .filter(|prior| *prior == event) | ||
| .count(), | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Helper occurrence semantics diverge from production; assertion may not exercise the dedup index.
current_action_occurrences counts prior identical events only within the passed slice, whereas both settle_* sites count over the full events prefix (events[..index] including pre-current_start events). These agree here only because every fixture event is unique, so occurrence is always 0. That means the occurrences == &expected assertion never actually validates a non-zero dedup index — the core behavior of ConsumeBeforePriority. Consider a fixture that emits a genuinely repeated GameEvent so a non-zero occurrence is asserted, or align the helper to the production global-prefix semantics.
🤖 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/tests/integration/cr733_resolved_trigger_collection.rs` around
lines 93 - 105, Update current_action_occurrences and its fixtures so occurrence
indices match the full events prefix used by both settle_* call sites, including
events before current_start. Add a genuinely repeated GameEvent to the fixture
and ensure the expected occurrences assert a non-zero dedup index, exercising
ConsumeBeforePriority behavior.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
phase-rs#6558) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
1 participant
Summary by CodeRabbit
Bug Fixes
Tests