fix(taxonomic-filter): stop the category label and hidden-event mismatches - #92075
fix(taxonomic-filter): stop the category label and hidden-event mismatches#92075posthog[bot] wants to merge 4 commits into
Conversation
…tches Gate the list heading on the control variant so the pill variant shows the category name once instead of three times. Drop hidden recent and pinned definitions from the aggregated tab's matches, so it no longer labels a row with a category whose own list (fetched with `exclude_hidden: true`) returns nothing for it. Generated-By: PostHog Desktop Task-Id: da53cb65-489a-49a7-a6ee-6be6d0389449
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 1 should fix, 1 consider. Published 3 findings (view the review). Resolved comments: 2 fixed, 1 left for you |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| Function | Location | Complexity |
|---|---|---|
<anonymous> |
frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts:1728 |
47 |
loadRemoteItems |
frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts:890 |
36 |
CategoryPillContent |
frontend/src/lib/components/TaxonomicFilter/InfiniteSelectResults.tsx:50 |
19 |
InfiniteSelectResults |
frontend/src/lib/components/TaxonomicFilter/InfiniteSelectResults.tsx:155 |
15 |
✅ Bundle size — 🟢 -257 B (-0.0%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.04 MiB · 🟢 -257 B (-0.0%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/src/scenes/surveys/Surveys.js |
26.6 KiB | 🟢 -2.4 KiB (-8.4%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.41 MiB · 22 files | 🔺 +5.7 KiB (+0.4%) | ███░░░░░░░ 31.4% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.83 MiB · 3,245 files | 🔺 +173 B (+0.0%) | █████████░ 90.9% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 307.0 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 263.5 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.js |
| 252.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.7 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.26 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.26 MiB · 18 files | 🔺 +74 B (+0.0%) | ████░░░░░░ 39.5% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 45 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 750.8 KiB | dist/toolbar/toolbar-app-X3YWLWXD.css |
| 588.3 KiB | dist/toolbar/chunk-chunk-IA6FHEMF.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-OQ4CRAT2.js |
| 134.1 KiB | dist/toolbar/chunk-chunk-CCNNNLCV.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 71.3 KiB | dist/toolbar/toolbar-app-SRVV5DDX.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-WNQQAHFE.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-CQNTSFCC.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +222.1 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1443.30 MiB · 🔺 +222.1 KiB (+0.0%)
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Contained frontend bug fix in the taxonomic filter picker, backed by new unit/render tests that fail without the fix; no risky territory (no auth, billing, API contract, schema, dependency, or CI changes) and no unresolved review concerns.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 43L, 3F substantive, 129L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (129L, 5F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ d0d62e2 · reviewed head d0d62e2 |
…n stories The smoke test waited on the first `.taxonomic-infinite-list`, which is an empty, hidden Recent/Pinned list that never becomes visible. Skip the empty lists so the runner waits on the visible, populated active list. Generated-By: PostHog Desktop Task-Id: da53cb65-489a-49a7-a6ee-6be6d0389449
A new stamphog review started for this PR — the fresh verdict replaces this approval.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| function isHiddenDefinition(item: TaxonomicDefinitionTypes): boolean { | ||
| return 'hidden' in item && !!item.hidden |
There was a problem hiding this comment.
Persisted shortcuts cannot report a later hidden state
Why we think it's a valid issue
- Checked: every production dispatcher of
recordRecentFilter, the pinned write and refresh paths, and the two tests the PR adds. - Found: all four production recent writers strip the field.
taxonomicFilterLogic.tsx:2497buildsconst cleanItem = { name: stripped.name, ...(stripped.id ? { id: stripped.id } : {}) };hooks/useTaxonomicFilter.ts:485-488builds the same shape;PropertyFilters/propertyFilterLogic.ts:206passesitem: { name: displayName };UniversalFilters/universalFiltersLogic.ts:64passesitem: { name: key }. A repo-wide grep finds no other non-test dispatcher.hiddentherefore never reachesrecentFilters, soisHiddenDefinitioncannot return true for any recent row in production. - Found: the new test manufactures a shape production cannot write —
infiniteListLogic.test.ts:1485-1490dispatchesrecordRecentFilterwithitem: { name: 'hidden event', hidden: true }. The test passes while the guard it covers is unreachable, so it cannot catch a regression. - Found: pinned snapshots do keep
hidden(PINNED_ITEM_DENYLISTattaxonomicFilterPinnedPropertiesLogic.ts:49does not list it, andDefinitionPopover.tsx:82passes the whole definition), but nothing refreshes them.togglePinwrites the snapshot once (taxonomicFilterPinnedPropertiesLogic.ts:206-224), and the only other writer,setPinnedFilters, runs solely for the one-time localStorage migration and default seeding (taxonomicFilterPinnedPropertiesLogic.ts:295,:328). - Found: the PR's own example cannot produce a hidden pin either. The Events group fetches with
exclude_hidden: true(utils/buildTaxonomicGroups.tsx:216), so a hidden event is not in that list to pin from. - Impact: the realistic route to the reported mismatch is "use or pin a visible definition, hide it later". In that case the persisted snapshot carries no
hidden, the guard passes, and the aggregated tab still offers a row the category endpoint rejects — the exact behavior the change claims to remove. The recent half of the change cannot fire at all, and the pinned half fires only when the pin was taken while the item already reportedhidden: true, which needs a group whose endpoint returns hidden definitions.
Issue description
isHiddenDefinition reads hidden from the locally persisted item. Production recent writes keep only name and id, so recent items never contain this field. Pinned items keep a snapshot and never refresh it. If a user hides a definition later, both shortcuts still pass this check. The source endpoint rejects the definition, so the category mismatch remains.
Suggested fix
Use the current source-group response as the authority before adding a recent or pinned match. Do not use the persisted hidden snapshot. Add a regression test that records a visible event through production actions. Hide that event, and verify that All does not return it.
Prompt to fix with AI (copy-paste)
## Context
@frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts#L80-81
@frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts#L1577-1578
@frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts#L1594-1595
<issue_description>
`isHiddenDefinition` reads `hidden` from the locally persisted item. Production recent writes keep only `name` and `id`, so recent items never contain this field. Pinned items keep a snapshot and never refresh it. If a user hides a definition later, both shortcuts still pass this check. The source endpoint rejects the definition, so the category mismatch remains.
</issue_description>
<issue_validation>
- **Checked:** every production dispatcher of `recordRecentFilter`, the pinned write and refresh paths, and the two tests the PR adds.
- **Found:** all four production recent writers strip the field. `taxonomicFilterLogic.tsx:2497` builds `const cleanItem = { name: stripped.name, ...(stripped.id ? { id: stripped.id } : {}) }`; `hooks/useTaxonomicFilter.ts:485-488` builds the same shape; `PropertyFilters/propertyFilterLogic.ts:206` passes `item: { name: displayName }`; `UniversalFilters/universalFiltersLogic.ts:64` passes `item: { name: key }`. A repo-wide grep finds no other non-test dispatcher. `hidden` therefore never reaches `recentFilters`, so `isHiddenDefinition` cannot return true for any recent row in production.
- **Found:** the new test manufactures a shape production cannot write — `infiniteListLogic.test.ts:1485-1490` dispatches `recordRecentFilter` with `item: { name: 'hidden event', hidden: true }`. The test passes while the guard it covers is unreachable, so it cannot catch a regression.
- **Found:** pinned snapshots do keep `hidden` (`PINNED_ITEM_DENYLIST` at `taxonomicFilterPinnedPropertiesLogic.ts:49` does not list it, and `DefinitionPopover.tsx:82` passes the whole definition), but nothing refreshes them. `togglePin` writes the snapshot once (`taxonomicFilterPinnedPropertiesLogic.ts:206-224`), and the only other writer, `setPinnedFilters`, runs solely for the one-time localStorage migration and default seeding (`taxonomicFilterPinnedPropertiesLogic.ts:295`, `:328`).
- **Found:** the PR's own example cannot produce a hidden pin either. The Events group fetches with `exclude_hidden: true` (`utils/buildTaxonomicGroups.tsx:216`), so a hidden event is not in that list to pin from.
- **Impact:** the realistic route to the reported mismatch is "use or pin a visible definition, hide it later". In that case the persisted snapshot carries no `hidden`, the guard passes, and the aggregated tab still offers a row the category endpoint rejects — the exact behavior the change claims to remove. The recent half of the change cannot fire at all, and the pinned half fires only when the pin was taken while the item already reported `hidden: true`, which needs a group whose endpoint returns hidden definitions.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Use the current source-group response as the authority before adding a recent or pinned match. Do not use the persisted `hidden` snapshot. Add a regression test that records a visible event through production actions. Hide that event, and verify that All does not return it.
</potential_solution>
There was a problem hiding this comment.
You're right, and I've confirmed it in the current code. The recent half of this filter is effectively dead in production: both recent writers persist only { name, id } (taxonomicFilterLogic.tsx and hooks/useTaxonomicFilter.ts), and recent items are rebuilt from that stored snapshot with no re-hydration (recentTaxonomicFiltersLogic.ts), so a recent row never carries hidden and the guard never fires for it. The added recent test records hidden: true directly, a shape production can't write, so it passes without covering a real path. Pinned items do keep hidden, but only when the pin was taken while the definition was already hidden (which needs a group whose endpoint returns hidden items); the common 'hide it later' case leaves the snapshot with no hidden, so the mismatch you describe remains.
I'm escalating rather than fixing this unattended, because the correct fix — judging hidden against the current source-group response instead of the persisted snapshot — is a design change, not a contained edit. It needs the aggregate to read each source group's live results, handle groups that haven't loaded yet, and define what 'not in the current response' means (hidden, not-yet-loaded, or just filtered out by the search). Those are decisions with real trade-offs and behavior that only shows up against live async data, so a human should pick the direction. The main options: (1) resolve hidden from live source-group data; (2) keep the snapshot approach as best-effort and make the test and the code comment honest that it only catches pinned-while-hidden; or (3) drop the aggregate hidden-filter entirely, since the item stays selectable through its group's own matches anyway. One connected note for whoever takes this: I extended the same snapshot-based filter to the aggregate's idle (no-query) prefix in an earlier commit on this PR for internal consistency, so it carries the same limitation and should be revisited alongside the search-path filter when you choose a direction.
There was a problem hiding this comment.
Approved.
Contained frontend bug fix in the taxonomic filter picker with new tests covering both fixed behaviors; no risky-territory surface (auth, billing, data models, APIs, CI/deploy) and no unresolved concerns.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 47L, 3F substantive, 133L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (133L, 5F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ c816fe0 · reviewed head c816fe0 |
|
|
The aggregated "All" tab filtered hidden recents and pinned only while a search query was typed. With no query it leads with recent/pinned prefixes built from the raw arrays, so a hidden event returned under its source category on open and disappeared once a character was typed. Apply the same `isHiddenDefinition` filter to the idle prefixes in the `items` selector and the `dedupedTopMatches` dedupe keys, so the aggregate no longer offers an "Events" row the Events tab returns nothing for. Generated-By: PostHog Desktop Task-Id: aeabee01-b2e8-4cc0-865d-1e5a6f4757be
New commits were pushed — dismissing the stamphog approval from an earlier head. This PR no longer qualifies for automatic review.
The comment claimed every category endpoint sends `exclude_hidden: true`. Feature flags and Numerical event properties omit the param, so their own tabs still list hidden items while the aggregate suppresses their hidden recents and pinned. Describe that exception and why the suppression is a deliberate minor inconsistency rather than a full mismatch. Generated-By: PostHog Desktop Task-Id: aeabee01-b2e8-4cc0-865d-1e5a6f4757be
Problem
taxonomic-filter-category-dropdownexperiment, the category name renders in the search-input pill, again as a list heading, and again inside the open menu. When the Categories column moved into the search input, the heading stayed ungated.event_definitionswithexclude_hidden: true. So the aggregate offers an "Events" row that the Events tab returns nothing for.Changes
featureFlagsstory parameter instead of an imperative call, so visual regression now captures them. Mechanical: they were skipped before because the imperative call raced the runner.How did you test this code?
@posthog/quillworkspace, none in the changed files.Automatic notifications
Docs update
None. No user-facing docs cover this picker's internal behavior.
🤖 Agent context
Autonomy: Fully autonomous
/writing-tests,/setting-feature-flags-in-storybook,/simplify,/writing-pr-descriptions.exclude_hiddenis the only server-side filter the aggregate bypassed.Created with PostHog Desktop from this inbox report.