From 37797f38d81b12d030ba85034baeb49192ea575c Mon Sep 17 00:00:00 2001 From: Mutasem Aldmour <4711238+mutdmour@users.noreply.github.com> Date: Fri, 16 Aug 2024 12:27:55 +0200 Subject: [PATCH 1/4] fix(editor): Buffer json chunks in stream response (#10439) --- packages/editor-ui/src/api/assistant.ts | 9 +- .../src/plugins/i18n/locales/en.json | 4 +- .../src/utils/__tests__/apiUtils.spec.ts | 112 ++++++++++++++++++ packages/editor-ui/src/utils/apiUtils.ts | 37 ++++-- 4 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 packages/editor-ui/src/utils/__tests__/apiUtils.spec.ts diff --git a/packages/editor-ui/src/api/assistant.ts b/packages/editor-ui/src/api/assistant.ts index d9e9ec0cded21..4b1d127ef15c6 100644 --- a/packages/editor-ui/src/api/assistant.ts +++ b/packages/editor-ui/src/api/assistant.ts @@ -9,7 +9,14 @@ export function chatWithAssistant( onDone: () => void, onError: (e: Error) => void, ): void { - void streamRequest(ctx, '/ai-assistant/chat', payload, onMessageUpdated, onDone, onError); + void streamRequest( + ctx, + '/ai-assistant/chat', + payload, + onMessageUpdated, + onDone, + onError, + ); } export async function replaceCode( diff --git a/packages/editor-ui/src/plugins/i18n/locales/en.json b/packages/editor-ui/src/plugins/i18n/locales/en.json index b082675d2d077..49b331e4c1787 100644 --- a/packages/editor-ui/src/plugins/i18n/locales/en.json +++ b/packages/editor-ui/src/plugins/i18n/locales/en.json @@ -131,7 +131,7 @@ "auth.signup.setupYourAccount": "Set up your account", "auth.signup.setupYourAccountError": "Problem setting up your account", "auth.signup.tokenValidationError": "Issue validating invite token", - "aiAssistant.name": "Ava", + "aiAssistant.name": "Assistant", "aiAssistant.assistant": "AI Assistant", "aiAssistant.newSessionModal.title.part1": "Start new", "aiAssistant.newSessionModal.title.part2": "session", @@ -139,7 +139,7 @@ "aiAssistant.newSessionModal.question": "Are you sure you want to start a new session?", "aiAssistant.newSessionModal.confirm": "Start new session", "aiAssistant.serviceError.message": "Unable to connect to n8n's AI service", - "aiAssistant.codeUpdated.message.title": "Ava modified workflow", + "aiAssistant.codeUpdated.message.title": "Assistant modified workflow", "aiAssistant.codeUpdated.message.body": "Open the {nodeName} node to see the changes", "banners.confirmEmail.message.1": "To secure your account and prevent future access issues, please confirm your", "banners.confirmEmail.message.2": "email address.", diff --git a/packages/editor-ui/src/utils/__tests__/apiUtils.spec.ts b/packages/editor-ui/src/utils/__tests__/apiUtils.spec.ts new file mode 100644 index 0000000000000..cefb6f3389bb7 --- /dev/null +++ b/packages/editor-ui/src/utils/__tests__/apiUtils.spec.ts @@ -0,0 +1,112 @@ +import { STREAM_SEPERATOR, streamRequest } from '../apiUtils'; + +describe('streamRequest', () => { + it('should stream data from the API endpoint', async () => { + const encoder = new TextEncoder(); + const mockResponse = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(`${JSON.stringify({ chunk: 1 })}${STREAM_SEPERATOR}`)); + controller.enqueue(encoder.encode(`${JSON.stringify({ chunk: 2 })}${STREAM_SEPERATOR}`)); + controller.enqueue(encoder.encode(`${JSON.stringify({ chunk: 3 })}${STREAM_SEPERATOR}`)); + controller.close(); + }, + }); + + const mockFetch = vi.fn().mockResolvedValue({ + ok: true, + body: mockResponse, + }); + + global.fetch = mockFetch; + + const onChunkMock = vi.fn(); + const onDoneMock = vi.fn(); + const onErrorMock = vi.fn(); + + await streamRequest( + { + baseUrl: 'https://api.example.com', + pushRef: '', + }, + '/data', + { key: 'value' }, + onChunkMock, + onDoneMock, + onErrorMock, + ); + + expect(mockFetch).toHaveBeenCalledWith('https://api.example.com/data', { + method: 'POST', + body: JSON.stringify({ key: 'value' }), + credentials: 'include', + headers: { + 'Content-Type': 'application/json', + 'browser-id': expect.stringContaining('-'), + }, + }); + + expect(onChunkMock).toHaveBeenCalledTimes(3); + expect(onChunkMock).toHaveBeenNthCalledWith(1, { chunk: 1 }); + expect(onChunkMock).toHaveBeenNthCalledWith(2, { chunk: 2 }); + expect(onChunkMock).toHaveBeenNthCalledWith(3, { chunk: 3 }); + + expect(onDoneMock).toHaveBeenCalledTimes(1); + expect(onErrorMock).not.toHaveBeenCalled(); + }); + + it('should handle broken stream data', async () => { + const encoder = new TextEncoder(); + const mockResponse = new ReadableStream({ + start(controller) { + controller.enqueue( + encoder.encode(`${JSON.stringify({ chunk: 1 })}${STREAM_SEPERATOR}{"chunk": `), + ); + controller.enqueue(encoder.encode(`2}${STREAM_SEPERATOR}{"ch`)); + controller.enqueue(encoder.encode('unk":')); + controller.enqueue(encoder.encode(`3}${STREAM_SEPERATOR}`)); + controller.close(); + }, + }); + + const mockFetch = vi.fn().mockResolvedValue({ + ok: true, + body: mockResponse, + }); + + global.fetch = mockFetch; + + const onChunkMock = vi.fn(); + const onDoneMock = vi.fn(); + const onErrorMock = vi.fn(); + + await streamRequest( + { + baseUrl: 'https://api.example.com', + pushRef: '', + }, + '/data', + { key: 'value' }, + onChunkMock, + onDoneMock, + onErrorMock, + ); + + expect(mockFetch).toHaveBeenCalledWith('https://api.example.com/data', { + method: 'POST', + body: JSON.stringify({ key: 'value' }), + credentials: 'include', + headers: { + 'Content-Type': 'application/json', + 'browser-id': expect.stringContaining('-'), + }, + }); + + expect(onChunkMock).toHaveBeenCalledTimes(3); + expect(onChunkMock).toHaveBeenNthCalledWith(1, { chunk: 1 }); + expect(onChunkMock).toHaveBeenNthCalledWith(2, { chunk: 2 }); + expect(onChunkMock).toHaveBeenNthCalledWith(3, { chunk: 3 }); + + expect(onDoneMock).toHaveBeenCalledTimes(1); + expect(onErrorMock).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/editor-ui/src/utils/apiUtils.ts b/packages/editor-ui/src/utils/apiUtils.ts index e9201b7c331ea..b07f8910b90c8 100644 --- a/packages/editor-ui/src/utils/apiUtils.ts +++ b/packages/editor-ui/src/utils/apiUtils.ts @@ -3,7 +3,6 @@ import axios from 'axios'; import { ApplicationError, jsonParse, type GenericValue, type IDataObject } from 'n8n-workflow'; import type { IExecutionFlattedResponse, IExecutionResponse, IRestApiContext } from '@/Interface'; import { parse } from 'flatted'; -import type { ChatRequest } from '@/types/assistant.types'; import { assert } from '@/utils/assert'; const BROWSER_ID_STORAGE_KEY = 'n8n-browserId'; @@ -14,6 +13,7 @@ if (!browserId && 'randomUUID' in crypto) { } export const NO_NETWORK_ERROR_CODE = 999; +export const STREAM_SEPERATOR = '⧉⇋⇋➽⌑⧉§§\n'; export class ResponseError extends ApplicationError { // The HTTP status code of response @@ -194,15 +194,15 @@ export function unflattenExecutionData(fullExecutionData: IExecutionFlattedRespo return returnData; } -export const streamRequest = async ( +export async function streamRequest( context: IRestApiContext, apiEndpoint: string, - payload: ChatRequest.RequestPayload, - onChunk?: (chunk: ChatRequest.ResponsePayload) => void, + payload: object, + onChunk?: (chunk: T) => void, onDone?: () => void, onError?: (e: Error) => void, - separator = '⧉⇋⇋➽⌑⧉§§\n', -): Promise => { + separator = STREAM_SEPERATOR, +): Promise { const headers: Record = { 'Content-Type': 'application/json', }; @@ -223,23 +223,36 @@ export const streamRequest = async ( const reader = response.body.getReader(); const decoder = new TextDecoder('utf-8'); + let buffer = ''; + async function readStream() { const { done, value } = await reader.read(); if (done) { onDone?.(); return; } - const chunk = decoder.decode(value); - const splitChunks = chunk.split(separator); + buffer += chunk; + const splitChunks = buffer.split(separator); + + buffer = ''; for (const splitChunk of splitChunks) { - if (splitChunk && onChunk) { + if (splitChunk) { + let data: T; + try { + data = jsonParse(splitChunk, { errorMessage: 'Invalid json' }); + } catch (e) { + // incomplete json. append to buffer to complete + buffer += splitChunk; + + continue; + } + try { - onChunk(jsonParse(splitChunk, { errorMessage: 'Invalid json chunk in stream' })); + onChunk?.(data); } catch (e: unknown) { if (e instanceof Error) { - console.log(`${e.message}: ${splitChunk}`); onError?.(e); } } @@ -257,4 +270,4 @@ export const streamRequest = async ( assert(e instanceof Error); onError?.(e); } -}; +} From 334d11175ed91936e91c0386752922f12c0ab94d Mon Sep 17 00:00:00 2001 From: Alex Grozav Date: Fri, 16 Aug 2024 13:29:32 +0300 Subject: [PATCH 2/4] fix(editor): Fix lazy loaded component not using suspense (no-changelog) (#10454) --- packages/editor-ui/src/components/RunData.vue | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/editor-ui/src/components/RunData.vue b/packages/editor-ui/src/components/RunData.vue index 2b1516aa514a9..ec26ef996402d 100644 --- a/packages/editor-ui/src/components/RunData.vue +++ b/packages/editor-ui/src/components/RunData.vue @@ -50,15 +50,17 @@ data-test-id="run-data-pane-header" @click.stop > - + + + = {}): IWorkflowDb { + return { + createdAt: '', + updatedAt: '', + id, + name, + nodes, + connections, + active, + settings, + versionId: '1', + meta: {}, + pinData, + ...rest, + }; +} + export function createTestNode(node: Partial = {}): INode { return { id: uuid(), diff --git a/packages/editor-ui/src/composables/__tests__/useWorkflowHelpers.spec.ts b/packages/editor-ui/src/composables/__tests__/useWorkflowHelpers.spec.ts index 81643f52aac84..f50261a1aa654 100644 --- a/packages/editor-ui/src/composables/__tests__/useWorkflowHelpers.spec.ts +++ b/packages/editor-ui/src/composables/__tests__/useWorkflowHelpers.spec.ts @@ -3,6 +3,10 @@ import { useWorkflowHelpers } from '@/composables/useWorkflowHelpers'; import router from '@/router'; import { createTestingPinia } from '@pinia/testing'; import { setActivePinia } from 'pinia'; +import { useWorkflowsStore } from '@/stores/workflows.store'; +import { useWorkflowsEEStore } from '@/stores/workflows.ee.store'; +import { useTagsStore } from '@/stores/tags.store'; +import { createTestWorkflow } from '@/__tests__/mocks'; const getDuplicateTestWorkflow = (): IWorkflowDataUpdate => ({ name: 'Duplicate webhook test', @@ -50,32 +54,23 @@ const getDuplicateTestWorkflow = (): IWorkflowDataUpdate => ({ connections: {}, }); -vi.mock('@/stores/workflows.store', () => ({ - useWorkflowsStore: vi.fn(() => ({ - workflowsById: {}, - createNewWorkflow: vi.fn(() => {}), - addWorkflow: vi.fn(() => {}), - setActive: vi.fn(() => {}), - setWorkflowId: vi.fn(() => {}), - setWorkflowVersionId: vi.fn(() => {}), - setWorkflowName: vi.fn(() => {}), - setWorkflowSettings: vi.fn(() => {}), - setNodeValue: vi.fn(() => {}), - setWorkflowTagIds: vi.fn(() => {}), - getCurrentWorkflow: vi.fn(() => ({})), - })), -})); - describe('useWorkflowHelpers', () => { - describe('saveAsNewWorkflow', () => { - beforeAll(() => { - setActivePinia(createTestingPinia()); - }); + let workflowsStore: ReturnType; + let workflowsEEStore: ReturnType; + let tagsStore: ReturnType; - afterEach(() => { - vi.clearAllMocks(); - }); + beforeAll(() => { + setActivePinia(createTestingPinia()); + workflowsStore = useWorkflowsStore(); + workflowsEEStore = useWorkflowsEEStore(); + tagsStore = useTagsStore(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + describe('saveAsNewWorkflow', () => { it('should respect `resetWebhookUrls: false` when duplicating workflows', async () => { const workflow = getDuplicateTestWorkflow(); if (!workflow.nodes) { @@ -120,4 +115,101 @@ describe('useWorkflowHelpers', () => { expect(pathsPreSave).not.toEqual(pathsPostSave); }); }); + + describe('initState', () => { + it('should initialize workflow state with provided data', () => { + const { initState } = useWorkflowHelpers({ router }); + + const workflowData = createTestWorkflow({ + id: '1', + name: 'Test Workflow', + active: true, + pinData: {}, + meta: {}, + scopes: ['workflow:create'], + usedCredentials: [], + sharedWithProjects: [], + tags: [], + }); + const addWorkflowSpy = vi.spyOn(workflowsStore, 'addWorkflow'); + const setActiveSpy = vi.spyOn(workflowsStore, 'setActive'); + const setWorkflowIdSpy = vi.spyOn(workflowsStore, 'setWorkflowId'); + const setWorkflowNameSpy = vi.spyOn(workflowsStore, 'setWorkflowName'); + const setWorkflowSettingsSpy = vi.spyOn(workflowsStore, 'setWorkflowSettings'); + const setWorkflowPinDataSpy = vi.spyOn(workflowsStore, 'setWorkflowPinData'); + const setWorkflowVersionIdSpy = vi.spyOn(workflowsStore, 'setWorkflowVersionId'); + const setWorkflowMetadataSpy = vi.spyOn(workflowsStore, 'setWorkflowMetadata'); + const setWorkflowScopesSpy = vi.spyOn(workflowsStore, 'setWorkflowScopes'); + const setUsedCredentialsSpy = vi.spyOn(workflowsStore, 'setUsedCredentials'); + const setWorkflowSharedWithSpy = vi.spyOn(workflowsEEStore, 'setWorkflowSharedWith'); + const setWorkflowTagIdsSpy = vi.spyOn(workflowsStore, 'setWorkflowTagIds'); + const upsertTagsSpy = vi.spyOn(tagsStore, 'upsertTags'); + + initState(workflowData); + + expect(addWorkflowSpy).toHaveBeenCalledWith(workflowData); + expect(setActiveSpy).toHaveBeenCalledWith(true); + expect(setWorkflowIdSpy).toHaveBeenCalledWith('1'); + expect(setWorkflowNameSpy).toHaveBeenCalledWith({ + newName: 'Test Workflow', + setStateDirty: false, + }); + expect(setWorkflowSettingsSpy).toHaveBeenCalledWith({ + executionOrder: 'v1', + timezone: 'DEFAULT', + }); + expect(setWorkflowPinDataSpy).toHaveBeenCalledWith({}); + expect(setWorkflowVersionIdSpy).toHaveBeenCalledWith('1'); + expect(setWorkflowMetadataSpy).toHaveBeenCalledWith({}); + expect(setWorkflowScopesSpy).toHaveBeenCalledWith(['workflow:create']); + expect(setUsedCredentialsSpy).toHaveBeenCalledWith([]); + expect(setWorkflowSharedWithSpy).toHaveBeenCalledWith({ + workflowId: '1', + sharedWithProjects: [], + }); + expect(setWorkflowTagIdsSpy).toHaveBeenCalledWith([]); + expect(upsertTagsSpy).toHaveBeenCalledWith([]); + }); + + it('should handle missing `usedCredentials` and `sharedWithProjects` gracefully', () => { + const { initState } = useWorkflowHelpers({ router }); + + const workflowData = createTestWorkflow({ + id: '1', + name: 'Test Workflow', + active: true, + pinData: {}, + meta: {}, + scopes: [], + tags: [], + }); + const setUsedCredentialsSpy = vi.spyOn(workflowsStore, 'setUsedCredentials'); + const setWorkflowSharedWithSpy = vi.spyOn(workflowsEEStore, 'setWorkflowSharedWith'); + + initState(workflowData); + + expect(setUsedCredentialsSpy).not.toHaveBeenCalled(); + expect(setWorkflowSharedWithSpy).not.toHaveBeenCalled(); + }); + + it('should handle missing `tags` gracefully', () => { + const { initState } = useWorkflowHelpers({ router }); + + const workflowData = createTestWorkflow({ + id: '1', + name: 'Test Workflow', + active: true, + pinData: {}, + meta: {}, + scopes: [], + }); + const setWorkflowTagIdsSpy = vi.spyOn(workflowsStore, 'setWorkflowTagIds'); + const upsertTagsSpy = vi.spyOn(tagsStore, 'upsertTags'); + + initState(workflowData); + + expect(setWorkflowTagIdsSpy).toHaveBeenCalledWith([]); + expect(upsertTagsSpy).toHaveBeenCalledWith([]); + }); + }); }); diff --git a/packages/editor-ui/src/composables/useWorkflowHelpers.ts b/packages/editor-ui/src/composables/useWorkflowHelpers.ts index 178b480ae57d4..fd203a9ecc22d 100644 --- a/packages/editor-ui/src/composables/useWorkflowHelpers.ts +++ b/packages/editor-ui/src/composables/useWorkflowHelpers.ts @@ -1116,6 +1116,7 @@ export function useWorkflowHelpers(options: { router: ReturnType { }; } + function setWorkflowScopes(scopes: IWorkflowDb['scopes']): void { + workflow.value.scopes = scopes; + } + function setWorkflowMetadata(metadata: WorkflowMetadata | undefined): void { workflow.value.meta = metadata; } @@ -1634,6 +1638,7 @@ export const useWorkflowsStore = defineStore(STORES.WORKFLOWS, () => { setWorkflowTagIds, addWorkflowTagIds, removeWorkflowTagId, + setWorkflowScopes, setWorkflowMetadata, addToWorkflowMetadata, setWorkflow,