From 0b0e7cc61412feb1b75a75e31f3be59d59415c3b Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Mon, 23 Jan 2023 17:00:04 +0100 Subject: [PATCH] Remove createMemoryRouter from reference panel (#46718) Co-authored-by: Valery Bugakov --- .../src/codeintel/ReferencesPanel.mocks.ts | 33 +----- .../src/codeintel/ReferencesPanel.story.tsx | 37 +++--- .../src/codeintel/ReferencesPanel.test.tsx | 36 +++--- client/web/src/codeintel/ReferencesPanel.tsx | 107 ++++++++++-------- client/web/src/repo/blob/panel/BlobPanel.tsx | 8 +- package.json | 12 +- pnpm-lock.yaml | 21 ++-- 7 files changed, 123 insertions(+), 131 deletions(-) diff --git a/client/web/src/codeintel/ReferencesPanel.mocks.ts b/client/web/src/codeintel/ReferencesPanel.mocks.ts index ee962bda7d9..1a019ef6ee0 100644 --- a/client/web/src/codeintel/ReferencesPanel.mocks.ts +++ b/client/web/src/codeintel/ReferencesPanel.mocks.ts @@ -1,14 +1,10 @@ import { useState } from 'react' import { MockedResponse } from '@apollo/client/testing' -import { EMPTY, NEVER, noop, of, Subscription } from 'rxjs' +import { of } from 'rxjs' import { logger } from '@sourcegraph/common' import { getDocumentNode, dataOrThrowErrors, useQuery } from '@sourcegraph/http-client' -import { FlatExtensionHostAPI } from '@sourcegraph/shared/src/api/contract' -import { pretendProxySubscribable, pretendRemote } from '@sourcegraph/shared/src/api/util' -import { ViewerId } from '@sourcegraph/shared/src/api/viewerTypes' -import { Controller } from '@sourcegraph/shared/src/extensions/controller' import { NOOP_TELEMETRY_SERVICE } from '@sourcegraph/shared/src/telemetry/telemetryService' import { NOOP_PLATFORM_CONTEXT } from '@sourcegraph/shared/src/testing/searchTestHelpers' @@ -100,7 +96,7 @@ export function buildReferencePanelMocks(): ReferencePanelMock { const resolveRepoRevisionBlobVariables: ResolveRepoAndRevisionVariables = { repoName, filePath: path, - revision: '', + revision: commit, } const fetchHighlightedBlobVariables: ReferencesPanelHighlightedBlobVariables = { @@ -112,7 +108,7 @@ export function buildReferencePanelMocks(): ReferencePanelMock { } return { - url: `/${repoName}/-/blob/${path}?L${line}:${character}&subtree=true#tab=references`, + url: `/${repoName}@${commit}/-/blob/${path}?L${line}:${character}&subtree=true#tab=references`, requestMocks: [ { request: { @@ -304,6 +300,7 @@ const HIGHLIGHTED_FILE_MOCK = { commit: { id: 'R2l0Q29tbWl0OnsiciI6IlVtVndiM05wZEc5eWVUb3lNRFE9IiwiYyI6IjlkMWYzNTNhMjg1YjMwOTRiYzMzYmRhZTI3N2ExOWFlZGFiZThiNzEifQ==', blob: { + content: 'content', highlight: { aborted: false, html: highlightedDiffFileContent, @@ -319,26 +316,8 @@ const HIGHLIGHTED_FILE_MOCK = { }, } -const NOOP_EXTENSIONS_CONTROLLER: Controller = { - executeCommand: () => Promise.resolve(), - registerCommand: () => new Subscription(), - extHostAPI: Promise.resolve( - pretendRemote({ - getContributions: () => pretendProxySubscribable(NEVER), - registerContributions: () => pretendProxySubscribable(EMPTY).subscribe(noop as never), - haveInitialExtensionsLoaded: () => pretendProxySubscribable(of(true)), - addTextDocumentIfNotExists: () => {}, - addViewerIfNotExists: (): ViewerId => ({ viewerId: 'MOCK_VIEWER_ID' }), - setEditorSelections: () => {}, - removeViewer: () => {}, - }) - ), - commandErrors: EMPTY, - unsubscribe: noop, -} - -export const defaultProps: Omit = { - extensionsController: NOOP_EXTENSIONS_CONTROLLER, +export const defaultProps: ReferencesPanelProps = { + extensionsController: null, telemetryService: NOOP_TELEMETRY_SERVICE, settingsCascade: { subjects: null, diff --git a/client/web/src/codeintel/ReferencesPanel.story.tsx b/client/web/src/codeintel/ReferencesPanel.story.tsx index 4a6e0fd9154..64446850b78 100644 --- a/client/web/src/codeintel/ReferencesPanel.story.tsx +++ b/client/web/src/codeintel/ReferencesPanel.story.tsx @@ -1,26 +1,19 @@ import { Meta, Story } from '@storybook/react' -import * as H from 'history' import { MockedTestProvider } from '@sourcegraph/shared/src/testing/apollo' import { BrandedStory } from '@sourcegraph/wildcard/src/stories' -import { ReferencesPanelWithMemoryRouter } from './ReferencesPanel' +import { ReferencesPanel } from './ReferencesPanel' import { buildReferencePanelMocks, defaultProps } from './ReferencesPanel.mocks' import webStyles from '../SourcegraphWebApp.scss' const config: Meta = { title: 'wildcard/ReferencesPanel', - component: ReferencesPanelWithMemoryRouter, - - decorators: [ - story => ( - {() =>
{story()}
} - ), - ], + component: ReferencesPanel, parameters: { - component: ReferencesPanelWithMemoryRouter, + component: ReferencesPanel, chromatic: { disableSnapshot: false, }, @@ -32,19 +25,17 @@ export default config export const Simple: Story = () => { const { url, requestMocks } = buildReferencePanelMocks() - const fakeExternalHistory = H.createMemoryHistory() - fakeExternalHistory.push(url) - return ( -
- - - -
+ + {() => ( +
+
+ + + +
+
+ )} + ) } diff --git a/client/web/src/codeintel/ReferencesPanel.test.tsx b/client/web/src/codeintel/ReferencesPanel.test.tsx index 7037727aa93..1014d421319 100644 --- a/client/web/src/codeintel/ReferencesPanel.test.tsx +++ b/client/web/src/codeintel/ReferencesPanel.test.tsx @@ -1,35 +1,31 @@ -import { render, RenderResult, within, fireEvent } from '@testing-library/react' -import * as H from 'history' +import { within, fireEvent } from '@testing-library/react' import { MockedTestProvider, waitForNextApolloResponse } from '@sourcegraph/shared/src/testing/apollo' import '@sourcegraph/shared/dev/mockReactVisibilitySensor' +import { renderWithBrandedContext } from '@sourcegraph/wildcard/src/testing' -import { ReferencesPanelWithMemoryRouter } from './ReferencesPanel' +import { ReferencesPanel } from './ReferencesPanel' import { buildReferencePanelMocks, defaultProps } from './ReferencesPanel.mocks' describe('ReferencesPanel', () => { async function renderReferencesPanel() { const { url, requestMocks } = buildReferencePanelMocks() - const fakeExternalHistory = H.createMemoryHistory() - fakeExternalHistory.push(url) - - const result: RenderResult = render( + const result = renderWithBrandedContext( - - + + , + { route: url } ) + await waitForNextApolloResponse() await waitForNextApolloResponse() - return { result, externalHistory: fakeExternalHistory } + + return result } it('renders definitions correctly', async () => { - const { result } = await renderReferencesPanel() + const result = await renderReferencesPanel() expect(result.getByText('Definitions')).toBeVisible() @@ -43,7 +39,7 @@ describe('ReferencesPanel', () => { }) it('renders references correctly', async () => { - const { result } = await renderReferencesPanel() + const result = await renderReferencesPanel() expect(result.getByText('References')).toBeVisible() @@ -57,7 +53,7 @@ describe('ReferencesPanel', () => { }) it('renders a code view when clicking on a location', async () => { - const { result, externalHistory } = await renderReferencesPanel() + const { history, ...result } = await renderReferencesPanel() const definitionsList = result.getByTestId('definitions') const referencesList = result.getByTestId('references') @@ -99,11 +95,11 @@ describe('ReferencesPanel', () => { expect(codeView).toHaveTextContent('package diff import') // Assert the current URL points at the reference panel - expect(externalHistory.createHref(externalHistory.location)).toBe( - '/github.com/sourcegraph/go-diff/-/blob/diff/diff.go?L16:2&subtree=true#tab=references' + expect(history.createHref(history.location)).toBe( + '/github.com/sourcegraph/go-diff@9d1f353a285b3094bc33bdae277a19aedabe8b71/-/blob/diff/diff.go?L16:2&subtree=true#tab=references' ) // Click on reference the second time promotes the active location to the URL (and main blob view) fireEvent.click(referenceButton) - expect(externalHistory.createHref(externalHistory.location)).toBe(fullReferenceURL) + expect(history.createHref(history.location)).toBe(fullReferenceURL) }) }) diff --git a/client/web/src/codeintel/ReferencesPanel.tsx b/client/web/src/codeintel/ReferencesPanel.tsx index f40b36cd211..8dcc34a98bd 100644 --- a/client/web/src/codeintel/ReferencesPanel.tsx +++ b/client/web/src/codeintel/ReferencesPanel.tsx @@ -4,7 +4,7 @@ import { mdiArrowCollapseRight, mdiChevronDown, mdiChevronUp, mdiFilterOutline, import classNames from 'classnames' import * as H from 'history' import { capitalize, uniqBy } from 'lodash' -import { MemoryRouter, useLocation } from 'react-router' +import { useNavigate, useLocation } from 'react-router-dom-v5-compat' import { Observable, of } from 'rxjs' import { map } from 'rxjs/operators' @@ -96,37 +96,28 @@ export interface ReferencesPanelProps /** Whether to show the first loaded reference in mini code view */ jumpToFirst?: boolean - /** - * The panel runs inside its own MemoryRouter, we keep track of externalHistory - * so that we're still able to actually navigate within the browser when required - */ - externalHistory: H.History - externalLocation: H.Location - /** * Used to overwrite the initial active URL */ initialActiveURL?: string } +interface State { + repoName: string + revision?: string + filePath: string + line: number + character: number + jumpToFirst: boolean + collapsedState: { + references: boolean + definitions: boolean + implementations: boolean + } +} -export const ReferencesPanelWithMemoryRouter: React.FunctionComponent< - React.PropsWithChildren -> = props => ( - // TODO: this won't be working with Router V6 - - - -) - -const ReferencesPanel: React.FunctionComponent> = props => { - const location = useLocation() - +function createStateFromLocation(location: H.Location): null | State { const { hash, pathname, search } = location - const { line, character } = parseQueryAndHash(search, hash) + const { line, character, viewState } = parseQueryAndHash(search, hash) const { filePath, repoName, revision } = parseBrowserRepoURL(pathname) // If we don't have enough information in the URL, we can't render the panel @@ -137,9 +128,42 @@ const ReferencesPanel: React.FunctionComponent, we open it (likely because the + // user clicked on a link in the preview code blob) to show definitions. + if (!collapsedState.references && !collapsedState.definitions && !collapsedState.implementations) { + collapsedState.definitions = true + } - return + return { repoName, revision, filePath, line, character, jumpToFirst, collapsedState } +} + +export const ReferencesPanel: React.FunctionComponent> = props => { + // We store the state in a React state so that we do not update it when the + // URL changes. + const location = useLocation() + const [state] = useState(createStateFromLocation(location)) + + if (state === null) { + return null + } + + return ( + + ) } export const RevisionResolvingReferencesList: React.FunctionComponent< @@ -150,6 +174,7 @@ export const RevisionResolvingReferencesList: React.FunctionComponent< character: number filePath: string revision?: string + collapsedState: State['collapsedState'] } > > = props => { @@ -204,6 +229,7 @@ interface ReferencesPanelPropsWithToken extends ReferencesPanelProps { isArchived: boolean fileContent: string useCodeIntel: NonNullable + collapsedState: State['collapsedState'] } const SearchTokenFindingReferencesList: React.FunctionComponent< @@ -258,12 +284,15 @@ const ReferencesList: React.FunctionComponent< searchToken: string spec: LanguageSpec fileContent: string + collapsedState: State['collapsedState'] } > > = props => { const [filter, setFilter] = useState() const debouncedFilter = useDebounce(filter, 150) + const navigate = useNavigate() + useEffect(() => { setFilter(undefined) }, [props.token]) @@ -386,34 +415,18 @@ const ReferencesList: React.FunctionComponent< // points to the same line. In case they press "back" in the browser history, // the promoted line should be highlighted. setActiveURL(url) - props.externalHistory.push(url) + navigate(url) } const navigateToUrl = (url: string): void => { - props.externalHistory.push(url) + navigate(url) } - // Manual management of the open/closed state of collapsible lists so they - // stay open/closed across re-renders and re-mounts. - const location = useLocation() - const initialCollapseState = useMemo(() => { - const { viewState } = parseQueryAndHash(location.search, location.hash) - const state = { - references: viewState === 'references', - definitions: viewState === 'definitions', - implementations: viewState?.startsWith('implementations_') ?? false, - } - // If the URL doesn't contain tab=, we open it (likely because the - // user clicked on a link in the preview code blob) to show definitions. - if (!state.references && !state.definitions && !state.implementations) { - state.definitions = true - } - return state - }, [location]) const [collapsed, setCollapsed] = useSessionStorage>( 'sideblob-collapse-state-' + sessionStorageKeyFromToken(props.token), - initialCollapseState + props.collapsedState ) + const handleOpenChange = (id: string, isOpen: boolean): void => setCollapsed(previous => ({ ...previous, [id]: isOpen })) diff --git a/client/web/src/repo/blob/panel/BlobPanel.tsx b/client/web/src/repo/blob/panel/BlobPanel.tsx index 739bca1825f..542d8858ac8 100644 --- a/client/web/src/repo/blob/panel/BlobPanel.tsx +++ b/client/web/src/repo/blob/panel/BlobPanel.tsx @@ -26,7 +26,7 @@ import { AbsoluteRepoFile, ModeSpec, parseQueryAndHash, UIPositionSpec } from '@ import { useObservable } from '@sourcegraph/wildcard' import { CodeIntelligenceProps } from '../../../codeintel' -import { ReferencesPanelWithMemoryRouter } from '../../../codeintel/ReferencesPanel' +import { ReferencesPanel } from '../../../codeintel/ReferencesPanel' import { RepoRevisionSidebarCommits } from '../../RepoRevisionSidebarCommits' interface Props @@ -238,7 +238,7 @@ function useBlobPanelViews({ panelDefinitions.push({ id: 'references', provider: panelSubjectChanges.pipe( - map(({ position, history, location }) => ({ + map(({ position }) => ({ title: 'References', content: '', priority: 180, @@ -251,15 +251,13 @@ function useBlobPanelViews({ // This panel doesn't need a wrapper noWrapper: true, reactElement: position ? ( - diff --git a/package.json b/package.json index f6e9d5f66b8..0f4a9b215db 100644 --- a/package.json +++ b/package.json @@ -456,7 +456,7 @@ "react-resizable": "^3.0.4", "react-router": "^5.2.0", "react-router-dom": "^5.2.0", - "react-router-dom-v5-compat": "^6.3.0", + "react-router-dom-v5-compat": "^6.7.0", "react-spring": "^9.4.2", "react-sticky-box": "1.0.2", "react-visibility-sensor": "^5.1.1", @@ -487,6 +487,15 @@ "zustand": "^3.6.9" }, "packageManager": "pnpm@7.24.2", + "pnpm": { + "packageExtensions": { + "react-router-dom-v5-compat": { + "dependencies": { + "@remix-run/router": "*" + } + } + } + }, "resolutions": { "@types/webpack": "5", "history": "4.5.1", @@ -495,4 +504,5 @@ "tslib": "2.1.0", "jsprim": "1.4.2" } + } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 86c472fb598..ca75dc8cd4b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -8,6 +8,8 @@ overrides: tslib: 2.1.0 jsprim: 1.4.2 +packageExtensionsChecksum: 4b1de42ef6cd5dc58798836c97782882 + importers: .: @@ -339,7 +341,7 @@ importers: react-resizable: ^3.0.4 react-router: ^5.2.0 react-router-dom: ^5.2.0 - react-router-dom-v5-compat: ^6.3.0 + react-router-dom-v5-compat: ^6.7.0 react-spring: ^9.4.2 react-sticky-box: 1.0.2 react-visibility-sensor: ^5.1.1 @@ -518,7 +520,7 @@ importers: react-resizable: 3.0.4_ef5jwxihqo6n7gxfmzogljlgcm react-router: 5.2.0_react@18.1.0 react-router-dom: 5.2.0_react@18.1.0 - react-router-dom-v5-compat: 6.3.0_xjeikfjhclro5pp2abrahotxli + react-router-dom-v5-compat: 6.7.0_xjeikfjhclro5pp2abrahotxli react-spring: 9.4.2_ssv3vkwxg74iun7wj74vdfwzhu react-sticky-box: 1.0.2_react@18.1.0 react-visibility-sensor: 5.1.1_ef5jwxihqo6n7gxfmzogljlgcm @@ -26281,17 +26283,19 @@ packages: resize-observer-polyfill: 1.5.1 dev: false - /react-router-dom-v5-compat/6.3.0_xjeikfjhclro5pp2abrahotxli: - resolution: {integrity: sha512-lfnfDEAdfXf92VQQ692+aMFj9JKb73GZ7EbiQJeJT4sK+KUGvua3FGN2H8n2H5zSMaY1cvrihGiaI373cvv7OQ==} + /react-router-dom-v5-compat/6.7.0_xjeikfjhclro5pp2abrahotxli: + resolution: {integrity: sha512-pwK0hdpgbBnTxCv0/Yi1yCaVX0iglPxxDvuy6VK7MF/0q2MYEkqS186diGdktgXikKXmFMhhXR2h3IanBv0tAA==} + engines: {node: '>=14'} peerDependencies: react: '>=16.8' react-dom: '>=16.8' react-router-dom: 4 || 5 dependencies: + '@remix-run/router': 1.3.0 history: 4.5.1 react: 18.1.0 react-dom: 18.1.0_react@18.1.0 - react-router: 6.3.0_react@18.1.0 + react-router: 6.7.0_react@18.1.0 react-router-dom: 5.2.0_react@18.1.0 dev: false @@ -26340,12 +26344,13 @@ packages: tiny-warning: 1.0.3 dev: false - /react-router/6.3.0_react@18.1.0: - resolution: {integrity: sha512-7Wh1DzVQ+tlFjkeo+ujvjSqSJmkt1+8JO+T5xklPlgrh70y7ogx75ODRW0ThWhY7S+6yEDks8TYrtQe/aoboBQ==} + /react-router/6.7.0_react@18.1.0: + resolution: {integrity: sha512-KNWlG622ddq29MAM159uUsNMdbX8USruoKnwMMQcs/QWZgFUayICSn2oB7reHce1zPj6CG18kfkZIunSSRyGHg==} + engines: {node: '>=14'} peerDependencies: react: '>=16.8' dependencies: - history: 4.5.1 + '@remix-run/router': 1.3.0 react: 18.1.0 dev: false