From 424e336ae166b9427617b816be923f7144b39c77 Mon Sep 17 00:00:00 2001 From: Leon Sorokin Date: Wed, 20 Aug 2025 16:12:32 -0500 Subject: [PATCH] TableNG: Memoize some components to improve rendering performance (#109763) * try React.memo * Table: Rework the renderers file and some types * more test cleanups * remove for gauge for now * memoize all of the renderers * also memoize the Tooltip component * memoize the TableCellActions * memo useStyles2 * update useStyles2 usage in TableNG * remove console.log * add a test for the AutoCell fallback rendering case for testField * style nit - update variable name to be more idiomatic --------- Co-authored-by: Paul Marbach --- .../Table/TableNG/Cells/renderers.test.tsx | 80 ++++--- .../Table/TableNG/Cells/renderers.tsx | 162 ++++++------- .../src/components/Table/TableNG/TableNG.tsx | 41 ++-- .../TableNG/components/TableCellActions.tsx | 13 +- .../TableNG/components/TableCellTooltip.tsx | 226 +++++++++--------- .../src/components/Table/TableNG/styles.ts | 5 +- .../src/components/Table/TableNG/types.ts | 4 +- 7 files changed, 265 insertions(+), 266 deletions(-) diff --git a/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.test.tsx b/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.test.tsx index 3c4e5421c4c..48b444839a6 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.test.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.test.tsx @@ -73,46 +73,46 @@ describe('TableNG Cells renderers', () => { } // Helper function to render a cell and get the test ID - const renderCell = (field: Field, cellOptions: TableCellOptions) => - render( - getCellRenderer( - field, - cellOptions - )({ - field, - value: 'test-value', - rowIdx: 0, - frame: createDataFrame({ fields: [field] }), - height: 100, - width: 100, - theme: createTheme(), - cellOptions, - cellInspect: false, - showFilters: false, - getActions: jest.fn(() => [ + const renderCell = (field: Field, cellOptions: TableCellOptions) => { + // eslint-disable-next-line testing-library/render-result-naming-convention + const CellComponent = getCellRenderer(field, cellOptions); + return render( + [ { title: 'Action', onClick: jest.fn(() => {}), confirmation: jest.fn(), style: {} }, - ]), - }) + ])} + /> ); + }; // Performance test helper const benchmarkCellPerformance = (field: Field, cellOptions: TableCellOptions, iterations = 100) => { // eslint-disable-next-line testing-library/render-result-naming-convention - const r = getCellRenderer(field, cellOptions); + const CellComponent = getCellRenderer(field, cellOptions); return measurePerformance(() => { render( - r({ - field, - value: 'test-value', - rowIdx: 0, - frame: createDataFrame({ fields: [field] }), - height: 100, - width: 100, - theme: createTheme(), - cellOptions, - cellInspect: false, - showFilters: false, - }) + ); }, iterations); }; @@ -289,6 +289,22 @@ describe('TableNG Cells renderers', () => { expect(container).toBeInTheDocument(); expect(container.childNodes).toHaveLength(1); }); + + it('should use AutoCell when attempting to render a field with an unsupported type', () => { + // confirm that a real pill cell has spans. + const stringField = createField(FieldType.string, ['42']); + const { container: stringFieldContainer } = renderCell(stringField, { type: TableCellDisplayMode.Pill }); + expect(stringFieldContainer).toBeInTheDocument(); + expect(stringFieldContainer.childNodes).toHaveLength(1); + expect(stringFieldContainer.querySelector('span')).toBeInTheDocument(); + + // confirm that number pill cell doesn't actually render a pill cell. + const numberField = createField(FieldType.number, [42]); + const { container: numberFieldContainer } = renderCell(numberField, { type: TableCellDisplayMode.Pill }); + expect(numberFieldContainer).toBeInTheDocument(); + expect(numberFieldContainer.childNodes).toHaveLength(1); + expect(numberFieldContainer.querySelector('span')).toBeNull(); + }); }); describe.skip('performance benchmarks', () => { diff --git a/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.tsx b/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.tsx index c0e2801d7f7..8006c2043cf 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/Cells/renderers.tsx @@ -1,9 +1,10 @@ import { clsx } from 'clsx'; +import { memo, MemoExoticComponent } from 'react'; import { Field, FieldType, GrafanaTheme2, isDataFrame, isTimeSeriesFrame } from '@grafana/data'; import { TableCellDisplayMode, TableCellOptions, TableCustomCellOptions } from '../../types'; -import { TableCellRenderer, TableCellStyleOptions, TableCellStyles } from '../types'; +import { TableCellRenderer, TableCellRendererProps, TableCellStyleOptions, TableCellStyles } from '../types'; import { getCellOptions } from '../utils'; import { ActionsCell, getStyles as getActionsCellStyles } from './ActionsCell'; @@ -16,49 +17,9 @@ import { MarkdownCell, getStyles as getMarkdownCellStyles } from './MarkdownCell import { PillCell, getStyles as getPillStyles } from './PillCell'; import { SparklineCell, getStyles as getSparklineCellStyles } from './SparklineCell'; -const GAUGE_RENDERER: TableCellRenderer = (props) => ( - -); - -const AUTO_RENDERER: TableCellRenderer = (props) => ( +const AutoCellRenderer = memo((props: TableCellRendererProps) => ( -); - -const SPARKLINE_RENDERER: TableCellRenderer = (props) => ( - -); - -const GEO_RENDERER: TableCellRenderer = (props) => ; - -const IMAGE_RENDERER: TableCellRenderer = (props) => ( - -); - -const DATA_LINKS_RENDERER: TableCellRenderer = (props) => ; - -const ACTIONS_RENDERER: TableCellRenderer = ({ field, rowIdx, getActions = () => [] }) => ( - -); - -const MARKDOWN_RENDERER: TableCellRenderer = (props) => ( - -); - -const PILL_RENDERER: TableCellRenderer = (props) => ; +)); function isCustomCellOptions(options: TableCellOptions): options is TableCustomCellOptions { return options.type === TableCellDisplayMode.Custom; @@ -71,73 +32,101 @@ function mixinAutoCellStyles(fn: TableCellStyles): TableCellStyles { }; } -const CUSTOM_RENDERER: TableCellRenderer = (props) => { - if (!isCustomCellOptions(props.cellOptions) || !props.cellOptions.cellComponent) { - return null; // nonsensical case, but better to typeguard it than throw. - } - const CustomCellComponent = props.cellOptions.cellComponent; - return ; -}; +interface CellRegistryEntry { + renderer: MemoExoticComponent; + getStyles?: TableCellStyles; + testField?: (field: Field) => boolean; +} -const CELL_RENDERERS: Record = { - [TableCellDisplayMode.Actions]: { - renderer: ACTIONS_RENDERER, - getStyles: getActionsCellStyles, - }, +const CELL_REGISTRY: Record = { [TableCellDisplayMode.Auto]: { - renderer: AUTO_RENDERER, + renderer: AutoCellRenderer, getStyles: getAutoCellStyles, }, [TableCellDisplayMode.ColorBackground]: { - renderer: AUTO_RENDERER, + renderer: AutoCellRenderer, getStyles: getAutoCellStyles, }, [TableCellDisplayMode.ColorText]: { - renderer: AUTO_RENDERER, + renderer: AutoCellRenderer, getStyles: getAutoCellStyles, }, - [TableCellDisplayMode.Custom]: { - renderer: CUSTOM_RENDERER, + [TableCellDisplayMode.JSONView]: { + renderer: AutoCellRenderer, + getStyles: mixinAutoCellStyles(getJsonCellStyles), + }, + [TableCellDisplayMode.Actions]: { + renderer: memo((props: TableCellRendererProps) => ( + [])} /> + )), + getStyles: getActionsCellStyles, }, [TableCellDisplayMode.DataLinks]: { - renderer: DATA_LINKS_RENDERER, + renderer: memo((props: TableCellRendererProps) => ), getStyles: getDataLinksStyles, }, [TableCellDisplayMode.Gauge]: { - renderer: GAUGE_RENDERER, + renderer: memo((props: TableCellRendererProps) => ( + + )), + }, + [TableCellDisplayMode.Sparkline]: { + renderer: memo((props: TableCellRendererProps) => ( + + )), + getStyles: getSparklineCellStyles, }, [TableCellDisplayMode.Geo]: { - renderer: GEO_RENDERER, + renderer: memo((props: TableCellRendererProps) => ), getStyles: getGeoCellStyles, }, [TableCellDisplayMode.Image]: { - renderer: IMAGE_RENDERER, + renderer: memo((props: TableCellRendererProps) => ( + + )), getStyles: getImageStyles, }, - [TableCellDisplayMode.JSONView]: { - renderer: AUTO_RENDERER, - getStyles: mixinAutoCellStyles(getJsonCellStyles), - }, [TableCellDisplayMode.Pill]: { - renderer: PILL_RENDERER, + renderer: memo((props: TableCellRendererProps) => ( + + )), getStyles: getPillStyles, - }, - [TableCellDisplayMode.Sparkline]: { - renderer: SPARKLINE_RENDERER, - getStyles: getSparklineCellStyles, + testField: (field: Field) => field.type === FieldType.string, }, [TableCellDisplayMode.Markdown]: { - renderer: MARKDOWN_RENDERER, + renderer: memo((props: TableCellRendererProps) => ( + + )), getStyles: getMarkdownCellStyles, + testField: (field: Field) => field.type === FieldType.string, + }, + [TableCellDisplayMode.Custom]: { + renderer: memo((props: TableCellRendererProps) => { + if (!isCustomCellOptions(props.cellOptions) || !props.cellOptions.cellComponent) { + return null; // nonsensical case, but better to typeguard it than throw. + } + const CustomCellComponent = props.cellOptions.cellComponent; + return ( + + ); + }), }, }; -// TODO: come up with a more elegant way to handle this. -const STRING_ONLY_RENDERERS = new Set([ - TableCellDisplayMode.Markdown, - TableCellDisplayMode.Pill, -]); - /** @internal */ export function getCellRenderer( field: Field, @@ -145,15 +134,16 @@ export function getCellRenderer( ): TableCellRenderer { const cellType = cellOptions?.type ?? TableCellDisplayMode.Auto; if (cellType === TableCellDisplayMode.Auto) { - return CELL_RENDERERS[getAutoRendererDisplayMode(field)].renderer; + return CELL_REGISTRY[getAutoRendererDisplayMode(field)].renderer; } - if (STRING_ONLY_RENDERERS.has(cellType) && field.type !== FieldType.string) { - return AUTO_RENDERER; + // if the field fails the test for a specific renderer, fallback to Auto + if (CELL_REGISTRY[cellType]?.testField && CELL_REGISTRY[cellType].testField(field) !== true) { + return AutoCellRenderer; } // cautious fallback to Auto renderer in case some garbage cell type has been provided. - return CELL_RENDERERS[cellType]?.renderer ?? AUTO_RENDERER; + return CELL_REGISTRY[cellType]?.renderer ?? AutoCellRenderer; } /** @internal */ @@ -166,7 +156,7 @@ export function getCellSpecificStyles( if (cellType === TableCellDisplayMode.Auto) { return getAutoRendererStyles(theme, options, field); } - return CELL_RENDERERS[cellType]?.getStyles?.(theme, options); + return CELL_REGISTRY[cellType]?.getStyles?.(theme, options); } /** @internal */ @@ -177,7 +167,7 @@ export function getAutoRendererStyles( ): string | undefined { const impliedDisplayMode = getAutoRendererDisplayMode(field); if (impliedDisplayMode !== TableCellDisplayMode.Auto) { - return CELL_RENDERERS[impliedDisplayMode]?.getStyles?.(theme, options); + return CELL_REGISTRY[impliedDisplayMode]?.getStyles?.(theme, options); } return getAutoCellStyles(theme, options); } diff --git a/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx b/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx index c461dd23a33..caea5e1fcad 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx @@ -111,10 +111,7 @@ export function TableNG(props: TableNGProps) { } = props; const theme = useTheme2(); - const styles = useStyles2(getGridStyles, { - enablePagination, - transparent, - }); + const styles = useStyles2(getGridStyles, enablePagination, transparent); const panelContext = usePanelContext(); const getCellActions = useCallback( @@ -328,7 +325,7 @@ export function TableNG(props: TableNGProps) { const footerStyles = getFooterStyles(justifyContent); const displayName = getDisplayName(field); const headerCellClass = getHeaderCellStyles(theme, justifyContent); - const renderFieldCell = getCellRenderer(field, cellOptions); + const CellType = getCellRenderer(field, cellOptions); const cellInspect = isCellInspectEnabled(field); const showFilters = Boolean(field.config.filterable && onCellFilterAdded != null); @@ -409,21 +406,21 @@ export function TableNG(props: TableNGProps) { return ( <> - {renderFieldCell({ - cellOptions, - frame, - field, - height, - rowIdx, - theme, - value, - width, - timeRange, - cellInspect, - showFilters, - getActions: getCellActions, - disableSanitizeHtml, - })} + {showActions && ( ): JSX.Element => { // cached so we don't care about multiple calls. - const height = rowHeightFn(props.row); + const tooltipHeight = rowHeightFn(props.row); let tooltipStyle: CSSProperties | undefined; if (tooltipCanBeColorized) { const tooltipDisplayValue = tooltipField.display!(props.row[tooltipDisplayName]); // this is yet another call to field.display() for the tooltip field @@ -504,7 +501,7 @@ export function TableNG(props: TableNGProps) { } return ( - + {renderBasicCellContent(props)} ); diff --git a/packages/grafana-ui/src/components/Table/TableNG/components/TableCellActions.tsx b/packages/grafana-ui/src/components/Table/TableNG/components/TableCellActions.tsx index cdbcde443c1..e85facade5a 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/components/TableCellActions.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/components/TableCellActions.tsx @@ -1,5 +1,6 @@ import WKT from 'ol/format/WKT'; import Geometry from 'ol/geom/Geometry'; +import { memo } from 'react'; import { FieldType } from '@grafana/data'; import { t } from '@grafana/i18n'; @@ -9,8 +10,8 @@ import { TableCellInspectorMode } from '../../TableCellInspector'; import { TableCellDisplayMode } from '../../types'; import { FILTER_FOR_OPERATOR, FILTER_OUT_OPERATOR, TableCellActionsProps } from '../types'; -export function TableCellActions(props: TableCellActionsProps) { - const { +export const TableCellActions = memo( + ({ field, value, cellOptions, @@ -20,9 +21,7 @@ export function TableCellActions(props: TableCellActionsProps) { className, cellInspect, showFilters, - } = props; - - return ( + }: TableCellActionsProps) => ( // stopping propagation to prevent clicks within the actions menu from triggering the cell click events // for things like the data links tooltip. // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions @@ -80,5 +79,5 @@ export function TableCellActions(props: TableCellActionsProps) { )} - ); -} + ) +); diff --git a/packages/grafana-ui/src/components/Table/TableNG/components/TableCellTooltip.tsx b/packages/grafana-ui/src/components/Table/TableNG/components/TableCellTooltip.tsx index a81f083a2f1..ce707ea43e3 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/components/TableCellTooltip.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/components/TableCellTooltip.tsx @@ -1,4 +1,4 @@ -import { CSSProperties, ReactElement, useMemo, useState, useRef, useEffect, RefObject } from 'react'; +import { CSSProperties, ReactElement, useMemo, useState, useRef, useEffect, memo, RefObject } from 'react'; import { DataGridHandle } from 'react-data-grid'; import { ActionModel, DataFrame, Field, GrafanaTheme2 } from '@grafana/data'; @@ -10,7 +10,7 @@ import { TableCellOptions } from '../../types'; import { getTooltipStyles } from '../styles'; import { TableCellRenderer, TableCellRendererProps } from '../types'; -export interface Props { +export interface TableCellTooltipProps { cellOptions: TableCellOptions; children: ReactElement; classes: ReturnType; @@ -30,130 +30,130 @@ export interface Props { width?: number; } -export function TableCellTooltip({ - cellOptions, - children, - classes, - className, - data, - disableSanitizeHtml, - field, - getActions, - gridRef, - height, - placement, - renderer, - rowIdx, - style, - theme, - tooltipField, - width = 300, -}: Props) { - const rawValue = field.values[rowIdx]; - const tooltipCaretRef = useRef(null); +export const TableCellTooltip = memo( + ({ + cellOptions, + children, + classes, + className, + data, + disableSanitizeHtml, + field, + getActions, + gridRef, + height, + placement, + renderer: CellRenderer, + rowIdx, + style, + theme, + tooltipField, + width = 300, + }: TableCellTooltipProps) => { + const rawValue = field.values[rowIdx]; + const tooltipCaretRef = useRef(null); - const [hovered, setHovered] = useState(false); - const [pinned, setPinned] = useState(false); + const [hovered, setHovered] = useState(false); + const [pinned, setPinned] = useState(false); - const show = hovered || pinned; - const dynamicHeight = tooltipField.config.custom?.cellOptions?.dynamicHeight; + const show = hovered || pinned; + const dynamicHeight = tooltipField.config.custom?.cellOptions?.dynamicHeight; - useEffect(() => { - if (pinned) { - const gridRoot = gridRef.current?.element; + useEffect(() => { + if (pinned) { + const gridRoot = gridRef.current?.element; - const windowListener = (ev: Event) => { - if (ev.target === tooltipCaretRef.current) { - return; - } + const windowListener = (ev: Event) => { + if (ev.target === tooltipCaretRef.current) { + return; + } - setPinned(false); - window.removeEventListener('click', windowListener); - }; + setPinned(false); + window.removeEventListener('click', windowListener); + }; - window.addEventListener('click', windowListener); + window.addEventListener('click', windowListener); - // right now, we kill the pinned tooltip on any form of scrolling to avoid awkward rendering - // where the tooltip bumps up against the edge of the scrollable container. we could try to - // kill the tooltip when it hits these boundaries rather than when scrolling starts. - const scrollListener = () => { - setPinned(false); - }; - gridRoot?.addEventListener('scroll', scrollListener, { once: true }); + // right now, we kill the pinned tooltip on any form of scrolling to avoid awkward rendering + // where the tooltip bumps up against the edge of the scrollable container. we could try to + // kill the tooltip when it hits these boundaries rather than when scrolling starts. + const scrollListener = () => { + setPinned(false); + }; + gridRoot?.addEventListener('scroll', scrollListener, { once: true }); - return () => { - window.removeEventListener('click', windowListener); - gridRoot?.removeEventListener('scroll', scrollListener); - }; + return () => { + window.removeEventListener('click', windowListener); + gridRoot?.removeEventListener('scroll', scrollListener); + }; + } + + return; + }, [pinned, gridRef]); + + const rendererProps = useMemo( + () => + ({ + cellInspect: false, + cellOptions, + disableSanitizeHtml, + field, + frame: data, + getActions, + height, + rowIdx, + showFilters: false, + theme, + value: rawValue, + width, + }) satisfies TableCellRendererProps, + [cellOptions, data, disableSanitizeHtml, field, getActions, height, rawValue, rowIdx, theme, width] + ); + + const cellElement = tooltipCaretRef.current?.closest('.rdg-cell'); + + if (rawValue === null || rawValue === undefined) { + return children; } - return; - }, [pinned, gridRef]); + // TODO: perist the hover if you mouse out of the trigger and into the popover + const onMouseLeave = () => setHovered(false); + const onMouseEnter = () => setHovered(true); - const rendererProps = useMemo( - () => - ({ - cellInspect: false, - cellOptions, - disableSanitizeHtml, - field, - frame: data, - getActions, - height, - rowIdx, - showFilters: false, - theme, - value: rawValue, - width, - }) satisfies TableCellRendererProps, - [cellOptions, data, disableSanitizeHtml, field, getActions, height, rawValue, rowIdx, theme, width] - ); + return ( + <> + {cellElement && ( + } + show={show} + placement={placement} + wrapperClassName={classes.tooltipWrapper} + className={className} + style={{ ...style, minWidth: width, ...(!dynamicHeight && { height }) }} + referenceElement={cellElement} + onMouseLeave={onMouseLeave} + onMouseEnter={onMouseEnter} + onClick={(ev) => ev.stopPropagation()} // prevent click from bubbling to the global click listener for un-pinning + data-testid={selectors.components.Panels.Visualization.TableNG.Tooltip.Wrapper} + /> + )} - const cellElement = tooltipCaretRef.current?.closest('.rdg-cell'); - - if (rawValue === null || rawValue === undefined) { - return children; - } - - const body = <>{renderer(rendererProps)}; - - // TODO: perist the hover if you mouse out of the trigger and into the popover - const onMouseLeave = () => setHovered(false); - const onMouseEnter = () => setHovered(true); - - return ( - <> - {cellElement && ( - setPinned((prev) => !prev)} onMouseLeave={onMouseLeave} onMouseEnter={onMouseEnter} - onClick={(ev) => ev.stopPropagation()} // prevent click from bubbling to the global click listener for un-pinning - data-testid={selectors.components.Panels.Visualization.TableNG.Tooltip.Wrapper} + onBlur={onMouseLeave} + onFocus={onMouseEnter} /> - )} - {/* TODO: figure out an accessible way to trigger the tooltip. */} - {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} -
setPinned((prev) => !prev)} - onMouseLeave={onMouseLeave} - onMouseEnter={onMouseEnter} - onBlur={onMouseLeave} - onFocus={onMouseEnter} - /> - - {children} - - ); -} + {children} + + ); + } +); diff --git a/packages/grafana-ui/src/components/Table/TableNG/styles.ts b/packages/grafana-ui/src/components/Table/TableNG/styles.ts index ca837f66dbc..b21bb702041 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/styles.ts +++ b/packages/grafana-ui/src/components/Table/TableNG/styles.ts @@ -7,10 +7,7 @@ import { COLUMN, TABLE } from './constants'; import { TableCellStyles } from './types'; import { getJustifyContent, TextAlign } from './utils'; -export const getGridStyles = ( - theme: GrafanaTheme2, - { enablePagination, transparent }: { enablePagination?: boolean; transparent?: boolean } -) => { +export const getGridStyles = (theme: GrafanaTheme2, enablePagination?: boolean, transparent?: boolean) => { const bgColor = transparent ? theme.colors.background.canvas : theme.colors.background.primary; // this needs to be pre-calc'd since the theme colors have alpha and the border color becomes // unpredictable for background color cells diff --git a/packages/grafana-ui/src/components/Table/TableNG/types.ts b/packages/grafana-ui/src/components/Table/TableNG/types.ts index 3434b24dcf0..0f1883456a7 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/types.ts +++ b/packages/grafana-ui/src/components/Table/TableNG/types.ts @@ -1,4 +1,4 @@ -import { ReactNode, SyntheticEvent } from 'react'; +import { FC, SyntheticEvent } from 'react'; import { Column } from 'react-data-grid'; import { @@ -150,7 +150,7 @@ export interface BaseTableProps { /* ---------------------------- Table cell props ---------------------------- */ export interface TableNGProps extends BaseTableProps {} -export type TableCellRenderer = (props: TableCellRendererProps) => ReactNode; +export type TableCellRenderer = FC; export interface TableCellRendererProps { rowIdx: number;