From b2f8f054ad74543c104c087ffc3199aed03192e2 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Sat, 25 May 2024 22:22:57 +0800 Subject: [PATCH] improve handling of human message cell states, show visual indicator of message submission --- vscode/webviews/chat/Transcript.tsx | 2 + .../human/HumanMessageCell.story.tsx | 25 +++-- .../messageCell/human/HumanMessageCell.tsx | 5 + .../editor/HumanMessageEditor.module.css | 8 +- .../human/editor/HumanMessageEditor.test.tsx | 106 ++++++++++++++++-- .../human/editor/HumanMessageEditor.tsx | 66 +++++++++-- .../editor/toolbar/SubmitButton.story.tsx | 39 +++++++ .../human/editor/toolbar/SubmitButton.tsx | 7 +- .../human/editor/toolbar/Toolbar.tsx | 9 ++ 9 files changed, 234 insertions(+), 33 deletions(-) create mode 100644 vscode/webviews/chat/cells/messageCell/human/editor/toolbar/SubmitButton.story.tsx diff --git a/vscode/webviews/chat/Transcript.tsx b/vscode/webviews/chat/Transcript.tsx index d1966b27351f..5c939f66e84c 100644 --- a/vscode/webviews/chat/Transcript.tsx +++ b/vscode/webviews/chat/Transcript.tsx @@ -61,6 +61,7 @@ export const Transcript: React.FunctionComponent<{ chatEnabled={chatEnabled} isFirstMessage={messageIndexInTranscript === 0} isSent={true} + isPendingResponse={isLastHumanMessage && isLoading} onSubmit={( editorValue: SerializedPromptEditorValue, addEnhancedContext: boolean @@ -125,6 +126,7 @@ export const Transcript: React.FunctionComponent<{ message={null} isFirstMessage={transcript.length === 0} isSent={false} + isPendingResponse={false} userInfo={userInfo} chatEnabled={chatEnabled} isEditorInitiallyFocused={true} diff --git a/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.story.tsx b/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.story.tsx index edfa6f783e8e..04cd64de8ab5 100644 --- a/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.story.tsx +++ b/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.story.tsx @@ -10,10 +10,8 @@ const meta: Meta = { component: HumanMessageCell, args: { - message: null, userInfo: FIXTURE_USER_ACCOUNT_INFO, onSubmit: () => {}, - __storybook__focus: false, }, decorators: [VSCodeCell], @@ -21,29 +19,32 @@ const meta: Meta = { export default meta -export const FirstMessageEmpty: StoryObj = { +export const NonEmptyFirstMessage: StoryObj = { args: { - isFirstMessage: true, + message: FIXTURE_TRANSCRIPT.explainCode2[0], + __storybook__focus: true, }, } -export const FirstMessageWithText: StoryObj = { +export const EmptyFollowup: StoryObj = { args: { - message: FIXTURE_TRANSCRIPT.explainCode2[0], - isFirstMessage: true, + message: null, + __storybook__focus: true, }, } -export const FollowupEmpty: StoryObj = { +export const SentPending: StoryObj = { args: { - message: null, - isFirstMessage: false, + message: FIXTURE_TRANSCRIPT.explainCode2[0], + isSent: true, + isPendingResponse: true, + __storybook__focus: true, }, } -export const FollowupWithText: StoryObj = { +export const SentComplete: StoryObj = { args: { message: FIXTURE_TRANSCRIPT.explainCode2[0], - isFirstMessage: false, + isSent: true, }, } diff --git a/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.tsx b/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.tsx index 04450f9464cf..72328eeb41de 100644 --- a/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.tsx +++ b/vscode/webviews/chat/cells/messageCell/human/HumanMessageCell.tsx @@ -27,6 +27,9 @@ export const HumanMessageCell: FunctionComponent<{ /** Whether this editor is for a message that has been sent already. */ isSent: boolean + /** Whether this editor is for a message whose assistant response is in progress. */ + isPendingResponse: boolean + onChange?: (editorState: SerializedPromptEditorValue) => void onSubmit: (editorValue: SerializedPromptEditorValue, addEnhancedContext: boolean) => void @@ -43,6 +46,7 @@ export const HumanMessageCell: FunctionComponent<{ userContextFromSelection, isFirstMessage, isSent, + isPendingResponse, onChange, onSubmit, isEditorInitiallyFocused, @@ -68,6 +72,7 @@ export const HumanMessageCell: FunctionComponent<{ } isFirstMessage={isFirstMessage} isSent={isSent} + isPendingResponse={isPendingResponse} onChange={onChange} onSubmit={onSubmit} disabled={!chatEnabled} diff --git a/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.module.css b/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.module.css index 2f0ffd95847d..708db0d5c2fc 100644 --- a/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.module.css +++ b/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.module.css @@ -14,12 +14,12 @@ } /* Dim toolbar when unfocused. */ -.container:not(:is(:not(.sent), .focused, :focus-within, :hover)) .toolbar { +.container:not(:is(:not(.sent-complete), .focused, :hover)) .toolbar { opacity: 0.5; } /* Make the whole container look like a big input field when focused. */ -.container:is(:not(.sent), .focused, :hover) { +.container:is(:not(.sent-complete), .focused, :hover) { background-color: var(--vscode-input-background); color: var(--vscode-input-foreground); @@ -41,11 +41,11 @@ } /* Hide the toolbar unless focused on all but the last cell. */ -[role="row"]:has(.container:is(:not(.sent), .focused, :focus-within)) { +[role="row"]:has(.container:is(:not(.sent-complete), .focused)) { /* HACK: Change HumanMessageCell style. */ padding-bottom: var(--human-message-editor-cell-spacing-bottom); } -.container.sent:not(:is(.focused, :focus-within)) { +.container.sent-complete:not(.focused) { margin-bottom: calc(var(--human-message-editor-toolbar-height) + var(--human-message-editor-gap) - (var(--cell-spacing) - var(--human-message-editor-cell-spacing-bottom))); .toolbar { display: none; diff --git a/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.test.tsx b/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.test.tsx index f7e7ad62c327..0ea9df3bcc8a 100644 --- a/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.test.tsx +++ b/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.test.tsx @@ -1,7 +1,8 @@ import { fireEvent, render, screen } from '@testing-library/react' import type { ComponentProps } from 'react' -import { type Mock, describe, expect, test, vi } from 'vitest' +import { type Assertion, type Mock, describe, expect, test, vi } from 'vitest' import { AppWrapper } from '../../../../../AppWrapper' +import { serializedPromptEditorStateFromText } from '../../../../../promptEditor/PromptEditor' import { FILE_MENTION_EDITOR_STATE_FIXTURE } from '../../../../../promptEditor/fixtures' import { FIXTURE_USER_ACCOUNT_INFO } from '../../../../fixtures' import { HumanMessageEditor } from './HumanMessageEditor' @@ -30,15 +31,87 @@ describe('HumanMessageEditor', () => { expect(screen.getByRole('textbox')).toBeInTheDocument() }) + describe('states', () => { + function expectState( + { container, mentionButton, submitButton }: ReturnType, + expected: { + toolbarVisible?: boolean + submitButtonEnabled?: boolean + submitButtonText?: string + } + ): void { + if (expected.toolbarVisible !== undefined) { + notUnless(expect.soft(mentionButton), expected.toolbarVisible).toBeVisible() + notUnless(expect.soft(submitButton), expected.toolbarVisible).toBeVisible() + } + + if (expected.submitButtonEnabled !== undefined) { + notUnless(expect.soft(submitButton), expected.submitButtonEnabled).toBeEnabled() + } + if (expected.submitButtonText !== undefined) { + expect.soft(submitButton).toHaveTextContent(expected.submitButtonText) + } + + function notUnless(assertion: Assertion, value: boolean): Assertion { + return value ? assertion : assertion.not + } + } + + test('unsent', () => { + expectState( + renderWithMocks({ + initialEditorState: serializedPromptEditorStateFromText('abc'), + isSent: false, + }), + { toolbarVisible: true, submitButtonEnabled: true, submitButtonText: 'Send' } + ) + }) + + describe('sent pending', () => { + function renderSentPending(): ReturnType { + const rendered = renderWithMocks({ + initialEditorState: serializedPromptEditorStateFromText('abc'), + isSent: true, + isEditorInitiallyFocused: true, + isPendingResponse: true, + }) + typeInEditor(rendered.editor, 'x') + fireEvent.keyDown(rendered.editor, ENTER_KEYBOARD_EVENT_DATA) + return rendered + } + + test('initial', () => { + const rendered = renderSentPending() + expectState(rendered, { + toolbarVisible: true, + submitButtonEnabled: false, + submitButtonText: 'Send', + }) + }) + }) + + test('sent complete', () => { + const rendered = renderWithMocks({ + initialEditorState: undefined, + isSent: true, + }) + expectState(rendered, { toolbarVisible: false }) + + fireEvent.focus(rendered.editor) + expectState(rendered, { toolbarVisible: true }) + }) + }) + describe('submitting', () => { test('empty editor', () => { const { container, submitButton, onSubmit } = renderWithMocks({ initialEditorState: undefined, + __test_dontTemporarilyDisableSubmit: true, }) expect(submitButton).toBeDisabled() // Click - fireEvent.click(submitButton) + fireEvent.click(submitButton!) expect(onSubmit).toHaveBeenCalledTimes(0) // Enter @@ -50,11 +123,12 @@ describe('HumanMessageEditor', () => { test('submit', async () => { const { container, submitButton, onSubmit } = renderWithMocks({ initialEditorState: FILE_MENTION_EDITOR_STATE_FIXTURE, + __test_dontTemporarilyDisableSubmit: true, }) expect(submitButton).toBeEnabled() // Click - fireEvent.click(submitButton) + fireEvent.click(submitButton!) expect(onSubmit).toHaveBeenCalledTimes(1) expect(onSubmit.mock.lastCall[1]).toBe(true) // addEnhancedContext === true @@ -66,11 +140,10 @@ describe('HumanMessageEditor', () => { }) test('submit w/o context', async () => { - const { container, onSubmit } = renderWithMocks({ + const { container, editor, onSubmit } = renderWithMocks({ initialEditorState: FILE_MENTION_EDITOR_STATE_FIXTURE, + __test_dontTemporarilyDisableSubmit: true, }) - - const editor = container.querySelector('[data-lexical-editor="true"]')! fireEvent.focus(editor) // Click @@ -91,9 +164,19 @@ describe('HumanMessageEditor', () => { }) }) +type EditorHTMLElement = HTMLDivElement & { dataset: { lexicalEditor: 'true' } } + +function typeInEditor(editor: EditorHTMLElement, text: string): void { + fireEvent.focus(editor) + fireEvent.click(editor) + fireEvent.input(editor, { data: text }) +} + function renderWithMocks(props: Partial>): { container: HTMLElement - submitButton: HTMLElement + editor: EditorHTMLElement + mentionButton: HTMLElement | null + submitButton: HTMLElement | null onChange: Mock onSubmit: Mock } { @@ -105,10 +188,10 @@ function renderWithMocks(props: Partial, { @@ -116,7 +199,12 @@ function renderWithMocks(props: Partial('[data-lexical-editor="true"]')!, + mentionButton: screen.queryByRole('button', { name: 'Add context', hidden: true }), + submitButton: screen.queryByRole('button', { + name: 'Send with automatic code context', + hidden: true, + }), onChange, onSubmit, } diff --git a/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.tsx b/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.tsx index c72a5c22c311..0b8954c8ad86 100644 --- a/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.tsx +++ b/vscode/webviews/chat/cells/messageCell/human/editor/HumanMessageEditor.tsx @@ -1,6 +1,13 @@ import type { ContextItem } from '@sourcegraph/cody-shared' import clsx from 'clsx' -import { type FunctionComponent, useCallback, useEffect, useRef, useState } from 'react' +import { + type FocusEventHandler, + type FunctionComponent, + useCallback, + useEffect, + useRef, + useState, +} from 'react' import type { UserAccountInfo } from '../../../../../Chat' import { PromptEditor, @@ -27,6 +34,9 @@ export const HumanMessageEditor: FunctionComponent<{ /** Whether this editor is for a message that has been sent already. */ isSent: boolean + /** Whether this editor is for a message whose assistant response is in progress. */ + isPendingResponse: boolean + disabled?: boolean onChange?: (editorState: SerializedPromptEditorValue) => void @@ -37,6 +47,9 @@ export const HumanMessageEditor: FunctionComponent<{ /** For use in storybooks only. */ __storybook__focus?: boolean + + /** For use in tests only. */ + __test_dontTemporarilyDisableSubmit?: boolean }> = ({ userInfo, userContextFromSelection, @@ -44,33 +57,55 @@ export const HumanMessageEditor: FunctionComponent<{ placeholder, isFirstMessage, isSent, + isPendingResponse, disabled = false, onChange, onSubmit, isEditorInitiallyFocused, className, __storybook__focus, + __test_dontTemporarilyDisableSubmit, }) => { const editorRef = useRef(null) + /** Avoid users pressing repeatedly and accidentally sending lots of messages. */ + const [isSubmitTemporarilyDisabled, setIsSubmitTemporarilyDisabled] = useState(false) + // The only PromptEditor state we really need to track in our own state is whether it's empty. const [isEmptyEditorValue, setIsEmptyEditorValue] = useState(initialEditorState === undefined) const onEditorChange = useCallback( (value: SerializedPromptEditorValue): void => { onChange?.(value) setIsEmptyEditorValue(!value?.text?.trim()) + + // Any change should immediately reenable submit. + setIsSubmitTemporarilyDisabled(false) }, [onChange] ) const onSubmitClick = useCallback( (addEnhancedContext: boolean) => { + if (isSubmitTemporarilyDisabled) { + console.log( + 'Prevented submit because it was temporarily disabled after a recent previous submit.' + ) + return + } + if (!editorRef.current) { throw new Error('No editorRef') } onSubmit(editorRef.current.getSerializedValue(), addEnhancedContext) + + if (!__test_dontTemporarilyDisableSubmit) { + setIsSubmitTemporarilyDisabled(true) + setTimeout((): void => { + setIsSubmitTemporarilyDisabled(false) + }, 2000) + } }, - [onSubmit] + [isSubmitTemporarilyDisabled, onSubmit, __test_dontTemporarilyDisableSubmit] ) const onEditorEnterKey = useCallback( @@ -95,7 +130,14 @@ export const HumanMessageEditor: FunctionComponent<{ const onFocus = useCallback(() => { setIsFocusWithin(true) }, []) - const onBlur = useCallback(() => { + const onBlur = useCallback(event => { + // If we're shifting focus to one of our child elements, just skip this call because we'll + // immediately set it back to true. + const container = event.currentTarget as HTMLElement + if (event.relatedTarget && container.contains(event.relatedTarget)) { + return + } + setIsFocusWithin(false) }, []) @@ -152,14 +194,22 @@ export const HumanMessageEditor: FunctionComponent<{ const focusEditor = useCallback(() => editorRef.current?.setFocus(true), []) + useEffect(() => { + if (__storybook__focus && editorRef.current) { + setTimeout(() => focusEditor()) + } + }, [__storybook__focus, focusEditor]) + + const focused = Boolean(isEditorFocused || isFocusWithin || __storybook__focus) + return ( // biome-ignore lint/a11y/useKeyWithClickEvents: only relevant to click areas