Track loading states of providers (#9914)

* Update codeintellify

* Track loading states of providers

Fixes #9349
Fixes #9350

* Remove dead code for support of MarkedStrings

* Fix test coverage errors on .d.ts files

browser/src/libs/code_intelligence/code_intelligence.tsx

@@ -4,6 +4,7 @@ import {
     findPositionsFromEvents,
     Hoverifier,
     HoverState,
+    MaybeLoadingResult,
 } from '@sourcegraph/codeintellify'
 import { TextDocumentDecoration } from '@sourcegraph/extension-api-types'
 import * as H from 'history'
@@ -81,7 +82,7 @@ import {
 import { observeStorageKey } from '../../browser/storage'
 import { isInPage } from '../../context'
 import { SourcegraphIntegrationURLs, BrowserPlatformContext } from '../../platform/context'
-import { createLSPFromExtensions, toTextDocumentIdentifier } from '../../shared/backend/lsp'
+import { toTextDocumentIdentifier, toTextDocumentPositionParams } from '../../shared/backend/ext-api-conversion'
 import { CodeViewToolbar, CodeViewToolbarClassProps } from '../../shared/components/CodeViewToolbar'
 import { resolveRev, retryWhenCloneInProgressError } from '../../shared/repo/backend'
 import { EventLogger } from '../../shared/tracking/eventLogger'
@@ -356,8 +357,6 @@ function initCodeIntelligence({
 } {
     const subscription = new Subscription()
 
-    const { getHover } = createLSPFromExtensions(extensionsController)
-
     /** Emits when the close button was clicked */
     const closeButtonClicks = new Subject<MouseEvent>()
     const nextCloseButtonClick = closeButtonClicks.next.bind(closeButtonClicks)
@@ -389,11 +388,18 @@ function initCodeIntelligence({
         ),
         getHover: ({ line, character, part, ...rest }) =>
             combineLatest([
-                getHover({ ...rest, position: { line, character } }),
+                extensionsController.services.textDocumentHover.getHover(
+                    toTextDocumentPositionParams({ ...rest, position: { line, character } })
+                ),
                 getActiveHoverAlerts(hoverAlerts),
             ]).pipe(
-                map(([hoverMerged, alerts]): HoverData<ExtensionHoverAlertType> | null =>
-                    hoverMerged ? { ...hoverMerged, alerts } : null
+                map(
+                    ([{ isLoading, result: hoverMerged }, alerts]): MaybeLoadingResult<HoverData<
+                        ExtensionHoverAlertType
+                    > | null> => ({
+                        isLoading,
+                        result: hoverMerged ? { ...hoverMerged, alerts } : null,
+                    })
                 )
             ),
         getActions: context => getHoverActions({ extensionsController, platformContext }, context),

browser/src/shared/backend/ext-api-conversion.tsx

@@ -0,0 +1,15 @@
+import { TextDocumentIdentifier } from '../../../../shared/src/api/client/types/textDocument'
+import { TextDocumentPositionParams } from '../../../../shared/src/api/protocol'
+import { AbsoluteRepoFilePosition, FileSpec, RepoSpec, ResolvedRevSpec } from '../../../../shared/src/util/url'
+
+export const toTextDocumentIdentifier = (pos: RepoSpec & ResolvedRevSpec & FileSpec): TextDocumentIdentifier => ({
+    uri: `git://${pos.repoName}?${pos.commitID}#${pos.filePath}`,
+})
+
+export const toTextDocumentPositionParams = (pos: AbsoluteRepoFilePosition): TextDocumentPositionParams => ({
+    textDocument: toTextDocumentIdentifier(pos),
+    position: {
+        character: pos.position.character - 1,
+        line: pos.position.line - 1,
+    },
+})

jest.config.base.js

@@ -11,7 +11,7 @@ const config = {
   collectCoverage: !!process.env.CI,
   collectCoverageFrom: ['<rootDir>/src/**/*.{ts,tsx}'],
   coverageDirectory: '<rootDir>/coverage',
-  coveragePathIgnorePatterns: [/\.(test|story)\.tsx?$/.source],
+  coveragePathIgnorePatterns: [/\.(test|story)\.tsx?$/.source, /\.d\.ts$/.source],
   roots: ['<rootDir>/src'],
 
   // Transform packages that do not distribute CommonJS packages (typically because they only distribute ES6

package.json

@@ -235,7 +235,7 @@
     "@reach/menu-button": "0.10.0",
     "@sentry/browser": "^5.15.4",
     "@slimsag/react-shortcuts": "^1.2.1",
-    "@sourcegraph/codeintellify": "^6.4.0",
+    "@sourcegraph/codeintellify": "^7.0.0",
     "@sourcegraph/comlink": "^3.1.1-fork.3",
     "@sourcegraph/extension-api-classes": "^1.0.3",
     "@sourcegraph/extension-api-types": "link:packages/@sourcegraph/extension-api-types",

shared/src/api/client/api/views.ts

@@ -1,12 +1,14 @@
 import { ProxyValue, proxyValue, proxyValueSymbol } from '@sourcegraph/comlink'
 import { isEqual, omit } from 'lodash'
-import { combineLatest, from, of, ReplaySubject, Unsubscribable } from 'rxjs'
+import { combineLatest, from, ReplaySubject, Unsubscribable, ObservableInput } from 'rxjs'
 import { distinctUntilChanged, map, switchMap } from 'rxjs/operators'
 import { PanelView } from 'sourcegraph'
 import { ContributableViewContainer } from '../../protocol'
 import { EditorService, getActiveCodeEditorPosition } from '../services/editorService'
 import { TextDocumentLocationProviderIDRegistry } from '../services/location'
 import { PanelViewWithComponent, ViewProviderRegistry } from '../services/view'
+import { Location } from '@sourcegraph/extension-api-types'
+import { MaybeLoadingResult } from '@sourcegraph/codeintellify'
 
 /** @internal */
 export interface PanelViewData extends Pick<PanelView, 'title' | 'content' | 'priority' | 'component'> {}
@@ -51,12 +53,14 @@ export class ClientViews implements ClientViewsAPI {
 
                         return from(this.editorService.activeEditorUpdates).pipe(
                             map(getActiveCodeEditorPosition),
-                            switchMap(params => {
-                                if (!params) {
-                                    return of(of(null))
+                            switchMap(
+                                (params): ObservableInput<MaybeLoadingResult<Location[]>> => {
+                                    if (!params) {
+                                        return [{ isLoading: false, result: [] }]
+                                    }
+                                    return this.textDocumentLocations.getLocations(component.locationProvider, params)
                                 }
-                                return this.textDocumentLocations.getLocations(component.locationProvider, params)
-                            })
+                            )
                         )
                     })
                 ),

shared/src/api/client/services/hover.test.ts

@@ -1,5 +1,4 @@
 import { MarkupKind } from '@sourcegraph/extension-api-classes'
-import { of, throwError } from 'rxjs'
 import { TestScheduler } from 'rxjs/testing'
 import { Hover } from 'sourcegraph'
 import { HoverMerged } from '../types/hover'
@@ -13,138 +12,161 @@ const FIXTURE_RESULT_MERGED: HoverMerged | null = { contents: [{ value: 'c', kin
 
 describe('getHover', () => {
     describe('0 providers', () => {
-        test('returns null', () =>
+        test('returns null', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-|', { a: [] }),
+                        cold<ProvideTextDocumentHoverSignature[]>('-a', { a: [] }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-|', {
-                    a: null,
+                ).toBe('-a', {
+                    a: { isLoading: false, result: null },
                 })
-            ))
+            )
+        })
     })
 
     describe('1 provider', () => {
-        test('returns null result from provider', () =>
+        it('returns null result from provider', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-|', { a: [() => of(null)] }),
+                        cold<ProvideTextDocumentHoverSignature[]>('-a', { a: [() => cold('--a', { a: null })] }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-|', {
-                    a: null,
+                ).toBe('-l-r', {
+                    l: { isLoading: true, result: null },
+                    r: { isLoading: false, result: null },
                 })
-            ))
+            )
+        })
 
-        test('returns result from provider', () =>
+        test('returns result from provider', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-|', {
-                            a: [() => of(FIXTURE_RESULT)],
+                        cold<ProvideTextDocumentHoverSignature[]>('-a', {
+                            a: [() => cold('-a', { a: FIXTURE_RESULT })],
                         }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-|', {
-                    a: FIXTURE_RESULT_MERGED,
+                ).toBe('-lr', {
+                    l: { isLoading: true, result: null },
+                    r: { isLoading: false, result: FIXTURE_RESULT_MERGED },
                 })
-            ))
+            )
+        })
     })
 
     describe('2 providers', () => {
-        test('returns null result if both providers return null', () =>
+        it('returns null result if both providers return null', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-|', {
-                            a: [() => of(null), () => of(null)],
+                        cold<ProvideTextDocumentHoverSignature[]>('-a', {
+                            a: [() => cold('-a', { a: null }), () => cold('-a', { a: null })],
                         }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-|', {
-                    a: null,
+                ).toBe('-lr', {
+                    l: { isLoading: true, result: null },
+                    r: { isLoading: false, result: null },
                 })
-            ))
+            )
+        })
 
-        test('omits null result from 1 provider', () =>
+        it('omits null result from 1 provider', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-|', {
-                            a: [() => of(FIXTURE_RESULT), () => of(null)],
+                        cold<ProvideTextDocumentHoverSignature[]>('-a', {
+                            a: [() => cold('-a', { a: FIXTURE_RESULT }), () => cold('-a', { a: null })],
                         }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-|', {
-                    a: FIXTURE_RESULT_MERGED,
+                ).toBe('-lr', {
+                    l: { isLoading: true, result: null },
+                    r: { isLoading: false, result: FIXTURE_RESULT_MERGED },
                 })
-            ))
+            )
+        })
 
-        test('omits error result from 1 provider', () =>
+        it('omits error result from 1 provider', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-|', {
-                            a: [() => of(FIXTURE_RESULT), () => throwError(new Error('err'))],
+                        cold<ProvideTextDocumentHoverSignature[]>('-a', {
+                            a: [() => cold('-a', { a: FIXTURE_RESULT }), () => cold('-#', {}, new Error('err'))],
                         }),
                         FIXTURE.TextDocumentPositionParams,
                         false
                     )
-                ).toBe('-a-|', {
-                    a: FIXTURE_RESULT_MERGED,
+                ).toBe('-lr', {
+                    l: { isLoading: true, result: null },
+                    r: { isLoading: false, result: FIXTURE_RESULT_MERGED },
                 })
-            ))
+            )
+        })
 
-        test('merges results from providers', () =>
+        it('merges results from providers', () => {
             scheduler().run(({ cold, expectObservable }) =>
                 expectObservable(
                     getHover(
                         cold<ProvideTextDocumentHoverSignature[]>('-a-|', {
                             a: [
                                 () =>
-                                    of({
-                                        contents: { value: 'c1' },
-                                        range: { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } },
+                                    cold('-a', {
+                                        a: {
+                                            contents: { value: 'c1' },
+                                            range: { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } },
+                                        },
                                     }),
                                 () =>
-                                    of({
-                                        contents: { value: 'c2' },
-                                        range: { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } },
+                                    cold('-a', {
+                                        a: {
+                                            contents: { value: 'c2' },
+                                            range: { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } },
+                                        },
                                     }),
                             ],
                         }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-|', {
-                    a: {
-                        contents: [
-                            { value: 'c1', kind: MarkupKind.PlainText },
-                            { value: 'c2', kind: MarkupKind.PlainText },
-                        ],
-                        range: { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } },
+                ).toBe('-lr', {
+                    l: { isLoading: true, result: null },
+                    r: {
+                        isLoading: false,
+                        result: {
+                            contents: [
+                                { value: 'c1', kind: MarkupKind.PlainText },
+                                { value: 'c2', kind: MarkupKind.PlainText },
+                            ],
+                            range: { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } },
+                        },
                     },
                 })
-            ))
+            )
+        })
     })
 
     describe('multiple emissions', () => {
-        test('returns stream of results', () =>
-            scheduler().run(({ cold, expectObservable }) =>
+        it('returns stream of results', () => {
+            scheduler().run(({ cold, expectObservable }) => {
                 expectObservable(
                     getHover(
-                        cold<ProvideTextDocumentHoverSignature[]>('-a-b-|', {
-                            a: [() => of(FIXTURE_RESULT)],
-                            b: [() => of(null)],
+                        cold<ProvideTextDocumentHoverSignature[]>('-a-b', {
+                            a: [() => cold('-a', { a: FIXTURE_RESULT })],
+                            b: [() => cold('-a', { a: null })],
                         }),
                         FIXTURE.TextDocumentPositionParams
                     )
-                ).toBe('-a-b-|', {
-                    a: FIXTURE_RESULT_MERGED,
-                    b: null,
+                ).toBe('-abcd', {
+                    a: { isLoading: true, result: null },
+                    b: { isLoading: false, result: FIXTURE_RESULT_MERGED },
+                    c: { isLoading: true, result: null },
+                    d: { isLoading: false, result: null },
                 })
-            ))
+            })
+        })
     })
 })