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 <a> 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'
This commit is contained in:
Danny Avila 2023-07-03 16:00:04 -04:00 committed by GitHub
parent 6b843429c5
commit 88683b9cc5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 108 additions and 67 deletions

2
.gitignore vendored
View file

@ -66,7 +66,7 @@ src/style - official.css
.idea
*.pem
config.local.ts
storageState.json
**/storageState.json
junit.xml
# meilisearch

View file

@ -96,7 +96,7 @@ export default function Conversation({ conversation, retainView }) {
}
return (
<a onClick={() => clickHandler()} {...aProps}>
<a data-testid="convo-item" onClick={() => clickHandler()} {...aProps}>
<ConvoIcon />
<div className="relative max-h-5 flex-1 overflow-hidden text-ellipsis break-all">
{renaming === true ? (

View file

@ -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 }) {
<div>{generateStatus()}</div>
</div>
</div>
{loading && <Spinner classProp="ml-1" />}
{loading && <Spinner className="ml-1" />}
<Disclosure.Button className="ml-12 flex items-center gap-2">
<ChevronDownIcon className={cn(open ? 'rotate-180 transform' : '', 'h-4 w-4')} />
</Disclosure.Button>

View file

@ -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 }) {
<div className="dark:gpt-dark-gray flex h-auto flex-col items-center text-sm">
<MessageHeader isSearchView={isSearchView} />
{_messagesTree === null ? (
<body className="h-screen flex items-center justify-center">
<Spinner />
</body>
) : _messagesTree?.length == 0 && isSearchView ? (
<div className="flex w-full items-center justify-center gap-1 bg-gray-50 p-3 text-sm text-gray-500 dark:border-gray-900/50 dark:bg-gray-800 dark:text-gray-300">
Nothing found

View file

@ -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 (
<svg
stroke="currentColor"
@ -10,7 +10,7 @@ export default function Spinner({ classProp = 'm-auto' }) {
viewBox="0 0 24 24"
strokeLinecap="round"
strokeLinejoin="round"
className={cn(classProp, 'animate-spin text-center')}
className={cn(className, 'animate-spin text-center')}
height="1em"
width="1em"
xmlns="http://www.w3.org/2000/svg"

View file

@ -2,4 +2,5 @@ export { default as Plugin } from './Plugin';
export { default as GPTIcon } from './GPTIcon';
export { default as BingIcon } from './BingIcon';
export { default as CogIcon } from './CogIcon';
export { default as Spinner } from './Spinner';
export { default as MessagesSquared } from './MessagesSquared';

View file

@ -1,4 +1,4 @@
import { useEffect } from 'react';
import { useState, useEffect } from 'react';
import { useNavigate, useParams } from 'react-router-dom';
import { useRecoilState, useRecoilValue, useSetRecoilState } from 'recoil';
@ -14,10 +14,12 @@ import {
} from '~/data-provider';
export default function Chat() {
const [shouldNavigate, setShouldNavigate] = useState(true);
const searchQuery = useRecoilValue(store.searchQuery);
const [conversation, setConversation] = useRecoilState(store.conversation);
const setMessages = useSetRecoilState(store.messages);
const messagesTree = useRecoilValue(store.messagesTree);
const isSubmitting = useRecoilValue(store.isSubmitting);
const { newConversation } = store.useConversation();
const { conversationId } = useParams();
const navigate = useNavigate();
@ -27,36 +29,58 @@ export default function Chat() {
const getConversationMutation = useGetConversationByIdMutation(conversationId);
const { data: config } = useGetStartupConfig();
useEffect(() => {
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
// No current conversation and conversationId is 'new'
if (conversation === null && conversationId === 'new') {
newConversation();
} else if (conversationId) {
// fetch it from server
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('Failed to fetch the conversation');
console.error(error);
navigate(`/chat/new`);
newConversation();
setShouldNavigate(true);
}
});
setMessages(null);
} else {
navigate(`/chat/new`);
}
} else if (conversation?.conversationId === 'search') {
// jump to search page
// 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) {
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 (
<>
<Messages />
<TextChat />
</>
)
}
return (
<>
{conversationId === 'new' && !messagesTree?.length ? <Landing /> : <Messages />}

View file

@ -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);
});
});

View file

@ -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);
});
});

View file

@ -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();

View file

@ -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();

View file

@ -1 +0,0 @@
{}

View file

@ -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",