PostHog
fix(cohorts): deleting cohort with 'not in cohort' filter in a cohort list
[!IMPORTANT] 👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Closes #32445 Closes #32599
Problem
Deleting a cohort from cohort list
When cohorts are in a list, you cannot delete a cohort if that cohort has a "Not in cohort" filter applied to it. The API call fails (photo below of network tab) and if you refresh the page, that "deleted" cohort returns from the grave.
Payload of failed endpoint:
I compared the payload of this failed action above with the payload of deleting it the cohort while inside the cohort (successful action) and suspect that sending the full payload with those "Not in cohort" filters were probably messing around with things.
Cannot save cohort when filtering an autocapture event (i.e. with Target)
When building cohort filters for autocapture events, the frontend was serializing element property filters with "type": "element". However, the cohort API expects all event property filters—including those for autocapture—to have "type": "event". This PR fixes that before passing it along.
Changes
Video below shows both 1) successful deletion with "Not in cohort" filter + undoing + cohort does not return from the grave 2) successful saving of cohort when filtering an autocapture event
Uploading Screen Recording 2025-05-25 at 6.28.28 PM.mov…
How did you test this code?
Locally.
Hi! I don't think this is the correct solution as it removes the ability to "undo" the delete. Normally, when you delete, there's a toast at the bottom with a note about the deletion and a button to "undo" the delete.
Take a look at the bug I recently fixed: https://github.com/PostHog/posthog/pull/32365
I believe the correct fix would be to modify line 126 of Cohorts.tsx
<LemonButton status="danger" onClick={() => deleteCohort(cohort)} fullWidth>
So that it calls createCohortFormData on cohort before passing it into deleteCohort. Or modifying deleteCohort to do it. Mainly, we just need to clean up the cohort before we pass it along.
Ah yes, missed that out. Just pushed some changes, would something like this work? I wasn't quite able to use createCohortFormData for deletion and was running into this error:
You get a type error (Type 'FormData' has no properties in common with type 'Partial<CohortType>').
I suspect it's because createCohortFormData returns a FormData object and deleteWithUndo spreads the object into a JSON payload and won't serialize as expected?
Have tested these changes + undoing it and it worked well
I suspect it's because createCohortFormData returns a FormData object and deleteWithUndo spreads the object into a JSON payload and won't serialize as expected?
Yeah, I think the other PR casts it to Partial<CohortType>.
Will update my code to do the same for consistency. Thanks so much @haacked !
Hey @haacked , I could be mistaken but would what I've just updated work? Just passing in the cohort id, name and deleted = True status was enough to allow things to be correctly deleted + restored (if the user clicks undo) and there seems to be no need to create createCohortFormData.
I've also put a fix for the other bug you linked this PR with #32599 and updated the PR accordingly, let me know if this works. Thanks a bunch Phil!
@hoppybunny Great job! The delete code is perfect. Would you mind moving the fix for #32599 into a separate pull request so I can review that separately.
@hoppybunny let me know if you can get to this or I can take over. Thanks!
Sorry @haacked would need your help to take over as I’m on very tight deadlines this week and don’t want this to be a blocker 🙏 Appreciate it!!
@hoppybunny I took care of it. It turns out your fix for $autocapture events wasn't correct. This is one reason I like a PR per issue. Makes it easy to independently confirm fixes.
If you're curious about the actual fix: https://github.com/PostHog/posthog/pull/33318
@hoppybunny I took care of it. It turns out your fix for $autocapture events wasn't correct. This is one reason I like a PR per issue. Makes it easy to independently confirm fixes.
If you're curious about the actual fix: #33318
Indeed, thanks for the catch Phil! Thanks for linking the actual fix too, really helps. @haacked
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.
