Skip to content

fix(desktop): keep catch-up reachable for buffered messages - #7151

Draft
loganj wants to merge 1 commit into
mainfrom
fix/timeline-buffer-catchup-3297b0fc
Draft

fix(desktop): keep catch-up reachable for buffered messages#7151
loganj wants to merge 1 commit into
mainfrom
fix/timeline-buffer-catchup-3297b0fc

Conversation

@loganj

@loganj loganj commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Keep the existing new messages / Jump to latest button reachable whenever the timeline has buffered messages, even if its frozen rows physically reach the bottom of the viewport.

A reader can leave the semantic live tail, then reflow to the physical bottom of the shorter frozen model. New accepted messages remain buffered intentionally to preserve reading position, but the old !isAtBottom render gate hides the only catch-up action. This change adds bufferedTimeline.pendingCount > 0 to that gate. The existing explicit click still releases the buffer and scrolls to latest; arrival, history, virtualizer and cache semantics are unchanged.

The regression renders the actual MessageTimeline, buffer/scroll/settle hooks and UnreadPill, stubbing only the layout-owning message list to drive its public bottom-state callback deterministically. Four cases cover the stranded queue and clickable recovery, zero pending, history prepends without forced scrolling, and genuine semantic return. Removing the production guard causes the recovery case to fail (3 pass / 1 fail); restoring it passes all four.

Related issue

Discovered while investigating #7145, CI run 33444394321. Smoke3 fails at messaging.spec.ts:2629, shared-message visibility/catch-up before the author/avatar assertions, not an avatar-image check. This is a separate baseline repair, not a Start/Move change. Related timeline work: #7006. Searched open PRs for timeline/catch-up duplicates; no duplicate repair found.

Baseline results

Baseline and fixed result evidence with source-labelled screenshots.

Preserved diagnosis experiment (not a claim of green main): unchanged CI-merge source and fresh main 2f3dd850db3afe27e56f18cbcd3548eabdd9b9c2 each fail 5/6 geometry cases at the same shared-row visibility predicate. Both caches contain the accepted message. Main plus this one-condition repair passes 6/6.

Fresh fetch for this PR: 0affe527 (seven newer commits). The original messaging spec and MessageTimeline, useBufferedTimelineMessages, useAnchoredScroll have no delta from the diagnosis baseline. The saved patch was verified against SHA-256 14e126c1ddb762b0a989f3a8b84a13c92b1502739ec4d7622122529ab65abcee before applying.

Fixed results

Final source b13c975cdf4fe2aabf93030b5c8f8e8566fc7d5b:

  • Focused component/buffer/snapshot/scroll-policy/settle tests: 74/74 pass.
  • Full desktop JS suite: 5,827/5,827 pass.
  • TypeScript --noEmit, package Biome 2.4.16 + text/pubkey guards, differential file-size gate: pass (existing package lint warnings remain).
  • Fresh Vite E2E build + original send-thread-to-channel workflow: 6/6 pass, retries 0, width 1280, heights 600/650/700/720/750/800, Chromium CDP CPU throttle 4. All original shared-row, author/avatar fallback, thread-root link/click, attachment/link-preview, mention/emoji, and signed-send-payload assertions remain intact. The temporary diagnostic geometry/telemetry harness is not part of the PR.
  • Mutation check: removing only the production repair yields 3 pass / 1 fail in the new component suite, specifically missing the accessible 1 new message button.

Local tooling note: pnpm's auto-install rejected the reused dependency symlinks before running tests; equivalent package commands ran directly without installing/mutating shared dependencies. Hermit's older Biome 2.4.7 reports unrelated baseline errors; the lockfile-installed package Biome 2.4.16 passes. No unrelated source was changed. Full Rust/mobile/service builds were intentionally not run for this two-file renderer/test repair; current PR CI supplies the remote gate.

Remote CI result

Exact-head CI and gate report: CI33455413702 completed successfully. All executed technical checks pass, including Desktop Core/builds/four smoke shards and both integration shards. The original failing messaging workflow passes first attempt (4.4s). Five other smoke cases required CI-configured retries; this is not a claim of universally first-attempt-clean smoke or green unpatched main.

Review / integration gate

Draft for human review; do not merge or ready-mark automatically. #7145 remains at 8671f76f untouched. After this baseline repair lands through repository/human authority, #7145 must rebase on updated main, rerun affected validation, and get a genuinely new source-bound package for the changed renderer. No merge-from-main, stacked base or cherry-pick into #7145 is being proposed without explicit disposition.

No native app launch, OS keychain/config/credential read, service/DB reset, paid security review, package rebuild/relabel, or changes to #7140. Mock browser evidence is not native release certification.

Signed-off-by: Logan Johnson <loganj@squareup.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 0affe527e880ddd300ae7d4dac3f0600c79b4f5e...b13c975cdf4fe2aabf93030b5c8f8e8566fc7d5b.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review b13c975cdf4fe2aabf93030b5c8f8e8566fc7d5b to authorize a new review.
Any previous review applies only to its recorded range.

loganj added a commit that referenced this pull request Sep 1, 2026
@loganj

loganj commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Baseline results

Preserved fresh-main 2f3dd850 headless Chromium diagnostic: 5 failures / 1 pass over heights 600/650/700/720/750/800 (width 1280, CPU throttle 4, retries 0). The screenshot below is the height-600 failure: accepted shared message absent, no catch-up control. This is the shared-message visibility predicate before avatar assertions in the original failing CI run. The CI-merge-source matrix independently had the same 5-fail/1-pass result. This is preserved baseline evidence, not a new claim that main passes.

baseline-stranded

Fixed results

Same diagnosis baseline plus the proposed condition: 6/6 pass. Fresh PR source b13c975cdf4fe2aabf93030b5c8f8e8566fc7d5b, built against 0affe527, also passes 6/6, retries 0, with all original workflow assertions unchanged. The original messaging spec and three timeline owners have no delta between those main baselines. Below is an unedited frame extracted from the final height-800 passing Playwright trace (JPEG converted to PNG), showing the accepted shared message, source-thread link, mention/emoji and attachment in the channel. It is an illustration from the passing workflow, not a pixel-matched before/after comparison. The full spec also validates the avatar fallback and thread-link click.

fixed-shared-message

The durable component regression supplies deterministic recovery-control coverage without depending on these browser geometries: it renders the real timeline/scroll/buffer hooks and button, drives the virtualizer callback seam, verifies no forced scroll before click, and checks that clicking reveals the buffered row. Removing only the production guard fails that case; restoring it passes all four. Full local JS suite: 5,827 pass. No native launch or native-release certification is implied.

@loganj

loganj commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Exact-head technical result

CI run 33455413702 has completed successfully for b13c975cdf4fe2aabf93030b5c8f8e8566fc7d5b (base 0affe527e880ddd300ae7d4dac3f0600c79b4f5e). Desktop Core, macOS build, Windows build, all four smoke shards, relay-backed integration shards/aggregate, Desktop aggregate, DCO, Semgrep and zizmor pass. Out-of-scope path-filtered jobs were skipped, not tested.

Crucially, Smoke3 passes the original messaging.spec.ts:2437 send-thread-to-channel workflow on its first attempt (4.4s). This is fresh remote evidence, in addition to the source-labelled baseline/fixed browser matrix and falsifiable component regression; it does not claim that unpatched main or the unchanged #7145 CI is green.

Transparency: the smoke jobs are successful but not universally first-attempt clean. CI's configured retries recovered five cases: channel-browser Enter/create, navigation chip-label wrapping, concurrent onboarding installs, and two workflow CRUD cases. I inspected their failure locations (create/geometry/install state/workflow controls), which are outside this catch-up render predicate. No assertions or retry policy were changed, no manual CI reruns were dispatched, and no unrelated fixes were folded in. Smoke totals: 1,254 passed, 5 retry-pass/flaky, 3 skipped.

Remaining external gates: draft remains draft; human/repository review and merge authority are required (GitHub REVIEW_REQUIRED, no human reviews). The security bot requests an exact-current-range review; no paid review was authorized/dispatched by this change. Baseline PR must land first through the normal authority before #7145 can rebase on updated main and receive affected validation and a new source-bound UI package. Native isolation/safety and package certification holds are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant