VS Code: fix repo name resolution cache (#5978)

- Ensures the repo name resolution is cached and we don't make
unexpected API calls for already resolved remote URLs.
- This fix is critical for enterprise clients because the regression
significantly increases the repo-name resolution latency, which is used
by every enterprise client with Cody Context filters and is on the
critical path for every autocomplete request. In my local tests, it adds
~500-600ms to every repo name resolution call.
- Closes
https://linear.app/sourcegraph/issue/CODY-4139/investigate-repo-name-resolution-latency-increase

lib/shared/src/misc/observable.ts

@@ -562,10 +562,31 @@ export function pick<T, K extends keyof T>(
 // 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<T>(): (observable: ObservableLike<T>) => Observable<T> {
+export function shareReplay<T>(
+    config?: ShareReplayConfig
+): (observable: ObservableLike<T>) => Observable<T> {
     // 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<T>(): (observable: ObservableLike<T>) => Observable<
             return () => {
                 refCount--
                 innerSub.unsubscribe()
-                if (refCount === 0) {
+                if (shouldCountRefs && refCount === 0) {
                     if (subscription) {
                         unsubscribe(subscription)
                         subscription = null

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<T> = Observable<T | typeof pendingOperation>
+export type MaybePendingObservableLike<T> = ObservableLike<T | typeof pendingOperation>
+
 /**
  * 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<T, R>(
-    operation: (value: T) => Observable<R>
-): (
-    source: ObservableLike<T | typeof pendingOperation>
-) => Observable<R | typeof pendingOperation | Error> {
-    return (
-        source: ObservableLike<T | typeof pendingOperation>
-    ): Observable<R | typeof pendingOperation | Error> => {
+    operation: (value: T) => Observable<R>,
+    shareReplayConfig?: ShareReplayConfig
+): (source: MaybePendingObservableLike<T>) => MaybePendingObservable<R | Error> {
+    return (source: MaybePendingObservableLike<T>): MaybePendingObservable<R | Error> => {
         return Observable.from(source).pipe(
             switchMap(outerValue =>
                 outerValue === pendingOperation
@@ -33,18 +41,16 @@ export function switchMapReplayOperation<T, R>(
                           startWith(pendingOperation)
                       )
             ),
-            shareReplay()
+            shareReplay(shareReplayConfig)
         )
     }
 }
 
 /**
  * An observable pipe operator to filter out {@link pendingOperation} values.
  */
-export function skipPendingOperation<T>(): (
-    source: ObservableLike<T | typeof pendingOperation>
-) => Observable<T> {
-    return (source: ObservableLike<T | typeof pendingOperation>) =>
+export function skipPendingOperation<T>(): (source: MaybePendingObservableLike<T>) => Observable<T> {
+    return (source: MaybePendingObservableLike<T>) =>
         Observable.from(source).pipe(filter((value): value is T => value !== pendingOperation))
 }
 

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

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://[email protected]: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://[email protected]:foo/bar.git'))).toEqual([])
         expect(statMock).toBeCalledTimes(0)
         expect(readFileMock).toBeCalledTimes(0)
     })

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<string[] | undefined> {
-    const fromGitExtension = gitRemoteUrlsFromGitExtension(uri)
-    if (fromGitExtension && fromGitExtension.length > 0) {
-        return fromGitExtension
+export async function gitRemoteUrlsForUri(uri: vscode.Uri, signal?: AbortSignal): Promise<string[]> {
+    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 []
 }
 
 /**

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)
     })