Skip to content

fix(experiments): improve error handling for the experiment actors query.#53817

Open
rodrigoi wants to merge 1 commit intomasterfrom
experiments/better-error-handling-for-experiments-actors-query
Open

fix(experiments): improve error handling for the experiment actors query.#53817
rodrigoi wants to merge 1 commit intomasterfrom
experiments/better-error-handling-for-experiments-actors-query

Conversation

@rodrigoi
Copy link
Copy Markdown
Contributor

@rodrigoi rodrigoi commented Apr 9, 2026

Problem

The experiment actor's query is failing for some edge cases, returning unusable errors.

Changes

We are covering the edge cases, adding tests, and improving the error messages returned by the query.

How did you test this code?

hogli test posthog/hogql_queries/experiments/test/test_experiment_actors_query.py -v

cat-type

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Publish to changelog?

do not publish to changelog

Docs update

Copy link
Copy Markdown
Contributor Author

rodrigoi commented Apr 9, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@rodrigoi rodrigoi self-assigned this Apr 9, 2026
@github-actions

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

🎭 Playwright report · View test results →

⚠️ 3 flaky tests:

  • Deleting an insight from dashboard redirects back (chromium)
  • Save view (chromium)
  • Materialize view pane (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this comment? Help fix flakies and failures and it'll disappear!

@rodrigoi rodrigoi marked this pull request as ready for review April 9, 2026 04:47
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 9, 2026

Vulnerabilities

No security concerns identified. The validation additions are server-side input guards that raise ValidationError on invalid funnelStep values rather than executing potentially dangerous queries.

Comments Outside Diff (1)

  1. posthog/hogql_queries/experiments/test/test_experiment_actors_query.py, line 406-522 (link)

    P2 Non-parameterized invalid-step test

    The four sub-tests here (invalid -1, invalid 0, out-of-range negative, out-of-range positive) are independent scenarios that each create their own fixtures and assertions. Per the project's stated preference, these should be parameterized rather than bundled into one test function.

    @parameterized.expand(
        [
            (
                "invalid_minus_one",
                -1,
                ["Cannot query drop-offs before the first metric step", "experiment funnel", "Exposure", "signup", "exposed but never entered the funnel", "Valid drop-off steps: -2", "-3"],
            ),
            (
                "invalid_zero",
                0,
                ["Funnel steps are 1-indexed", "Step 0 does not exist", "Valid conversion steps: 1", "2"],
            ),
            (
                "out_of_range_negative",
                -4,
                ["Invalid drop-off step -4", "2 metric steps", "Valid drop-off steps: -2", "-3"],
            ),
            (
                "out_of_range_positive",
                3,
                ["Invalid conversion step 3", "2 metric steps", "Valid conversion steps: 1", "(first metric step) to 2"],
            ),
        ]
    )
    @freeze_time("2020-01-01T12:00:00Z")
    def test_experiment_funnel_actors_invalid_steps(self, _name, funnel_step, expected_fragments):
        _, _, experiment_query = self._create_experiment_with_funnel()
        q = ExperimentActorsQuery(
            kind="ExperimentActorsQuery",
            source=experiment_query,
            funnelStep=funnel_step,
            funnelStepBreakdown="control",
            includeRecordings=False,
        )
        actors_query = ActorsQuery(source=q, select=["id", "person"])
        with self.assertRaises(Exception) as ctx:
            ActorsQueryRunner(query=actors_query, team=self.team).calculate()
        for fragment in expected_fragments:
            self.assertIn(fragment, str(ctx.exception))
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: posthog/hogql_queries/experiments/test/test_experiment_actors_query.py
    Line: 406-522
    
    Comment:
    **Non-parameterized invalid-step test**
    
    The four sub-tests here (invalid `-1`, invalid `0`, out-of-range negative, out-of-range positive) are independent scenarios that each create their own fixtures and assertions. Per the project's stated preference, these should be parameterized rather than bundled into one test function.
    
    ```python
    @parameterized.expand(
        [
            (
                "invalid_minus_one",
                -1,
                ["Cannot query drop-offs before the first metric step", "experiment funnel", "Exposure", "signup", "exposed but never entered the funnel", "Valid drop-off steps: -2", "-3"],
            ),
            (
                "invalid_zero",
                0,
                ["Funnel steps are 1-indexed", "Step 0 does not exist", "Valid conversion steps: 1", "2"],
            ),
            (
                "out_of_range_negative",
                -4,
                ["Invalid drop-off step -4", "2 metric steps", "Valid drop-off steps: -2", "-3"],
            ),
            (
                "out_of_range_positive",
                3,
                ["Invalid conversion step 3", "2 metric steps", "Valid conversion steps: 1", "(first metric step) to 2"],
            ),
        ]
    )
    @freeze_time("2020-01-01T12:00:00Z")
    def test_experiment_funnel_actors_invalid_steps(self, _name, funnel_step, expected_fragments):
        _, _, experiment_query = self._create_experiment_with_funnel()
        q = ExperimentActorsQuery(
            kind="ExperimentActorsQuery",
            source=experiment_query,
            funnelStep=funnel_step,
            funnelStepBreakdown="control",
            includeRecordings=False,
        )
        actors_query = ActorsQuery(source=q, select=["id", "person"])
        with self.assertRaises(Exception) as ctx:
            ActorsQueryRunner(query=actors_query, team=self.team).calculate()
        for fragment in expected_fragments:
            self.assertIn(fragment, str(ctx.exception))
    ```
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
This is a comment left during a code review.
Path: posthog/hogql_queries/experiments/test/test_experiment_actors_query.py
Line: 406-522

Comment:
**Non-parameterized invalid-step test**

The four sub-tests here (invalid `-1`, invalid `0`, out-of-range negative, out-of-range positive) are independent scenarios that each create their own fixtures and assertions. Per the project's stated preference, these should be parameterized rather than bundled into one test function.

```python
@parameterized.expand(
    [
        (
            "invalid_minus_one",
            -1,
            ["Cannot query drop-offs before the first metric step", "experiment funnel", "Exposure", "signup", "exposed but never entered the funnel", "Valid drop-off steps: -2", "-3"],
        ),
        (
            "invalid_zero",
            0,
            ["Funnel steps are 1-indexed", "Step 0 does not exist", "Valid conversion steps: 1", "2"],
        ),
        (
            "out_of_range_negative",
            -4,
            ["Invalid drop-off step -4", "2 metric steps", "Valid drop-off steps: -2", "-3"],
        ),
        (
            "out_of_range_positive",
            3,
            ["Invalid conversion step 3", "2 metric steps", "Valid conversion steps: 1", "(first metric step) to 2"],
        ),
    ]
)
@freeze_time("2020-01-01T12:00:00Z")
def test_experiment_funnel_actors_invalid_steps(self, _name, funnel_step, expected_fragments):
    _, _, experiment_query = self._create_experiment_with_funnel()
    q = ExperimentActorsQuery(
        kind="ExperimentActorsQuery",
        source=experiment_query,
        funnelStep=funnel_step,
        funnelStepBreakdown="control",
        includeRecordings=False,
    )
    actors_query = ActorsQuery(source=q, select=["id", "person"])
    with self.assertRaises(Exception) as ctx:
        ActorsQueryRunner(query=actors_query, team=self.team).calculate()
    for fragment in expected_fragments:
        self.assertIn(fragment, str(ctx.exception))
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: frontend/src/scenes/experiments/charts/funnel/StepBar.tsx
Line: 189-195

Comment:
**Tooltip hint shown for non-clickable drop-off area at step 1**

`hasClickableData` is `true` for `stepIndex === 1` (since `stepIndex > 0`), so the "Click to inspect actors" tooltip hint appears when hovering anywhere over the step 1 bar — including the drop-off backdrop which has a `default` cursor and silently returns early on click. The cursor already signals non-clickability, but the tooltip text is still misleading for that segment.

The comment above correctly notes "Step 1 drop-offs: conversions work", so the tooltip is intentionally showing for conversions at step 1. Worth clarifying the comment or, if the tooltip implementation supports it, passing a more granular signal to distinguish conversion vs. drop-off clickability.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(experiments): improve error handling..." | Re-trigger Greptile

Comment on lines 189 to 195
// Only show "Click to inspect actors" hint when clicking will actually work:
// - Step 0 (exposure) with new feature enabled: can't use actors query (returns early), so don't show hint
// - Step 0 with legacy: can use sampled sessions, show hint if sessionData exists
// - Step > 0 with new feature: can use actors query, show hint
// - Step > 0 with legacy: can use sampled sessions, show hint if sessionData exists
// - Step 1 (first metric) drop-offs with new feature: can't query (no exposure in backend funnel), conversions work
// - Step 2+ with new feature: both conversions and drop-offs work
// - Legacy mode: show hint if sessionData exists
const hasClickableData = hasActorsQueryFeature ? stepIndex > 0 : !!sessionData
showTooltip([rect.x, rect.y, rect.width], stepIndex, step, hasClickableData)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Tooltip hint shown for non-clickable drop-off area at step 1

hasClickableData is true for stepIndex === 1 (since stepIndex > 0), so the "Click to inspect actors" tooltip hint appears when hovering anywhere over the step 1 bar — including the drop-off backdrop which has a default cursor and silently returns early on click. The cursor already signals non-clickability, but the tooltip text is still misleading for that segment.

The comment above correctly notes "Step 1 drop-offs: conversions work", so the tooltip is intentionally showing for conversions at step 1. Worth clarifying the comment or, if the tooltip implementation supports it, passing a more granular signal to distinguish conversion vs. drop-off clickability.

Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/experiments/charts/funnel/StepBar.tsx
Line: 189-195

Comment:
**Tooltip hint shown for non-clickable drop-off area at step 1**

`hasClickableData` is `true` for `stepIndex === 1` (since `stepIndex > 0`), so the "Click to inspect actors" tooltip hint appears when hovering anywhere over the step 1 bar — including the drop-off backdrop which has a `default` cursor and silently returns early on click. The cursor already signals non-clickability, but the tooltip text is still misleading for that segment.

The comment above correctly notes "Step 1 drop-offs: conversions work", so the tooltip is intentionally showing for conversions at step 1. Worth clarifying the comment or, if the tooltip implementation supports it, passing a more granular signal to distinguish conversion vs. drop-off clickability.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +86 to +89
// For drop-offs, the mapping is straightforward
// Frontend step 2 drop-off = "completed backend step 0 ($pageview) but not step 1 (uploaded_file)" = backend -2 = -stepIndex
// Frontend step 3 drop-off = "completed backend step 1 (uploaded_file) but not step 2 (next event)" = backend -3 = -stepIndex
const funnelStep = converted ? backendStepNo : -backendStepNo
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment contains incorrect backend step numbering that contradicts the earlier comment and actual implementation. Lines 69-70 correctly state that backend uses step 1 for the first metric event ($pageview), but lines 87-88 incorrectly claim backend step 0 is $pageview. This creates confusion about the step mapping.

According to the test data:

  • Step -2 means "completed step 1 (signup) but not step 2 (purchase)"
  • Not "completed backend step 0 but not step 1" as the comment suggests

The comment should be:

// For drop-offs, the mapping is straightforward
// Frontend step 2 drop-off = "completed step 1 ($pageview) but not step 2 (click)" = backend -2 = -stepIndex
// Frontend step 3 drop-off = "completed step 2 (click) but not step 3 (next event)" = backend -3 = -stepIndex
Suggested change
// For drop-offs, the mapping is straightforward
// Frontend step 2 drop-off = "completed backend step 0 ($pageview) but not step 1 (uploaded_file)" = backend -2 = -stepIndex
// Frontend step 3 drop-off = "completed backend step 1 (uploaded_file) but not step 2 (next event)" = backend -3 = -stepIndex
const funnelStep = converted ? backendStepNo : -backendStepNo
// For drop-offs, the mapping is straightforward
// Frontend step 2 drop-off = "completed step 1 ($pageview) but not step 2 (click)" = backend -2 = -stepIndex
// Frontend step 3 drop-off = "completed step 2 (click) but not step 3 (next event)" = backend -3 = -stepIndex
const funnelStep = converted ? backendStepNo : -backendStepNo

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant