diff --git a/.changeset/fix_duplicate_reactions_and_pending_event_redaction.md b/.changeset/fix_duplicate_reactions_and_pending_event_redaction.md new file mode 100644 index 0000000000..0ef15c0db7 --- /dev/null +++ b/.changeset/fix_duplicate_reactions_and_pending_event_redaction.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +# Fix duplicate reactions and deleting a message that is still sending diff --git a/src/app/components/message/modals/MessageDelete.tsx b/src/app/components/message/modals/MessageDelete.tsx index cda942786b..d9496017ce 100644 --- a/src/app/components/message/modals/MessageDelete.tsx +++ b/src/app/components/message/modals/MessageDelete.tsx @@ -21,6 +21,7 @@ import { AsyncStatus, useAsyncCallback } from '$hooks/useAsyncCallback'; import { modalAtom, ModalType } from '$state/modal'; import * as css from '$features/room/message/styles.css'; import { createDebugLogger } from '$utils/debugLogger'; +import { redactEvent } from '$utils/room/redaction'; import * as Sentry from '@sentry/react'; const debugLog = createDebugLogger('MessageDelete'); @@ -72,9 +73,8 @@ export function MessageDeleteInternal({ room, mEvent, onClose }: MessageDeleteIn const [deleteState, deleteMessage] = useAsyncCallback( useCallback( - (eventId: string, reason?: string) => - mx.redactEvent(room.roomId, eventId, undefined, reason ? { reason } : undefined), - [mx, room] + (reason?: string) => redactEvent(mx, room, mEvent, reason ? { reason } : undefined), + [mx, room, mEvent] ) ); @@ -106,7 +106,7 @@ export function MessageDeleteInternal({ room, mEvent, onClose }: MessageDeleteIn debugLog.info('ui', 'Deleting message', { eventId, hasReason: !!reason }); Sentry.metrics.count('sable.message.delete.attempt', 1); - deleteMessage(eventId, reason); + deleteMessage(reason); }; return ( diff --git a/src/app/features/room/RoomInput.tsx b/src/app/features/room/RoomInput.tsx index d081c7e229..6fa22b330a 100644 --- a/src/app/features/room/RoomInput.tsx +++ b/src/app/features/room/RoomInput.tsx @@ -81,13 +81,8 @@ import type { GifData } from '$components/emoji-board'; import { EmojiBoard, EmojiBoardTab } from '$components/emoji-board'; import { UseStateProvider } from '$components/UseStateProvider'; import type { TUploadContent } from '$utils/matrix'; -import { - cancelUploadContent, - encryptFile, - getImageInfo, - mxcUrlToHttp, - toggleReaction, -} from '$utils/matrix'; +import { cancelUploadContent, encryptFile, getImageInfo, mxcUrlToHttp } from '$utils/matrix'; +import { toggleReaction } from '$utils/room/reactions'; import { useTypingStatusUpdater } from '$hooks/useTypingStatusUpdater'; import { useFilePicker } from '$hooks/useFilePicker'; import { useFilePasteHandler } from '$hooks/useFilePasteHandler'; @@ -112,6 +107,7 @@ import type { EditorButtonId } from '$state/settings'; import { settingsAtom } from '$state/settings'; import { matchesShortcut } from '../../keyboard/shortcuts'; import { getEditedEvent, getMentionContent, getThreadReplyEvents } from '$utils/room/relations'; +import { isLocalEventId, waitForRemoteEventId } from '$utils/room/redaction'; import { buildReplacementContent } from './buildReplacementContent'; import { htmlToMarkdown } from '$plugins/markdown'; import { Command, SHRUG, TABLEFLIP, UNFLIP, useCommands } from '$hooks/useCommands'; @@ -1039,7 +1035,9 @@ export const RoomInput = forwardRef( const lastMessageId = lastMessage?.getId(); if (lastMessageId) { - toggleReaction(mx, room, lastMessageId, key, shortcode); + toggleReaction(mx, room, lastMessageId, key, shortcode).catch((err: unknown) => { + debugLog.error('ui', 'Reaction toggle failed', { eventId: lastMessageId, key, err }); + }); } } @@ -1067,9 +1065,14 @@ export const RoomInput = forwardRef( ); const oldContent = editingEvent.getContent(); const currentContent = getEditingContent(editingEvent); - const eventId = editingEvent.getId(); + let eventId = editingEvent.getId(); if (!eventId) return; + if (isLocalEventId(eventId)) { + eventId = await waitForRemoteEventId(editingEvent); + if (!eventId) return; + } + const rawPmp = currentContent['com.beeper.per_message_profile'] ?? oldContent['com.beeper.per_message_profile']; diff --git a/src/app/features/room/RoomTimeline.tsx b/src/app/features/room/RoomTimeline.tsx index 378d2fb733..863248469f 100644 --- a/src/app/features/room/RoomTimeline.tsx +++ b/src/app/features/room/RoomTimeline.tsx @@ -1135,6 +1135,7 @@ export function RoomTimeline({ const processedEvents = useProcessedTimeline({ items: vListIndices, linkedTimelines: timelineSync.timeline.linkedTimelines, + pendingEvents: timelineSync.pendingEvents, ignoredUsersSet, hiddenEvents, mxUserId: mx.getUserId(), diff --git a/src/app/features/room/message/MessageEditor.tsx b/src/app/features/room/message/MessageEditor.tsx index 7829440f4a..c4454a9f67 100644 --- a/src/app/features/room/message/MessageEditor.tsx +++ b/src/app/features/room/message/MessageEditor.tsx @@ -62,6 +62,7 @@ import { useMatrixClient } from '$hooks/useMatrixClient'; import { useDismissOnBack } from '$utils/androidBack'; import { nicknamesAtom } from '$state/nicknames'; import { getEditedEvent, getMentionContent } from '$utils/room/relations'; +import { isLocalEventId, waitForRemoteEventId } from '$utils/room/redaction'; import { trimReplyFromFormattedBody } from '$utils/room/display'; import { buildReplacementContent } from '../buildReplacementContent'; import { isMobileOrTablet } from '$utils/platform'; @@ -245,9 +246,14 @@ export const MessageEditor = as<'div', MessageEditorProps>( const [prevBody, prevCustomHtml, prevMentions] = getPrevBodyAndFormattedBody(); if (plainText === '') return undefined; - const eventId = mEvent.getId(); + let eventId = mEvent.getId(); if (!eventId) return undefined; + if (isLocalEventId(eventId)) { + eventId = await waitForRemoteEventId(mEvent); + if (!eventId) return undefined; + } + if (prevBody) { if (prevCustomHtml && trimReplyFromFormattedBody(prevCustomHtml) === customHtml) { return undefined; diff --git a/src/app/features/room/message/Reactions.tsx b/src/app/features/room/message/Reactions.tsx index 8a56749173..571d891384 100644 --- a/src/app/features/room/message/Reactions.tsx +++ b/src/app/features/room/message/Reactions.tsx @@ -28,6 +28,7 @@ import { useRelations } from '$hooks/useRelations'; import { stopPropagation } from '$utils/keyboard'; import { useMediaAuthentication } from '$hooks/useMediaAuthentication'; import { useDismissOnBack } from '$utils/androidBack'; +import { dedupeAnnotationsBySender } from '$utils/room/reactions'; import { ReactionViewer } from '$features/room/reaction-viewer'; import * as css from './styles.css'; @@ -90,7 +91,7 @@ export const Reactions = as<'div', ReactionsProps>( ref={ref} > {reactions.map(([key, events]) => { - const rEvents = Array.from(events); + const rEvents = dedupeAnnotationsBySender(events); if (rEvents.length === 0 || typeof key !== 'string') return null; const myREvent = myUserId ? rEvents.find(factoryEventSentBy(myUserId)) : undefined; const isPressed = !!myREvent?.getRelation(); @@ -116,7 +117,7 @@ export const Reactions = as<'div', ReactionsProps>( key={key} mx={mx} reaction={key} - count={events.size} + count={rEvents.length} onClick={canToggle ? () => onReactionToggle(mEventId, key) : undefined} onContextMenu={handleViewReaction} onTouchStart={(evt) => evt.stopPropagation()} diff --git a/src/app/features/room/reaction-viewer/ReactionViewer.tsx b/src/app/features/room/reaction-viewer/ReactionViewer.tsx index 93e3dcf7aa..4c8d672b3f 100644 --- a/src/app/features/room/reaction-viewer/ReactionViewer.tsx +++ b/src/app/features/room/reaction-viewer/ReactionViewer.tsx @@ -16,6 +16,7 @@ import { useMediaAuthentication } from '$hooks/useMediaAuthentication'; import { useOpenUserRoomProfile } from '$state/hooks/userRoomProfile'; import { useSpaceOptionally } from '$hooks/useSpace'; import { getMouseEventCords } from '$utils/dom'; +import { dedupeAnnotationsBySender } from '$utils/room/reactions'; import * as css from './ReactionViewer.css'; type ReactionViewerProps = { @@ -50,7 +51,7 @@ export const ReactionViewer = as<'div', ReactionViewerProps>( const getReactionsForKey = (key: string): MatrixEvent[] => { const reactSet = reactions.find(([k]) => k === key)?.[1]; if (!reactSet) return []; - return Array.from(reactSet); + return dedupeAnnotationsBySender(reactSet); }; const selectedReactions = getReactionsForKey(selectedKey); diff --git a/src/app/hooks/timeline/useProcessedTimeline.test.tsx b/src/app/hooks/timeline/useProcessedTimeline.test.tsx index aa636988a6..8e84c805b3 100644 --- a/src/app/hooks/timeline/useProcessedTimeline.test.tsx +++ b/src/app/hooks/timeline/useProcessedTimeline.test.tsx @@ -140,12 +140,14 @@ function createTimeline(events: MatrixEvent[]): EventTimeline { function processTimeline( events: MatrixEvent[], - readUptoEventId: string | undefined + readUptoEventId: string | undefined, + pendingEvents: MatrixEvent[] = [] ): ProcessedEvent[] { const { result } = renderHook(() => useProcessedTimeline({ - items: events.map((_, i) => i), + items: [...events, ...pendingEvents].map((_, i) => i), linkedTimelines: [createTimeline(events)], + pendingEvents, ignoredUsersSet: new Set(), hiddenEvents, mxUserId: MY_USER, @@ -163,6 +165,16 @@ const renderedIds = (processed: ProcessedEvent[]) => processed.map((e) => e.id); const dividerIds = (processed: ProcessedEvent[]) => processed.filter((e) => e.willRenderNewDivider).map((e) => e.id); +describe('useProcessedTimeline pending events', () => { + it('appends detached pending events after the live timeline', () => { + const processed = processTimeline([createEvent({ id: '$sent' })], undefined, [ + createEvent({ id: '~pending', sender: MY_USER }), + ]); + + expect(renderedIds(processed)).toEqual(['$sent', '~pending']); + }); +}); + describe('useProcessedTimeline new-messages divider', () => { it('renders an event that is still encrypted', () => { const processed = processTimeline( diff --git a/src/app/hooks/timeline/useProcessedTimeline.ts b/src/app/hooks/timeline/useProcessedTimeline.ts index 8b8689e47f..44cbbab9d8 100644 --- a/src/app/hooks/timeline/useProcessedTimeline.ts +++ b/src/app/hooks/timeline/useProcessedTimeline.ts @@ -21,6 +21,7 @@ import { M_POLL_START } from 'matrix-js-sdk'; export interface UseProcessedTimelineOptions { items: number[]; linkedTimelines: EventTimeline[]; + pendingEvents?: MatrixEvent[]; ignoredUsersSet: Set; hiddenEvents: ResolvedHiddenEventSettings; mxUserId: string | null; @@ -112,12 +113,17 @@ type TimelineEventEntry = { timelineSet: EventTimelineSet; }; -const flattenTimelineEvents = (linkedTimelines: EventTimeline[]): TimelineEventEntry[] => { +const flattenTimelineEvents = ( + linkedTimelines: EventTimeline[], + pendingEvents: MatrixEvent[] +): TimelineEventEntry[] => { const entries: TimelineEventEntry[] = []; linkedTimelines.forEach((timeline) => { const timelineSet = timeline.getTimelineSet(); timeline.getEvents().forEach((mEvent) => entries.push({ mEvent, timelineSet })); }); + const timelineSet = linkedTimelines.at(-1)?.getTimelineSet(); + if (timelineSet) pendingEvents.forEach((mEvent) => entries.push({ mEvent, timelineSet })); return entries; }; @@ -569,6 +575,7 @@ type ProcessingCache = { export function useProcessedTimeline({ items, linkedTimelines, + pendingEvents = [], ignoredUsersSet, hiddenEvents, mxUserId, @@ -592,7 +599,7 @@ export function useProcessedTimeline({ const cacheRef = useRef(); return useMemo(() => { - const timelineEvents = flattenTimelineEvents(linkedTimelines); + const timelineEvents = flattenTimelineEvents(linkedTimelines, pendingEvents); const processingOptions: TimelineProcessingOptions = { ignoredUsersSet, showHiddenEvents, @@ -711,6 +718,7 @@ export function useProcessedTimeline({ }, [ items, linkedTimelines, + pendingEvents, ignoredUsersSet, showHiddenEvents, showTombstoneEvents, diff --git a/src/app/hooks/timeline/useTimelineActions.ts b/src/app/hooks/timeline/useTimelineActions.ts index b68a71f71f..a5bdb0399a 100644 --- a/src/app/hooks/timeline/useTimelineActions.ts +++ b/src/app/hooks/timeline/useTimelineActions.ts @@ -6,12 +6,16 @@ import { EventStatus, RelationType } from '$types/matrix-sdk'; import type { Editor } from 'slate'; import { ReactEditor } from 'slate-react'; -import { getMxIdLocalPart, toggleReaction } from '$utils/matrix'; +import { getMxIdLocalPart } from '$utils/matrix'; +import { toggleReaction } from '$utils/room/reactions'; import { getMemberDisplayName } from '$utils/room/display'; import { extractReplyDraftBody, resolveReplyDraftTarget } from '$utils/room/relations'; import { createMentionElement, moveCursor } from '$components/editor'; +import { createDebugLogger } from '$utils/debugLogger'; import * as prefix from '$unstable/prefixes'; +const debugLog = createDebugLogger('TimelineActions'); + /** * The profile popup reads name, avatar and the identity fields off the room * member, so the cached copies would shadow fresher state event data. @@ -205,14 +209,11 @@ export function useTimelineActions({ const handleReactionToggle = useCallback( (targetEventId: string, key: string, shortcode?: string) => { - // Thread reactions live in the thread's own timeline set; without it the - // existing reaction is never found and un-reacting sends a second one. - const threadTimelineSet = threadRootId - ? room.getThread(threadRootId)?.timelineSet - : undefined; - toggleReaction(mx, room, targetEventId, key, shortcode, threadTimelineSet); + toggleReaction(mx, room, targetEventId, key, shortcode).catch((err: unknown) => { + debugLog.error('ui', 'Reaction toggle failed', { targetEventId, key, err }); + }); }, - [mx, room, threadRootId] + [mx, room] ); const handleResend = useCallback( diff --git a/src/app/hooks/timeline/useTimelineSync.ts b/src/app/hooks/timeline/useTimelineSync.ts index b7f1475a93..010f477535 100644 --- a/src/app/hooks/timeline/useTimelineSync.ts +++ b/src/app/hooks/timeline/useTimelineSync.ts @@ -451,7 +451,11 @@ export function useTimelineSync({ const resetAutoScrollPendingRef = useRef(false); const pendingAutoScrollBehaviorRef = useRef<'instant' | 'smooth' | undefined>(undefined); - const eventsLength = getTimelinesEventsCount(timeline.linkedTimelines); + let pendingEvents: MatrixEvent[] = []; + try { + pendingEvents = room.getPendingEvents(); + } catch {} + const eventsLength = getTimelinesEventsCount(timeline.linkedTimelines) + pendingEvents.length; const liveTimelineLinked = timeline.linkedTimelines.at(-1) === getLiveTimeline(room); const canPaginateBack = @@ -672,6 +676,7 @@ export function useTimelineSync({ timeline, setTimeline, eventsLength, + pendingEvents, liveTimelineLinked, canPaginateBack, backwardStatus, diff --git a/src/app/utils/matrix.ts b/src/app/utils/matrix.ts index 09e0f9f983..6505a37e14 100644 --- a/src/app/utils/matrix.ts +++ b/src/app/utils/matrix.ts @@ -3,12 +3,10 @@ import { decryptAttachment } from 'browser-encrypt-attachment'; import { Channel, invoke, isTauri } from '@tauri-apps/api/core'; import type { AccountDataEvents, - EventTimelineSet, MatrixClient, MatrixEvent, Room, RoomMember, - TimelineEvents, UploadProgress, UploadResponse, } from '$types/matrix-sdk'; @@ -25,9 +23,7 @@ import type { IImageInfo, IThumbnailContent, IVideoInfo } from '$types/matrix/co import * as Sentry from '@sentry/react'; import { encryptBlobInWorker } from '$utils/mediaWorker'; import { encryptAttachmentStreaming } from '$utils/attachmentCrypto'; -import { getEventReactions } from './room/relations'; import { getStateEvent } from './room/hierarchy'; -import { getReactionContent } from './messageReaction'; import { matchMxId, validMxId } from './mxIdHelper'; export { mxcUrlToHttp, rewriteAuthenticatedMediaUrl } from './mediaUrl'; @@ -526,41 +522,3 @@ export const rateLimitedActions = async ( } } }; - -export const toggleReaction = ( - mx: MatrixClient, - room: Room, - targetEventId: string, - key: string, - shortcode?: string, - timelineSet?: EventTimelineSet -) => { - const relations = getEventReactions( - timelineSet ?? room.getUnfilteredTimelineSet(), - targetEventId - ); - const allReactions = relations?.getSortedAnnotationsByKey() ?? []; - const [, reactionsSet] = allReactions.find(([k]) => k === key) ?? []; - const reactions: MatrixEvent[] = reactionsSet ? Array.from(reactionsSet) : []; - const myReaction = reactions.find(factoryEventSentBy(mx.getUserId()!)); - - if (myReaction && myReaction.isRelation?.()) { - const eventId = myReaction.getId(); - if (eventId) mx.redactEvent(room.roomId, eventId); - return; - } - const rShortcode = - shortcode || (reactions.find(eventWithShortcode)?.getContent().shortcode as string | undefined); - // send the reaction - mx.sendEvent( - room.roomId, - EventType.Reaction as string as unknown as keyof TimelineEvents, - getReactionContent( - targetEventId, - key, - mx, - room, - rShortcode - ) as TimelineEvents[keyof TimelineEvents] - ); -}; diff --git a/src/app/utils/room/reactions.test.ts b/src/app/utils/room/reactions.test.ts new file mode 100644 index 0000000000..11ceade168 --- /dev/null +++ b/src/app/utils/room/reactions.test.ts @@ -0,0 +1,212 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { createClient, MatrixEvent, Room } from '$types/matrix-sdk'; +import { dedupeAnnotationsBySender, toggleReaction } from './reactions'; + +const USER = '@me:example.org'; +const ROOM_ID = '!r:example.org'; +const TARGET = '$target'; +const KEY = '❤️'; + +type Harness = { + mx: MatrixClient; + room: Room; + sends: () => number; + redactions: () => string[]; + releaseSend: () => void; +}; + +/** + * A client whose /send response is held back until releaseSend(), so a second + * toggle can be made to land while the first reaction is still in flight. + */ +const makeHarness = (sendFailure?: { status: number; body: unknown }): Harness => { + let sends = 0; + const redactions: string[] = []; + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + + const fetchFn = vi.fn<(url: unknown) => Promise>(async (url: unknown) => { + const u = String(url); + if (u.includes('/redact/')) { + redactions.push(decodeURIComponent(u.split('/redact/')[1]!.split('/')[0]!)); + return new Response(JSON.stringify({ event_id: '$redaction' }), { status: 200 }); + } + if (u.includes('/send/')) { + sends += 1; + await gate; + if (sendFailure) { + return new Response(JSON.stringify(sendFailure.body), { + status: sendFailure.status, + headers: { 'Content-Type': 'application/json' }, + }); + } + return new Response(JSON.stringify({ event_id: `$sent${sends}` }), { status: 200 }); + } + return new Response('{}', { status: 200 }); + }); + + const mx = createClient({ + baseUrl: 'https://hs.example.org', + userId: USER, + accessToken: 't', + deviceId: 'D', + fetchFn: fetchFn as never, + }); + + const room = new Room(ROOM_ID, mx, USER, { timelineSupport: true }); + room.addLiveEvents( + [ + new MatrixEvent({ + type: 'm.room.message', + event_id: TARGET, + room_id: ROOM_ID, + sender: '@other:example.org', + origin_server_ts: 1, + content: { msgtype: 'm.text', body: 'hi' }, + }), + ], + { addToState: false } + ); + mx.store.storeRoom(room); + + return { mx, room, sends: () => sends, redactions: () => redactions, releaseSend: release }; +}; + +const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); + +const addRemoteReactions = (room: Room, events: { eventId: string; sender: string }[]) => + room.addLiveEvents( + events.map( + ({ eventId, sender }, index) => + new MatrixEvent({ + type: 'm.reaction', + event_id: eventId, + room_id: ROOM_ID, + sender, + origin_server_ts: 2 + index, + content: { 'm.relates_to': { rel_type: 'm.annotation', event_id: TARGET, key: KEY } }, + }) + ), + { addToState: false } + ); + +describe('dedupeAnnotationsBySender', () => { + const annotation = (eventId: string, sender: string) => + new MatrixEvent({ + type: 'm.reaction', + event_id: eventId, + room_id: ROOM_ID, + sender, + content: { 'm.relates_to': { rel_type: 'm.annotation', event_id: TARGET, key: KEY } }, + }); + + it('counts repeats from one sender once, keeping the earliest', () => { + const deduped = dedupeAnnotationsBySender([ + annotation('$a1', '@a:example.org'), + annotation('$b1', '@b:example.org'), + annotation('$a2', '@a:example.org'), + ]); + + expect(deduped.map((mEvent) => mEvent.getId())).toEqual(['$a1', '$b1']); + }); +}); + +describe('toggleReaction', () => { + let h: Harness; + + beforeEach(() => { + h = makeHarness(); + }); + + it('sends the reaction when there is none yet', async () => { + const toggling = toggleReaction(h.mx, h.room, TARGET, KEY); + h.releaseSend(); + await toggling; + + expect(h.sends()).toBe(1); + expect(h.redactions()).toEqual([]); + }); + + it('redacts an existing reaction of ours instead of sending another', async () => { + const first = toggleReaction(h.mx, h.room, TARGET, KEY); + h.releaseSend(); + await first; + + await toggleReaction(h.mx, h.room, TARGET, KEY); + + expect(h.sends()).toBe(1); + expect(h.redactions()).toEqual(['$sent1']); + }); + + it('ignores a toggle made while the first one is still in flight', async () => { + const first = toggleReaction(h.mx, h.room, TARGET, KEY); + await flush(); + const second = toggleReaction(h.mx, h.room, TARGET, KEY); + + h.releaseSend(); + await Promise.all([first, second]); + + expect(h.sends()).toBe(1); + expect(h.redactions()).toEqual([]); + }); + + it('sends a single reaction when the button is mashed', async () => { + const clicks = Array.from({ length: 10 }, () => toggleReaction(h.mx, h.room, TARGET, KEY)); + h.releaseSend(); + await Promise.all(clicks); + + expect(h.sends()).toBe(1); + expect(h.redactions()).toEqual([]); + }); + + it('removes the reaction on the next toggle after the send settles', async () => { + const clicks = Array.from({ length: 10 }, () => toggleReaction(h.mx, h.room, TARGET, KEY)); + h.releaseSend(); + await Promise.all(clicks); + + await toggleReaction(h.mx, h.room, TARGET, KEY); + + expect(h.sends()).toBe(1); + expect(h.redactions()).toEqual(['$sent1']); + }); + + it('redacts every duplicate of ours, so the reaction stops being counted', async () => { + addRemoteReactions(h.room, [ + { eventId: '$dup1', sender: USER }, + { eventId: '$dup2', sender: USER }, + { eventId: '$other', sender: '@other:example.org' }, + ]); + + await toggleReaction(h.mx, h.room, TARGET, KEY); + + expect(h.sends()).toBe(0); + expect(h.redactions().toSorted()).toEqual(['$dup1', '$dup2']); + }); + + it('accepts a server that rejects the send as a duplicate annotation', async () => { + const failing = makeHarness({ + status: 400, + body: { errcode: 'M_DUPLICATE_ANNOTATION', error: 'Duplicate annotation' }, + }); + + const toggling = toggleReaction(failing.mx, failing.room, TARGET, KEY); + failing.releaseSend(); + + await expect(toggling).resolves.toBeUndefined(); + }); + + it('keeps reactions with different keys independent', async () => { + const toggling = Promise.all([ + toggleReaction(h.mx, h.room, TARGET, KEY), + toggleReaction(h.mx, h.room, TARGET, '🫀'), + ]); + h.releaseSend(); + await toggling; + + expect(h.sends()).toBe(2); + expect(h.redactions()).toEqual([]); + }); +}); diff --git a/src/app/utils/room/reactions.ts b/src/app/utils/room/reactions.ts new file mode 100644 index 0000000000..432ea2cdf1 --- /dev/null +++ b/src/app/utils/room/reactions.ts @@ -0,0 +1,84 @@ +import type { MatrixClient, MatrixEvent, Room, TimelineEvents } from '$types/matrix-sdk'; +import { EventType, MatrixError } from '$types/matrix-sdk'; +import { factoryEventSentBy, eventWithShortcode } from '$utils/matrix'; +import { getReactionContent } from '$utils/messageReaction'; +import { getEventReactions } from './relations'; +import { redactEvent } from './redaction'; + +// Duplicates reach us over federation even though servers reject them locally, and the +// spec counts repeats from one sender as a single annotation. +export const dedupeAnnotationsBySender = (events: Iterable): MatrixEvent[] => { + const bySender = new Map(); + for (const mEvent of events) { + const sender = mEvent.getSender(); + if (sender && !bySender.has(sender)) bySender.set(sender, mEvent); + } + return [...bySender.values()]; +}; + +// Add vs remove is read from the relation aggregation, which only catches up once the +// sent event is acknowledged, so a click landing before then would add a duplicate. +const runningToggles = new Map>(); + +const toggleKey = (roomId: string, targetEventId: string, key: string) => + `${roomId}|${targetEventId}|${key}`; + +const runToggle = async ( + mx: MatrixClient, + room: Room, + targetEventId: string, + key: string, + shortcode?: string +): Promise => { + const relations = getEventReactions(room.getUnfilteredTimelineSet(), targetEventId); + const allReactions = relations?.getSortedAnnotationsByKey() ?? []; + const [, reactionsSet] = allReactions.find(([k]) => k === key) ?? []; + const reactions = reactionsSet ? Array.from(reactionsSet) : []; + + const myReactions = reactions + .filter(factoryEventSentBy(mx.getSafeUserId())) + .filter((mEvent) => mEvent.isRelation()); + if (myReactions.length > 0) { + // Redact every duplicate, not just one, or the reaction stays counted. + await Promise.all(myReactions.map((mEvent) => redactEvent(mx, room, mEvent))); + return; + } + + const rShortcode = + shortcode || (reactions.find(eventWithShortcode)?.getContent().shortcode as string | undefined); + + try { + await mx.sendEvent( + room.roomId, + EventType.Reaction as string as unknown as keyof TimelineEvents, + getReactionContent( + targetEventId, + key, + mx, + room, + rShortcode + ) as TimelineEvents[keyof TimelineEvents] + ); + } catch (err) { + // The reaction the server refused as a duplicate is the one we wanted there. + if (!(err instanceof MatrixError && err.errcode === 'M_DUPLICATE_ANNOTATION')) throw err; + } +}; + +export const toggleReaction = ( + mx: MatrixClient, + room: Room, + targetEventId: string, + key: string, + shortcode?: string +): Promise => { + const flightKey = toggleKey(room.roomId, targetEventId, key); + const running = runningToggles.get(flightKey); + if (running) return running; + + const toggle = runToggle(mx, room, targetEventId, key, shortcode).finally(() => { + runningToggles.delete(flightKey); + }); + runningToggles.set(flightKey, toggle); + return toggle; +}; diff --git a/src/app/utils/room/redaction.test.ts b/src/app/utils/room/redaction.test.ts new file mode 100644 index 0000000000..0f9c8c51c9 --- /dev/null +++ b/src/app/utils/room/redaction.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect, vi } from 'vitest'; +import type { MatrixClient, Room } from '$types/matrix-sdk'; +import { EventStatus, MatrixEvent } from '$types/matrix-sdk'; +import { isLocalEventId, redactEvent, waitForRemoteEventId } from './redaction'; + +const ROOM_ID = '!r:example.org'; + +const makeMx = () => + ({ + redactEvent: vi.fn<() => Promise<{ event_id: string }>>(async () => ({ + event_id: '$redaction', + })), + cancelPendingEvent: vi.fn<() => void>(), + }) as unknown as MatrixClient & { + redactEvent: ReturnType; + cancelPendingEvent: ReturnType; + }; + +const room = { roomId: ROOM_ID } as Room; + +const makeEvent = (eventId: string, status: EventStatus | null = null) => { + const mEvent = new MatrixEvent({ + type: 'm.reaction', + event_id: eventId, + room_id: ROOM_ID, + sender: '@me:example.org', + content: {}, + }); + mEvent.setStatus(status); + return mEvent; +}; + +describe('isLocalEventId', () => { + it('recognises the local echo prefix', () => { + expect(isLocalEventId('~!r:example.org:txn1')).toBe(true); + expect(isLocalEventId('$real')).toBe(false); + }); +}); + +describe('redactEvent', () => { + it('redacts an event the server already knows about', async () => { + const mx = makeMx(); + await redactEvent(mx, room, makeEvent('$real'), { reason: 'spam' }); + + expect(mx.redactEvent).toHaveBeenCalledWith(ROOM_ID, '$real', undefined, { reason: 'spam' }); + expect(mx.cancelPendingEvent).not.toHaveBeenCalled(); + }); + + it('cancels a queued event instead of redacting a local echo id', async () => { + const mx = makeMx(); + const mEvent = makeEvent('~!r:example.org:txn1', EventStatus.QUEUED); + + await redactEvent(mx, room, mEvent); + + expect(mx.cancelPendingEvent).toHaveBeenCalledWith(mEvent); + expect(mx.redactEvent).not.toHaveBeenCalled(); + }); + + it('waits for the server id before redacting an event that is in flight', async () => { + const mx = makeMx(); + const mEvent = makeEvent('~!r:example.org:txn1', EventStatus.SENDING); + + const redacting = redactEvent(mx, room, mEvent); + await Promise.resolve(); + expect(mx.redactEvent).not.toHaveBeenCalled(); + + mEvent.replaceLocalEventId('$real'); + await redacting; + + expect(mx.redactEvent).toHaveBeenCalledWith(ROOM_ID, '$real', undefined, undefined); + }); + + it('gives up when the in flight send fails', async () => { + const mx = makeMx(); + const mEvent = makeEvent('~!r:example.org:txn1', EventStatus.SENDING); + + const redacting = redactEvent(mx, room, mEvent); + mEvent.setStatus(EventStatus.NOT_SENT); + await redacting; + + expect(mx.redactEvent).not.toHaveBeenCalled(); + }); +}); + +describe('waitForRemoteEventId', () => { + it('waits for a pending event to receive its server id', async () => { + const mEvent = makeEvent('~!r:example.org:txn1', EventStatus.SENDING); + const eventId = waitForRemoteEventId(mEvent); + + mEvent.replaceLocalEventId('$real'); + + await expect(eventId).resolves.toBe('$real'); + }); +}); diff --git a/src/app/utils/room/redaction.ts b/src/app/utils/room/redaction.ts new file mode 100644 index 0000000000..6f48163e71 --- /dev/null +++ b/src/app/utils/room/redaction.ts @@ -0,0 +1,59 @@ +import type { IRedactOpts, MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk'; +import { EventStatus, MatrixEventEvent } from '$types/matrix-sdk'; + +const LOCAL_EVENT_ID_PREFIX = '~'; + +const CANCELLABLE_STATUSES = new Set([ + EventStatus.QUEUED, + EventStatus.NOT_SENT, + EventStatus.ENCRYPTING, +]); + +export const isLocalEventId = (eventId: string): boolean => + eventId.startsWith(LOCAL_EVENT_ID_PREFIX); + +export const waitForRemoteEventId = (mEvent: MatrixEvent): Promise => + new Promise((resolve) => { + const settle = (eventId: string | undefined) => { + mEvent.off(MatrixEventEvent.LocalEventIdReplaced, onIdReplaced); + mEvent.off(MatrixEventEvent.Status, onStatus); + resolve(eventId); + }; + function onIdReplaced() { + settle(mEvent.getId()); + } + function onStatus(_: MatrixEvent, status: EventStatus | null) { + if (status === EventStatus.NOT_SENT || status === EventStatus.CANCELLED) settle(undefined); + } + mEvent.on(MatrixEventEvent.LocalEventIdReplaced, onIdReplaced); + mEvent.on(MatrixEventEvent.Status, onStatus); + + const eventId = mEvent.getId(); + if (eventId && !isLocalEventId(eventId)) settle(eventId); + else if (mEvent.status === EventStatus.NOT_SENT || mEvent.status === EventStatus.CANCELLED) { + settle(undefined); + } + }); + +// mx.redactEvent throws on a local echo id: it resolves the target through +// Room.getPendingEvents(), unavailable under chronological pending event ordering. +export const redactEvent = async ( + mx: MatrixClient, + room: Room, + mEvent: MatrixEvent, + opts?: IRedactOpts +): Promise => { + let eventId = mEvent.getId(); + if (!eventId) return; + + if (isLocalEventId(eventId)) { + if (CANCELLABLE_STATUSES.has(mEvent.status)) { + mx.cancelPendingEvent(mEvent); + return; + } + eventId = await waitForRemoteEventId(mEvent); + if (!eventId || isLocalEventId(eventId)) return; + } + + await mx.redactEvent(room.roomId, eventId, undefined, opts); +}; diff --git a/src/client/initMatrix.ts b/src/client/initMatrix.ts index 74a4967491..e4265b650c 100644 --- a/src/client/initMatrix.ts +++ b/src/client/initMatrix.ts @@ -10,6 +10,7 @@ import { IndexedDBStore, IndexedDBCryptoStore, KnownMembership, + PendingEventOrdering, SyncState, } from '$types/matrix-sdk'; import { fetch } from '$utils/fetch'; @@ -459,6 +460,7 @@ export const startClient = async (mx: MatrixClient, config?: StartClientConfig): () => mx.startClient({ lazyLoadMembers: true, + pendingEventOrdering: PendingEventOrdering.Detached, slidingSync: manager?.slidingSync, threadSupport: true, }),