Merged
Conversation
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: frontend/src/lib/components/TaxonomicFilter/taxonomicFilterPinnedPropertiesLogic.ts
Line: 41
Comment:
**Wrong localStorage key — migration silently no-ops for all existing users**
The old `playerSettingsLogic` declares `path(['scenes', 'session-recordings', 'player', 'playerSettingsLogic'])`. Kea's `persist: true` builds the storage key from the full path, so the key actually written by the old code is:
`kea.scenes.session-recordings.player.playerSettingsLogic.quickFilterProperties`
The constant here is missing the `player` segment:
`kea.scenes.session-recordings.playerSettingsLogic.quickFilterProperties`
Because `localStorage.getItem(OLD_PERSIST_KEY)` will always return `null`, no existing user's pinned person properties will be migrated — they are silently dropped on first load. The migration flag is still written, so the window to fix this closes on the very first app load after deployment.
The `player` segment needs to be added to `OLD_PERSIST_KEY`.
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/lib/components/DefinitionPopover/DefinitionPopover.tsx
Line: 78
Comment:
**Machine-readable enum value stored as human-readable `groupName`**
`type` here is a `TaxonomicFilterGroupType` enum value (e.g. `'event_properties'`, `'person_properties'`). It is being passed as the second argument to `togglePin`, whose signature is:
```ts
togglePin(groupType: TaxonomicFilterGroupType, groupName: string, value: TaxonomicFilterValue, item: any)
```
`groupName` is intended to be the human-readable label (e.g. `'Event properties'`, `'Person properties'`) — that is what the migration code writes (`groupName: 'Person properties'`) and what is stored in `_pinnedContext.sourceGroupName` in persisted localStorage. Passing `type` stores the raw enum string instead, creating permanently incorrect data for every pin action made through the definition popover.
The popover needs access to the group's display name. The `taxonomicFilterLogic`/`TaxonomicFilterGroup.name` is the source of truth; `DefinitionPopover` would need to receive or look up that name. One option is to expose the group name from `definitionPopoverLogic`, or pass it as a prop from the call site where the group is already known.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: generic property pinning in Taxono..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Generalizes “pinned properties” from replay-only UI into the core TaxonomicFilter, adding a new pinned group/tab, a pin/unpin action in the definition popover, and a migration from the old replay localStorage key.
Changes:
- Adds
taxonomicFilterPinnedPropertiesLogic(persisted, team-scoped pins + migration from replay quick filter properties). - Introduces
PinnedFiltersgroup type and injects it into TaxonomicFilter tab ordering and top-match redistribution. - Removes replay-specific pinned-person-properties UI and the corresponding
playerSettingsLogicpersistence.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/scenes/session-recordings/player/playerSettingsLogic.ts | Removes replay-only quick filter property persistence/actions. |
| frontend/src/scenes/session-recordings/filters/ReplayTaxonomicFilters.tsx | Removes replay-specific “Pinned person properties” section/UI. |
| frontend/src/lib/components/TaxonomicFilter/types.ts | Adds PinnedFilters group type. |
| frontend/src/lib/components/TaxonomicFilter/taxonomicFilterPinnedPropertiesLogic.ts | New kea logic for pins + migration from old replay storage. |
| frontend/src/lib/components/TaxonomicFilter/taxonomicFilterPinnedPropertiesLogic.test.ts | Adds logic-level test coverage for pinning and migration utils. |
| frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx | Injects pinned group/tab and adjusts ordering/top-match behavior. |
| frontend/src/lib/components/TaxonomicFilter/recentTaxonomicFiltersLogic.ts | Excludes pinned group from being tracked as “recent”. |
| frontend/src/lib/components/TaxonomicFilter/TaxonomicFilterPinning.test.tsx | Adds RTL tests for pinned tab rendering and pin button presence. |
| frontend/src/lib/components/TaxonomicFilter/InfiniteList.tsx | Renders pinned-context items and labels them as pinned. |
| frontend/src/lib/components/DefinitionPopover/DefinitionPopover.tsx | Adds pin/unpin button to definition popover header. |
| frontend/src/lib/components/DefinitionPopover/DefinitionPopover.scss | Adds layout styling for the new header actions row. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd880fa75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Move property pinning from replay-specific UI into the TaxonomicFilter itself, allowing users to pin any property from any group via the definition popover. Pinned items appear in a new searchable "Pinned" tab (after Suggested and Recents) and persist per-team in localStorage. - New `taxonomicFilterPinnedPropertiesLogic` (localStorage, team-scoped) - Pin/unpin button in DefinitionPopover header - `PinnedFilters` group type auto-injected into TaxonomicFilter - Migration copies old replay `quickFilterProperties` on first use - Remove "Pinned person properties" section from ReplayTaxonomicFilters - Remove `quickFilterProperties` from playerSettingsLogic Generated-By: PostHog Code Task-Id: c0655c2e-7595-4994-892b-a1b8d03228cf
- Pass actual group name (from taxonomicGroups) to togglePin instead of the type enum value, so cross-group tags read "Person properties - pinned" not "person_properties - pinned" - Fix OLD_PERSIST_KEY to match actual kea-localstorage key format: path segments joined with dots, no "kea." prefix, includes ".player." - Add data-attr="definition-popover-pin" to the pin button for analytics Generated-By: PostHog Code Task-Id: c0655c2e-7595-4994-892b-a1b8d03228cf
- Extract META_GROUP_TYPES shared constant to types.ts, replacing
identical EXCLUDED_GROUP_TYPES sets in both recents and pinned logics
- Add isMetaGroup flag to TaxonomicFilterGroup interface and set it on
Suggested, Recent, and Pinned group definitions
- Add metaGroupTypes selector derived from taxonomicGroups, replacing
8 scattered type !== SuggestedFilters && type !== RecentFilters &&
type !== PinnedFilters checks with !metaGroupTypes.has(type)
- Store only { name } in pinned items instead of the full definition
object — leaner localStorage, no stale data over time
Generated-By: PostHog Code
Task-Id: c0655c2e-7595-4994-892b-a1b8d03228cf
- Guard window access with typeof check for SSR/test safety - Make MIGRATION_KEY team-scoped so multi-team users get migration per team, not just the first one - Hide pin button when definition name is null (reducer ignores null values anyway, so the button was a no-op) Generated-By: PostHog Code Task-Id: c0655c2e-7595-4994-892b-a1b8d03228cf
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
from
299633a to
f025e23
Compare
Contributor
|
Size Change: +10.8 kB (+0.01%) Total Size: 127 MB
ℹ️ View Unchanged
|
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
10 times, most recently
from
71049ed to
13b740b
Compare
pauldambra
added
the
update-snapshots
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
3 times, most recently
from
4f7aa9a to
dda9f5e
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
from
dda9f5e to
538f62f
Compare
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
2 times, most recently
from
8fb6c69 to
61e5923
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
pauldambra
changed the title
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
2 times, most recently
from
1fd82a1 to
764c71e
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
from
764c71e to
59c6818
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
from
59c6818 to
bfaec75
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
from
bfaec75 to
11962a7
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
Include matching pinned items in Suggested tab search results using group-aware search — resolves each pinned item's source group and searches across both the raw name and the core filter label. Pin button now shows "Pin"/"Unpin" label text alongside the icon. Tooltip explains the purpose: "Pinned items appear in the Pinned tab for quick access". Pinned items are NOT promoted in the empty/no-search state. Skip stale/unused indicators for pinned items. Search-aware empty state for pinned group. Tailwind for popover action buttons. Generated-By: PostHog Code Task-Id: c0655c2e-7595-4994-892b-a1b8d03228cf
pauldambra
force-pushed
the
posthog-code/generic-taxonomic-filter-pinning
branch
from
11962a7 to
6768bb7
Compare
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 28 snapshots (28 modified, 0 added, 0 deleted) What this means:
Next steps:
|
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 12 snapshots (12 modified, 0 added, 0 deleted) What this means:
Next steps:
|
sampennington
approved these changes
Apr 4, 2026
Contributor
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
Nice, I am wondering if it would be a bit nicer with the pin in the top right, next to the Edit/View. Then the heading is still in the top left. But not a big deal 😄
Member
Author
Oh, i started there but it was all really tight so i split it down |
MattBro
pushed a commit
that referenced
this pull request
Apr 7, 2026
## Problem The TaxonomicFilter's property pinning is replay-specific — users can only pin person properties, and only from the Replay tab. This limits discoverability and reuse across the product. ## Changes  Generalizes property pinning into the TaxonomicFilter itself: - **New `taxonomicFilterPinnedPropertiesLogic`** — kea logic with team-scoped localStorage persistence, `togglePin` action, `isPinned` selector - **Pin button in DefinitionPopover header** — appears on the left (secondary style) when hovering any property; View/Edit links stay on the right - **New `PinnedFilters` group type** — auto-injected after Suggested → Recents → Pinned in the tab order; searchable like Recents but not promoted in Suggested - **Migration** — copies existing `quickFilterProperties` from replay's `playerSettingsLogic` on first use, then cleans up - **Removes replay-specific pinning UI** — the "Pinned person properties" section and its TaxonomicFilter popover are gone from `ReplayTaxonomicFilters` - **Cleans up `playerSettingsLogic`** — removes `quickFilterProperties`, `setQuickFilterProperties`, unused `connect` to `teamLogic` The sidebar overview grid (`sessionRecordingPinnedPropertiesLogic`) is untouched — that's a separate concern. ## How did you test this code? - **Kea logic tests** (`taxonomicFilterPinnedPropertiesLogic.test.ts`) — 27 tests covering toggle on/off, excluded groups, null values, `isPinned`/`pinnedFilterItems` selectors, migration (4 scenarios), and `hasPinnedContext`/`stripPinnedContext` utilities. All pass. - **RTL component tests** (`TaxonomicFilterPinning.test.tsx`) — 4 tests covering pin tab visibility, pinned item rendering, pin button in definition popover. These follow the exact patterns from `TaxonomicFilter.test.tsx` but cannot run in the worktree due to a pre-existing `@posthog/hogvm` module resolution issue (same issue affects all existing RTL tests in this area). - **Existing tests** — `recentTaxonomicFiltersLogic.test.ts` (22 tests) and `dataWarehouseItemUtils.test.ts` continue to pass. - made sure it works locally- ## 🤖 LLM context Agent-authored PR. The `@posthog/hogvm` native dependency is missing in the git worktree, preventing RTL component tests from running locally — this is a pre-existing environment issue affecting all TaxonomicFilter RTL tests, not specific to these changes. --- *Created with [PostHog Code](https://posthog.com/code?ref=pr)*
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
The TaxonomicFilter's property pinning is replay-specific — users can only pin person properties, and only from the Replay tab. This limits discoverability and reuse across the product.
Changes
Generalizes property pinning into the TaxonomicFilter itself:
taxonomicFilterPinnedPropertiesLogic— kea logic with team-scoped localStorage persistence,togglePinaction,isPinnedselectorPinnedFiltersgroup type — auto-injected after Suggested → Recents → Pinned in the tab order; searchable like Recents but not promoted in SuggestedquickFilterPropertiesfrom replay'splayerSettingsLogicon first use, then cleans upReplayTaxonomicFiltersplayerSettingsLogic— removesquickFilterProperties,setQuickFilterProperties, unusedconnecttoteamLogicThe sidebar overview grid (
sessionRecordingPinnedPropertiesLogic) is untouched — that's a separate concern.How did you test this code?
taxonomicFilterPinnedPropertiesLogic.test.ts) — 27 tests covering toggle on/off, excluded groups, null values,isPinned/pinnedFilterItemsselectors, migration (4 scenarios), andhasPinnedContext/stripPinnedContextutilities. All pass.TaxonomicFilterPinning.test.tsx) — 4 tests covering pin tab visibility, pinned item rendering, pin button in definition popover. These follow the exact patterns fromTaxonomicFilter.test.tsxbut cannot run in the worktree due to a pre-existing@posthog/hogvmmodule resolution issue (same issue affects all existing RTL tests in this area).recentTaxonomicFiltersLogic.test.ts(22 tests) anddataWarehouseItemUtils.test.tscontinue to pass.🤖 LLM context
Agent-authored PR. The
@posthog/hogvmnative dependency is missing in the git worktree, preventing RTL component tests from running locally — this is a pre-existing environment issue affecting all TaxonomicFilter RTL tests, not specific to these changes.Created with PostHog Code