Browse Source

fix(replays): Safe defaults and consistency when rendering tabbed areas in Replay Details (#46199)

We need to make sure that if the user types something weird into their
window.location, by default we render a real tab instead of `null`.

We can also use the enum more, which is probably good enough for the
FocusArea case, but making sure the default switch works too is nice and
consistent.

Follow up from https://github.com/getsentry/sentry/pull/46103
Ryan Albrecht 1 year ago
parent
commit
0c6ca62756

+ 14 - 0
static/app/components/replays/scrollableTabs.tsx

@@ -0,0 +1,14 @@
+import styled from '@emotion/styled';
+
+import NavTabs from 'sentry/components/navTabs';
+
+const ScrollableTabs = styled(NavTabs)`
+  display: flex;
+  flex-wrap: nowrap;
+  overflow-y: hidden;
+  overflow-x: auto;
+  white-space: nowrap;
+  margin-bottom: 0;
+`;
+
+export default ScrollableTabs;

+ 13 - 14
static/app/views/replays/detail/layout/focusArea.tsx

@@ -1,6 +1,6 @@
 import Placeholder from 'sentry/components/placeholder';
 import {useReplayContext} from 'sentry/components/replays/replayContext';
-import useActiveReplayTab from 'sentry/utils/replays/hooks/useActiveReplayTab';
+import useActiveReplayTab, {TabKey} from 'sentry/utils/replays/hooks/useActiveReplayTab';
 import useOrganization from 'sentry/utils/useOrganization';
 import Console from 'sentry/views/replays/detail/console';
 import DomMutations from 'sentry/views/replays/detail/domMutations';
@@ -18,26 +18,19 @@ function FocusArea({}: Props) {
   const organization = useOrganization();
 
   switch (getActiveTab()) {
-    case 'console':
-      return (
-        <Console
-          breadcrumbs={replay?.getConsoleCrumbs()}
-          startTimestampMs={replay?.getReplay()?.started_at?.getTime() || 0}
-        />
-      );
-    case 'network':
+    case TabKey.network:
       return (
         <NetworkList
           networkSpans={replay?.getNetworkSpans()}
           startTimestampMs={replay?.getReplay()?.started_at?.getTime() || 0}
         />
       );
-    case 'trace':
+    case TabKey.trace:
       if (!replay) {
         return <Placeholder height="150px" />;
       }
       return <Trace organization={organization} replayRecord={replay.getReplay()} />;
-    case 'issues':
+    case TabKey.issues:
       if (!replay) {
         return <Placeholder height="150px" />;
       }
@@ -47,14 +40,14 @@ function FocusArea({}: Props) {
           projectId={replay.getReplay()?.project_id}
         />
       );
-    case 'dom':
+    case TabKey.dom:
       return (
         <DomMutations
           replay={replay}
           startTimestampMs={replay?.getReplay()?.started_at?.getTime() || 0}
         />
       );
-    case 'memory':
+    case TabKey.memory:
       return (
         <MemoryChart
           currentTime={currentTime}
@@ -65,8 +58,14 @@ function FocusArea({}: Props) {
           startTimestampMs={replay?.getReplay()?.started_at?.getTime()}
         />
       );
+    case TabKey.console:
     default:
-      return null;
+      return (
+        <Console
+          breadcrumbs={replay?.getConsoleCrumbs()}
+          startTimestampMs={replay?.getReplay()?.started_at?.getTime() || 0}
+        />
+      );
   }
 }
 

+ 29 - 38
static/app/views/replays/detail/layout/focusTabs.tsx

@@ -1,8 +1,7 @@
-import {MouseEvent} from 'react';
-import styled from '@emotion/styled';
 import queryString from 'query-string';
 
-import NavTabs from 'sentry/components/navTabs';
+import ListLink from 'sentry/components/links/listLink';
+import ScrollableTabs from 'sentry/components/replays/scrollableTabs';
 import {t} from 'sentry/locale';
 import trackAdvancedAnalyticsEvent from 'sentry/utils/analytics/trackAdvancedAnalyticsEvent';
 import useActiveReplayTab, {TabKey} from 'sentry/utils/replays/hooks/useActiveReplayTab';
@@ -10,15 +9,17 @@ import {useLocation} from 'sentry/utils/useLocation';
 import useOrganization from 'sentry/utils/useOrganization';
 
 const ReplayTabs: Record<TabKey, string> = {
-  console: t('Console'),
-  network: t('Network'),
-  dom: t('DOM Events'),
-  issues: t('Issues'),
-  memory: t('Memory'),
-  trace: t('Trace'),
+  [TabKey.console]: t('Console'),
+  [TabKey.network]: t('Network'),
+  [TabKey.dom]: t('DOM Events'),
+  [TabKey.issues]: t('Issues'),
+  [TabKey.memory]: t('Memory'),
+  [TabKey.trace]: t('Trace'),
 };
 
-type Props = {className?: string};
+type Props = {
+  className?: string;
+};
 
 function FocusTabs({className}: Props) {
   const organization = useOrganization();
@@ -26,38 +27,28 @@ function FocusTabs({className}: Props) {
   const {getActiveTab, setActiveTab} = useActiveReplayTab();
   const activeTab = getActiveTab();
 
-  const createTabChangeHandler = (tab: string) => (e: MouseEvent) => {
-    e.preventDefault();
-    setActiveTab(tab);
-
-    trackAdvancedAnalyticsEvent('replay.details-tab-changed', {
-      tab,
-      organization,
-    });
-  };
-
   return (
-    <ScrollableNavTabs underlined className={className}>
+    <ScrollableTabs className={className} underlined>
       {Object.entries(ReplayTabs).map(([tab, label]) => (
-        <li key={tab} className={activeTab === tab ? 'active' : ''}>
-          <a
-            href={`${pathname}?${queryString.stringify({...query, t_main: tab})}`}
-            onClick={createTabChangeHandler(tab)}
-          >
-            <span>{label}</span>
-          </a>
-        </li>
+        <ListLink
+          key={tab}
+          isActive={() => tab === activeTab}
+          to={`${pathname}?${queryString.stringify({...query, t_main: tab})}`}
+          onClick={e => {
+            e.preventDefault();
+            setActiveTab(tab);
+
+            trackAdvancedAnalyticsEvent('replay.details-tab-changed', {
+              tab,
+              organization,
+            });
+          }}
+        >
+          {label}
+        </ListLink>
       ))}
-    </ScrollableNavTabs>
+    </ScrollableTabs>
   );
 }
 
-const ScrollableNavTabs = styled(NavTabs)`
-  display: flex;
-  flex-wrap: nowrap;
-  overflow-y: hidden;
-  overflow-x: auto;
-  white-space: nowrap;
-`;
-
 export default FocusTabs;

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

@@ -12,8 +12,8 @@ import FocusArea from 'sentry/views/replays/detail/layout/focusArea';
 import FocusTabs from 'sentry/views/replays/detail/layout/focusTabs';
 import MeasureSize from 'sentry/views/replays/detail/layout/measureSize';
 import SidebarArea from 'sentry/views/replays/detail/layout/sidebarArea';
+import SideTabs from 'sentry/views/replays/detail/layout/sideTabs';
 import SplitPanel from 'sentry/views/replays/detail/layout/splitPanel';
-import SideTabs from 'sentry/views/replays/detail/sideTabs';
 
 const MIN_VIDEO_WIDTH = 325;
 const MIN_CONTENT_WIDTH = 325;

+ 42 - 0
static/app/views/replays/detail/layout/sideTabs.tsx

@@ -0,0 +1,42 @@
+import queryString from 'query-string';
+
+import ListLink from 'sentry/components/links/listLink';
+import ScrollableTabs from 'sentry/components/replays/scrollableTabs';
+import {t} from 'sentry/locale';
+import {useLocation} from 'sentry/utils/useLocation';
+import useUrlParams from 'sentry/utils/useUrlParams';
+
+const TABS = {
+  crumbs: t('Breadcrumbs'),
+  tags: t('Tags'),
+};
+
+type Props = {
+  className?: string;
+};
+
+function SideTabs({className}: Props) {
+  const {pathname, query} = useLocation();
+  const {getParamValue, setParamValue} = useUrlParams('t_side', 'crumbs');
+  const activeTab = getParamValue();
+
+  return (
+    <ScrollableTabs className={className} underlined>
+      {Object.entries(TABS).map(([tab, label]) => (
+        <ListLink
+          key={tab}
+          isActive={() => tab === activeTab}
+          to={`${pathname}?${queryString.stringify({...query, t_side: tab})}`}
+          onClick={e => {
+            e.preventDefault();
+            setParamValue(tab);
+          }}
+        >
+          {label}
+        </ListLink>
+      ))}
+    </ScrollableTabs>
+  );
+}
+
+export default SideTabs;

+ 1 - 2
static/app/views/replays/detail/layout/sidebarArea.tsx

@@ -11,14 +11,13 @@ function SidebarArea() {
     case 'tags':
       return <TagPanel />;
     case 'crumbs':
+    default:
       return (
         <Breadcrumbs
           breadcrumbs={replay?.getRawCrumbs()}
           startTimestampMs={replay?.getReplay()?.started_at?.getTime() || 0}
         />
       );
-    default:
-      return null;
   }
 }
 

+ 0 - 42
static/app/views/replays/detail/sideTabs.tsx

@@ -1,42 +0,0 @@
-import NavTabs from 'sentry/components/navTabs';
-import {t} from 'sentry/locale';
-import trackAdvancedAnalyticsEvent from 'sentry/utils/analytics/trackAdvancedAnalyticsEvent';
-import useOrganization from 'sentry/utils/useOrganization';
-import useUrlParams from 'sentry/utils/useUrlParams';
-
-const TABS = {
-  crumbs: t('Breadcrumbs'),
-  tags: t('Tags'),
-};
-
-type Props = {
-  className?: string;
-};
-
-function SideTabs({className}: Props) {
-  const organization = useOrganization();
-  const {getParamValue, setParamValue} = useUrlParams('t_side', 'crumbs');
-  const active = getParamValue();
-
-  const createTabChangeHandler = (tab: string) => () => {
-    trackAdvancedAnalyticsEvent('replay.details-tab-changed', {
-      tab,
-      organization,
-    });
-    setParamValue(tab);
-  };
-
-  return (
-    <NavTabs underlined className={className}>
-      {Object.entries(TABS).map(([tab, label]) => {
-        return (
-          <li key={tab} className={active === tab ? 'active' : ''}>
-            <a onClick={createTabChangeHandler(tab)}>{label}</a>
-          </li>
-        );
-      })}
-    </NavTabs>
-  );
-}
-
-export default SideTabs;