fix(cohorts): surface load errors instead of empty state on list pages#63151
Open
posthog[bot] wants to merge 1 commit into
Open
fix(cohorts): surface load errors instead of empty state on list pages#63151posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
The cohorts list silently rendered the "create your first cohort" empty state when the API request failed, and DataTable-based lists (persons, groups, events) showed the query error without a way to retry. Track load failures in cohortsSceneLogic and render an error state with a retry button, and wire onRetry into DataTable's InsightErrorState so failed queries can be re-run in place. Generated-By: PostHog Code Task-Id: b6b32e87-c802-45bf-a255-fa9a967639ce
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
frontend/src/scenes/cohorts/cohortsSceneLogic.test.ts:192-209
**Prefer parameterised tests for error-extraction cases**
The two `loadCohortsFailure` scenarios — `errorObject` with a `detail` key vs. without — are distinct inputs producing distinct outputs, which is exactly what `it.each` is designed for. Packing them into a single `it` block means a failure in the first case can mask the second, and the intent is less readable at a glance. The "empty-state is suppressed" and "retry clears the error" assertions are separate behaviours that can each live in their own `it` block.
Reviews (1): Last reviewed commit: "fix(cohorts): surface load errors instea..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+192
to
+209
| describe('load errors', () => { | ||
| it('tracks the load error and suppresses the empty state', async () => { | ||
| router.actions.push(urls.cohorts()) | ||
| await expectLogic(logic).toDispatchActions(['loadCohortsSuccess']) | ||
| expect(logic.values.cohortsLoadError).toBe(null) | ||
|
|
||
| logic.actions.loadCohortsFailure('Internal server error', { detail: 'Unknown table `person`' }) | ||
| expect(logic.values.cohortsLoadError).toBe('Unknown table `person`') | ||
| expect(logic.values.shouldShowEmptyState).toBe(false) | ||
|
|
||
| // Falls back to the error string when there is no detail | ||
| logic.actions.loadCohortsFailure('Internal server error', {}) | ||
| expect(logic.values.cohortsLoadError).toBe('Internal server error') | ||
|
|
||
| // Retrying clears the error | ||
| logic.actions.loadCohorts() | ||
| expect(logic.values.cohortsLoadError).toBe(null) | ||
| }) |
Contributor
There was a problem hiding this comment.
The two loadCohortsFailure scenarios — errorObject with a detail key vs. without — are distinct inputs producing distinct outputs, which is exactly what it.each is designed for. Packing them into a single it block means a failure in the first case can mask the second, and the intent is less readable at a glance. The "empty-state is suppressed" and "retry clears the error" assertions are separate behaviours that can each live in their own it block.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/cohorts/cohortsSceneLogic.test.ts
Line: 192-209
Comment:
**Prefer parameterised tests for error-extraction cases**
The two `loadCohortsFailure` scenarios — `errorObject` with a `detail` key vs. without — are distinct inputs producing distinct outputs, which is exactly what `it.each` is designed for. Packing them into a single `it` block means a failure in the first case can mask the second, and the intent is less readable at a glance. The "empty-state is suppressed" and "retry clears the error" assertions are separate behaviours that can each live in their own `it` block.
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!
Contributor
|
Size Change: 0 B Total Size: 72.4 MB ℹ️ View Unchanged
|
Contributor
🕸️ Eager graphHow much code each root forces the browser to download and decode through static imports — the regression class total bundle size can't see.
✅ Largest files eagerly reachable from
|
| Size | File |
|---|---|
| 878.2 KiB | src/styles/global.scss |
| 609.0 KiB | public/hedgehog/burning-money-hog.png |
| 541.9 KiB | public/hedgehog/waving-hog.png |
| 448.2 KiB | public/hedgehog/stop-sign-hog.png |
| 362.0 KiB | public/hedgehog/phone-pair-hogs.png |
| 354.8 KiB | ../node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 335.6 KiB | public/hedgehog/desk-hog.png |
| 323.2 KiB | public/hedgehog/3-bears-hogs.png |
| 297.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 286.4 KiB | src/lib/api.ts |
Largest files eagerly reachable from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 878.2 KiB | src/styles/global.scss |
| 760.0 KiB | src/queries/validators.js |
| 609.0 KiB | public/hedgehog/burning-money-hog.png |
| 541.9 KiB | public/hedgehog/waving-hog.png |
| 448.2 KiB | public/hedgehog/stop-sign-hog.png |
| 398.7 KiB | ../node_modules/.pnpm/chart.js@4.5.1/node_modules/chart.js/dist/chart.js |
| 362.0 KiB | public/hedgehog/phone-pair-hogs.png |
| 354.8 KiB | ../node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 335.6 KiB | public/hedgehog/desk-hog.png |
| 323.2 KiB | public/hedgehog/3-bears-hogs.png |
Posted automatically by check-eager-graph · sizes are input-source bytes from the esbuild metafile · part of #32479
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PostHog inbox report 019e09c0-1406-7dad-9f69-69150484f13f (P1): a user hit /persons and /cohorts during a wave of backend HogQL resolution errors and saw both pages silently render empty, with no indication anything went wrong — leading to repeated reloads and an abandoned investigation.
Investigating the report's root-cause hypothesis against the current code:
cohortsSceneLogicdoes not trackloadCohortsfailures at all. On an API error the list stays at{count: 0, results: []},shouldShowEmptyStatereturnstrue(with default filters), and the scene shows the "create your first cohort" product intro plus an empty table — indistinguishable from genuinely having no cohorts.DataTablealready branches onresponseErrorand rendersInsightErrorStatewith the HogQL error message forActorsQuery(since feat(datatable): explicitly enable features per query #17667), so failed queries are not silently swallowed there. What was missing is the retry affordance the report calls for — the error state only offered "open in query debugger".Changes
cohortsSceneLogic: newcohortsLoadErrorreducer (prefers the API errordetail, falls back to the loader error message), cleared on load/retry/success.shouldShowEmptyStatenow returnsfalsewhile a load error is present, so the product intro no longer claims the project has no cohorts when the request failed.Cohorts.tsx: the table's empty state renders the error message with a "Try again" button (guarded against double-submission while a reload is in flight) instead of the default "No cohorts" copy.DataTable.tsx: passonRetryto bothInsightErrorStatebranches, re-running the query withloadData('force_blocking').InsightErrorStatekeeps the query-debugger link as a side action of the retry button, so nothing is lost. This benefits every DataTable surface (persons, groups, events, sessions, ...).The report also flags project-specific backend HogQL errors (
Unknown table person,Could not find cohort with ID ...) as a parallel investigation track; those are over a month old, tied to one project's queries, and not reproducible from the repo — this PR addresses the durable product gap that made them invisible to users.How did you test this code?
I'm an agent; no manual testing was done. Automated tests:
cohortsSceneLogic.test.tswith a load-error spec (error captured fromdetail/message, empty state suppressed, error cleared on retry) — 6/6 passing.src/queries/nodes/DataTable/) — 138/138 passing.tsc --noEmitclean; kea typegen regenerated; oxlint/oxfmt run.Automatic notifications
Docs update
🤖 Agent context
Autonomy: Fully autonomous
ph-exploreto trace error handling throughdataNodeLogic,DataTable, andcohortsSceneLogic.InsightErrorState(the report's claim there predates checking the code), so the fix focuses on the genuine cohorts gap plus a retry affordance shared by all DataTable surfaces.LemonButtonfor the cohorts table rather thanInsightErrorState, since the cohorts list is a REST endpoint and the insight-flavored actions (query debugger, analytics bug report) don't apply.🤖 Generated with Claude Code