From 38baf091864cd390e0a5bd7f162f795b26059a21 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Wed, 4 Nov 2026 13:32:43 +0000 Subject: [PATCH] fix(desktop): compensate head trims from the unclamped scroll offset Problem: `message-target-highlight.spec.ts:79` ("message target flashes: ...") flaked in Desktop Smoke E2E, always at :179: the linked row had left the viewport 2.2 s after it flashed. - main: https://github.com/block/buzz/actions/runs/41568206043/job/125695139845 https://github.com/block/buzz/actions/runs/41669669478/job/126035103096 https://github.com/block/buzz/actions/runs/41669740241/job/126035480466 https://github.com/block/buzz/actions/runs/41694874145/job/126123865714 https://github.com/block/buzz/actions/runs/41728489172/job/126240193479 https://github.com/block/buzz/actions/runs/41737019256/job/126269365379 - branches: https://github.com/block/buzz/actions/runs/41575774131/job/125719148353 https://github.com/block/buzz/actions/runs/41636043464/job/125918005733 https://github.com/block/buzz/actions/runs/41751914119/job/126319694104 https://github.com/block/buzz/actions/runs/41544910958/job/125618626645 https://github.com/block/buzz/actions/runs/41714110046/job/126190515793 All eleven runs predate #8742, and each failed in a case that picks `mounted[2]`: the bottom-settle mode #8742 fixed by holding the target. Run 41694874145 also failed once in "other channel, row near the bottom", and that mode survives #8742: on current main it fails 40 of 60 local runs with tracing on, as CI's retries run. Cause: Re-entering a channel refreshes its newest window, which drops the older pages from the head of the cached timeline. #8660 passes that head trim to Virtua as a `shift`, and Virtua compensates by the removed height in a layout effect. Our Virtua patch (#2855) applies shift compensation to the live DOM `scrollTop` so that corrections landing before the next scroll event accumulate. But by then the browser has laid out the shorter list and clamped `scrollTop` to the new floor whenever the reader was within the removed height of it, so the removal is counted twice. In a failing run the view sat 6px above the floor: trimming the 412px channel intro moved it 406px too far, and trimming 3054px of older rows then wrote `scrollTop` -907, pinning it to the top. `onStartReached` loaded older history and the target never came back. A reader scrolled slightly above the bottom loses their place the same way. Fix: The patched Virtua scroller remembers the last offset it observed in a scroll event or wrote itself. When a shift compensation finds the live offset below that and pinned to the floor, the browser clamped it, so the compensation starts from the remembered offset. Otherwise it keeps #2855's live-offset base. The ESM and CJS builds change identically, and the lockfile carries the new patch hash. Validation: - New `virtuaShiftClampPatch.jsdom-test.mjs` drives the real patched `VList` over a scroller that clamps like a browser. Head trims far from, near and at the floor keep the reader's rows in place, a trim keeps a scroll whose event has not fired yet, and a prepend then a trim before any scroll event accumulate. Each part of the new condition fails a case when removed: the previous patch line fails the near-floor, at-floor and accumulation cases (3500 for 3900 and 4000, 3000 for 4000); upstream's store-offset base fails the unreported-scroll and accumulation cases; always using the remembered offset, or dropping the floor check, fails the unreported-scroll case (2500 for 1900). - "other channel, row near the bottom" with `--repeat-each=30 --workers=4 --trace=retain-on-failure`: 40/60 failed at :179 on current main, 0/60 passed with the fix, and 25/30 failed again with the previous patch reinstalled. - All five cases, 20 traced runs each: 100/100 passed. - scroll-history, virtualization, timeline-no-shift, overscroll-boundary, dm-history-live-regression and live-broadcast-reply-timeline, 3 runs each: 117/117 passed. - Full smoke project: 1485 passed, 3 skipped, 2 failed. `mentions.spec.ts:807` passed 5/5 on rerun. `messaging.spec.ts:2785` (own-message avatar) fails the same way on a current-main build here (3/3). - `pnpm test`: 6933 unit and 176 jsdom tests passed. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit 6a0b08a9510469b0fe45d23fdcc4bcf15ef3402f) --- .../ui/virtuaShiftClampPatch.jsdom-test.mjs | 172 ++++++++++++++++++ patches/virtua@0.49.3.patch | 58 +++++- pnpm-lock.yaml | 6 +- 3 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 desktop/src/features/messages/ui/virtuaShiftClampPatch.jsdom-test.mjs diff --git a/desktop/src/features/messages/ui/virtuaShiftClampPatch.jsdom-test.mjs b/desktop/src/features/messages/ui/virtuaShiftClampPatch.jsdom-test.mjs new file mode 100644 index 00000000000..5a534db271f --- /dev/null +++ b/desktop/src/features/messages/ui/virtuaShiftClampPatch.jsdom-test.mjs @@ -0,0 +1,172 @@ +/** + * Shift-mode scroll compensation in the patched Virtua build + * (patches/virtua@0.49.3.patch), driven through the real `VList`. + * + * A head trim (a channel-window refresh dropping older pages) shrinks the + * list from the top. When the reader is within the removed height of the + * floor, the browser clamps `scrollTop` to the new floor before Virtua's + * layout-effect compensation runs. Compensating from that clamped offset + * counts the removal twice and throws the reader toward older history. + */ +import assert from "node:assert/strict"; +import { afterEach, test } from "node:test"; + +import React, { act } from "react"; +import { createRoot } from "react-dom/client"; +import { VList } from "virtua"; + +const ROW = 50; +const VIEWPORT = 500; +const ROWS = Array.from({ length: 100 }, (_, index) => index); + +// jsdom has no layout. Model the browser behavior under test: an +// `overflow-y: auto` element's scroll offset is clamped to its scrollable +// extent, which Virtua sets as its inner container's height. +const offsets = new WeakMap(); +const isScroller = (element) => element.style?.overflowY === "auto"; +const maxOffset = (element) => + Math.max( + (Number.parseFloat(element.firstElementChild?.style.height) || 0) - + VIEWPORT, + 0, + ); +const clampOffset = (element, value) => + Math.min(Math.max(value, 0), maxOffset(element)); +Object.defineProperties(window.HTMLElement.prototype, { + clientHeight: { + configurable: true, + get() { + return isScroller(this) ? VIEWPORT : 0; + }, + }, + scrollHeight: { + configurable: true, + get() { + return isScroller(this) ? maxOffset(this) + VIEWPORT : 0; + }, + }, + scrollTop: { + configurable: true, + get() { + const offset = clampOffset(this, offsets.get(this) ?? 0); + offsets.set(this, offset); + return offset; + }, + set(value) { + offsets.set(this, clampOffset(this, value)); + }, + }, + offsetParent: { + configurable: true, + get() { + return this.parentElement; + }, + }, +}); + +const resizeObservers = new Set(); +window.ResizeObserver = class { + constructor(callback) { + this.callback = callback; + this.targets = new Set(); + resizeObservers.add(this); + } + observe(target) { + this.targets.add(target); + } + unobserve(target) { + this.targets.delete(target); + } + disconnect() { + resizeObservers.delete(this); + } +}; + +let root; +let container; +afterEach(() => { + act(() => root?.unmount()); + container?.remove(); +}); + +function renderList(rows, shift) { + act(() => + root.render( + React.createElement( + VList, + { data: rows, itemSize: () => ROW, shift }, + (row) => React.createElement("div", { key: row }, `row ${row}`), + ), + ), + ); +} + +/** Mounts the list and returns its scroller, with the viewport measured. */ +function mountList(rows) { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + renderList(rows, false); + const scroller = container.firstElementChild; + act(() => { + for (const observer of resizeObservers) { + if (observer.targets.has(scroller)) { + observer.callback([ + { target: scroller, contentRect: { width: 300, height: VIEWPORT } }, + ]); + } + } + }); + return scroller; +} + +function readerScrollsTo(scroller, offset) { + act(() => { + scroller.scrollTop = offset; + scroller.dispatchEvent(new window.Event("scroll")); + }); +} + +// 100 rows of 50px in a 500px viewport: the floor is at 4500, and trimming +// 10 rows removes 500px, more than the reader's distance from the floor in +// the last two cases. +for (const { name, start, trim, expected } of [ + { name: "far from the floor", start: 2000, trim: 10, expected: 1500 }, + { name: "near the floor", start: 4400, trim: 10, expected: 3900 }, + { name: "at the floor", start: 4500, trim: 10, expected: 4000 }, +]) { + test(`a head trim ${name} keeps the reader's rows in place`, () => { + const scroller = mountList(ROWS); + readerScrollsTo(scroller, start); + + renderList(ROWS.slice(trim), true); + + assert.equal(scroller.scrollTop, expected); + }); +} + +test("a head trim keeps a scroll the browser has not reported yet", () => { + // The reader's latest move is still waiting for its scroll event, so the + // live offset, not the last observed one, is where the reader is. + const scroller = mountList(ROWS); + readerScrollsTo(scroller, 3000); + scroller.scrollTop = 2400; + + renderList(ROWS.slice(10), true); + + assert.equal(scroller.scrollTop, 1900); +}); + +test("shift compensations before the next scroll event accumulate", () => { + // Successive shift writes land before the browser reports any of them, so + // each must build on the last write, including across a floor clamp. + const scroller = mountList(ROWS); + readerScrollsTo(scroller, 4500); + const older = Array.from({ length: 10 }, (_, index) => -10 + index); + + renderList([...older, ...ROWS], true); + assert.equal(scroller.scrollTop, 5000); + + renderList(ROWS.slice(10), true); + assert.equal(scroller.scrollTop, 4000); +}); diff --git a/patches/virtua@0.49.3.patch b/patches/virtua@0.49.3.patch index 5debe418ebd..2677841967e 100644 --- a/patches/virtua@0.49.3.patch +++ b/patches/virtua@0.49.3.patch @@ -1,5 +1,5 @@ diff --git a/lib/index.cjs b/lib/index.cjs -index e02dfd0b3db60faff2cc705d7ce0aa51cb958f5a..b12677a356f748ebccf6e16cb781efe1315bb55b 100644 +index e02dfd0b3db60faff2cc705d7ce0aa51cb958f5a..c1f84bac4349fa3d0148eac9a7815111b35ce3fb 100644 --- a/lib/index.cjs +++ b/lib/index.cjs @@ -35,10 +35,18 @@ const r = null, {min: n, max: s, abs: i, floor: l} = Math, c = (e, t, o) => n(o, @@ -64,16 +64,37 @@ index e02dfd0b3db60faff2cc705d7ce0aa51cb958f5a..b12677a356f748ebccf6e16cb781efe1 const r = l() - c; 150 > r && 50 < r && (o ? t.deltaX : t.deltaY) && (f = !0); }, v = () => { -@@ -266,7 +279,7 @@ const r = null, {min: n, max: s, abs: i, floor: l} = Math, c = (e, t, o) => n(o, +@@ -250,23 +264,24 @@ const r = null, {min: n, max: s, abs: i, floor: l} = Math, c = (e, t, o) => n(o, + r && r(); + } ]; + }, M = (e, t) => { +- let o, r, n = d(), s = !1; +- const i = t ? "scrollLeft" : "scrollTop", l = t ? "overflowX" : "overflowY", [f, u] = C(e, () => n[0], (e, r) => { ++ let o, r, n = d(), s = !1, kk = 0; ++ const i = t ? "scrollLeft" : "scrollTop", l = t ? "overflowX" : "overflowY", ss = t ? "scrollWidth" : "scrollHeight", cc = t ? "clientWidth" : "clientHeight", [f, u] = C(e, () => n[0], (e, r) => { + e = R(e, s), r ? o.scrollTo({ + [t ? "left" : "top"]: e, + behavior: "smooth" +- }) : o[i] = e; ++ }) : (o[i] = e, kk = R(o[i], s)); + }); + return { + L(c, f) { +- o = f, t && (s = "rtl" === getComputedStyle(f).direction), r = T(e, f, t, () => R(f[i], s), (t, o, r) => { ++ o = f, t && (s = "rtl" === getComputedStyle(f).direction), r = T(e, f, t, () => kk = R(f[i], s), (t, o, r) => { + if (r) { + const e = f.style, t = e[l]; + e[l] = "hidden", k(() => { e[l] = t; }); } - f[i] = R(e.C() + t, s), o && u(); -+ f[i] = R((o ? R(f[i], s) : e.C()) + t, s), o && u(); ++ const dd = R(f[i], s); ++ f[i] = R((o ? dd < kk && dd >= f[ss] - f[cc] - 1 ? kk : dd : e.C()) + t, s), kk = R(f[i], s), o && u(); }), n[1](!0); }, v() { -@@ -519,11 +532,11 @@ const r = null, {min: n, max: s, abs: i, floor: l} = Math, c = (e, t, o) => n(o, +@@ -519,11 +534,11 @@ const r = null, {min: n, max: s, abs: i, floor: l} = Math, c = (e, t, o) => n(o, })(e); return [ e => t[e], t.length ]; }, [ e, o ]), j = /*#__PURE__*/ t.forwardRef(({children: r, data: n, bufferSize: s, itemSize: i, shift: l, horizontal: c, keepMounted: d, cache: a, startMargin: h = 0, ssrCount: g, as: p = "div", item: v = "div", scrollRef: _, onScroll: w, onScrollEnd: S}, m) => { @@ -89,7 +110,7 @@ index e02dfd0b3db60faff2cc705d7ce0aa51cb958f5a..b12677a356f748ebccf6e16cb781efe1 const o = $(t); return e.jsx(Y, { diff --git a/lib/index.js b/lib/index.js -index 110ac3858a002a6cdb698da2b56350bc1bf609d2..db83efc83fb3b6d7aa07d83b7463d6ed7746287e 100644 +index 110ac3858a002a6cdb698da2b56350bc1bf609d2..5ccbaee734c167b2df98e9fe8233dd467fed8ff1 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1,7 +1,7 @@ @@ -162,16 +183,37 @@ index 110ac3858a002a6cdb698da2b56350bc1bf609d2..db83efc83fb3b6d7aa07d83b7463d6ed const n = i() - l; 150 > n && 50 < n && (o ? t.deltaX : t.deltaY) && (c = !0); }, v = () => { -@@ -270,7 +283,7 @@ const u = null, {min: d, max: a, abs: h, floor: g} = Math, p = (e, t, o) => d(o, +@@ -254,23 +268,24 @@ const u = null, {min: d, max: a, abs: h, floor: g} = Math, p = (e, t, o) => d(o, + n && n(); + } ]; + }, A = (e, t) => { +- let o, n, r = _(), s = !1; +- const i = t ? "scrollLeft" : "scrollTop", l = t ? "overflowX" : "overflowY", [c, f] = J(e, () => r[0], (e, n) => { ++ let o, n, r = _(), s = !1, kk = 0; ++ const i = t ? "scrollLeft" : "scrollTop", l = t ? "overflowX" : "overflowY", ss = t ? "scrollWidth" : "scrollHeight", cc = t ? "clientWidth" : "clientHeight", [c, f] = J(e, () => r[0], (e, n) => { + e = W(e, s), n ? o.scrollTo({ + [t ? "left" : "top"]: e, + behavior: "smooth" +- }) : o[i] = e; ++ }) : (o[i] = e, kk = W(o[i], s)); + }); + return { + N(c, u) { +- o = u, t && (s = "rtl" === getComputedStyle(u).direction), n = B(e, u, t, () => W(u[i], s), (t, o, n) => { ++ o = u, t && (s = "rtl" === getComputedStyle(u).direction), n = B(e, u, t, () => kk = W(u[i], s), (t, o, n) => { + if (n) { + const e = u.style, t = e[l]; + e[l] = "hidden", H(() => { e[l] = t; }); } - u[i] = W(e.C() + t, s), o && f(); -+ u[i] = W((o ? W(u[i], s) : e.C()) + t, s), o && f(); ++ const dd = W(u[i], s); ++ u[i] = W((o ? dd < kk && dd >= u[ss] - u[cc] - 1 ? kk : dd : e.C()) + t, s), kk = W(u[i], s), o && f(); }), r[1](!0); }, v() { -@@ -523,11 +536,11 @@ const u = null, {min: d, max: a, abs: h, floor: g} = Math, p = (e, t, o) => d(o, +@@ -523,11 +538,11 @@ const u = null, {min: d, max: a, abs: h, floor: g} = Math, p = (e, t, o) => d(o, })(e); return [ e => o[e], o.length ]; }, [ e, t ]), Z = /*#__PURE__*/ i(({children: t, data: o, bufferSize: r, itemSize: s, shift: i, horizontal: u, keepMounted: d, cache: a, startMargin: h = 0, ssrCount: g, as: p = "div", item: _ = "div", scrollRef: w, onScroll: S, onScrollEnd: $}, z) => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3847cdc5e7d..5a193207b1d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -10,7 +10,7 @@ overrides: patchedDependencies: isomorphic-git: e9b414a60d4cf1d8aa18f7a779483984e821989967c235662566e94ef0238d3f - virtua@0.49.3: 30a7a92b94800ec37be8d36546ea98c04a05dcd0637f9fac27a06f016ba2eff4 + virtua@0.49.3: f4df71c39f6c25acb1179850d15f6de8eb97fc6af9bbefdfa7fd4cb074b3b669 importers: @@ -248,7 +248,7 @@ importers: version: 2.1.0 virtua: specifier: 0.49.3 - version: 0.49.3(patch_hash=30a7a92b94800ec37be8d36546ea98c04a05dcd0637f9fac27a06f016ba2eff4)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) + version: 0.49.3(patch_hash=f4df71c39f6c25acb1179850d15f6de8eb97fc6af9bbefdfa7fd4cb074b3b669)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) yaml: specifier: ^2.8.3 version: 2.9.0 @@ -7043,7 +7043,7 @@ snapshots: '@types/unist': 3.0.3 vfile-message: 4.0.3 - virtua@0.49.3(patch_hash=30a7a92b94800ec37be8d36546ea98c04a05dcd0637f9fac27a06f016ba2eff4)(react-dom@19.2.8(react@19.2.8))(react@19.2.8): + virtua@0.49.3(patch_hash=f4df71c39f6c25acb1179850d15f6de8eb97fc6af9bbefdfa7fd4cb074b3b669)(react-dom@19.2.8(react@19.2.8))(react@19.2.8): optionalDependencies: react: 19.2.8 react-dom: 19.2.8(react@19.2.8)