feat(cohorts): add activity log and edit history tracking#57702
Conversation
Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
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/activityDescriptions.test.tsx:72-113
**Prefer parameterised tests for repeated activity-type assertions**
The five simple-activity tests (`created`, `deleted`, `restored`, `persons_added_manually`, `person_removed_manually`) are structurally identical: they differ only in the `activity` string and the expected output text. An `it.each` table would express all five in a single test and is the preferred pattern in this codebase. The same applies to the three `description` field sub-cases below (adding / clearing / editing), which also vary only in their `before`/`after` values.
Reviews (1): Last reviewed commit: "feat(cohorts): add activity log and edit..." | Re-trigger Greptile |
|
Size Change: +13.4 kB (+0.01%) Total Size: 118 MB 📦 View Changed
ℹ️ View Unchanged
|
Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
…point api.activity.listLegacy had no requestForScope entry for Cohort, so it silently returned an empty result set. Opt cohorts into the generic /activity_log endpoint alongside experiments, hog functions, etc. Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
Wraps existing edit form in an Overview tab and exposes activity log via a sibling History tab. Tabs hidden for new cohorts. Overview content stays mounted (display:none when inactive) so form state survives tab switches. Save button stays in the title section above the tabs. Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
Move tab state from React useState into cohortEditLogic with hash-based URL routing (#tab=history). Overview is the default and clears the hash key. Reload, share-by-link, and browser back/forward all preserve the active tab. Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
PostHog routes are project-prefixed (/project/<id>/cohorts/<id>), so the
startsWith('/cohorts/') check was always false and the URL never got
updated when clicking a tab.
Generated-By: PostHog Code
Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
Adds 6 logic tests around the new tab state: default, write, strip, read-from-url, garbage-fallback, and stale-hash cleanup on mount for new cohorts. Also clears any inherited #tab=… hash when creating a new cohort, so the URL reflects the fact that tabs aren't shown until save. Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
4 updated Run: a2023686-2e48-4685-ba84-91dfea95fb54
When landing on /cohorts/new#tab=history, urlToAction set activeTab to 'history' before afterMount cleared the hash. The hash got cleaned but the in-memory tab state stuck at 'history', leaving the user on a blank page (overview hidden via display:none, history gated on a saved cohort id). Now resets activeTab to 'overview' alongside the hash cleanup. Strengthens the existing test to assert the reducer state. Generated-By: PostHog Code Task-Id: 3f6fa9a8-e6da-4c84-bbe2-8ded493b5051
|
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, please remove the |
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
There was a problem hiding this comment.
Really cool new feature, nice work.
I haven't found anything that is worth mentioning, but
claude was able to find some nits, but I leave it up to you.
Nit: Missing Loading State
CohortEdit.tsx (L307-315):
The ActivityLog component renders immediately but may take time to load data. Consider adding a loading indicator for better UX.
Nit: Type Safety Enhancement
activityDescriptions.tsx (L32-36):
The cohortFieldMapping could benefit from stronger typing:
type FieldHandler = (change?: ActivityChange) => ChangeMapping | null
const cohortFieldMapping: Record<keyof Partial<CohortType>, FieldHandler> = { ... }Nit: Extract Tab Configuration
CohortEdit.tsx (L297-304):
Consider extracting tab configuration for better maintainability:
const COHORT_TABS = [
{ key: 'overview' as const, label: 'Overview' },
{ key: 'history' as const, label: 'History' }
] satisfies { key: CohortEditTab; label: string }[]
TL;DR
Adds edit history tracking to cohorts by integrating the existing ActivityLog component and implementing a cohort activity describer to humanize change events. Users can now see what changed on a cohort, who changed it, and when.
Closes #29959
Changes
cohortActivityDescriberto convert raw activity log entries into human-readable descriptions for:is_static+cohort_typeco-emit so flipping a static cohort doesn't double-print/activity_logAPI inapi.activity.listLegacy— without this opt-in, the cohort scope fell through to a hardcoded empty result and no history was ever displayed.activityDescriptions.test.tsx) with 19 parameterized tests covering all activity types, every field handler, the dedupe logic, the unknown-field fallback, and the excluded-fields path.How did you test this code?
hogli test frontend/src/scenes/cohorts/activityDescriptions.test.tsx(19/19 passing).posthog_activitylogrows) and the History UI rendering.Created with PostHog Code