fix: Fix infinite render loops for values from DB (#522)

This pach overhauls the initial rerender with better use of hooks, namely `useCallback` but also using intial state creation function to fix the infinite render loop reported in #521. Fixes #521.
getsentry · Sep 12, 2024 · df9cd95 · df9cd95
1 parent 79d9beb
commit df9cd95
Show file tree

Hide file tree

Showing 3 changed files with 130 additions and 89 deletions.
diff --git a/.changeset/big-countries-explode.md b/.changeset/big-countries-explode.md
@@ -0,0 +1,8 @@
+---
+'@spotlightjs/overlay': patch
+'@spotlightjs/astro': patch
+'@spotlightjs/electron': patch
+'@spotlightjs/spotlight': patch
+---
+
+Fixed infinite render loops and optimized rerenders
diff --git a/packages/overlay/src/App.tsx b/packages/overlay/src/App.tsx
@@ -1,4 +1,4 @@
-import { useEffect, useMemo, useState } from 'react';
+import { useCallback, useEffect, useMemo, useState } from 'react';
 import { useNavigate } from 'react-router-dom';
 import Debugger from './components/Debugger';
 import Trigger from './components/Trigger';
@@ -15,6 +15,14 @@ type AppProps = Omit<SpotlightOverlayOptions, 'debug' | 'injectImmediately'> &
 
 type EventData = { contentType: string; data: string | Uint8Array };
 
+const processEvent = (contentType: string, event: { data: string | Uint8Array }, integration: Integration) =>
+  integration.processEvent
+    ? integration.processEvent({
+        contentType,
+        data: event.data,
+      })
+    : { event };
+
 export default function App({
   openOnInit = false,
   showTriggerButton = true,
@@ -25,7 +33,25 @@ export default function App({
   showClearEventsButton = true,
   initialEvents = {},
 }: AppProps) {
-  const [integrationData, setIntegrationData] = useState<IntegrationData<unknown>>({});
+  const [integrationData, setIntegrationData] = useState<IntegrationData<unknown>>(() => {
+    const result: IntegrationData<unknown> = {};
+    // Process initial events
+    for (const integration of integrations) {
+      result[integration.name] = [];
+      for (const contentType in initialEvents) {
+        if (!integration.forwardedContentType?.includes(contentType)) {
+          continue;
+        }
+        for (const data of initialEvents[contentType]) {
+          const processedEvent = processEvent(contentType, { data }, integration);
+          if (processedEvent) {
+            result[integration.name].push(processedEvent);
+          }
+        }
+      }
+    }
+    return result;
+  });
   const [isOnline, setOnline] = useState(false);
   const [triggerButtonCount, setTriggerButtonCount] = useState<NotificationCount>({ count: 0, severe: false });
   const [isOpen, setOpen] = useState(openOnInit);
@@ -52,12 +78,7 @@ export default function App({
       const listener = (event: { data: string | Uint8Array }): void => {
         log(`Received new ${contentType} event`);
         for (const integration of integrations) {
-          const newIntegrationData = integration.processEvent
-            ? integration.processEvent({
-                contentType,
-                data: event.data,
-              })
-            : { event };
+          const newIntegrationData = processEvent(contentType, event, integration);
 
           if (!newIntegrationData) {
             continue;
@@ -86,115 +107,113 @@ export default function App({
 
   const spotlightEventTarget = useMemo(() => getSpotlightEventTarget(), []);
 
+  const dispatchToContentTypeListener = useCallback(
+    ({ contentType, data }: EventData) => {
+      const listener = contentTypeListeners[contentType];
+      if (!listener) {
+        log('Got event for unknown content type:', contentType);
+        return;
+      }
+      listener({ data });
+    },
+    [contentTypeListeners],
+  );
+
+  useEffect(() => {
+    // Populate from DB
+    db.getEntries().then(entries => {
+      for (const detail of entries as EventData[]) {
+        dispatchToContentTypeListener(detail);
+      }
+    });
+  }, [dispatchToContentTypeListener]);
+
   // Note that `useNavigate()` relies on `useLocation()` which
   // causes a full re-render here every time we change the location
   // We can fix this by doing `const router = createMemoryRouter()`
   // and using `router.navigate()` but that requires a larger refactor
   // as our <Route>s are scattered around a bit
   // See https://github.com/remix-run/react-router/issues/7634
   const navigate = useNavigate();
-  const eventHandlers = useMemo(() => {
-    log('useMemo: initializing event handlers');
-    const clearEvents = async () => {
-      const { origin } = new URL(sidecarUrl);
-      const clearEventsUrl: string = `${origin}/clear`;
-
-      try {
-        await db.reset();
-        await fetch(clearEventsUrl, {
-          method: 'DELETE',
-          mode: 'cors',
-        });
-      } catch (err) {
-        console.error(
-          `Spotlight can't connect to Sidecar is it running? See: https://spotlightjs.com/sidecar/npx/`,
-          err,
-        );
-        return;
-      }
+  const clearEvents = useCallback(async () => {
+    const { origin } = new URL(sidecarUrl);
+    const clearEventsUrl: string = `${origin}/clear`;
+
+    try {
+      await db.reset();
+      await fetch(clearEventsUrl, {
+        method: 'DELETE',
+        mode: 'cors',
+      });
+    } catch (err) {
+      console.error(`Spotlight can't connect to Sidecar is it running? See: https://spotlightjs.com/sidecar/npx/`, err);
+      return;
+    }
 
-      for (const integration of integrations) {
-        setIntegrationData(prev => ({ ...prev, [integration.name]: [] }));
-        if (integration.reset) integration.reset();
-      }
-    };
+    for (const integration of integrations) {
+      setIntegrationData(prev => ({ ...prev, [integration.name]: [] }));
+      if (integration.reset) integration.reset();
+    }
+  }, [integrations, sidecarUrl]);
 
-    const onOpen = (
+  const onOpen = useCallback(
+    (
       e: CustomEvent<{
         path: string | undefined;
       }>,
     ) => {
       log('Open');
       setOpen(true);
       if (e.detail.path) navigate(e.detail.path);
-    };
+    },
+    [navigate],
+  );
 
-    const onClose = () => {
-      log('Close');
-      setOpen(false);
-    };
+  const onClose = useCallback(() => {
+    log('Close');
+    setOpen(false);
+  }, []);
 
-    const onToggle = () => {
-      log('Toggle');
-      setOpen(prev => !prev);
-    };
+  const onToggle = useCallback(() => {
+    log('Toggle');
+    setOpen(prev => !prev);
+  }, []);
 
-    const onNavigate = (e: CustomEvent<string>) => {
+  const onNavigate = useCallback(
+    (e: CustomEvent<string>) => {
       log('Navigate');
       navigate(e.detail);
-    };
-
-    const dispatchToContentTypeListener = ({ contentType, data }: EventData) => {
-      const listener = contentTypeListeners[contentType];
-      if (!listener) {
-        log('Got event for unknown content type:', contentType);
-        return;
-      }
-      listener({ data });
-    };
+    },
+    [navigate],
+  );
 
-    const onEvent = ({ detail }: CustomEvent<EventData>) => {
+  const onEvent = useCallback(
+    ({ detail }: CustomEvent<EventData>) => {
       dispatchToContentTypeListener(detail);
       db.add(detail);
-    };
-
-    // Populate from DB
-    db.getEntries().then(entries => {
-      for (const detail of entries as EventData[]) {
-        dispatchToContentTypeListener(detail);
-      }
-    });
-
-    // Populate from initial events
-    for (const contentType in initialEvents) {
-      log(`Injecting initial events for ${contentType}`);
-      for (const data of initialEvents[contentType]) {
-        dispatchToContentTypeListener({ contentType, data });
-      }
-    }
-
-    return { clearEvents, onEvent, onOpen, onClose, onNavigate, onToggle };
-  }, [integrations, navigate, sidecarUrl, contentTypeListeners, initialEvents]);
+    },
+    [dispatchToContentTypeListener],
+  );
 
-  useKeyPress('F12', ['ctrlKey'], eventHandlers.onToggle);
+  useKeyPress('F12', ['ctrlKey'], onToggle);
 
   useEffect(() => {
     log('useEffect: Adding event listeners');
-    spotlightEventTarget.addEventListener('open', eventHandlers.onOpen as EventListener);
-    spotlightEventTarget.addEventListener('close', eventHandlers.onClose);
-    spotlightEventTarget.addEventListener('navigate', eventHandlers.onNavigate as EventListener);
-    spotlightEventTarget.addEventListener('clearEvents', eventHandlers.clearEvents as EventListener);
-    spotlightEventTarget.addEventListener('event', eventHandlers.onEvent as EventListener);
+    spotlightEventTarget.addEventListener('open', onOpen as EventListener);
+    spotlightEventTarget.addEventListener('close', onClose);
+    spotlightEventTarget.addEventListener('navigate', onNavigate as EventListener);
+    spotlightEventTarget.addEventListener('clearEvents', clearEvents as EventListener);
+    spotlightEventTarget.addEventListener('event', onEvent as EventListener);
 
     return (): undefined => {
       log('useEffect[destructor]: Removing event listeners');
-      spotlightEventTarget.removeEventListener('open', eventHandlers.onOpen as EventListener);
-      spotlightEventTarget.removeEventListener('close', eventHandlers.onClose);
-      spotlightEventTarget.removeEventListener('navigate', eventHandlers.onNavigate as EventListener);
-      spotlightEventTarget.removeEventListener('clearEvents', eventHandlers.clearEvents as EventListener);
-      spotlightEventTarget.removeEventListener('event', eventHandlers.onEvent as EventListener);
+      spotlightEventTarget.removeEventListener('open', onOpen as EventListener);
+      spotlightEventTarget.removeEventListener('close', onClose);
+      spotlightEventTarget.removeEventListener('navigate', onNavigate as EventListener);
+      spotlightEventTarget.removeEventListener('clearEvents', clearEvents as EventListener);
+      spotlightEventTarget.removeEventListener('event', onEvent as EventListener);
     };
-  }, [spotlightEventTarget, eventHandlers]);
+  }, [spotlightEventTarget, onOpen, onClose, onNavigate, clearEvents, onEvent]);
 
   useEffect(() => {
     if (!isOpen) {

diff --git a/packages/overlay/src/lib/db.ts b/packages/overlay/src/lib/db.ts
@@ -1,4 +1,5 @@
 export const MAX_AGE = 30 * 60 * 1000; // 30 minutes
+export const MAX_ITEMS = 1000;
 export const DB_NAME = 'SentrySpotlight';
 export const OBJECT_STORE_NAME = 'events';
 export const DB_VERSION = 2;
@@ -85,11 +86,24 @@ export async function getEntries() {
   const { promise, resolve, reject } = promiseWithResolvers();
   const rejectFromErrorEvent = (evt: Event) => reject((evt.target as IDBOpenDBRequest).error);
   const db = await getDB();
-  const tx = db.transaction([OBJECT_STORE_NAME], 'readonly');
+  const tx = db.transaction([OBJECT_STORE_NAME], 'readwrite');
   tx.onerror = rejectFromErrorEvent;
-  const getRequest = tx.objectStore(OBJECT_STORE_NAME).getAll();
-  getRequest.onerror = rejectFromErrorEvent;
-  getRequest.onsuccess = () => resolve(getRequest.result.map(({ value }) => value));
+
+  const items: unknown[] = [];
+  const cursorRequest = tx.objectStore(OBJECT_STORE_NAME).openCursor();
+  cursorRequest.onerror = rejectFromErrorEvent;
+  cursorRequest.onsuccess = () => {
+    const cursor = cursorRequest.result;
+    if (!cursor) resolve(items);
+    else {
+      if (items.length < MAX_ITEMS) {
+        items.push(cursor.value.value);
+      } else {
+        cursor.delete();
+      }
+      cursor.continue();
+    }
+  };
   return promise;
 }