Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix(plg): add links to the user nav dropdown #63462

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion client/web/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ ts_project(
"src/cody/management/UseCodyInEditorSection.tsx",
"src/cody/management/api/client.ts",
"src/cody/management/api/components/CodyProApiClient.ts",
"src/cody/management/api/react-query/QueryClientProvider.tsx",
"src/cody/management/api/react-query/CodyProApiProvider.tsx",
"src/cody/management/api/react-query/callCodyProApi.ts",
"src/cody/management/api/react-query/invites.ts",
"src/cody/management/api/react-query/queryKeys.ts",
Expand Down Expand Up @@ -281,6 +281,7 @@ ts_project(
"src/cody/upsell/vs-code.tsx",
"src/cody/useCodyChat.tsx",
"src/cody/useCodyIgnore.tsx",
"src/cody/useCodyProNavLinks.ts",
"src/cody/util.ts",
"src/cody/widgets/CodyRecipesWidget.tsx",
"src/cody/widgets/components/Recipe.tsx",
Expand Down Expand Up @@ -1887,6 +1888,7 @@ ts_project(
"src/codeintel/ReferencesPanel.test.tsx",
"src/cody/team/TeamMemberList.test.ts",
"src/cody/useCodyIgnore.test.ts",
"src/cody/useCodyProNavLinks.test.tsx",
"src/components/ErrorBoundary.test.tsx",
"src/components/FilteredConnection/FilteredConnection.test.tsx",
"src/components/FilteredConnection/hooks/usePageSwitcherPagination.test.tsx",
Expand Down
2 changes: 2 additions & 0 deletions client/web/src/LegacySourcegraphWebApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { WildcardThemeContext, type WildcardTheme } from '@sourcegraph/wildcard'
import { authenticatedUser as authenticatedUserSubject, authenticatedUserValue, type AuthenticatedUser } from './auth'
import { getWebGraphQLClient } from './backend/graphql'
import { isBatchChangesExecutionEnabled } from './batches'
import { CodyProApiProvider } from './cody/management/api/react-query/CodyProApiProvider'
import { ComponentsComposer } from './components/ComponentsComposer'
import { ErrorBoundary } from './components/ErrorBoundary'
import { FeatureFlagsLocalOverrideAgent } from './featureFlags/FeatureFlagsProvider'
Expand Down Expand Up @@ -258,6 +259,7 @@ export class LegacySourcegraphWebApp extends React.Component<StaticAppConfig, Le
<SearchResultsCacheProvider />,
<SearchQueryStateStoreProvider useSearchQueryState={useNavbarQueryState} />,
<LegacyRouteContextProvider context={legacyContext} />,
<CodyProApiProvider />,
/* eslint-enable react/no-children-prop, react/jsx-key */
]}
>
Expand Down
2 changes: 2 additions & 0 deletions client/web/src/SourcegraphWebApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import type { TelemetryV2Props } from '@sourcegraph/shared/src/telemetry'
import { WildcardThemeContext, type WildcardTheme } from '@sourcegraph/wildcard'

import { authenticatedUser as authenticatedUserSubject, type AuthenticatedUser, authenticatedUserValue } from './auth'
import { CodyProApiProvider } from './cody/management/api/react-query/CodyProApiProvider'
import { ComponentsComposer } from './components/ComponentsComposer'
import { ErrorBoundary, RouteError } from './components/ErrorBoundary'
import { FeatureFlagsLocalOverrideAgent } from './featureFlags/FeatureFlagsProvider'
Expand Down Expand Up @@ -283,6 +284,7 @@ export const SourcegraphWebApp: FC<SourcegraphWebAppProps> = props => {
...props,
}}
/>,
<CodyProApiProvider />,
/* eslint-enable react/no-children-prop, react/jsx-key */
]}
>
Expand Down
7 changes: 1 addition & 6 deletions client/web/src/cody/codyProRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { lazyComponent } from '@sourcegraph/shared/src/util/lazyComponent'

import { LegacyRoute, type LegacyLayoutRouteContext } from '../LegacyRouteContext'

import { QueryClientProvider } from './management/api/react-query/QueryClientProvider'
import { isEmbeddedCodyProUIEnabled } from './util'

export enum CodyProRoutes {
Expand Down Expand Up @@ -78,9 +77,5 @@ interface CodyProPageProps extends Pick<LegacyLayoutRouteContext, 'authenticated
*/
const CodyProPage: React.FC<CodyProPageProps> = props => {
const Component = routeComponents[props.path]
return (
<QueryClientProvider>
<Component authenticatedUser={props.authenticatedUser} telemetryRecorder={props.telemetryRecorder} />
</QueryClientProvider>
)
return <Component authenticatedUser={props.authenticatedUser} telemetryRecorder={props.telemetryRecorder} />
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { QueryClient, QueryClientProvider as ReactQueryClientProvider } from '@tanstack/react-query'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'

// Tweak the default queries and mutations behavior.
// See defaults here: https://tanstack.com/query/latest/docs/framework/react/guides/important-defaults
Expand All @@ -15,6 +15,10 @@ const queryClient = new QueryClient({
},
})

export const QueryClientProvider: React.FC<React.PropsWithChildren<{}>> = ({ children }) => (
<ReactQueryClientProvider client={queryClient}>{children}</ReactQueryClientProvider>
/**
* CodyProApiProvider wraps its children with the react-query QueryClientProvider.
* It is used to access the Cody Pro API and is only utilized on dotcom.
*/
export const CodyProApiProvider: React.FC<React.PropsWithChildren<{}>> = ({ children }) => (
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
)
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import type { LegacyLayoutRouteContext } from '../../../../LegacyRouteContext'
import { CodyProRoutes } from '../../../codyProRoutes'
import { PageHeaderIcon } from '../../../components/PageHeaderIcon'
import { USER_CODY_PLAN } from '../../../subscription/queries'
import { useCurrentSubscription } from '../../api/react-query/subscriptions'
import { useCurrentSubscription, useSubscriptionSummary } from '../../api/react-query/subscriptions'

import { InvoiceHistory } from './InvoiceHistory'
import { PaymentDetails } from './PaymentDetails'
Expand All @@ -38,6 +38,7 @@ const AuthenticatedCodySubscriptionManagePage: React.FC<Props> = ({ telemetryRec
error: useCodyPlanError,
data: userCodyPlanData,
} = useQuery<UserCodyPlanResult, UserCodyPlanVariables>(USER_CODY_PLAN, {})
const subscriptionSummaryQuery = useSubscriptionSummary()

useEffect(
function recordViewEvent() {
Expand All @@ -46,7 +47,7 @@ const AuthenticatedCodySubscriptionManagePage: React.FC<Props> = ({ telemetryRec
[telemetryRecorder]
)

if (userCodyPlanLoading) {
if (userCodyPlanLoading || subscriptionSummaryQuery.isLoading) {
return <LoadingSpinner />
}

Expand All @@ -55,18 +56,32 @@ const AuthenticatedCodySubscriptionManagePage: React.FC<Props> = ({ telemetryRec
return null
}

if (subscriptionSummaryQuery.isError) {
logger.error('Failed to fetch Cody subscription summary', subscriptionSummaryQuery.error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is just logging an error and returning null the right call here? Or should we have something like return <GenericErrorPage message="{...}"></GenericErrorPage>?

It just seems like this could lead to a situation where the user has a blank page or some awkward looking UI, which is a little worse than having a clear "error" state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrsmith, WDYT about throwing an error that is propagated to the closest ErrorBoundary and results in a page looking like this?

Screenshot 2024-06-26 at 12 26 22

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.error('Failed to fetch Cody subscription summary', subscriptionSummaryQuery.error)
throw new Error('Failed to fetch Cody subscription summary.')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's fine, then we may want to handle the other failed API calls inside components in a similar way (search).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @rrhyne

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that error message is fine for now but it would be better to update the text to be more end user friendly by removing “your site admin” and linking “Sourcegraph support” to the proper channel on discord, or the best place to resolve the error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SGTM. The "fall back to the default error handler" seems super reasonable. And I also like Rob's suggestions. (Which I'm guessing would just be within the <GenericErrorHandlerDodad> use slightly different wording if in dotcom mode.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing logger.error calls with error propagation to the nearest error boundary might not be sufficient.
For instance, a 404 error for the subscription summary should redirect the user to the "/cody/manage" page and possibly provide UI feedback indicating they tried to access the page they are not allowed to because of their team status or role on the team.
Additionally, I think we need a consistent approach to handling API call failures across the entire PLG flow. To address this comprehensively, I’ve created a dedicated issue, and I suggest we handle it in a separate PR.
WDYT, @chrsmith?

return null
}

const subscriptionData = userCodyPlanData?.currentUser?.codySubscription
if (!subscriptionData) {
logger.error('Cody subscription data is not available.')
return null
}

if (!subscriptionSummaryQuery.data) {
logger.error('Cody subscription summary is not available.')
return null
}

// This page only applies to users who have a Cody Pro subscription to manage.
// Otherwise, direct them to the ./new page to sign up.
if (subscriptionData.plan !== CodySubscriptionPlan.PRO) {
return <Navigate to={CodyProRoutes.NewProSubscription} replace={true} />
}

if (subscriptionSummaryQuery.data.userRole !== 'admin') {
return <Navigate to={CodyProRoutes.Manage} replace={true} />
}

return (
<Page className="d-flex flex-column">
<PageTitle title="Manage subscription" />
Expand Down
85 changes: 85 additions & 0 deletions client/web/src/cody/useCodyProNavLinks.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { renderHook } from '@testing-library/react'
import { vi, describe, afterEach, test, expect, beforeEach } from 'vitest'

import { CodyProRoutes } from './codyProRoutes'
import * as subscriptionQueries from './management/api/react-query/subscriptions'
import type { SubscriptionSummary } from './management/api/teamSubscriptions'
import { useCodyProNavLinks } from './useCodyProNavLinks'

describe('useCodyProNavLinks', () => {
const useSubscriptionSummaryMock = vi.spyOn(subscriptionQueries, 'useSubscriptionSummary')

afterEach(() => {
useSubscriptionSummaryMock.mockReset()
})

const mockSubscriptionSummary = (summary?: SubscriptionSummary): void => {
useSubscriptionSummaryMock.mockReturnValue({ data: summary } as ReturnType<
typeof subscriptionQueries.useSubscriptionSummary
>)
}

test('returns empty array if subscription summary is undefined', () => {
mockSubscriptionSummary()
const { result } = renderHook(() => useCodyProNavLinks())
expect(result.current).toHaveLength(0)
})

test('returns empty array user is not admin', () => {
const summary: SubscriptionSummary = {
teamId: '018ff1b3-118c-7789-82e4-ab9106eed204',
userRole: 'member',
teamCurrentMembers: 2,
teamMaxMembers: 6,
subscriptionStatus: 'active',
cancelAtPeriodEnd: false,
}
mockSubscriptionSummary(summary)
const { result } = renderHook(() => useCodyProNavLinks())
expect(result.current).toHaveLength(0)
})

describe('user is admin', () => {
const summary: SubscriptionSummary = {
teamId: '018ff1b3-118c-7789-82e4-ab9106eed204',
userRole: 'admin',
teamCurrentMembers: 2,
teamMaxMembers: 6,
subscriptionStatus: 'active',
cancelAtPeriodEnd: false,
}

const setUseEmbeddedUI = (useEmbeddedUI: boolean) => {
vi.stubGlobal('context', {
frontendCodyProConfig: {
stripePublishableKey: 'pk_test_123',
sscBaseUrl: '',
useEmbeddedUI,
},
})
}

beforeEach(() => {
vi.stubGlobal('context', {})
mockSubscriptionSummary(summary)
})

test.skip('returns links to subscription and team management pages if embedded UI is enabled', () => {
setUseEmbeddedUI(true)
const { result } = renderHook(() => useCodyProNavLinks())
expect(result.current).toHaveLength(2)
expect(result.current[0].label).toBe('Manage subscription')
expect(result.current[0].to).toBe(CodyProRoutes.SubscriptionManage)
expect(result.current[1].label).toBe('Manage team')
expect(result.current[1].to).toBe(CodyProRoutes.ManageTeam)
})

test('returns link to subscription management page if embedded UI is disabled', () => {
setUseEmbeddedUI(false)
const { result } = renderHook(() => useCodyProNavLinks())
expect(result.current).toHaveLength(1)
expect(result.current[0].label).toBe('Manage subscription')
expect(result.current[0].to).toBe('https://accounts.sourcegraph.com/cody/subscription')
})
})
})
23 changes: 23 additions & 0 deletions client/web/src/cody/useCodyProNavLinks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { useMemo } from 'react'

import { CodyProRoutes } from './codyProRoutes'
import { useSubscriptionSummary } from './management/api/react-query/subscriptions'
import { getManageSubscriptionPageURL, isEmbeddedCodyProUIEnabled } from './util'

export const useCodyProNavLinks = (): { to: string; label: string }[] => {
const { data } = useSubscriptionSummary()

return useMemo(() => {
if (!data || data.userRole !== 'admin') {
return []
}

const items = [{ to: getManageSubscriptionPageURL(), label: 'Manage subscription' }]

if (isEmbeddedCodyProUIEnabled()) {
items.push({ to: CodyProRoutes.ManageTeam, label: 'Manage team' })
}

return items
}, [data])
}
62 changes: 61 additions & 1 deletion client/web/src/nav/UserNavItem.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { render, screen } from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import { MemoryRouter } from 'react-router-dom'
import sinon from 'sinon'
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from 'vitest'
import { afterAll, beforeAll, afterEach, describe, expect, test, vi, beforeEach } from 'vitest'

import { NOOP_TELEMETRY_SERVICE } from '@sourcegraph/shared/src/telemetry/telemetryService'
import { MockedTestProvider } from '@sourcegraph/shared/src/testing/apollo'
import { AnchorLink, RouterLink, setLinkComponent } from '@sourcegraph/wildcard'
import { renderWithBrandedContext } from '@sourcegraph/wildcard/src/testing'

import * as codyProHooks from '../cody/useCodyProNavLinks'

import { UserNavItem, type UserNavItemProps } from './UserNavItem'

describe('UserNavItem', () => {
Expand All @@ -30,6 +32,14 @@ describe('UserNavItem', () => {
setLinkComponent(AnchorLink)
})

const useCodyProNavLinksMock = vi.spyOn(codyProHooks, 'useCodyProNavLinks')
beforeEach(() => {
useCodyProNavLinksMock.mockReturnValue([])
})
afterEach(() => {
useCodyProNavLinksMock.mockReset()
})

const USER: UserNavItemProps['authenticatedUser'] = {
username: 'alice',
displayName: 'alice doe',
Expand Down Expand Up @@ -100,4 +110,54 @@ describe('UserNavItem', () => {
expect(result.locationRef.entries.length).toBe(1)
expect(result.locationRef.entries.find(({ pathname }) => pathname.includes('sign-out'))).toBe(undefined)
})

describe('Cody Pro section', () => {
const setup = (isSourcegraphDotCom: boolean) => {
renderWithBrandedContext(
<MockedTestProvider>
<UserNavItem
showKeyboardShortcutsHelp={() => undefined}
authenticatedUser={USER}
isSourcegraphDotCom={isSourcegraphDotCom}
showFeedbackModal={() => undefined}
telemetryService={NOOP_TELEMETRY_SERVICE}
/>
</MockedTestProvider>
)
userEvent.click(screen.getByRole('button'))
}

describe('dotcom', () => {
test('renders provided links', () => {
const links = [
{ to: '/foo', label: 'Foo' },
{ to: '/bar', label: 'Bar' },
]
useCodyProNavLinksMock.mockReturnValue(links)
setup(true)

for (const link of links) {
const el = screen.getByText(link.label)
expect(el).toHaveAttribute('href', link.to)
}
})

test('is not rendered if no links provided', () => {
useCodyProNavLinksMock.mockReturnValue([])
setup(true)

expect(useCodyProNavLinksMock).toHaveBeenCalled()
expect(screen.queryByText('Cody Pro')).not.toBeInTheDocument()
})
})

describe('enterprise', () => {
test('is not rendered', () => {
setup(false)

// Cody Pro section is not rendered thus useCodyProNavLinks hook is not called
expect(useCodyProNavLinksMock).not.toHaveBeenCalled()
})
})
})
})
23 changes: 23 additions & 0 deletions client/web/src/nav/UserNavItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {

import type { AuthenticatedUser } from '../auth'
import { CodyProRoutes } from '../cody/codyProRoutes'
import { useCodyProNavLinks } from '../cody/useCodyProNavLinks'
import { PageRoutes } from '../routes.constants'
import { enableDevSettings, isSourcegraphDev, useDeveloperSettings } from '../stores'

Expand Down Expand Up @@ -132,6 +133,7 @@ export const UserNavItem: FC<UserNavItemProps> = props => {
<MenuHeader className={styles.dropdownHeader}>
Signed in as <strong>@{authenticatedUser.username}</strong>
</MenuHeader>
{isSourcegraphDotCom && <CodyProSection />}
<MenuDivider className={styles.dropdownDivider} />
<MenuLink as={Link} to={authenticatedUser.settingsURL!}>
Settings
Expand Down Expand Up @@ -251,3 +253,24 @@ export const UserNavItem: FC<UserNavItemProps> = props => {
</>
)
}

const CodyProSection: React.FC = () => {
const links = useCodyProNavLinks()

if (!links.length) {
return null
}

return (
<>
<MenuDivider className={styles.dropdownDivider} />
<MenuHeader className={styles.dropdownHeader}>Cody PRO</MenuHeader>

{links.map(({ to, label }) => (
<MenuLink as={Link} key={to} to={to}>
{label}
</MenuLink>
))}
</>
)
}
Loading