Skip to content

fix(desktop): keep an own send out of the frozen timeline tail - #9095

Merged
matt2e merged 4 commits into
mainfrom
mtoohey/flaky-own-send-timeline
Nov 5, 2026
Merged

matt2e merged 4 commits into
mainfrom
mtoohey/flaky-own-send-timeline

Conversation

@matt2e

@matt2e matt2e commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The timeline freezes its live tail when Virtua reports that the reader has left the bottom, and a send arms a one-shot pin for its own row. A not-at-bottom report between the send and the sent row reaching the deferred snapshot froze the tail again, so the sent row waited behind a "1 new message" pill. That is the messaging.spec.ts:483 flake in Desktop Smoke E2E. The first commit holds off that freeze while the send is pending. The other three are follow-ups from pre-PR review that bound the hold to the send itself: it ends when the row lands, it is disarmed when the send fails, and it is armed only for the channel the send belongs to.

fix(desktop): keep an own send out of the frozen timeline tail

Problem

messaging.spec.ts:483 "long autolink wraps without widening the timeline" flaked in Desktop Smoke E2E at :497: the timeline never showed the sent message. The composer was empty, and a "1 new message" pill sat at the bottom of a timeline that was already at the floor.

The failure video shows the composer filling while the channel still showed its skeleton, the rows rendering under the tall composer, and the send landing about 120 ms later.

Cause

The timeline freezes its live tail when Virtua reports that the reader has left the bottom, and later rows wait behind the pill. A send lifts the freeze and arms a one-shot pin for the next append (prepareForOwnMessage). Nothing kept a not-at-bottom report between the send and the sent row reaching the deferred snapshot from freezing the tail again. The sent row then waited behind the pill, and the pin, which waits for that append, never fired. Only a later scroll to the floor would have released it.

Virtua 0.49.3 makes that report while the reader sits at the floor. It applies a list length change to its store during render (Virtualizer.tsx). React discarded the deferred render that added the sent row, but the store kept the row's estimated height. When the cleared composer shrank the trailing spacer, the next scroll event put the reader 96 px from a floor the DOM didn't have: Virtua read 941 - 479 - 366 = 96, the DOM read 917 - 551 - 366 = 0. The freeze it queued renders at default priority, ahead of the deferred render, which then restarted against the frozen tail.

Fix

While the send's pin is armed, a not-at-bottom report doesn't freeze the tail. useAnchoredScroll exposes isBottomPinArmed(), which reads the pin's own flag, so the hold ends when the pin fires or the channel changes. prepareForOwnMessage also cancels a freeze queued before the send, which would otherwise land after its release.

Bottom state still comes from Virtua. Reading it from the DOM would add false gaps of its own, because unmeasured rows overflow their slots: a passing run read 51 px from the DOM where Virtua read 0.

A scroll up between the send and its row no longer freezes the tail. The pin brings the reader down to the sent row, as it already does when the row lands first. A send that adds no row leaves the pin armed until the next append, as before, and the hold follows it.

Validation

  • New MessageTimeline.ownSend.jsdom-test.mjs renders the real MessageTimeline over Virtua with a modelled layout and manual frames. It sends with a gap reported after the send, and with a freeze already queued at the send. The sent row must render at the floor with no pill. Without the pin guard the first case fails, and without the cancel the second does, each showing "1 new message".
  • Repro, using temporary, uncommitted changes: the test with a 600 ms channel-head delay, filling during the skeleton, sending once rows render, with CPU throttled 4x over CDP. Without the fix: 3/20 failed with the CI signature, and instrumentation showed the 96 px report above. With the fix: 40/40 passed, and 6 of those runs logged a not-at-bottom report while the pin was armed.
  • messaging.spec.ts --grep "long autolink": 30/30 passed at 4 workers, 10/10 serially.
  • messaging, virtualization, scroll-history, stream, smoke, thread-follow, file-attachment and message-target-highlight specs, run twice at 4 workers: 185 passed and 1 failed each time, a different test each run. Repeating those two fails as often without the fix: :2785 (own avatar) 22/25 vs 20/25 at 4 workers and 8/10 vs 9/10 serially; :3170 (thread panel width) 4/25 vs 8/25 and 1/10 vs 3/10. :2785 fails on the avatar image after its message has rendered, and :3170 never sends, so the change can't reach it.
  • tsc --noEmit, biome check, and the messages UI unit (337) and jsdom (9) tests pass.

A green run doesn't prove the flake is gone. Hosted CI covers broad validation.

fix(desktop): end an own send's freeze hold when its row lands

Problem

Review of f2c57af ("fix(desktop): keep an own send out of the frozen timeline tail") raised two findings in MessageTimeline.tsx.

  • Warning: the hold on freezing the tail lasted as long as the send's bottom pin, not as long as the send. The pin fires only when the tail id changes (newLatestArrived in useAnchoredScroll), and only firing or a channel change cleared it. The window cache sorts rows by created_at, so the sent row doesn't become the tail when the last row has a later created_at (a sender whose clock runs ahead) or a same-second row sorts after it. The pin then stayed armed after the sent row rendered, and a reader who scrolled up never froze the tail. The next arrival to become the tail fired the pin and pulled them to the bottom instead of showing the pill. Before f2c57af the freeze buffered that arrival, so a stale pin could not fire under a reader.
  • Nit: prepareForOwnMessage set semanticAtBottomRef.current = true, which the hold already made redundant, and cancelling the queued freeze left behind the suppressNextSemanticBottomRef it had set.

Cause

The hold read the bottom pin's own flag. The pin is cleared only when it fires on a new tail id (newLatestArrived in useAnchoredScroll) or when the channel changes. The window cache sorts rows by created_at, so a sent row that sorts above the current tail never becomes the tail. That happens when the last row's sender has a clock running ahead, or when a same-second row sorts after the sent one. The pin then stays armed after the sent row renders, and the hold stays with it.

Fix

useAnchoredScroll keeps a pending-send flag next to the pin. scrollToBottomOnNextUpdate sets both. The restoration effect clears the flag on the first append after arming, whether or not it moves the tail (newLatestArrived or an "append" delta), and the channel reset clears it. isBottomPinArmed becomes isOwnSendPending, which reads the flag, and MessageTimeline holds off freezing only while it is set. The pin itself is unchanged and still waits for a new tail. Once the hold ends, a reader who scrolls up freezes the tail as before, so later arrivals wait behind the pill and a stale pin can't fire under them.

This keeps arming and disarming in the hook. Disarming from ChannelPane after onSendMessage settles would need a second call site (the welcome add-agent flow also arms the pin), and the send's promise isn't tied to when the sent row reaches the deferred snapshot.

prepareForOwnMessage now clears suppressNextSemanticBottomRef instead of setting the semantic ref, so dropping the queued freeze also drops the suppression it armed. No test covers this: a leftover suppression only swallowed a bottom report while the send's release was already making the timeline semantically at the bottom.

Validation

  • New case in MessageTimeline.ownSend.jsdom-test.mjs: the tail's createdAt is an hour ahead, the send lands above it, the reader scrolls up 200 px, and a row from the same fast clock arrives and becomes the tail. It expects "1 new message" and the reader's offset unchanged. On f2c57af it fails: no pill, and the pin moved the reader from 756 to the floor at 1052 and rendered the arrival. With the fix the reader stays at 756 and the arrival waits behind the pill.
  • Mutations: clearing the flag only on a new tail, or never on an append, fails only the new case. Removing the hold check fails the original "gap after the send" case, as before.
  • Messages UI jsdom tests (10) and src/features/messages unit tests (1530) pass. Full desktop pnpm test: 6933 unit and 179 jsdom tests pass.
  • tsc --noEmit, biome check on the changed files, check:file-sizes and git diff --check are clean. No browser E2E was run for this follow-up.

fix(desktop): disarm an own send's pin and hold when the send fails

Problem

Second review of the branch (warning, regression vs main) on c176734 ("fix(desktop): end an own send's freeze hold when its row lands"), useAnchoredScroll.ts and ChannelPane.tsx. The hold can still go stale when a send never adds a row to this timeline.

ChannelPane.handleSendMessage armed the pin and the hold before it awaited onSendMessage, for every main-composer send. That includes a DM send that useMentionSendFlow's onPrepareSendChannel moves to another DM (the message mentions an agent who isn't in this one). Its optimistic row goes to that channel's window, and goChannel runs only after the send succeeds. If the publish is rejected or times out, the user stays on the original DM with both flags armed. A same-channel send whose onMutate can't resolve the channel also adds no row. Then the reader scrolls up, the hold skips the freeze, the next arrival from the other person becomes the tail, and the stale pin moves the reader to the bottom with no pill. The reviewer's jsdom repro: at HEAD, no pill and scrollTop 660 -> 956. At the merge base, "1 new message" and 660 stays.

A failed send to the same channel was fine: its optimistic row lands in onMutate, an append that clears the hold, and is then reverted.

Cause

ChannelPane.handleSendMessage armed the pin and the hold before it awaited onSendMessage, for every main-composer send, and nothing disarmed them if the send then added no row to this timeline. A send that onPrepareSendChannel moves to another DM lands its optimistic row in that channel's window. A same-channel send whose onMutate can't resolve the channel adds no row either. When such a send fails, the user stays on the original timeline with both flags still armed.

Fix

  • ChannelPane.handleSendMessage arms only when the send targets this channel (!channelId || channelId === activeChannelId). A send prepared into another channel lands its row there, and the pane navigates to it once the send succeeds.
  • MessageTimelineHandle.cancelScrollToBottomOnNextUpdate disarms a send. useAnchoredScroll clears the pin and the pending-send flag. The hold skipped every freeze while the send was pending, including one queued before the send that the arm cancelled. So if the send was still pending and Virtua's last report has the reader off the floor, the cancel queues that freeze, as the report would have without the hold. Later arrivals then wait behind the pill. If the send's row already landed, the hold had ended and the cancel only clears the pin.
  • ChannelPane arms through armOwnSend, which returns the disarm. The main composer calls it when onSendMessage rejects, and the Welcome "create in chat" flow when its mutateAsync rejects. Its beforeSend option now returns the disarm. A generation counter keeps a late failure from disarming a pin that a later send armed.

Validation

  • MessageTimeline.ownSend.jsdom-test.mjs adds the reviewer's no-row case in three orders: fail before the reader scrolls up, after, and with the freeze queued before the send. Each expects "1 new message" and the reader's offset unchanged after a new tail arrives. With the cancel a no-op (HEAD's behavior), all three fail: no pill, and the reader moves from 660 to 956. With the cancel clearing the flags but not queueing the freeze, the two orders where the reader left the floor during the send fail with "Jump to latest". A fourth case, a same-channel send whose row lands and is then reverted, still shows "1 new message" and keeps the offset. It passes on HEAD as well, because it pins behavior that must not change. 7/7 pass.
  • useAnchoredScroll.lifecycle.test.mjs adds a hook case: arm, cancel, the reader leaves the floor, a new tail arrives without a jump, and a later send still pins. It fails if the cancel is a no-op, or if it clears only the pending flag. useAnchoredScroll unit tests: 45/45.
  • Full desktop pnpm test: 6934 unit and 183 jsdom tests pass.
  • tsc --noEmit, biome check --error-on-warnings on the changed files and git diff --check are clean.
  • No test renders ChannelPane, so its arm condition, the rejection handlers and the generation counter are covered only by tsc and review. No browser E2E was run.

fix(desktop): arm an own send's pin only for the channel it belongs to

Problem

Third review of the branch (review 89cce157, warning) on c257ff1 ("fix(desktop): disarm an own send's pin and hold when the send fails"), ChannelPane.tsx. handleSendMessage armed the timeline when !channelId || channelId === activeChannelId, and activeChannelId was the value captured in its closure. A send whose slow upload spans a channel switch passes that check against the channel it was written in, then arms messageTimelineRef.current, the timeline now showing the other channel. That channel keeps its tail open and its pin armed until its next append, because the sent row lands in the original channel's window. A reader who scrolls up there doesn't freeze the tail, and the next arrival fires the pin and pulls them to the bottom with no pill.

Cause

useMentionSendFlow reads onSendRef.current before it starts a deferred upload and calls that send from the upload's onComplete. The arm carried no channel, so only the stale closure decided whether to arm, and the timeline accepted an arm for whatever channel it rendered.

Fix

  • MessageTimelineHandle.scrollToBottomOnNextUpdate(sendChannelId) takes the send's channel. useAnchoredScroll keeps the channel its state belongs to in a ref that its channel reset updates, and its arm returns false without arming for any other channel. MessageTimeline releases the frozen tail only when the arm is accepted, so a stale send neither releases nor pins the channel shown now.
  • The arm and disarm move from ChannelPane into useOwnSendArm(timelineRef, getTimelineChannelId). It skips an arm whose channel isn't the one getTimelineChannelId reads now (ChannelPane passes its activeChannelIdRef) and returns a no-op disarm. A skipped arm leaves the generation alone, so it can't stop a pending send in the shown channel from disarming when it fails. Otherwise the generation works as before: a later send's arm turns an earlier send's disarm into a no-op.
  • handleSendMessage arms channelId ?? activeChannelId. A send prepared into another DM is still skipped, now by the hook's live check instead of the closure.
  • The Welcome "create in chat" flow names the channel it sends to: beforeSend takes a channelId, and useWelcomeAgentCreate passes activeChannel.id.
  • useAnchoredScroll exposes isVirtualizerAtBottom(), and MessageTimeline's cancel reads it instead of keeping its own copy of Virtua's last report. The two agree once the channel has confirmed its bottom, which the cancel already requires. The hook's copy is also cleared by a target jump or a live row under a held target before Virtua reports, so a cancel in that window freezes as the report would.
  • The channel reset effect now reads channelId, so its exhaustive-deps suppression goes.

"a cancelled own send leaves the next tail with the reader" in useAnchoredScroll.lifecycle.test.mjs (from c257ff1) failed 3 of 6 runs at HEAD on this workstation. The mount's settling frame, a setTimeout(0) in that file's shim, could run after the test cleared its recorded pins and add a "jump". The test now waits for that frame first, as other cases in the file do: 0 of 15 runs failed. It still fails when the cancel is a no-op or clears only the pending flag.

Validation

  • New useOwnSendArm.test.mjs (node:test, like the rest of desktop's unit tests): a send to the shown channel arms it, a send that settles after a switch isn't armed and leaves the pending send's disarm working, a failed send disarms, and a later arm outlives an earlier failure. Each mutation fails a case: dropping the live check, bumping the generation before it, dropping the generation check, and a no-op disarm.
  • useAnchoredScroll.lifecycle.test.mjs adds an 11-row table for the two gates the cancel reads, isOwnSendPending() and isVirtualizerAtBottom(): no send, a send whose row hasn't landed, landed as the tail, landed above the tail, a send to another channel, a send then a channel change, a channel change then a send there, and the reader off the floor, back at it, after a channel change, and with a send pending. Dropping the arm's channel check, not updating the channel ref on reset, and a constant isVirtualizerAtBottom each fail rows.
  • MessageTimeline.ownSend.jsdom-test.mjs, 9/9:
    • New: with the reader scrolled up and an arrival behind the pill, an arm for another channel and a second arrival leave "2 new messages" and the offset unchanged. It fails if useAnchoredScroll accepts the arm, or if MessageTimeline releases the tail when it is refused.
    • New: a send that fails at the floor leaves the next arrival rendered at once. Without the off-floor gate the cancel freezes and the arrival is missing right after it renders.
    • Extended: after a same-channel send lands and fails, the reader returns to the floor and the arrival renders with the pill gone. Without the pending gate the cancel re-arms the freeze's bottom-report suppression and the arrival stays behind the pill.
    • The existing cases now pass the channel.
  • Desktop unit tests under src/features/messages and src/features/channels: 2006/2006. jsdom tests there: 21/21.
  • Playwright smoke after pnpm build:e2e, --workers=4: messaging, send-channel-binding and welcome-agent-modal-screenshots, run twice. First run 99 passed and 1 failed, second run 98 passed and 2 failed:
    • messaging.spec.ts:2785 (own avatar) fails on the avatar image after its message renders: 5/6 failed with this change and 6/6 at HEAD (--workers=2 --repeat-each=6). f2c57af recorded the same local failure.
    • messaging.spec.ts:3170 (thread panel width) never sends. It passed 5/5 on rerun.
  • send-channel-binding.spec.ts, --repeat-each=5: 10/10. Its mid-send case switches channels while a send is in flight. welcome-agent-modal-screenshots (its chat-first case clicks "create in chat" and finds the message in the timeline) plus nine messaging cases including "long autolink" and the channel-switch cases, --repeat-each=5: 80/80.
  • tsc --noEmit, biome check --error-on-warnings on the changed files, check:file-sizes and git diff --check are clean.
  • No test renders ChannelPane, so its getter and its armOwnSend(channelId ?? activeChannelId) call are covered only by tsc, the hook's tests and the specs above.

Rebased onto origin/main 64d12e297 (this branch, all four commits): Linux workstation. This branch doesn't include the Virtua patch change from #9094. pnpm install --frozen-lockfile installs main's virtua patch (30a7a92b9480...), so every result below runs against the Virtua build on main.

  • pnpm typecheck and biome check --error-on-warnings on the 11 changed desktop files are clean.
  • Unit tests under src/features/messages and src/features/channels: 2006/2006. jsdom tests there: 16/16, including all 9 MessageTimeline.ownSend.jsdom-test.mjs cases. Full desktop pnpm test: 6949 unit and 180 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, message-target-highlight and welcome-agent-modal-screenshots, 142 passed and 2 failed. The failures are messaging.spec.ts:2785 (own-message avatar), which fails the same way on main here as noted above, and messaging.spec.ts:3170 (thread panel width), which never sends and passed 5/5 on rerun.
  • messaging.spec.ts --grep "long autolink", --repeat-each=30: 30/30 passed. send-channel-binding.spec.ts, --repeat-each=5: 10/10 passed.

Coordination

🤖 Generated with Claude Code

matt2e and others added 4 commits November 5, 2026 03:27
Problem:
`messaging.spec.ts:483` "long autolink wraps without widening the
timeline" flaked in Desktop Smoke E2E at :497: the timeline never
showed the sent message. The composer was empty, and a "1 new message"
pill sat at the bottom of a timeline that was already at the floor.
- seiler/BUZZ-517/desktop-upgrade-guidance:
  https://github.com/block/buzz/actions/runs/41751914119/job/126319694104
  (retry 1 passed)
The failure video shows the composer filling while the channel still
showed its skeleton, the rows rendering under the tall composer, and
the send landing about 120 ms later.

Cause:
The timeline freezes its live tail when Virtua reports that the reader
has left the bottom, and later rows wait behind the pill. A send lifts
the freeze and arms a one-shot pin for the next append
(`prepareForOwnMessage`). Nothing kept a not-at-bottom report between
the send and the sent row reaching the deferred snapshot from freezing
the tail again. The sent row then waited behind the pill, and the pin,
which waits for that append, never fired. Only a later scroll to the
floor would have released it.

Virtua 0.49.3 makes that report while the reader sits at the floor. It
applies a list length change to its store during render
(Virtualizer.tsx). React discarded the deferred render that added the
sent row, but the store kept the row's estimated height. When the
cleared composer shrank the trailing spacer, the next scroll event put
the reader 96 px from a floor the DOM didn't have: Virtua read
941 - 479 - 366 = 96, the DOM read 917 - 551 - 366 = 0. The freeze it
queued renders at default priority, ahead of the deferred render, which
then restarted against the frozen tail.

Fix:
While the send's pin is armed, a not-at-bottom report doesn't freeze
the tail. `useAnchoredScroll` exposes `isBottomPinArmed()`, which reads
the pin's own flag, so the hold ends when the pin fires or the channel
changes. `prepareForOwnMessage` also cancels a freeze queued before the
send, which would otherwise land after its release.

Bottom state still comes from Virtua. Reading it from the DOM would add
false gaps of its own, because unmeasured rows overflow their slots: a
passing run read 51 px from the DOM where Virtua read 0.

A scroll up between the send and its row no longer freezes the tail.
The pin brings the reader down to the sent row, as it already does when
the row lands first. A send that adds no row leaves the pin armed until
the next append, as before, and the hold follows it.

Validation:
- New `MessageTimeline.ownSend.jsdom-test.mjs` renders the real
  MessageTimeline over Virtua with a modelled layout and manual frames.
  It sends with a gap reported after the send, and with a freeze
  already queued at the send. The sent row must render at the floor
  with no pill. Without the pin guard the first case fails, and
  without the cancel the second does, each showing "1 new message".
- Repro, using temporary, uncommitted changes: the test with a 600 ms
  channel-head delay, filling during the skeleton, sending once rows
  render, with CPU throttled 4x over CDP. Without the fix: 3/20 failed
  with the CI signature, and instrumentation showed the 96 px report
  above. With the fix: 40/40 passed, and 6 of those runs logged a
  not-at-bottom report while the pin was armed.
- `messaging.spec.ts --grep "long autolink"`: 30/30 passed at 4
  workers, 10/10 serially.
- messaging, virtualization, scroll-history, stream, smoke,
  thread-follow, file-attachment and message-target-highlight specs,
  run twice at 4 workers: 185 passed and 1 failed each time, a
  different test each run. Repeating those two fails as often without
  the fix: `:2785` (own avatar) 22/25 vs 20/25 at 4 workers and 8/10
  vs 9/10 serially; `:3170` (thread panel width) 4/25 vs 8/25 and 1/10
  vs 3/10. `:2785` fails on the avatar image after its message has
  rendered, and `:3170` never sends, so the change can't reach it.
- `tsc --noEmit`, `biome check`, and the messages UI unit (337) and
  jsdom (9) tests pass.

A green run doesn't prove the flake is gone. Hosted CI covers broad
validation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
(cherry picked from commit 4c9272a362eb6f01aacb8d0133fbe41066eaf308)
Problem:
Review of 3c47951dc ("fix(desktop): keep an own send out of the frozen
timeline tail") raised two findings in MessageTimeline.tsx.

- Warning: the hold on freezing the tail lasted as long as the send's
  bottom pin, not as long as the send. The pin fires only when the tail
  id changes (`newLatestArrived` in useAnchoredScroll), and only firing
  or a channel change cleared it. The window cache sorts rows by
  created_at, so the sent row doesn't become the tail when the last row
  has a later created_at (a sender whose clock runs ahead) or a
  same-second row sorts after it. The pin then stayed armed after the
  sent row rendered, and a reader who scrolled up never froze the tail.
  The next arrival to become the tail fired the pin and pulled them to
  the bottom instead of showing the pill. Before 3c47951dc the freeze
  buffered that arrival, so a stale pin could not fire under a reader.
- Nit: `prepareForOwnMessage` set `semanticAtBottomRef.current = true`,
  which the hold already made redundant, and cancelling the queued
  freeze left behind the `suppressNextSemanticBottomRef` it had set.

Fix:
`useAnchoredScroll` keeps a pending-send flag next to the pin.
`scrollToBottomOnNextUpdate` sets both. The restoration effect clears
the flag on the first append after arming, whether or not it moves the
tail (`newLatestArrived` or an "append" delta), and the channel reset
clears it. `isBottomPinArmed` becomes `isOwnSendPending`, which reads
the flag, and MessageTimeline holds off freezing only while it is set.
The pin itself is unchanged and still waits for a new tail. Once the
hold ends, a reader who scrolls up freezes the tail as before, so later
arrivals wait behind the pill and a stale pin can't fire under them.

This keeps arming and disarming in the hook. Disarming from ChannelPane
after `onSendMessage` settles would need a second call site (the
welcome add-agent flow also arms the pin), and the send's promise isn't
tied to when the sent row reaches the deferred snapshot.

`prepareForOwnMessage` now clears `suppressNextSemanticBottomRef`
instead of setting the semantic ref, so dropping the queued freeze also
drops the suppression it armed. No test covers this: a leftover
suppression only swallowed a bottom report while the send's release
was already making the timeline semantically at the bottom.

Validation:
- New case in `MessageTimeline.ownSend.jsdom-test.mjs`: the tail's
  createdAt is an hour ahead, the send lands above it, the reader
  scrolls up 200 px, and a row from the same fast clock arrives and
  becomes the tail. It expects "1 new message" and the reader's offset
  unchanged. On 3c47951dc it fails: no pill, and the pin moved the
  reader from 756 to the floor at 1052 and rendered the arrival. With
  the fix the reader stays at 756 and the arrival waits behind the pill.
- Mutations: clearing the flag only on a new tail, or never on an
  append, fails only the new case. Removing the hold check fails the
  original "gap after the send" case, as before.
- Messages UI jsdom tests (10) and `src/features/messages` unit tests
  (1530) pass. Full desktop `pnpm test`: 6933 unit and 179 jsdom tests
  pass.
- `tsc --noEmit`, `biome check` on the changed files,
  `check:file-sizes` and `git diff --check` are clean. No browser E2E
  was run for this follow-up.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
(cherry picked from commit a2a9767de1b4f01879c652b51e9c10e508678aa6)
Problem:
Second review of the branch (warning, regression vs main) on a2a9767de
("fix(desktop): end an own send's freeze hold when its row lands"),
useAnchoredScroll.ts and ChannelPane.tsx. The hold can still go stale
when a send never adds a row to this timeline.

`ChannelPane.handleSendMessage` armed the pin and the hold before it
awaited `onSendMessage`, for every main-composer send. That includes a
DM send that `useMentionSendFlow`'s `onPrepareSendChannel` moves to
another DM (the message mentions an agent who isn't in this one). Its
optimistic row goes to that channel's window, and `goChannel` runs only
after the send succeeds. If the publish is rejected or times out, the
user stays on the original DM with both flags armed. A same-channel send
whose `onMutate` can't resolve the channel also adds no row. Then the
reader scrolls up, the hold skips the freeze, the next arrival from the
other person becomes the tail, and the stale pin moves the reader to the
bottom with no pill. The reviewer's jsdom repro: at HEAD, no pill and
scrollTop 660 -> 956. At the merge base, "1 new message" and 660 stays.

A failed send to the same channel was fine: its optimistic row lands in
`onMutate`, an append that clears the hold, and is then reverted.

Fix:
- `ChannelPane.handleSendMessage` arms only when the send targets this
  channel (`!channelId || channelId === activeChannelId`). A send
  prepared into another channel lands its row there, and the pane
  navigates to it once the send succeeds.
- `MessageTimelineHandle.cancelScrollToBottomOnNextUpdate` disarms a
  send. `useAnchoredScroll` clears the pin and the pending-send flag.
  The hold skipped every freeze while the send was pending, including
  one queued before the send that the arm cancelled. So if the send was
  still pending and Virtua's last report has the reader off the floor,
  the cancel queues that freeze, as the report would have without the
  hold. Later arrivals then wait behind the pill. If the send's row
  already landed, the hold had ended and the cancel only clears the
  pin.
- `ChannelPane` arms through `armOwnSend`, which returns the disarm. The
  main composer calls it when `onSendMessage` rejects, and the Welcome
  "create in chat" flow when its `mutateAsync` rejects. Its `beforeSend`
  option now returns the disarm. A generation counter keeps a late
  failure from disarming a pin that a later send armed.

Validation:
- `MessageTimeline.ownSend.jsdom-test.mjs` adds the reviewer's no-row
  case in three orders: fail before the reader scrolls up, after, and
  with the freeze queued before the send. Each expects "1 new message"
  and the reader's offset unchanged after a new tail arrives. With the
  cancel a no-op (HEAD's behavior), all three fail: no pill, and the
  reader moves from 660 to 956. With the cancel clearing the flags but
  not queueing the freeze, the two orders where the reader left the
  floor during the send fail with "Jump to latest". A fourth case, a
  same-channel send whose row lands and is then reverted, still shows
  "1 new message" and keeps the offset. It passes on HEAD as well,
  because it pins behavior that must not change. 7/7 pass.
- `useAnchoredScroll.lifecycle.test.mjs` adds a hook case: arm, cancel,
  the reader leaves the floor, a new tail arrives without a jump, and a
  later send still pins. It fails if the cancel is a no-op, or if it
  clears only the pending flag. useAnchoredScroll unit tests: 45/45.
- Full desktop `pnpm test`: 6934 unit and 183 jsdom tests pass.
- `tsc --noEmit`, `biome check --error-on-warnings` on the changed files
  and `git diff --check` are clean.
- No test renders `ChannelPane`, so its arm condition, the rejection
  handlers and the generation counter are covered only by tsc and
  review. No browser E2E was run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
(cherry picked from commit 159a87b411947e390e39c5bf94376bea9400a4fd)
Problem:
Third review of the branch (review 89cce157, warning) on 159a87b41
("fix(desktop): disarm an own send's pin and hold when the send fails"),
ChannelPane.tsx. `handleSendMessage` armed the timeline when
`!channelId || channelId === activeChannelId`, and `activeChannelId` was
the value captured in its closure. A send whose slow upload spans a
channel switch passes that check against the channel it was written in,
then arms `messageTimelineRef.current`, the timeline now showing the
other channel. That channel keeps its tail open and its pin armed until
its next append, because the sent row lands in the original channel's
window. A reader who scrolls up there doesn't freeze the tail, and the
next arrival fires the pin and pulls them to the bottom with no pill.

Cause:
`useMentionSendFlow` reads `onSendRef.current` before it starts a
deferred upload and calls that `send` from the upload's `onComplete`.
The arm carried no channel, so only the stale closure decided whether to
arm, and the timeline accepted an arm for whatever channel it rendered.

Fix:
- `MessageTimelineHandle.scrollToBottomOnNextUpdate(sendChannelId)`
  takes the send's channel. `useAnchoredScroll` keeps the channel its
  state belongs to in a ref that its channel reset updates, and its arm
  returns false without arming for any other channel. MessageTimeline
  releases the frozen tail only when the arm is accepted, so a stale
  send neither releases nor pins the channel shown now.
- The arm and disarm move from ChannelPane into
  `useOwnSendArm(timelineRef, getTimelineChannelId)`. It skips an arm
  whose channel isn't the one `getTimelineChannelId` reads now
  (ChannelPane passes its `activeChannelIdRef`) and returns a no-op
  disarm. A skipped arm leaves the generation alone, so it can't stop
  a pending send in the shown channel from disarming when it fails.
  Otherwise the generation works as before: a later send's arm turns an
  earlier send's disarm into a no-op.
- `handleSendMessage` arms `channelId ?? activeChannelId`. A send
  prepared into another DM is still skipped, now by the hook's live
  check instead of the closure.
- The Welcome "create in chat" flow names the channel it sends to:
  `beforeSend` takes a `channelId`, and `useWelcomeAgentCreate` passes
  `activeChannel.id`.
- `useAnchoredScroll` exposes `isVirtualizerAtBottom()`, and
  MessageTimeline's cancel reads it instead of keeping its own copy of
  Virtua's last report. The two agree once the channel has confirmed
  its bottom, which the cancel already requires. The hook's copy is
  also cleared by a target jump or a live row under a held target
  before Virtua reports, so a cancel in that window freezes as the
  report would.
- The channel reset effect now reads `channelId`, so its
  exhaustive-deps suppression goes.

"a cancelled own send leaves the next tail with the reader" in
`useAnchoredScroll.lifecycle.test.mjs` (from 159a87b41) failed 3 of 6
runs at HEAD on this workstation. The mount's settling frame, a
`setTimeout(0)` in that file's shim, could run after the test cleared
its recorded pins and add a "jump". The test now waits for that frame
first, as other cases in the file do: 0 of 15 runs failed. It still
fails when the cancel is a no-op or clears only the pending flag.

Validation:
- New `useOwnSendArm.test.mjs` (node:test, like the rest of desktop's
  unit tests): a send to the shown channel arms it, a send that settles
  after a switch isn't armed and leaves the pending send's disarm
  working, a failed send disarms, and a later arm outlives an earlier
  failure. Each mutation fails a case: dropping the live check, bumping
  the generation before it, dropping the generation check, and a no-op
  disarm.
- `useAnchoredScroll.lifecycle.test.mjs` adds an 11-row table for the
  two gates the cancel reads, `isOwnSendPending()` and
  `isVirtualizerAtBottom()`: no send, a send whose row hasn't landed,
  landed as the tail, landed above the tail, a send to another channel,
  a send then a channel change, a channel change then a send there, and
  the reader off the floor, back at it, after a channel change, and
  with a send pending. Dropping the arm's channel check, not updating
  the channel ref on reset, and a constant `isVirtualizerAtBottom` each
  fail rows.
- `MessageTimeline.ownSend.jsdom-test.mjs`, 9/9:
  - New: with the reader scrolled up and an arrival behind the pill, an
    arm for another channel and a second arrival leave "2 new messages"
    and the offset unchanged. It fails if useAnchoredScroll accepts the
    arm, or if MessageTimeline releases the tail when it is refused.
  - New: a send that fails at the floor leaves the next arrival
    rendered at once. Without the off-floor gate the cancel freezes and
    the arrival is missing right after it renders.
  - Extended: after a same-channel send lands and fails, the reader
    returns to the floor and the arrival renders with the pill gone.
    Without the pending gate the cancel re-arms the freeze's
    bottom-report suppression and the arrival stays behind the pill.
  - The existing cases now pass the channel.
- Desktop unit tests under `src/features/messages` and
  `src/features/channels`: 2006/2006. jsdom tests there: 21/21.
- Playwright smoke after `pnpm build:e2e`, `--workers=4`: messaging,
  send-channel-binding and welcome-agent-modal-screenshots, run twice.
  First run 99 passed and 1 failed, second run 98 passed and 2 failed:
  - `messaging.spec.ts:2785` (own avatar) fails on the avatar image
    after its message renders: 5/6 failed with this change and 6/6 at
    HEAD (`--workers=2 --repeat-each=6`). 4c9272a36 recorded the same
    local failure.
  - `messaging.spec.ts:3170` (thread panel width) never sends. It
    passed 5/5 on rerun.
- `send-channel-binding.spec.ts`, `--repeat-each=5`: 10/10. Its
  mid-send case switches channels while a send is in flight.
  welcome-agent-modal-screenshots (its chat-first case clicks "create
  in chat" and finds the message in the timeline) plus nine messaging
  cases including "long autolink" and the channel-switch cases,
  `--repeat-each=5`: 80/80.
- `tsc --noEmit`, `biome check --error-on-warnings` on the changed
  files, `check:file-sizes` and `git diff --check` are clean.
- No test renders ChannelPane, so its getter and its
  `armOwnSend(channelId ?? activeChannelId)` call are covered only by
  tsc, the hook's tests and the specs above.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
(cherry picked from commit 55c690586f15e7274754831975c33e62675f7144)
@matt2e
matt2e requested a review from a team as a code owner November 5, 2026 05:36
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026

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: 920b1e64bdd05c6605a2562b5b05618bc8bb264a...82af4ceb773acc4a2825190e53b5b6f442581175
  • 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 findings were identified in the authorized PR range after tracing send handling, channel changes, buffering, and failure cleanup.

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, as requested.

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

@joahg joahg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Read the own-send arm/disarm flow. The pending hold ends on the first append, channel change or failed send, and the channel guard stops a late send from arming the wrong timeline.

@matt2e
matt2e merged commit e94b547 into main Nov 5, 2026
73 checks passed
@matt2e
matt2e deleted the mtoohey/flaky-own-send-timeline branch November 5, 2026 22:49
wpfleger96 added a commit that referenced this pull request Nov 5, 2026
* origin/main: (60 commits)
  feat(cli): user status expiry and status read (#8777)
  feat(cli): send a thread message to the channel (#8778)
  feat(relay): add kind 30180 custom emoji entry (#8783)
  feat(cli): read-only unread state (NIP-RS) (#8789)
  feat(cli): report a message to relay moderators (#8829)
  buzz-db: read explicit #h sets one channel at a time (#9110)
  feat(cli): list huddles and their participants (#8830)
  docs(agents): add open-source boundary and drop private repo list (#9114)
  fix(mobile): answer push snapshot bridge calls at user-initiated QoS; report hop latency on callback timeout (#8916)
  test(mobile): lock in invite-page and bare-host community link entry (#9047)
  fix(desktop): keep an own send out of the frozen timeline tail (#9095)
  fix(db): end each table's maintenance transaction before the next (#9096)
  perf(mobile): show a static placeholder for posterless videos (#9138)
  fix(desktop): compensate head trims from the unclamped scroll offset (#9094)
  fix(relay): read kind:39000/39001 inputs behind the replacement lock (#9035)
  fix(db): unlock the storage-accounting lease before its session closes (#9097)
  test(relay,db): remove three timing races from Postgres and E2E tests (#9098)
  fix(db): keep busy-reader history reads off the writer and cap them per pod (#9132)
  perf(mobile): memoize npub labels (#9137)
  mobile: stop re-parsing messages on every resume (#9140)
  ...

Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>

# Conflicts:
#	crates/buzz-cli/src/commands/users.rs
wpfleger96 added a commit that referenced this pull request Nov 6, 2026
…-log-ws-writes

* origin/main:
  fix(mobile): find verified people who have no profile (#9086)
  feat(protocol): define compact membership discovery (BUZZ-615) (#9117)
  perf(db): hydrate search hits with exact event timestamps (#9126)
  perf(db): prune post-commit push enqueue by signed event timestamp (#9128)
  perf(db): use exact event timestamps for transactional deletion (#9130)
  perf(redis): index mesh ready records and share one Redis connection (#9113)
  fix(mobile): delay iOS message actions until a deliberate hold (#9049)
  feat(cli): user status expiry and status read (#8777)
  feat(cli): send a thread message to the channel (#8778)
  feat(relay): add kind 30180 custom emoji entry (#8783)
  feat(cli): read-only unread state (NIP-RS) (#8789)
  feat(cli): report a message to relay moderators (#8829)
  buzz-db: read explicit #h sets one channel at a time (#9110)
  feat(cli): list huddles and their participants (#8830)
  docs(agents): add open-source boundary and drop private repo list (#9114)
  fix(mobile): answer push snapshot bridge calls at user-initiated QoS; report hop latency on callback timeout (#8916)
  test(mobile): lock in invite-page and bare-host community link entry (#9047)
  fix(desktop): keep an own send out of the frozen timeline tail (#9095)
  fix(db): end each table's maintenance transaction before the next (#9096)

Signed-off-by: Will Pfleger <wpfleger@block.xyz>

This branch was previously deployed

1 inactive deployment
codex-review — 82af4ceb Deployed Nov 5, 2026 by matt2e via Run Codex Security Review #12794
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.

2 participants