feat(cohorts): add used-in references endpoint and UI#54738
Conversation
Closes PostHog#25935 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/api/cohort.py
Line: 1412
Comment:
**No tests for the new endpoint**
The PR description explicitly notes "I have not tested this manually." There are no backend tests covering the `GET /cohorts/:id/used_in/` endpoint — its response shape, the name-normalisation loop (`derived_name` fallback → "Unnamed"), the 100-item truncation, or the combination of the three result lists. The adjacent deletion-protection tests only exercise the existing `update()` path. A basic test suite (e.g. flag present/absent, insight present/absent, cohort present/absent, name-normalisation logic) should be added before merging.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/api/cohort.py
Line: 1413-1470
Comment:
**Code duplication violates OnceAndOnlyOnce**
The three query blocks inside `used_in` are near-identical copies of the queries in `CohortSerializer.update()` (lines ~929–1012). The feature-flag loop, the `jsonb_path_exists` insights query, and the cohort-criteria query are all duplicated verbatim (minus the error-raising logic). If the query logic ever needs to change (e.g. to also search `legacy_filters`, or to cover insights in sibling environments), it will need to be updated in two places.
Consider extracting helpers such as `_flags_using_cohort(cohort)`, `_insights_using_cohort(cohort)`, and `_cohorts_using_cohort(cohort)` and calling them from both `used_in` and the deletion check.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/api/cohort.py
Line: 1441-1443
Comment:
**Redundant second `pop` call**
The second `insight.pop("derived_name", None)` on line 1443 is only needed when `insight["name"]` was truthy (short-circuiting the first `pop` in the assignment). When `insight["name"]` is falsy the first `pop` already removes the key, making line 1443 a no-op in that branch. Using `get` in the assignment and a single unconditional `pop` is equivalent and clearer:
```suggestion
for insight in insights_using_cohort:
insight["name"] = insight["name"] or insight.get("derived_name") or "Unnamed"
insight.pop("derived_name", None)
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(cohorts): add used-in references en..." | Re-trigger Greptile |
- Extract shared helpers (get_flags_using_cohort, get_insights_using_cohort, get_cohorts_using_cohort) to eliminate duplication with deletion-protection - Fix redundant pop by using .get() before unconditional .pop() - Add tests for used_in endpoint: flags present/absent, inactive flags excluded, dependent cohorts returned Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all three Greptile findings: P1 — No tests: Added 4 backend tests in
P2 — Code duplication: Extracted three shared helpers ( P2 — Redundant pop: Fixed to use |
Resolves conflict in frontend/src/scenes/cohorts/CohortEdit.tsx where both sides added a value to the useValues destructure (usedIn from this branch, staticCohortMode from master). Kept both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmarticus
left a comment
There was a problem hiding this comment.
Thanks for extracting the three deletion-protection queries into reusable helpers — the refactor is clean and the new endpoint reads well. Main feedback:
- Declare a response schema (
@extend_schema(responses=...)) onused_inand regenerate types viahogli build:openapiso the frontend can use the generated client/types instead of the hand-writtenCohortUsedInResponseandapi.cohorts.getUsedIn(). - The 100-row cap on insights/cohorts is silent; expose a
has_moreor total so the UI can say "and N more". get_flags_using_cohortfiltersactive=True— reasonable for deletion protection, probably wrong for an informational "used in" banner. Please confirm intent.- Missing a positive test for the
insightsbranch ofused_in. - The loader is never re-fetched after saves — fine for v1, but worth a follow-up.
Prompt for an agent to address the above (copy/paste):
Please address this review on PR #54738. Specifically:
- In
posthog/api/cohort.py, add@extend_schema(responses=...)to the newused_inaction using aninline_serializer(or a dedicatedCohortUsedInSerializer) with fieldsfeature_flags: list[{id:int, key:str, name:str|None}],insights: list[{id:int, short_id:str, name:str}],cohorts: list[{id:int, name:str}], then runhogli build:openapito regenerate types.- Replace the hand-written
CohortUsedInResponseinfrontend/src/types.tsand the manualapi.cohorts.getUsedIn()/cohortsUsedInhelpers infrontend/src/lib/api.tswith the generated client call. UpdatecohortEditLogic.tsandCohortEdit.tsxto use the generated types.- Return a
has_moreboolean (or total count) alongsideinsightsandcohorts, and surface it in the banner as "and N more" when truncated.- Reconsider
active=Trueinget_flags_using_cohort: for theused_inendpoint, include inactive flags too (or split into a separate helper so the deletion-protection caller keeps its stricter filter). Updatetest_used_in_excludes_inactive_flagsaccordingly.- Add a test under
TestCohortUsedInthat creates an insight whose query references a cohort (both viajsonb_path_existsand via breakdown filter) and asserts it's returned by the endpoint.- (Optional) Re-fetch
usedInaftersubmitCohortSuccessincohortEditLogic.ts.Do not manually test — just ensure tests pass and types are regenerated.
|
@ricardothe3rd left you some feedback, and please be sure to resolve the CI failures too. Thanks for your contribution! |
|
Thank you so much @dmarticus, love to tackle all of these as well! |
2ba61fe to
09509e6
Compare
|
@ricardothe3rd still some CI failures here. |
- Split get_flags_using_cohort into two helpers: get_active_flags_using_cohort
(deletion guard, keeps active=True filter) and get_flags_using_cohort
(used_in informational endpoint, drops active=True). Both exclude soft-deleted
flags for consistency with get_insights_using_cohort and get_cohorts_using_cohort.
- Declare a typed response schema via dedicated CohortUsedInResponseSerializer
and nested block serializers, so codegen replaces the hand-written frontend
type and the response shape is self-documenting via help_text.
- Return {results, total, has_more} for insights and cohorts so the UI can
surface "(N of M shown)" when the 100-row truncation cap kicks in.
- Add tests covering the insights jsonb_path branch, the breakdown-filter
branch, derived_name fallback, soft-deleted flag exclusion, deletion-guard
preserved behavior, and the truncation/has_more contract.
Regenerated via hogli build:openapi to pick up CohortUsedInResponseSerializer and its nested block serializers from the previous commit. Adds typed CohortUsedInResponseApi (and the per-block companions) plus the cohortsUsedInRetrieve client function. MCP tool definitions are regenerated from the same source.
…anner - Drop hand-written CohortUsedInResponse from frontend/src/types.ts plus the cohortsUsedIn URL builder and api.cohorts.getUsedIn helper, replaced by the generated cohortsUsedInRetrieve and CohortUsedInResponseApi. - Wire posthog.captureException into the loadUsedIn error path so a broken endpoint surfaces in error tracking instead of rendering nothing forever. - Reload the references banner on submitCohortSuccess so it doesn't go stale after a save. - Restructure the banner under three subheadings (Feature flags, Insights, Cohorts), and surface "(N of M shown)" when has_more is true on the insights or cohorts blocks.
|
@dmarticus pushed fixes addressing all 7 review threads in
Locally verified clean: The new push is currently in |
CI's mypy step found two type errors after the helper split landed:
- cohort.py:1044 surfaced a pre-existing nullability bug — Cohort.name is
Optional[str], so the list comp passed `list[str | None]` to ", ".join().
Coalesce to "Unnamed" so the user-facing error message stays well-formed
for unnamed dependent cohorts.
- cohort.py:1522 — popping "derived_name" from the dict returned by
values("id", "short_id", "name", "derived_name") tripped mypy's TypedDict
immutability check. Build the response dict directly in the comprehension
instead of mutating; same end state, cleaner control flow.
Auto-generated entries from scaffold-yaml.ts -- --sync-all that I missed in the earlier regen pass. Both files now declare cohorts-used-in-retrieve as disabled by default, matching the codegen output CI runs on every PR.
Resolves the two auto-generated files by taking master and re-running hogli build:openapi + scaffold-yaml --sync-all so the new used_in operation is folded back into both products/cohorts/frontend/generated/api.ts and services/mcp/src/api/generated.ts.
f551953 to
5b6de09
Compare
|
I saw that you summoned security. Is there something specific you'd like us to review? |
|
@Piccirello sorry, that was me. Botched a merge yesterday that briefly pulled in posthog/auth.py and a bunch of other master files, which is what tripped the auto-assign. Force-pushed a clean merge after, the actual diff is 9 cohort files now. Nothing security-touching, safe to drop yourselves (and @hogql too). Thanks for checking! |
The empty-async-generator pattern (if False: yield {}) is intentional but
mypy --cache-fine-grained surfaces the unreachable yield on fresh runs even
though master CI passes thanks to its cached state. Adds a type: ignore so
fresh mypy runs (i.e. every fork PR) do not fail on a pre-existing master
quirk.
haacked
left a comment
There was a problem hiding this comment.
Nice feature. One blocking bug inline: the "Used in" banner never loads on initial page open of an existing cohort (only after a save). The rest are non-blocking suggestions and a couple of test-coverage notes.
|
@ricardothe3rd you still working on this? |
|
I'm going to work on this. |
# Conflicts: # services/mcp/src/api/generated.ts
- Filter used-in results by object-level access controls, matching the flag and insight list endpoints - Share one cohort cache and select_related(team) across flag reference checks instead of per-flag queries - Order truncated reference lists by id so pages are stable - Render the used-in banner for static cohorts too and extract a UsedInBanner component with semantic headings - Refresh references on saveCohortSuccess instead of submitCohortSuccess, which fires before the save resolves - Interpolate COHORT_USED_IN_PAGE_SIZE into serializer help_text - Add tests: flags truncation, unnamed dependent cohort deletion message, save-triggered refresh
There was a problem hiding this comment.
Pull request overview
Adds a new “used in” discovery feature for cohorts so users can see where a cohort is referenced (feature flags, insights, other cohorts) before editing/deleting it.
Changes:
- Backend: Introduces
GET /api/projects/:id/cohorts/:id/used_in/plus supporting serializers/helpers, reusing deletion-protection query patterns and adding truncation metadata. - Frontend: Adds a
usedInloader to cohort edit logic, fetching references on mount and after save. - UI: Renders a “Used in” banner on the cohort edit page with links to referencing flags/insights/cohorts.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| services/mcp/src/api/generated.ts | Adds generated schema types for the new CohortUsedInResponse. |
| services/mcp/definitions/core.yaml | Registers the new MCP operation ID (disabled by default). |
| products/cohorts/mcp/tools.yaml | Adds the cohorts “used in” MCP tool entry (disabled by default). |
| products/cohorts/frontend/generated/api.ts | Adds generated frontend client function for /used_in/. |
| products/cohorts/frontend/generated/api.schemas.ts | Adds generated TS schema types for the new response shape. |
| posthog/api/test/test_cohort.py | Adds backend tests covering flags/insights/cohort references, truncation, and scoping. |
| posthog/api/cohort.py | Implements the new used_in action and extracts shared “usage” query helpers. |
| frontend/src/scenes/cohorts/cohortEditLogic.ts | Adds usedIn loader + refresh-after-save behavior. |
| frontend/src/scenes/cohorts/cohortEditLogic.test.ts | Updates logic tests to account for the new loader/network call. |
| frontend/src/scenes/cohorts/CohortEdit.tsx | Adds the “Used in” banner UI and link rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
haacked
left a comment
There was a problem hiding this comment.
This is ready! Thanks @ricardothe3rd for getting this started and @dmarticus for the reviews.
Problem
When viewing a cohort, there's no way to know if it's being used in feature flags, insights, or other cohorts. Users can accidentally edit or delete a cohort without realizing it powers active features.
Closes #25935
Changes
Backend: New
GET /api/projects/:id/cohorts/:id/used_in/endpoint that returns feature flags, insights, and other cohorts referencing the cohort. The queries are extracted from the existing deletion-protection code inCohortSerializer.update(), following the same patterns (includingnosemgrepannotations for the parameterized.extra()calls).This also fixes a latent 500 in the existing deletion guard:
Cohort.nameis nullable, and building the "used as criteria in other cohorts" error message raised aTypeErrorwhen any dependent cohort had aNonename. Dependent cohort names now fall back to "Unnamed" in both the error message and the new endpoint, and the dependent-cohort/insight queries are now ordered byidso the names shown in the error are deterministic.Frontend:
usedInloader incohortEditLogic.tsthat fetches references on mount for existing cohortsCohortUsedInResponsetype intypes.tsapi.cohorts.getUsedIn()method andcohortsUsedInrequest builder inapi.tsThe banner only appears when there are actual references — no empty state UI.
How did you test this code?
I have not tested this manually. The backend queries are taken directly from the existing deletion-protection code that is already tested in production. Reviewers can verify by:
Note:
hogli build:openapishould be run to regenerate types for the new endpoint.Publish to changelog?
no
Docs update
N/A