Skip to content

Commit

Permalink
Backport 6833 to jb v7.12.x (#6836)
Browse files Browse the repository at this point in the history
backport: #6833


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.


<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->
  • Loading branch information
thenamankumar authored Jan 28, 2025
1 parent a97cbfa commit 3819091
Show file tree
Hide file tree
Showing 18 changed files with 439 additions and 685 deletions.

Large diffs are not rendered by default.

7 changes: 0 additions & 7 deletions lib/shared/src/misc/rpc/webviewAPI.ts
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
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: {},
codeSearchEnabled: true,
omniBoxEnabled: false,
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
84 changes: 29 additions & 55 deletions lib/shared/src/sourcegraph-api/clientConfig.ts
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,17 +58,15 @@ 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>

// Whether code search is enabled for the SG instance.
codeSearchEnabled: boolean

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

Expand All @@ -90,7 +83,6 @@ export const dummyClientConfigForTest: CodyClientConfig = {
modelsAPIEnabled: true,
userShouldUseEnterprise: false,
intentDetection: 'enabled',
temporarySettings: {},
notices: [],
codeSearchEnabled: false,
siteVersion: undefined,
Expand Down Expand Up @@ -118,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 @@ -237,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: omniBoxEnabled && (isError(codeSearchEnabled) || codeSearchEnabled),
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 @@ -269,8 +244,8 @@ export class ClientConfigSingleton {
)
}

if (!isError(temporarySettings)) {
config.temporarySettings = temporarySettings
if (codeSearchEnabled === false) {
config.intentDetection = 'disabled'
}

return config
Expand Down Expand Up @@ -303,7 +278,6 @@ export class ClientConfigSingleton {
modelsAPIEnabled: false,
userShouldUseEnterprise: false,
notices: [],
temporarySettings: {},
codeSearchEnabled: false,
omniBoxEnabled: false,
}
Expand Down
Loading

0 comments on commit 3819091

Please sign in to comment.