Tianpok/2d gameplay upgrade#35
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the GUI into an MVC/controller-driven design: replaces modal decision dialogs with a controller-backed decision queue, introduces a 2D arena renderer and sprite pipeline with input/animation/playback, reorganizes UI into composed panels, and updates README run/controls documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Player
participant View as BattleView
participant Controller as BattleController
participant Model as BattleSessionModel
participant Engine as BattleEngine
participant Playback as BattlePlaybackController
Player->>View: startBattle(request)
View->>Controller: startBattle(request)
Controller->>Model: beginBattle()
Controller->>View: showBattleLoading()
Controller->>Engine: (async) run BattleEngine with provider
Engine->>Controller: request decision (provider.decide)
Controller->>Model: queuePlayerTurn(PlayerTurnRequest)
Controller->>View: showPlayerTurn(turn)
Player->>View: sendCommand(command)
View->>Controller: handleCommand(command)
Controller->>Controller: PlayerCommandResolver.resolve(...)
Controller->>Model: takeQueuedPlayerTurn() -> active turn
Controller->>Controller: offer decision into turn.responseQueue
Controller->>View: showCommandResolving(actionName)
Engine->>Controller: emits BattleEvent
Controller->>Playback: enqueue(event)
Playback->>View: showBattleEvent(event, message, transcript)
View->>View: update sprites / HUD / banners
Engine->>Controller: battle ends
Controller->>View: showBattleComplete()
View->>Player: askPostGameChoice()
Player->>Controller: (REPLAY / NEW_SETUP / EXIT)
Controller->>Model: finishBattle()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
src/main/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommand.java (1)
5-10: Add null-contract validation in the record constructor.This model looks like a required-data transport object; allowing null
decision,actionName, ortargetLabelcan push failures downstream.Proposed fix
+import java.util.Objects; + public record ResolvedPlayerCommand( PlayerDecision decision, String actionName, String targetLabel ) { + public ResolvedPlayerCommand { + decision = Objects.requireNonNull(decision, "decision"); + actionName = Objects.requireNonNull(actionName, "actionName"); + targetLabel = Objects.requireNonNull(targetLabel, "targetLabel"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommand.java` around lines 5 - 10, ResolvedPlayerCommand currently allows null values for its components (decision, actionName, targetLabel); add a compact record constructor in ResolvedPlayerCommand that validates each component (e.g., via Objects.requireNonNull) and throws a clear NPE when any is null so callers fail fast; reference the record name ResolvedPlayerCommand and component names decision, actionName, targetLabel when making the change.src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteDtoTest.java (1)
43-89: Add a regression test for mismatchedCombatantIdupdates.Given the mutable sprite state, it’s worth adding one negative test that
updateFrom(...)rejects a different fighter id to prevent accidental state crossover.Suggested test shape
+ `@Test` + void rejectsUpdateFromDifferentCombatantId() { + FighterSpriteDto sprite = FighterSpriteDto.fromSummary( + new CombatantSummary(CombatantId.generate(), "Wolf", 30, 45, 12, 12, true, List.of()), + false + ); + CombatantSummary other = new CombatantSummary( + CombatantId.generate(), "Goblin", 20, 20, 8, 8, true, List.of() + ); + + assertThrows(IllegalArgumentException.class, () -> sprite.updateFrom(other)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteDtoTest.java` around lines 43 - 89, Add a negative unit test that constructs a FighterSpriteDto via FighterSpriteDto.fromSummary and then calls sprite.updateFrom(...) with a CombatantSummary whose CombatantId differs from the sprite's original id, asserting that updateFrom throws an exception (e.g., IllegalArgumentException) and does not mutate sprite state; reference the methods/classes CombatantId.generate(), FighterSpriteDto.fromSummary(...), and FighterSpriteDto.updateFrom(...) so the test fails if updateFrom accepts summaries for a different CombatantId.src/main/java/sc2002/turnbased/ui/gui/view/BattleSetupPanel.java (1)
93-99: RedundantsetEnabledcalls after tree traversal.Lines 96-97 explicitly set
startButtonandsecondWaveCheckenabled state, butsetComponentTreeEnabledon line 95 already traversed and set all children including these. If the intent is to ensure specific state, the current code works but is confusing. If these should have different behavior than the tree traversal, consider documenting why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/BattleSetupPanel.java` around lines 93 - 99, The method setSetupControlsEnabled currently calls setComponentTreeEnabled(this, enabled) which already sets enabled state for all child components, making the explicit startButton.setEnabled(enabled) and secondWaveCheck.setEnabled(enabled) redundant and confusing; remove those two explicit calls (or, if they require special behavior different from the tree traversal, document that intention and implement the differing logic explicitly), and keep battleRunning toggle and updateSecondWaveEnabled() as-is to preserve behavior in setSetupControlsEnabled.src/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.java (2)
12-18: Add a class-level Javadoc noting EDT-only usage.This controller relies on
javax.swing.Timerwhich fires callbacks on the EDT. Document that all public methods (enqueue,playNextIfIdle,reset) must be called from the EDT to avoid race conditions on theactiveflag andeventsqueue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.java` around lines 12 - 18, Add a class-level Javadoc to BattlePlaybackController that states the class is EDT-only: mention it uses javax.swing.Timer which fires on the Event Dispatch Thread and therefore all public methods (enqueue, playNextIfIdle, reset) must be invoked from the EDT to avoid race conditions on the events queue and the active flag; include a short example note suggesting SwingUtilities.isEventDispatchThread() checks or SwingUtilities.invokeLater/invokeAndWait when calling these methods.
35-56: Consider exception safety in the playback loop.If
eventPlayer.accept(event)on line 47 throws an exception, the timer is still created and started, but the exception propagates and the current event's playback state may be inconsistent. Consider wrapping the event dispatch in a try-catch to ensureactiveis reset on failure, preventing the queue from getting stuck.🛡️ Proposed defensive fix
public void playNextIfIdle() { if (active) { return; } BattleEvent event = events.poll(); if (event == null) { drainedCallback.run(); return; } active = true; - eventPlayer.accept(event); + try { + eventPlayer.accept(event); + } catch (RuntimeException ex) { + active = false; + throw ex; + } playbackTimer = new Timer(dialogueFormatter.playbackDelayMillis(event), e -> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.java` around lines 35 - 56, playNextIfIdle can leave the controller in an inconsistent state if eventPlayer.accept(event) throws; wrap the dispatch in a try/catch (catch Throwable) so you always reset active to false and stop/clear playbackTimer if it was created/started, and then either call playNextIfIdle() to continue processing or rethrow the exception after cleanup; update references in this method (playNextIfIdle, eventPlayer.accept, active, playbackTimer, dialogueFormatter.playbackDelayMillis, drainedCallback, events.poll) to ensure timer is not left running and active is never stuck true on exception.src/main/java/sc2002/turnbased/ui/gui/view/BattleCommandPanel.java (2)
310-331: Timer may leak if component is removed during animation.The
messageTimeris created inanimateMessage()but only stopped by explicit calls tostopMessageAnimation(). If the panel is removed from the component hierarchy while an animation is running, the timer continues firing and holds a reference to this panel, preventing garbage collection.🛡️ Proposed fix: stop timer on component removal
Add a cleanup listener in the constructor:
addAncestorListener(new javax.swing.event.AncestorListener() { `@Override` public void ancestorRemoved(javax.swing.event.AncestorEvent event) { stopMessageAnimation(); } `@Override` public void ancestorAdded(javax.swing.event.AncestorEvent event) {} `@Override` public void ancestorMoved(javax.swing.event.AncestorEvent event) {} });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/BattleCommandPanel.java` around lines 310 - 331, The Timer started in animateMessage(...) can keep firing after this panel is removed, leaking the panel; ensure the timer is stopped when the component is removed by adding a cleanup hook in the panel's constructor that calls stopMessageAnimation() on removal (e.g., attach an AncestorListener and call stopMessageAnimation() from ancestorRemoved), leaving animateMessage(...) and stopMessageAnimation() unchanged except they will now be reliably stopped when the component is removed.
339-344:escapeHtml()doesn't escape quotes or newlines.While
&,<, and>are escaped, double quotes (") and newlines are not. If messages contain quotes, they could break attribute contexts. Consider also converting newlines to<br>for proper HTML rendering.♻️ Proposed enhancement
private static String escapeHtml(String text) { return text .replace("&", "&") .replace("<", "<") - .replace(">", ">"); + .replace(">", ">") + .replace("\"", """) + .replace("\n", "<br>"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/BattleCommandPanel.java` around lines 339 - 344, The escapeHtml method in BattleCommandPanel currently only replaces &, < and >; update escapeHtml(String text) to also replace the double-quote character (") with " and normalize/convert newlines to HTML line breaks (e.g., replace "\r\n" and "\n" with "<br/>") so messages can't break attribute contexts and render newlines correctly; keep the existing replacements and ensure the replacements are applied in a safe order (escape ampersand first) within escapeHtml.src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java (2)
508-508: CallinglayoutEnemies()insidepaintComponent()may cause visual inconsistencies.
layoutEnemies()modifies sprite positions (sprite.x,sprite.y) during the paint cycle. If multiple repaints occur rapidly, this could cause sprites to appear to "jump" or render inconsistently. Paint methods should ideally be side-effect free.Consider moving the layout call to state-changing methods only (e.g.,
applyBattleEvent(),showPlayerTurn()) where it's already called, and removing it frompaintComponent().♻️ Proposed fix
`@Override` protected void paintComponent(Graphics graphics) { super.paintComponent(graphics); Graphics2D g = (Graphics2D) graphics.create(); g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON); g.setRenderingHint(RenderingHints.KEY_TEXT_ANTIALIASING, RenderingHints.VALUE_TEXT_ANTIALIAS_ON); - layoutEnemies(); drawBackground(g);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java` at line 508, The call to layoutEnemies() inside paintComponent() causes side effects (mutating sprite.x/y) during painting; remove that call from paintComponent() and ensure layoutEnemies() is invoked only from state-changing methods that already trigger repaints (e.g., applyBattleEvent(), showPlayerTurn(), any other methods that change enemy/state before calling repaint()). Update callers so they call layoutEnemies() immediately after state changes and before calling repaint(), and verify paintComponent() remains side-effect free and only reads sprite positions for rendering.
636-640: Enemy type detection by name is fragile.Determining sprite type via
sprite.name.toLowerCase().contains("wolf")is brittle - a player named "Wolfgang" or an enemy named "Wolfsbane Goblin" would incorrectly render as a wolf. Consider using a dedicated type field inFighterSpriteDtoinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java` around lines 636 - 640, Replace fragile name-based detection with an explicit type field on the sprite DTO: add a type (preferably an enum like FighterType {WOLF, GOBLIN, PLAYER, ...} or a String) to FighterSpriteDto and populate it where FighterSpriteDto instances are constructed, then update ArenaScenePanel.draw logic to switch on sprite.getType() instead of sprite.name.contains(...), calling drawWolfSprite(copy, sprite) for WOLF and drawGoblinSprite(copy, sprite) for GOBLIN; ensure null/default handling for unknown types so existing names don’t misclassify.
🤖 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/ui/gui/command/PlayerCommandResolver.java`:
- Around line 79-89: In findLivingEnemy(PlayerTurnRequest turn, CombatantId
selectedTarget) remove the redundant enemy.isAlive() condition because
turn.livingEnemies() already yields only living combatants; simply match
enemy.combatantId().equals(selectedTarget) and return that enemy (keeping the
null checks and fallback return null intact). Update the method body in
PlayerCommandResolver accordingly so it iterates turn.livingEnemies(), compares
IDs, and returns the match without calling Combatant.isAlive().
In `@src/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.java`:
- Around line 51-56: The method takeQueuedPlayerTurn in BattleSessionModel
performs a non-atomic read-modify-write on the fields queuedPlayerTurn and
activePlayerTurn; make the operation atomic by synchronizing it (e.g., declare
takeQueuedPlayerTurn synchronized or wrap its body in a synchronized(this)
block) or by using an AtomicReference for queuedPlayerTurn and performing a
single atomic get-and-set while also updating activePlayerTurn inside the same
synchronized/atomic operation so no other thread can observe or mutate the
fields mid-update.
- Around line 5-9: The BattleSessionModel has race conditions: make its shared
state thread-safe by adding synchronization; specifically mark the mutable
fields (battleRunning, queuedPlayerTurn, activePlayerTurn, queuedPostGameConfig)
as volatile and/or make all public mutating and reading methods (beginBattle(),
endBattle(), queuePlayerTurn(), takeQueuedPlayerTurn(), onPlaybackDrained(), and
any methods called by BattleController.awaitPlayerDecision()) synchronized so
the check-then-set in beginBattle() becomes atomic and queuedPlayerTurn
visibility is guaranteed across the battle-engine thread and the Swing EDT;
ensure paired read/write accesses use the same synchronization strategy to
eliminate TOCTOU and visibility issues.
In `@src/main/java/sc2002/turnbased/ui/gui/model/PlayerTurnRequest.java`:
- Around line 10-15: Add a canonical constructor to the PlayerTurnRequest record
that defensively validates and snapshots inputs: use Objects.requireNonNull(...)
for player, livingEnemies, and responseQueue, and assign this.livingEnemies =
List.copyOf(livingEnemies) to take an immutable snapshot (so later mutations of
the original list don't affect the request); leave player and responseQueue
as-is after null checks (or wrap/validate them similarly if needed).
In `@src/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.java`:
- Around line 76-77: The describeCustomConfiguration() method risks
IndexOutOfBoundsException by directly calling config.selectedItems().get(0) and
get(1); update it to defensively handle variable-length selectedItems() (e.g.,
check size() before accessing indices) and produce a safe description when there
are 0, 1, or more items (for example, use a loop or join all item display names
instead of assuming two). Locate describeCustomConfiguration() and replace the
direct get(0)/get(1) usage with bounds-checked logic that falls back to an
appropriate message when items are missing and concatenates all available item
display names when present.
In `@src/main/java/sc2002/turnbased/ui/gui/TurnBasedArenaGui.java`:
- Around line 189-192: The exitGame() method currently calls System.exit(0)
which can bypass cleanup; update exitGame() in TurnBasedArenaGui to first invoke
controller.shutdown() (or check for null and handle exceptions) and wait/ensure
it completes before calling System.exit(0), so calls from handlePostGameChoice()
or elsewhere run the same shutdown path as the WindowAdapter; ensure you
reference the controller instance used by the WindowAdapter and perform graceful
shutdown (e.g., call controller.shutdown() then System.exit(0)).
In `@src/main/java/sc2002/turnbased/ui/gui/util/SwingThread.java`:
- Around line 21-27: The catch block handling InvocationTargetException in
SwingThread currently unwraps RuntimeException but wraps Error causes in an
IllegalStateException; change it to rethrow Error instances directly as well. In
the InvocationTargetException catch (the code that gets the Throwable cause and
currently has "if (cause instanceof RuntimeException runtimeException) { throw
runtimeException; } throw new IllegalStateException(cause);"), add a branch that
checks "if (cause instanceof Error) throw (Error) cause;" before wrapping, so
Errors are propagated unchanged while still unwrapping RuntimeExceptions and
wrapping other Throwables.
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java`:
- Around line 87-88: The Timer created in the ArenaScenePanel constructor is
started but not stored or stopped, which prevents the panel from being GC'd;
make the Timer a private field (e.g. private Timer tickTimer), assign the new
Timer(16, this::onTick) to that field in the constructor, and stop it when the
panel is removed or no longer displayable by overriding removeNotify() (or
adding a HierarchyListener/AncestorListener) to call tickTimer.stop() (and null
it) so the background ticking ceases and references are released; ensure onTick
remains unchanged.
In `@src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteDto.java`:
- Around line 85-105: The updateFrom(Combatant) and updateFrom(CombatantSummary)
methods copy mutable state but do not verify or update the sprite identity,
risking cross-combatant corruption; modify both methods to either validate the
source identity (compare this.id with combatant.getId()/summary.getId() and
throw/ignore if mismatched) or explicitly update this.id from the source before
copying fields so the DTO and source remain consistent; locate the two
updateFrom methods in FighterSpriteDto and implement one of these
identity-guarding behaviors consistently for both overloads.
In `@src/test/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequestTest.java`:
- Around line 61-62: The intro string currently always uses "enemies total";
update the pluralization in the code that builds wave summaries in
BattleLaunchRequest (the method that produces intro(), around the logic that
appends "enemies total") so that it uses "enemy total" when the count equals 1
and "enemies total" otherwise, and then change the test assertion in
BattleLaunchRequestTest to expect "Wave 2: 1 Goblin - 1 enemy total" instead of
"1 enemies total".
---
Nitpick comments:
In `@src/main/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommand.java`:
- Around line 5-10: ResolvedPlayerCommand currently allows null values for its
components (decision, actionName, targetLabel); add a compact record constructor
in ResolvedPlayerCommand that validates each component (e.g., via
Objects.requireNonNull) and throws a clear NPE when any is null so callers fail
fast; reference the record name ResolvedPlayerCommand and component names
decision, actionName, targetLabel when making the change.
In
`@src/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.java`:
- Around line 12-18: Add a class-level Javadoc to BattlePlaybackController that
states the class is EDT-only: mention it uses javax.swing.Timer which fires on
the Event Dispatch Thread and therefore all public methods (enqueue,
playNextIfIdle, reset) must be invoked from the EDT to avoid race conditions on
the events queue and the active flag; include a short example note suggesting
SwingUtilities.isEventDispatchThread() checks or
SwingUtilities.invokeLater/invokeAndWait when calling these methods.
- Around line 35-56: playNextIfIdle can leave the controller in an inconsistent
state if eventPlayer.accept(event) throws; wrap the dispatch in a try/catch
(catch Throwable) so you always reset active to false and stop/clear
playbackTimer if it was created/started, and then either call playNextIfIdle()
to continue processing or rethrow the exception after cleanup; update references
in this method (playNextIfIdle, eventPlayer.accept, active, playbackTimer,
dialogueFormatter.playbackDelayMillis, drainedCallback, events.poll) to ensure
timer is not left running and active is never stuck true on exception.
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java`:
- Line 508: The call to layoutEnemies() inside paintComponent() causes side
effects (mutating sprite.x/y) during painting; remove that call from
paintComponent() and ensure layoutEnemies() is invoked only from state-changing
methods that already trigger repaints (e.g., applyBattleEvent(),
showPlayerTurn(), any other methods that change enemy/state before calling
repaint()). Update callers so they call layoutEnemies() immediately after state
changes and before calling repaint(), and verify paintComponent() remains
side-effect free and only reads sprite positions for rendering.
- Around line 636-640: Replace fragile name-based detection with an explicit
type field on the sprite DTO: add a type (preferably an enum like FighterType
{WOLF, GOBLIN, PLAYER, ...} or a String) to FighterSpriteDto and populate it
where FighterSpriteDto instances are constructed, then update
ArenaScenePanel.draw logic to switch on sprite.getType() instead of
sprite.name.contains(...), calling drawWolfSprite(copy, sprite) for WOLF and
drawGoblinSprite(copy, sprite) for GOBLIN; ensure null/default handling for
unknown types so existing names don’t misclassify.
In `@src/main/java/sc2002/turnbased/ui/gui/view/BattleCommandPanel.java`:
- Around line 310-331: The Timer started in animateMessage(...) can keep firing
after this panel is removed, leaking the panel; ensure the timer is stopped when
the component is removed by adding a cleanup hook in the panel's constructor
that calls stopMessageAnimation() on removal (e.g., attach an AncestorListener
and call stopMessageAnimation() from ancestorRemoved), leaving
animateMessage(...) and stopMessageAnimation() unchanged except they will now be
reliably stopped when the component is removed.
- Around line 339-344: The escapeHtml method in BattleCommandPanel currently
only replaces &, < and >; update escapeHtml(String text) to also replace the
double-quote character (") with " and normalize/convert newlines to HTML
line breaks (e.g., replace "\r\n" and "\n" with "<br/>") so messages can't break
attribute contexts and render newlines correctly; keep the existing replacements
and ensure the replacements are applied in a safe order (escape ampersand first)
within escapeHtml.
In `@src/main/java/sc2002/turnbased/ui/gui/view/BattleSetupPanel.java`:
- Around line 93-99: The method setSetupControlsEnabled currently calls
setComponentTreeEnabled(this, enabled) which already sets enabled state for all
child components, making the explicit startButton.setEnabled(enabled) and
secondWaveCheck.setEnabled(enabled) redundant and confusing; remove those two
explicit calls (or, if they require special behavior different from the tree
traversal, document that intention and implement the differing logic
explicitly), and keep battleRunning toggle and updateSecondWaveEnabled() as-is
to preserve behavior in setSetupControlsEnabled.
In `@src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteDtoTest.java`:
- Around line 43-89: Add a negative unit test that constructs a FighterSpriteDto
via FighterSpriteDto.fromSummary and then calls sprite.updateFrom(...) with a
CombatantSummary whose CombatantId differs from the sprite's original id,
asserting that updateFrom throws an exception (e.g., IllegalArgumentException)
and does not mutate sprite state; reference the methods/classes
CombatantId.generate(), FighterSpriteDto.fromSummary(...), and
FighterSpriteDto.updateFrom(...) so the test fails if updateFrom accepts
summaries for a different CombatantId.
🪄 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: 83a12c39-ec62-4ceb-b686-405a888f8cd6
📒 Files selected for processing (24)
README.mdsrc/main/java/sc2002/turnbased/ui/gui/GuiPlayerDecisionProvider.javasrc/main/java/sc2002/turnbased/ui/gui/TurnBasedArenaGui.javasrc/main/java/sc2002/turnbased/ui/gui/command/PlayerCommandResolver.javasrc/main/java/sc2002/turnbased/ui/gui/controller/BattleController.javasrc/main/java/sc2002/turnbased/ui/gui/controller/GuiPlayerDecisionProvider.javasrc/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.javasrc/main/java/sc2002/turnbased/ui/gui/model/PlayerTurnRequest.javasrc/main/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommand.javasrc/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.javasrc/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.javasrc/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.javasrc/main/java/sc2002/turnbased/ui/gui/util/SwingThread.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/BattleCommandPanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/BattleSetupPanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/BattleView.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteDto.javasrc/main/java/sc2002/turnbased/ui/gui/view/PostGameChoice.javasrc/test/java/sc2002/turnbased/ui/gui/command/PlayerCommandResolverTest.javasrc/test/java/sc2002/turnbased/ui/gui/model/BattleSessionModelTest.javasrc/test/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatterTest.javasrc/test/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequestTest.javasrc/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteDtoTest.java
💤 Files with no reviewable changes (1)
- src/main/java/sc2002/turnbased/ui/gui/GuiPlayerDecisionProvider.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.java (1)
9-9: Use a typed post-game configuration instead of rawObject.
queuedPostGameConfigcurrently accepts any runtime type, which increases cast-risk at consumers. A typed model (or generic parameter) will make this contract safer.♻️ Possible direction
-public final class BattleSessionModel { +public final class BattleSessionModel<TPostGameConfig> { ... - private Object queuedPostGameConfig; + private TPostGameConfig queuedPostGameConfig; ... - public synchronized void queuePostGame(Object configuration) { + public synchronized void queuePostGame(TPostGameConfig configuration) { queuedPostGameConfig = configuration; } ... - public synchronized Optional<Object> takeQueuedPostGameConfig() { - Object configuration = queuedPostGameConfig; + public synchronized Optional<TPostGameConfig> takeQueuedPostGameConfig() { + TPostGameConfig configuration = queuedPostGameConfig; queuedPostGameConfig = null; return Optional.ofNullable(configuration); }Also applies to: 58-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.java` at line 9, Replace the raw Object typed field queuedPostGameConfig in BattleSessionModel with a concrete, typed post-game configuration (e.g., a PostGameConfig class) or make BattleSessionModel generic (e.g., BattleSessionModel<TPostGameConfig>) so the field is strongly typed; update the field declaration, constructor, getter/setter methods that reference queuedPostGameConfig and any usages around the referenced block (the consumer code at the other affected lines) to use the new concrete type or generic parameter and adjust casts/assignments accordingly so consumers no longer rely on unchecked casts.src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java (1)
140-145: Silent failure whenoffer()returns false.If
responseQueue.offer()fails (returns false), the resolved command is silently discarded. While this shouldn't happen under normal operation since the queue is drained bytake(), a defensive log statement would help diagnose unexpected states.♻️ Suggested logging for defensive debugging
ResolvedPlayerCommand playerCommand = resolved.get(); - if (activeTurn.get().responseQueue().offer(playerCommand.decision())) { + boolean offered = activeTurn.get().responseQueue().offer(playerCommand.decision()); + if (offered) { model.clearActivePlayerTurn(); view.showCommandResolving(playerCommand.actionName()); view.appendLog(">> " + playerCommand.actionName() + playerCommand.targetLabel()); + } else { + // This indicates a bug: multiple decisions attempted for the same turn + view.appendLog("Warning: command ignored - decision already submitted"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java` around lines 140 - 145, The block in BattleController that calls activeTurn.get().responseQueue().offer(playerCommand.decision()) drops the command silently when offer() returns false; add a defensive branch to handle the false case by logging a warning (e.g., via view.appendLog or the controller logger) that includes the ResolvedPlayerCommand.actionName() and targetLabel(), and avoid clearing state unless the offer succeeded (so move model.clearActivePlayerTurn(), view.showCommandResolving(), and view.appendLog(...) into the true branch only); this will ensure unexpected queue-rejections are visible for debugging.
🤖 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/ui/gui/view/FighterSpriteHudRenderer.java`:
- Line 13: The HP ratio calculation in FighterSpriteHudRenderer uses double
ratio = sprite.maxHp == 0 ? 0 : Math.max(0, sprite.hp / (double) sprite.maxHp)
which prevents negative values but not values >1, allowing overheal to overdraw
the bar; change the computation to also cap the upper bound (e.g., wrap the
computed value with Math.min(1.0, ...)) so ratio is always between 0 and 1 while
preserving the maxHp==0 guard and the use of sprite.hp/sprite.maxHp.
In `@src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteRenderer.java`:
- Around line 66-69: The bodyRendererFor method can return null if the injected
bodyRenderers map lacks FighterType.UNKNOWN; either validate in the class
constructor (the package-private constructor that accepts bodyRenderers) that
bodyRenderers.containsKey(FighterType.UNKNOWN) and throw an
IllegalArgumentException if missing, or change bodyRendererFor to ensure a safe
non-null fallback by resolving sprite.type, then falling back to
bodyRenderers.get(FighterType.UNKNOWN) and if that is still null throw a clear
IllegalStateException with context (mentioning FighterSpriteDto and
FighterType.UNKNOWN) so callers don't get a NullPointerException when calling
renderBody on the result.
---
Nitpick comments:
In `@src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java`:
- Around line 140-145: The block in BattleController that calls
activeTurn.get().responseQueue().offer(playerCommand.decision()) drops the
command silently when offer() returns false; add a defensive branch to handle
the false case by logging a warning (e.g., via view.appendLog or the controller
logger) that includes the ResolvedPlayerCommand.actionName() and targetLabel(),
and avoid clearing state unless the offer succeeded (so move
model.clearActivePlayerTurn(), view.showCommandResolving(), and
view.appendLog(...) into the true branch only); this will ensure unexpected
queue-rejections are visible for debugging.
In `@src/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.java`:
- Line 9: Replace the raw Object typed field queuedPostGameConfig in
BattleSessionModel with a concrete, typed post-game configuration (e.g., a
PostGameConfig class) or make BattleSessionModel generic (e.g.,
BattleSessionModel<TPostGameConfig>) so the field is strongly typed; update the
field declaration, constructor, getter/setter methods that reference
queuedPostGameConfig and any usages around the referenced block (the consumer
code at the other affected lines) to use the new concrete type or generic
parameter and adjust casts/assignments accordingly so consumers no longer rely
on unchecked casts.
🪄 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: 2f483eec-8674-4d32-9378-fb182415fcbf
📒 Files selected for processing (30)
src/main/java/sc2002/turnbased/ui/gui/TurnBasedArenaGui.javasrc/main/java/sc2002/turnbased/ui/gui/command/PlayerCommandResolver.javasrc/main/java/sc2002/turnbased/ui/gui/controller/BattleController.javasrc/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.javasrc/main/java/sc2002/turnbased/ui/gui/model/PlayerTurnRequest.javasrc/main/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommand.javasrc/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.javasrc/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.javasrc/main/java/sc2002/turnbased/ui/gui/util/SwingThread.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/BattleCommandPanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/BattleSetupPanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterBodyRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteDto.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteEffectRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterType.javasrc/main/java/sc2002/turnbased/ui/gui/view/GoblinBodyRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/UnknownFighterBodyRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/WarriorBodyRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/WizardBodyRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/WolfBodyRenderer.javasrc/test/java/sc2002/turnbased/ui/gui/model/PlayerTurnRequestTest.javasrc/test/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommandTest.javasrc/test/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackControllerTest.javasrc/test/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequestTest.javasrc/test/java/sc2002/turnbased/ui/gui/view/BattleCommandPanelTest.javasrc/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteDtoTest.javasrc/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteRendererTest.java
✅ Files skipped from review due to trivial changes (5)
- src/main/java/sc2002/turnbased/ui/gui/view/UnknownFighterBodyRenderer.java
- src/main/java/sc2002/turnbased/ui/gui/view/WizardBodyRenderer.java
- src/main/java/sc2002/turnbased/ui/gui/view/WarriorBodyRenderer.java
- src/test/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequestTest.java
- src/main/java/sc2002/turnbased/ui/gui/command/PlayerCommandResolver.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/sc2002/turnbased/ui/gui/model/PlayerTurnRequest.java
- src/main/java/sc2002/turnbased/ui/gui/model/ResolvedPlayerCommand.java
- src/main/java/sc2002/turnbased/ui/gui/playback/BattlePlaybackController.java
- src/main/java/sc2002/turnbased/ui/gui/util/SwingThread.java
- src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteDtoTest.java
- src/main/java/sc2002/turnbased/ui/gui/view/BattleSetupPanel.java
- src/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.java
- src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteDto.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java (2)
119-130: Graphics state not fully restored afterdrawFloatingTexts.The composite is reset to
AlphaComposite.SrcOverat line 129, which is good. However, the font set at line 120 persists. WhiledrawOverlayimmediately sets its own font (line 133), explicitly restoring graphics state would be more defensive.Additionally, passing
System.nanoTime()directly at line 24 creates a slight time skew from whenmodel.floatingTexts()were created/updated. Consider acceptingnowas a parameter torender()for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java` around lines 119 - 130, Restore the Graphics2D state after drawFloatingTexts by saving the original Font and Composite at the start of ArenaSceneRenderer.drawFloatingTexts and restoring them before returning (so the font set at line 120 does not leak); also change the render flow to accept a consistent timestamp by passing the same now value into render() and through to drawFloatingTexts instead of calling System.nanoTime() in different places to avoid time skew (update the render(...) signature and callers to forward the now parameter to drawFloatingTexts).
88-99: Minor: Color reuse inside the loop may cause visual inconsistency.Lines 95-97 set a different color for some elements but then reset to the original color. However, each iteration reuses the loop variable
x, and the color change at line 95 persists to the next iteration's firstfillRectcalls (lines 92-94). This appears intentional for visual variety but could be clarified with a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java` around lines 88 - 99, In drawRuins(Graphics2D g, int width, int floorTop) the loop temporarily switches color for the middle stripe (using new Color(120, 58, 58, 150)) then resets, which can be confusing because the color change could persist between iterations; update the loop to either explicitly set the desired color before every fillRect call (so colors are deterministic) or add a concise inline comment explaining the intentional alternating color pattern (reference: drawRuins, loop variable x and the Color(...) used for the middle stripe) to make the intent clear.src/test/java/sc2002/turnbased/ui/gui/view/ArenaSceneModelTest.java (1)
99-113: Consider adding edge case tests.The current tests cover happy-path scenarios well. Consider adding tests for:
selectNextEnemywhen all enemies are deadselectEnemyAtwith invalid/out-of-bounds pointsshowSetupPreviewstate reset behaviorThese are optional enhancements for improved coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sc2002/turnbased/ui/gui/view/ArenaSceneModelTest.java` around lines 99 - 113, Add unit tests in ArenaSceneModelTest to cover the suggested edge cases: create a scenario (use setupWithTwoEnemies and spriteById helpers) where all enemy combatants are marked dead and assert selectNextEnemy returns the expected “none” behavior (e.g., empty optional or sentinel) and does not throw; add tests for selectEnemyAt feeding out-of-bounds/invalid Points and assert it returns no selection and is safe; and add tests for showSetupPreview that toggle preview on then off and verify model state (selected sprite, highlighted indices, or preview flag) resets to the initial state. Use the existing helper methods (setupWithTwoEnemies, spriteById, ArenaSceneModel) to locate where to wire these tests.src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java (1)
102-111: Minor redundancy:selectedEnemyId()callschooseDefaultTarget()which may silently change state.The
selectedEnemyId()getter has a side effect of potentially updatingselectedEnemyIdviachooseDefaultTarget(). This could be unexpected behavior for a method that appears to be a simple accessor. Consider documenting this behavior or separating the validation/fallback logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java` around lines 102 - 111, The getter selectedEnemyId() currently performs validation and may mutate state by calling chooseDefaultTarget(), which is surprising for an accessor; change this by separating concerns: make selectedEnemyId() a pure accessor that just returns the field, and move the validation/fallback logic into a new explicit method (e.g. validateOrChooseDefaultTarget() or keep chooseDefaultTarget() but call it from places that perform selection state updates such as the UI controller or when starting a turn). Update callers that relied on side effects to invoke the new validation method explicitly; reference selectedEnemyId (field), selectedEnemyId() (method), chooseDefaultTarget(), sprites, and FighterSpriteDto to locate affected code. Ensure any retained public API is documented to reflect whether it mutates state.
🤖 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/ui/gui/view/ArenaSceneLoop.java`:
- Around line 19-20: Wrap the call to tickCallback.run() inside the Swing Timer
action handler in a try-catch inside ArenaSceneLoop so any Throwable is caught;
on exception, log the error (include the exception) and stop the tickTimer (call
tickTimer.stop() or your existing loop-stop method) to prevent EDT shutdown and
cascading failures. Ensure you reference the tickCallback.run() invocation and
the tickTimer variable when making the change.
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java`:
- Around line 302-303: The code in ArenaSceneModel currently treats sprite.x and
sprite.y == 0 as "unset", which wrongly resets valid coordinates at 0; modify
the logic to use Double.isNaN for unset detection (e.g., replace checks like
sprite.x == 0 and sprite.y == 0 with Double.isNaN(sprite.x) and
Double.isNaN(sprite.y) in ArenaSceneModel where oldX/oldY are computed and the
corresponding later block), and initialize the position fields to Double.NaN in
FighterSpriteDto so NaN is the sentinel for "uninitialized" rather than 0.0.
---
Nitpick comments:
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java`:
- Around line 102-111: The getter selectedEnemyId() currently performs
validation and may mutate state by calling chooseDefaultTarget(), which is
surprising for an accessor; change this by separating concerns: make
selectedEnemyId() a pure accessor that just returns the field, and move the
validation/fallback logic into a new explicit method (e.g.
validateOrChooseDefaultTarget() or keep chooseDefaultTarget() but call it from
places that perform selection state updates such as the UI controller or when
starting a turn). Update callers that relied on side effects to invoke the new
validation method explicitly; reference selectedEnemyId (field),
selectedEnemyId() (method), chooseDefaultTarget(), sprites, and FighterSpriteDto
to locate affected code. Ensure any retained public API is documented to reflect
whether it mutates state.
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java`:
- Around line 119-130: Restore the Graphics2D state after drawFloatingTexts by
saving the original Font and Composite at the start of
ArenaSceneRenderer.drawFloatingTexts and restoring them before returning (so the
font set at line 120 does not leak); also change the render flow to accept a
consistent timestamp by passing the same now value into render() and through to
drawFloatingTexts instead of calling System.nanoTime() in different places to
avoid time skew (update the render(...) signature and callers to forward the now
parameter to drawFloatingTexts).
- Around line 88-99: In drawRuins(Graphics2D g, int width, int floorTop) the
loop temporarily switches color for the middle stripe (using new Color(120, 58,
58, 150)) then resets, which can be confusing because the color change could
persist between iterations; update the loop to either explicitly set the desired
color before every fillRect call (so colors are deterministic) or add a concise
inline comment explaining the intentional alternating color pattern (reference:
drawRuins, loop variable x and the Color(...) used for the middle stripe) to
make the intent clear.
In `@src/test/java/sc2002/turnbased/ui/gui/view/ArenaSceneModelTest.java`:
- Around line 99-113: Add unit tests in ArenaSceneModelTest to cover the
suggested edge cases: create a scenario (use setupWithTwoEnemies and spriteById
helpers) where all enemy combatants are marked dead and assert selectNextEnemy
returns the expected “none” behavior (e.g., empty optional or sentinel) and does
not throw; add tests for selectEnemyAt feeding out-of-bounds/invalid Points and
assert it returns no selection and is safe; and add tests for showSetupPreview
that toggle preview on then off and verify model state (selected sprite,
highlighted indices, or preview flag) resets to the initial state. Use the
existing helper methods (setupWithTwoEnemies, spriteById, ArenaSceneModel) to
locate where to wire these tests.
🪄 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: d86b589e-4596-4441-b679-09868c93a57d
📒 Files selected for processing (5)
src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneLoop.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.javasrc/test/java/sc2002/turnbased/ui/gui/view/ArenaSceneModelTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java (2)
146-150: Include the command type in the warning log for better debuggability.The warning log message includes the action name but not the original command that was rejected. For debugging purposes, including the command would help trace issues.
♻️ Suggested improvement
} else { - view.appendLog("Warning: could not queue command " + view.appendLog("Warning: could not queue command (" + command + ") " + playerCommand.actionName() + playerCommand.targetLabel()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java` around lines 146 - 150, The warning in BattleController currently logs only actionName() and targetLabel(); update the view.appendLog call to include the original command type/representation (e.g., playerCommand.getClass().getSimpleName() or playerCommand.toString()) so the rejected command is clear for debugging; change the message built in the else branch that calls view.appendLog to concatenate a clear command identifier (type or toString) along with actionName() and targetLabel() and include separators for readability.
179-192: Exception handling could lose original stack trace.In
runBattle(), the catch block callsshowBattleError(ex)which only logsex.getMessage()to the view. Consider logging the full exception to a logger or console for debugging, as the message alone may not provide enough context for troubleshooting.♻️ Suggested improvement for debugging
private void runBattle(BattleLaunchRequest request) { try { BattleSetup setup = request.createSetup(setupFactory); SwingThread.runAndWait(() -> view.showBattleLoaded(setup)); BattleEngine engine = new BattleEngine(setup, new SpeedTurnOrderStrategy()); GuiPlayerDecisionProvider decisions = new GuiPlayerDecisionProvider(this); BattleEventListener listener = event -> SwingUtilities.invokeLater(() -> playbackController.enqueue(event)); engine.runUntilBattleEnds(decisions, listener); SwingUtilities.invokeLater(() -> queuePostGame(request.replayConfiguration())); } catch (Exception ex) { + ex.printStackTrace(); // Or use a proper logger SwingUtilities.invokeLater(() -> showBattleError(ex)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java` around lines 179 - 192, The catch block in runBattle currently only forwards ex to showBattleError which uses ex.getMessage(), losing stack trace; update the catch to log the full exception (e.g., using a logger or ex.printStackTrace()) before calling SwingUtilities.invokeLater(() -> showBattleError(ex)), and also consider modifying showBattleError to accept and display or record the full Throwable; reference: runBattle, showBattleError, SwingUtilities.invokeLater, BattleController.src/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.java (1)
48-61: Consider using a switch expression for exhaustive sealed-type handling.The
replay()method uses sequentialinstanceofchecks, but sincePostGameConfigis a sealed interface, a switch expression would be cleaner, more idiomatic, and would allow the compiler to verify exhaustiveness (eliminating the need for the fallbackthrowat line 60).Also note the asymmetric behavior:
Presetreplay uses a generic "Replaying same settings" intro, whileCustomreplay regenerates the full configuration description. If this is intentional, consider adding a brief comment.♻️ Suggested refactor using switch expression
public static BattleLaunchRequest replay(PostGameConfig configuration) { Objects.requireNonNull(configuration, "configuration"); - if (configuration instanceof PostGameConfig.Preset preset) { - return new BattleLaunchRequest( + return switch (configuration) { + case PostGameConfig.Preset preset -> new BattleLaunchRequest( factory -> factory.create(preset.configuration()), configuration, "=== Replaying same settings ===" ); - } - if (configuration instanceof PostGameConfig.Custom custom) { - return custom(custom.configuration()); - } - throw new IllegalArgumentException("Unsupported replay configuration: " + configuration); + case PostGameConfig.Custom custom -> custom(custom.configuration()); + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.java` around lines 48 - 61, Refactor the replay(PostGameConfig) method to use a switch expression on the sealed type PostGameConfig (handling PostGameConfig.Preset and PostGameConfig.Custom) instead of sequential instanceof checks: replace the instanceof branches with a switch that returns the appropriate BattleLaunchRequest for Preset (using factory -> factory.create(preset.configuration()), configuration, "=== Replaying same settings ===") and for Custom call custom(custom.configuration()); remove the trailing IllegalArgumentException because the switch will be exhaustive enforced by the compiler; if the different intro text for Preset vs Custom is intentional, add a brief comment above the Preset arm explaining the asymmetry.src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRendererTest.java (1)
15-18: Consider using delta-based assertions for floating-point comparisonsLines 15–18 use exact equality for
doublevalues. While these specific calculations (integer division and clamping) produce exactly representable results, using tolerance-based assertions is safer and more idiomatic for floating-point comparisons in JUnit.Proposed change
- assertEquals(1.0, FighterSpriteHudRenderer.healthRatio(sprite(130, 100))); - assertEquals(0.0, FighterSpriteHudRenderer.healthRatio(sprite(-10, 100))); - assertEquals(0.0, FighterSpriteHudRenderer.healthRatio(sprite(20, 0))); - assertEquals(0.4, FighterSpriteHudRenderer.healthRatio(sprite(40, 100))); + assertEquals(1.0, FighterSpriteHudRenderer.healthRatio(sprite(130, 100)), 1e-9); + assertEquals(0.0, FighterSpriteHudRenderer.healthRatio(sprite(-10, 100)), 1e-9); + assertEquals(0.0, FighterSpriteHudRenderer.healthRatio(sprite(20, 0)), 1e-9); + assertEquals(0.4, FighterSpriteHudRenderer.healthRatio(sprite(40, 100)), 1e-9);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRendererTest.java` around lines 15 - 18, The tests in FighterSpriteHudRendererTest use exact equality for floating-point results from FighterSpriteHudRenderer.healthRatio; change the assertions to use a delta-based assertion (e.g., assertEquals(expected, actual, delta)) with a small tolerance like 1e-6 to make comparisons robust for floating-point arithmetic while keeping the same expected values; update each assertEquals that checks healthRatio(...) accordingly.src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteRenderer.java (1)
54-54: Use null-safe ID comparison in the selection check.Line 54 can throw if
sprite.idis unexpectedly null;Objects.equalsmakes this path safer.♻️ Proposed fix
- if (sprite.id.equals(selectedEnemyId) && sprite.alive) { + if (sprite.alive && Objects.equals(sprite.id, selectedEnemyId)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteRenderer.java` at line 54, The selection check in FighterSpriteRenderer uses sprite.id.equals(selectedEnemyId) which can NPE if sprite.id is null; update the condition to a null-safe comparison using Objects.equals(sprite.id, selectedEnemyId) and add the necessary import (java.util.Objects) so the selected-enemy check remains safe and behaves identically when either ID is null.
🤖 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/ui/gui/view/FighterSpriteRenderer.java`:
- Around line 41-66: The Graphics2D copy created in FighterSpriteRenderer is not
protected against exceptions from bodyRenderer.renderBody(copy, sprite) or
effectRenderer.renderPulse(copy, sprite), so move all operations that use "copy"
(translate, setComposite/rotate/translate, fill/draw calls,
bodyRenderer.renderBody(copy, sprite), effectRenderer.renderPulse(copy, sprite))
into a try block and call copy.dispose() in a finally block to guarantee
disposal; leave hudRenderer.render(g, sprite, (int)x, (int)y) outside the
try/finally using the original Graphics "g". Ensure you retain the existing
logic and transforms when restructuring.
---
Nitpick comments:
In `@src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.java`:
- Around line 146-150: The warning in BattleController currently logs only
actionName() and targetLabel(); update the view.appendLog call to include the
original command type/representation (e.g.,
playerCommand.getClass().getSimpleName() or playerCommand.toString()) so the
rejected command is clear for debugging; change the message built in the else
branch that calls view.appendLog to concatenate a clear command identifier (type
or toString) along with actionName() and targetLabel() and include separators
for readability.
- Around line 179-192: The catch block in runBattle currently only forwards ex
to showBattleError which uses ex.getMessage(), losing stack trace; update the
catch to log the full exception (e.g., using a logger or ex.printStackTrace())
before calling SwingUtilities.invokeLater(() -> showBattleError(ex)), and also
consider modifying showBattleError to accept and display or record the full
Throwable; reference: runBattle, showBattleError, SwingUtilities.invokeLater,
BattleController.
In `@src/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.java`:
- Around line 48-61: Refactor the replay(PostGameConfig) method to use a switch
expression on the sealed type PostGameConfig (handling PostGameConfig.Preset and
PostGameConfig.Custom) instead of sequential instanceof checks: replace the
instanceof branches with a switch that returns the appropriate
BattleLaunchRequest for Preset (using factory ->
factory.create(preset.configuration()), configuration, "=== Replaying same
settings ===") and for Custom call custom(custom.configuration()); remove the
trailing IllegalArgumentException because the switch will be exhaustive enforced
by the compiler; if the different intro text for Preset vs Custom is
intentional, add a brief comment above the Preset arm explaining the asymmetry.
In `@src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteRenderer.java`:
- Line 54: The selection check in FighterSpriteRenderer uses
sprite.id.equals(selectedEnemyId) which can NPE if sprite.id is null; update the
condition to a null-safe comparison using Objects.equals(sprite.id,
selectedEnemyId) and add the necessary import (java.util.Objects) so the
selected-enemy check remains safe and behaves identically when either ID is
null.
In
`@src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRendererTest.java`:
- Around line 15-18: The tests in FighterSpriteHudRendererTest use exact
equality for floating-point results from FighterSpriteHudRenderer.healthRatio;
change the assertions to use a delta-based assertion (e.g.,
assertEquals(expected, actual, delta)) with a small tolerance like 1e-6 to make
comparisons robust for floating-point arithmetic while keeping the same expected
values; update each assertEquals that checks healthRatio(...) accordingly.
🪄 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: f611b0ea-1b3f-4ba4-bea5-36deef76ac1e
📒 Files selected for processing (10)
src/main/java/sc2002/turnbased/ui/gui/controller/BattleController.javasrc/main/java/sc2002/turnbased/ui/gui/model/BattleSessionModel.javasrc/main/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequest.javasrc/main/java/sc2002/turnbased/ui/gui/setup/PostGameConfig.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteRenderer.javasrc/test/java/sc2002/turnbased/ui/gui/model/BattleSessionModelTest.javasrc/test/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequestTest.javasrc/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRendererTest.javasrc/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteRendererTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/sc2002/turnbased/ui/gui/setup/BattleLaunchRequestTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteHudRenderer.java
- src/test/java/sc2002/turnbased/ui/gui/view/FighterSpriteRendererTest.java
- src/test/java/sc2002/turnbased/ui/gui/model/BattleSessionModelTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java (2)
94-98: Remove redundantsetColorcall.Line 96 sets
stoneColorimmediately after line 94 already set it, with no intervening draw operation.Proposed fix
for (int x = 44; x < width; x += 245) { g.setColor(stoneColor); g.fillRect(x, base - 94, 26, 94); - g.setColor(stoneColor); g.fillRect(x + 76, base - 118, 28, 118); g.setColor(stoneColor); g.fillRect(x - 10, base - 120, 128, 18);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java` around lines 94 - 98, In ArenaSceneRenderer (the rendering method that draws stones) remove the redundant g.setColor(stoneColor) call that immediately follows the previous g.setColor(stoneColor) with no intervening drawing; simply delete the second duplicate setColor invocation so the two fillRect calls use the single prior setColor call.
166-176: Consider binary search or early exit for text fitting.The
fitTextmethod performs O(n)stringWidthcalls in the worst case. For typical UI text lengths this is fine, but if banner messages can be very long, consider:
- Binary search on the end index
- Caching font metrics
This is a minor optimization opportunity—current implementation is correct.
src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java (3)
162-167: Fragile victory/defeat detection via string prefix.Checking
banner.startsWith("Victory")orbanner.startsWith("Defeat")couples battle-end logic to specific narration text. If the text changes (e.g., "You are victorious!", localization, or formatting), the battle won't properly deactivate.Consider having
NarrationEventcarry a semantic flag (e.g.,isVictory(),isDefeat(), or aNarrationTypeenum) rather than parsing the display text.Sketch of a cleaner approach
// In NarrationEvent, add: public enum Type { NORMAL, VICTORY, DEFEAT } private final Type type; // Then in applyBattleEvent: } else if (event instanceof NarrationEvent narrationEvent) { banner = narrationEvent.getText(); if (narrationEvent.getType() == NarrationEvent.Type.VICTORY || narrationEvent.getType() == NarrationEvent.Type.DEFEAT) { acceptingPlayerTurn = false; battleActive = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java` around lines 162 - 167, Replace fragile string-prefix checks in the ArenaSceneModel by adding a semantic type to NarrationEvent and using it to decide battle end: add a NarrationEvent.Type enum (e.g., NORMAL, VICTORY, DEFEAT) and a getter (getType()) on NarrationEvent, then change the logic in the block that currently tests banner.startsWith(...) (the branch handling "NarrationEvent narrationEvent" where banner, acceptingPlayerTurn and battleActive are set) to check narrationEvent.getType() == Type.VICTORY || narrationEvent.getType() == Type.DEFEAT; update any NarrationEvent constructors/call sites to supply the correct Type instead of relying on text content.
324-326:enemyOrder.contains()is O(n) on ArrayList.For typical enemy counts (≤10), this is fine. If enemy counts grow significantly, consider maintaining a parallel
Set<CombatantId>for O(1) membership checks.Also applies to: 336-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java` around lines 324 - 326, The code uses enemyOrder.contains(sprite.id) which is O(n) on an ArrayList; change to maintain a parallel Set<CombatantId> (e.g., enemyOrderSet) inside ArenaSceneModel and use enemyOrderSet.contains(sprite.id) for membership checks, and when adding/removing IDs ensure you update both enemyOrder (preserve list order) and enemyOrderSet (for O(1) checks); apply the same change to the other occurrence(s) that currently call enemyOrder.contains(...) so all membership checks use the Set and all list mutations keep the Set in sync.
200-204: Minor:spritesByDrawOrder()allocates a new sorted list per call.This is invoked every render frame. For the typical small sprite count (~5-10), this is acceptable. If performance becomes a concern, consider caching the sorted list and invalidating on sprite add/remove/position change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java` around lines 200 - 204, The spritesByDrawOrder() method currently allocates and sorts a new list each call; change it to cache the sorted list and only recompute when sprite state changes by adding a cached List<FighterSpriteDto> (e.g., cachedOrderedSprites) and a dirty flag (e.g., orderedSpritesDirty) in ArenaSceneModel, mark orderedSpritesDirty = true whenever sprites map is mutated or any FighterSpriteDto position/ drawY changes (update points where sprites are added/removed or their position setters are called), and have spritesByDrawOrder() return the cached list after sorting/rebuilding it only when orderedSpritesDirty is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.java`:
- Around line 162-167: Replace fragile string-prefix checks in the
ArenaSceneModel by adding a semantic type to NarrationEvent and using it to
decide battle end: add a NarrationEvent.Type enum (e.g., NORMAL, VICTORY,
DEFEAT) and a getter (getType()) on NarrationEvent, then change the logic in the
block that currently tests banner.startsWith(...) (the branch handling
"NarrationEvent narrationEvent" where banner, acceptingPlayerTurn and
battleActive are set) to check narrationEvent.getType() == Type.VICTORY ||
narrationEvent.getType() == Type.DEFEAT; update any NarrationEvent
constructors/call sites to supply the correct Type instead of relying on text
content.
- Around line 324-326: The code uses enemyOrder.contains(sprite.id) which is
O(n) on an ArrayList; change to maintain a parallel Set<CombatantId> (e.g.,
enemyOrderSet) inside ArenaSceneModel and use enemyOrderSet.contains(sprite.id)
for membership checks, and when adding/removing IDs ensure you update both
enemyOrder (preserve list order) and enemyOrderSet (for O(1) checks); apply the
same change to the other occurrence(s) that currently call
enemyOrder.contains(...) so all membership checks use the Set and all list
mutations keep the Set in sync.
- Around line 200-204: The spritesByDrawOrder() method currently allocates and
sorts a new list each call; change it to cache the sorted list and only
recompute when sprite state changes by adding a cached List<FighterSpriteDto>
(e.g., cachedOrderedSprites) and a dirty flag (e.g., orderedSpritesDirty) in
ArenaSceneModel, mark orderedSpritesDirty = true whenever sprites map is mutated
or any FighterSpriteDto position/ drawY changes (update points where sprites are
added/removed or their position setters are called), and have
spritesByDrawOrder() return the cached list after sorting/rebuilding it only
when orderedSpritesDirty is true.
In `@src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.java`:
- Around line 94-98: In ArenaSceneRenderer (the rendering method that draws
stones) remove the redundant g.setColor(stoneColor) call that immediately
follows the previous g.setColor(stoneColor) with no intervening drawing; simply
delete the second duplicate setColor invocation so the two fillRect calls use
the single prior setColor call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd669e14-af66-4f61-99e7-8be693eb286e
📒 Files selected for processing (7)
src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneLoop.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneRenderer.javasrc/main/java/sc2002/turnbased/ui/gui/view/BattleView.javasrc/main/java/sc2002/turnbased/ui/gui/view/FighterSpriteDto.javasrc/test/java/sc2002/turnbased/ui/gui/view/ArenaSceneModelTest.java
✅ Files skipped from review due to trivial changes (2)
- src/test/java/sc2002/turnbased/ui/gui/view/ArenaSceneModelTest.java
- src/main/java/sc2002/turnbased/ui/gui/view/BattleView.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneLoop.java
- src/main/java/sc2002/turnbased/ui/gui/view/ArenaScenePanel.java
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.
If merged
This is a major change! Finally implemented proper GUI! wohoo!
It this merges, tag this with v2.0.0
Release Notes
New Features
Documentation
Refactor
Tests
Summary by CodeRabbit
New Features
Documentation