From 389ce1dac5fa59e7f79e432b0f3d952a6d20d840 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 6 Aug 2024 14:28:42 +0100 Subject: [PATCH] fix: Various React improvements --- .changeset/twenty-ghosts-float.md | 8 + .vscode/settings.json | 1 + packages/overlay/src/App.tsx | 137 ++++++++++-------- packages/overlay/src/components/Debugger.tsx | 4 +- packages/overlay/src/components/Overview.tsx | 26 ++-- packages/overlay/src/index.tsx | 24 ++- .../src/integrations/console/console-tab.tsx | 14 +- .../sentry/components/HiddenItemsButton.tsx | 2 +- .../sentry/components/events/EventDetails.tsx | 2 +- .../webVitals/PerformanceChart.tsx | 2 +- .../sentry/components/traces/TraceIcon.tsx | 2 +- .../sentry/data/sentryEventsContext.tsx | 24 +-- packages/overlay/src/lib/eventTarget.ts | 2 +- packages/overlay/src/lib/useKeyPress.ts | 13 +- packages/overlay/src/sidecar.ts | 8 +- .../src/ui/Chart/RingChart/RingChart.tsx | 19 ++- 16 files changed, 155 insertions(+), 133 deletions(-) create mode 100644 .changeset/twenty-ghosts-float.md diff --git a/.changeset/twenty-ghosts-float.md b/.changeset/twenty-ghosts-float.md new file mode 100644 index 00000000..01c96827 --- /dev/null +++ b/.changeset/twenty-ghosts-float.md @@ -0,0 +1,8 @@ +--- +'@spotlightjs/overlay': patch +'@spotlightjs/astro': patch +'@spotlightjs/electron': patch +'@spotlightjs/spotlight': patch +--- + +Various improvements in React code for better stability & performance diff --git a/.vscode/settings.json b/.vscode/settings.json index 81af5f75..0c6fa872 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -4,6 +4,7 @@ "Astro", "Astro's", "contextlines", + "Endcaps", "fontsource", "pageload", "spotlightjs", diff --git a/packages/overlay/src/App.tsx b/packages/overlay/src/App.tsx index 535836c5..a7d11ce7 100644 --- a/packages/overlay/src/App.tsx +++ b/packages/overlay/src/App.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import { useNavigate } from 'react-router-dom'; import Debugger from './components/Debugger'; import Trigger from './components/Trigger'; @@ -7,7 +7,7 @@ import { getSpotlightEventTarget } from './lib/eventTarget'; import { log } from './lib/logger'; import useKeyPress from './lib/useKeyPress'; import { connectToSidecar } from './sidecar'; -import { NotificationCount, SpotlightOverlayOptions } from './types'; +import type { NotificationCount, SpotlightOverlayOptions } from './types'; type AppProps = Omit & Required>; @@ -21,65 +21,69 @@ export default function App({ fullPage = false, showClearEventsButton = true, }: AppProps) { - log('App rerender'); const [integrationData, setIntegrationData] = useState>({}); const [isOnline, setOnline] = useState(false); const [triggerButtonCount, setTriggerButtonCount] = useState({ count: 0, severe: false }); const [isOpen, setOpen] = useState(openOnInit); - const [reloadSpotlight, setReloadSpotlight] = useState(0); - - useKeyPress(['ctrlKey', 'F12'], () => { - setOpen(prev => !prev); - }); + log('App rerender', integrationData, isOnline, triggerButtonCount, isOpen); + + // Map that holds the information which kind of content type should be dispatched to which integration(s) + const contentTypeToIntegrations = useMemo(() => { + const result = new Map(); + for (const integration of integrations) { + if (!integration.forwardedContentType) continue; + + for (const contentType of integration.forwardedContentType) { + let integrationsForContentType = result.get(contentType); + if (!integrationsForContentType) { + integrationsForContentType = []; + result.set(contentType, integrationsForContentType); + } + integrationsForContentType.push(integration); + } + } + return result; + }, [integrations]); - useEffect(() => { - // Map that holds the information which kind of content type should be dispatched to which integration(s) - const contentTypeToIntegrations = new Map(); - - integrations.forEach( - integration => - integration.forwardedContentType?.forEach(contentType => { - const i = contentTypeToIntegrations.get(contentType) || []; - i.push(integration); - contentTypeToIntegrations.set(contentType, i); - }), - ); - - const cleanupListeners = connectToSidecar(sidecarUrl, contentTypeToIntegrations, setIntegrationData, setOnline); - - return () => { - log('useEffect cleanup'); - cleanupListeners(); - }; - }, [integrations, sidecarUrl, reloadSpotlight]); + useEffect( + () => connectToSidecar(sidecarUrl, contentTypeToIntegrations, setIntegrationData, setOnline) as () => undefined, + [sidecarUrl, contentTypeToIntegrations], + ); - const spotlightEventTarget = getSpotlightEventTarget(); + const spotlightEventTarget = useMemo(() => getSpotlightEventTarget(), []); + // 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 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 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 = () => { - const { origin } = new URL(sidecarUrl); - const clearEventsUrl: string = `${origin}/clear`; - - 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; - }); - - integrations.forEach(integration => { - const IntegrationName = integration.name; - if (integration.processEvent) { - setIntegrationData(prev => ({ ...prev, [IntegrationName]: [] })); + for (const integration of integrations) { + setIntegrationData(prev => ({ ...prev, [integration.name]: [] })); if (integration.reset) integration.reset(); } - }); - setReloadSpotlight(prev => prev + 1); - }; + }; - useEffect(() => { const onOpen = ( e: CustomEvent<{ path: string | undefined; @@ -95,23 +99,36 @@ export default function App({ setOpen(false); }; + const onToggle = () => { + log('Toggle'); + setOpen(prev => !prev); + }; + const onNavigate = (e: CustomEvent) => { log('Navigate'); navigate(e.detail); }; - spotlightEventTarget.addEventListener('open', onOpen as EventListener); - spotlightEventTarget.addEventListener('close', onClose); - spotlightEventTarget.addEventListener('navigate', onNavigate as EventListener); - spotlightEventTarget.addEventListener('clearEvents', clearEvents as EventListener); + return { clearEvents, onOpen, onClose, onNavigate, onToggle }; + }, [integrations, navigate, sidecarUrl]); - return () => { - spotlightEventTarget.removeEventListener('open', onOpen as EventListener); - spotlightEventTarget.removeEventListener('close', onClose); - spotlightEventTarget.removeEventListener('navigate', onNavigate as EventListener); - spotlightEventTarget.removeEventListener('clearEvents', clearEvents as EventListener); + useKeyPress(['ctrlKey', 'F12'], eventHandlers.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); + + 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, navigate]); + }, [spotlightEventTarget, eventHandlers]); useEffect(() => { if (!isOpen) { @@ -131,8 +148,6 @@ export default function App({ } }, [triggerButtonCount, spotlightEventTarget]); - log('Integration data:', integrationData); - return ( <> {showTriggerButton && ( diff --git a/packages/overlay/src/components/Debugger.tsx b/packages/overlay/src/components/Debugger.tsx index a9d39a03..5d4e0d1e 100644 --- a/packages/overlay/src/components/Debugger.tsx +++ b/packages/overlay/src/components/Debugger.tsx @@ -1,8 +1,8 @@ import { ReactComponent as DeleteIcon } from '~/assets/deleteIcon.svg'; import { ReactComponent as Logo } from '~/assets/glyph.svg'; -import { Integration, IntegrationData } from '~/integrations/integration'; +import type { Integration, IntegrationData } from '~/integrations/integration'; import { getSpotlightEventTarget } from '~/lib/eventTarget'; -import { NotificationCount } from '~/types'; +import type { NotificationCount } from '~/types'; import classNames from '../lib/classNames'; import Overview from './Overview'; diff --git a/packages/overlay/src/components/Overview.tsx b/packages/overlay/src/components/Overview.tsx index 69c2f234..6e5b3b6a 100644 --- a/packages/overlay/src/components/Overview.tsx +++ b/packages/overlay/src/components/Overview.tsx @@ -1,7 +1,7 @@ import { createElement, useEffect, useState } from 'react'; import { Route, Routes } from 'react-router-dom'; -import { Integration, IntegrationData } from '~/integrations/integration'; -import { NotificationCount } from '~/types'; +import type { Integration, IntegrationData } from '~/integrations/integration'; +import type { NotificationCount } from '~/types'; import Tabs from './Tabs'; export default function Overview({ @@ -17,18 +17,16 @@ export default function Overview({ }) { const [notificationCountSum, setNotificationCountSum] = useState({ count: 0, severe: false }); - const tabs = integrations - .map(integration => { - if (integration.tabs) { - const processedEvents = integrationData[integration.name]?.map(container => container.event) || []; - return integration.tabs({ processedEvents }).map(tab => ({ - ...tab, - processedEvents: processedEvents, - })); - } - return []; - }) - .flat(); + const tabs = integrations.flatMap(integration => { + if (integration.tabs) { + const processedEvents = integrationData[integration.name]?.map(container => container.event) || []; + return integration.tabs({ processedEvents }).map(tab => ({ + ...tab, + processedEvents: processedEvents, + })); + } + return []; + }); const newNotificationSum = tabs.reduce( (sum, tab) => ({ diff --git a/packages/overlay/src/index.tsx b/packages/overlay/src/index.tsx index bd9ecf07..efbdf19d 100644 --- a/packages/overlay/src/index.tsx +++ b/packages/overlay/src/index.tsx @@ -11,7 +11,7 @@ import { off, on, trigger } from './lib/eventTarget.ts'; import { activateLogger, log } from './lib/logger.ts'; import { SpotlightContextProvider } from './lib/useSpotlightContext.tsx'; import { React, ReactDOM } from './react-instance.tsx'; // Import specific exports -import { SpotlightOverlayOptions, WindowWithSpotlight } from './types.ts'; +import type { SpotlightOverlayOptions, WindowWithSpotlight } from './types.ts'; export { default as console } from './integrations/console/index.ts'; export { default as hydrationError } from './integrations/hydration-error/index.ts'; @@ -23,10 +23,10 @@ export { DEFAULT_ANCHOR, DEFAULT_EXPERIMENTS, DEFAULT_SIDECAR_URL, - React, - ReactDOM, off, on, + React, + ReactDOM, trigger, }; @@ -146,17 +146,13 @@ export async function init({ }); } - const tabs = initializedIntegrations - .map(integration => { - if (integration.tabs) { - return integration.tabs({ processedEvents: [] }).map(tab => ({ - ...tab, - processedEvents: [], - })); - } - return []; - }) - .flat(); + const tabs = initializedIntegrations.flatMap( + integration => + integration.tabs?.({ processedEvents: [] }).map(tab => ({ + ...tab, + processedEvents: [], + })) || [], + ); const initialTab = tabs.length ? `/${tabs[0].id}` : '/no-tabs'; diff --git a/packages/overlay/src/integrations/console/console-tab.tsx b/packages/overlay/src/integrations/console/console-tab.tsx index dd72ed9c..205c5fb1 100644 --- a/packages/overlay/src/integrations/console/console-tab.tsx +++ b/packages/overlay/src/integrations/console/console-tab.tsx @@ -10,14 +10,12 @@ export default function ConsoleTab({ processedEvents }: Props) {

Console Logs

- {messages.map((message, index) => { - return ( -
- {message.type.toUpperCase()} - {message.msg} -
- ); - })} + {messages.map(message => ( +
+ {message.type.toUpperCase()} + {message.msg} +
+ ))}
); diff --git a/packages/overlay/src/integrations/sentry/components/HiddenItemsButton.tsx b/packages/overlay/src/integrations/sentry/components/HiddenItemsButton.tsx index a34578a5..2e18d893 100644 --- a/packages/overlay/src/integrations/sentry/components/HiddenItemsButton.tsx +++ b/packages/overlay/src/integrations/sentry/components/HiddenItemsButton.tsx @@ -14,7 +14,7 @@ export default function HiddenItemsButton({ {itemCount.toLocaleString()} {itemCount !== 1 ? 'items were' : 'item was'} hidden from different sessions. - Reveal + Reveal ); } diff --git a/packages/overlay/src/integrations/sentry/components/events/EventDetails.tsx b/packages/overlay/src/integrations/sentry/components/events/EventDetails.tsx index 0ab72e28..ee563720 100644 --- a/packages/overlay/src/integrations/sentry/components/events/EventDetails.tsx +++ b/packages/overlay/src/integrations/sentry/components/events/EventDetails.tsx @@ -2,7 +2,7 @@ import { useState } from 'react'; import { Link, Outlet, Route, Routes, useParams } from 'react-router-dom'; import Tabs from '../../../../components/Tabs'; import sentryDataCache from '../../data/sentryDataCache'; -import { SentryEvent } from '../../types'; +import type { SentryEvent } from '../../types'; import PlatformIcon from '../PlatformIcon'; import Event, { EventTitle } from './Event'; import EventBreadcrumbs from './EventBreadcrumbs'; diff --git a/packages/overlay/src/integrations/sentry/components/performance/webVitals/PerformanceChart.tsx b/packages/overlay/src/integrations/sentry/components/performance/webVitals/PerformanceChart.tsx index b025b505..e7c2e8f1 100644 --- a/packages/overlay/src/integrations/sentry/components/performance/webVitals/PerformanceChart.tsx +++ b/packages/overlay/src/integrations/sentry/components/performance/webVitals/PerformanceChart.tsx @@ -184,7 +184,7 @@ const PerformanceChart = ({ () => setWebVitalTooltip('cls'), () => setWebVitalTooltip('ttfb'), ]} - onUnhover={() => setWebVitalTooltip(null)} + onBlur={() => setWebVitalTooltip(null)} /> diff --git a/packages/overlay/src/integrations/sentry/components/traces/TraceIcon.tsx b/packages/overlay/src/integrations/sentry/components/traces/TraceIcon.tsx index 212a3dcb..2d45170f 100644 --- a/packages/overlay/src/integrations/sentry/components/traces/TraceIcon.tsx +++ b/packages/overlay/src/integrations/sentry/components/traces/TraceIcon.tsx @@ -1,4 +1,4 @@ -import { Trace } from '../../types'; +import type { Trace } from '../../types'; import { sdkToPlatform } from '../../utils/sdkToPlatform'; import PlatformIcon from '../PlatformIcon'; diff --git a/packages/overlay/src/integrations/sentry/data/sentryEventsContext.tsx b/packages/overlay/src/integrations/sentry/data/sentryEventsContext.tsx index 5905b222..d307badc 100644 --- a/packages/overlay/src/integrations/sentry/data/sentryEventsContext.tsx +++ b/packages/overlay/src/integrations/sentry/data/sentryEventsContext.tsx @@ -19,10 +19,14 @@ export const SentryEventsContext = React.createContext }); function eventReducer(state: SentryEvent[], message: SetEventsAction): SentryEvent[] { - if (message.action === 'RESET' && Array.isArray(message.e)) { - return message.e; - } else if (message.action === 'APPEND' && !Array.isArray(message.e)) { - return [message.e, ...state]; + if (Array.isArray(message.e)) { + if (message.action === 'RESET') { + return message.e; + } + } else { + if (message.action === 'APPEND') { + return [message.e, ...state]; + } } return state; @@ -33,11 +37,13 @@ export const SentryEventsContextProvider: React.FC<{ }> = ({ children }) => { const [events, setEvents] = useReducer(eventReducer, sentryDataCache.getEvents()); - useEffect(() => { - return sentryDataCache.subscribe('event', (e: SentryEvent) => { - setEvents({ action: 'APPEND', e }); - }); - }, []); + useEffect( + () => + sentryDataCache.subscribe('event', (e: SentryEvent) => { + setEvents({ action: 'APPEND', e }); + }) as () => undefined, + [], + ); const contextValue: SentryEventsContextProps = { events, diff --git a/packages/overlay/src/lib/eventTarget.ts b/packages/overlay/src/lib/eventTarget.ts index ed7e2292..f74e6236 100644 --- a/packages/overlay/src/lib/eventTarget.ts +++ b/packages/overlay/src/lib/eventTarget.ts @@ -1,4 +1,4 @@ -import { WindowWithSpotlight } from '~/types'; +import type { WindowWithSpotlight } from '~/types'; const fallbackEventTarget = new EventTarget(); diff --git a/packages/overlay/src/lib/useKeyPress.ts b/packages/overlay/src/lib/useKeyPress.ts index 4f1b1810..39a4b19a 100644 --- a/packages/overlay/src/lib/useKeyPress.ts +++ b/packages/overlay/src/lib/useKeyPress.ts @@ -14,12 +14,9 @@ export default function useKeyPress(keys: string[], action: () => void, propagat } if ( - keys.every((key: string) => { - if (key in e) { - return e[key as keyof KeyboardEvent]; - } - return e.key.toLowerCase() === key.toLowerCase(); - }) + keys.every((key: string) => + key in e ? e[key as keyof KeyboardEvent] : e.key.toLowerCase() === key.toLowerCase(), + ) ) { action(); } @@ -27,8 +24,6 @@ export default function useKeyPress(keys: string[], action: () => void, propagat window.addEventListener('keyup', onKeyup); - return () => { - window.removeEventListener('keyup', onKeyup); - }; + return () => window.removeEventListener('keyup', onKeyup) as undefined; }, [keys, action, propagate]); } diff --git a/packages/overlay/src/sidecar.ts b/packages/overlay/src/sidecar.ts index 15c1242a..f8e1eb23 100644 --- a/packages/overlay/src/sidecar.ts +++ b/packages/overlay/src/sidecar.ts @@ -1,5 +1,5 @@ -import React from 'react'; -import { Integration, IntegrationData } from './integrations/integration'; +import type React from 'react'; +import type { Integration, IntegrationData } from './integrations/integration'; import { log } from './lib/logger'; export function connectToSidecar( @@ -56,9 +56,9 @@ export function connectToSidecar( return () => { log(`Removing ${contentTypeListeners.length} listeners`); - contentTypeListeners.forEach(typeAndListener => { + for (const typeAndListener of contentTypeListeners) { source.removeEventListener(typeAndListener[0], typeAndListener[1]); log('Removed listener for type', typeAndListener[0]); - }); + } }; } diff --git a/packages/overlay/src/ui/Chart/RingChart/RingChart.tsx b/packages/overlay/src/ui/Chart/RingChart/RingChart.tsx index 81599ece..87fe796b 100644 --- a/packages/overlay/src/ui/Chart/RingChart/RingChart.tsx +++ b/packages/overlay/src/ui/Chart/RingChart/RingChart.tsx @@ -13,7 +13,7 @@ export type RingChartProps = React.HTMLAttributes & { */ barWidth?: number; onHoverActions?: (() => void)[]; - onUnhover?: () => void; + onBlur?: () => void; /** * Endcaps on the progress bar */ @@ -42,7 +42,7 @@ function RingChart({ backgroundColors, progressEndcaps, onHoverActions, - onUnhover, + onBlur, ...p }: RingChartProps) { const foreignObjectSize = size / 2; @@ -74,7 +74,9 @@ function RingChart({ cx={cx} cy={cx} onMouseOver={() => onHoverActions?.[index]()} - onMouseLeave={() => onUnhover?.()} + onFocus={() => onHoverActions?.[index]()} + onMouseLeave={() => onBlur?.()} + onBlur={() => onBlur?.()} className={classNames(backgroundColors[index])} style={{ fill: 'none', @@ -82,7 +84,7 @@ function RingChart({ strokeDasharray: `${circumference} ${circumference}`, transform: `rotate(${rotate}deg)`, transformOrigin: '50% 50%', - transition: `stroke 300ms`, + transition: 'stroke 300ms', }} />, onHoverActions?.[index]()} - onMouseLeave={() => onUnhover?.()} + onFocus={() => onHoverActions?.[index]()} + onMouseLeave={() => onBlur?.()} + onBlur={() => onBlur?.()} className={classNames(segmentColors[index])} style={{ fill: 'none', @@ -101,7 +105,7 @@ function RingChart({ strokeDasharray: `${circumference} ${circumference}`, transform: `rotate(${rotate}deg)`, transformOrigin: '50% 50%', - transition: `stroke 300ms, stroke-dashoffset 300ms`, + transition: 'stroke 300ms, stroke-dashoffset 300ms', }} />, ]; @@ -112,7 +116,7 @@ function RingChart({ circumference, maxValues, onHoverActions, - onUnhover, + onBlur, progressEndcaps, radius, segmentColors, @@ -121,6 +125,7 @@ function RingChart({ return ( + Web Vitals Breakdown {rings}