From c0cb31489b448b866d1774247d294bfd18a3d7f9 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Mon, 1 Sep 2025 18:05:25 -0300 Subject: [PATCH 1/6] Modify app slash command registering method --- apps/meteor/client/hooks/useAppSlashCommands.ts | 2 +- packages/apps-engine/src/server/AppManager.ts | 14 ++++++++++---- .../src/server/managers/AppSlashCommandManager.ts | 10 ++++++---- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/apps/meteor/client/hooks/useAppSlashCommands.ts b/apps/meteor/client/hooks/useAppSlashCommands.ts index df33dc8af6150..0425d5172c476 100644 --- a/apps/meteor/client/hooks/useAppSlashCommands.ts +++ b/apps/meteor/client/hooks/useAppSlashCommands.ts @@ -29,7 +29,7 @@ export const useAppSlashCommands = () => { return; } return apps('apps', ([key, [command]]) => { - if (['command/added', 'command/updated', 'command/disabled', 'command/removed'].includes(key)) { + if (['command/added', 'command/updated', 'command/removed'].includes(key)) { if (typeof command === 'string') { delete slashCommands.commands[command]; } diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 745aabc6105c7..2d2498af7de83 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -58,6 +58,7 @@ export interface IAppManagerDeps { interface IPurgeAppConfigOpts { keepScheduledJobs?: boolean; + keepSlashcommands?: boolean; } export class AppManager { @@ -491,7 +492,7 @@ export class AppManager { await app.call(AppMethod.ONDISABLE).catch((e) => console.warn('Error while disabling:', e)); } - await this.purgeAppConfig(app, { keepScheduledJobs: true }); + await this.purgeAppConfig(app, { keepScheduledJobs: true, keepSlashcommands: true }); await app.setStatus(status, silent); @@ -1049,6 +1050,8 @@ export class AppManager { await app.call(AppMethod.INITIALIZE); await app.setStatus(AppStatus.INITIALIZED, silenceStatus); + await this.commandManager.registerCommands(app.getID()); + result = true; } catch (e) { let status = AppStatus.ERROR_DISABLED; @@ -1085,9 +1088,13 @@ export class AppManager { if (!opts.keepScheduledJobs) { await this.schedulerManager.cleanUp(app.getID()); } + + if (!opts.keepSlashcommands) { + await this.commandManager.unregisterCommands(app.getID()); + } + this.listenerManager.unregisterListeners(app); this.listenerManager.lockEssentialEvents(app); - await this.commandManager.unregisterCommands(app.getID()); this.externalComponentManager.unregisterExternalComponents(app.getID()); await this.apiManager.unregisterApis(app.getID()); this.accessorManager.purifyApp(app.getID()); @@ -1161,7 +1168,6 @@ export class AppManager { } if (enable) { - await this.commandManager.registerCommands(app.getID()); this.externalComponentManager.registerExternalComponents(app.getID()); await this.apiManager.registerApis(app.getID()); this.listenerManager.registerListeners(app); @@ -1169,7 +1175,7 @@ export class AppManager { this.videoConfProviderManager.registerProviders(app.getID()); await this.outboundCommunicationProviderManager.registerProviders(app.getID()); } else { - await this.purgeAppConfig(app); + await this.purgeAppConfig(app, { keepScheduledJobs: true, keepSlashcommands: true }); } if (saveToDb) { diff --git a/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts b/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts index 6bdbc7bf85e95..b5c5662adf2a7 100644 --- a/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts +++ b/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts @@ -333,10 +333,12 @@ export class AppSlashCommandManager { const app = this.manager.getOneById(this.touchedCommandsToApps.get(cmd)); - if (!app || AppStatusUtils.isDisabled(await app.getStatus())) { - // Just in case someone decides to do something they shouldn't - // let's ensure the app actually exists - return; + if (!app) { + throw new Error('App not found'); + } + + if (!AppStatusUtils.isEnabled(await app.getStatus())) { + throw new Error('App not enabled'); } const appCmd = this.retrieveCommandInfo(cmd, app.getID()); From f6eb8bb22fa917c16a20a64dae1d9548be429d3c Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Mon, 1 Sep 2025 19:08:20 -0300 Subject: [PATCH 2/6] Add contextual comment --- packages/apps-engine/src/server/AppManager.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 2d2498af7de83..9b7e8fcde1a9d 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -826,6 +826,7 @@ export class AppManager { } })(); + // We don't keep slashcommands here as the update could potentially not provide the same list await this.purgeAppConfig(app, { keepScheduledJobs: true }); this.apps.set(app.getID(), app); From 6070e6693b2eab02735183a21e3313027f1a8334 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Mon, 1 Sep 2025 19:08:33 -0300 Subject: [PATCH 3/6] Update tests --- .../tests/server/managers/AppSlashCommandManager.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts b/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts index 7ac88e13ddf69..73d09c18f5e91 100644 --- a/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts +++ b/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts @@ -407,7 +407,7 @@ export class AppSlashCommandManagerTestFixture { failedItems.set('failure', asm); (ascm as any).providedCommands.set('failMePlease', failedItems); (ascm as any).touchedCommandsToApps.set('failure', 'failMePlease'); - await Expect(() => ascm.executeCommand('failure', context)).not.toThrowAsync(); + await Expect(() => ascm.executeCommand('failure', context)).toThrowAsync(); AppSlashCommandManagerTestFixture.doThrow = true; await Expect(() => ascm.executeCommand('command', context)).not.toThrowAsync(); From 17ada67d492fbf0c1917f42473d03d7071a11a24 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 2 Sep 2025 18:09:42 -0300 Subject: [PATCH 4/6] Add tests --- .../client/hooks/useAppSlashCommands.spec.tsx | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 apps/meteor/client/hooks/useAppSlashCommands.spec.tsx diff --git a/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx new file mode 100644 index 0000000000000..0bb16feb30c6f --- /dev/null +++ b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx @@ -0,0 +1,200 @@ +import type { SlashCommand } from '@rocket.chat/core-typings'; +import { useEndpoint, useStream, useUserId } from '@rocket.chat/ui-contexts'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, waitFor } from '@testing-library/react'; + +import { useAppSlashCommands } from './useAppSlashCommands'; +import { slashCommands } from '../../app/utils/client/slashCommand'; + +jest.mock('../../app/utils/client/slashCommand', () => ({ + slashCommands: { + commands: {}, + add: jest.fn(), + }, +})); + +jest.mock('@rocket.chat/fuselage-hooks', () => ({ + useDebouncedCallback: jest.fn((callback) => callback), +})); + +jest.mock('@rocket.chat/ui-contexts', () => ({ + useUserId: jest.fn(), + useStream: jest.fn(), + useEndpoint: jest.fn(), +})); + +const mockUseUserId = jest.mocked(useUserId); +const mockUseStream = jest.mocked(useStream); +const mockUseEndpoint = jest.mocked(useEndpoint); + +const mockSlashCommands: Pick[] = [ + { + command: '/test', + description: 'Test command', + params: 'param1 param2', + clientOnly: false, + providesPreview: false, + appId: 'test-app-1', + }, + { + command: '/weather', + description: 'Get weather information', + params: 'city', + clientOnly: false, + providesPreview: true, + appId: 'weather-app', + }, +]; + +const mockApiResponse = { + commands: mockSlashCommands, + total: mockSlashCommands.length, +}; + +describe('useAppSlashCommands', () => { + let queryClient: QueryClient; + let mockGetSlashCommands: jest.Mock; + let mockStreamCallback: jest.Mock; + + beforeEach(() => { + queryClient = new QueryClient({ + defaultOptions: { + queries: { + retry: false, + }, + }, + }); + + mockUseUserId.mockReturnValue('test-user-id'); + + mockStreamCallback = jest.fn(); + mockUseStream.mockReturnValue(mockStreamCallback); + + mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse); + mockUseEndpoint.mockReturnValue(mockGetSlashCommands); + + slashCommands.commands = {}; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + const renderHookWithProviders = () => { + return renderHook(() => useAppSlashCommands(), { + wrapper: ({ children }: { children: React.ReactNode }) => {children}, + }); + }; + + it('should not fetch data when user ID is not available', () => { + mockUseUserId.mockReturnValue(null); + + renderHook(() => useAppSlashCommands(), { + wrapper: ({ children }: { children: React.ReactNode }) => {children}, + }); + + expect(mockGetSlashCommands).not.toHaveBeenCalled(); + }); + + it('should fetch slash commands when user ID is available', async () => { + renderHookWithProviders(); + + await waitFor(() => { + expect(mockGetSlashCommands).toHaveBeenCalledWith({ offset: 0, count: 50 }); + }); + }); + + it('should add fetched commands to slashCommands', async () => { + renderHookWithProviders(); + + await waitFor(() => { + expect(slashCommands.add).toHaveBeenCalledTimes(mockSlashCommands.length); + mockSlashCommands.forEach((command) => { + expect(slashCommands.add).toHaveBeenCalledWith(command); + }); + }); + }); + + it('should set up stream listener for app events', () => { + renderHookWithProviders(); + + expect(mockUseStream).toHaveBeenCalledWith('apps'); + expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + }); + + it('should handle command/removed event by invalidating queries', () => { + renderHookWithProviders(); + + // Get the stream callback function + const streamCallback = mockStreamCallback.mock.calls[0][1]; + + // Simulate command/removed event + streamCallback(['command/removed', ['/test']]); + + // The hook should handle the event (we can't easily test the debounced invalidation without more complex setup) + expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + }); + + it('should handle command/added event by invalidating queries', () => { + renderHookWithProviders(); + + // Get the stream callback function + const streamCallback = mockStreamCallback.mock.calls[0][1]; + + // Simulate command/added event + streamCallback(['command/added', ['/newcommand']]); + + // The hook should handle the event + expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + }); + + it('should handle command/updated event by invalidating queries', () => { + renderHookWithProviders(); + + // Get the stream callback function + const streamCallback = mockStreamCallback.mock.calls[0][1]; + + // Simulate command/updated event + streamCallback(['command/updated', ['/updatedcommand']]); + + // The hook should handle the event + expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + }); + + it('should ignore unknown stream events', () => { + renderHookWithProviders(); + + // Get the stream callback function + const streamCallback = mockStreamCallback.mock.calls[0][1]; + + // Simulate unknown event + streamCallback(['unknown/event', ['/test']]); + + // The hook should handle the event gracefully + expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + }); + + it('should clean up stream listener when component unmounts', () => { + const mockCleanup = jest.fn(); + mockStreamCallback.mockReturnValue(mockCleanup); + + const { unmount } = renderHookWithProviders(); + + unmount(); + + expect(mockCleanup).toHaveBeenCalled(); + }); + + it('should not set up stream listener when user ID is not available', () => { + mockUseUserId.mockReturnValue(null); + + renderHook(() => useAppSlashCommands(), { + wrapper: ({ children }: { children: React.ReactNode }) => {children}, + }); + + // The useStream hook is called, but the useEffect that sets up the listener should not run + expect(mockUseStream).toHaveBeenCalledWith('apps'); + // But the stream callback should not be called because uid is null + expect(mockStreamCallback).not.toHaveBeenCalled(); + }); +}); From 1fe3cbb54106ef4f5734da837f81da1367308226 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 2 Sep 2025 19:52:21 -0300 Subject: [PATCH 5/6] Add changeset --- .changeset/old-meals-pull.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/old-meals-pull.md diff --git a/.changeset/old-meals-pull.md b/.changeset/old-meals-pull.md new file mode 100644 index 0000000000000..80a5dadf14f02 --- /dev/null +++ b/.changeset/old-meals-pull.md @@ -0,0 +1,6 @@ +--- +'@rocket.chat/apps-engine': patch +'@rocket.chat/meteor': patch +--- + +Changes a strict behavior on reporting slash commands provided by apps From f4dfac17d634f23c37152bcc34a944c9d374b2ab Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 4 Sep 2025 20:21:10 -0300 Subject: [PATCH 6/6] Improve tests --- .../client/hooks/useAppSlashCommands.spec.tsx | 203 ++++++++---------- 1 file changed, 94 insertions(+), 109 deletions(-) diff --git a/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx index 0bb16feb30c6f..1640e50d9189d 100644 --- a/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx +++ b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx @@ -1,33 +1,11 @@ import type { SlashCommand } from '@rocket.chat/core-typings'; -import { useEndpoint, useStream, useUserId } from '@rocket.chat/ui-contexts'; -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { mockAppRoot, type StreamControllerRef } from '@rocket.chat/mock-providers'; import { renderHook, waitFor } from '@testing-library/react'; import { useAppSlashCommands } from './useAppSlashCommands'; import { slashCommands } from '../../app/utils/client/slashCommand'; -jest.mock('../../app/utils/client/slashCommand', () => ({ - slashCommands: { - commands: {}, - add: jest.fn(), - }, -})); - -jest.mock('@rocket.chat/fuselage-hooks', () => ({ - useDebouncedCallback: jest.fn((callback) => callback), -})); - -jest.mock('@rocket.chat/ui-contexts', () => ({ - useUserId: jest.fn(), - useStream: jest.fn(), - useEndpoint: jest.fn(), -})); - -const mockUseUserId = jest.mocked(useUserId); -const mockUseStream = jest.mocked(useStream); -const mockUseEndpoint = jest.mocked(useEndpoint); - -const mockSlashCommands: Pick[] = [ +const mockSlashCommands: SlashCommand[] = [ { command: '/test', description: 'Test command', @@ -35,6 +13,7 @@ const mockSlashCommands: Pick { - let queryClient: QueryClient; let mockGetSlashCommands: jest.Mock; - let mockStreamCallback: jest.Mock; beforeEach(() => { - queryClient = new QueryClient({ - defaultOptions: { - queries: { - retry: false, - }, - }, - }); - - mockUseUserId.mockReturnValue('test-user-id'); - - mockStreamCallback = jest.fn(); - mockUseStream.mockReturnValue(mockStreamCallback); - mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse); - mockUseEndpoint.mockReturnValue(mockGetSlashCommands); slashCommands.commands = {}; }); @@ -80,121 +44,142 @@ describe('useAppSlashCommands', () => { jest.clearAllMocks(); }); - const renderHookWithProviders = () => { - return renderHook(() => useAppSlashCommands(), { - wrapper: ({ children }: { children: React.ReactNode }) => {children}, - }); - }; - it('should not fetch data when user ID is not available', () => { - mockUseUserId.mockReturnValue(null); - renderHook(() => useAppSlashCommands(), { - wrapper: ({ children }: { children: React.ReactNode }) => {children}, + wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).build(), }); expect(mockGetSlashCommands).not.toHaveBeenCalled(); + expect(slashCommands.commands).toEqual({}); }); it('should fetch slash commands when user ID is available', async () => { - renderHookWithProviders(); + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).withJohnDoe().build(), + }); await waitFor(() => { - expect(mockGetSlashCommands).toHaveBeenCalledWith({ offset: 0, count: 50 }); + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); }); }); - it('should add fetched commands to slashCommands', async () => { - renderHookWithProviders(); + it('should handle command/removed event by invalidating queries', 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(slashCommands.add).toHaveBeenCalledTimes(mockSlashCommands.length); - mockSlashCommands.forEach((command) => { - expect(slashCommands.add).toHaveBeenCalledWith(command); - }); + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); }); - }); - it('should set up stream listener for app events', () => { - renderHookWithProviders(); + streamRef.controller?.emit('apps', [['command/removed', ['/test']]]); - expect(mockUseStream).toHaveBeenCalledWith('apps'); - expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + expect(slashCommands.commands['/test']).toBeUndefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); }); - it('should handle command/removed event by invalidating queries', () => { - renderHookWithProviders(); + it('should handle command/added event by invalidating queries', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; - // Get the stream callback function - const streamCallback = mockStreamCallback.mock.calls[0][1]; + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); - // Simulate command/removed event - streamCallback(['command/removed', ['/test']]); + expect(streamRef.controller).toBeDefined(); - // The hook should handle the event (we can't easily test the debounced invalidation without more complex setup) - expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); - }); + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); - it('should handle command/added event by invalidating queries', () => { - renderHookWithProviders(); + mockGetSlashCommands.mockResolvedValue({ + commands: [ + ...mockSlashCommands, + { + command: '/newcommand', + description: 'New command', + params: 'param1 param2', + clientOnly: false, + }, + ], + total: mockSlashCommands.length + 1, + }); - // Get the stream callback function - const streamCallback = mockStreamCallback.mock.calls[0][1]; + streamRef.controller?.emit('apps', [['command/added', ['/newcommand']]]); - // Simulate command/added event - streamCallback(['command/added', ['/newcommand']]); + await waitFor(() => { + expect(slashCommands.commands['/newcommand']).toBeDefined(); + }); - // The hook should handle the event - expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); + expect(slashCommands.commands['/test']).toBeDefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); }); - it('should handle command/updated event by invalidating queries', () => { - renderHookWithProviders(); + it('should handle command/updated event by invalidating queries', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; - // Get the stream callback function - const streamCallback = mockStreamCallback.mock.calls[0][1]; + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); - // Simulate command/updated event - streamCallback(['command/updated', ['/updatedcommand']]); + expect(streamRef.controller).toBeDefined(); - // The hook should handle the event - expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); - }); + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); - it('should ignore unknown stream events', () => { - renderHookWithProviders(); + streamRef.controller?.emit('apps', [['command/updated', ['/test']]]); - // Get the stream callback function - const streamCallback = mockStreamCallback.mock.calls[0][1]; + expect(slashCommands.commands['/test']).toBeUndefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); + }); - // Simulate unknown event - streamCallback(['unknown/event', ['/test']]); + it('should ignore command/disabled event', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; - // The hook should handle the event gracefully - expect(mockStreamCallback).toHaveBeenCalledWith('apps', expect.any(Function)); - }); + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); - it('should clean up stream listener when component unmounts', () => { - const mockCleanup = jest.fn(); - mockStreamCallback.mockReturnValue(mockCleanup); + expect(streamRef.controller).toBeDefined(); - const { unmount } = renderHookWithProviders(); + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); - unmount(); + streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]); - expect(mockCleanup).toHaveBeenCalled(); + expect(slashCommands.commands['/test']).toBeDefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); }); it('should not set up stream listener when user ID is not available', () => { - mockUseUserId.mockReturnValue(null); + const streamRef: StreamControllerRef<'apps'> = {}; renderHook(() => useAppSlashCommands(), { - wrapper: ({ children }: { children: React.ReactNode }) => {children}, + wrapper: mockAppRoot().withStream('apps', streamRef).build(), }); - // The useStream hook is called, but the useEffect that sets up the listener should not run - expect(mockUseStream).toHaveBeenCalledWith('apps'); - // But the stream callback should not be called because uid is null - expect(mockStreamCallback).not.toHaveBeenCalled(); + expect(streamRef.controller).toBeDefined(); + expect(streamRef.controller?.has('apps')).toBe(false); }); });