Skip to content

fix(desktop): always flash a linked message in the channel timeline - #8660

Merged
loganj merged 4 commits into
mainfrom
larry/message-target-flash
Oct 29, 2026
Merged

loganj merged 4 commits into
mainfrom
larry/message-target-flash

Conversation

@loganj

@loganj loganj commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

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 main the flash often does not appear:

How you reach the message Flash on main
Same channel, row already on screen yes
Same channel, row loaded but off screen no
Same channel, row in older history not loaded yet yes
From another channel, row near the bottom no
From another channel, row further up no, and the timeline stays at the bottom

The main timeline is always virtualized. useAnchoredScroll had 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 a shift, 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

  • In virtualized mode, every target scroll now goes through Virtua's scrollToIndex (centered). That replaces any bottom pin still in flight. Smooth scrolling is kept only when the row is already in the viewport.
  • Every target scroll that Virtua accepts now fires the flash. The highlight is keyed by message id, so a row that Virtua mounts after the scroll still shows it.
  • The scroll returns true once Virtua accepts the target and false while 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.
  • A target scroll also clears the virtualizer's at-bottom state, so a resize right after it (for example the thread panel opening) does not pin back to the bottom.
  • Only an accepted target scroll retires bottom intent. It also cancels the hook's queued second mount-to-bottom frame. A target that is not loaded yet leaves the opening bottom pin alone while it retries.
  • didPrependVirtualizedTimeline now also admits a pure head trim (the new keys are an exact suffix of the previous keys), so Virtua's shift keeps 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:

  • New 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).
    • On unfixed main, 3 of the 5 cases fail the flash check.
    • With only the head-trim change reverted, "same channel, loaded off screen" and "other channel, further up" fail the 2.2 s 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).
  • These e2e specs, run together: message-target-highlight, scroll-history, virtualization, timeline-no-shift, unread-pill, overscroll-boundary, dm-history-live-regression, live-broadcast-reply-timeline, message-copy-link, thread-focus-mode, thread-follow, thread-reply-anchor-roleplay, thread-unread, channels, messaging. 257 passed, 0 failed.

Later commits at b8e172672 add no production behavior. They move one helper above computeAnchor'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-push pnpm test passed at b8e172672.

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. main has the same gap for isAtBottom.

Not tested yet in the native app.

🤖 Generated with Claude Code

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]>
loganj added a commit that referenced this pull request Oct 28, 2026
@loganj

loganj commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Before (main)

From another channel, a link to a row further up in #general: the thread panel opens, the timeline stays at the bottom, and there is no flash.

01-before

After

The same link: the timeline centers the row and flashes it.

02-after

loganj and others added 3 commits October 28, 2026 18:34
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]>
@loganj
loganj marked this pull request as ready for review October 29, 2026 00:40
@loganj
loganj requested a review from a team as a code owner October 29, 2026 00:40
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 29, 2026
@github-actions

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 974495fd4a5d77310efd9de45a6f4339e881446a...b8e172672525d7eb4fbb2834bf4caa23686ed926
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability regressions found in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source and history inspection. Builds and tests were not executed.

Generated by Codex Security Review |
Requested by: @loganj |
Workflow run

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 :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 false and keep retrying without clearing the route or falsely flashing (desktop/src/features/messages/ui/useAnchoredScroll.ts:453-480, :634-649, :897-909).
  • TimelineMessageList maps every timeline item message id to its Virtua row index and centers via scrollToIndex(..., { 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 shift when 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 at desktop/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 .login returned jedwards27; live PR author is loganj. Immediately before this review, gh pr view 8660 --repo block/buzz --json baseRefOid,headRefOid,author,statusCheckRollup,reviews confirmed base 0a362eef150ab16453993d442a8037baad766797, head b8e172672525d7eb4fbb2834bf4caa23686ed926, no prior jedwards27 terminal 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...HEAD passed; git rev-parse HEAD returned b8e172672525d7eb4fbb2834bf4caa23686ed926; working tree clean.
  • Local Desktop JS/package gates at exact head: cd desktop && pnpm test passed (6838 + 161, 0 failures); cd desktop && pnpm check completed successfully with 3 unrelated repo-wide warnings before passing px-text/pubkey-truncation; cd desktop && pnpm typecheck passed.
  • 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.mjs passed 23/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.ts passed 5/5; the equivalent smoke command CI=1 pnpm test:e2e:smoke -- message-target-highlight.spec.ts also passed 5/5.
  • Mutation proof: removing the accepted-target highlightMessage(messageId) call caused message target flashes: same channel, row loaded off screen to 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.

@loganj
loganj merged commit 5ea5ae1 into main Oct 29, 2026
74 checks passed
@loganj
loganj deleted the larry/message-target-flash branch October 29, 2026 16:11

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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.

matt2e added a commit that referenced this pull request Nov 5, 2026
…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]>

This branch was previously deployed

1 inactive deployment
codex-review — b8e17267 Deployed Oct 29, 2026 by loganj via Run Codex Security Review #10373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants