Skip to content

Commit

Permalink
Make Play Button stateful and remove intent toggle (#6833)
Browse files Browse the repository at this point in the history 
This PR:
- Removes the intent toggle feature completely. 
- Changes the intent dropdown behavior to only select the intent and not
execute it automatically.
- Enables the submit button even when the chat is empty.

## Test plan
- the intent options in the dropdown only selects the intent mode and
doesn't execute it.
- after selecting the intent, changing the input, doesn't reset the
intent.
- on submit the selected intent is used and the intent selection message
is displayed accordingly.
- loading a chat from history and editing the message, doesn't persist
the intent mode previously manually selected.
- selecting a prompt correctly executes it in chat & edit modes. 
- make sure the submit button isn't disabled when the input is empty.
  • Loading branch information
@thenamankumar
thenamankumar authored Jan 28, 2025
1 parent 0e4c3e8 commit 2edd264
Show file tree
Hide file tree
Showing 18 changed files with 340 additions and 688 deletions.
451 changes: 183 additions & 268 deletions ...s/src/integrationTest/resources/recordings/integration-test_2927926756/recording.har.yaml
View file

Large diffs are not rendered by default.

7 changes: 0 additions & 7 deletions lib/shared/src/misc/rpc/webviewAPI.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import type {
FetchHighlightFileParameters,
Prompt,
PromptTag,
TemporarySettings,
} from '../../sourcegraph-api/graphql/client'
import { type createMessageAPIForWebview, proxyExtensionAPI } from './rpc'

Expand Down Expand Up @@ -110,11 +109,6 @@ export interface WebviewToExtensionAPI {
* The current user's product subscription information (Cody Free/Pro).
*/
userProductSubscription(): Observable<UserProductSubscription | null>

/**
* Edit the current user's temporary settings.
*/
editTemporarySettings(settingsToEdit: Partial<TemporarySettings>): Observable<boolean>
}

export function createExtensionAPI(
Expand Down Expand Up @@ -150,7 +144,6 @@ export function createExtensionAPI(
userHistory: proxyExtensionAPI(messageAPI, 'userHistory'),
userProductSubscription: proxyExtensionAPI(messageAPI, 'userProductSubscription'),
repos: proxyExtensionAPI(messageAPI, 'repos'),
editTemporarySettings: proxyExtensionAPI(messageAPI, 'editTemporarySettings'),
}
}

Expand Down
19 changes: 0 additions & 19 deletions lib/shared/src/sourcegraph-api/clientConfig.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ const CLIENT_CONFIG_FIXTURE: CodyClientConfig = {
userShouldUseEnterprise: false,
intentDetection: 'enabled',
notices: [],
temporarySettings: {},
omniBoxEnabled: false,
codeSearchEnabled: true,
siteVersion: '5.5.0',
Expand All @@ -38,7 +37,6 @@ describe('ClientConfigSingleton', () => {
mockAuthStatus(authStatusSubject)
const getSiteVersionMock = vi.spyOn(graphqlClient, 'getSiteVersion').mockResolvedValue('5.5.0')
const viewerSettingsMock = vi.spyOn(graphqlClient, 'viewerSettings').mockResolvedValue({})
const temporarySettingsMock = vi.spyOn(graphqlClient, 'temporarySettings').mockResolvedValue({})
const codeSearchEnabledMock = vi
.spyOn(graphqlClient, 'codeSearchEnabled')
.mockResolvedValue(true)
Expand All @@ -58,15 +56,13 @@ describe('ClientConfigSingleton', () => {
await vi.advanceTimersByTimeAsync(0)
expect(getSiteVersionMock).toHaveBeenCalledTimes(1)
expect(viewerSettingsMock).toHaveBeenCalledTimes(1)
expect(temporarySettingsMock).toHaveBeenCalledTimes(1)
expect(codeSearchEnabledMock).toHaveBeenCalledTimes(1)
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
expect(await clientConfigSingleton.getConfig()).toEqual(CLIENT_CONFIG_FIXTURE)
expect(getSiteVersionMock).toHaveBeenCalledTimes(1)
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()
})
Expand All @@ -77,7 +73,6 @@ describe('ClientConfigSingleton', () => {
mockAuthStatus(authStatusSubject)
const getSiteVersionMock = vi.spyOn(graphqlClient, 'getSiteVersion').mockResolvedValue('5.5.0')
const viewerSettingsMock = vi.spyOn(graphqlClient, 'viewerSettings').mockResolvedValue({})
const temporarySettingsMock = vi.spyOn(graphqlClient, 'temporarySettings').mockResolvedValue({})
const codeSearchEnabledMock = vi
.spyOn(graphqlClient, 'codeSearchEnabled')
.mockResolvedValue(true)
Expand All @@ -96,12 +91,10 @@ describe('ClientConfigSingleton', () => {
await vi.advanceTimersByTimeAsync(0)
expect(getSiteVersionMock).toHaveBeenCalledTimes(1)
expect(viewerSettingsMock).toHaveBeenCalledTimes(1)
expect(temporarySettingsMock).toHaveBeenCalledTimes(1)
expect(codeSearchEnabledMock).toHaveBeenCalledTimes(1)
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()

Expand Down Expand Up @@ -139,7 +132,6 @@ describe('ClientConfigSingleton', () => {
.mockImplementation(() => new Promise<string>(resolve => setTimeout(resolve, 100, '5.5.0')))

const viewerSettingsMock = vi.spyOn(graphqlClient, 'viewerSettings').mockResolvedValue({})
const temporarySettingsMock = vi.spyOn(graphqlClient, 'temporarySettings').mockResolvedValue({})
const codeSearchEnabledMock = vi
.spyOn(graphqlClient, 'codeSearchEnabled')
.mockResolvedValue(true)
Expand Down Expand Up @@ -194,7 +186,6 @@ describe('ClientConfigSingleton', () => {
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()
})
Expand All @@ -207,7 +198,6 @@ describe('ClientConfigSingleton', () => {
.spyOn(graphqlClient, 'getSiteVersion')
.mockImplementation(() => new Promise<string>(resolve => setTimeout(resolve, 100, '5.5.0')))
const viewerSettingsMock = vi.spyOn(graphqlClient, 'viewerSettings').mockResolvedValue({})
const temporarySettingsMock = vi.spyOn(graphqlClient, 'temporarySettings').mockResolvedValue({})
const codeSearchEnabledMock = vi
.spyOn(graphqlClient, 'codeSearchEnabled')
.mockResolvedValue(true)
Expand All @@ -228,7 +218,6 @@ describe('ClientConfigSingleton', () => {
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()

Expand All @@ -249,20 +238,17 @@ describe('ClientConfigSingleton', () => {
await vi.advanceTimersByTimeAsync(ClientConfigSingleton.REFETCH_INTERVAL + 1)
expect(getSiteVersionMock).toHaveBeenCalledTimes(1)
expect(viewerSettingsMock).toHaveBeenCalledTimes(0)
expect(temporarySettingsMock).toHaveBeenCalledTimes(0)
expect(codeSearchEnabledMock).toHaveBeenCalledTimes(0)
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()

// A stale cached value will still be returned.
expect(await clientConfigSingleton.getConfig()).toEqual(CLIENT_CONFIG_FIXTURE)
expect(getSiteVersionMock).toHaveBeenCalledTimes(0)
expect(viewerSettingsMock).toHaveBeenCalledTimes(0)
expect(temporarySettingsMock).toHaveBeenCalledTimes(0)
expect(codeSearchEnabledMock).toHaveBeenCalledTimes(0)
expect(fetchHTTPMock).toHaveBeenCalledTimes(0)

Expand All @@ -273,7 +259,6 @@ describe('ClientConfigSingleton', () => {
expect(fetchHTTPMock).toHaveBeenCalledTimes(0)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()
})
Expand All @@ -286,7 +271,6 @@ describe('ClientConfigSingleton', () => {
.spyOn(graphqlClient, 'getSiteVersion')
.mockImplementation(() => new Promise<string>(resolve => setTimeout(resolve, 100, '5.5.0')))
const viewerSettingsMock = vi.spyOn(graphqlClient, 'viewerSettings').mockResolvedValue({})
const temporarySettingsMock = vi.spyOn(graphqlClient, 'temporarySettings').mockResolvedValue({})
const codeSearchEnabledMock = vi
.spyOn(graphqlClient, 'codeSearchEnabled')
.mockResolvedValue(true)
Expand All @@ -305,12 +289,10 @@ describe('ClientConfigSingleton', () => {
await vi.advanceTimersByTimeAsync(100)
expect(getSiteVersionMock).toHaveBeenCalledTimes(1)
expect(viewerSettingsMock).toHaveBeenCalledTimes(1)
expect(temporarySettingsMock).toHaveBeenCalledTimes(1)
expect(codeSearchEnabledMock).toHaveBeenCalledTimes(1)
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()

Expand Down Expand Up @@ -338,7 +320,6 @@ describe('ClientConfigSingleton', () => {
expect(fetchHTTPMock).toHaveBeenCalledTimes(1)
getSiteVersionMock.mockClear()
viewerSettingsMock.mockClear()
temporarySettingsMock.mockClear()
codeSearchEnabledMock.mockClear()
fetchHTTPMock.mockClear()
})
Expand Down
83 changes: 26 additions & 57 deletions lib/shared/src/sourcegraph-api/clientConfig.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Observable, Subject, interval, map, merge } from 'observable-fns'
import { Observable, interval, map } from 'observable-fns'
import semver from 'semver'
import type { AuthStatus } from '..'
import { authStatus } from '../auth/authStatus'
Expand All @@ -20,12 +20,7 @@ import {
} from '../misc/observableOperation'
import { isError } from '../utils'
import { isAbortError } from './errors'
import {
type CodyConfigFeatures,
type GraphQLAPIClientConfig,
type TemporarySettings,
graphqlClient,
} from './graphql/client'
import { type CodyConfigFeatures, type GraphQLAPIClientConfig, graphqlClient } from './graphql/client'

export interface CodyNotice {
key: string
Expand Down Expand Up @@ -63,15 +58,12 @@ export interface CodyClientConfig {
userShouldUseEnterprise: boolean

// Whether the user should be able to use the intent detection feature.
// `opt-in` means the user must explicitly enable it.
intentDetection: 'disabled' | 'enabled' | 'opt-in'
intentDetection: 'disabled' | 'enabled'

// List of global instance-level cody notice/banners (set only by admins in global
// instance configuration file
notices: CodyNotice[]

temporarySettings: Partial<TemporarySettings>

// The version of the Sourcegraph instance.
siteVersion?: string

Expand All @@ -91,7 +83,6 @@ export const dummyClientConfigForTest: CodyClientConfig = {
modelsAPIEnabled: true,
userShouldUseEnterprise: false,
intentDetection: 'enabled',
temporarySettings: {},
notices: [],
siteVersion: undefined,
omniBoxEnabled: false,
Expand Down Expand Up @@ -119,47 +110,32 @@ export class ClientConfigSingleton {
attribution: false,
}

private readonly forceUpdateSubject = new Subject<any>()

/**
* Forces an immediate update of the client configuration by triggering a new fetch.
* This method is called when temporary settings are edited from the client to ensure
* the configuration is immediately synchronized with the latest changes.
*
* @returns A promise that resolves to the updated CodyClientConfig or undefined
*/
public async forceUpdate(): Promise<CodyClientConfig | undefined> {
this.forceUpdateSubject.next(true)
return firstValueFrom(this.changes.pipe(skipPendingOperation()))
}
/**
* An observable that immediately emits the last-cached value (or fetches it if needed) and then
* emits changes.
*/
public readonly changes: Observable<CodyClientConfig | undefined | typeof pendingOperation> = merge(
authStatus,
this.forceUpdateSubject
).pipe(
debounceTime(0), // wait a tick for graphqlClient's auth to be updated
switchMapReplayOperation(authStatus =>
authStatus.authenticated
? interval(ClientConfigSingleton.REFETCH_INTERVAL).pipe(
map(() => undefined),
// Don't update if the editor is in the background, to avoid network
// activity that can cause OS warnings or authorization flows when the
// user is not using Cody. See
// linear.app/sourcegraph/issue/CODY-3745/codys-background-periodic-network-access-causes-2fa.
filter((_value): _value is undefined => editorWindowIsFocused()),
startWith(undefined),
switchMap(() =>
promiseFactoryToObservable(signal => this.fetchConfig(authStatus, signal))
public readonly changes: Observable<CodyClientConfig | undefined | typeof pendingOperation> =
authStatus.pipe(
debounceTime(0), // wait a tick for graphqlClient's auth to be updated
switchMapReplayOperation(authStatus =>
authStatus.authenticated
? interval(ClientConfigSingleton.REFETCH_INTERVAL).pipe(
map(() => undefined),
// Don't update if the editor is in the background, to avoid network
// activity that can cause OS warnings or authorization flows when the
// user is not using Cody. See
// linear.app/sourcegraph/issue/CODY-3745/codys-background-periodic-network-access-causes-2fa.
filter((_value): _value is undefined => editorWindowIsFocused()),
startWith(undefined),
switchMap(() =>
promiseFactoryToObservable(signal => this.fetchConfig(authStatus, signal))
)
)
)
: Observable.of(undefined)
),
map(value => (isError(value) ? undefined : value)),
distinctUntilChanged()
)
: Observable.of(undefined)
),
map(value => (isError(value) ? undefined : value)),
distinctUntilChanged()
)

public readonly updates: Observable<CodyClientConfig> = this.changes.pipe(
filter(value => value !== undefined && value !== pendingOperation),
Expand Down Expand Up @@ -238,22 +214,20 @@ export class ClientConfigSingleton {

return Promise.all([
graphqlClient.viewerSettings(signal),
graphqlClient.temporarySettings(signal),
graphqlClient.codeSearchEnabled(signal),
]).then(([viewerSettings, temporarySettings, codeSearchEnabled]) => {
]).then(([viewerSettings, codeSearchEnabled]) => {
const config: CodyClientConfig = {
...clientConfig,
intentDetection: 'enabled',
notices: [],
temporarySettings: {},
siteVersion: isError(siteVersion) ? undefined : siteVersion,
omniBoxEnabled,
codeSearchEnabled: isError(codeSearchEnabled) ? true : codeSearchEnabled,
}

// Don't fail the whole chat because of viewer setting (used only to show banners)
if (!isError(viewerSettings)) {
config.intentDetection = ['disabled', 'enabled', 'opt-in'].includes(
config.intentDetection = ['disabled', 'enabled'].includes(
viewerSettings['omnibox.intentDetection']
)
? viewerSettings['omnibox.intentDetection']
Expand All @@ -274,10 +248,6 @@ export class ClientConfigSingleton {
config.intentDetection = 'disabled'
}

if (!isError(temporarySettings)) {
config.temporarySettings = temporarySettings
}

return config
})
} catch (e) {
Expand Down Expand Up @@ -308,7 +278,6 @@ export class ClientConfigSingleton {
modelsAPIEnabled: false,
userShouldUseEnterprise: false,
notices: [],
temporarySettings: {},
omniBoxEnabled: false,
codeSearchEnabled: false,
}
Expand Down
42 changes: 0 additions & 42 deletions lib/shared/src/sourcegraph-api/graphql/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import {
CURRENT_USER_INFO_QUERY,
CURRENT_USER_ROLE_QUERY,
DELETE_ACCESS_TOKEN_MUTATION,
EDIT_TEMPORARY_SETTINGS_QUERY,
EVALUATE_FEATURE_FLAG_QUERY,
FILE_CONTENTS_QUERY,
FILE_MATCH_SEARCH_QUERY,
Expand All @@ -66,7 +65,6 @@ import {
REPOS_SUGGESTIONS_QUERY,
REPO_NAME_QUERY,
SEARCH_ATTRIBUTION_QUERY,
TEMPORARY_SETTINGS_QUERY,
VIEWER_SETTINGS_QUERY,
} from './queries'
import { buildGraphQLUrl } from './url'
Expand Down Expand Up @@ -629,18 +627,6 @@ interface CodeSearchEnabledResponse {
codeSearchEnabled: boolean
}

interface TemporarySettingsResponse {
temporarySettings: { contents: string }
}

export interface TemporarySettings {
'omnibox.intentDetectionToggleOn': boolean
}

export interface EditTemporarySettingsResponse {
editTemporarySettings: { alwaysNil: string }
}

function extractDataOrError<T, R>(response: APIResponse<T> | Error, extract: (data: T) => R): R | Error {
if (isError(response)) {
return response
Expand Down Expand Up @@ -1574,34 +1560,6 @@ export class SourcegraphGraphQLAPIClient {
return extractDataOrError(response, data => data.codeSearchEnabled)
}

public async temporarySettings(signal?: AbortSignal): Promise<Partial<TemporarySettings> | Error> {
const response = await this.fetchSourcegraphAPI<APIResponse<TemporarySettingsResponse>>(
TEMPORARY_SETTINGS_QUERY,
{},
signal
)
return extractDataOrError(response, data => {
try {
return JSON.parse(data.temporarySettings.contents)
} catch {
return {}
}
})
}

public async editTemporarySettings(
settingsToEdit: Partial<TemporarySettings>,
signal?: AbortSignal
): Promise<{ alwaysNil: string } | Error> {
const response = await this.fetchSourcegraphAPI<APIResponse<EditTemporarySettingsResponse>>(
EDIT_TEMPORARY_SETTINGS_QUERY,
{ settingsToEdit: JSON.stringify(settingsToEdit) },
signal
)

return extractDataOrError(response, data => data.editTemporarySettings)
}

public async fetchSourcegraphAPI<T>(
query: string,
variables: Record<string, any> = {},
Expand Down
Loading

0 comments on commit 2edd264

Please sign in to comment.