Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions desktop/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export default defineConfig({
"**/tokens.spec.ts",
"**/persona-env-vars.spec.ts",
"**/mesh-compute.spec.ts",
"**/reaction-coldload-repro.spec.ts",
],
use: {
...devices["Desktop Chrome"],
Expand Down
1 change: 1 addition & 0 deletions desktop/src/features/channels/ui/ChannelScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ export function ChannelScreen({
handleSelectThreadReplyTarget,
handleToggleReaction,
} = useChannelPaneHandlers({
channelId: activeChannelId,
deleteMessageMutation,
editMessageMutation,
editTargetId,
Expand Down
6 changes: 6 additions & 0 deletions desktop/src/features/channels/useChannelPaneHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type {
* rather than listing the whole mutation as a dependency.
*/
export function useChannelPaneHandlers({
channelId,
deleteMessageMutation,
editMessageMutation,
editTargetId,
Expand All @@ -37,6 +38,7 @@ export function useChannelPaneHandlers({
threadReplyTargetId,
toggleReactionMutation,
}: {
channelId: string | null;
deleteMessageMutation: ReturnType<typeof useDeleteMessageMutation>;
editMessageMutation: ReturnType<typeof useEditMessageMutation>;
editTargetId: string | null;
Expand Down Expand Up @@ -83,6 +85,9 @@ export function useChannelPaneHandlers({
const toggleMutateRef = React.useRef(toggleReactionMutation.mutateAsync);
toggleMutateRef.current = toggleReactionMutation.mutateAsync;

const channelIdRef = React.useRef(channelId);
channelIdRef.current = channelId;

const handleCancelThreadReply = React.useCallback(() => {
setThreadReplyTargetId(openThreadHeadIdRef.current);
}, [setThreadReplyTargetId]);
Expand Down Expand Up @@ -290,6 +295,7 @@ export function useChannelPaneHandlers({
emoji,
eventId: message.id,
remove,
channelId: channelIdRef.current ?? undefined,
});
},
[],
Expand Down
1 change: 1 addition & 0 deletions desktop/src/features/home/ui/HomeView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ export function HomeView({
emoji,
eventId: message.id,
remove,
channelId: selectedChannel?.id,
});
await channelMessagesQuery.refetch();
onRefresh();
Expand Down
96 changes: 91 additions & 5 deletions desktop/src/features/messages/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,19 @@ import type { Channel, Identity, RelayEvent } from "@/shared/api/types";
// from the on-render overlay.
import { applyEditTagOverlay } from "@/features/messages/lib/applyEditTagOverlay.mjs";
import { backfillAuxForMessages } from "@/features/messages/lib/auxBackfill";
import { createOptimisticReaction } from "@/features/messages/lib/optimisticReaction";
import { countTopLevelTimelineRows } from "@/features/messages/lib/formatTimelineMessages";
import {
MIN_TOP_LEVEL_ROWS_PER_FETCH,
pageOlderMessagesUntilRowFloor,
} from "@/features/messages/lib/pageOlderMessages";
import { isDuplicateReactionError } from "@/features/pulse/lib/noteActions";
import {
KIND_REACTION,
KIND_STREAM_MESSAGE,
KIND_SYSTEM_MESSAGE,
} from "@/shared/constants/kinds";
import { useIdentityQuery } from "@/shared/api/hooks";

type MessageQueryContext = {
optimisticId: string;
Expand Down Expand Up @@ -491,29 +495,111 @@ export function useSendMessageMutation(

export function useToggleReactionMutation() {
const queryClient = useQueryClient();
const identityQuery = useIdentityQuery();
return useMutation<
void,
Error,
{
eventId: string;
emoji: string;
remove: boolean;
}
// The channel whose timeline cache to optimistically update. Omit for
// surfaces that own their own reaction cache (e.g. Pulse notes), so they
// are not double-counted in two caches.
channelId?: string;
},
| {
queryKey: ReturnType<typeof channelMessagesKey>;
previous: RelayEvent[];
}
| undefined
>({
// Reactions never round-trip back to the client (kind:7 carries only an
// `e` tag, so the `#h`-scoped live subscription misses them — they only
// arrive via the cold-load `#e` aux backfill). So apply the change to the
// channel cache ourselves, optimistically, or the reactor's own reaction
// stays invisible until the next channel switch. Mirrors the apply-on-
// success cache writes in the edit/delete mutations below.
onMutate: async ({ eventId, emoji, remove, channelId }) => {
const pubkey = identityQuery.data?.pubkey;
if (!channelId || !pubkey) {
return undefined;
}

const queryKey = channelMessagesKey(channelId);
// Cancel any in-flight channel fetch so its post-write setQueryData
// can't clobber the optimistic reaction with a pre-write snapshot
// (matches the send-message onMutate below).
await queryClient.cancelQueries({ queryKey });
const previous = queryClient.getQueryData<RelayEvent[]>(queryKey) ?? [];
const pubkeyLower = pubkey.toLowerCase();
const trimmedEmoji = emoji.trim();

const isOwnReactionForTarget = (event: RelayEvent) =>
event.kind === KIND_REACTION &&
event.pubkey.toLowerCase() === pubkeyLower &&
event.content.trim() === trimmedEmoji &&
event.tags.some((tag) => tag[0] === "e" && tag[1] === eventId);

if (remove) {
queryClient.setQueryData<RelayEvent[]>(queryKey, (current = []) =>
current.filter((event) => !isOwnReactionForTarget(event)),
);
} else {
// Custom-emoji reaction: emoji is `:shortcode:`. Resolve its image URL
// from the cached workspace palette so the optimistic kind:7 renders
// the right glyph. Unicode reactions resolve to no URL.
const emojiUrl = reactionEmojiUrl(
emoji,
queryClient.getQueryData<CustomEmoji[]>(customEmojiQueryKey),
);
const optimistic = createOptimisticReaction(
eventId,
emoji,
emojiUrl,
pubkey,
);
queryClient.setQueryData<RelayEvent[]>(queryKey, (current = []) =>
sortMessages([...current, optimistic]),
);
}

return { queryKey, previous };
},
mutationFn: async ({ eventId, emoji, remove }) => {
if (remove) {
await removeReaction(eventId, emoji);
return;
}

// Custom-emoji reaction: emoji is `:shortcode:`. Resolve its image URL
// from the cached workspace palette so the kind:7 carries the NIP-30
// `["emoji", shortcode, url]` tag. Unicode reactions resolve to no URL.
const emojiUrl = reactionEmojiUrl(
emoji,
queryClient.getQueryData<CustomEmoji[]>(customEmojiQueryKey),
);
await addReaction(eventId, emoji, emojiUrl);
try {
await addReaction(eventId, emoji, emojiUrl);
} catch (error) {
// The relay already has this reaction (its `e`-only kind:7 never came
// back over the `#h` live sub, so the cache didn't render it and the
// user clicked again — Tyler's exact repro). That's idempotent
// success: swallow it so onError doesn't roll back the optimistic
// write and leave us back at "duplicate but nothing visible".
if (!isDuplicateReactionError(error)) {
throw error;
}
}
},
onError: (_error, _variables, context) => {
if (context) {
// Rollback restores the pre-click snapshot. Narrow accepted edge: if a
// backfill landed a real event between onMutate and onError, this drops
// it too — but that needs a relay error racing an in-flight backfill,
// and the next backfill self-heals it. Not worth guarding.
queryClient.setQueryData<RelayEvent[]>(
context.queryKey,
context.previous,
);
}
},
});
}
Expand Down
169 changes: 169 additions & 0 deletions desktop/src/features/messages/lib/auxBackfill.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@ import assert from "node:assert/strict";
import test from "node:test";

import {
backfillAuxForMessages,
collectAuxEventIdsForDeletionBackfill,
collectMessageIdsForAuxBackfill,
mergeAuxEventsWithDeletionBackfill,
} from "./auxBackfill.ts";
import { formatTimelineMessages } from "./formatTimelineMessages.ts";
import { buildChannelReactionAuxFilter } from "@/shared/api/relayChannelFilters.ts";
import { channelMessagesKey } from "./messageQueryKeys.ts";

const CHANNEL_ID = "36411e44-0e2d-4cfe-bd6e-567eb169db9f";

Expand Down Expand Up @@ -126,3 +130,168 @@ test("merges deletion markers that target cached or fetched auxiliary event ids"
[fetchedReactionId, cachedReactionDeletionId, fetchedReactionDeletionId],
);
});

// Regression for the "duplicate: reaction already exists" report: a reaction
// older than the content-kinds history window never rendered, because the old
// single-filter history fetch (all CHANNEL_EVENT_KINDS under one `limit`) let
// reactions/deletions evict older reactions from the window. The fix (#1153)
// fetches history with content kinds only and backfills reactions by `#e`
// reference over the loaded message ids — recency-independent. This test pins
// that path end-to-end: a loaded message whose only reaction is NOT in the
// history window still renders that reaction after the `#e` backfill.
//
// Would fail pre-#1153: the reaction was never fetched, so the message
// rendered with no reactions and re-reacting hit the relay's duplicate guard.
test("old reaction outside the history window is backfilled by #e and renders", async () => {
const messageId = hex("1");
const reactionId = hex("2");
const currentUser = hex("c");

// The cold-load history window: the message, but NOT its (older) reaction.
const history = [
event(messageId, 9, {
pubkey: hex("a"),
content: "ship it?",
created_at: 1_700_001_000,
}),
];

// Step 1: backfill keys off the loaded content message ids.
const messageIds = collectMessageIdsForAuxBackfill(history);
assert.deepEqual(messageIds, [messageId]);

// Step 2: the reaction aux filter references those ids by `#e`, with no time
// window — so an old reaction is reachable regardless of when it was created.
const auxFilter = buildChannelReactionAuxFilter(CHANNEL_ID, messageIds);
assert.deepEqual(auxFilter["#e"], [messageId]);
assert.equal("since" in auxFilter, false);
assert.equal("until" in auxFilter, false);

// Step 3: the relay returns the old reaction for that `#e` filter. Its
// created_at predates the loaded message — the exact case the old window
// dropped.
const oldReaction = event(reactionId, 7, {
pubkey: currentUser,
content: "✅",
created_at: 1_700_000_000,
tags: [["e", messageId]],
});

const merged = await mergeAuxEventsWithDeletionBackfill({
channelId: CHANNEL_ID,
cachedEvents: history,
fetchedAuxEvents: [oldReaction],
// No deletions target the reaction.
fetchAuxEventsForMessages: async () => [],
});
assert.deepEqual(
merged.map((e) => e.id),
[reactionId],
);

// Step 4: the reaction merged into the timeline renders on its target
// message, attributed to the current user (so the UI shows it as already
// reacted and won't prompt a duplicate add).
const timeline = formatTimelineMessages(
[...history, ...merged],
null,
currentUser,
null,
);
const row = timeline.find((m) => m.id === messageId);
assert.ok(row, "loaded message should render a timeline row");
assert.deepEqual(
row.reactions?.map((r) => ({
emoji: r.emoji,
count: r.count,
mine: r.reactedByCurrentUser,
})),
[{ emoji: "✅", count: 1, mine: true }],
);
});

// Minimal in-memory stand-in for the React-Query client: only the two methods
// backfillAuxForMessages touches. `setQueryData` mirrors React-Query's updater
// contract (receives current value, defaulted to [] by the caller).
function makeQueryClientStub() {
const store = new Map();
return {
getQueryData(key) {
return store.get(JSON.stringify(key));
},
setQueryData(key, updater) {
const k = JSON.stringify(key);
const next =
typeof updater === "function" ? updater(store.get(k) ?? []) : updater;
store.set(k, next);
return next;
},
};
}

function reactionAux(id, messageId, emoji = "✅") {
return event(id, 7, {
pubkey: hex("c"),
content: emoji,
tags: [["e", messageId]],
});
}

// The whole point of the kind-split: a slow/failed structural (kind:5/9005/
// 40003) fetch must NOT blank reactions. Reactions are committed first on their
// own REQ; the structural overlay's failure is caught and logged, leaving the
// reactions in cache. Pre-fix, both rode one bundled REQ under one try/catch,
// so a structural timeout dropped every reaction in the view.
test("structural-overlay failure does not strand already-committed reactions", async () => {
const messageId = hex("1");
const queryClient = makeQueryClientStub();
// Seed the content message into cache, as the cold-load history fetch would.
queryClient.setQueryData(channelMessagesKey(CHANNEL_ID), [
event(messageId, 9, { pubkey: hex("a"), content: "ship it?" }),
]);

await backfillAuxForMessages(queryClient, CHANNEL_ID, [event(messageId, 9)], {
fetchReactionAuxEventsForMessages: async () => [
reactionAux(hex("2"), messageId),
],
// The slow half blows up — exactly the cold-load kind:5 timeout.
fetchStructuralAuxEventsForMessages: async () => {
throw new Error("Timed out while loading channel history.");
},
fetchAuxDeletionEventsForAuxEvents: async () => [],
});

const cached = queryClient.getQueryData(channelMessagesKey(CHANNEL_ID));
assert.ok(
cached.some((e) => e.id === hex("2") && e.kind === 7),
"reaction must survive a structural-overlay fetch failure",
);
});

// Symmetric guarantee: a reaction-fetch failure must not abort the structural
// overlay. Each half owns its try/catch, so an edit/deletion still applies even
// if reactions couldn't be fetched this pass (they self-heal next backfill).
test("reaction-fetch failure does not block the structural overlay", async () => {
const messageId = hex("1");
const editId = hex("3");
const queryClient = makeQueryClientStub();
queryClient.setQueryData(channelMessagesKey(CHANNEL_ID), [
event(messageId, 9, { pubkey: hex("a"), content: "original" }),
]);

await backfillAuxForMessages(queryClient, CHANNEL_ID, [event(messageId, 9)], {
fetchReactionAuxEventsForMessages: async () => {
throw new Error("Timed out while loading channel history.");
},
fetchStructuralAuxEventsForMessages: async () => [
event(editId, 40003, { tags: [["e", messageId]] }),
],
fetchAuxDeletionEventsForAuxEvents: async () => [],
});

const cached = queryClient.getQueryData(channelMessagesKey(CHANNEL_ID));
assert.ok(
cached.some((e) => e.id === editId && e.kind === 40003),
"edit must apply even when the reaction fetch failed",
);
});
Loading
Loading