fix(replay): use live click attributes in breadcrumbs#20262
fix(replay): use live click attributes in breadcrumbs#20262
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Cloudflare
Core
Deps
Other
Bug Fixes 🐛Deno
Other
Internal Changes 🔧Ci
Deps
Other
🤖 This preview updates automatically when you update the PR. |
There was a problem hiding this comment.
Pull request overview
Fixes Replay click breadcrumbs so recorded node attributes prefer the current/live DOM element attributes (to avoid stale rrweb mirror metadata), while still using rrweb metadata for the rest of the breadcrumb node payload. Adds a regression unit test covering attribute mutation/staleness.
Changes:
- Update DOM breadcrumb construction to source
attributesfrom the liveElementwhen available. - Keep rrweb snapshot metadata as the source for
tagName/textContent(when present). - Add a unit test ensuring live attributes win over stale mirror metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/replay-internal/src/coreHandlers/handleDom.ts | Prefer live element attributes for breadcrumb node attributes; add helper to serialize Element.attributes. |
| packages/replay-internal/test/unit/coreHandlers/handleDom.test.ts | Add regression test asserting live attributes override stale rrweb meta; restore mocks after each test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/replay-internal/test/unit/coreHandlers/handleDom.test.ts
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
billyvg
left a comment
billyvg
left a comment
There was a problem hiding this comment.
This makes sense to me, my only question is should we be mutating the event rather than returning a new event?
| !data || | ||
| typeof data !== 'object' || | ||
| !('source' in data) || | ||
| data.source !== IncrementalSource.Mutation || |
There was a problem hiding this comment.
Can this be simplified?
| !data || | |
| typeof data !== 'object' || | |
| !('source' in data) || | |
| data.source !== IncrementalSource.Mutation || | |
| data?.source !== IncrementalSource.Mutation || |
There was a problem hiding this comment.
data is unknown due to the ReplayEventWithTime being typed as such, I didn't want to cast it to be able to simplify it. WDYT? is it safe to cast it?
There was a problem hiding this comment.
Oh hmmm… would using a type predicate work to narrow the event type down to an incremental snapshot event? we might have some helpers already
| !('attributes' in data) || | ||
| !Array.isArray(data.attributes) |
There was a problem hiding this comment.
and this too?
| !('attributes' in data) || | |
| !Array.isArray(data.attributes) | |
| !Array.isArray(data?.attributes) |
There was a problem hiding this comment.
a similar thing here, data is typed as Record<"source", unknown> so optional chaining will case a type error without casting.
Should we maybe lax the types upstream or could we keep them for now to keep the changes simple.
Co-Authored-By: GPT-5 <noreply@anthropic.com>
Co-Authored-By: GPT-5 <noreply@anthropic.com>
Co-Authored-By: GPT-5 <noreply@anthropic.com>
Co-Authored-By: GPT-5 <noreply@anthropic.com>
Co-Authored-By: GPT-5 <noreply@anthropic.com>
logaretm
force-pushed
the
awad/js-2135-replay-click-live-attributes
branch
from
f435b1a to
e5bbdb0
Compare
|
@billyvg we are mutating the mirrored state using the event as an input, not the event itself. Do you have another approach in mind? |
|
@logaretm oh sorry misunderstood — do you know why mirror becomes stale? |
|
@billyvg AFAIK (take it with lots of salt), replay stores an initial snapshot and then a stream of mutation events. Eventually it won't be stale. But because breadcrumbs read those attributes from the snapshot they will be always outdated since it doesn't fold the streamed mutation events to the values it records. So this approach does this folding on the emit event, I tried initially finding the real element and reading from it but we will then have to mask every single attribute again since they will be unmasked, which felt like more work than folding the mutations in a flat loop. |
Fixes replay element attributes grabbing a potentially stale version of the attributes, we basically now prefer the live element if available, otherwise we keep the old behavior.
closes #20238