From 83c3a98cf4875cc7b105063f972cf7d08b0561f4 Mon Sep 17 00:00:00 2001 From: Mutasem Aldmour <4711238+mutdmour@users.noreply.github.com> Date: Thu, 14 Aug 2025 10:05:36 +0200 Subject: [PATCH] feat(editor): Improve feedback buttons behavior (#18247) Co-authored-by: Claude --- .../messages/MessageRating.test.ts | 277 ++++++++++ .../messages/MessageRating.vue | 9 +- .../__snapshots__/MessageRating.test.ts.snap | 12 + .../@n8n/design-system/src/locale/lang/en.ts | 2 +- .../composables/useBuilderMessages.test.ts | 493 +++++++++++++++++- .../src/composables/useBuilderMessages.ts | 105 ++-- .../src/stores/builder.store.test.ts | 99 +++- .../editor-ui/src/stores/builder.store.ts | 3 +- 8 files changed, 925 insertions(+), 75 deletions(-) create mode 100644 packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.test.ts create mode 100644 packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/__snapshots__/MessageRating.test.ts.snap diff --git a/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.test.ts b/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.test.ts new file mode 100644 index 0000000000..2127e9597b --- /dev/null +++ b/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.test.ts @@ -0,0 +1,277 @@ +import { render, fireEvent, waitFor } from '@testing-library/vue'; +import { createPinia, setActivePinia } from 'pinia'; +import { nextTick } from 'vue'; + +import MessageRating from './MessageRating.vue'; + +const stubs = ['n8n-button', 'n8n-icon-button', 'n8n-input']; + +// Mock i18n to return keys instead of translated text +vi.mock('@n8n/design-system/composables/useI18n', () => ({ + useI18n: () => ({ + t: (key: string) => key, + }), +})); + +beforeEach(() => { + setActivePinia(createPinia()); +}); + +describe('MessageRating', () => { + it('should render correctly with default props', () => { + const wrapper = render(MessageRating, { + global: { stubs }, + }); + + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-up-button"]'), + ).toBeTruthy(); + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-down-button"]'), + ).toBeTruthy(); + expect(wrapper.html()).toMatchSnapshot(); + }); + + describe('rating interactions', () => { + it('should emit feedback when thumbs up is clicked', async () => { + const wrapper = render(MessageRating, { + global: { stubs }, + }); + + const upButton = wrapper.container.querySelector('[data-test-id="message-thumbs-up-button"]'); + await fireEvent.click(upButton!); + + expect(wrapper.emitted()).toHaveProperty('feedback'); + expect(wrapper.emitted().feedback[0]).toEqual([{ rating: 'up' }]); + }); + + it('should emit feedback when thumbs down is clicked', async () => { + const wrapper = render(MessageRating, { + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + + expect(wrapper.emitted()).toHaveProperty('feedback'); + expect(wrapper.emitted().feedback[0]).toEqual([{ rating: 'down' }]); + }); + + it('should hide rating buttons and show success after thumbs up', async () => { + const wrapper = render(MessageRating, { + global: { stubs }, + }); + + const upButton = wrapper.container.querySelector('[data-test-id="message-thumbs-up-button"]'); + await fireEvent.click(upButton!); + await nextTick(); + + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-up-button"]'), + ).toBeFalsy(); + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-down-button"]'), + ).toBeFalsy(); + expect(wrapper.getByText('assistantChat.builder.success')).toBeTruthy(); + }); + + it('should hide rating buttons and show feedback form after thumbs down when showFeedback is true', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-up-button"]'), + ).toBeFalsy(); + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-down-button"]'), + ).toBeFalsy(); + expect( + wrapper.container.querySelector('[data-test-id="message-feedback-input"]'), + ).toBeTruthy(); + expect( + wrapper.container.querySelector('[data-test-id="message-submit-feedback-button"]'), + ).toBeTruthy(); + }); + + it('should show success immediately after thumbs down when showFeedback is false', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: false }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + expect( + wrapper.container.querySelector('[data-test-id="message-feedback-input"]'), + ).toBeFalsy(); + expect(wrapper.getByText('assistantChat.builder.success')).toBeTruthy(); + }); + }); + + describe('feedback form interactions', () => { + it('should submit feedback and show success', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true }, + global: { stubs: ['n8n-button', 'n8n-icon-button'] }, // Don't stub n8n-input + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + // Find the actual textarea element within the N8nInput component + const textarea = wrapper.container.querySelector( + 'textarea[data-test-id="message-feedback-input"]', + ); + await fireEvent.update(textarea!, 'This is my feedback about the response'); + await nextTick(); + + const submitButton = wrapper.container.querySelector( + '[data-test-id="message-submit-feedback-button"]', + ); + await fireEvent.click(submitButton!); + await nextTick(); + + expect(wrapper.emitted().feedback).toHaveLength(2); + expect(wrapper.emitted().feedback[1]).toEqual([ + { feedback: 'This is my feedback about the response' }, + ]); + expect( + wrapper.container.querySelector('[data-test-id="message-feedback-input"]'), + ).toBeFalsy(); + expect(wrapper.getByText('assistantChat.builder.success')).toBeTruthy(); + }); + + it('should cancel feedback and return to rating buttons', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + const cancelButton = wrapper.container.querySelector( + 'n8n-button-stub[label="generic.cancel"]', + ); + await fireEvent.click(cancelButton!); + await nextTick(); + + expect( + wrapper.container.querySelector('[data-test-id="message-feedback-input"]'), + ).toBeFalsy(); + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-up-button"]'), + ).toBeTruthy(); + expect( + wrapper.container.querySelector('[data-test-id="message-thumbs-down-button"]'), + ).toBeTruthy(); + }); + + it('should focus feedback input after thumbs down in regular mode', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true, style: 'regular' }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + + await waitFor(() => { + const feedbackInput = wrapper.container.querySelector( + '[data-test-id="message-feedback-input"]', + ); + expect(feedbackInput).toBeTruthy(); + }); + }); + + it('should clear feedback text when cancelling', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + const cancelButton = wrapper.container.querySelector( + 'n8n-button-stub[label="generic.cancel"]', + ); + await fireEvent.click(cancelButton!); + await nextTick(); + + const downButtonAgain = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButtonAgain!); + await nextTick(); + + const feedbackInputAfter = wrapper.container.querySelector( + '[data-test-id="message-feedback-input"]', + ); + expect(feedbackInputAfter?.getAttribute('modelvalue')).toBe(''); + }); + }); + + describe('textarea rows based on style', () => { + it('should have 5 rows for regular style', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true, style: 'regular' }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + const feedbackInput = wrapper.container.querySelector( + '[data-test-id="message-feedback-input"]', + ); + expect(feedbackInput?.getAttribute('rows')).toBe('5'); + }); + + it('should have 3 rows for minimal style', async () => { + const wrapper = render(MessageRating, { + props: { showFeedback: true, style: 'minimal' }, + global: { stubs }, + }); + + const downButton = wrapper.container.querySelector( + '[data-test-id="message-thumbs-down-button"]', + ); + await fireEvent.click(downButton!); + await nextTick(); + + const feedbackInput = wrapper.container.querySelector( + '[data-test-id="message-feedback-input"]', + ); + expect(feedbackInput?.getAttribute('rows')).toBe('3'); + }); + }); +}); diff --git a/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.vue b/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.vue index d62c78471d..bfeef15297 100644 --- a/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.vue +++ b/packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/MessageRating.vue @@ -25,6 +25,7 @@ const emit = defineEmits<{ const { t } = useI18n(); const showRatingButtons = ref(true); const showFeedbackArea = ref(false); +const feedbackInput = ref(null); const showSuccess = ref(false); const selectedRating = ref<'up' | 'down' | null>(null); const feedback = ref(''); @@ -34,8 +35,13 @@ function onRateButton(rating: 'up' | 'down') { showRatingButtons.value = false; emit('feedback', { rating }); - if (props.showFeedback) { + if (props.showFeedback && rating === 'down') { showFeedbackArea.value = true; + setTimeout(() => { + if (feedbackInput.value) { + feedbackInput.value.focus(); + } + }, 0); } else { showSuccess.value = true; } @@ -100,6 +106,7 @@ function onCancelFeedback() {
should render correctly with default props 1`] = ` +"
+
+ + +
+ + +
" +`; diff --git a/packages/frontend/@n8n/design-system/src/locale/lang/en.ts b/packages/frontend/@n8n/design-system/src/locale/lang/en.ts index dee957d5f7..5712315281 100644 --- a/packages/frontend/@n8n/design-system/src/locale/lang/en.ts +++ b/packages/frontend/@n8n/design-system/src/locale/lang/en.ts @@ -40,7 +40,7 @@ export default { 'assistantChat.builder.configuredNodes': 'Configured nodes', 'assistantChat.builder.thumbsUp': 'Helpful', 'assistantChat.builder.thumbsDown': 'Not helpful', - 'assistantChat.builder.feedbackPlaceholder': 'Tell us about your experience', + 'assistantChat.builder.feedbackPlaceholder': 'What went wrong?', 'assistantChat.builder.success': 'Thank you for your feedback!', 'assistantChat.builder.submit': 'Submit feedback', 'assistantChat.builder.workflowGenerated1': 'Your workflow was created successfully!', diff --git a/packages/frontend/editor-ui/src/composables/useBuilderMessages.test.ts b/packages/frontend/editor-ui/src/composables/useBuilderMessages.test.ts index e0c37956d2..de5e839f8e 100644 --- a/packages/frontend/editor-ui/src/composables/useBuilderMessages.test.ts +++ b/packages/frontend/editor-ui/src/composables/useBuilderMessages.test.ts @@ -1,8 +1,15 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { useBuilderMessages } from '@/composables/useBuilderMessages'; import type { ChatUI } from '@n8n/design-system/types/assistant'; import type { ChatRequest } from '@/types/assistant.types'; +// Mock useI18n to return the keys instead of translations +vi.mock('@n8n/i18n', () => ({ + useI18n: () => ({ + baseText: (key: string) => key, + }), +})); + describe('useBuilderMessages', () => { let builderMessages: ReturnType; @@ -282,7 +289,7 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(1); - expect(result.thinkingMessage).toBe('Running tools...'); + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); expect(result.shouldClearThinking).toBe(false); }); @@ -309,7 +316,7 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(1); - expect(result.thinkingMessage).toBe('Processing results...'); + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.processingResults'); expect(result.shouldClearThinking).toBe(false); }); @@ -382,8 +389,8 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(2); - // Should show "Running tools..." for the new running tool, not "Processing results..." - expect(result.thinkingMessage).toBe('Running tools...'); + // Should show "aiAssistant.thinkingSteps.runningTools" for the new running tool, not "aiAssistant.thinkingSteps.processingResults" + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); }); it('should show processing message when second tool completes', () => { @@ -419,7 +426,7 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(2); - expect(result.thinkingMessage).toBe('Processing results...'); + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.processingResults'); }); it('should keep showing running tools message when parallel tools complete one by one', () => { @@ -466,8 +473,8 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(2); - // Should still show "Running tools..." because call-456 is still running - expect(result.thinkingMessage).toBe('Running tools...'); + // Should still show "aiAssistant.thinkingSteps.runningTools" because call-456 is still running + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); // Verify first tool is now completed const firstTool = result.messages.find( @@ -526,8 +533,8 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(2); - // Should now show "Processing results..." because all tools are completed - expect(result.thinkingMessage).toBe('Processing results...'); + // Should now show "aiAssistant.thinkingSteps.processingResults" because all tools are completed + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.processingResults'); }); it('should keep processing message when workflow-updated arrives after tools complete', () => { @@ -561,8 +568,8 @@ describe('useBuilderMessages', () => { ); expect(result.messages).toHaveLength(2); - // Should still show "Processing results..." because workflow-updated is not a text response - expect(result.thinkingMessage).toBe('Processing results...'); + // Should still show "aiAssistant.thinkingSteps.processingResults" because workflow-updated is not a text response + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.processingResults'); // Should NOT clear thinking for workflow updates expect(result.shouldClearThinking).toBe(false); }); @@ -1256,7 +1263,7 @@ describe('useBuilderMessages', () => { ]; let result = builderMessages.processAssistantMessages(currentMessages, batch1, 'batch-1'); - expect(result.thinkingMessage).toBe('Running tools...'); + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); currentMessages = result.messages; // Second batch: tool completes @@ -1272,7 +1279,7 @@ describe('useBuilderMessages', () => { ]; result = builderMessages.processAssistantMessages(currentMessages, batch2, 'batch-2'); - expect(result.thinkingMessage).toBe('Processing results...'); + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.processingResults'); currentMessages = result.messages; // Third batch: workflow updated @@ -1285,7 +1292,7 @@ describe('useBuilderMessages', () => { ]; result = builderMessages.processAssistantMessages(currentMessages, batch3, 'batch-3'); - expect(result.thinkingMessage).toBe('Processing results...'); + expect(result.thinkingMessage).toBe('aiAssistant.thinkingSteps.processingResults'); currentMessages = result.messages; // Fourth batch: final text response @@ -1308,4 +1315,460 @@ describe('useBuilderMessages', () => { expect(result.messages.find((m) => m.type === 'text')).toBeTruthy(); }); }); + + describe('clearRatingLogic', () => { + it('should remove showRating and ratingStyle properties from text messages', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Hello there!', + showRating: true, + ratingStyle: 'regular', + read: false, + }, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'How can I help?', + showRating: false, + ratingStyle: 'minimal', + read: false, + }, + ]; + + const result = builderMessages.clearRatingLogic(messages); + + expect(result).toHaveLength(2); + expect(result[0]).toMatchObject({ + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Hello there!', + read: false, + }); + expect(result[0]).not.toHaveProperty('showRating'); + expect(result[0]).not.toHaveProperty('ratingStyle'); + + expect(result[1]).toMatchObject({ + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'How can I help?', + read: false, + }); + expect(result[1]).not.toHaveProperty('showRating'); + expect(result[1]).not.toHaveProperty('ratingStyle'); + }); + + it('should leave non-text messages unchanged', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'tool-1', + role: 'assistant', + type: 'tool', + toolName: 'add_nodes', + toolCallId: 'call-1', + status: 'completed', + updates: [], + read: false, + }, + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{}', + read: false, + }, + ]; + + const result = builderMessages.clearRatingLogic(messages); + + expect(result).toHaveLength(2); + expect(result[0]).toEqual(messages[0]); + expect(result[1]).toEqual(messages[1]); + }); + + it('should handle text messages without rating properties', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'No rating here', + read: false, + }, + ]; + + const result = builderMessages.clearRatingLogic(messages); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual(messages[0]); + }); + + it('should handle empty message array', () => { + const result = builderMessages.clearRatingLogic([]); + expect(result).toHaveLength(0); + }); + + it('should handle mixed message types with some having rating properties', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'With rating', + showRating: true, + ratingStyle: 'regular', + read: false, + }, + { + id: 'tool-1', + role: 'assistant', + type: 'tool', + toolName: 'test_tool', + toolCallId: 'call-1', + status: 'completed', + updates: [], + read: false, + }, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'Without rating', + read: false, + }, + ]; + + const result = builderMessages.clearRatingLogic(messages); + + expect(result).toHaveLength(3); + expect(result[0]).not.toHaveProperty('showRating'); + expect(result[0]).not.toHaveProperty('ratingStyle'); + expect(result[1]).toEqual(messages[1]); // tool message unchanged + expect(result[2]).toEqual(messages[2]); // text without rating unchanged + }); + }); + + describe('applyRatingLogic', () => { + it('should apply rating to the last assistant text message after workflow-updated when no tools are running', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Starting process...', + read: false, + }, + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{"nodes": [], "connections": {}}', + read: false, + }, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'Process completed!', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(3); + expect(result[0].showRating).toBeUndefined(); + expect(result[1].showRating).toBeUndefined(); + expect(result[1].type).toBe('workflow-updated'); + expect(result[2]).toMatchObject({ + id: 'msg-2', + content: 'Process completed!', + showRating: true, + ratingStyle: 'regular', + }); + }); + + it('should not apply rating when tools are still running', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{}', + read: false, + }, + { + id: 'tool-1', + role: 'assistant', + type: 'tool', + toolName: 'add_nodes', + toolCallId: 'call-1', + status: 'running', + updates: [], + read: false, + }, + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Working on it...', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(3); + result.forEach((message) => { + expect(message.showRating).toBeUndefined(); + }); + }); + + it('should not apply rating when still thinking (tools completed but no text response)', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{}', + read: false, + }, + { + id: 'tool-1', + role: 'assistant', + type: 'tool', + toolName: 'add_nodes', + toolCallId: 'call-1', + status: 'completed', + updates: [], + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(2); + result.forEach((message) => { + expect(message.showRating).toBeUndefined(); + }); + }); + + it('should not apply rating when no workflow-updated message exists', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Hello there!', + read: false, + }, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'How can I help?', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(2); + expect(result[0].showRating).toBeUndefined(); + expect(result[1].showRating).toBeUndefined(); + }); + + it('should remove existing ratings when tools are running', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Previous message', + showRating: true, + ratingStyle: 'regular', + read: false, + }, + { + id: 'tool-1', + role: 'assistant', + type: 'tool', + toolName: 'add_nodes', + toolCallId: 'call-1', + status: 'running', + updates: [], + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(2); + expect(result[0]).not.toHaveProperty('showRating'); + expect(result[0]).not.toHaveProperty('ratingStyle'); + }); + + it('should remove ratings from non-target messages when applying rating to target message', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Earlier message', + showRating: true, + ratingStyle: 'minimal', + read: false, + }, + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{}', + read: false, + }, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'Target message', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(3); + expect(result[0]).not.toHaveProperty('showRating'); + expect(result[0]).not.toHaveProperty('ratingStyle'); + expect(result[2]).toMatchObject({ + showRating: true, + ratingStyle: 'regular', + }); + }); + + it('should handle multiple workflow-updated messages and apply rating after the last one', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{"nodes": []}', + read: false, + }, + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'First update done', + read: false, + }, + { + id: 'workflow-2', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{"nodes": [{"name": "HTTP"}]}', + read: false, + }, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'Final update complete', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(4); + expect(result[1].showRating).toBeUndefined(); // First text message + expect(result[3]).toMatchObject({ + content: 'Final update complete', + showRating: true, + ratingStyle: 'regular', + }); + }); + + it('should handle user messages mixed with assistant messages', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'user-1', + role: 'user', + type: 'text', + content: 'Create a workflow', + read: true, + }, + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{}', + read: false, + }, + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Workflow created!', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(3); + expect(result[0].showRating).toBeUndefined(); // User message + expect(result[2]).toMatchObject({ + content: 'Workflow created!', + showRating: true, + ratingStyle: 'regular', + }); + }); + + it('should handle empty message array', () => { + const result = builderMessages.applyRatingLogic([]); + expect(result).toHaveLength(0); + }); + + it('should apply rating only to assistant text messages, not user text messages', () => { + const messages: ChatUI.AssistantMessage[] = [ + { + id: 'workflow-1', + role: 'assistant', + type: 'workflow-updated', + codeSnippet: '{}', + read: false, + }, + { + id: 'user-1', + role: 'user', + type: 'text', + content: 'Thanks!', + read: true, + }, + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'You are welcome!', + read: false, + }, + ]; + + const result = builderMessages.applyRatingLogic(messages); + + expect(result).toHaveLength(3); + expect(result[1].showRating).toBeUndefined(); // User message should not have rating + expect(result[2]).toMatchObject({ + content: 'You are welcome!', + showRating: true, + ratingStyle: 'regular', + }); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/composables/useBuilderMessages.ts b/packages/frontend/editor-ui/src/composables/useBuilderMessages.ts index aa2930a8e3..42c57b7fd5 100644 --- a/packages/frontend/editor-ui/src/composables/useBuilderMessages.ts +++ b/packages/frontend/editor-ui/src/composables/useBuilderMessages.ts @@ -12,31 +12,32 @@ export interface MessageProcessingResult { export function useBuilderMessages() { const locale = useI18n(); + /** + * Clear rating from all messages + */ + function clearRatingLogic(messages: ChatUI.AssistantMessage[]): ChatUI.AssistantMessage[] { + return messages.map((message) => { + if (message.type === 'text' && 'showRating' in message) { + // Pick all properties except showRating and ratingStyle + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { showRating, ratingStyle, ...cleanMessage } = message; + return cleanMessage; + } + return message; + }); + } + /** * Apply rating logic to messages - only show rating on the last AI text message after workflow-updated * when no tools are running */ function applyRatingLogic(messages: ChatUI.AssistantMessage[]): ChatUI.AssistantMessage[] { - // Check if any tools are still running - const hasRunningTools = messages.some( - (m) => m.type === 'tool' && (m as ChatUI.ToolMessage).status === 'running', - ); + const { hasAnyRunningTools, isStillThinking } = getThinkingState(messages); // Don't apply rating if tools are still running - if (hasRunningTools) { + if (hasAnyRunningTools || isStillThinking) { // Remove any existing ratings - return messages.map((message) => { - if (message.type === 'text' && 'showRating' in message) { - // Pick all properties except showRating and ratingStyle - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { showRating, ratingStyle, ...cleanMessage } = message as ChatUI.TextMessage & { - showRating?: boolean; - ratingStyle?: string; - }; - return cleanMessage; - } - return message; - }); + return clearRatingLogic(messages); } // Find the index of the last workflow-updated message @@ -82,10 +83,7 @@ export function useBuilderMessages() { // Remove any existing rating from other messages if (message.type === 'text' && 'showRating' in message) { // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { showRating, ratingStyle, ...cleanMessage } = message as ChatUI.TextMessage & { - showRating?: boolean; - ratingStyle?: string; - }; + const { showRating, ratingStyle, ...cleanMessage } = message; return cleanMessage; } return message; @@ -109,14 +107,14 @@ export function useBuilderMessages() { type: 'text', content: msg.text, read: false, - } as ChatUI.AssistantMessage); + } satisfies ChatUI.AssistantMessage); shouldClearThinking = true; } else if (isWorkflowUpdatedMessage(msg)) { messages.push({ ...msg, id: messageId, read: false, - } as ChatUI.AssistantMessage); + } satisfies ChatUI.AssistantMessage); // Don't clear thinking for workflow updates - they're just state changes } else if (isToolMessage(msg)) { processToolMessage(messages, msg, messageId); @@ -149,9 +147,7 @@ export function useBuilderMessages() { // Check if we already have this tool message const existingIndex = msg.toolCallId - ? messages.findIndex( - (m) => m.type === 'tool' && (m as ChatUI.ToolMessage).toolCallId === msg.toolCallId, - ) + ? messages.findIndex((m) => m.type === 'tool' && m.toolCallId === msg.toolCallId) : -1; if (existingIndex !== -1) { @@ -180,21 +176,35 @@ export function useBuilderMessages() { } /** - * Determine the thinking message based on tool states + * If any tools are running, then it's still running tools and not done thinking + * If all tools are done and no text response yet, then it's still thinking + * Otherwise, it's done + * + * @param messages + * @returns */ - function determineThinkingMessage(messages: ChatUI.AssistantMessage[]): string | undefined { - // Check ALL messages to determine state + function getThinkingState(messages: ChatUI.AssistantMessage[]): { + hasAnyRunningTools: boolean; + isStillThinking: boolean; + } { const allToolMessages = messages.filter( (msg): msg is ChatUI.ToolMessage => msg.type === 'tool', ); const hasAnyRunningTools = allToolMessages.some((msg) => msg.status === 'running'); + if (hasAnyRunningTools) { + return { + hasAnyRunningTools: true, + isStillThinking: false, + }; + } + const hasCompletedTools = allToolMessages.some((msg) => msg.status === 'completed'); // Find the last completed tool message let lastCompletedToolIndex = -1; for (let i = messages.length - 1; i >= 0; i--) { const msg = messages[i]; - if (msg.type === 'tool' && (msg as ChatUI.ToolMessage).status === 'completed') { + if (msg.type === 'tool' && msg.status === 'completed') { lastCompletedToolIndex = i; break; } @@ -213,12 +223,21 @@ export function useBuilderMessages() { } } - // - If any tools are running, show "Running tools..." - // - If all tools are done and no text response yet, show "Processing results..." - // - Otherwise, clear the thinking message + return { + hasAnyRunningTools: false, + isStillThinking: hasCompletedTools && !hasTextAfterTools, + }; + } + + /** + * Determine the thinking message based on tool states + */ + function determineThinkingMessage(messages: ChatUI.AssistantMessage[]): string | undefined { + const { hasAnyRunningTools, isStillThinking } = getThinkingState(messages); + if (hasAnyRunningTools) { return locale.baseText('aiAssistant.thinkingSteps.runningTools'); - } else if (hasCompletedTools && !hasTextAfterTools) { + } else if (isStillThinking) { return locale.baseText('aiAssistant.thinkingSteps.processingResults'); } @@ -259,7 +278,7 @@ export function useBuilderMessages() { type: 'text', content, read: true, - } as ChatUI.AssistantMessage; + }; } function createAssistantMessage(content: string, id: string): ChatUI.AssistantMessage { @@ -269,7 +288,7 @@ export function useBuilderMessages() { type: 'text', content, read: true, - } as ChatUI.AssistantMessage; + }; } function createErrorMessage( @@ -284,7 +303,7 @@ export function useBuilderMessages() { content, retry, read: false, - } as ChatUI.AssistantMessage; + }; } function clearMessages(): ChatUI.AssistantMessage[] { @@ -310,7 +329,7 @@ export function useBuilderMessages() { type: 'text', content: message.text, read: false, - } as ChatUI.AssistantMessage; + } satisfies ChatUI.AssistantMessage; } if (isWorkflowUpdatedMessage(message)) { @@ -318,7 +337,7 @@ export function useBuilderMessages() { ...message, id, read: false, - } as ChatUI.AssistantMessage; + } satisfies ChatUI.AssistantMessage; } if (isToolMessage(message)) { @@ -331,7 +350,7 @@ export function useBuilderMessages() { status: message.status, updates: message.updates || [], read: false, - } as ChatUI.AssistantMessage; + } satisfies ChatUI.AssistantMessage; } // Handle event messages @@ -340,7 +359,7 @@ export function useBuilderMessages() { ...message, id, read: false, - } as ChatUI.AssistantMessage; + } satisfies ChatUI.AssistantMessage; } // Default fallback @@ -350,7 +369,7 @@ export function useBuilderMessages() { type: 'text', content: locale.baseText('aiAssistant.thinkingSteps.thinking'), read: false, - } as ChatUI.AssistantMessage; + } satisfies ChatUI.AssistantMessage; } return { @@ -361,5 +380,7 @@ export function useBuilderMessages() { clearMessages, addMessages, mapAssistantMessageToUI, + applyRatingLogic, + clearRatingLogic, }; } diff --git a/packages/frontend/editor-ui/src/stores/builder.store.test.ts b/packages/frontend/editor-ui/src/stores/builder.store.test.ts index da29fc349c..5f3eed0405 100644 --- a/packages/frontend/editor-ui/src/stores/builder.store.test.ts +++ b/packages/frontend/editor-ui/src/stores/builder.store.test.ts @@ -17,6 +17,13 @@ import type { Telemetry } from '@/plugins/telemetry'; import type { ChatUI } from '@n8n/design-system/types/assistant'; import { DEFAULT_CHAT_WIDTH, MAX_CHAT_WIDTH, MIN_CHAT_WIDTH } from './assistant.store'; +// Mock useI18n to return the keys instead of translations +vi.mock('@n8n/i18n', () => ({ + useI18n: () => ({ + baseText: (key: string) => key, + }), +})); + let settingsStore: ReturnType; let posthogStore: ReturnType; @@ -195,8 +202,8 @@ describe('AI Builder store', () => { builderStore.sendChatMessage({ text: 'Add nodes and connect them' }); - // Initially shows "Thinking..." from prepareForStreaming - expect(builderStore.assistantThinkingMessage).toBe('Thinking...'); + // Initially shows "aiAssistant.thinkingSteps.thinking" from prepareForStreaming + expect(builderStore.assistantThinkingMessage).toBe('aiAssistant.thinkingSteps.thinking'); // First tool starts onMessageCallback({ @@ -212,8 +219,8 @@ describe('AI Builder store', () => { ], }); - // Should show "Running tools..." - expect(builderStore.assistantThinkingMessage).toBe('Running tools...'); + // Should show "aiAssistant.thinkingSteps.runningTools" + expect(builderStore.assistantThinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); // Second tool starts (different toolCallId) onMessageCallback({ @@ -229,8 +236,8 @@ describe('AI Builder store', () => { ], }); - // Still showing "Running tools..." with multiple tools - expect(builderStore.assistantThinkingMessage).toBe('Running tools...'); + // Still showing "aiAssistant.thinkingSteps.runningTools" with multiple tools + expect(builderStore.assistantThinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); // First tool completes onMessageCallback({ @@ -246,8 +253,8 @@ describe('AI Builder store', () => { ], }); - // Still "Running tools..." because second tool is still running - expect(builderStore.assistantThinkingMessage).toBe('Running tools...'); + // Still "aiAssistant.thinkingSteps.runningTools" because second tool is still running + expect(builderStore.assistantThinkingMessage).toBe('aiAssistant.thinkingSteps.runningTools'); // Second tool completes onMessageCallback({ @@ -263,15 +270,19 @@ describe('AI Builder store', () => { ], }); - // Now should show "Processing results..." because all tools completed - expect(builderStore.assistantThinkingMessage).toBe('Processing results...'); + // Now should show "aiAssistant.thinkingSteps.processingResults" because all tools completed + expect(builderStore.assistantThinkingMessage).toBe( + 'aiAssistant.thinkingSteps.processingResults', + ); // Call onDone to stop streaming onDoneCallback(); // Message should persist after streaming ends expect(builderStore.streaming).toBe(false); - expect(builderStore.assistantThinkingMessage).toBe('Processing results...'); + expect(builderStore.assistantThinkingMessage).toBe( + 'aiAssistant.thinkingSteps.processingResults', + ); vi.useRealTimers(); }); @@ -311,14 +322,18 @@ describe('AI Builder store', () => { builderStore.sendChatMessage({ text: 'Add a node' }); - // Should show "Processing results..." when tool completes + // Should show "aiAssistant.thinkingSteps.processingResults" when tool completes await vi.waitFor(() => - expect(builderStore.assistantThinkingMessage).toBe('Processing results...'), + expect(builderStore.assistantThinkingMessage).toBe( + 'aiAssistant.thinkingSteps.processingResults', + ), ); - // Should still show "Processing results..." after workflow-updated + // Should still show "aiAssistant.thinkingSteps.processingResults" after workflow-updated await vi.waitFor(() => expect(builderStore.chatMessages).toHaveLength(3)); // user + tool + workflow - expect(builderStore.assistantThinkingMessage).toBe('Processing results...'); + expect(builderStore.assistantThinkingMessage).toBe( + 'aiAssistant.thinkingSteps.processingResults', + ); // Verify streaming has ended expect(builderStore.streaming).toBe(false); @@ -709,4 +724,58 @@ describe('AI Builder store', () => { expect((assistantMessages[0] as ChatUI.TextMessage).content).toBe('[Task aborted]'); }); }); + + describe('Rating logic integration', () => { + it('should clear ratings from existing messages when preparing for streaming', () => { + const builderStore = useBuilderStore(); + + // Setup initial messages with ratings + builderStore.chatMessages = [ + { + id: 'msg-1', + role: 'assistant', + type: 'text', + content: 'Previous message', + showRating: true, + ratingStyle: 'regular', + read: false, + } satisfies ChatUI.AssistantMessage, + { + id: 'msg-2', + role: 'assistant', + type: 'text', + content: 'Another message', + showRating: true, + ratingStyle: 'minimal', + read: false, + } satisfies ChatUI.AssistantMessage, + ]; + + // Mock API to prevent actual network calls + apiSpy.mockImplementationOnce(() => {}); + + // Send new message which calls prepareForStreaming + builderStore.sendChatMessage({ text: 'New message' }); + + // Verify that existing messages no longer have rating properties + expect(builderStore.chatMessages).toHaveLength(3); // 2 existing + 1 new user message + + const firstMessage = builderStore.chatMessages[0] as ChatUI.TextMessage; + expect(firstMessage).not.toHaveProperty('showRating'); + expect(firstMessage).not.toHaveProperty('ratingStyle'); + expect(firstMessage.content).toBe('Previous message'); + + const secondMessage = builderStore.chatMessages[1] as ChatUI.TextMessage; + expect(secondMessage).not.toHaveProperty('showRating'); + expect(secondMessage).not.toHaveProperty('ratingStyle'); + expect(secondMessage.content).toBe('Another message'); + + // New user message should not have rating properties + const userMessage = builderStore.chatMessages[2] as ChatUI.TextMessage; + expect(userMessage.role).toBe('user'); + expect(userMessage.content).toBe('New message'); + expect(userMessage).not.toHaveProperty('showRating'); + expect(userMessage).not.toHaveProperty('ratingStyle'); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/stores/builder.store.ts b/packages/frontend/editor-ui/src/stores/builder.store.ts index 60e7884faf..d3f593dce2 100644 --- a/packages/frontend/editor-ui/src/stores/builder.store.ts +++ b/packages/frontend/editor-ui/src/stores/builder.store.ts @@ -56,6 +56,7 @@ export const useBuilderStore = defineStore(STORES.BUILDER, () => { createErrorMessage, clearMessages, mapAssistantMessageToUI, + clearRatingLogic, } = useBuilderMessages(); // Computed properties @@ -203,7 +204,7 @@ export const useBuilderStore = defineStore(STORES.BUILDER, () => { */ function prepareForStreaming(userMessage: string, messageId: string) { const userMsg = createUserMessage(userMessage, messageId); - chatMessages.value = [...chatMessages.value, userMsg]; + chatMessages.value = clearRatingLogic([...chatMessages.value, userMsg]); addLoadingAssistantMessage(locale.baseText('aiAssistant.thinkingSteps.thinking')); streaming.value = true; }