Use local state for commit context menu state to reduce rerenders

Summary:
This is an alternate version of D116326998 that is a bit simpler / more targeted. This should give the same perf win while also simplifying the code a bit.

The underlying issue was that actioningCommit was global state that every commit subscribed to, so context menu triggered every commit to rerender. WIth giant dags this can be noticeable.

By making this state local to the component (useState instead of jotai atom), we only rerender the current atom.

Well, that would be true but we also need to update the dismissal path, which luckily we can do with useContextMenu.

Reviewed By: stbfb

Differential Revision: D116696808

fbshipit-source-id: d8a6a8cd39dba03c2f8df7b40af7e90bd7a9cb4f

Author: evangrayk

addons/isl/src/Commit.tsx

@@ -17,9 +17,9 @@ import {Icon} from 'isl-components/Icon';
 import {Subtle} from 'isl-components/Subtle';
 import {Tooltip} from 'isl-components/Tooltip';
 import {atom, useAtomValue, useSetAtom} from 'jotai';
-import React, {memo, useEffect} from 'react';
+import React, {memo, useState} from 'react';
 import {ComparisonType} from 'shared/Comparison';
-import {contextMenuState, useContextMenu} from 'shared/ContextMenu';
+import {useContextMenu} from 'shared/ContextMenu';
 import {MS_PER_DAY} from 'shared/constants';
 import {useAutofocusRef} from 'shared/hooks';
 import {notEmpty, nullthrows} from 'shared/utils';
@@ -83,7 +83,6 @@ import {RelativeDate, relativeDate} from './relativeDate';
 import {repoRelativeCwd, useIsIrrelevantToCwd} from './repositoryData';
 import {commitTreeWidth} from './responsive';
 import {
-  actioningCommit,
   selectedCommitInfos,
   selectedCommitInfosInDagOrder,
   selectedCommits,
@@ -182,14 +181,7 @@ export const Commit = memo(
     const {isSelected, onDoubleClickToShowDrawer} = useCommitCallbacks(commit);
     const actionsPrevented = previewPreventsActions(previewType);
 
-    const isActioning = useAtomValue(actioningCommit) === commit.hash;
-    const isContextMenuOpen = useAtomValue(contextMenuState) != null;
-
-    useEffect(() => {
-      if (!isContextMenuOpen && isActioning) {
-        writeAtom(actioningCommit, null);
-      }
-    }, [isContextMenuOpen, isActioning]);
+    const [isActioning, setIsActioning] = useState(false);
 
     const inConflicts = useAtomValue(inMergeConflicts);
 
@@ -591,22 +583,25 @@ export const Commit = memo(
       return items;
     };
 
-    const contextMenu = useContextMenu((): Array<ContextMenuItem> => {
-      return makeContextMenuOptions().map((item: ContextMenuItem & {loggingLabel?: string}) => {
-        if (item.type == null && notEmpty(item.loggingLabel)) {
-          return {
-            ...item,
-            onClick: () => {
-              tracker.track('CommitContextMenuItemClick', {
-                extras: {choice: item.loggingLabel},
-              });
-              item.onClick?.();
-            },
-          };
-        }
-        return item;
-      });
-    });
+    const contextMenu = useContextMenu(
+      (): Array<ContextMenuItem> => {
+        return makeContextMenuOptions().map((item: ContextMenuItem & {loggingLabel?: string}) => {
+          if (item.type == null && notEmpty(item.loggingLabel)) {
+            return {
+              ...item,
+              onClick: () => {
+                tracker.track('CommitContextMenuItemClick', {
+                  extras: {choice: item.loggingLabel},
+                });
+                item.onClick?.();
+              },
+            };
+          }
+          return item;
+        });
+      },
+      {onOpen: () => setIsActioning(true), onDismiss: () => setIsActioning(false)},
+    );
 
     const inlineCommitActions = [];
     const floatingCommitActions = [];
@@ -720,10 +715,7 @@ export const Commit = memo(
           (commit.successorInfo != null ? ' obsolete' : '') +
           (isIrrelevantToCwd ? ' irrelevant' : '')
         }
-        onContextMenu={e => {
-          writeAtom(actioningCommit, commit.hash);
-          contextMenu(e);
-        }}
+        onContextMenu={contextMenu}
         data-testid={`commit-${commit.hash}`}>
         <div
           className={'commit-rows' + (isActioning ? ' commit-row-actioning' : '')}

addons/isl/src/__tests__/CommitTreeList.test.tsx

@@ -91,6 +91,34 @@ describe('CommitTreeList', () => {
     });
   });
 
+  it('highlights only the commit with an open context menu', () => {
+    render(<App />);
+    act(() => {
+      closeCommitInfoSidebar();
+      simulateCommits({
+        value: [
+          COMMIT('a', 'Commit A', '1'),
+          COMMIT('b', 'Commit B', 'a', {isDot: true}),
+          COMMIT('1', 'some public base', '0', {phase: 'public'}),
+        ],
+      });
+    });
+
+    const commitA = screen.getByTestId('commit-a');
+    const commitB = screen.getByTestId('commit-b');
+
+    fireEvent.contextMenu(commitA);
+    expect(commitA.querySelector('.commit-rows')).toHaveClass('commit-row-actioning');
+    expect(commitB.querySelector('.commit-rows')).not.toHaveClass('commit-row-actioning');
+
+    fireEvent.contextMenu(commitB);
+    expect(commitA.querySelector('.commit-rows')).not.toHaveClass('commit-row-actioning');
+    expect(commitB.querySelector('.commit-rows')).toHaveClass('commit-row-actioning');
+
+    fireEvent.keyUp(window, {key: 'Escape'});
+    expect(commitB.querySelector('.commit-rows')).not.toHaveClass('commit-row-actioning');
+  });
+
   it('clears the top bar height when it unmounts', () => {
     const {unmount} = render(<App />);
     act(() => {

addons/isl/src/selection.ts

@@ -46,11 +46,6 @@ export const individualToggleKey: 'metaKey' | 'ctrlKey' = isMac ? 'metaKey' : 'c
  */
 export const selectedCommits = atom(new Set<Hash>());
 
-/**
- * Commit that is currently being actioned on (i.e., from the context menu).
- * This is a temporary visual state separate from selection.
- */
-export const actioningCommit = atom<Hash | null>(null);
 registerCleanup(
   selectedCommits,
   successionTracker.onSuccessions(successions => {

addons/shared/ContextMenu.tsx

@@ -28,6 +28,7 @@ import './ContextMenu.css';
  */
 export function useContextMenu<T>(
   creator: () => Array<ContextMenuItem>,
+  lifecycle?: {onOpen?: () => void; onDismiss?: () => void},
 ): React.MouseEventHandler<T> {
   const setState = useSetAtom(contextMenuState);
   return e => {
@@ -36,14 +37,25 @@ export function useContextMenu<T>(
     if (items.length === 0) {
       return;
     }
-    setState({x: e.clientX / zoom, y: e.clientY / zoom, items});
+    setState({
+      x: e.clientX / zoom,
+      y: e.clientY / zoom,
+      items,
+      onDismiss: lifecycle?.onDismiss,
+    });
+    lifecycle?.onOpen?.();
 
     e.preventDefault();
     e.stopPropagation();
   };
 }
 
-type ContextMenuData = {x: number; y: number; items: Array<ContextMenuItem>};
+type ContextMenuData = {
+  x: number;
+  y: number;
+  items: Array<ContextMenuItem>;
+  onDismiss?: () => void;
+};
 export type ContextMenuItem =
   | {
       type?: undefined;
@@ -58,7 +70,14 @@ export type ContextMenuItem =
     }
   | {type: 'divider'};
 
-export const contextMenuState = atom<null | ContextMenuData>(null);
+const contextMenuData = atom<null | ContextMenuData>(null);
+export const contextMenuState = atom(
+  get => get(contextMenuData),
+  (get, set, value: ContextMenuData | null) => {
+    get(contextMenuData)?.onDismiss?.();
+    set(contextMenuData, value);
+  },
+);
 
 /**
  * Compute the absolute placement for the context menu overlay from the click

addons/shared/__tests__/ContextMenu.test.tsx

@@ -14,12 +14,17 @@ import {computeContextMenuStyle, ContextMenus, useContextMenu} from '../ContextM
 
 const onClick1 = jest.fn();
 const onClick2 = jest.fn();
+const onOpen = jest.fn();
+const onDismiss = jest.fn();
 
 function TestComponent() {
-  const menu = useContextMenu(() => [
-    {label: 'Context item 1', onClick: onClick1},
-    {label: 'Context item 2', onClick: onClick2},
-  ]);
+  const menu = useContextMenu(
+    () => [
+      {label: 'Context item 1', onClick: onClick1},
+      {label: 'Context item 2', onClick: onClick2},
+    ],
+    {onOpen, onDismiss},
+  );
   return (
     <div data-testid="test-component" onContextMenu={menu}>
       Hello click me
@@ -43,6 +48,11 @@ function rightClick(el: HTMLElement) {
 }
 
 describe('Context Menu', () => {
+  beforeEach(() => {
+    onOpen.mockClear();
+    onDismiss.mockClear();
+  });
+
   it('shows context menu items on right click', () => {
     render(<TestApp />);
 
@@ -53,6 +63,8 @@ describe('Context Menu', () => {
 
     expect(screen.getByText('Context item 1')).toBeInTheDocument();
     expect(screen.getByText('Context item 2')).toBeInTheDocument();
+    expect(onOpen).toHaveBeenCalledTimes(1);
+    expect(onDismiss).not.toHaveBeenCalled();
   });
 
   it('runs callbacks on clicking an item', () => {
@@ -68,6 +80,7 @@ describe('Context Menu', () => {
     });
     expect(onClick1).toHaveBeenCalled();
     expect(onClick2).not.toHaveBeenCalled();
+    expect(onDismiss).toHaveBeenCalledTimes(1);
   });
 
   it('dismisses on escape key', () => {
@@ -84,6 +97,7 @@ describe('Context Menu', () => {
 
     expect(screen.queryByText('Context item 1')).not.toBeInTheDocument();
     expect(screen.queryByText('Context item 2')).not.toBeInTheDocument();
+    expect(onDismiss).toHaveBeenCalledTimes(1);
   });
 
   it('dismisses on click outside', () => {
@@ -100,6 +114,7 @@ describe('Context Menu', () => {
 
     expect(screen.queryByText('Context item 1')).not.toBeInTheDocument();
     expect(screen.queryByText('Context item 2')).not.toBeInTheDocument();
+    expect(onDismiss).toHaveBeenCalledTimes(1);
   });
 });