diff --git a/plugins/backstage-plugin-coder/src/api/queryOptions.ts b/plugins/backstage-plugin-coder/src/api/queryOptions.ts index 4e55861d..6bfbd800 100644 --- a/plugins/backstage-plugin-coder/src/api/queryOptions.ts +++ b/plugins/backstage-plugin-coder/src/api/queryOptions.ts @@ -4,7 +4,10 @@ import type { CoderWorkspacesConfig } from '../hooks/useCoderWorkspacesConfig'; import type { BackstageCoderSdk } from './CoderClient'; import type { CoderAuth } from '../components/CoderProvider'; -export const CODER_QUERY_KEY_PREFIX = 'coder-backstage-plugin'; +// Making the type more broad to hide some implementation details from the end +// user; the prefix should be treated as an opaque string we can change whenever +// we want +export const CODER_QUERY_KEY_PREFIX = 'coder-backstage-plugin' as string; // Defined here and not in CoderAuthProvider.ts to avoid circular dependency // issues diff --git a/plugins/backstage-plugin-coder/src/api/vendoredSdk/api/api.ts b/plugins/backstage-plugin-coder/src/api/vendoredSdk/api/api.ts index e0eafd1d..bf293267 100644 --- a/plugins/backstage-plugin-coder/src/api/vendoredSdk/api/api.ts +++ b/plugins/backstage-plugin-coder/src/api/vendoredSdk/api/api.ts @@ -312,7 +312,7 @@ type RestartWorkspaceParameters = Readonly<{ export type DeleteWorkspaceOptions = Pick< TypesGen.CreateWorkspaceBuildRequest, - 'log_level' & 'orphan' + 'log_level' | 'orphan' >; type Claims = { diff --git a/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderAuthProvider.tsx b/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderAuthProvider.tsx index 664bb311..33b5bc0a 100644 --- a/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderAuthProvider.tsx +++ b/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderAuthProvider.tsx @@ -1,4 +1,5 @@ import React, { + type FC, type PropsWithChildren, createContext, useCallback, @@ -136,10 +137,16 @@ function useAuthState(): CoderAuth { return () => window.clearTimeout(distrustTimeoutId); }, [authState.status]); + const isAuthenticated = validAuthStatuses.includes(authState.status); + // Sets up subscription to spy on potentially-expired tokens. Can't do this // outside React because we let the user connect their own queryClient const queryClient = useQueryClient(); useEffect(() => { + if (!isAuthenticated) { + return undefined; + } + // Pseudo-mutex; makes sure that if we get a bunch of errors, only one // revalidation will be processed at a time let isRevalidatingToken = false; @@ -163,7 +170,7 @@ function useAuthState(): CoderAuth { const queryCache = queryClient.getQueryCache(); const unsubscribe = queryCache.subscribe(revalidateTokenOnError); return unsubscribe; - }, [queryClient]); + }, [queryClient, isAuthenticated]); const registerNewToken = useCallback((newToken: string) => { if (newToken !== '') { @@ -179,7 +186,7 @@ function useAuthState(): CoderAuth { return { ...authState, - isAuthenticated: validAuthStatuses.includes(authState.status), + isAuthenticated, registerNewToken, ejectToken, }; @@ -607,24 +614,75 @@ export const dummyTrackComponent: TrackComponent = () => { }; }; +export type FallbackAuthInputBehavior = 'restrained' | 'assertive' | 'hidden'; +type AuthFallbackProvider = FC< + Readonly< + PropsWithChildren<{ + isAuthenticated: boolean; + }> + > +>; + +// Matches each behavior for the fallback auth UI to a specific provider. This +// is screwy code, but by doing this, we ensure that if the user chooses not to +// have a dynamic auth fallback UI, their app will have far less tracking logic, +// meaning less performance overhead and fewer re-renders from something the +// user isn't even using +const fallbackProviders = { + hidden: ({ children }) => ( + + {children} + + ), + + assertive: ({ children, isAuthenticated }) => ( + // Don't need the live version of the tracker function if we're always + // going to be showing the fallback auth input no matter what + + {children} + {!isAuthenticated && } + + ), + + // Have to give function a name to satisfy ES Lint (rules of hooks) + restrained: function Restrained({ children, isAuthenticated }) { + const { hasNoAuthInputs, trackComponent } = useAuthFallbackState(); + const needFallbackUi = !isAuthenticated && hasNoAuthInputs; + + return ( + <> + + {children} + + + {needFallbackUi && ( + + + + )} + + ); + }, +} as const satisfies Record; + +export type CoderAuthProviderProps = Readonly< + PropsWithChildren<{ + fallbackAuthUiMode?: FallbackAuthInputBehavior; + }> +>; + export function CoderAuthProvider({ children, -}: Readonly>) { + fallbackAuthUiMode = 'restrained', +}: CoderAuthProviderProps) { const authState = useAuthState(); - const { hasNoAuthInputs, trackComponent } = useAuthFallbackState(); - const needFallbackUi = !authState.isAuthenticated && hasNoAuthInputs; + const AuthFallbackProvider = fallbackProviders[fallbackAuthUiMode]; return ( - + {children} - - - {needFallbackUi && ( - - - - )} + ); } diff --git a/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.test.tsx b/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.test.tsx index 73acc13c..382917d8 100644 --- a/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.test.tsx +++ b/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.test.tsx @@ -86,6 +86,7 @@ describe(`${CoderProvider.name}`, () => { {children} diff --git a/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.tsx b/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.tsx index 1b825404..fd562851 100644 --- a/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.tsx +++ b/plugins/backstage-plugin-coder/src/components/CoderProvider/CoderProvider.tsx @@ -46,12 +46,15 @@ export const CoderProvider = ({ children, appConfig, queryClient = defaultClient, + fallbackAuthUiMode = 'restrained', }: CoderProviderProps) => { return ( - {children} + + {children} + diff --git a/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/Root.tsx b/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/Root.tsx index 452f0a9c..5814d55b 100644 --- a/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/Root.tsx +++ b/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/Root.tsx @@ -16,7 +16,7 @@ import { type CoderWorkspacesConfig, } from '../../hooks/useCoderWorkspacesConfig'; import type { Workspace } from '../../api/vendoredSdk'; -import { useCoderWorkspacesQuery } from '../../hooks/useCoderWorkspacesQuery'; +import { useCoderWorkspacesQuery } from './useCoderWorkspacesQuery'; import { CoderAuthFormCardWrapper } from '../CoderAuthFormCardWrapper'; export type WorkspacesQuery = UseQueryResult; diff --git a/plugins/backstage-plugin-coder/src/hooks/useCoderWorkspacesQuery.test.ts b/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/useCoderWorkspacesQuery.test.ts similarity index 91% rename from plugins/backstage-plugin-coder/src/hooks/useCoderWorkspacesQuery.test.ts rename to plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/useCoderWorkspacesQuery.test.ts index 49535619..9f22cf94 100644 --- a/plugins/backstage-plugin-coder/src/hooks/useCoderWorkspacesQuery.test.ts +++ b/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/useCoderWorkspacesQuery.test.ts @@ -1,12 +1,11 @@ import { waitFor } from '@testing-library/react'; import { useCoderWorkspacesQuery } from './useCoderWorkspacesQuery'; - -import { renderHookAsCoderEntity } from '../testHelpers/setup'; -import { mockCoderWorkspacesConfig } from '../testHelpers/mockBackstageData'; +import { renderHookAsCoderEntity } from '../../testHelpers/setup'; +import { mockCoderWorkspacesConfig } from '../../testHelpers/mockBackstageData'; import { mockWorkspaceNoParameters, mockWorkspacesList, -} from '../testHelpers/mockCoderPluginData'; +} from '../../testHelpers/mockCoderPluginData'; beforeAll(() => { jest.useFakeTimers(); diff --git a/plugins/backstage-plugin-coder/src/hooks/useCoderWorkspacesQuery.ts b/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/useCoderWorkspacesQuery.ts similarity index 66% rename from plugins/backstage-plugin-coder/src/hooks/useCoderWorkspacesQuery.ts rename to plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/useCoderWorkspacesQuery.ts index 63b4f2f7..5f82e6b7 100644 --- a/plugins/backstage-plugin-coder/src/hooks/useCoderWorkspacesQuery.ts +++ b/plugins/backstage-plugin-coder/src/components/CoderWorkspacesCard/useCoderWorkspacesQuery.ts @@ -1,8 +1,8 @@ import { useQuery } from '@tanstack/react-query'; -import { workspaces, workspacesByRepo } from '../api/queryOptions'; -import type { CoderWorkspacesConfig } from './useCoderWorkspacesConfig'; -import { useCoderSdk } from './useCoderSdk'; -import { useInternalCoderAuth } from '../components/CoderProvider'; +import { workspaces, workspacesByRepo } from '../../api/queryOptions'; +import type { CoderWorkspacesConfig } from '../../hooks/useCoderWorkspacesConfig'; +import { useCoderSdk } from '../../hooks/useCoderSdk'; +import { useInternalCoderAuth } from '../../components/CoderProvider'; type QueryInput = Readonly<{ coderQuery: string; diff --git a/plugins/backstage-plugin-coder/src/hooks/reactQueryWrappers.test.tsx b/plugins/backstage-plugin-coder/src/hooks/reactQueryWrappers.test.tsx new file mode 100644 index 00000000..83309a08 --- /dev/null +++ b/plugins/backstage-plugin-coder/src/hooks/reactQueryWrappers.test.tsx @@ -0,0 +1,248 @@ +import React from 'react'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import type { + QueryClient, + QueryKey, + UseQueryResult, +} from '@tanstack/react-query'; +import { + type UseCoderQueryOptions, + useCoderQuery, + CoderQueryFunction, +} from './reactQueryWrappers'; +import { + type CoderAuth, + CoderProvider, + useEndUserCoderAuth, +} from '../components/CoderProvider'; +import { + getMockApiList, + mockAppConfig, + mockCoderAuthToken, +} from '../testHelpers/mockBackstageData'; +import { + createInvertedPromise, + getMockQueryClient, +} from '../testHelpers/setup'; +import { TestApiProvider, wrapInTestApp } from '@backstage/test-utils'; +import { CODER_QUERY_KEY_PREFIX } from '../api/queryOptions'; +import { mockWorkspacesList } from '../testHelpers/mockCoderPluginData'; + +type RenderUseQueryOptions< + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Readonly<{ + authenticateOnMount?: boolean; + queryClient?: QueryClient; + queryOptions: UseCoderQueryOptions; +}>; + +async function renderCoderQuery< + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +>(options: RenderUseQueryOptions) { + const { + queryOptions, + authenticateOnMount = true, + queryClient = getMockQueryClient(), + } = options; + + let latestRegisterNewToken!: CoderAuth['registerNewToken']; + let latestEjectToken!: CoderAuth['ejectToken']; + const AuthEscapeHatch = () => { + const auth = useEndUserCoderAuth(); + latestRegisterNewToken = auth.registerNewToken; + latestEjectToken = auth.ejectToken; + + return null; + }; + + type Result = UseQueryResult; + const renderOutput = renderHook( + newOptions => useCoderQuery(newOptions), + { + initialProps: queryOptions, + wrapper: ({ children }) => { + const mainMarkup = ( + + + {children} + + + + ); + + return wrapInTestApp(mainMarkup) as unknown as typeof mainMarkup; + }, + }, + ); + + await waitFor(() => expect(renderOutput.result.current).not.toBeNull()); + + const registerMockToken = () => { + return act(() => latestRegisterNewToken(mockCoderAuthToken)); + }; + + const ejectToken = () => { + return act(() => latestEjectToken()); + }; + + if (authenticateOnMount) { + registerMockToken(); + } + + return { ...renderOutput, registerMockToken, ejectToken }; +} + +describe(`${useCoderQuery.name}`, () => { + /** + * Really wanted to make mock components for each test case, to simulate some + * of the steps of using the hook as an actual end-user, but the setup steps + * got to be a bit much, just because of all the dependencies to juggle. + * + * @todo Add a new describe block with custom components to mirror some + * example user flows + */ + describe('Hook functionality', () => { + it('Disables requests while user is not authenticated', async () => { + const { result, registerMockToken, ejectToken } = await renderCoderQuery({ + authenticateOnMount: false, + queryOptions: { + queryKey: ['workspaces'], + queryFn: ({ sdk }) => sdk.getWorkspaces({ q: 'owner:me' }), + select: response => response.workspaces, + }, + }); + + expect(result.current.isLoading).toBe(true); + registerMockToken(); + + await waitFor(() => { + expect(result.current.isLoading).toBe(false); + expect(result.current.isSuccess).toBe(true); + expect(result.current.data?.length).toBeGreaterThan(0); + }); + + ejectToken(); + await waitFor(() => expect(result.current.isLoading).toBe(true)); + }); + + it("Automatically prefixes queryKey with the global Coder query key prefix if it isn't already there", async () => { + // Have to escape out the key because useQuery doesn't expose any way to + // access the key after it's been processed into a query result object + let processedQueryKey: QueryKey | undefined = undefined; + + const queryFnWithEscape: CoderQueryFunction = ({ queryKey }) => { + processedQueryKey = queryKey; + return Promise.resolve(mockWorkspacesList); + }; + + // Verify that key is updated if the prefix isn't already there + const { unmount } = await renderCoderQuery({ + queryOptions: { + queryKey: ['blah'], + queryFn: queryFnWithEscape, + }, + }); + + await waitFor(() => { + expect(processedQueryKey).toEqual([ + CODER_QUERY_KEY_PREFIX, + 'blah', + ]); + }); + + // Unmounting shouldn't really be necessary, but it helps guarantee that + // there's never any risks of states messing with each other + unmount(); + + // Verify that the key is unchanged if the prefix is already present + await renderCoderQuery({ + queryOptions: { + queryKey: [CODER_QUERY_KEY_PREFIX, 'nah'], + queryFn: queryFnWithEscape, + }, + }); + + await waitFor(() => { + expect(processedQueryKey).toEqual([ + CODER_QUERY_KEY_PREFIX, + 'nah', + ]); + }); + }); + + it('Disables everything when the user unlinks their access token', async () => { + const { result, ejectToken } = await renderCoderQuery({ + queryOptions: { + queryKey: ['workspaces'], + queryFn: () => Promise.resolve(mockWorkspacesList), + }, + }); + + await waitFor(() => { + expect(result.current).toEqual( + expect.objectContaining>({ + isSuccess: true, + isPaused: false, + data: mockWorkspacesList, + }), + ); + }); + + ejectToken(); + + await waitFor(() => { + expect(result.current).toEqual( + expect.objectContaining>({ + isLoading: true, + isPaused: false, + data: undefined, + }), + ); + }); + }); + + /** + * In case the title isn't clear (had to rewrite it a bunch), the flow is: + * + * 1. User gets authenticated + * 2. User makes a request that will fail + * 3. Before the request comes back, the user revokes their authentication + * 4. The failed request comes back, which would normally add error state, + * and kick off a bunch of retry logic for React Query + * 5. But the hook should tell the Query Client NOT retry the request + * because the user is no longer authenticated + */ + it('Will not retry a request if it gets sent out while the user is authenticated, but then fails after the user revokes authentication', async () => { + const { promise, reject } = createInvertedPromise(); + const queryFn = jest.fn(() => promise); + + const { ejectToken } = await renderCoderQuery({ + queryOptions: { + queryFn, + queryKey: ['blah'], + + // From the end user's perspective, the query should always retry, but + // the hook should override that when the user isn't authenticated + retry: true, + }, + }); + + await waitFor(() => expect(queryFn).toHaveBeenCalled()); + ejectToken(); + + queryFn.mockRestore(); + act(() => reject(new Error("Don't feel like giving you data today"))); + expect(queryFn).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/plugins/backstage-plugin-coder/src/hooks/reactQueryWrappers.ts b/plugins/backstage-plugin-coder/src/hooks/reactQueryWrappers.ts new file mode 100644 index 00000000..6dff0240 --- /dev/null +++ b/plugins/backstage-plugin-coder/src/hooks/reactQueryWrappers.ts @@ -0,0 +1,157 @@ +/** + * @file Defines a couple of wrappers over React Query/Tanstack Query that make + * it easier to use the Coder SDK within UI logic. + * + * These hooks are designed 100% for end-users, and should not be used + * internally. Use useEndUserCoderAuth when working with auth logic within these + * hooks. + * + * --- + * @todo 2024-05-28 - This isn't fully complete until we have an equivalent + * wrapper for useMutation, and have an idea of how useCoderQuery and + * useCoderMutation can be used together. + * + * Making the useMutation wrapper shouldn't be hard, but you want some good + * integration tests to verify that the two hooks can satisfy common user flows. + */ +import { + type QueryFunctionContext, + type QueryKey, + type UseQueryOptions, + type UseQueryResult, + useQuery, + useQueryClient, +} from '@tanstack/react-query'; +import { DEFAULT_TANSTACK_QUERY_RETRY_COUNT } from '../typesConstants'; +import { useEndUserCoderAuth } from '../components/CoderProvider'; +import { CODER_QUERY_KEY_PREFIX } from '../api/queryOptions'; +import { useCoderSdk } from './useCoderSdk'; +import type { BackstageCoderSdk } from '../api/CoderClient'; + +export type CoderQueryFunctionContext = + QueryFunctionContext & { + sdk: BackstageCoderSdk; + }; + +export type CoderQueryFunction< + T = unknown, + TQueryKey extends QueryKey = QueryKey, +> = (context: CoderQueryFunctionContext) => Promise; + +export type UseCoderQueryOptions< + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Omit< + UseQueryOptions, + // queryFn omitted so that a custom version can be patched in; all other + // properties omitted because they are officially deprecated in React Query v4 + // and outright removed in v5. Want better future-proofing + 'queryFn' | 'isDataEqual' | 'onError' | 'onSuccess' | 'onSettled' +> & { + queryFn: CoderQueryFunction; +}; + +export function useCoderQuery< + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +>( + queryOptions: UseCoderQueryOptions, +): UseQueryResult { + const queryClient = useQueryClient(); + const { isAuthenticated } = useEndUserCoderAuth(); + const sdk = useCoderSdk(); + + let patchedQueryKey = queryOptions.queryKey; + if ( + patchedQueryKey === undefined || + patchedQueryKey[0] !== CODER_QUERY_KEY_PREFIX + ) { + const baseKey = + queryOptions.queryKey ?? queryClient.defaultQueryOptions().queryKey; + + if (baseKey === undefined) { + throw new Error('No queryKey value provided to useCoderQuery'); + } + + patchedQueryKey = [ + CODER_QUERY_KEY_PREFIX, + ...baseKey, + ] as QueryKey as TQueryKey; + } + + type Options = UseQueryOptions; + const patchedOptions: Options = { + ...queryOptions, + queryKey: patchedQueryKey, + enabled: isAuthenticated && (queryOptions.enabled ?? true), + keepPreviousData: + isAuthenticated && (queryOptions.keepPreviousData ?? false), + refetchIntervalInBackground: + isAuthenticated && (queryOptions.refetchIntervalInBackground ?? false), + + queryFn: async context => { + if (!isAuthenticated) { + throw new Error('Cannot complete request - user is not authenticated'); + } + + return queryOptions.queryFn({ ...context, sdk }); + }, + + refetchInterval: (data, query) => { + if (!isAuthenticated) { + return false; + } + + const externalRefetchInterval = queryOptions.refetchInterval; + if (typeof externalRefetchInterval !== 'function') { + return externalRefetchInterval ?? false; + } + + return externalRefetchInterval(data, query); + }, + + refetchOnMount: query => { + if (!isAuthenticated) { + return false; + } + + const externalRefetchOnMount = queryOptions.refetchOnMount; + if (typeof externalRefetchOnMount !== 'function') { + return externalRefetchOnMount ?? true; + } + + return externalRefetchOnMount(query); + }, + + retry: (failureCount, error) => { + if (!isAuthenticated) { + return false; + } + + const externalRetry = queryOptions.retry; + if (typeof externalRetry === 'number') { + const normalized = Number.isInteger(externalRetry) + ? Math.max(1, externalRetry) + : DEFAULT_TANSTACK_QUERY_RETRY_COUNT; + + return failureCount < normalized; + } + + if (typeof externalRetry !== 'function') { + // Could use the nullish coalescing operator here, but Prettier made the + // output hard to read + return externalRetry + ? externalRetry + : failureCount < DEFAULT_TANSTACK_QUERY_RETRY_COUNT; + } + + return externalRetry(failureCount, error); + }, + }; + + return useQuery(patchedOptions); +} diff --git a/plugins/backstage-plugin-coder/src/plugin.ts b/plugins/backstage-plugin-coder/src/plugin.ts index 2aaaab89..904b7705 100644 --- a/plugins/backstage-plugin-coder/src/plugin.ts +++ b/plugins/backstage-plugin-coder/src/plugin.ts @@ -192,6 +192,12 @@ export { useWorkspacesCardContext } from './components/CoderWorkspacesCard/Root' export { useCoderWorkspacesConfig } from './hooks/useCoderWorkspacesConfig'; export { useCoderSdk } from './hooks/useCoderSdk'; export { useEndUserCoderAuth as useCoderAuth } from './components/CoderProvider/CoderAuthProvider'; +export { useCoderQuery } from './hooks/reactQueryWrappers'; + +/** + * General constants + */ +export { CODER_QUERY_KEY_PREFIX } from './api/queryOptions'; /** * All custom types diff --git a/plugins/backstage-plugin-coder/src/testHelpers/setup.tsx b/plugins/backstage-plugin-coder/src/testHelpers/setup.tsx index 86ceedcb..cc8c67ad 100644 --- a/plugins/backstage-plugin-coder/src/testHelpers/setup.tsx +++ b/plugins/backstage-plugin-coder/src/testHelpers/setup.tsx @@ -106,7 +106,7 @@ export function getMockQueryClient(): QueryClient { } type MockAuthProps = Readonly< - CoderProviderProps & { + Omit & { auth?: CoderAuth; /** @@ -221,3 +221,24 @@ export async function renderInCoderEnvironment({ await waitFor(() => expect(loadingIndicator).not.toBeInTheDocument()); return renderOutput; } + +type InvertedPromiseResult = Readonly<{ + promise: Promise; + resolve: (value: TData) => void; + reject: (errorReason: TError) => void; +}>; + +export function createInvertedPromise< + TData = unknown, + TError = Error, +>(): InvertedPromiseResult { + let resolve!: (value: TData) => void; + let reject!: (error: TError) => void; + + const promise = new Promise((innerResolve, innerReject) => { + resolve = innerResolve; + reject = innerReject; + }); + + return { promise, resolve, reject }; +}