Tianpok/refactor status effect domain events#24
Merged
Conversation
📝 WalkthroughWalkthroughCentralized attack resolution into a new immutable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player as Player
participant Combatant as Attacker(Combatant)
participant Registry as StatusEffectRegistry
participant Target as Target(Combatant)
participant Engine as BattleEngine
Player->>Combatant: attack(target) / attack(target, power)
Combatant->>Registry: modifyIncomingDamage(owner=target, attacker=this, damage)
Registry-->>Combatant: DamageAdjustment(damage, modifiers)
Combatant->>Target: receiveDamage(adjustedDamage)
Target-->>Combatant: hpBefore,hpAfter,eliminated
Combatant->>Registry: record/prune effects, collect outcomes (onApply/onExpire/onRoundEnd)
Combatant-->>Player: AttackResolution(attackUsed,targetDefense,hpBefore,hpAfter,damage,eliminated,statusEffectOutcomes)
Combatant->>Engine: emit ActionEvent(actor, actionName, target, AttackResolution)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 7
🧹 Nitpick comments (2)
src/test/java/sc2002/turnbased/domain/status/SmokeBombStatusEffectTest.java (1)
77-98: Consider simplifying the notes assertion.The current approach of extracting individual notes via
get(0)and comparing to a constructed list works, but could be more direct.♻️ Optional: Simplify notes assertions
assertAll( () -> assertEquals(0, firstAdjustment.damage()), () -> assertEquals(0, secondAdjustment.damage()), () -> assertEquals(25, thirdAdjustment.damage()), - () -> assertEquals( - List.of("Smoke Bomb blocked the attack", "Smoke Bomb blocked the attack"), - List.of(firstAdjustment.notes().get(0), secondAdjustment.notes().get(0)) - ), + () -> assertEquals(List.of("Smoke Bomb blocked the attack"), firstAdjustment.notes()), + () -> assertEquals(List.of("Smoke Bomb blocked the attack"), secondAdjustment.notes()), () -> assertEquals(List.of(), thirdAdjustment.notes()), () -> assertTrue(effect.isExpired()) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sc2002/turnbased/domain/status/SmokeBombStatusEffectTest.java` around lines 77 - 98, The notes assertion can be simplified by directly comparing the notes lists instead of extracting get(0) entries; update the assertions in SmokeBombStatusEffectTest (using firstAdjustment, secondAdjustment, thirdAdjustment and their notes() results) to assert that firstAdjustment.notes() equals List.of("Smoke Bomb blocked the attack") and secondAdjustment.notes() equals List.of("Smoke Bomb blocked the attack"), and assert that thirdAdjustment.notes() equals List.of() (or emptyList()); keep the other damage and isExpired assertions unchanged.src/main/java/sc2002/turnbased/domain/status/StunStatusEffect.java (1)
33-41: Note:getTurnBlockReasonhas side effects.This method decrements
blockedTurnsRemainingwhen blocking a turn, which could be unexpected given the "get" prefix. However, this pattern is consistent with how status effects are consumed when activated. Consider renaming toconsumeBlockedTurn()or adding a brief doc comment explaining the mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/domain/status/StunStatusEffect.java` around lines 33 - 41, getTurnBlockReason in StunStatusEffect mutates blockedTurnsRemaining (it decrements the counter) which is surprising for a getter; rename getTurnBlockReason to consumeBlockedTurn (or similar) to signal side effects, update all callers, and keep the same behavior (decrement blockedTurnsRemaining and return Optional.of(description()) when >0, Optional.empty() when 0); alternatively, if renaming is too invasive, add a concise Javadoc to getTurnBlockReason documenting that it consumes one blocked turn by decrementing blockedTurnsRemaining and returns the blocking reason via description(), so callers know it has side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/sc2002/turnbased/actions/DefendAction.java`:
- Around line 24-26: DefendAction is returning presentation strings by calling
actor.addStatusEffect(...) which ultimately produces List<String> via
StatusEffectRegistry.add and DefendStatusEffect.onApply; instead, change
DefendAction to return a structured domain event/value (e.g., a
StatusEffectApplied domain object) rather than StatusEffectReportEvent populated
with strings. Concretely, have actor.addStatusEffect(...) (or a new method)
return a semantic result object (like StatusEffectApplicationResult or
StatusEffectInstance) and construct a domain-level StatusEffectAppliedEvent from
that, leaving rendering of human-readable messages to the reporting/presentation
layer; update usages of StatusEffectReportEvent so it accepts these structured
domain objects rather than List<String>.
In
`@src/main/java/sc2002/turnbased/domain/status/DefaultStatusEffectRegistryFactory.java`:
- Around line 17-18: The factory method
DefaultStatusEffectRegistryFactory.create() currently returns
statusEffectRegistrySupplier.get() without validating the produced registry;
modify create() to fail fast if the supplier returns null by checking the result
of statusEffectRegistrySupplier.get() (e.g. via Objects.requireNonNull or an
explicit null check) and throw a clear exception (IllegalStateException or NPE)
with a descriptive message so wiring errors are detected at the factory
boundary.
In `@src/main/java/sc2002/turnbased/domain/status/StatusEffectRegistry.java`:
- Around line 80-83: pruneExpiredEffects currently appends onExpire(...) output
to the shared pendingNotes buffer, causing side-effects during read-only calls
like activeStatuses() and apply(); change pruneExpiredEffects(Combatant) to
return a List<String> of expiry notes instead of mutating pendingNotes, then
update callers: have mutating operations (e.g., apply(), any methods that should
report expiries) collect the returned notes and append them to pendingNotes,
while read-only methods (activeStatuses(), other query paths around lines 93-95
and 104-123) call pruneExpiredEffects but ignore or discard the returned notes
so no global state is changed. Ensure references to onExpire(...) remain but are
used only to build the returned List<String> inside pruneExpiredEffects.
- Around line 36-49: In adjustIncomingDamage (StatusEffectRegistry), ensure the
final damage value cannot be negative by clamping damage to zero before
constructing and returning the DamageAdjustment; keep collecting and consuming
the notes as-is so existing status effect notes (from modifyIncomingDamage and
onExpire) are preserved—i.e., after the loop but before new
DamageAdjustment(damage, ...), set damage = Math.max(0, damage) (or equivalent)
so DamageAdjustment never carries a negative damage value used by
Combatant.resolveAttackFrom.
In `@src/main/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffect.java`:
- Around line 13-22: The constructor StrengthBoostStatusEffect currently permits
roundsRemaining == 0 which yields an immediately-expired effect and misleading
onApply messaging; update the validation in the StrengthBoostStatusEffect(int
attackBonus, int roundsRemaining) constructor to require roundsRemaining > 0
(throw IllegalArgumentException with a clear message like "roundsRemaining must
be positive") and leave attackBonus validation as-is, and update or add a short
comment on the constructor to document that roundsRemaining must be positive; if
zero is intended instead, add a clarifying comment and adjust onApply()
messaging to handle the zero-case.
In `@src/main/java/sc2002/turnbased/domain/status/StunStatusEffect.java`:
- Around line 11-16: The constructor StunStatusEffect currently allows
blockedTurnsRemaining == 0 which creates an immediately-expired stun; change the
validation to require blockedTurnsRemaining > 0 (e.g., if (blockedTurnsRemaining
<= 0) throw new IllegalArgumentException("blockedTurnsRemaining must be
positive")) so only positive turn counts are accepted, matching the approach
used in StrengthBoostStatusEffect; update the error message accordingly.
In `@src/main/java/sc2002/turnbased/report/StatusEffectReportEvent.java`:
- Around line 6-8: The event currently exposes raw narration strings
(StatusEffectReportEvent with List<String> statusEffectNotes) which loses
structured state; change the payload to a typed value object (e.g.,
StatusEffectNote) or a specific domain event type and update the record
signature to use List<StatusEffectNote> statusEffectNotes, keeping the defensive
copy and null check (List.copyOf(Objects.requireNonNull(...))) in the compact
constructor; then update all producers/consumers of StatusEffectReportEvent to
construct and consume the StatusEffectNote instances and move any string
formatting into the formatter/renderer layer rather than in the event.
---
Nitpick comments:
In `@src/main/java/sc2002/turnbased/domain/status/StunStatusEffect.java`:
- Around line 33-41: getTurnBlockReason in StunStatusEffect mutates
blockedTurnsRemaining (it decrements the counter) which is surprising for a
getter; rename getTurnBlockReason to consumeBlockedTurn (or similar) to signal
side effects, update all callers, and keep the same behavior (decrement
blockedTurnsRemaining and return Optional.of(description()) when >0,
Optional.empty() when 0); alternatively, if renaming is too invasive, add a
concise Javadoc to getTurnBlockReason documenting that it consumes one blocked
turn by decrementing blockedTurnsRemaining and returns the blocking reason via
description(), so callers know it has side effects.
In `@src/test/java/sc2002/turnbased/domain/status/SmokeBombStatusEffectTest.java`:
- Around line 77-98: The notes assertion can be simplified by directly comparing
the notes lists instead of extracting get(0) entries; update the assertions in
SmokeBombStatusEffectTest (using firstAdjustment, secondAdjustment,
thirdAdjustment and their notes() results) to assert that
firstAdjustment.notes() equals List.of("Smoke Bomb blocked the attack") and
secondAdjustment.notes() equals List.of("Smoke Bomb blocked the attack"), and
assert that thirdAdjustment.notes() equals List.of() (or emptyList()); keep the
other damage and isExpired assertions 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9825057-8eae-4508-8b36-fdbe5336ded6
📒 Files selected for processing (68)
src/main/java/sc2002/turnbased/actions/ArcaneBlastAction.javasrc/main/java/sc2002/turnbased/actions/BasicAttackAction.javasrc/main/java/sc2002/turnbased/actions/DefendAction.javasrc/main/java/sc2002/turnbased/actions/ShieldBashAction.javasrc/main/java/sc2002/turnbased/actions/UsePowerStoneSkillAction.javasrc/main/java/sc2002/turnbased/actions/UseSmokeBombAction.javasrc/main/java/sc2002/turnbased/actions/UseSpecialSkillAction.javasrc/main/java/sc2002/turnbased/domain/AttackResolution.javasrc/main/java/sc2002/turnbased/domain/Combatant.javasrc/main/java/sc2002/turnbased/domain/CombatantId.javasrc/main/java/sc2002/turnbased/domain/EnemyCombatant.javasrc/main/java/sc2002/turnbased/domain/PlayerCharacter.javasrc/main/java/sc2002/turnbased/domain/SpecialSkill.javasrc/main/java/sc2002/turnbased/domain/status/ArcanePowerStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/DamageAdjustment.javasrc/main/java/sc2002/turnbased/domain/status/DefaultStatusEffectRegistryFactory.javasrc/main/java/sc2002/turnbased/domain/status/DefendStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/IncomingDamageModifierEffect.javasrc/main/java/sc2002/turnbased/domain/status/MergeableStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/SmokeBombStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/StatModifierEffect.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectEventPublisher.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectKind.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectObservationScope.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectObserver.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectRegistry.javasrc/main/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/StunStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/TurnEffectResolution.javasrc/main/java/sc2002/turnbased/domain/status/TurnInterferingEffect.javasrc/main/java/sc2002/turnbased/domain/status/TurnWindow.javasrc/main/java/sc2002/turnbased/domain/status/event/ArcanePowerAppliedEvent.javasrc/main/java/sc2002/turnbased/domain/status/event/DefendAppliedEvent.javasrc/main/java/sc2002/turnbased/domain/status/event/SmokeBombActivatedEvent.javasrc/main/java/sc2002/turnbased/domain/status/event/SmokeBombAppliedEvent.javasrc/main/java/sc2002/turnbased/domain/status/event/StatusEffectEvent.javasrc/main/java/sc2002/turnbased/domain/status/event/StatusEffectExpiredEvent.javasrc/main/java/sc2002/turnbased/domain/status/event/StunAppliedEvent.javasrc/main/java/sc2002/turnbased/engine/BattleEngine.javasrc/main/java/sc2002/turnbased/engine/EasyLevelSetup.javasrc/main/java/sc2002/turnbased/engine/HardLevelSetup.javasrc/main/java/sc2002/turnbased/engine/MediumLevelSetup.javasrc/main/java/sc2002/turnbased/report/ActionEvent.javasrc/main/java/sc2002/turnbased/report/SkippedTurnEvent.javasrc/main/java/sc2002/turnbased/report/StatusEffectReportEvent.javasrc/main/java/sc2002/turnbased/ui/BattleConsoleFormatter.javasrc/main/java/sc2002/turnbased/ui/CliPlayerDecisionProvider.javasrc/main/java/sc2002/turnbased/ui/ConsoleBattleUi.javasrc/main/java/sc2002/turnbased/ui/TurnBasedArenaCli.javasrc/main/java/sc2002/turnbased/ui/gui/GuiPlayerDecisionProvider.javasrc/main/java/sc2002/turnbased/ui/gui/TurnBasedArenaGui.javasrc/test/java/sc2002/turnbased/actions/ArcaneBlastActionTest.javasrc/test/java/sc2002/turnbased/domain/CombatantTest.javasrc/test/java/sc2002/turnbased/domain/EnemyCombatantTest.javasrc/test/java/sc2002/turnbased/domain/PlayerCharacterTest.javasrc/test/java/sc2002/turnbased/domain/status/ArcanePowerStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/DefendStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/SmokeBombStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/StatusEffectRegistryTest.javasrc/test/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/StunStatusEffectTest.javasrc/test/java/sc2002/turnbased/e2e/CustomBattleFlowE2ETest.javasrc/test/java/sc2002/turnbased/integration/BattleEngineAppendixAScenariosIntegrationTest.javasrc/test/java/sc2002/turnbased/integration/SmokeBombBattleFlowIntegrationTest.javasrc/test/java/sc2002/turnbased/integration/StatusEffectObservationScopeBattleFlowIntegrationTest.javasrc/test/java/sc2002/turnbased/support/FakeStatusEffectEventPublisher.javasrc/test/java/sc2002/turnbased/support/TestDependencies.java
💤 Files with no reviewable changes (18)
- src/main/java/sc2002/turnbased/domain/status/StatModifierEffect.java
- src/main/java/sc2002/turnbased/domain/status/MergeableStatusEffect.java
- src/main/java/sc2002/turnbased/domain/status/event/SmokeBombAppliedEvent.java
- src/main/java/sc2002/turnbased/domain/status/TurnInterferingEffect.java
- src/main/java/sc2002/turnbased/domain/status/event/ArcanePowerAppliedEvent.java
- src/main/java/sc2002/turnbased/domain/status/StatusEffectEventPublisher.java
- src/main/java/sc2002/turnbased/domain/status/event/StunAppliedEvent.java
- src/main/java/sc2002/turnbased/domain/status/event/StatusEffectExpiredEvent.java
- src/main/java/sc2002/turnbased/domain/status/TurnWindow.java
- src/main/java/sc2002/turnbased/domain/status/event/SmokeBombActivatedEvent.java
- src/main/java/sc2002/turnbased/domain/status/TurnEffectResolution.java
- src/test/java/sc2002/turnbased/integration/StatusEffectObservationScopeBattleFlowIntegrationTest.java
- src/main/java/sc2002/turnbased/domain/status/IncomingDamageModifierEffect.java
- src/main/java/sc2002/turnbased/domain/status/event/DefendAppliedEvent.java
- src/main/java/sc2002/turnbased/domain/status/StatusEffectObservationScope.java
- src/main/java/sc2002/turnbased/domain/status/event/StatusEffectEvent.java
- src/main/java/sc2002/turnbased/domain/status/StatusEffectObserver.java
- src/test/java/sc2002/turnbased/support/FakeStatusEffectEventPublisher.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/sc2002/turnbased/domain/status/StatusEffectChange.java (1)
11-20: Tighten the invariants bytype.The factories only build sensible states, but the public constructor still allows impossible combinations like
EXPIREDwith a non-zeromagnitudeorduration. Guarding that here will keep the new outcome type self-consistent.Suggested guard
public StatusEffectChange { Objects.requireNonNull(source, "source"); Objects.requireNonNull(type, "type"); if (magnitude < 0) { throw new IllegalArgumentException("magnitude must not be negative"); } if (duration < 0) { throw new IllegalArgumentException("duration must not be negative"); } + if (type == StatusEffectChangeType.EXPIRED && (magnitude != 0 || duration != 0)) { + throw new IllegalArgumentException("expired changes must not carry magnitude or duration"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/domain/status/StatusEffectChange.java` around lines 11 - 20, The constructor for StatusEffectChange must enforce that certain Outcome/Type values are self-consistent: add a guard in the public constructor (the StatusEffectChange(...) initializer block) validating the 'type' value so impossible combinations are rejected — e.g., if type == EXPIRED then require magnitude == 0 and duration == 0 (throw IllegalArgumentException otherwise); keep the existing null checks and non-negative checks for magnitude and duration and throw similarly descriptive IllegalArgumentException when the invariant is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/sc2002/turnbased/engine/BattleEngine.java`:
- Around line 232-236: The current BattleEngine.completeRound() calls
player.completeRound() and enemy.completeRound() but ignores their return
values; change BattleEngine.completeRound() so it captures the
CombatantStatusOutcome(s) returned by Combatant.completeRound() for the player
and for each Combatant in spawnedEnemies, and forward those outcomes into the
engine's event stream/queue using the engine's existing event dispatch method
(e.g., call the event dispatch/publish/addEvent helper your engine uses) so
end-of-round expiries/notes are emitted rather than dropped.
- Around line 101-120: The code currently calls
actor.consumeStatusEffectOutcomes() unconditionally which drains status outcomes
even when the actor takes a normal turn; change the logic in BattleEngine so you
only call actor.consumeStatusEffectOutcomes() inside the branches that emit
skipped-turn events (i.e., after checking actor.isAlive() and when
actor.getTurnBlockReason() is present) and pass those outcomes to
SkippedTurnEvent.fromStatusEffectOutcomes; leave the normal-turn path untouched
so no outcomes are consumed unless a skipped/ELIMINATED event is being emitted
(keep emit(...) and battleEventListener usage the same).
---
Nitpick comments:
In `@src/main/java/sc2002/turnbased/domain/status/StatusEffectChange.java`:
- Around line 11-20: The constructor for StatusEffectChange must enforce that
certain Outcome/Type values are self-consistent: add a guard in the public
constructor (the StatusEffectChange(...) initializer block) validating the
'type' value so impossible combinations are rejected — e.g., if type == EXPIRED
then require magnitude == 0 and duration == 0 (throw IllegalArgumentException
otherwise); keep the existing null checks and non-negative checks for magnitude
and duration and throw similarly descriptive IllegalArgumentException when the
invariant is violated.
🪄 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: 4093f7bf-e623-4f44-8021-2e0be4fefff2
📒 Files selected for processing (33)
src/main/java/sc2002/turnbased/actions/ArcaneBlastAction.javasrc/main/java/sc2002/turnbased/actions/DefendAction.javasrc/main/java/sc2002/turnbased/actions/ShieldBashAction.javasrc/main/java/sc2002/turnbased/actions/UseSmokeBombAction.javasrc/main/java/sc2002/turnbased/domain/AttackResolution.javasrc/main/java/sc2002/turnbased/domain/Combatant.javasrc/main/java/sc2002/turnbased/domain/status/ArcanePowerStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/CombatantStatusOutcome.javasrc/main/java/sc2002/turnbased/domain/status/DamageAdjustment.javasrc/main/java/sc2002/turnbased/domain/status/DamageModifier.javasrc/main/java/sc2002/turnbased/domain/status/DamageModifierType.javasrc/main/java/sc2002/turnbased/domain/status/DefaultStatusEffectRegistryFactory.javasrc/main/java/sc2002/turnbased/domain/status/DefendStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/SmokeBombStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectChange.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectChangeType.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectOutcome.javasrc/main/java/sc2002/turnbased/domain/status/StatusEffectRegistry.javasrc/main/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffect.javasrc/main/java/sc2002/turnbased/domain/status/StunStatusEffect.javasrc/main/java/sc2002/turnbased/engine/BattleEngine.javasrc/main/java/sc2002/turnbased/report/ActionEvent.javasrc/main/java/sc2002/turnbased/report/SkippedTurnEvent.javasrc/main/java/sc2002/turnbased/report/StatusEffectReportEvent.javasrc/main/java/sc2002/turnbased/report/StatusEffectReportMapper.javasrc/test/java/sc2002/turnbased/domain/CombatantTest.javasrc/test/java/sc2002/turnbased/domain/status/ArcanePowerStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/DefaultStatusEffectRegistryFactoryTest.javasrc/test/java/sc2002/turnbased/domain/status/SmokeBombStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/StatusEffectRegistryTest.javasrc/test/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffectTest.javasrc/test/java/sc2002/turnbased/domain/status/StunStatusEffectTest.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/sc2002/turnbased/domain/status/StatusEffectChangeType.java
- src/main/java/sc2002/turnbased/domain/status/DamageModifierType.java
- src/main/java/sc2002/turnbased/actions/UseSmokeBombAction.java
🚧 Files skipped from review as they are similar to previous changes (13)
- src/main/java/sc2002/turnbased/actions/DefendAction.java
- src/test/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffectTest.java
- src/test/java/sc2002/turnbased/domain/status/ArcanePowerStatusEffectTest.java
- src/main/java/sc2002/turnbased/domain/status/DamageAdjustment.java
- src/main/java/sc2002/turnbased/report/StatusEffectReportEvent.java
- src/main/java/sc2002/turnbased/report/ActionEvent.java
- src/main/java/sc2002/turnbased/domain/status/ArcanePowerStatusEffect.java
- src/main/java/sc2002/turnbased/domain/status/StatusEffect.java
- src/main/java/sc2002/turnbased/domain/status/StrengthBoostStatusEffect.java
- src/main/java/sc2002/turnbased/report/SkippedTurnEvent.java
- src/test/java/sc2002/turnbased/domain/status/SmokeBombStatusEffectTest.java
- src/test/java/sc2002/turnbased/domain/status/StunStatusEffectTest.java
- src/main/java/sc2002/turnbased/domain/status/StatusEffectRegistry.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/sc2002/turnbased/engine/BattleEngine.java`:
- Around line 61-63: The RoundSummaryEvent is created from stale status data
because createRoundSummary() is called before completeRound() mutates status
expiry; reorder the calls so completeRound(battleEventListener) runs before
emit(createRoundSummary(roundNumber), battleEventListener), or alternatively
modify createRoundSummary() to query getActiveStatuses() after completeRound()
completes; apply the same reorder/adjustment to the analogous block around the
code referenced at lines 236-250 to ensure RoundSummaryEvent and
StatusEffectReportEvent see a consistent, post-completion status view
(referencing spawnBackupIfNeeded, createRoundSummary, completeRound, emit,
getActiveStatuses, RoundSummaryEvent, StatusEffectReportEvent).
- Around line 102-115: The code calls actor.getTurnBlockReason() before checking
actor.isAlive(), which lets getTurnBlockReason() expire effects and queue
outcomes for actors that are already dead; move the getTurnBlockReason()
invocation so the isAlive() check (and the
SkippedTurnEvent.fromStatusEffectOutcomes/actor.consumeStatusEffectOutcomes()
branch) runs first—i.e., check actor.isAlive() and handle
elimination/consumeStatusEffectOutcomes() before calling getTurnBlockReason() to
avoid mutating state for eliminated combatants.
🪄 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: 6309ef9e-13fd-48b1-a449-cd48eb46476e
📒 Files selected for processing (2)
src/main/java/sc2002/turnbased/engine/BattleEngine.javasrc/test/java/sc2002/turnbased/engine/BattleEngineTest.java
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor