fix: less strict slash command registration condition

.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

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

@@ -0,0 +1,185 @@
+import type { SlashCommand } from '@rocket.chat/core-typings';
+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';
+
+const mockSlashCommands: SlashCommand[] = [
+	{
+		command: '/test',
+		description: 'Test command',
+		params: 'param1 param2',
+		clientOnly: false,
+		providesPreview: false,
+		appId: 'test-app-1',
+		permission: undefined,
+	},
+	{
+		command: '/weather',
+		description: 'Get weather information',
+		params: 'city',
+		clientOnly: false,
+		providesPreview: true,
+		appId: 'weather-app',
+		permission: undefined,
+	},
+];
+
+const mockApiResponse = {
+	commands: mockSlashCommands,
+	total: mockSlashCommands.length,
+};
+
+describe('useAppSlashCommands', () => {
+	let mockGetSlashCommands: jest.Mock;
+
+	beforeEach(() => {
+		mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse);
+
+		slashCommands.commands = {};
+	});
+
+	afterEach(() => {
+		jest.clearAllMocks();
+	});
+
+	it('should not fetch data when user ID is not available', () => {
+		renderHook(() => useAppSlashCommands(), {
+			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 () => {
+		renderHook(() => useAppSlashCommands(), {
+			wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).withJohnDoe().build(),
+		});
+
+		await waitFor(() => {
+			expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
+		});
+	});
+
+	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(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
+		});
+
+		streamRef.controller?.emit('apps', [['command/removed', ['/test']]]);
+
+		expect(slashCommands.commands['/test']).toBeUndefined();
+		expect(slashCommands.commands['/weather']).toBeDefined();
+	});
+
+	it('should handle command/added 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(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
+		});
+
+		mockGetSlashCommands.mockResolvedValue({
+			commands: [
+				...mockSlashCommands,
+				{
+					command: '/newcommand',
+					description: 'New command',
+					params: 'param1 param2',
+					clientOnly: false,
+				},
+			],
+			total: mockSlashCommands.length + 1,
+		});
+
+		streamRef.controller?.emit('apps', [['command/added', ['/newcommand']]]);
+
+		await waitFor(() => {
+			expect(slashCommands.commands['/newcommand']).toBeDefined();
+		});
+
+		expect(slashCommands.commands['/test']).toBeDefined();
+		expect(slashCommands.commands['/weather']).toBeDefined();
+	});
+
+	it('should handle command/updated 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(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
+		});
+
+		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);
+		});
+
+		streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]);
+
+		expect(slashCommands.commands['/test']).toBeDefined();
+		expect(slashCommands.commands['/weather']).toBeDefined();
+	});
+
+	it('should not set up stream listener when user ID is not available', () => {
+		const streamRef: StreamControllerRef<'apps'> = {};
+
+		renderHook(() => useAppSlashCommands(), {
+			wrapper: mockAppRoot().withStream('apps', streamRef).build(),
+		});
+
+		expect(streamRef.controller).toBeDefined();
+		expect(streamRef.controller?.has('apps')).toBe(false);
+	});
+});

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];
 				}

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);
 
@@ -825,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);
@@ -1049,6 +1051,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 +1089,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,15 +1169,14 @@ 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);
 			this.listenerManager.releaseEssentialEvents(app);
 			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) {

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());

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();