diff --git a/js/historyView.js b/js/historyView.js index 2b31b5e2..8a4b6ef2 100644 --- a/js/historyView.js +++ b/js/historyView.js @@ -327,7 +327,9 @@ async function defaultRemoveHandler({ .filter(Boolean) : []; if (typeof snapshot === "function") { - await snapshot(sanitized, { actor, reason }); + // Removal replaces the list — without this the snapshot would merge the + // removed item back from cache and the deletion would have no effect. + await snapshot(sanitized, { actor, reason, replace: true }); } try { if (typeof updateWatchHistoryList === "function") { diff --git a/js/ui/watchHistoryController.js b/js/ui/watchHistoryController.js index 4f2d9dc9..2064d944 100644 --- a/js/ui/watchHistoryController.js +++ b/js/ui/watchHistoryController.js @@ -313,6 +313,8 @@ export default class WatchHistoryController { const snapshotResult = await this.watchHistoryService.snapshot(normalizedItems, { actor: actorCandidate || undefined, reason, + // Removal must replace the list, not merge the removed item back in. + replace: true, }); try { diff --git a/js/watchHistoryService.js b/js/watchHistoryService.js index fc41faa6..b61252bc 100644 --- a/js/watchHistoryService.js +++ b/js/watchHistoryService.js @@ -784,6 +784,10 @@ async function publishView(pointerInput, createdAt) { async function snapshot(items, options = {}) { const reason = typeof options.reason === "string" ? options.reason : "manual"; + // Removal/clear paths must REPLACE the published list — otherwise the items the + // user just removed get merged back from the existing cache and the deletion + // silently has no effect. Adds (the default) keep merge semantics. + const replace = options.replace === true; const actorKey = resolveActorKey(options.actor); if (!actorKey) { return { ok: false, error: "missing-actor" }; @@ -810,11 +814,16 @@ async function snapshot(items, options = {}) { payloadItems = items .map((item) => normalizePointerInput(item)) .filter(Boolean); + } else if (replace && Array.isArray(items)) { + // Explicit replace with an empty list = clear the history. Must NOT fall + // back to the pending queue (that would resurrect items just removed) and + // must still publish so relays drop the old list. + payloadItems = []; } else { payloadItems = collectQueueItems(actorKey); } - if (!payloadItems.length) { + if (!payloadItems.length && !replace) { const result = { ok: true, empty: true, actor: actorKey }; emit("snapshot-empty", result); return result; @@ -830,7 +839,7 @@ async function snapshot(items, options = {}) { ) : new Set(); - if (!items) { + if (!items && !replace) { const cachedItems = getCachedSnapshotItems(actorKey); if (cachedItems.length) { payloadItems = [...payloadItems, ...cachedItems]; @@ -854,6 +863,7 @@ async function snapshot(items, options = {}) { payloadItems, { actorPubkey: actorKey, + replace, } ); diff --git a/tests/watch-history-remove-snapshot.test.mjs b/tests/watch-history-remove-snapshot.test.mjs new file mode 100644 index 00000000..65d0a4f3 --- /dev/null +++ b/tests/watch-history-remove-snapshot.test.mjs @@ -0,0 +1,98 @@ +// Watch-history removal must publish the reduced list with REPLACE semantics. +// +// Bug (SCN-watch-history-remove-replaces): the removal path calls +// watchHistoryService.snapshot(remaining, …) but snapshot published via +// updateWatchHistoryList WITHOUT replace:true — the default merges incoming +// items with the existing cached list, so the just-removed item was merged +// straight back and the deletion had no effect ("watch history delete does not +// work at all"). Removing the LAST item was even worse: an empty list fell +// through to republishing the pending queue instead of clearing. +// +// Observable outcome asserted at the boundary: the options passed to +// nostrClient.updateWatchHistoryList (the relay-publish entry point). + +import "./test-helpers/setup-localstorage.mjs"; +import assert from "node:assert/strict"; +import test from "node:test"; +import { watchHistoryService } from "../js/watchHistoryService.js"; +import { nostrClient } from "../js/nostrClientFacade.js"; + +// snapshot calls these as property lookups on the shared singleton, so stubbing +// the properties is sufficient to virtualize the relay boundary. +function installStub() { + const calls = []; + nostrClient.updateWatchHistoryList = async (items, opts) => { + calls.push({ items: items.slice(), opts }); + return { ok: true, items }; + }; + nostrClient.getWatchHistoryFingerprint = async () => `fp-${calls.length}`; + return calls; +} + +test("removing an item snapshots the remaining list with replace:true", async () => { + const calls = installStub(); + const actor = "1".repeat(64); + const remaining = [ + { type: "e", value: "keep-1" }, + { type: "e", value: "keep-2" }, + ]; + + await watchHistoryService.snapshot(remaining, { + actor, + reason: "remove-item", + replace: true, + }); + + assert.equal(calls.length, 1, "must publish exactly once"); + assert.equal( + calls[0].opts.replace, + true, + "snapshot must forward replace:true so the removed item is not merged back", + ); + assert.deepEqual( + calls[0].items.map((p) => p.value).sort(), + ["keep-1", "keep-2"], + "must publish the remaining items", + ); +}); + +test("removing the LAST item publishes an empty replace (clears, not queue republish)", async () => { + const calls = installStub(); + const actor = "2".repeat(64); + + const result = await watchHistoryService.snapshot([], { + actor, + reason: "remove-item", + replace: true, + }); + + assert.notEqual( + result?.empty, + true, + "an empty replace must NOT short-circuit as a no-op", + ); + assert.equal(calls.length, 1, "empty replace must still publish to clear relays"); + assert.equal(calls[0].opts.replace, true); + assert.equal( + calls[0].items.length, + 0, + "must publish an empty list, never fall back to the pending queue", + ); +}); + +test("adds (no replace) keep merge semantics — replace is not forced", async () => { + const calls = installStub(); + const actor = "3".repeat(64); + + await watchHistoryService.snapshot([{ type: "e", value: "new-watch" }], { + actor, + reason: "watch", + }); + + assert.equal(calls.length, 1); + assert.notEqual( + calls[0].opts.replace, + true, + "a normal watch-tracking snapshot must NOT replace (it appends)", + ); +});