fix: slashcommand query incorrectly removing commands from UI (#37654)

Co-authored-by: Douglas Fabris <27704687+dougfabris@users.noreply.github.com>

.changeset/fuzzy-teachers-juggle.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/mock-providers': patch
+'@rocket.chat/meteor': patch
+---
+
+Fixes an issue that could cause slashcommands to disappear for the user in certain high-availability scenarios

apps/meteor/client/hooks/useAppSlashCommands.spec.tsx

@@ -1,5 +1,6 @@
 import type { SlashCommand } from '@rocket.chat/core-typings';
 import { mockAppRoot, type StreamControllerRef } from '@rocket.chat/mock-providers';
+import { QueryClient } from '@tanstack/react-query';
 import { renderHook, waitFor } from '@testing-library/react';
 
 import { useAppSlashCommands } from './useAppSlashCommands';
@@ -33,9 +34,18 @@ const mockApiResponse = {
 
 describe('useAppSlashCommands', () => {
 	let mockGetSlashCommands: jest.Mock;
+	let queryClient: QueryClient;
 
 	beforeEach(() => {
 		mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse);
+		queryClient = new QueryClient({
+			defaultOptions: {
+				queries: { retry: false },
+				mutations: { retry: false },
+			},
+		});
+
+		jest.spyOn(queryClient, 'invalidateQueries');
 
 		slashCommands.commands = {};
 	});
@@ -69,6 +79,7 @@ describe('useAppSlashCommands', () => {
 		renderHook(() => useAppSlashCommands(), {
 			wrapper: mockAppRoot()
 				.withJohnDoe()
+				.withQueryClient(queryClient)
 				.withStream('apps', streamRef)
 				.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
 				.build(),
@@ -83,7 +94,37 @@ describe('useAppSlashCommands', () => {
 		streamRef.controller?.emit('apps', [['command/removed', ['/test']]]);
 
 		expect(slashCommands.commands['/test']).toBeUndefined();
-		expect(slashCommands.commands['/weather']).toBeDefined();
+
+		await waitFor(() => {
+			expect(queryClient.invalidateQueries).toHaveBeenCalledWith(expect.objectContaining({ queryKey: ['apps', 'slashCommands'] }));
+		});
+	});
+
+	it('should handle command/disabled event by invalidating queries', async () => {
+		const streamRef: StreamControllerRef<'apps'> = {};
+
+		renderHook(() => useAppSlashCommands(), {
+			wrapper: mockAppRoot()
+				.withJohnDoe()
+				.withQueryClient(queryClient)
+				.withStream('apps', streamRef)
+				.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
+				.build(),
+		});
+
+		expect(streamRef.controller).toBeDefined();
+
+		await waitFor(() => {
+			expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
+		});
+
+		streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]);
+
+		expect(slashCommands.commands['/test']).toBeUndefined();
+
+		await waitFor(() => {
+			expect(queryClient.invalidateQueries).toHaveBeenCalledWith(expect.objectContaining({ queryKey: ['apps', 'slashCommands'] }));
+		});
 	});
 
 	it('should handle command/added event by invalidating queries', async () => {
@@ -132,6 +173,7 @@ describe('useAppSlashCommands', () => {
 		renderHook(() => useAppSlashCommands(), {
 			wrapper: mockAppRoot()
 				.withJohnDoe()
+				.withQueryClient(queryClient)
 				.withStream('apps', streamRef)
 				.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
 				.build(),
@@ -145,29 +187,10 @@ describe('useAppSlashCommands', () => {
 
 		streamRef.controller?.emit('apps', [['command/updated', ['/test']]]);
 
-		expect(slashCommands.commands['/test']).toBeUndefined();
-		expect(slashCommands.commands['/weather']).toBeDefined();
-	});
-
-	it('should ignore command/disabled event', async () => {
-		const streamRef: StreamControllerRef<'apps'> = {};
-
-		renderHook(() => useAppSlashCommands(), {
-			wrapper: mockAppRoot()
-				.withJohnDoe()
-				.withStream('apps', streamRef)
-				.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
-				.build(),
-		});
-
-		expect(streamRef.controller).toBeDefined();
-
 		await waitFor(() => {
-			expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
+			expect(queryClient.invalidateQueries).toHaveBeenCalledWith(expect.objectContaining({ queryKey: ['apps', 'slashCommands'] }));
 		});
 
-		streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]);
-
 		expect(slashCommands.commands['/test']).toBeDefined();
 		expect(slashCommands.commands['/weather']).toBeDefined();
 	});
@@ -182,4 +205,31 @@ describe('useAppSlashCommands', () => {
 		expect(streamRef.controller).toBeDefined();
 		expect(streamRef.controller?.has('apps')).toBe(false);
 	});
+
+	it('should fetch all commands in batches if total exceeds count', async () => {
+		const largeMockCommands: SlashCommand[] = Array.from({ length: 120 }, (_, i) => ({
+			command: `/command${i + 1}`,
+			description: `Description for command ${i + 1}`,
+			params: '',
+			clientOnly: false,
+			providesPreview: false,
+			appId: `app-${i + 1}`,
+			permission: undefined,
+		}));
+
+		mockGetSlashCommands.mockImplementation(({ offset, count }) => {
+			return Promise.resolve({
+				commands: largeMockCommands.slice(offset, offset + count),
+				total: largeMockCommands.length,
+			});
+		});
+
+		renderHook(() => useAppSlashCommands(), {
+			wrapper: mockAppRoot().withJohnDoe().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).build(),
+		});
+
+		await waitFor(() => {
+			expect(Object.keys(slashCommands.commands)).toHaveLength(largeMockCommands.length);
+		});
+	});
 });

apps/meteor/client/hooks/useAppSlashCommands.ts

@@ -5,6 +5,7 @@ import { useQueryClient, useQuery } from '@tanstack/react-query';
 import { useEffect } from 'react';
 
 import { slashCommands } from '../../app/utils/client/slashCommand';
+import { appsQueryKeys } from '../lib/queryKeys';
 
 type SlashCommandBasicInfo = Pick<SlashCommand, 'clientOnly' | 'command' | 'description' | 'params' | 'providesPreview' | 'appId'>;
 
@@ -17,7 +18,7 @@ export const useAppSlashCommands = () => {
 	const invalidate = useDebouncedCallback(
 		() => {
 			queryClient.invalidateQueries({
-				queryKey: ['apps', 'slashCommands'],
+				queryKey: appsQueryKeys.slashCommands(),
 			});
 		},
 		100,
@@ -29,19 +30,24 @@ export const useAppSlashCommands = () => {
 			return;
 		}
 		return apps('apps', ([key, [command]]) => {
-			if (['command/added', 'command/updated', 'command/removed'].includes(key)) {
-				if (typeof command === 'string') {
+			if (!key.startsWith('command/')) {
+				return;
+			}
+
+			if (['command/removed', 'command/disabled'].includes(key) && typeof command === 'string') {
 				delete slashCommands.commands[command];
 			}
+
 			invalidate();
-			}
 		});
 	}, [apps, uid, invalidate]);
 
 	const getSlashCommands = useEndpoint('GET', '/v1/commands.list');
 
 	const { data } = useQuery({
-		queryKey: ['apps', 'slashCommands'] as const,
+		queryKey: appsQueryKeys.slashCommands(),
+		enabled: !!uid,
+		structuralSharing: false,
 		queryFn: async () => {
 			const fetchBatch = async (currentOffset: number, accumulator: SlashCommandBasicInfo[] = []): Promise<SlashCommandBasicInfo[]> => {
 				const count = 50;
@@ -58,14 +64,18 @@ export const useAppSlashCommands = () => {
 
 			return fetchBatch(0);
 		},
-		enabled: !!uid,
 	});
 
-	useEffect(() => {
-		if (!data) {
-			return;
-		}
-
-		data.forEach((command) => slashCommands.add(command));
-	}, [data]);
+	/**
+	 * We're deliberately not using `useEffect` here because we want the forEach to run on every call
+	 *
+	 * What we considered:
+	 *
+	 * 1. Slash command list is really small (< 100 items)
+	 * 2. `slashCommands.add` is idempotent
+	 * 3. `slashCommands.add` doesn't trigger re-renders
+	 *
+	 * @TODO the `slashCommands` singleton should be refactored to fit the React data flow
+	 */
+	data?.forEach((command) => slashCommands.add(command));
 };

apps/meteor/client/lib/chats/flows/processSlashCommand.ts

@@ -73,7 +73,7 @@ export const processSlashCommand = async (chat: ChatAPI, message: IMessage): Pro
 		return true;
 	}
 
-	await sdk.rest.post('/v1/statistics.telemetry', {
+	void sdk.rest.post('/v1/statistics.telemetry', {
 		params: [{ eventName: 'slashCommandsStats', timestamp: Date.now(), command: commandName }],
 	});
 

apps/meteor/client/lib/queryKeys.ts

@@ -118,3 +118,8 @@ export const teamsQueryKeys = {
 		[...teamsQueryKeys.team(teamId), 'rooms-of-user', userId, options] as const,
 	listUserTeams: (userId: IUser['_id']) => [...teamsQueryKeys.all, 'listUserTeams', userId] as const,
 };
+
+export const appsQueryKeys = {
+	all: ['apps'] as const,
+	slashCommands: () => [...appsQueryKeys.all, 'slashCommands'] as const,
+};

packages/mock-providers/src/MockedAppRootBuilder.tsx

@@ -260,6 +260,20 @@ export class MockedAppRootBuilder {
 		permissionStatus: undefined,
 	};
 
+	private _providedQueryClient: QueryClient | undefined;
+
+	private get queryClient(): QueryClient {
+		return (
+			this._providedQueryClient ||
+			new QueryClient({
+				defaultOptions: {
+					queries: { retry: false },
+					mutations: { retry: false },
+				},
+			})
+		);
+	}
+
 	wrap(wrapper: (children: ReactNode) => ReactNode): this {
 		this.wrappers.push(wrapper);
 		return this;
@@ -634,15 +648,26 @@ export class MockedAppRootBuilder {
 		return this;
 	}
 
-	build(): JSXElementConstructor<{ children: ReactNode }> {
-		const queryClient = new QueryClient({
-			defaultOptions: {
-				queries: { retry: false },
-				mutations: { retry: false },
-			},
-		});
+	withQueryClient(client: QueryClient): this {
+		this._providedQueryClient = client;
+		return this;
+	}
 
-		const { server, router, settings, user, userPresence, videoConf, i18n, authorization, wrappers, deviceContext, authentication } = this;
+	build(): JSXElementConstructor<{ children: ReactNode }> {
+		const {
+			queryClient,
+			server,
+			router,
+			settings,
+			user,
+			userPresence,
+			videoConf,
+			i18n,
+			authorization,
+			wrappers,
+			deviceContext,
+			authentication,
+		} = this;
 
 		const reduceTranslation = (translation?: ContextType<typeof TranslationContext>): ContextType<typeof TranslationContext> => {
 			return {