TODO.md

@@ -4,9 +4,13 @@ Backlog of bugs + long-term items. Milestones live in REWORK_PLAN.md.
 
 ## Open bugs
 
-### BUG-7: reorderParticipants has no undo
+### BUG-7: reorderParticipants not logged
-- `reorderParticipants` returns `log: null`. Other ops return `log.undo`.
+- FIXED — now returns `log: { message, undo }`. Handler calls logAction.
-- Cannot undo drag-drop. Candidate for undo system (M6).
+  Undo snapshots participants[] + turnOrderIds + pointer.
+- Found second gap: deathSave also returned null log. Fixed (message + undo).
+- Logging contract test added: turn.logging.test.js (all mutating ops logged,
+  no-ops null, undo payloads valid). Documents addParticipants +
+  updateParticipant gaps (null log, intentional).
 
 ## Open features
 

shared/tests/turn.bug7.test.js

@@ -0,0 +1,54 @@
+// BUG-7: reorderParticipants not logged.
+// Every combat op returns { patch, log }. Handler calls logAction(log.message,
+// ctx, { updates: log.undo }) → writes entry to logs collection.
+// reorderParticipants returns log: null → handler skips logAction → drag
+// invisible in combat log + no undo payload (siblings all have one).
+//
+// RED: prove log is null (bug). Fix: return { message, undo }.
+
+'use strict';
+
+const shared = require('@ttrpg/shared');
+const { makeParticipant, reorderParticipants } = shared;
+
+function p(id, init) {
+  return makeParticipant({ id, name: id, type: 'monster',
+    initiative: init, maxHp: 100, currentHp: 100 });
+}
+function enc(ps) {
+  return { name:'t', participants:ps, isStarted:false, isPaused:false,
+    round:0, currentTurnParticipantId:null, turnOrderIds:[] };
+}
+
+describe('BUG-7: reorderParticipants logged', () => {
+  test('returns log object (not null) with message + undo', () => {
+    const e = enc([p('a', 10), p('b', 10), p('c', 10)]);
+    const r = reorderParticipants(e, 'c', 'b');
+    expect(r.patch).not.toBeNull();
+    expect(r.log).not.toBeNull();
+    expect(typeof r.log.message).toBe('string');
+    expect(r.log.message.length).toBeGreaterThan(0);
+    expect(r.log.undo).toBeDefined();
+  });
+
+  test('undo restores original participants order', () => {
+    const e = enc([p('a', 10), p('b', 10), p('c', 10)]);
+    const orig = e.participants.map(p => p.id);
+    const r = reorderParticipants(e, 'c', 'b');
+    const restored = { ...e, ...r.log.undo };
+    expect(restored.participants.map(p => p.id)).toEqual(orig);
+  });
+
+  test('no-op (same id) returns null log', () => {
+    const e = enc([p('a', 10), p('b', 10)]);
+    const r = reorderParticipants(e, 'a', 'a');
+    expect(r.log).toBeNull();
+  });
+
+  test('no-op (cross-init blocked) returns null log', () => {
+    const e = enc([p('a', 10), p('b', 5)]);
+    const r = reorderParticipants(e, 'a', 'b');
+    expect(r.patch).toBeNull();
+    expect(r.log).toBeNull();
+  });
+});

shared/tests/turn.logging.test.js

@@ -0,0 +1,188 @@
+// Logging contract: every mutating combat op returns { patch, log } where
+// log = { message: string, undo: object|null }. No-op (no state change)
+// returns { patch: null, log: null }. Display lifecycle ops return { patch }
+// only (no log field expected).
+//
+// logAction handler does:
+//   if (log) logAction(log.message, ctx, { updates: log.undo })
+// → null log = skipped = gap in audit trail.
+//
+// Contract = every combat mutation MUST produce a log entry.
+
+'use strict';
+
+const shared = require('@ttrpg/shared');
+const {
+  makeParticipant, buildMonsterParticipant,
+  startEncounter, nextTurn, togglePause,
+  addParticipant, addParticipants, updateParticipant, removeParticipant,
+  toggleParticipantActive, applyHpChange, deathSave, toggleCondition,
+  reorderParticipants, endEncounter,
+} = shared;
+
+function p(id, init) {
+  return makeParticipant({ id, name: id, type: 'monster',
+    initiative: init, maxHp: 100, currentHp: 100 });
+}
+function enc(ps, extra = {}) {
+  return { name:'t', participants:ps, isStarted:false, isPaused:false,
+    round:0, currentTurnParticipantId:null, turnOrderIds:[], ...extra };
+}
+const apply = (e, r) => r && r.patch ? { ...e, ...r.patch } : e;
+
+// Helper: assert a result is a logged mutation (patch + valid log).
+function expectLogged(r) {
+  expect(r.patch).not.toBeNull();
+  expect(r.log).not.toBeNull();
+  expect(typeof r.log).toBe('object');
+  expect(typeof r.log.message).toBe('string');
+  expect(r.log.message.length).toBeGreaterThan(0);
+}
+function expectNoOp(r) {
+  expect(r.patch).toBeNull();
+  expect(r.log).toBeNull();
+}
+
+describe('Logging contract: mutating ops', () => {
+  let e;
+  beforeEach(() => {
+    e = enc([p('a', 10), p('b', 7), p('c', 3)]);
+  });
+
+  test('startEncounter logs', () => {
+    const r = startEncounter(e);
+    expectLogged(r);
+  });
+
+  test('nextTurn logs', () => {
+    const started = apply(e, startEncounter(e));
+    const r = nextTurn(started);
+    expectLogged(r);
+  });
+
+  test('togglePause logs', () => {
+    const r = togglePause(apply(e, startEncounter(e)));
+    expectLogged(r);
+  });
+
+  test('addParticipant logs', () => {
+    const r = addParticipant(e, p('d', 5));
+    expectLogged(r);
+  });
+
+  test('removeParticipant logs', () => {
+    const r = removeParticipant(e, 'b');
+    expectLogged(r);
+  });
+
+  test('toggleParticipantActive logs', () => {
+    const r = toggleParticipantActive(e, 'b');
+    expectLogged(r);
+  });
+
+  test('applyHpChange (damage) logs', () => {
+    const r = applyHpChange(e, 'b', 'damage', 10);
+    expectLogged(r);
+  });
+
+  test('applyHpChange (heal) logs', () => {
+    const r = applyHpChange(e, 'b', 'heal', 5);
+    expectLogged(r);
+  });
+
+  test('deathSave logs', () => {
+    const started = apply(e, startEncounter(e));
+    const dying = apply(started, applyHpChange(started, 'b', 'damage', 100));
+    const r = deathSave(dying, 'b', 1);
+    expectLogged(r);
+  });
+
+  test('toggleCondition logs', () => {
+    const r = toggleCondition(e, 'b', 'poisoned');
+    expectLogged(r);
+  });
+
+  test('reorderParticipants logs (BUG-7)', () => {
+    // same-init tie (both 10) for valid reorder
+    const e2 = enc([p('a', 10), p('x', 10), p('c', 3)]);
+    const r = reorderParticipants(e2, 'x', 'a');
+    expectLogged(r);
+  });
+
+  test('endEncounter logs', () => {
+    const started = apply(e, startEncounter(e));
+    const r = endEncounter(started);
+    expectLogged(r);
+  });
+});
+
+describe('Logging contract: no-ops', () => {
+  let e;
+  beforeEach(() => {
+    e = enc([p('a', 10), p('b', 7), p('c', 3)]);
+  });
+
+  test('reorder same-id = no-op', () => {
+    expectNoOp(reorderParticipants(e, 'a', 'a'));
+  });
+
+  test('reorder cross-init = no-op', () => {
+    expectNoOp(reorderParticipants(e, 'a', 'b'));
+  });
+});
+
+describe('Logging undo payloads', () => {
+  let e;
+  beforeEach(() => {
+    e = enc([p('a', 10), p('b', 7), p('c', 3)]);
+  });
+
+  test('startEncounter undo restores pre-combat state', () => {
+    const r = startEncounter(e);
+    expect(r.log.undo).toBeDefined();
+    expect(r.log.undo.isStarted).toBe(false);
+  });
+
+  test('endEncounter undo restores combat state', () => {
+    const started = apply(e, startEncounter(e));
+    const r = endEncounter(started);
+    expect(r.log.undo).toBeDefined();
+    expect(r.log.undo.isStarted).toBe(true);
+  });
+
+  test('applyHpChange undo restores prior hp', () => {
+    const r = applyHpChange(e, 'b', 'damage', 10);
+    expect(r.log.undo).toBeDefined();
+    expect(r.log.undo.participants).toBeDefined();
+  });
+
+  test('reorder undo restores prior order (BUG-7)', () => {
+    const e2 = enc([p('a', 10), p('x', 10), p('c', 3)]);
+    const orig = e2.participants.map(p => p.id);
+    const r = reorderParticipants(e2, 'x', 'a');
+    expect(r.log.undo).toBeDefined();
+    const restored = { ...e2, ...r.log.undo };
+    expect(restored.participants.map(p => p.id)).toEqual(orig);
+  });
+});
+
+describe('Logging: gaps (currently unlogged mutations)', () => {
+  // Document current behavior. addParticipants + updateParticipant return
+  // null log → invisible in combat log. May be intended (bulk/no-op) or bug.
+  let e;
+  beforeEach(() => {
+    e = enc([p('a', 10), p('b', 7)]);
+  });
+
+  test('addParticipants returns null log (bulk gap?)', () => {
+    const r = addParticipants(e, [p('c', 3)]);
+    // Currently null. Document so decision is explicit.
+    expect(r.log).toBeNull();
+  });
+
+  test('updateParticipant returns null log (edit gap?)', () => {
+    const r = updateParticipant(e, 'b', { name: 'B' });
+    // Currently null. Document so decision is explicit.
+    expect(r.log).toBeNull();
+  });
+});

shared/turn.js

@@ -477,6 +477,8 @@ function deathSave(encounter, participantId, saveNumber) {
   if (!participant) throw new Error('Participant not found.');
   const currentSaves = participant.deathSaves || 0;
   const newSaves = currentSaves === saveNumber ? saveNumber - 1 : saveNumber;
+  const name = participant.name;
+  const undo = { participants: encounter.participants || [] };
 
   if (newSaves === 3) {
     // Mark dying — caller waits for animation, then calls removeParticipant.
@@ -485,7 +487,10 @@ function deathSave(encounter, participantId, saveNumber) {
     );
     return {
       patch: { participants: updatedParticipants },
-      log: null,
+      log: {
+        message: `${name} failed 3 death saves — dying`,
+        undo,
+      },
       isDying: true,
     };
   }
@@ -493,7 +498,14 @@ function deathSave(encounter, participantId, saveNumber) {
   const updatedParticipants = (encounter.participants || []).map(p =>
     p.id === participantId ? { ...p, deathSaves: newSaves } : p
   );
-  return { patch: { participants: updatedParticipants }, log: null, isDying: false };
+  return {
+    patch: { participants: updatedParticipants },
+    log: {
+      message: `${name} death save: ${newSaves}/3`,
+      undo,
+    },
+    isDying: false,
+  };
 }
 
 // TOGGLE_CONDITION — verbatim from ParticipantManager.toggleCondition
@@ -553,7 +565,17 @@ function reorderParticipants(encounter, draggedId, targetId) {
   const newTargetIndex = participants.findIndex(p => p.id === targetId);
   participants.splice(newTargetIndex, 0, removedItem);
   const turnUpdates = syncTurnOrder(participants); // 1-list: always mirror
-  return { patch: { participants, ...turnUpdates }, log: null };
+  return {
+    patch: { participants, ...turnUpdates },
+    log: {
+      message: `${removedItem.name} reordered before ${(participants.find(p => p.id === targetId) || {}).name || targetId}`,
+      undo: {
+        participants: encounter.participants || [],
+        turnOrderIds: encounter.turnOrderIds || [],
+        currentTurnParticipantId: encounter.currentTurnParticipantId ?? null,
+      },
+    },
+  };
 }
 
 // END_ENCOUNTER — verbatim from InitiativeControls.confirmEndEncounter

src/App.js

@@ -1084,8 +1084,14 @@ function ParticipantManager({ encounter, encounterPath, campaignCharacters }) {
       return;
     }
     try {
-      const { patch } = reorderParticipants(encounter, draggedItemId, targetId);
+      const { patch, log } = reorderParticipants(encounter, draggedItemId, targetId);
       await storage.updateDoc(encounterPath, patch);
+      if (log) {
+        logAction(log.message, { encounterName: encounter.name }, {
+          encounterPath,
+          updates: log.undo,
+        });
+      }
     } catch (err) {
       // drag invalid (id not found) — ignore
     }