Skip to content

Commit

Permalink
fix: Make it possible to dogfood new prompt editor (#7094)
Browse files Browse the repository at this point in the history
Closes
https://linear.app/sourcegraph/issue/SRCH-1667/no-results-returned-when-enabling-feature-for-queries

This PR fixes an issue with the prompt input being cleared out when a
query is submitted and the new input is enabled.

## Root cause

The root cause for this issue is twofold:

1. Evaluating feature flags is always asynchronous. The first time
`HumanMessageEditor` is rendered we will render the
   old editor.
1. In the same component, `useImperativeHandle` is only run once when
the component is mounted. This means we never
update the ref with the ref from the new editor once the feature flag
updates.

The consequence of this is that everywhere else where `editorRef` is
used we have a reference to the _old_ editor API,
which has been destroyed since. Reading the value from it will of course
be empty, which in turn causes the input value
of the rendered editor to be overwritten.

## Solution

It seems obvious to remove the empty dependency list from the
`useImperativeHandle` call to make that the ref is always
updated. We do this in other places too (`Transcript.tsx`).

However, Vova pointed out that there can still be race conditions
between accessing `humanEditoRef` and the feature flag
loading and suggested that we should retrieve the feature flag value
before we render any UI.
I've extended the object returned by `getConfigForWebview` but I'm not
sure whether that's the right place to put it.
I've also heard that we want to make as few network requests as possible
during initialization, which is understandable.

> [!IMPORTANT]
> I'd like some guidance from the cody team on where put this setting to
ensure avoid the aforementioned problem.

## Test plan

1. Enabled the feature flag for myself on S2.
1. Ran the standalone web demo (`cd client/web; pnpm dev`)
1. Verified that the new input used (typing `@...` looks slightly
different)
1. Submitting a query does not clear the input and generates an
appropriate response.
  • Loading branch information
fkling authored Feb 15, 2025
1 parent 5888c21 commit d80689c
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 16 deletions.
11 changes: 2 additions & 9 deletions lib/prompt-editor/src/v2/promptInput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,18 +183,11 @@ const prosemirrorActor = fromCallback<ProseMirrorMachineEvent, ProseMirrorMachin
}
})

function doFocus() {
editor.focus()
editor.dispatch(editor.state.tr.scrollIntoView())
}

receive(event => {
switch (event.type) {
case 'focus':
doFocus()
// HACK(sqs): Needed in VS Code webviews to actually get it to focus
// on initial load, for some reason.
setTimeout(doFocus)
editor.focus()
editor.dispatch(editor.state.tr.scrollIntoView())
break
case 'blur':
editor.dom.blur()
Expand Down
6 changes: 6 additions & 0 deletions vscode/src/chat/chat-view/ChatController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
DOTCOM_URL,
type DefaultChatCommands,
type EventSource,
FeatureFlag,
type Guardrails,
ModelUsage,
type NLSSearchDynamicFilter,
Expand All @@ -36,6 +37,7 @@ import {
extractContextFromTraceparent,
featureFlagProvider,
firstResultFromOperation,
firstValueFrom,
forceHydration,
graphqlClient,
hydrateAfterPostMessage,
Expand Down Expand Up @@ -528,6 +530,9 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv

private async getConfigForWebview(): Promise<ConfigurationSubsetForWebview & LocalEnv> {
const { configuration, auth } = await currentResolvedConfig()
const experimentalPromptEditorEnabled = await firstValueFrom(
featureFlagProvider.evaluatedFeatureFlag(FeatureFlag.CodyExperimentalPromptEditor)
)
const sidebarViewOnly = this.extensionClient.capabilities?.webviewNativeConfig?.view === 'single'
const isEditorViewType = this.webviewPanelOrView?.viewType === 'cody.editorPanel'
const webviewType = isEditorViewType && !sidebarViewOnly ? 'editor' : 'sidebar'
Expand All @@ -545,6 +550,7 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
multipleWebviewsEnabled: !sidebarViewOnly,
internalDebugContext: configuration.internalDebugContext,
allowEndpointChange: configuration.overrideServerEndpoint === undefined,
experimentalPromptEditorEnabled,
}
}

Expand Down
1 change: 1 addition & 0 deletions vscode/src/chat/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ export interface ConfigurationSubsetForWebview
multipleWebviewsEnabled?: boolean | undefined | null
endpointHistory?: string[] | undefined | null
allowEndpointChange: boolean
experimentalPromptEditorEnabled: boolean
}

/**
Expand Down
1 change: 1 addition & 0 deletions vscode/webviews/App.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const dummyVSCodeAPI: VSCodeWrapper = {
smartApply: false,
hasEditCapability: false,
allowEndpointChange: true,
experimentalPromptEditorEnabled: false,
},
clientCapabilities: CLIENT_CAPABILITIES_FIXTURE,
authStatus: {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
type ChatMessage,
FAST_CHAT_INPUT_TOKEN_BUDGET,
FeatureFlag,
type Model,
ModelTag,
type SerializedPromptEditorState,
Expand Down Expand Up @@ -33,7 +32,6 @@ import { type ClientActionListener, useClientActionListener } from '../../../../
import { promptModeToIntent } from '../../../../../prompts/PromptsTab'
import { useTelemetryRecorder } from '../../../../../utils/telemetry'
import { useConfig } from '../../../../../utils/useConfig'
import { useFeatureFlag } from '../../../../../utils/useFeatureFlags'
import { useLinkOpener } from '../../../../../utils/useLinkOpener'
import { useOmniBox } from '../../../../../utils/useOmniBox'
import styles from './HumanMessageEditor.module.css'
Expand Down Expand Up @@ -102,7 +100,7 @@ export const HumanMessageEditor: FunctionComponent<{
const telemetryRecorder = useTelemetryRecorder()

const editorRef = useRef<PromptEditorRefAPI>(null)
useImperativeHandle(parentEditorRef, (): PromptEditorRefAPI | null => editorRef.current, [])
useImperativeHandle(parentEditorRef, (): PromptEditorRefAPI | null => editorRef.current)

// The only PromptEditor state we really need to track in our own state is whether it's empty.
const [isEmptyEditorValue, setIsEmptyEditorValue] = useState(
Expand All @@ -124,9 +122,6 @@ export const HumanMessageEditor: FunctionComponent<{
? 'emptyEditorValue'
: 'submittable'

// TODO: Finish implementing "current repo not indexed" handling for v2 editor
const experimentalPromptEditorEnabled = useFeatureFlag(FeatureFlag.CodyExperimentalPromptEditor)

const onSubmitClick = useCallback(
(intent?: ChatMessage['intent'], forceSubmit?: boolean): void => {
if (!forceSubmit && submitState === 'emptyEditorValue') {
Expand Down Expand Up @@ -163,7 +158,10 @@ export const HumanMessageEditor: FunctionComponent<{
)

const omniBoxEnabled = useOmniBox()
const { isDotComUser } = useConfig()
const {
isDotComUser,
config: { experimentalPromptEditorEnabled },
} = useConfig()

const onEditorEnterKey = useCallback(
(event: KeyboardEvent | null): void => {
Expand Down Expand Up @@ -425,6 +423,7 @@ export const HumanMessageEditor: FunctionComponent<{
[linkOpener]
)

// TODO: Finish implementing "current repo not indexed" handling for v2 editor
const Editor = experimentalPromptEditorEnabled ? PromptEditorV2 : PromptEditor

return (
Expand Down

0 comments on commit d80689c

Please sign in to comment.