From 942545c0854ac0ca0e0a24a0758dcc79889d0de6 Mon Sep 17 00:00:00 2001 From: david raistrick <1108844+keen99@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:12:13 -0400 Subject: [PATCH] fix(turn): block cross-pointer reorder (BUG-13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reorderParticipants allowed dragging across current pointer mid-round. nextTurn walks turnOrderIds forward from pointer, so cross-pointer drag = ambiguous who-acts-next: upcoming dragged ahead of pointer = skipped (walk wraps past), acted dragged behind pointer = double-act. Full fix needs actedThisRound tracking. Pragmatic now: block both directions during active encounter. Pre-combat: free reorder (no pointer). reorderParticipants: added pointer check (currentTurn index). If drag crosses pointer, return null patch (no-op). Same-side reorder allowed. syncTurnOrder now always mirrors (was started-only — pre-combat reorder left turnOrderIds stale). Tests: - turn.bug13.test.js: cross-pointer blocked (both dirs), same-side allowed, pre-combat free. - turn.reorder.test.js: 3 tests updated to pre-combat (cross-pointer now blocked post-start). - turn.invariant.test.js: reorder test uses same-side upcoming pair. 214 tests green. --- TODO.md | 58 ++++++++++++----------- shared/tests/turn.bug13.test.js | 72 +++++++++++++++++++++++++++++ shared/tests/turn.invariant.test.js | 15 +++--- shared/tests/turn.reorder.test.js | 30 ++++++------ shared/turn.js | 25 ++++++++-- 5 files changed, 148 insertions(+), 52 deletions(-) create mode 100644 shared/tests/turn.bug13.test.js diff --git a/TODO.md b/TODO.md index b3adffd..ed61b97 100644 --- a/TODO.md +++ b/TODO.md @@ -8,19 +8,11 @@ Backlog of bugs + long-term items. Milestones live in REWORK_PLAN.md. - `reorderParticipants` returns `log: null`. Other ops return `log.undo`. - Cannot undo drag-drop. Candidate for undo system (M6). -### BUG-13: reorderParticipants crossing current pointer = ambiguous acted-semantics -- `reorderParticipants` (shared/turn.js) = pure drag, no pointer logic. -- Swapping two actors across current pointer mid-round = ambiguous - who-acted-this-round. Replay avoids crossing (adjacent upcoming pair only). -- Real app untested: if DM drags actor past current pointer mid-round, skip/ - double behavior undefined. -- Decide: block cross-pointer reorder, or define acted-semantics. RED needed. - ## Open features ### FEAT-2: upgrade app internal logs to be parseable - Goal: combat logs in storage store enough structured state to run - skip/rotation analysis on ANY historic round — not just replay stdout. + skip/rotation analysis on ANY historic round --- not just replay stdout. - Current logs: `{timestamp, message, encounterName, undo}`. Parser must guess roster from message strings. Brittle. - Upgrade: add structured fields at turn-state mutation log sites in @@ -32,11 +24,6 @@ Backlog of bugs + long-term items. Milestones live in REWORK_PLAN.md. - Then `scripts/analyze-turns.js` ingests app logs directly (adapter fetch). Works on real game sessions, any round, deterministic. -### FEAT-3: initiative first-class entry (add + edit) -- Current: only initMod at char-build. No initiative field at add-participant - or edit. 3-step to set after other steps. -- Need: initiative field at add-char, add-monster, AND edit participant. -- Related: tie-break = drag order (current, works). Expose clearly. ### FEAT-M6: Transactional undo (moved from REWORK_PLAN) - Every mutating action writes event: `(type, payload, undo_payload, undone, ts)`. @@ -57,6 +44,12 @@ Backlog of bugs + long-term items. Milestones live in REWORK_PLAN.md. ### Test integrated timeouts - No per-test timeout. Tests hang on regression. Add jest timeout (<60s). +### add participants duplication throws a alert but it disappears fast + +### add particopents list dropdown - remove ones already added + + + ## Done (history) ### Architecture: 1-list turn order model @@ -75,51 +68,62 @@ Backlog of bugs + long-term items. Milestones live in REWORK_PLAN.md. - FIXED (addParticipant throws on dup id). ### BUG-4: hide-player-HP breaks display view -- FIXED — mock honors setDoc{merge}, all 5 activeDisplay sites use merge. +- FIXED --- mock honors setDoc{merge}, all 5 activeDisplay sites use merge. ### BUG-5: mid-round addParticipant/revive corrupts rotation -- FIXED — slot-array + DRY advance core `nextActiveAfter`. +- FIXED --- slot-array + DRY advance core `nextActiveAfter`. ### BUG-6: reorderParticipants doesn't update turnOrderIds - FIXED structurally by 1-list model. ### BUG-8: server adapter has no reconnect -- FIXED — onclose reconnects + re-subscribes existing paths. +- FIXED --- onclose reconnects + re-subscribes existing paths. ### BUG-10: deact+reactivate same round double-acts participant -- FIXED — 1-list model keeps slot position on toggle. Reactivate does not +- FIXED --- 1-list model keeps slot position on toggle. Reactivate does not grant second turn. Test: turn.bug10.test.js. ### BUG-11: FE Combat.scenario test crashes -- FIXED — moved to shared/turn.combat.test.js, pure functions, 100 rounds. +- FIXED --- moved to shared/turn.combat.test.js, pure functions, 100 rounds. ### BUG-12: campaign selection follows activeDisplay - FIXED. +### BUG-13: reorderParticipants crossing current pointer = ambiguous acted-semantics +- FIXED --- block cross-pointer reorder during active encounter (both dirs). + Full fix needs actedThisRound tracking. Pragmatic block prevents skip/double. + Pre-combat: free reorder. + ### BUG-14: addParticipant init-insertion breaks after drag-reorder -- FIXED — slotIndexForInit scans current list (post-drag aware). +- FIXED --- slotIndexForInit scans current list (post-drag aware). ### BUG-15: DisplayView re-sorts (drag order not preserved) -- FIXED — display renders participants[] directly. +- FIXED --- display renders participants[] directly. ### BUG-16: subscribeCollection hook drops queryConstraints -- FIXED — neutral builders, both adapters honor orderBy/limit. +- FIXED --- neutral builders, both adapters honor orderBy/limit. ### BUG-17: dead SDK imports in App.js -- FIXED — trimmed (auth + getFirestore + getStorage remain). +- FIXED --- trimmed (auth + getFirestore + getStorage remain). ### BUG-18: stale comments reference deleted memory adapter - FIXED. ### FEAT-1: Dead participants stay in turn order -- DONE — applyHpChange no longer flips isActive on death. Dead stay in +- DONE --- applyHpChange no longer flips isActive on death. Dead stay in rotation, nextTurn visits them, PCs get death-save turn. ### feat: add all characters to participants list -- DONE — addParticipants bulk add wired (App.js:943). +- DONE --- addParticipants bulk add wired (App.js:943). ### Warning = failure in tests -- DONE — console.error/warn throw in test env. +- DONE --- console.error/warn throw in test env. ### combat.scenario 100 rounds not turns -- DONE — loops by actual round-wrap count. +- DONE --- loops by actual round-wrap count. + +### FEAT-3: initiative first-class entry (add + edit) DONE +- Current: only initMod at char-build. No initiative field at add-participant + or edit. 3-step to set after other steps. +- Need: initiative field at add-char, add-monster, AND edit participant. +- Related: tie-break = drag order (current, works). Expose clearly. diff --git a/shared/tests/turn.bug13.test.js b/shared/tests/turn.bug13.test.js new file mode 100644 index 0000000..6436f44 --- /dev/null +++ b/shared/tests/turn.bug13.test.js @@ -0,0 +1,72 @@ +// BUG-13: cross-pointer reorder blocked (skip/double-act otherwise). +// nextTurn walks turnOrderIds forward from current pointer. Dragging across +// pointer = ambiguous who-acts-next: +// - upcoming dragged ahead of pointer → walk wraps past → SKIPPED +// - acted dragged behind pointer → acts again → DOUBLE-ACT +// Full fix needs actedThisRound tracking. Pragmatic now: block both dirs +// during active encounter. Pre-combat: free reorder (no pointer). + +'use strict'; + +const shared = require('@ttrpg/shared'); +const { + makeParticipant, + startEncounter, nextTurn, 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:[] }; +} +const apply = (e, r) => r && r.patch ? { ...e, ...r.patch } : e; + +describe('BUG-13: cross-pointer reorder blocked during active encounter', () => { + test('dragging not-yet-acted participant ahead of pointer = no-op', () => { + // [a(10), b(10), c(10)]. a acts -> b current. c not yet acted. + let e = enc([p('a',10),p('b',10),p('c',10)]); + e = apply(e, startEncounter(e)); // a (idx0, pointer) + e = apply(e, nextTurn(e)); // b (idx1, pointer) + expect(e.currentTurnParticipantId).toBe('b'); + + // Drag c (idx2, behind pointer) before b (idx1, pointer). Crosses pointer. + // Would let c act by initiative-reinsert but nextTurn forward-walk skips. + // Blocked instead. + const r = reorderParticipants(e, 'c', 'b'); + expect(r.patch).toBeNull(); + expect(e.turnOrderIds).toEqual(['a','b','c']); // unchanged + }); + + test('dragging already-acted participant behind pointer = no-op', () => { + // [a(10), b(10), c(10)]. a acts (idx0, ahead of pointer). + let e = enc([p('a',10),p('b',10),p('c',10)]); + e = apply(e, startEncounter(e)); // a (pointer idx0) + e = apply(e, nextTurn(e)); // b (pointer idx1) + // Drag a (already acted, ahead of pointer) after c (behind pointer). + // Crosses pointer. Would let a act again. Blocked. + const r = reorderParticipants(e, 'a', 'c'); + expect(r.patch).toBeNull(); + }); + + test('reorder within same side of pointer = allowed', () => { + // [a(10), b(10), c(10), d(10)]. a current (pointer idx0). + // b,c,d all behind pointer (upcoming). Reorder among them = safe. + let e = enc([p('a',10),p('b',10),p('c',10),p('d',10)]); + e = apply(e, startEncounter(e)); // a (idx0) + // swap b,c (both idx1,2 — behind pointer). No cross. + const r = reorderParticipants(e, 'c', 'b'); + expect(r.patch).not.toBeNull(); + expect(r.patch.participants.map(p => p.id)).toEqual(['a','c','b','d']); + }); + + test('pre-combat: free reorder (no pointer)', () => { + // isStarted=false. No pointer. Reorder allowed freely. + let e = enc([p('a',10),p('b',10),p('c',10)]); + const r = reorderParticipants(e, 'c', 'a'); + expect(r.patch).not.toBeNull(); + expect(r.patch.participants.map(p => p.id)).toEqual(['c','a','b']); + }); +}); diff --git a/shared/tests/turn.invariant.test.js b/shared/tests/turn.invariant.test.js index 0471321..7a36428 100644 --- a/shared/tests/turn.invariant.test.js +++ b/shared/tests/turn.invariant.test.js @@ -90,12 +90,15 @@ describe('1-list model: turnOrderIds === participants.map(id), no re-sort', () = expect(e.turnOrderIds).toEqual(['a','b','c']); // unchanged }); - test('reorder same-init: rotation follows new list order', () => { - let e = enc([p('a',10),p('b',10),p('c',3)]); // a,b tie - e = apply(e, startEncounter(e)); // [a,b,c], cur=a - e = apply(e, reorderParticipants(e, 'b', 'a')); // [b,a,c], cur still a - const rot = walkRotation(e); // start a, next c (wrap), next b, back a - expect(rot).toEqual(['a','c','b']); + test('reorder same-init: within upcoming side of pointer follows new order', () => { + let e = enc([p('a',10),p('b',10),p('c',10),p('d',3)]); // a,b,c tie + e = apply(e, startEncounter(e)); // [a,b,c,d], cur=a (idx0) + // Reorder c before b (both upcoming idx1,2). Within same side = allowed. + e = apply(e, reorderParticipants(e, 'c', 'b')); // [a,c,b,d], cur=a + expect(e.turnOrderIds).toEqual(['a','c','b','d']); + // walk: a current, next c, next b, next d, wrap a + e = apply(e, nextTurn(e)); + expect(e.currentTurnParticipantId).toBe('c'); }); test('toggle inactive: list unchanged (stays in rotation slot)', () => { diff --git a/shared/tests/turn.reorder.test.js b/shared/tests/turn.reorder.test.js index 050c5bc..933e0fa 100644 --- a/shared/tests/turn.reorder.test.js +++ b/shared/tests/turn.reorder.test.js @@ -18,15 +18,12 @@ function enc(ps) { } describe('reorderParticipants', () => { - test('drag before target (1-list model)', () => { + test('drag before target (1-list model, pre-combat)', () => { const ps = [p('a', 10), p('b', 20), p('c', 20)]; // b,c tie - let e = enc(ps); - e = { ...e, ...startEncounter(e).patch }; - // initial order: b,c,a (init 20,20,10) - expect(e.turnOrderIds).toEqual(['b', 'c', 'a']); + let e = enc(ps); // pre-combat, no pointer const r = reorderParticipants(e, 'c', 'b'); - // drag c before b: remove c → [b,a], insert before b → [c,b,a] - expect(r.patch.participants.map(p => p.id)).toEqual(['c', 'b', 'a']); + // drag c before b: remove c → [a,b], insert before b → [a,c,b] + expect(r.patch.participants.map(p => p.id)).toEqual(['a', 'c', 'b']); }); test('cross-init drag blocked (no-op)', () => { @@ -47,21 +44,22 @@ describe('reorderParticipants', () => { test('syncs turnOrderIds = participants order (1-list, fixes BUG-6)', () => { const ps = [p('a', 10), p('b', 20), p('c', 20)]; let e = enc(ps); - e = { ...e, ...startEncounter(e).patch }; - const r = reorderParticipants(e, 'c', 'b'); - expect(r.patch.turnOrderIds).toEqual(['c', 'b', 'a']); - expect(r.patch.turnOrderIds).toEqual(r.patch.participants.map(p => p.id)); + e = { ...e, ...startEncounter(e).patch }; // started, but reorder pre-pointer advance + // startEncounter sets current=b (idx0). reorder c before b crosses pointer. + // Use pre-combat to test syncTurnOrder. + let pre = enc(ps); + pre = { ...pre, ...reorderParticipants(pre, 'c', 'b').patch }; + expect(pre.turnOrderIds).toEqual(['a','c','b']); + expect(pre.turnOrderIds).toEqual(pre.participants.map(p => p.id)); }); // BUG-6 candidate: reorder should affect turnOrderIds so mid-combat // drag-drop changes who goes next within same-initiative tie. // Currently RED (turnOrderIds not in patch). - test('reorder updates turnOrderIds to reflect new participant order', () => { + test('reorder updates turnOrderIds to reflect new participant order (pre-combat)', () => { const ps = [p('a', 10), p('b', 20), p('c', 20)]; - let e = enc(ps); - e = { ...e, ...startEncounter(e).patch }; - // order: b,c,a + let e = enc(ps); // pre-combat, no pointer e = { ...e, ...reorderParticipants(e, 'c', 'b').patch }; - expect(e.turnOrderIds).toEqual(['c', 'b', 'a']); + expect(e.turnOrderIds).toEqual(['a', 'c', 'b']); }); }); diff --git a/shared/turn.js b/shared/turn.js index 1888d0d..b87fc71 100644 --- a/shared/turn.js +++ b/shared/turn.js @@ -536,10 +536,13 @@ function toggleCondition(encounter, participantId, conditionId) { }; } -// REORDER_PARTICIPANTS — drag-drop. 1-list model: drag overrides initiative -// (DM choice). Cross-init drag allowed. Splices participants[], syncs turnOrderIds. // REORDER_PARTICIPANTS — drag. Same-initiative ONLY (tie-break override). // Cross-init drag = no-op (use edit-initiative field instead). Splice move. +// Cross-pointer drag = no-op once encounter started (BUG-13). nextTurn walks +// turnOrderIds forward from current pointer. Dragging a not-yet-acted +// participant to a position behind the pointer would skip them (wrap past). +// Dragging an already-acted participant ahead of pointer would re-act them. +// Blocking cross-pointer keeps rotation deterministic. Pre-combat: free. function reorderParticipants(encounter, draggedId, targetId) { const participants = [...(encounter.participants || [])]; const dragged = participants.find(p => p.id === draggedId); @@ -550,10 +553,26 @@ function reorderParticipants(encounter, draggedId, targetId) { return { patch: null, log: null }; // cross-init blocked } const draggedIndex = participants.findIndex(p => p.id === draggedId); + // BUG-13: block cross-pointer reorder while encounter running. nextTurn + // walks turnOrderIds forward from current pointer. Dragging across the + // pointer makes who-acts-next ambiguous — either skip (upcoming dragged + // ahead of pointer, walk wraps past) or double-act (acted dragged behind). + // Full fix needs actedThisRound tracking. Pragmatic now: block both dirs. + // Pre-combat: free reorder (no pointer yet). + if (encounter.isStarted && encounter.currentTurnParticipantId) { + const pointerIdx = participants.findIndex(p => p.id === encounter.currentTurnParticipantId); + const targetIdx0 = participants.findIndex(p => p.id === targetId); + if (pointerIdx >= 0) { + const crosses = (a, b) => (a <= pointerIdx && b > pointerIdx) || (a > pointerIdx && b <= pointerIdx); + if (crosses(draggedIndex, targetIdx0)) { + return { patch: null, log: null }; // cross-pointer blocked + } + } + } const [removedItem] = participants.splice(draggedIndex, 1); const newTargetIndex = participants.findIndex(p => p.id === targetId); participants.splice(newTargetIndex, 0, removedItem); - const turnUpdates = encounter.isStarted ? syncTurnOrder(participants) : {}; + const turnUpdates = syncTurnOrder(participants); // 1-list: always mirror return { patch: { participants, ...turnUpdates }, log: null }; }