fix(tablev2): keep RenderRow stable #1120
Conversation
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const rows = data; | ||
| const row = data[index]; | ||
| prepareRowRef.current(row); | ||
| const onSingleRowSelected = onSingleRowSelectedRef.current; |
There was a problem hiding this comment.
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:
| 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.
| memo(({ index, style, data }: ListChildComponentProps<Row<DATA_ROW>[]>) => { | ||
| const row = data[index]; | ||
| prepareRowRef.current(row); | ||
| const onRowSelected = onRowSelectedRef.current; |
There was a problem hiding this comment.
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:
| 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; |
| </TableRowMultiSelectable> | ||
| ); | ||
| }, areEqual), | ||
| [activeRowId, separationLineVariant], |
There was a problem hiding this comment.
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.
| svg { | ||
| width: 1em; | ||
| height: 1em; | ||
| fill: #a14fbf; |
There was a problem hiding this comment.
Hardcoded hex color bypasses the theme system. This won't adapt to dark/light mode or custom themes. Use a theme token instead:
| fill: #a14fbf; | |
| fill: ${({ theme }) => theme.infoPrimary}; |
JeanMarcMilletScality
force-pushed
the
improvement/fix-tablev2-row-remount-on-rerender
branch
from
09fa42f to
8010104
Compare
| RenderRow: React.MemoExoticComponent< | ||
| ({ index, style }: RenderRowType) => JSX.Element | ||
| >; | ||
| RenderRow: ComponentType<ListChildComponentProps<Row<DATA_ROW>[]>>; |
There was a problem hiding this comment.
RenderRowType (line 120) and its CSSProperties import (line 18) are now dead code — no remaining importers after this change. Clean them up.
JeanMarcMilletScality
force-pushed
the
improvement/fix-tablev2-row-remount-on-rerender
branch
from
caf9b49 to
8010104
Compare
…nder RenderRow was redefined inline on every render, so react-window saw a new component type and remounted every row (and cell) whenever the table re-rendered, making async cell content reload and flash. Read the row from react-window's itemData and volatile callbacks from refs so RenderRow keeps a stable identity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JeanMarcMilletScality
force-pushed
the
improvement/fix-tablev2-row-remount-on-rerender
branch
from
8010104 to
f463fc4
Compare
…time Addresses review feedback on PR #1120. `onSingleRowSelected` (Multi) and `onRowSelected` (Single) were snapshotted from their refs at render time and closed over by the click/keydown handlers. Since RenderRow only re-renders when `data` identity changes, a callback whose identity changed without a `rows` change would leave the row with a stale closure until its next re-render — while the other volatile values (handleMultipleSelectedRows, toggleAllRowsSelected, selectedRowIds) were already read from refs at call time. Read these callbacks from `.current` inside the handlers so they're always current, consistent with the other refs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
Please check the status of the associated issue None. Goodbye jeanmarcmilletscality. |
3 participants
Problem
TableV2renders its rows virtualized viareact-window.RenderRowwas defined inline inside the component body, so it got a new function identity on every render.react-windowcaches rows by component type, so a fresh identity made it unmount and remount every visible row — and therefore every cell — on any table re-render, regardless of the cause (a parent state change, a context update, etc.), not just when the data changed.Remounting (as opposed to re-rendering) is destructive: cell subtrees lose their internal state and re-run their mount effects. Cells whose content is loaded asynchronously on mount, or that animate/resolve on mount, re-trigger all of that work — producing visible flicker and redundant refetches every time the table re-rendered for any reason.
What this PR does
RenderRow. Wrap it inuseMemoso its identity only changes when something that affects the rendered row actually changes (activeRowId/selectedId,separationLineVariant). The row is read fromreact-window'sitemData(data) prop instead of closing overrows, and the volatile values (prepareRow, the selection callbacks,selectedRowIds) are read through refs. Unrelated re-renders now re-render rows (cheap, state-preserving) instead of remounting them.onSingleRowSelected/onRowSelectedfrom their refs at event time rather than from a render-time snapshot, removing a stale-closure edge case (the other volatile callbacks were already read this way).columns/datacontract. Added guidance onTableProps.columns: callers must memoize their columns and keep eachCellrenderer stable across renders.react-tablerenders cells as<column.Cell />, so an unstableCellidentity is itself a new component type and remounts the cell subtree — independently of this fix.What's still missing (follow-up)
RenderRow'suseMemodeps include the selection state (activeRowId/selectedId). So while unrelated re-renders no longer remount rows, changing the selection still recreatesRenderRowand remounts all rows — async/animated cells still flicker on row click/selection. This is intentional here: the selection is read by value so the row highlight stays correct.Eliminating it requires keeping
RenderRowfully stable and instead passing the selection through a memoizeditemDataobject (e.g.{ rows, selectedId }), letting a customareEqualre-render (not remount) only the affected rows. That ripples intoTableCommon/VirtualizedRows, theitemKey/customItemKeyselectors, and the single-select highlight path, so it is deferred to a separate PR to keep this one focused and low-risk.Verification
npm run build-storybook(exit 0),tsc --noEmitclean, ESLint clean, full Jest suite passing (446/446).Split out of the original combined branch — icon fallback rendering is in #1122, the Storybook guideline import fix was #1121 (merged).
🤖 Generated with Claude Code