diff --git a/frontend/common/utils/__tests__/shouldRedirectMissingEnvironment.test.ts b/frontend/common/utils/__tests__/shouldRedirectMissingEnvironment.test.ts new file mode 100644 index 000000000000..fd09ad628dd1 --- /dev/null +++ b/frontend/common/utils/__tests__/shouldRedirectMissingEnvironment.test.ts @@ -0,0 +1,116 @@ +import { + shouldRedirectMissingEnvironment, + type ShouldRedirectMissingEnvironmentArgs, +} from 'common/utils/shouldRedirectMissingEnvironment' + +const buildArgs = ( + overrides?: Partial, +): ShouldRedirectMissingEnvironmentArgs => ({ + envByIdError: undefined, + environmentId: 'real-key', + environmentsData: { results: [{ api_key: 'real-key' }] }, + hasEnvironmentId: true, + hasRedirected: false, + isLoadingEnvById: false, + isLoadingEnvironments: false, + ...overrides, +}) + +describe('shouldRedirectMissingEnvironment', () => { + it('returns false when the environmentId matches an entry in the loaded list', () => { + expect(shouldRedirectMissingEnvironment(buildArgs())).toBe(false) + }) + + it('returns true when the loaded environments list does not include the environmentId', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ + environmentId: 'undefined', + environmentsData: { results: [{ api_key: 'real-key' }] }, + }), + ) + + expect(result).toBe(true) + }) + + it('returns true when the per-environment query returns a 404', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ + envByIdError: { status: 404 }, + environmentId: 'stale-key', + environmentsData: { results: [{ api_key: 'real-key' }] }, + }), + ) + + expect(result).toBe(true) + }) + + it('returns false on non-404 errors (e.g. 500)', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ + envByIdError: { status: 500 }, + environmentsData: { results: [{ api_key: 'real-key' }] }, + }), + ) + + expect(result).toBe(false) + }) + + it('returns false while either query is still loading', () => { + const stillLoadingList = shouldRedirectMissingEnvironment( + buildArgs({ + environmentId: 'undefined', + environmentsData: undefined, + isLoadingEnvironments: true, + }), + ) + const stillLoadingEnvById = shouldRedirectMissingEnvironment( + buildArgs({ + envByIdError: { status: 404 }, + isLoadingEnvById: true, + }), + ) + + expect(stillLoadingList).toBe(false) + expect(stillLoadingEnvById).toBe(false) + }) + + it('returns false when the URL has no environmentId (e.g. project-level routes)', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ hasEnvironmentId: false }), + ) + + expect(result).toBe(false) + }) + + it('returns false when the project has zero environments (let create-flow handle it)', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ + environmentId: 'undefined', + environmentsData: { results: [] }, + }), + ) + + expect(result).toBe(false) + }) + + it('returns false after a redirect has already been issued (ref guard)', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ + envByIdError: { status: 404 }, + environmentId: 'undefined', + environmentsData: { results: [{ api_key: 'real-key' }] }, + hasRedirected: true, + }), + ) + + expect(result).toBe(false) + }) + + it('returns false for an undefined error object', () => { + const result = shouldRedirectMissingEnvironment( + buildArgs({ envByIdError: undefined }), + ) + + expect(result).toBe(false) + }) +}) diff --git a/frontend/common/utils/shouldRedirectMissingEnvironment.ts b/frontend/common/utils/shouldRedirectMissingEnvironment.ts new file mode 100644 index 000000000000..cf0426e6b053 --- /dev/null +++ b/frontend/common/utils/shouldRedirectMissingEnvironment.ts @@ -0,0 +1,68 @@ +/** + * Pure helper that decides whether the EnvironmentReadyChecker should redirect + * away from a URL whose environment identifier does not correspond to a real + * environment for the current project. See Flagsmith#7446. + * + * Both signals are combined defensively: + * + * - `envListLoadedAndMissing` is the fast path: once the environments list + * for the project has loaded, we can tell immediately that the URL's + * `environmentId` (treated as an `api_key`) is not in it. + * + * - `envByIdReturned404` is the belt-and-braces path: if the per-environment + * query returns a 404 (e.g. when the list cache is stale after permission + * changes), we redirect even when the list hasn't yet resolved. + * + * The helper does no IO and renders nothing — testing it does not require a + * DOM. The calling component is responsible for invoking `history.replace`. + */ + +type EnvironmentLike = { + api_key: string +} + +type EnvironmentsListLike = { + results?: EnvironmentLike[] +} + +export type ShouldRedirectMissingEnvironmentArgs = { + hasEnvironmentId: boolean + hasRedirected: boolean + isLoadingEnvironments: boolean + isLoadingEnvById: boolean + environmentsData: EnvironmentsListLike | undefined + environmentId: string | undefined + envByIdError: unknown +} + +const is404Error = (error: unknown): boolean => { + if (!error || typeof error !== 'object') { + return false + } + const status = (error as { status?: unknown }).status + return status === 404 +} + +export const shouldRedirectMissingEnvironment = ({ + envByIdError, + environmentId, + environmentsData, + hasEnvironmentId, + hasRedirected, + isLoadingEnvById, + isLoadingEnvironments, +}: ShouldRedirectMissingEnvironmentArgs): boolean => { + if (hasRedirected) return false + if (!hasEnvironmentId) return false + if (isLoadingEnvironments || isLoadingEnvById) return false + + const envList = environmentsData?.results ?? [] + const envListLoadedAndMissing = + !!environmentsData && + envList.length > 0 && + !envList.some((env) => env.api_key === environmentId) + + const envByIdReturned404 = is404Error(envByIdError) + + return envListLoadedAndMissing || envByIdReturned404 +} diff --git a/frontend/web/components/EnvironmentReadyChecker.tsx b/frontend/web/components/EnvironmentReadyChecker.tsx index 3541220ff4ff..495e890ce98c 100644 --- a/frontend/web/components/EnvironmentReadyChecker.tsx +++ b/frontend/web/components/EnvironmentReadyChecker.tsx @@ -1,10 +1,11 @@ -import { PropsWithChildren, useEffect, useState } from 'react' -import { useGetEnvironmentQuery } from 'common/services/useEnvironment' -import { useRouteMatch } from 'react-router-dom' - -interface RouteParams { - environmentId?: string -} +import { PropsWithChildren, useEffect, useRef, useState } from 'react' +import { useHistory } from 'react-router-dom' +import { + useGetEnvironmentQuery, + useGetEnvironmentsQuery, +} from 'common/services/useEnvironment' +import { useRouteContext } from './providers/RouteContext' +import { shouldRedirectMissingEnvironment } from 'common/utils/shouldRedirectMissingEnvironment' type EnvironmentReadyCheckerType = { children: React.ReactNode @@ -13,27 +14,69 @@ type EnvironmentReadyCheckerType = { const EnvironmentReadyChecker = ({ children, }: PropsWithChildren) => { - const match = useRouteMatch() + const history = useHistory() + const { environmentId, projectId } = useRouteContext() const [environmentCreated, setEnvironmentCreated] = useState(false) + const hasRedirected = useRef(false) - const { data, isLoading } = useGetEnvironmentQuery( - { - id: match?.params?.environmentId || '', - }, + // 'create' is the new-environment form route sentinel, not an env api_key. + const hasEnvironmentId = !!environmentId && environmentId !== 'create' + + const { + data, + error: envByIdError, + isLoading: isLoadingEnvById, + } = useGetEnvironmentQuery( + { id: environmentId || '' }, { pollingInterval: 1000, - skip: !match?.params?.environmentId || environmentCreated, + skip: !hasEnvironmentId || environmentCreated, }, ) + + const { data: environmentsData, isLoading: isLoadingEnvironments } = + useGetEnvironmentsQuery({ projectId: projectId ?? 0 }, { skip: !projectId }) + useEffect(() => { if (!!data && !data?.is_creating) { setEnvironmentCreated(true) } }, [data]) - if (!match?.params?.environmentId) { + + // Redirect to the project's default environment when the URL points at an + // environment that does not exist for this project. Fixes Flagsmith#7446. + useEffect(() => { + if (!projectId) return + + const shouldRedirect = shouldRedirectMissingEnvironment({ + envByIdError, + environmentId, + environmentsData, + hasEnvironmentId, + hasRedirected: hasRedirected.current, + isLoadingEnvById, + isLoadingEnvironments, + }) + + if (shouldRedirect) { + hasRedirected.current = true + history.replace(`/project/${projectId}/`) + } + }, [ + envByIdError, + environmentId, + environmentsData, + hasEnvironmentId, + history, + isLoadingEnvById, + isLoadingEnvironments, + projectId, + ]) + + if (!hasEnvironmentId) { return children } - if (isLoading) { + if (isLoadingEnvById || isLoadingEnvironments) { return (
@@ -51,6 +94,17 @@ const EnvironmentReadyChecker = ({
) } + // Keep the loader on screen while a redirect is in-flight or the + // env-by-id query has produced no data yet. Without this guard, children + // would mount with an invalid environmentId — the root cause of the + // infinite loader prior to this fix. + if (hasRedirected.current || !data) { + return ( +
+ +
+ ) + } return children } diff --git a/frontend/web/components/base/higher-order/ParameterizedRoute.tsx b/frontend/web/components/base/higher-order/ParameterizedRoute.tsx index 8a9b59cd9c13..4d04207786c0 100644 --- a/frontend/web/components/base/higher-order/ParameterizedRoute.tsx +++ b/frontend/web/components/base/higher-order/ParameterizedRoute.tsx @@ -12,7 +12,7 @@ export const ParameterizedRoute = ({ component: Component, ...props }: ParameterizedRouteType) => { - const { organisationId, projectId } = props.computedMatch?.params + const { organisationId, projectId } = props.computedMatch?.params ?? {} const parsedOrganisationId = organisationId && parseInt(organisationId) const parsedProjectId = projectId && parseInt(projectId) @@ -32,7 +32,9 @@ export const ParameterizedRoute = ({ return ( ( + render={( + componentProps: RouteComponentProps<{ environmentId?: string }>, + ) => ( - +