fix(turn): slot-not-sort in add/update; static guard against stray .sort()
addParticipant + updateParticipant used sortParticipantsByInitiative (stable sort, tie-break = original array index). That destroyed manually-dragged tie order when a drag moved a same-init pair — violated the slot-not-sort design (docs/INITIATIVE_ORDERING.md: 'Re-slotting on add/edit must preserve drag-established tie order'). Fix: new slotIndexForInit(list, init) returns splice index into the CURRENT list (initiative-descending). addParticipant inserts there; updateParticipant re-inserts only when initiative changed (unrelated edits keep slot — avoids mid-round rotation dupes surfaced by the 100-round combat test). Tests: - turn.slot-not-sort.test.js: drag tie order survives add/edit (4 cases). - static.no-sort.test.js: errs if .sort( added outside allowlist (sortParticipantsByInitiative only). Verified by injecting a stray sort — guard caught it.
This commit is contained in:
@@ -0,0 +1,70 @@
|
|||||||
|
// STATIC GUARD: no `.sort(` introduced in shared/turn.js outside the ONE
|
||||||
|
// allowed function (sortParticipantsByInitiative, used by startEncounter to
|
||||||
|
// freeze the list once). Slot-not-sort design (docs/INITIATIVE_ORDERING.md):
|
||||||
|
// mutations = insert/move, never wholesale re-sort. A stray `.sort(` after
|
||||||
|
// start destroys drag tie-break order.
|
||||||
|
//
|
||||||
|
// This test errs the moment someone adds `.sort(` anywhere but the allowlist.
|
||||||
|
// Maintenance: add a function to ALLOWED only if it runs at startEncounter
|
||||||
|
// (one-time freeze), NOT in add/update/reorder/nextTurn paths.
|
||||||
|
|
||||||
|
'use strict';
|
||||||
|
|
||||||
|
const fs = require('fs');
|
||||||
|
const path = require('path');
|
||||||
|
|
||||||
|
const SRC = fs.readFileSync(path.join(__dirname, '..', 'turn.js'), 'utf8');
|
||||||
|
|
||||||
|
// Functions permitted to call .sort(. Listed so intent is explicit + reviewed.
|
||||||
|
const ALLOWED_SORT_FUNCTIONS = new Set([
|
||||||
|
'sortParticipantsByInitiative',
|
||||||
|
]);
|
||||||
|
|
||||||
|
// Collect top-level function bodies by brace counting.
|
||||||
|
// Matches `function NAME(` decls AND `const NAME = ... =>` arrow fns.
|
||||||
|
// Returns [{ name, body }].
|
||||||
|
function collectBody(src, fromIdx) {
|
||||||
|
let i = fromIdx;
|
||||||
|
while (i < src.length && src[i] !== '{') i++;
|
||||||
|
let depth = 0;
|
||||||
|
const start = i;
|
||||||
|
for (; i < src.length; i++) {
|
||||||
|
if (src[i] === '{') depth++;
|
||||||
|
else if (src[i] === '}') { depth--; if (depth === 0) break; }
|
||||||
|
}
|
||||||
|
return src.slice(start, i + 1);
|
||||||
|
}
|
||||||
|
|
||||||
|
function extractFunctions(src) {
|
||||||
|
const out = [];
|
||||||
|
const fnRe = /\bfunction\s+([A-Za-z0-9_$]+)\s*\(/g;
|
||||||
|
const arrowRe = /(?:const|let|var)\s+([A-Za-z0-9_$]+)\s*=\s*[^=;]*?=>/g;
|
||||||
|
let m;
|
||||||
|
while ((m = fnRe.exec(src)) !== null) {
|
||||||
|
out.push({ name: m[1], body: collectBody(src, m.index + m[0].length) });
|
||||||
|
}
|
||||||
|
while ((m = arrowRe.exec(src)) !== null) {
|
||||||
|
out.push({ name: m[1], body: collectBody(src, m.index + m[0].length) });
|
||||||
|
}
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('STATIC: no .sort( outside allowlist (slot-not-sort design)', () => {
|
||||||
|
test('every .sort( call lives in an allowed function', () => {
|
||||||
|
const fns = extractFunctions(SRC);
|
||||||
|
const offenders = [];
|
||||||
|
for (const { name, body } of fns) {
|
||||||
|
if (!ALLOWED_SORT_FUNCTIONS.has(name) && /\.sort\(/.test(body)) {
|
||||||
|
offenders.push(name);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
expect(offenders).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('allowed sort function is declared (no silent allowlist drift)', () => {
|
||||||
|
const declared = new Set(extractFunctions(SRC).map(f => f.name));
|
||||||
|
for (const name of ALLOWED_SORT_FUNCTIONS) {
|
||||||
|
expect(declared.has(name)).toBe(true);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -0,0 +1,77 @@
|
|||||||
|
// SLOT-NOT-SORT invariant: addParticipant + updateParticipant must PRESERVE
|
||||||
|
// manually-dragged tie order (same-initiative participants).
|
||||||
|
//
|
||||||
|
// Design doc (docs/INITIATIVE_ORDERING.md):
|
||||||
|
// "Re-slotting on add/edit must preserve drag-established tie order."
|
||||||
|
// "Tie-break = original add order. Later additions slot AFTER existing
|
||||||
|
// same-init participants. Stable insertion."
|
||||||
|
//
|
||||||
|
// Current code uses sortParticipantsByInitiative (stable sort, tie-break =
|
||||||
|
// original array index). That is NOT stable insertion: it re-sorts the whole
|
||||||
|
// list, destroying drag order when a drag moved a same-init pair.
|
||||||
|
//
|
||||||
|
// RED now: drag tie order survives neither add nor edit.
|
||||||
|
|
||||||
|
'use strict';
|
||||||
|
|
||||||
|
const shared = require('@ttrpg/shared');
|
||||||
|
const {
|
||||||
|
makeParticipant,
|
||||||
|
startEncounter, addParticipant, updateParticipant, reorderParticipants,
|
||||||
|
} = shared;
|
||||||
|
|
||||||
|
function p(id, init, extra = {}) {
|
||||||
|
return makeParticipant({ id, name: id, type: 'monster',
|
||||||
|
initiative: init, maxHp: 100, currentHp: 100, ...extra });
|
||||||
|
}
|
||||||
|
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;
|
||||||
|
|
||||||
|
describe('SLOT-NOT-SORT: drag tie order survives add/edit', () => {
|
||||||
|
test('addParticipant preserves existing drag tie order', () => {
|
||||||
|
// Two same-init participants. DM drags b BEFORE a (tie override).
|
||||||
|
let e = enc([p('a', 10), p('b', 10)]);
|
||||||
|
e = apply(e, reorderParticipants(e, 'b', 'a')); // drag b ahead of a → [b,a]
|
||||||
|
expect(e.participants.map(x => x.id)).toEqual(['b', 'a']);
|
||||||
|
|
||||||
|
// Add unrelated participant (different init). Tie order [b,a] MUST survive.
|
||||||
|
e = apply(e, addParticipant(e, p('c', 5)));
|
||||||
|
const ids = e.participants.map(x => x.id);
|
||||||
|
// c slots last (init 5 < 10). b,a tie order preserved.
|
||||||
|
expect(ids).toEqual(['b', 'a', 'c']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('addParticipant with same init as dragged pair slots AFTER tie group', () => {
|
||||||
|
// DM drags b before a (both init 10). Then add d also init 10.
|
||||||
|
// d must slot AFTER existing same-init pair (stable insertion), not re-sort.
|
||||||
|
let e = enc([p('a', 10), p('b', 10)]);
|
||||||
|
e = apply(e, reorderParticipants(e, 'b', 'a')); // [b,a]
|
||||||
|
e = apply(e, addParticipant(e, p('d', 10))); // init 10, after tie group
|
||||||
|
const ids = e.participants.map(x => x.id);
|
||||||
|
expect(ids).toEqual(['b', 'a', 'd']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('updateParticipant preserves drag tie order on unrelated field edit', () => {
|
||||||
|
let e = enc([p('a', 10), p('b', 10)]);
|
||||||
|
e = apply(e, reorderParticipants(e, 'b', 'a')); // [b,a]
|
||||||
|
// Edit a's name (not initiative). Tie order MUST survive.
|
||||||
|
e = apply(e, updateParticipant(e, 'a', { name: 'A2' }));
|
||||||
|
expect(e.participants.map(x => x.id)).toEqual(['b', 'a']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('updateParticipant init change slots, preserves OTHER tie group order', () => {
|
||||||
|
// Two tie groups: [a,b] init 10, [c,d] init 5. DM drags b before a.
|
||||||
|
let e = enc([p('a', 10), p('b', 10), p('c', 5), p('d', 5)]);
|
||||||
|
e = apply(e, reorderParticipants(e, 'b', 'a')); // [b,a,c,d]
|
||||||
|
// Edit c's initiative to 10 — slots into top group. [c] order within its
|
||||||
|
// own old group irrelevant (it leaves that group). But [b,a] tie MUST stay.
|
||||||
|
e = apply(e, updateParticipant(e, 'c', { initiative: 10 }));
|
||||||
|
const topTwo = e.participants.filter(x => x.initiative === 10).map(x => x.id);
|
||||||
|
// b before a preserved. c joins the 10-group (exact slot less critical
|
||||||
|
// than b,a order surviving).
|
||||||
|
expect(topTwo.indexOf('b')).toBeLessThan(topTwo.indexOf('a'));
|
||||||
|
});
|
||||||
|
});
|
||||||
+44
-16
@@ -31,9 +31,18 @@ const formatInitMod = (mod) => {
|
|||||||
return mod >= 0 ? `+${mod}` : `${mod}`;
|
return mod >= 0 ? `+${mod}` : `${mod}`;
|
||||||
};
|
};
|
||||||
|
|
||||||
// Sort used ONLY at insert points (startEncounter, addParticipant) to position
|
// SLOT, NEVER SORT. 1-list model (docs/INITIATIVE_ORDERING.md).
|
||||||
// participants by initiative. Once positioned, turnOrderIds = participants.map(id)
|
//
|
||||||
// (1-list model). No re-sort after start — drag/edit are manual overrides.
|
// `sortParticipantsByInitiative` exists for ONE call site: startEncounter
|
||||||
|
// (freeze list once by init). After that, mutations = insert/move ops that
|
||||||
|
// PRESERVE existing array order. No wholesale re-sort ever — destroys drag
|
||||||
|
// tie-break order.
|
||||||
|
//
|
||||||
|
// slotIndexForInit: pure insert position. Scans existing list, returns index
|
||||||
|
// of first participant with init STRICTLY LESS than target. New participant
|
||||||
|
// splices there. Same-init participants stay ahead (stable). Existing tie
|
||||||
|
// order (incl. drag-established) untouched because scan reads current array,
|
||||||
|
// not original add-order.
|
||||||
const sortParticipantsByInitiative = (participants, originalOrder) => {
|
const sortParticipantsByInitiative = (participants, originalOrder) => {
|
||||||
return [...participants].sort((a, b) => {
|
return [...participants].sort((a, b) => {
|
||||||
if (a.initiative === b.initiative) {
|
if (a.initiative === b.initiative) {
|
||||||
@@ -45,6 +54,17 @@ const sortParticipantsByInitiative = (participants, originalOrder) => {
|
|||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// Find splice index for a single participant by initiative. List assumed
|
||||||
|
// already initiative-descending (startEncounter freezes it; add/edit maintain).
|
||||||
|
// Returns position to insert so list stays descending, ties go AFTER existing
|
||||||
|
// same-init (stable insertion). Current array order = source of truth.
|
||||||
|
function slotIndexForInit(list, init) {
|
||||||
|
for (let i = 0; i < list.length; i++) {
|
||||||
|
if (init > list[i].initiative) return i;
|
||||||
|
}
|
||||||
|
return list.length;
|
||||||
|
}
|
||||||
|
|
||||||
// 1-LIST SYNC: turnOrderIds always mirrors participants[].map(id).
|
// 1-LIST SYNC: turnOrderIds always mirrors participants[].map(id).
|
||||||
// Call after any participants[] mutation. Returns turnOrderIds patch.
|
// Call after any participants[] mutation. Returns turnOrderIds patch.
|
||||||
const syncTurnOrder = (participants) => ({
|
const syncTurnOrder = (participants) => ({
|
||||||
@@ -311,12 +331,12 @@ function addParticipant(encounter, participant) {
|
|||||||
if ((encounter.participants || []).some(p => p.id === participant.id)) {
|
if ((encounter.participants || []).some(p => p.id === participant.id)) {
|
||||||
throw new Error(`Participant with id "${participant.id}" already exists in encounter.`);
|
throw new Error(`Participant with id "${participant.id}" already exists in encounter.`);
|
||||||
}
|
}
|
||||||
// FEAT-3: reslot by initiative on add (stable sort, tie-break original index).
|
// SLOT (not sort): insert by initiative into current list. Preserves
|
||||||
// Pre-combat: list reflects init order immediately. Post-combat: slots into running order.
|
// existing array order incl. drag-established tie order.
|
||||||
const updatedParticipants = sortParticipantsByInitiative(
|
const base = [...(encounter.participants || [])];
|
||||||
[...(encounter.participants || []), participant],
|
const idx = slotIndexForInit(base, participant.initiative);
|
||||||
encounter.participants || []
|
base.splice(idx, 0, participant);
|
||||||
);
|
const updatedParticipants = base;
|
||||||
return {
|
return {
|
||||||
patch: { participants: updatedParticipants, ...syncTurnOrder(updatedParticipants) },
|
patch: { participants: updatedParticipants, ...syncTurnOrder(updatedParticipants) },
|
||||||
log: {
|
log: {
|
||||||
@@ -339,13 +359,21 @@ function addParticipants(encounter, newParticipants) {
|
|||||||
|
|
||||||
// UPDATE_PARTICIPANT — edit modal save (name/initiative/hp/isNpc).
|
// UPDATE_PARTICIPANT — edit modal save (name/initiative/hp/isNpc).
|
||||||
function updateParticipant(encounter, participantId, updatedData) {
|
function updateParticipant(encounter, participantId, updatedData) {
|
||||||
const updatedParticipants = (encounter.participants || []).map(p =>
|
const existing = encounter.participants || [];
|
||||||
p.id === participantId ? { ...p, ...updatedData } : p
|
const target = existing.find(p => p.id === participantId);
|
||||||
);
|
if (!target) throw new Error(`Participant "${participantId}" not found.`);
|
||||||
// FEAT-3: reslot by initiative so any init change reflects in list order
|
const merged = { ...target, ...updatedData };
|
||||||
// immediately (stable sort, tie-break = original array index).
|
// SLOT only if initiative changed. Other edits (name/hp/notes/isNpc) = no
|
||||||
const reslotted = sortParticipantsByInitiative(updatedParticipants, encounter.participants || []);
|
// position change (design doc). Re-slots on unrelated edits shuffle the
|
||||||
return { patch: { participants: reslotted, ...syncTurnOrder(reslotted) }, log: null };
|
// list mid-round → rotation dupes.
|
||||||
|
if (!('initiative' in updatedData) || updatedData.initiative === target.initiative) {
|
||||||
|
const sameSlot = existing.map(p => p.id === participantId ? merged : p);
|
||||||
|
return { patch: { participants: sameSlot, ...syncTurnOrder(sameSlot) }, log: null };
|
||||||
|
}
|
||||||
|
const without = existing.filter(p => p.id !== participantId);
|
||||||
|
const idx = slotIndexForInit(without, merged.initiative);
|
||||||
|
without.splice(idx, 0, merged);
|
||||||
|
return { patch: { participants: without, ...syncTurnOrder(without) }, log: null };
|
||||||
}
|
}
|
||||||
|
|
||||||
// REMOVE_PARTICIPANT — verbatim from ParticipantManager.confirmDeleteParticipant
|
// REMOVE_PARTICIPANT — verbatim from ParticipantManager.confirmDeleteParticipant
|
||||||
|
|||||||
Reference in New Issue
Block a user