fix: message deep links (?msg=) reload loop when jumping to an old message

.changeset/plenty-lines-double.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': patch
+---
+
+Fixes a reload loop when opening a link to an older message (`?msg=`): the room would flash the linked message and then keep loading until it hit the server rate limit (429) instead of settling. The room now opens positioned on the linked message, and when the linked message cannot be loaded the room settles on the most recent messages instead of retrying.

apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts

@@ -330,28 +330,25 @@ class RoomHistoryManagerClass extends Emitter {
 		const subscription = Subscriptions.state.find((record) => record.rid === message.rid);
 		const result = await callWithErrorHandling('loadSurroundingMessages', message, defaultLimit, showThreadMessages);
 
-		this.clear(message.rid);
-
 		if (!result) {
 			this.updateRoom(message.rid, { isLoading: false });
+			if (!this.isLoaded(message.rid)) {
+				await this.getMore(message.rid);
+			}
 			return;
 		}
-		const { messages = [] } = result;
 
-		if (messages.length > 0) {
-			room.oldestTs = messages[messages.length - 1].ts;
-		}
+		this.clear(message.rid);
+		this.updateRoom(message.rid, { isLoading: true });
 
 		await upsertMessageBulk({ msgs: Array.from(result.messages).filter((msg) => msg.t !== 'command'), subscription });
 
 		this.emit('loaded-messages');
-		this.updateRoom(message.rid, { isLoading: false });
 
-		if (!room.loaded) {
-			room.loaded = 0;
-		}
-		room.loaded += result.messages.length;
 		this.updateRoom(message.rid, {
+			isLoading: false,
+			oldestTs: result.messages.at(-1)?.ts,
+			loaded: (room.loaded ?? 0) + result.messages.length,
 			hasMore: result.moreBefore,
 			hasMoreNext: result.moreAfter,
 		});

apps/meteor/client/views/room/MessageList/hooks/useTryToJumpToMessage.spec.tsx

@@ -21,6 +21,7 @@ jest.mock('../../hooks/useGoToRoom', () => ({
 jest.mock('../../../../providers/RouterProvider', () => ({
 	router: {
 		getSearchParameters: jest.fn().mockReturnValue({}),
+		navigate: jest.fn(),
 	},
 }));
 
@@ -30,6 +31,7 @@ const mockedSetIsJumpingToMessage = jest.fn();
 
 beforeEach(() => {
 	jest.mocked(useGoToRoom).mockReturnValue(mockedGoToRoom);
+	mockedRoomHistoryManager.getSurroundingChannelMessages.mockResolvedValue(undefined);
 });
 
 afterEach(() => {
@@ -48,7 +50,12 @@ const renderJumpHook = (rid: string, message: unknown) =>
 		{
 			wrapper: mockAppRoot()
 				.withRouter({ getSearchParameters: () => ({ msg: 'msg-1' }) })
-				.withEndpoint('GET', '/v1/chat.getMessage', (() => ({ message })) as any)
+				.withEndpoint('GET', '/v1/chat.getMessage', (() => {
+					if (message instanceof Error) {
+						throw message;
+					}
+					return { message };
+				}) as any)
 				.wrap((children) => (
 					<RoomContext.Provider
 						value={
@@ -92,6 +99,15 @@ it('should load surrounding messages in place when the message belongs to the cu
 	expect(mockedGoToRoom).not.toHaveBeenCalled();
 });
 
+it('should discard the jump when the target message cannot be fetched', async () => {
+	renderJumpHook('room-1', new Error('error-invalid-message'));
+
+	await waitFor(() => expect(mockedSetIsJumpingToMessage).toHaveBeenCalledWith(false));
+
+	expect(mockedRoomHistoryManager.getSurroundingChannelMessages).not.toHaveBeenCalled();
+	expect(mockedGoToRoom).not.toHaveBeenCalled();
+});
+
 it('should not navigate for a cross-room thread message, as it is handled by useTryToJumpToThreadMessage', async () => {
 	renderJumpHook('room-1', { ...message, rid: 'room-2', tmid: 'parent-msg-1', tshow: true });
 
@@ -100,3 +116,26 @@ it('should not navigate for a cross-room thread message, as it is handled by use
 	expect(mockedGoToRoom).not.toHaveBeenCalled();
 	expect(mockedRoomHistoryManager.getSurroundingChannelMessages).not.toHaveBeenCalled();
 });
+
+it('should request surrounding messages only once while the target stays out of the list', async () => {
+	const { rerender } = renderJumpHook('room-1', { ...message, rid: 'room-1' });
+
+	await waitFor(() => expect(mockedRoomHistoryManager.getSurroundingChannelMessages).toHaveBeenCalledTimes(1));
+
+	rerender();
+	rerender();
+
+	expect(mockedRoomHistoryManager.getSurroundingChannelMessages).toHaveBeenCalledTimes(1);
+});
+
+it('should stop jumping when the surrounding messages request fails', async () => {
+	mockedRoomHistoryManager.getSurroundingChannelMessages.mockRejectedValue(new Error('too-many-requests'));
+
+	const { rerender } = renderJumpHook('room-1', { ...message, rid: 'room-1' });
+
+	await waitFor(() => expect(mockedSetIsJumpingToMessage).toHaveBeenLastCalledWith(false));
+
+	rerender();
+
+	expect(mockedRoomHistoryManager.getSurroundingChannelMessages).toHaveBeenCalledTimes(1);
+});

apps/meteor/client/views/room/MessageList/hooks/useTryToJumpToMessage.ts

@@ -29,7 +29,7 @@ const useTryToJumpToMessage = ({ rid, virtualizerRef, setIsJumpingToMessage, mes
 
 	const goToRoom = useGoToRoom();
 
-	const { data: message } = useQuery({
+	const { data: message, isError } = useQuery({
 		queryKey: messageJumpParam ? messagesQueryKeys.message(messageJumpParam) : [],
 		queryFn: async () => {
 			if (!messageJumpParam) return null;
@@ -39,18 +39,36 @@ const useTryToJumpToMessage = ({ rid, virtualizerRef, setIsJumpingToMessage, mes
 		enabled: !!messageJumpParam,
 	});
 
+	const isThreadReply = !!message && isThreadMessage(message) && !isThreadMainMessage(message) && message.tshow !== true;
+	const targetId = isThreadReply ? message.tmid : messageJumpParam;
+
+	useEffect(() => {
+		if (!targetId || !message) {
+			return;
+		}
+
+		setIsJumpingToMessage(true);
+
+		if (message.rid !== rid) {
+			return;
+		}
+
+		void RoomHistoryManager.getSurroundingChannelMessages({ _id: targetId, rid })
+			.catch(() => undefined)
+			.finally(() => setIsJumpingToMessage(false));
+	}, [targetId, rid, message, setIsJumpingToMessage]);
+
 	useEffect(() => {
 		if (!messageJumpParam) {
 			setIsJumpingToMessage(false);
 			return;
 		}
-		if (!message) {
+		if (isError) {
+			setIsJumpingToMessage(false);
+			setMessageJumpQueryStringParameter(null);
 			return;
 		}
-		// Thread deep links are handled by useTryToJumpToThreadMessage; do not use the main list virtualizer
-		// If tshow is true, there is a preview on the main list, in this case we scroll to it
-		if (message && isThreadMessage(message) && !isThreadMainMessage(message) && message.tshow !== true) {
-			setIsJumpingToMessage(false);
+		if (!message) {
 			return;
 		}
 		if (!isThreadMessage(message) && !isThreadMainMessage(message) && message.rid !== rid) {
@@ -61,38 +79,35 @@ const useTryToJumpToMessage = ({ rid, virtualizerRef, setIsJumpingToMessage, mes
 		if (!virtualizerRef.current) {
 			return;
 		}
-		setIsJumpingToMessage(true);
 
 		if (isLoadingMoreMessages || messages.length === 0) {
 			return;
 		}
-		const loadedMessage = messages.find((message) => message._id === messageJumpParam);
-		if (!loadedMessage) {
-			// Do not load surrounding messages for thread messages that have a tshow: true
-			// as these are previews on the main list and will be handled by useTryToJumpToThreadMessage
-			if (message && (!isThreadMessage(message) || isThreadMainMessage(message))) {
-				RoomHistoryManager.getSurroundingChannelMessages(message);
-			}
+
+		const targetIndex = targetId ? messages.findIndex((current) => current._id === targetId) : -1;
+
+		if (!targetId || targetIndex < 0) {
 			return;
 		}
-		const messageIndex = messages.indexOf(loadedMessage);
 
 		// TODO: Calculate the offset of the page, for the message to be in the center of the page
-		virtualizerRef.current?.scrollToIndex(messageIndex, {
+		virtualizerRef.current?.scrollToIndex(targetIndex, {
 			align: 'center',
 		});
 
-		setHighlightMessage(loadedMessage._id);
+		setHighlightMessage(targetId);
 
 		setTimeout(() => {
 			clearHighlightMessage();
 		}, 2000);
 
 		setTimeout(() => {
 			setIsJumpingToMessage(false);
-			setMessageJumpQueryStringParameter(null);
+			if (targetId === messageJumpParam) {
+				setMessageJumpQueryStringParameter(null);
+			}
 		}, 500);
-	}, [messageJumpParam, virtualizerRef, setIsJumpingToMessage, rid, messages, message, isLoadingMoreMessages, goToRoom]);
+	}, [messageJumpParam, virtualizerRef, setIsJumpingToMessage, rid, messages, message, isError, isLoadingMoreMessages, targetId, goToRoom]);
 };
 
 export default useTryToJumpToMessage;

apps/meteor/client/views/room/providers/RoomProvider.tsx

@@ -1,4 +1,5 @@
 import type { IRoom } from '@rocket.chat/core-typings';
+import { useSearchParameter } from '@rocket.chat/ui-contexts';
 import type { ReactNode, ContextType } from 'react';
 import { useMemo, memo, useEffect } from 'react';
 
@@ -29,6 +30,8 @@ export type RoomProviderProps = {
 const RoomProvider = ({ rid, children }: RoomProviderProps) => {
 	const room = Rooms.use((state) => state.get(rid));
 
+	const messageJumpParam = useSearchParameter('msg');
+
 	const subscritionFromLocal = Subscriptions.use((state) => state.find((record) => record.rid === rid));
 
 	useRedirectOnSettingsChanged(subscritionFromLocal);
@@ -85,14 +88,11 @@ const RoomProvider = ({ rid, children }: RoomProviderProps) => {
 	// Prefetch first batch of history in parallel with room metadata fetches, instead of waiting
 	// for RoomBody's scroll/resize observer in useGetMore to fire.
 	useEffect(() => {
-		if (!room) {
-			return;
-		}
-		if (RoomHistoryManager.isLoaded(rid) || RoomHistoryManager.isLoading(rid)) {
+		if (!room || messageJumpParam || RoomHistoryManager.isLoaded(rid) || RoomHistoryManager.isLoading(rid)) {
 			return;
 		}
 		void RoomHistoryManager.getMore(rid);
-	}, [rid, room]);
+	}, [rid, room, messageJumpParam]);
 
 	// Prefetch room roles alongside history so message rendering doesn't trigger a late fetch.
 	useRoomRolesQuery(rid, { enabled: !!room });

apps/meteor/server/meteor-methods/messages/loadSurroundingMessages.ts

@@ -7,6 +7,7 @@ import type { FindOptions } from 'mongodb';
 
 import { canAccessRoomIdAsync } from '../../lib/authorization/canAccessRoom';
 import { normalizeMessagesForUser } from '../../lib/utils/lib/normalizeMessagesForUser';
+import { settings } from '../../settings';
 
 declare module '@rocket.chat/ddp-client' {
 	// eslint-disable-next-line @typescript-eslint/naming-convention
@@ -31,14 +32,14 @@ Meteor.methods<ServerMethods>({
 		check(limit, Number);
 		check(showThreadMessages, Boolean);
 
-		if (!Meteor.userId()) {
+		const fromId = Meteor.userId() ?? undefined;
+
+		if (!fromId && settings.get('Accounts_AllowAnonymousRead') === false) {
 			throw new Meteor.Error('error-invalid-user', 'Invalid user', {
 				method: 'loadSurroundingMessages',
 			});
 		}
 
-		const fromId = Meteor.userId() ?? undefined;
-
 		if (!message._id) {
 			return false;
 		}

apps/meteor/tests/unit/server/meteor-methods/messages/loadSurroundingMessages.spec.ts

@@ -0,0 +1,103 @@
+import { MeteorError } from '@rocket.chat/core-services';
+import { expect } from 'chai';
+import { beforeEach, describe, it } from 'mocha';
+import p from 'proxyquire';
+import sinon from 'sinon';
+
+const checkMock = sinon.stub();
+const meteorUserIdMock = sinon.stub();
+const meteorMethodsMock = sinon.stub();
+const canAccessRoomIdMock = sinon.stub();
+const normalizeMessagesForUserMock = sinon.stub();
+const settingsGetMock = sinon.stub();
+
+// the method pushes into the array it gets back, so each call needs a fresh one
+const emptyCursor = () => ({ toArray: async () => [] });
+
+const modelsMock = {
+	Messages: {
+		findOneById: sinon.stub(),
+		findVisibleByRoomIdBeforeTimestamp: sinon.stub().callsFake(emptyCursor),
+		findVisibleByRoomIdAfterTimestamp: sinon.stub().callsFake(emptyCursor),
+	},
+};
+
+p.noCallThru().load('../../../../../server/meteor-methods/messages/loadSurroundingMessages', {
+	'meteor/meteor': {
+		Meteor: {
+			userId: meteorUserIdMock,
+			Error: MeteorError,
+			methods: meteorMethodsMock,
+		},
+	},
+	'meteor/check': {
+		check: checkMock,
+	},
+	'@rocket.chat/models': modelsMock,
+	'../../lib/authorization/canAccessRoom': {
+		canAccessRoomIdAsync: canAccessRoomIdMock,
+	},
+	'../../lib/utils/lib/normalizeMessagesForUser': {
+		normalizeMessagesForUser: normalizeMessagesForUserMock,
+	},
+	'../../settings': {
+		settings: { get: settingsGetMock },
+	},
+});
+
+const loadSurroundingMessagesMethod = meteorMethodsMock.firstCall.args[0].loadSurroundingMessages;
+
+const mainMessage = { _id: 'msg123', rid: 'room123', ts: new Date('2024-01-01T00:00:00Z') };
+
+describe('loadSurroundingMessages', () => {
+	beforeEach(() => {
+		checkMock.resetHistory();
+		meteorUserIdMock.reset();
+		canAccessRoomIdMock.reset();
+		normalizeMessagesForUserMock.reset();
+		settingsGetMock.reset();
+		modelsMock.Messages.findOneById.reset();
+	});
+
+	it('should throw for an anonymous user when anonymous read is disabled', async () => {
+		meteorUserIdMock.returns(null);
+		settingsGetMock.withArgs('Accounts_AllowAnonymousRead').returns(false);
+
+		await expect(loadSurroundingMessagesMethod({ _id: 'msg123', rid: 'room123' })).to.be.rejectedWith('Invalid user');
+	});
+
+	it('should load the surrounding messages for an anonymous user when anonymous read is enabled', async () => {
+		meteorUserIdMock.returns(null);
+		settingsGetMock.withArgs('Accounts_AllowAnonymousRead').returns(true);
+		modelsMock.Messages.findOneById.resolves(mainMessage);
+		canAccessRoomIdMock.resolves(true);
+
+		const result = await loadSurroundingMessagesMethod({ _id: 'msg123', rid: 'room123' });
+
+		expect(result).to.not.be.false;
+		expect(result.messages).to.deep.equal([mainMessage]);
+		expect(canAccessRoomIdMock.calledOnceWith('room123', undefined)).to.be.true;
+		expect(normalizeMessagesForUserMock.called).to.be.false;
+	});
+
+	it('should not load the surrounding messages when the room is out of reach', async () => {
+		meteorUserIdMock.returns(null);
+		settingsGetMock.withArgs('Accounts_AllowAnonymousRead').returns(true);
+		modelsMock.Messages.findOneById.resolves(mainMessage);
+		canAccessRoomIdMock.resolves(false);
+
+		expect(await loadSurroundingMessagesMethod({ _id: 'msg123', rid: 'room123' })).to.be.false;
+	});
+
+	it('should normalize the messages for a logged in user', async () => {
+		meteorUserIdMock.returns('user123');
+		modelsMock.Messages.findOneById.resolves(mainMessage);
+		canAccessRoomIdMock.resolves(true);
+		normalizeMessagesForUserMock.resolves([mainMessage]);
+
+		await loadSurroundingMessagesMethod({ _id: 'msg123', rid: 'room123' });
+
+		expect(settingsGetMock.called).to.be.false;
+		expect(normalizeMessagesForUserMock.calledOnceWith([mainMessage], 'user123')).to.be.true;
+	});
+});