Repository navigation
chore(taxonomic-filter): share the context filters and cover the menu - #91206
Conversation
|
😎 Merged successfully - details. |
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 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. ✅ Bundle size — 🟢 -457 B (-0.0%)Uncompressed size of every built Total: 68.25 MiB · 🟢 -457 B (-0.0%) No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow 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
🟢 Largest files eagerly shipped from
|
| 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 | no change | ████░░░░░░ 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.3 KiB | dist/toolbar/toolbar-app-3NDRKAXD.css |
| 588.1 KiB | dist/toolbar/chunk-chunk-ASICNWQ7.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-FOUIAG54.js |
| 134.1 KiB | dist/toolbar/chunk-chunk-WZV5GVIO.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 71.3 KiB | dist/toolbar/toolbar-app-TQL7FPZQ.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-R3WZARKB.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-ELTH4LPQ.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 — 🟢 -8.2 KiB (-0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1439.85 MiB · 🟢 -8.2 KiB (-0.0%)
|
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}
haacked
changed the title
There was a problem hiding this comment.
Pull request overview
This PR hardens TaxonomicFilter “pinned” and “recents” context-filtering by centralizing the filtering rules in shared helpers and adding a menu-level regression test to ensure out-of-context pinned items do not render in the rebuild menu.
Changes:
- Added a rebuild-menu test that verifies pinned items from groups not offered by the picker do not appear in the “Pinned” tab.
- Updated legacy
infiniteListLogicto reusefilterPinnedForContext/filterRecentsForContext(and widenedselectingKeyOnlyto the sharedSelectingKeyOnlytype). - Applied the shared pinned filtering to
useTaxonomicLocalOverridesand added a hook-level test to cover that behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/components/TaxonomicFilter/utils/suggestedContextFilters.ts | Clarifies helper ownership and strengthens the “excluded values leak” comment; remains the shared source for recents/pinned context filtering. |
| frontend/src/lib/components/TaxonomicFilter/menu/TaxonomicFilterMenu.pinned.test.tsx | New menu integration test ensuring out-of-context pinned items are not rendered in the rebuild menu. |
| frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts | Removes duplicated context-filter logic in selectors by delegating to shared helpers; updates selectingKeyOnly typing to match the shared type. |
| frontend/src/lib/components/TaxonomicFilter/hooks/useTaxonomicLocalOverrides.ts | Filters pinned overrides via the shared helper so headless overrides match menu behavior. |
| frontend/src/lib/components/TaxonomicFilter/hooks/useTaxonomicFilter.test.tsx | Adds coverage to assert pinned overrides are filtered to the picker’s available group types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
haacked
changed the title
🦔 Hogbox preview · ❌ build failedThe preview didn't come up for commit Previews are optional and never block merging. A failure here is often a hogland or tailnet hiccup rather than anything in your PR, so the check stays green and this comment is the status. |
|
Reviews (1): Last reviewed commit: "chore(taxonomic-filter): refresh the gen..." | Re-trigger Greptile |
Pins are global, so the rebuilt picker's Pinned tab listed every pin a person had, including ones sourced from groups that picker does not offer. Selecting one handed back a value the picker cannot represent, such as a pinned cohort in an events-only picker. The same menu's shortcut rows already filtered this way, and so does the legacy picker, so this brings the tab in line with both. The legacy copy of the rule now calls the shared helper rather than repeating it. Generated-By: PostHog Desktop Task-Id: 7a1fcf77-9508-47d3-b09c-503207b5c938
The seeded array already matches PinnedTaxonomicFilter field for field, so the cast only stopped the compiler from checking the fixture against a future change to that type.
The rebuilt menu filters its pinned rows through filterPinnedForContext, and nothing exercised that call. Combobox tests take pinnedEntries as a prop, so they all start downstream of it, and the filter could be deleted with the suite still green. Deleting it is not a silent failure. resolveShortcutGroup falls back to the first non-meta group, so an unfiltered cohort pin renders as an Events row and commits a cohort id under the Events group. Also drops the user-visible claim from the hook test's name and comment. Only the headless primitives read that override and none of them renders today, so it aligns the hook with the menu rather than changing a list.
…picker infiniteListLogic held its own copy of the recents context rule, identical to filterRecentsForContext apart from the kea selector wrapper. The two had already drifted: the hand-written selector typed selectingKeyOnly as boolean, while the generated types and the prop itself allow a per-group dict. The selector now calls the shared helper with the wider annotation, and the comment in suggestedContextFilters no longer tells readers to maintain both copies. The clause about excluded values leaking back in moves to the helper with the rule it explains.
Importing SelectingKeyOnly by name lets kea-typegen emit the short form instead of an inline import(), so the committed block no longer matched what typegen produces.
The comment named which components read the override and asserted nothing user-visible changed, both of which go stale as soon as the headless primitives ship. It now states why the override needs scoping at all.
6beffba to
55e80ae
Compare
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Frontend-only refactor consolidating duplicated pin/recents filtering logic behind shared helpers, with substantial new test coverage proving the behavior; not risky territory, and no unresolved substantive review concerns shown against the current diff.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 81L, 3F substantive, 221L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (221L, 5F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 55e80ae · reviewed head 55e80ae |
Important
This is a refactoring and test PR. It does not change the user experience in any way.
Problem
Pins and recent items are saved globally, but each picker should only offer the ones whose source group it can actually represent. An events-only picker should not list a pinned cohort. That rule was written out in several places, and one of those places had no test.
filterPinnedForContextatTaxonomicFilterMenu.tsx:378. No test ran that call. Removing it left all 602 tests in theTaxonomicFiltersuite passing.resolveShortcutGroupfalls back to the first non-meta group, so the pinned cohort appears as an Events row and commits a cohort id under the Events group.infiniteListLogicspelled both rules out again in its own selectors. The two recents versions had already drifted apart: the hand-written one typedselectingKeyOnlyasboolean, while the generated types and the prop both allow a per-group dict.useTaxonomicLocalOverridesreturned pins without applying the rule, so it handed back a different list than the menu did. Only the headless primitives read that list, and none of them renders today.Changes
menu/TaxonomicFilterMenu.pinned.test.tsxcovers the menu's pinned filter. It renders the menu with an events-only picker and a pinned cohort, then asserts the cohort does not appear.infiniteListLogiccallsfilterPinnedForContextandfilterRecentsForContextinstead of holding its own copies. The pinned body it replaced was character-level equivalent; the recents body differed only in the selector wrapper.selectingKeyOnlyasSelectingKeyOnly, matching the generated types and the prop.useTaxonomicLocalOverridesruns pins through the same shared helper.suggestedContextFilters.tsdescribes one shared rule instead of telling readers to maintain two copies.Each rule is now written once, and every caller uses that copy.
How did you test this code?
The new test catches deletion of
filterPinnedForContextatTaxonomicFilterMenu.tsx:378. Nothing else does.Combobox.test.tsxtakespinnedEntriesas a prop, so its seven pinned tests run after the filter has already been applied. The helper's own unit tests never assert that the menu calls it.Measured rather than assumed. Delete the filter and the full
TaxonomicFiltersuite fails exactly one test out of 602, the new one. Restore it and all 602 pass.The two delegations are covered by the helpers' existing unit tests in
utils/suggestedContextFilters.test.tsand by the legacy suites, which stay green.Not checked: the running app. No manual pass through the picker, and no screenshot.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
None. No user-facing doc describes which pins a picker lists.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)