From 8b22cca3a03e443927aae11cb72298fbf862da87 Mon Sep 17 00:00:00 2001 From: Alex Grozav Date: Tue, 11 Aug 2026 10:54:50 +0300 Subject: [PATCH] refactor(editor): Derive modal state from the registry instead of mirroring it (no-changelog) (#35859) Co-authored-by: Claude Opus 5 Co-authored-by: multica-agent --- .../src/registries/modalRegistry.test.ts | 32 ++ .../src/registries/modalRegistry.ts | 9 +- .../src/app/components/ChatEmbedModal.test.ts | 2 +- .../app/components/DynamicModalLoader.test.ts | 279 +++------- .../src/app/components/DynamicModalLoader.vue | 53 +- .../components/FromAiParametersModal.test.ts | 2 +- .../components/ImportWorkflowUrlModal.test.ts | 10 +- .../MainHeader/WorkflowPublishModal.test.ts | 2 +- .../src/app/components/ModalRoot.test.ts | 21 +- .../src/app/components/ModalRoot.vue | 2 +- .../app/components/VersionUpdateCTA.test.ts | 2 +- .../src/app/components/WhatsNewModal.test.ts | 2 +- .../moduleInitializer/moduleInitializer.ts | 2 - .../src/app/stores/defaults/modals.ts | 239 +++++++++ .../app/stores/ui.store.registration.spec.ts | 212 +++----- .../editor-ui/src/app/stores/ui.store.test.ts | 267 +++++++++- .../editor-ui/src/app/stores/ui.store.ts | 487 +++++------------- .../NodeRecommendationModal.test.ts | 2 +- .../AgentToolsConnectionModalWrapper.test.ts | 8 +- .../InstanceAiToolsConnectionModalWrapper.vue | 4 +- ...tanceAiToolsConnectionModalWrapper.test.ts | 22 + .../modals/MCPConnectWorkflowsModal.test.ts | 2 +- .../components/ConfirmPasswordModal.test.ts | 2 +- .../components/MoveToFolderModal.test.ts | 2 +- .../CredentialEdit/CredentialEdit.test.ts | 12 +- .../CredentialEdit/CredentialEdit.vue | 20 +- .../SecretsProviderConnectionModal.ee.test.ts | 2 +- .../components/ImportCurlModal.test.ts | 28 +- .../parameters/components/ImportCurlModal.vue | 13 +- .../components/BinaryDataViewModal.test.ts | 2 +- .../ApiKeyCreateOrEditModal.test.ts | 2 +- .../CommunityPackageInstallModal.test.ts | 2 +- ...CommunityPackageManageConfirmModal.test.ts | 2 +- .../components/VariableModal.test.ts | 2 +- .../users/components/DeleteUserModal.test.ts | 2 +- .../settings/users/users.constants.test.ts | 4 +- .../completions/completions.test.ts | 4 +- .../plugins/codemirror/completions/utils.ts | 3 +- .../WorkflowVersionFormModal.test.ts | 2 +- 39 files changed, 925 insertions(+), 840 deletions(-) create mode 100644 packages/frontend/editor-ui/src/app/stores/defaults/modals.ts diff --git a/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.test.ts b/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.test.ts index 81606aa1c62..dc283b0a642 100644 --- a/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.test.ts +++ b/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { computed, isReactive } from 'vue'; import type { Component } from 'vue'; import * as modalRegistry from './modalRegistry'; @@ -341,4 +342,35 @@ describe('modalRegistry', () => { expect(keys).toEqual(['test-modal-2', 'test-modal-1', 'test-modal-3']); }); }); + + // The registry is the single definition source: consumers derive from it rather + // than mirroring it, which only works if reading it inside a computed tracks. + describe('reactivity', () => { + it('should re-evaluate a computed over getAll on register, unregister and clear', () => { + const keys = computed(() => [...modalRegistry.getAll().keys()]); + + expect(keys.value).toEqual([]); + + modalRegistry.register(mockModal1); + expect(keys.value).toEqual(['test-modal-1']); + + modalRegistry.register(mockModal2); + expect(keys.value).toEqual(['test-modal-1', 'test-modal-2']); + + modalRegistry.unregister('test-modal-1'); + expect(keys.value).toEqual(['test-modal-2']); + + modalRegistry.clear(); + expect(keys.value).toEqual([]); + }); + + it('should not make a registered component reactive', () => { + modalRegistry.register(mockModal1); + + // Wrapping a component in a reactive proxy costs render performance and + // breaks identity checks — the registry is shallow for this reason. + expect(isReactive(modalRegistry.get('test-modal-1')?.component)).toBe(false); + expect(modalRegistry.get('test-modal-1')?.component).toBe(mockComponent1); + }); + }); }); diff --git a/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.ts b/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.ts index 7e2eed47a8b..eeeb6956a63 100644 --- a/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.ts +++ b/packages/frontend/@n8n/frontend-module-sdk/src/registries/modalRegistry.ts @@ -1,6 +1,13 @@ +import { shallowReactive } from 'vue'; + import type { ModalDefinition } from '../types/modal'; -const modals = new Map(); +/** + * Shallow-reactive so consumers can derive from the registry with a plain + * `computed` instead of mirroring it through a subscription. Shallow on purpose: + * a definition's `component` must not be turned into a reactive object. + */ +const modals = shallowReactive(new Map()); const listeners = new Set<(modals: Map) => void>(); export function getAll(): Map { diff --git a/packages/frontend/editor-ui/src/app/components/ChatEmbedModal.test.ts b/packages/frontend/editor-ui/src/app/components/ChatEmbedModal.test.ts index 18dff2887c4..66bcc04be3b 100644 --- a/packages/frontend/editor-ui/src/app/components/ChatEmbedModal.test.ts +++ b/packages/frontend/editor-ui/src/app/components/ChatEmbedModal.test.ts @@ -9,7 +9,7 @@ const renderComponent = createComponentRenderer(ChatEmbedModal, { pinia: createTestingPinia({ initialState: { [STORES.UI]: { - modalsById: { + modalStateById: { [CHAT_EMBED_MODAL_KEY]: { open: true }, }, }, diff --git a/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.test.ts b/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.test.ts index 084d9003b0b..5b88a6f1a76 100644 --- a/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.test.ts +++ b/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.test.ts @@ -3,18 +3,22 @@ import { defineComponent, nextTick } from 'vue'; import { screen } from '@testing-library/vue'; import DynamicModalLoader from '@/app/components/DynamicModalLoader.vue'; import { modalRegistry } from '@n8n/frontend-module-sdk'; -import type { ModalDefinition } from '@n8n/frontend-module-sdk'; import { createComponentRenderer } from '@/__tests__/render'; -// Mock the modalRegistry namespace exported by the SDK barrel -vi.mock('@n8n/frontend-module-sdk', () => ({ - modalRegistry: { - getAll: vi.fn(), - subscribe: vi.fn(), - }, -})); +/** + * Exercised against the real registry, not a mock of it: the loader derives from + * the registry rather than subscribing to it, so what needs proving is that the + * rendered list follows registration in both directions. + */ -const mockModalRegistry = vi.mocked(modalRegistry); +const modalProps = { + modalName: { type: String, required: true }, + active: { type: Boolean, required: true }, + open: { type: Boolean, required: true }, + activeId: { type: String, required: true }, + mode: { type: String, required: true }, + data: { type: Object, required: true }, +} as const; const renderComponent = createComponentRenderer(DynamicModalLoader, { global: { @@ -38,17 +42,15 @@ const renderComponent = createComponentRenderer(DynamicModalLoader, { }, }); +const renderedModalNames = (container: Element) => + Array.from(container.querySelectorAll('[data-testid="modal-root"]'), (root) => + root.getAttribute('data-modal-name'), + ); + describe('DynamicModalLoader', () => { const mockModalComponent = defineComponent({ name: 'MockModal', - props: { - modalName: { type: String, required: true }, - active: { type: Boolean, required: true }, - open: { type: Boolean, required: true }, - activeId: { type: String, required: true }, - mode: { type: String, required: true }, - data: { type: Object, required: true }, - }, + props: modalProps, template: '
Mock Modal
', }); @@ -56,254 +58,95 @@ describe('DynamicModalLoader', () => { beforeEach(() => { vi.clearAllMocks(); + modalRegistry.clear(); }); it('should render empty div when no modals are registered', () => { - mockModalRegistry.getAll.mockReturnValue(new Map()); - mockModalRegistry.subscribe.mockReturnValue(vi.fn()); - - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); + const { container } = renderComponent({ pinia: createTestingPinia() }); expect(container.firstChild).toBeInTheDocument(); expect(screen.queryByTestId('modal-root')).not.toBeInTheDocument(); }); it('should render ModalRoot components for registered modals', async () => { - const modalsMap = new Map([ - [ - 'testModal1', - { - key: 'testModal1', - component: mockModalComponent, - }, - ], - [ - 'testModal2', - { - key: 'testModal2', - component: mockModalComponent, - }, - ], - ]); - - mockModalRegistry.getAll.mockReturnValue(modalsMap); - mockModalRegistry.subscribe.mockReturnValue(vi.fn()); - - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); + modalRegistry.register({ key: 'testModal1', component: mockModalComponent }); + modalRegistry.register({ key: 'testModal2', component: mockModalComponent }); + const { container } = renderComponent({ pinia: createTestingPinia() }); await nextTick(); - const modalRoots = container.querySelectorAll('[data-testid="modal-root"]'); - expect(modalRoots).toHaveLength(2); - - const modalNames = Array.from(modalRoots).map((root) => root.getAttribute('data-modal-name')); - expect(modalNames).toContain('testModal1'); - expect(modalNames).toContain('testModal2'); + expect(renderedModalNames(container)).toEqual(['testModal1', 'testModal2']); }); it('should handle async component factories', async () => { - const modalsMap = new Map([ - [ - 'asyncModal', - { - key: 'asyncModal', - component: mockAsyncModalComponent, - }, - ], - ]); - - mockModalRegistry.getAll.mockReturnValue(modalsMap); - mockModalRegistry.subscribe.mockReturnValue(vi.fn()); - - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); + modalRegistry.register({ key: 'asyncModal', component: mockAsyncModalComponent }); + const { container } = renderComponent({ pinia: createTestingPinia() }); await nextTick(); - const modalRoot = container.querySelector('[data-testid="modal-root"]'); - expect(modalRoot).toBeInTheDocument(); - expect(modalRoot?.getAttribute('data-modal-name')).toBe('asyncModal'); + expect(renderedModalNames(container)).toEqual(['asyncModal']); expect(mockAsyncModalComponent).toHaveBeenCalled(); }); - it('should subscribe to modalRegistry changes on mount', () => { - mockModalRegistry.getAll.mockReturnValue(new Map()); - const mockUnsubscribe = vi.fn(); - mockModalRegistry.subscribe.mockReturnValue(mockUnsubscribe); - - renderComponent({ - pinia: createTestingPinia(), - }); - - expect(mockModalRegistry.subscribe).toHaveBeenCalledTimes(1); - expect(mockModalRegistry.subscribe).toHaveBeenCalledWith(expect.any(Function)); - }); - - it('should update modals when registry changes', async () => { - let subscribeCallback: ((modals: Map) => void) | undefined; - - // Initial empty registry - mockModalRegistry.getAll.mockReturnValue(new Map()); - mockModalRegistry.subscribe.mockImplementation((listener) => { - subscribeCallback = listener; - return vi.fn(); - }); - - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); + it('should render a modal registered after mount', async () => { + const { container } = renderComponent({ pinia: createTestingPinia() }); + await nextTick(); + expect(renderedModalNames(container)).toEqual([]); + modalRegistry.register({ key: 'newModal', component: mockModalComponent }); await nextTick(); - // Initially no modal roots - expect(container.querySelector('[data-testid="modal-root"]')).not.toBeInTheDocument(); - - // Simulate registry change - const newModalsMap = new Map([ - [ - 'newModal', - { - key: 'newModal', - component: mockModalComponent, - }, - ], - ]); - - mockModalRegistry.getAll.mockReturnValue(newModalsMap); - subscribeCallback?.(newModalsMap); - - await nextTick(); - - // Should now have one modal root - const modalRoot = container.querySelector('[data-testid="modal-root"]'); - expect(modalRoot).toBeInTheDocument(); - expect(modalRoot?.getAttribute('data-modal-name')).toBe('newModal'); + expect(renderedModalNames(container)).toEqual(['newModal']); }); - it('should unsubscribe from registry changes on unmount', () => { - mockModalRegistry.getAll.mockReturnValue(new Map()); - const mockUnsubscribe = vi.fn(); - mockModalRegistry.subscribe.mockReturnValue(mockUnsubscribe); + it('should stop rendering a modal that is unregistered', async () => { + modalRegistry.register({ key: 'staying', component: mockModalComponent }); + modalRegistry.register({ key: 'leaving', component: mockModalComponent }); - const wrapper = renderComponent({ - pinia: createTestingPinia(), - }); + const { container } = renderComponent({ pinia: createTestingPinia() }); + await nextTick(); + expect(renderedModalNames(container)).toEqual(['staying', 'leaving']); - expect(mockUnsubscribe).not.toHaveBeenCalled(); + modalRegistry.unregister('leaving'); + await nextTick(); - wrapper.unmount(); + expect(renderedModalNames(container)).toEqual(['staying']); + }); - expect(mockUnsubscribe).toHaveBeenCalledTimes(1); + it('should render nothing once the registry is cleared', async () => { + modalRegistry.register({ key: 'testModal', component: mockModalComponent }); + + const { container } = renderComponent({ pinia: createTestingPinia() }); + await nextTick(); + expect(renderedModalNames(container)).toEqual(['testModal']); + + modalRegistry.clear(); + await nextTick(); + + expect(renderedModalNames(container)).toEqual([]); }); it('should handle modals with different component types', async () => { const regularComponent = defineComponent({ name: 'RegularModal', - props: { - modalName: { type: String, required: true }, - active: { type: Boolean, required: true }, - open: { type: Boolean, required: true }, - activeId: { type: String, required: true }, - mode: { type: String, required: true }, - data: { type: Object, required: true }, - }, + props: modalProps, template: '
Regular
', }); - const asyncComponent = async () => await Promise.resolve( defineComponent({ name: 'AsyncModal', - props: { - modalName: { type: String, required: true }, - active: { type: Boolean, required: true }, - open: { type: Boolean, required: true }, - activeId: { type: String, required: true }, - mode: { type: String, required: true }, - data: { type: Object, required: true }, - }, + props: modalProps, template: '
Async
', }), ); - const modalsMap = new Map([ - ['regular', { key: 'regular', component: regularComponent }], - ['async', { key: 'async', component: asyncComponent }], - ]); - - mockModalRegistry.getAll.mockReturnValue(modalsMap); - mockModalRegistry.subscribe.mockReturnValue(vi.fn()); - - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); + modalRegistry.register({ key: 'regular', component: regularComponent }); + modalRegistry.register({ key: 'async', component: asyncComponent }); + const { container } = renderComponent({ pinia: createTestingPinia() }); await nextTick(); - const modalRoots = container.querySelectorAll('[data-testid="modal-root"]'); - expect(modalRoots).toHaveLength(2); - - // Both modals should be rendered - const modalNames = Array.from(modalRoots).map((root) => root.getAttribute('data-modal-name')); - expect(modalNames).toContain('regular'); - expect(modalNames).toContain('async'); - }); - - it('should call updateModals on mount', () => { - const modalsMap = new Map([ - ['testModal', { key: 'testModal', component: mockModalComponent }], - ]); - - mockModalRegistry.getAll.mockReturnValue(modalsMap); - mockModalRegistry.subscribe.mockReturnValue(vi.fn()); - - renderComponent({ - pinia: createTestingPinia(), - }); - - // getAll should be called during updateModals on mount - expect(mockModalRegistry.getAll).toHaveBeenCalledTimes(1); - }); - - it('should correctly identify async component factories', async () => { - // Test with regular component (not async) - const regularComponent = defineComponent({ - name: 'RegularComponent', - props: { - modalName: { type: String, required: true }, - active: { type: Boolean, required: true }, - open: { type: Boolean, required: true }, - activeId: { type: String, required: true }, - mode: { type: String, required: true }, - data: { type: Object, required: true }, - }, - template: '
Regular
', - }); - - // Test with async component factory - const asyncFactory = async () => await Promise.resolve(regularComponent); - - const modalsMap = new Map([ - ['regularModal', { key: 'regularModal', component: regularComponent }], - ['asyncModal', { key: 'asyncModal', component: asyncFactory }], - ]); - - mockModalRegistry.getAll.mockReturnValue(modalsMap); - mockModalRegistry.subscribe.mockReturnValue(vi.fn()); - - const { container } = renderComponent({ - pinia: createTestingPinia(), - }); - - await nextTick(); - - // Both should render successfully - const modalRoots = container.querySelectorAll('[data-testid="modal-root"]'); - expect(modalRoots).toHaveLength(2); + expect(renderedModalNames(container)).toEqual(['regular', 'async']); }); }); diff --git a/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.vue b/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.vue index 8d4de7f0ea9..a15bd97dcbd 100644 --- a/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.vue +++ b/packages/frontend/editor-ui/src/app/components/DynamicModalLoader.vue @@ -1,18 +1,9 @@