From 9113bbb1fd4571d4cf4ef5757f5007c480dc2709 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Tue, 8 Sep 2026 14:21:41 -0300 Subject: [PATCH 01/21] fix: stop shared audio player when its message is deleted The app-wide audio player kept playing and the Now Playing card stayed visible after the message holding the audio attachment was deleted. A new hook in the MediaPlayerProvider listens to the track's room streams (deleteMessage, deleteMessageBulk, and room-messages updates flagged as removed) and closes the player when its message is deleted, whether playing or paused. The track now carries ts and pinned so bulk deletes by date range can be matched with the existing delete criteria helper. --- .changeset/stop-audio-on-message-delete.md | 5 + .../attachments/file/AudioAttachment.tsx | 6 +- .../variants/room/RoomMessageContent.tsx | 18 +- .../variants/thread/ThreadMessageContent.tsx | 18 +- .../MediaPlayerProvider/MediaPlayerContext.ts | 4 + .../MediaPlayerProvider.tsx | 3 + .../useCloseOnTrackMessageDeleted.spec.ts | 205 ++++++++++++++++++ .../useCloseOnTrackMessageDeleted.ts | 56 +++++ 8 files changed, 310 insertions(+), 5 deletions(-) create mode 100644 .changeset/stop-audio-on-message-delete.md create mode 100644 apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts create mode 100644 apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts diff --git a/.changeset/stop-audio-on-message-delete.md b/.changeset/stop-audio-on-message-delete.md new file mode 100644 index 0000000000000..e8705bc454a84 --- /dev/null +++ b/.changeset/stop-audio-on-message-delete.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Stops the shared audio player and hides the Now Playing card when the message that owns the playing audio attachment is deleted diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 16999733b295c..0dbeef270d1b8 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -15,6 +15,8 @@ export type AudioAttachmentSource = { mid?: string; username?: string; name?: string; + ts?: Date; + pinned?: boolean; }; type AudioAttachmentComponentProps = AudioAttachmentProps & { @@ -49,8 +51,10 @@ const AudioAttachment = ({ mid: source?.mid, username: source?.username, name: source?.name, + ts: source?.ts, + pinned: source?.pinned, }), - [source?.mid, source?.rid, source?.username, source?.name, url, src, type, title, size], + [source?.mid, source?.rid, source?.username, source?.name, source?.ts, source?.pinned, url, src, type, title, size], ); const active = isActive(track.id); diff --git a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx index c95204ac6ec13..0cf157aa6b18f 100644 --- a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx @@ -60,7 +60,14 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM {!!quotes?.length && ( )} @@ -83,7 +90,14 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM )} diff --git a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx index 17fa32c8f0eea..a78366731d76a 100644 --- a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx @@ -54,7 +54,14 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { {!!quotes?.length && ( )} @@ -79,7 +86,14 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { )} diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index 50607808b6177..c75c490a2dfc1 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -25,6 +25,10 @@ export type PersistentAudioTrack = { username?: string; /** Display name of the sender. */ name?: string; + /** Timestamp of the message the audio belongs to (used to match bulk-delete criteria). */ + ts?: Date; + /** Whether the owning message is pinned (used to match bulk-delete criteria). */ + pinned?: boolean; }; export type MediaPlayerContextValue = { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx index ac0c843dc94e8..6a2b13d37ac5d 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx @@ -4,6 +4,7 @@ import { useCallback, useMemo, useRef, useState } from 'react'; import type { MediaPlayerContextValue, PersistentAudioTrack } from './MediaPlayerContext'; import { MediaPlayerContext } from './MediaPlayerContext'; +import { useCloseOnTrackMessageDeleted } from './useCloseOnTrackMessageDeleted'; import { useReloadOnError } from '../../components/message/content/attachments/file/hooks/useReloadOnError'; const PLAYBACK_RATES = [1, 1.5, 2] as const; @@ -102,6 +103,8 @@ const MediaPlayerProvider = ({ children }: MediaPlayerProviderProps) => { const isActive = useCallback((id: string) => trackRef.current?.id === id, []); + useCloseOnTrackMessageDeleted(track, close); + const value = useMemo( () => ({ track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, isActive }), [track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, isActive], diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts new file mode 100644 index 0000000000000..5c7d741501139 --- /dev/null +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -0,0 +1,205 @@ +import { mockAppRoot, type StreamControllerRef } from '@rocket.chat/mock-providers'; +import { renderHook } from '@testing-library/react'; + +import type { PersistentAudioTrack } from './MediaPlayerContext'; +import { useCloseOnTrackMessageDeleted } from './useCloseOnTrackMessageDeleted'; + +const buildTrack = (overrides: Partial = {}): PersistentAudioTrack => ({ + id: 'mid1:url', + url: 'https://example.com/audio.mp3', + title: 'audio.mp3', + rid: 'room1', + mid: 'mid1', + username: 'john.doe', + ts: new Date('2024-01-01T00:00:00.000Z'), + pinned: false, + ...overrides, +}); + +describe('useCloseOnTrackMessageDeleted', () => { + it('closes the player when deleteMessage matches the track message id', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: track.mid! }]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when deleteMessage targets another message id', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: 'other-mid' }]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('closes the player when room-messages delivers a soft-deleted (t: rm) update for the track message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + roomMessagesRef.controller?.emit(track.rid!, [{ _id: track.mid!, t: 'rm' } as any]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when room-messages delivers an update for the track message without t: rm', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + roomMessagesRef.controller?.emit(track.rid!, [{ _id: track.mid! } as any]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('closes the player when deleteMessageBulk targets the track message id', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date(0) }, + users: [], + ids: [track.mid!], + }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when deleteMessageBulk ids do not include the track message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date(0) }, + users: [], + ids: ['other-mid'], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('closes the player when deleteMessageBulk matches by ts range and user, without ids', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date(0) }, + users: [track.username!], + }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when deleteMessageBulk targets a different user, without ids', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date(0) }, + users: ['someone-else'], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('does not subscribe to streams when track is null', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + + renderHook(() => useCloseOnTrackMessageDeleted(null, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + expect(notifyRef.controller?.has(`room1/deleteMessage`)).toBe(false); + expect(close).not.toHaveBeenCalled(); + }); + + it('unsubscribes from the previous track when the track changes, so old events no longer trigger close', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const firstTrack = buildTrack(); + const secondTrack = buildTrack({ id: 'mid2:url', mid: 'mid2' }); + + const { rerender } = renderHook(({ track }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: firstTrack as PersistentAudioTrack | null }, + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + rerender({ track: secondTrack }); + + notifyRef.controller?.emit(`${firstTrack.rid}/deleteMessage`, [{ _id: firstTrack.mid! }]); + + expect(close).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts new file mode 100644 index 0000000000000..aa1a8c92e339f --- /dev/null +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -0,0 +1,56 @@ +import type { IMessage } from '@rocket.chat/core-typings'; +import { useStream } from '@rocket.chat/ui-contexts'; +import { useEffect } from 'react'; + +import type { PersistentAudioTrack } from './MediaPlayerContext'; +import { createDeleteCriteria } from '../../lib/utils/threadMessageUtils'; + +/** + * Closes the shared audio player when the message that owns the currently + * loaded track is deleted, either individually or through a bulk/prune + * operation. Runs regardless of playback state, so a paused player is + * closed too. + */ +export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null, close: () => void): void => { + const subscribeToNotifyRoom = useStream('notify-room'); + const subscribeToRoomMessages = useStream('room-messages'); + + const rid = track?.rid; + const mid = track?.mid; + const ts = track?.ts; + const pinned = track?.pinned; + const username = track?.username; + + useEffect(() => { + if (!rid || !mid) { + return; + } + + const unsubscribeFromDeleteMessage = subscribeToNotifyRoom(`${rid}/deleteMessage`, ({ _id }) => { + if (_id === mid) { + close(); + } + }); + + const unsubscribeFromDeleteMessageBulk = subscribeToNotifyRoom(`${rid}/deleteMessageBulk`, (params) => { + const matchesCriteria = createDeleteCriteria(params); + const trackMessage = { _id: mid, rid, ts, pinned, u: { username } } as IMessage; + + if (matchesCriteria(trackMessage)) { + close(); + } + }); + + const unsubscribeFromRoomMessages = subscribeToRoomMessages(rid, (message) => { + if (message._id === mid && message.t === 'rm') { + close(); + } + }); + + return () => { + unsubscribeFromDeleteMessage(); + unsubscribeFromDeleteMessageBulk(); + unsubscribeFromRoomMessages(); + }; + }, [rid, mid, ts, pinned, username, subscribeToNotifyRoom, subscribeToRoomMessages, close]); +}; From 5a70ded931a9a8d01bd76390763a7168dedc1d9a Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Tue, 8 Sep 2026 14:46:13 -0300 Subject: [PATCH 02/21] fix: carry drid and quote metadata into the audio track delete criteria Review follow-up: the synthetic message used for bulk-delete matching now includes drid so prunes with ignoreDiscussion do not close audio owned by a discussion message; QuoteAttachment forwards the full source metadata to nested attachments so quoted audio can be matched by timestamp and sender; the hook's JSDoc block is removed per lint guidance. --- .../content/attachments/QuoteAttachment.tsx | 8 ++++- .../attachments/file/AudioAttachment.tsx | 4 ++- .../variants/room/RoomMessageContent.tsx | 2 ++ .../variants/thread/ThreadMessageContent.tsx | 2 ++ .../MediaPlayerProvider/MediaPlayerContext.ts | 2 ++ .../useCloseOnTrackMessageDeleted.spec.ts | 32 +++++++++++++++++++ .../useCloseOnTrackMessageDeleted.ts | 11 ++----- 7 files changed, 51 insertions(+), 10 deletions(-) diff --git a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx index b49fc2b000f74..0e1d41b88f470 100644 --- a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx @@ -73,7 +73,13 @@ export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentPro diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 0dbeef270d1b8..953e4f308f3e9 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -17,6 +17,7 @@ export type AudioAttachmentSource = { name?: string; ts?: Date; pinned?: boolean; + drid?: string; }; type AudioAttachmentComponentProps = AudioAttachmentProps & { @@ -53,8 +54,9 @@ const AudioAttachment = ({ name: source?.name, ts: source?.ts, pinned: source?.pinned, + drid: source?.drid, }), - [source?.mid, source?.rid, source?.username, source?.name, source?.ts, source?.pinned, url, src, type, title, size], + [source?.mid, source?.rid, source?.username, source?.name, source?.ts, source?.pinned, source?.drid, url, src, type, title, size], ); const active = isActive(track.id); diff --git a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx index 0cf157aa6b18f..70f88a040484a 100644 --- a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx @@ -67,6 +67,7 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM name: message.u.name, ts: message.ts, pinned: message.pinned, + drid: message.drid, }} /> )} @@ -97,6 +98,7 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM name: message.u.name, ts: message.ts, pinned: message.pinned, + drid: message.drid, }} /> )} diff --git a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx index a78366731d76a..52880a7d8ab32 100644 --- a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx @@ -61,6 +61,7 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { name: message.u.name, ts: message.ts, pinned: message.pinned, + drid: message.drid, }} /> )} @@ -93,6 +94,7 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { name: message.u.name, ts: message.ts, pinned: message.pinned, + drid: message.drid, }} /> )} diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index c75c490a2dfc1..495f764dc40f4 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -29,6 +29,8 @@ export type PersistentAudioTrack = { ts?: Date; /** Whether the owning message is pinned (used to match bulk-delete criteria). */ pinned?: boolean; + /** Discussion room id the owning message belongs to (used to match bulk-delete criteria). */ + drid?: string; }; export type MediaPlayerContextValue = { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index 5c7d741501139..14db55f14579b 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -171,6 +171,38 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).not.toHaveBeenCalled(); }); + it('does not close the player when deleteMessageBulk ignores discussions and the track belongs to one, but closes when it does not', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const closeDiscussion = jest.fn(); + const closeNonDiscussion = jest.fn(); + const discussionTrack = buildTrack({ drid: 'disc1' }); + const nonDiscussionTrack = buildTrack({ drid: undefined }); + + const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: discussionTrack as PersistentAudioTrack | null, close: closeDiscussion }, + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + const bulkParams = { + rid: discussionTrack.rid!, + excludePinned: false, + ignoreDiscussion: true, + ts: { $gt: new Date(0) }, + users: [], + }; + + notifyRef.controller?.emit(`${discussionTrack.rid}/deleteMessageBulk`, [bulkParams]); + + expect(closeDiscussion).not.toHaveBeenCalled(); + + rerender({ track: nonDiscussionTrack, close: closeNonDiscussion }); + + notifyRef.controller?.emit(`${nonDiscussionTrack.rid}/deleteMessageBulk`, [bulkParams]); + + expect(closeNonDiscussion).toHaveBeenCalledTimes(1); + }); + it('does not subscribe to streams when track is null', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index aa1a8c92e339f..b0abb2d3b4905 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -5,12 +5,6 @@ import { useEffect } from 'react'; import type { PersistentAudioTrack } from './MediaPlayerContext'; import { createDeleteCriteria } from '../../lib/utils/threadMessageUtils'; -/** - * Closes the shared audio player when the message that owns the currently - * loaded track is deleted, either individually or through a bulk/prune - * operation. Runs regardless of playback state, so a paused player is - * closed too. - */ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null, close: () => void): void => { const subscribeToNotifyRoom = useStream('notify-room'); const subscribeToRoomMessages = useStream('room-messages'); @@ -20,6 +14,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const ts = track?.ts; const pinned = track?.pinned; const username = track?.username; + const drid = track?.drid; useEffect(() => { if (!rid || !mid) { @@ -34,7 +29,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const unsubscribeFromDeleteMessageBulk = subscribeToNotifyRoom(`${rid}/deleteMessageBulk`, (params) => { const matchesCriteria = createDeleteCriteria(params); - const trackMessage = { _id: mid, rid, ts, pinned, u: { username } } as IMessage; + const trackMessage = { _id: mid, rid, ts, pinned, drid, u: { username } } as IMessage; if (matchesCriteria(trackMessage)) { close(); @@ -52,5 +47,5 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null unsubscribeFromDeleteMessageBulk(); unsubscribeFromRoomMessages(); }; - }, [rid, mid, ts, pinned, username, subscribeToNotifyRoom, subscribeToRoomMessages, close]); + }, [rid, mid, ts, pinned, username, drid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); }; From 251291f5547e4a5bfbd0f0aedced86d0c3970e77 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Tue, 8 Sep 2026 14:57:18 -0300 Subject: [PATCH 03/21] fix: keep the quoting message timestamp on quoted audio tracks Bulk-delete matching compares ts against the message that owns the quote, so the track must carry the outer message timestamp rather than the quoted message's ts. --- .../message/content/attachments/QuoteAttachment.tsx | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx index 0e1d41b88f470..b7dee7cc42a3e 100644 --- a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx @@ -73,13 +73,7 @@ export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentPro From 49a9c42c3b5c58df08e97c6d7facd8dadba1aa02 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Tue, 8 Sep 2026 15:58:34 -0300 Subject: [PATCH 04/21] fix: honor explicit bulk-delete ids and pass message identity from quotes and moderation When deleteMessageBulk carries explicit ids they are the exact set the server removed, so the player closes on an id match before the metadata criteria apply (moderation emits ids with excludePinned and ignoreDiscussion set). QuoteAttachment now forwards the source unchanged so the track's name, username, mid and rid describe the same message. The moderation ContextMessage view passes the owning message identity so audio played there can be closed on deletion. --- .../content/attachments/QuoteAttachment.tsx | 2 +- .../useCloseOnTrackMessageDeleted.spec.ts | 24 ++++++++++++++ .../useCloseOnTrackMessageDeleted.ts | 2 +- .../moderation/helpers/ContextMessage.tsx | 31 +++++++++++++++++-- 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx index b7dee7cc42a3e..ec9b87eeef711 100644 --- a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx @@ -73,7 +73,7 @@ export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentPro diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index 14db55f14579b..776109f228b1d 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -101,6 +101,30 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).toHaveBeenCalledTimes(1); }); + it('closes when deleteMessageBulk lists the track id even though excludePinned/ignoreDiscussion would exclude it', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack({ pinned: true, drid: 'disc1' }); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: true, + ignoreDiscussion: true, + ts: { $gt: new Date() }, + users: [], + ids: [track.mid!], + }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); + it('does not close the player when deleteMessageBulk ids do not include the track message', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index b0abb2d3b4905..cce3ef44f38fc 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -31,7 +31,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const matchesCriteria = createDeleteCriteria(params); const trackMessage = { _id: mid, rid, ts, pinned, drid, u: { username } } as IMessage; - if (matchesCriteria(trackMessage)) { + if (params.ids?.includes(mid) || matchesCriteria(trackMessage)) { close(); } }); diff --git a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx index aaf93c26ee9d0..4fcd6450b0602 100644 --- a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx +++ b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx @@ -84,7 +84,20 @@ const ContextMessage = ({ {room.name || room.fname || 'DM'} - {!!quotes?.length && } + {!!quotes?.length && ( + + )} {!message.blocks?.length && !!message.md?.length ? ( <> {(!isEncryptedMessage || message.e2e === 'done') && ( @@ -98,7 +111,21 @@ const ContextMessage = ({ ) )} - {!!attachments && } + {!!attachments && ( + + )} {message.blocks && } From 6d99164ebc2b96c7754cde103cb0562b9d086e82 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Tue, 8 Sep 2026 16:27:15 -0300 Subject: [PATCH 05/21] fix: use a Date timestamp in moderation audio sources and pass message identity from contact history Co-Authored-By: Claude Fable 5.1 --- .../moderation/helpers/ContextMessage.tsx | 42 ++++++------------- .../MessageList/ContactHistoryMessage.tsx | 15 ++++++- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx index 4fcd6450b0602..3b988ebe040c7 100644 --- a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx +++ b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx @@ -23,6 +23,7 @@ import { useTranslation } from 'react-i18next'; import ReportReasonCollapsible from './ReportReasonCollapsible'; import MessageContentBody from '../../../../components/message/MessageContentBody'; import Attachments from '../../../../components/message/content/Attachments'; +import type { AudioAttachmentSource } from '../../../../components/message/content/attachments/file/AudioAttachment'; import UiKitMessageBlock from '../../../../components/message/uikit/UiKitMessageBlock'; import { useFormatDate } from '../../../../hooks/useFormatDate'; import { useFormatDateAndTime } from '../../../../hooks/useFormatDateAndTime'; @@ -66,6 +67,16 @@ const ContextMessage = ({ const attachments = message?.attachments?.filter((attachment: MessageAttachment) => !isQuoteAttachment(attachment)) || []; + const source: AudioAttachmentSource = { + rid: message.rid, + mid: message._id, + username: message.u.username, + name: message.u.name, + ts: new Date(message.ts), + pinned: message.pinned, + drid: message.drid, + }; + return ( <> {formatDate(message._updatedAt)} @@ -84,20 +95,7 @@ const ContextMessage = ({ {room.name || room.fname || 'DM'} - {!!quotes?.length && ( - - )} + {!!quotes?.length && } {!message.blocks?.length && !!message.md?.length ? ( <> {(!isEncryptedMessage || message.e2e === 'done') && ( @@ -111,21 +109,7 @@ const ContextMessage = ({ ) )} - {!!attachments && ( - - )} + {!!attachments && } {message.blocks && } diff --git a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx index 075da3d89214b..49c8d72b48368 100644 --- a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx +++ b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx @@ -26,6 +26,7 @@ import { useTranslation } from 'react-i18next'; import MessageContentBody from '../../../../components/message/MessageContentBody'; import StatusIndicators from '../../../../components/message/StatusIndicators'; import Attachments from '../../../../components/message/content/Attachments'; +import type { AudioAttachmentSource } from '../../../../components/message/content/attachments/file/AudioAttachment'; import UiKitMessageBlock from '../../../../components/message/uikit/UiKitMessageBlock'; import { useFormatDate } from '../../../../hooks/useFormatDate'; import { useFormatTime } from '../../../../hooks/useFormatTime'; @@ -50,6 +51,16 @@ const ContactHistoryMessage = ({ message, sequential, isNewDay, showUserAvatar } const attachments = message?.attachments?.filter((attachment: MessageAttachment) => !isQuoteAttachment(attachment)) || []; + const source: AudioAttachmentSource = { + rid: message.rid, + mid: message._id, + username: message.u.username, + name: message.u.name, + ts: message.ts, + pinned: message.pinned, + drid: message.drid, + }; + if (message.t === 'livechat-close') { return ( @@ -116,7 +127,7 @@ const ContactHistoryMessage = ({ message, sequential, isNewDay, showUserAvatar } )} - {!!quotes?.length && } + {!!quotes?.length && } {!message.blocks && ( )} {message.blocks && } - {!!attachments && } + {!!attachments && } From 877b67d18380fe89ed2bfc5ad965c470ec555e58 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Tue, 15 Sep 2026 12:59:09 -0300 Subject: [PATCH 06/21] fix: stop quoted audio playback when the original quoted message is deleted Audio played from a quote block was tracked only by the quoting message, so deleting the original message that holds the attachment left the player running against a file that no longer exists. The quote attachment now derives the original message id from its permalink and forwards it, with the original timestamp, into the audio track. The deletion hook watches both the quoting and the original message across hard delete, soft delete and bulk delete, and prune by timestamp range also closes the player when it covers the original and carries no user filter. Follow-up to CORE-2484 (PR #42074). Closes CORE-2695. --- .../stop-quoted-audio-on-original-delete.md | 5 + .../content/attachments/QuoteAttachment.tsx | 4 +- .../attachments/file/AudioAttachment.tsx | 23 ++- .../utils/getMessageIdFromPermalink.spec.ts | 26 ++++ .../lib/utils/getMessageIdFromPermalink.ts | 17 +++ .../MediaPlayerProvider/MediaPlayerContext.ts | 4 + .../useCloseOnTrackMessageDeleted.spec.ts | 136 ++++++++++++++++++ .../useCloseOnTrackMessageDeleted.ts | 30 +++- 8 files changed, 239 insertions(+), 6 deletions(-) create mode 100644 .changeset/stop-quoted-audio-on-original-delete.md create mode 100644 apps/meteor/client/lib/utils/getMessageIdFromPermalink.spec.ts create mode 100644 apps/meteor/client/lib/utils/getMessageIdFromPermalink.ts diff --git a/.changeset/stop-quoted-audio-on-original-delete.md b/.changeset/stop-quoted-audio-on-original-delete.md new file mode 100644 index 0000000000000..330117d6f9b06 --- /dev/null +++ b/.changeset/stop-quoted-audio-on-original-delete.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted while it is playing diff --git a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx index ec9b87eeef711..777b35a174407 100644 --- a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx @@ -16,6 +16,7 @@ import AttachmentDetails from './structure/AttachmentDetails'; import AttachmentInner from './structure/AttachmentInner'; import AttachmentMessageLink from './structure/AttachmentMessageLink'; import { toPlainTextRoot } from '../../../../lib/toPlainTextRoot'; +import { getMessageIdFromPermalink } from '../../../../lib/utils/getMessageIdFromPermalink'; // TODO: remove this team collaboration const quoteStyles = css` @@ -44,6 +45,7 @@ export type QuoteAttachmentProps = { export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentProps) => { const formatTime = useTimeAgo(); const displayAvatarPreference = useUserPreference('displayAvatars'); + const originMid = getMessageIdFromPermalink(attachment.message_link); return ( <> @@ -73,7 +75,7 @@ export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentPro diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 953e4f308f3e9..9fa4f81d68210 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -18,6 +18,10 @@ export type AudioAttachmentSource = { ts?: Date; pinned?: boolean; drid?: string; + /** When the audio is rendered inside a quote, the id of the original message that holds the attachment. */ + originMid?: string; + /** Timestamp of the original quoted message. */ + originTs?: Date; }; type AudioAttachmentComponentProps = AudioAttachmentProps & { @@ -55,8 +59,25 @@ const AudioAttachment = ({ ts: source?.ts, pinned: source?.pinned, drid: source?.drid, + originMid: source?.originMid, + originTs: source?.originTs, }), - [source?.mid, source?.rid, source?.username, source?.name, source?.ts, source?.pinned, source?.drid, url, src, type, title, size], + [ + source?.mid, + source?.rid, + source?.username, + source?.name, + source?.ts, + source?.pinned, + source?.drid, + source?.originMid, + source?.originTs, + url, + src, + type, + title, + size, + ], ); const active = isActive(track.id); diff --git a/apps/meteor/client/lib/utils/getMessageIdFromPermalink.spec.ts b/apps/meteor/client/lib/utils/getMessageIdFromPermalink.spec.ts new file mode 100644 index 0000000000000..6b42cbb1f81e1 --- /dev/null +++ b/apps/meteor/client/lib/utils/getMessageIdFromPermalink.spec.ts @@ -0,0 +1,26 @@ +import { getMessageIdFromPermalink } from './getMessageIdFromPermalink'; + +describe('getMessageIdFromPermalink', () => { + it('returns the msg query parameter from an absolute permalink', () => { + expect(getMessageIdFromPermalink('https://open.rocket.chat/channel/general?msg=abc123')).toBe('abc123'); + }); + + it('returns the msg query parameter when other parameters are present', () => { + expect(getMessageIdFromPermalink('https://open.rocket.chat/group/team?tab=thread&msg=xyz789&foo=bar')).toBe('xyz789'); + }); + + it('returns the msg query parameter from a relative permalink', () => { + expect(getMessageIdFromPermalink('/direct/abc?msg=def456')).toBe('def456'); + }); + + it('returns undefined when there is no msg query parameter', () => { + expect(getMessageIdFromPermalink('https://open.rocket.chat/channel/general')).toBeUndefined(); + expect(getMessageIdFromPermalink('https://open.rocket.chat/channel/general?msg=')).toBeUndefined(); + }); + + it('returns undefined for empty or unparsable input', () => { + expect(getMessageIdFromPermalink(undefined)).toBeUndefined(); + expect(getMessageIdFromPermalink('')).toBeUndefined(); + expect(getMessageIdFromPermalink('http://[invalid')).toBeUndefined(); + }); +}); diff --git a/apps/meteor/client/lib/utils/getMessageIdFromPermalink.ts b/apps/meteor/client/lib/utils/getMessageIdFromPermalink.ts new file mode 100644 index 0000000000000..b790db7d86d2c --- /dev/null +++ b/apps/meteor/client/lib/utils/getMessageIdFromPermalink.ts @@ -0,0 +1,17 @@ +/** + * Extracts the message id from a message permalink such as + * `https://open.rocket.chat/channel/general?msg=abc123`. + * Returns `undefined` when the link has no `msg` query parameter or cannot be parsed. + */ +export const getMessageIdFromPermalink = (permalink: string | undefined): string | undefined => { + if (!permalink) { + return undefined; + } + + try { + const msgId = new URL(permalink, 'http://localhost').searchParams.get('msg'); + return msgId || undefined; + } catch { + return undefined; + } +}; diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index 495f764dc40f4..0907dbe5dfa1f 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -31,6 +31,10 @@ export type PersistentAudioTrack = { pinned?: boolean; /** Discussion room id the owning message belongs to (used to match bulk-delete criteria). */ drid?: string; + /** When played from a quote, the id of the original message that holds the attachment (its deletion also closes the player). */ + originMid?: string; + /** Timestamp of the original quoted message (used to match bulk-delete criteria). */ + originTs?: Date; }; export type MediaPlayerContextValue = { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index 776109f228b1d..d2bedf9c8749d 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -227,6 +227,142 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(closeNonDiscussion).toHaveBeenCalledTimes(1); }); + describe('when the audio was played from a quote', () => { + const originTs = new Date('2023-12-31T00:00:00.000Z'); + const buildQuotedTrack = (overrides: Partial = {}) => + buildTrack({ id: 'mid2:url', mid: 'mid2', originMid: 'mid1', originTs, ...overrides }); + + it('closes the player when deleteMessage targets the original quoted message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: 'mid1' }]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('still closes the player when deleteMessage targets the quoting message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: 'mid2' }]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when deleteMessage targets an unrelated message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: 'other-mid' }]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('closes the player when the original quoted message is soft-deleted (t: rm)', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + roomMessagesRef.controller?.emit(track.rid!, [{ _id: 'mid1', t: 'rm' } as any]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('closes the player when deleteMessageBulk ids include the original quoted message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date(0) }, + users: [], + ids: ['mid1'], + }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('closes the player when a prune by ts range covers the original but not the quoting message', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not evaluate the original against a prune filtered by users, since its author is unknown', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: ['someone-else'], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); + }); + it('does not subscribe to streams when track is null', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index cce3ef44f38fc..76a8854c34184 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -15,29 +15,51 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const pinned = track?.pinned; const username = track?.username; const drid = track?.drid; + const originMid = track?.originMid; + const originTs = track?.originTs; useEffect(() => { if (!rid || !mid) { return; } + // The player closes when the message that renders the audio is deleted and, when the audio + // was played from a quote, when the original message that holds the attachment is deleted. + const watchedIds = originMid && originMid !== mid ? [mid, originMid] : [mid]; + const unsubscribeFromDeleteMessage = subscribeToNotifyRoom(`${rid}/deleteMessage`, ({ _id }) => { - if (_id === mid) { + if (watchedIds.includes(_id)) { close(); } }); const unsubscribeFromDeleteMessageBulk = subscribeToNotifyRoom(`${rid}/deleteMessageBulk`, (params) => { + if (params.ids?.some((id) => watchedIds.includes(id))) { + close(); + return; + } + const matchesCriteria = createDeleteCriteria(params); const trackMessage = { _id: mid, rid, ts, pinned, drid, u: { username } } as IMessage; - if (params.ids?.includes(mid) || matchesCriteria(trackMessage)) { + if (matchesCriteria(trackMessage)) { close(); + return; + } + + // Only the id and timestamp of the quoted original are known on the client, so the + // pinned, discussion and author filters cannot be evaluated for it. + if (originMid && originMid !== mid && originTs && !params.users?.length) { + const originMessage = { _id: originMid, rid, ts: originTs } as IMessage; + + if (matchesCriteria(originMessage)) { + close(); + } } }); const unsubscribeFromRoomMessages = subscribeToRoomMessages(rid, (message) => { - if (message._id === mid && message.t === 'rm') { + if (message.t === 'rm' && watchedIds.includes(message._id)) { close(); } }); @@ -47,5 +69,5 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null unsubscribeFromDeleteMessageBulk(); unsubscribeFromRoomMessages(); }; - }, [rid, mid, ts, pinned, username, drid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); + }, [rid, mid, ts, pinned, username, drid, originMid, originTs, subscribeToNotifyRoom, subscribeToRoomMessages, close]); }; From a68669f53c34037b90eaec6f4497f02d553df5e8 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 14:40:18 -0300 Subject: [PATCH 07/21] fix: skip the quoted-origin prune fallback when filters cannot be evaluated The timestamp fallback builds a synthetic message for the quoted original from the only fields the client has, `_id` and `ts`. `createDeleteCriteria` turns `excludePinned` into `pinned: { $ne: true }` and `ignoreDiscussion` into `drid: { $exists: false }`, and both predicates match a missing field, so a pinned or discussion original that the prune actually spared still matched and closed the player. Limit the fallback to prunes that use none of the filters the origin cannot be evaluated against. Deletions that name the original by id are unaffected and still close the player regardless of those flags. Co-Authored-By: Claude Opus 5 (1M context) --- .../useCloseOnTrackMessageDeleted.spec.ts | 70 +++++++++++++++++++ .../useCloseOnTrackMessageDeleted.ts | 7 +- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index d2bedf9c8749d..a99739845401c 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -361,6 +361,76 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).not.toHaveBeenCalled(); }); + + it('does not evaluate the original against a prune that excludes pinned messages, since its pinned state is unknown', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: true, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('does not evaluate the original against a prune that ignores discussions, since its discussion is unknown', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: false, + ignoreDiscussion: true, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('still closes for an explicit id even when the prune excludes pinned and ignores discussions', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildQuotedTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { + rid: track.rid!, + excludePinned: true, + ignoreDiscussion: true, + ts: { $gt: new Date(0) }, + users: [], + ids: ['mid1'], + }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); }); it('does not subscribe to streams when track is null', () => { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index 76a8854c34184..8e7b89a82d548 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -48,8 +48,11 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null } // Only the id and timestamp of the quoted original are known on the client, so the - // pinned, discussion and author filters cannot be evaluated for it. - if (originMid && originMid !== mid && originTs && !params.users?.length) { + // pinned, discussion and author filters cannot be evaluated for it. A synthetic message + // without `pinned`/`drid` would satisfy `excludePinned`/`ignoreDiscussion`, closing the + // player for an original the prune actually spared, so the timestamp fallback is limited + // to prunes that use none of those filters. + if (originMid && originMid !== mid && originTs && !params.users?.length && !params.excludePinned && !params.ignoreDiscussion) { const originMessage = { _id: originMid, rid, ts: originTs } as IMessage; if (matchesCriteria(originMessage)) { From 9e942024f6830ca727f730a1edf6c145f8f42e83 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 14:40:18 -0300 Subject: [PATCH 08/21] chore: scope the quoted-audio changeset to same-room deletion The hook subscribes to the deletion streams of the playing track's room only, so the note now says the original is deleted in that room rather than implying any deletion closes the player. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/stop-quoted-audio-on-original-delete.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/stop-quoted-audio-on-original-delete.md b/.changeset/stop-quoted-audio-on-original-delete.md index 330117d6f9b06..be195af2db99b 100644 --- a/.changeset/stop-quoted-audio-on-original-delete.md +++ b/.changeset/stop-quoted-audio-on-original-delete.md @@ -2,4 +2,4 @@ '@rocket.chat/meteor': patch --- -Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted while it is playing +Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted in the same room while the audio is playing From 7aca69635fa9ec9c1f86a04c60ee6a57f605b58f Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 15:33:45 -0300 Subject: [PATCH 09/21] fix: stop cross-room quoted audio playback when the original is deleted Quoting across rooms is supported: BeforeSaveJumpToMessage resolves a pasted permalink by message id with no room constraint and authorizes against the quoted message's room, so a message in one room can be quoted into another. The player subscribed to the playing track's room only, so a deletion in the origin room did not reach it. The quote attachment carried no origin identity for the client to subscribe to, although createQuoteAttachment already receives the whole quoted message and kept seven of its fields. Persist `rid`, `pinned` and `drid` alongside them, forward these into the audio track as originRid/originPinned/originDrid, and group the hook's subscriptions per distinct room so the original is watched where it lives. Same-room quotes collapse to a single subscription set and keep their existing behaviour. `pinned` is normalised to a boolean so `undefined` marks a quote stored before these fields existed; those keep the conservative prune guard, while quotes carrying the metadata are evaluated against the real criteria. A prune filtered by users still skips the original, whose author is not persisted. Co-Authored-By: Claude Opus 5 (1M context) --- ...ss-room-quoted-audio-on-original-delete.md | 6 + .../content/attachments/QuoteAttachment.tsx | 11 +- .../attachments/file/AudioAttachment.tsx | 12 ++ .../MediaPlayerProvider/MediaPlayerContext.ts | 10 + .../useCloseOnTrackMessageDeleted.spec.ts | 183 ++++++++++++++++++ .../useCloseOnTrackMessageDeleted.ts | 134 +++++++++---- apps/meteor/lib/createQuoteAttachment.ts | 6 + .../MessageQuoteAttachment.ts | 10 + 8 files changed, 333 insertions(+), 39 deletions(-) create mode 100644 .changeset/stop-cross-room-quoted-audio-on-original-delete.md diff --git a/.changeset/stop-cross-room-quoted-audio-on-original-delete.md b/.changeset/stop-cross-room-quoted-audio-on-original-delete.md new file mode 100644 index 0000000000000..e01974a6c27cb --- /dev/null +++ b/.changeset/stop-cross-room-quoted-audio-on-original-delete.md @@ -0,0 +1,6 @@ +--- +'@rocket.chat/core-typings': patch +'@rocket.chat/meteor': patch +--- + +Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted in the room it was quoted from, including when that is a different room than the one the quote is rendered in diff --git a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx index 777b35a174407..177d2d3f35c5f 100644 --- a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx @@ -75,7 +75,16 @@ export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentPro diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 9fa4f81d68210..75297ef93f372 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -22,6 +22,12 @@ export type AudioAttachmentSource = { originMid?: string; /** Timestamp of the original quoted message. */ originTs?: Date; + /** Room of the original quoted message, which may differ from the room the quote is rendered in. */ + originRid?: string; + /** Whether the original quoted message is pinned. */ + originPinned?: boolean; + /** Discussion room id of the original quoted message, when it belongs to one. */ + originDrid?: string; }; type AudioAttachmentComponentProps = AudioAttachmentProps & { @@ -61,6 +67,9 @@ const AudioAttachment = ({ drid: source?.drid, originMid: source?.originMid, originTs: source?.originTs, + originRid: source?.originRid, + originPinned: source?.originPinned, + originDrid: source?.originDrid, }), [ source?.mid, @@ -72,6 +81,9 @@ const AudioAttachment = ({ source?.drid, source?.originMid, source?.originTs, + source?.originRid, + source?.originPinned, + source?.originDrid, url, src, type, diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index 0907dbe5dfa1f..342863e4d6c2b 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -35,6 +35,16 @@ export type PersistentAudioTrack = { originMid?: string; /** Timestamp of the original quoted message (used to match bulk-delete criteria). */ originTs?: Date; + /** + * Room of the original quoted message. A quote may point at another room, in which case the + * player also watches that room for deletions. Absent on quotes stored before the origin + * metadata was persisted, which is what tells the player its criteria cannot be evaluated. + */ + originRid?: string; + /** Whether the original quoted message is pinned (used to match bulk-delete criteria). */ + originPinned?: boolean; + /** Discussion room id of the original quoted message (used to match bulk-delete criteria). */ + originDrid?: string; }; export type MediaPlayerContextValue = { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index a99739845401c..f17235f15b77f 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -433,6 +433,189 @@ describe('useCloseOnTrackMessageDeleted', () => { }); }); + describe('when the quoted original lives in another room', () => { + const originTs = new Date('2023-12-31T00:00:00.000Z'); + const buildCrossRoomTrack = (overrides: Partial = {}) => + buildTrack({ id: 'mid2:url', mid: 'mid2', originMid: 'mid1', originTs, originRid: 'room2', originPinned: false, ...overrides }); + + it('closes the player when the original is deleted in its own room', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit('room2/deleteMessage', [{ _id: 'mid1' }]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('closes the player when the original is soft-deleted (t: rm) in its own room', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + roomMessagesRef.controller?.emit('room2', [{ _id: 'mid1', t: 'rm' } as any]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('closes the player when a bulk delete in the origin room lists the original', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit('room2/deleteMessageBulk', [ + { rid: 'room2', excludePinned: false, ignoreDiscussion: false, ts: { $gt: new Date(0) }, users: [], ids: ['mid1'] }, + ]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('still closes the player when the quoting message itself is deleted in its own room', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: 'mid2' }]); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when the origin id is announced in the quoting room instead', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit(`${track.rid}/deleteMessage`, [{ _id: 'mid1' }]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('evaluates the original against a prune that excludes pinned messages, now that its pinned state is known', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const closeUnpinned = jest.fn(); + const closePinned = jest.fn(); + + const bulkParams = { + rid: 'room2', + excludePinned: true, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }; + + const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: buildCrossRoomTrack() as PersistentAudioTrack | null, close: closeUnpinned }, + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + + expect(closeUnpinned).toHaveBeenCalledTimes(1); + + rerender({ track: buildCrossRoomTrack({ originPinned: true }), close: closePinned }); + + notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + + expect(closePinned).not.toHaveBeenCalled(); + }); + + it('evaluates the original against a prune that ignores discussions, now that its discussion is known', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const closeNonDiscussion = jest.fn(); + const closeDiscussion = jest.fn(); + + const bulkParams = { + rid: 'room2', + excludePinned: false, + ignoreDiscussion: true, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }; + + const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: buildCrossRoomTrack() as PersistentAudioTrack | null, close: closeNonDiscussion }, + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + + expect(closeNonDiscussion).toHaveBeenCalledTimes(1); + + rerender({ track: buildCrossRoomTrack({ originDrid: 'disc1' }), close: closeDiscussion }); + + notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + + expect(closeDiscussion).not.toHaveBeenCalled(); + }); + + it('does not evaluate the original against a prune filtered by users, since its author is still unknown', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); + + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + notifyRef.controller?.emit('room2/deleteMessageBulk', [ + { + rid: 'room2', + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: ['someone-else'], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); + + it('unsubscribes from the origin room when the track changes', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + + const { rerender } = renderHook(({ track }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: buildCrossRoomTrack() as PersistentAudioTrack | null }, + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); + + rerender({ track: buildTrack() }); + + notifyRef.controller?.emit('room2/deleteMessage', [{ _id: 'mid1' }]); + + expect(close).not.toHaveBeenCalled(); + }); + }); + it('does not subscribe to streams when track is null', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index 8e7b89a82d548..20179125f17a5 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -5,6 +5,12 @@ import { useEffect } from 'react'; import type { PersistentAudioTrack } from './MediaPlayerContext'; import { createDeleteCriteria } from '../../lib/utils/threadMessageUtils'; +/** A message the player watches, and the room whose deletion events can remove it. */ +type RoomWatch = { + ids: string[]; + criteria: { message: IMessage; isOrigin: boolean }[]; +}; + export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null, close: () => void): void => { const subscribeToNotifyRoom = useStream('notify-room'); const subscribeToRoomMessages = useStream('room-messages'); @@ -17,60 +23,112 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const drid = track?.drid; const originMid = track?.originMid; const originTs = track?.originTs; + const originRid = track?.originRid; + const originPinned = track?.originPinned; + const originDrid = track?.originDrid; useEffect(() => { if (!rid || !mid) { return; } - // The player closes when the message that renders the audio is deleted and, when the audio - // was played from a quote, when the original message that holds the attachment is deleted. - const watchedIds = originMid && originMid !== mid ? [mid, originMid] : [mid]; + const hasOrigin = Boolean(originMid && originMid !== mid); + // Quotes stored before the origin metadata existed carry only an id and a timestamp. Their + // original is assumed to live in the quoting room, which is what the player watched before. + const hasOriginMetadata = Boolean(originRid); + const originRoom = hasOrigin ? (originRid ?? rid) : undefined; - const unsubscribeFromDeleteMessage = subscribeToNotifyRoom(`${rid}/deleteMessage`, ({ _id }) => { - if (watchedIds.includes(_id)) { - close(); + const watches = new Map(); + const watchRoom = (roomId: string): RoomWatch => { + const existing = watches.get(roomId); + if (existing) { + return existing; } - }); - const unsubscribeFromDeleteMessageBulk = subscribeToNotifyRoom(`${rid}/deleteMessageBulk`, (params) => { - if (params.ids?.some((id) => watchedIds.includes(id))) { - close(); - return; - } + const created: RoomWatch = { ids: [], criteria: [] }; + watches.set(roomId, created); + return created; + }; + + const trackWatch = watchRoom(rid); + trackWatch.ids.push(mid); + trackWatch.criteria.push({ message: { _id: mid, rid, ts, pinned, drid, u: { username } } as IMessage, isOrigin: false }); - const matchesCriteria = createDeleteCriteria(params); - const trackMessage = { _id: mid, rid, ts, pinned, drid, u: { username } } as IMessage; + if (hasOrigin && originMid && originRoom) { + const originWatch = watchRoom(originRoom); + originWatch.ids.push(originMid); - if (matchesCriteria(trackMessage)) { - close(); - return; + if (originTs) { + originWatch.criteria.push({ + message: { + _id: originMid, + rid: originRoom, + ts: originTs, + ...(hasOriginMetadata && { pinned: originPinned, ...(originDrid && { drid: originDrid }) }), + } as IMessage, + isOrigin: true, + }); } + } - // Only the id and timestamp of the quoted original are known on the client, so the - // pinned, discussion and author filters cannot be evaluated for it. A synthetic message - // without `pinned`/`drid` would satisfy `excludePinned`/`ignoreDiscussion`, closing the - // player for an original the prune actually spared, so the timestamp fallback is limited - // to prunes that use none of those filters. - if (originMid && originMid !== mid && originTs && !params.users?.length && !params.excludePinned && !params.ignoreDiscussion) { - const originMessage = { _id: originMid, rid, ts: originTs } as IMessage; + const unsubscribers = [...watches].flatMap(([roomId, { ids, criteria }]) => [ + subscribeToNotifyRoom(`${roomId}/deleteMessage`, ({ _id }) => { + if (ids.includes(_id)) { + close(); + } + }), - if (matchesCriteria(originMessage)) { + subscribeToNotifyRoom(`${roomId}/deleteMessageBulk`, (params) => { + if (params.ids?.some((id) => ids.includes(id))) { close(); + return; } - } - }); - const unsubscribeFromRoomMessages = subscribeToRoomMessages(rid, (message) => { - if (message.t === 'rm' && watchedIds.includes(message._id)) { - close(); - } - }); + const matchesCriteria = createDeleteCriteria(params); - return () => { - unsubscribeFromDeleteMessage(); - unsubscribeFromDeleteMessageBulk(); - unsubscribeFromRoomMessages(); - }; - }, [rid, mid, ts, pinned, username, drid, originMid, originTs, subscribeToNotifyRoom, subscribeToRoomMessages, close]); + // The author of a quoted original is not persisted, so a prune filtered by user can + // never be evaluated for it. Without the origin metadata `pinned` and `drid` are + // unknown too, and a synthetic message missing them would satisfy `excludePinned` + // and `ignoreDiscussion`, closing the player for an original the prune spared. + const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => { + if (!isOrigin) { + return true; + } + + if (params.users?.length) { + return false; + } + + return hasOriginMetadata || (!params.excludePinned && !params.ignoreDiscussion); + }; + + if (criteria.some((entry) => canEvaluate(entry) && matchesCriteria(entry.message))) { + close(); + } + }), + + subscribeToRoomMessages(roomId, (message) => { + if (message.t === 'rm' && ids.includes(message._id)) { + close(); + } + }), + ]); + + return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); + }, [ + rid, + mid, + ts, + pinned, + username, + drid, + originMid, + originTs, + originRid, + originPinned, + originDrid, + subscribeToNotifyRoom, + subscribeToRoomMessages, + close, + ]); }; diff --git a/apps/meteor/lib/createQuoteAttachment.ts b/apps/meteor/lib/createQuoteAttachment.ts index 7f2ca4e62202a..615d0ac7925c4 100644 --- a/apps/meteor/lib/createQuoteAttachment.ts +++ b/apps/meteor/lib/createQuoteAttachment.ts @@ -16,5 +16,11 @@ export function createQuoteAttachment( author_icon: userAvatarUrl, attachments: message.attachments || [], ts: message.ts, + // Identity of the quoted message, so clients can tell whether a deletion in its room applies + // to it. `pinned` is normalised to a boolean: `undefined` then means "quote stored before + // these fields existed", which is distinct from "known to be unpinned". + rid: message.rid, + pinned: Boolean(message.pinned), + ...(message.drid && { drid: message.drid }), }; } diff --git a/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts b/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts index 03c1fcb9cb5e5..9659414bd25a6 100644 --- a/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts +++ b/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts @@ -11,6 +11,16 @@ export type MessageQuoteAttachment = { text: string; md?: Root; attachments?: Array; // TODO this is causing issues to define a model, see @ts-expect-error at apps/meteor/app/api/server/v1/channels.ts:274 + /** + * Room the quoted message lives in. A quote may point at a different room than the one it is + * rendered in, so this is not necessarily the room of the message carrying the attachment. + * Absent on quotes stored before this field was introduced. + */ + rid?: string; + /** Whether the quoted message is pinned. Absent on quotes stored before this field was introduced. */ + pinned?: boolean; + /** Discussion room id of the quoted message, when it belongs to one. */ + drid?: string; } & MessageAttachmentBase; export const isQuoteAttachment = (attachment: MessageAttachment): attachment is MessageQuoteAttachment => From d18e02429dc8c6bea3108a683e499f50517402de Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 15:52:52 -0300 Subject: [PATCH 10/21] fix: keep the quoted-origin prune guard, persist only the origin room Review raised that a track's `pinned` is a snapshot: MediaPlayerProvider only calls setTrack when the track id changes, so pinning a message mid-playback leaves the old value behind. The same problem is worse for a quoted original. `pinned` and `drid` were being read from the stored quote attachment, and nothing refreshes that attachment when the original is later pinned or moved into a discussion, so those values are frozen at quote-creation time. Deciding `excludePinned` and `ignoreDiscussion` from them would close the player for an original the prune spared, which is what the guard added in #42143 exists to prevent. Restore that guard for the origin and drop both fields rather than persist state that cannot be kept correct; only the origin room, which is immutable, stays. The cross-room coverage this PR is for is unaffected: the origin is matched by id in its own room, and message ids are globally unique. Co-Authored-By: Claude Opus 5 (1M context) --- .../content/attachments/QuoteAttachment.tsx | 11 +-- .../attachments/file/AudioAttachment.tsx | 8 -- .../MediaPlayerProvider/MediaPlayerContext.ts | 8 +- .../useCloseOnTrackMessageDeleted.spec.ts | 89 ++++++++++--------- .../useCloseOnTrackMessageDeleted.ts | 52 +++-------- apps/meteor/lib/createQuoteAttachment.ts | 9 +- .../MessageQuoteAttachment.ts | 8 +- 7 files changed, 71 insertions(+), 114 deletions(-) diff --git a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx index 177d2d3f35c5f..b3596b2014d16 100644 --- a/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx @@ -75,16 +75,7 @@ export const QuoteAttachment = ({ attachment, source, path }: QuoteAttachmentPro diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 75297ef93f372..977f030f4b504 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -24,10 +24,6 @@ export type AudioAttachmentSource = { originTs?: Date; /** Room of the original quoted message, which may differ from the room the quote is rendered in. */ originRid?: string; - /** Whether the original quoted message is pinned. */ - originPinned?: boolean; - /** Discussion room id of the original quoted message, when it belongs to one. */ - originDrid?: string; }; type AudioAttachmentComponentProps = AudioAttachmentProps & { @@ -68,8 +64,6 @@ const AudioAttachment = ({ originMid: source?.originMid, originTs: source?.originTs, originRid: source?.originRid, - originPinned: source?.originPinned, - originDrid: source?.originDrid, }), [ source?.mid, @@ -82,8 +76,6 @@ const AudioAttachment = ({ source?.originMid, source?.originTs, source?.originRid, - source?.originPinned, - source?.originDrid, url, src, type, diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index 342863e4d6c2b..42528e11d783c 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -37,14 +37,10 @@ export type PersistentAudioTrack = { originTs?: Date; /** * Room of the original quoted message. A quote may point at another room, in which case the - * player also watches that room for deletions. Absent on quotes stored before the origin - * metadata was persisted, which is what tells the player its criteria cannot be evaluated. + * player also watches that room for deletions. Absent on quotes stored before the origin room + * was persisted, which fall back to assuming the quoting room. */ originRid?: string; - /** Whether the original quoted message is pinned (used to match bulk-delete criteria). */ - originPinned?: boolean; - /** Discussion room id of the original quoted message (used to match bulk-delete criteria). */ - originDrid?: string; }; export type MediaPlayerContextValue = { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index f17235f15b77f..64daf29e7e564 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -436,7 +436,7 @@ describe('useCloseOnTrackMessageDeleted', () => { describe('when the quoted original lives in another room', () => { const originTs = new Date('2023-12-31T00:00:00.000Z'); const buildCrossRoomTrack = (overrides: Partial = {}) => - buildTrack({ id: 'mid2:url', mid: 'mid2', originMid: 'mid1', originTs, originRid: 'room2', originPinned: false, ...overrides }); + buildTrack({ id: 'mid2:url', mid: 'mid2', originMid: 'mid1', originTs, originRid: 'room2', ...overrides }); it('closes the player when the original is deleted in its own room', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; @@ -515,64 +515,73 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).not.toHaveBeenCalled(); }); - it('evaluates the original against a prune that excludes pinned messages, now that its pinned state is known', () => { + it('closes the player when an unfiltered ts prune in the origin room covers the original', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; - const closeUnpinned = jest.fn(); - const closePinned = jest.fn(); - - const bulkParams = { - rid: 'room2', - excludePinned: true, - ignoreDiscussion: false, - ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, - users: [], - }; + const close = jest.fn(); + const track = buildCrossRoomTrack(); - const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { - initialProps: { track: buildCrossRoomTrack() as PersistentAudioTrack | null, close: closeUnpinned }, + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), }); - notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); - - expect(closeUnpinned).toHaveBeenCalledTimes(1); - - rerender({ track: buildCrossRoomTrack({ originPinned: true }), close: closePinned }); - - notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + notifyRef.controller?.emit('room2/deleteMessageBulk', [ + { + rid: 'room2', + excludePinned: false, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }, + ]); - expect(closePinned).not.toHaveBeenCalled(); + expect(close).toHaveBeenCalledTimes(1); }); - it('evaluates the original against a prune that ignores discussions, now that its discussion is known', () => { + it('does not evaluate the original against a prune that excludes pinned messages, since a stored quote never learns it was pinned', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; - const closeNonDiscussion = jest.fn(); - const closeDiscussion = jest.fn(); - - const bulkParams = { - rid: 'room2', - excludePinned: false, - ignoreDiscussion: true, - ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, - users: [], - }; + const close = jest.fn(); + const track = buildCrossRoomTrack(); - const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { - initialProps: { track: buildCrossRoomTrack() as PersistentAudioTrack | null, close: closeNonDiscussion }, + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), }); - notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + notifyRef.controller?.emit('room2/deleteMessageBulk', [ + { + rid: 'room2', + excludePinned: true, + ignoreDiscussion: false, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }, + ]); + + expect(close).not.toHaveBeenCalled(); + }); - expect(closeNonDiscussion).toHaveBeenCalledTimes(1); + it('does not evaluate the original against a prune that ignores discussions, since a stored quote never learns it became one', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildCrossRoomTrack(); - rerender({ track: buildCrossRoomTrack({ originDrid: 'disc1' }), close: closeDiscussion }); + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); - notifyRef.controller?.emit('room2/deleteMessageBulk', [bulkParams]); + notifyRef.controller?.emit('room2/deleteMessageBulk', [ + { + rid: 'room2', + excludePinned: false, + ignoreDiscussion: true, + ts: { $gt: new Date('2023-12-30T00:00:00.000Z'), $lt: new Date('2023-12-31T12:00:00.000Z') }, + users: [], + }, + ]); - expect(closeDiscussion).not.toHaveBeenCalled(); + expect(close).not.toHaveBeenCalled(); }); it('does not evaluate the original against a prune filtered by users, since its author is still unknown', () => { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index 20179125f17a5..53bb79502c104 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -24,8 +24,6 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const originMid = track?.originMid; const originTs = track?.originTs; const originRid = track?.originRid; - const originPinned = track?.originPinned; - const originDrid = track?.originDrid; useEffect(() => { if (!rid || !mid) { @@ -33,9 +31,8 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null } const hasOrigin = Boolean(originMid && originMid !== mid); - // Quotes stored before the origin metadata existed carry only an id and a timestamp. Their + // Quotes stored before the origin room existed carry only an id and a timestamp. Their // original is assumed to live in the quoting room, which is what the player watched before. - const hasOriginMetadata = Boolean(originRid); const originRoom = hasOrigin ? (originRid ?? rid) : undefined; const watches = new Map(); @@ -60,12 +57,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null if (originTs) { originWatch.criteria.push({ - message: { - _id: originMid, - rid: originRoom, - ts: originTs, - ...(hasOriginMetadata && { pinned: originPinned, ...(originDrid && { drid: originDrid }) }), - } as IMessage, + message: { _id: originMid, rid: originRoom, ts: originTs } as IMessage, isOrigin: true, }); } @@ -86,21 +78,14 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const matchesCriteria = createDeleteCriteria(params); - // The author of a quoted original is not persisted, so a prune filtered by user can - // never be evaluated for it. Without the origin metadata `pinned` and `drid` are - // unknown too, and a synthetic message missing them would satisfy `excludePinned` - // and `ignoreDiscussion`, closing the player for an original the prune spared. - const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => { - if (!isOrigin) { - return true; - } - - if (params.users?.length) { - return false; - } - - return hasOriginMetadata || (!params.excludePinned && !params.ignoreDiscussion); - }; + // Only the id, room and timestamp of a quoted original are known. Its author is not + // persisted, and its pinned and discussion state cannot be either: those change after + // the quote is saved and nothing refreshes a stored quote attachment, so any snapshot + // would go stale silently. A synthetic message missing them satisfies + // `pinned: { $ne: true }` and `drid: { $exists: false }`, which would close the player + // for an original the prune spared, so those prunes skip the origin entirely. + const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => + !isOrigin || (!params.users?.length && !params.excludePinned && !params.ignoreDiscussion); if (criteria.some((entry) => canEvaluate(entry) && matchesCriteria(entry.message))) { close(); @@ -115,20 +100,5 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null ]); return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); - }, [ - rid, - mid, - ts, - pinned, - username, - drid, - originMid, - originTs, - originRid, - originPinned, - originDrid, - subscribeToNotifyRoom, - subscribeToRoomMessages, - close, - ]); + }, [rid, mid, ts, pinned, username, drid, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); }; diff --git a/apps/meteor/lib/createQuoteAttachment.ts b/apps/meteor/lib/createQuoteAttachment.ts index 615d0ac7925c4..7ad114b7d420c 100644 --- a/apps/meteor/lib/createQuoteAttachment.ts +++ b/apps/meteor/lib/createQuoteAttachment.ts @@ -16,11 +16,10 @@ export function createQuoteAttachment( author_icon: userAvatarUrl, attachments: message.attachments || [], ts: message.ts, - // Identity of the quoted message, so clients can tell whether a deletion in its room applies - // to it. `pinned` is normalised to a boolean: `undefined` then means "quote stored before - // these fields existed", which is distinct from "known to be unpinned". + // Room of the quoted message, so a client can watch it for deletions even when the quote is + // rendered elsewhere. Only immutable identity is stored: `pinned` and `drid` can change after + // the quote is saved and nothing refreshes the stored attachment, so a snapshot of them here + // would go stale silently. rid: message.rid, - pinned: Boolean(message.pinned), - ...(message.drid && { drid: message.drid }), }; } diff --git a/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts b/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts index 9659414bd25a6..861e6043e7f5b 100644 --- a/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts +++ b/packages/core-typings/src/IMessage/MessageAttachment/MessageQuoteAttachment.ts @@ -15,12 +15,12 @@ export type MessageQuoteAttachment = { * Room the quoted message lives in. A quote may point at a different room than the one it is * rendered in, so this is not necessarily the room of the message carrying the attachment. * Absent on quotes stored before this field was introduced. + * + * Only immutable identity is stored here. Mutable state such as `pinned` or `drid` would be a + * snapshot from the moment the quote was created — nothing refreshes a stored quote attachment + * when the original is later pinned or moved into a discussion — so consumers must not infer it. */ rid?: string; - /** Whether the quoted message is pinned. Absent on quotes stored before this field was introduced. */ - pinned?: boolean; - /** Discussion room id of the quoted message, when it belongs to one. */ - drid?: string; } & MessageAttachmentBase; export const isQuoteAttachment = (attachment: MessageAttachment): attachment is MessageQuoteAttachment => From 72157af19ab15b5b6cb44480d65c14b50afbf99e Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:02:01 -0300 Subject: [PATCH 11/21] fix: do not match audio-player delete criteria on stale pinned state `pinned` and `drid` can change while audio is playing, and every copy the player holds is a snapshot. MediaPlayerProvider only replaces the active track when its id changes, so pinning the playing message leaves the previous value behind; a quoted original is worse still, since nothing refreshes a stored quote attachment after the original is pinned or moved into a discussion. Matching a stale value against `pinned: { $ne: true }` or `drid: { $exists: false }` closes the player for a message the prune spared, so bulk deletes filtering on either are no longer evaluated. A deletion that names the message by id still closes it, whatever those flags say, and a user filter is still evaluated for the playing message because a message's author does not change. With no criteria left that read them, `pinned` and `drid` are dropped from PersistentAudioTrack and AudioAttachmentSource, and from the four call sites that supplied them, rather than left as state that cannot be kept correct. Co-Authored-By: Claude Opus 5 (1M context) --- ...o-not-trust-stale-pinned-in-audio-track.md | 5 +++ .../attachments/file/AudioAttachment.tsx | 6 --- .../variants/room/RoomMessageContent.tsx | 4 -- .../variants/thread/ThreadMessageContent.tsx | 4 -- .../MediaPlayerProvider/MediaPlayerContext.ts | 10 ++--- .../useCloseOnTrackMessageDeleted.spec.ts | 43 ++++++++++--------- .../useCloseOnTrackMessageDeleted.ts | 30 +++++++------ .../moderation/helpers/ContextMessage.tsx | 2 - .../MessageList/ContactHistoryMessage.tsx | 2 - 9 files changed, 50 insertions(+), 56 deletions(-) create mode 100644 .changeset/do-not-trust-stale-pinned-in-audio-track.md diff --git a/.changeset/do-not-trust-stale-pinned-in-audio-track.md b/.changeset/do-not-trust-stale-pinned-in-audio-track.md new file mode 100644 index 0000000000000..3faa847debc23 --- /dev/null +++ b/.changeset/do-not-trust-stale-pinned-in-audio-track.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Stops the shared audio player closing on a bulk delete that excludes pinned messages or ignores discussions, in the case where the message was pinned or moved into a discussion after playback started diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 977f030f4b504..1e7e6d22324c0 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -16,8 +16,6 @@ export type AudioAttachmentSource = { username?: string; name?: string; ts?: Date; - pinned?: boolean; - drid?: string; /** When the audio is rendered inside a quote, the id of the original message that holds the attachment. */ originMid?: string; /** Timestamp of the original quoted message. */ @@ -59,8 +57,6 @@ const AudioAttachment = ({ username: source?.username, name: source?.name, ts: source?.ts, - pinned: source?.pinned, - drid: source?.drid, originMid: source?.originMid, originTs: source?.originTs, originRid: source?.originRid, @@ -71,8 +67,6 @@ const AudioAttachment = ({ source?.username, source?.name, source?.ts, - source?.pinned, - source?.drid, source?.originMid, source?.originTs, source?.originRid, diff --git a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx index 70f88a040484a..d99fbb5bdcb06 100644 --- a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx @@ -66,8 +66,6 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM username: message.u.username, name: message.u.name, ts: message.ts, - pinned: message.pinned, - drid: message.drid, }} /> )} @@ -97,8 +95,6 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM username: message.u.username, name: message.u.name, ts: message.ts, - pinned: message.pinned, - drid: message.drid, }} /> )} diff --git a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx index 52880a7d8ab32..680d78c7979e2 100644 --- a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx @@ -60,8 +60,6 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { username: message.u.username, name: message.u.name, ts: message.ts, - pinned: message.pinned, - drid: message.drid, }} /> )} @@ -93,8 +91,6 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { username: message.u.username, name: message.u.name, ts: message.ts, - pinned: message.pinned, - drid: message.drid, }} /> )} diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index 42528e11d783c..fc86773212ecb 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -25,12 +25,12 @@ export type PersistentAudioTrack = { username?: string; /** Display name of the sender. */ name?: string; - /** Timestamp of the message the audio belongs to (used to match bulk-delete criteria). */ + /** + * Timestamp of the message the audio belongs to (used to match bulk-delete criteria). + * Only immutable message state is kept here: a track is replaced on the shared element + * only when its id changes, so anything that can change mid-playback would go stale. + */ ts?: Date; - /** Whether the owning message is pinned (used to match bulk-delete criteria). */ - pinned?: boolean; - /** Discussion room id the owning message belongs to (used to match bulk-delete criteria). */ - drid?: string; /** When played from a quote, the id of the original message that holds the attachment (its deletion also closes the player). */ originMid?: string; /** Timestamp of the original quoted message (used to match bulk-delete criteria). */ diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index 64daf29e7e564..f88c41bed21ec 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -12,7 +12,6 @@ const buildTrack = (overrides: Partial = {}): PersistentAu mid: 'mid1', username: 'john.doe', ts: new Date('2024-01-01T00:00:00.000Z'), - pinned: false, ...overrides, }); @@ -105,7 +104,7 @@ describe('useCloseOnTrackMessageDeleted', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; const close = jest.fn(); - const track = buildTrack({ pinned: true, drid: 'disc1' }); + const track = buildTrack(); renderHook(() => useCloseOnTrackMessageDeleted(track, close), { wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), @@ -195,36 +194,38 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).not.toHaveBeenCalled(); }); - it('does not close the player when deleteMessageBulk ignores discussions and the track belongs to one, but closes when it does not', () => { + it('does not close the player for a prune that excludes pinned messages, since the tracked pin state is a snapshot', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; - const closeDiscussion = jest.fn(); - const closeNonDiscussion = jest.fn(); - const discussionTrack = buildTrack({ drid: 'disc1' }); - const nonDiscussionTrack = buildTrack({ drid: undefined }); + const close = jest.fn(); + const track = buildTrack(); - const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { - initialProps: { track: discussionTrack as PersistentAudioTrack | null, close: closeDiscussion }, + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), }); - const bulkParams = { - rid: discussionTrack.rid!, - excludePinned: false, - ignoreDiscussion: true, - ts: { $gt: new Date(0) }, - users: [], - }; + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { rid: track.rid!, excludePinned: true, ignoreDiscussion: false, ts: { $gt: new Date(0) }, users: [] }, + ]); - notifyRef.controller?.emit(`${discussionTrack.rid}/deleteMessageBulk`, [bulkParams]); + expect(close).not.toHaveBeenCalled(); + }); - expect(closeDiscussion).not.toHaveBeenCalled(); + it('does not close the player for a prune that ignores discussions, since the tracked discussion state is a snapshot', () => { + const notifyRef: StreamControllerRef<'notify-room'> = {}; + const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; + const close = jest.fn(); + const track = buildTrack(); - rerender({ track: nonDiscussionTrack, close: closeNonDiscussion }); + renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), + }); - notifyRef.controller?.emit(`${nonDiscussionTrack.rid}/deleteMessageBulk`, [bulkParams]); + notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ + { rid: track.rid!, excludePinned: false, ignoreDiscussion: true, ts: { $gt: new Date(0) }, users: [] }, + ]); - expect(closeNonDiscussion).toHaveBeenCalledTimes(1); + expect(close).not.toHaveBeenCalled(); }); describe('when the audio was played from a quote', () => { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index 53bb79502c104..04aec150f5405 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -18,9 +18,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const rid = track?.rid; const mid = track?.mid; const ts = track?.ts; - const pinned = track?.pinned; const username = track?.username; - const drid = track?.drid; const originMid = track?.originMid; const originTs = track?.originTs; const originRid = track?.originRid; @@ -49,7 +47,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const trackWatch = watchRoom(rid); trackWatch.ids.push(mid); - trackWatch.criteria.push({ message: { _id: mid, rid, ts, pinned, drid, u: { username } } as IMessage, isOrigin: false }); + trackWatch.criteria.push({ message: { _id: mid, rid, ts, u: { username } } as IMessage, isOrigin: false }); if (hasOrigin && originMid && originRoom) { const originWatch = watchRoom(originRoom); @@ -78,14 +76,22 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const matchesCriteria = createDeleteCriteria(params); - // Only the id, room and timestamp of a quoted original are known. Its author is not - // persisted, and its pinned and discussion state cannot be either: those change after - // the quote is saved and nothing refreshes a stored quote attachment, so any snapshot - // would go stale silently. A synthetic message missing them satisfies - // `pinned: { $ne: true }` and `drid: { $exists: false }`, which would close the player - // for an original the prune spared, so those prunes skip the origin entirely. - const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => - !isOrigin || (!params.users?.length && !params.excludePinned && !params.ignoreDiscussion); + // `pinned` and `drid` are mutable, and every copy the player holds is a snapshot: the + // playing track is only replaced when its id changes, and a stored quote attachment is + // never refreshed after the original is pinned or moved into a discussion. Matching a + // stale value against `pinned: { $ne: true }` or `drid: { $exists: false }` would close + // the player for a message the prune spared, so no such prune is evaluated here. A + // deletion naming the message by id still closes it, whatever those flags say. + // + // The author of a message never changes, so a user filter is answerable for the playing + // message; a quoted original does not carry its author at all. + const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => { + if (params.excludePinned || params.ignoreDiscussion) { + return false; + } + + return !isOrigin || !params.users?.length; + }; if (criteria.some((entry) => canEvaluate(entry) && matchesCriteria(entry.message))) { close(); @@ -100,5 +106,5 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null ]); return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); - }, [rid, mid, ts, pinned, username, drid, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); + }, [rid, mid, ts, username, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); }; diff --git a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx index 3b988ebe040c7..e462607e49fd3 100644 --- a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx +++ b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx @@ -73,8 +73,6 @@ const ContextMessage = ({ username: message.u.username, name: message.u.name, ts: new Date(message.ts), - pinned: message.pinned, - drid: message.drid, }; return ( diff --git a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx index 49c8d72b48368..9628919c8d25f 100644 --- a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx +++ b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx @@ -57,8 +57,6 @@ const ContactHistoryMessage = ({ message, sequential, isNewDay, showUserAvatar } username: message.u.username, name: message.u.name, ts: message.ts, - pinned: message.pinned, - drid: message.drid, }; if (message.t === 'livechat-close') { From ed2690d74b60949380ec29ce0cedbec6a0830b53 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:08:27 -0300 Subject: [PATCH 12/21] fix: close the audio player when a moderator deletes the message The `notify-room` and `room-messages` deletion streams are authorized against room access (`canAccessRoom` / `canReadRoom`), while the moderation console only requires `view-moderation-console`. A moderator acting on a reported message from a room they have not joined therefore never receives the deletion event, and the audio keeps playing with the Now Playing card still visible. Close the track from the moderation deletion flow itself instead of relying on a stream that cannot reach that viewer. Co-Authored-By: Claude Opus 5 (1M context) --- .../views/admin/moderation/hooks/useDeleteMessage.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/apps/meteor/client/views/admin/moderation/hooks/useDeleteMessage.tsx b/apps/meteor/client/views/admin/moderation/hooks/useDeleteMessage.tsx index 9816d1d00ad33..f34a1cb510f39 100644 --- a/apps/meteor/client/views/admin/moderation/hooks/useDeleteMessage.tsx +++ b/apps/meteor/client/views/admin/moderation/hooks/useDeleteMessage.tsx @@ -3,6 +3,8 @@ import { useEndpoint, useSetModal, useToastMessageDispatch } from '@rocket.chat/ import { useMutation, useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; +import { useMediaPlayer } from '../../../../providers/MediaPlayerProvider/MediaPlayerContext'; + const useDeleteMessage = (mid: string, rid: string, onChange: () => void) => { const { t } = useTranslation(); const deleteMessage = useEndpoint('POST', '/v1/chat.delete'); @@ -10,6 +12,7 @@ const useDeleteMessage = (mid: string, rid: string, onChange: () => void) => { const dispatchToastMessage = useToastMessageDispatch(); const setModal = useSetModal(); const queryClient = useQueryClient(); + const { track, close: closeMediaPlayer } = useMediaPlayer(); const handleDeleteMessages = useMutation({ mutationFn: deleteMessage, @@ -18,6 +21,13 @@ const useDeleteMessage = (mid: string, rid: string, onChange: () => void) => { setModal(); }, onSuccess: async () => { + // Moderation only requires `view-moderation-console`, but the `notify-room` and + // `room-messages` deletion streams are authorized against room access, so a moderator + // acting on a room they have not joined never receives the event that closes the track. + if (track?.mid === mid) { + closeMediaPlayer(); + } + await handleDismissMessage.mutateAsync({ msgId: mid }); }, }); From a028d7eb5cb1f4276acd57959678080e4e64f2e7 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:10:07 -0300 Subject: [PATCH 13/21] fix: keep matching on drid, which a message cannot gain or lose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pointed out that discussion metadata does not go stale the way pinned state does, and that is right: a message is created with its `drid` by createDiscussionMessage and no code path assigns one afterwards — refreshDiscussionMetadata only writes `dcount` and `dlm`, and the remaining references read it or delete the linked room. Only `pinned` is mutable, via the pin and unpin mutations. Skipping `ignoreDiscussion` alongside `excludePinned` therefore gave up matching that was correct, and left the player running after a prune that had in fact deleted the message. Restore `drid` on the track and evaluate `ignoreDiscussion` for the playing message again; keep skipping it for a quoted original, which carries neither its author nor its discussion id. Co-Authored-By: Claude Opus 5 (1M context) --- ...o-not-trust-stale-pinned-in-audio-track.md | 2 +- .../attachments/file/AudioAttachment.tsx | 4 +++ .../variants/room/RoomMessageContent.tsx | 2 ++ .../variants/thread/ThreadMessageContent.tsx | 2 ++ .../MediaPlayerProvider/MediaPlayerContext.ts | 6 ++++ .../useCloseOnTrackMessageDeleted.spec.ts | 31 ++++++++++++++----- .../useCloseOnTrackMessageDeleted.ts | 30 +++++++++++------- .../moderation/helpers/ContextMessage.tsx | 1 + .../MessageList/ContactHistoryMessage.tsx | 1 + 9 files changed, 58 insertions(+), 21 deletions(-) diff --git a/.changeset/do-not-trust-stale-pinned-in-audio-track.md b/.changeset/do-not-trust-stale-pinned-in-audio-track.md index 3faa847debc23..a75b6fe3f0ea8 100644 --- a/.changeset/do-not-trust-stale-pinned-in-audio-track.md +++ b/.changeset/do-not-trust-stale-pinned-in-audio-track.md @@ -2,4 +2,4 @@ '@rocket.chat/meteor': patch --- -Stops the shared audio player closing on a bulk delete that excludes pinned messages or ignores discussions, in the case where the message was pinned or moved into a discussion after playback started +Stops the shared audio player closing on a bulk delete that excludes pinned messages, in the case where the message was pinned after playback started diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 1e7e6d22324c0..6b02249228d15 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -16,6 +16,8 @@ export type AudioAttachmentSource = { username?: string; name?: string; ts?: Date; + /** Discussion room id the owning message links to. Immutable once set, so it is safe to snapshot. */ + drid?: string; /** When the audio is rendered inside a quote, the id of the original message that holds the attachment. */ originMid?: string; /** Timestamp of the original quoted message. */ @@ -57,6 +59,7 @@ const AudioAttachment = ({ username: source?.username, name: source?.name, ts: source?.ts, + drid: source?.drid, originMid: source?.originMid, originTs: source?.originTs, originRid: source?.originRid, @@ -67,6 +70,7 @@ const AudioAttachment = ({ source?.username, source?.name, source?.ts, + source?.drid, source?.originMid, source?.originTs, source?.originRid, diff --git a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx index d99fbb5bdcb06..71adb4917bbbd 100644 --- a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx @@ -66,6 +66,7 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM username: message.u.username, name: message.u.name, ts: message.ts, + drid: message.drid, }} /> )} @@ -95,6 +96,7 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM username: message.u.username, name: message.u.name, ts: message.ts, + drid: message.drid, }} /> )} diff --git a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx index 680d78c7979e2..a9d265bace395 100644 --- a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx @@ -60,6 +60,7 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { username: message.u.username, name: message.u.name, ts: message.ts, + drid: message.drid, }} /> )} @@ -91,6 +92,7 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { username: message.u.username, name: message.u.name, ts: message.ts, + drid: message.drid, }} /> )} diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index fc86773212ecb..cf83701199314 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -31,6 +31,12 @@ export type PersistentAudioTrack = { * only when its id changes, so anything that can change mid-playback would go stale. */ ts?: Date; + /** + * Discussion room id the owning message links to (used to match bulk-delete criteria). + * A message is created with its `drid` and never gains or loses one, so this snapshot + * stays accurate for as long as the track lives. + */ + drid?: string; /** When played from a quote, the id of the original message that holds the attachment (its deletion also closes the player). */ originMid?: string; /** Timestamp of the original quoted message (used to match bulk-delete criteria). */ diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index f88c41bed21ec..31bed07b03305 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -211,21 +211,36 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).not.toHaveBeenCalled(); }); - it('does not close the player for a prune that ignores discussions, since the tracked discussion state is a snapshot', () => { + it('does not close the player when deleteMessageBulk ignores discussions and the track belongs to one, but closes when it does not', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; - const close = jest.fn(); - const track = buildTrack(); + const closeDiscussion = jest.fn(); + const closeNonDiscussion = jest.fn(); + const discussionTrack = buildTrack({ drid: 'disc1' }); + const nonDiscussionTrack = buildTrack({ drid: undefined }); - renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: discussionTrack as PersistentAudioTrack | null, close: closeDiscussion }, wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), }); - notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ - { rid: track.rid!, excludePinned: false, ignoreDiscussion: true, ts: { $gt: new Date(0) }, users: [] }, - ]); + const bulkParams = { + rid: discussionTrack.rid!, + excludePinned: false, + ignoreDiscussion: true, + ts: { $gt: new Date(0) }, + users: [], + }; - expect(close).not.toHaveBeenCalled(); + notifyRef.controller?.emit(`${discussionTrack.rid}/deleteMessageBulk`, [bulkParams]); + + expect(closeDiscussion).not.toHaveBeenCalled(); + + rerender({ track: nonDiscussionTrack, close: closeNonDiscussion }); + + notifyRef.controller?.emit(`${nonDiscussionTrack.rid}/deleteMessageBulk`, [bulkParams]); + + expect(closeNonDiscussion).toHaveBeenCalledTimes(1); }); describe('when the audio was played from a quote', () => { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index 04aec150f5405..cea8b2f8fc54f 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -19,6 +19,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const mid = track?.mid; const ts = track?.ts; const username = track?.username; + const drid = track?.drid; const originMid = track?.originMid; const originTs = track?.originTs; const originRid = track?.originRid; @@ -47,7 +48,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const trackWatch = watchRoom(rid); trackWatch.ids.push(mid); - trackWatch.criteria.push({ message: { _id: mid, rid, ts, u: { username } } as IMessage, isOrigin: false }); + trackWatch.criteria.push({ message: { _id: mid, rid, ts, drid, u: { username } } as IMessage, isOrigin: false }); if (hasOrigin && originMid && originRoom) { const originWatch = watchRoom(originRoom); @@ -76,21 +77,26 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const matchesCriteria = createDeleteCriteria(params); - // `pinned` and `drid` are mutable, and every copy the player holds is a snapshot: the - // playing track is only replaced when its id changes, and a stored quote attachment is - // never refreshed after the original is pinned or moved into a discussion. Matching a - // stale value against `pinned: { $ne: true }` or `drid: { $exists: false }` would close - // the player for a message the prune spared, so no such prune is evaluated here. A - // deletion naming the message by id still closes it, whatever those flags say. + // `pinned` flips over a message's lifetime and every copy the player holds is a + // snapshot: the active track is only replaced when its id changes, and a stored quote + // attachment is never rewritten. Matching a stale value against `pinned: { $ne: true }` + // would close the player for a message the prune spared, so no prune filtering on it is + // evaluated. A deletion naming the message by id still closes it either way. // - // The author of a message never changes, so a user filter is answerable for the playing - // message; a quoted original does not carry its author at all. + // `drid` is different: a message is created with its discussion id and never gains or + // loses one, so the snapshot cannot go stale and `ignoreDiscussion` stays matchable for + // the playing message. A quoted original carries neither its author nor its `drid`, so + // both of those filters remain unanswerable for it. const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => { - if (params.excludePinned || params.ignoreDiscussion) { + if (params.excludePinned) { return false; } - return !isOrigin || !params.users?.length; + if (!isOrigin) { + return true; + } + + return !params.users?.length && !params.ignoreDiscussion; }; if (criteria.some((entry) => canEvaluate(entry) && matchesCriteria(entry.message))) { @@ -106,5 +112,5 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null ]); return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); - }, [rid, mid, ts, username, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); + }, [rid, mid, ts, username, drid, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); }; diff --git a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx index e462607e49fd3..867e1599a9030 100644 --- a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx +++ b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx @@ -73,6 +73,7 @@ const ContextMessage = ({ username: message.u.username, name: message.u.name, ts: new Date(message.ts), + drid: message.drid, }; return ( diff --git a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx index 9628919c8d25f..daaf922599341 100644 --- a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx +++ b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx @@ -57,6 +57,7 @@ const ContactHistoryMessage = ({ message, sequential, isNewDay, showUserAvatar } username: message.u.username, name: message.u.name, ts: message.ts, + drid: message.drid, }; if (message.t === 'livechat-close') { From bf955e9fc759ea983ffa70cc7519474b657cd237 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:16:32 -0300 Subject: [PATCH 14/21] fix: refresh the audio player's track when the playing message changes Two independent reviews pushed back on guarding the delete criteria instead of the data, and they were right: skipping a prune that filters on `pinned` also skips the deletions it correctly matched, leaving the player running on audio whose file the server has removed. That is the failure this work exists to fix, and it is far commoner than the stale snapshot, since `excludePinned` is an ordinary retention setting. Fix the snapshot rather than work around it. `play` only swaps the active track when its id changes, so the player kept whatever `pinned` was captured when playback began. Add `updateTrack`, which adopts the mutable fields of the active track and is otherwise a no-op, and have AudioAttachment call it while it owns playback. Criteria matching for the playing message is restored in full. Residual gap, accepted and documented: the refresh needs the message rendered, so state can still drift while it is unmounted. A quoted original keeps the conservative guard, since the attachment stores neither its author nor its pinned state and nothing refreshes it. Co-Authored-By: Claude Opus 5 (1M context) --- ...o-not-trust-stale-pinned-in-audio-track.md | 2 +- .../attachments/file/AudioAttachment.tsx | 16 ++- .../variants/room/RoomMessageContent.tsx | 2 + .../variants/thread/ThreadMessageContent.tsx | 2 + .../MediaPlayerProvider/MediaPlayerContext.ts | 13 +++ .../MediaPlayerProvider.spec.tsx | 100 ++++++++++++++++++ .../MediaPlayerProvider.tsx | 17 ++- .../useCloseOnTrackMessageDeleted.spec.ts | 23 ++-- .../useCloseOnTrackMessageDeleted.ts | 33 ++---- .../moderation/helpers/ContextMessage.tsx | 1 + .../MessageList/ContactHistoryMessage.tsx | 1 + 11 files changed, 175 insertions(+), 35 deletions(-) create mode 100644 apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx diff --git a/.changeset/do-not-trust-stale-pinned-in-audio-track.md b/.changeset/do-not-trust-stale-pinned-in-audio-track.md index a75b6fe3f0ea8..4c6d7eb5c5848 100644 --- a/.changeset/do-not-trust-stale-pinned-in-audio-track.md +++ b/.changeset/do-not-trust-stale-pinned-in-audio-track.md @@ -2,4 +2,4 @@ '@rocket.chat/meteor': patch --- -Stops the shared audio player closing on a bulk delete that excludes pinned messages, in the case where the message was pinned after playback started +Keeps the shared audio player's copy of the playing message up to date, so pinning a message while its audio is playing no longer stops playback on a later bulk delete that excludes pinned messages diff --git a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx index 6b02249228d15..3025a27f4b80e 100644 --- a/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx +++ b/apps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsx @@ -1,7 +1,7 @@ import type { AudioAttachmentProps } from '@rocket.chat/core-typings'; import { AudioPlayerControls, Box } from '@rocket.chat/fuselage'; import { useMediaUrl } from '@rocket.chat/ui-contexts'; -import { useMemo, useState } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import { useMediaPlayer } from '../../../../../providers/MediaPlayerProvider'; import type { PersistentAudioTrack } from '../../../../../providers/MediaPlayerProvider'; @@ -18,6 +18,8 @@ export type AudioAttachmentSource = { ts?: Date; /** Discussion room id the owning message links to. Immutable once set, so it is safe to snapshot. */ drid?: string; + /** Whether the owning message is pinned. Mutable, so the player is refreshed while this is rendered. */ + pinned?: boolean; /** When the audio is rendered inside a quote, the id of the original message that holds the attachment. */ originMid?: string; /** Timestamp of the original quoted message. */ @@ -45,7 +47,7 @@ const AudioAttachment = ({ const getURL = useMediaUrl(); const src = useMemo(() => getURL(url), [getURL, url]); - const { play, toggle, seek, cyclePlaybackRate, isActive, playing, currentTime, duration, playbackRate } = useMediaPlayer(); + const { play, toggle, seek, cyclePlaybackRate, isActive, updateTrack, playing, currentTime, duration, playbackRate } = useMediaPlayer(); const track = useMemo( () => ({ @@ -60,6 +62,7 @@ const AudioAttachment = ({ name: source?.name, ts: source?.ts, drid: source?.drid, + pinned: source?.pinned, originMid: source?.originMid, originTs: source?.originTs, originRid: source?.originRid, @@ -71,6 +74,7 @@ const AudioAttachment = ({ source?.name, source?.ts, source?.drid, + source?.pinned, source?.originMid, source?.originTs, source?.originRid, @@ -83,6 +87,14 @@ const AudioAttachment = ({ ); const active = isActive(track.id); + + // The shared player keeps the descriptor it was handed, so hand it a fresh one whenever this + // message re-renders with different mutable state while it owns playback. + useEffect(() => { + if (active) { + updateTrack(track); + } + }, [active, track, updateTrack]); const [previewDuration, setPreviewDuration] = useState(0); return ( diff --git a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx index 71adb4917bbbd..70f88a040484a 100644 --- a/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/room/RoomMessageContent.tsx @@ -66,6 +66,7 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM username: message.u.username, name: message.u.name, ts: message.ts, + pinned: message.pinned, drid: message.drid, }} /> @@ -96,6 +97,7 @@ const RoomMessageContent = ({ message, unread, all, mention, searchText }: RoomM username: message.u.username, name: message.u.name, ts: message.ts, + pinned: message.pinned, drid: message.drid, }} /> diff --git a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx index a9d265bace395..52880a7d8ab32 100644 --- a/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx +++ b/apps/meteor/client/components/message/variants/thread/ThreadMessageContent.tsx @@ -60,6 +60,7 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { username: message.u.username, name: message.u.name, ts: message.ts, + pinned: message.pinned, drid: message.drid, }} /> @@ -92,6 +93,7 @@ const ThreadMessageContent = ({ message }: ThreadMessageContentProps) => { username: message.u.username, name: message.u.name, ts: message.ts, + pinned: message.pinned, drid: message.drid, }} /> diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts index cf83701199314..ce13839ff904e 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerContext.ts @@ -37,6 +37,12 @@ export type PersistentAudioTrack = { * stays accurate for as long as the track lives. */ drid?: string; + /** + * Whether the owning message is pinned (used to match bulk-delete criteria). Unlike `drid` + * this can change while the track is active, so the provider refreshes it from the rendered + * message; it can still drift while that message is unmounted. + */ + pinned?: boolean; /** When played from a quote, the id of the original message that holds the attachment (its deletion also closes the player). */ originMid?: string; /** Timestamp of the original quoted message (used to match bulk-delete criteria). */ @@ -65,6 +71,12 @@ export type MediaPlayerContextValue = { cyclePlaybackRate: () => void; /** Stops playback and clears the active track. */ close: () => void; + /** + * Refreshes the mutable metadata of the active track when `next` describes it. Ignored when no + * track is active or `next` is a different one, so a re-rendering message can keep the player's + * copy of its own state current without disturbing playback. + */ + updateTrack: (next: PersistentAudioTrack) => void; /** Whether the given track id is the one currently owned by the shared element. */ isActive: (id: string) => boolean; }; @@ -82,6 +94,7 @@ export const MediaPlayerContext = createContext({ seek: noop, cyclePlaybackRate: noop, close: noop, + updateTrack: noop, isActive: () => false, }); diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx new file mode 100644 index 0000000000000..ec6ca2dc9a35d --- /dev/null +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx @@ -0,0 +1,100 @@ +import { mockAppRoot } from '@rocket.chat/mock-providers'; +import { act, renderHook } from '@testing-library/react'; +import type { ReactNode } from 'react'; + +import type { PersistentAudioTrack } from './MediaPlayerContext'; +import { useMediaPlayer } from './MediaPlayerContext'; +import MediaPlayerProvider from './MediaPlayerProvider'; + +const buildTrack = (overrides: Partial = {}): PersistentAudioTrack => ({ + id: 'mid1:url', + url: 'https://example.com/audio.mp3', + title: 'audio.mp3', + rid: 'room1', + mid: 'mid1', + username: 'john.doe', + ts: new Date('2024-01-01T00:00:00.000Z'), + pinned: false, + ...overrides, +}); + +const wrapper = ({ children }: { children: ReactNode }) => { + const AppRoot = mockAppRoot().build(); + return ( + + {children} + + ); +}; + +// jsdom does not implement media playback. +beforeAll(() => { + Object.defineProperty(HTMLMediaElement.prototype, 'play', { configurable: true, value: jest.fn().mockResolvedValue(undefined) }); + Object.defineProperty(HTMLMediaElement.prototype, 'load', { configurable: true, value: jest.fn() }); +}); + +describe('MediaPlayerProvider updateTrack', () => { + it('adopts new mutable state for the active track', () => { + const { result } = renderHook(() => useMediaPlayer(), { wrapper }); + + act(() => result.current.play(buildTrack({ pinned: false }))); + + expect(result.current.track?.pinned).toBe(false); + + act(() => result.current.updateTrack(buildTrack({ pinned: true }))); + + expect(result.current.track?.pinned).toBe(true); + }); + + it('adopts a discussion id that appears after playback started', () => { + const { result } = renderHook(() => useMediaPlayer(), { wrapper }); + + act(() => result.current.play(buildTrack())); + + expect(result.current.track?.drid).toBeUndefined(); + + act(() => result.current.updateTrack(buildTrack({ drid: 'disc1' }))); + + expect(result.current.track?.drid).toBe('disc1'); + }); + + it('ignores an update describing a different track', () => { + const { result } = renderHook(() => useMediaPlayer(), { wrapper }); + + act(() => result.current.play(buildTrack({ pinned: false }))); + act(() => result.current.updateTrack(buildTrack({ id: 'mid2:url', mid: 'mid2', pinned: true }))); + + expect(result.current.track?.id).toBe('mid1:url'); + expect(result.current.track?.pinned).toBe(false); + }); + + it('ignores an update when no track is active', () => { + const { result } = renderHook(() => useMediaPlayer(), { wrapper }); + + act(() => result.current.updateTrack(buildTrack({ pinned: true }))); + + expect(result.current.track).toBeNull(); + }); + + it('keeps the same track object when nothing mutable changed, so the player does not re-render', () => { + const { result } = renderHook(() => useMediaPlayer(), { wrapper }); + + act(() => result.current.play(buildTrack())); + + const before = result.current.track; + + act(() => result.current.updateTrack(buildTrack())); + + expect(result.current.track).toBe(before); + }); + + it('does not disturb the url or identity of the active track', () => { + const { result } = renderHook(() => useMediaPlayer(), { wrapper }); + + act(() => result.current.play(buildTrack())); + act(() => result.current.updateTrack(buildTrack({ pinned: true }))); + + expect(result.current.track?.url).toBe('https://example.com/audio.mp3'); + expect(result.current.track?.mid).toBe('mid1'); + }); +}); diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx index 6a2b13d37ac5d..5bbcb1796f75b 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx @@ -57,6 +57,19 @@ const MediaPlayerProvider = ({ children }: MediaPlayerProviderProps) => { audio.play().catch((err) => console.warn('Failed to start audio playback:', err)); }); + // `play` only swaps the track when its id changes, so a message that is re-rendered with new + // mutable state (pinning, for instance) would otherwise leave the player matching delete + // criteria against the values captured when playback started. + const updateTrack = useStableCallback((next: PersistentAudioTrack) => { + setTrack((current) => { + if (!current || current.id !== next.id || (current.pinned === next.pinned && current.drid === next.drid)) { + return current; + } + + return { ...current, pinned: next.pinned, drid: next.drid }; + }); + }); + const toggle = useStableCallback(() => { const audio = audioRef.current; if (!audio || !trackRef.current) { @@ -106,8 +119,8 @@ const MediaPlayerProvider = ({ children }: MediaPlayerProviderProps) => { useCloseOnTrackMessageDeleted(track, close); const value = useMemo( - () => ({ track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, isActive }), - [track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, isActive], + () => ({ track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, updateTrack, isActive }), + [track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, updateTrack, isActive], ); return ( diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index 31bed07b03305..92101905dedee 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -194,21 +194,28 @@ describe('useCloseOnTrackMessageDeleted', () => { expect(close).not.toHaveBeenCalled(); }); - it('does not close the player for a prune that excludes pinned messages, since the tracked pin state is a snapshot', () => { + it('does not close the player when deleteMessageBulk excludes pinned messages and the track is pinned, but closes when it is not', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; - const close = jest.fn(); - const track = buildTrack(); + const closePinned = jest.fn(); + const closeUnpinned = jest.fn(); - renderHook(() => useCloseOnTrackMessageDeleted(track, close), { + const { rerender } = renderHook(({ track, close }) => useCloseOnTrackMessageDeleted(track, close), { + initialProps: { track: buildTrack({ pinned: true }) as PersistentAudioTrack | null, close: closePinned }, wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), }); - notifyRef.controller?.emit(`${track.rid}/deleteMessageBulk`, [ - { rid: track.rid!, excludePinned: true, ignoreDiscussion: false, ts: { $gt: new Date(0) }, users: [] }, - ]); + const bulkParams = { rid: 'room1', excludePinned: true, ignoreDiscussion: false, ts: { $gt: new Date(0) }, users: [] }; - expect(close).not.toHaveBeenCalled(); + notifyRef.controller?.emit('room1/deleteMessageBulk', [bulkParams]); + + expect(closePinned).not.toHaveBeenCalled(); + + rerender({ track: buildTrack({ pinned: false }), close: closeUnpinned }); + + notifyRef.controller?.emit('room1/deleteMessageBulk', [bulkParams]); + + expect(closeUnpinned).toHaveBeenCalledTimes(1); }); it('does not close the player when deleteMessageBulk ignores discussions and the track belongs to one, but closes when it does not', () => { diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts index cea8b2f8fc54f..bd32778dcebbc 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.ts @@ -20,6 +20,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const ts = track?.ts; const username = track?.username; const drid = track?.drid; + const pinned = track?.pinned; const originMid = track?.originMid; const originTs = track?.originTs; const originRid = track?.originRid; @@ -48,7 +49,7 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const trackWatch = watchRoom(rid); trackWatch.ids.push(mid); - trackWatch.criteria.push({ message: { _id: mid, rid, ts, drid, u: { username } } as IMessage, isOrigin: false }); + trackWatch.criteria.push({ message: { _id: mid, rid, ts, drid, pinned, u: { username } } as IMessage, isOrigin: false }); if (hasOrigin && originMid && originRoom) { const originWatch = watchRoom(originRoom); @@ -77,27 +78,15 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null const matchesCriteria = createDeleteCriteria(params); - // `pinned` flips over a message's lifetime and every copy the player holds is a - // snapshot: the active track is only replaced when its id changes, and a stored quote - // attachment is never rewritten. Matching a stale value against `pinned: { $ne: true }` - // would close the player for a message the prune spared, so no prune filtering on it is - // evaluated. A deletion naming the message by id still closes it either way. + // The playing message is matched on its full state: `drid` never changes once set, and + // `pinned` is refreshed by the provider while that message is rendered. It can still drift + // if the message is unmounted, which is accepted — leaving the player running on audio the + // server deleted, and whose file is gone, is the worse outcome. // - // `drid` is different: a message is created with its discussion id and never gains or - // loses one, so the snapshot cannot go stale and `ignoreDiscussion` stays matchable for - // the playing message. A quoted original carries neither its author nor its `drid`, so - // both of those filters remain unanswerable for it. - const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => { - if (params.excludePinned) { - return false; - } - - if (!isOrigin) { - return true; - } - - return !params.users?.length && !params.ignoreDiscussion; - }; + // A quoted original has no such refresh: the attachment stores its room but not its author + // or pinned state, so prunes filtering on those cannot be evaluated for it. + const canEvaluate = ({ isOrigin }: { isOrigin: boolean }): boolean => + !isOrigin || (!params.users?.length && !params.excludePinned && !params.ignoreDiscussion); if (criteria.some((entry) => canEvaluate(entry) && matchesCriteria(entry.message))) { close(); @@ -112,5 +101,5 @@ export const useCloseOnTrackMessageDeleted = (track: PersistentAudioTrack | null ]); return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); - }, [rid, mid, ts, username, drid, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); + }, [rid, mid, ts, username, drid, pinned, originMid, originTs, originRid, subscribeToNotifyRoom, subscribeToRoomMessages, close]); }; diff --git a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx index 867e1599a9030..3b988ebe040c7 100644 --- a/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx +++ b/apps/meteor/client/views/admin/moderation/helpers/ContextMessage.tsx @@ -73,6 +73,7 @@ const ContextMessage = ({ username: message.u.username, name: message.u.name, ts: new Date(message.ts), + pinned: message.pinned, drid: message.drid, }; diff --git a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx index daaf922599341..49c8d72b48368 100644 --- a/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx +++ b/apps/meteor/client/views/omnichannel/contactHistory/MessageList/ContactHistoryMessage.tsx @@ -57,6 +57,7 @@ const ContactHistoryMessage = ({ message, sequential, isNewDay, showUserAvatar } username: message.u.username, name: message.u.name, ts: message.ts, + pinned: message.pinned, drid: message.drid, }; From e8f8383e4b5d7234bdc52e279b9b785c7b2884de Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:24:30 -0300 Subject: [PATCH 15/21] chore: fold the quoted-audio changesets into one The three branches land as a single change, so they describe it in a single release note instead of three overlapping lines. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/do-not-trust-stale-pinned-in-audio-track.md | 5 ----- .changeset/stop-audio-on-quoted-message-delete.md | 6 ++++++ .../stop-cross-room-quoted-audio-on-original-delete.md | 6 ------ .changeset/stop-quoted-audio-on-original-delete.md | 5 ----- 4 files changed, 6 insertions(+), 16 deletions(-) delete mode 100644 .changeset/do-not-trust-stale-pinned-in-audio-track.md create mode 100644 .changeset/stop-audio-on-quoted-message-delete.md delete mode 100644 .changeset/stop-cross-room-quoted-audio-on-original-delete.md delete mode 100644 .changeset/stop-quoted-audio-on-original-delete.md diff --git a/.changeset/do-not-trust-stale-pinned-in-audio-track.md b/.changeset/do-not-trust-stale-pinned-in-audio-track.md deleted file mode 100644 index 4c6d7eb5c5848..0000000000000 --- a/.changeset/do-not-trust-stale-pinned-in-audio-track.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@rocket.chat/meteor': patch ---- - -Keeps the shared audio player's copy of the playing message up to date, so pinning a message while its audio is playing no longer stops playback on a later bulk delete that excludes pinned messages diff --git a/.changeset/stop-audio-on-quoted-message-delete.md b/.changeset/stop-audio-on-quoted-message-delete.md new file mode 100644 index 0000000000000..afba47d528c88 --- /dev/null +++ b/.changeset/stop-audio-on-quoted-message-delete.md @@ -0,0 +1,6 @@ +--- +'@rocket.chat/core-typings': patch +'@rocket.chat/meteor': patch +--- + +Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted, including when it was quoted from a different room, and keeps the player's copy of the playing message current so pinning it mid-playback no longer stops playback on a later bulk delete diff --git a/.changeset/stop-cross-room-quoted-audio-on-original-delete.md b/.changeset/stop-cross-room-quoted-audio-on-original-delete.md deleted file mode 100644 index e01974a6c27cb..0000000000000 --- a/.changeset/stop-cross-room-quoted-audio-on-original-delete.md +++ /dev/null @@ -1,6 +0,0 @@ ---- -'@rocket.chat/core-typings': patch -'@rocket.chat/meteor': patch ---- - -Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted in the room it was quoted from, including when that is a different room than the one the quote is rendered in diff --git a/.changeset/stop-quoted-audio-on-original-delete.md b/.changeset/stop-quoted-audio-on-original-delete.md deleted file mode 100644 index be195af2db99b..0000000000000 --- a/.changeset/stop-quoted-audio-on-original-delete.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@rocket.chat/meteor': patch ---- - -Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted in the same room while the audio is playing From 88e2c59586c9624db811c522dd2d1f0b19a45440 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:35:57 -0300 Subject: [PATCH 16/21] feat: close the audio player when the listener loses the track's room MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the supersede claim in #42074. Of the three behaviours in #41600, two were already covered here and more thoroughly — its delete handling matches only explicit ids, where this branch also matches bulk-prune criteria. The remaining one was not covered at all: leaving a room, or being removed from it, does not delete the message, so no deletion stream reports it and the audio kept playing. Add useCloseOnTrackRoomLeft, which watches the listener's own `subscriptions-changed` events and closes the player when the subscription for the track's room is removed. Kept as its own hook so deletion and access stay separate concerns. Losing the origin room of a quote deliberately does not close the player: the attachment is embedded in the quoting message, which the listener can still see. Co-authored-by: yash-rajpal <58601732+yash-rajpal@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) --- .../stop-audio-on-quoted-message-delete.md | 2 +- .../MediaPlayerProvider.spec.tsx | 5 +- .../MediaPlayerProvider.tsx | 2 + .../useCloseOnTrackRoomLeft.spec.ts | 121 ++++++++++++++++++ .../useCloseOnTrackRoomLeft.ts | 31 +++++ 5 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.spec.ts create mode 100644 apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.ts diff --git a/.changeset/stop-audio-on-quoted-message-delete.md b/.changeset/stop-audio-on-quoted-message-delete.md index afba47d528c88..8d071e7270ded 100644 --- a/.changeset/stop-audio-on-quoted-message-delete.md +++ b/.changeset/stop-audio-on-quoted-message-delete.md @@ -3,4 +3,4 @@ '@rocket.chat/meteor': patch --- -Stops the shared audio player and hides the Now Playing card when the original message of a quoted audio attachment is deleted, including when it was quoted from a different room, and keeps the player's copy of the playing message current so pinning it mid-playback no longer stops playback on a later bulk delete +Stops the shared audio player and hides the Now Playing card when the audio is no longer the listener's to hear: when the original message of a quoted audio attachment is deleted, including from another room, and when the listener leaves or is removed from the room the audio belongs to. Also keeps the player's copy of the playing message current, so pinning it mid-playback no longer stops playback on a later bulk delete diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx index ec6ca2dc9a35d..7c83617703eb6 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.spec.tsx @@ -46,7 +46,10 @@ describe('MediaPlayerProvider updateTrack', () => { expect(result.current.track?.pinned).toBe(true); }); - it('adopts a discussion id that appears after playback started', () => { + // A message is not expected to gain a discussion id after it exists, so this is defensive: + // should the descriptors ever disagree, the player takes the one the message currently renders + // rather than keeping a value it can no longer justify. + it('refreshes the discussion id when the supplied descriptor differs', () => { const { result } = renderHook(() => useMediaPlayer(), { wrapper }); act(() => result.current.play(buildTrack())); diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx index 5bbcb1796f75b..f362391d1baea 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx @@ -5,6 +5,7 @@ import { useCallback, useMemo, useRef, useState } from 'react'; import type { MediaPlayerContextValue, PersistentAudioTrack } from './MediaPlayerContext'; import { MediaPlayerContext } from './MediaPlayerContext'; import { useCloseOnTrackMessageDeleted } from './useCloseOnTrackMessageDeleted'; +import { useCloseOnTrackRoomLeft } from './useCloseOnTrackRoomLeft'; import { useReloadOnError } from '../../components/message/content/attachments/file/hooks/useReloadOnError'; const PLAYBACK_RATES = [1, 1.5, 2] as const; @@ -117,6 +118,7 @@ const MediaPlayerProvider = ({ children }: MediaPlayerProviderProps) => { const isActive = useCallback((id: string) => trackRef.current?.id === id, []); useCloseOnTrackMessageDeleted(track, close); + useCloseOnTrackRoomLeft(track, close); const value = useMemo( () => ({ track, playing, currentTime, duration, playbackRate, play, toggle, seek, cyclePlaybackRate, close, updateTrack, isActive }), diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.spec.ts new file mode 100644 index 0000000000000..4cd46815e13bb --- /dev/null +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.spec.ts @@ -0,0 +1,121 @@ +import { renderHook } from '@testing-library/react'; + +import type { PersistentAudioTrack } from './MediaPlayerContext'; +import { useCloseOnTrackRoomLeft } from './useCloseOnTrackRoomLeft'; + +// `notify-user`'s `subscriptions-changed` delivers two arguments, and the shared stream mock in +// `@rocket.chat/mock-providers` forwards only the first, so the stream is stubbed directly here. +type SubscriptionsChangedCallback = (event: string, subscription: { rid?: string }) => void; + +const mockUnsubscribe = jest.fn(); +const mockSubscribe = jest.fn((_eventName: string, _callback: SubscriptionsChangedCallback) => mockUnsubscribe); +const mockUserId = { current: 'john.doe' as string | null }; + +jest.mock('@rocket.chat/ui-contexts', () => ({ + useStream: () => mockSubscribe, + useUserId: () => mockUserId.current, +})); + +const lastCall = () => { + const call = mockSubscribe.mock.calls.at(-1); + if (!call) { + throw new Error('the hook did not subscribe'); + } + + return call; +}; + +const lastCallback = (): SubscriptionsChangedCallback => lastCall()[1]; +const lastEventName = (): string => lastCall()[0]; + +const buildTrack = (overrides: Partial = {}): PersistentAudioTrack => ({ + id: 'mid1:url', + url: 'https://example.com/audio.mp3', + title: 'audio.mp3', + rid: 'room1', + mid: 'mid1', + username: 'john.doe', + ts: new Date('2024-01-01T00:00:00.000Z'), + ...overrides, +}); + +beforeEach(() => { + mockUserId.current = 'john.doe'; + mockSubscribe.mockClear(); + mockUnsubscribe.mockClear(); +}); + +describe('useCloseOnTrackRoomLeft', () => { + it("subscribes to the listener's own subscriptions-changed events", () => { + renderHook(() => useCloseOnTrackRoomLeft(buildTrack(), jest.fn())); + + expect(lastEventName()).toBe('john.doe/subscriptions-changed'); + }); + + it('closes the player when the listener loses the subscription to the track room', () => { + const close = jest.fn(); + + renderHook(() => useCloseOnTrackRoomLeft(buildTrack(), close)); + lastCallback()('removed', { rid: 'room1' }); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not close the player when another room subscription is removed', () => { + const close = jest.fn(); + + renderHook(() => useCloseOnTrackRoomLeft(buildTrack(), close)); + lastCallback()('removed', { rid: 'other-room' }); + + expect(close).not.toHaveBeenCalled(); + }); + + it('does not close the player for subscription changes other than removal', () => { + const close = jest.fn(); + + renderHook(() => useCloseOnTrackRoomLeft(buildTrack(), close)); + lastCallback()('updated', { rid: 'room1' }); + lastCallback()('inserted', { rid: 'room1' }); + + expect(close).not.toHaveBeenCalled(); + }); + + it('leaves playback alone when the origin room of a quote is left, since the quote is still visible', () => { + const close = jest.fn(); + + renderHook(() => useCloseOnTrackRoomLeft(buildTrack({ originMid: 'mid0', originRid: 'room2' }), close)); + lastCallback()('removed', { rid: 'room2' }); + + expect(close).not.toHaveBeenCalled(); + }); + + it('does not subscribe when no track is active', () => { + renderHook(() => useCloseOnTrackRoomLeft(null, jest.fn())); + + expect(mockSubscribe).not.toHaveBeenCalled(); + }); + + it('does not subscribe when there is no logged in user', () => { + mockUserId.current = null; + + renderHook(() => useCloseOnTrackRoomLeft(buildTrack(), jest.fn())); + + expect(mockSubscribe).not.toHaveBeenCalled(); + }); + + it('unsubscribes from the previous room once the track changes', () => { + const close = jest.fn(); + + const { rerender } = renderHook(({ track }) => useCloseOnTrackRoomLeft(track, close), { + initialProps: { track: buildTrack() as PersistentAudioTrack | null }, + }); + + rerender({ track: buildTrack({ id: 'mid2:url', mid: 'mid2', rid: 'room2' }) }); + + expect(mockUnsubscribe).toHaveBeenCalledTimes(1); + + lastCallback()('removed', { rid: 'room1' }); + + expect(close).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.ts new file mode 100644 index 0000000000000..0b0c507610bae --- /dev/null +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackRoomLeft.ts @@ -0,0 +1,31 @@ +import { useStream, useUserId } from '@rocket.chat/ui-contexts'; +import { useEffect } from 'react'; + +import type { PersistentAudioTrack } from './MediaPlayerContext'; + +/** + * Closes the shared player when the listener loses the room the track belongs to, by leaving it or + * being removed from it. The message is not deleted in that case, so no deletion stream reports it, + * but the audio is no longer theirs to hear. + * + * Audio played from a quote is left alone when the *origin* room goes away: the attachment is + * embedded in the quoting message, which the listener can still see. + */ +export const useCloseOnTrackRoomLeft = (track: PersistentAudioTrack | null, close: () => void): void => { + const subscribeToNotifyUser = useStream('notify-user'); + const userId = useUserId(); + + const rid = track?.rid; + + useEffect(() => { + if (!userId || !rid) { + return; + } + + return subscribeToNotifyUser(`${userId}/subscriptions-changed`, (event, subscription) => { + if (event === 'removed' && subscription.rid === rid) { + close(); + } + }); + }, [userId, rid, subscribeToNotifyUser, close]); +}; From 203ca6f3cf97800d60b2fae581c1a7b6161380fc Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 16:49:21 -0300 Subject: [PATCH 17/21] fix: give the composer reply preview its quoted message's identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found that MessageBoxReply renders QuoteAttachment without a source, so audio previewed in the composer reached the shared player with no rid or mid. Both cleanup hooks bail when those are missing, leaving that playback running after the quoted message is deleted or its room is lost. Pass the quoted message's identity as the source. This predates the branch — the prop has always been optional and this call site never set it — but it is a hole in exactly the behaviour the rest of this PR provides. Co-Authored-By: Claude Opus 5 (1M context) --- .../room/composer/messageBox/MessageBoxReply.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/apps/meteor/client/views/room/composer/messageBox/MessageBoxReply.tsx b/apps/meteor/client/views/room/composer/messageBox/MessageBoxReply.tsx index d36d4d34fa3a2..486c184fdedca 100644 --- a/apps/meteor/client/views/room/composer/messageBox/MessageBoxReply.tsx +++ b/apps/meteor/client/views/room/composer/messageBox/MessageBoxReply.tsx @@ -39,6 +39,17 @@ const MessageBoxReply = ({ reply }: MessageBoxReplyProps) => { collapsed: true, } as MessageQuoteAttachment } + // Audio previewed here plays through the shared player, so it needs the quoted + // message's identity for the player to notice that message going away. + source={{ + rid: reply.rid, + mid: reply._id, + username: reply.u.username, + name: reply.u.name, + ts: reply.ts, + drid: reply.drid, + pinned: reply.pinned, + }} /> Date: Wed, 16 Sep 2026 16:52:24 -0300 Subject: [PATCH 18/21] fix: keep refreshed track state and restore the ids short-circuit test Three points from review: The test for the explicit-id branch of deleteMessageBulk had lost its meaning. Its track stopped being pinned and in a discussion when those fields moved, so the prune criteria no longer excluded it and the assertion passed through `matchesCriteria` whether or not the id short-circuit existed. Restore the overrides so only the id can close it. `updateTrack` adopted `pinned` and `drid` from any same-id descriptor, so a partial snapshot carrying `undefined` cleared state a fuller one had just refreshed. Absent fields now mean "unchanged"; unpinning sends `false`, so it still comes through. The release note promised more than the change delivers. Cross-room only applies to quotes created from now on, and the pin refresh only while the owning message stays rendered. Both are now stated. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/stop-audio-on-quoted-message-delete.md | 2 +- .../MediaPlayerProvider/MediaPlayerProvider.tsx | 14 ++++++++++++-- .../useCloseOnTrackMessageDeleted.spec.ts | 3 ++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/.changeset/stop-audio-on-quoted-message-delete.md b/.changeset/stop-audio-on-quoted-message-delete.md index 8d071e7270ded..bb758248e81e1 100644 --- a/.changeset/stop-audio-on-quoted-message-delete.md +++ b/.changeset/stop-audio-on-quoted-message-delete.md @@ -3,4 +3,4 @@ '@rocket.chat/meteor': patch --- -Stops the shared audio player and hides the Now Playing card when the audio is no longer the listener's to hear: when the original message of a quoted audio attachment is deleted, including from another room, and when the listener leaves or is removed from the room the audio belongs to. Also keeps the player's copy of the playing message current, so pinning it mid-playback no longer stops playback on a later bulk delete +Stops the shared audio player and hides the Now Playing card when the audio is no longer the listener's to hear: when the original message of a quoted audio attachment is deleted, when the listener leaves or is removed from the room the audio belongs to, and — for quotes created from now on — when the original is deleted in another room it was quoted from. Pinning a message while its audio plays no longer stops playback on a later bulk delete that excludes pinned messages, for as long as that message stays rendered diff --git a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx index f362391d1baea..e3b89d4f60da7 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx +++ b/apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx @@ -63,11 +63,21 @@ const MediaPlayerProvider = ({ children }: MediaPlayerProviderProps) => { // criteria against the values captured when playback started. const updateTrack = useStableCallback((next: PersistentAudioTrack) => { setTrack((current) => { - if (!current || current.id !== next.id || (current.pinned === next.pinned && current.drid === next.drid)) { + if (!current || current.id !== next.id) { return current; } - return { ...current, pinned: next.pinned, drid: next.drid }; + // A descriptor that omits a field says nothing about it, so keep what was last known + // rather than letting a partial snapshot clear state a fuller one had refreshed. + // Unpinning sends `false`, not `undefined`, so it still comes through. + const pinned = next.pinned ?? current.pinned; + const drid = next.drid ?? current.drid; + + if (current.pinned === pinned && current.drid === drid) { + return current; + } + + return { ...current, pinned, drid }; }); }); diff --git a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts index 92101905dedee..a910247bafd5d 100644 --- a/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts +++ b/apps/meteor/client/providers/MediaPlayerProvider/useCloseOnTrackMessageDeleted.spec.ts @@ -104,7 +104,8 @@ describe('useCloseOnTrackMessageDeleted', () => { const notifyRef: StreamControllerRef<'notify-room'> = {}; const roomMessagesRef: StreamControllerRef<'room-messages'> = {}; const close = jest.fn(); - const track = buildTrack(); + // Pinned and in a discussion, so the criteria genuinely exclude it: only the explicit id can close. + const track = buildTrack({ pinned: true, drid: 'disc1' }); renderHook(() => useCloseOnTrackMessageDeleted(track, close), { wrapper: mockAppRoot().withStream('notify-room', notifyRef).withStream('room-messages', roomMessagesRef).build(), From f97e3ab702e9454089b0afbe504a75c12af4372b Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 17:18:36 -0300 Subject: [PATCH 19/21] fix: give the forward and pin modals their quoted message's identity Same gap review found in the composer reply preview, in the two remaining places that render QuoteAttachment directly. ForwardMessageModal and PinMessageModal passed no source, so audio previewed in either reached the shared player without a rid or mid and both cleanup hooks bailed, leaving that playback running after the message was deleted or its room was lost. With these, every call site that renders a quote now carries the owning message's identity: AttachmentsItem forwards what it was given, and the three direct renderers set it. Co-Authored-By: Claude Opus 5 (1M context) --- .../ForwardMessageModal/ForwardMessageModal.tsx | 15 ++++++++++++++- .../modals/PinMessageModal/PinMessageModal.tsx | 15 ++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/apps/meteor/client/views/room/modals/ForwardMessageModal/ForwardMessageModal.tsx b/apps/meteor/client/views/room/modals/ForwardMessageModal/ForwardMessageModal.tsx index 85b9fc598eb55..8eb9156dfe56f 100644 --- a/apps/meteor/client/views/room/modals/ForwardMessageModal/ForwardMessageModal.tsx +++ b/apps/meteor/client/views/room/modals/ForwardMessageModal/ForwardMessageModal.tsx @@ -120,7 +120,20 @@ const ForwardMessageModal = ({ onClose, permalink, message }: ForwardMessageProp )} - + diff --git a/apps/meteor/client/views/room/modals/PinMessageModal/PinMessageModal.tsx b/apps/meteor/client/views/room/modals/PinMessageModal/PinMessageModal.tsx index 4fe09b2c0554b..9452b2565753b 100644 --- a/apps/meteor/client/views/room/modals/PinMessageModal/PinMessageModal.tsx +++ b/apps/meteor/client/views/room/modals/PinMessageModal/PinMessageModal.tsx @@ -31,7 +31,20 @@ const PinMessageModal = ({ message, ...props }: PinMessageModalProps) => { {t('Are_you_sure_you_want_to_pin_this_message')} - + {t('Pinned_messages_are_visible_to_everyone')} From a8b196b6cc7e681074979302978f507d8e699bf0 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 19:03:23 -0300 Subject: [PATCH 20/21] test: cover the audio player's stop paths end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three behaviours this branch adds had unit coverage only, and the one that matters most — the player refreshing its copy of a message mid-playback — cannot be proven by a hook test, because it depends on a real component re-render pushing state into the provider. Covers the quoted original deleted in another room, a prune excluding pinned messages sparing a message pinned while it played, and the listener leaving the room. Setup and triggers go through REST; the browser is used only for what is being verified. Two details worth keeping: The Now Playing card is located by its playback slider rather than its play/pause button, whose accessible name flips with playback state. Pinning goes through the UI rather than REST, because the client must have processed the pin before the prune arrives and an API call gives no signal for when that has happened. --- .../tests/e2e/audio-player-stop.spec.ts | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 apps/meteor/tests/e2e/audio-player-stop.spec.ts diff --git a/apps/meteor/tests/e2e/audio-player-stop.spec.ts b/apps/meteor/tests/e2e/audio-player-stop.spec.ts new file mode 100644 index 0000000000000..37f5840a56cb6 --- /dev/null +++ b/apps/meteor/tests/e2e/audio-player-stop.spec.ts @@ -0,0 +1,150 @@ +import { Users } from './fixtures/userStates'; +import { HomeChannel } from './page-objects/home-channel'; +import { createTargetChannelAndReturnFullRoom } from './utils'; +import { test, expect } from './utils/test'; + +test.use({ storageState: Users.admin.state }); + +// The suite is normally run against a production build; a local dev server serves an unminified +// bundle and needs noticeably longer to hydrate a room. +test.describe.configure({ timeout: 180 * 1000 }); + +const AUDIO_FILE = 'sample-audio.mp3'; + +/** + * The shared player is only meant to keep running while the audio is still the listener's to + * hear. These cover the three ways that can stop being true without the playing message itself + * being deleted in the room being viewed. + */ +test.describe('audio player stops when the audio is no longer available', () => { + let poHomeChannel: HomeChannel; + + test.beforeEach(async ({ page }) => { + poHomeChannel = new HomeChannel(page); + }); + + /** + * The Now Playing card. Identified by the player's own slider rather than its play/pause + * button, whose accessible name flips with playback state, and scoped to the sidebar so it is + * not confused with the player rendered inside the message itself. + */ + const nowPlayingCard = (page: HomeChannel['page']) => + page.getByRole('navigation', { name: 'Sidebar' }).getByRole('slider', { name: 'Audio Playback Range' }); + + const sendAudio = async (channel: string) => { + await poHomeChannel.gotoChannel(channel); + await poHomeChannel.content.sendFileMessage(AUDIO_FILE); + // The upload lands in the composer first; sending before it is attached posts an empty message. + await expect(poHomeChannel.content.composer.getFileByName(AUDIO_FILE)).toBeVisible(); + await poHomeChannel.composer.btnSend.click(); + // An audio attachment renders its filename as plain text, not a link, so assert on the + // player itself — which is what the rest of the test needs anyway. + await expect(poHomeChannel.content.lastUserMessage.getByRole('button', { name: 'Play', exact: true })).toBeVisible(); + }; + + const lastMessageIdOf = async (api: Parameters[0], roomId: string) => { + const history = await (await api.get(`/channels.history?roomId=${roomId}&count=1`)).json(); + return history.messages[0]._id as string; + }; + + test('closes when the quoted original is deleted in another room', async ({ page, api }) => { + const { channel: originRoom } = await createTargetChannelAndReturnFullRoom(api); + const { channel: quotingRoom } = await createTargetChannelAndReturnFullRoom(api); + + await test.step('send the audio in the origin room', async () => { + await sendAudio(originRoom.name!); + }); + + await test.step('quote it from another room by pasting its permalink', async () => { + const originMessageId = await lastMessageIdOf(api, originRoom._id); + const permalink = `${new URL(page.url()).origin}/channel/${originRoom.name}?msg=${originMessageId}`; + + await poHomeChannel.gotoChannel(quotingRoom.name!); + await poHomeChannel.content.sendMessage(permalink); + + // The quote is built server-side, so this also proves the permalink resolved. + await expect(poHomeChannel.content.lastUserMessage.getByRole('button', { name: 'Play', exact: true })).toBeVisible(); + }); + + await test.step('play the audio from the quote', async () => { + await poHomeChannel.content.lastUserMessage.getByRole('button', { name: 'Play', exact: true }).click(); + await expect(nowPlayingCard(page)).toBeVisible(); + }); + + await test.step('deleting the original in its own room closes the player', async () => { + const originMessageId = await lastMessageIdOf(api, originRoom._id); + + expect((await api.post('/chat.delete', { roomId: originRoom._id, msgId: originMessageId, asUser: true })).status()).toBe(200); + + await expect(nowPlayingCard(page)).not.toBeVisible(); + }); + + await api.post('/channels.delete', { roomId: originRoom._id }); + await api.post('/channels.delete', { roomId: quotingRoom._id }); + }); + + test('keeps playing when a prune that excludes pinned messages spares the pinned message', async ({ page, api }) => { + const { channel: targetChannel } = await createTargetChannelAndReturnFullRoom(api); + + await test.step('play the audio', async () => { + await sendAudio(targetChannel.name!); + await poHomeChannel.content.lastUserMessage.getByRole('button', { name: 'Play', exact: true }).click(); + await expect(nowPlayingCard(page)).toBeVisible(); + }); + + await test.step('pin the message while it is playing', async () => { + // Pinning appends a `message_pinned` system message, so the audio is no longer the last + // one afterwards — take its id first. + const audioMessageId = await lastMessageIdOf(api, targetChannel._id); + + // Pinned through the UI on purpose: the client must have processed the pin before the + // prune arrives, and an API call gives no signal for when that has happened. + await poHomeChannel.content.openLastMessageMenu(); + await poHomeChannel.content.btnOptionPinMessage.click(); + await page.getByRole('button', { name: 'Yes, pin message' }).click(); + + // Separates a server-side pin failure from a player bug if this ever regresses. + await expect.poll(async () => (await (await api.get(`/chat.getMessage?msgId=${audioMessageId}`)).json()).message?.pinned).toBe(true); + }); + + await test.step('a prune excluding pinned messages leaves playback alone', async () => { + expect( + ( + await api.post('/rooms.cleanHistory', { + roomId: targetChannel._id, + excludePinned: true, + latest: new Date(Date.now() + 30 * 24 * 3600 * 1000), + oldest: new Date(Date.now() - 30 * 24 * 3600 * 1000), + }) + ).status(), + ).toBe(200); + + // The server kept the message, so the player must keep it too. Give the deletion + // streams a chance to arrive before trusting that nothing closed it. + await page.waitForTimeout(2000); + await expect(nowPlayingCard(page)).toBeVisible(); + }); + + await api.post('/channels.delete', { roomId: targetChannel._id }); + }); + + test('closes when the listener leaves the room the audio belongs to', async ({ page, api }) => { + const { channel: targetChannel } = await createTargetChannelAndReturnFullRoom(api, { members: ['user1'] }); + + await test.step('play the audio', async () => { + await sendAudio(targetChannel.name!); + await poHomeChannel.content.lastUserMessage.getByRole('button', { name: 'Play', exact: true }).click(); + await expect(nowPlayingCard(page)).toBeVisible(); + }); + + await test.step('leave the room', async () => { + // A channel's last owner cannot leave it, so hand ownership over first. + expect((await api.post('/channels.addOwner', { roomId: targetChannel._id, userId: Users.user1.data._id })).status()).toBe(200); + expect((await api.post('/channels.leave', { roomId: targetChannel._id })).status()).toBe(200); + + await expect(nowPlayingCard(page)).not.toBeVisible(); + }); + + await api.post('/channels.delete', { roomId: targetChannel._id }); + }); +}); From ce7112117f7c77bdb87afd203b145b0da0b87721 Mon Sep 17 00:00:00 2001 From: Jean Brito Date: Wed, 16 Sep 2026 19:50:07 -0300 Subject: [PATCH 21/21] test: stop the audio player spec from leaking rooms, and cut a page reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanup moves to afterEach. A failing test never reached its own cleanup, so every red run left its channels behind; the admin stays a member of all of them and each later page load hydrates a longer list. The cross-room test also posts its permalink over REST and opens the room from the sidebar instead of calling gotoChannel a second time. That helper does a full page.goto, and the message it posts is setup rather than the behaviour under test — the quote is built server-side either way. Local runs against a dev server still hang on a page load roughly one run in five, spread evenly across the three tests rather than concentrated in any one of them, which points at the unminified bundle rather than the specs. Left for CI to confirm against a production build. --- .../tests/e2e/audio-player-stop.spec.ts | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/apps/meteor/tests/e2e/audio-player-stop.spec.ts b/apps/meteor/tests/e2e/audio-player-stop.spec.ts index 37f5840a56cb6..0de526c920d42 100644 --- a/apps/meteor/tests/e2e/audio-player-stop.spec.ts +++ b/apps/meteor/tests/e2e/audio-player-stop.spec.ts @@ -18,11 +18,26 @@ const AUDIO_FILE = 'sample-audio.mp3'; */ test.describe('audio player stops when the audio is no longer available', () => { let poHomeChannel: HomeChannel; + let createdRoomIds: string[] = []; test.beforeEach(async ({ page }) => { poHomeChannel = new HomeChannel(page); + createdRoomIds = []; }); + // In afterEach rather than at the end of each test: a failing test never reaches its own + // cleanup, and rooms left behind accumulate in the admin's sidebar, slowing every later run + // until the page stops hydrating within the timeout. + test.afterEach(async ({ api }) => { + await Promise.all(createdRoomIds.map((roomId) => api.post('/channels.delete', { roomId }))); + }); + + const createRoom = async (api: Parameters[0], members?: string[]) => { + const { channel } = await createTargetChannelAndReturnFullRoom(api, members ? { members } : undefined); + createdRoomIds.push(channel._id); + return channel; + }; + /** * The Now Playing card. Identified by the player's own slider rather than its play/pause * button, whose accessible name flips with playback state, and scoped to the sidebar so it is @@ -48,8 +63,8 @@ test.describe('audio player stops when the audio is no longer available', () => }; test('closes when the quoted original is deleted in another room', async ({ page, api }) => { - const { channel: originRoom } = await createTargetChannelAndReturnFullRoom(api); - const { channel: quotingRoom } = await createTargetChannelAndReturnFullRoom(api); + const originRoom = await createRoom(api); + const quotingRoom = await createRoom(api); await test.step('send the audio in the origin room', async () => { await sendAudio(originRoom.name!); @@ -59,8 +74,11 @@ test.describe('audio player stops when the audio is no longer available', () => const originMessageId = await lastMessageIdOf(api, originRoom._id); const permalink = `${new URL(page.url()).origin}/channel/${originRoom.name}?msg=${originMessageId}`; - await poHomeChannel.gotoChannel(quotingRoom.name!); - await poHomeChannel.content.sendMessage(permalink); + // Posted over REST and opened from the sidebar rather than a second `gotoChannel`: that + // helper does a full `page.goto`, and a second reload of the dev bundle is what made this + // test hang intermittently. The quote itself is built server-side either way. + expect((await api.post('/chat.postMessage', { roomId: quotingRoom._id, text: permalink })).status()).toBe(200); + await poHomeChannel.navbar.openChat(quotingRoom.name!); // The quote is built server-side, so this also proves the permalink resolved. await expect(poHomeChannel.content.lastUserMessage.getByRole('button', { name: 'Play', exact: true })).toBeVisible(); @@ -78,13 +96,10 @@ test.describe('audio player stops when the audio is no longer available', () => await expect(nowPlayingCard(page)).not.toBeVisible(); }); - - await api.post('/channels.delete', { roomId: originRoom._id }); - await api.post('/channels.delete', { roomId: quotingRoom._id }); }); test('keeps playing when a prune that excludes pinned messages spares the pinned message', async ({ page, api }) => { - const { channel: targetChannel } = await createTargetChannelAndReturnFullRoom(api); + const targetChannel = await createRoom(api); await test.step('play the audio', async () => { await sendAudio(targetChannel.name!); @@ -124,12 +139,10 @@ test.describe('audio player stops when the audio is no longer available', () => await page.waitForTimeout(2000); await expect(nowPlayingCard(page)).toBeVisible(); }); - - await api.post('/channels.delete', { roomId: targetChannel._id }); }); test('closes when the listener leaves the room the audio belongs to', async ({ page, api }) => { - const { channel: targetChannel } = await createTargetChannelAndReturnFullRoom(api, { members: ['user1'] }); + const targetChannel = await createRoom(api, ['user1']); await test.step('play the audio', async () => { await sendAudio(targetChannel.name!); @@ -144,7 +157,5 @@ test.describe('audio player stops when the audio is no longer available', () => await expect(nowPlayingCard(page)).not.toBeVisible(); }); - - await api.post('/channels.delete', { roomId: targetChannel._id }); }); });