Browse Source

fix(replays): Extract DOM nodes before removal event is rendered (#40421)

Tl/DR: Sometimes the node we want to extract is already removed from the
DOM by the time `BreadcrumbReferencesPlugin.handle` is called. This
means that we'd have a click event in the breadcrumbs, but can't see the
html of the dom node that was clicked.

After this change, on every `IncrementalSnapshot` (every dom change) we
look forward at the next crumb and see if that node is already on the
page. If it is we grab the html for it and can use that as a fallback if
we can't find the node at the timestamp when we need it.

| Before | After |
| --- | --- |
| <img width="948" alt="before"
src="https://user-images.githubusercontent.com/187460/197289052-ea042c8c-5628-4b4e-96a7-436c3687c915.png">
| <img width="947" alt="after"
src="https://user-images.githubusercontent.com/187460/197289073-667d2c93-4482-4564-a12d-923569f97223.png">
|

---

**Long Version**

Previously the RRWeb plugin was only looking at the DOM after each
incremental snapshot had been rendered.

So the exec steps are something like:
1. RRWeb renders full dom snapshot
2. RRWeb renders incremental eventId=1
3. Extractor finds sentry breadcrumb with the highest timestamp that is
before the timestamp of eventId=1
4. Extractor searches in dom for the domnode related to the breadcrumb
5. goto step 2 with next event

The problem is that if the incremental render in 2 removed something,
then we can see it in step 4. Our plugin code runs in what could be
called an 'afterRender' callback, we'd need a 'beforeRender'.

So what I added is a look ahead step, it helps to find the HTML related
to the next breadcrumb, not the current breadcrumb:
4. Extractor searches in dom for the domnode related to the breadcrumb
(as before)
5. Extractor peeks the next breadcrumb in the list
6. Extractor searches in dom for the domnode related to the next
breadcrumb, and puts it inside `nextExtract` as a fallback

It's a fallback because step 2 could add content into the domnode that
the breadcrumb points towards, so we want to prefer that. But also step
2 could remove the node, so we need to read it before that happens.


This isn't really efficient, we're grabbing often the same html over and
over again :(


Fixes #40399
Ryan Albrecht 2 years ago
parent
commit
baf8bb6ac4

+ 52 - 28
static/app/utils/replays/hooks/useExtractedCrumbHtml.tsx

@@ -139,6 +139,7 @@ class BreadcrumbReferencesPlugin {
   isFinished: (event: eventWithTime) => boolean;
   onFinish: (mutations: Extraction[]) => void;
 
+  nextExtract: null | Extraction['html'] = null;
   activities: Extraction[] = [];
 
   constructor({crumbs, isFinished, onFinish}: PluginOpts) {
@@ -149,40 +150,63 @@ class BreadcrumbReferencesPlugin {
 
   handler(event: eventWithTime, _isSync: boolean, {replayer}: {replayer: Replayer}) {
     if (event.type === EventType.IncrementalSnapshot) {
-      const crumb = first(this.crumbs);
-      const nextTimestamp = +new Date(crumb?.timestamp || '');
-
-      if (crumb && nextTimestamp && nextTimestamp <= event.timestamp) {
-        // we passed the next one, grab the dom, and pop the timestamp off
-        const mirror = replayer.getMirror();
-        // @ts-expect-error
-        const node = mirror.getNode(crumb.data?.nodeId || '');
-        // @ts-expect-error
-        const html = node?.outerHTML || node?.textContent || '';
-
-        // Limit document node depth to 2
-        let truncated = removeNodesAtLevel(html, 2);
-        // If still very long and/or removeNodesAtLevel failed, truncate
-        if (truncated.length > 1500) {
-          truncated = truncated.substring(0, 1500);
-        }
-
-        if (truncated) {
-          this.activities.push({
-            crumb,
-            html: truncated,
-            timestamp: nextTimestamp,
-          });
-        }
-
-        this.crumbs.shift();
-      }
+      this.extractCurrentCrumb(event, {replayer});
+      this.extractNextCrumb({replayer});
     }
 
     if (this.isFinished(event)) {
       this.onFinish(this.activities);
     }
   }
+
+  extractCurrentCrumb(event: eventWithTime, {replayer}: {replayer: Replayer}) {
+    const crumb = first(this.crumbs);
+    const crumbTimestamp = +new Date(crumb?.timestamp || '');
+
+    if (!crumb || !crumbTimestamp || crumbTimestamp > event.timestamp) {
+      return;
+    }
+
+    const truncated = extractNode(crumb, replayer) || this.nextExtract;
+    if (truncated) {
+      this.activities.push({
+        crumb,
+        html: truncated,
+        timestamp: crumbTimestamp,
+      });
+    }
+
+    this.nextExtract = null;
+    this.crumbs.shift();
+  }
+
+  extractNextCrumb({replayer}: {replayer: Replayer}) {
+    const crumb = first(this.crumbs);
+    const crumbTimestamp = +new Date(crumb?.timestamp || '');
+
+    if (!crumb || !crumbTimestamp) {
+      return;
+    }
+
+    this.nextExtract = extractNode(crumb, replayer);
+  }
+}
+
+function extractNode(crumb: Crumb, replayer: Replayer) {
+  const mirror = replayer.getMirror();
+  // @ts-expect-error
+  const nodeId = crumb.data?.nodeId || '';
+  const node = mirror.getNode(nodeId);
+  // @ts-expect-error
+  const html = node?.outerHTML || node?.textContent || '';
+
+  // Limit document node depth to 2
+  let truncated = removeNodesAtLevel(html, 2);
+  // If still very long and/or removeNodesAtLevel failed, truncate
+  if (truncated.length > 1500) {
+    truncated = truncated.substring(0, 1500);
+  }
+  return truncated;
 }
 
 function removeNodesAtLevel(html: string, level: number) {

+ 1 - 0
static/app/views/replays/detail/domMutations/index.tsx

@@ -296,6 +296,7 @@ const MutationMessage = styled('p')`
 const CodeContainer = styled('div')`
   max-height: 400px;
   max-width: 100%;
+  overflow: auto;
 `;
 
 export default DomMutations;