Implement new card type 'Locations' - #1051
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (12)
📝 WalkthroughWalkthroughThis change adds Location entities, field storage, play and activation tasks, location targeting, replay support, and Castle Nathria card implementations. Existing field logic now distinguishes minions from locations. ChangesLocation support and minion-only field handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Player
participant PlayCard
participant PlayLocationTask
participant FieldZone
participant Location
Player->>PlayCard: play location card
PlayCard->>FieldZone: check field capacity
PlayCard->>PlayLocationTask: dispatch location play
PlayLocationTask->>Location: validate and activate
Location->>FieldZone: occupy field slot
PlayLocationTask->>Location: use location ability
Location->>Location: set cooldown and increase damage
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 4
🧹 Nitpick comments (4)
Tests/UnitTests/PlayMode/Zones/FieldZoneTests.cpp (1)
94-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the card-flow test explicit and deterministic.
Set the game format before
game.Start(). Replace the newCHECK_FALSEandCHECKassertions withCHECK_EQ. The test currently relies onGameConfigdefaults and does not follow the required assertion contract.As per coding guidelines,
Tests/UnitTests/**/*.cpp: “Drive C++ card tests through realistic game flow: initializeGameConfig, set the format, start the game, process toStep::MAIN_ACTION, draw and play cards through game tasks, and assert outcomes withCHECK_EQ.”🤖 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 `@Tests/UnitTests/PlayMode/Zones/FieldZoneTests.cpp` around lines 94 - 121, Update the “[FieldZone] - Location does not satisfy minion presence condition” test to set the intended game format on GameConfig before game.Start(). Replace both SelfCondition assertions with CHECK_EQ comparisons against the expected boolean values, while preserving the existing draw, play, and game-flow setup.Source: Coding guidelines
Tests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cpp (1)
5789-5817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new test with file conventions: set the format and follow the naming/header pattern.
This test omits
config.formatType. Every otherTEST_CASEin this file, including the existing CORE_GIL_598 test right above it, sets the format explicitly before starting the game.This test also does not follow the
"[Class : Type] - CARD_ID : Card Name"naming pattern and has no preceding card-info comment block. Every other test case in this file uses this pattern consistently.Set
config.formatTypeand rename the test case to match the established convention, for example"[Rogue : Minion] - CORE_GIL_598 : Tess Greymane"with a location-specific suffix, or add a short card-info comment block above it.As per path instructions, "initialize
GameConfig, set the format, start the game, process toStep::MAIN_ACTION, draw and play cards through game tasks, and assert outcomes withCHECK_EQ."🤖 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 `@Tests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cpp` around lines 5789 - 5817, Update the TEST_CASE for CORE_GIL_598 to set config.formatType consistently with the surrounding tests before starting the game, and rename it to the “[Rogue : Minion] - CARD_ID : Card Name” convention with a location-specific suffix if appropriate. Add the required card-info comment block above the test when following the file’s established header pattern, while preserving the existing setup, card-play flow, and CHECK_EQ assertions.Source: Path instructions
Sources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp (1)
1867-1889: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicate "replay played cards" logic (see consolidated comment).
This
CardType::LOCATIONcase duplicates the same block added inCoreCardsGen.cpp's Murozond and Tess Greymane replay logic. See the consolidated comment anchored inCoreCardsGen.cppfor the shared root cause and verification script.🤖 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 `@Sources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp` around lines 1867 - 1889, The CardType::LOCATION branch in the replay logic duplicates the shared replay-played-card handling from CoreCardsGen.cpp. Consolidate this behavior into the common replay implementation and remove the duplicate location-specific block here, preserving the existing field-capacity check and location play behavior through the shared path.Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cpp (1)
1508-1530: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicate "replay played cards" switch-over-CardType logic, now updated a third time.
The
CardType::LOCATIONcase added here duplicates the same block already present (and now also updated) in Tess Greymane's replay logic later in this file, and in Murozond's counterpart inDragonsCardsGen.cpp. This near-90-line switch statement overCardTypeis copy-pasted at least three times; each future addition (this PR'sLOCATIONcase) has to be applied identically everywhere, and a fourth copy (Tess Greymane's original, non-CORE_, definition) may exist outside this review batch and could have been missed.See the consolidated comment below for the shared root cause and the sites that need attention.
🤖 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 `@Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cpp` around lines 1508 - 1530, Consolidate the duplicated replay-played-cards CardType switch logic used by this block and the corresponding Tess Greymane and Murozond implementations into a shared helper, then replace each copy with calls to that helper. Ensure the shared implementation includes the CardType::LOCATION behavior and preserves the existing summon, location-play, field-capacity, and aura-update handling.
🤖 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 `@Includes/Rosetta/PlayMode/Models/Location.hpp`:
- Around line 17-21: Update Location durability handling so Location::Use(),
destruction, activation, and playability checks use GameTag::DURABILITY rather
than Character::DAMAGE or computed health. Revise GetHealth-related
expectations, comments, and tests to describe durability and ensure generic
character-health effects no longer affect Location activation state.
In `@Includes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hpp`:
- Around line 22-23: Mark the PlayLocationTask constructor explicit, matching
the constructor declaration for PlayCardTask and preventing implicit conversion
from Entity* when the optional target defaults to nullptr.
In `@Includes/Rosetta/PlayMode/Zones/FieldZone.hpp`:
- Around line 35-38: Update the Deathwing, Mad Aspect handler in the relevant
card effect to handle curField[GetCount() - 1] returning nullptr: select the
last available minion instead, or branch safely without calling Generic::Attack
or dereferencing a null target. Preserve the existing attack behavior for valid
minion targets.
In `@Tests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cpp`:
- Around line 278-290: Set config.formatType explicitly to the intended game
format before each affected card scenario: the parent fixture before the
Location subcase in Tests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cpp
lines 278-290, the Great Hall test in
Tests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cpp lines 631-743, and
the Demolition Renovator test in the same file lines 962-1012. Use the existing
test fixture configuration pattern.
---
Nitpick comments:
In `@Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cpp`:
- Around line 1508-1530: Consolidate the duplicated replay-played-cards CardType
switch logic used by this block and the corresponding Tess Greymane and Murozond
implementations into a shared helper, then replace each copy with calls to that
helper. Ensure the shared implementation includes the CardType::LOCATION
behavior and preserves the existing summon, location-play, field-capacity, and
aura-update handling.
In `@Sources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp`:
- Around line 1867-1889: The CardType::LOCATION branch in the replay logic
duplicates the shared replay-played-card handling from CoreCardsGen.cpp.
Consolidate this behavior into the common replay implementation and remove the
duplicate location-specific block here, preserving the existing field-capacity
check and location play behavior through the shared path.
In `@Tests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cpp`:
- Around line 5789-5817: Update the TEST_CASE for CORE_GIL_598 to set
config.formatType consistently with the surrounding tests before starting the
game, and rename it to the “[Rogue : Minion] - CARD_ID : Card Name” convention
with a location-specific suffix if appropriate. Add the required card-info
comment block above the test when following the file’s established header
pattern, while preserving the existing setup, card-play flow, and CHECK_EQ
assertions.
In `@Tests/UnitTests/PlayMode/Zones/FieldZoneTests.cpp`:
- Around line 94-121: Update the “[FieldZone] - Location does not satisfy minion
presence condition” test to set the intended game format on GameConfig before
game.Start(). Replace both SelfCondition assertions with CHECK_EQ comparisons
against the expected boolean values, while preserving the existing draw, play,
and game-flow setup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c0ba6d9-f45d-467a-befe-5c60c36b39bc
📒 Files selected for processing (67)
Documents/CardList - Standard.mdIncludes/Rosetta/Common/Utils.hppIncludes/Rosetta/PlayMode/Actions/PlayCard.hppIncludes/Rosetta/PlayMode/Models/Location.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/ConsecutiveDamageTask.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DamageTask.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DiscardTask.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/SummonTask.hppIncludes/Rosetta/PlayMode/Zones/FieldZone.hppIncludes/Rosetta/RosettaStone.hppREADME.mdSources/Rosetta/Battlegrounds/Cards/Card.cppSources/Rosetta/Battlegrounds/Tasks/SimpleTasks/CountTask.cppSources/Rosetta/PlayMode/Actions/Generic.cppSources/Rosetta/PlayMode/Actions/PlayCard.cppSources/Rosetta/PlayMode/Auras/AdjacentAura.cppSources/Rosetta/PlayMode/Auras/Aura.cppSources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cppSources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/DalaranCardsGen.cppSources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cppSources/Rosetta/PlayMode/CardSets/Expert1CardsGen.cppSources/Rosetta/PlayMode/CardSets/GangsCardsGen.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cppSources/Rosetta/PlayMode/CardSets/GvgCardsGen.cppSources/Rosetta/PlayMode/CardSets/LegacyCardsGen.cppSources/Rosetta/PlayMode/CardSets/LoECardsGen.cppSources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cppSources/Rosetta/PlayMode/CardSets/TheBarrensCardsGen.cppSources/Rosetta/PlayMode/CardSets/UldumCardsGen.cppSources/Rosetta/PlayMode/CardSets/VanillaCardsGen.cppSources/Rosetta/PlayMode/Cards/Card.cppSources/Rosetta/PlayMode/Conditions/SelfCondition.cppSources/Rosetta/PlayMode/Enchants/Enchant.cppSources/Rosetta/PlayMode/Games/Game.cppSources/Rosetta/PlayMode/Models/Character.cppSources/Rosetta/PlayMode/Models/Entity.cppSources/Rosetta/PlayMode/Models/Location.cppSources/Rosetta/PlayMode/Models/Minion.cppSources/Rosetta/PlayMode/Models/Playable.cppSources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.cppSources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DestroyAllTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DiscoverTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DrawSpellTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/RemoveEnchantmentTask.cppSources/Rosetta/PlayMode/Zones/FieldZone.cppTests/UnitTests/Common/UtilsTests.cppTests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cppTests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/ScholomanceCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/UldumCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/UngoroCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cppTests/UnitTests/PlayMode/Enchants/PlayerAuraEffectsTests.cppTests/UnitTests/PlayMode/Zones/FieldZoneTests.cppTests/UnitTests/Utils/CardSetHeaders.hppTests/UnitTests/doctest_proxy.hpp
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🔎 Static Analysis - SonarCloud
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-14
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-13
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-15
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,h,hpp}: Preserve C++17 portability across Ubuntu, macOS, Windows, and multiple compilers; isolate or guard compiler-specific assumptions with CMake.
Prefer the standard library and existing vendored libraries; do not add dependencies casually.
Files:
Sources/Rosetta/Battlegrounds/Cards/Card.cppSources/Rosetta/PlayMode/Models/Character.cppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks.hppIncludes/Rosetta/RosettaStone.hppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DrawSpellTask.cppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/SummonTask.hppSources/Rosetta/PlayMode/CardSets/GvgCardsGen.cppTests/UnitTests/Utils/CardSetHeaders.hppTests/UnitTests/Common/UtilsTests.cppSources/Rosetta/PlayMode/CardSets/LoECardsGen.cppTests/UnitTests/doctest_proxy.hppIncludes/Rosetta/Common/Utils.hppSources/Rosetta/PlayMode/Models/Playable.cppSources/Rosetta/PlayMode/CardSets/GangsCardsGen.cppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DiscoverTask.cppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.hppTests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cppSources/Rosetta/PlayMode/Enchants/Enchant.cppTests/UnitTests/PlayMode/Zones/FieldZoneTests.cppTests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cppSources/Rosetta/PlayMode/CardSets/UldumCardsGen.cppSources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cppTests/UnitTests/PlayMode/CardSets/ScholomanceCardsGenTests.cppIncludes/Rosetta/PlayMode/Actions/PlayCard.hppSources/Rosetta/PlayMode/Models/Entity.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/RemoveEnchantmentTask.cppTests/UnitTests/PlayMode/Enchants/PlayerAuraEffectsTests.cppSources/Rosetta/PlayMode/Actions/Generic.cppTests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cppSources/Rosetta/PlayMode/CardSets/DalaranCardsGen.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DestroyAllTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DamageTask.hppTests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/ConsecutiveDamageTask.hppSources/Rosetta/PlayMode/Models/Minion.cppSources/Rosetta/PlayMode/CardSets/LegacyCardsGen.cppSources/Rosetta/PlayMode/CardSets/TheBarrensCardsGen.cppSources/Rosetta/PlayMode/Games/Game.cppIncludes/Rosetta/PlayMode/Models/Location.hppTests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DiscardTask.hppSources/Rosetta/PlayMode/CardSets/VanillaCardsGen.cppTests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cppSources/Rosetta/Battlegrounds/Tasks/SimpleTasks/CountTask.cppTests/UnitTests/PlayMode/CardSets/UldumCardsGenTests.cppSources/Rosetta/PlayMode/Auras/AdjacentAura.cppSources/Rosetta/PlayMode/Models/Location.cppSources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.cppTests/UnitTests/PlayMode/CardSets/UngoroCardsGenTests.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeTask.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cppSources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/Expert1CardsGen.cppSources/Rosetta/PlayMode/Zones/FieldZone.cppSources/Rosetta/PlayMode/Actions/PlayCard.cppIncludes/Rosetta/PlayMode/Zones/FieldZone.hppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cppSources/Rosetta/PlayMode/Conditions/SelfCondition.cppSources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cppSources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cppSources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.cppSources/Rosetta/PlayMode/Cards/Card.cppSources/Rosetta/PlayMode/Auras/Aura.cpp
Includes/Rosetta/**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit the generated aggregate header
Includes/Rosetta/RosettaStone.hppby hand; modify source headers orScripts/header_gen.pyinstead.
Files:
Includes/Rosetta/PlayMode/Tasks/PlayerTasks.hppIncludes/Rosetta/RosettaStone.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/SummonTask.hppIncludes/Rosetta/Common/Utils.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.hppIncludes/Rosetta/PlayMode/Actions/PlayCard.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DamageTask.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/ConsecutiveDamageTask.hppIncludes/Rosetta/PlayMode/Models/Location.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DiscardTask.hppIncludes/Rosetta/PlayMode/Zones/FieldZone.hpp
{Includes,Rosetta}/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep Battlegrounds-only cards, tasks, behavior, and tests in the Battlegrounds trees rather than mixing them into PlayMode unless a shared type already exists outside both trees.
Files:
Includes/Rosetta/PlayMode/Tasks/PlayerTasks.hppIncludes/Rosetta/RosettaStone.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/SummonTask.hppIncludes/Rosetta/Common/Utils.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.hppIncludes/Rosetta/PlayMode/Actions/PlayCard.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DamageTask.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/ConsecutiveDamageTask.hppIncludes/Rosetta/PlayMode/Models/Location.hppIncludes/Rosetta/PlayMode/Tasks/SimpleTasks/DiscardTask.hppIncludes/Rosetta/PlayMode/Zones/FieldZone.hpp
Sources/Rosetta/PlayMode/Tasks/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Before adding a new task, search existing task implementations and
Documents/TaskList.md; prefer composing existing tasks such asIncludeTask,ConditionTask,DamageTask,DrawTask, andSummonTask.
Files:
Sources/Rosetta/PlayMode/Tasks/SimpleTasks/DrawSpellTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DiscoverTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/RemoveEnchantmentTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/DestroyAllTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cppSources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeTask.cppSources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.cpp
Sources/Rosetta/PlayMode/CardSets/*CardsGen.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Implement PlayMode card behavior in the matching checked-in
*CardsGen.cppfile; begin each card block withcardDef.ClearData(), setcardDef.powerandcardDef.property, and insert it withcards.emplace("CARD_ID", cardDef).
Files:
Sources/Rosetta/PlayMode/CardSets/GvgCardsGen.cppSources/Rosetta/PlayMode/CardSets/LoECardsGen.cppSources/Rosetta/PlayMode/CardSets/GangsCardsGen.cppSources/Rosetta/PlayMode/CardSets/UldumCardsGen.cppSources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cppSources/Rosetta/PlayMode/CardSets/DalaranCardsGen.cppSources/Rosetta/PlayMode/CardSets/LegacyCardsGen.cppSources/Rosetta/PlayMode/CardSets/TheBarrensCardsGen.cppSources/Rosetta/PlayMode/CardSets/VanillaCardsGen.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cppSources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/Expert1CardsGen.cppSources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cppSources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp
Sources/Rosetta/PlayMode/CardSets/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Sources/Rosetta/PlayMode/CardSets/*.cpp: Use existing engine building blocks for card effects, such asAddPowerTask,AddAura,AddEnchantmentTask,AddTrigger, and existing task chains, before adding new engine code.
Put targeting rules incardDef.property.playReqs, not in ad hoc task guards.
Files:
Sources/Rosetta/PlayMode/CardSets/GvgCardsGen.cppSources/Rosetta/PlayMode/CardSets/LoECardsGen.cppSources/Rosetta/PlayMode/CardSets/GangsCardsGen.cppSources/Rosetta/PlayMode/CardSets/UldumCardsGen.cppSources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cppSources/Rosetta/PlayMode/CardSets/DalaranCardsGen.cppSources/Rosetta/PlayMode/CardSets/LegacyCardsGen.cppSources/Rosetta/PlayMode/CardSets/TheBarrensCardsGen.cppSources/Rosetta/PlayMode/CardSets/VanillaCardsGen.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cppSources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/Expert1CardsGen.cppSources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cppSources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp
Tests/UnitTests/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Tests/UnitTests/**/*.cpp: Drive C++ card tests through realistic game flow: initializeGameConfig, set the format, start the game, process toStep::MAIN_ACTION, draw and play cards through game tasks, and assert outcomes withCHECK_EQ.
Run focused C++ unit tests for code or API changes; documentation-only changes generally do not require a build.
Files:
Tests/UnitTests/Common/UtilsTests.cppTests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cppTests/UnitTests/PlayMode/Zones/FieldZoneTests.cppTests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/ScholomanceCardsGenTests.cppTests/UnitTests/PlayMode/Enchants/PlayerAuraEffectsTests.cppTests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cppTests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/UldumCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/UngoroCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp
Tests/UnitTests/PlayMode/CardSets/*CardsGenTests.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Add focused scenario coverage in the matching
*CardsGenTests.cppfile for card changes.
Files:
Tests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/ScholomanceCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cppTests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/UldumCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/UngoroCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp
🪛 GitHub Check: Codacy Static Code Analysis
Includes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hpp
[warning] 23-23: Includes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hpp#L23
Class 'PlayLocationTask' has a constructor with 1 argument that is not explicit.
🪛 markdownlint-cli2 (0.23.1)
Documents/CardList - Standard.md
[warning] 1149-1149: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
[warning] 1149-1149: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (65)
Sources/Rosetta/PlayMode/Tasks/SimpleTasks/DrawSpellTask.cpp (1)
83-86: LGTM!Tests/UnitTests/Common/UtilsTests.cpp (1)
17-18: LGTM!Sources/Rosetta/PlayMode/Tasks/SimpleTasks/DiscoverTask.cpp (1)
126-126: LGTM!Also applies to: 674-686
Documents/CardList - Standard.md (1)
1030-1030: LGTM!Also applies to: 1148-1148, 1151-1151
README.md (1)
118-120: LGTM!Sources/Rosetta/Battlegrounds/Cards/Card.cpp (1)
118-125: LGTM!Sources/Rosetta/Battlegrounds/Tasks/SimpleTasks/CountTask.cpp (1)
58-58: LGTM!Tests/UnitTests/PlayMode/CardSets/UldumCardsGenTests.cpp (1)
1631-1631: LGTM!Also applies to: 1636-1639
Tests/UnitTests/PlayMode/CardSets/UngoroCardsGenTests.cpp (1)
181-182: LGTM!Also applies to: 193-193, 378-380, 423-423, 564-564
Tests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cpp (1)
1-1: LGTM!Also applies to: 21094-21094, 21166-21166
Tests/UnitTests/PlayMode/Enchants/PlayerAuraEffectsTests.cpp (1)
97-97: LGTM!Also applies to: 110-110, 123-123, 134-134
Tests/UnitTests/PlayMode/Zones/FieldZoneTests.cpp (1)
11-13: LGTM!Also applies to: 48-48, 72-92
Tests/UnitTests/Utils/CardSetHeaders.hpp (1)
10-12: LGTM!Also applies to: 25-25
Tests/UnitTests/doctest_proxy.hpp (1)
1-2: LGTM!Includes/Rosetta/PlayMode/Tasks/SimpleTasks/ConsecutiveDamageTask.hpp (1)
21-22: LGTM!Includes/Rosetta/PlayMode/Tasks/SimpleTasks/DamageTask.hpp (1)
21-22: LGTM!Includes/Rosetta/PlayMode/Tasks/SimpleTasks/DiscardTask.hpp (1)
19-19: LGTM!Includes/Rosetta/PlayMode/Tasks/SimpleTasks/SummonTask.hpp (1)
32-33: LGTM!Tests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cpp (1)
4429-4475: LGTM!Also applies to: 13769-13769
Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp (1)
3697-3744: LGTM!Also applies to: 4178-4178, 4201-4201, 4217-4223
Tests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cpp (1)
1-1: LGTM!Also applies to: 12010-12010, 14559-14559, 14625-14625
Tests/UnitTests/PlayMode/CardSets/GilneasCardsGenTests.cpp (1)
236-276: LGTM!Also applies to: 508-508
Tests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cpp (1)
1272-1272: LGTM!Tests/UnitTests/PlayMode/CardSets/ScholomanceCardsGenTests.cpp (1)
3690-3694: LGTM!Also applies to: 3827-3827, 4463-4465
Tests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cpp (1)
1289-1289: LGTM!Includes/Rosetta/Common/Utils.hpp (1)
66-67: LGTM!Includes/Rosetta/PlayMode/Actions/PlayCard.hpp (1)
9-9: LGTM!Also applies to: 56-61
Includes/Rosetta/PlayMode/Tasks/PlayerTasks.hpp (1)
14-14: LGTM!Includes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.hpp (1)
44-47: LGTM!Includes/Rosetta/PlayMode/Zones/FieldZone.hpp (1)
28-54: LGTM!Also applies to: 74-82
Includes/Rosetta/RosettaStone.hpp (1)
164-164: LGTM!Also applies to: 180-180
Sources/Rosetta/PlayMode/Auras/AdjacentAura.cpp (1)
77-85: LGTM!Also applies to: 98-106
Sources/Rosetta/PlayMode/Auras/Aura.cpp (1)
274-297: LGTM!Also applies to: 400-448, 502-514, 530-566, 671-694
Sources/Rosetta/PlayMode/Cards/Card.cpp (2)
21-26: LGTM!Also applies to: 487-524, 530-547, 599-609, 717-737
649-673: 🎯 Functional CorrectnessRemove this comment.
Player::GetFieldZone()is declaredconst, so the lambda compiles as written.> Likely an incorrect or invalid review comment.Sources/Rosetta/PlayMode/Models/Entity.cpp (1)
9-9: LGTM!Also applies to: 150-152
Sources/Rosetta/PlayMode/Models/Location.cpp (1)
1-71: LGTM!Sources/Rosetta/PlayMode/Zones/FieldZone.cpp (1)
21-78: LGTM!Also applies to: 95-97, 114-128, 191-191, 204-204
Sources/Rosetta/PlayMode/Actions/Generic.cpp (1)
11-11: LGTM!Also applies to: 255-258, 334-361
Sources/Rosetta/PlayMode/Actions/PlayCard.cpp (1)
29-40: LGTM!Also applies to: 126-131, 181-182, 193-193, 425-425, 558-567
Sources/Rosetta/PlayMode/Conditions/SelfCondition.cpp (1)
211-211: LGTM!Also applies to: 289-289, 301-301, 327-327, 338-338, 349-349, 1185-1186, 1215-1218
Sources/Rosetta/PlayMode/Models/Minion.cpp (1)
57-78: LGTM!Sources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cpp (1)
63-75: LGTM!Also applies to: 84-86
Sources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeTask.cpp (1)
50-73: LGTM!Also applies to: 84-110, 182-205, 216-249, 260-277, 313-343
Sources/Rosetta/PlayMode/Enchants/Enchant.cpp (1)
40-41: LGTM!Sources/Rosetta/PlayMode/Tasks/SimpleTasks/RemoveEnchantmentTask.cpp (1)
52-53: 🎯 Functional CorrectnessKeep
std::ranges::contains.The project requires C++23 through
Builds/CMake/CompileOptions.cmake, so this call is valid for the configured build.> Likely an incorrect or invalid review comment.Sources/Rosetta/PlayMode/Models/Playable.cpp (1)
86-86: LGTM!Also applies to: 428-431
Sources/Rosetta/PlayMode/Tasks/PlayerTasks/PlayCardTask.cpp (1)
29-33: LGTM!Sources/Rosetta/PlayMode/CardSets/LegacyCardsGen.cpp (1)
1-1: LGTM!Also applies to: 165-165, 3445-3448
Sources/Rosetta/PlayMode/CardSets/LoECardsGen.cpp (1)
537-537: LGTM!Sources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cpp (2)
1038-1039: LGTM!Also applies to: 1194-1195, 1156-1161, 1280-1283, 3667-3667
2985-2997: 🗄️ Data Integrity & IntegrationNo change needed for
REV_023targeting.REQ_TARGET_IF_AVAILABLEsetstargetingTypetoTargetingType::ENEMY_CHARACTERSthroughREQ_ENEMY_TARGET.CheckTargetingTypeaccepts enemyLocationobjects, andTargetingRequirementsenforces the location type.> Likely an incorrect or invalid review comment.Sources/Rosetta/PlayMode/CardSets/TheBarrensCardsGen.cpp (1)
1-1: LGTM!Also applies to: 927-927, 1043-1043, 2195-2195
Sources/Rosetta/PlayMode/CardSets/UldumCardsGen.cpp (1)
1819-1820: LGTM!Also applies to: 2233-2234
Sources/Rosetta/PlayMode/CardSets/VanillaCardsGen.cpp (1)
1-1: LGTM!Also applies to: 80-80, 6484-6487, 7379-7380
Sources/Rosetta/PlayMode/Models/Character.cpp (1)
221-221: LGTM!Sources/Rosetta/PlayMode/Tasks/SimpleTasks/DestroyAllTask.cpp (1)
30-30: LGTM!Also applies to: 39-40
Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cpp (1)
1-1: LGTM!Also applies to: 2088-2110, 4229-4230
Sources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp (1)
520-520: LGTM!Also applies to: 1150-1166, 1193-1219, 3301-3301, 4023-4024
Sources/Rosetta/PlayMode/CardSets/Expert1CardsGen.cpp (1)
1-1: LGTM!Also applies to: 5866-5867
Sources/Rosetta/PlayMode/CardSets/GangsCardsGen.cpp (1)
967-967: LGTM!Sources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cpp (3)
861-861: LGTM!
873-883: LGTM!
2368-2368: LGTM!Sources/Rosetta/PlayMode/CardSets/GvgCardsGen.cpp (1)
1133-1133: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1051 +/- ##
==========================================
+ Coverage 94.62% 95.11% +0.49%
==========================================
Files 304 307 +3
Lines 33857 33869 +12
Branches 1778 1751 -27
==========================================
+ Hits 32037 32216 +179
+ Misses 1820 1653 -167 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Sources/Rosetta/PlayMode/Zones/FieldZone.cpp (1)
32-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCount minions without building a temporary vector.
GetMinionCount()allocates astd::vector<Minion*>on every call.AdaptiveCostEffectfor Sea Giant calls it for both players on each aura update, so this runs on a hot path. Count the entities directly.♻️ Proposed allocation-free implementation
int FieldZone::GetMinionCount() const { - return static_cast<int>(GetMinions().size()); + int count = 0; + + for (const auto entity : PositioningZone::GetAll()) + { + if (dynamic_cast<Minion*>(entity)) + { + ++count; + } + } + + return count; }🤖 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 `@Sources/Rosetta/PlayMode/Zones/FieldZone.cpp` around lines 32 - 35, Update FieldZone::GetMinionCount() to count qualifying minion entities directly instead of calling GetMinions() and materializing a temporary vector; preserve the existing integer count semantics and filtering behavior used by GetMinions().Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp (1)
2781-2782: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse a distinct
SUBCASEname for the middle-location scenario.Doctest distinguishes these subcases by name, file, and line, so both scenarios execute. Rename the second subcase to make test reports unambiguous.
🤖 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 `@Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp` around lines 2781 - 2782, Rename the second location-related SUBCASE in the relevant test from the duplicate name to a distinct name identifying the middle-location scenario, while leaving the test logic unchanged.
🤖 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 `@Sources/Rosetta/PlayMode/Actions/PlayCard.cpp`:
- Around line 593-601: Update the target selection logic after
GetValidPlayTargets so Random::get is called only when validTargets is
non-empty; preserve the existing early return for mustHaveToTargetToPlay and use
a null target for empty optional-target lists without computing
validTargets.size() - 1.
- Around line 34-37: Update the CardType::LOCATION branch in CanPlayCard so it
evaluates source->IsPlayableByCardReq(chooseOne) instead of returning solely
from the target-null check. Preserve the location targeting requirement while
ensuring cardDef.property.playReqs are enforced for locations.
- Around line 628-633: Update the discover loop around ChoicePick to handle an
empty player->choice->choices collection explicitly, then generate a random
index from 0 through size() - 1 and pass choices[choiceIdx] as the entity ID.
Ensure the empty case exits or otherwise clears the pending choice so the loop
cannot hang.
In `@Tests/UnitTests/PlayMode/Auras/AuraTests.cpp`:
- Around line 40-42: Update the AddLocation setup in the aura test to play the
location through PlayCardTask::Location instead of calling field.Add directly.
Ensure the test uses the established game-flow setup, including GameConfig
initialization, format selection, starting the game, processing to
Step::MAIN_ACTION, and handling the location from the player’s hand so
Generic::PlayLocation, PLAY_CARD triggers, mana payment, and hand removal
execute normally.
---
Nitpick comments:
In `@Sources/Rosetta/PlayMode/Zones/FieldZone.cpp`:
- Around line 32-35: Update FieldZone::GetMinionCount() to count qualifying
minion entities directly instead of calling GetMinions() and materializing a
temporary vector; preserve the existing integer count semantics and filtering
behavior used by GetMinions().
In `@Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp`:
- Around line 2781-2782: Rename the second location-related SUBCASE in the
relevant test from the duplicate name to a distinct name identifying the
middle-location scenario, while leaving the test logic unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb1e6e06-6950-48a1-982f-d4b8d06d0cde
📒 Files selected for processing (28)
Folder.DotSettings.userIncludes/Rosetta/PlayMode/Actions/PlayCard.hppIncludes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hppIncludes/Rosetta/PlayMode/Zones/FieldZone.hppSources/Rosetta/PlayMode/Actions/Generic.cppSources/Rosetta/PlayMode/Actions/PlayCard.cppSources/Rosetta/PlayMode/Auras/AdjacentAura.cppSources/Rosetta/PlayMode/Auras/Aura.cppSources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cppSources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cppSources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cppSources/Rosetta/PlayMode/Cards/Card.cppSources/Rosetta/PlayMode/Models/Minion.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeTask.cppSources/Rosetta/PlayMode/Zones/FieldZone.cppTests/UnitTests/PlayMode/Actions/GenericTests.cppTests/UnitTests/PlayMode/Auras/AuraTests.cppTests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cppTests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cppTests/UnitTests/PlayMode/Zones/FieldZoneTests.cppTests/UnitTests/main.cpp
💤 Files with no reviewable changes (1)
- Folder.DotSettings.user
🚧 Files skipped from review as they are similar to previous changes (16)
- Tests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cpp
- Tests/UnitTests/PlayMode/Zones/FieldZoneTests.cpp
- Sources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cpp
- Includes/Rosetta/PlayMode/Tasks/PlayerTasks/PlayLocationTask.hpp
- Sources/Rosetta/PlayMode/Models/Minion.cpp
- Sources/Rosetta/PlayMode/Auras/Aura.cpp
- Tests/UnitTests/PlayMode/CardSets/RevendrethCardsGenTests.cpp
- Sources/Rosetta/PlayMode/Actions/Generic.cpp
- Sources/Rosetta/PlayMode/CardSets/RevendrethCardsGen.cpp
- Includes/Rosetta/PlayMode/Zones/FieldZone.hpp
- Tests/UnitTests/PlayMode/CardSets/VanillaCardsGenTests.cpp
- Sources/Rosetta/PlayMode/CardSets/DragonsCardsGen.cpp
- Tests/UnitTests/PlayMode/CardSets/Expert1CardsGenTests.cpp
- Sources/Rosetta/PlayMode/Cards/Card.cpp
- Tests/UnitTests/PlayMode/CardSets/CoreCardsGenTests.cpp
- Sources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeTask.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-14
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-15
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-13
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🔎 Static Analysis - SonarCloud
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,h,hpp}: Preserve C++17 portability across Ubuntu, macOS, Windows, and multiple compilers; isolate or guard compiler-specific assumptions with CMake.
Prefer the standard library and existing vendored libraries; do not add dependencies casually.
Files:
Tests/UnitTests/PlayMode/Auras/AuraTests.cppTests/UnitTests/PlayMode/Actions/GenericTests.cppTests/UnitTests/main.cppIncludes/Rosetta/PlayMode/Actions/PlayCard.hppSources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/Auras/AdjacentAura.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cppSources/Rosetta/PlayMode/Actions/PlayCard.cppSources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cppSources/Rosetta/PlayMode/Zones/FieldZone.cpp
Tests/UnitTests/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Tests/UnitTests/**/*.cpp: Drive C++ card tests through realistic game flow: initializeGameConfig, set the format, start the game, process toStep::MAIN_ACTION, draw and play cards through game tasks, and assert outcomes withCHECK_EQ.
Run focused C++ unit tests for code or API changes; documentation-only changes generally do not require a build.
Files:
Tests/UnitTests/PlayMode/Auras/AuraTests.cppTests/UnitTests/PlayMode/Actions/GenericTests.cppTests/UnitTests/main.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp
Includes/Rosetta/**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit the generated aggregate header
Includes/Rosetta/RosettaStone.hppby hand; modify source headers orScripts/header_gen.pyinstead.
Files:
Includes/Rosetta/PlayMode/Actions/PlayCard.hpp
{Includes,Rosetta}/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep Battlegrounds-only cards, tasks, behavior, and tests in the Battlegrounds trees rather than mixing them into PlayMode unless a shared type already exists outside both trees.
Files:
Includes/Rosetta/PlayMode/Actions/PlayCard.hpp
Sources/Rosetta/PlayMode/CardSets/*CardsGen.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Implement PlayMode card behavior in the matching checked-in
*CardsGen.cppfile; begin each card block withcardDef.ClearData(), setcardDef.powerandcardDef.property, and insert it withcards.emplace("CARD_ID", cardDef).
Files:
Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cpp
Sources/Rosetta/PlayMode/CardSets/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Sources/Rosetta/PlayMode/CardSets/*.cpp: Use existing engine building blocks for card effects, such asAddPowerTask,AddAura,AddEnchantmentTask,AddTrigger, and existing task chains, before adding new engine code.
Put targeting rules incardDef.property.playReqs, not in ad hoc task guards.
Files:
Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cppSources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cpp
Sources/Rosetta/PlayMode/Tasks/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Before adding a new task, search existing task implementations and
Documents/TaskList.md; prefer composing existing tasks such asIncludeTask,ConditionTask,DamageTask,DrawTask, andSummonTask.
Files:
Sources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cpp
Tests/UnitTests/PlayMode/CardSets/*CardsGenTests.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Add focused scenario coverage in the matching
*CardsGenTests.cppfile for card changes.
Files:
Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp
🔇 Additional comments (14)
Includes/Rosetta/PlayMode/Actions/PlayCard.hpp (1)
57-66: LGTM!Sources/Rosetta/PlayMode/Zones/FieldZone.cpp (2)
67-107: LGTM!Also applies to: 195-206
21-25: 🩺 Stability & AvailabilityNo null-dereference call sites remain. All PlayMode
FieldZone::operator[]callers check the returned pointer before dereference.> Likely an incorrect or invalid review comment.Tests/UnitTests/PlayMode/Actions/GenericTests.cpp (1)
159-163: LGTM!Sources/Rosetta/PlayMode/Actions/PlayCard.cpp (1)
53-59: LGTM!Also applies to: 143-153, 215-215, 447-447, 580-589
Sources/Rosetta/PlayMode/Auras/AdjacentAura.cpp (1)
79-84: LGTM!Also applies to: 100-105, 185-185
Sources/Rosetta/PlayMode/Tasks/SimpleTasks/IncludeAdjacentTask.cpp (1)
63-91: LGTM!Sources/Rosetta/PlayMode/CardSets/CoreCardsGen.cpp (1)
1485-1485: LGTM!Also applies to: 1967-1967, 4033-4034
Sources/Rosetta/PlayMode/CardSets/GilneasCardsGen.cpp (1)
838-838: LGTM!Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp (2)
3765-3811: LGTM!Also applies to: 6585-6603
2750-2751: 🎯 Functional CorrectnessKeep
Cards::FindCardByName("Great Hall").
REV_983is registered as"Great Hall", so this lookup is correct.> Likely an incorrect or invalid review comment.Tests/UnitTests/main.cpp (1)
15-17: LGTM!Tests/UnitTests/PlayMode/Auras/AuraTests.cpp (2)
20-38: LGTM!Also applies to: 49-67
43-47: 🩺 Stability & AvailabilityDo not flag the stack-local
Aura.Aura::Activatecopies its state into a game-owned clone, andGame::AddAuraretains that clone. The clone does not reference the prototype, so the scopes at lines 43–47 and 75–85 do not cause a lifetime error.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Tests/UnitTests/PlayMode/Actions/GenericTests.cpp (4)
62-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the task-driven card flow.
Generic::DrawCardandGeneric::PlayCardcall the lower-level actions directly. Use the corresponding player-task flow so this test also covers task wiring.As per coding guidelines, card tests must draw and play cards through game tasks.
🤖 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 `@Tests/UnitTests/PlayMode/Actions/GenericTests.cpp` around lines 62 - 64, Update the test flow around Generic::DrawCard and Generic::PlayCard to draw and play the card through the corresponding player-task APIs instead of calling the lower-level Generic actions directly, while preserving the existing player and card setup and assertions.Source: Coding guidelines
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CHECK_EQfor the new outcome assertions.Line 87 uses
CHECK_FALSE, and Line 213 usesCHECK. Replace both with explicit equality assertions to follow the repository test convention.As per coding guidelines, tests under
Tests/UnitTests/**/*.cppmust assert outcomes withCHECK_EQ.Also applies to: 211-213
🤖 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 `@Tests/UnitTests/PlayMode/Actions/GenericTests.cpp` at line 87, Replace the outcome assertions around player->choice in the affected tests with explicit CHECK_EQ assertions, updating both the CHECK_FALSE assertion near line 87 and the CHECK assertion near lines 211-213 to compare against their expected boolean values.Source: Coding guidelines
211-213: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete Location replacement contract.
Line 213 only proves that
Generic::ChangeEntityproduced aLocation. Also assert that the replacement retainsentityIDandplayer->GetHandZone()membership. A type-correct replacement can still lose identity or zone state.🤖 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 `@Tests/UnitTests/PlayMode/Actions/GenericTests.cpp` around lines 211 - 213, Extend the assertions after Generic::ChangeEntity to verify that the replacement Location retains entityID and remains present in player->GetHandZone(), in addition to the existing type check. Use the returned entity and hand-zone collection to validate identity and membership without changing the replacement setup.
56-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the satisfied
REQ_WEAPON_EQUIPPEDcase.This test only verifies the rejected play. Add a separate flow that equips a weapon, plays the same Location, and checks that it enters the field and leaves the hand. Otherwise, code that rejects every Location still passes.
As per PR objectives, the test should verify the Location play requirement, not only the failure path.
🤖 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 `@Tests/UnitTests/PlayMode/Actions/GenericTests.cpp` around lines 56 - 68, Extend the test around Generic::PlayCard with a separate satisfied-requirement flow: equip a weapon for player, play the same Location requiring REQ_WEAPON_EQUIPPED, and assert it leaves the hand and enters the field. Preserve the existing rejected-play assertions so both failure and success paths verify the Location requirement.
🤖 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 `@Tests/UnitTests/PlayMode/Actions/GenericTests.cpp`:
- Around line 45-50: Set GameConfig::formatType explicitly in the test
configuration block before Game::Start(), using the format intended by this
Location test. Do not rely on GameConfig’s default value; keep the existing
player, deck, and auto-run settings unchanged.
---
Nitpick comments:
In `@Tests/UnitTests/PlayMode/Actions/GenericTests.cpp`:
- Around line 62-64: Update the test flow around Generic::DrawCard and
Generic::PlayCard to draw and play the card through the corresponding
player-task APIs instead of calling the lower-level Generic actions directly,
while preserving the existing player and card setup and assertions.
- Line 87: Replace the outcome assertions around player->choice in the affected
tests with explicit CHECK_EQ assertions, updating both the CHECK_FALSE assertion
near line 87 and the CHECK assertion near lines 211-213 to compare against their
expected boolean values.
- Around line 211-213: Extend the assertions after Generic::ChangeEntity to
verify that the replacement Location retains entityID and remains present in
player->GetHandZone(), in addition to the existing type check. Use the returned
entity and hand-zone collection to validate identity and membership without
changing the replacement setup.
- Around line 56-68: Extend the test around Generic::PlayCard with a separate
satisfied-requirement flow: equip a weapon for player, play the same Location
requiring REQ_WEAPON_EQUIPPED, and assert it leaves the hand and enters the
field. Preserve the existing rejected-play assertions so both failure and
success paths verify the Location requirement.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4e25075-b92e-44b0-9faa-dcf318ce93cf
📒 Files selected for processing (9)
Includes/Rosetta/PlayMode/Zones/FieldZone.hppSources/Rosetta/PlayMode/Actions/PlayCard.cppSources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cppSources/Rosetta/PlayMode/Cards/Card.cppSources/Rosetta/PlayMode/Zones/FieldZone.cppTests/UnitTests/PlayMode/Actions/GenericTests.cppTests/UnitTests/PlayMode/Auras/AuraTests.cppTests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cppTests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- Sources/Rosetta/PlayMode/CardSets/BlackTempleCardsGen.cpp
- Tests/UnitTests/PlayMode/CardSets/TheBarrensCardsGenTests.cpp
- Tests/UnitTests/PlayMode/Auras/AuraTests.cpp
- Tests/UnitTests/PlayMode/CardSets/DragonsCardsGenTests.cpp
- Includes/Rosetta/PlayMode/Zones/FieldZone.hpp
- Sources/Rosetta/PlayMode/Cards/Card.cpp
- Sources/Rosetta/PlayMode/Zones/FieldZone.cpp
- Sources/Rosetta/PlayMode/Actions/PlayCard.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🔎 Static Analysis - SonarCloud
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-14
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-13
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🍎 Build - macOS 26.3 + gcc-15
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,h,hpp}: Preserve C++17 portability across Ubuntu, macOS, Windows, and multiple compilers; isolate or guard compiler-specific assumptions with CMake.
Prefer the standard library and existing vendored libraries; do not add dependencies casually.
Files:
Tests/UnitTests/PlayMode/Actions/GenericTests.cpp
Tests/UnitTests/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Tests/UnitTests/**/*.cpp: Drive C++ card tests through realistic game flow: initializeGameConfig, set the format, start the game, process toStep::MAIN_ACTION, draw and play cards through game tasks, and assert outcomes withCHECK_EQ.
Run focused C++ unit tests for code or API changes; documentation-only changes generally do not require a build.
Files:
Tests/UnitTests/PlayMode/Actions/GenericTests.cpp
🔇 Additional comments (1)
Tests/UnitTests/PlayMode/Actions/GenericTests.cpp (1)
12-21: LGTM!
|
1 participant
This revision includes:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation