fix(flags): surface real errors in the flag testing tab (#108975)

Co-authored-by: posthog[bot] <206114724+posthog[bot]@users.noreply.github.com>
Co-authored-by: tests-posthog[bot] <250237707+tests-posthog[bot]@users.noreply.github.com>
Co-authored-by: Gustavo H. Strassburger <gustavo@posthog.com>

Author: 3 people

frontend/src/scenes/feature-flags/FeatureFlagTestingTab.tsx

@@ -99,22 +99,11 @@ export function FeatureFlagTestingTab({ featureFlag }: { featureFlag: FeatureFla
         setIncludeTime,
         setSelectedResultDistinctId,
         clearTestForm,
-        testFlagEvaluation,
-        testAllDistinctIds,
+        submitTestEvaluation,
     } = useActions(logic)
 
     const isLoading = testEvaluationLoading || allEvaluationsLoading
 
-    const handleSubmit = (): void => {
-        if (hasMultipleDistinctIds) {
-            // Evaluate every merged distinct ID in one go so their variants can be
-            // compared side by side, rather than re-running the tool per ID.
-            testAllDistinctIds({ flagId: featureFlag.id!, distinctIds: personDistinctIds, formData })
-        } else {
-            testFlagEvaluation({ flagId: featureFlag.id!, formData })
-        }
-    }
-
     const hasConditions = !!result?.conditions?.length
     // The batch row whose detail is currently expanded — the explicit selection, or
     // the first row when nothing has been clicked yet (matches the activeResult selector).
@@ -270,7 +259,7 @@ export function FeatureFlagTestingTab({ featureFlag }: { featureFlag: FeatureFla
                         <LemonButton
                             type="primary"
                             loading={isLoading}
-                            onClick={handleSubmit}
+                            onClick={() => submitTestEvaluation()}
                             disabledReason={!hasValidPerson ? 'Please select a person' : undefined}
                         >
                             {hasMultipleDistinctIds

frontend/src/scenes/feature-flags/featureFlagTestingLogic.test.ts

@@ -445,11 +445,7 @@ describe('featureFlagTestingLogic', () => {
         })
     })
 
-    describe('groups validation through testFlagEvaluation', () => {
-        // The invalid-groups cases fail the evaluation loader on purpose; kea-loaders would log each
-        beforeEach(silenceKeaLoadersErrors)
-        afterEach(resumeKeaLoadersErrors)
-
+    describe('form validation through submitTestEvaluation', () => {
         it.each([
             { description: 'valid object succeeds', groups: '{"team": "backend"}', expectedError: null },
             { description: 'empty string succeeds', groups: '', expectedError: null },
@@ -466,13 +462,99 @@ describe('featureFlagTestingLogic', () => {
                 expectedError: 'groups must be a JSON object',
             },
             { description: 'json number fails', groups: '42', expectedError: 'groups must be a JSON object' },
-        ])('$description', async ({ groups, expectedError }) => {
+            { description: 'json null fails', groups: 'null', expectedError: 'groups must be a JSON object' },
+            {
+                description: 'malformed timestamp fails',
+                groups: '',
+                timestamp: '2024-01-01',
+                expectedError: 'Invalid timestamp format',
+            },
+        ])('$description', async ({ groups, timestamp = '', expectedError }) => {
+            logic.actions.setTestFormData({ distinct_id: 'p1', timestamp, groups })
+
+            if (expectedError) {
+                // Invalid input must never reach the loader, where kea-loaders reports it as an exception.
+                await expectLogic(logic, () => {
+                    logic.actions.submitTestEvaluation()
+                })
+                    .toFinishAllListeners()
+                    .toNotHaveDispatchedActions(['testFlagEvaluation', 'testFlagEvaluationFailure'])
+            } else {
+                await expectLogic(logic, () => {
+                    logic.actions.submitTestEvaluation()
+                }).toDispatchActions(['testFlagEvaluation', 'testFlagEvaluationSuccess'])
+            }
+
+            expect(logic.values.testError).toBe(expectedError)
+        })
+
+        it('runs the batch evaluation for a person with merged distinct IDs', async () => {
+            logic.actions.setSelectedPerson({
+                name: 'Jane Doe',
+                uuid: 'uuid-abc',
+                distinct_ids: ['user-123', 'user-456'],
+            })
+            logic.actions.setTestFormData({ distinct_id: 'user-123', timestamp: '', groups: '' })
+
+            await expectLogic(logic, () => {
+                logic.actions.submitTestEvaluation()
+            })
+                .toDispatchActions([
+                    logic.actionCreators.testAllDistinctIds({
+                        flagId: 1,
+                        distinctIds: ['user-123', 'user-456'],
+                        formData: { distinct_id: 'user-123', timestamp: '', groups: '' },
+                    }),
+                    'testAllDistinctIdsSuccess',
+                ])
+                .toNotHaveDispatchedActions(['testFlagEvaluation'])
+        })
+    })
+
+    describe('API error messages', () => {
+        // The failing responses fail the evaluation loader on purpose; kea-loaders would log each
+        beforeEach(silenceKeaLoadersErrors)
+        afterEach(resumeKeaLoadersErrors)
+
+        it.each([
+            {
+                description: 'maps a timestamp failure in the error field',
+                status: 500,
+                body: { error: 'Failed to build person properties at specified timestamp.' },
+                expectedError:
+                    'Unable to build person properties at the selected timestamp. This person may not have had any recorded activity at that time, or the timestamp may be too far in the past.',
+            },
+            {
+                description: 'maps an invalid timestamp in the error field',
+                status: 400,
+                body: { error: 'Invalid timestamp format.' },
+                expectedError: 'Invalid timestamp. Please select a valid date and time.',
+            },
+            {
+                description: 'keeps a specific timestamp reason as is',
+                status: 400,
+                body: { error: "Feature flag 'test-flag' did not exist at the specified timestamp." },
+                expectedError: "Feature flag 'test-flag' did not exist at the specified timestamp.",
+            },
+            {
+                description: 'maps a missing person in the detail field',
+                status: 404,
+                body: { detail: 'Person not found for person_id: abc' },
+                expectedError: 'Person not found. This person may not have existed at the selected timestamp.',
+            },
+        ])('$description', async ({ status, body, expectedError }) => {
+            useMocks({
+                post: {
+                    '/api/projects/:team/feature_flags/1/test_evaluation': () => [status, body],
+                },
+            })
+
             await expectLogic(logic, () => {
                 logic.actions.testFlagEvaluation({
                     flagId: 1,
-                    formData: { distinct_id: 'p1', timestamp: '', groups },
+                    formData: { distinct_id: 'p1', timestamp: '', groups: '' },
                 })
-            }).toFinishAllListeners()
+            }).toDispatchActions(['testFlagEvaluationFailure'])
 
             expect(logic.values.testError).toBe(expectedError)
         })

frontend/src/scenes/feature-flags/featureFlagTestingLogic.ts

@@ -45,8 +45,8 @@ const testEvaluationConcurrencyController = new ConcurrencyController(TEST_EVALU
 
 // Build the request body for a single evaluation. Shared by the single-ID and
 // batch paths so groups/timestamp are parsed and validated identically. Throws on
-// invalid groups or timestamp — callers let that fail the loader so the error
-// surfaces once via testError rather than per distinct ID.
+// invalid groups or timestamp. submitTestEvaluation checks the form with
+// testFormValidationError first, so this throw does not reach the loaders.
 function buildEvaluationRequest(formData: TestFormData, distinctId: string): FeatureFlagTestEvaluationRequestApi {
     const data: FeatureFlagTestEvaluationRequestApi = {}
 
@@ -70,34 +70,38 @@ function buildEvaluationRequest(formData: TestFormData, distinctId: string): Fea
     return data
 }
 
+// A loader that throws an error without an HTTP status is reported to error tracking,
+// so invalid user input must stop before a loader runs.
+function testFormValidationError(formData: TestFormData): string | null {
+    try {
+        buildEvaluationRequest(formData, '')
+        return null
+    } catch (e) {
+        return e instanceof Error ? e.message : String(e)
+    }
+}
+
 // Map an evaluation failure to the user-facing message. Shared by the single-ID
 // and batch failure handlers so both surface the same friendly rewrites.
 function evaluationErrorMessage(error: string, errorObject?: unknown): string {
-    const apiError = errorObject as ApiError
-    if (apiError?.detail) {
-        const errorDetail = apiError.detail
-
-        if (errorDetail.includes('Failed to build person properties at specified timestamp')) {
-            return 'Unable to build person properties at the selected timestamp. This person may not have had any recorded activity at that time, or the timestamp may be too far in the past.'
-        }
-
-        if (errorDetail.includes('person') && errorDetail.includes('not found')) {
-            return 'Person not found. This person may not have existed at the selected timestamp.'
-        }
+    const apiError = errorObject as ApiError | undefined
+    // ApiError.message already holds the body's `error` field, which test_evaluation uses for its own failures.
+    const message = apiError?.detail || apiError?.message || error || ''
+    const lowerMessage = message.toLowerCase()
 
-        if (errorDetail.includes('timestamp') || errorDetail.toLowerCase() === 'invalid timestamp') {
-            return 'Invalid timestamp. Please select a valid date and time.'
-        }
+    if (lowerMessage.includes('failed to build person properties at specified timestamp')) {
+        return 'Unable to build person properties at the selected timestamp. This person may not have had any recorded activity at that time, or the timestamp may be too far in the past.'
+    }
 
-        return errorDetail
+    if (lowerMessage.includes('person') && lowerMessage.includes('not found')) {
+        return 'Person not found. This person may not have existed at the selected timestamp.'
     }
 
-    const errorMessage = apiError?.message || error || ''
-    if (errorMessage.includes('Failed to build person properties at specified timestamp')) {
-        return 'Unable to build person properties at the selected timestamp. This person may not have had any recorded activity at that time, or the timestamp may be too far in the past.'
+    if (lowerMessage.includes('invalid timestamp') || apiError?.attr === 'timestamp') {
+        return 'Invalid timestamp. Please select a valid date and time.'
     }
 
-    return errorMessage || 'An unexpected error occurred while testing the feature flag'
+    return message || 'An unexpected error occurred while testing the feature flag'
 }
 
 function validateAndParseGroups(groups: string): Record<string, any> {
@@ -108,7 +112,7 @@ function validateAndParseGroups(groups: string): Record<string, any> {
 
     try {
         const parsed = JSON.parse(trimmed)
-        if (typeof parsed !== 'object' || Array.isArray(parsed)) {
+        if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) {
             throw new Error('groups must be a JSON object')
         }
         return parsed
@@ -271,6 +275,9 @@ export interface featureFlagTestingLogicActions {
     setTestFormData: (formData: Partial<TestFormData>) => {
         formData: Partial<TestFormData>
     }
+    submitTestEvaluation: () => {
+        value: true
+    }
     testAllDistinctIds: ({
         flagId,
         distinctIds,
@@ -397,6 +404,7 @@ export const featureFlagTestingLogic = kea<featureFlagTestingLogicType>([
         setSelectedResultDistinctId: (distinctId: string | null) => ({ distinctId }),
         clearBatchEvaluations: true,
         clearTestForm: true,
+        submitTestEvaluation: true,
     }),
     loaders(({ values }) => ({
         resolvedPersonDistinctIds: [
@@ -434,8 +442,7 @@ export const featureFlagTestingLogic = kea<featureFlagTestingLogicType>([
                     distinctIds: string[]
                     formData: TestFormData
                 }) => {
-                    // Parse groups/timestamp once up front. A malformed form throws here and
-                    // fails the whole batch loudly (one testError) rather than N identical times.
+                    // Parse groups/timestamp once up front, not once per distinct ID.
                     const baseData = buildEvaluationRequest(formData, '')
 
                     return await Promise.all(
@@ -484,8 +491,7 @@ export const featureFlagTestingLogic = kea<featureFlagTestingLogicType>([
                 clearTestForm: () => null,
                 testFlagEvaluationFailure: (_, { error, errorObject }: { error: string; errorObject?: unknown }) =>
                     evaluationErrorMessage(error, errorObject),
-                // A batch failure means the shared form (groups/timestamp) was invalid — the
-                // per-ID API errors are caught inside the loader and shown in the table.
+                // The loader catches per-ID API errors and shows them in the table.
                 testAllDistinctIdsFailure: (_, { error, errorObject }: { error: string; errorObject?: unknown }) =>
                     evaluationErrorMessage(error, errorObject),
             },
@@ -539,7 +545,22 @@ export const featureFlagTestingLogic = kea<featureFlagTestingLogicType>([
             },
         ],
     }),
-    listeners(({ actions, values }) => ({
+    listeners(({ actions, values, props }) => ({
+        submitTestEvaluation: () => {
+            const formData = values.testFormData
+            const formError = testFormValidationError(formData)
+            if (formError) {
+                actions.setTestError(formError)
+                return
+            }
+            if (values.hasMultipleDistinctIds) {
+                // Evaluate every merged distinct ID in one go so their variants can be
+                // compared side by side, rather than re-running the tool per ID.
+                actions.testAllDistinctIds({ flagId: props.flagId, distinctIds: values.personDistinctIds, formData })
+            } else {
+                actions.testFlagEvaluation({ flagId: props.flagId, formData })
+            }
+        },
         setSelectedPerson: ({ person, distinctId }) => {
             // A different person invalidates any batch table from the previous one.
             actions.clearBatchEvaluations()