fix(combat): dash actually extends the enforced movement budget#60
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new CombatEngine.applyDash(participantId) method and updates the consolidated combat-action Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as CombatActionHandler
participant Manager as CombatManager (in-memory)
participant DB as EncounterRepository
participant Engine as CombatEngine
Client->>Server: POST /combat_action dash {sessionId, encounterId, actorId}
Server->>Manager: getEngine(sessionId:encounterId)
alt Engine in memory
Manager-->>Server: engine
Server->>Engine: applyDash(actorId)
Engine-->>Server: {ok:true, movementRemaining}
Server-->>Client: 200 {success:true, movementRemaining,...}
else Engine missing
Server->>DB: loadState(encounterId)
alt state found
DB-->>Server: persistedState
Server->>Engine: new Engine()
Server->>Engine: loadState(persistedState)
Server->>Manager: registerEngine(sessionId:encounterId, Engine)
Server->>Engine: applyDash(actorId)
Engine-->>Server: {ok:true, movementRemaining}
Server-->>Client: 200 {success:true, movementRemaining,...}
else not found
DB-->>Server: null
Server-->>Client: 404 {success:false, error:"encounter not found"}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The consolidated dash handler was a stub: it returned a success message saying "Movement speed doubled" but never touched the engine, so the follow-up move action still enforced the original 30ft budget and rejected with "Insufficient movement." Observed in live session when Rurk tried to dash + move to close the aisle. - CombatEngine.applyDash(): adds a fresh baseSpeed allotment on top of any existing movementRemaining, flags hasDashed, and consumes the main action (matches 5e dash rules). - Consolidated dash handler calls applyDash and reports the new movementRemaining so callers can see their budget. Closes #50 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/server/consolidated/combat-action.test.ts (1)
296-297: Consider adding negative test case.The test verifies dash works, but consider adding a case that verifies a move exceeding 60ft fails after dash. This would confirm the budget is actually enforced (not just that any move succeeds).
Example: After dash (60ft), attempt to move to a position requiring >60ft and assert
success: false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/consolidated/combat-action.test.ts` around lines 296 - 297, Add a negative test that, after performing the dash (which yields dashData.movementRemaining === 60), attempts a subsequent move to a target requiring >60ft using the same movement action helper and assert that that move's result.success is false (and that movementRemaining is not reduced/unchanged), thereby verifying the 60ft budget is enforced; use the existing dash invocation and the same move API used elsewhere in this test file and reference dashData, movementRemaining and the move result object when adding the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/server/consolidated/combat-action.test.ts`:
- Around line 296-297: Add a negative test that, after performing the dash
(which yields dashData.movementRemaining === 60), attempts a subsequent move to
a target requiring >60ft using the same movement action helper and assert that
that move's result.success is false (and that movementRemaining is not
reduced/unchanged), thereby verifying the 60ft budget is enforced; use the
existing dash invocation and the same move API used elsewhere in this test file
and reference dashData, movementRemaining and the move result object when adding
the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 643cc6fc-4b41-4895-9beb-e5aafb2de7b4
📒 Files selected for processing (3)
src/engine/combat/engine.tssrc/server/consolidated/combat-action.tstests/server/consolidated/combat-action.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4596008b73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (participant.hasDashed) return { ok: false, error: 'Already dashed this turn' }; | ||
|
|
||
| const baseSpeed = participant.movementSpeed ?? 30; | ||
| const currentRemaining = participant.movementRemaining ?? baseSpeed; | ||
| participant.movementRemaining = currentRemaining + baseSpeed; |
There was a problem hiding this comment.
Validate action economy before applying Dash
applyDash only checks hasDashed and then always adds movement, so a participant who already spent their Action can still dash and gain extra movement in the same turn. This is reachable because attack/heal/disengage consume actions via commitAction, but this path never calls validateActionEconomy(..., 'action'); as a result, dash can be used as an illegal second main action (and by incapacitated actors) and produces incorrect combat state.
Useful? React with 👍 / 👎.
| const engine = getCombatManager().get(`${currentContext.sessionId}:${params.encounterId}`); | ||
| if (!engine) { | ||
| return { | ||
| error: true, | ||
| actionType: 'dash', |
There was a problem hiding this comment.
Reuse encounter auto-load path for dash
This handler now depends on an in-memory engine and immediately returns Encounter ... not found when getCombatManager().get(...) misses. Unlike the other combat actions, it does not use handleExecuteCombatAction’s DB auto-load behavior, so dash fails after process restart or manager eviction even when the encounter exists and other actions still work. Dash should follow the same load path to avoid this inconsistent failure mode.
Useful? React with 👍 / 👎.
Reviewers (PR #60) flagged two gaps: 1. applyDash only checked hasDashed before consuming the action, so a participant who had already attacked could still dash for a free 60ft of movement. Dash IS the action in 5e — calling validateActionEconomy(participantId, 'action') first rejects the illegal sequence. 2. The dash handler short-circuited with "Encounter not found" when the engine wasn't in memory, unlike other combat actions which auto-load from EncounterRepository. After a process restart or manager eviction, dash failed even though attack/move still worked. Adds matching auto-load path in the consolidated dash handler and regression tests for both behaviors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/consolidated/combat-action.ts`:
- Around line 203-212: When restoring an evicted engine, avoid the race where
two requests both load from DB and call getCombatManager().create(sessionKey,
engine) causing an exception; change the logic around the engine restoration
(the block that calls getDb(...), new EncounterRepository(...).loadState(...),
new CombatEngine(...).loadState(...), and getCombatManager().create(sessionKey,
engine)) to use a get-or-create pattern: after loading the persisted state and
constructing the engine, attempt to create but catch the specific error from
CombatManager.create and, on that error, call getCombatManager().get(sessionKey)
to retrieve the already-created engine (or re-check
getCombatManager().get(sessionKey) before create and only call create if get
returned null), ensuring only one creation succeeds and the second request uses
the existing engine.
🪄 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: c5aa9e42-5ac8-43c1-949a-b2e5bcbab2c8
📒 Files selected for processing (3)
src/engine/combat/engine.tssrc/server/consolidated/combat-action.tstests/server/consolidated/combat-action.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/server/consolidated/combat-action.test.ts
- src/engine/combat/engine.ts
Reviewers (PR #60, Minor) noted that two concurrent dash requests hitting the auto-load path could both see the engine missing, both load from DB, and both call CombatManager.create — the loser would throw "Encounter X already exists". Adopts the get-or-create pattern: re-check the manager after loading from DB, and wrap create in a try/get fallback so the loser of the race adopts the winner's engine instead of crashing. Regression test fires two dash requests in parallel against an evicted engine; one must succeed and neither may throw 'already exists'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai both findings are stale regenerations against the same commit that addresses them — |
|
Tip For best results, initiate chat on the files or code changes.
Apologies for the noise from the stale regeneration. The implementation and test coverage look solid for both behaviors. |
Summary
The consolidated
dashhandler was a no-op stub — it returned "Movement speed doubled" but never told the engine. The nextmovestill enforced the base 30ft budget and rejected withInsufficient movement. Observed live when a dashing zealot couldn't cross the aisle.Fix
CombatEngine.applyDash()— adds a freshbaseSpeedallotment on top of any existingmovementRemaining, flagshasDashed, and consumes the main action (5e dash rules).dashhandler incombat-action.tscallsapplyDashand echoes the newmovementRemainingso callers can see their budget.Test plan
Closes #50
🤖 Generated with Claude Code
Summary by CodeRabbit