From 9fba984a74bfd6940ea7e9aedf15a50135d95e58 Mon Sep 17 00:00:00 2001 From: SketchRudy Date: Tue, 2 Jun 2026 11:52:13 -0700 Subject: [PATCH] fix(frontend): redirect to default environment when URL env id is invalid Closes #7446 When a URL contains an environment identifier that does not correspond to a real environment for the current project (e.g. /project/1/environment/undefined/features), the dashboard previously showed an infinite loading spinner. The root cause: 1. useFeatureListWithApiKey returns skipToken when the env api_key does not resolve, so the features query is permanently skipped and isLoading never flips to false. 2. EnvironmentReadyChecker silently ignored the 404 from useGetEnvironmentQuery and fell through to render children with the invalid environmentId. EnvironmentReadyChecker now redirects to /project/:projectId/ when either the loaded environments list does not contain the URL's environmentId, or the per-environment query returns 404. That route already handles "pick the default environment" via ProjectRedirectPage, so no new default-environment logic is introduced. The redirect decision is extracted into a pure helper (common/utils/shouldRedirectMissingEnvironment) with unit tests covering nine cases including the ref guard against double-fire, partial loading states, zero-environment projects, and non-404 errors. Also switches the checker from useRouteMatch to useRouteContext to match the convention used by FeaturesPage and ProjectRedirectPage, and removes the now-unused match prop passed in by ParameterizedRoute. Acknowledges the reverted PR #7284 history: that attempt rendered a NotFoundState UX which was reverted by #7312; this PR takes the redirect approach explicitly called out in #7446's acceptance criterion. --- .../shouldRedirectMissingEnvironment.test.ts | 116 ++++++++++++++++++ .../utils/shouldRedirectMissingEnvironment.ts | 68 ++++++++++ .../components/EnvironmentReadyChecker.tsx | 84 ++++++++++--- .../base/higher-order/ParameterizedRoute.tsx | 15 +-- 4 files changed, 258 insertions(+), 25 deletions(-) create mode 100644 frontend/common/utils/__tests__/shouldRedirectMissingEnvironment.test.ts create mode 100644 frontend/common/utils/shouldRedirectMissingEnvironment.ts 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 }>, + ) => ( - +