test(editor): Ratchet shell modal keys and catch lost registrations in CI (no-changelog) (#36324)

Co-authored-by: multica-agent <github@multica.ai>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

.github/workflows/ci-pull-requests.yml

@@ -102,6 +102,8 @@ jobs:
               packages/testing/containers/**
             dev-server-smoke:
               packages/frontend/editor-ui/vite.config.mts
+              # If this job becomes too slow, use a smaller glob. Do not delete the assertion.
+              packages/frontend/editor-ui/src/**
               pnpm-workspace.yaml
               packages/@n8n/*/package.json
               packages/frontend/@n8n/frontend-vite-config/**

packages/frontend/editor-ui/eslint.config.mjs

@@ -249,6 +249,49 @@ export default defineConfig(
 			'no-restricted-syntax': 'off',
 		},
 	},
+	{
+		// This is half 1 of 2 of the modal-key ratchet (CAT-3688).
+		// Change the level to 'error' when CAT-3973 is complete.
+		// This file does not use workflowsStore. So this rule replaces the
+		// package-wide list safely.
+		files: ['src/app/constants/modals.ts'],
+		rules: {
+			'no-restricted-syntax': [
+				'warn',
+				{
+					selector:
+						'ExportNamedDeclaration > VariableDeclaration > VariableDeclarator[id.name!=/^MODAL_(CANCEL|CONFIRM|CLOSE)$/]',
+					message:
+						'Do not declare a modal key here. Declare the key in the constants file of the feature that owns it. Then register the modal from the modals.ts fragment of that feature. To see an example, open src/features/core/auth/modals.ts. If the shell owns the modal, declare its key next to its fragment in src/app/modals.manifest.ts. Only MODAL_CANCEL, MODAL_CONFIRM and MODAL_CLOSE stay here. These three are dialog result sentinels, not modal keys.',
+				},
+				{
+					// This selector matches `export { X } from '...'` and also a bare
+					// `export { X }` list.
+					selector: 'ExportNamedDeclaration[specifiers.length>0]',
+					message:
+						'Do not re-export a modal key from the shell. A re-export makes @/app/constants an import path for a key that the shell does not own. The shell must not get such a key again. Import the key directly from its feature or from its package.',
+				},
+			],
+		},
+	},
+	{
+		// This is half 2 of 2 of the modal-key ratchet (CAT-3688).
+		// The selector matches only the direct members, so you can still change the
+		// nested state of each entry. `sneakyModal:` opens the same as `[KEY]:`.
+		// So the selector `[computed=true]` was too narrow.
+		files: ['src/app/stores/defaults/modals.ts'],
+		rules: {
+			'no-restricted-syntax': [
+				'warn',
+				{
+					selector:
+						"VariableDeclarator[id.name='SHELL_MODAL_INITIAL_STATE'] > CallExpression > ObjectExpression > :matches(Property, SpreadElement)",
+					message:
+						'Do not add an entry to SHELL_MODAL_INITIAL_STATE. This catalogue can only become smaller. Write a ModalDefinition for the modal in the modals.ts fragment of its feature. Give the definition a component and an initialState. Then modalRegistry registers the modal, and DynamicModalLoader shows it. In the same change, delete the <ModalRoot> of the modal from Modals.vue. To see an example, open src/features/core/auth/modals.ts.',
+				},
+			],
+		},
+	},
 	{
 		files: ['src/features/agents/**/*.ts', 'src/features/agents/**/*.vue'],
 		rules: {

packages/frontend/editor-ui/src/app/__tests__/fixtures/exampleFeature/ExampleFeatureModal.vue

@@ -0,0 +1,10 @@
+<script setup lang="ts">
+defineProps<{
+	modalName: string;
+	open: boolean;
+}>();
+</script>
+
+<template>
+	<div data-test-id="example-feature-modal">Example feature modal ({{ modalName }})</div>
+</template>

packages/frontend/editor-ui/src/app/__tests__/fixtures/exampleFeature/exampleFeature.constants.ts

@@ -0,0 +1 @@
+export const EXAMPLE_FEATURE_MODAL_KEY = 'exampleFeatureModal';

packages/frontend/editor-ui/src/app/__tests__/fixtures/exampleFeature/modals.ts

@@ -0,0 +1,12 @@
+import type { ModalDefinition } from '@n8n/frontend-module-sdk';
+
+import { EXAMPLE_FEATURE_MODAL_KEY } from './exampleFeature.constants';
+
+/** This fixture has the same shape as a real fragment. See `src/features/core/auth/modals.ts`. */
+export const EXAMPLE_FEATURE_MODALS: ModalDefinition[] = [
+	{
+		key: EXAMPLE_FEATURE_MODAL_KEY,
+		component: async () => await import('./ExampleFeatureModal.vue'),
+		initialState: { open: false },
+	},
+];

packages/frontend/editor-ui/src/app/__tests__/modal-definition.test.ts

@@ -0,0 +1,90 @@
+import { modalRegistry } from '@n8n/frontend-module-sdk';
+import { screen, waitFor } from '@testing-library/vue';
+import { createPinia, setActivePinia } from 'pinia';
+
+import { createComponentRenderer } from '@/__tests__/render';
+import DynamicModalLoader from '@/app/components/DynamicModalLoader.vue';
+import * as shellConstants from '@/app/constants/modals';
+import { SHELL_MODAL_INITIAL_STATE } from '@/app/stores/defaults/modals';
+import { useUIStore } from '@/app/stores/ui.store';
+
+import { EXAMPLE_FEATURE_MODAL_KEY } from './fixtures/exampleFeature/exampleFeature.constants';
+import { EXAMPLE_FEATURE_MODALS } from './fixtures/exampleFeature/modals';
+
+/**
+ * A new modal needs no change to `ui.store` or `app/constants`.
+ *
+ * The first test registers a fixture modal and opens it on the screen. The second
+ * test makes sure that the shell does not define the same key. Without the second
+ * test, the first test can pass because the shell defines the modal.
+ *
+ * The fixture is not in `src/features/`, because `modals.manifest.ts` imports
+ * every real feature.
+ */
+
+const renderLoader = createComponentRenderer(DynamicModalLoader);
+
+describe('adding a modal through a fragment', () => {
+	let pinia: ReturnType<typeof createPinia>;
+
+	beforeEach(() => {
+		pinia = createPinia();
+		setActivePinia(pinia);
+		modalRegistry.clear();
+	});
+
+	const registerFixtureFragment = () => {
+		// `registerEagerModals` and `registerModuleModals` use this same loop on real
+		// fragments. `modals.manifest.test.ts` and `moduleInitializer.test.ts` test
+		// those two functions.
+		EXAMPLE_FEATURE_MODALS.forEach((modal) => modalRegistry.register(modal));
+	};
+
+	it('registers, renders and opens — with no shell edit anywhere in the path', async () => {
+		const uiStore = useUIStore();
+
+		expect(modalRegistry.has(EXAMPLE_FEATURE_MODAL_KEY)).toBe(false);
+		expect(uiStore.modalsById[EXAMPLE_FEATURE_MODAL_KEY]).toEqual({ open: false });
+
+		registerFixtureFragment();
+
+		expect(modalRegistry.has(EXAMPLE_FEATURE_MODAL_KEY)).toBe(true);
+		expect(uiStore.modalsById[EXAMPLE_FEATURE_MODAL_KEY]).toEqual({ open: false });
+
+		// A second pinia gives the component a different ui.store.
+		renderLoader({ pinia });
+
+		expect(screen.queryByTestId('example-feature-modal')).not.toBeInTheDocument();
+
+		uiStore.openModal(EXAMPLE_FEATURE_MODAL_KEY);
+
+		await waitFor(() => {
+			expect(screen.queryByTestId('example-feature-modal')).toBeInTheDocument();
+		});
+		expect(uiStore.isModalActiveById[EXAMPLE_FEATURE_MODAL_KEY]).toBe(true);
+
+		uiStore.closeModal(EXAMPLE_FEATURE_MODAL_KEY);
+
+		await waitFor(() => {
+			expect(screen.queryByTestId('example-feature-modal')).not.toBeInTheDocument();
+		});
+	});
+
+	it('opens without tripping the unknown-key warning', () => {
+		// If a warning occurs, the fixture opened through the self-registration in
+		// `openModal`, and not through the fragment.
+		const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
+
+		registerFixtureFragment();
+		useUIStore().openModal(EXAMPLE_FEATURE_MODAL_KEY);
+
+		expect(warn).not.toHaveBeenCalled();
+
+		warn.mockRestore();
+	});
+
+	it('is defined in neither shell surface', () => {
+		expect(Object.keys(shellConstants)).not.toContain('EXAMPLE_FEATURE_MODAL_KEY');
+		expect(Object.keys(SHELL_MODAL_INITIAL_STATE)).not.toContain(EXAMPLE_FEATURE_MODAL_KEY);
+	});
+});

packages/frontend/editor-ui/src/app/__tests__/modal-key-ratchet.test.ts

@@ -0,0 +1,139 @@
+import * as shellConstants from '@/app/constants/modals';
+import { SHELL_MODAL_INITIAL_STATE } from '@/app/stores/defaults/modals';
+
+/**
+ * The two modal-key surfaces of the shell can become smaller, but never larger
+ * (CAT-3688).
+ *
+ * `eslint.config.mjs` bans the same shapes, but only at level `warn` until
+ * CAT-3973 removes the remaining entries. So this test is the gate that fails.
+ *
+ * This test reads both lists from the modules at runtime. It does not parse the
+ * source text. So it counts an entry in the same way as the application.
+ *
+ * The two lists below are the CAT-3973 backlog. When a modal moves to its
+ * feature, delete its name from the list. Both lists are empty at the end.
+ */
+
+/** These are dialog result sentinels, not modal keys. They stay after the migration. */
+const RESULT_SENTINELS: string[] = ['MODAL_CANCEL', 'MODAL_CONFIRM', 'MODAL_CLOSE'];
+
+const ALLOWED_KEY_EXPORTS = [
+	'ABOUT_MODAL_KEY',
+	'ADD_EXECUTION_TO_DATASET_MODAL_KEY',
+	'AI_BUILDER_DIFF_MODAL_KEY',
+	'AI_GATEWAY_TOP_UP_MODAL_KEY',
+	'BINARY_DATA_VIEW_MODAL_KEY',
+	'CHAT_EMBED_MODAL_KEY',
+	'CREDENTIAL_RESOLVER_EDIT_MODAL_KEY',
+	'DELETE_SECRETS_PROVIDER_MODAL_KEY',
+	'DUPLICATE_MODAL_KEY',
+	'EXPERIMENT_TEMPLATE_RECO_V2_KEY',
+	'EXPERIMENT_TEMPLATE_RECO_V3_KEY',
+	'EXTERNAL_SECRETS_PROVIDER_MODAL_KEY',
+	'FROM_AI_PARAMETERS_MODAL_KEY',
+	'IMPORT_CURL_MODAL_KEY',
+	'IMPORT_WORKFLOW_URL_MODAL_KEY',
+	'LOG_STREAM_MODAL_KEY',
+	'MIGRATE_WORKFLOW_MODAL_KEY',
+	'NEW_ASSISTANT_SESSION_MODAL',
+	'NPS_SURVEY_MODAL_KEY',
+	'SECRETS_PROVIDER_CONNECTION_MODAL_KEY',
+	'SETUP_CREDENTIALS_MODAL_KEY',
+	'STOP_MANY_EXECUTIONS_MODAL_KEY',
+	'TRIAL_INTRO_MODAL_KEY',
+	'VERSIONS_MODAL_KEY',
+	'WHATS_NEW_MODAL_KEY',
+	'WORKFLOW_ACTIVATION_CONFLICTING_WEBHOOK_MODAL_KEY',
+	'WORKFLOW_ACTIVE_MODAL_KEY',
+	'WORKFLOW_DESCRIPTION_MODAL_KEY',
+	'WORKFLOW_DIFF_MODAL_KEY',
+	'WORKFLOW_EXTRACTION_NAME_MODAL_KEY',
+	'WORKFLOW_HISTORY_DIFF_MODAL_KEY',
+	'WORKFLOW_HISTORY_NAME_VERSION_MODAL_KEY',
+	'WORKFLOW_HISTORY_PUBLISH_MODAL_KEY',
+	'WORKFLOW_HISTORY_VERSION_UNPUBLISH',
+	'WORKFLOW_PUBLISH_MODAL_KEY',
+	'WORKFLOW_SETTINGS_MODAL_KEY',
+	'WORKFLOW_SHARE_MODAL_KEY',
+];
+
+const ALLOWED_CATALOGUE_KEYS = [
+	'about',
+	'activation',
+	'addExecutionToDataset',
+	'aiBuilderDiff',
+	'aiGatewayTopUp',
+	'annotationTagsManager',
+	'binaryDataView',
+	'chatEmbed',
+	'communityPackageInstall',
+	'communityPackageManageConfirm',
+	'communityPlusEnrollment',
+	'createOrEditApiKey',
+	'credentialResolverEdit',
+	'debugPaywall',
+	'deleteFolder',
+	'deleteSecretsProvider',
+	'deleteUser',
+	'duplicate',
+	'editCredential',
+	'externalSecretsProvider',
+	'fromAiParameters',
+	'importCurl',
+	'importWorkflowUrl',
+	'inviteUser',
+	'migrateWorkflow',
+	'moveFolder',
+	'newAssistantSession',
+	'npsSurvey',
+	'personalization',
+	'projectMoveResourceModal',
+	'secretsProviderConnection',
+	'selectCredential',
+	'settings',
+	'settingsLogStream',
+	'setupCredentials',
+	'sourceControlPull',
+	'sourceControlPullResult',
+	'sourceControlPush',
+	'stopManyExecutions',
+	'tagsManager',
+	'templateRecoV2',
+	'templateRecoV3',
+	'trialIntroModal',
+	'variableModal',
+	'versions',
+	'whatsNew',
+	'workflowActivationConflictingWebhook',
+	'workflowDescription',
+	'workflowDiff',
+	'workflowExtractionName',
+	'workflowHistoryDiff',
+	'workflowHistoryNameVersion',
+	'workflowHistoryPublish',
+	'workflowHistoryVersionUnpublish',
+	'workflowPublish',
+	'workflowShare',
+];
+
+const shellKeyExports = () =>
+	Object.keys(shellConstants)
+		.filter((name) => !RESULT_SENTINELS.includes(name))
+		.sort();
+
+describe('modal-key ratchet', () => {
+	it('does not let the shell reacquire a modal key constant', () => {
+		expect(
+			shellKeyExports(),
+			'Declare the key in the constants file of the feature that owns it. Then register the modal from the modals.ts fragment of that feature. If a key moved to its feature, delete its name from ALLOWED_KEY_EXPORTS in this file.',
+		).toEqual(ALLOWED_KEY_EXPORTS);
+	});
+
+	it('does not let the shell reacquire a modal definition', () => {
+		expect(
+			Object.keys(SHELL_MODAL_INITIAL_STATE).sort(),
+			'Write a ModalDefinition for the modal in the fragment of its feature. Then modalRegistry registers the modal. In the same change, delete the <ModalRoot> of the modal from Modals.vue. If a modal moved to its feature, delete its key from ALLOWED_CATALOGUE_KEYS in this file.',
+		).toEqual(ALLOWED_CATALOGUE_KEYS);
+	});
+});

packages/frontend/editor-ui/src/app/constants/modals.ts

@@ -1,3 +1,5 @@
+// Modal keys belong to the feature that owns them. Only the three
+// dialog result sentinels stay here. To read the removal policy, see PR #36324.
 export const MODAL_CANCEL = 'cancel';
 export const MODAL_CONFIRM = 'confirm';
 export const MODAL_CLOSE = 'close';

packages/testing/playwright/tests/dev-server-smoke/dev-server-boots.spec.ts

@@ -11,6 +11,8 @@ import { test } from '../../fixtures/base';
  * any error-level console message or uncaught page error is observed during the
  * load. They are intentionally light — UI behaviour is covered elsewhere.
  *
+ * These tests also fail on `[modals]` warnings. See `MODAL_WARNING_RE`.
+ *
  * Must run against the Vite dev server (`N8N_EDITOR_URL` set), which is what the
  * `test:dev-server-smoke` script wires up.
  */
@@ -24,18 +26,40 @@ const BENIGN_PATTERNS: Array<{ messageRe: RegExp; reason: string }> = [
 
 const isBenign = (text: string) => BENIGN_PATTERNS.some((p) => p.messageRe.test(text));
 
+/**
+ * An unregistered modal key shows a closed modal. It does not throw an error
+ * (CAT-3967). So a lost registration does not fail any test.
+ *
+ * This warning is the only signal. `import.meta.env.DEV` removes the warning from
+ * the production bundle that the e2e job builds. Only this job starts the dev
+ * server, so only this job can find the warning.
+ *
+ * Other warnings are not fatal. The dev server writes many warnings that are not
+ * defects.
+ */
+const MODAL_WARNING_RE = /\[modals\]/;
+
 const navigateAndAssertNoErrors = async (
 	page: Page,
 	label: string,
 	navigate: () => Promise<void>,
 ) => {
 	const consoleErrors: string[] = [];
+	const modalWarnings: string[] = [];
 	const pageErrors: string[] = [];
 
 	const onConsole = (message: ConsoleMessage) => {
+		const text = message.text();
+		const at = `(at ${message.location().url ?? '<unknown>'})`;
+
+		if (message.type() === 'warning') {
+			if (MODAL_WARNING_RE.test(text)) modalWarnings.push(`${text} ${at}`);
+			return;
+		}
+
 		if (message.type() !== 'error') return;
-		if (isBenign(message.text())) return;
-		consoleErrors.push(`${message.text()} (at ${message.location().url ?? '<unknown>'})`);
+		if (isBenign(text)) return;
+		consoleErrors.push(`${text} ${at}`);
 	};
 	const onPageError = (error: Error) => {
 		const firstFrame = error.stack?.split('\n')[1]?.trim() ?? '';
@@ -65,10 +89,14 @@ const navigateAndAssertNoErrors = async (
 		page.off('pageerror', onPageError);
 	}
 
-	if (pageErrors.length > 0 || consoleErrors.length > 0) {
+	if (pageErrors.length > 0 || consoleErrors.length > 0 || modalWarnings.length > 0) {
 		const sections = [
 			pageErrors.length > 0 && `Uncaught page errors:\n  ${pageErrors.join('\n  ')}`,
 			consoleErrors.length > 0 && `Error-level console messages:\n  ${consoleErrors.join('\n  ')}`,
+			modalWarnings.length > 0 &&
+				`Modal keys nothing defines — the modal will not open, it will not throw:\n  ${modalWarnings.join(
+					'\n  ',
+				)}`,
 			navigationError &&
 				`Navigation also failed (likely a downstream effect): ${navigationError.message.split('\n')[0]}`,
 		].filter(Boolean);
@@ -86,7 +114,7 @@ test.describe(
 			{
 				type: 'description',
 				description:
-					'Boots representative routes against the Vite dev server and fails on any error-level console message or uncaught page error during load.',
+					'Boots representative routes against the Vite dev server and fails on any error-level console message, uncaught page error, or [modals] unknown-key warning during load.',
 			},
 		],
 	},