diff --git a/desktop/src/features/sidebar/lib/channelSectionsStorage.test.mjs b/desktop/src/features/sidebar/lib/channelSectionsStorage.test.mjs index f97ef8a7df..52415bebe9 100644 --- a/desktop/src/features/sidebar/lib/channelSectionsStorage.test.mjs +++ b/desktop/src/features/sidebar/lib/channelSectionsStorage.test.mjs @@ -62,13 +62,34 @@ test("parseChannelSectionPayload: non-object input returns null", () => { assert.equal(parseChannelSectionPayload(true), null); }); +test("parseChannelSectionPayload: unsupported version returns null (Carl P1.2)", () => { + // A future schema version must not be accepted as v1 state — must return null + // so the relay path triggers retain/retry rather than publishing over it. + assert.equal( + parseChannelSectionPayload({ version: 2, sections: [], assignments: {} }), + null, + "version 2 must be rejected", + ); + assert.equal( + parseChannelSectionPayload({ sections: [], assignments: {} }), + null, + "missing version must be rejected", + ); + assert.equal( + parseChannelSectionPayload({ version: 0, sections: [], assignments: {} }), + null, + "version 0 must be rejected", + ); +}); + test("parseChannelSectionPayload: missing sections returns empty sections array", () => { - const result = parseChannelSectionPayload({ assignments: {} }); + const result = parseChannelSectionPayload({ version: 1, assignments: {} }); assert.deepEqual(result?.sections, []); }); test("parseChannelSectionPayload: malformed section entries are filtered out", () => { const payload = { + version: 1, sections: [ { id: 123, name: "Bad ID", order: 0 }, { id: "s1", name: 456, order: 0 }, @@ -84,6 +105,7 @@ test("parseChannelSectionPayload: malformed section entries are filtered out", ( test("parseChannelSectionPayload: valid sections with some invalid ones filters correctly", () => { const payload = { + version: 1, sections: [ { id: "s1", name: "Valid", order: 0 }, { id: 99, name: "Bad ID", order: 1 }, @@ -99,12 +121,13 @@ test("parseChannelSectionPayload: valid sections with some invalid ones filters }); test("parseChannelSectionPayload: missing assignments returns empty assignments object", () => { - const result = parseChannelSectionPayload({ sections: [] }); + const result = parseChannelSectionPayload({ version: 1, sections: [] }); assert.deepEqual(result?.assignments, {}); }); test("parseChannelSectionPayload: assignments with non-string values are filtered out", () => { const payload = { + version: 1, sections: [{ id: "s1", name: "Test", order: 0 }], assignments: { chan1: "s1", chan2: 42, chan3: null, chan4: true }, }; @@ -114,6 +137,7 @@ test("parseChannelSectionPayload: assignments with non-string values are filtere test("parseChannelSectionPayload: orphaned assignments are stripped", () => { const payload = { + version: 1, sections: [{ id: "s1", name: "Exists", order: 0 }], assignments: { chan1: "s1", chan2: "missing-section" }, }; diff --git a/desktop/src/features/sidebar/lib/channelSectionsStorage.ts b/desktop/src/features/sidebar/lib/channelSectionsStorage.ts index afaf9a6e4c..19582657f9 100644 --- a/desktop/src/features/sidebar/lib/channelSectionsStorage.ts +++ b/desktop/src/features/sidebar/lib/channelSectionsStorage.ts @@ -87,6 +87,13 @@ export function parseChannelSectionPayload( ): ChannelSectionStore | null { if (typeof json !== "object" || json === null) return null; const obj = json as Record; + // Reject payloads from a future schema version we cannot safely interpret: + // a v2+ blob on the relay must trigger the retain/retry path so a later local + // edit never publishes over authoritative state this client does not understand + // (matches sort/stars/mutes parsers and Carl P1). Local-storage reads always + // go through `parseRaw`, which checks `version !== 1` before calling here, so + // this guard does not change the local-storage parse contract. + if (obj.version !== 1) return null; const sections: ChannelSection[] = Array.isArray(obj.sections) ? obj.sections.flatMap((entry: unknown): ChannelSection[] => { if (typeof entry !== "object" || entry === null) return []; diff --git a/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs b/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs index 92ac913454..1ac94d8dc6 100644 --- a/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs +++ b/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs @@ -441,6 +441,63 @@ test("malformed (non-JSON) head payload retains the pending edit, never publishi } }); +// Carl P1.2 regression (sections): an unsupported/future payload version +// arriving from the relay must trigger the retain/retry path, never publish +// over it. Sort, stars, and mutes all check `version !== 1` in their parsers; +// sections was the odd one out. `parseChannelSectionPayload` now rejects +// non-v1 payloads so `decryptAndParse` returns null and the manager retains. +// +// Mutation: removing `if (obj.version !== 1) return null` from +// `parseChannelSectionPayload` causes the parser to accept the v2 blob as v1 +// state, `decryptAndParse` returns a non-null result, and the manager falls +// through to a publish that overwrites authoritative state this client does not +// understand. +test("unsupported head payload version (sections) retains the pending edit, never publishing", async () => { + mock.method(relayClient, "fetchEvents", () => + Promise.resolve([ + { + pubkey: "pk-secver", + content: "good-cipher", + created_at: 500, + id: "evt-secver", + }, + ]), + ); + const publishCalls = []; + mock.method(relayClient, "publishEvent", (...args) => { + publishCalls.push(args); + return Promise.resolve(); + }); + const fw = makeFakeWindow(); + const restore = installFakeWindow(fw); + // Decrypts cleanly but carries a future schema version the parser now rejects. + const tauri = installTauriMock( + JSON.stringify({ version: 2, sections: [], assignments: {} }), + ); + try { + const manager = new ChannelSectionSyncManager("pk-secver", RELAY); + manager.publishSections( + makeSectionsStore([{ id: "s1", name: "Work", order: 0 }]), + ); + fw._fireTimer(); + await new Promise((r) => setTimeout(r, 20)); + assert.equal( + publishCalls.length, + 0, + "must not publish over a head whose payload version we do not support", + ); + assert.ok( + manager.getPendingStore() !== null, + "unsupported head must retain the pending edit", + ); + assert.ok(fw._hasTimer(), "a retry must be scheduled"); + } finally { + tauri.restore(); + restore(); + mock.reset(); + } +}); + // ─── Failed pre-publish fetch: retain, never publish (Carl P1) ──────────────── // The pre-publish fetch THROWS (timeout / auth / socket) — this is NOT proof diff --git a/desktop/src/features/sidebar/lib/useChannelSections.test.mjs b/desktop/src/features/sidebar/lib/useChannelSections.test.mjs index 783df2c329..ae6098c3dd 100644 --- a/desktop/src/features/sidebar/lib/useChannelSections.test.mjs +++ b/desktop/src/features/sidebar/lib/useChannelSections.test.mjs @@ -77,6 +77,136 @@ test("assignChannel refreshes an existing assignment before the next eviction", } }); +// Carl P1.1 regression (sections): a peer-cache storage event arriving while a +// local whole-blob edit is pending must NOT clobber the optimistic edit or +// strand its outbox. The storage handler must defer to the pending edit just as +// `applyRemote` does for relay arrivals. The pending edit's debounced publish +// owns convergence and will publish-or-adopt against the relay. +// +// Scenario: window A creates section A1 (pending in outbox). Window B writes +// B1 (a different store) to the shared cache. A's storage handler fires. Before +// A's debounce fires, the user makes a second edit A2 inside the same debounce +// window. The resulting store must be derived from A1's state (A2 added on top +// of A1), and the outbox must still hold A's intent — not B1's shape. +// +// Mutation: removing the `hasPendingEdit()` guard in the storage handler causes +// setStore to apply B1, so A2 is derived from B1 instead. The outbox record +// still holds A's first edit shape, not the B1-derived A2 — an inconsistency +// the manager cannot resolve. The test detects the replacement: if B1 were +// applied, the final sections array would include B1's section id. +test("storage event while a local edit is pending is deferred, not applied", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { relayClient } = await import("@/shared/api/relayClient"); + const { useChannelSections } = await import("./useChannelSections.ts"); + const { storageKey } = await import("./channelSectionsStorage.ts"); + + const origFetch = relayClient.fetchEvents; + const origLive = relayClient.subscribeLive; + const origReconnect = relayClient.subscribeToReconnects; + const origPublish = relayClient.publishEvent; + const origTauri = window.__TAURI_INTERNALS__; + + // Block all publishes so the debounce never fires during the test. + relayClient.fetchEvents = async () => []; + relayClient.subscribeLive = async () => async () => {}; + relayClient.subscribeToReconnects = () => () => {}; + relayClient.publishEvent = async () => {}; + window.__TAURI_INTERNALS__ = { + invoke: (cmd) => { + if (cmd === "nip44_encrypt_to_self") return Promise.resolve("ct"); + if (cmd === "sign_event") + return Promise.resolve( + JSON.stringify({ + id: "signed", + pubkey: "pk-storage-guard", + content: "ct", + created_at: 0, + kind: 30078, + tags: [], + sig: "s", + }), + ); + return Promise.reject(new Error(`unmocked ${cmd}`)); + }, + }; + + const pubkey = "pk-storage-guard"; + const relayUrl = "wss://r.storage-guard"; + let hook = null; + try { + await act(async () => { + hook = renderHook(() => useChannelSections(pubkey, relayUrl)); + await Promise.resolve(); + await Promise.resolve(); + }); + + // A1: window A creates a section — this becomes the pending edit. + await act(async () => { + hook.result.current.createSection("A1-Section"); + }); + const storeAfterA1 = hook.result.current.sections.map((s) => s.id); + assert.ok( + storeAfterA1.some((id) => id.length > 0), + "A1 section created", + ); + + // Window B writes B1 to the shared scoped cache key (simulates a peer + // window's edit landing in localStorage and firing the storage event). + const b1Store = JSON.stringify({ + version: 1, + sections: [{ id: "b1-section", name: "B1", order: 0 }], + assignments: {}, + }); + await act(async () => { + window.localStorage.setItem(storageKey(pubkey, relayUrl), b1Store); + window.dispatchEvent( + new window.StorageEvent("storage", { + key: storageKey(pubkey, relayUrl), + newValue: b1Store, + storageArea: window.localStorage, + }), + ); + await Promise.resolve(); + await Promise.resolve(); + }); + + // B1 must NOT have replaced A1's optimistic state. + const storeAfterStorageEvent = hook.result.current.sections.map( + (s) => s.id, + ); + assert.ok( + !storeAfterStorageEvent.includes("b1-section"), + "storage event while pending must not apply B1 to A's optimistic state", + ); + assert.deepEqual( + storeAfterStorageEvent, + storeAfterA1, + "optimistic state must be unchanged after the deferred storage event", + ); + + // A2: user makes a second edit inside the debounce window. It is derived + // from A1's state (since B1 was deferred), so the result contains A1's + // section plus A2's. + await act(async () => { + hook.result.current.createSection("A2-Section"); + }); + const finalIds = hook.result.current.sections.map((s) => s.id); + assert.ok( + !finalIds.includes("b1-section"), + "A2 must be derived from A1 state, not B1 — b1-section must not appear", + ); + + hook.unmount(); + } finally { + cleanup(); + relayClient.fetchEvents = origFetch; + relayClient.subscribeLive = origLive; + relayClient.subscribeToReconnects = origReconnect; + relayClient.publishEvent = origPublish; + window.__TAURI_INTERNALS__ = origTauri; + } +}); + // Fix 2 regression: a live remote arriving while a local edit is pending must // NOT overwrite the optimistic edit or strand its durable outbox. The pending // edit's own debounced publish owns convergence (publish-or-adopt). Reverting diff --git a/desktop/src/features/sidebar/lib/useChannelSections.ts b/desktop/src/features/sidebar/lib/useChannelSections.ts index 348ba531ae..c360eb9e55 100644 --- a/desktop/src/features/sidebar/lib/useChannelSections.ts +++ b/desktop/src/features/sidebar/lib/useChannelSections.ts @@ -82,6 +82,15 @@ export function useChannelSections( if (e.key !== key) { return; } + // A peer window wrote to the shared cache. Honour the same pending-aware + // guard that relay arrivals use in `applyRemote`: if a local whole-blob + // edit is in flight the manager owns convergence (its debounced + // publish-or-adopt is the single arbitration point). Applying a peer cache + // write unconditionally would clobber the optimistic edit and, on the next + // keystroke, strand the outbox record for the old intent (Carl P1). + // When nothing is pending the peer write applies immediately, preserving + // cross-window state mirroring for the idle case. + if (managerRef.current?.hasPendingEdit()) return; setStore(readChannelSectionsStore(pubkey, relayUrl)); }; window.addEventListener("storage", handler); diff --git a/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs b/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs index 80d2f08ab6..7d41ecd68d 100644 --- a/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs +++ b/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs @@ -18,6 +18,124 @@ before(() => { after(() => dom.window.close()); +// Carl P1.1 regression (sort): a peer-cache storage event arriving while a +// local whole-blob sort edit is pending must NOT clobber the optimistic edit. +// The storage handler must defer to the pending edit just as `applyRemote` does +// for relay arrivals. Peer writes arrive immediately when nothing is pending. +// +// Scenario: window A sets sort mode for "channels" (pending). Window B writes +// B1 (sort mode "recent" for "channels") to the shared cache. A's storage event +// fires. A2 is then set inside the debounce window. The resulting sort mode for +// A must reflect A2, not B1's value — B1's write was ignored while A1 was live. +// +// Mutation: removing the `hasPendingEdit()` guard in the storage handler lets +// B1 apply, so A2's `setStore` updater derives from B1, and the visible mode +// reverts to B1's value. +test("storage event while a local sort edit is pending is deferred, not applied", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { relayClient } = await import("@/shared/api/relayClient"); + const { useChannelSortPreference } = await import( + "./useChannelSortPreference.ts" + ); + const { storageKey } = await import("./channelSortPreference.ts"); + + const origFetch = relayClient.fetchEvents; + const origLive = relayClient.subscribeLive; + const origReconnect = relayClient.subscribeToReconnects; + const origPublish = relayClient.publishEvent; + const origTauri = window.__TAURI_INTERNALS__; + + relayClient.fetchEvents = async () => []; + relayClient.subscribeLive = async () => async () => {}; + relayClient.subscribeToReconnects = () => () => {}; + relayClient.publishEvent = async () => {}; + window.__TAURI_INTERNALS__ = { + invoke: (cmd) => { + if (cmd === "nip44_encrypt_to_self") return Promise.resolve("ct"); + if (cmd === "sign_event") + return Promise.resolve( + JSON.stringify({ + id: "signed", + pubkey: "pk-sort-sg", + content: "ct", + created_at: 0, + kind: 30078, + tags: [], + sig: "s", + }), + ); + return Promise.reject(new Error(`unmocked ${cmd}`)); + }, + }; + + const pubkey = "pk-sort-sg"; + const relayUrl = "wss://r.sort-sg"; + let hook = null; + try { + await act(async () => { + hook = renderHook(() => useChannelSortPreference(pubkey, relayUrl)); + await Promise.resolve(); + await Promise.resolve(); + }); + + // A1: set "channels" to "alpha" — becomes the pending edit. + await act(async () => { + hook.result.current.setSortModeFor("channels", "alpha"); + }); + assert.equal( + hook.result.current.sortModeFor("channels"), + "alpha", + "A1: channels sort set to alpha", + ); + + // Window B writes "recent" for "channels" to the shared cache and fires a + // storage event. + const b1Store = JSON.stringify({ + version: 1, + groups: { channels: "recent" }, + }); + await act(async () => { + window.localStorage.setItem(storageKey(pubkey, relayUrl), b1Store); + window.dispatchEvent( + new window.StorageEvent("storage", { + key: storageKey(pubkey, relayUrl), + newValue: b1Store, + storageArea: window.localStorage, + }), + ); + await Promise.resolve(); + await Promise.resolve(); + }); + + // B1's "recent" must NOT have replaced A1's "alpha". + assert.equal( + hook.result.current.sortModeFor("channels"), + "alpha", + "storage event while pending must not apply B1 (recent) over A1 (alpha)", + ); + + // A2: set "starred" to "recent" — derived from A1 state (channels stays alpha). + await act(async () => { + hook.result.current.setSortModeFor("starred", "recent"); + }); + // channels must still reflect A1 (alpha), not B1 (recent). + assert.equal( + hook.result.current.sortModeFor("channels"), + "alpha", + "A2 must be derived from A1 state — channels must remain alpha, not revert to B1 recent", + ); + + hook.unmount(); + } finally { + cleanup(); + relayClient.fetchEvents = origFetch; + relayClient.subscribeLive = origLive; + relayClient.subscribeToReconnects = origReconnect; + relayClient.publishEvent = origPublish; + window.__TAURI_INTERNALS__ = origTauri; + } +}); + // Carl P1-sort regression: a live remote arriving while a local sort edit is // still pending must NOT overwrite the optimistic edit or strand its durable // outbox. Reverting applyRemote's `hasPendingEdit()` guard (or restoring the diff --git a/desktop/src/features/sidebar/lib/useChannelSortPreference.ts b/desktop/src/features/sidebar/lib/useChannelSortPreference.ts index f198c4de2c..0d0937c682 100644 --- a/desktop/src/features/sidebar/lib/useChannelSortPreference.ts +++ b/desktop/src/features/sidebar/lib/useChannelSortPreference.ts @@ -82,6 +82,15 @@ export function useChannelSortPreference( const key = storageKey(pubkey, relayUrl); const handler = (e: StorageEvent) => { if (e.key !== key) return; + // A peer window wrote to the shared cache. Honour the same pending-aware + // guard that relay arrivals use in `applyRemote`: if a local whole-blob + // edit is in flight the manager owns convergence (its debounced + // publish-or-adopt is the single arbitration point). Applying a peer cache + // write unconditionally would clobber the optimistic edit and, on the next + // keystroke, strand the outbox record for the old intent (Carl P1). + // When nothing is pending the peer write applies immediately, preserving + // cross-window state mirroring for the idle case. + if (managerRef.current?.hasPendingEdit()) return; setStore(readChannelSortStore(pubkey, relayUrl)); }; window.addEventListener("storage", handler);