From c7f6e7d63845ad159df98ed08477f5f3079485e1 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 01:59:26 -0800 Subject: [PATCH 1/9] do not propagate errors in getLocations, prevent a bad references provider from breaking all others --- shared/src/api/client/services/location.test.ts | 16 +++++++++++++++- shared/src/api/client/services/location.ts | 13 +++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/shared/src/api/client/services/location.test.ts b/shared/src/api/client/services/location.test.ts index f5b4a3b018d0..d16558fd3be6 100644 --- a/shared/src/api/client/services/location.test.ts +++ b/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('-a-|', { + a: [() => of(FIXTURE_LOCATION), () => throwError('x')], + }), + FIXTURE.TextDocumentPositionParams + ) + ).toBe('-a-|', { + a: [FIXTURE_LOCATION], + }) + )) + it('preserves array results', () => scheduler().run(({ cold, expectObservable }) => expectObservable( diff --git a/shared/src/api/client/services/location.ts b/shared/src/api/client/services/location.ts index bb2b639944d6..5c9fa6bd006a 100644 --- a/shared/src/api/client/services/location.ts +++ b/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) ) From 66e24e13dd9fd83e412d66ca6f252bbf413e7b5e Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 02:00:26 -0800 Subject: [PATCH 2/9] expose extension API version in context --- shared/src/api/client/context/contextService.ts | 11 ++++++++++- shared/src/api/integration-test/context.test.ts | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/shared/src/api/client/context/contextService.ts b/shared/src/api/client/context/contextService.ts index 01baa0ebd72c..773fb56d3f27 100644 --- a/shared/src/api/client/context/contextService.ts +++ b/shared/src/api/client/context/contextService.ts @@ -18,6 +18,15 @@ export function createContextService({ clientApplication, }: Pick): ContextService { return { - data: new BehaviorSubject({ 'clientApplication.isSourcegraph': clientApplication === 'sourcegraph' }), + data: new BehaviorSubject({ + '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, + }), } } diff --git a/shared/src/api/integration-test/context.test.ts b/shared/src/api/integration-test/context.test.ts index b92b6de5e51b..5ec65d3c83fe 100644 --- a/shared/src/api/integration-test/context.test.ts +++ b/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[]) }) }) From 6a5a4008ad146fa6675ca99cf28f1e019810e7c3 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 02:00:43 -0800 Subject: [PATCH 3/9] better error messages and lenience in updateConfiguration command --- shared/src/commands/commands.ts | 37 +++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/shared/src/commands/commands.ts b/shared/src/commands/commands.ts index fe8c30c1bcc3..8188479c9e64 100644 --- a/shared/src/commands/commands.ts +++ b/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] } } From 6ee3653b2243ee231a1706869e16cbcdf6c502eb Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 02:50:44 -0800 Subject: [PATCH 4/9] show user menu above hover tooltip --- web/src/nav/GlobalNavbar.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/nav/GlobalNavbar.scss b/web/src/nav/GlobalNavbar.scss index f6f5772e8adc..433fe42932c1 100644 --- a/web/src/nav/GlobalNavbar.scss +++ b/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; } From 072eaa8a50680972da8cbdf5c52ba5ba0189377b Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 02:51:05 -0800 Subject: [PATCH 5/9] make sourcegraph.configuration.subscribe more like a valid subscribe func --- .../src/sourcegraph.d.ts | 28 +++++++++++++++++-- shared/src/api/extension/api/configuration.ts | 8 ++++-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/packages/sourcegraph-extension-api/src/sourcegraph.d.ts b/packages/sourcegraph-extension-api/src/sourcegraph.d.ts index 993c3451b779..fe21dce9f5e1 100644 --- a/packages/sourcegraph-extension-api/src/sourcegraph.d.ts +++ b/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 { + closed?: boolean + next: (value: T) => void + error?: (err: any) => void + complete?: () => void + } + interface ErrorObserver { + closed?: boolean + next?: (value: T) => void + error: (err: any) => void + complete?: () => void + } + interface CompletionObserver { + closed?: boolean + next?: (value: T) => void + error?: (err: any) => void + complete: () => void + } + type PartialObserver = NextObserver | ErrorObserver | CompletionObserver + /** * A stream of values that may be subscribed to. */ export interface Subscribable { /** - * 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): Unsubscribable + subscribe(next?: (value: T) => void, error?: (error: any) => void, complete?: () => void): Unsubscribable } /** diff --git a/shared/src/api/extension/api/configuration.ts b/shared/src/api/extension/api/configuration.ts index dff502419cad..94bb1ccae8fb 100644 --- a/shared/src/api/extension/api/configuration.ts +++ b/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 implements ExtConfigurationAPI(this.proxy, this.getData().value.final)) } - public subscribe(next: () => void): sourcegraph.Unsubscribable { - return this.getData().subscribe(next) + public subscribe(observer?: PartialObserver): 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) } } From 6f884bdc26e4899e25b3548a0968db062c66d0da Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 03:15:32 -0800 Subject: [PATCH 6/9] fix null deref when none of the current repo's locations are in the results --- .../panel/views/HierarchicalLocationsView.tsx | 104 +++++++++++------- 1 file changed, 64 insertions(+), 40 deletions(-) diff --git a/shared/src/panel/views/HierarchicalLocationsView.tsx b/shared/src/panel/views/HierarchicalLocationsView.tsx index c547e59f6163..7d397df30ff9 100644 --- a/shared/src/panel/views/HierarchicalLocationsView.tsx +++ b/shared/src/panel/views/HierarchicalLocationsView.tsx @@ -156,50 +156,74 @@ export class HierarchicalLocationsView extends React.PureComponent { 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 (
{selectedGroups && - GROUPS.map( - ({ name, defaultSize }, i) => - ((groupByFile && name === 'file') || groups[i].length > 1) && ( - - {groups[i].map((group, j) => ( - this.onSelectTree(e, selectedGroups, i, group.key)} - > - - - - - - - {group.count} - + groupsToDisplay.map(({ name, defaultSize }, i) => ( + + {groups[i].map((group, j) => ( + this.onSelectTree(e, selectedGroups, i, group.key)} + > + + + - ))} - {!this.state.locationsComplete && ( - - )} -
- } - /> - ) - )} + + + {group.count} + + + ))} + {!this.state.locationsComplete && } + + } + /> + ))} Date: Wed, 5 Dec 2018 21:33:58 -0800 Subject: [PATCH 7/9] remove focus ring for actions triggered by mouse --- shared/src/actions/ActionItem.tsx | 5 ++++- shared/src/components/LinkOrButton.tsx | 2 +- web/src/global-styles/nav.scss | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/shared/src/actions/ActionItem.tsx b/shared/src/actions/ActionItem.tsx index f477967095b4..9f912dff8643 100644 --- a/shared/src/actions/ActionItem.tsx +++ b/shared/src/actions/ActionItem.tsx @@ -139,7 +139,7 @@ export class ActionItem extends React.PureComponent { ) } - public runAction = (e: React.MouseEvent | React.KeyboardEvent) => { + public runAction = (e: React.MouseEvent | React.KeyboardEvent) => { 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 { // ensure the default event handler for the 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, diff --git a/shared/src/components/LinkOrButton.tsx b/shared/src/components/LinkOrButton.tsx index cfaa494d5ba3..994ac7bfa9a3 100644 --- a/shared/src/components/LinkOrButton.tsx +++ b/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 | React.KeyboardEvent) => void /** A tooltip to display when the user hovers or focuses this element. */ ['data-tooltip']?: string diff --git a/web/src/global-styles/nav.scss b/web/src/global-styles/nav.scss index 165a82a3cbe6..030607c7c682 100644 --- a/web/src/global-styles/nav.scss +++ b/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; +} From 8b22e2bd9bda9c93cb1340863a1d766273edb309 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 22:10:22 -0800 Subject: [PATCH 8/9] fix #1252 --- web/src/repo/blob/panel/BlobPanel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/repo/blob/panel/BlobPanel.tsx b/web/src/repo/blob/panel/BlobPanel.tsx index 46eea4c8d779..0c41d8cd17f5 100644 --- a/web/src/repo/blob/panel/BlobPanel.tsx +++ b/web/src/repo/blob/panel/BlobPanel.tsx @@ -180,7 +180,7 @@ export class BlobPanel extends React.PureComponent { [] ) )} - defaultGroup={this.props.repoPath} + defaultGroup={'git://' + this.props.repoPath} isLightTheme={this.props.isLightTheme} fetchHighlightedFileLines={fetchHighlightedFileLines} settingsCascade={this.props.settingsCascade} From 3fea5599611e54c18eb38fa0fe5fd7922238d2b8 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Wed, 5 Dec 2018 22:36:07 -0800 Subject: [PATCH 9/9] fix #1251 NOCHANGELOG --- web/src/components/FilteredConnection.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/web/src/components/FilteredConnection.tsx b/web/src/components/FilteredConnection.tsx index 9d4ba9983e79..d79134c69d2d 100644 --- a/web/src/components/FilteredConnection.tsx +++ b/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, N, NP = {}> class ConnectionNodes, N, NP = {}> extends React.PureComponent> { 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) } }