Repository navigation
fix(relay): show one quiet reconnect toast after 30 s, above the composer - #2968
Conversation
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Production code looks right; one blocker in the changed browser test (inline). Unit tests (RecoveryNotice, LiveStatus, ChannelTimeline, MessageComposer, recovery: 757/757), biome and tsc are clean at a511ca2.
a511ca2 to
70302da
Compare
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70302daf35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🤖 Design review (critical path: fixes a reconnect notice that showed too often and too loudly). No must-fix items. Reconnect trouble now shows as one calm toast after 30 seconds, instead of a warning strip with Retry above the composer plus repeats in the timeline and agent banner. Toasts also no longer cover Send, which helps every toast. Design preference (blocks merge):
Optional:
Not checked: the running app, dark mode, narrow windows, side panels with two composers, enlarged text. |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 70302daf35efbe2462f5ff6c63721cbd6927dc91 against target ffd546e3b28031176f3b1cb60219e7c3367dfc42; actual merge-base ae9efa2015f7674bc6a2f3d227271dce383e818f.
Two P2 regressions remain, already reported in the existing comments:
- Older-history recovery: confirmed. In Solo chats (
ChannelBody.tsx:138), hidingedgeErroralso removes the only Retry button, whileloadNearTopstill refuses paging on the originalwindow.error(ChannelTimeline.tsx:1625,1791). An unrelated catalog/preferences recovery can therefore strand history paging. Preserve the error and retry unless recovery actually owns that failed read. - Agent inventory recovery: confirmed.
choices.ts:158-161includes native inventory failures, but the recovery source list insession.ts:2363-2391cannot retry that inventory. Preserve its error and Retry agents during unrelated relay recovery.
No additional actionable findings from the timer/lifecycle, observer cleanup, shared toast, accessibility, test-source or target-integration review. The separate LiveStatus suppression comment describes behavior already present at the merge-base: its recovery section already replaced the manual error/control, and retryNow() skipped settled denials. We do not count that as a regression introduced here.
Source-only: no tests, builds, app/browser execution or performance measurements. The PR description was inspected; its referenced screenshots are not attached and were not inspected. Desktop network-drop behavior and rendered layout remain unverified by this review.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account.
Reviewed 6d56e122085fa063292e01ee2e77b4fcc42dc980 against target ffd546e3b28031176f3b1cb60219e7c3367dfc42 (merge-base ae9efa2015f7674bc6a2f3d227271dce383e818f). The only change since 70302daf moves the route-request wait before the 30-second clock advance in tests/browser/live-status.spec.mjs.
No new actionable finding in that change. The wait observes a fresh request for the refused route before advancing the notice timer; the fixture still holds EOSE, and the existing notice, layout and recovery assertions remain.
Production code is unchanged, so the two P2 findings confirmed in the prior team review—hidden history retry and hidden agent-inventory recovery during unrelated relay recovery—remain unresolved. No duplicate inline comments added.
This bounded test-only follow-up uses the small-follow-up exception and retains the completed Rocket/Mantis/Groot feature coverage. Source-only: no tests, builds or app/browser execution; this does not establish flake freedom. Referenced screenshots remain unavailable for inspection.
…oser Relay recovery now shows a single buttonless bottom-right notice, "Buzz is having trouble connecting / Retrying automatically.", on every page and only once recovery has lasted 30 s. The inline strip and its Retry now are gone; while recovery runs, the timeline edge error and the composer's agents error stay hidden. Docked composers and the preview Join notice register with useToastClearance so the toast stack rises above them instead of covering Send. Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Signed-off-by: Bradley Axen <baxen@squareup.com>
The 30 s fast-forwards could run before the failure they skip had reached the app, leaving the notice a real 30 s away. The Join and long-error journeys now wait for recovery's retry REQ first, and relay-recovery waits for the channel's catch-up read to fail rather than any earlier failure, which can recover by itself while the network is still down. relay-recovery also keeps read-state saves behind the outage's edge page from the outage on: a save that fires after the network returns but before the live socket reattaches is refused by the broker (503). The journey is about reads, not read-state publication. Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Signed-off-by: Bradley Axen <baxen@squareup.com>
…e delay Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Recovery does not retry older history, and paging stops on a failed read, so hiding the button during an unrelated recovery stranded paging. Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
6d56e12 to
caea6d2
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed caea6d2e18750c352e26545ee80d8d98689d77d8 against target f98fa1517fbddc1072850b50a194ad014c228710 (merge-base c0c0fb3c48bb65080dd2fac74f0d3a2ebe1b5ae1), comparing the fixes and updated-base integration with previously reviewed 6d56e122.
No new actionable findings in this follow-up. The two previously confirmed recovery regressions are addressed:
- History Retry:
ChannelTimeline.tsx:1792–1807now useswindow.errorfor both the button’s presence and its “Retry older messages” label, and still callschannels.loadOlder. This restores the recovery path in both button and scroll modes, retaining the existing cached-only, history-limit and in-flight guards. The history reason text intentionally remains hidden during aggregate recovery; this review does not claim that suppression was removed. - Agent inventory: the rebase drops this PR’s recovery-dependent suppression. Updated main’s mention/agent pickers retain error feedback and Retry through
session.agentChoices.refresh(), reaching both legacy and native inventories independently of relay recovery. The composer’s remaining feature change is toast clearance wiring.
The previously discussed LiveStatus suppression remains baseline behavior, not a regression introduced here. Rocket checked retry ownership and inventory integration; Mantis checked paging guards/concurrency; Groot checked source-visible recovery/focus and public material; Star Lord integrated and verified the source paths.
Source-only: no tests, builds, app/browser execution or CI verification performed. Test and workflow results in the PR are author-reported. Screenshots referenced in the unspecified Buzz thread were unavailable for inspection; desktop network-drop behavior remains unverified by this review.
A failed stream-retry POST left its error on the live snapshot until the next stream status, so the reconnect notice could stay stuck after a later retry succeeded. Clear only that control error, leaving any newer failure in place. Taken from #3000. Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Summary
The "Reconnecting to relay…" notice showed up too often and too loudly. It appeared 3 s after any background read failed, as a warning strip with Retry now above the composer, and the same failure was often repeated in the timeline and the agents banner. Most of those moments were a slow background read, not a real disconnect, and the app already retries on its own (1, 2, 4, 8, 16 s, then every 30 s forever, plus immediately on wake, focus and network-online).
Now:
One quiet notice on every page. Relay recovery shows a single bottom-right toast reading "Buzz is having trouble connecting" with the description "Retrying automatically." It has no button and the calm
infotone.It waits 30 s (
RECOVERY_NOTICE_DELAY_MS, up from 3 s). Blips, laptop wake and failures that the first retries fix stay silent.No duplicates. The inline strip in
LiveStatusis gone. While recovery is running, the timeline's history-edge error text stays hidden; it comes back if the failure outlasts recovery. Retry older messages stays either way, because recovery does not retry older history and paging stops on a failed read.Toasts never cover Send. The composer and the preview's Join notice register with a new
useToastClearance(ref)inToast.tsx, and a CSS rule lifts the toast stack above them. This applies to every toast, not only this one. Pages with no composer keep the usual bottom-right spot.A retry that works clears its old error. A failed
stream-retryrequest used to leave its error on the live snapshot until the next stream status, so the notice could stay stuck after a later retry succeeded.broker-live.tsnow clears only that request's own error, and leaves a newer failure in place. Taken from Taylor's feat(doctor): consolidate background failures and recovery #3000.Unchanged: the recovery backoff, the retry signals,
retryNowwiring (wake/focus/online still call it), and "Live updates limited" with its Retry for failures that recovery does not retry (denials, partial roster).Decided in the Buzz thread with baxen: toast on every page, a longer grace period, simple states, no button, and a toast that keeps the composer clear. Related: #2958 (wake ping) is independent and touches different files.
Screenshots
Taken at 1440 px (light and dark) and 390 px, with a draft typed and Send clickable. They're posted in the Buzz thread where this was decided.
Tests
RecoveryNotice: quiet until exactly 30 s, then one buttonless notice that clears once recovery settles.LiveStatus: empty while recovery runs or shows; focus hands off when the notice replaces a focused Retry.ChannelTimeline: the edge error text is hidden while recovering, and Retry older messages stays, in button and scroll modes. Fails without the fix.broker-live: a successful retry clears its earlier control error, and keeps a newer stream failure. The case without the newer failure fails without the fix.relay-recovery: quiet, then one notice, with no Retry buttons and no "Relay request failed" anywhere. Recovers with no click.live-status: the notice sits above the composer and Send stays clickable at 390/800/1440 px. A long relay reason at enlarged text stays out of the notice. The notice sits above the preview's Join button. An empty preview recovers with no Retry. These layout cases fail without the clearance CSS (checked by removing it).live: quota catch-up and the empty-roster recovery stay quiet inside the delay and recover with no click.focus-navigation: the covered-composer F6 case pins clearance to 0 so that path stays exercised.history-loading,presence,navigation-groups,websocket-actions.profile: locator updates.relay-recovery,live-status,live,history-loading,presence,navigation-groups, plus the toast-touching specs (focus-navigation,message-actions,composer-attachments,profiles,reactions,updates,startup, …).Browser cases removed: the Retry-now keyboard and focus cases in
live-status, because the control no longer exists. The layout contract they guarded (notice never blocks the composer) is kept as the toast-above-composer assertions.Deferred
relay-recoveryfailed 2 of about 60 local Chromium runs. Both times the notice cleared before its entrance finished, which suggests recovery settled while the fixture's HTTP outage was still on. I couldn't reproduce it with debug logging on a heavily loaded machine. Watch it in CI.