withAuthenticatedUser: use react-router-dom hooks & add test (#63906)

This improves the typings to remove some inscrutable inferred types and
some weird type errors if you didn't use React.PropsWithChildren. Also
refactors the code and exposes a new component AuthenticatedUserOnly
that's simpler and can be used when you don't need to do prop
propagation of authenticatedUser.

## Test plan

CI

client/web/BUILD.bazel

@@ -1894,6 +1894,7 @@ ts_project(
         "src/auth/RequestAccessPage.test.tsx",
         "src/auth/SignInPage.test.tsx",
         "src/auth/SignUpPage.test.tsx",
+        "src/auth/withAuthenticatedUser.test.tsx",
         "src/backend/persistenceMapper.test.ts",
         "src/codeintel/ReferencesPanel.mocks.ts",
         "src/codeintel/ReferencesPanel.test.tsx",

client/web/src/auth/withAuthenticatedUser.test.tsx

@@ -0,0 +1,112 @@
+import type { FunctionComponent } from 'react'
+
+import { render, screen } from '@testing-library/react'
+import { MemoryRouter, useLocation, useNavigate } from 'react-router-dom'
+import { describe, expect, it, vi } from 'vitest'
+
+import type { AuthenticatedUser } from '@sourcegraph/shared/src/auth'
+
+import { AuthenticatedUserOnly, withAuthenticatedUser } from './withAuthenticatedUser'
+
+vi.mock('react-router-dom', async () => {
+    const actual = await vi.importActual('react-router-dom')
+    return {
+        ...actual,
+        useNavigate: vi.fn(),
+        useLocation: vi.fn(),
+    }
+})
+
+const MockComponent: FunctionComponent<{
+    authenticatedUser: Pick<AuthenticatedUser, 'username'>
+    foo: string
+}> = ({ authenticatedUser, foo }) => (
+    <div>
+        Authenticated: {authenticatedUser.username} - {foo}
+    </div>
+)
+
+const WrappedComponent: FunctionComponent<{
+    authenticatedUser: Pick<AuthenticatedUser, 'username'> | null
+    foo: string
+}> = withAuthenticatedUser(MockComponent)
+
+describe('withAuthenticatedUser', () => {
+    it('renders the component when user is authenticated', () => {
+        render(<WrappedComponent authenticatedUser={{ username: 'testuser' }} foo="bar" />, {
+            wrapper: MemoryRouter,
+        })
+        expect(screen.getByText('Authenticated: testuser - bar')).toBeInTheDocument()
+    })
+
+    it('redirects to sign-in when user is not authenticated', () => {
+        const mockNavigate = vi.fn()
+        vi.mocked(useNavigate).mockReturnValue(mockNavigate)
+
+        vi.mocked(useLocation).mockReturnValue({
+            pathname: '/foo',
+            hash: '#h',
+            search: '?q',
+            key: '',
+            state: undefined,
+        })
+
+        const { container } = render(<WrappedComponent authenticatedUser={null} foo="bar" />, { wrapper: MemoryRouter })
+        expect(container).toBeEmptyDOMElement()
+        expect(mockNavigate).toHaveBeenCalledWith('/sign-in?returnTo=%2Ffoo%3Fq%23h', { replace: true })
+    })
+})
+
+// Test of typechecking only. We want to make sure that it's possible to use withAuthenticatedUser
+// with an arrow-expression component.
+// @ts-ignore unused
+const WrappedArrowComponent: FunctionComponent<{
+    authenticatedUser: Pick<AuthenticatedUser, 'username'> | null
+    foo: string
+}> = withAuthenticatedUser<
+    {
+        authenticatedUser: Pick<AuthenticatedUser, 'username'>
+        foo: string
+    },
+    Pick<AuthenticatedUser, 'username'>
+>(({ authenticatedUser, foo }) => (
+    <div>
+        Authenticated: {authenticatedUser.username} - {foo}
+    </div>
+))
+// @ts-ignore unused
+const WrappedArrowComponentWithoutAuthenticatedUserProp: FunctionComponent<{
+    authenticatedUser: Pick<AuthenticatedUser, 'username'> | null
+    foo: string
+}> = withAuthenticatedUser<{
+    foo: string
+    authenticatedUser: never
+}>(({ foo }) => <div>{foo}</div>)
+
+describe('AuthenticatedUserOnly', () => {
+    it('renders the component when user is authenticated', () => {
+        render(<AuthenticatedUserOnly authenticatedUser={{ id: 'u' }}>authed</AuthenticatedUserOnly>, {
+            wrapper: MemoryRouter,
+        })
+        expect(screen.getByText('authed')).toBeInTheDocument()
+    })
+
+    it('redirects to sign-in when user is not authenticated', () => {
+        const mockNavigate = vi.fn()
+        vi.mocked(useNavigate).mockReturnValue(mockNavigate)
+
+        vi.mocked(useLocation).mockReturnValue({
+            pathname: '/foo',
+            hash: '#h',
+            search: '?q',
+            key: '',
+            state: undefined,
+        })
+
+        const { container } = render(<AuthenticatedUserOnly authenticatedUser={null}>authed</AuthenticatedUserOnly>, {
+            wrapper: MemoryRouter,
+        })
+        expect(container).toBeEmptyDOMElement()
+        expect(mockNavigate).toHaveBeenCalledWith('/sign-in?returnTo=%2Ffoo%3Fq%23h', { replace: true })
+    })
+})

client/web/src/auth/withAuthenticatedUser.tsx

@@ -1,29 +1,65 @@
-import React from 'react'
+import React, { useEffect, type FunctionComponent, type PropsWithChildren } from 'react'
 
-import { Navigate } from 'react-router-dom'
+import { useLocation, useNavigate, type Location, type NavigateFunction } from 'react-router-dom'
 
 import type { AuthenticatedUser } from '../auth'
 
+interface WithAuthenticatedUserProps<U extends {} = AuthenticatedUser> {
+    authenticatedUser: U
+}
+
 /**
  * Wraps a React component and requires an authenticated user. If the viewer is not authenticated, it redirects to
  * the sign-in flow.
  */
-export const withAuthenticatedUser = <P extends object & { authenticatedUser: AuthenticatedUser }>(
-    Component: React.ComponentType<React.PropsWithChildren<P>>
-): React.ComponentType<
-    React.PropsWithChildren<
-        Pick<P, Exclude<keyof P, 'authenticatedUser'>> & { authenticatedUser: AuthenticatedUser | null }
-    >
-> =>
+export function withAuthenticatedUser<P extends WithAuthenticatedUserProps<U>, U extends {} = AuthenticatedUser>(
+    Component: React.ComponentType<P>
+): React.FunctionComponent<
+    Omit<P, 'authenticatedUser'> & {
+        authenticatedUser: U | null
+    }
+> {
     // It's important to add names to all components to avoid full reload on hot-update.
-    function WithAuthenticatedUser({ authenticatedUser, ...props }) {
-        // If not logged in, redirect to sign in.
-        if (!authenticatedUser) {
-            const newUrl = new URL(window.location.href)
-            newUrl.pathname = '/sign-in'
-            // Return to the current page after sign up/in.
-            newUrl.searchParams.set('returnTo', window.location.href)
-            return <Navigate to={newUrl.pathname + newUrl.search} replace={true} />
+    return function WithAuthenticatedUser({ authenticatedUser, ...props }) {
+        const navigate = useNavigate()
+        const location = useLocation()
+
+        if (useRedirectToSignIn(authenticatedUser, navigate, location)) {
+            return null
         }
+
         return <Component {...({ ...props, authenticatedUser } as P)} />
     }
+}
+
+export const AuthenticatedUserOnly: FunctionComponent<
+    PropsWithChildren<{
+        authenticatedUser: Pick<AuthenticatedUser, 'id'> | null
+    }>
+> = ({ authenticatedUser, children }) => {
+    const navigate = useNavigate()
+    const location = useLocation()
+
+    if (useRedirectToSignIn(authenticatedUser, navigate, location)) {
+        return null
+    }
+
+    return <>{children}</>
+}
+
+function useRedirectToSignIn<U extends {} = AuthenticatedUser>(
+    authenticatedUser: U | null,
+    navigate: NavigateFunction,
+    location: Location
+): boolean {
+    useEffect(() => {
+        // If not logged in, redirect to sign in.
+        if (!authenticatedUser) {
+            // Return to the current page after sign up/in.
+            const returnTo = `${location.pathname}${location.search}${location.hash}`
+            navigate(`/sign-in?returnTo=${encodeURIComponent(returnTo)}`, { replace: true })
+        }
+    }, [authenticatedUser, navigate, location])
+
+    return !authenticatedUser
+}

client/web/src/enterprise/searchContexts/CreateSearchContextPage.tsx

@@ -31,9 +31,7 @@ export interface CreateSearchContextPageProps
     isSourcegraphDotCom: boolean
 }
 
-export const AuthenticatedCreateSearchContextPage: React.FunctionComponent<
-    React.PropsWithChildren<CreateSearchContextPageProps>
-> = props => {
+export const AuthenticatedCreateSearchContextPage: React.FunctionComponent<CreateSearchContextPageProps> = props => {
     const { authenticatedUser, createSearchContext, platformContext } = props
 
     const location = useLocation()

client/web/src/enterprise/searchContexts/EditSearchContextPage.tsx

@@ -9,13 +9,13 @@ import { asError, isErrorLike } from '@sourcegraph/common'
 import type {
     Scalars,
     SearchContextEditInput,
-    SearchContextRepositoryRevisionsInput,
     SearchContextFields,
+    SearchContextRepositoryRevisionsInput,
 } from '@sourcegraph/shared/src/graphql-operations'
 import type { PlatformContextProps } from '@sourcegraph/shared/src/platform/context'
 import type { SearchContextProps } from '@sourcegraph/shared/src/search'
 import type { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
-import { PageHeader, LoadingSpinner, useObservable, Alert } from '@sourcegraph/wildcard'
+import { Alert, LoadingSpinner, PageHeader, useObservable } from '@sourcegraph/wildcard'
 
 import type { AuthenticatedUser } from '../../auth'
 import { withAuthenticatedUser } from '../../auth/withAuthenticatedUser'
@@ -32,9 +32,7 @@ export interface EditSearchContextPageProps
     isSourcegraphDotCom: boolean
 }
 
-export const AuthenticatedEditSearchContextPage: React.FunctionComponent<
-    React.PropsWithChildren<EditSearchContextPageProps>
-> = props => {
+export const AuthenticatedEditSearchContextPage: React.FunctionComponent<EditSearchContextPageProps> = props => {
     const LOADING = 'loading' as const
 
     const params = useParams()

client/web/src/savedSearches/Area.tsx

@@ -1,4 +1,4 @@
-import type { FunctionComponent, PropsWithChildren } from 'react'
+import type { FunctionComponent } from 'react'
 
 import { mdiPlus } from '@mdi/js'
 import { Route, Routes } from 'react-router-dom'
@@ -21,7 +21,7 @@ interface Props extends TelemetryV2Props {
     isSourcegraphDotCom: boolean
 }
 
-const AuthenticatedArea: FunctionComponent<PropsWithChildren<Props>> = ({ telemetryRecorder, isSourcegraphDotCom }) => (
+const AuthenticatedArea: FunctionComponent<Props> = ({ telemetryRecorder, isSourcegraphDotCom }) => (
     <Routes>
         <Route
             path=""