🤖 fix(desktop): keep buffered messages reachable - #7553
Conversation
Retain original share coverage and add native wheel-return recovery. Replace dirty thread edits through the installed native select-all command before insertion. Resolve fixed-base unused import and extract existing error formatting to satisfy lint and the file-size ratchet without changing admission behavior. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
APPROVE on exact head c83cc06174e9a2a900bc5372c7cf9d3e5436d7aa against fixed base 802ba6170e57ac71d1ecfd6cf6fb12f02ecf910c.
The frozen-window recovery contract is sound. When Virtua reports physical bottom while the semantic timeline remains frozen, pending buffered messages now keep the existing “Jump to latest” release affordance mounted. Activation releases the frozen model before scrolling the expanded model. The exact-head browser journey reaches the physical floor with real wheel input, proves the pending affordance remains visible, releases it, and preserves thread-share, panel close/reopen, attachment, mention, emoji, preview, source-link, and hover behavior. Removing only the pending-count predicate fails the load-bearing visibility assertion on all three attempts.
Integrated exact-head evidence:
- Desktop package: 5,994/5,994; typecheck and E2E build passed.
- Both original and physical-floor recovery rows passed 2/2, zero retries.
- The full 92-test messaging suite passed; messaging + unchanged virtualization produced 102 passes and one unrelated drift failure in existing row 08.
- Dirty-edit helpers now establish native focus, select the exact seeded document through the installed keyboard command, insert the replacement, and immediately assert the authored document across the existing navigation workflows.
- Authorization-error formatting is centralized without changing its two production callers; the exact authorization message and generic behavior remain tested. Markdown only drops an unused import. The six-file diff and
git diff --checkare clean.
Author action: none.
Confidence gaps / verification owner: required Desktop Smoke E2E (2) is red on an unchanged ambiguous-name toast test, with two unchanged retry-pass flakes. The reviewed delta does not change ambiguous-name admission, and current evidence does not establish PR causality. CI/release/reviewer owns same-command comparison against fixed base and rerun/clearance; if the candidate fails while the base passes, this approval expires. Desktop Core and integration shards remain in progress. Native WKWebView, keyboard activation, zoom/reduced-motion, and OS accessibility observation remain reviewer/tooling-owned, not author rework.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed 802ba6170e57ac71d1ecfd6cf6fb12f02ecf910c..c83cc06174e9a2a900bc5372c7cf9d3e5436d7aa at exact head c83cc06174e9a2a900bc5372c7cf9d3e5436d7aa.
Risk: high — this changes virtualized timeline recovery and rewrites messaging E2E coverage, with adjacent compatibility edits in mention-error handling and dirty-edit navigation.
Behavior/contracts traced: frozen timeline ID ownership and pending-count derivation; physical-bottom versus semantic-bottom state; release/RAF-scroll sequencing; panel close/reopen and thread sharing; Tiptap selection and dirty-edit refusal; both production mention-send error callers; markdown/import compatibility.
Findings: no blocking or non-blocking code defect found. MessageTimeline.tsx:878-907 now keeps the sole release affordance mounted whenever buffered messages remain, including when Virtua reports physical bottom while the semantic timeline is frozen. The recovery E2E drives a real wheel return to distance zero, requires the pending pill there, releases through it, and then verifies the thread-share/panel-close contract. Removing only the new pendingCount predicate made that load-bearing assertion fail on all three CI-mode attempts; restoring the exact head passed. The dirty-edit helper uses real ControlOrMeta+a, verifies exact native selection and resulting document content, without sleeps or timeout inflation. formatMessageSendError preserves AgentMentionAuthorizationError.message, and both send paths use it; no admission logic changed.
Author action: none.
Verification owner: CI/reviewer tooling owns the remaining red smoke lane and native confidence gaps. The failing unchanged mention-recipients.spec.ts chat-toast row reproduced 3/3 at the candidate and 3/3 at fixed base under the same CI-mode command, so it is not a PR-caused required-gate failure. CI remains an external merge gate until rerun/closure.
Validation at matching head: full Desktop package gate 5,994/5,994 across 86 suites; pnpm typecheck; pnpm build:e2e; both changed thread-share variants; mutation run; tracked tree clean. A combined messaging/virtualization run had 102 pass and one unchanged virtualization drift failure. Current GitHub checks have macOS/Windows builds, three smoke shards, relay E2E, Semgrep, zizmor, and DCO green; one smoke shard is red as classified above, with integration checks still running at review time.
Manual/native evidence: Chromium E2E only; no real WKWebView artifact.
Residual risk: no dedicated WKWebView, keyboard-only activation, zoom, or reduced-motion observation. Pill semantics and activation were unchanged, while the changed visibility/recovery contract is directly exercised and mutation-proved.
— :bot: Jude’s code review agent
|
Requesting a CI/reviewer disposition on the existing evidence for the two red required-gate lanes of run 34428138869 — not a new run, rerun, re-review, or review re-request, and no merge or CI execution is requested. 1. The two actual failures, and where they stop. Both are the missing ambiguous-mention toast. Each failure occurs at the toast-visibility assertion, so the subsequent draft-preservation / no-send assertions in those specs never ran: this CI shows an absent toast, not a proven draft loss.
(The aggregate and final Desktop jobs of the same run also failed; they are not separately diagnosed here. Desktop Core passed.) 2. Relevant source is shared between base and candidate — qualified scope. Between base 3. No local same-command comparison exists. The accepted local 103-test gate (messaging 92/92 + virtualization 11/11) did not include either failing spec. A subsequent attempt to run these two cases unchanged on both 4. The second review's chat evidence — exact wording and its limits. Review 5162037688 (APPROVE, author action: none) states: "The failing unchanged 5. Approval condition unresolved — disposition requested. Review 5162017638 set the governing condition: "CI/release/reviewer owns same-command comparison against fixed base and rerun/clearance; if the candidate fails while the base passes, this approval expires." That condition is not closed by the evidence above: the local paired run was blocked before execution, and the reviewer's 3/3 chat comparison covers only the smoke row. Please record a disposition on the existing evidence — either accept it as closure of the expiring-approval condition for these two lanes, or name the precise missing evidence (for example, a paired onboarding comparison or an authorized base/candidate same-command route). No rerun, re-review, review re-request, CI execution, or merge is requested, and no author code change is proposed. |
Summary
Keep buffered messages reachable when the timeline is physically at the bottom of its frozen window. “Jump to latest” now remains available whenever buffered messages are pending (
!isAtBottom || bufferedTimeline.pendingCount > 0), so a new shared message cannot remain hidden with no recovery button.invokeTauriimport from markdown; move the two inherited authorization-error presentation conditionals into the existingformatMessageSendErrorhelper and directly test preservation of the authorization message. This preserves both callers' semantics and satisfies formatting and the inherited-file size ratchet without suppression. MessageComposer is untouched; no mention-admission backport.This is a separate six-file candidate against fixed
integration/mention-picker-stack-844abd33-base(802ba6170e57ac71d1ecfd6cf6fb12f02ecf910c), not main or a restack of the existing nine PRs. Published source:c83cc06174e9a2a900bc5372c7cf9d3e5436d7aa; tested and committed tree:5d3070e36c8570d88b63329580ab61357a04a2fb. Independent compatibility review approved the four additional paths; original production recovery review remains applicable to its unchanged patch.Related issue
Related #7124 / #7128. Discovered during #7239, the closest related PR; this is its separately authorized buffered-recovery follow-up, not a replacement. No closing claim for these PRs, and no claim to prove the historical c6 failure: original run 34416836974 remains RED and its cause remains unproved. #7542 and the existing nine heads/bases are untouched.
Originating Buzz conversation
Testing
Actual fixed-base local validation, bound to the tested tree that was subsequently committed, not a claim that precommit commands ran at postcommit HEAD:
pnpm typecheck,pnpm build:e2e, actual safepnpm_config_verify_deps_before_run=false just desktop-check,CHECK_FILE_SIZES_BASE=802ba6170e57ac71d1ecfd6cf6fb12f02ecf910c just file-size-check, and base diff check passed.cce3faa2dbf10f92532f212bd8d387299290ed69398efbe11fc6e5998f0368d8; recovery-only patch:aa37926c0fb2b23aafa07c8c3087e6a8da70a883a8b1bee52ee4eca1d3256528. Receipts:OUTBOX/BUFFERED_RECOVERY_FIXED_BASE/REPORT.md+SHA256SUMS, andOUTBOX/BUFFERED_RECOVERY_COMPAT_REVIEW/REVIEW.md.Limits: installed donor dependencies were reused, not a fresh install. Authorization-toast E2E specs were not run on this base (both helper branches are unit-covered; PR CI is a separate gate). The new test pins preservation of
error.message, not its literal text;submitMessageEditremains separate by design. No repository-widejust ci, native Tauri/live-relay certification, or transferred older 92-test result is claimed. No new screenshot/media upload in this publication-only pass. Remote CI and initial review remain pending, not engineering completion.