diff --git a/.changeset/fuzzy-teachers-juggle.md b/.changeset/fuzzy-teachers-juggle.md new file mode 100644 index 00000000000..fce7559bbcb --- /dev/null +++ b/.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 diff --git a/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx index 1640e50d918..058e2c8dc4d 100644 --- a/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx +++ b/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); + }); + }); }); diff --git a/apps/meteor/client/hooks/useAppSlashCommands.ts b/apps/meteor/client/hooks/useAppSlashCommands.ts index 0425d5172c4..0f2a1123c0b 100644 --- a/apps/meteor/client/hooks/useAppSlashCommands.ts +++ b/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; @@ -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') { - delete slashCommands.commands[command]; - } - invalidate(); + 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 => { 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)); }; diff --git a/apps/meteor/client/lib/chats/flows/processSlashCommand.ts b/apps/meteor/client/lib/chats/flows/processSlashCommand.ts index ffcdc0ad244..e27565cea29 100644 --- a/apps/meteor/client/lib/chats/flows/processSlashCommand.ts +++ b/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 }], }); diff --git a/apps/meteor/client/lib/queryKeys.ts b/apps/meteor/client/lib/queryKeys.ts index 8ac2af48d0a..a7a8cdf655a 100644 --- a/apps/meteor/client/lib/queryKeys.ts +++ b/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, +}; diff --git a/packages/mock-providers/src/MockedAppRootBuilder.tsx b/packages/mock-providers/src/MockedAppRootBuilder.tsx index 48305a3f14d..4058b91a58e 100644 --- a/packages/mock-providers/src/MockedAppRootBuilder.tsx +++ b/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): ContextType => { return {