Просмотр исходного кода

bug(replays): Fix loading Replay Details at a specific timestamp (#50779)

The issue was a race-condition between setting up the `replayerRef` and
getting the timestamp/highlight config out of the query params.

When the query params are set to `?t=` or `?event_t=` then getting that
value is fast, and will be done before the ref is ready. In this case,
previously, we tried to set the current time, didn't have a ref, and
that was all.
Now we set the current timestamp whenever we're updating the replayRef.

Tested by setting `?t=30`: was able to see that we jumped to the correct
timestamp
Also by doing a click search for `click.tag:button` and then clicking
through to some results: saw that the video did not start at 0 anymore.

Fixes https://github.com/getsentry/sentry/issues/50312
Fixes https://github.com/getsentry/sentry/issues/49980
Fixes https://github.com/getsentry/sentry/issues/50843
Ryan Albrecht 1 год назад
Родитель
Сommit
e93ccec710
1 измененных файлов с 95 добавлено и 91 удалено
  1. 95 91
      static/app/components/replays/replayContext.tsx

+ 95 - 91
static/app/components/replays/replayContext.tsx

@@ -243,7 +243,7 @@ export function Provider({
   const [fastForwardSpeed, setFFSpeed] = useState(0);
   const [buffer, setBufferTime] = useState({target: -1, previous: -1});
   const playTimer = useRef<number | undefined>(undefined);
-  const unMountedRef = useRef(false);
+  const didApplyInitialOffset = useRef(false);
 
   const isFinished = replayerRef.current?.getCurrentTime() === finishedAtMS;
 
@@ -289,23 +289,97 @@ export function Provider({
     setIsPlaying(false);
   }, []);
 
-  const initRoot = useCallback(
-    (root: RootElem) => {
-      if (events === undefined) {
+  const getCurrentTime = useCallback(
+    () => (replayerRef.current ? Math.max(replayerRef.current.getCurrentTime(), 0) : 0),
+    []
+  );
+
+  const privateSetCurrentTime = useCallback(
+    (requestedTimeMs: number) => {
+      const replayer = replayerRef.current;
+      if (!replayer) {
         return;
       }
 
-      if (root === null) {
-        return;
+      const skipInactive = replayer.config;
+      if (skipInactive) {
+        // If the replayer is set to skip inactive, we should turn it off before
+        // manually scrubbing, so when the player resumes playing its not stuck
+        replayer.setConfig({skipInactive: false});
       }
 
-      if (isFetching) {
+      const maxTimeMs = replayerRef.current?.getMetaData().totalTime;
+      const time = requestedTimeMs > maxTimeMs ? 0 : requestedTimeMs;
+
+      // Sometimes rrweb doesn't get to the exact target time, as long as it has
+      // changed away from the previous time then we can hide then buffering message.
+      setBufferTime({target: time, previous: getCurrentTime()});
+
+      // Clear previous timers. Without this (but with the setTimeout) multiple
+      // requests to set the currentTime could finish out of order and cause jumping.
+      if (playTimer.current) {
+        window.clearTimeout(playTimer.current);
+      }
+      if (skipInactive) {
+        replayer.setConfig({skipInactive: true});
+      }
+      if (isPlaying) {
+        playTimer.current = window.setTimeout(() => replayer.play(time), 0);
+        setIsPlaying(true);
+      } else {
+        playTimer.current = window.setTimeout(() => replayer.pause(time), 0);
+        setIsPlaying(false);
+      }
+    },
+    [getCurrentTime, isPlaying]
+  );
+
+  const setCurrentTime = useCallback(
+    (requestedTimeMs: number) => {
+      privateSetCurrentTime(requestedTimeMs);
+      clearAllHighlightsCallback();
+    },
+    [privateSetCurrentTime, clearAllHighlightsCallback]
+  );
+
+  const applyInitialOffset = useCallback(() => {
+    if (
+      !didApplyInitialOffset.current &&
+      initialTimeOffsetMs &&
+      events &&
+      replayerRef.current
+    ) {
+      const {highlight: highlightArgs, offsetMs} = initialTimeOffsetMs;
+      if (offsetMs > 0) {
+        privateSetCurrentTime(offsetMs);
+      }
+      if (highlightArgs) {
+        highlight(highlightArgs);
+        setTimeout(() => {
+          clearAllHighlightsCallback();
+          highlight(highlightArgs);
+        });
+      }
+      didApplyInitialOffset.current = true;
+    }
+  }, [
+    clearAllHighlightsCallback,
+    events,
+    highlight,
+    initialTimeOffsetMs,
+    privateSetCurrentTime,
+  ]);
+
+  useEffect(clearAllHighlightsCallback, [clearAllHighlightsCallback, isPlaying]);
+
+  const initRoot = useCallback(
+    (root: RootElem) => {
+      if (events === undefined || root === null || isFetching) {
         return;
       }
 
       if (replayerRef.current) {
-        if (!hasNewEvents && !unMountedRef.current) {
-          // Already have a player for these events, the parent node must've re-rendered
+        if (!hasNewEvents) {
           return;
         }
 
@@ -352,56 +426,16 @@ export function Provider({
       // @ts-expect-error
       replayerRef.current = inst;
 
-      if (unMountedRef.current) {
-        unMountedRef.current = false;
-      }
-    },
-    [events, isFetching, theme.purple200, setReplayFinished, hasNewEvents]
-  );
-
-  const getCurrentTime = useCallback(
-    () => (replayerRef.current ? Math.max(replayerRef.current.getCurrentTime(), 0) : 0),
-    []
-  );
-
-  const setCurrentTime = useCallback(
-    (requestedTimeMs: number) => {
-      const replayer = replayerRef.current;
-      if (!replayer) {
-        return;
-      }
-
-      const skipInactive = replayer.config;
-      if (skipInactive) {
-        // If the replayer is set to skip inactive, we should turn it off before
-        // manually scrubbing, so when the player resumes playing its not stuck
-        replayer.setConfig({skipInactive: false});
-      }
-
-      const maxTimeMs = replayerRef.current?.getMetaData().totalTime;
-      const time = requestedTimeMs > maxTimeMs ? 0 : requestedTimeMs;
-
-      // Sometimes rrweb doesn't get to the exact target time, as long as it has
-      // changed away from the previous time then we can hide then buffering message.
-      setBufferTime({target: time, previous: getCurrentTime()});
-
-      // Clear previous timers. Without this (but with the setTimeout) multiple
-      // requests to set the currentTime could finish out of order and cause jumping.
-      if (playTimer.current) {
-        window.clearTimeout(playTimer.current);
-      }
-      if (skipInactive) {
-        replayer.setConfig({skipInactive: true});
-      }
-      if (isPlaying) {
-        playTimer.current = window.setTimeout(() => replayer.play(time), 0);
-        setIsPlaying(true);
-      } else {
-        playTimer.current = window.setTimeout(() => replayer.pause(time), 0);
-        setIsPlaying(false);
-      }
+      applyInitialOffset();
     },
-    [getCurrentTime, isPlaying]
+    [
+      events,
+      isFetching,
+      theme.purple200,
+      setReplayFinished,
+      hasNewEvents,
+      applyInitialOffset,
+    ]
   );
 
   const setSpeed = useCallback(
@@ -496,17 +530,6 @@ export function Provider({
     setIsSkippingInactive(skip);
   }, []);
 
-  // Only on pageload: set the initial playback timestamp
-  useEffect(() => {
-    if (initialTimeOffsetMs?.offsetMs && events && replayerRef.current) {
-      setCurrentTime(initialTimeOffsetMs.offsetMs);
-    }
-
-    return () => {
-      unMountedRef.current = true;
-    };
-  }, [events, initialTimeOffsetMs, setCurrentTime]);
-
   const currentPlayerTime = useCurrentTime(getCurrentTime);
 
   const [isBuffering, currentTime] =
@@ -516,30 +539,11 @@ export function Provider({
       ? [true, buffer.target]
       : [false, currentPlayerTime];
 
-  // Only on pageload: highlight the node that relates to the initialTimeOffset
   useEffect(() => {
-    if (
-      !isBuffering &&
-      initialTimeOffsetMs?.highlight &&
-      events &&
-      events?.length >= 2 &&
-      replayerRef.current
-    ) {
-      const highlightArgs = initialTimeOffsetMs.highlight;
-      highlight(highlightArgs);
-      setTimeout(() => {
-        clearAllHighlightsCallback();
-        highlight(highlightArgs);
-      });
+    if (!isBuffering && events && events.length >= 2 && replayerRef.current) {
+      applyInitialOffset();
     }
-  }, [
-    clearAllHighlightsCallback,
-    events,
-    dimensions,
-    highlight,
-    initialTimeOffsetMs,
-    isBuffering,
-  ]);
+  }, [isBuffering, events, applyInitialOffset]);
 
   useEffect(() => {
     if (!isBuffering && buffer.target !== -1) {