feat(replay): pin a primary property from the session replay inspector#60812
Merged
Conversation
Adds a hover-revealed pin to the event-properties list in the replay inspector so a property can be promoted to the event's primary property in one click. The update is optimistic and reverts on failure, and it respects taxonomy-locked events: built-in primary properties show a disabled pin and other rows show none. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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: ffdb422c0b
ℹ️ 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".
Contributor
|
Size Change: +8.84 kB (+0.01%) Total Size: 80.8 MB 📦 View Changed
ℹ️ View Unchanged
|
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/models/primaryEventPropertiesModel.ts:45-53
**Stale optimistic state permanently shadows server refreshes**
`optimisticPrimaryProperties` entries are never removed — they accumulate for the lifetime of the model (a global singleton). The `primaryProperties` selector always lets `optimisticPrimaryProperties` override `loadedPrimaryProperties`, so once a user pins or unpins a property, any subsequent server refresh via `refreshLoadedPrimaryProperties` will be silently overridden by that stale local value.
Concrete breakage: User A unpins `prop_a` (optimistic entry becomes `{ my_event: null }`). User B pins `prop_b`. User A's model refreshes, loading `{ my_event: 'prop_b' }` into `loadedPrimaryProperties`. The selector still deletes `my_event` because `optimisticPrimaryProperties[my_event]` is `null`, so User A's UI shows no primary property even though the server has one.
The same happens after a revert on failure: `applyOptimisticPrimaryProperty(eventName, previous)` writes the old loaded value into optimistic state, meaning any future server update to that event's definition will be shadowed until a full page reload. A `clearOptimisticPrimaryProperty` action dispatched from the `finishSavingPrimaryProperty` success path would fix this.
### Issue 2 of 2
frontend/src/models/primaryEventPropertiesModel.test.ts:46-79
**Success and failure paths are candidates for parameterised tests**
The `'optimistically applies the pin'` and `'reverts the pin'` cases share the same trigger (`setPrimaryProperty('my_event', 'chosen_prop')`), the same initial dispatch (`applyOptimisticPrimaryProperty('my_event', 'chosen_prop')`), and diverge only in the mock response code, the revert dispatch, and the expected final state. Per the codebase preference for parameterised tests, combining them via `it.each` would remove the duplication and make the behavioural contract easier to extend.
Reviews (1): Last reviewed commit: "feat(replay): pin a primary property fro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
pauldambra
commented
May 31, 2026
Member
Author
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
QA Swarm review complete. See inline comments and the summary comment below.
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: qa-team (specialists + generalists), paul-reviewer, xp-reviewer, security-audit Verdict:
|
| Reviewer | Assessment |
|---|---|
| 🔍 qa-team | Well-scoped, flag-gated, well-tested; one HIGH data-integrity issue (no success reconciliation), rest LOW/NIT. |
| 👤 paul | Clean optimistic flow; wants a clearer 404 message and a data-attr for adoption; rowActions/group coupling is a minor fragility. |
| 📐 xp | Good separation of concerns; reconcile optimistic state on success, add a test for the branchy button, drop/comment the redundant guard. |
| 🛡 security-audit | No exploitable findings — backend endpoints are team-scoped, no XSS sink, no mass assignment, no IDOR. |
Automated by QA Swarm — not a human review
The primary-property model kept its optimistic override forever: on a successful save the optimistic entry was never cleared and the loaded map was never updated, so the primaryProperties selector shadowed any later server refresh (e.g. from the edit page or another tab) with the stale optimistic value, and the optimistic map only ever grew. Make the loaded map a plain reducer so a successful save folds the confirmed value straight into it (no extra round trip), and clear the optimistic entry in a finally block on both the success and failure paths. Optimistic state now only holds in-flight writes; loaded is the single source of truth once a save settles. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
loadedEventNames was updated when loadPrimaryProperties was dispatched, so a failed primary-properties fetch marked those events as loaded and ensureLoadedForEvents skipped them forever (until a page reload). Key the reducer off the primaryPropertiesLoaded success action instead, and swallow a failed best-effort fetch so the events stay retryable. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Feature is gated behind a flag, all critical data-integrity issues (stale optimistic state, unreconciled writes) were fixed in subsequent commits and marked resolved, and the backend by_name endpoint already exists. Remaining unresolved comments are low/medium UX nits the author is aware of — none are showstoppers.
pauldambra
commented
May 31, 2026
Member
Author
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
QA Swarm second pass — both prior fixes confirmed correct. Two new findings inline; verdict improved to APPROVE WITH NITS.
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review (second pass): qa-team, paul-reviewer, xp-reviewer, security-audit Verdict: 💬 APPROVE WITH NITSBoth fixes from the first pass are confirmed correct by all reviewers — optimistic state now reconciles on save and failed loads stay retryable. security-audit clean again. Two new findings, both MEDIUM and both narrow. New findings
Still open from pass 1 (deferred)
Reviewer summaries
Automated by QA Swarm — not a human review |
A primary-properties refresh that started before a pin save but resolves after it would overwrite the freshly saved value with stale server data, reverting the pin until the next refresh. Track a per-event save version and skip applying any load whose event was saved while the load was in flight. Also capture load and save failures (posthog.captureException) instead of swallowing them silently. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
… group The pin reveal previously depended on consumers knowing the magic group/kv-row class to pair with SimpleKeyValueList's row. Render each row through a SimpleKeyValueRow that owns its hover state and hand isRowHovered to the rowActions render prop, so the reveal contract is a typed boolean and no consumer reaches for a class name. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
pauldambra
added
stamphog
Contributor
There was a problem hiding this comment.
All critical issues (stale optimistic state, save/refresh race, silent error swallowing) were fixed in follow-up commits and verified with targeted tests. The feature is gated behind a flag, the backend endpoint already exists, and the remaining unresolved comments are low-priority UX nits with no data-integrity impact.
Contributor
There was a problem hiding this comment.
Feature is flag-gated, all critical data-integrity issues (stale optimistic state, save/refresh race, silent error swallowing) were fixed and verified with targeted tests, and the current-head bot review approved. Remaining unresolved comments are UX nits with no production risk.
… failures setPrimaryProperty resolved the event definition by name and then updated it inside one try block, so a failed name lookup showed "Could not update the pinned property" even though no update was attempted. Split the two calls: a lookup failure now reports that the definition could not be found and skips the update entirely, while update failures keep their own message. Each failure is captured with its stage for observability. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
github-actions
Bot
dismissed stale reviews from themself
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
Contributor
There was a problem hiding this comment.
This is a well-structured, feature-flag-gated frontend addition. All critical issues raised in reviews (stale optimistic state, save/refresh race condition, silent error swallowing, misleading 404 error message) were addressed in subsequent commits and are visible in the current diff. The model correctly handles optimistic updates with version-tracking to guard against in-flight refresh races, clears optimistic state in finally blocks on both success and failure, and the tests cover the key scenarios. Remaining unresolved comments are low-severity nits (memoization, data-attr tracking, component test for PinPrimaryPropertyButton) with no production risk.
Contributor
There was a problem hiding this comment.
Well-reviewed, feature-flag-gated frontend addition. All critical issues raised across multiple review rounds (stale optimistic state, save/refresh race condition, silent error swallowing, misleading 404 message) were addressed in subsequent commits and covered by targeted tests. Remaining unresolved comments are low-priority nits (missing data-attr, missing useCallback, PinPrimaryPropertyButton component tests) with no data-integrity or production risk.
…r branches Add data-attrs to the pin (so pin/unpin adoption can be tracked) and give the built-in (taxonomy) pin the same active styling as a team-pinned one so the two "pinned" states look alike. Add a parameterized component test over the render branches: built-in disabled pin, hidden non-primary rows of a taxonomy event, and the hover/pinned reveal logic. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
github-actions
Bot
dismissed stale reviews from themself
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
pauldambra
added
stamphog
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR exceeded the automated size ceiling (548 lines), which requires a human reviewer to sign off. The code itself is well-reviewed — all critical issues (stale optimistic state, save/refresh race, silent error swallowing, misleading 404 message) were addressed in follow-up commits with tests. One minor unresolved inline comment remains about a rapid double-pin race, but it's low-likelihood for a user-initiated action behind a feature flag.
…imistic overlay The model had grown a loaded/optimistic reducer split, a merge selector, a per-event save-version guard and a saving-set reducer — all of it scaffolding to support an optimistic overlay. Replace it with a kea loader whose primaryProperties value is the single source of truth and a second updatePrimaryProperty loader action that PATCHes and folds the value the API returns back into the map. A failed update leaves the map untouched, so there is nothing to reconcile and the stale-shadow / interleaving races disappear. The pin button shows the loader's loading state on click instead of tracking per-event saving itself, and lookup vs update failures keep distinct messages. Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
pauldambra
commented
May 31, 2026
Member
Author
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
QA Swarm third pass — review of the loader re-architecture. All reviewers approve; findings are LOW/NIT.
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review (third pass) of the loader re-architecture: qa-team, paul-reviewer, xp-reviewer, security-audit Verdict: 💬 APPROVE WITH NITSThe revert from the optimistic overlay to a plain kea loader is a genuine simplification, not a reshuffle — the loaded/optimistic split, merge selector, save-version guard and saving-set reducer all collapse into one source of truth (the loader value). All four reviewers approve; no correctness, data-integrity, or security issues. Findings are LOW/NIT. Key findings
Convergence
Reviewer summaries
Automated by QA Swarm — not a human review |
…pdate Generated-By: PostHog Code Task-Id: 35d6c5ec-a607-44ce-806d-1a4055a59e7b
There was a problem hiding this comment.
No human reviewer has signed off on the current head of this T1d-complex PR. All prior inline concerns were addressed in follow-up commits and all 44 comments are resolved, but the PR introduces a team-wide write path (pinning event definition primary properties for all team members) and needs at least one human sign-off before auto-approval.
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
veryayskiy
approved these changes
May 31, 2026
Member
Author
Merge activity
|
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
Power users repeatedly scan the same property to make sense of an event in the session replay inspector. Promoting that property to the event's primary property surfaces its value next to the event everywhere it matters (inspector rows, seekbar tooltips). Until now the only place to set it was the event-definition edit page — a surface very few people visit — so the payoff and the control lived far apart. This adds a way to promote a property from the exact place the payoff is felt.
Changes
SimpleKeyValueListgained a genericrowActions(key, isRowHovered)slot — the row owns its own hover state and hands it to the action, so the reveal is a typed boolean rather than a shared CSS class.primaryEventPropertiesModelis a kea loader:primaryPropertiesis the single source of truth, with aloadPrimaryPropertiesaction (read) and anupdatePrimaryPropertyaction (write — resolves the definition by name, PATCHes, and folds the value the API returns back into the map). A failed update returns the unchanged map, so there's nothing to reconcile. The pin shows the loader's loading state on click; lookup vs update failures keep distinct messages and are captured to error tracking.promoted-event-properties-editflag, and the pin carries adata-attrso pin/unpin adoption can be tracked.No UI screenshots: I'm an agent and did not capture them — flagging for the reviewer.
How did you test this code?
I'm an agent; automated checks only, no manual UI testing:
primaryEventPropertiesModel: load filtering, update-folds-API-response, unpin, update-failure-leaves-map-unchanged, lookup-failure-skips-update, failed-load-stays-retryable, and the loading lifecycle.PinPrimaryPropertyButton(built-in disabled pin, null on non-primary taxonomy rows, hover/pinned reveal).oxlintclean and repo-widetscshows no new errors in the changed files.Automatic notifications
🤖 Agent context
Authored with PostHog Code (Claude). The user asked to roll the promoted-properties flag out to 100% (done separately via the feature-flag API) and to design a UX for prompting people to set a primary property. We rejected a behaviour-detection approach (too ambiguous about intent) in favour of a discoverable hover affordance where the payoff is felt.
The model went through one wrong turn worth noting for reviewers: an initial version used an optimistic overlay (separate loaded/optimistic reducers + a merge selector + a save-version race guard). That design generated its own reconciliation and race bugs, so on review it was reverted to a plain kea loader whose value is the single source of truth — simpler, and the bug class disappears. Other decisions: taxonomy-locked events disable/hide the pin rather than silently no-op; the hover-reveal coupling was replaced with a typed
isRowHoveredarg; failures are captured with a per-stage tag.Created with PostHog Code