Skip to content

🤖 fix(desktop): keep buffered messages reachable - #7553

Open
loganj wants to merge 1 commit into
integration/mention-picker-stack-844abd33-basefrom
fix/buffered-timeline-recovery-2c27d89f
Open

🤖 fix(desktop): keep buffered messages reachable#7553
loganj wants to merge 1 commit into
integration/mention-picker-stack-844abd33-basefrom
fix/buffered-timeline-recovery-2c27d89f

Conversation

@loganj

@loganj loganj commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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.

  • Preserve the original at-bottom thread-share/panel-close regression and add a dedicated native wheel-return case that reaches the frozen floor and activates the still-visible latest button before closing the panel. Existing payload/rendered-row/link/reopen assertions remain intact.
  • Correct the dirty-edit fixture through the installed native select-all command, with exact seeded document, native focus, exact selection and immediate authored-document assertions. All ten dirty-edit workflows, refused-navigation coverage and three origin-scoped clipboard cases are preserved; no idle fences, sleeps, selection injection or timeout inflation.
  • Include independently reviewed fixed-base compatibility fixes: remove the actually unused invokeTauri import from markdown; move the two inherited authorization-error presentation conditionals into the existing formatMessageSendError helper 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:

  • 103/103 browser tests: messaging 92 + virtualization 11, one worker, zero retries, normal exit 0 in ~245 seconds. Original and dedicated buffered-recovery regressions both passed.
  • 5994/5994 full desktop package tests, 86 suites, normal exit 0.
  • pnpm typecheck, pnpm build:e2e, actual safe pnpm_config_verify_deps_before_run=false just desktop-check, CHECK_FILE_SIZES_BASE=802ba6170e57ac71d1ecfd6cf6fb12f02ecf910c just file-size-check, and base diff check passed.
  • Own exact build served on exclusive localhost:4179; 103 traces / 1246 retained response bodies, zero binding mismatches. No post-gate source changes; temporary config/server/test children released.
  • Final delivery patch SHA256: cce3faa2dbf10f92532f212bd8d387299290ed69398efbe11fc6e5998f0368d8; recovery-only patch: aa37926c0fb2b23aafa07c8c3087e6a8da70a883a8b1bee52ee4eca1d3256528. Receipts: OUTBOX/BUFFERED_RECOVERY_FIXED_BASE/REPORT.md + SHA256SUMS, and OUTBOX/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; submitMessageEdit remains separate by design. No repository-wide just 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.

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>
@loganj
loganj requested a review from a team as a code owner September 10, 2026 02:06
@loganj
loganj requested a review from jedwards27 September 10, 2026 02:06

@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

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 --check are 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 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.

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

@loganj

loganj commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

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 802ba617 and head c83cc061: MessageComposer.tsx, useMentions.ts, extractMentionPubkeys.ts, and both failing spec files are byte-identical; in useMentionSendFlow.ts the only changes are an authorization-error import removal and two later-send toast-formatting expressions, and the entire sendMessageWithMentionFlow declaration through the end of the file is byte-identical. On that shared path the extraction call at useMentionSendFlow.ts:769 precedes the try at :777, so the toast catch at :908 and finally at :912 cannot intercept that throw, and MessageComposer.tsx:640–666 awaits it with try/finally and no catch. This is source-level reasoning on the two immutable commits, not a runtime base/candidate comparison. The catch-placement repair already exists in the sibling lane — commit db566eb57 in #7190 ("retain drafts after synchronous mention extraction failure"; an ancestor of the #7190/#7192 heads, not of this PR's base or head) — and #7192's e7b84342 explicitly restores the same onboarding ambiguous-submit coverage as expected to fail on the fixed root until the independent composer recovery lands. No repair is proposed or duplicated here.

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 802 and c83 executed zero tests: the required build:e2e route (tsc && vite build --mode e2e) is blocked on unchanged base 802 by the known TS6133 unused import at markdown.tsx:17 (inherited, untouched by this PR). No typecheck bypass, Vite-only route, base edit, or candidate-only substitution was attempted.

4. The second review's chat evidence — exact wording and its limits. Review 5162037688 (APPROVE, author action: none) states: "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." — i.e. the same toast-absent failure reproduced on all three attempts at the candidate and all three at fixed base. Limits: this is a reviewer-reported comparison in review prose; the precise command string is not recorded, its manual/native evidence was "Chromium E2E only; no real WKWebView artifact", no linked run or native receipt accompanies it, and no paired onboarding comparison is reported (integration checks were still running at review time).

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.

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.

2 participants