Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chat: preserve unsent chat messages when switching tabs #5847

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion vscode/webviews/Chat.story.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { ExtensionAPIProviderForTestsOnly, MOCK_API } from '@sourcegraph/prompt-editor'
import {
ExtensionAPIProviderForTestsOnly,
MOCK_API,
type SerializedPromptEditorValue,
} from '@sourcegraph/prompt-editor'
import type { Meta, StoryObj } from '@storybook/react'
import { Observable } from 'observable-fns'
import { Chat } from './Chat'
Expand Down Expand Up @@ -27,6 +31,7 @@ const meta: Meta<typeof Chat> = {
onMessage: () => () => {},
},
setView: () => {},
updateEditorStateOnChange: () => {},
} satisfies React.ComponentProps<typeof Chat>,

decorators: [VSCodeWebview],
Expand Down Expand Up @@ -76,3 +81,12 @@ export const EmptyWithNoPrompts: StoryObj<typeof meta> = {
}

export const Disabled: StoryObj<typeof meta> = { args: { chatEnabled: false } }

export const UnsentMessagePreservation: StoryObj<typeof meta> = {
args: {
transcript: [],
updateEditorStateOnChange: (index: number, state: SerializedPromptEditorValue) => {
console.log(`Editor state updated for message ${index}:`, state)
},
},
}
4 changes: 4 additions & 0 deletions vscode/webviews/Chat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
CodyIDE,
Guardrails,
PromptString,
SerializedPromptEditorValue,
} from '@sourcegraph/cody-shared'
import { Transcript, focusLastHumanMessageEditor } from './chat/Transcript'
import type { VSCodeWrapper } from './utils/VSCodeApi'
Expand All @@ -32,6 +33,7 @@ interface ChatboxProps {
showIDESnippetActions?: boolean
setView: (view: View) => void
smartApplyEnabled?: boolean
updateEditorStateOnChange: (index: number, state: SerializedPromptEditorValue) => void
}

export const Chat: React.FunctionComponent<React.PropsWithChildren<ChatboxProps>> = ({
Expand All @@ -45,6 +47,7 @@ export const Chat: React.FunctionComponent<React.PropsWithChildren<ChatboxProps>
showIDESnippetActions = true,
setView,
smartApplyEnabled,
updateEditorStateOnChange,
}) => {
const telemetryRecorder = useTelemetryRecorder()

Expand Down Expand Up @@ -224,6 +227,7 @@ export const Chat: React.FunctionComponent<React.PropsWithChildren<ChatboxProps>
postMessage={postMessage}
guardrails={guardrails}
smartApplyEnabled={smartApplyEnabled}
updateEditorStateOnChange={updateEditorStateOnChange}
/>
{transcript.length === 0 && showWelcomeMessage && (
<>
Expand Down
54 changes: 51 additions & 3 deletions vscode/webviews/CodyPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,20 @@
import { type AuthStatus, type ClientCapabilities, CodyIDE } from '@sourcegraph/cody-shared'
import {
type AuthStatus,
type ChatMessage,
type ClientCapabilities,
CodyIDE,
type SerializedPromptEditorValue,
} from '@sourcegraph/cody-shared'
import type React from 'react'
import { type ComponentProps, type FunctionComponent, useMemo, useRef } from 'react'
import {
type ComponentProps,
type FunctionComponent,
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'react'
import type { ConfigurationSubsetForWebview, LocalEnv } from '../src/chat/protocol'
import styles from './App.module.css'
import { Chat } from './Chat'
Expand Down Expand Up @@ -55,6 +69,39 @@ export const CodyPanel: FunctionComponent<
}) => {
const tabContainerRef = useRef<HTMLDivElement>(null)

const [activeTranscript, setActiveTranscript] = useState<ChatMessage[] | undefined>(transcript)
const [storedTranscriptState, setStoredTranscriptState] = useState(transcript)

// Update the Transcript State for each input box value change.
const updateEditorStateOnChange = useCallback(
(index: number, newEditorValue: SerializedPromptEditorValue) => {
setStoredTranscriptState(prev => {
const updated = [...prev]
updated[index] = {
...updated[index],
editorState: newEditorValue.editorState,
speaker: 'human',
}
return updated
})
},
[]
)

// Reset transcripts on new transcript change.
useEffect(() => {
setActiveTranscript(undefined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the most weak spot of this change, syncing state might be very fragile and unsafe.
I guess because of this we have UI flashes, I checked it manually and I was able to see some UI
delays (when you submit the message but don't see the loading UI below the input)

The main case with switching from the welcome chat screen to history and back to chat produces
a chat UI with no welcome UI

Screenshot 2024-10-09 at 15 48 23

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main case with switching from the welcome chat screen to history and back to chat produces
a chat UI with no welcome UI

It was the expected behavior to me because the user has already typed something in with the updated state, so transcript.length is no longer 0, which would not trigger the welcome message. I agree it's confusing though. will update it to change that.

setStoredTranscriptState(transcript)
}, [transcript])

// Set the current transcript to the transcript with the last stored editor states when switching to a different tab.
// This ensures the editor states are preserved when switching back to the chat tab.
useEffect(() => {
if (view !== View.Chat) {
setActiveTranscript(storedTranscriptState)
}
}, [view, storedTranscriptState])

return (
<TabViewContext.Provider value={useMemo(() => ({ view, setView }), [view, setView])}>
<TabRoot
Expand All @@ -76,9 +123,10 @@ export const CodyPanel: FunctionComponent<
<TabContainer value={view} ref={tabContainerRef}>
{view === View.Chat && (
<Chat
transcript={activeTranscript ?? transcript}
updateEditorStateOnChange={updateEditorStateOnChange}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the activeTranscript ?? transcript won't work when we want to preserve input text in the follow up message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are only saving the input text state but we don't want to use it until the user has switched to another tab.
This way we won't be re-rendering the transcript every time the state has changed, as we are still serving the same transcript up front if that makes sense?

Copy link
Contributor

@vovakulikov vovakulikov Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but do we want to preserve text in the following scenario?

  1. you open a new chat
  2. you type something in the textbox but don't submit it
  3. you switch to the history tab or prompts tab
  4. you switch back to the chat UI
  5. your text has been preserved!
  6. now you submit the message
  7. and you type something in the follow-up
  8. you switch to another tab again
  9. you switch back
  10. Does the follow-up textbox have preserved text from step 7.?

chatEnabled={chatEnabled}
messageInProgress={messageInProgress}
transcript={transcript}
vscodeAPI={vscodeAPI}
guardrails={attributionEnabled ? guardrails : undefined}
showIDESnippetActions={showIDESnippetActions}
Expand Down
1 change: 1 addition & 0 deletions vscode/webviews/chat/Transcript.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const meta: Meta<typeof Transcript> = {
userInfo: FIXTURE_USER_ACCOUNT_INFO,
postMessage: () => {},
chatEnabled: true,
updateEditorStateOnChange: () => {},
} satisfies ComponentProps<typeof Transcript>,

decorators: [
Expand Down
40 changes: 40 additions & 0 deletions vscode/webviews/chat/Transcript.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const PROPS: Omit<ComponentProps<typeof Transcript>, 'transcript'> = {
userInfo: FIXTURE_USER_ACCOUNT_INFO,
chatEnabled: true,
postMessage: () => {},
updateEditorStateOnChange: () => {},
}

vi.mock('@vscode/webview-ui-toolkit/react', () => ({
Expand Down Expand Up @@ -398,4 +399,43 @@ describe('transcriptToInteractionPairs', () => {
},
])
})

test('preserves editor state when switching tabs', async () => {
const waitForDebounce = () => new Promise(resolve => setTimeout(resolve, 350)) // 350ms to be safe as we debounce on change.

const humanMessage: ChatMessage = {
speaker: 'human',
text: ps`Foo`,
contextFiles: [],
}
const assistantMessage: ChatMessage = { speaker: 'assistant', text: ps`Bar` }
const updateEditorStateOnChange = vi.fn()
const { container, rerender } = render(
<Transcript
{...PROPS}
transcript={[humanMessage, assistantMessage]}
updateEditorStateOnChange={updateEditorStateOnChange}
/>
)

const editor = container.querySelector<EditorHTMLElement>(
'[role="row"]:last-child [data-lexical-editor="true"]'
)! as EditorHTMLElement
editor.textContent = 'Unsent message'

await typeInEditor(editor, 'Unsent message updated')

// Wait for the debounce
await waitForDebounce()

rerender(
<Transcript
{...PROPS}
transcript={[humanMessage, assistantMessage]}
updateEditorStateOnChange={updateEditorStateOnChange}
/>
)

expect(editor.textContent).toBe('Unsent message updated')
})
})
13 changes: 12 additions & 1 deletion vscode/webviews/chat/Transcript.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ interface TranscriptProps {
insertButtonOnSubmit?: CodeBlockActionsProps['insertButtonOnSubmit']
smartApply?: CodeBlockActionsProps['smartApply']
smartApplyEnabled?: boolean

updateEditorStateOnChange: (index: number, state: SerializedPromptEditorValue) => void
}

export const Transcript: FC<TranscriptProps> = props => {
Expand All @@ -57,6 +59,7 @@ export const Transcript: FC<TranscriptProps> = props => {
insertButtonOnSubmit,
smartApply,
smartApplyEnabled,
updateEditorStateOnChange,
} = props

const interactions = useMemo(
Expand Down Expand Up @@ -92,6 +95,7 @@ export const Transcript: FC<TranscriptProps> = props => {
)}
smartApply={smartApply}
smartApplyEnabled={smartApplyEnabled}
updateEditorStateOnChange={updateEditorStateOnChange}
/>
))}
</div>
Expand Down Expand Up @@ -160,6 +164,8 @@ interface TranscriptInteractionProps
isLastInteraction: boolean
isLastSentInteraction: boolean
priorAssistantMessageIsLoading: boolean

updateEditorStateOnChange: (index: number, state: SerializedPromptEditorValue) => void
}

const TranscriptInteraction: FC<TranscriptInteractionProps> = memo(props => {
Expand All @@ -178,6 +184,7 @@ const TranscriptInteraction: FC<TranscriptInteractionProps> = memo(props => {
copyButtonOnSubmit,
smartApply,
smartApplyEnabled,
updateEditorStateOnChange,
} = props

const [intentResults, setIntentResults] = useState<
Expand Down Expand Up @@ -221,6 +228,10 @@ const TranscriptInteraction: FC<TranscriptInteractionProps> = memo(props => {
return debounce(async (editorValue: SerializedPromptEditorValue) => {
setIntentResults(undefined)

if (editorValue.text) {
updateEditorStateOnChange(humanMessage.index, editorValue)
}

if (!experimentalOneBoxEnabled) {
return
}
Expand All @@ -240,7 +251,7 @@ const TranscriptInteraction: FC<TranscriptInteractionProps> = memo(props => {
})
}
}, 300)
}, [experimentalOneBoxEnabled, extensionAPI])
}, [humanMessage.index, experimentalOneBoxEnabled, extensionAPI, updateEditorStateOnChange])

const onStop = useCallback(() => {
getVSCodeAPI().postMessage({
Expand Down
Loading