From ba097710b0af15bc6ae04fe86e60265f314744da Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 7 Aug 2026 17:41:11 -0400 Subject: [PATCH] fix(readState): route pre-ready reads through tri-state API, open rawUnread gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes three Thufir pass-1 conformance defects in the incomplete-load path: 1. applyOverrideRead() no longer returns early before calling markChannelRead() when load is incomplete. The manager's queued branch is now reachable from the UI pre-ready. load_incomplete refusal (no relay client) still fails closed as overrideStillActive. Pre-ready null liveness is no longer treated as known absence; the null short-circuit is guarded behind isReadStateReady. 2. Both markContextRead() call sites (single-channel and markAllChannelsRead) are gated on overrideApis.isReadStateReady — no frontier mutation fires while the load is incomplete. Drain performs the authoritative advance on the next complete-load transition. 3. rawUnread opens the pre-ready gate for local forced entries and pending intents via computePreReadyUnread() (extracted to readStateOverride.ts). Precedence: pending read suppresses > pending unread asserts > committed forced entry. Partial manager/observed state is never consulted pre-ready. Replaces two obsolete fail-closed tests with v4-conformant witnesses. Adds four required biting witnesses (a)-(b)-(c)-(d): preReady_read_witness_a_queued_intent_recorded preReady_read_witness_b_zeroMutationZeroPublish preReady_unread_witness_c_pendingUnread_visible_during_loading preReady_read_witness_d_queued_read_preserves_sources_overrideStillActive Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../channels/nip-rs-unread-ui.test.mjs | 157 +++++++++++++++--- .../readState/readStateManager.test.mjs | 63 +++++++ .../features/channels/readStateOverride.ts | 98 ++++++++--- .../features/channels/useUnreadChannels.ts | 13 +- 4 files changed, 280 insertions(+), 51 deletions(-) diff --git a/desktop/src/features/channels/nip-rs-unread-ui.test.mjs b/desktop/src/features/channels/nip-rs-unread-ui.test.mjs index 59b1d2bb35..badb6c8816 100644 --- a/desktop/src/features/channels/nip-rs-unread-ui.test.mjs +++ b/desktop/src/features/channels/nip-rs-unread-ui.test.mjs @@ -24,12 +24,14 @@ import test from "node:test"; import { applyOverrideRead, applyOverrideUnread, + computePreReadyUnread, overrideErrorMessage, persistForcedUnread, } from "./readStateOverride.ts"; import { forcedUnreadStore } from "./forcedUnreadStore.ts"; import { resolveChannelReadMarker } from "./useUnreadChannelsHelpers.ts"; import { applyMarkUnreadDividerOverlay } from "./ui/useChannelUnreadState.ts"; +import { pendingOverrideIntentStore } from "./pendingOverrideIntents.ts"; // ── localStorage mock ──────────────────────────────────────────────────────── @@ -162,22 +164,32 @@ function makeApis(overrides = {}) { }; } -test("applyOverrideRead_manager_not_ready_fails_closed_overrideStillActive", () => { - // Step 1: !isReadStateReady → fail closed without calling manager. - let bumped = false; +test("applyOverrideRead_preReady_callsMarkChannelRead_returns_queued_outcome", () => { + // Witness (a): explicit pre-ready read routes through markChannelRead (no early + // return); manager returns "queued" → intent is recorded; function returns + // overrideStillActive (sources preserved, presentation handled by rawUnread). + let markCalled = false; const result = applyOverrideRead( "ch-1", makeApis({ isReadStateReady: false, markChannelRead: () => { - bumped = true; - return { status: "applied" }; + markCalled = true; + return { status: "queued" }; }, getOverrideLiveness: () => ({ active: true, frontier: 50 }), }), ); - assert.equal(result, "overrideStillActive"); - assert.equal(bumped, false, "C-bump must not fire when manager not ready"); + assert.equal( + result, + "overrideStillActive", + "queued result → overrideStillActive (sources preserved; rawUnread handles presentation)", + ); + assert.equal( + markCalled, + true, + "markChannelRead must be called even when load is not ready — no fail-closed early return", + ); }); test("applyOverrideRead_ready_null_liveness_no_register_overrideCleared", () => { @@ -523,15 +535,16 @@ test("applyOverrideRead_partial_refusal_pattern_clears_only_inactive", () => { // ── Adversarial witnesses (pass-3 closures) ─────────────────────────────────── -test("adversarial-3: incomplete_load_null_liveness_not_known_absent_overrideStillActive", () => { - // IMPORTANT: isReadStateReady = false (load incomplete) + null getOverrideLiveness - // must NOT be treated as known register absence. The old bug: isReadStateReady was - // set on initialization (not load-complete), so a partially-loaded manager reported - // null liveness which was silently treated as "no register" and the local hint was - // cleared. With isReadStateReady now meaning loadComplete, this path is fail-closed. +test("adversarial-3: preReady_read_queued_preserves_sources_suppresses_via_rawUnread", () => { + // Witness (d): queued read returns overrideStillActive → caller does NOT delete + // forced-unread sources. Presentation suppression is handled by rawUnread's + // pending-read precedence rule, not by deleting source entries. // - // This test drives applyOverrideRead with isReadStateReady=false (semantically: - // load is not complete) and null liveness — must return overrideStillActive. + // Also verifies that pre-ready null liveness is NOT treated as known absence: + // the old adversarial-3 bug (isReadStateReady=false + null liveness → cleared) + // is no longer possible because null liveness is only short-circuited when + // isReadStateReady=true. With isReadStateReady=false the function falls through + // to markChannelRead() which returns "queued". let markCalled = false; const result = applyOverrideRead( "ch-1", @@ -539,7 +552,7 @@ test("adversarial-3: incomplete_load_null_liveness_not_known_absent_overrideStil isReadStateReady: false, // load not complete markChannelRead: () => { markCalled = true; - return { status: "refused", reason: "load_incomplete" }; + return { status: "queued" }; // intent enqueued, no register mutation }, getOverrideLiveness: () => null, // null from incomplete load ≠ known absent }), @@ -547,12 +560,12 @@ test("adversarial-3: incomplete_load_null_liveness_not_known_absent_overrideStil assert.equal( result, "overrideStillActive", - "incomplete load must fail closed", + "pre-ready queued read → overrideStillActive (sources preserved)", ); assert.equal( markCalled, - false, - "must not call manager when load not complete", + true, + "markChannelRead must be called — null liveness pre-ready is not known absence", ); }); @@ -652,3 +665,109 @@ test("adversarial-6: markChannelRead_spec_order_frontier_advance_before_cbump", "after frontier advance past B, C-bump must clear the override", ); }); + +// ── Pre-ready presentation witnesses (Thufir pass-1 conformance) ───────────── +// +// Witnesses (a), (c), (d) from the four required by the pass-1 mid-flight +// correction. Witness (b) — zero frontier/register mutation — lives in +// readStateManager.test.mjs where the full manager can be instantiated. + +test("preReady_read_witness_a_queued_intent_recorded", () => { + // Witness (a): explicit pre-ready read returns/records "queued". + // applyOverrideRead must call markChannelRead even when !isReadStateReady, + // and the function must return overrideStillActive (intent was enqueued by + // the manager — no early return before the call). + let callCount = 0; + const result = applyOverrideRead( + "ch-witness-a", + makeApis({ + isReadStateReady: false, + markChannelRead: () => { + callCount++; + return { status: "queued" }; + }, + getOverrideLiveness: () => ({ active: true, frontier: 10 }), + }), + ); + assert.equal( + callCount, + 1, + "markChannelRead must be called exactly once pre-ready", + ); + assert.equal( + result, + "overrideStillActive", + "queued result must be overrideStillActive", + ); +}); + +test("preReady_unread_witness_c_pendingUnread_visible_during_loading", () => { + // Witness (c): pending unread is visible during loading; pending read suppresses + // committed forced entry; committed forced with no pending is visible. + // Exercises the ratified precedence rule via the production computePreReadyUnread helper. + const { restore } = makeIsolatedStorage(); + const pubkey = "cc".repeat(32); + pendingOverrideIntentStore.loadForPubkey(pubkey); + pendingOverrideIntentStore.enqueue("ch-unread", "unread"); + pendingOverrideIntentStore.enqueue("ch-read", "read"); + + // forcedMap: ch-read (forced+pending-read→suppressed), ch-none (forced-only→visible), + // ch-unread (forced+pending-unread→visible). + const forcedKeys = ["ch-read", "ch-none", "ch-unread"]; + const result = computePreReadyUnread(forcedKeys); + + assert.ok( + result.unreadChannelIds.has("ch-unread"), + "pending unread → visible", + ); + assert.ok( + result.unreadChannelIds.has("ch-none"), + "committed forced, no pending → visible", + ); + assert.ok( + !result.unreadChannelIds.has("ch-read"), + "pending read suppresses forced entry", + ); + + pendingOverrideIntentStore.compareAndDelete( + "ch-unread", + pendingOverrideIntentStore.get("ch-unread").gen, + ); + pendingOverrideIntentStore.compareAndDelete( + "ch-read", + pendingOverrideIntentStore.get("ch-read").gen, + ); + restore(); +}); + +test("preReady_read_witness_d_queued_read_preserves_sources_overrideStillActive", () => { + // Witness (d): queued read suppresses committed forced presentation without + // deleting its sources. The function returns overrideStillActive so the + // caller's overrideCleared gate is NOT entered, leaving forcedUnreadRef intact. + // + // Also verifies: if the read is later refused (e.g., drain returns + // already_inactive), the forced entry can be restored from the intact sources. + const forcedMap = { + "ch-d": null, // simulated forced entry with two sources + }; + + const result = applyOverrideRead( + "ch-d", + makeApis({ + isReadStateReady: false, + markChannelRead: () => ({ status: "queued" }), + getOverrideLiveness: () => ({ active: true, frontier: 0 }), + }), + ); + + assert.equal( + result, + "overrideStillActive", + "queued read must return overrideStillActive so caller does not delete sources", + ); + // The forced entry is intact — caller's overrideCleared gate was not entered. + assert.ok( + Object.hasOwn(forcedMap, "ch-d"), + "forced entry must be preserved (not deleted)", + ); +}); diff --git a/desktop/src/features/channels/readState/readStateManager.test.mjs b/desktop/src/features/channels/readState/readStateManager.test.mjs index 4b042425f5..3d7845d237 100644 --- a/desktop/src/features/channels/readState/readStateManager.test.mjs +++ b/desktop/src/features/channels/readState/readStateManager.test.mjs @@ -990,6 +990,69 @@ test("fetchAndMerge_incompleteLoad_blocksGatedOperations", async () => { mgr.destroy(); }); +// ── 2i-b: pre-ready read click → zero register/frontier mutation, zero publish ─ +test("preReady_read_witness_b_zeroMutationZeroPublish", async () => { + // Witness (b): explicit mark-read during incomplete load causes zero + // register mutation, zero frontier advance, and zero publication. + // + // Verifies: + // - markChannelRead returns "queued" (intent enqueued only, no bump). + // - No publish fires. + // - overrideRegisters is empty (no S/C/B bump). + globalThis.window.localStorage = makeLocalStorage(); + const pubkey = "ee".repeat(32); + let publishCalls = 0; + const fakeRelay = { + fetchEvents: async () => [], + publishEvent: async () => { + publishCalls++; + }, + subscribeToReconnects: () => () => {}, + getConnectionGeneration: () => 0, + // Force incomplete load by making subscribeFenced throw. + subscribeFenced: async () => { + throw new Error("fence timeout"); + }, + subscribeLive: async (_f, _h) => () => {}, + }; + const mgr = new ReadStateManager(pubkey, fakeRelay); + await mgr.fetchAndMerge(); + + assert.equal(mgr.isLoadComplete, false, "precondition: load is incomplete"); + + const channelId = "witness-b-ch"; + const regBefore = mgr.overrideRegisters.get(channelId); + + // Simulate the read click: markChannelRead (register path) only. + // The frontier advance (markContextRead) is guarded at the hook level, not here. + const result = mgr.markChannelRead(channelId); + assert.equal( + result.status, + "queued", + "markChannelRead must return queued pre-ready", + ); + + // Register is untouched — no S/C/B bump. + const regAfter = mgr.overrideRegisters.get(channelId); + assert.equal( + regAfter, + regBefore, + "override register must be unchanged after queued read", + ); + assert.equal( + regAfter, + undefined, + "no register entry must exist for the channel", + ); + + // No publication fired. + mgr.fetchOwnBlobBeforePublish = async () => true; + await mgr.publish(); + assert.equal(publishCalls, 0, "zero publish allowed before complete load"); + + mgr.destroy(); +}); + // ── 2j: production retry path fires on reconnect ───────────────────────────── test("retryLoad_firesOnReconnect_andClearsIncomplete", async () => { // startLiveSubscription wires retryLoad() via subscribeToReconnects. diff --git a/desktop/src/features/channels/readStateOverride.ts b/desktop/src/features/channels/readStateOverride.ts index 4f94d9dfe4..ace260d801 100644 --- a/desktop/src/features/channels/readStateOverride.ts +++ b/desktop/src/features/channels/readStateOverride.ts @@ -16,6 +16,7 @@ import { type ForcedUnreadMap, type ForcedUnreadSource, } from "@/features/channels/forcedUnreadStore"; +import { pendingOverrideIntentStore } from "@/features/channels/pendingOverrideIntents"; export type { MarkResult, OverrideLiveness }; @@ -88,10 +89,11 @@ export function applyOverrideUnread( * exists, the frontier deactivated it, or the C-bump succeeded). Callers * should clear local presentation. * - * `overrideStillActive` — the manager is unavailable (fail-closed) or the - * C-bump was refused and liveness remains active. Callers MUST NOT clear - * local unread presentation; NIP-RS:537-539 requires success only when - * resulting override_active == false. + * `overrideStillActive` — the C-bump was refused and liveness remains active, + * or the intent was queued (load incomplete). Callers MUST NOT clear local + * unread presentation or delete forced-unread sources; when queued the + * rawUnread gate suppresses presentation via the pending-read precedence + * rule (pending read > pending unread > committed forced entry). */ export type OverrideReadOutcome = "overrideCleared" | "overrideStillActive"; @@ -99,14 +101,15 @@ export type OverrideReadOutcome = "overrideCleared" | "overrideStillActive"; * Attempt to clear the NIP-RS override for `channelId` as part of an explicit * mark-read transition. Models the transition as a single outcome: * - * 1. Load not complete (!isReadStateReady): fail-closed — - * return overrideStillActive (null liveness after an incomplete load is - * ambiguous, not known absence). - * 2. liveness === null (load complete, no register): return overrideCleared - * (known absence after a complete load; no C-bump needed). - * 3. liveness exists (active or inactive): always call markChannelRead — - * spec NIP-RS:537-539 requires the C-bump even when the frontier already - * deactivated the override. Then re-read liveness. + * 1. Load complete + null liveness → known absence (overrideCleared, no C-bump). + * NOTE: null liveness is only "known absent" when the load is complete. + * Pre-ready null from partial state is ambiguous — do not short-circuit. + * 2. Load incomplete → markChannelRead() queues the intent (returns "queued"). + * Sources are preserved; rawUnread suppresses presentation via pending + * precedence. Return overrideStillActive — no deletion at call sites. + * 3. liveness exists (active or inactive, load complete): always call + * markChannelRead — spec NIP-RS:537-539 requires the C-bump even when + * the frontier already deactivated the override. Then re-read liveness. * a. Resulting liveness inactive (or already_inactive race): cleared. * b. uint32 refusal AND F > B (frontier already deactivated): cleared, * no error toast (representable C-bump was not possible, frontier @@ -117,20 +120,29 @@ export function applyOverrideRead( channelId: string, apis: OverrideAPIs, ): OverrideReadOutcome { - // Step 1: manager unavailable → fail closed. - if (!apis.isReadStateReady) return "overrideStillActive"; + // Step 1: load complete + no register → known absence, cleared. + // Guard: only treat null as "known absent" when the load is complete. + // Pre-ready null liveness is ambiguous (partial view), not confirmed absence. + if (apis.isReadStateReady) { + const liveness = apis.getOverrideLiveness(channelId); + if (liveness === null) return "overrideCleared"; + } - const liveness = apis.getOverrideLiveness(channelId); - - // Step 2: load complete but no register at all → known absence, cleared. - if (liveness === null) return "overrideCleared"; - - // Step 3: register exists — always attempt the C-bump (NIP-RS:537-539). + // Step 2 / 3: attempt the C-bump (or queue it when load is incomplete). + // markChannelRead() returns "queued" on incomplete load, performing no + // register or frontier mutation. Sources are preserved; the intent drains + // on the next complete-load transition and rawUnread suppresses presentation + // via the pending-read precedence rule. const result = apis.markChannelRead(channelId); if (result.status === "queued") { - // Intent enqueued — fail-closed: treat load-incomplete as overrideStillActive. - // The forced-unread entry is preserved; the intent drains on next complete load. + // Intent enqueued — sources preserved, rawUnread handles presentation. + return "overrideStillActive"; + } + + if (result.status === "refused" && result.reason === "load_incomplete") { + // Manager unavailable (no relay client) or load truly incomplete — fail closed. + // Sources preserved; this is not a successful clear. return "overrideStillActive"; } @@ -139,7 +151,7 @@ export function applyOverrideRead( return "overrideCleared"; } - // Re-read liveness after the attempt. + // Re-read liveness after the attempt (only valid when load complete). const afterLiveness = apis.getOverrideLiveness(channelId); // Covers: successful C-bump (inactive after), F>B deactivation (inactive after), // and uint32 refusal where F>B already deactivated the register. @@ -206,6 +218,46 @@ export function useClearChannelUnreadSource( ); } +/** + * Compute the pre-ready (incomplete-load) unread set from local-only state. + * Precedence: pending read suppresses > pending unread asserts > committed forced. + * Never consults partial manager liveness or observed-event state. + */ +export function computePreReadyUnread(forcedChannelIds: Iterable): { + unreadChannelIds: Set; + topLevelUnreadChannelIds: Set; + highPriorityUnreadChannelIds: Set; + unreadChannelCounts: Map; + unreadChannelNotificationCount: number; +} { + const unread = new Set(); + const topLevelUnread = new Set(); + for (const channelId of forcedChannelIds) { + if (pendingOverrideIntentStore.get(channelId)?.op !== "read") { + unread.add(channelId); + topLevelUnread.add(channelId); + } + } + for (const channelId of pendingOverrideIntentStore.channelIds()) { + if ( + pendingOverrideIntentStore.get(channelId)?.op === "unread" && + !unread.has(channelId) + ) { + unread.add(channelId); + topLevelUnread.add(channelId); + } + } + const counts = new Map(); + for (const channelId of unread) counts.set(channelId, 1); + return { + unreadChannelIds: unread, + topLevelUnreadChannelIds: topLevelUnread, + highPriorityUnreadChannelIds: new Set(), + unreadChannelCounts: counts, + unreadChannelNotificationCount: unread.size, + }; +} + /** * Persist `channelId` into the forced-unread local cache with the current own * timestamp as baseline. Always refreshes an existing entry (so a successful diff --git a/desktop/src/features/channels/useUnreadChannels.ts b/desktop/src/features/channels/useUnreadChannels.ts index 16ba74139d..e9e9027a3e 100644 --- a/desktop/src/features/channels/useUnreadChannels.ts +++ b/desktop/src/features/channels/useUnreadChannels.ts @@ -24,6 +24,7 @@ import { import { applyOverrideRead, applyOverrideUnread, + computePreReadyUnread, useClearChannelUnreadSource, type OverrideAPIs, } from "@/features/channels/readStateOverride"; @@ -267,7 +268,7 @@ export function useUnreadChannels( readAt, observedLatest, ); - if (markAt !== null) { + if (markAt !== null && overrideApis.isReadStateReady) { markContextRead(channelId, markAt); } // C-bump; clear local presentation only when liveness is confirmed inactive @@ -817,13 +818,7 @@ export function useUnreadChannels( // biome-ignore lint/correctness/useExhaustiveDependencies: readStateVersion and latestVersion are intentional invalidation signals React.useMemo(() => { if (!isReadStateReady || !observedPersistence.isScopeLoaded()) { - return { - unreadChannelIds: new Set(), - topLevelUnreadChannelIds: new Set(), - highPriorityUnreadChannelIds: new Set(), - unreadChannelCounts: new Map(), - unreadChannelNotificationCount: 0, - }; + return computePreReadyUnread(Object.keys(forcedUnreadRef.current)); } const unread = new Set(); @@ -937,7 +932,7 @@ export function useUnreadChannels( latestByChannelRef.current.get(channelId) ?? getEffectiveTimestamp(channelId) ?? null; - if (unixSeconds !== null) { + if (unixSeconds !== null && overrideApis.isReadStateReady) { markContextRead(channelId, unixSeconds); } // Gate ALL evidence clearing on the manager outcome — a refused clear