Skip to content

Commit 16dd548

Browse files
bloveclaude
andauthored
fix(renderer-dom): journal path anchors a removed top row like the replacement path (#491) (#508)
The cooperative replacement path re-anchors a removed viewport-top row to its nearest surviving OLD-order neighbor (+1, -1, +2, ...), and G3c gave the synchronous reorder/refilter reset paths the same semantics through resolveSyncAnchorRef. The incremental journal path, however, still called restoreAnchorRequest with exact-only resolution: a journaled applyTransaction that removed the anchored row fell straight to globalScroll(viewport.scrollTop). User-visible in a narrow window: one journaled transaction removing the viewport-top row AND other rows above it makes the journal path keep the pixel scroll (content jumps) where the replacement path holds the neighbor - and the SAME logical transaction can reach either path nondeterministically (journal overflow, an active replacement, or a misaligned revision all reroute to replacement). The searchOldNeighbors parameter is deleted rather than flipped: every synchronous publish now resolves through the same ladder - exact ref first (O(1), the common case), neighbor search only when the anchored row is gone; "reorder" structurally never engages it since the row set is unchanged. The old comment's "pinned behavior" claim was unbacked - no test pinned the fall-to-global; the new test pins the neighbor semantics against a forced-replacement oracle instead. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 9e163bf commit 16dd548

2 files changed

Lines changed: 93 additions & 9 deletions

File tree

packages/renderer-dom/src/__tests__/indexed-renderer.test.ts

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4399,6 +4399,89 @@ describe("indexed DOM row layout controller", () => {
43994399
expect(controller.getState().rowHeights).toBe(result);
44004400
});
44014401
});
4402+
4403+
test("a journaled remove of the anchored row degrades like a full replacement (#491)", () => {
4404+
// The incremental JOURNAL sibling of "an anchor row filtered out
4405+
// degrades like a full replacement": one journaled transaction removes
4406+
// the anchored viewport-top row (row 4) AND a measured row above it
4407+
// (row 2), so the neighbor-anchored scrollTop and the retained pixel
4408+
// scrollTop provably differ — the fixture can DISPROVE.
4409+
//
4410+
// Anchor inside row 4 (rank 3, offsets 126..170 under the 41..50
4411+
// measured heights), 4px below its top. Removing rows 2 and 4 makes the
4412+
// old-order neighbor search (+1 first) land on row 5, whose new rank is
4413+
// 2 with offset 41 + 43 = 84, so the anchored viewport follows it to
4414+
// 84 + 4 = 88px. Exact-only resolution falls to the retained pixel
4415+
// scroll, 130 — the divergence #491 pins shut.
4416+
const neighborAnchoredScrollTop = 41 + 43 + 4;
4417+
const retainedPixelScrollTop = 130;
4418+
expect(neighborAnchoredScrollTop).not.toBe(retainedPixelScrollTop);
4419+
4420+
const model = createModel(tenRows);
4421+
const { controller } = createReadyController(model);
4422+
for (const [rowId, height] of allMeasurements) {
4423+
controller.measure(data(rowId), height);
4424+
}
4425+
controller.setViewport({
4426+
scrollTop: retainedPixelScrollTop,
4427+
viewportHeight: 88,
4428+
overscan: 0,
4429+
});
4430+
expect(controller.getState().scrollTop).toBe(retainedPixelScrollTop);
4431+
const before = getRowLayoutControllerDiagnosticsForTesting(controller);
4432+
4433+
model.applyTransaction({ remove: [2, 4] });
4434+
4435+
// Synchronous, and through the JOURNAL path: no replacement started,
4436+
// and neither synchronous reset fast path fired.
4437+
const after = controller.getState();
4438+
expect(after.status.kind).toBe("ready");
4439+
expect(after.snapshot?.visibleRowCount).toBe(8);
4440+
expect(after.observedRevision).toBe(model.getState().snapshot.revision);
4441+
const diagnostics =
4442+
getRowLayoutControllerDiagnosticsForTesting(controller);
4443+
expect(diagnostics.replacementStartCount).toBe(
4444+
before.replacementStartCount,
4445+
);
4446+
expect(diagnostics.reorderPathCount).toBe(before.reorderPathCount);
4447+
expect(diagnostics.refilterPathCount).toBe(before.refilterPathCount);
4448+
expect(after.scrollTop).toBe(neighborAnchoredScrollTop);
4449+
4450+
// The replacement reference. `createReplacementOracle` only rewrites
4451+
// RESETS, and a journaled transaction never produces one, so this
4452+
// oracle's `changesSince` reports every range as a bulk-replace reset —
4453+
// forcing the cooperative replacement over the same commit.
4454+
const oracleModel = createModel(tenRows);
4455+
const oracle = createReadyController(oracleModel);
4456+
for (const [rowId, height] of allMeasurements) {
4457+
oracle.controller.measure(data(rowId), height);
4458+
}
4459+
oracle.controller.setViewport({
4460+
scrollTop: retainedPixelScrollTop,
4461+
viewportHeight: 88,
4462+
overscan: 0,
4463+
});
4464+
vi.spyOn(oracleModel, "changesSince").mockImplementation(() => ({
4465+
kind: "reset" as const,
4466+
toRevision: oracleModel.getState().snapshot.revision,
4467+
reason: "bulk-replace" as const,
4468+
}));
4469+
const oracleBefore = getRowLayoutControllerDiagnosticsForTesting(
4470+
oracle.controller,
4471+
);
4472+
4473+
oracleModel.applyTransaction({ remove: [2, 4] });
4474+
oracle.scheduler.flushAll();
4475+
4476+
const reference = oracle.controller.getState();
4477+
expect(reference.status.kind).toBe("ready");
4478+
// Prove the oracle really took the replacement path.
4479+
expect(
4480+
getRowLayoutControllerDiagnosticsForTesting(oracle.controller)
4481+
.replacementStartCount,
4482+
).toBe(oracleBefore.replacementStartCount + 1);
4483+
expect(reference.scrollTop).toBe(after.scrollTop);
4484+
});
44024485
});
44034486
});
44044487

packages/renderer-dom/src/row-layout-controller.ts

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1992,16 +1992,17 @@ export function createRowLayoutController<
19921992
target: PretableRowModelSnapshot<TRow, TRowId, TColumns>,
19931993
root: RowHeightIndex<PretableVisibleRowRef<TRowId>>,
19941994
anchor: CapturedAnchor<TRowId> | undefined,
1995-
// The incremental journal path keeps exact-only resolution (its remove
1996-
// operations already re-anchor via the surviving exact ref or fall to a
1997-
// global scroll — pinned behavior); the synchronous reset paths opt into
1998-
// the replacement-mirroring neighbor search above.
1999-
searchOldNeighbors = false,
20001995
): ScrollRequest => {
20011996
if (anchor !== undefined) {
2002-
const resolved = searchOldNeighbors
2003-
? resolveSyncAnchorRef(target, anchor)
2004-
: target.nearestVisibleRef(anchor.heightAnchor.ref);
1997+
// Every synchronous publish — the incremental journal path and both
1998+
// reset fast paths — resolves through the replacement-mirroring ladder
1999+
// above: exact ref first (O(1), the common case), the old-order
2000+
// neighbor search only when a remove took the anchored row out. A
2001+
// journaled remove of the viewport-top row therefore anchors its
2002+
// nearest surviving neighbor exactly as the cooperative replacement
2003+
// would for the same commit (#491), instead of degrading to a global
2004+
// pixel scroll.
2005+
const resolved = resolveSyncAnchorRef(target, anchor);
20052006
if (resolved !== undefined) {
20062007
const index = target.indexOf(resolved);
20072008
if (index >= 0) {
@@ -2113,7 +2114,7 @@ export function createRowLayoutController<
21132114
// and the replacement path would re-anchor its nearest
21142115
// OLD-order neighbor. Under "reorder" the exact ref always
21152116
// survives and the search never engages.
2116-
restoreAnchorRequest(target, root, anchor, true),
2117+
restoreAnchorRequest(target, root, anchor),
21172118
);
21182119
// Mirrors `finishReplacement`'s commit: a deferred viewport is
21192120
// applied by the publish above (it reads the live `viewport`),

0 commit comments

Comments
 (0)