Skip to content

fix(tablev2): keep RenderRow stable #1120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JeanMarcMilletScality wants to merge 2 commits into
base: development/1.0
Choose a base branch
from
Draft

fix(tablev2): keep RenderRow stable #1120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fb49f0
fix(tablev2): keep RenderRow stable so rows aren't remounted on re-re…
JeanMarcMilletScality May 29, 2026
f463fc4
docs(tablev2): document memoizing columns to avoid cell remounts
JeanMarcMilletScality Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 109 additions & 72 deletions src/lib/components/tablev2/MultiSelectableContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useEffect, useState, memo, CSSProperties } from 'react';
import { useEffect, useState, memo, useMemo, useRef } from 'react';
import { Row } from 'react-table';
import { areEqual } from 'react-window';
import { areEqual, ListChildComponentProps } from 'react-window';
import { useTableContext } from './Tablev2.component';
import {
HeadRow,
Expand All @@ -14,7 +14,7 @@ import {
TableLocalType,
TableVariantType,
} from './TableUtils';
import { RenderRowType, TableRows, useTableScrollbar } from './TableCommon';
import { TableRows, useTableScrollbar } from './TableCommon';
import useSyncedScroll from './useSyncedScroll';
import { Box } from '../box/Box';
import { Loader } from '../loader/Loader.component';
Expand Down Expand Up @@ -124,79 +124,116 @@ export const MultiSelectableContent = <

const { headerRef } = useSyncedScroll<DATA_ROW>();

const RenderRow = memo(({ index, style }: RenderRowType) => {
const row = rows[index];
prepareRow(row);
/**
* These values change identity on (almost) every render. We read them through refs so the row
* renderer below can keep a stable identity (see RenderRow).
*/
const prepareRowRef = useRef(prepareRow);
prepareRowRef.current = prepareRow;
const selectedRowIdsRef = useRef(selectedRowIds);
selectedRowIdsRef.current = selectedRowIds;
const onSingleRowSelectedRef = useRef(onSingleRowSelected);
onSingleRowSelectedRef.current = onSingleRowSelected;
const toggleAllRowsSelectedRef = useRef(toggleAllRowsSelected);
toggleAllRowsSelectedRef.current = toggleAllRowsSelected;
const handleMultipleSelectedRowsRef = useRef(handleMultipleSelectedRows);
handleMultipleSelectedRowsRef.current = handleMultipleSelectedRows;

const rowProps = {
...row.getRowProps({
/**
* Note:We need to pass the style property to the row component.
* Otherwise when we scroll down, the next rows are flashing
* because they are re-rendered in loop.
*/
style: { ...style },
}),
onClick: onSingleRowSelected
? () => {
onSingleRowSelected(row);
toggleAllRowsSelected(false);
setActiveRowId(row.id);
}
: () => handleMultipleSelectedRows(selectedRowIds, rows, row, index),
};
/**
* RenderRow MUST keep a stable identity across re-renders. It used to be redefined inline on
* every render, so react-window saw a new component type each time and remounted (not just
* re-rendered) every row — and therefore every cell — whenever the table re-rendered for any
* reason. That made async cell content reload and flash. We now read the row from react-window's
* `data` (itemData) prop and the volatile callbacks/state from refs, so the component is only
* recreated when something that affects the rendered output (activeRowId / separationLineVariant)
* actually changes. Checkbox selection still updates because react-table rebuilds `rows` (and
* therefore `data`) when `selectedRowIds` changes.
*/
const RenderRow = useMemo(
() =>
memo(({ index, style, data }: ListChildComponentProps<Row<DATA_ROW>[]>) => {
const rows = data;
const row = data[index];
prepareRowRef.current(row);
const onSingleRowSelected = onSingleRowSelectedRef.current;
Copy link
Copy Markdown
Contributor

@claude claude Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSingleRowSelected is captured from the ref at render time into a local variable, then used in the click handler ternary on line 169. The other refs (handleMultipleSelectedRowsRef, toggleAllRowsSelectedRef, selectedRowIdsRef) are correctly read from .current at click time — but this one isn't, so if the prop changes after the row renders, the click handler uses a stale reference and may pick the wrong branch (single-select vs multi-select).

Read the ref inside the click handler instead:

Suggested change
const onSingleRowSelected = onSingleRowSelectedRef.current;
const rowProps = {
...row.getRowProps({
/**
* Note:We need to pass the style property to the row component.
* Otherwise when we scroll down, the next rows are flashing
* because they are re-rendered in loop.
*/
style: { ...style },
}),
onClick: () => {
const onSingleRowSelected = onSingleRowSelectedRef.current;
if (onSingleRowSelected) {
onSingleRowSelected(row);
toggleAllRowsSelectedRef.current(false);
setActiveRowId(row.id);
} else {
handleMultipleSelectedRowsRef.current(
selectedRowIdsRef.current,
rows,
row,
index,
);
}
},

The same change applies to the checkbox onClick on the selection column (line 207) — read onSingleRowSelectedRef.current there too.


return (
<TableRowMultiSelectable
{...rowProps}
isSelected={row.isSelected || activeRowId === row.id}
separationLineVariant={separationLineVariant}
className="tr"
>
{row.cells.map((cell) => {
const cellProps = cell.getCellProps({
style: {
...cell.column.cellStyle,
// Vertically center the text in cells.
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
},
role: 'gridcell',
});
const rowProps = {
...row.getRowProps({
/**
* Note:We need to pass the style property to the row component.
* Otherwise when we scroll down, the next rows are flashing
* because they are re-rendered in loop.
*/
style: { ...style },
}),
onClick: onSingleRowSelected
? () => {
onSingleRowSelected(row);
toggleAllRowsSelectedRef.current(false);
setActiveRowId(row.id);
}
: () =>
handleMultipleSelectedRowsRef.current(
selectedRowIdsRef.current,
rows,
row,
index,
),
};

if (cell.column.id === 'selection') {
return (
<div
{...cellProps}
onClick={
onSingleRowSelected
? (event) => {
event.stopPropagation();
handleMultipleSelectedRows(
selectedRowIds,
rows,
row,
index,
);
}
: undefined
}
>
{cell.render('Cell')}
</div>
);
}
return (
<TableRowMultiSelectable
{...rowProps}
isSelected={row.isSelected || activeRowId === row.id}
separationLineVariant={separationLineVariant}
className="tr"
>
{row.cells.map((cell) => {
const cellProps = cell.getCellProps({
style: {
...cell.column.cellStyle,
// Vertically center the text in cells.
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
},
role: 'gridcell',
});

if (cell.column.id === 'selection') {
return (
<div
{...cellProps}
onClick={
onSingleRowSelected
? (event) => {
event.stopPropagation();
handleMultipleSelectedRowsRef.current(
selectedRowIdsRef.current,
rows,
row,
index,
);
}
: undefined
}
>
{cell.render('Cell')}
</div>
);
}

return (
<div {...cellProps} className="td">
{cell.render('Cell')}
</div>
);
})}
</TableRowMultiSelectable>
);
}, areEqual);
return (
<div {...cellProps} className="td">
{cell.render('Cell')}
</div>
);
})}
</TableRowMultiSelectable>
);
}, areEqual),
[activeRowId, separationLineVariant],
Copy link
Copy Markdown
Contributor

@claude claude Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activeRowId in the useMemo deps means every row click (via onSingleRowSelected) creates a new component type, causing react-window to unmount and remount all visible rows — the same flashing problem this PR fixes for other re-render triggers. The same applies to selectedId in SingleSelectableContent.

To keep the component stable across selection changes, move activeRowId into a ref (like the other volatile values) and pass it through itemData so areEqual can trigger re-renders without changing the component identity:

const activeRowIdRef = useRef<string | null>(null);
activeRowIdRef.current = activeRowId;

// in useMemo deps: remove activeRowId, keep only [separationLineVariant]
// in the row component: read activeRowIdRef.current instead of activeRowId
// pass activeRowId through itemData so areEqual detects the change:
//   itemData={useMemo(() => ({ rows, activeRowId }), [rows, activeRowId])}

This changes the itemData shape from Row[] to { rows, activeRowId }, which touches VirtualizedRows and TableRows types — a bigger refactor, so it may belong in a follow-up.

);

return (
<>
Expand Down
153 changes: 88 additions & 65 deletions src/lib/components/tablev2/SingleSelectableContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { memo, useEffect, useRef } from 'react';
import { areEqual, FixedSizeList } from 'react-window';
import { memo, useEffect, useMemo, useRef } from 'react';
import { areEqual, FixedSizeList, ListChildComponentProps } from 'react-window';
import { Row } from 'react-table';
import { useTableContext } from './Tablev2.component';
import {
Expand All @@ -14,7 +14,7 @@ import {
TableLocalType,
TableVariantType,
} from './TableUtils';
import { RenderRowType, TableRows, useTableScrollbar } from './TableCommon';
import { TableRows, useTableScrollbar } from './TableCommon';
import useSyncedScroll from './useSyncedScroll';
import { Loader } from '../loader/Loader.component';
import { Box } from '../box/Box';
Expand Down Expand Up @@ -77,69 +77,92 @@ export function SingleSelectableContent<
return () => clearTimeout(timer);
}, [autoScrollToSelected, selectedId, rows]);

const RenderRow = memo(({ index, style }: RenderRowType) => {
const row = rows[index];
prepareRow(row);
let rowProps = row.getRowProps({
/**
* Note: We need to pass the style property to the row component.
* Otherwise when we scroll down, the next rows are flashing
* because they are re-rendered in loop.
*/
style: { ...style },
});

rowProps = {
...rowProps,
...{
onClick: () => {
if (onRowSelected) return onRowSelected(row);
},
tabIndex: onRowSelected ? 0 : undefined,
onKeyDown: (event) => {
if (
onRowSelected &&
(event.key === ' ' ||
event.key === 'Enter' ||
event.key === 'Spacebar')
) {
event.preventDefault();
onRowSelected(row);
}
},
},
};

return (
<TableRow
{...rowProps}
isSelected={selectedId === row.id}
aria-selected={selectedId === row.id ? 'true' : 'false'}
separationLineVariant={separationLineVariant}
selectedId={selectedId}
className="tr"
>
{row.cells.map((cell) => {
let cellProps = cell.getCellProps({
style: {
...cell.column.cellStyle,
// Vertically center the text in cells.
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
/**
* `prepareRow` and `onRowSelected` change identity on every render. We read them through refs
* so the row renderer below can keep a stable identity (see RenderRow).
*/
const prepareRowRef = useRef(prepareRow);
prepareRowRef.current = prepareRow;
const onRowSelectedRef = useRef(onRowSelected);
onRowSelectedRef.current = onRowSelected;

/**
* RenderRow MUST keep a stable identity across re-renders. It used to be redefined inline on
* every render, so react-window saw a new component type each time and remounted (not just
* re-rendered) every row — and therefore every cell — whenever the table re-rendered for any
* reason. That made async cell content reload and flash. We now read the row from react-window's
* `data` (itemData) prop and the volatile callbacks from refs, so the component only needs to be
* recreated when something that affects the rendered output (selectedId / separationLineVariant)
* actually changes.
*/
const RenderRow = useMemo(
() =>
memo(({ index, style, data }: ListChildComponentProps<Row<DATA_ROW>[]>) => {
const row = data[index];
prepareRowRef.current(row);
const onRowSelected = onRowSelectedRef.current;
Copy link
Copy Markdown
Contributor

@claude claude Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as in MultiSelectableContent — onRowSelected is captured from the ref at render time and used in the click/keyDown handlers. Read it from the ref at call time instead:

Suggested change
const onRowSelected = onRowSelectedRef.current;
const rowProps = {
...rowProps,
...{
onClick: () => {
const onRowSelected = onRowSelectedRef.current;
if (onRowSelected) return onRowSelected(row);
},
tabIndex: onRowSelectedRef.current ? 0 : undefined,
onKeyDown: (event) => {
const onRowSelected = onRowSelectedRef.current;

let rowProps = row.getRowProps({
/**
* Note: We need to pass the style property to the row component.
* Otherwise when we scroll down, the next rows are flashing
* because they are re-rendered in loop.
*/
style: { ...style },
});

rowProps = {
...rowProps,
...{
onClick: () => {
if (onRowSelected) return onRowSelected(row);
},
role: 'gridcell',
});

return (
<div {...cellProps} className="td">
{cell.render('Cell')}
</div>
);
})}
</TableRow>
);
}, areEqual);
tabIndex: onRowSelected ? 0 : undefined,
onKeyDown: (event) => {
if (
onRowSelected &&
(event.key === ' ' ||
event.key === 'Enter' ||
event.key === 'Spacebar')
) {
event.preventDefault();
onRowSelected(row);
}
},
},
};

return (
<TableRow
{...rowProps}
isSelected={selectedId === row.id}
aria-selected={selectedId === row.id ? 'true' : 'false'}
separationLineVariant={separationLineVariant}
selectedId={selectedId}
className="tr"
>
{row.cells.map((cell) => {
let cellProps = cell.getCellProps({
style: {
...cell.column.cellStyle,
// Vertically center the text in cells.
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
},
role: 'gridcell',
});

return (
<div {...cellProps} className="td">
{cell.render('Cell')}
</div>
);
})}
</TableRow>
);
}, areEqual),
[selectedId, separationLineVariant],
);

const { hasScrollbar, scrollBarWidth, handleScrollbarWidth } =
useTableScrollbar();
Expand Down
4 changes: 1 addition & 3 deletions src/lib/components/tablev2/TableCommon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,7 @@ type TableRowsProps<
locale?: TableLocalType;
children?: (children: JSX.Element) => JSX.Element;
customItemKey?: (index: number, data: DATA_ROW) => string;
RenderRow: React.MemoExoticComponent<
({ index, style }: RenderRowType) => JSX.Element
>;
RenderRow: ComponentType<ListChildComponentProps<Row<DATA_ROW>[]>>;
Copy link
Copy Markdown
Contributor

@claude claude Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RenderRowType (line 120) and its CSSProperties import (line 18) are now dead code — no remaining importers after this change. Clean them up.

listRef?: Ref<FixedSizeList<Row<DATA_ROW>[]>>;
};
export function TableRows<
Expand Down
Loading
Loading