mirror of
https://github.com/block/buzz.git
synced 2026-11-12 20:31:12 +00:00
fix(nip-rs): close round-3 findings — production wiring, tombstone ordering, mark-all transition
CRITICAL 1 (production rail never receives manager projection):
Wire getProjection through the actual production path:
useCommunityUnread: destructure getProjection from useAppShell()
pollCommunityUnread: add getProjection param, pass to fetchCommunityUnread
AppShellContext: add getProjection: () => ReadStateProjection | null to context type
and default value; AppShell destructures from useUnreadChannels and supplies
to AppShellProvider. Production polls now receive the live manager projection.
CRITICAL 2 (stale complete projection overrides fresher fetched tombstone):
Add fetchedTombstoneChannels guard in fetchCommunityUnread. When completeProjection
is present, iterate readState.overrides; any channel whose fetched register has s===0
(wire tombstone: ov_c present, ov_s absent) is added to the set. The projection's
liveness check is skipped for those channels — a fresher relay tombstone is
authoritative even when loadComplete=true, because loadComplete establishes
enumeration completeness, NOT temporal ordering against this observer's independent
fetch. The projection live register (S=5,C=0,B=100,F=50) can no longer resurrect
a channel cleared on the relay. Adversarial-5 witness added:
stale_complete_projection_yieldsTo_fresher_fetched_tombstone
IMPORTANT (mark-all transition tested by simulation):
Extract applyMarkAllReadTransition to readStateOverride.ts: exported function that
calls applyOverrideRead and conditionally deletes from forcedUnread, latestByChannel,
observedUnreadEvents only on overrideCleared. Also export MarkAllEvidenceMaps type.
useUnreadChannels.ts calls the production function directly. adversarial-4 test
updated to call applyMarkAllReadTransition (not a reimplementation), asserting both
branches: cleared purges all three maps; refused preserves all three.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
parent
58f5feb289
commit
0edaad3d8c
@@ -342,6 +342,7 @@ export function AppShell() {
|
||||
mutedRootIds,
|
||||
muteThread,
|
||||
unmuteThread,
|
||||
getProjection,
|
||||
} = useUnreadChannels(sidebarChannels, activeChannel, {
|
||||
pubkey: identityQuery.data?.pubkey,
|
||||
relayClient,
|
||||
@@ -404,8 +405,6 @@ export function AppShell() {
|
||||
mutedRootIds,
|
||||
channels,
|
||||
);
|
||||
|
||||
// Badge count consumes the shared NIP-RS read-state from useUnreadChannels.
|
||||
const { homeBadgeCount, homeBadgeCountExcludingHighPriority } =
|
||||
useHomeFeedNotificationState(
|
||||
homeFeedQuery.data,
|
||||
@@ -733,6 +732,7 @@ export function AppShell() {
|
||||
threadActivityFeedItems,
|
||||
feedItemState,
|
||||
onOpenSettings: handleOpenSettings,
|
||||
getProjection,
|
||||
}}
|
||||
>
|
||||
<HuddleProvider>
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import * as React from "react";
|
||||
import type { ContextParentResolver } from "@/features/channels/readState/readStateManager";
|
||||
import type { ReadStateProjection } from "@/features/channels/readState/readStateManager";
|
||||
import type { ThreadActivityItem } from "@/features/channels/useUnreadChannels";
|
||||
import type { FeedItemState } from "@/features/home/useFeedItemState";
|
||||
import type { FeedItem } from "@/shared/api/types";
|
||||
@@ -51,6 +52,9 @@ type AppShellContextValue = {
|
||||
// that render under AppShell (channel, home, projects, pulse, agents).
|
||||
// Used by config-nudge cards to deep-link to Settings → Agents.
|
||||
onOpenSettings: ((section: SettingsSection) => void) | null;
|
||||
// Manager projection accessor for community rail polls — re-read per poll,
|
||||
// never cached across notification boundaries.
|
||||
getProjection: () => ReadStateProjection | null;
|
||||
};
|
||||
|
||||
const AppShellContext = React.createContext<AppShellContextValue>({
|
||||
@@ -83,6 +87,7 @@ const AppShellContext = React.createContext<AppShellContextValue>({
|
||||
unreadSet: EMPTY_SET,
|
||||
},
|
||||
onOpenSettings: null,
|
||||
getProjection: () => null,
|
||||
});
|
||||
|
||||
export function AppShellProvider({
|
||||
|
||||
@@ -22,6 +22,7 @@ import assert from "node:assert/strict";
|
||||
import test from "node:test";
|
||||
|
||||
import {
|
||||
applyMarkAllReadTransition,
|
||||
applyOverrideRead,
|
||||
applyOverrideUnread,
|
||||
overrideErrorMessage,
|
||||
@@ -544,15 +545,15 @@ test("adversarial-3: incomplete_load_null_liveness_not_known_absent_overrideStil
|
||||
);
|
||||
});
|
||||
|
||||
test("adversarial-4: markAllChannelsRead_confirmed_active_refusal_keeps_evidence", () => {
|
||||
// IMPORTANT: markAllChannelsRead must NOT clear latestByChannelRef or
|
||||
// observedUnreadEventsByChannelRef when the override read is refused and
|
||||
// liveness remains active. Evidence must be preserved so the UI can still
|
||||
// represent the failed channel's unread state.
|
||||
test("adversarial-4: applyMarkAllReadTransition_both_branches", () => {
|
||||
// IMPORTANT: applyMarkAllReadTransition is the extracted production transition
|
||||
// used inside markAllChannelsRead. Testing it directly (not a simulation)
|
||||
// ensures production regressions to unconditional deletion are caught.
|
||||
//
|
||||
// This test uses applyOverrideRead directly to simulate the per-channel loop
|
||||
// in markAllChannelsRead and verifies that refused channels keep their evidence
|
||||
// while cleared channels discard theirs.
|
||||
// ch-cleared: applyOverrideRead succeeds (liveness goes inactive) →
|
||||
// forcedUnread, latestByChannel, observedUnreadEvents all removed.
|
||||
// ch-refused: applyOverrideRead refused (liveness stays active) →
|
||||
// all three maps preserved intact.
|
||||
const livenessMap = {
|
||||
"ch-cleared": { active: true, frontier: 100 },
|
||||
"ch-refused": { active: true, frontier: 100 }, // uint32_overflow refusal
|
||||
@@ -566,13 +567,11 @@ test("adversarial-4: markAllChannelsRead_confirmed_active_refusal_keeps_evidence
|
||||
livenessMap["ch-cleared"] = { active: false, frontier: 101 };
|
||||
return { success: true };
|
||||
}
|
||||
// ch-refused: refuse; liveness stays active
|
||||
return { success: false, reason: "uint32_overflow" };
|
||||
},
|
||||
getOverrideLiveness: (id) => livenessMap[id] ?? null,
|
||||
};
|
||||
|
||||
// latestByChannel and observedEvents are the evidence maps.
|
||||
const latestByChannel = new Map([
|
||||
["ch-cleared", 200],
|
||||
["ch-refused", 200],
|
||||
@@ -582,49 +581,54 @@ test("adversarial-4: markAllChannelsRead_confirmed_active_refusal_keeps_evidence
|
||||
["ch-refused", new Map([["ev2", { kind: 9, createdAt: 200 }]])],
|
||||
]);
|
||||
const forcedMap = { "ch-cleared": 99, "ch-refused": 99 };
|
||||
const maps = {
|
||||
forcedUnread: forcedMap,
|
||||
latestByChannel,
|
||||
observedUnreadEvents: observedEvents,
|
||||
};
|
||||
|
||||
// Simulate the production per-channel loop in markAllChannelsRead.
|
||||
for (const channelId of ["ch-cleared", "ch-refused"]) {
|
||||
const outcome = applyOverrideRead(channelId, apis);
|
||||
if (outcome === "overrideCleared") {
|
||||
delete forcedMap[channelId];
|
||||
latestByChannel.delete(channelId);
|
||||
observedEvents.delete(channelId);
|
||||
}
|
||||
// On overrideStillActive: do NOT touch latestByChannel or observedEvents.
|
||||
}
|
||||
// Drive the production transition directly (not a reimplementation).
|
||||
const clearedOutcome = applyMarkAllReadTransition("ch-cleared", apis, maps);
|
||||
const refusedOutcome = applyMarkAllReadTransition("ch-refused", apis, maps);
|
||||
|
||||
// ch-cleared: override confirmed inactive → evidence discarded.
|
||||
assert.equal(clearedOutcome, "overrideCleared", "cleared channel: outcome");
|
||||
assert.equal(
|
||||
refusedOutcome,
|
||||
"overrideStillActive",
|
||||
"refused channel: outcome",
|
||||
);
|
||||
|
||||
// ch-cleared: override confirmed inactive → all three maps purged.
|
||||
assert.equal(
|
||||
latestByChannel.has("ch-cleared"),
|
||||
false,
|
||||
"cleared channel: latestByChannel removed",
|
||||
"cleared: latestByChannel removed",
|
||||
);
|
||||
assert.equal(
|
||||
observedEvents.has("ch-cleared"),
|
||||
false,
|
||||
"cleared channel: observedEvents removed",
|
||||
"cleared: observedEvents removed",
|
||||
);
|
||||
assert.equal(
|
||||
Object.hasOwn(forcedMap, "ch-cleared"),
|
||||
false,
|
||||
"cleared channel: forcedMap entry removed",
|
||||
"cleared: forcedMap removed",
|
||||
);
|
||||
|
||||
// ch-refused: override still active → evidence preserved.
|
||||
// ch-refused: override still active → all three maps preserved.
|
||||
assert.equal(
|
||||
latestByChannel.has("ch-refused"),
|
||||
true,
|
||||
"refused channel: latestByChannel preserved",
|
||||
"refused: latestByChannel preserved",
|
||||
);
|
||||
assert.equal(
|
||||
observedEvents.has("ch-refused"),
|
||||
true,
|
||||
"refused channel: observedEvents preserved",
|
||||
"refused: observedEvents preserved",
|
||||
);
|
||||
assert.equal(
|
||||
Object.hasOwn(forcedMap, "ch-refused"),
|
||||
true,
|
||||
"refused channel: forcedMap entry preserved",
|
||||
"refused: forcedMap preserved",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -13,6 +13,7 @@ import {
|
||||
forcedUnreadStore,
|
||||
type ForcedUnreadMap,
|
||||
} from "@/features/channels/forcedUnreadStore";
|
||||
import type { ObservedUnreadEvent } from "@/features/channels/unreadChannelCounts";
|
||||
|
||||
export type { MarkResult, OverrideLiveness };
|
||||
|
||||
@@ -158,3 +159,36 @@ export function persistForcedUnread(
|
||||
if (pubkey) forcedUnreadStore.write(pubkey, forcedMap);
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Per-channel evidence maps that `markAllChannelsRead` mutates.
|
||||
* Extracted so the per-channel transition can be tested without the full hook.
|
||||
*/
|
||||
export type MarkAllEvidenceMaps = {
|
||||
forcedUnread: ForcedUnreadMap;
|
||||
latestByChannel: Map<string, number>;
|
||||
observedUnreadEvents: Map<string, Map<string, ObservedUnreadEvent>>;
|
||||
};
|
||||
|
||||
/**
|
||||
* Apply the per-channel mark-all-read transition for a single channel ID.
|
||||
*
|
||||
* Calls `applyOverrideRead` to attempt the NIP-RS C-bump. On `overrideCleared`,
|
||||
* removes the channel's entry from all three evidence maps. On
|
||||
* `overrideStillActive` (refused or load incomplete), all maps are left
|
||||
* intact — the caller must NOT wipe evidence for channels whose clear was
|
||||
* refused. Returns the outcome for caller use.
|
||||
*/
|
||||
export function applyMarkAllReadTransition(
|
||||
channelId: string,
|
||||
apis: OverrideAPIs,
|
||||
maps: MarkAllEvidenceMaps,
|
||||
): OverrideReadOutcome {
|
||||
const outcome = applyOverrideRead(channelId, apis);
|
||||
if (outcome === "overrideCleared") {
|
||||
delete maps.forcedUnread[channelId];
|
||||
maps.latestByChannel.delete(channelId);
|
||||
maps.observedUnreadEvents.delete(channelId);
|
||||
}
|
||||
return outcome;
|
||||
}
|
||||
|
||||
@@ -22,6 +22,7 @@ import {
|
||||
type ForcedUnreadMap,
|
||||
} from "@/features/channels/forcedUnreadStore";
|
||||
import {
|
||||
applyMarkAllReadTransition,
|
||||
applyOverrideRead,
|
||||
applyOverrideUnread,
|
||||
persistForcedUnread,
|
||||
@@ -172,6 +173,7 @@ export function useUnreadChannels(
|
||||
markChannelUnread: markChannelOverrideUnread,
|
||||
markChannelRead: markChannelOverrideRead,
|
||||
getOverrideLiveness,
|
||||
getProjection,
|
||||
} = useReadState(pubkey, relayClient);
|
||||
const overrideApis = React.useMemo<OverrideAPIs>(
|
||||
() => ({
|
||||
@@ -952,7 +954,6 @@ export function useUnreadChannels(
|
||||
|
||||
const markAllChannelsRead = React.useCallback(() => {
|
||||
for (const channelId of unreadChannelIdsRef.current) {
|
||||
// Frontier advance first, then C-bump; gate local clear on confirmed liveness.
|
||||
const unixSeconds =
|
||||
latestByChannelRef.current.get(channelId) ??
|
||||
getEffectiveTimestamp(channelId) ??
|
||||
@@ -960,11 +961,11 @@ export function useUnreadChannels(
|
||||
if (unixSeconds !== null) {
|
||||
markContextRead(channelId, unixSeconds);
|
||||
}
|
||||
if (applyOverrideRead(channelId, overrideApis) === "overrideCleared") {
|
||||
delete forcedUnreadRef.current[channelId];
|
||||
latestByChannelRef.current.delete(channelId);
|
||||
observedUnreadEventsByChannelRef.current.delete(channelId);
|
||||
}
|
||||
applyMarkAllReadTransition(channelId, overrideApis, {
|
||||
forcedUnread: forcedUnreadRef.current,
|
||||
latestByChannel: latestByChannelRef.current,
|
||||
observedUnreadEvents: observedUnreadEventsByChannelRef.current,
|
||||
});
|
||||
}
|
||||
if (pubkey) {
|
||||
forcedUnreadStore.write(pubkey, forcedUnreadRef.current);
|
||||
@@ -998,10 +999,8 @@ export function useUnreadChannels(
|
||||
markAllChannelsRead,
|
||||
markChannelRead,
|
||||
markChannelUnread,
|
||||
// Exposed so other surfaces (e.g. Home) can project per-item read state
|
||||
// off the same NIP-RS read marker without instantiating a second
|
||||
// ReadStateManager. readStateVersion is the invalidation signal callers
|
||||
// should include in memo deps.
|
||||
// Exposed for surfaces projecting per-item read state (Home) off the shared
|
||||
// NIP-RS marker without a second ReadStateManager; readStateVersion = invalidation signal.
|
||||
getEffectiveTimestamp,
|
||||
getOwnTimestamp,
|
||||
readStateVersion,
|
||||
@@ -1017,5 +1016,6 @@ export function useUnreadChannels(
|
||||
mutedRootIds: mutedRootIdsRef.current as ReadonlySet<string>,
|
||||
muteThread,
|
||||
unmuteThread,
|
||||
getProjection,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -1060,3 +1060,86 @@ test("fetchCommunityUnread adversarial-2: projection with frontier deactivates r
|
||||
|
||||
assert.deepEqual(result, { hasUnread: false, mentionCount: 0 });
|
||||
});
|
||||
|
||||
test("fetchCommunityUnread adversarial-5: stale_complete_projection_yieldsTo_fresher_fetched_tombstone", async () => {
|
||||
// CRITICAL 2 ordering fix: `loadComplete` establishes enumeration completeness,
|
||||
// NOT temporal ordering. A complete projection with a live register
|
||||
// (S=5,C=0,B=100,F=50) must yield to a fresher fetched tombstone (S=0,C=4,B=0)
|
||||
// for the same channel. Without the fetchedTombstoneChannels guard, the projection
|
||||
// overrides authoritative map (S=5,C=0,B=100) would produce hasUnread:true;
|
||||
// with the guard, the fetched tombstone wins and the rail stays dark.
|
||||
//
|
||||
// Wire: fetched event has ov_c:CHANNEL_ID=4 but no ov_s: → decoded as {s:0,c:4,b:0}.
|
||||
// fetchedTombstoneChannels includes CHANNEL_ID (s===0). Override liveness check is
|
||||
// skipped for that channel even though authoritative (projection) has s=5.
|
||||
const SLOT = "a".repeat(32); // valid 32-hex slot
|
||||
const relay = relayFor([
|
||||
// 1. member events
|
||||
() => [
|
||||
event({
|
||||
tags: [
|
||||
["d", CHANNEL_ID],
|
||||
["p", PUBKEY],
|
||||
],
|
||||
}),
|
||||
],
|
||||
// 2. metadata events
|
||||
() => [
|
||||
event({
|
||||
tags: [
|
||||
["d", CHANNEL_ID],
|
||||
["t", "stream"],
|
||||
],
|
||||
}),
|
||||
],
|
||||
// 3. visibility events
|
||||
() => [],
|
||||
// 4. read-state: tombstone wire format (ov_c present, ov_s absent → s=0,c=4,b=0)
|
||||
// Frontier F=50 is also present so the projection frontier is not strictly
|
||||
// newer — only the tombstone vs live-register ordering is under test.
|
||||
() => [
|
||||
event({
|
||||
pubkey: PUBKEY,
|
||||
created_at: 400,
|
||||
tags: [
|
||||
["d", `read-state:${SLOT}`],
|
||||
["t", "read-state"],
|
||||
],
|
||||
content: JSON.stringify({
|
||||
v: 1,
|
||||
client_id: "client",
|
||||
contexts: {
|
||||
[CHANNEL_ID]: 50,
|
||||
[`ov_c:${CHANNEL_ID}`]: 4,
|
||||
// ov_s: intentionally absent → tombstone (s=0)
|
||||
},
|
||||
}),
|
||||
}),
|
||||
],
|
||||
// 5. mutes
|
||||
() => [],
|
||||
// 6. unread events — none
|
||||
() => [],
|
||||
// 7. mention events — none
|
||||
() => [],
|
||||
]);
|
||||
|
||||
// Complete projection: live register S=5,C=0,B=100 and frontier F=50.
|
||||
// F=50 ≤ B=100, S=5 > C=0 → isOverrideActive would return true WITHOUT the guard.
|
||||
const overrides = new Map([[CHANNEL_ID, { s: 5, c: 0, b: 100 }]]);
|
||||
const frontiers = new Map([[CHANNEL_ID, 50]]);
|
||||
|
||||
const result = await fetchCommunityUnread({
|
||||
client: relay,
|
||||
pubkey: PUBKEY,
|
||||
nowSeconds: 500,
|
||||
decryptReadState: async (v) => v,
|
||||
decryptMutes: async (v) => v,
|
||||
readThreadRelationships: readRelationships(),
|
||||
getProjection: () => ({ loadComplete: true, frontiers, overrides }),
|
||||
});
|
||||
|
||||
// Fetched tombstone (s=0) is authoritative; projection's stale live register must not
|
||||
// resurrect unread when the relay has confirmed a clear.
|
||||
assert.deepEqual(result, { hasUnread: false, mentionCount: 0 });
|
||||
});
|
||||
|
||||
@@ -145,9 +145,10 @@ export async function fetchObservedChannels(
|
||||
export async function pollCommunityUnread(
|
||||
community: Community,
|
||||
pubkey: string,
|
||||
getProjection?: () => ReadStateProjection | null,
|
||||
): Promise<CommunityUnreadObserverResult> {
|
||||
return withReadOnlyRelayClient(community.relayUrl, (client) =>
|
||||
fetchCommunityUnread({ client, pubkey }),
|
||||
fetchCommunityUnread({ client, pubkey, getProjection }),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -220,6 +221,21 @@ export async function fetchCommunityUnread(args: {
|
||||
const projection = args.getProjection?.() ?? null;
|
||||
const completeProjection = projection?.loadComplete ? projection : null;
|
||||
|
||||
// Override liveness resolution: projection wins when complete, BUT a fetched
|
||||
// wire tombstone (s===0, c>0) for the same coordinate is treated as
|
||||
// authoritative — it proves the relay consensus has a clear with no
|
||||
// countering mark-unread, and the projection's live S counter may be from
|
||||
// an uncommitted/unpublished local action that the relay hasn't confirmed.
|
||||
//
|
||||
// fetchedTombstoneChannels is the set of channel IDs whose fetched register
|
||||
// has s===0 (tombstone wire format). We skip projection liveness for those.
|
||||
const fetchedTombstoneChannels = new Set<string>();
|
||||
if (completeProjection !== null) {
|
||||
for (const [ctx, reg] of readState.overrides) {
|
||||
if (reg.s === 0) fetchedTombstoneChannels.add(ctx);
|
||||
}
|
||||
}
|
||||
|
||||
const authoritative: ReadonlyMap<string, OverrideRegister> =
|
||||
completeProjection !== null
|
||||
? completeProjection.overrides
|
||||
@@ -279,9 +295,15 @@ export async function fetchCommunityUnread(args: {
|
||||
// Override liveness: evaluate the authoritative register against the
|
||||
// effective frontier. Uses projection.overrides when complete (no
|
||||
// per-field max with fetched); fetched-deduped otherwise.
|
||||
// Skip projection liveness when fetched relay has a wire tombstone (s=0)
|
||||
// for this channel — a fresher relay tombstone is authoritative.
|
||||
if (!hasUnread) {
|
||||
const reg = authoritative.get(channel.id);
|
||||
if (reg !== undefined && isOverrideActive(reg, readAt ?? 0)) {
|
||||
if (
|
||||
reg !== undefined &&
|
||||
!fetchedTombstoneChannels.has(channel.id) &&
|
||||
isOverrideActive(reg, readAt ?? 0)
|
||||
) {
|
||||
hasUnread = true;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ import * as React from "react";
|
||||
import { getIdentity } from "@/shared/api/tauriIdentity";
|
||||
import { markCommunityRead } from "@/features/communities/communityMarkRead";
|
||||
import { pollCommunityUnread } from "@/features/communities/communityUnreadObserver";
|
||||
import { useAppShell } from "@/app/AppShellContext";
|
||||
|
||||
import type { Community } from "./types";
|
||||
|
||||
@@ -54,6 +55,7 @@ export function useCommunityUnread(
|
||||
unreadByCommunity: Record<string, CommunityUnreadState>;
|
||||
markCommunityRead: (communityId: string) => Promise<void>;
|
||||
} {
|
||||
const { getProjection } = useAppShell();
|
||||
const [unreadByCommunity, setUnreadByCommunity] = React.useState<
|
||||
Record<string, CommunityUnreadState>
|
||||
>(() => seedCommunityStates(communities, {}));
|
||||
@@ -131,7 +133,11 @@ export function useCommunityUnread(
|
||||
if (cancelled) return;
|
||||
markLoading(community.id);
|
||||
try {
|
||||
const result = await pollCommunityUnread(community, pubkey);
|
||||
const result = await pollCommunityUnread(
|
||||
community,
|
||||
pubkey,
|
||||
getProjection,
|
||||
);
|
||||
if (cancelled) return;
|
||||
markReady(community.id, result);
|
||||
} catch (error) {
|
||||
@@ -155,7 +161,7 @@ export function useCommunityUnread(
|
||||
window.clearTimeout(pollTimer);
|
||||
}
|
||||
};
|
||||
}, [activeCommunityId, communities]);
|
||||
}, [activeCommunityId, communities, getProjection]);
|
||||
|
||||
const communitiesRef = React.useRef(communities);
|
||||
communitiesRef.current = communities;
|
||||
|
||||
Reference in New Issue
Block a user