Skip to content

Commit ea8e493

Browse files
serpentbladeclaude
serpentblade
and
claude
committed
fix(53-05): re-measure recycled rows on every window commit (CR-01)
Root cause: virtual-core@3.17.1's Virtualizer.measureElement(node) is the SOLE caller of observer.observe() (dist/esm/index.js:794-817). DataTable's windowing engine called it once at mount (remeasureWindow in $onMount) for the FIRST window only, so rows that recycle into view on scroll got new DOM nodes that were never observed — they kept the 40px estimateRowHeight seed forever and getTotalSize() never converged to the real measured total, defeating variable-height windowing (req-2). Fix (entirely inside the $props.virtual guard; non-virtual r-else path byte-identical): - onChange now schedules a per-commit remeasureWindow() sweep, deduped via a remeasurePending flag (at most one rAF per frame) and deferred one frame so the recycled <tr> nodes exist before measuring. - remeasureWindow() bails while virtualizer.scrollState is non-null (a programmatic scrollToIndex in flight) so measuring can't nudge the offset and starve the D-12 scroll-then-focus target — the regression that initially broke the Solid off-window focus path. Manual-scroll recycling has scrollState===null, so it measures normally. - WR-01: windowedRows() filters out rows whose vi.index outruns $data.rows (avoids wr.row.id deref on a transient shrink). - WR-05: dropped the unreachable `ver < 0` sentinel guards (windowVer only increments); the bare windowVer read still performs the subscription. - IN-01/IN-04: corrected the false "virtual-core auto-registers rows" comment and added the subscribe-first back-reference to padBottom(). Verification gate (closes the gap that let CR-01 pass): strengthened data-table-virtual.spec.ts with a CR-01 case that drives the var-height fixture top→bottom so every row recycles, then asserts getTotalSize converges above the estimate. FAILS pre-fix (~20.3k, all 6) / PASSES post-fix (~26.3k, all 6). data-table-virtual 48/48 + data-table-grid 18/18 green ×6 in the pinned Linux Playwright container; whole-repo build 135/135; dist-parity 875/875 zero drift. Deferred: WR-02/03/04 (reactive virtual/estimate prop tracking, empty-dataset teardown sweep, aria-rowcount coupling) + IN-02/03. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013wvxhsB5hcDKD8UadLLSyw
1 parent e46e600 commit ea8e493

8 files changed

Lines changed: 563 additions & 89 deletions

File tree

‎packages/ui/data-table/packages/angular/src/DataTable.ts‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -840,6 +840,7 @@ export class DataTable {
840840
virtualizer: any = null;
841841
virtualizerCleanup: any = null;
842842
gridScrollEl: any = null;
843+
remeasurePending = false;
843844
GRID_PAGE_STEP = 10;
844845
gridRoot: any = null;
845846
programmatic = 0;
@@ -1073,8 +1074,29 @@ export class DataTable {
10731074
getItemKey: this.virtualItemKey,
10741075
onChange: () => {
10751076
this.windowVer.set(this.windowVer() + 1);
1077+
// CR-01: re-observe the freshly-committed window so RECYCLED rows get measured.
1078+
// virtual-core only observe()s a node you explicitly hand to measureElement (it does
1079+
// NOT auto-discover rendered rows — measureElement is the SOLE caller of
1080+
// observer.observe, virtual-core@3.17.1 dist/esm/index.js:794-817). Rows that recycle
1081+
// into view on scroll are brand-new DOM nodes; without re-sweeping they keep the
1082+
// estimateRowHeight seed forever and the spacer math drifts (req-2). Deferred one frame
1083+
// so the new <tr> set is in the DOM before we measure. Safe from an infinite
1084+
// measure→onChange→measure loop: measureElement is idempotent on an already-observed
1085+
// node (the `prevNode !== node` guard), and resizeItem only re-fires onChange when the
1086+
// measured height actually DIFFERS from the cached one (delta !== 0) — an unchanged
1087+
// re-measure is a no-op.
1088+
this.scheduleRemeasure();
10761089
}
10771090
});
1091+
scheduleRemeasure = () => {
1092+
if (this.remeasurePending) return;
1093+
this.remeasurePending = true;
1094+
const run = () => {
1095+
this.remeasurePending = false;
1096+
this.remeasureWindow();
1097+
};
1098+
if (typeof requestAnimationFrame === 'function') requestAnimationFrame(run);else if (typeof queueMicrotask !== 'undefined') queueMicrotask(run);else setTimeout(run, 0);
1099+
};
10781100
windowedRows = () => {
10791101
const __rows = this.rows();
10801102
// SUBSCRIBE FIRST (fine-grained targets): touch the reactive windowVer at the TOP — BEFORE any
@@ -1086,7 +1108,7 @@ export class DataTable {
10861108
// blank forever (the Solid/Svelte fine-grained bug). Coarse targets re-render wholesale so the
10871109
// placement is a no-op for them. The post-construction windowVer bump in $onMount fires the
10881110
// first re-run that picks up the now-non-null virtualizer.
1089-
const ver = this.windowVer();
1111+
void this.windowVer();
10901112
if (!this.virtualizer) {
10911113
// Virtual OFF → full set (the r-else table never calls this, but keep it total). Virtual ON
10921114
// but the virtualizer is not yet constructed (pre-$onMount first paint) → render NOTHING so
@@ -1101,25 +1123,30 @@ export class DataTable {
11011123
}
11021124
return [];
11031125
}
1104-
if (ver < 0) return [];
11051126
const items = this.virtualizer.getVirtualItems();
11061127
const rowList = __rows || [];
1128+
// WR-01: drop any virtual item whose index outruns the current full-model rows (a brief
1129+
// shrink window where the virtualizer count is stale relative to $data.rows on the async
1130+
// onChange→windowVer path). The template keys on wr.row.id, so a row:undefined entry would
1131+
// throw "Cannot read properties of undefined"; filter it here so the template never sees it.
11071132
return items.map((vi: any) => ({
11081133
vi,
11091134
row: rowList[vi.index]
1110-
}));
1135+
})).filter((wr: any) => wr.row);
11111136
};
11121137
padTop = () => {
1113-
// SUBSCRIBE FIRST (the windowedRows() discipline): read windowVer at the TOP so the spacer-<td>
1138+
// SUBSCRIBE FIRST (the windowedRows() discipline): touch windowVer at the TOP so the spacer-<td>
11141139
// :style binding subscribes on the fine-grained targets before the `!virtualizer` early return.
1115-
const ver = this.windowVer();
1116-
if (!this.virtual() || !this.virtualizer || ver < 0) return 0;
1140+
void this.windowVer();
1141+
if (!this.virtual() || !this.virtualizer) return 0;
11171142
const items = this.virtualizer.getVirtualItems();
11181143
return items.length ? items[0].start : 0;
11191144
};
11201145
padBottom = () => {
1121-
const ver = this.windowVer();
1122-
if (!this.virtual() || !this.virtualizer || ver < 0) return 0;
1146+
// subscribe-first, see windowedRows() (IN-04): touch windowVer before the early return so the
1147+
// fine-grained spacer :style binding subscribes on its first eval while virtualizer is null.
1148+
void this.windowVer();
1149+
if (!this.virtual() || !this.virtualizer) return 0;
11231150
const items = this.virtualizer.getVirtualItems();
11241151
if (!items.length) return 0;
11251152
return this.virtualizer.getTotalSize() - items[items.length - 1].end;
@@ -1132,6 +1159,14 @@ export class DataTable {
11321159
};
11331160
remeasureWindow = () => {
11341161
if (!this.virtualizer || !this.gridRoot) return;
1162+
// Bail ONLY while a PROGRAMMATIC scroll is in flight: virtualizer.scrollState is non-null
1163+
// exclusively during scrollToIndex / scrollToOffset (the D-12 scroll-then-focus seam) and
1164+
// null for ordinary user/scrollTop-driven scrolling (verified virtual-core@3.17.1: set in
1165+
// scrollToIndex L992, cleared to null on reconcile L378). Measuring mid-scrollToIndex lets
1166+
// resizeItem nudge the offset and starve the scroll target (the Solid off-window focus
1167+
// regression); the next settled onChange re-measures the stable window. Manual-scroll
1168+
// recycling (the CR-01 case) has scrollState === null, so it measures normally.
1169+
if (this.virtualizer.scrollState) return;
11351170
const trs = this.gridRoot.querySelectorAll('tbody.rdt-tbody > tr[data-index]');
11361171
for (const tr of trs as any) this.virtualizer.measureElement(tr);
11371172
};

‎packages/ui/data-table/packages/lit/src/DataTable.ts‎

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,8 @@ ${this.virtual ? html`<div class="rdt-scroll" style=${this.maxHeight ? 'max-heig
785785

786786
gridScrollEl: any = null;
787787

788+
remeasurePending = false;
789+
788790
GRID_PAGE_STEP = 10;
789791

790792
gridRoot: any = null;
@@ -1088,9 +1090,31 @@ ${this.virtual ? html`<div class="rdt-scroll" style=${this.maxHeight ? 'max-heig
10881090
getItemKey: this.virtualItemKey,
10891091
onChange: () => {
10901092
this._windowVer.value = this._windowVer.value + 1;
1093+
// CR-01: re-observe the freshly-committed window so RECYCLED rows get measured.
1094+
// virtual-core only observe()s a node you explicitly hand to measureElement (it does
1095+
// NOT auto-discover rendered rows — measureElement is the SOLE caller of
1096+
// observer.observe, virtual-core@3.17.1 dist/esm/index.js:794-817). Rows that recycle
1097+
// into view on scroll are brand-new DOM nodes; without re-sweeping they keep the
1098+
// estimateRowHeight seed forever and the spacer math drifts (req-2). Deferred one frame
1099+
// so the new <tr> set is in the DOM before we measure. Safe from an infinite
1100+
// measure→onChange→measure loop: measureElement is idempotent on an already-observed
1101+
// node (the `prevNode !== node` guard), and resizeItem only re-fires onChange when the
1102+
// measured height actually DIFFERS from the cached one (delta !== 0) — an unchanged
1103+
// re-measure is a no-op.
1104+
this.scheduleRemeasure();
10911105
}
10921106
});
10931107

1108+
scheduleRemeasure = () => {
1109+
if (this.remeasurePending) return;
1110+
this.remeasurePending = true;
1111+
const run = () => {
1112+
this.remeasurePending = false;
1113+
this.remeasureWindow();
1114+
};
1115+
if (typeof requestAnimationFrame === 'function') requestAnimationFrame(run);else if (typeof queueMicrotask !== 'undefined') queueMicrotask(run);else setTimeout(run, 0);
1116+
};
1117+
10941118
windowedRows = () => {
10951119
// SUBSCRIBE FIRST (fine-grained targets): touch the reactive windowVer at the TOP — BEFORE any
10961120
// early return — so Solid's <For>/Svelte's {#each} accessor subscribes to it on its FIRST eval,
@@ -1101,7 +1125,7 @@ ${this.virtual ? html`<div class="rdt-scroll" style=${this.maxHeight ? 'max-heig
11011125
// blank forever (the Solid/Svelte fine-grained bug). Coarse targets re-render wholesale so the
11021126
// placement is a no-op for them. The post-construction windowVer bump in $onMount fires the
11031127
// first re-run that picks up the now-non-null virtualizer.
1104-
const ver = this._windowVer.value;
1128+
void this._windowVer.value;
11051129
if (!this.virtualizer) {
11061130
// Virtual OFF → full set (the r-else table never calls this, but keep it total). Virtual ON
11071131
// but the virtualizer is not yet constructed (pre-$onMount first paint) → render NOTHING so
@@ -1116,27 +1140,32 @@ ${this.virtual ? html`<div class="rdt-scroll" style=${this.maxHeight ? 'max-heig
11161140
}
11171141
return [];
11181142
}
1119-
if (ver < 0) return [];
11201143
const items = this.virtualizer.getVirtualItems();
11211144
const rowList = this._rows.value || [];
1145+
// WR-01: drop any virtual item whose index outruns the current full-model rows (a brief
1146+
// shrink window where the virtualizer count is stale relative to $data.rows on the async
1147+
// onChange→windowVer path). The template keys on wr.row.id, so a row:undefined entry would
1148+
// throw "Cannot read properties of undefined"; filter it here so the template never sees it.
11221149
return items.map((vi: any) => ({
11231150
vi,
11241151
row: rowList[vi.index]
1125-
}));
1152+
})).filter((wr: any) => wr.row);
11261153
};
11271154

11281155
padTop = () => {
1129-
// SUBSCRIBE FIRST (the windowedRows() discipline): read windowVer at the TOP so the spacer-<td>
1156+
// SUBSCRIBE FIRST (the windowedRows() discipline): touch windowVer at the TOP so the spacer-<td>
11301157
// :style binding subscribes on the fine-grained targets before the `!virtualizer` early return.
1131-
const ver = this._windowVer.value;
1132-
if (!this.virtual || !this.virtualizer || ver < 0) return 0;
1158+
void this._windowVer.value;
1159+
if (!this.virtual || !this.virtualizer) return 0;
11331160
const items = this.virtualizer.getVirtualItems();
11341161
return items.length ? items[0].start : 0;
11351162
};
11361163

11371164
padBottom = () => {
1138-
const ver = this._windowVer.value;
1139-
if (!this.virtual || !this.virtualizer || ver < 0) return 0;
1165+
// subscribe-first, see windowedRows() (IN-04): touch windowVer before the early return so the
1166+
// fine-grained spacer :style binding subscribes on its first eval while virtualizer is null.
1167+
void this._windowVer.value;
1168+
if (!this.virtual || !this.virtualizer) return 0;
11401169
const items = this.virtualizer.getVirtualItems();
11411170
if (!items.length) return 0;
11421171
return this.virtualizer.getTotalSize() - items[items.length - 1].end;
@@ -1151,6 +1180,14 @@ ${this.virtual ? html`<div class="rdt-scroll" style=${this.maxHeight ? 'max-heig
11511180

11521181
remeasureWindow = () => {
11531182
if (!this.virtualizer || !this.gridRoot) return;
1183+
// Bail ONLY while a PROGRAMMATIC scroll is in flight: virtualizer.scrollState is non-null
1184+
// exclusively during scrollToIndex / scrollToOffset (the D-12 scroll-then-focus seam) and
1185+
// null for ordinary user/scrollTop-driven scrolling (verified virtual-core@3.17.1: set in
1186+
// scrollToIndex L992, cleared to null on reconcile L378). Measuring mid-scrollToIndex lets
1187+
// resizeItem nudge the offset and starve the scroll target (the Solid off-window focus
1188+
// regression); the next settled onChange re-measures the stable window. Manual-scroll
1189+
// recycling (the CR-01 case) has scrollState === null, so it measures normally.
1190+
if (this.virtualizer.scrollState) return;
11541191
const trs = this.gridRoot.querySelectorAll('tbody.rdt-tbody > tr[data-index]');
11551192
for (const tr of trs as any) this.virtualizer.measureElement(tr);
11561193
};

‎packages/ui/data-table/packages/react/src/DataTable.tsx‎

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ const DataTable = forwardRef<DataTableHandle, DataTableProps>(function DataTable
122122
const gridScrollEl = useRef<any>(null);
123123
const virtualizerCleanup = useRef<any>(null);
124124
const programmatic = useRef(0);
125+
const remeasurePending = useRef(false);
125126
const selectAllBox = useRef<any>(null);
126127
const lastData = useRef<any>(null);
127128
const lastDataLen = useRef(-1);
@@ -474,8 +475,29 @@ const DataTable = forwardRef<DataTableHandle, DataTableProps>(function DataTable
474475
getItemKey: virtualItemKey,
475476
onChange: () => {
476477
setWindowVer(prev => prev + 1);
478+
// CR-01: re-observe the freshly-committed window so RECYCLED rows get measured.
479+
// virtual-core only observe()s a node you explicitly hand to measureElement (it does
480+
// NOT auto-discover rendered rows — measureElement is the SOLE caller of
481+
// observer.observe, virtual-core@3.17.1 dist/esm/index.js:794-817). Rows that recycle
482+
// into view on scroll are brand-new DOM nodes; without re-sweeping they keep the
483+
// estimateRowHeight seed forever and the spacer math drifts (req-2). Deferred one frame
484+
// so the new <tr> set is in the DOM before we measure. Safe from an infinite
485+
// measure→onChange→measure loop: measureElement is idempotent on an already-observed
486+
// node (the `prevNode !== node` guard), and resizeItem only re-fires onChange when the
487+
// measured height actually DIFFERS from the cached one (delta !== 0) — an unchanged
488+
// re-measure is a no-op.
489+
scheduleRemeasure();
477490
}
478-
}), [props.estimateRowHeight, virtualItemKey, windowingSource]);
491+
}), [props.estimateRowHeight, scheduleRemeasure, virtualItemKey, windowingSource]);
492+
function scheduleRemeasure() {
493+
if (remeasurePending.current) return;
494+
remeasurePending.current = true;
495+
const run = () => {
496+
remeasurePending.current = false;
497+
remeasureWindow();
498+
};
499+
if (typeof requestAnimationFrame === 'function') requestAnimationFrame(run);else if (typeof queueMicrotask !== 'undefined') queueMicrotask(run);else setTimeout(run, 0);
500+
}
479501
function windowedRows() {
480502
// SUBSCRIBE FIRST (fine-grained targets): touch the reactive windowVer at the TOP — BEFORE any
481503
// early return — so Solid's <For>/Svelte's {#each} accessor subscribes to it on its FIRST eval,
@@ -486,7 +508,7 @@ const DataTable = forwardRef<DataTableHandle, DataTableProps>(function DataTable
486508
// blank forever (the Solid/Svelte fine-grained bug). Coarse targets re-render wholesale so the
487509
// placement is a no-op for them. The post-construction windowVer bump in $onMount fires the
488510
// first re-run that picks up the now-non-null virtualizer.
489-
const ver = windowVer;
511+
void windowVer;
490512
if (!virtualizer.current) {
491513
// Virtual OFF → full set (the r-else table never calls this, but keep it total). Virtual ON
492514
// but the virtualizer is not yet constructed (pre-$onMount first paint) → render NOTHING so
@@ -501,25 +523,30 @@ const DataTable = forwardRef<DataTableHandle, DataTableProps>(function DataTable
501523
}
502524
return [];
503525
}
504-
if (ver < 0) return [];
505526
const items = virtualizer.current.getVirtualItems();
506527
const rowList = rows || [];
528+
// WR-01: drop any virtual item whose index outruns the current full-model rows (a brief
529+
// shrink window where the virtualizer count is stale relative to $data.rows on the async
530+
// onChange→windowVer path). The template keys on wr.row.id, so a row:undefined entry would
531+
// throw "Cannot read properties of undefined"; filter it here so the template never sees it.
507532
return items.map((vi: any) => ({
508533
vi,
509534
row: rowList[vi.index]
510-
}));
535+
})).filter((wr: any) => wr.row);
511536
}
512537
function padTop() {
513-
// SUBSCRIBE FIRST (the windowedRows() discipline): read windowVer at the TOP so the spacer-<td>
538+
// SUBSCRIBE FIRST (the windowedRows() discipline): touch windowVer at the TOP so the spacer-<td>
514539
// :style binding subscribes on the fine-grained targets before the `!virtualizer` early return.
515-
const ver = windowVer;
516-
if (!props.virtual || !virtualizer.current || ver < 0) return 0;
540+
void windowVer;
541+
if (!props.virtual || !virtualizer.current) return 0;
517542
const items = virtualizer.current.getVirtualItems();
518543
return items.length ? items[0].start : 0;
519544
}
520545
function padBottom() {
521-
const ver = windowVer;
522-
if (!props.virtual || !virtualizer.current || ver < 0) return 0;
546+
// subscribe-first, see windowedRows() (IN-04): touch windowVer before the early return so the
547+
// fine-grained spacer :style binding subscribes on its first eval while virtualizer is null.
548+
void windowVer;
549+
if (!props.virtual || !virtualizer.current) return 0;
523550
const items = virtualizer.current.getVirtualItems();
524551
if (!items.length) return 0;
525552
return virtualizer.current.getTotalSize() - items[items.length - 1].end;
@@ -532,6 +559,14 @@ const DataTable = forwardRef<DataTableHandle, DataTableProps>(function DataTable
532559
}
533560
const remeasureWindow = useCallback(() => {
534561
if (!virtualizer.current || !gridRoot.current) return;
562+
// Bail ONLY while a PROGRAMMATIC scroll is in flight: virtualizer.scrollState is non-null
563+
// exclusively during scrollToIndex / scrollToOffset (the D-12 scroll-then-focus seam) and
564+
// null for ordinary user/scrollTop-driven scrolling (verified virtual-core@3.17.1: set in
565+
// scrollToIndex L992, cleared to null on reconcile L378). Measuring mid-scrollToIndex lets
566+
// resizeItem nudge the offset and starve the scroll target (the Solid off-window focus
567+
// regression); the next settled onChange re-measures the stable window. Manual-scroll
568+
// recycling (the CR-01 case) has scrollState === null, so it measures normally.
569+
if (virtualizer.current.scrollState) return;
535570
const trs = gridRoot.current.querySelectorAll('tbody.rdt-tbody > tr[data-index]');
536571
for (const tr of trs as any) virtualizer.current.measureElement(tr);
537572
}, []);

0 commit comments

Comments
 (0)