From 364145452b337bafc6a9b10e1fe9ee722b74aa6f Mon Sep 17 00:00:00 2001 From: Tom Ross Date: Tue, 17 Aug 2021 15:30:46 +0100 Subject: [PATCH] RevisionsPopover: Refactor and support speculative results (#23973) --- .../branded/src/components/BrandedStory.tsx | 15 +- client/shared/src/graphql/cache.ts | 21 +- client/shared/src/testing/apollo.ts | 7 - client/shared/src/testing/apollo.tsx | 34 + .../src/apollo/MockedStoryProvider.tsx | 18 +- .../web/src/components/ConnectionPopover.scss | 3 + .../FilteredConnection/ConnectionNodes.tsx | 5 +- .../FilteredConnection/FilteredConnection.tsx | 14 +- .../hooks/useConnection.test.tsx | 8 +- .../FilteredConnection/hooks/useConnection.ts | 10 +- .../FilteredConnection/ui/ConnectionForm.tsx | 10 +- .../ui/ConnectionSummary.tsx | 10 +- client/web/src/components/WebStory.tsx | 9 +- .../CodeMonitoringPage.test.tsx.snap | 7284 +++++++++-------- .../components/form/form-input/FormInput.tsx | 17 +- .../src/nav/Feedback/FeedbackPrompt.test.tsx | 24 +- client/web/src/repo/GitReference.tsx | 87 +- .../web/src/repo/RepoRevisionContainer.scss | 2 +- client/web/src/repo/RepoRevisionContainer.tsx | 19 +- client/web/src/repo/RevisionsPopover.tsx | 306 - .../RevisionsPopover.mocks.ts | 378 + .../RevisionsPopover.scss | 2 +- .../RevisionsPopover.story.tsx | 38 + .../RevisionsPopover.test.tsx | 223 + .../RevisionsPopover/RevisionsPopover.tsx | 129 + .../RevisionsPopoverCommits.tsx | 200 + .../RevisionsPopoverReferences.tsx | 209 + .../RevisionsPopover/RevisionsPopoverTab.tsx | 52 + client/web/src/repo/RevisionsPopover/index.ts | 1 + .../profile/UserSettingsProfilePage.test.tsx | 7 +- client/wildcard/src/hooks/index.ts | 1 + client/wildcard/src/hooks/useAutoFocus.ts | 23 + doc/dev/background-information/web/graphql.md | 6 +- package.json | 1 + yarn.lock | 2 +- 35 files changed, 5086 insertions(+), 4089 deletions(-) delete mode 100644 client/shared/src/testing/apollo.ts create mode 100644 client/shared/src/testing/apollo.tsx delete mode 100644 client/web/src/repo/RevisionsPopover.tsx create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopover.mocks.ts rename client/web/src/repo/{ => RevisionsPopover}/RevisionsPopover.scss (95%) create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopover.story.tsx create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopover.test.tsx create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopover.tsx create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopoverCommits.tsx create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopoverReferences.tsx create mode 100644 client/web/src/repo/RevisionsPopover/RevisionsPopoverTab.tsx create mode 100644 client/web/src/repo/RevisionsPopover/index.ts create mode 100644 client/wildcard/src/hooks/useAutoFocus.ts diff --git a/client/branded/src/components/BrandedStory.tsx b/client/branded/src/components/BrandedStory.tsx index 127857afd5a..af842bba6d6 100644 --- a/client/branded/src/components/BrandedStory.tsx +++ b/client/branded/src/components/BrandedStory.tsx @@ -2,6 +2,7 @@ import React from 'react' import { MemoryRouter, MemoryRouterProps } from 'react-router' import { ThemeProps } from '@sourcegraph/shared/src/theme' +import { MockedStoryProvider, MockedStoryProviderProps } from '@sourcegraph/storybook/src/apollo/MockedStoryProvider' import { usePrependStyles } from '@sourcegraph/storybook/src/hooks/usePrependStyles' import { useTheme } from '@sourcegraph/storybook/src/hooks/useTheme' @@ -9,7 +10,7 @@ import brandedStyles from '../global-styles/index.scss' import { Tooltip } from './tooltip/Tooltip' -export interface BrandedProps extends MemoryRouterProps { +export interface BrandedProps extends MemoryRouterProps, Pick { children: React.FunctionComponent styles?: string } @@ -21,15 +22,19 @@ export interface BrandedProps extends MemoryRouterProps { export const BrandedStory: React.FunctionComponent = ({ children: Children, styles = brandedStyles, + mocks, + useStrictMocking, ...memoryRouterProps }) => { const isLightTheme = useTheme() usePrependStyles('branded-story-styles', styles) return ( - - - - + + + + + + ) } diff --git a/client/shared/src/graphql/cache.ts b/client/shared/src/graphql/cache.ts index 5e21690d58f..e31b36b2893 100644 --- a/client/shared/src/graphql/cache.ts +++ b/client/shared/src/graphql/cache.ts @@ -4,8 +4,21 @@ import { TypedTypePolicies } from '../graphql-operations' // Defines how the Apollo cache interacts with our GraphQL schema. // See https://www.apollographql.com/docs/react/caching/cache-configuration/#typepolicy-fields -const typePolicies: TypedTypePolicies = {} +const typePolicies: TypedTypePolicies = { + Query: { + fields: { + node: { + // Node is a top-level interface field used to easily fetch from different parts of the schema through the relevant `id`. + // We always want to merge responses from this field as it will be used through very different queries. + merge: true, + }, + }, + }, +} -export const cache = new InMemoryCache({ - typePolicies, -}) +export const generateCache = (): InMemoryCache => + new InMemoryCache({ + typePolicies, + }) + +export const cache = generateCache() diff --git a/client/shared/src/testing/apollo.ts b/client/shared/src/testing/apollo.ts deleted file mode 100644 index e5cf6fefa18..00000000000 --- a/client/shared/src/testing/apollo.ts +++ /dev/null @@ -1,7 +0,0 @@ -import { act } from '@testing-library/react' - -/* - * Wait one tick to load the next response from Apollo - * https://www.apollographql.com/docs/react/development-testing/testing/#testing-the-success-state - */ -export const waitForNextApolloResponse = (): Promise => act(() => new Promise(resolve => setTimeout(resolve, 0))) diff --git a/client/shared/src/testing/apollo.tsx b/client/shared/src/testing/apollo.tsx new file mode 100644 index 00000000000..22abc04a693 --- /dev/null +++ b/client/shared/src/testing/apollo.tsx @@ -0,0 +1,34 @@ +import { MockedProvider, MockedProviderProps } from '@apollo/client/testing' +import { act } from '@testing-library/react' +import React, { useMemo } from 'react' + +import { generateCache } from '../graphql/cache' + +/* + * Wait one tick to load the next response from Apollo + * https://www.apollographql.com/docs/react/development-testing/testing/#testing-the-success-state + */ +export const waitForNextApolloResponse = (): Promise => act(() => new Promise(resolve => setTimeout(resolve, 0))) + +export const MockedTestProvider: React.FunctionComponent = ({ children, ...props }) => { + /** + * Generate a fresh cache for each instance of MockedTestProvider. + * Important to ensure tests don't share cached data. + */ + const cache = useMemo(() => generateCache(), []) + + return ( + + {children} + + ) +} diff --git a/client/storybook/src/apollo/MockedStoryProvider.tsx b/client/storybook/src/apollo/MockedStoryProvider.tsx index 77ddc7f678a..31e1150b6ad 100644 --- a/client/storybook/src/apollo/MockedStoryProvider.tsx +++ b/client/storybook/src/apollo/MockedStoryProvider.tsx @@ -3,6 +3,8 @@ import { MockedProvider, MockedProviderProps, MockedResponse, MockLink } from '@ import { getOperationName } from '@apollo/client/utilities' import React from 'react' +import { cache } from '@sourcegraph/shared/src/graphql/cache' + /** * Intercept each mocked Apollo request and ensure that any request variables match the specified mock. * This effectively means we are mocking agains the operationName of the query being fired. @@ -18,20 +20,32 @@ const forceMockVariablesLink = (mocks: readonly MockedResponse[]): ApolloLink => return forward(operation) }) +export interface MockedStoryProviderProps extends MockedProviderProps { + /** + * Set this to `true` to preserve the default behavior of MockedProvider. + * Requests will require that both the `operationName` **and** `variables` match the mock to be resolved. + */ + useStrictMocking?: boolean +} + /** * A wrapper around MockedProvider with a custom ApolloLink to ensure flexible request mocking. * * MockedProvider does not support dynamic variable matching for mocks. * This wrapper **only** mocks against the operation name, the specific provided variables are not used to match against a mock. */ -export const MockedStoryProvider: React.FunctionComponent = ({ +export const MockedStoryProvider: React.FunctionComponent = ({ children, mocks = [], + useStrictMocking, ...props }) => ( {children} diff --git a/client/web/src/components/ConnectionPopover.scss b/client/web/src/components/ConnectionPopover.scss index 98ea94610bd..be682488d85 100644 --- a/client/web/src/components/ConnectionPopover.scss +++ b/client/web/src/components/ConnectionPopover.scss @@ -34,6 +34,9 @@ } &__nodes { + &:empty { + display: none; + } .theme-redesign & { border-top: solid 1px var(--border-color-2); padding-top: 0.25rem; diff --git a/client/web/src/components/FilteredConnection/ConnectionNodes.tsx b/client/web/src/components/FilteredConnection/ConnectionNodes.tsx index 21786a98044..ac4f3f03039 100644 --- a/client/web/src/components/FilteredConnection/ConnectionNodes.tsx +++ b/client/web/src/components/FilteredConnection/ConnectionNodes.tsx @@ -73,7 +73,7 @@ export interface ConnectionNodesDisplayProps { noSummaryIfAllNodesVisible?: boolean /** The component displayed when the list of nodes is empty. */ - emptyElement?: JSX.Element + emptyElement?: JSX.Element | null /** The component displayed when all nodes have been fetched. */ totalCountSummaryComponent?: React.ComponentType<{ totalCount: number }> @@ -133,11 +133,10 @@ export const ConnectionNodes = , N, NP = {}, HP = {}>({ }: ConnectionNodesProps): JSX.Element => { const nextPage = hasNextPage(connection) - const totalCount = getTotalCount(connection, first) const summary = ( 0 && // this.props.hideControlsWhenEmpty - const compactnessClass = `filtered-connection--${this.props.compact ? 'compact' : 'noncompact'}` return ( -
+ { /* shouldShowControls && */ (!this.props.hideSearch || this.props.filters) && ( )} {this.state.loading && } -
+ ) } diff --git a/client/web/src/components/FilteredConnection/hooks/useConnection.test.tsx b/client/web/src/components/FilteredConnection/hooks/useConnection.test.tsx index 904a8972161..aa14508a3da 100644 --- a/client/web/src/components/FilteredConnection/hooks/useConnection.test.tsx +++ b/client/web/src/components/FilteredConnection/hooks/useConnection.test.tsx @@ -1,9 +1,9 @@ -import { MockedProvider, MockedResponse } from '@apollo/client/testing' +import { MockedResponse } from '@apollo/client/testing' import { fireEvent } from '@testing-library/react' import React from 'react' import { dataOrThrowErrors, getDocumentNode, gql } from '@sourcegraph/shared/src/graphql/graphql' -import { waitForNextApolloResponse } from '@sourcegraph/shared/src/testing/apollo' +import { MockedTestProvider, waitForNextApolloResponse } from '@sourcegraph/shared/src/testing/apollo' import { renderWithRouter, RenderWithRouterResult } from '@sourcegraph/shared/src/testing/render-with-router' import { @@ -148,9 +148,9 @@ describe('useConnection', () => { const renderWithMocks = async (mocks: MockedResponse[], route = '/') => { const renderResult = renderWithRouter( - + - , + , { route } ) diff --git a/client/web/src/components/FilteredConnection/hooks/useConnection.ts b/client/web/src/components/FilteredConnection/hooks/useConnection.ts index 13f954595cd..f4f7865080c 100644 --- a/client/web/src/components/FilteredConnection/hooks/useConnection.ts +++ b/client/web/src/components/FilteredConnection/hooks/useConnection.ts @@ -1,5 +1,4 @@ -import { QueryResult } from '@apollo/client' -import { GraphQLError } from 'graphql' +import { ApolloError, QueryResult } from '@apollo/client' import { useMemo, useRef } from 'react' import { GraphQLResult, useQuery } from '@sourcegraph/shared/src/graphql/graphql' @@ -10,9 +9,9 @@ import { Connection, ConnectionQueryArguments } from '../ConnectionType' import { useConnectionUrl } from './useConnectionUrl' -interface UseConnectionResult { +export interface UseConnectionResult { connection?: Connection - errors?: readonly GraphQLError[] + error?: ApolloError fetchMore: () => void loading: boolean hasNextPage: boolean @@ -93,6 +92,7 @@ export const useConnection = ({ ...variables, ...initialControls, }, + notifyOnNetworkStatusChange: true, // Ensures loading state is updated on `fetchMore` }) /** @@ -147,7 +147,7 @@ export const useConnection = ({ return { connection, loading, - errors: error?.graphQLErrors, + error, fetchMore: fetchMoreData, hasNextPage: connection ? hasNextPage(connection) : false, } diff --git a/client/web/src/components/FilteredConnection/ui/ConnectionForm.tsx b/client/web/src/components/FilteredConnection/ui/ConnectionForm.tsx index 5682160aeca..15056c02c33 100644 --- a/client/web/src/components/FilteredConnection/ui/ConnectionForm.tsx +++ b/client/web/src/components/FilteredConnection/ui/ConnectionForm.tsx @@ -1,7 +1,9 @@ import classNames from 'classnames' -import React, { useCallback } from 'react' +import React, { useCallback, useRef } from 'react' +import { useMergeRefs } from 'use-callback-ref' import { Form } from '@sourcegraph/branded/src/components/Form' +import { useAutoFocus } from '@sourcegraph/wildcard' import { FilterControl, FilteredConnectionFilter, FilteredConnectionFilterValue } from '../FilterControl' @@ -59,11 +61,15 @@ export const ConnectionForm = React.forwardRef { + const localReference = useRef(null) + const mergedReference = useMergeRefs([localReference, reference]) const handleSubmit = useCallback>(event => { // Do nothing. The handler will pick up any changes shortly. event.preventDefault() }, []) + useAutoFocus({ autoFocus, reference: localReference }) + return (
)} diff --git a/client/web/src/components/FilteredConnection/ui/ConnectionSummary.tsx b/client/web/src/components/FilteredConnection/ui/ConnectionSummary.tsx index be87f52060b..4980d7f0f96 100644 --- a/client/web/src/components/FilteredConnection/ui/ConnectionSummary.tsx +++ b/client/web/src/components/FilteredConnection/ui/ConnectionSummary.tsx @@ -2,7 +2,7 @@ import * as React from 'react' import { pluralize } from '@sourcegraph/shared/src/util/strings' -import { ConnectionNodesState, ConnectionProps } from '../ConnectionNodes' +import { ConnectionNodesState, ConnectionProps, getTotalCount } from '../ConnectionNodes' import { Connection } from '../ConnectionType' interface ConnectionNodesSummaryProps, N, NP = {}, HP = {}> @@ -14,13 +14,12 @@ interface ConnectionNodesSummaryProps, N, NP = {}, HP = | 'pluralNoun' | 'connectionQuery' | 'emptyElement' + | 'first' > { /** The fetched connection data or an error (if an error occurred). */ connection: C hasNextPage: boolean - - totalCount: number | null } /** @@ -31,12 +30,12 @@ export const ConnectionSummary = , N, NP = {}, HP = {}>( noSummaryIfAllNodesVisible, connection, hasNextPage, - totalCount, totalCountSummaryComponent: TotalCountSummaryComponent, noun, pluralNoun, connectionQuery, emptyElement, + first, }: ConnectionNodesSummaryProps): JSX.Element | null => { const shouldShowSummary = !noSummaryIfAllNodesVisible || connection.nodes.length === 0 || hasNextPage @@ -44,6 +43,9 @@ export const ConnectionSummary = , N, NP = {}, HP = {}>( return null } + // We cannot always rely on `connection.totalCount` to be returned, fallback to `connection.nodes.length` if possible. + const totalCount = getTotalCount(connection, first) + if (totalCount !== null && totalCount > 0 && TotalCountSummaryComponent) { return } diff --git a/client/web/src/components/WebStory.tsx b/client/web/src/components/WebStory.tsx index d0beb51345a..709e8118d5e 100644 --- a/client/web/src/components/WebStory.tsx +++ b/client/web/src/components/WebStory.tsx @@ -1,11 +1,10 @@ -import { MockedResponse } from '@apollo/client/testing' import React, { useMemo } from 'react' import { MemoryRouter, MemoryRouterProps, RouteComponentProps, withRouter } from 'react-router' import { Tooltip } from '@sourcegraph/branded/src/components/tooltip/Tooltip' import { NOOP_TELEMETRY_SERVICE, TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService' import { ThemeProps } from '@sourcegraph/shared/src/theme' -import { MockedStoryProvider } from '@sourcegraph/storybook/src/apollo/MockedStoryProvider' +import { MockedStoryProvider, MockedStoryProviderProps } from '@sourcegraph/storybook/src/apollo/MockedStoryProvider' import { usePrependStyles } from '@sourcegraph/storybook/src/hooks/usePrependStyles' import { useTheme } from '@sourcegraph/storybook/src/hooks/useTheme' @@ -13,12 +12,11 @@ import webStyles from '../SourcegraphWebApp.scss' import { BreadcrumbSetters, BreadcrumbsProps, useBreadcrumbs } from './Breadcrumbs' -export interface WebStoryProps extends MemoryRouterProps { +export interface WebStoryProps extends MemoryRouterProps, Pick { children: React.FunctionComponent< ThemeProps & BreadcrumbSetters & BreadcrumbsProps & TelemetryProps & RouteComponentProps > additionalWebStyles?: string - mocks?: readonly MockedResponse[] } /** @@ -29,6 +27,7 @@ export const WebStory: React.FunctionComponent = ({ children, additionalWebStyles, mocks, + useStrictMocking, ...memoryRouterProps }) => { const isLightTheme = useTheme() @@ -39,7 +38,7 @@ export const WebStory: React.FunctionComponent = ({ usePrependStyles('web-styles', webStyles) return ( - + -
- - } - first={10} - loading={false} - location="[Location path=/code-monitoring]" - noSummaryIfAllNodesVisible={true} - nodeComponent={[Function]} - nodeComponentProps={ - Object { - "isSiteAdminUser": false, - "location": "[Location path=/code-monitoring]", - "showCodeMonitoringTestEmailButton": false, - "toggleCodeMonitorEnabled": [Function], - } - } - noun="code monitor" - onShowMore={[Function]} - pluralNoun="code monitors" - query="" +
- -
- - - + + +
+ + } + first={10} + hasNextPage={false} + noSummaryIfAllNodesVisible={true} + noun="code monitor" + pluralNoun="code monitors" + /> +
+ + +
+
@@ -2366,146 +2370,16 @@ exports[`CodeMonitoringListPage Code monitoring page with less than 10 results 1 queryConnection={[Function]} useURLQuery={true} > -
- - } - first={10} - loading={false} - location="[Location path=/code-monitoring?visible=3]" - noSummaryIfAllNodesVisible={true} - nodeComponent={[Function]} - nodeComponentProps={ - Object { - "isSiteAdminUser": false, - "location": "[Location path=/code-monitoring?visible=3]", - "showCodeMonitoringTestEmailButton": false, - "toggleCodeMonitorEnabled": [Function], - } - } - noun="code monitor" - onShowMore={[Function]} - pluralNoun="code monitors" - query="" +
- -
- - -
    - -
    -
    -
    - -
    - New search result → Sends email notifications - -
    -
    -
    -
    - - - -
    - - - Edit - - -
    -
    -
    -     - -
    -
    -
    - -
    - New search result → Sends email notifications - -
    -
    -
    -
    - - - -
    - - - Edit - - -
    -
    -
    -     - + } + first={10} + loading={false} + location="[Location path=/code-monitoring?visible=3]" + noSummaryIfAllNodesVisible={true} + nodeComponent={[Function]} + nodeComponentProps={ + Object { + "isSiteAdminUser": false, + "location": "[Location path=/code-monitoring?visible=3]", + "showCodeMonitoringTestEmailButton": false, + "toggleCodeMonitorEnabled": [Function], + } + } + noun="code monitor" + onShowMore={[Function]} + pluralNoun="code monitors" + query="" + > + +
    + + + - - -
    - + - } - hasNextPage={false} - noSummaryIfAllNodesVisible={true} - noun="code monitor" - pluralNoun="code monitors" - totalCount={3} - /> -
    -     - -
    + } + showCodeMonitoringTestEmailButton={false} + toggleCodeMonitorEnabled={[Function]} + > +
    +
    +
    + +
    + New search result → Sends email notifications + +
    +
    +
    +
    + + + +
    + + + Edit + + +
    +
    +
    +     + +
    +
    +
    + +
    + New search result → Sends email notifications + +
    +
    +
    +
    + + + +
    + + + Edit + + +
    +
    +
    +     +
+ + +
+ + } + first={10} + hasNextPage={false} + noSummaryIfAllNodesVisible={true} + noun="code monitor" + pluralNoun="code monitors" + /> +
+ + +
+
@@ -3287,380 +3295,16 @@ exports[`CodeMonitoringListPage Code monitoring page with more than 10 results 1 queryConnection={[Function]} useURLQuery={true} > -
- - } - first={10} - loading={false} - location="[Location path=/code-monitoring?visible=12]" - noSummaryIfAllNodesVisible={true} - nodeComponent={[Function]} - nodeComponentProps={ - Object { - "isSiteAdminUser": false, - "location": "[Location path=/code-monitoring?visible=12]", - "showCodeMonitoringTestEmailButton": false, - "toggleCodeMonitorEnabled": [Function], - } - } - noun="code monitor" - onShowMore={[Function]} - pluralNoun="code monitors" - query="" +
- -
- - - + + +
- - -
- - -
+ + +
+ + +
+
diff --git a/client/web/src/insights/components/form/form-input/FormInput.tsx b/client/web/src/insights/components/form/form-input/FormInput.tsx index 1a5b1a751cb..bd9f8ec7cdb 100644 --- a/client/web/src/insights/components/form/form-input/FormInput.tsx +++ b/client/web/src/insights/components/form/form-input/FormInput.tsx @@ -1,8 +1,9 @@ import classnames from 'classnames' -import React, { useRef, forwardRef, InputHTMLAttributes, ReactNode, useEffect } from 'react' +import React, { useRef, forwardRef, InputHTMLAttributes, ReactNode } from 'react' import { useMergeRefs } from 'use-callback-ref' import { LoaderInput } from '@sourcegraph/branded/src/components/LoaderInput' +import { useAutoFocus } from '@sourcegraph/wildcard' import styles from './FormInput.module.scss' import { ForwardReferenceComponent } from './types' @@ -53,19 +54,7 @@ const FormInput = forwardRef((props, reference) => { const localReference = useRef(null) const mergedReference = useMergeRefs([localReference, reference]) - useEffect(() => { - if (autoFocus) { - // In some cases if form input has been rendered within reach/portal element - // react autoFocus set focus too early and in this case we have to - // call focus explicitly in the next tick to be sure that focus will be - // on input element. See reach/portal implementation and notice async way to - // render children in react portal component. - // https://github.com/reach/reach-ui/blob/0ae833201cf842fc00859612cfc6c30a593d593d/packages/portal/src/index.tsx#L45 - requestAnimationFrame(() => { - localReference.current?.focus() - }) - } - }, [autoFocus]) + useAutoFocus({ autoFocus, reference: localReference }) return (