fix various extensions bugs to improve the basic-code-intel experience

packages/sourcegraph-extension-api/src/sourcegraph.d.ts

@@ -1100,17 +1100,39 @@ declare module 'sourcegraph' {
         export const clientApplication: 'sourcegraph' | 'other'
     }
 
+    /** Support types for {@link Subscribable}. */
+    interface NextObserver<T> {
+        closed?: boolean
+        next: (value: T) => void
+        error?: (err: any) => void
+        complete?: () => void
+    }
+    interface ErrorObserver<T> {
+        closed?: boolean
+        next?: (value: T) => void
+        error: (err: any) => void
+        complete?: () => void
+    }
+    interface CompletionObserver<T> {
+        closed?: boolean
+        next?: (value: T) => void
+        error?: (err: any) => void
+        complete: () => void
+    }
+    type PartialObserver<T> = NextObserver<T> | ErrorObserver<T> | CompletionObserver<T>
+
     /**
      * A stream of values that may be subscribed to.
      */
     export interface Subscribable<T> {
         /**
-         * Subscribes to the stream of values, calling {@link next} for each value until unsubscribed.
+         * Subscribes to the stream of values.
          *
          * @returns An unsubscribable that, when its {@link Unsubscribable#unsubscribe} method is called, causes
-         *          the subscription to stop calling {@link next} with values.
+         * the subscription to stop reacting to the stream.
          */
-        subscribe(next: (value: T) => void): Unsubscribable
+        subscribe(observer?: PartialObserver<T>): Unsubscribable
+        subscribe(next?: (value: T) => void, error?: (error: any) => void, complete?: () => void): Unsubscribable
     }
 
     /**

shared/src/actions/ActionItem.tsx

@@ -139,7 +139,7 @@ export class ActionItem extends React.PureComponent<Props, State> {
         )
     }
 
-    public runAction = (e: React.MouseEvent | React.KeyboardEvent) => {
+    public runAction = (e: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => {
         const action = (isAltEvent(e) && this.props.altAction) || this.props.action
         if (urlForClientCommandOpen(action, this.props.location)) {
             if (e.currentTarget.tagName === 'A' && e.currentTarget.hasAttribute('href')) {
@@ -162,6 +162,9 @@ export class ActionItem extends React.PureComponent<Props, State> {
         // ensure the default event handler for the <LinkOrButton> doesn't run (which might open the URL).
         e.preventDefault()
 
+        // Do not show focus ring on element after running action.
+        e.currentTarget.blur()
+
         this.commandExecutions.next({
             command: this.props.action.command,
             arguments: this.props.action.commandArguments,

shared/src/api/client/context/contextService.ts

@@ -18,6 +18,15 @@ export function createContextService({
     clientApplication,
 }: Pick<PlatformContext, 'clientApplication'>): ContextService {
     return {
-        data: new BehaviorSubject<Context>({ 'clientApplication.isSourcegraph': clientApplication === 'sourcegraph' }),
+        data: new BehaviorSubject<Context>({
+            'clientApplication.isSourcegraph': clientApplication === 'sourcegraph',
+
+            // Arbitrary, undocumented versioning for extensions that need different behavior for different
+            // Sourcegraph versions.
+            //
+            // TODO: Make this more advanced if many extensions need this (although we should try to avoid
+            // extensions needing this).
+            'clientApplication.extensionAPIVersion.major': 3,
+        }),
     }
 }

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

@@ -1,5 +1,5 @@
 import * as assert from 'assert'
-import { of } from 'rxjs'
+import { of, throwError } from 'rxjs'
 import { TestScheduler } from 'rxjs/testing'
 import { Location } from '../../protocol/plainTypes'
 import { getLocation, getLocations, ProvideTextDocumentLocationSignature } from './location'
@@ -168,6 +168,20 @@ describe('getLocations', () => {
             })
         ))
 
+    it('errors do not propagate', () =>
+        scheduler().run(({ cold, expectObservable }) =>
+            expectObservable(
+                getLocations(
+                    cold<ProvideTextDocumentLocationSignature[]>('-a-|', {
+                        a: [() => of(FIXTURE_LOCATION), () => throwError('x')],
+                    }),
+                    FIXTURE.TextDocumentPositionParams
+                )
+            ).toBe('-a-|', {
+                a: [FIXTURE_LOCATION],
+            })
+        ))
+
     it('preserves array results', () =>
         scheduler().run(({ cold, expectObservable }) =>
             expectObservable(

shared/src/api/client/services/location.ts

@@ -1,5 +1,5 @@
 import { combineLatest, from, Observable, of } from 'rxjs'
-import { map, switchMap } from 'rxjs/operators'
+import { catchError, map, switchMap } from 'rxjs/operators'
 import { ReferenceParams, TextDocumentPositionParams, TextDocumentRegistrationOptions } from '../../protocol'
 import { Location } from '../../protocol/plainTypes'
 import { Model, modelToTextDocumentPositionParams } from '../model'
@@ -88,7 +88,16 @@ export function getLocations<
             if (providers.length === 0) {
                 return [null]
             }
-            return combineLatest(providers.map(provider => from(provider(params))))
+            return combineLatest(
+                providers.map(provider =>
+                    from(provider(params)).pipe(
+                        catchError(err => {
+                            console.error(err)
+                            return [null]
+                        })
+                    )
+                )
+            )
         }),
         map(flattenAndCompact)
     )

shared/src/api/extension/api/configuration.ts

@@ -1,4 +1,4 @@
-import { BehaviorSubject } from 'rxjs'
+import { BehaviorSubject, PartialObserver, Unsubscribable } from 'rxjs'
 import * as sourcegraph from 'sourcegraph'
 import { SettingsCascade } from '../../../settings/settings'
 import { ClientConfigurationAPI } from '../../client/api/configuration'
@@ -70,7 +70,9 @@ export class ExtConfiguration<C extends object> implements ExtConfigurationAPI<C
         return Object.freeze(new ExtConfigurationSection<C>(this.proxy, this.getData().value.final))
     }
 
-    public subscribe(next: () => void): sourcegraph.Unsubscribable {
-        return this.getData().subscribe(next)
+    public subscribe(observer?: PartialObserver<C>): Unsubscribable
+    public subscribe(next?: (value: C) => void, error?: (error: any) => void, complete?: () => void): Unsubscribable
+    public subscribe(...args: any[]): sourcegraph.Unsubscribable {
+        return this.getData().subscribe(...args)
     }
 }

shared/src/api/integration-test/context.test.ts

@@ -13,8 +13,8 @@ describe('Context (integration)', () => {
             extensionHost.internal.updateContext({ a: 1 })
             await extensionHost.internal.sync()
             assert.deepStrictEqual(values, [
-                { 'clientApplication.isSourcegraph': true },
-                { a: 1, 'clientApplication.isSourcegraph': true },
+                { 'clientApplication.isSourcegraph': true, 'clientApplication.extensionAPIVersion.major': 3 },
+                { a: 1, 'clientApplication.isSourcegraph': true, 'clientApplication.extensionAPIVersion.major': 3 },
             ] as ContextValues[])
         })
     })

shared/src/commands/commands.ts

@@ -2,7 +2,7 @@ import { isArray } from 'lodash-es'
 import { concat, from, of, Subscription, Unsubscribable } from 'rxjs'
 import { first } from 'rxjs/operators'
 import { Services } from '../api/client/services'
-import { SettingsEdit } from '../api/client/services/settings'
+import { KeyPath, SettingsEdit } from '../api/client/services/settings'
 import { ActionContributionClientCommandUpdateConfiguration } from '../api/protocol'
 import { Position } from '../api/protocol/plainTypes'
 import { PlatformContext } from '../platform/context'
@@ -133,14 +133,33 @@ export function urlForOpenPanel(viewID: string, urlHash: string): string {
 export function convertUpdateConfigurationCommandArgs(
     args: ActionContributionClientCommandUpdateConfiguration['commandArguments']
 ): SettingsEdit {
-    if (
-        !isArray(args) ||
-        !(args.length >= 2 && args.length <= 4) ||
-        !isArray(args[0]) ||
-        !(args[2] === undefined || args[2] === null)
-    ) {
-        throw new Error(`invalid updateConfiguration arguments: ${JSON.stringify(args)}`)
+    if (!isArray(args) || !(args.length >= 2 && args.length <= 4)) {
+        throw new Error(
+            `invalid updateConfiguration arguments: ${JSON.stringify(
+                args
+            )} (must be an array with either 2 or 4 elements)`
+        )
     }
+
+    let keyPath: KeyPath
+    if (isArray(args[0])) {
+        keyPath = args[0]
+    } else if (typeof args[0] === 'string') {
+        // For convenience, allow the 1st arg (the key path) to be a string, and interpret this as referring to the
+        // object property.
+        keyPath = [args[0]]
+    } else {
+        throw new Error(
+            `invalid updateConfiguration arguments: ${JSON.stringify(
+                args
+            )} (1st element, the key path, must be a string (referring to a settings property) or an array of type (string|number)[] (referring to a deeply nested settings property))`
+        )
+    }
+
+    if (!(args[2] === undefined || args[2] === null)) {
+        throw new Error(`invalid updateConfiguration arguments: ${JSON.stringify(args)} (3rd element must be null)`)
+    }
+
     const valueIsJSONEncoded = args.length === 4 && args[3] === 'json'
-    return { path: args[0], value: valueIsJSONEncoded ? JSON.parse(args[1]) : args[1] }
+    return { path: keyPath, value: valueIsJSONEncoded ? JSON.parse(args[1]) : args[1] }
 }

shared/src/components/LinkOrButton.tsx

@@ -13,7 +13,7 @@ interface Props {
     /**
      * Called when the user clicks or presses enter on this element.
      */
-    onSelect?: (event: React.MouseEvent | React.KeyboardEvent) => void
+    onSelect?: (event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => void
 
     /** A tooltip to display when the user hovers or focuses this element. */
     ['data-tooltip']?: string

shared/src/panel/views/HierarchicalLocationsView.tsx

@@ -156,50 +156,74 @@ export class HierarchicalLocationsView extends React.PureComponent<Props, State>
             { uri: this.props.defaultGroup }
         )
 
+        const groupsToDisplay = GROUPS.filter(({ name, key }, i) => {
+            if (!groups[i]) {
+                // No groups exist at this level. Don't display anything.
+                return false
+            }
+            if (groups[i].length > 1) {
+                // Always display when there is more than 1 group.
+                return true
+            }
+            if (groups[i].length === 1) {
+                if (selectedGroups[i] !== groups[i][0].key) {
+                    // When the only group is not the currently selected group, show it. This occurs when the
+                    // references list changes after the user made an initial selection. The group must be shown so
+                    // that the user can update their selection to the only available group; otherwise they would
+                    // be stuck viewing the (zero) results from the previously selected group that no longer
+                    // exists.
+                    return true
+                }
+                if (key({ uri: this.props.defaultGroup }) !== selectedGroups[i]) {
+                    // When the only group is other than the default group, show it. This is important because it
+                    // often indicates that the match comes from another repository. If it isn't shown, the user
+                    // would likely assume the match is from the current repository.
+                    return true
+                }
+            }
+            if (groupByFile && name === 'file') {
+                // Always display the file groups when group-by-file is enabled.
+                return true
+            }
+            return false
+        })
+
         return (
             <div className={`hierarchical-locations-view ${this.props.className || ''}`}>
                 {selectedGroups &&
-                    GROUPS.map(
-                        ({ name, defaultSize }, i) =>
-                            ((groupByFile && name === 'file') || groups[i].length > 1) && (
-                                <Resizable
-                                    key={i}
-                                    className="hierarchical-locations-view__resizable"
-                                    handlePosition="right"
-                                    storageKey={`hierarchical-locations-view-resizable:${name}`}
-                                    defaultSize={defaultSize}
-                                    element={
-                                        <div className="list-group list-group-flush hierarchical-locations-view__list">
-                                            {groups[i].map((group, j) => (
-                                                <span
-                                                    key={j}
-                                                    className={`list-group-item hierarchical-locations-view__item ${
-                                                        selectedGroups[i] === group.key ? 'active' : ''
-                                                    }`}
-                                                    // tslint:disable-next-line:jsx-no-lambda
-                                                    onClick={e => this.onSelectTree(e, selectedGroups, i, group.key)}
-                                                >
-                                                    <span
-                                                        className="hierarchical-locations-view__item-name"
-                                                        title={group.key}
-                                                    >
-                                                        <span className="hierarchical-locations-view__item-name-text">
-                                                            <RepoLink to={null} repoPath={group.key} />
-                                                        </span>
-                                                    </span>
-                                                    <span className="badge badge-secondary badge-pill hierarchical-locations-view__item-badge">
-                                                        {group.count}
-                                                    </span>
+                    groupsToDisplay.map(({ name, defaultSize }, i) => (
+                        <Resizable
+                            key={i}
+                            className="hierarchical-locations-view__resizable"
+                            handlePosition="right"
+                            storageKey={`hierarchical-locations-view-resizable:${name}`}
+                            defaultSize={defaultSize}
+                            element={
+                                <div className="list-group list-group-flush hierarchical-locations-view__list">
+                                    {groups[i].map((group, j) => (
+                                        <span
+                                            key={j}
+                                            className={`list-group-item hierarchical-locations-view__item ${
+                                                selectedGroups[i] === group.key ? 'active' : ''
+                                            }`}
+                                            // tslint:disable-next-line:jsx-no-lambda
+                                            onClick={e => this.onSelectTree(e, selectedGroups, i, group.key)}
+                                        >
+                                            <span className="hierarchical-locations-view__item-name" title={group.key}>
+                                                <span className="hierarchical-locations-view__item-name-text">
+                                                    <RepoLink to={null} repoPath={group.key} />
                                                 </span>
-                                            ))}
-                                            {!this.state.locationsComplete && (
-                                                <LoadingSpinner className="icon-inline m-2" />
-                                            )}
-                                        </div>
-                                    }
-                                />
-                            )
-                    )}
+                                            </span>
+                                            <span className="badge badge-secondary badge-pill hierarchical-locations-view__item-badge">
+                                                {group.count}
+                                            </span>
+                                        </span>
+                                    ))}
+                                    {!this.state.locationsComplete && <LoadingSpinner className="icon-inline m-2" />}
+                                </div>
+                            }
+                        />
+                    ))}
                 <FileLocations
                     className="hierarchical-locations-view__content"
                     locations={of(visibleLocations)}

web/src/components/FilteredConnection.tsx

@@ -21,6 +21,7 @@ import {
 import * as GQL from '../../../shared/src/graphql/schema'
 import { asError, ErrorLike, isErrorLike } from '../../../shared/src/util/errors'
 import { pluralize } from '../../../shared/src/util/strings'
+import { parseHash } from '../../../shared/src/util/url'
 import { Form } from './Form'
 import { RadioButtons } from './RadioButtons'
 
@@ -130,7 +131,9 @@ interface ConnectionNodesProps<C extends Connection<N>, N, NP = {}>
 
 class ConnectionNodes<C extends Connection<N>, N, NP = {}> extends React.PureComponent<ConnectionNodesProps<C, N, NP>> {
     public componentDidMount(): void {
-        if (this.props.location.hash) {
+        // Scroll to hash. But if the hash is a line number in a blob (or any viewState), then do not scroll,
+        // because the hash is handled separately by the Blob component.
+        if (this.props.location.hash && !parseHash(this.props.location.hash).viewState) {
             this.scrollToHash(this.props.location.hash)
         }
     }

web/src/global-styles/nav.scss

@@ -3,3 +3,21 @@ $navbar-padding-x: ($spacer / 2);
 
 @import 'bootstrap/scss/nav';
 @import 'bootstrap/scss/navbar';
+
+// Remove focus ring around nav links during mousedown.
+//
+// The focus ring on nav links is usually undesirable. It shows up when you click on a nav link and remains until
+// you click elsewhere. Because many of our nav links are toolbar buttons, this means that (1) the focus ring
+// remains visible even when it is not useful anymore (after you've clicked the button) and (2) the focus ring is
+// chopped off on the top and bottom by the toolbar boundaries.
+//
+// But the focus ring is important when using the keyboard to focus and for accessibility. In the future, the CSS
+// :focus-visible pseudo-selector will solve our problems (and we could consider using the polyfill
+// https://github.com/wicg/focus-visible).
+//
+// For now, we only remove the focus ring during mousedown. This lets elements that want to eliminate the focus
+// ring for mouse focus (without affecting it for keyboard focus or a11y) do so by calling HTMLElement#blur in an
+// onclick handler.
+.nav-link:active {
+    box-shadow: none !important;
+}

web/src/nav/GlobalNavbar.scss

@@ -8,7 +8,7 @@
     justify-content: space-between;
     height: 2.75rem;
     width: 100%;
-    z-index: 4;
+    z-index: 200;
     &--bg {
         background-color: $color-bg-2;
     }

web/src/repo/blob/panel/BlobPanel.tsx

@@ -180,7 +180,7 @@ export class BlobPanel extends React.PureComponent<Props> {
                                                       []
                                                   )
                                               )}
-                                              defaultGroup={this.props.repoPath}
+                                              defaultGroup={'git://' + this.props.repoPath}
                                               isLightTheme={this.props.isLightTheme}
                                               fetchHighlightedFileLines={fetchHighlightedFileLines}
                                               settingsCascade={this.props.settingsCascade}