Log rows performance: Render LogRowMenuCell on demand (#71354)

* Log row menu: refactor visibility

* LogRowMenuCell: display if mouse over or pinned

* LogRowMenuCell: use unique wrapper for all buttons

* Revert to using table row as position reference

* Log row message: update test

* Fix tests

* LogRow: handle mouse over behavior

public/app/features/explore/Logs/Logs.test.tsx

@@ -365,12 +365,12 @@ describe('Logs', () => {
       const row = screen.getAllByRole('row');
       await userEvent.hover(row[0]);
 
-      const linkButtons = row[1].querySelectorAll('button');
+      const linkButton = screen.getByLabelText('Copy shortlink');
-      await userEvent.click(linkButtons[2]);
+      await userEvent.click(linkButton);
 
       expect(reportInteraction).toHaveBeenCalledWith('grafana_explore_logs_permalink_clicked', {
         datasourceType: 'unknown',
-        logRowUid: '2',
+        logRowUid: '1',
         logRowLevel: 'debug',
       });
     });
@@ -382,11 +382,11 @@ describe('Logs', () => {
       const row = screen.getAllByRole('row');
       await userEvent.hover(row[0]);
 
-      const linkButtons = row[1].querySelectorAll('button');
+      const linkButton = screen.getByLabelText('Copy shortlink');
-      await userEvent.click(linkButtons[2]);
+      await userEvent.click(linkButton);
 
       expect(createAndCopyShortLink).toHaveBeenCalledWith(
-        'http://localhost:3000/explore?left=%7B%22datasource%22:%22%22,%22queries%22:%5B%7B%22refId%22:%22A%22,%22expr%22:%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22id%22%7D%7D%5D,%22range%22:%7B%22from%22:%222019-01-01T10:00:00.000Z%22,%22to%22:%222019-01-01T16:00:00.000Z%22%7D,%22panelsState%22:%7B%22logs%22:%7B%22id%22:%222%22%7D%7D%7D'
+        'http://localhost:3000/explore?left=%7B%22datasource%22:%22%22,%22queries%22:%5B%7B%22refId%22:%22A%22,%22expr%22:%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22id%22%7D%7D%5D,%22range%22:%7B%22from%22:%222019-01-01T10:00:00.000Z%22,%22to%22:%222019-01-01T16:00:00.000Z%22%7D,%22panelsState%22:%7B%22logs%22:%7B%22id%22:%221%22%7D%7D%7D'
       );
     });
   });

public/app/features/logs/components/LogRow.test.tsx

@@ -110,4 +110,14 @@ describe('LogRow', () => {
       expect(scrollIntoView).not.toHaveBeenCalled();
     });
   });
+
+  it('should render the menu cell on mouse over', async () => {
+    setup({ showContextToggle: jest.fn().mockReturnValue(true) });
+
+    expect(screen.queryByLabelText('Show context')).not.toBeInTheDocument();
+
+    await userEvent.hover(screen.getByText('test123'));
+
+    expect(screen.getByLabelText('Show context')).toBeInTheDocument();
+  });
 });

public/app/features/logs/components/LogRow.tsx

@@ -50,6 +50,7 @@ interface Props extends Themeable2 {
 interface State {
   highlightBackround: boolean;
   showDetails: boolean;
+  mouseIsOver: boolean;
 }
 
 /**
@@ -63,6 +64,7 @@ class UnThemedLogRow extends PureComponent<Props, State> {
   state: State = {
     highlightBackround: false,
     showDetails: false,
+    mouseIsOver: false,
   };
   logLineRef: React.RefObject<HTMLTableRowElement>;
 
@@ -108,12 +110,14 @@ class UnThemedLogRow extends PureComponent<Props, State> {
   }
 
   onMouseEnter = () => {
+    this.setState({ mouseIsOver: true });
     if (this.props.onLogRowHover) {
       this.props.onLogRowHover(this.props.row);
     }
   };
 
   onMouseLeave = () => {
+    this.setState({ mouseIsOver: false });
     if (this.props.onLogRowHover) {
       this.props.onLogRowHover(undefined);
     }
@@ -249,6 +253,7 @@ class UnThemedLogRow extends PureComponent<Props, State> {
               onPinLine={this.props.onPinLine}
               onUnpinLine={this.props.onUnpinLine}
               pinned={this.props.pinned}
+              mouseIsOver={this.state.mouseIsOver}
             />
           )}
         </tr>

public/app/features/logs/components/LogRowMenuCell.tsx

@@ -15,6 +15,7 @@ interface Props {
   onUnpinLine?: (row: LogRowModel) => void;
   pinned?: boolean;
   styles: LogRowStyles;
+  mouseIsOver: boolean;
 }
 
 export const LogRowMenuCell = React.memo(
@@ -28,6 +29,7 @@ export const LogRowMenuCell = React.memo(
     row,
     showContextToggle,
     styles,
+    mouseIsOver,
   }: Props) => {
     const shouldShowContextToggle = showContextToggle ? showContextToggle(row) : false;
     const onLogRowClick = useCallback((e: SyntheticEvent) => {
@@ -42,11 +44,10 @@ export const LogRowMenuCell = React.memo(
     );
     const getLogText = useCallback(() => logText, [logText]);
     return (
-      <>
-        {pinned && (
       // TODO: fix keyboard a11y
       // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions
-          <span className={`log-row-menu log-row-menu-visible ${styles.rowMenu}`} onClick={onLogRowClick}>
+      <span className={`log-row-menu ${styles.rowMenu}`} onClick={onLogRowClick}>
+        {pinned && !mouseIsOver && (
           <IconButton
             className={styles.unPinButton}
             size="md"
@@ -56,11 +57,9 @@ export const LogRowMenuCell = React.memo(
             tooltipPlacement="top"
             aria-label="Unpin line"
           />
-          </span>
         )}
-        {/* TODO: fix keyboard a11y */}
+        {mouseIsOver && (
-        {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
+          <>
-        <span className={`log-row-menu ${styles.rowMenu} ${styles.hidden}`} onClick={onLogRowClick}>
             {shouldShowContextToggle && (
               <IconButton
                 size="md"
@@ -113,8 +112,9 @@ export const LogRowMenuCell = React.memo(
                 onClick={() => onPermalinkClick(row)}
               />
             )}
-        </span>
           </>
+        )}
+      </span>
     );
   }
 );

public/app/features/logs/components/LogRowMessage.test.tsx

@@ -1,4 +1,4 @@
-import { render, screen } from '@testing-library/react';
+import { render, screen, fireEvent } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import React, { ComponentProps } from 'react';
 
@@ -18,6 +18,7 @@ const setup = (propOverrides?: Partial<ComponentProps<typeof LogRowMessage>>, ro
     prettifyLogMessage: false,
     app: CoreApp.Explore,
     styles,
+    mouseIsOver: true,
     ...(propOverrides || {}),
   };
 
@@ -41,8 +42,9 @@ describe('LogRowMessage', () => {
   });
 
   describe('with show context', () => {
-    it('should show context button', () => {
+    it('should show context button', async () => {
       setup({ showContextToggle: () => true });
+      await userEvent.hover(screen.getByText('test123'));
       expect(screen.queryByLabelText('Show context')).toBeInTheDocument();
     });
 
@@ -54,6 +56,7 @@ describe('LogRowMessage', () => {
     it('should call `onOpenContext` with row on click', async () => {
       const showContextToggle = jest.fn();
       const props = setup({ showContextToggle: () => true, onOpenContext: showContextToggle });
+      await userEvent.hover(screen.getByText('test123'));
       const button = screen.getByLabelText('Show context');
 
       await userEvent.click(button);
@@ -63,8 +66,9 @@ describe('LogRowMessage', () => {
   });
 
   describe('with permalinking', () => {
-    it('should show permalinking button when `onPermalinkClick` is defined', () => {
+    it('should show permalinking button when `onPermalinkClick` is defined', async () => {
       setup({ onPermalinkClick: jest.fn() });
+      await userEvent.hover(screen.getByText('test123'));
       expect(screen.queryByLabelText('Copy shortlink')).toBeInTheDocument();
     });
 
@@ -76,6 +80,7 @@ describe('LogRowMessage', () => {
     it('should call `onPermalinkClick` with row on click', async () => {
       const permalinkClick = jest.fn();
       const props = setup({ onPermalinkClick: permalinkClick });
+      await userEvent.hover(screen.getByText('test123'));
       const button = screen.getByLabelText('Copy shortlink');
 
       await userEvent.click(button);
@@ -86,8 +91,9 @@ describe('LogRowMessage', () => {
 
   describe('with pinning', () => {
     describe('for `onPinLine`', () => {
-      it('should show pinning button when `onPinLine` is defined', () => {
+      it('should show pinning button when `onPinLine` is defined', async () => {
         setup({ onPinLine: jest.fn() });
+        await userEvent.hover(screen.getByText('test123'));
         expect(screen.queryByLabelText('Pin line')).toBeInTheDocument();
       });
 
@@ -104,6 +110,7 @@ describe('LogRowMessage', () => {
       it('should call `onPinLine` on click', async () => {
         const onPinLine = jest.fn();
         setup({ onPinLine });
+        await userEvent.hover(screen.getByText('test123'));
         const button = screen.getByLabelText('Pin line');
 
         await userEvent.click(button);
@@ -118,10 +125,10 @@ describe('LogRowMessage', () => {
         expect(screen.queryByLabelText('Unpin line')).not.toBeInTheDocument();
       });
 
-      it('should show 2 pinning buttons when `onUnpinLine` and `pinned` is defined', () => {
+      it('should show 1 pinning buttons when `onUnpinLine` and `pinned` is defined', () => {
-        // we show 2 because we now have an "always visible" menu, and a "hover" menu
+        // we show 1 because we now have an "always visible" menu, the others are rendered on mouse over
         setup({ onUnpinLine: jest.fn(), pinned: true });
-        expect(screen.queryAllByLabelText('Unpin line').length).toBe(2);
+        expect(screen.queryAllByLabelText('Unpin line').length).toBe(1);
       });
 
       it('should not show pinning button when `onUnpinLine` is not defined', () => {
@@ -132,9 +139,10 @@ describe('LogRowMessage', () => {
       it('should call `onUnpinLine` on click', async () => {
         const onUnpinLine = jest.fn();
         setup({ onUnpinLine, pinned: true });
-        const button = screen.getAllByLabelText('Unpin line')[0];
+        const button = screen.getByLabelText('Unpin line');
 
-        await userEvent.click(button);
+        // There's an issue with userEvent and this button, so we use fireEvent instead
+        fireEvent.click(button);
 
         expect(onUnpinLine).toHaveBeenCalledTimes(1);
       });

public/app/features/logs/components/LogRowMessage.tsx

@@ -21,6 +21,7 @@ interface Props {
   onUnpinLine?: (row: LogRowModel) => void;
   pinned?: boolean;
   styles: LogRowStyles;
+  mouseIsOver: boolean;
 }
 
 interface LogMessageProps {
@@ -73,9 +74,11 @@ export const LogRowMessage = React.memo((props: Props) => {
     onUnpinLine,
     onPinLine,
     pinned,
+    mouseIsOver,
   } = props;
   const { hasAnsi, raw } = row;
   const restructuredEntry = useMemo(() => restructureLog(raw, prettifyLogMessage), [raw, prettifyLogMessage]);
+  const shouldShowMenu = useMemo(() => mouseIsOver || pinned, [mouseIsOver, pinned]);
   return (
     <>
       {
@@ -90,6 +93,7 @@ export const LogRowMessage = React.memo((props: Props) => {
         </div>
       </td>
       <td className={`log-row-menu-cell ${styles.logRowMenuCell}`}>
+        {shouldShowMenu && (
           <LogRowMenuCell
             logText={restructuredEntry}
             row={row}
@@ -100,7 +104,9 @@ export const LogRowMessage = React.memo((props: Props) => {
             onUnpinLine={onUnpinLine}
             pinned={pinned}
             styles={styles}
+            mouseIsOver={mouseIsOver}
           />
+        )}
       </td>
     </>
   );

public/app/features/logs/components/LogRowMessageDisplayedFields.tsx

@@ -1,5 +1,5 @@
 import { css } from '@emotion/css';
-import React from 'react';
+import React, { useCallback, useMemo, useState } from 'react';
 
 import { LogRowModel, Field, LinkModel, DataFrame } from '@grafana/data';
 
@@ -22,11 +22,14 @@ export interface Props {
 }
 
 export const LogRowMessageDisplayedFields = React.memo((props: Props) => {
-  const { row, detectedFields, getFieldLinks, wrapLogMessage, styles, ...rest } = props;
+  const [hover, setHover] = useState(false);
+  const { row, detectedFields, getFieldLinks, wrapLogMessage, styles, pinned, ...rest } = props;
   const fields = getAllFields(row, getFieldLinks);
   const wrapClassName = wrapLogMessage ? '' : displayedFieldsStyles.noWrap;
   // only single key/value rows are filterable, so we only need the first field key for filtering
-  const line = detectedFields
+  const line = useMemo(
+    () =>
+      detectedFields
         .map((parsedKey) => {
           const field = fields.find((field) => {
             const { keys } = field;
@@ -44,15 +47,27 @@ export const LogRowMessageDisplayedFields = React.memo((props: Props) => {
           return null;
         })
         .filter((s) => s !== null)
-    .join(' ');
+        .join(' '),
+    [detectedFields, fields, row.labels]
+  );
+
+  const showMenu = useCallback(() => {
+    setHover(true);
+  }, []);
+  const hideMenu = useCallback(() => {
+    setHover(false);
+  }, []);
+  const shouldShowMenu = useMemo(() => hover || pinned, [hover, pinned]);
 
   return (
     <>
-      <td className={styles.logsRowMessage}>
+      <td className={styles.logsRowMessage} onMouseEnter={showMenu} onMouseLeave={hideMenu}>
         <div className={wrapClassName}>{line}</div>
       </td>
-      <td className={`log-row-menu-cell ${styles.logRowMenuCell}`}>
+      <td className={`log-row-menu-cell ${styles.logRowMenuCell}`} onMouseEnter={showMenu} onMouseLeave={hideMenu}>
-        <LogRowMenuCell logText={line} row={row} styles={styles} {...rest} />
+        {shouldShowMenu && (
+          <LogRowMenuCell logText={line} row={row} styles={styles} pinned={pinned} {...rest} mouseIsOver={false} />
+        )}
       </td>
     </>
   );

public/app/features/logs/components/getLogRowStyles.ts

@@ -85,14 +85,9 @@ export const getLogRowStyles = memoizeOne((theme: GrafanaTheme2) => {
 
       &:hover {
         .log-row-menu {
-          visibility: visible;
           z-index: 1;
         }
 
-        .log-row-menu-visible {
-          visibility: hidden;
-        }
-
         background: ${hoverBgColor};
       }
 

public/app/features/logs/components/log-context/LogRowContextModal.test.tsx

@@ -1,4 +1,4 @@
-import { screen, waitFor } from '@testing-library/react';
+import { screen, waitFor, fireEvent } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import React from 'react';
 import { render } from 'test/redux-rtl';
@@ -475,8 +475,10 @@ describe('LogRowContextModal', () => {
       expect(rows).toHaveStyle('position: sticky');
     });
     const unpinButtons = screen.getAllByLabelText('Unpin line')[0];
-    await userEvent.click(unpinButtons);
+    fireEvent.click(unpinButtons);
+    await waitFor(() => {
       const rows = screen.getByTestId('entry-row');
       expect(rows).not.toHaveStyle('position: sticky');
     });
+  });
 });