fix(BUG-4): updateDoc patch on activeDisplay (not setDoc replace)
All 5 storage.setDoc(activeDisplay, {...}, {merge:true}) →
storage.updateDoc(activeDisplay, {...}).
setDoc merge:true worked in prod (firebase honors merge) but ws adapter
+ mock ignore opts arg entirely → clobbers doc. updateDoc uses PATCH
across all adapters (firebase real updateDoc, ws PATCH endpoint, mock
merge). Consistent, no clobber.
Sites fixed:
- hidePlayerHp toggle
- startEncounter (set active ids)
- endEncounter (null active ids)
- deactivate active display
- activate new display
TDD: HideHpToggle.test RED first (assert updateDoc patch, impl still
setDoc → 0 calls found). GREEN after switch.
Char tests updated: Encounter.characterization (2) + Combat.characterization
(2) assert updateDoc on activeDisplay, not setDoc.
BUG-4: prod was already fixed (merge:true), test was RED due to mock
ignoring opts. Now all 3 adapters consistent via updateDoc.
This commit is contained in:
@@ -90,7 +90,7 @@ REWORK_PLAN.md.
|
|||||||
|
|
||||||
### bug-3 was a halucination has been removed
|
### bug-3 was a halucination has been removed
|
||||||
|
|
||||||
### BUG-4: hide-player-HP breaks display view (preexisting)
|
### BUG-4: hide-player-HP breaks display view (preexisting) --- PROD FIXED, TEST RED (mock bug)
|
||||||
- **Broader than hide-HP**: ALL 5 `storage.setDoc(getPath.activeDisplay(), ...)` calls
|
- **Broader than hide-HP**: ALL 5 `storage.setDoc(getPath.activeDisplay(), ...)` calls
|
||||||
use `{merge:true}` which is IGNORED (setDoc = replace per contract).
|
use `{merge:true}` which is IGNORED (setDoc = replace per contract).
|
||||||
Each write clobbers other fields on activeDisplay/status doc.
|
Each write clobbers other fields on activeDisplay/status doc.
|
||||||
@@ -108,6 +108,13 @@ REWORK_PLAN.md.
|
|||||||
activeEncounterId with null (setDoc replace vs updateDoc patch).
|
activeEncounterId with null (setDoc replace vs updateDoc patch).
|
||||||
- Fix: use updateDoc (patch) not setDoc (replace); or include all existing
|
- Fix: use updateDoc (patch) not setDoc (replace); or include all existing
|
||||||
fields when writing.
|
fields when writing.
|
||||||
|
- Status update (2026-07): all 5 sites now use `{merge:true}`. Real firebase
|
||||||
|
adapter honors merge → production works. BUT jsdom test still RED because
|
||||||
|
`src/__mocks__/firebase/firestore.js` setDoc records call, IGNORES opts
|
||||||
|
(no actual merge). Mock must simulate firebase merge semantics for test
|
||||||
|
to pass. Fix = mock setDoc: if opts.merge, MOCK_DB.merge(path,data) else
|
||||||
|
replace. OR change App.js setDoc(merge) → updateDoc (cleaner, ws adapter
|
||||||
|
uses PATCH). Decide which.
|
||||||
- Test: render App + DisplayView, toggle hide-HP, assert display still shows
|
- Test: render App + DisplayView, toggle hide-HP, assert display still shows
|
||||||
encounter (not paused).
|
encounter (not paused).
|
||||||
|
|
||||||
|
|||||||
+9
-9
@@ -1662,7 +1662,7 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) {
|
|||||||
const handleToggleHidePlayerHp = async () => {
|
const handleToggleHidePlayerHp = async () => {
|
||||||
if (!db) return;
|
if (!db) return;
|
||||||
try {
|
try {
|
||||||
await storage.setDoc(getPath.activeDisplay(), { hidePlayerHp: !hidePlayerHp }, { merge: true });
|
await storage.updateDoc(getPath.activeDisplay(), { hidePlayerHp: !hidePlayerHp });
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
console.error("Error toggling hidePlayerHp:", err);
|
console.error("Error toggling hidePlayerHp:", err);
|
||||||
}
|
}
|
||||||
@@ -1695,10 +1695,10 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) {
|
|||||||
turnOrderIds: sortedParticipants.map(p => p.id)
|
turnOrderIds: sortedParticipants.map(p => p.id)
|
||||||
});
|
});
|
||||||
|
|
||||||
await storage.setDoc(getPath.activeDisplay(), {
|
await storage.updateDoc(getPath.activeDisplay(), {
|
||||||
activeCampaignId: campaignId,
|
activeCampaignId: campaignId,
|
||||||
activeEncounterId: encounter.id
|
activeEncounterId: encounter.id
|
||||||
}, { merge: true });
|
});
|
||||||
|
|
||||||
logAction(`Combat started: "${encounter.name}" — ${sortedParticipants[0].name}'s turn (Round 1)`, { encounterName: encounter.name }, {
|
logAction(`Combat started: "${encounter.name}" — ${sortedParticipants[0].name}'s turn (Round 1)`, { encounterName: encounter.name }, {
|
||||||
encounterPath,
|
encounterPath,
|
||||||
@@ -1826,10 +1826,10 @@ function InitiativeControls({ campaignId, encounter, encounterPath }) {
|
|||||||
turnOrderIds: []
|
turnOrderIds: []
|
||||||
});
|
});
|
||||||
|
|
||||||
await storage.setDoc(getPath.activeDisplay(), {
|
await storage.updateDoc(getPath.activeDisplay(), {
|
||||||
activeCampaignId: null,
|
activeCampaignId: null,
|
||||||
activeEncounterId: null
|
activeEncounterId: null
|
||||||
}, { merge: true });
|
});
|
||||||
|
|
||||||
logAction(`Combat ended: "${encounter.name}"`, { encounterName: encounter.name }, {
|
logAction(`Combat ended: "${encounter.name}"`, { encounterName: encounter.name }, {
|
||||||
encounterPath,
|
encounterPath,
|
||||||
@@ -2044,15 +2044,15 @@ function EncounterManager({ campaignId, initialActiveEncounterId, campaignCharac
|
|||||||
const currentActiveEncounter = activeDisplayInfo?.activeEncounterId;
|
const currentActiveEncounter = activeDisplayInfo?.activeEncounterId;
|
||||||
|
|
||||||
if (currentActiveCampaign === campaignId && currentActiveEncounter === encounterId) {
|
if (currentActiveCampaign === campaignId && currentActiveEncounter === encounterId) {
|
||||||
await storage.setDoc(getPath.activeDisplay(), {
|
await storage.updateDoc(getPath.activeDisplay(), {
|
||||||
activeCampaignId: null,
|
activeCampaignId: null,
|
||||||
activeEncounterId: null,
|
activeEncounterId: null,
|
||||||
}, { merge: true });
|
});
|
||||||
} else {
|
} else {
|
||||||
await storage.setDoc(getPath.activeDisplay(), {
|
await storage.updateDoc(getPath.activeDisplay(), {
|
||||||
activeCampaignId: campaignId,
|
activeCampaignId: campaignId,
|
||||||
activeEncounterId: encounterId,
|
activeEncounterId: encounterId,
|
||||||
}, { merge: true });
|
});
|
||||||
}
|
}
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
console.error("Error toggling Player Display:", err);
|
console.error("Error toggling Player Display:", err);
|
||||||
|
|||||||
@@ -41,7 +41,7 @@ describe('Combat -> Firebase', () => {
|
|||||||
test('startEncounter: also sets activeDisplay to this encounter', async () => {
|
test('startEncounter: also sets activeDisplay to this encounter', async () => {
|
||||||
await setupWithMonsters();
|
await setupWithMonsters();
|
||||||
await startCombatViaUI();
|
await startCombatViaUI();
|
||||||
const adCalls = findCallActiveDisplay('setDoc');
|
const adCalls = findCallActiveDisplay('updateDoc');
|
||||||
const last = adCalls[adCalls.length - 1];
|
const last = adCalls[adCalls.length - 1];
|
||||||
expect(last.data.activeCampaignId).toBeTruthy();
|
expect(last.data.activeCampaignId).toBeTruthy();
|
||||||
expect(last.data.activeEncounterId).toBeTruthy();
|
expect(last.data.activeEncounterId).toBeTruthy();
|
||||||
@@ -111,26 +111,26 @@ describe('Combat -> Firebase', () => {
|
|||||||
fireEvent.click(screen.getByRole('button', { name: /End Combat/i }));
|
fireEvent.click(screen.getByRole('button', { name: /End Combat/i }));
|
||||||
fireEvent.click(await screen.findByRole('button', { name: /Confirm/i }));
|
fireEvent.click(await screen.findByRole('button', { name: /Confirm/i }));
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
const adCalls = findCallActiveDisplay('setDoc');
|
const adCalls = findCallActiveDisplay('updateDoc');
|
||||||
const last = adCalls[adCalls.length - 1];
|
const last = adCalls[adCalls.length - 1];
|
||||||
return last && last.data.activeCampaignId === null;
|
return last && last.data.activeCampaignId === null;
|
||||||
});
|
});
|
||||||
const adCalls = findCallActiveDisplay('setDoc');
|
const adCalls = findCallActiveDisplay('updateDoc');
|
||||||
const last = adCalls[adCalls.length - 1];
|
const last = adCalls[adCalls.length - 1];
|
||||||
expect(last.data).toMatchObject({ activeCampaignId: null, activeEncounterId: null });
|
expect(last.data).toMatchObject({ activeCampaignId: null, activeEncounterId: null });
|
||||||
});
|
});
|
||||||
|
|
||||||
test('toggleHidePlayerHp: setDoc merge on activeDisplay/status', async () => {
|
test('toggleHidePlayerHp: updateDoc patch on activeDisplay/status', async () => {
|
||||||
await setupWithMonsters();
|
await setupWithMonsters();
|
||||||
await startCombatViaUI();
|
await startCombatViaUI();
|
||||||
const switchBtn = screen.getByRole('switch');
|
const switchBtn = screen.getByRole('switch');
|
||||||
fireEvent.click(switchBtn);
|
fireEvent.click(switchBtn);
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
const adCalls = findCallActiveDisplay('setDoc');
|
const adCalls = findCallActiveDisplay('updateDoc');
|
||||||
const last = adCalls[adCalls.length - 1];
|
const last = adCalls[adCalls.length - 1];
|
||||||
return last && 'hidePlayerHp' in last.data;
|
return last && 'hidePlayerHp' in last.data;
|
||||||
});
|
});
|
||||||
const adCalls = findCallActiveDisplay('setDoc');
|
const adCalls = findCallActiveDisplay('updateDoc');
|
||||||
const last = adCalls[adCalls.length - 1];
|
const last = adCalls[adCalls.length - 1];
|
||||||
expect(last.data).toHaveProperty('hidePlayerHp');
|
expect(last.data).toHaveProperty('hidePlayerHp');
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -42,7 +42,7 @@ describe('Encounter -> Firebase', () => {
|
|||||||
expect(call.path).toMatch(/campaigns\/[^/]+\/encounters\//);
|
expect(call.path).toMatch(/campaigns\/[^/]+\/encounters\//);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('togglePlayerDisplay: setDoc merge on activeDisplay/status', async () => {
|
test('togglePlayerDisplay: updateDoc patch on activeDisplay/status', async () => {
|
||||||
await setupCampaignAndEncounter('Camp D', 'Enc D');
|
await setupCampaignAndEncounter('Camp D', 'Enc D');
|
||||||
await selectEncounterByName('Enc D');
|
await selectEncounterByName('Enc D');
|
||||||
|
|
||||||
@@ -50,33 +50,33 @@ describe('Encounter -> Firebase', () => {
|
|||||||
const eyeBtn = await screen.findByTitle('Activate for Player Display');
|
const eyeBtn = await screen.findByTitle('Activate for Player Display');
|
||||||
fireEvent.click(eyeBtn);
|
fireEvent.click(eyeBtn);
|
||||||
|
|
||||||
await waitFor(() => findCall('setDoc', 'activeDisplay/status'));
|
await waitFor(() => findCall('updateDoc', 'activeDisplay/status'));
|
||||||
const call = findCall('setDoc', 'activeDisplay/status');
|
const call = findCall('updateDoc', 'activeDisplay/status');
|
||||||
// activeDisplay/status setDoc is called with merge option in App
|
// BUG-4 fix: updateDoc patch, not setDoc replace (was clobbering fields)
|
||||||
expect(call.data).toMatchObject({
|
expect(call.data).toMatchObject({
|
||||||
activeCampaignId: expect.any(String),
|
activeCampaignId: expect.any(String),
|
||||||
activeEncounterId: expect.any(String),
|
activeEncounterId: expect.any(String),
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
test('togglePlayerDisplay off: setDoc nulls active ids', async () => {
|
test('togglePlayerDisplay off: updateDoc nulls active ids', async () => {
|
||||||
await setupCampaignAndEncounter('Camp O', 'Enc O');
|
await setupCampaignAndEncounter('Camp O', 'Enc O');
|
||||||
await selectEncounterByName('Enc O');
|
await selectEncounterByName('Enc O');
|
||||||
|
|
||||||
// turn ON
|
// turn ON
|
||||||
const onBtn = await screen.findByTitle('Activate for Player Display');
|
const onBtn = await screen.findByTitle('Activate for Player Display');
|
||||||
fireEvent.click(onBtn);
|
fireEvent.click(onBtn);
|
||||||
await waitFor(() => findCall('setDoc', 'activeDisplay/status'));
|
await waitFor(() => findCall('updateDoc', 'activeDisplay/status'));
|
||||||
|
|
||||||
// turn OFF
|
// turn OFF
|
||||||
const offBtn = await screen.findByTitle('Deactivate for Player Display');
|
const offBtn = await screen.findByTitle('Deactivate for Player Display');
|
||||||
fireEvent.click(offBtn);
|
fireEvent.click(offBtn);
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
const calls = findCalls('setDoc', 'activeDisplay/status');
|
const calls = findCalls('updateDoc', 'activeDisplay/status');
|
||||||
const last = calls[calls.length - 1];
|
const last = calls[calls.length - 1];
|
||||||
return last.data.activeCampaignId === null;
|
return last.data.activeCampaignId === null;
|
||||||
});
|
});
|
||||||
const calls = findCalls('setDoc', 'activeDisplay/status');
|
const calls = findCalls('updateDoc', 'activeDisplay/status');
|
||||||
const last = calls[calls.length - 1];
|
const last = calls[calls.length - 1];
|
||||||
expect(last.data).toMatchObject({ activeCampaignId: null, activeEncounterId: null });
|
expect(last.data).toMatchObject({ activeCampaignId: null, activeEncounterId: null });
|
||||||
});
|
});
|
||||||
@@ -103,7 +103,7 @@ describe('Encounter -> Firebase', () => {
|
|||||||
// activate display first
|
// activate display first
|
||||||
const onBtn = await screen.findByTitle('Activate for Player Display');
|
const onBtn = await screen.findByTitle('Activate for Player Display');
|
||||||
fireEvent.click(onBtn);
|
fireEvent.click(onBtn);
|
||||||
await waitFor(() => findCall('setDoc', 'activeDisplay/status'));
|
await waitFor(() => findCall('updateDoc', 'activeDisplay/status'));
|
||||||
|
|
||||||
// delete the active encounter
|
// delete the active encounter
|
||||||
const trashBtn = screen.getAllByTitle('Delete Encounter')[0];
|
const trashBtn = screen.getAllByTitle('Delete Encounter')[0];
|
||||||
|
|||||||
@@ -50,14 +50,14 @@ describe('BUG-4: hide-player-HP toggle preserves activeDisplay', () => {
|
|||||||
|
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
const writes = getAdapterCalls().filter(
|
const writes = getAdapterCalls().filter(
|
||||||
c => c.fn === 'setDoc' && c.path.includes('activeDisplay/status')
|
c => c.fn === 'updateDoc' && c.path.includes('activeDisplay/status')
|
||||||
);
|
);
|
||||||
expect(writes.length).toBeGreaterThan(0);
|
expect(writes.length).toBeGreaterThan(0);
|
||||||
const last = writes[writes.length - 1];
|
const last = writes[writes.length - 1];
|
||||||
// data written must include activeCampaignId AND activeEncounterId
|
// patch must NOT clobber activeCampaignId/activeEncounterId.
|
||||||
// BUG: writes only {hidePlayerHp:true}, clobbering them.
|
// BUG: setDoc replace writes only {hidePlayerHp:true} → clobbers.
|
||||||
expect(last.data.activeCampaignId).toBe('c1');
|
// Fix: updateDoc patch — other fields untouched.
|
||||||
expect(last.data.activeEncounterId).toBe('e1');
|
expect(last.patch.hidePlayerHp).toBe(true);
|
||||||
}, { timeout: 3000 });
|
}, { timeout: 3000 });
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user