diff --git a/lib/shared/src/misc/observable.ts b/lib/shared/src/misc/observable.ts index 3c99022e95ae..2a64e58b24a5 100644 --- a/lib/shared/src/misc/observable.ts +++ b/lib/shared/src/misc/observable.ts @@ -562,10 +562,31 @@ export function pick( // biome-ignore lint/suspicious/noConfusingVoidType: export type UnsubscribableLike = Unsubscribable | (() => void) | void | null +export type ShareReplayConfig = { + /** + * When `shouldCountRefs` is true (default), the source observable will be unsubscribed when the number of + * subscribers (reference count) drops to zero. This means that when there are no active subscribers, + * the observable will stop emitting values and release resources, and future subscribers will trigger + * a new subscription to the source observable. + * + * If `shouldCountRefs` is false (the default behavior in RxJS), the source observable will remain + * active even when there are no subscribers. This is useful when you want to keep an expensive + * or long-running observable alive, avoiding the need for a costly re-subscription when new + * subscribers join later. + * + * See more context and examples at: https://rxjs.dev/api/operators/shareReplay + * It has a similar but _not_ identical implementation. + */ + shouldCountRefs: boolean +} + let shareReplaySeq = 0 -export function shareReplay(): (observable: ObservableLike) => Observable { +export function shareReplay( + config?: ShareReplayConfig +): (observable: ObservableLike) => Observable { // NOTE(sqs): This is helpful for debugging why shareReplay does not have a buffered value. const shouldLog = false + const shouldCountRefs = config?.shouldCountRefs ?? true const logID = shareReplaySeq++ function logDebug(msg: string, ...args: any[]): void { if (shouldLog) console.debug(`shareReplay#${logID}:`, msg, ...args) @@ -601,7 +622,7 @@ export function shareReplay(): (observable: ObservableLike) => Observable< return () => { refCount-- innerSub.unsubscribe() - if (refCount === 0) { + if (shouldCountRefs && refCount === 0) { if (subscription) { unsubscribe(subscription) subscription = null diff --git a/lib/shared/src/misc/observableOperation.ts b/lib/shared/src/misc/observableOperation.ts index 9961a3d2586f..9b6251d62095 100644 --- a/lib/shared/src/misc/observableOperation.ts +++ b/lib/shared/src/misc/observableOperation.ts @@ -1,10 +1,21 @@ import { Observable, type ObservableLike } from 'observable-fns' import { isError } from '../utils' -import { catchError, filter, firstValueFrom, shareReplay, startWith, switchMap } from './observable' +import { + type ShareReplayConfig, + catchError, + filter, + firstValueFrom, + shareReplay, + startWith, + switchMap, +} from './observable' /** A sentinel value to indicate that the value has not yet been emitted. */ export const pendingOperation = Symbol.for('@@pendingOperation') +export type MaybePendingObservable = Observable +export type MaybePendingObservableLike = ObservableLike + /** * Run an operation with the outer observable as input. The result is replayed to all subscribers * (including future subscribers) using {@link shareReplay}, but it is immediately invalidated when @@ -15,13 +26,10 @@ export const pendingOperation = Symbol.for('@@pendingOperation') * operation completes with the new input. */ export function switchMapReplayOperation( - operation: (value: T) => Observable -): ( - source: ObservableLike -) => Observable { - return ( - source: ObservableLike - ): Observable => { + operation: (value: T) => Observable, + shareReplayConfig?: ShareReplayConfig +): (source: MaybePendingObservableLike) => MaybePendingObservable { + return (source: MaybePendingObservableLike): MaybePendingObservable => { return Observable.from(source).pipe( switchMap(outerValue => outerValue === pendingOperation @@ -33,7 +41,7 @@ export function switchMapReplayOperation( startWith(pendingOperation) ) ), - shareReplay() + shareReplay(shareReplayConfig) ) } } @@ -41,10 +49,8 @@ export function switchMapReplayOperation( /** * An observable pipe operator to filter out {@link pendingOperation} values. */ -export function skipPendingOperation(): ( - source: ObservableLike -) => Observable { - return (source: ObservableLike) => +export function skipPendingOperation(): (source: MaybePendingObservableLike) => Observable { + return (source: MaybePendingObservableLike) => Observable.from(source).pipe(filter((value): value is T => value !== pendingOperation)) } diff --git a/vscode/CHANGELOG.md b/vscode/CHANGELOG.md index 35503c7f3d08..e50fa4a06d2b 100644 --- a/vscode/CHANGELOG.md +++ b/vscode/CHANGELOG.md @@ -10,15 +10,19 @@ This is a log of all notable changes to Cody for VS Code. [Unreleased] changes a ### Changed +### Fixed + +- Context Filters: fixed repo name resolution cache. [pull/5978](https://github.com/sourcegraph/cody/pull/5978) + ## 1.38.2 ### Changed - Telemetry: Account for visible ranges in the characters logger. [pull/5931](https://github.com/sourcegraph/cody/pull/5931) -### Fixed -- Chat: Improved handling of duplicated priority context items. [pull/5860](https://github.com/sourcegraph/cody/pull/5860) +### Fixed +- Chat: Improved handling of duplicated priority context items. [pull/5860](https://github.com/sourcegraph/cody/pull/5860) - Chat: Improved handling of duplicated priority context items. [pull/5860](https://github.com/sourcegraph/cody/pull/5860) ### Changed diff --git a/vscode/src/repository/remote-urls-from-parent-dirs.test.ts b/vscode/src/repository/remote-urls-from-parent-dirs.test.ts index e861e655e666..a76382ea5b69 100644 --- a/vscode/src/repository/remote-urls-from-parent-dirs.test.ts +++ b/vscode/src/repository/remote-urls-from-parent-dirs.test.ts @@ -75,13 +75,13 @@ describe('gitRemoteUrlsForUri', () => { const remoteUrls = await gitRemoteUrlsForUri(fileUri) expect(remoteUrls).toEqual([ - 'https://github.com/username/yourproject.git', - 'https://github.com/originalauthor/yourproject.git', 'git@backupserver:repositories/yourproject.git', + 'https://github.com/originalauthor/yourproject.git', + 'https://github.com/username/yourproject.git', ]) }) - it('returns `undefined` from the .git/config file with no remotes specified', async () => { + it('returns `[]` from the .git/config file with no remotes specified', async () => { const { fileUri } = mockFsCalls({ filePath: '/repo/src/dir/foo.ts', gitRepoPath: '/repo', @@ -99,10 +99,10 @@ describe('gitRemoteUrlsForUri', () => { }) const remoteUrls = await gitRemoteUrlsForUri(fileUri) - expect(remoteUrls).toBe(undefined) + expect(remoteUrls).toEqual([]) }) - it('returns `undefined` if .git/config is not found', async () => { + it('returns `[]` if .git/config is not found', async () => { const statMock = vi .spyOn(vscode.workspace.fs, 'stat') .mockResolvedValueOnce({ type: vscode.FileType.File } as vscode.FileStat) @@ -112,7 +112,7 @@ describe('gitRemoteUrlsForUri', () => { const remoteUrls = await gitRemoteUrlsForUri(uri) expect(statMock).toBeCalledTimes(5) - expect(remoteUrls).toBe(undefined) + expect(remoteUrls).toEqual([]) }) it('finds remote urls in a submodule', async () => { @@ -175,7 +175,7 @@ describe('gitRemoteUrlsForUri', () => { expect(remoteUrls).toEqual(['https://github.com/nested/submodule.git']) }) - it('returns `undefined` for a submodule without a remote url', async () => { + it('returns `[]` for a submodule without a remote url', async () => { const { fileUri } = mockFsCalls({ filePath: '/repo/submodule/foo.ts', gitRepoPath: '/repo', @@ -199,7 +199,7 @@ describe('gitRemoteUrlsForUri', () => { }) const remoteUrls = await gitRemoteUrlsForUri(fileUri) - expect(remoteUrls).toBe(undefined) + expect(remoteUrls).toEqual([]) }) it('refuses to work on non-file URIs', async () => { @@ -209,10 +209,10 @@ describe('gitRemoteUrlsForUri', () => { gitConfig: 'a', }) - expect(await gitRemoteUrlsForUri(URI.parse('https://example.com/foo/bar'))).toBe(undefined) - expect(await gitRemoteUrlsForUri(URI.parse('https://gitlab.com/foo/bar'))).toBe(undefined) - expect(await gitRemoteUrlsForUri(URI.parse('https://github.com/foo/bar'))).toBe(undefined) - expect(await gitRemoteUrlsForUri(URI.parse('ssh://git@github.com:foo/bar.git'))).toBe(undefined) + expect(await gitRemoteUrlsForUri(URI.parse('https://example.com/foo/bar'))).toEqual([]) + expect(await gitRemoteUrlsForUri(URI.parse('https://gitlab.com/foo/bar'))).toEqual([]) + expect(await gitRemoteUrlsForUri(URI.parse('https://github.com/foo/bar'))).toEqual([]) + expect(await gitRemoteUrlsForUri(URI.parse('ssh://git@github.com:foo/bar.git'))).toEqual([]) expect(statMock).toBeCalledTimes(0) expect(readFileMock).toBeCalledTimes(0) }) diff --git a/vscode/src/repository/remote-urls-from-parent-dirs.ts b/vscode/src/repository/remote-urls-from-parent-dirs.ts index be57c46ba870..40dcd8004341 100644 --- a/vscode/src/repository/remote-urls-from-parent-dirs.ts +++ b/vscode/src/repository/remote-urls-from-parent-dirs.ts @@ -13,15 +13,20 @@ const textDecoder = new TextDecoder('utf-8') * This function tries 2 different ways to get the remote URLs: (1) by using the Git extension API * available in VS Code only, and (2) by crawling the file system for the `.git/config` file. */ -export async function gitRemoteUrlsForUri( - uri: vscode.Uri, - signal?: AbortSignal -): Promise { - const fromGitExtension = gitRemoteUrlsFromGitExtension(uri) - if (fromGitExtension && fromGitExtension.length > 0) { - return fromGitExtension +export async function gitRemoteUrlsForUri(uri: vscode.Uri, signal?: AbortSignal): Promise { + let remoteUrls = gitRemoteUrlsFromGitExtension(uri) + + // If not results from the Git extension API, try crawling the file system. + if (!remoteUrls || remoteUrls.length === 0) { + remoteUrls = await gitRemoteUrlsFromParentDirs(uri, signal) } - return await gitRemoteUrlsFromParentDirs(uri, signal) + + // Ensure the remote URLs are sorted and unique no matter what source we used. + if (remoteUrls && remoteUrls.length > 0) { + return Array.from(new Set(remoteUrls)).sort() + } + + return [] } /** diff --git a/vscode/src/repository/repo-name-resolver.test.ts b/vscode/src/repository/repo-name-resolver.test.ts index 9249b92a9ac2..1b8f8e953af8 100644 --- a/vscode/src/repository/repo-name-resolver.test.ts +++ b/vscode/src/repository/repo-name-resolver.test.ts @@ -18,7 +18,7 @@ import { mockFsCalls } from './test-helpers' vi.mock('../services/AuthProvider') describe('getRepoNamesContainingUri', () => { - it('resolves the repo name using graphql for enterprise accounts', async () => { + function prepareEnterpriseMocks() { const repoNameResolver = new RepoNameResolver() mockAuthStatus(AUTH_STATUS_FIXTURE_AUTHED) mockResolvedConfig({ auth: {} }) @@ -45,9 +45,34 @@ describe('getRepoNamesContainingUri', () => { .spyOn(graphqlClient, 'getRepoName') .mockResolvedValue('sourcegraph/cody') - expect( - await firstResultFromOperation(repoNameResolver.getRepoNamesContainingUri(fileUri)) - ).toEqual(['sourcegraph/cody']) + return { + repoNameResolver, + fileUri, + getRepoNameGraphQLMock, + } + } + it('resolves the repo name using graphql for enterprise accounts', async () => { + const { repoNameResolver, fileUri, getRepoNameGraphQLMock } = prepareEnterpriseMocks() + const repoNames = await firstResultFromOperation( + repoNameResolver.getRepoNamesContainingUri(fileUri) + ) + + expect(repoNames).toEqual(['sourcegraph/cody']) + expect(getRepoNameGraphQLMock).toBeCalledTimes(1) + }) + + it('reuses cached API responses that are needed to resolve enterprise repo names', async () => { + const { repoNameResolver, fileUri, getRepoNameGraphQLMock } = prepareEnterpriseMocks() + + const repoNames = await firstResultFromOperation( + repoNameResolver.getRepoNamesContainingUri(fileUri) + ) + const shouldReuseCachedValue = await firstResultFromOperation( + repoNameResolver.getRepoNamesContainingUri(fileUri) + ) + + expect(repoNames).toEqual(['sourcegraph/cody']) + expect(shouldReuseCachedValue).toEqual(repoNames) expect(getRepoNameGraphQLMock).toBeCalledTimes(1) }) diff --git a/vscode/src/repository/repo-name-resolver.ts b/vscode/src/repository/repo-name-resolver.ts index 0f42093a402f..84fc1bd882a3 100644 --- a/vscode/src/repository/repo-name-resolver.ts +++ b/vscode/src/repository/repo-name-resolver.ts @@ -1,7 +1,10 @@ +import { LRUCache } from 'lru-cache' +import { Observable, map } from 'observable-fns' import type * as vscode from 'vscode' import { ContextFiltersProvider, + type MaybePendingObservable, authStatus, combineLatest, convertGitCloneURLToCodebaseName, @@ -19,10 +22,14 @@ import { switchMapReplayOperation, } from '@sourcegraph/cody-shared' -import { Observable, map } from 'observable-fns' import { logDebug } from '../output-channel-logger' + import { gitRemoteUrlsForUri } from './remote-urls-from-parent-dirs' +type RemoteUrl = string +type RepoName = string +type UriFsPath = string + export class RepoNameResolver { /** * Get the names of repositories (such as `github.com/foo/bar`) that contain the given file URI. @@ -31,36 +38,34 @@ export class RepoNameResolver { * ❗️ For enterprise, this uses the Sourcegraph API to resolve repo names instead of the local * conversion function. ❗️ */ - public getRepoNamesContainingUri(uri: vscode.Uri): Observable { + public getRepoNamesContainingUri(uri: vscode.Uri): MaybePendingObservable { return combineLatest( - promiseFactoryToObservable(signal => this.getUniqueRemoteUrlsCached(uri, signal)), + promiseFactoryToObservable(signal => this.getRemoteUrlsCached(uri, signal)), authStatus ).pipe( - switchMapReplayOperation( - ([uniqueRemoteUrls, authStatus]): Observable => { - // Use local conversion function for non-enterprise accounts. - if (isDotCom(authStatus)) { - return Observable.of( - uniqueRemoteUrls.map(convertGitCloneURLToCodebaseName).filter(isDefined) - ) - } - - return combineLatest( - ...uniqueRemoteUrls.map(remoteUrl => this.getRepoNameCached(remoteUrl)) - ).pipe( - map(repoNames => - repoNames.includes(pendingOperation) - ? pendingOperation - : ( - repoNames as Exclude< - (typeof repoNames)[number], - typeof pendingOperation - >[] - ).filter(isDefined) - ) + switchMapReplayOperation(([remoteUrls, authStatus]) => { + // Use local conversion function for non-enterprise accounts. + if (isDotCom(authStatus)) { + return Observable.of( + remoteUrls.map(convertGitCloneURLToCodebaseName).filter(isDefined) ) } - ), + + return combineLatest( + ...remoteUrls.map(remoteUrl => this.getRepoNameCached(remoteUrl)) + ).pipe( + map(repoNames => + repoNames.includes(pendingOperation) + ? pendingOperation + : ( + repoNames as Exclude< + (typeof repoNames)[number], + typeof pendingOperation + >[] + ).filter(isDefined) + ) + ) + }), map(value => { if (isError(value)) { logDebug('RepoNameResolver:getRepoNamesContainingUri', 'error', { verbose: value }) @@ -71,40 +76,49 @@ export class RepoNameResolver { ) } - private getUniqueRemoteUrlsCache: Partial>> = {} - private async getUniqueRemoteUrlsCached(uri: vscode.Uri, signal?: AbortSignal): Promise { + private fsPathToRemoteUrlsInfo = new LRUCache>({ + max: 1000, + }) + + private async getRemoteUrlsCached(uri: vscode.Uri, signal?: AbortSignal): Promise { const key = uri.toString() - let uniqueRemoteUrls: Promise | undefined = this.getUniqueRemoteUrlsCache[key] - if (!uniqueRemoteUrls) { - uniqueRemoteUrls = gitRemoteUrlsForUri(uri, signal) - .then(remoteUrls => { - const uniqueRemoteUrls = Array.from(new Set(remoteUrls ?? [])).sort() - return uniqueRemoteUrls - }) - .catch(error => { - logError('RepoNameResolver:getUniqueRemoteUrlsCached', 'error', { - verbose: error, - }) - return [] + let remoteUrlsInfo = this.fsPathToRemoteUrlsInfo.get(key) + + if (!remoteUrlsInfo) { + remoteUrlsInfo = gitRemoteUrlsForUri(uri, signal).catch(error => { + logError('RepoNameResolver:getRemoteUrlsInfoCached', 'error', { + verbose: error, }) - this.getUniqueRemoteUrlsCache[key] = uniqueRemoteUrls + return [] + }) + this.fsPathToRemoteUrlsInfo.set(key, remoteUrlsInfo) } - return uniqueRemoteUrls + return remoteUrlsInfo } - private getRepoNameCache: Partial< - Record> - > = {} - private getRepoNameCached(remoteUrl: string): Observable { + private remoteUrlToRepoName = new LRUCache>({ + max: 100, + }) + private getRepoNameCached(remoteUrl: string): MaybePendingObservable { const key = remoteUrl - let observable: ReturnType | undefined = - this.getRepoNameCache[key] + let observable = this.remoteUrlToRepoName.get(key) + if (!observable) { observable = resolvedConfig.pipe( pluck('auth'), distinctUntilChanged(), - switchMapReplayOperation(() => - promiseFactoryToObservable(signal => graphqlClient.getRepoName(remoteUrl, signal)) + switchMapReplayOperation( + () => + promiseFactoryToObservable(signal => + graphqlClient.getRepoName(remoteUrl, signal) + ), + { + // Keep this observable alive with cached repo names, + // even without active subscribers. It's essential for + // `getRepoNameCached` in `ContextFiltersProvider`, which is + // part of the latency-sensitive autocomplete critical path. + shouldCountRefs: false, + } ), map(value => { if (isError(value)) { @@ -114,7 +128,7 @@ export class RepoNameResolver { return value }) ) - this.getRepoNameCache[key] = observable + this.remoteUrlToRepoName.set(key, observable) } return observable }