From aa9fe68d674feeda743ac9a0ea75104e84588291 Mon Sep 17 00:00:00 2001 From: Adam Vialpando Date: Mon, 8 Jun 2026 23:15:49 -0700 Subject: [PATCH 1/2] fix: open feature slideout for deep links to off-page features --- frontend/e2e/tests/deep-link-test.pw.ts | 94 +++++++++++++++ .../pages/features/FeaturesPage.tsx | 108 ++++++++++++------ .../hooks/__tests__/deepLinkedFeature.test.ts | 79 +++++++++++++ .../pages/features/hooks/deepLinkedFeature.ts | 38 ++++++ .../features/hooks/useDeepLinkedFeature.ts | 71 ++++++++++++ 5 files changed, 356 insertions(+), 34 deletions(-) create mode 100644 frontend/e2e/tests/deep-link-test.pw.ts create mode 100644 frontend/web/components/pages/features/hooks/__tests__/deepLinkedFeature.test.ts create mode 100644 frontend/web/components/pages/features/hooks/deepLinkedFeature.ts create mode 100644 frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts diff --git a/frontend/e2e/tests/deep-link-test.pw.ts b/frontend/e2e/tests/deep-link-test.pw.ts new file mode 100644 index 000000000000..64e708d8cb44 --- /dev/null +++ b/frontend/e2e/tests/deep-link-test.pw.ts @@ -0,0 +1,94 @@ +import { test, expect } from '../test-setup' +import { createHelpers, LONG_TIMEOUT, log } from '../helpers' +import { E2E_USER, PASSWORD, E2E_TEST_PROJECT } from '../config' +import Project from '../../common/project' + +const PAGE_SIZE = 50 +const FEATURE_COUNT = 60 +const FEATURE_PREFIX = 'e2e_deeplink_' + +type ApiList = T[] | { results: T[] } + +const unwrap = (body: ApiList): T[] => + Array.isArray(body) ? body : body.results + +test.describe('Deep link to feature slideout', () => { + test('opens the slideout for a feature on any page of the list @oss', async ({ + page, + request, + }) => { + const { login } = createHelpers(page) + const api = Project.api + + // Given - an authenticated API session and a project with > PAGE_SIZE features + log('Authenticate against the API') + const loginRes = await request.post(`${api}auth/login/`, { + data: { email: E2E_USER, password: PASSWORD }, + }) + expect(loginRes.ok()).toBeTruthy() + const { key } = await loginRes.json() + const headers = { Authorization: `Token ${key}` } + + const project = unwrap<{ id: number; name: string }>( + await (await request.get(`${api}projects/`, { headers })).json(), + ).find((p) => p.name === E2E_TEST_PROJECT)! + expect(project).toBeTruthy() + + const environment = unwrap<{ id: number; name: string; api_key: string }>( + await ( + await request.get(`${api}environments/?project=${project.id}`, { + headers, + }) + ).json(), + ).find((e) => e.name === 'Development')! + expect(environment).toBeTruthy() + + log(`Create ${FEATURE_COUNT} features`) + for (let i = 0; i < FEATURE_COUNT; i++) { + const res = await request.post( + `${api}projects/${project.id}/features/`, + { + data: { name: `${FEATURE_PREFIX}${String(i).padStart(3, '0')}` }, + headers, + }, + ) + expect(res.ok()).toBeTruthy() + } + + // The list renders sorted by name ascending, so page 1 holds the first + // PAGE_SIZE features and a feature on page 2 never mounts a row on page 1. + const listUrl = (pageNumber: number) => + `${api}projects/${project.id}/features/?environment=${environment.id}&page=${pageNumber}&page_size=${PAGE_SIZE}&sort_field=name&sort_direction=ASC` + const page1 = await (await request.get(listUrl(1), { headers })).json() + const page2 = await (await request.get(listUrl(2), { headers })).json() + const onPageFeature = page1.results[0] as { id: number; name: string } + const offPageFeature = page2.results[0] as { id: number; name: string } + expect(onPageFeature).toBeTruthy() + expect(offPageFeature).toBeTruthy() + log(`On-page: ${onPageFeature.name}, off-page: ${offPageFeature.name}`) + + await login(E2E_USER, PASSWORD) + const featuresPath = `/project/${project.id}/environment/${environment.api_key}/features` + const slideout = page.locator('.create-feature-modal') + + // When/Then - this is the #7652 regression: a deep link to a feature that is + // NOT on the first page previously rendered the list without opening any + // modal, because the deep-link handler only fired for mounted rows. + await page.goto(`${featuresPath}?feature=${offPageFeature.id}&tab=value`) + await expect(slideout).toBeVisible({ timeout: LONG_TIMEOUT }) + await expect(slideout).toContainText(offPageFeature.name) + + // And - the existing on-page deep link still works (no regression). A fresh + // navigation reloads the page, dismissing the previous slideout. + await page.goto(`${featuresPath}?feature=${onPageFeature.id}&tab=value`) + await expect(slideout).toBeVisible({ timeout: LONG_TIMEOUT }) + await expect(slideout).toContainText(onPageFeature.name) + + // And - an unknown feature id degrades gracefully (no slideout, no crash). + await page.goto(`${featuresPath}?feature=999999999&tab=value`) + await expect(page.locator('[data-test="features-page"]')).toBeVisible({ + timeout: LONG_TIMEOUT, + }) + await expect(slideout).toBeHidden() + }) +}) diff --git a/frontend/web/components/pages/features/FeaturesPage.tsx b/frontend/web/components/pages/features/FeaturesPage.tsx index 6bcec90d8b30..0bedb57c79ca 100644 --- a/frontend/web/components/pages/features/FeaturesPage.tsx +++ b/frontend/web/components/pages/features/FeaturesPage.tsx @@ -20,6 +20,7 @@ import { FeaturesTableFilters, FeaturesSDKIntegration, } from './components' +import { useDeepLinkedFeature } from './hooks/useDeepLinkedFeature' import { useFeatureFilters } from './hooks/useFeatureFilters' import { useRemoveFeatureWithToast } from './hooks/useRemoveFeatureWithToast' import { useToggleFeatureWithToast } from './hooks/useToggleFeatureWithToast' @@ -27,7 +28,11 @@ import { useProjectEnvironments } from 'common/hooks/useProjectEnvironments' import { useFeatureListWithApiKey } from 'common/hooks/useFeatureListWithApiKey' import { useViewMode } from 'common/useViewMode' import type { Pagination } from './types' -import type { ProjectFlag, FeatureState } from 'common/types/responses' +import type { + FeatureListProviderData, + FeatureState, + ProjectFlag, +} from 'common/types/responses' import { ProjectPermission } from 'common/types/permissions.types' const DEFAULT_PAGINATION: Pagination = { @@ -88,6 +93,7 @@ const FeaturesPage: FC = ({ const { error: projectEnvError, getEnvironment, + getEnvironmentIdFromKey, project, } = useProjectEnvironments(projectId) const { currentData, data, error, isFetching, isLoading, refetch } = @@ -174,6 +180,16 @@ const FeaturesPage: FC = ({ [data?.pagination], ) + // Deep-link support: open the slideout for a `?feature=` target that isn't on + // the current page by fetching it and rendering a hidden FeatureRow below. + const deepLinkedFeature = useDeepLinkedFeature({ + environmentApiKey: environmentId, + getEnvironmentIdFromKey, + isListLoaded: !!data && !isFetching, + projectFlags, + projectId, + }) + usePageTracking({ context: { environmentId, @@ -245,39 +261,37 @@ const FeaturesPage: FC = ({ [projectFlags, environmentFlags], ) - const renderFeatureRow = useCallback( - (projectFlag: ProjectFlag | SkeletonItem, i: number) => { - if (isSkeletonItem(projectFlag)) { - return - } - - return ( - - {({ permission }) => ( - - )} - - ) - }, + const renderPermissionedFeatureRow = useCallback( + ( + projectFlag: ProjectFlag, + i: number, + environmentFlagsOverride?: FeatureListProviderData['environmentFlags'], + ) => ( + + {({ permission }) => ( + + )} + + ), [ environmentFlags, environmentId, @@ -290,6 +304,17 @@ const FeaturesPage: FC = ({ ], ) + const renderFeatureRow = useCallback( + (projectFlag: ProjectFlag | SkeletonItem, i: number) => { + if (isSkeletonItem(projectFlag)) { + return + } + + return renderPermissionedFeatureRow(projectFlag, i) + }, + [renderPermissionedFeatureRow], + ) + const handleNextPage = () => { if (paging?.next) { goToPage(page + 1) @@ -378,6 +403,21 @@ const FeaturesPage: FC = ({ {renderFeaturesList()} + {deepLinkedFeature && ( +
+ {renderPermissionedFeatureRow( + deepLinkedFeature.projectFlag, + -1, + deepLinkedFeature.environmentFlag + ? { + [deepLinkedFeature.projectFlag.id]: + deepLinkedFeature.environmentFlag, + } + : {}, + )} +
+ )} + { + it('returns null when there is no feature param', () => { + expect( + shouldDeepFetchFeature({ + featureParam: undefined, + isListLoaded: true, + projectFlags, + }), + ).toBeNull() + }) + + it('returns null when the list has not loaded yet', () => { + expect( + shouldDeepFetchFeature({ + featureParam: '99', + isListLoaded: false, + projectFlags: [], + }), + ).toBeNull() + }) + + it('returns null when the feature is on the current page', () => { + expect( + shouldDeepFetchFeature({ + featureParam: '2', + isListLoaded: true, + projectFlags, + }), + ).toBeNull() + }) + + it('returns the feature id when the feature is off the current page', () => { + expect( + shouldDeepFetchFeature({ + featureParam: '99', + isListLoaded: true, + projectFlags, + }), + ).toEqual({ featureId: 99 }) + }) + + it('returns null for a non-numeric feature param', () => { + expect( + shouldDeepFetchFeature({ + featureParam: 'not-a-number', + isListLoaded: true, + projectFlags, + }), + ).toBeNull() + }) +}) + +describe('pickEnvironmentFlag', () => { + const make = (id: number, feature: number) => + ({ feature, id } as FeatureState) + + it('returns the state matching the feature id', () => { + const results = [make(10, 1), make(11, 99), make(12, 2)] + expect(pickEnvironmentFlag(results, 99)).toBe(results[1]) + }) + + it('falls back to the first result when there is no exact match', () => { + const results = [make(10, 1), make(12, 2)] + expect(pickEnvironmentFlag(results, 99)).toBe(results[0]) + }) + + it('returns undefined when there are no results', () => { + expect(pickEnvironmentFlag([], 99)).toBeUndefined() + expect(pickEnvironmentFlag(undefined, 99)).toBeUndefined() + }) +}) diff --git a/frontend/web/components/pages/features/hooks/deepLinkedFeature.ts b/frontend/web/components/pages/features/hooks/deepLinkedFeature.ts new file mode 100644 index 000000000000..4c9acfae0ddf --- /dev/null +++ b/frontend/web/components/pages/features/hooks/deepLinkedFeature.ts @@ -0,0 +1,38 @@ +import type { FeatureState } from 'common/types/responses' + +/** + * Decides whether the `?feature=` deep link targets a feature that is NOT on the + * currently loaded page, and therefore needs to be fetched directly. + * + * Returns `null` (no direct fetch needed) when there is no param, the list has + * not loaded yet, the param is not a valid id, or the feature is already on the + * current page (in which case its rendered row handles the deep link). + */ +export function shouldDeepFetchFeature(args: { + featureParam: string | undefined + projectFlags: { id: number }[] + isListLoaded: boolean +}): { featureId: number } | null { + const { featureParam, isListLoaded, projectFlags } = args + if (!isListLoaded || !featureParam) { + return null + } + const featureId = Number(featureParam) + if (!Number.isInteger(featureId)) { + return null + } + const isOnPage = projectFlags.some((flag) => flag.id === featureId) + return isOnPage ? null : { featureId } +} + +/** Pick the environment feature state matching `featureId`, falling back to the + * first result, or `undefined` when there is none. */ +export function pickEnvironmentFlag( + results: FeatureState[] | undefined, + featureId: number, +): FeatureState | undefined { + return ( + results?.find((featureState) => featureState.feature === featureId) ?? + results?.[0] + ) +} diff --git a/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts b/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts new file mode 100644 index 000000000000..800d4373c3c8 --- /dev/null +++ b/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts @@ -0,0 +1,71 @@ +import { useMemo } from 'react' +import { skipToken } from '@reduxjs/toolkit/query' +import { useGetProjectFlagQuery } from 'common/services/useProjectFlag' +import { useGetFeatureStatesQuery } from 'common/services/useFeatureState' +import Utils from 'common/utils/utils' +import type { FeatureState, ProjectFlag } from 'common/types/responses' +import { + pickEnvironmentFlag, + shouldDeepFetchFeature, +} from './deepLinkedFeature' + +type DeepLinkedFeature = { + projectFlag: ProjectFlag + environmentFlag: FeatureState | undefined +} + +/** + * Supports deep-linking to a feature slideout (`?feature=`) when the targeted + * feature is not on the current page of the paginated list. The page renders a + * hidden FeatureRow for the returned feature so its existing deep-link effect can + * open the slideout. Returns `null` when no direct fetch is needed or possible. + */ +export function useDeepLinkedFeature(args: { + projectId: number + environmentApiKey: string + getEnvironmentIdFromKey: (apiKey: string) => number | undefined + projectFlags: ProjectFlag[] + isListLoaded: boolean +}): DeepLinkedFeature | null { + const { + environmentApiKey, + getEnvironmentIdFromKey, + isListLoaded, + projectFlags, + projectId, + } = args + + const featureParam = (Utils.fromParam() as Record).feature + const decision = shouldDeepFetchFeature({ + featureParam, + isListLoaded, + projectFlags, + }) + + const environmentNumericId = environmentApiKey + ? getEnvironmentIdFromKey(environmentApiKey) + : undefined + + const { data: projectFlag, isError } = useGetProjectFlagQuery( + decision ? { id: decision.featureId, project: projectId } : skipToken, + ) + + const { data: featureStatesData } = useGetFeatureStatesQuery( + decision && environmentNumericId + ? { environment: environmentNumericId, feature: decision.featureId } + : skipToken, + ) + + return useMemo(() => { + if (!decision || isError || !projectFlag) { + return null + } + return { + environmentFlag: pickEnvironmentFlag( + featureStatesData?.results, + decision.featureId, + ), + projectFlag, + } + }, [decision, isError, projectFlag, featureStatesData]) +} From 5972101bdab58877dae3d00ab6c0b3a73bcfbbe9 Mon Sep 17 00:00:00 2001 From: Adam Vialpando Date: Tue, 23 Jun 2026 21:55:12 -0700 Subject: [PATCH 2/2] fix: address PR review on off-page deep-link slideout --- frontend/e2e/tests/deep-link-test.pw.ts | 100 +++++++++++------- .../pages/features/FeaturesPage.tsx | 2 +- .../features/hooks/useDeepLinkedFeature.ts | 34 +++--- 3 files changed, 83 insertions(+), 53 deletions(-) diff --git a/frontend/e2e/tests/deep-link-test.pw.ts b/frontend/e2e/tests/deep-link-test.pw.ts index 64e708d8cb44..4ae8f5833dde 100644 --- a/frontend/e2e/tests/deep-link-test.pw.ts +++ b/frontend/e2e/tests/deep-link-test.pw.ts @@ -44,51 +44,71 @@ test.describe('Deep link to feature slideout', () => { expect(environment).toBeTruthy() log(`Create ${FEATURE_COUNT} features`) - for (let i = 0; i < FEATURE_COUNT; i++) { - const res = await request.post( - `${api}projects/${project.id}/features/`, - { - data: { name: `${FEATURE_PREFIX}${String(i).padStart(3, '0')}` }, - headers, - }, + // Unique names per run so the flakiness check (`E2E_REPEAT` / `/e2e N`) does + // not collide with features created by a previous iteration. + const runId = Date.now() + const featureName = (i: number) => + `${FEATURE_PREFIX}${runId}_${String(i).padStart(3, '0')}` + const createdFeatureIds: number[] = [] + + try { + const created = await Promise.all( + Array.from({ length: FEATURE_COUNT }, (_, i) => + request.post(`${api}projects/${project.id}/features/`, { + data: { name: featureName(i) }, + headers, + }), + ), ) - expect(res.ok()).toBeTruthy() - } + for (const res of created) { + expect(res.ok()).toBeTruthy() + createdFeatureIds.push((await res.json()).id) + } - // The list renders sorted by name ascending, so page 1 holds the first - // PAGE_SIZE features and a feature on page 2 never mounts a row on page 1. - const listUrl = (pageNumber: number) => - `${api}projects/${project.id}/features/?environment=${environment.id}&page=${pageNumber}&page_size=${PAGE_SIZE}&sort_field=name&sort_direction=ASC` - const page1 = await (await request.get(listUrl(1), { headers })).json() - const page2 = await (await request.get(listUrl(2), { headers })).json() - const onPageFeature = page1.results[0] as { id: number; name: string } - const offPageFeature = page2.results[0] as { id: number; name: string } - expect(onPageFeature).toBeTruthy() - expect(offPageFeature).toBeTruthy() - log(`On-page: ${onPageFeature.name}, off-page: ${offPageFeature.name}`) + // The list renders sorted by name ascending, so page 1 holds the first + // PAGE_SIZE features and a feature on page 2 never mounts a row on page 1. + const listUrl = (pageNumber: number) => + `${api}projects/${project.id}/features/?environment=${environment.id}&page=${pageNumber}&page_size=${PAGE_SIZE}&sort_field=name&sort_direction=ASC` + const page1 = await (await request.get(listUrl(1), { headers })).json() + const page2 = await (await request.get(listUrl(2), { headers })).json() + const onPageFeature = page1.results[0] as { id: number; name: string } + const offPageFeature = page2.results[0] as { id: number; name: string } + expect(onPageFeature).toBeTruthy() + expect(offPageFeature).toBeTruthy() + log(`On-page: ${onPageFeature.name}, off-page: ${offPageFeature.name}`) - await login(E2E_USER, PASSWORD) - const featuresPath = `/project/${project.id}/environment/${environment.api_key}/features` - const slideout = page.locator('.create-feature-modal') + await login(E2E_USER, PASSWORD) + const featuresPath = `/project/${project.id}/environment/${environment.api_key}/features` + const slideout = page.locator('.create-feature-modal') - // When/Then - this is the #7652 regression: a deep link to a feature that is - // NOT on the first page previously rendered the list without opening any - // modal, because the deep-link handler only fired for mounted rows. - await page.goto(`${featuresPath}?feature=${offPageFeature.id}&tab=value`) - await expect(slideout).toBeVisible({ timeout: LONG_TIMEOUT }) - await expect(slideout).toContainText(offPageFeature.name) + // When/Then - this is the #7652 regression: a deep link to a feature that + // is NOT on the first page previously rendered the list without opening + // any modal, because the deep-link handler only fired for mounted rows. + await page.goto(`${featuresPath}?feature=${offPageFeature.id}&tab=value`) + await expect(slideout).toBeVisible({ timeout: LONG_TIMEOUT }) + await expect(slideout).toContainText(offPageFeature.name) - // And - the existing on-page deep link still works (no regression). A fresh - // navigation reloads the page, dismissing the previous slideout. - await page.goto(`${featuresPath}?feature=${onPageFeature.id}&tab=value`) - await expect(slideout).toBeVisible({ timeout: LONG_TIMEOUT }) - await expect(slideout).toContainText(onPageFeature.name) + // And - the existing on-page deep link still works (no regression). A + // fresh navigation reloads the page, dismissing the previous slideout. + await page.goto(`${featuresPath}?feature=${onPageFeature.id}&tab=value`) + await expect(slideout).toBeVisible({ timeout: LONG_TIMEOUT }) + await expect(slideout).toContainText(onPageFeature.name) - // And - an unknown feature id degrades gracefully (no slideout, no crash). - await page.goto(`${featuresPath}?feature=999999999&tab=value`) - await expect(page.locator('[data-test="features-page"]')).toBeVisible({ - timeout: LONG_TIMEOUT, - }) - await expect(slideout).toBeHidden() + // And - an unknown feature id degrades gracefully (no slideout, no crash). + await page.goto(`${featuresPath}?feature=999999999&tab=value`) + await expect(page.locator('[data-test="features-page"]')).toBeVisible({ + timeout: LONG_TIMEOUT, + }) + await expect(slideout).toBeHidden() + } finally { + // Clean up so reruns against a persistent project don't accumulate rows. + await Promise.all( + createdFeatureIds.map((id) => + request.delete(`${api}projects/${project.id}/features/${id}/`, { + headers, + }), + ), + ) + } }) }) diff --git a/frontend/web/components/pages/features/FeaturesPage.tsx b/frontend/web/components/pages/features/FeaturesPage.tsx index 0bedb57c79ca..89b019317d1d 100644 --- a/frontend/web/components/pages/features/FeaturesPage.tsx +++ b/frontend/web/components/pages/features/FeaturesPage.tsx @@ -404,7 +404,7 @@ const FeaturesPage: FC = ({ {renderFeaturesList()} {deepLinkedFeature && ( -
+
{renderPermissionedFeatureRow( deepLinkedFeature.projectFlag, -1, diff --git a/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts b/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts index 800d4373c3c8..e4d5ec30f879 100644 --- a/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts +++ b/frontend/web/components/pages/features/hooks/useDeepLinkedFeature.ts @@ -36,11 +36,10 @@ export function useDeepLinkedFeature(args: { } = args const featureParam = (Utils.fromParam() as Record).feature - const decision = shouldDeepFetchFeature({ - featureParam, - isListLoaded, - projectFlags, - }) + const decision = useMemo( + () => shouldDeepFetchFeature({ featureParam, isListLoaded, projectFlags }), + [featureParam, isListLoaded, projectFlags], + ) const environmentNumericId = environmentApiKey ? getEnvironmentIdFromKey(environmentApiKey) @@ -50,14 +49,25 @@ export function useDeepLinkedFeature(args: { decision ? { id: decision.featureId, project: projectId } : skipToken, ) - const { data: featureStatesData } = useGetFeatureStatesQuery( - decision && environmentNumericId - ? { environment: environmentNumericId, feature: decision.featureId } - : skipToken, - ) + const { data: featureStatesData, isFetching: isFetchingFeatureStates } = + useGetFeatureStatesQuery( + decision && environmentNumericId + ? { environment: environmentNumericId, feature: decision.featureId } + : skipToken, + ) + + // When the feature has an environment, hold off until its state has resolved + // so the slideout opens with the real enabled/value state rather than blanks. + // The two queries fire together but settle independently, and the slideout is + // opened with a one-time snapshot, so a late feature state would never reach + // it. When no environment id is known we can't fetch state, so open anyway. + const awaitingFeatureState = + !!decision && + !!environmentNumericId && + (isFetchingFeatureStates || !featureStatesData) return useMemo(() => { - if (!decision || isError || !projectFlag) { + if (!decision || isError || !projectFlag || awaitingFeatureState) { return null } return { @@ -67,5 +77,5 @@ export function useDeepLinkedFeature(args: { ), projectFlag, } - }, [decision, isError, projectFlag, featureStatesData]) + }, [decision, isError, projectFlag, featureStatesData, awaitingFeatureState]) }