Skip to content

[BUGFIX] stabilize VirtualizedTable component references to prevent remounts - #358

Open
adrianSepiol wants to merge 2 commits into
perses:mainfrom
adrianSepiol:fix/virtualized-table-stable-components
Open

adrianSepiol wants to merge 2 commits into
perses:mainfrom
adrianSepiol:fix/virtualized-table-stable-components

Conversation

@adrianSepiol

Copy link
Copy Markdown
Contributor

Description

Fixes: perses/perses#4571

The fix moves VirtuosoTableComponents out of the component entirely as a module-level constant, giving it a permanently stable reference. Per-render values (width, density, keyboard handlers, row event handlers) are threaded through via Virtuoso's context prop and a tableContext memo instead of closing over them.

Additionally, useVirtualizedTableKeyboardNav now wraps onActiveCellChange in useCallback so the returned handler references are stable when its inputs haven't changed, which was the other source of memo churn.

This fix also enables stateful components to be used inside the table (e.g. a filter dropdown that tracks its own open state) — previously, remounting on every render would silently reset any local state those components held.

Screenshots

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
    following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
  • All commits have DCO signoffs.

UI Changes

  • Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
  • Code follows the UI guidelines.
  • E2E tests are stable and unlikely to be flaky.
    See e2e docs for more details. Common issues include:
    • Is the data inconsistent? You need to mock API requests.
    • Does the time change? You need to use consistent time values or mock time utilities.
    • Does it have loading states? You need to wait for loading to complete.

@adrianSepiol
adrianSepiol requested a review from a team as a code owner October 9, 2026 08:22
@adrianSepiol
adrianSepiol force-pushed the fix/virtualized-table-stable-components branch from e95b89a to a2cc56a Compare October 9, 2026 08:26
@ibakshay
ibakshay requested a balanced review from Copilot October 9, 2026 14:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The remount regression needs focused automated coverage to prevent recurrence.

1 open finding
What changed in this PR

Stabilizes VirtualizedTable internals to prevent child remounts and state loss.

Changes:

  • Moves Virtuoso components to module scope and supplies dynamic values through context.
  • Stabilizes keyboard-navigation callbacks with useCallback.
  • Uses row data directly through Virtuoso’s data prop.
File Description
components/​src/​Table/​VirtualizedTable.tsx Stabilizes Virtuoso component identities and passes render context.
components/​src/​Table/​hooks/​useVirtualizedTableKeyboardNav.tsx Memoizes the active-cell callback.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +216 to +218
data={rows}
components={VirtuosoTableComponents as TableComponents<Row<TableData>, TableContext<TableData>>}
context={tableContext}
…emounts

Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
@adrianSepiol
adrianSepiol force-pushed the fix/virtualized-table-stable-components branch from a2cc56a to 07c4473 Compare October 12, 2026 08:04
// Defined outside VirtualizedTable so the object reference is stable and
// TableVirtuoso never remounts its internal components due to a changed prop.
// Props that vary per-render are passed via the `context` prop instead.
const VirtuosoTableComponents: TableComponents<Row<unknown>, TableContext<unknown>> = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

context is "leaking" on TableHead, TableFoot and TableBody components, because it's passed by default (where on others, it's separated). So the scroller div, thead, tbody and tfoot (when paginated) now render context="[object Object]"

@Gladorme Gladorme Oct 12, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Once it's fixed, this PR is a nice improvement for the rendering :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch :) I've pushed fix

Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VirtuosoTableComponents remounts on every render due to unstable useMemo dependencies

3 participants