ship/storm ci review followups - #7044
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
📝 WalkthroughWalkthroughThe PR adds a reusable storm-copy badge to hand views and updates engine trigger doubling to use captured source context and LKI snapshots. Integration tests cover Veyran’s handling of controlled cast triggers and creature spell self-triggers. ChangesStorm-copy badge UI
Storm trigger source scope
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Veyran
participant TriggerDoubling
participant CapturedSource
participant LkiSnapshot
Veyran->>TriggerDoubling: evaluate trigger source
TriggerDoubling->>CapturedSource: check zone and incarnation
CapturedSource->>LkiSnapshot: use LKI when source left battlefield
LkiSnapshot-->>TriggerDoubling: return source filter match
TriggerDoubling-->>Veyran: apply doubling decision
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@crates/engine/src/game/triggers.rs`:
- Around line 7805-7843: Update trigger_source_matches_doubler_filter so a
missing trigger.ability.trigger_source falls back to matches_target_filter
against the live trigger.source_id when the source is still available, instead
of immediately returning false; preserve the existing captured-context and LKI
handling when source context exists.
In `@crates/engine/tests/integration/veyran_storm_source_scope.rs`:
- Around line 65-93: Add an integration test alongside
veyran_does_not_double_cast_trigger_of_permanent_spell that removes Veyran
before the cast-trigger doubling step, using cast(...).commit() to exercise the
LKI path. Assert the trigger source is evaluated from its last-known battlefield
state and the controlled permanent’s cast trigger is doubled, while preserving
the existing non-doubling coverage.
🪄 Autofix
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: 4d0a553b-ca05-476f-8349-7358b21b0b79
📒 Files selected for processing (9)
client/src/components/hand/MobileHandDrawer.tsxclient/src/components/hand/MobileHeldHandCard.tsxclient/src/components/hand/PlayerHand.tsxclient/src/components/hand/StormCopyBadge.tsxcrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/tests/integration/granted_storm_snapshot.rscrates/engine/tests/integration/veyran_storm_source_scope.rscrates/phase-ai/baselines/perf-baseline.json
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
…#6826 cost (phase-rs#7053) phase-rs#7044 refreshed the baseline onto the phase-rs#6826 regression itself (layers_full_eval 3495 -> 15877). Once phase-rs#7049 fixed that regression the gate carried ~3x slack -- real cost 5464 against a 15877 baseline under a ~6% band -- so it could no longer catch a new decision-cost regression. Re-measured on a clean worktree against an immutable card-data snapshot (scoped card_data_hash 670a4a14, covering 46/46 scenario deck cards): counter | old | new | threshold layers_full_eval | 15877 | 5464 | 5801 state_clone_for_legality | 19342 | 11099 | 11717 restriction_static_mode_gate_scans | 155747 | 110891 | 116499 sba_battlefield_snapshot_builds | 27462 | 19626 | 20671 scripts/validate-ai-perf-reproducibility.sh: PASSED (margin+band). 25/25 band runs clean, "0 OVER-MARGIN of 29 counters", and worst_current == baseline on every counter across 125 cold processes. CI budget: T_run_max 90s * 2.5 + T_build ~630s = ~14.3 min < 25 min. (The script reported T_build=1s from a warm cache; the 630s figure is the cold isolated server-release build measured separately, which is the conservative ceiling the check intends.) git_sha stamps 7f5f0b4, where the 25-run validation ran. Re-confirmed against f26f4e3 (current main, including phase-rs#7051 and phase-rs#7017): 0 FAIL, 29 PASS, +0 on every counter. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes