fix(v10/replay): don't rewrite already-emitted nodes when syncing mirror attributes (#23588)

Backport of: #23426

Author: logaretm

packages/replay-internal/src/coreHandlers/util/getAttributesToRecord.ts

@@ -15,14 +15,19 @@ const ATTRIBUTES_TO_RECORD = new Set([
   'data-sentry-component',
 ]);
 
+/**
+ * Attributes that can end up in a click breadcrumb. This is `ATTRIBUTES_TO_RECORD` plus
+ * `data-sentry-element`, which is not recorded itself but is used as a fallback for
+ * `data-sentry-component`.
+ */
+export const BREADCRUMB_RELEVANT_ATTRIBUTES = new Set([...ATTRIBUTES_TO_RECORD, 'data-sentry-element']);
+
 /**
  * Inclusion list of attributes that we want to record from the DOM element
  */
 export function getAttributesToRecord(attributes: Record<string, unknown>): Record<string, unknown> {
   const obj: Record<string, unknown> = {};
-  if (!attributes['data-sentry-component'] && attributes['data-sentry-element']) {
-    attributes['data-sentry-component'] = attributes['data-sentry-element'];
-  }
+
   for (const key in attributes) {
     if (ATTRIBUTES_TO_RECORD.has(key)) {
       let normalizedKey = key;
@@ -35,5 +40,11 @@ export function getAttributesToRecord(attributes: Record<string, unknown>): Reco
     }
   }
 
+  // `attributes` is the serialized node held by rrweb's mirror, which is the same object that was
+  // emitted in an earlier `adds` payload, so this fallback must not be written back onto it.
+  if (!obj['data-sentry-component'] && attributes['data-sentry-element']) {
+    obj['data-sentry-component'] = attributes['data-sentry-element'];
+  }
+
   return obj;
 }

packages/replay-internal/src/util/handleRecordingEmit.ts

@@ -1,6 +1,7 @@
 import { EventType, IncrementalSource, record } from '@sentry/rrweb';
 import { NodeType } from '@sentry/rrweb-snapshot';
 import { updateClickDetectorForRecordingEvent } from '../coreHandlers/handleClick';
+import { BREADCRUMB_RELEVANT_ATTRIBUTES } from '../coreHandlers/util/getAttributesToRecord';
 import { DEBUG_BUILD } from '../debug-build';
 import { saveSession } from '../session/saveSession';
 import type { RecordingEvent, ReplayContainer, ReplayOptionFrameEvent } from '../types';
@@ -147,17 +148,35 @@ export function syncMirrorAttributesFromMutationEvent(event: RecordingEvent): vo
     const node = record.mirror.getNode(mutation.id);
     const meta = node && record.mirror.getMeta(node);
 
-    if (meta?.type !== NodeType.Element) {
+    if (!node || meta?.type !== NodeType.Element) {
       continue;
     }
 
+    const attributes = { ...meta.attributes };
+    let changed = false;
+
     for (const [attributeName, value] of Object.entries(mutation.attributes)) {
+      // We only need the handful of attributes that can show up in a click breadcrumb.
+      // Anything else (notably `style`) is dead weight here.
+      if (!BREADCRUMB_RELEVANT_ATTRIBUTES.has(attributeName)) {
+        continue;
+      }
+
       if (value === null) {
         // oxlint-disable-next-line typescript/no-dynamic-delete
-        delete meta.attributes[attributeName];
+        delete attributes[attributeName];
       } else {
-        meta.attributes[attributeName] = value;
+        attributes[attributeName] = value;
       }
+      changed = true;
+    }
+
+    if (changed) {
+      // rrweb hands out the very same serialized node object that it emitted in an earlier `adds`
+      // payload, and that event may still be sitting unserialized in the event buffer (this is the
+      // case whenever compression is disabled). Mutating it in place rewrites already recorded
+      // history, so swap in a copy instead of touching the original.
+      record.mirror.add(node, { ...meta, attributes });
     }
   }
 }

packages/replay-internal/test/unit/coreHandlers/util/getAttributesToRecord.test.ts

@@ -49,3 +49,12 @@ it('records data-sentry-element as data-sentry-component when appropriate', func
     ['data-sentry-component']: 'element',
   });
 });
+
+it('does not write the data-sentry-component fallback back onto the passed attributes', function () {
+  // These are rrweb's serialized attributes, shared with events that may not be serialized yet.
+  const attributes = { ['data-sentry-element']: 'element' };
+
+  getAttributesToRecord(attributes);
+
+  expect(attributes).toEqual({ ['data-sentry-element']: 'element' });
+});

packages/replay-internal/test/unit/util/handleRecordingEmit.test.ts

@@ -175,9 +175,7 @@ describe('Unit | util | handleRecordingEmit', () => {
       },
     };
 
-    vi.spyOn(record.mirror, 'getNode').mockReturnValue(target);
-    vi.spyOn(record.mirror, 'getMeta').mockReturnValue(meta as serializedElementNodeWithId);
-    vi.spyOn(record.mirror, 'getId').mockReturnValue(42);
+    record.mirror.add(target, meta as serializedElementNodeWithId);
 
     syncMirrorAttributesFromMutationEvent({
       type: EventType.IncrementalSnapshot,
@@ -237,8 +235,7 @@ describe('Unit | util | handleRecordingEmit', () => {
       },
     };
 
-    vi.spyOn(record.mirror, 'getNode').mockReturnValue(target);
-    vi.spyOn(record.mirror, 'getMeta').mockReturnValue(meta as serializedElementNodeWithId);
+    record.mirror.add(target, meta as serializedElementNodeWithId);
 
     syncMirrorAttributesFromMutationEvent({
       type: EventType.IncrementalSnapshot,
@@ -259,6 +256,74 @@ describe('Unit | util | handleRecordingEmit', () => {
       },
     });
 
-    expect(meta.attributes['aria-label']).toBe('*********');
+    expect(record.mirror.getMeta(target)?.attributes['aria-label']).toBe('*********');
+  });
+
+  it('does not rewrite the serialized node that was already emitted in an `adds` payload', function () {
+    const target = document.createElement('div');
+
+    // rrweb stores the very same object in the mirror that it emits in `adds`, so a
+    // previously emitted event and the mirror share this reference.
+    const meta = {
+      id: 42,
+      type: NodeType.Element,
+      tagName: 'div',
+      childNodes: [],
+      attributes: {
+        id: 'popover',
+        style: 'position: fixed; left: 0px; top: 0px; transform: translate(0px, -200%); min-width: max-content;',
+      },
+    };
+
+    record.mirror.add(target, meta as serializedElementNodeWithId);
+
+    const addEvent = {
+      type: EventType.IncrementalSnapshot,
+      timestamp: BASE_TIMESTAMP,
+      data: {
+        source: IncrementalSource.Mutation,
+        texts: [],
+        attributes: [],
+        removes: [],
+        adds: [{ parentId: 1, nextId: null, node: meta }],
+      },
+    };
+
+    // rrweb emits a compact style mutation, where `style` is a partial diff object rather
+    // than the full style string.
+    syncMirrorAttributesFromMutationEvent({
+      type: EventType.IncrementalSnapshot,
+      timestamp: BASE_TIMESTAMP + 10,
+      data: {
+        source: IncrementalSource.Mutation,
+        texts: [],
+        attributes: [
+          {
+            id: 42,
+            attributes: {
+              id: 'popover-open',
+              // eslint-disable-next-line @typescript-eslint/no-explicit-any
+              style: { transform: 'translate(631px, 210px)' } as any,
+            },
+          },
+        ],
+        removes: [],
+        adds: [],
+      },
+    });
+
+    // The already emitted event still describes the element as it was when it was serialized.
+    // Buffers that hold events unserialized (i.e. when compression is disabled) would otherwise
+    // ship this partial style diff in place of the full inline style.
+    expect(addEvent.data.adds[0]?.node.attributes).toEqual({
+      id: 'popover',
+      style: 'position: fixed; left: 0px; top: 0px; transform: translate(0px, -200%); min-width: max-content;',
+    });
+
+    // But the mirror is up to date for the attributes that click breadcrumbs care about.
+    expect(record.mirror.getMeta(target)?.attributes).toEqual({
+      id: 'popover-open',
+      style: 'position: fixed; left: 0px; top: 0px; transform: translate(0px, -200%); min-width: max-content;',
+    });
   });
 });