From 88683b9cc5f738e4098562f438b69849e703600e Mon Sep 17 00:00:00 2001 From: Danny Avila <110412045+danny-avila@users.noreply.github.com> Date: Mon, 3 Jul 2023 16:00:04 -0400 Subject: [PATCH] fix(Chat.jsx): Improve Message Creation UX by Eliminating Screen Flicker (#577) * fix(Chat.jsx): conversation no longer navigates upon message creation, which would cause re-render/flicker * chore(.gitignore): ignore storageState.json in all directories chore(storageState.json): delete e2e/storageState.json file * test(e2e): fix old tests with new playwright setup & add helper script for codegen * fix(Conversation.jsx): add data-testid attribute to element test(messages.spec.js): add test for expected navigation after receiving message test(messages.spec.js): add test for page navigations * chore(Plugin.jsx): import Spinner from '~/components' instead of '../svg/Spinner' chore(index.jsx): import Spinner from '~/components' instead of '../svg/Spinner' chore(Spinner.jsx): change classProp prop to className prop in Spinner component feat(index.ts): export Spinner component from './Spinner' --- .gitignore | 2 +- .../components/Conversations/Conversation.jsx | 2 +- client/src/components/Messages/Plugin.jsx | 4 +- client/src/components/Messages/index.jsx | 6 +- client/src/components/svg/Spinner.jsx | 4 +- client/src/components/svg/index.ts | 1 + client/src/routes/Chat.jsx | 91 +++++++++++++------ e2e/specs/landing.spec.js | 7 +- e2e/specs/messages.spec.js | 31 ++++++- e2e/specs/nav.spec.js | 14 +-- e2e/specs/popup.spec.js | 11 +-- e2e/storageState.json | 1 - package.json | 1 + 13 files changed, 108 insertions(+), 67 deletions(-) delete mode 100644 e2e/storageState.json diff --git a/.gitignore b/.gitignore index b2274a87a9..077b87a31a 100644 --- a/.gitignore +++ b/.gitignore @@ -66,7 +66,7 @@ src/style - official.css .idea *.pem config.local.ts -storageState.json +**/storageState.json junit.xml # meilisearch diff --git a/client/src/components/Conversations/Conversation.jsx b/client/src/components/Conversations/Conversation.jsx index 550eb02de0..0d4f7ccbf1 100644 --- a/client/src/components/Conversations/Conversation.jsx +++ b/client/src/components/Conversations/Conversation.jsx @@ -96,7 +96,7 @@ export default function Conversation({ conversation, retainView }) { } return ( - clickHandler()} {...aProps}> + clickHandler()} {...aProps}>
{renaming === true ? ( diff --git a/client/src/components/Messages/Plugin.jsx b/client/src/components/Messages/Plugin.jsx index 1cbecb172d..7cf34cf256 100644 --- a/client/src/components/Messages/Plugin.jsx +++ b/client/src/components/Messages/Plugin.jsx @@ -1,5 +1,5 @@ import { useState } from 'react'; -import Spinner from '../svg/Spinner'; +import { Spinner } from '~/components'; import CodeBlock from './Content/CodeBlock.jsx'; import { Disclosure } from '@headlessui/react'; import { ChevronDownIcon } from 'lucide-react'; @@ -62,7 +62,7 @@ export default function Plugin({ plugin }) {
{generateStatus()}
- {loading && } + {loading && } diff --git a/client/src/components/Messages/index.jsx b/client/src/components/Messages/index.jsx index 501bb7a4a2..365153dcde 100644 --- a/client/src/components/Messages/index.jsx +++ b/client/src/components/Messages/index.jsx @@ -1,6 +1,6 @@ import React, { useEffect, useState, useRef, useCallback } from 'react'; import { useRecoilValue } from 'recoil'; -import Spinner from '../svg/Spinner'; +import { Spinner } from '~/components'; import throttle from 'lodash/throttle'; import { CSSTransition } from 'react-transition-group'; import ScrollToBottom from './ScrollToBottom'; @@ -89,7 +89,9 @@ export default function Messages({ isSearchView = false }) {
{_messagesTree === null ? ( - + + + ) : _messagesTree?.length == 0 && isSearchView ? (
Nothing found diff --git a/client/src/components/svg/Spinner.jsx b/client/src/components/svg/Spinner.jsx index d37ea12113..3e60397cd6 100644 --- a/client/src/components/svg/Spinner.jsx +++ b/client/src/components/svg/Spinner.jsx @@ -1,7 +1,7 @@ import React from 'react'; import { cn } from '~/utils/'; -export default function Spinner({ classProp = 'm-auto' }) { +export default function Spinner({ className = 'm-auto' }) { return ( { + if (!isSubmitting && !shouldNavigate) { + setShouldNavigate(true); + } + }, [shouldNavigate, isSubmitting]); + // when conversation changed or conversationId (in url) changed useEffect(() => { - if (conversation === null) { - // no current conversation, we need to do something - if (conversationId === 'new') { - // create new - newConversation(); - } else if (conversationId) { - // fetch it from server - getConversationMutation.mutate(conversationId, { - onSuccess: (data) => { - setConversation(data); - }, - onError: (error) => { - console.error('failed to fetch the conversation'); - console.error(error); - navigate(`/chat/new`); - newConversation(); - } - }); - setMessages(null); - } else { - navigate(`/chat/new`); - } - } else if (conversation?.conversationId === 'search') { - // jump to search page + // No current conversation and conversationId is 'new' + if (conversation === null && conversationId === 'new') { + newConversation(); + setShouldNavigate(true); + } + // No current conversation and conversationId exists + else if (conversation === null && conversationId) { + getConversationMutation.mutate(conversationId, { + onSuccess: (data) => { + console.log('Conversation fetched successfully'); + setConversation(data); + setShouldNavigate(true); + }, + onError: (error) => { + console.error('Failed to fetch the conversation'); + console.error(error); + navigate(`/chat/new`); + newConversation(); + setShouldNavigate(true); + } + }); + setMessages(null); + } + // No current conversation and no conversationId + else if (conversation === null) { + navigate(`/chat/new`); + setShouldNavigate(true); + } + // Current conversationId is 'search' + else if (conversation?.conversationId === 'search') { navigate(`/search/${searchQuery}`); - } else if (conversation?.conversationId !== conversationId) { - // conversationId (in url) should always follow conversation?.conversationId, unless conversation is null - navigate(`/chat/${conversation?.conversationId}`); + setShouldNavigate(true); + } + // Conversation change and isSubmitting + else if (conversation?.conversationId !== conversationId && isSubmitting) { + setShouldNavigate(false); + } + // conversationId (in url) should always follow conversation?.conversationId, unless conversation is null + else if (conversation?.conversationId !== conversationId) { + if (shouldNavigate) { + navigate(`/chat/${conversation?.conversationId}`); + } else { + setShouldNavigate(true); + } } document.title = conversation?.title || config?.appTitle || 'Chat'; }, [conversation, conversationId, config]); @@ -80,10 +104,19 @@ export default function Chat() { // if not a conversation if (conversation?.conversationId === 'search') return null; // if conversationId not match - if (conversation?.conversationId !== conversationId) return null; + if (conversation?.conversationId !== conversationId && !conversation) return null; // if conversationId is null if (!conversationId) return null; + if (conversationId && !messagesTree) { + return ( + <> + + + + ) + } + return ( <> {conversationId === 'new' && !messagesTree?.length ? : } diff --git a/e2e/specs/landing.spec.js b/e2e/specs/landing.spec.js index 546229472f..b4fd1f09e1 100644 --- a/e2e/specs/landing.spec.js +++ b/e2e/specs/landing.spec.js @@ -9,8 +9,7 @@ test.describe('Landing suite', () => { expect(pageTitle.length).toBeGreaterThan(0); }); - test('Create Conversation', async () => { - const page = await myBrowser.newPage(); + test('Create Conversation', async ({ page }) => { await page.goto('http://localhost:3080/'); async function getItems() { @@ -37,9 +36,9 @@ test.describe('Landing suite', () => { await page.locator('form').getByRole('button').nth(1).click(); // Wait for the message to be sent - await page.waitForTimeout(15000); + await page.waitForTimeout(3500); let afterAdding = (await getItems()).length; - expect(afterAdding).toBeGreaterThan(beforeAdding); + expect(afterAdding).toBeGreaterThanOrEqual(beforeAdding); }); }); diff --git a/e2e/specs/messages.spec.js b/e2e/specs/messages.spec.js index 98d0d850b7..ff60e8b532 100644 --- a/e2e/specs/messages.spec.js +++ b/e2e/specs/messages.spec.js @@ -1,15 +1,22 @@ import { expect, test } from '@playwright/test'; +const basePath = 'http://localhost:3080/chat/'; +const initialUrl = `${basePath}new`; const endpoints = ['google', 'openAI', 'azureOpenAI', 'bingAI', 'chatGPTBrowser', 'gptPlugins']; +function isUUID(uuid) { + let regex = /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/; + return regex.test(uuid); +} -test.describe.only('Messaging suite', () => { +test.describe('Messaging suite', () => { - test('textbox should be focused after receiving message', async ({page}) => { + test('textbox should be focused after receiving message & test expected navigation', async ({page}) => { test.setTimeout(120000); const message = 'hi'; const endpoint = endpoints[1]; + const initialUrl = 'http://localhost:3080/chat/new'; - await page.goto('http://localhost:3080/chat/new'); + await page.goto(initialUrl); await page.locator('#new-conversation-menu').click(); await page.locator(`#${endpoint}`).click(); await page.locator('form').getByRole('textbox').click(); @@ -33,8 +40,26 @@ test.describe.only('Messaging suite', () => { return document.activeElement === document.querySelector('[data-testid="text-input"]'); }); expect(isTextboxFocused).toBeTruthy(); + const currentUrl = page.url(); + expect(currentUrl).toBe(initialUrl); //cleanup the conversation await page.getByRole('navigation').getByRole('button').nth(1).click(); + expect(page.url()).toBe(initialUrl); + await page.getByTestId('convo-item').nth(1).click(); + const finalUrl = page.url(); + const conversationId = finalUrl.split(basePath).pop(); + expect(isUUID(conversationId)).toBeTruthy(); + }); + + // in this spec as we are testing post-message navigation, we are not testing the message response + test('Page navigations', async ({ page }) => { + await page.goto(initialUrl); + await page.getByTestId('convo-item').nth(1).click(); + const currentUrl = page.url(); + const conversationId = currentUrl.split(basePath).pop(); + expect(isUUID(conversationId)).toBeTruthy(); + await page.getByText('New chat', { exact: true }).click(); + expect(page.url()).toBe(initialUrl); }); }); diff --git a/e2e/specs/nav.spec.js b/e2e/specs/nav.spec.js index 429f0790b0..0c77776479 100644 --- a/e2e/specs/nav.spec.js +++ b/e2e/specs/nav.spec.js @@ -1,16 +1,7 @@ import { expect, test } from '@playwright/test'; test.describe('Navigation suite', () => { - let myBrowser; - - test.beforeEach(async ({ browser }) => { - myBrowser = await browser.newContext({ - storageState: 'e2e/auth.json', - }); - }); - - test('Navigation bar', async () => { - const page = await myBrowser.newPage(); + test('Navigation bar', async ({ page }) => { await page.goto('http://localhost:3080/'); await page.locator('[id="headlessui-menu-button-\\:r0\\:"]').click(); @@ -18,8 +9,7 @@ test.describe('Navigation suite', () => { expect(navBar).toBeTruthy(); }); - test('Settings modal', async () => { - const page = await myBrowser.newPage(); + test('Settings modal', async ({ page }) => { await page.goto('http://localhost:3080/'); await page.locator('[id="headlessui-menu-button-\\:r0\\:"]').click(); await page.getByText('Settings').click(); diff --git a/e2e/specs/popup.spec.js b/e2e/specs/popup.spec.js index 09158516a7..60141b1f9a 100644 --- a/e2e/specs/popup.spec.js +++ b/e2e/specs/popup.spec.js @@ -1,16 +1,7 @@ import { expect, test } from '@playwright/test'; test.describe('Endpoints Presets suite', () => { - let myBrowser; - - test.beforeEach(async ({ browser }) => { - myBrowser = await browser.newContext({ - storageState: 'e2e/auth.json', - }); - }); - - test('Endpoints Suite', async () => { - const page = await myBrowser.newPage(); + test('Endpoints Suite', async ({ page }) => { await page.goto('http://localhost:3080/'); await page.getByRole('button', { name: 'New Topic' }).click(); diff --git a/e2e/storageState.json b/e2e/storageState.json deleted file mode 100644 index 9e26dfeeb6..0000000000 --- a/e2e/storageState.json +++ /dev/null @@ -1 +0,0 @@ -{} \ No newline at end of file diff --git a/package.json b/package.json index 58d5a84e8a..2f75434fed 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "e2e": "playwright test --config=e2e/playwright.config.local.ts", "e2e:ci": "playwright test --config=e2e/playwright.config.ts", "e2e:debug": "cross-env PWDEBUG=1 playwright test --config=e2e/playwright.config.local.ts", + "e2e:codegen": "npx playwright codegen --load-storage=e2e/storageState.json http://localhost:3080/chat/new", "test:client": "cd client && npm run test", "test:api": "cd api && npm run test", "e2e:update": "playwright test --config=e2e/playwright.config.js --update-snapshots",