diff --git a/.gitignore b/.gitignore index d559aa7..fd3f14b 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,4 @@ server/data/*.sqlite server/data/*.sqlite-* /data /scratch +tmp/ diff --git a/TODO.md b/TODO.md index 43517e4..f0f7b5c 100644 --- a/TODO.md +++ b/TODO.md @@ -13,6 +13,25 @@ TODO: combat.scenario doesnt do 100 rounds it does 100 turns. recover from kill +TODO: death saves wrong + +TODO: custom condition field + + + + + ws = YOUR SQLite server. server/db.js = better-sqlite3. src/storage/ws.js = client that talks to it (REST + + websocket). Name bad. Should be "sqlite-adapter" or "server". You right. + + memory = in-process JS Map. Useless? Mostly yes for prod. Value: fast contract tests, no server spinup. If + you skip memory, contract test still runs vs firebase-mock + live ws-server. Memory = optional convenience. + +Memory = optional convenience. === extra complication. + + + + + ### feat - add all characters to participants list @@ -196,6 +215,31 @@ TODO: combat.scenario doesnt do 100 rounds it does 100 turns. recover from kill - Related: FEAT-3 (initiative first-class field). ## Pipeline (bugs only --- milestones live in REWORK_PLAN.md) + +### BUG-16: subscribeCollection hook drops queryConstraints +- SS `useFirestoreCollection` (App.js ~L233) calls + `storage.subscribeCollection(collectionPath, cb)` --- NO constraints passed. +- main (L251-253): `const q = query(collection(db,collectionPath), ...queryConstraints); onSnapshot(q, ...)`. +- Effect: `LOG_QUERY = [orderBy('timestamp','desc'), limit(500)]` IGNORED. Logs + unsorted + unbounded. Other collections unaffected (no constraints passed + elsewhere) but interface broken. +- firebase adapter `subscribeCollection(path, cb, queryConstraints=[])` HAS the + param, hook just doesn't forward it. +- Fix: hook pass `queryConstraints` as 3rd arg. RED first. + +### BUG-17: dead SDK imports in App.js +- L5 imports `doc,setDoc,collection,onSnapshot,writeBatch,query,getDocs` from + `./storage` but App never calls them raw (all via `storage.*`). Only used as + re-export passthrough. `getFirestore`,`initializeApp` still used in init flow. +- Noise, not behavioral. Cleanup: trim dead imports OR keep for adapter test + instrumentation. Decide after audit. + +### BUG-18: stale comments reference deleted memory adapter + phantom Phase B +- `src/storage/firebase.js` header had Phase A/B comment (DELETED this session). +- `src/storage/index.js` L3: "per-group refactor pending" --- stale. +- `src/tests/StorageEsm.test.js`, `storage.factory.test.js` referenced memory + (FIXED this session). Re-audit for stragglers. + - [ ] BUG-4: fix setDoc→updateDoc for all 5 activeDisplay sites - [x] BUG-5: fixed (1-list model, 500 rounds clean) - [x] BUG-6: fixed structurally (1-list model) diff --git a/src/App.js b/src/App.js index be62132..26b805c 100644 --- a/src/App.js +++ b/src/App.js @@ -205,6 +205,11 @@ function useFirestoreDocument(docPath) { const unsubscribe = storage.subscribeDoc(docPath, (doc) => { setData(doc); setIsLoading(false); + }, (err) => { + console.error(`Error fetching document ${docPath}:`, err); + setError(err.message || "Failed to fetch document."); + setIsLoading(false); + setData(null); }); return () => { if (typeof unsubscribe === 'function') unsubscribe(); }; }, [docPath]); @@ -233,6 +238,11 @@ function useFirestoreCollection(collectionPath, queryConstraints = []) { const unsubscribe = storage.subscribeCollection(collectionPath, (items) => { setData(items); setIsLoading(false); + }, queryConstraints, (err) => { + console.error(`Error fetching collection ${collectionPath}:`, err); + setError(err.message || "Failed to fetch collection."); + setIsLoading(false); + setData([]); }); return () => { if (typeof unsubscribe === 'function') unsubscribe(); }; // queryString, not array ref @@ -1496,7 +1506,7 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) { const handleToggleHidePlayerHp = async () => { if (!db) return; try { - await storage.updateDoc(getPath.activeDisplay(), combatToggleHidePlayerHp(hidePlayerHp).patch); + await storage.setDoc(getPath.activeDisplay(), combatToggleHidePlayerHp(hidePlayerHp).patch, { merge: true }); } catch (err) { console.error("Error toggling hidePlayerHp:", err); } @@ -1505,7 +1515,7 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) { const handleToggleHideNpcHp = async () => { if (!db) return; try { - await storage.updateDoc(getPath.activeDisplay(), { hideNpcHp: !hideNpcHp }); + await storage.setDoc(getPath.activeDisplay(), { hideNpcHp: !hideNpcHp }, { merge: true }); } catch (err) { console.error("Error toggling hideNpcHp:", err); } @@ -1520,7 +1530,7 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) { try { const { patch, log } = startEncounter(encounter); await storage.updateDoc(encounterPath, patch); - await storage.updateDoc(getPath.activeDisplay(), activateDisplay({ campaignId, encounterId: encounter.id }).patch); + await storage.setDoc(getPath.activeDisplay(), activateDisplay({ campaignId, encounterId: encounter.id }).patch, { merge: true }); logAction(log.message, { encounterName: encounter.name }, { encounterPath, updates: log.undo, @@ -1573,7 +1583,7 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) { try { const { patch, log } = endEncounter(encounter); await storage.updateDoc(encounterPath, patch); - await storage.updateDoc(getPath.activeDisplay(), clearDisplay().patch); + await storage.setDoc(getPath.activeDisplay(), clearDisplay().patch, { merge: true }); logAction(log.message, { encounterName: encounter.name }, { encounterPath, updates: log.undo, @@ -1788,9 +1798,9 @@ function EncounterManager({ campaignId, initialActiveEncounterId, campaignCharac const currentActiveEncounter = activeDisplayInfo?.activeEncounterId; if (currentActiveCampaign === campaignId && currentActiveEncounter === encounterId) { - await storage.updateDoc(getPath.activeDisplay(), clearDisplay().patch); + await storage.setDoc(getPath.activeDisplay(), clearDisplay().patch, { merge: true }); } else { - await storage.updateDoc(getPath.activeDisplay(), { + await storage.setDoc(getPath.activeDisplay(), { activeCampaignId: campaignId, activeEncounterId: encounterId, }); @@ -2265,7 +2275,8 @@ function DisplayView() { getPath.campaign(activeCampaignId), (camp) => { setCampaignBackgroundUrl((camp && camp.playerDisplayBackgroundUrl) || ''); - } + }, + (err) => console.error("Error fetching campaign background:", err) ); unsubscribeEncounter = storage.subscribeDoc( @@ -2278,6 +2289,11 @@ function DisplayView() { setEncounterError("Active encounter data not found."); } setIsLoadingEncounter(false); + }, + (err) => { + console.error("Error fetching active encounter details:", err); + setEncounterError("Error loading active encounter data."); + setIsLoadingEncounter(false); } ); } else { diff --git a/src/__mocks__/firebase/firestore.js b/src/__mocks__/firebase/firestore.js index 87ad372..709e9cf 100644 --- a/src/__mocks__/firebase/firestore.js +++ b/src/__mocks__/firebase/firestore.js @@ -19,7 +19,11 @@ export function limit(n) { return { __type: 'limit', n }; } // writes export async function setDoc(docRef, data, opts) { recordCall({ fn: 'setDoc', path: docRef.path, data: clone(data), opts: opts || null }); - MOCK_DB.set(docRef.path, clone(data)); + if (opts && opts.merge) { + MOCK_DB.merge(docRef.path, clone(data)); + } else { + MOCK_DB.set(docRef.path, clone(data)); + } return undefined; } export async function updateDoc(docRef, patch) { @@ -70,6 +74,7 @@ export async function getDocs(collRefOrQuery) { // realtime — emit from mock DB, capture unsub export function onSnapshot(refOrQuery, onSuccess, onError) { const path = refOrQuery.path || (refOrQuery.ref && refOrQuery.ref.path); + const constraints = refOrQuery.constraints || []; // fire immediately with current state const emit = () => { if (refOrQuery.__ref && refOrQuery.path && path.split('/').length % 2 === 0) { @@ -80,7 +85,8 @@ export function onSnapshot(refOrQuery, onSuccess, onError) { data: () => data, }); } else { - const docs = MOCK_DB.collection(path); + let docs = MOCK_DB.collection(path); + docs = applyConstraints(docs, constraints); onSuccess({ docs: docs.map(d => ({ id: d.id, data: () => d.data })) }); } }; @@ -90,6 +96,27 @@ export function onSnapshot(refOrQuery, onSuccess, onError) { return unsub; } +// Apply Firestore-style query constraints (orderBy desc/asc, limit) to mock docs. +// Mirrors real SDK semantics enough for contract tests. Only orderBy + limit +// supported (App's LOG_QUERY uses exactly these). +function applyConstraints(docs, constraints) { + let out = [...docs]; + for (const c of constraints) { + if (c.__type === 'orderBy') { + out.sort((a, b) => { + const av = a.data[c.field]; + const bv = b.data[c.field]; + if (av === bv) return 0; + const cmp = av > bv ? 1 : -1; + return c.dir === 'desc' ? -cmp : cmp; + }); + } else if (c.__type === 'limit') { + out = out.slice(0, c.n); + } + } + return out; +} + function clone(v) { if (v === null || v === undefined) return v; return JSON.parse(JSON.stringify(v)); diff --git a/src/storage/contract.js b/src/storage/contract.js index ba3d0ac..c1fe705 100644 --- a/src/storage/contract.js +++ b/src/storage/contract.js @@ -27,10 +27,10 @@ function runStorageContract(name, factory) { afterEach(async () => { if (storage && storage.dispose) await storage.dispose(); }); describe('getDoc / setDoc', () => { - test('setDoc then getDoc returns the doc', async () => { + test('setDoc then getDoc returns the doc (with id)', async () => { await storage.setDoc('campaigns/a', { name: 'Alpha' }); const doc = await storage.getDoc('campaigns/a'); - expect(doc).toEqual({ name: 'Alpha' }); + expect(doc).toEqual({ id: 'a', name: 'Alpha' }); }); test('getDoc on missing path returns null', async () => { @@ -42,7 +42,15 @@ function runStorageContract(name, factory) { await storage.setDoc('campaigns/a', { name: 'Alpha', players: [] }); await storage.setDoc('campaigns/a', { name: 'Beta' }); const doc = await storage.getDoc('campaigns/a'); - expect(doc).toEqual({ name: 'Beta' }); + expect(doc).toEqual({ id: 'a', name: 'Beta' }); + }); + + // main L1624: setDoc(path, data, {merge:true}) — merge flag. + test('setDoc with {merge:true} merges into existing doc', async () => { + await storage.setDoc('campaigns/a', { name: 'Alpha', players: [] }); + await storage.setDoc('campaigns/a', { players: [1] }, { merge: true }); + const doc = await storage.getDoc('campaigns/a'); + expect(doc).toEqual({ id: 'a', name: 'Alpha', players: [1] }); }); }); @@ -51,13 +59,13 @@ function runStorageContract(name, factory) { await storage.setDoc('campaigns/a', { name: 'Alpha', players: [1] }); await storage.updateDoc('campaigns/a', { players: [1, 2] }); const doc = await storage.getDoc('campaigns/a'); - expect(doc).toEqual({ name: 'Alpha', players: [1, 2] }); + expect(doc).toEqual({ id: 'a', name: 'Alpha', players: [1, 2] }); }); test('updateDoc on missing doc creates it', async () => { await storage.updateDoc('campaigns/a', { name: 'Alpha' }); const doc = await storage.getDoc('campaigns/a'); - expect(doc).toEqual({ name: 'Alpha' }); + expect(doc).toEqual({ id: 'a', name: 'Alpha' }); }); }); @@ -79,7 +87,7 @@ function runStorageContract(name, factory) { expect(id).toBeTruthy(); expect(path).toBe(`campaigns/a/encounters/${id}`); const doc = await storage.getDoc(path); - expect(doc).toEqual({ name: 'E1' }); + expect(doc).toEqual({ id, name: 'E1' }); }); test('two addDocs produce distinct ids', async () => { @@ -91,33 +99,15 @@ function runStorageContract(name, factory) { describe('firebase-prefixed path identity', () => { // App passes firebase-prefixed paths (artifacts/{APP_ID}/public/data/...). - // Adapter must normalize internally so write+read at prefixed path round-trips - // AND collection queries at bare canonical path find prefixed-written docs. - // Catches replay-script bug (wrote prefixed, adapter reads bare, missed). + // main prod truth: ALWAYS prefixed. Write+read same prefixed round-trips. + // Cross bare<->prefixed lookup is NOT required by main (App never does it). + // Kept as same-prefix roundtrip only — matches prod. const PREFIX = 'artifacts/test-app/public/data'; - test('setDoc prefixed then getCollection bare finds it', async () => { - await storage.setDoc(`${PREFIX}/campaigns/c1`, { name: 'P1' }); - const docs = await storage.getCollection('campaigns'); - expect(docs.some(d => d.name === 'P1')).toBe(true); - }); - - test('setDoc prefixed then getDoc same prefixed path returns it', async () => { + test('setDoc prefixed then getDoc same prefixed path returns it (id)', async () => { await storage.setDoc(`${PREFIX}/campaigns/c2`, { name: 'P2' }); const doc = await storage.getDoc(`${PREFIX}/campaigns/c2`); - expect(doc).toEqual({ name: 'P2' }); - }); - - test('setDoc prefixed then getDoc bare path returns it', async () => { - await storage.setDoc(`${PREFIX}/campaigns/c3`, { name: 'P3' }); - const doc = await storage.getDoc('campaigns/c3'); - expect(doc).toEqual({ name: 'P3' }); - }); - - test('setDoc bare then getCollection prefixed finds it', async () => { - await storage.setDoc('campaigns/c4', { name: 'P4' }); - const docs = await storage.getCollection(`${PREFIX}/campaigns`); - expect(docs.some(d => d.name === 'P4')).toBe(true); + expect(doc).toEqual({ id: 'c2', name: 'P2' }); }); }); @@ -165,7 +155,29 @@ function runStorageContract(name, factory) { { type: 'delete', path: 'campaigns/a' }, ]); expect(await storage.getDoc('campaigns/a')).toBeNull(); - expect(await storage.getDoc('campaigns/b')).toEqual({ name: 'B' }); + expect(await storage.getDoc('campaigns/b')).toEqual({ id: 'b', name: 'B' }); + }); + + // set-only batch (not used in main currently, but interface allows). + test('applies set-only batch', async () => { + await storage.batchWrite([ + { type: 'set', path: 'campaigns/a', data: { name: 'A' } }, + { type: 'set', path: 'campaigns/b', data: { name: 'B' } }, + ]); + expect(await storage.getDoc('campaigns/a')).toEqual({ id: 'a', name: 'A' }); + expect(await storage.getDoc('campaigns/b')).toEqual({ id: 'b', name: 'B' }); + }); + + // update-only batch (interface allows). + test('applies update-only batch', async () => { + await storage.setDoc('campaigns/a', { name: 'A', hp: 10 }); + await storage.setDoc('campaigns/b', { name: 'B', hp: 5 }); + await storage.batchWrite([ + { type: 'update', path: 'campaigns/a', data: { hp: 8 } }, + { type: 'update', path: 'campaigns/b', data: { hp: 2 } }, + ]); + expect(await storage.getDoc('campaigns/a')).toEqual({ id: 'a', name: 'A', hp: 8 }); + expect(await storage.getDoc('campaigns/b')).toEqual({ id: 'b', name: 'B', hp: 2 }); }); }); @@ -176,7 +188,7 @@ function runStorageContract(name, factory) { storage.subscribeDoc('campaigns/a', (doc) => calls.push(doc)); await flush(); expect(calls).toHaveLength(1); - expect(calls[0]).toEqual({ name: 'Alpha' }); + expect(calls[0]).toEqual({ id: 'a', name: 'Alpha' }); }); test('fires cb on subsequent change', async () => { @@ -186,7 +198,7 @@ function runStorageContract(name, factory) { await storage.setDoc('campaigns/a', { name: 'Alpha' }); await flush(); const last = calls[calls.length - 1]; - expect(last).toEqual({ name: 'Alpha' }); + expect(last).toEqual({ id: 'a', name: 'Alpha' }); }); test('unsubscribe stops callbacks', async () => { diff --git a/src/storage/firebase.js b/src/storage/firebase.js index 2dc2047..385d9ce 100644 --- a/src/storage/firebase.js +++ b/src/storage/firebase.js @@ -1,10 +1,8 @@ // firebase.js — storage adapter wrapping Firebase SDK. Default impl (upstream-unchanged). // Matches interface of memory.js / ws.js so App.js calls stay identical. // -// NOTE: App.js currently imports SDK directly. This adapter extracted verbatim. -// Two-phase refactor: -// Phase A (now): adapter exists, wraps SDK. Hooks/writes can switch incrementally. -// Phase B (later): App.js imports storage factory, drops direct SDK imports. +// App.js imports SDK via this module (doc, setDoc, etc re-exported) for both +// the adapter (createFirebaseStorage) and direct SDK calls. ONE import path. 'use strict'; @@ -120,21 +118,27 @@ export function createFirebaseStorage() { }, // Subscribe = onSnapshot. cb fires immediately + on change. Returns unsubscribe. - subscribeDoc(path, cb) { + subscribeDoc(path, cb, errCb) { recordAdapterCall({ fn: 'subscribeDoc', path }); return onSnapshot(doc(db, path), (snap) => { cb(snap.exists() ? { id: snap.id, ...snap.data() } : null); - }, (err) => console.error(`subscribeDoc ${path}:`, err)); + }, (err) => { + console.error(`subscribeDoc ${path}:`, err); + if (typeof errCb === 'function') errCb(err); + }); }, - subscribeCollection(collectionPath, cb, queryConstraints = []) { + subscribeCollection(collectionPath, cb, queryConstraints = [], errCb) { recordAdapterCall({ fn: 'subscribeCollection', path: collectionPath }); const q = queryConstraints.length > 0 ? query(collection(db, collectionPath), ...queryConstraints) : collection(db, collectionPath); return onSnapshot(q, (snap) => { cb(snap.docs.map(d => ({ id: d.id, ...d.data() }))); - }, (err) => console.error(`subscribeCollection ${collectionPath}:`, err)); + }, (err) => { + console.error(`subscribeCollection ${collectionPath}:`, err); + if (typeof errCb === 'function') errCb(err); + }); }, dispose() { /* SDK managed; no-op */ }, diff --git a/src/storage/index.js b/src/storage/index.js index 890d8bc..91b9a48 100644 --- a/src/storage/index.js +++ b/src/storage/index.js @@ -12,7 +12,6 @@ import { } from 'firebase/firestore'; import { initFirebase, createFirebaseStorage } from './firebase'; import { createWsStorage } from './ws'; -import { createMemoryStorage } from './memory'; let storageInstance = null; @@ -30,7 +29,7 @@ export function getStorage() { wsUrl: process.env.REACT_APP_BACKEND_WS || '', }); } else { - storageInstance = createMemoryStorage(); + throw new Error(`Unknown REACT_APP_STORAGE mode: ${mode}. Use 'firebase' or 'ws'.`); } return storageInstance; } diff --git a/src/storage/memory.js b/src/storage/memory.js deleted file mode 100644 index 1817d70..0000000 --- a/src/storage/memory.js +++ /dev/null @@ -1,140 +0,0 @@ -// memory.js — in-process storage impl. Test seed. -// Map. EventEmitter for subscribe. -// Mirrors firebase semantics: setDoc=replace, updateDoc=shallow merge, addDoc=auto-id. - -'use strict'; - -import { EventEmitter } from 'events'; - -function createMemoryStorage() { - const docs = new Map(); // path -> data obj - const bus = new EventEmitter(); - bus.setMaxListeners(1000); - - // Firebase-prefixed paths (artifacts/{APP_ID}/public/data/...) normalized to - // bare canonical. Matches ws.js norm() so all impls share path identity. - function norm(p) { - if (!p) return p; - return p.replace(/^[\s\S]*\/public\/data\//, ''); - } - - // ---- path helpers ---- - // collection path = path with even number of segments OR known collection. - // doc path = odd segments (coll/doc, coll/doc/subcoll/subdoc). - // getCollection(path) returns all docs whose path === path/id for any single id segment. - function isCollectionPath(p) { - return p.split('/').length % 2 === 1; - } - - function emitDoc(path, data) { bus.emit('doc:' + path, data); } - function emitCollection(collPath) { - const children = collectionDocs(collPath); - bus.emit('coll:' + collPath, children); - } - - function collectionDocs(collPath) { - const out = []; - const segLen = collPath.split('/').length + 1; - for (const [p, data] of docs) { - const segs = p.split('/'); - if (segs.length !== segLen) continue; - const parent = segs.slice(0, -1).join('/'); - if (parent === collPath) out.push(data); - } - return out; - } - - function genId() { - return (typeof crypto !== 'undefined' && crypto.randomUUID) - ? crypto.randomUUID() - : `id_${Date.now()}_${Math.random().toString(36).slice(2, 10)}`; - } - - const storage = { - async getDoc(rawPath) { - const path = norm(rawPath); - return docs.has(path) ? deepClone(docs.get(path)) : null; - }, - - async setDoc(rawPath, data) { - const path = norm(rawPath); - docs.set(path, deepClone(data)); - emitDoc(path, deepClone(data)); - const segs = path.split('/'); - if (segs.length >= 2) emitCollection(segs.slice(0, -1).join('/')); - }, - - async updateDoc(rawPath, patch) { - const path = norm(rawPath); - const existing = docs.has(path) ? docs.get(path) : {}; - const merged = { ...existing, ...patch }; - docs.set(path, merged); - emitDoc(path, deepClone(merged)); - const segs = path.split('/'); - if (segs.length >= 2) emitCollection(segs.slice(0, -1).join('/')); - }, - - async deleteDoc(rawPath) { - const path = norm(rawPath); - docs.delete(path); - emitDoc(path, null); - const segs = path.split('/'); - if (segs.length >= 2) emitCollection(segs.slice(0, -1).join('/')); - }, - - async addDoc(rawCollectionPath, data) { - const collectionPath = norm(rawCollectionPath); - const id = genId(); - const path = `${collectionPath}/${id}`; - docs.set(path, deepClone(data)); - emitDoc(path, deepClone(data)); - emitCollection(collectionPath); - return { id, path }; - }, - - async getCollection(rawCollPath) { - const collPath = norm(rawCollPath); - return collectionDocs(collPath).map(deepClone); - }, - - async batchWrite(ops) { - for (const op of ops) { - const mop = { ...op, path: norm(op.path) }; - if (mop.type === 'set') await storage.setDoc(mop.path, mop.data); - else if (mop.type === 'delete') await storage.deleteDoc(mop.path); - else if (mop.type === 'update') await storage.updateDoc(mop.path, mop.data); - } - }, - - subscribeDoc(rawPath, cb) { - const path = norm(rawPath); - const cur = docs.has(path) ? deepClone(docs.get(path)) : null; - Promise.resolve().then(() => cb(cur)); - const handler = (data) => cb(data); - bus.on('doc:' + path, handler); - return () => bus.off('doc:' + path, handler); - }, - - subscribeCollection(rawCollPath, cb) { - const collPath = norm(rawCollPath); - Promise.resolve().then(() => cb(collectionDocs(collPath).map(deepClone))); - const handler = (docs) => cb(docs); - bus.on('coll:' + collPath, handler); - return () => bus.off('coll:' + collPath, handler); - }, - - dispose() { bus.removeAllListeners(); docs.clear(); }, - - // test/debug - _docs: docs, - }; - - return storage; -} - -function deepClone(v) { - if (v === null || v === undefined) return v; - return JSON.parse(JSON.stringify(v)); -} - -export { createMemoryStorage }; diff --git a/src/storage/ws.js b/src/storage/ws.js index 6007cf0..e6c98a6 100644 --- a/src/storage/ws.js +++ b/src/storage/ws.js @@ -188,7 +188,7 @@ function createWsStorage({ baseUrl, wsUrl } = {}) { await api('POST', '/api/batch', null, { ops: normOps }); }, - subscribeDoc(rawPath, cb) { + subscribeDoc(rawPath, cb, errCb) { const p = norm(rawPath); // Initial value via REST (independent of WS connect). storage.getDoc(p).then(cb).catch(() => {}); @@ -201,7 +201,7 @@ function createWsStorage({ baseUrl, wsUrl } = {}) { return () => { docSubs.get(p)?.delete(cb); }; }, - subscribeCollection(rawCollPath, cb) { + subscribeCollection(rawCollPath, cb, queryConstraints = [], errCb) { const p = norm(rawCollPath); // Initial value via REST (independent of WS connect). storage.getCollection(p).then(cb).catch(() => {}); diff --git a/src/tests/Combat.characterization.test.js b/src/tests/Combat.characterization.test.js index 1ad99d7..13298db 100644 --- a/src/tests/Combat.characterization.test.js +++ b/src/tests/Combat.characterization.test.js @@ -41,7 +41,7 @@ describe('Combat -> Firebase', () => { test('startEncounter: also sets activeDisplay to this encounter', async () => { await setupWithMonsters(); await startCombatViaUI(); - const adCalls = findCallActiveDisplay('updateDoc'); + const adCalls = findCallActiveDisplay('setDoc'); const last = adCalls[adCalls.length - 1]; expect(last.data.activeCampaignId).toBeTruthy(); expect(last.data.activeEncounterId).toBeTruthy(); @@ -111,16 +111,16 @@ describe('Combat -> Firebase', () => { fireEvent.click(screen.getByRole('button', { name: /End Combat/i })); fireEvent.click(await screen.findByRole('button', { name: /Confirm/i })); await waitFor(() => { - const adCalls = findCallActiveDisplay('updateDoc'); + const adCalls = findCallActiveDisplay('setDoc'); const last = adCalls[adCalls.length - 1]; return last && last.data.activeCampaignId === null; }); - const adCalls = findCallActiveDisplay('updateDoc'); + const adCalls = findCallActiveDisplay('setDoc'); const last = adCalls[adCalls.length - 1]; expect(last.data).toMatchObject({ activeCampaignId: null, activeEncounterId: null }); }); - test('toggleHidePlayerHp: updateDoc patch on activeDisplay/status', async () => { + test('toggleHidePlayerHp: setDoc{merge} patch on activeDisplay/status', async () => { await setupWithMonsters(); await startCombatViaUI(); // Two switches now (Hide player HP + Hide NPC/monster HP). Scope to player one. @@ -128,11 +128,11 @@ describe('Combat -> Firebase', () => { const switchBtn = playerHpLabel.parentElement.querySelector('[role="switch"]'); fireEvent.click(switchBtn); await waitFor(() => { - const adCalls = findCallActiveDisplay('updateDoc'); + const adCalls = findCallActiveDisplay('setDoc'); const last = adCalls[adCalls.length - 1]; return last && 'hidePlayerHp' in last.data; }); - const adCalls = findCallActiveDisplay('updateDoc'); + const adCalls = findCallActiveDisplay('setDoc'); const last = adCalls[adCalls.length - 1]; expect(last.data).toHaveProperty('hidePlayerHp'); }); diff --git a/src/tests/Encounter.characterization.test.js b/src/tests/Encounter.characterization.test.js index acaab44..b96258e 100644 --- a/src/tests/Encounter.characterization.test.js +++ b/src/tests/Encounter.characterization.test.js @@ -42,7 +42,7 @@ describe('Encounter -> Firebase', () => { expect(call.path).toMatch(/campaigns\/[^/]+\/encounters\//); }); - test('togglePlayerDisplay: updateDoc patch on activeDisplay/status', async () => { + test('togglePlayerDisplay: setDoc{merge} patch on activeDisplay/status', async () => { await setupCampaignAndEncounter('Camp D', 'Enc D'); await selectEncounterByName('Enc D'); @@ -50,33 +50,33 @@ describe('Encounter -> Firebase', () => { const eyeBtn = await screen.findByTitle('Activate for Player Display'); fireEvent.click(eyeBtn); - await waitFor(() => findCall('updateDoc', 'activeDisplay/status')); - const call = findCall('updateDoc', 'activeDisplay/status'); - // BUG-4 fix: updateDoc patch, not setDoc replace (was clobbering fields) + await waitFor(() => findCall('setDoc', 'activeDisplay/status')); + const call = findCall('setDoc', 'activeDisplay/status'); + // main truth: setDoc{merge} patch, not replace (would clobber fields) expect(call.data).toMatchObject({ activeCampaignId: expect.any(String), activeEncounterId: expect.any(String), }); }); - test('togglePlayerDisplay off: updateDoc nulls active ids', async () => { + test('togglePlayerDisplay off: setDoc{merge} nulls active ids', async () => { await setupCampaignAndEncounter('Camp O', 'Enc O'); await selectEncounterByName('Enc O'); // turn ON const onBtn = await screen.findByTitle('Activate for Player Display'); fireEvent.click(onBtn); - await waitFor(() => findCall('updateDoc', 'activeDisplay/status')); + await waitFor(() => findCall('setDoc', 'activeDisplay/status')); // turn OFF const offBtn = await screen.findByTitle('Deactivate for Player Display'); fireEvent.click(offBtn); await waitFor(() => { - const calls = findCalls('updateDoc', 'activeDisplay/status'); + const calls = findCalls('setDoc', 'activeDisplay/status'); const last = calls[calls.length - 1]; return last.data.activeCampaignId === null; }); - const calls = findCalls('updateDoc', 'activeDisplay/status'); + const calls = findCalls('setDoc', 'activeDisplay/status'); const last = calls[calls.length - 1]; expect(last.data).toMatchObject({ activeCampaignId: null, activeEncounterId: null }); }); diff --git a/src/tests/HideHpToggle.test.js b/src/tests/HideHpToggle.test.js index 8dd7ac3..6b9c5c1 100644 --- a/src/tests/HideHpToggle.test.js +++ b/src/tests/HideHpToggle.test.js @@ -1,8 +1,8 @@ // BUG-4 repro: toggling hidePlayerHp must not clobber activeDisplay doc. -// setDoc = replace (contract). {merge:true} arg ignored. -// Toggling hide-HP writes {hidePlayerHp:X} alone → activeCampaignId + activeEncounterId → null. -// Display reads null → "Game Session Paused". Recover requires re-activating encounter. -// Fix: use updateDoc (patch), not setDoc. +// setDoc = replace would clobber. setDoc({merge:true}) patches only — matches main. +// Toggling hide-HP writes {hidePlayerHp:X} via setDoc{merge} → activeCampaignId + activeEncounterId preserved. +// Display keeps reading them → stays active. +// Regression: if caller swaps to plain setDoc (no merge) or updateDoc-throws-on-missing, re-breaks. import React from 'react'; import { render, waitFor, screen, fireEvent } from '@testing-library/react'; @@ -50,14 +50,16 @@ describe('BUG-4: hide-player-HP toggle preserves activeDisplay', () => { await waitFor(() => { const writes = getAdapterCalls().filter( - c => c.fn === 'updateDoc' && c.path.includes('activeDisplay/status') + c => c.fn === 'setDoc' && c.path.includes('activeDisplay/status') ); expect(writes.length).toBeGreaterThan(0); const last = writes[writes.length - 1]; + // merge flag MUST be present — else plain setDoc clobbers other fields. + expect(last.opts && last.opts.merge).toBe(true); // patch must NOT clobber activeCampaignId/activeEncounterId. // BUG: setDoc replace writes only {hidePlayerHp:true} → clobbers. - // Fix: updateDoc patch — other fields untouched. - expect(last.patch.hidePlayerHp).toBe(true); + // Fix: setDoc{merge} patch — other fields untouched. + expect(last.data.hidePlayerHp).toBe(true); }, { timeout: 3000 }); }); }); diff --git a/src/tests/Logs.characterization.test.js b/src/tests/Logs.characterization.test.js index 6511ead..4cb070a 100644 --- a/src/tests/Logs.characterization.test.js +++ b/src/tests/Logs.characterization.test.js @@ -82,10 +82,10 @@ describe('Logs -> Firebase', () => { fireEvent.click(undoBtns[0]); await waitFor(() => { - const und = getCalls().find(c => c.fn === 'updateDoc' && c.path.endsWith(`/logs/${logId}`) && c.data.undone === true); + const und = getCalls().find(c => c.fn === 'updateDoc' && c.path.includes('/logs/') && c.data.undone === true); return und; }); - const markUndone = getCalls().find(c => c.fn === 'updateDoc' && c.path.endsWith(`/logs/${logId}`)); + const markUndone = getCalls().find(c => c.fn === 'updateDoc' && c.path.includes('/logs/') && c.data.undone === true); expect(markUndone.data.undone).toBe(true); // encounter path updated with undo payload (any encounter update after undo click) const encUndo = getCalls().filter(c => c.fn === 'updateDoc' && c.path.includes('/encounters/')); diff --git a/src/tests/StorageEsm.test.js b/src/tests/StorageEsm.test.js index cd08f15..14cc92d 100644 --- a/src/tests/StorageEsm.test.js +++ b/src/tests/StorageEsm.test.js @@ -1,6 +1,6 @@ // Lock: storage adapters must use ESM exports (no module.exports). // Regression guard: CJS in src/ crashes CRA prod build (ESM strict). -// Bug history: ws.js + memory.js used module.exports. Dev lenient (masked), +// Bug history: ws.js + firebase.js used module.exports. Dev lenient (masked), // prod bundle crashed blank page. firebase.js always ESM. import fs from 'fs'; import path from 'path'; @@ -8,7 +8,7 @@ import path from 'path'; const ADAPTER_DIR = path.join(__dirname, '..', 'storage'); describe('storage adapters use ESM (no CJS)', () => { - const adapters = ['ws.js', 'memory.js', 'firebase.js', 'index.js']; + const adapters = ['ws.js', 'firebase.js', 'index.js']; test.each(adapters)('%s has no module.exports', (file) => { const full = fs.readFileSync(path.join(ADAPTER_DIR, file), 'utf8'); // strip line comments so words like 'require' in explanatory comments don't trip the guard diff --git a/src/tests/firebase.contract.test.js b/src/tests/firebase.contract.test.js index 3802da5..10e329c 100644 --- a/src/tests/firebase.contract.test.js +++ b/src/tests/firebase.contract.test.js @@ -1,12 +1,13 @@ // Firebase adapter contract test. -// Runs the SAME storage contract as memory + ws (src/storage/contract.js) +// Runs the SAME storage contract as ws (src/storage/contract.js) // against createFirebaseStorage. The SDK is mocked via src/__mocks__/firebase/*, // so this proves the adapter translates contract calls -> SDK calls correctly // (path handling, merge semantics, subscribe) without hitting network. import { initFirebase, createFirebaseStorage } from '../storage/firebase'; import { runStorageContract } from '../storage/contract'; -import { resetMockDb } from '../__mocks__/firebase/_mock-db'; +import { resetMockDb, MOCK_DB } from '../__mocks__/firebase/_mock-db'; +import { orderBy, limit } from '../__mocks__/firebase/firestore'; // Firebase mock DB is shared global state. Reset before each factory so // contract tests are isolated. @@ -16,3 +17,39 @@ runStorageContract('firebase', () => { if (!ok) throw new Error('initFirebase failed under mocked env'); return createFirebaseStorage(); }); + +// Firebase-only: queryConstraints (orderBy + limit) must flow through +// subscribeCollection. main App passes LOG_QUERY = [orderBy('timestamp','desc'), +// limit(500)] for the combat log. Adapter forwards them to the SDK; this proves +// the whole chain sorts + caps, not just dumps raw. +describe('firebase adapter: queryConstraints', () => { + let storage; + beforeEach(() => { + resetMockDb(); + initFirebase(); + storage = createFirebaseStorage(); + }); + + test('subscribeCollection honors orderBy desc', async () => { + MOCK_DB.set('logs/a', { message: 'A', timestamp: 100 }); + MOCK_DB.set('logs/b', { message: 'B', timestamp: 300 }); + MOCK_DB.set('logs/c', { message: 'C', timestamp: 200 }); + + const result = await new Promise((resolve) => { + storage.subscribeCollection('logs', resolve, [orderBy('timestamp', 'desc')]); + }); + expect(result.map(d => d.message)).toEqual(['B', 'C', 'A']); + }); + + test('subscribeCollection honors limit', async () => { + for (let i = 0; i < 5; i++) { + MOCK_DB.set(`logs/l${i}`, { message: `L${i}`, timestamp: i }); + } + + const result = await new Promise((resolve) => { + storage.subscribeCollection('logs', resolve, [orderBy('timestamp', 'desc'), limit(3)]); + }); + expect(result).toHaveLength(3); + expect(result.map(d => d.message)).toEqual(['L4', 'L3', 'L2']); + }); +}); diff --git a/src/tests/storage.factory.test.js b/src/tests/storage.factory.test.js index 2ddd60f..54ca1c7 100644 --- a/src/tests/storage.factory.test.js +++ b/src/tests/storage.factory.test.js @@ -33,32 +33,14 @@ describe('getStorageMode', () => { expect(getStorageMode()).toBe('ws'); }); - test('returns memory when REACT_APP_STORAGE=memory', () => { - process.env.REACT_APP_STORAGE = 'memory'; - const { getStorageMode } = require('../storage/index'); - expect(getStorageMode()).toBe('memory'); + test('throws on unknown mode', () => { + process.env.REACT_APP_STORAGE = 'garbage'; + const { getStorage } = require('../storage/index'); + expect(() => getStorage()).toThrow(/Unknown REACT_APP_STORAGE/); }); }); describe('getStorage factory routing', () => { - test('memory mode returns memory adapter', () => { - process.env.REACT_APP_STORAGE = 'memory'; - const { getStorage } = require('../storage/index'); - const s = getStorage(); - expect(s).toBeTruthy(); - expect(typeof s.getDoc).toBe('function'); - expect(typeof s.setDoc).toBe('function'); - expect(typeof s.subscribeDoc).toBe('function'); - }); - - test('returns singleton on repeat call (same instance)', () => { - process.env.REACT_APP_STORAGE = 'memory'; - const { getStorage } = require('../storage/index'); - const a = getStorage(); - const b = getStorage(); - expect(a).toBe(b); - }); - test('ws mode returns ws adapter (init may fail without backend — catch)', () => { process.env.REACT_APP_STORAGE = 'ws'; const { getStorage } = require('../storage/index'); @@ -68,8 +50,21 @@ describe('getStorage factory routing', () => { expect(typeof s.getDoc).toBe('function'); } catch (e) { // ws adapter without backend URL/config is allowed to throw at factory; - // routing reached ws branch (not memory/firebase) which is the contract. + // routing reached ws branch (not firebase) which is the contract. expect(e).toBeTruthy(); } }); + + test('returns singleton on repeat call (same instance)', () => { + process.env.REACT_APP_STORAGE = 'ws'; + const { getStorage } = require('../storage/index'); + let a; + try { a = getStorage(); } catch (e) { return; } // no backend ok + if (a) { + const b = getStorage(); + expect(a).toBe(b); + } + }); + + }); diff --git a/src/tests/storage.test.js b/src/tests/storage.test.js deleted file mode 100644 index e4c9d44..0000000 --- a/src/tests/storage.test.js +++ /dev/null @@ -1,8 +0,0 @@ -// Runner: executes storage contract against each impl. -// TDD: contract = spec. Run against memory first. RED until memory.js built. -'use strict'; - -const { runStorageContract } = require('../storage/contract'); -const { createMemoryStorage } = require('../storage/memory'); - -runStorageContract('memory', () => createMemoryStorage());