feat(experiments): add pagination to the shared metrics modal.#60757
Merged
Conversation
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2 tasks
Contributor
|
Size Change: +1.27 kB (0%) Total Size: 80.9 MB 📦 View Changed
ℹ️ View Unchanged
|
rodrigoi
force-pushed
the
experiments/shared-metrics-pagination
branch
from
9feb56f to
994c3ea
Compare
rodrigoi
force-pushed
the
experiments/shared-metrics-modal-pagination
branch
2 times, most recently
from
1d0d21e to
a842dd1
Compare
rodrigoi
force-pushed
the
experiments/shared-metrics-pagination
branch
from
2fb2e9e to
82f5c6f
Compare
Contributor
|
👋 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. |
rodrigoi
force-pushed
the
experiments/shared-metrics-modal-pagination
branch
2 times, most recently
from
4b6c947 to
56b7c0e
Compare
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/scenes/experiments/Metrics/SharedMetricModal.tsx:103
Premature empty-state flash on modal open. When the modal opens, `hasAnyCompatibleSharedMetrics` is synchronously reset to `false` by its reducer, but the async `loadSharedMetrics` call hasn't been dispatched yet. For at least one React render the modal body shows the "no compatible metrics" banner, even when metrics exist. Including `sharedMetricsResponseLoading` in the guard prevents the banner from appearing before the first response arrives.
```suggestion
{hasAnyCompatibleSharedMetrics || sharedMetricsResponseLoading ? (
```
### Issue 2 of 2
frontend/src/scenes/experiments/Metrics/sharedMetricModalLogic.ts:98-108
Race condition between `loadNextSharedMetrics` and a concurrent `loadSharedMetrics`. If the user clicks "Load more" and then types a search term within the pagination round-trip time, both API calls are in-flight. When `loadNextSharedMetrics` resolves last it reads `values.sharedMetricsResponse?.results` — which at that point already holds the fresh search results — and appends the stale page-2 items on top of them. The result is a corrupted list that mixes two different queries. A minimal fix is to capture the base results snapshot before the `await`, or to reset `sharedMetricsResponse` to `null` when `loadSharedMetrics` fires so that a stale `loadNextSharedMetrics` completing afterwards produces a recognisably wrong (but at least not silently mixed) state. Using `breakpoint()` inside the loader would fully prevent both races.
Reviews (1): Last reviewed commit: "fix(experiments): fixing shared modal em..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
rodrigoi
force-pushed
the
experiments/shared-metrics-modal-pagination
branch
from
56b7c0e to
3cae5ae
Compare
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
jurajmajerik
approved these changes
May 31, 2026
rodrigoi
force-pushed
the
experiments/shared-metrics-pagination
branch
from
b680eaa to
b1dc523
Compare
rodrigoi
force-pushed
the
experiments/shared-metrics-modal-pagination
branch
from
3cae5ae to
2532871
Compare
rodrigoi
changed the base branch from
experiments/shared-metrics-pagination
to
graphite-base/60757
rodrigoi
force-pushed
the
graphite-base/60757
branch
from
b1dc523 to
ea7e8b7
Compare
rodrigoi
force-pushed
the
experiments/shared-metrics-modal-pagination
branch
from
2532871 to
c33af90
Compare
rodrigoi
changed the base branch from
graphite-base/60757
to
experiments/shared-metrics-pagination
rodrigoi
force-pushed
the
experiments/shared-metrics-modal-pagination
branch
from
c33af90 to
2c84d32
Compare
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
2 participants
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 "add shared metric" modal loaded all saved metrics for the project up front and filtered them client-side, following the same pattern as the shared metrics scene. For teams with a large metric library, this is a heavy, unpaginated payload every time the modal opens, and search only matches what is already in memory.
Changes
This PR moves the shared metric modal to server-side search, using a "load more" pagination pattern, and stops relying on
sharedMetricsLogicfor its data.Paginated modal logic
sharedMetricModalLogicnow owns its own data via aloadersblock instead of borrowingsharedMetricsfromsharedMetricsLogic.loadSharedMetricsfetches the first page (limit=20,offset=0, optionalsearch), andloadNextSharedMetricsfollows the response'snextURL and appends results, driving a "Load more metrics" button via thecanLoadMoreselector. Opening the modal resets the search term, and typing is debounced for 300ms before reloading.The tricky bit is the empty state. We still want to distinguish "you have no compatible shared metrics at all" from "your current search returned nothing". A new
hasAnyCompatibleSharedMetricsreducer captures that answer from the unfiltered (empty-search) load only, so a search returning zero results does not flip the empty state to "you have no metrics". This is covered explicitly in the logic tests.frontend/src/scenes/experiments/Metrics/sharedMetricModalLogic.tsModal wiring
The modal renders
compatibleSharedMetricsdirectly (as paginated server results) rather than a client-filtered list. Already-added metrics now stay visible in the table but are rendered with an "(already added)" label and a disabled checkbox, rather than being hidden entirely. The footer gained the "Load more metrics" button above the existing "See all shared metrics" link, and the table shows a loading state and an "No shared metrics match your search." empty state. Copy was tidied up to usepluralize.frontend/src/scenes/experiments/Metrics/SharedMetricModal.tsxSide effects
With both the scene (#60755) and the modal now filtering server-side, the client-side
matchesSharedMetricSearchhelper has no remaining callers and is deleted.frontend/src/scenes/experiments/utils.tsHow did you test this code?
pnpm --filter=@posthog/frontend jest --testPathPattern="sharedMetricModalLogic"👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
🤖 Agent context
Model: Opus 4.8
Manually refactored: yes
Skills used:
Relevant decisions:
sharedMetricModalLogicvia aloadersblock rather than continuing to borrowsharedMetricsfromsharedMetricsLogic, so the modal can paginate and search server-side independently of the scene.