diff --git a/desktop/src/features/sidebar/lib/useChannelSections.test.mjs b/desktop/src/features/sidebar/lib/useChannelSections.test.mjs index 62c0d1f7f0..4c2b1844ff 100644 --- a/desktop/src/features/sidebar/lib/useChannelSections.test.mjs +++ b/desktop/src/features/sidebar/lib/useChannelSections.test.mjs @@ -13,6 +13,7 @@ import { runWholeBlobP2a1HookSuite, runWholeBlobP2bSuite, } from "./wholeBlobSyncCarl.shared.test.mjs"; +import { runWholeBlobCarlP2SecondClickSuite } from "./wholeBlobSyncCarlP2SecondClick.shared.test.mjs"; const { act, cleanup, renderHook } = await import("@testing-library/react"); const { relayClient } = await import("@/shared/api/relayClient"); @@ -384,3 +385,15 @@ runWholeBlobP2a1HookSuite({ ); }, }); + +runWholeBlobCarlP2SecondClickSuite({ + label: "sections", + useHook: useChannelSections, + makeEdit1: (r) => r.createSection("A1-Section"), + makeEdit2: (r) => r.createSection("A2-Section"), + makeRemoteStore: () => ({ + version: 1, + sections: [{ id: "remote", name: "Remote", order: 0 }], + assignments: {}, + }), +}); diff --git a/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs b/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs index b6a80e862a..6d8755e05e 100644 --- a/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs +++ b/desktop/src/features/sidebar/lib/useChannelSortPreference.test.mjs @@ -13,6 +13,7 @@ import { runWholeBlobP2a1HookSuite, runWholeBlobP2bSuite, } from "./wholeBlobSyncCarl.shared.test.mjs"; +import { runWholeBlobCarlP2SecondClickSuite } from "./wholeBlobSyncCarlP2SecondClick.shared.test.mjs"; const { act, cleanup, renderHook } = await import("@testing-library/react"); const { relayClient } = await import("@/shared/api/relayClient"); @@ -233,3 +234,11 @@ runWholeBlobP2a1HookSuite({ ); }, }); + +runWholeBlobCarlP2SecondClickSuite({ + label: "sort", + useHook: useChannelSortPreference, + makeEdit1: (r) => r.setSortModeFor("channels", "alpha"), + makeEdit2: (r) => r.setSortModeFor("groups", "recent"), + makeRemoteStore: () => ({ version: 1, groups: { remote: "recent" } }), +}); diff --git a/desktop/src/features/sidebar/lib/wholeBlobSyncCarlP2SecondClick.shared.test.mjs b/desktop/src/features/sidebar/lib/wholeBlobSyncCarlP2SecondClick.shared.test.mjs new file mode 100644 index 0000000000..c943b3c351 --- /dev/null +++ b/desktop/src/features/sidebar/lib/wholeBlobSyncCarlP2SecondClick.shared.test.mjs @@ -0,0 +1,295 @@ +// Carl P2 second-click regression (IMPORTANT/P2, Carl review 5096781519). +// +// Two timing variants — both must hold: +// +// Variant A (debounce window): A1 click pending (debounce timer running) → +// live H102 suppressed → A2 second click → debounce fires. +// Mutation (unconditional reseed / wasIdle || !inDebounceWindow): +// publishBaseline = H102 → A2 published over H102. +// +// Variant B (in-flight): A1's debounce fires → A1 preflight parked → +// live H102 suppressed → A2 second click → A1 preflight released (stale) → +// A2 cycle runs. Mutation (wasIdle || !inDebounceWindow, timer already null): +// publishBaseline = H102 → A2 published over H102. +// +// With correct fix (wasIdle || lastIsOwnAttempt — seed only for new sequence +// or own in-flight attempt; preserve for suppressed live remote): +// Both variants: H102 ∉ ambiguousAttemptIds → lastIsOwnAttempt=false → +// publishBaseline preserved as {0,""} → remoteAdvancedSince = true +// → ADOPT H102. H102's remote-only changes are preserved. + +import assert from "node:assert/strict"; +import test, { mock } from "node:test"; + +import { relayClient } from "@/shared/api/relayClient"; +import { + makeHookTimerBed, + installEchoTauri, +} from "./sidebarSyncTestHelpers.mjs"; + +/** + * Hook-layer Carl P2 second-click regression for a single whole-blob lane. + * + * @param {object} opts + * @param {string} opts.label "sections"|"sort" + * @param {Function} opts.useHook the hook under test + * @param {Function} opts.makeEdit1 (hookResult) => void — first click + * @param {Function} opts.makeEdit2 (hookResult) => void — second click + * @param {Function} opts.makeRemoteStore () => store (live remote H102 content) + */ +export function runWholeBlobCarlP2SecondClickSuite({ + label, + useHook, + makeEdit1, + makeEdit2, + makeRemoteStore, +}) { + // --- Variant A: second click while debounce timer is still running ----------- + test(`Carl-P2 ${label} [debounce-window]: second click must not reseed baseline from suppressed live head — H102 must remain a genuine advance (adopt)`, async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const pubkey = `pk-p2-sc-dw-${label}`; + const relayUrl = `wss://r.p2sc.dw.${label}`; + + const tauri = installEchoTauri(pubkey); + + const H102 = tauri.mintHead( + makeRemoteStore(), + 200, + `evt-h102-p2sc-dw-${label}`, + ); + H102.pubkey = pubkey; + H102.kind = 30078; + H102.tags = [ + ["d", label === "sections" ? "channel-sections" : "channel-sort"], + ]; + + const live = { cb: null }; + const origFetch = relayClient.fetchEvents; + const origSubscribeLive = relayClient.subscribeLive; + const origPublish = relayClient.publishEvent; + const origReconnects = relayClient.subscribeToReconnects; + + const publishCalls = []; + let fetchCalls = 0; + + relayClient.fetchEvents = async () => { + fetchCalls++; + if (fetchCalls === 1) return []; // bootstrap: no remote head + return [H102]; // pre-publish: H102 is now the relay head + }; + relayClient.subscribeLive = async (_f, cb) => { + live.cb = cb; + return async () => {}; + }; + relayClient.publishEvent = async (evt) => { + publishCalls.push(evt); + }; + relayClient.subscribeToReconnects = () => () => {}; + + const { fireDelay, restore: restoreTimers } = makeHookTimerBed(); + + let hook = null; + try { + await act(async () => { + hook = renderHook(() => useHook(pubkey, relayUrl)); + for (let i = 0; i < 80; i++) await Promise.resolve(); + }); + + // A1: first click — debounce timer scheduled at 2000ms. + await act(async () => { + makeEdit1(hook.result.current); + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + // H102 arrives live while debounce is running — suppressed by hasPendingEdit. + assert.ok( + live.cb !== null, + `${label}: subscribeLive callback must be registered`, + ); + await act(async () => { + live.cb(H102); + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + // A2: second click — debounce timer still running (pendingStore !== null). + // Fix: wasIdle=false, H102 ∉ ambiguousAttemptIds → lastIsOwnAttempt=false + // → baseline preserved {0,""}. Mutation catches this via Variant B. + await act(async () => { + makeEdit2(hook.result.current); + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + // Fire debounce. Pre-publish fetch returns H102. + // Fix: baseline={0,""} → remoteAdvancedSince → ADOPT. publishCalls===0. + await fireDelay(2000); + for (let i = 0; i < 100; i++) await Promise.resolve(); + + assert.equal( + publishCalls.length, + 0, + `Carl-P2 ${label} [debounce-window]: H102 must be adopted — ` + + `reseeding baseline from suppressed live head silently overwrites H102`, + ); + + hook.unmount(); + } finally { + cleanup(); + tauri.restore(); + relayClient.fetchEvents = origFetch; + relayClient.subscribeLive = origSubscribeLive; + relayClient.publishEvent = origPublish; + relayClient.subscribeToReconnects = origReconnects; + restoreTimers(); + window.localStorage.clear(); + mock.reset(); + } + }); + + // --- Variant B: second click while A1's publish is in flight ----------------- + // A1's debounce has fired (debounceTimer=null, publishInFlight=true). + // H102 arrives suppressed. A2 click: wasIdle=false, H102 ∉ ambiguousAttemptIds + // → lastIsOwnAttempt=false. Old predicate (wasIdle || !inDebounceWindow) reseeds + // baseline to H102 → A2 publishes over H102. + // Correct predicate (wasIdle || lastIsOwnAttempt): preserves → adopts H102. + test(`Carl-P2 ${label} [in-flight]: second click after debounce fired must not reseed baseline from suppressed live head — H102 must remain a genuine advance (adopt)`, async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const pubkey = `pk-p2-sc-if-${label}`; + const relayUrl = `wss://r.p2sc.if.${label}`; + + const tauri = installEchoTauri(pubkey); + + const H102 = tauri.mintHead( + makeRemoteStore(), + 200, + `evt-h102-p2sc-if-${label}`, + ); + H102.pubkey = pubkey; + H102.kind = 30078; + H102.tags = [ + ["d", label === "sections" ? "channel-sections" : "channel-sort"], + ]; + + const live = { cb: null }; + const origFetch = relayClient.fetchEvents; + const origSubscribeLive = relayClient.subscribeLive; + const origPublish = relayClient.publishEvent; + const origReconnects = relayClient.subscribeToReconnects; + + const publishCalls = []; + let fetchCalls = 0; + + // A1-preflight resolver: lets us park A1's fetch until after A2 clicks. + let releaseA1Preflight = null; + + relayClient.fetchEvents = async () => { + fetchCalls++; + if (fetchCalls === 1) return []; // bootstrap + if (fetchCalls === 2) { + // A1 preflight — park until we've delivered H102 and clicked A2. + await new Promise((resolve) => { + releaseA1Preflight = resolve; + }); + return []; // returns empty so A1 would publish — but gen CAS exits it + } + // fetchCalls >= 3: A2 preflight — H102 is now the relay head. + return [H102]; + }; + relayClient.subscribeLive = async (_f, cb) => { + live.cb = cb; + return async () => {}; + }; + relayClient.publishEvent = async (evt) => { + publishCalls.push(evt); + }; + relayClient.subscribeToReconnects = () => () => {}; + + const { fireDelay, restore: restoreTimers } = makeHookTimerBed(); + + let hook = null; + try { + // Mount + bootstrap. + await act(async () => { + hook = renderHook(() => useHook(pubkey, relayUrl)); + for (let i = 0; i < 80; i++) await Promise.resolve(); + }); + + // A1 click — publishBaseline frozen to {0,""} (wasIdle=true). + await act(async () => { + makeEdit1(hook.result.current); + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + // Fire A1's debounce: debounceTimer→null, startCycle()→doPublish(A1,1) + // → fetchOwnBlobBeforePublish → fetchEvents call #2 → PARKED. + // publishInFlight=true, pendingStore=A1, pendingGeneration=1. + await act(async () => { + // fireDelay fires the timer fn and drains microtasks; the async + // doPublish chain starts but parks at the awaited fetchEvents. + await fireDelay(2000); + for (let i = 0; i < 30; i++) await Promise.resolve(); + }); + + // H102 arrives while A1's preflight is parked — suppressed by hasPendingEdit. + assert.ok( + live.cb !== null, + `${label}: subscribeLive callback must be registered`, + ); + await act(async () => { + live.cb(H102); + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + // A2 click — pendingStore !== null (A1 still pending), debounceTimer=null + // (timer already fired). Fix: wasIdle=false, H102 ∉ ambiguousAttemptIds + // → lastIsOwnAttempt=false → baseline preserved {0,""}. + // Mutation: wasIdle || !inDebounceWindow → inDebounceWindow=false → reseeds to H102. + await act(async () => { + makeEdit2(hook.result.current); + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + // Release A1's parked preflight → returns [] → A1's doPublish checks + // gen(1) !== pendingGeneration(2) → exits stale without publishing. + // startCycle() fires from finally → A2's cycle starts → schedules + // a fresh 2000ms debounce for A2's pending edit. + await act(async () => { + assert.ok( + releaseA1Preflight !== null, + `${label}: A1 preflight must have been parked`, + ); + releaseA1Preflight(); + for (let i = 0; i < 60; i++) await Promise.resolve(); + }); + + // Fire A2's debounce timer so doPublish(A2) runs → fetchEvents #3 → + // remoteAdvancedSince arbitration. Without this the timer sits non-null + // and publishCalls===0 passes vacuously under both predicates. + await fireDelay(2000); + for (let i = 0; i < 100; i++) await Promise.resolve(); + + // A2's cycle: pre-publish fetch returns H102 (fetchCalls=3). + // Fix: baseline={0,""} → remoteAdvancedSince(H102,{0,""})=true → ADOPT. + // Broken: baseline=H102 → equality → publishEvent called → test fails. + for (let i = 0; i < 100; i++) await Promise.resolve(); + + assert.equal( + publishCalls.length, + 0, + `Carl-P2 ${label} [in-flight]: second click after debounce fired must not reseed baseline — ` + + `wasIdle||!inDebounceWindow reseeds H102 as baseline → A2 publishes over H102`, + ); + + hook.unmount(); + } finally { + cleanup(); + tauri.restore(); + relayClient.fetchEvents = origFetch; + relayClient.subscribeLive = origSubscribeLive; + relayClient.publishEvent = origPublish; + relayClient.subscribeToReconnects = origReconnects; + restoreTimers(); + window.localStorage.clear(); + mock.reset(); + } + }); +} diff --git a/desktop/src/features/sidebar/lib/wholeBlobSyncManager.ts b/desktop/src/features/sidebar/lib/wholeBlobSyncManager.ts index b440d111c5..3a6912f1ae 100644 --- a/desktop/src/features/sidebar/lib/wholeBlobSyncManager.ts +++ b/desktop/src/features/sidebar/lib/wholeBlobSyncManager.ts @@ -188,10 +188,8 @@ export class WholeBlobSyncManager { // publish after a newer edit exists. private publishInFlight = false; // Whether bootstrap() has been called (started) and whether it has completed. - // publish() defers the debounce timer only when bootstrap was started but has - // not yet resolved. When bootstrap() is never called (e.g. standalone tests - // or managers used without the standard hook lifecycle), both flags stay false - // and publish() behaves as before — schedules the timer immediately. + // publish() defers the debounce timer until bootstrap resolves; when bootstrap + // is never called, both flags stay false and the timer schedules immediately. // // The P2a queue-until-bootstrap contract: no publish debounce fires between // bootstrap starting and bootstrap resolving, so a click during an unresolved @@ -203,8 +201,7 @@ export class WholeBlobSyncManager { // fetchOwnBlobBeforePublish so an edit whose baseline is {0,""} because // bootstrap failed still publishes ABOVE the first head it discovers rather // than adopting it away. (For a successful bootstrap, releaseDeferred re-freezes - // publishBaseline to the bootstrap-result head, so the baseline is not {0,""} - // at doPublish and this flag is never consulted.) + // publishBaseline to the bootstrap-result head, so this flag is never consulted.) private bootstrapFailed = false; // Disarmed once any head is independently observed after a failed bootstrap // (live relay subscription or a reconnect/periodic fetch). When disarmed, the @@ -500,14 +497,20 @@ export class WholeBlobSyncManager { isRestoredReplay = false, restoredQueuedAt?: number, ): boolean { + const wasIdle = this.pendingStore === null; + // Reseed only when (a) starting a new sequence (wasIdle) or (b) + // lastRemoteHead is our OWN attempt (lastIsOwnAttempt: eventId ∈ + // ambiguousAttemptIds). Case (b) lets foldSupersedingAttemptWinner + // repair the baseline on a same-second collision. All other superseding + // edits preserve the existing baseline: lastRemoteHead may carry a + // suppressed live H102 that must not be silently overwritten (Carl P2). + const lastIsOwnAttempt = + this.lastRemoteHead.eventId !== "" && + this.ambiguousAttemptIds.has(this.lastRemoteHead.eventId); this.pendingStore = store; ++this.pendingGeneration; - // Freeze the canonical head this edit races against at queue time. The - // pre-publish check compares against this baseline — not the mutable - // watermark — so a live event during the debounce window that advances the - // watermark still gets adopted (baseline only advances via our own - // confirmed publishes). For a restored replay: use canonicalMax(current, - // bootstrapResultHead) from the immutable bootstrap snapshot (C2): + // Freeze the canonical head this edit races against at queue time. + // For a restored replay: use canonicalMax(current, bootstrapResultHead): // No prior pending: publishBaseline={0,""}, canonicalMax sets bootstrap head. // Blocked-bootstrap: releaseDeferred already set it; re-apply is idempotent. // lastRemoteHead excluded (may carry suppressed H102 from subscribeLive). @@ -516,9 +519,10 @@ export class WholeBlobSyncManager { this.publishBaseline, this.bootstrapResultHead, ); - } else { + } else if (wasIdle || lastIsOwnAttempt) { this.publishBaseline = { ...this.lastRemoteHead }; } + // else: superseding edit — preserve baseline (Carl P2). // Suppress failed-bootstrap exception for replays (P1/C1); preserve original // queuedAt for the failed-bootstrap adopt-guard (publish when relay head // createdAt <= restoredQueuedAt — restored edit is genuinely newer).