Repository navigation
fix(desktop): always flash a linked message in the channel timeline - #8660
Conversation
Following a message link, opening a search hit or jumping to a message scrolls the virtualized timeline to it and should flash the row. The flash fired only when the row was already fully on screen at the moment the scroll ran, so it was skipped for a loaded row further up. Opening a link from another channel could also lose the target entirely: the row looked on screen while the channel's initial bottom pin was still in flight, the direct DOM scroll did not cancel that pin, and the timeline ended at the bottom. Route every virtualized target scroll through Virtua's scrollToIndex, which supersedes a pending bottom pin, and flash on every accepted target scroll. The highlight is keyed by id, so a row Virtua mounts after the scroll still renders it. Callers now read one return value: true once Virtua accepted the target, false while it is not loaded yet. Add an e2e table that checks the target row is on screen and flashing for same-channel and cross-channel links, on-screen, off-screen and unloaded targets. Three of its five cases fail on the previous code. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Logan Johnson <[email protected]>
Adversarial review of the target flash found the target could still be carried off screen after it first arrived: - A channel-window refresh drops older pages from the head of the list. Without Virtua's shift the rows above the reader vanished uncompensated, so the view fell to the floor. A pure head trim now shifts like a prepend. - An accepted target scroll did not cancel the hook's queued second mount-to-bottom frame, so a target that loaded before that frame was scrolled back to the bottom. - A target that was not loaded yet cancelled the opening bottom pin on every retry. Only an accepted target retires bottom intent now. Also animate only a row already in the viewport, and report a target as unreachable while the list is unmounted. The e2e spec now requires the row to stay on screen after the flash ends. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Logan Johnson <[email protected]>
Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Logan Johnson <[email protected]>
A target that loads before the hook's queued second mount-to-bottom frame must not be scrolled back to the bottom by that frame. The test fails without the cancellation in the accepted-target path. Co-authored-by: Cerveau <fc91c5708d48aecea993d9219505dc841bc7b0222b7ed325b8edfd026bfb43b5@buzz.block.builderlab.xyz> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Logan Johnson <[email protected]>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 0a362eef150ab16453993d442a8037baad766797..b8e172672525d7eb4fbb2834bf4caa23686ed926 (exact head b8e172672525d7eb4fbb2834bf4caa23686ed926)
Risk: High — this touches Desktop message-timeline virtualization, route-target scrolling, older-history loading, bottom-pin cancellation, and user-visible flash state. The change is bounded to Desktop renderer timeline behavior and tests.
Behavior/contracts traced:
- Virtualized target navigation now succeeds only after the virtualizer accepts the target message index; accepted targets cancel pending bottom intent and the queued second mount-bottom frame, mark the virtualizer non-bottom, and start the id-keyed highlight. Unloaded targets return
falseand keep retrying without clearing the route or falsely flashing (desktop/src/features/messages/ui/useAnchoredScroll.ts:453-480,:634-649,:897-909). TimelineMessageListmaps every timeline item message id to its Virtua row index and centers viascrollToIndex(..., { align: "center" }), avoiding stale direct DOM scroll ownership for off-screen/windowed rows (desktop/src/features/messages/ui/TimelineMessageList.tsx:686-694).- The head-trim compensation only enables Virtua
shiftwhen the new key stream is an exact non-empty suffix of the previous stream; shrink+append and middle deletion shapes stay rejected (desktop/src/features/messages/lib/virtualizedTimelineItems.ts:106-123; tests atdesktop/src/features/messages/lib/virtualizedTimelineItems.test.mjs:209-238). - The visible route-target flash remains the existing row pseudo-element and keeps the existing reduced-motion behavior (
motion-reduce:before:animate-none).
Findings: No blocking or non-blocking author-actionable findings.
Author action: none.
Verification owner: none for author rework.
Validation:
- Freshness/identity:
gh api user --jq .loginreturnedjedwards27; live PR author isloganj. Immediately before this review,gh pr view 8660 --repo block/buzz --json baseRefOid,headRefOid,author,statusCheckRollup,reviewsconfirmed base0a362eef150ab16453993d442a8037baad766797, headb8e172672525d7eb4fbb2834bf4caa23686ed926, no priorjedwards27terminal review at this head, and no failing/pending non-skipped checks in the returned Desktop-relevant rollup. - Local static/diff proof at exact head:
git diff --check 0a362eef150ab16453993d442a8037baad766797...HEADpassed;git rev-parse HEADreturnedb8e172672525d7eb4fbb2834bf4caa23686ed926; working tree clean. - Local Desktop JS/package gates at exact head:
cd desktop && pnpm testpassed (6838 + 161, 0 failures);cd desktop && pnpm checkcompleted successfully with 3 unrelated repo-wide warnings before passing px-text/pubkey-truncation;cd desktop && pnpm typecheckpassed. - Focused regression tests at exact head:
cd desktop && node --import ./test-loader.mjs --experimental-strip-types --test src/features/messages/lib/virtualizedTimelineItems.test.mjs src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjspassed23/23. - Targeted E2E at exact head:
cd desktop && CI=1 pnpm build:e2e && CI=1 pnpm exec playwright test --project=smoke message-target-highlight.spec.tspassed5/5; the equivalent smoke commandCI=1 pnpm test:e2e:smoke -- message-target-highlight.spec.tsalso passed5/5. - Mutation proof: removing the accepted-target
highlightMessage(messageId)call causedmessage target flashes: same channel, row loaded off screento fail as expected; restored afterward and final tree was clean.
Manual/native evidence: Browser Playwright exercised the five target journeys: same-channel on-screen, same-channel loaded/off-screen, same-channel unloaded older history, cross-channel near-bottom, and cross-channel further-up. I did not run the native Desktop visual-recording harness.
Residual risk: Native WKWebView/Desktop visual recording remains a reviewer/tooling confidence gap, not an author action: source trace, targeted E2E, mutation proof, local gates, and green macOS/Windows/Desktop CI cover the changed contract at this head.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Post-merge review of b8e172672 (merged as 5ea5ae172). No blockers. The fix holds up: every accepted virtual target goes through scrollToIndex, and the flash is keyed by id. We found no path that re-pins away from an accepted target, and the head-trim suffix check only lets through exact surviving suffixes. This was a source-only review by four reviewers. Nobody ran it locally or in the native app. CI was green.
There are two optional follow-ups, each in its own inline thread. Neither one regresses main.
| virtualCancelBottomIntent?.(); | ||
| cancelMountBottomPin(); | ||
| anchorRef.current = { kind: "message", messageId, topOffset: 0 }; | ||
| virtualizerAtBottomRef.current = false; |
There was a problem hiding this comment.
🤖 Optional, extends the follow-up already listed in the PR body. Clearing virtualizerAtBottomRef is new; main cleared only isAtBottom. scrollToBottomImperative (around L366-392) never sets the ref back to true. Say a jump targets the last row, so centering clamps and no scroll event fires. The user then clicks "Jump to latest" and that is clamped too. Now isAtBottom is true but the ref is still false. Live appends stop following, and the pill stays hidden until the user really scrolls. That takes three no-movement steps in a row, so it's rare. Suggested fix, in the same follow-up: set the ref back to true in scrollToBottomImperative, or derive it from Virtua's geometry instead of writing false here.
| previousKeys.every((key, index) => key === keys[index + prependedCount]) | ||
| delta < 0 && | ||
| keys.length > 0 && | ||
| keys.every((key, index) => key === previousKeys[index - delta]) |
There was a problem hiding this comment.
🤖 Optional, pre-existing scope gap. leading-content is always key 0 when an intro is present. selectTimelineIntroSurface returns the DM intro whatever hasReachedChannelStart is (timelineSnapshot.ts:373), and huddle transcripts always pass pinnedIntro (ChannelPane.tsx:640). In DMs and huddles, [leading, a, b, c, spacer] → [leading, b, c, spacer] fails this suffix check, so the head-trim fix doesn't reach those surfaces. The existing prepend check fails the same way there on main. A follow-up could compare the stream with a stable leading-content head pinned on both sides.
…9094) ## Summary Re-entering a channel trims older pages from the head of the cached timeline, and our Virtua patch compensates for each trim from the live DOM `scrollTop`. When the reader was near the floor, the browser had already clamped `scrollTop` to the shorter list, so the removal was counted twice and the reader could be thrown to the top of history. That is the `message-target-highlight.spec.ts` flake in Desktop Smoke E2E. The patched scroller now remembers the last offset it observed or wrote, and compensates from that offset when the live one was clamped to the floor. ### 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 failed (60/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. **Rebased onto origin/main `64d12e297` (this branch):** Linux workstation. - `pnpm install --frozen-lockfile` installs virtua with the new patch hash (`f4df71c39f6c...`). - `pnpm typecheck` and `biome check --error-on-warnings` on the new jsdom test are clean. - Unit tests under `src/features/messages` and `src/features/channels`: 1990/1990. jsdom tests there: 12/12. Full desktop `pnpm test`: 6933 unit and 176 jsdom tests passed. - Playwright smoke after `pnpm build:e2e`, `--workers=4 --retries=0`: messaging, send-channel-binding, scroll-history, virtualization, timeline-no-shift, overscroll-boundary, live-broadcast-reply-timeline and message-target-highlight, 137 passed and 1 failed. The failure is `messaging.spec.ts:2785` (own-message avatar), which fails the same way on main here, as noted above. `dm-history-live-regression.spec.ts` isn't in the smoke project, so this run left it out. - `message-target-highlight.spec.ts`, `--repeat-each=10`: 50/50 passed. "other channel, row near the bottom", `--repeat-each=30`: 30/30 passed. ## Coordination No open PR edits `patches/[email protected]`. The overlaps are all in `pnpm-lock.yaml`: - #5547 ("fix(desktop): ship a bundle that parses on WebKit without lookbehind (Intel/macOS 12)") and #7074 ("chore(deps): update react monorepo (patch)") merge cleanly with main but conflict with this branch on the `[email protected]` patch-hash lines. #5547 adds a patched dependency next to the virtua entry, and #7074 changes the React peer versions in the same lines. Whichever lands second resolves it by taking both sides' package changes and re-running `pnpm install` so the lockfile records `patch_hash=f4df71c39f6c...` for virtua. - #7073, #7080, #7633, #8218 and #8643 also touch `pnpm-lock.yaml` and merge cleanly with this branch. Older open PRs that touch it already conflict with main, and none gains a new conflict from this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: mtoohey <[email protected]> Co-authored-by: Claude Opus 5.5 <[email protected]>


Problem
When you follow a link to a message, the channel timeline should scroll to it and flash the row for about 2 seconds. On
mainthe flash often does not appear:mainThe main timeline is always virtualized.
useAnchoredScrollhad three ways to scroll to a target there. Only one of them, "the row is already fully on screen", fired the flash. In the other cases the scroll returned without the flash, and the follow-up target effect marked the target handled without checking the result, so the flash never fired.Opening a link from another channel had a second problem. The row could look on screen while the channel's first bottom pin was still in flight in Virtua. The shortcut used a direct DOM
scrollTo, which does not cancel that pin, so Virtua kept going to the bottom and took the target with it. Top-level message links also open the thread panel, and that resize settled the timeline at the bottom again.A third problem also exists on
main. Centering a row near the top of the loaded history loads an older page. Opening a channel also starts a refresh of the newest channel window, and that refresh then drops the older pages from the head of the list. A head trim was not passed to Virtua as ashift, so the rows above the reader vanished with no compensation and the view fell to the bottom about 0.3–0.8 s after the flash started.The deep-link e2e test stopped checking the flash in #5306, so CI did not catch this.
Change
scrollToIndex(centered). That replaces any bottom pin still in flight. Smooth scrolling is kept only when the row is already in the viewport.trueonce Virtua accepts the target andfalsewhile the target is not loaded yet. The follow-up target effect and the search retry now just read that value. This removes the "phase 1" special case and the duplicate DOM lookup.didPrependVirtualizedTimelinenow also admits a pure head trim (the new keys are an exact suffix of the previous keys), so Virtua'sshiftkeeps the reading row in place when a window refresh drops older pages. The oldest loaded day has no divider while more history exists, so a head trim stays an exact suffix even when it cuts inside a day.The non-virtualized path (thread panel) is unchanged.
Screenshots
Before and after, from another channel, for a link to a row further up: #8660 (comment)
Testing
Evidence at head
653d89766:desktop/tests/e2e/message-target-highlight.spec.ts: a 5-row table with the cases above. Each case asserts that the target row is on screen and has the flash class at the same time, and that the row is still on screen 2.2 s later, after the flash ends. 15/15 passed (3 runs each).main, 3 of the 5 cases fail the flash check.useAnchoredScroll.lifecycle.test.mjs: an on-screen virtual target scrolls through the virtualizer, makes no direct DOM write, retires bottom intent and does not re-pin later. New: a target that is not loaded yet keeps the bottom pin. It fails with the old cancel order.virtualizedTimelineItems.test.mjs: new rows for a head trim inside the oldest day and at its boundary (admitted, with the size cache aligned), and for a trim plus append and a middle removal (rejected).pnpm test(6837 + 161 pass),pnpm check(tsc via pre-push, biome, px-text, pubkey truncation).Later commits at
b8e172672add no production behavior. They move one helper abovecomputeAnchor's doc comment, and add a lifecycle test (from Cerveau's probe): a target that loads before the queued second mount frame is not scrolled back to the bottom. That test fails without the mount-frame cancellation. Pre-pushpnpm testpassed atb8e172672.Adversarial review by Auto (virtualizer and callers) and Cerveau (lifecycle, tests and thread panel). Their must-fix findings are fixed above, and both approved
653d89766.Follow-up, not in this PR: after a jump to a row at the very bottom, centering is clamped, so no scroll event fires and the at-bottom state stays false until the next scroll.
mainhas the same gap forisAtBottom.Not tested yet in the native app.
🤖 Generated with Claude Code