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:
📝 WalkthroughWalkthroughAdds CombatEngine.addParticipants to initialize and merge new combatants into an active encounter. Extends spawn_quick_enemy to append enemies to an existing in-memory or DB-loaded encounter, persist state with rollback on failure, and update/return turnOrder and currentTurn (or explicit errors if encounterId unknown). Changes
Sequence DiagramsequenceDiagram
actor Client
participant Server as spawn_quick_enemy Handler
participant Manager as CombatManager
participant Engine as CombatEngine
participant DB as EncounterRepository
Client->>Server: spawn_quick_enemy(creature, count, encounterId?)
activate Server
Server->>Manager: get(sessionId:encounterId)
activate Manager
Manager-->>Server: engine | null
deactivate Manager
alt Engine in memory
Server->>Engine: addParticipants(newEnemies)
activate Engine
Engine->>Engine: init participants (initiative, flags, resources)
Engine->>Engine: merge participants, rebuild turnOrder, reinsert 'LAIR', preserve currentTurnIndex
Engine-->>Server: updated CombatState
deactivate Engine
Server-->>Client: success + appendedToExisting: true, turnOrder, currentTurn
else Engine not in memory
Server->>DB: loadEncounter(encounterId)
activate DB
DB-->>Server: persisted CombatState | not found
deactivate DB
alt persisted state found
Server->>Engine: create + load(state)
Server->>Engine: addParticipants(newEnemies)
activate Engine
Engine-->>Server: updated CombatState
deactivate Engine
Server->>DB: saveState(updated state) (try/catch)
alt save success
Server-->>Client: success + appendedToExisting: true, loadedFromDb: true, turnOrder, currentTurn
else save failure
Server->>Server: rollback in-memory participants/turnOrder
Server-->>Client: error { rolledBack: true }
end
else not found
Server-->>Client: error (requestedEncounterId not found)
end
end
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e037209d37
ℹ️ 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".
Comment on lines
+268
to
+269
| const state = engine.addParticipants(participants as unknown as Parameters<typeof engine.addParticipants>[0]); | ||
| return { |
There was a problem hiding this comment.
Persist appended encounter state before returning
This append path mutates the in-memory engine via addParticipants and immediately returns success, but it never writes the updated state back to the encounter repository. If the process is restarted (or in-memory combat state is cleared) before another action performs a save, get/load will recover stale DB state without the newly spawned enemies, effectively dropping user-visible combat changes.
Useful? React with 👍 / 👎.
| attack: preset.defaultAttack | ||
| })), | ||
| turnOrder: state.turnOrder, | ||
| currentTurn: state.participants[state.currentTurnIndex]?.id, |
There was a problem hiding this comment.
Read current turn from turnOrder when building append result
currentTurn is derived with state.participants[state.currentTurnIndex]?.id, but currentTurnIndex indexes turnOrder, not participants. In encounters that include the special LAIR slot, these arrays are offset, so the response can report the wrong active combatant (or a non-lair actor during a lair turn), which can mislead clients consuming this field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/server/consolidated/combat-manage.ts (1)
268-268: Type castas unknown asbypasses type safety.Consider typing the
participantsarray more precisely to avoid the double cast. The participant objects could be annotated withCombatParticipant[]from the engine types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/consolidated/combat-manage.ts` at line 268, The call to engine.addParticipants uses a double-cast "participants as unknown as Parameters<typeof engine.addParticipants>[0]" which bypasses type safety; instead, type the participants array correctly (e.g., annotate the variable or its source as CombatParticipant[] or the specific parameter type expected by engine.addParticipants) and remove the double-cast so you call engine.addParticipants(participants) with participants declared as the correct type; update the declaration/assignment site of participants (or apply a single cast to the exact parameter type) to ensure the compiler verifies the shape matches engine.addParticipants.src/engine/combat/engine.ts (1)
199-213: Missing legendary action/resistance initialization.When adding legendary creatures mid-combat,
legendaryActionsRemainingandlegendaryResistancesRemainingare not initialized from their base values. Compare withstartEncounterlines 255-256 which set these fields.For consistency, consider initializing these fields:
♻️ Proposed fix
const withInit = newParticipants.map(p => ({ ...p, initiative: this.rng.d20(p.initiativeBonus), isEnemy: p.isEnemy ?? this.detectIsEnemy(p.id, p.name), + legendaryActionsRemaining: p.legendaryActions ?? p.legendaryActionsRemaining, + legendaryResistancesRemaining: p.legendaryResistances ?? p.legendaryResistancesRemaining, movementRemaining: p.movementSpeed ?? 30, actionUsed: false, bonusActionUsed: false, spellsCast: {}, reactionUsed: false, hasDashed: false, hasDisengaged: false }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/combat/engine.ts` around lines 199 - 213, In addParticipants (method addParticipants in engine.ts) initialize legendaryActionsRemaining and legendaryResistancesRemaining for each new participant the same way startEncounter does: set legendaryActionsRemaining to p.legendaryActions ?? 0 and legendaryResistancesRemaining to p.legendaryResistances ?? 0 (or whatever base fields are used in startEncounter) when building the withInit participant object so legendary creatures added mid-combat start with their proper counters.tests/server/consolidated/combat-manage.test.ts (1)
149-159: Consider adding test forencounterIdnot found scenario.The current tests verify both append (encounterId exists) and create (encounterId omitted) paths. However, there's no test for when
encounterIdis provided but the encounter doesn't exist in memory. This edge case currently silently creates a new encounter, which may not be the intended behavior.📝 Suggested test case
it('spawn_quick_enemy with non-existent encounterId should [error/fallback]', async () => { const result = await handleCombatManage({ action: 'spawn_quick_enemy', encounterId: 'non-existent-encounter-id', creature: 'goblin', count: 1 }, ctx); const data = parseResult(result); // Document expected behavior - currently falls through to create new encounter // Consider whether this should error instead });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/consolidated/combat-manage.test.ts` around lines 149 - 159, Add a test covering the case where handleCombatManage is called with action 'spawn_quick_enemy' and an encounterId that does not exist: call handleCombatManage({ action: 'spawn_quick_enemy', encounterId: 'non-existent-encounter-id', creature: 'goblin', count: 1 }, ctx), parse the result with parseResult, and assert the expected behavior (either that it returns an error flag/message or that it creates a new encounter and sets encounterId); update the test to reflect the chosen contract and adjust handleCombatManage's spawn_quick_enemy branch if you decide to change behavior from creating a new encounter to returning an error.
🤖 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-manage.ts`:
- Line 286: currentTurn is being derived from
state.participants[state.currentTurnIndex]?.id but state.currentTurnIndex
indexes into state.turnOrder, not state.participants; change the assignment so
you pull the correct participant via state.turnOrder[state.currentTurnIndex] (if
turnOrder holds participant IDs use state.turnOrder[state.currentTurnIndex],
otherwise if turnOrder holds participant indices use
state.participants[state.turnOrder[state.currentTurnIndex]]?.id) and update the
currentTurn expression accordingly to reference state.turnOrder and
state.participants as appropriate.
- Around line 265-293: When params.encounterId is present but
getCombatManager().get(`${currentContext.sessionId}:${params.encounterId}`)
returns undefined, don’t silently fall through to new-encounter behavior: first
attempt to auto-load the encounter from persistent storage via
EncounterRepository (import and call the repo’s load/get method),
recreate/register the engine in getCombatManager() for
`${currentContext.sessionId}:${params.encounterId}`, then call
engine.addParticipants(...) and return the append response; if the repo load
returns no encounter, return an explicit error response indicating the requested
encounterId was not found (mirroring the pattern used by
handleGetEncounterState) instead of creating a new encounter.
---
Nitpick comments:
In `@src/engine/combat/engine.ts`:
- Around line 199-213: In addParticipants (method addParticipants in engine.ts)
initialize legendaryActionsRemaining and legendaryResistancesRemaining for each
new participant the same way startEncounter does: set legendaryActionsRemaining
to p.legendaryActions ?? 0 and legendaryResistancesRemaining to
p.legendaryResistances ?? 0 (or whatever base fields are used in startEncounter)
when building the withInit participant object so legendary creatures added
mid-combat start with their proper counters.
In `@src/server/consolidated/combat-manage.ts`:
- Line 268: The call to engine.addParticipants uses a double-cast "participants
as unknown as Parameters<typeof engine.addParticipants>[0]" which bypasses type
safety; instead, type the participants array correctly (e.g., annotate the
variable or its source as CombatParticipant[] or the specific parameter type
expected by engine.addParticipants) and remove the double-cast so you call
engine.addParticipants(participants) with participants declared as the correct
type; update the declaration/assignment site of participants (or apply a single
cast to the exact parameter type) to ensure the compiler verifies the shape
matches engine.addParticipants.
In `@tests/server/consolidated/combat-manage.test.ts`:
- Around line 149-159: Add a test covering the case where handleCombatManage is
called with action 'spawn_quick_enemy' and an encounterId that does not exist:
call handleCombatManage({ action: 'spawn_quick_enemy', encounterId:
'non-existent-encounter-id', creature: 'goblin', count: 1 }, ctx), parse the
result with parseResult, and assert the expected behavior (either that it
returns an error flag/message or that it creates a new encounter and sets
encounterId); update the test to reflect the chosen contract and adjust
handleCombatManage's spawn_quick_enemy branch if you decide to change behavior
from creating a new encounter to returning an error.
🪄 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: 453bd3c2-e8e0-4fea-af41-569be41ee149
📒 Files selected for processing (3)
src/engine/combat/engine.tssrc/server/consolidated/combat-manage.tstests/server/consolidated/combat-manage.test.ts
Mnehmos
added a commit
that referenced
this pull request
Apr 19, 2026
…s currentTurn from turnOrder Reviewers (PR #58) flagged three issues with the append path: 1. currentTurn was derived as state.participants[state.currentTurnIndex].id, but currentTurnIndex indexes turnOrder, not participants. With LAIR in the mix the two arrays can diverge and we'd report the wrong actor. 2. When encounterId was supplied but the engine wasn't in memory (server restart, eviction), we silently fell through to creating a new encounter — clients lost their existing one without warning. 3. The mutation lived only in memory; a restart would lose the spawned enemies entirely. This commit: - Auto-loads the engine from EncounterRepository when not in memory (mirrors handleGetEncounterState / handleExecuteCombatAction). - Persists state via repo.saveState after addParticipants. - Reads currentTurn from state.turnOrder[state.currentTurnIndex]. - Adds loadedFromDb to the response so callers can tell the path apart. - Regression tests for both the currentTurn invariant and the auto-load path. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Mnehmos
added a commit
that referenced
this pull request
Apr 19, 2026
Reviewers (PR #58) flagged that even with the new auto-load path, when the requested encounter was missing from BOTH memory and DB the code silently fell through to creating a fresh encounter. That hid typos and stale ids — callers got back a different encounter id than they asked for and never noticed. Now returns an explicit error pointing back at the requested id so the caller can correct it. The omitted-encounterId path (legitimate fresh-encounter case) is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/server/consolidated/combat-manage.ts (1)
289-291: Typeparticipantsarray explicitly to eliminate the unsafe double-cast.The cast
as unknown as Parameters<typeof engine.addParticipants>[0]bypasses compile-time type checking. Replace the untyped array initialization at line 238 with:const participants: CombatParticipant[] = [];This allows TypeScript to validate each object pushed to the array against the
CombatParticipantshape, catching any drift between the payload construction and the engine's expected input at compile time rather than hiding it with a cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/consolidated/combat-manage.ts` around lines 289 - 291, The participants array is being double-cast to fit engine.addParticipants which bypasses type checking; explicitly type the array as CombatParticipant instead of using an untyped initialization and the unsafe cast. Find where participants is declared/initialized and change it to const participants: CombatParticipant[] = []; then ensure each push/constructed object conforms to CombatParticipant so the call engine.addParticipants(participants) no longer requires a cast.
🤖 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-manage.ts`:
- Around line 289-302: addParticipants mutates engine state but persistence
errors are swallowed; capture the prior engine state (e.g., const prev =
engine.getState() or equivalent) before calling
engine.addParticipants(participants), then attempt to persist with getDb(...)
and new EncounterRepository(db).saveState(params.encounterId, state); if
persistence fails, restore the engine to prev (via engine.setState(prev) or the
engine's undo/removeParticipants API) and propagate/return an explicit error
instead of returning success so callers know the append failed; ensure you
reference engine.addParticipants, params.encounterId, getDb, and
EncounterRepository.saveState in your changes.
In `@tests/server/consolidated/combat-manage.test.ts`:
- Around line 170-172: The assertion comparing spawnData.turnOrder[0] to
spawnData.currentTurn is order-coupled and flaky; change it to verify that the
reported currentTurn is present at the expected index by locating
spawnData.currentTurn inside spawnData.turnOrder (e.g., find its index with
indexOf) and asserting that this index equals spawnData.currentTurnIndex, or at
minimum assert that spawnData.turnOrder.includes(spawnData.currentTurn) and/or
that the found index === spawnData.currentTurnIndex to make the test robust;
update the assertion referencing spawnData.turnOrder, spawnData.currentTurn, and
spawnData.currentTurnIndex accordingly.
---
Nitpick comments:
In `@src/server/consolidated/combat-manage.ts`:
- Around line 289-291: The participants array is being double-cast to fit
engine.addParticipants which bypasses type checking; explicitly type the array
as CombatParticipant instead of using an untyped initialization and the unsafe
cast. Find where participants is declared/initialized and change it to const
participants: CombatParticipant[] = []; then ensure each push/constructed object
conforms to CombatParticipant so the call engine.addParticipants(participants)
no longer requires a cast.
🪄 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: 167d0276-3a81-44da-92ec-bba6a86f58db
📒 Files selected for processing (2)
src/server/consolidated/combat-manage.tstests/server/consolidated/combat-manage.test.ts
Mnehmos
added a commit
that referenced
this pull request
Apr 19, 2026
Reviewers (PR #58 round 3) flagged that swallowing saveState errors left memory and DB out of sync — the caller saw success, but a restart would lose the appended enemies. The encounter would silently revert without any warning. Now snapshots the participant id set before addParticipants, attempts saveState, and on failure rolls back the in-memory mutation (participants + turnOrder) before returning an explicit error with { error: true, rolledBack: true, message }. Also tightened the currentTurn assertion per the same reviewer note — the previous test was order-coupled to turnOrder[0] which can flake on initiative resort. Adds a regression test that stubs EncounterRepository.saveState to throw and asserts: error response, rolledBack flag, and unchanged in-memory participant count. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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-manage.ts`:
- Around line 304-320: The rollback restores participants and turnOrder but
misses restoring currentTurnIndex; capture the pre-append currentTurnIndex (from
engine.getState().currentTurnIndex) before calling addParticipants(), and in the
catch block set live.currentTurnIndex = previousIndex (with a safety clamp to
0..live.turnOrder.length-1) so the active turn is correctly restored when you
revert live.participants and live.turnOrder; update the catch handler that
references engine.getState() and live to also restore currentTurnIndex.
🪄 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: 512330fd-e43e-4aa8-89da-668327b7aff5
📒 Files selected for processing (2)
src/server/consolidated/combat-manage.tstests/server/consolidated/combat-manage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/server/consolidated/combat-manage.test.ts
Comment on lines
+304
to
+320
| } catch (err) { | ||
| // Roll back: drop the just-added participants so memory | ||
| // matches DB. Use the engine's state directly since we | ||
| // know the schema. | ||
| const live = engine.getState(); | ||
| if (live) { | ||
| live.participants = live.participants.filter((p) => beforeIds.has(p.id)); | ||
| live.turnOrder = live.turnOrder.filter((id) => id === 'LAIR' || beforeIds.has(id)); | ||
| } | ||
| return { | ||
| error: true, | ||
| actionType: 'spawn_quick_enemy', | ||
| encounterId: params.encounterId, | ||
| message: `Failed to persist appended encounter state: ${(err as Error).message}. In-memory append rolled back.`, | ||
| rolledBack: true | ||
| }; | ||
| } |
There was a problem hiding this comment.
Incomplete rollback: currentTurnIndex is not restored.
The rollback correctly restores participants and turnOrder, but addParticipants() (see context snippet 1) also modifies currentTurnIndex to anchor the active turn in the re-sorted order. After rollback, the index may point to the wrong participant or be out of bounds.
Example: if turnOrder was [A, B, C] with index 1 (turn=B), after adding a high-initiative enemy X, it becomes [X, A, B, C] with index 2 (still B). After rollback, turnOrder is [A, B, C] but index remains 2, pointing to C instead of B.
🐛 Proposed fix: capture and restore currentTurnIndex
if (engine) {
// Snapshot for rollback before mutating in-memory state.
const beforeIds = new Set(engine.getState()?.participants.map((p) => p.id) ?? []);
+ const beforeTurnIndex = engine.getState()?.currentTurnIndex ?? 0;
const state = engine.addParticipants(
participants as unknown as Parameters<typeof engine.addParticipants>[0]
);
// ... persistence attempt ...
} catch (err) {
// Roll back: drop the just-added participants so memory
// matches DB. Use the engine's state directly since we
// know the schema.
const live = engine.getState();
if (live) {
live.participants = live.participants.filter((p) => beforeIds.has(p.id));
live.turnOrder = live.turnOrder.filter((id) => id === 'LAIR' || beforeIds.has(id));
+ live.currentTurnIndex = beforeTurnIndex;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/consolidated/combat-manage.ts` around lines 304 - 320, The
rollback restores participants and turnOrder but misses restoring
currentTurnIndex; capture the pre-append currentTurnIndex (from
engine.getState().currentTurnIndex) before calling addParticipants(), and in the
catch block set live.currentTurnIndex = previousIndex (with a safety clamp to
0..live.turnOrder.length-1) so the active turn is correctly restored when you
revert live.participants and live.turnOrder; update the catch handler that
references engine.getState() and live to also restore currentTurnIndex.
…ounterId given The schema advertised "Add to existing encounter (creates new if omitted)" but the handler always called handleCreateEncounter, minting a fresh encounter regardless of the supplied encounterId. PCs were left without opponents while the cultists sat in a different encounter-id. - CombatEngine: new addParticipants() method rolls initiative, resorts turn order, and anchors currentTurnIndex to the same actor so insertions ahead of the active turn don't skip it. - combat-manage handler: when encounterId is set and refers to an active engine, call addParticipants instead of creating a new encounter. Falls back to new-encounter behavior when the id is missing or the engine can't be found. - Also propagates preset ac/attackDamage/attackBonus on the spawned participants so they hit and take hits like regular enemies. Closes #48 Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…s currentTurn from turnOrder Reviewers (PR #58) flagged three issues with the append path: 1. currentTurn was derived as state.participants[state.currentTurnIndex].id, but currentTurnIndex indexes turnOrder, not participants. With LAIR in the mix the two arrays can diverge and we'd report the wrong actor. 2. When encounterId was supplied but the engine wasn't in memory (server restart, eviction), we silently fell through to creating a new encounter — clients lost their existing one without warning. 3. The mutation lived only in memory; a restart would lose the spawned enemies entirely. This commit: - Auto-loads the engine from EncounterRepository when not in memory (mirrors handleGetEncounterState / handleExecuteCombatAction). - Persists state via repo.saveState after addParticipants. - Reads currentTurn from state.turnOrder[state.currentTurnIndex]. - Adds loadedFromDb to the response so callers can tell the path apart. - Regression tests for both the currentTurn invariant and the auto-load path. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Reviewers (PR #58) flagged that even with the new auto-load path, when the requested encounter was missing from BOTH memory and DB the code silently fell through to creating a fresh encounter. That hid typos and stale ids — callers got back a different encounter id than they asked for and never noticed. Now returns an explicit error pointing back at the requested id so the caller can correct it. The omitted-encounterId path (legitimate fresh-encounter case) is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Reviewers (PR #58 round 3) flagged that swallowing saveState errors left memory and DB out of sync — the caller saw success, but a restart would lose the appended enemies. The encounter would silently revert without any warning. Now snapshots the participant id set before addParticipants, attempts saveState, and on failure rolls back the in-memory mutation (participants + turnOrder) before returning an explicit error with { error: true, rolledBack: true, message }. Also tightened the currentTurn assertion per the same reviewer note — the previous test was order-coupled to turnOrder[0] which can flake on initiative resort. Adds a regression test that stubs EncounterRepository.saveState to throw and asserts: error response, rolledBack flag, and unchanged in-memory participant count. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
e72a765 to
5cd7b1a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/server/consolidated/combat-manage.ts (1)
311-334:⚠️ Potential issue | 🟠 MajorRestore
currentTurnIndexin the rollback path.
addParticipants()re-anchors the active turn, but thecatchblock only removes the appended IDs fromparticipantsandturnOrder. After a failed save,currentTurnIndexcan still point at the shifted position, which changes the active actor or leaves the index out of bounds for the restored order.Suggested fix
if (engine) { // Snapshot for rollback before mutating in-memory state. const beforeIds = new Set(engine.getState()?.participants.map((p) => p.id) ?? []); + const beforeTurnIndex = engine.getState()?.currentTurnIndex ?? 0; const state = engine.addParticipants( participants as unknown as Parameters<typeof engine.addParticipants>[0] ); @@ const live = engine.getState(); if (live) { live.participants = live.participants.filter((p) => beforeIds.has(p.id)); live.turnOrder = live.turnOrder.filter((id) => id === 'LAIR' || beforeIds.has(id)); + live.currentTurnIndex = Math.min( + Math.max(beforeTurnIndex, 0), + Math.max(live.turnOrder.length - 1, 0) + ); } return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/consolidated/combat-manage.ts` around lines 311 - 334, The rollback in the catch block removes appended participants and cleans live.participants and live.turnOrder but forgets to restore the engine's currentTurnIndex, leaving it pointing at a shifted or out-of-bounds position; update the rollback to compute and restore the original currentTurnIndex from the pre-mutation snapshot (e.g., capture beforeIndex from engine.getState()?.currentTurnIndex before calling addParticipants or recompute it by locating the active id within the filtered live.turnOrder) so that live.currentTurnIndex (or engine.getState()?.currentTurnIndex) is set back to the original value whenever you filter participants/turnOrder as part of the rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/consolidated/combat-manage.ts`:
- Around line 311-334: The rollback in the catch block removes appended
participants and cleans live.participants and live.turnOrder but forgets to
restore the engine's currentTurnIndex, leaving it pointing at a shifted or
out-of-bounds position; update the rollback to compute and restore the original
currentTurnIndex from the pre-mutation snapshot (e.g., capture beforeIndex from
engine.getState()?.currentTurnIndex before calling addParticipants or recompute
it by locating the active id within the filtered live.turnOrder) so that
live.currentTurnIndex (or engine.getState()?.currentTurnIndex) is set back to
the original value whenever you filter participants/turnOrder as part of the
rollback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cd905e0-db91-4a46-8f0f-b2684baeded4
📒 Files selected for processing (3)
src/engine/combat/engine.tssrc/server/consolidated/combat-manage.tstests/server/consolidated/combat-manage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/combat/engine.ts
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
The
encounterIdparameter onspawn_quick_enemywas documented as "Add to existing encounter" but ignored — every call created a fresh encounter. Party and enemies ended up split across two engines.Fix
CombatEngine.addParticipants()— rolls initiative, resorts turn order, anchorscurrentTurnIndexto the same actor so insertions ahead of the active turn don't skip it.spawn_quick_enemyhandler: whenencounterIdis supplied and the engine exists, append viaaddParticipants. Omitted / unknown id → current new-encounter behavior.ac/attackDamage/attackBonusso spawned enemies resolve attacks normally.Test plan
appendedToExisting: true, sameencounterId,turnOrder.length === 3.encounterId→ creates a fresh encounter (current behavior preserved).Closes #48
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests