fix(dashboard): improve analytics replay replayer lifecycle#1349
Conversation
- Introduced `isReplayerStale` function to check the state of the replayer. - Added effects to manage the replayer lifecycle, including pausing and restarting based on its state. - Updated `executeEffects` to utilize the new replayer management logic, ensuring smoother playback and pause operations. - Improved cleanup of replayer instances to prevent memory leaks and ensure proper resource management. This update enhances the reliability and performance of the replay feature in the analytics section.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PostHog client initialization in the instrumentation client has been updated to disable automatic web session recording by replacing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR refactors the session replay lifecycle in the analytics dashboard by introducing Confidence Score: 5/5Safe to merge — the lifecycle refactor is well-structured with no new regressions introduced. No new P0 or P1 issues found. The staleness detection, dispose/restart helpers, and snapshot-before-mutate iteration pattern are all implemented correctly. The No files require special attention beyond the open previous-thread items in page-client.tsx. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[executeEffects] --> B{effect type}
B --> C[play_replayer / pause_replayer_at / sync_mini_tabs]
B --> D[pause_all]
B --> E[set_replayer_speed / set_replayer_skip_inactive]
B --> F[recreate_replayer]
C --> G[getUsableReplayerForTab]
G --> H{isReplayerStale?}
H -- No --> I[return replayer]
H -- Yes --> J[restartReplayerForTab]
J --> K[disposeReplayerForTab\nscheduleReinit=true]
K --> L[root.innerHTML cleared]
L --> M[runAsynchronously\nensureReplayerForTab]
G -- null --> N[ensureReplayerForTab directly]
D --> O{isReplayerStale?}
O -- Yes --> P[disposeReplayerForTab\nno reinit]
O -- No --> Q[replayer.pause]
Q -- throws --> J
E --> R{isReplayerStale?}
R -- Yes --> J
R -- No --> S[replayer.setConfig]
S -- throws --> J
F --> T[disposeReplayerForTab\nscheduleReinit=false]
T --> U[pendingInitByTabRef.add]
U --> M
Reviews (4): Last reviewed commit: "fix replay replayer lifecycle cleanup" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx (1)
1244-1249: Unmount cleanup still uses pre-staleness-checkdestroyReplayers.The unmount effect calls the older
destroyReplayershelper (lines 627-649), which callsr.pause()inside try/catch without first checkingisReplayerStale. That's functionally fine (the catch absorbs errors on disconnected iframes), but it's inconsistent with the new pattern introduced in this PR where stale checks gatepause()calls.For consistency and slightly cleaner logs/error traces, consider having
destroyReplayersreusedisposeReplayerForTab({ pause: true, scheduleReinit: false })for each tab, or at least guard thepause()call withisReplayerStale.Not blocking — just a coherence nit so the two teardown paths follow the same discipline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx around lines 1244 - 1249, The unmount cleanup currently calls the older destroyReplayers which pauses replayers without checking staleness; change the teardown to follow the new pattern by iterating through tabs and calling disposeReplayerForTab({ pause: true, scheduleReinit: false }) (or at minimum check isReplayerStale before calling r.pause()) instead of using destroyReplayers, so genCounterRef.current += 1; teardown uses the same staleness-guarded disposal logic as other paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx:
- Around line 75-84: The isReplayerStale function uses an unnecessary cast ("as
any") to access iframe and wrapper; remove the cast and access replayer.iframe
and replayer.wrapper directly (RrwebReplayer exposes these fields). Also replace
the truthy/falsy checks with explicit null/undefined checks: use == null / !=
null semantics (e.g., if (replayer == null) return true; treat iframe and
wrapper with explicit null checks and check isConnected only when they != null),
and keep the existing contentDocument/contentWindow checks accordingly to
preserve the original logic.
---
Nitpick comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx:
- Around line 1244-1249: The unmount cleanup currently calls the older
destroyReplayers which pauses replayers without checking staleness; change the
teardown to follow the new pattern by iterating through tabs and calling
disposeReplayerForTab({ pause: true, scheduleReinit: false }) (or at minimum
check isReplayerStale before calling r.pause()) instead of using
destroyReplayers, so genCounterRef.current += 1; teardown uses the same
staleness-guarded disposal logic as other paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84149ed5-fb10-4f5e-ba07-d94c15d95c59
📒 Files selected for processing (3)
CLAUDE.mdCLAUDE.mdapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
There was a problem hiding this comment.
Pull request overview
Improves the dashboard’s Analytics → Replays session replay viewer reliability by adding replayer “staleness” detection and centralizing per-tab replayer disposal/restart logic, then wiring this into the effect executor to better keep playback state and rrweb instance state in sync.
Changes:
- Add
isReplayerStale(...)and route rrweb play/pause/config effects through a “usable replayer” accessor that restarts stale instances. - Introduce
disposeReplayerForTab/restartReplayerForTabhelpers to coordinate teardown and re-init scheduling. - Adjust rrweb Replayer initialization (e.g.
showWarning: false) and add a page-levelrr-blockbody class during mount.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… on reinit - Added a check to prevent adding the "rr-block" class if it already exists on the document body. - Introduced a mechanism to clear any leftover DOM elements from the previous replayer instance to avoid stacking issues. - Improved handling of replayer root references to ensure proper cleanup and initialization.
…er restart logic. This change ensures that when an error occurs during replayer operations (pause, setConfig), the replayer is restarted for the affected tab, improving stability and user experience.
|
@greptile-ai review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx (1)
994-1003:⚠️ Potential issue | 🟠 Major
recreate_replayereffect can stack iframes inside the container.
disposeReplayerForTab(tabKey, { scheduleReinit: false })only clearsroot.innerHTMLwhenscheduleReinitistrue(lines 846–850). Becauserecreate_replayerpassesscheduleReinit: falseand then manually schedulesensureReplayerForTab, the previous rrweb-renderediframe/wrapperare left in the container DOM, and the newnew Replayer(events, { root: rootEl, ... })will append a second pair of children — exactly the stacking case the dispose helper comments warn about ("so the new one doesn't stack a second iframe/wrapper into the same container").Either route this through the existing reinit path (preferred), or clear the root DOM explicitly before re-initialising.
🛠️ Proposed fix (explicit DOM clear before reinit)
case "recreate_replayer": { const tabKey = effect.tabKey; disposeReplayerForTab(tabKey, { pause: replayerByTabRef.current.has(tabKey), scheduleReinit: false, }); + const root = containerByTabRef.current.get(tabKey) ?? null; + if (root) root.innerHTML = ""; pendingInitByTabRef.current.add(tabKey); runAsynchronously(() => ensureReplayerForTab(tabKey, effect.generation), { noErrorLogging: true }); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx around lines 994 - 1003, The "recreate_replayer" effect currently calls disposeReplayerForTab(tabKey, { scheduleReinit: false }) then schedules ensureReplayerForTab, which leaves the previous rrweb iframe/wrapper in the container and causes stacking; fix by routing through the existing reinit path or explicitly clearing the container before re-init: either call disposeReplayerForTab(tabKey, { scheduleReinit: true }) so the helper clears root.innerHTML and schedules reinit, or immediately locate the tab's root element (via replayerByTabRef/current lookup used by disposeReplayerForTab) and set root.innerHTML = '' before invoking pendingInitByTabRef.current.add(tabKey) and runAsynchronously(() => ensureReplayerForTab(tabKey, effect.generation), { noErrorLogging: true }); ensure you update only the recreate_replayer branch to prevent stacked iframes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx:
- Around line 994-1003: The "recreate_replayer" effect currently calls
disposeReplayerForTab(tabKey, { scheduleReinit: false }) then schedules
ensureReplayerForTab, which leaves the previous rrweb iframe/wrapper in the
container and causes stacking; fix by routing through the existing reinit path
or explicitly clearing the container before re-init: either call
disposeReplayerForTab(tabKey, { scheduleReinit: true }) so the helper clears
root.innerHTML and schedules reinit, or immediately locate the tab's root
element (via replayerByTabRef/current lookup used by disposeReplayerForTab) and
set root.innerHTML = '' before invoking pendingInitByTabRef.current.add(tabKey)
and runAsynchronously(() => ensureReplayerForTab(tabKey, effect.generation), {
noErrorLogging: true }); ensure you update only the recreate_replayer branch to
prevent stacked iframes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c440c8a5-d701-4bd7-920b-7134d0f9320f
📒 Files selected for processing (1)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
…eplay handling - Updated instrumentation-client to disable PostHog session recording to prevent conflicts with Sentry's Replay integration. - Removed unused session replay event types and lifecycle management code from PageClient to streamline functionality. - Enhanced error handling in PageClient by directly accessing replayer instances, improving stability during playback operations.
mantrakp04
requested review from
BilalG1
and removed request for
N2D4 and
nams1570
Summary
Improves reliability of the session replay viewer on the project analytics replays page by tracking replayer staleness, coordinating pause/restart with effects, and cleaning up instances to avoid leaks.
Changes
isReplayerStaleand wire replayer lifecycle intoexecuteEffectsso playback and pause stay in sync with the replayer state.Test plan
pnpm lint/pnpm typecheckon touched packages if CI does not cover.Notes
Small
CLAUDE.mdtweak included in the same commit.Made with Cursor
Summary by CodeRabbit