Profiler bugfix for filtering out all commits after selecting a fiber

facebook · Aug 18, 2019 · 4697f5b · 4697f5b
1 parent 95ca079
commit 4697f5b
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 26 deletions.
diff --git a/src/devtools/views/Profiler/Profiler.js b/src/devtools/views/Profiler/Profiler.js
@@ -28,6 +28,7 @@ function Profiler(_: {||}) {
     didRecordCommits,
     isProcessingData,
     isProfiling,
+    selectedCommitIndex,
     selectedFiberID,
     selectedTabID,
     selectTab,
@@ -67,10 +68,18 @@ function Profiler(_: {||}) {
         break;
       case 'flame-chart':
       case 'ranked-chart':
-        if (selectedFiberID !== null) {
-          sidebar = <SidebarSelectedFiberInfo />;
-        } else {
-          sidebar = <SidebarCommitInfo />;
+        // TRICKY
+        // Handle edge case where no commit is selected because of a min-duration filter update.
+        // In that case, the selected commit index would be null.
+        // We could still show a sidebar for the previously selected fiber,
+        // but it would be an odd user experience.
+        // TODO (ProfilerContext) This check should not be necessary.
+        if (selectedCommitIndex !== null) {
+          if (selectedFiberID !== null) {
+            sidebar = <SidebarSelectedFiberInfo />;
+          } else {
+            sidebar = <SidebarCommitInfo />;
+          }
         }
         break;
       default:

diff --git a/src/devtools/views/Profiler/ProfilerContext.js b/src/devtools/views/Profiler/ProfilerContext.js
@@ -127,28 +127,32 @@ function ProfilerContextController({ children }: Props) {
   const [rootID, setRootID] = useState<number | null>(null);
 
   if (prevProfilingData !== profilingData) {
-    setPrevProfilingData(profilingData);
-
-    const dataForRoots =
-      profilingData !== null ? profilingData.dataForRoots : null;
-    if (dataForRoots != null) {
-      const firstRootID = dataForRoots.keys().next().value || null;
-
-      if (rootID === null || !dataForRoots.has(rootID)) {
-        let selectedElementRootID = null;
-        if (selectedElementID !== null) {
-          selectedElementRootID = store.getRootIDForElement(selectedElementID);
-        }
-        if (
-          selectedElementRootID !== null &&
-          dataForRoots.has(selectedElementRootID)
-        ) {
-          setRootID(selectedElementRootID);
-        } else {
-          setRootID(firstRootID);
+    batchedUpdates(() => {
+      setPrevProfilingData(profilingData);
+
+      const dataForRoots =
+        profilingData !== null ? profilingData.dataForRoots : null;
+      if (dataForRoots != null) {
+        const firstRootID = dataForRoots.keys().next().value || null;
+
+        if (rootID === null || !dataForRoots.has(rootID)) {
+          let selectedElementRootID = null;
+          if (selectedElementID !== null) {
+            selectedElementRootID = store.getRootIDForElement(
+              selectedElementID
+            );
+          }
+          if (
+            selectedElementRootID !== null &&
+            dataForRoots.has(selectedElementRootID)
+          ) {
+            setRootID(selectedElementRootID);
+          } else {
+            setRootID(firstRootID);
+          }
         }
       }
-    }
+    });
   }
 
   const startProfiling = useCallback(

diff --git a/src/devtools/views/Profiler/SidebarSelectedFiberInfo.js b/src/devtools/views/Profiler/SidebarSelectedFiberInfo.js
@@ -89,7 +89,7 @@ export default function SidebarSelectedFiberInfo(_: Props) {
 }
 
 type WhatChangedProps = {|
-  commitIndex: number,
+  commitIndex: number | null,
   fiberID: number,
   profilerStore: ProfilerStore,
   rootID: number,
@@ -101,6 +101,14 @@ function WhatChanged({
   profilerStore,
   rootID,
 }: WhatChangedProps) {
+  // TRICKY
+  // Handle edge case where no commit is selected because of a min-duration filter update.
+  // If the commit index is null, suspending for data below would throw an error.
+  // TODO (ProfilerContext) This check should not be necessary.
+  if (commitIndex === null) {
+    return null;
+  }
+
   const { changeDescriptions } = profilerStore.getCommitData(
     ((rootID: any): number),
     commitIndex

diff --git a/src/devtools/views/Profiler/SnapshotSelector.js b/src/devtools/views/Profiler/SnapshotSelector.js
@@ -59,7 +59,7 @@ export default function SnapshotSelector(_: Props) {
     return null;
   }, [filteredCommitIndices, selectedCommitIndex]);
 
-  // TODO (profiling) This should be managed by the context controller (reducer).
+  // TODO (ProfilerContext) This should be managed by the context controller (reducer).
   // It doesn't currently know about the filtered commits though (since it doesn't suspend).
   // Maybe this component should pass filteredCommitIndices up?
   if (selectedFilteredCommitIndex === null) {