fix(dashboard): improve analytics replay replayer lifecycle#1349
fix(dashboard): improve analytics replay replayer lifecycle#1349mantrakp04 wants to merge 1 commit intodevfrom
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.
|
📝 WalkthroughWalkthroughThe PR adds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 improves the session replay viewer's reliability by introducing a
Confidence Score: 5/5Safe to merge; all findings are P2 quality-of-life improvements that don't block the primary replay flow. No P0 or P1 correctness bugs found. The three comments address defensive edge cases that are unlikely to manifest in normal use and were pre-existing concerns in the original code. The core staleness detection and lifecycle cleanup logic is sound. page-client.tsx around the disposeReplayerForTab and rr-block useEffect changes. 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 / set_replayer_speed / set_replayer_skip_inactive]
B --> E[recreate_replayer]
C --> F[getUsableReplayerForTab]
F --> G{isReplayerStale?}
G -- No --> H[Return replayer and call play/pause]
G -- Yes --> I[restartReplayerForTab]
H --> J{throws?}
J -- Yes --> I
D --> K{isReplayerStale?}
K -- Yes --> L[disposeReplayerForTab]
K -- No --> M[Apply config / pause]
E --> N[disposeReplayerForTab pause if exists]
N --> O[pendingInitByTabRef.add]
O --> P[ensureReplayerForTab async]
I --> Q[disposeReplayerForTab scheduleReinit=true]
Q --> R[pendingInitByTabRef.add]
R --> S{container connected and has events?}
S -- Yes --> P
S -- No --> T[Wait for setContainerRefForTab]
P --> U[new Replayer created]
U --> V[REPLAYER_READY dispatched]
V --> A
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
Line: 477-482
Comment:
**`rr-block` removal may conflict with other consumers**
`document.body.classList.remove("rr-block")` unconditionally removes the class on unmount, but doesn't check whether the class was already present before this component mounted. If another component (or the app's own session-recording SDK) independently adds `rr-block` to `<body>`, unmounting this page will strip the class they set, breaking their masking.
A safer approach is to only remove the class if this component was the one that added it:
```ts
useEffect(() => {
const alreadyBlocked = document.body.classList.contains("rr-block");
if (!alreadyBlocked) document.body.classList.add("rr-block");
return () => {
if (!alreadyBlocked) document.body.classList.remove("rr-block");
};
}, []);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
Line: 75-84
Comment:
**Stale replayer detection relies on private rrweb internals**
`isReplayerStale` accesses `replayer.iframe` and `replayer.wrapper` via `as any` cast — these are undocumented internal properties of rrweb's `Replayer`. If a future rrweb upgrade renames or removes them, the function will silently return `false` for every replayer (both fields resolve to `null` → checked as `null`), disabling the entire staleness-detection and cleanup path introduced in this PR.
Consider adding an explicit version pin or a narrow unit test that asserts these fields exist, so a breaking rrweb upgrade is caught before it reaches production.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
Line: 842-849
Comment:
**Stale replayer DOM is not cleared before re-init**
`disposeReplayerForTab` removes the replayer from internal maps and disconnects the `ResizeObserver`, but does **not** clear the container element's innerHTML. When `scheduleReinit: true` and `ensureReplayerForTab` runs, rrweb's `Replayer` constructor appends a new `<iframe>` and wrapper to the same root, leaving the old replayer's DOM nodes in place.
The full-reset path (`destroyReplayers`) does clear innerHTML, but the per-tab restart path does not. This can result in duplicate iframes inside a tab container during a mid-session stale restart.
Adding `root.innerHTML = ""` before scheduling the reinit in `disposeReplayerForTab` when `scheduleReinit: true` would prevent this.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Enhance replay functionality in analytic..." | Re-trigger Greptile |
| useEffect(() => { | ||
| document.body.classList.add("rr-block"); | ||
| return () => { | ||
| document.body.classList.remove("rr-block"); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
rr-block removal may conflict with other consumers
document.body.classList.remove("rr-block") unconditionally removes the class on unmount, but doesn't check whether the class was already present before this component mounted. If another component (or the app's own session-recording SDK) independently adds rr-block to <body>, unmounting this page will strip the class they set, breaking their masking.
A safer approach is to only remove the class if this component was the one that added it:
useEffect(() => {
const alreadyBlocked = document.body.classList.contains("rr-block");
if (!alreadyBlocked) document.body.classList.add("rr-block");
return () => {
if (!alreadyBlocked) document.body.classList.remove("rr-block");
};
}, []);Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
Line: 477-482
Comment:
**`rr-block` removal may conflict with other consumers**
`document.body.classList.remove("rr-block")` unconditionally removes the class on unmount, but doesn't check whether the class was already present before this component mounted. If another component (or the app's own session-recording SDK) independently adds `rr-block` to `<body>`, unmounting this page will strip the class they set, breaking their masking.
A safer approach is to only remove the class if this component was the one that added it:
```ts
useEffect(() => {
const alreadyBlocked = document.body.classList.contains("rr-block");
if (!alreadyBlocked) document.body.classList.add("rr-block");
return () => {
if (!alreadyBlocked) document.body.classList.remove("rr-block");
};
}, []);
```
How can I resolve this? If you propose a fix, please make it concise.
| function isReplayerStale(replayer: RrwebReplayer | null | undefined) { | ||
| if (!replayer) return true; | ||
| const candidate = replayer as any; | ||
| const iframe = (candidate.iframe ?? null) as HTMLIFrameElement | null; | ||
| const wrapper = (candidate.wrapper ?? null) as HTMLElement | null; | ||
| if (!iframe || !iframe.isConnected) return true; | ||
| if (!iframe.contentDocument || !iframe.contentWindow) return true; | ||
| if (wrapper && !wrapper.isConnected) return true; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isReplayerStale accesses replayer.iframe and replayer.wrapper via as any cast — these are undocumented internal properties of rrweb's Replayer. If a future rrweb upgrade renames or removes them, the function will silently return false for every replayer (both fields resolve to null → checked as null), disabling the entire staleness-detection and cleanup path introduced in this PR.
Consider adding an explicit version pin or a narrow unit test that asserts these fields exist, so a breaking rrweb upgrade is caught before it reaches production.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
Line: 75-84
Comment:
**Stale replayer detection relies on private rrweb internals**
`isReplayerStale` accesses `replayer.iframe` and `replayer.wrapper` via `as any` cast — these are undocumented internal properties of rrweb's `Replayer`. If a future rrweb upgrade renames or removes them, the function will silently return `false` for every replayer (both fields resolve to `null` → checked as `null`), disabling the entire staleness-detection and cleanup path introduced in this PR.
Consider adding an explicit version pin or a narrow unit test that asserts these fields exist, so a breaking rrweb upgrade is caught before it reaches production.
How can I resolve this? If you propose a fix, please make it concise.| if (!scheduleReinit) return; | ||
|
|
||
| pendingInitByTabRef.current.add(tabKey); | ||
| const container = containerByTabRef.current.get(tabKey) ?? null; | ||
| const hasEvents = (eventsByTabRef.current.get(tabKey)?.length ?? 0) > 0; | ||
| if (!container || !container.isConnected || !hasEvents) return; | ||
|
|
||
| runAsynchronously(() => ensureReplayerForTab(tabKey, msRef.current.generation), { noErrorLogging: true }); |
There was a problem hiding this comment.
disposeReplayerForTab removes the replayer from internal maps and disconnects the ResizeObserver, but does not clear the container element's innerHTML. When scheduleReinit: true and ensureReplayerForTab runs, rrweb's Replayer constructor appends a new <iframe> and wrapper to the same root, leaving the old replayer's DOM nodes in place.
The full-reset path (destroyReplayers) does clear innerHTML, but the per-tab restart path does not. This can result in duplicate iframes inside a tab container during a mid-session stale restart.
Adding root.innerHTML = "" before scheduling the reinit in disposeReplayerForTab when scheduleReinit: true would prevent this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
Line: 842-849
Comment:
**Stale replayer DOM is not cleared before re-init**
`disposeReplayerForTab` removes the replayer from internal maps and disconnects the `ResizeObserver`, but does **not** clear the container element's innerHTML. When `scheduleReinit: true` and `ensureReplayerForTab` runs, rrweb's `Replayer` constructor appends a new `<iframe>` and wrapper to the same root, leaving the old replayer's DOM nodes in place.
The full-reset path (`destroyReplayers`) does clear innerHTML, but the per-tab restart path does not. This can result in duplicate iframes inside a tab container during a mid-session stale restart.
Adding `root.innerHTML = ""` before scheduling the reinit in `disposeReplayerForTab` when `scheduleReinit: true` would prevent this.
How can I resolve this? If you propose a fix, please make it concise.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
| function isReplayerStale(replayer: RrwebReplayer | null | undefined) { | ||
| if (!replayer) return true; | ||
| const candidate = replayer as any; | ||
| const iframe = (candidate.iframe ?? null) as HTMLIFrameElement | null; | ||
| const wrapper = (candidate.wrapper ?? null) as HTMLElement | null; | ||
| if (!iframe || !iframe.isConnected) return true; | ||
| if (!iframe.contentDocument || !iframe.contentWindow) return true; | ||
| if (wrapper && !wrapper.isConnected) return true; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm rrweb's Replayer publicly exposes `iframe` and `wrapper` so the cast isn't needed.
fd -t f -e d.ts . node_modules/rrweb 2>/dev/null | head -20
rg -nP --type=ts -C2 '\b(iframe|wrapper)\s*:\s*HTML' -g '**/rrweb/**/*.d.ts' 2>/dev/null | head -40
# Also verify current usage in the file under review assumes these fields without casts
rg -nP --type=tsx -C1 'replayer\.(iframe|wrapper)' 'apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx'Repository: stack-auth/stack-auth
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for rrweb type definitions
echo "=== Looking for rrweb type definitions ==="
find node_modules/rrweb -name "*.d.ts" 2>/dev/null | head -10
echo -e "\n=== Searching for 'iframe' and 'wrapper' in rrweb types ==="
find node_modules/rrweb -name "*.d.ts" -exec grep -l "iframe\|wrapper" {} \; 2>/dev/null
echo -e "\n=== Checking RrwebReplayer usage in the file at lines 714-720 ==="
sed -n '710,725p' apps/dashboard/src/app/\(main\)/\(protected\)/projects/\[projectId\]/analytics/replays/page-client.tsxRepository: stack-auth/stack-auth
Length of output: 830
🏁 Script executed:
#!/bin/bash
# Search for all usage of replayer.iframe or replayer.wrapper in the file (without type casts)
echo "=== All uses of replayer.iframe or replayer.wrapper in page-client.tsx ==="
rg -n 'replayer\.(iframe|wrapper)' apps/dashboard/src/app/\(main\)/\(protected\)/projects/\[projectId\]/analytics/replays/page-client.tsx -A2 -B2Repository: stack-auth/stack-auth
Length of output: 1669
🏁 Script executed:
#!/bin/bash
# Find where RrwebReplayer is imported/defined
echo "=== RrwebReplayer import/definition ==="
rg -n 'RrwebReplayer|import.*Replayer|from.*rrweb' apps/dashboard/src/app/\(main\)/\(protected\)/projects/\[projectId\]/analytics/replays/page-client.tsx | head -20
# Check the isReplayerStale function to see the full context
echo -e "\n=== isReplayerStale function (lines 75-84) ==="
sed -n '75,85p' apps/dashboard/src/app/\(main\)/\(protected\)/projects/\[projectId\]/analytics/replays/page-client.tsxRepository: stack-auth/stack-auth
Length of output: 916
Remove as any — the RrwebReplayer type properly exposes these fields.
The RrwebReplayer type (imported from rrweb.Replayer at line 49) is fully typed. Throughout the file (lines 714–764), replayer.iframe and replayer.wrapper are accessed directly without any casts, confirming these properties are publicly available with proper types. The as any bypass violates the repo's guideline: "Do NOT use as/any/type casts or anything else like that to bypass the type system."
Additionally, replace the truthy checks (!replayer, !iframe, !wrapper) with explicit null checks (== null, != null) per the guideline preferring explicit null/undefinedness checks.
Refactor
function isReplayerStale(replayer: RrwebReplayer | null | undefined) {
- if (!replayer) return true;
- const candidate = replayer as any;
- const iframe = (candidate.iframe ?? null) as HTMLIFrameElement | null;
- const wrapper = (candidate.wrapper ?? null) as HTMLElement | null;
- if (!iframe || !iframe.isConnected) return true;
- if (!iframe.contentDocument || !iframe.contentWindow) return true;
- if (wrapper && !wrapper.isConnected) return true;
+ if (replayer == null) return true;
+ const iframe = replayer.iframe;
+ const wrapper = replayer.wrapper;
+ if (iframe == null || !iframe.isConnected) return true;
+ if (iframe.contentDocument == null || iframe.contentWindow == null) return true;
+ if (wrapper != null && !wrapper.isConnected) return true;
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isReplayerStale(replayer: RrwebReplayer | null | undefined) { | |
| if (!replayer) return true; | |
| const candidate = replayer as any; | |
| const iframe = (candidate.iframe ?? null) as HTMLIFrameElement | null; | |
| const wrapper = (candidate.wrapper ?? null) as HTMLElement | null; | |
| if (!iframe || !iframe.isConnected) return true; | |
| if (!iframe.contentDocument || !iframe.contentWindow) return true; | |
| if (wrapper && !wrapper.isConnected) return true; | |
| return false; | |
| } | |
| function isReplayerStale(replayer: RrwebReplayer | null | undefined) { | |
| if (replayer == null) return true; | |
| const iframe = replayer.iframe; | |
| const wrapper = replayer.wrapper; | |
| if (iframe == null || !iframe.isConnected) return true; | |
| if (iframe.contentDocument == null || iframe.contentWindow == null) return true; | |
| if (wrapper != null && !wrapper.isConnected) return true; | |
| return false; | |
| } |
🤖 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 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.
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.
| function isReplayerStale(replayer: RrwebReplayer | null | undefined) { | ||
| if (!replayer) return true; | ||
| const candidate = replayer as any; | ||
| const iframe = (candidate.iframe ?? null) as HTMLIFrameElement | null; | ||
| const wrapper = (candidate.wrapper ?? null) as HTMLElement | null; | ||
| if (!iframe || !iframe.isConnected) return true; | ||
| if (!iframe.contentDocument || !iframe.contentWindow) return true; | ||
| if (wrapper && !wrapper.isConnected) return true; | ||
| return false; |
There was a problem hiding this comment.
isReplayerStale casts the replayer to any to access iframe/wrapper, but this file already uses replayer.iframe and replayer.wrapper with proper typing in ensureReplayerForTab. Avoiding the any cast here would preserve type-safety and catch API changes at compile time (e.g. access replayer.iframe / replayer.wrapper directly after the null check).
| const disposeReplayerForTab = useCallback((tabKey: TabKey, options?: { | ||
| pause?: boolean, | ||
| scheduleReinit?: boolean, | ||
| }) => { | ||
| const pause = options?.pause ?? true; | ||
| const scheduleReinit = options?.scheduleReinit ?? false; | ||
|
|
There was a problem hiding this comment.
disposeReplayerForTab removes the replayer from the refs, but it doesn’t clear the speedSubRef subscription when disposing the active tab’s replayer. If the active replayer is disposed due to staleness (e.g. via pause_all) without a uiVersion change, the existing speedService.subscribe callback can remain attached to the old replayer and keep updating state. Consider explicitly unsubscribing (and nulling) speedSubRef when disposing msRef.current.activeTabKey (or otherwise triggering the skip-indicator effect to rerun).
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