From a4523e9f136f274bce435318f46a77ba9107f23f Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Sat, 22 Aug 2026 06:42:42 -0400 Subject: [PATCH 1/3] Show TUI connection status --- AGENTS.md | 54 ++++++---- README.md | 7 +- internal/cable/cable.go | 4 + internal/cable/cable_test.go | 8 ++ internal/cmd/tui_watch.go | 141 +++++++++++++++++++------ internal/cmd/tui_watch_test.go | 131 +++++++++++++++++++++--- internal/tui/errors.go | 7 +- internal/tui/live.go | 173 ++++++++++++++++++++++++++----- internal/tui/live_test.go | 181 +++++++++++++++++++++++---------- internal/tui/mail.go | 34 ++----- internal/tui/screener_test.go | 5 + internal/tui/tui.go | 77 +++++++++++--- 12 files changed, 630 insertions(+), 192 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b47eb261..88ee01d5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -578,25 +578,31 @@ click-to-focus and the replace-not-stack id all live in the plugin), and nothing desktop-shaped lives in `watch*.go`. The TUI's mail list follows the same channel and wants less from it. `internal/cmd/tui_watch.go` -subscribes and relays the changed box IDs down a channel; `internal/tui/live.go` defines -that contract as `tui.MailWatcher`, so the TUI never sees cable or auth, and a test hands -it a plain channel. There is no sync cursor: the doorbell says which box changed and -`mailView.refreshBox` reads that box's top page again, which is where a change shows up. -A reconnect sends `tui.AnyBoxChanged`, standing for the changes broadcast while the -connection was down. - -`internal/tui/live.go` is also where the two delays live. `liveRefreshDelay` collects one -delivery's changes into a single re-read — a thread rings the doorbell once per posting. -`liveRetryDelay` is how long a re-read waits when a form or a picker is open over the list, -or a write hasn't landed: the change is held rather than dropped, because a re-read -replaces what the reader is looking at. `contentList.refreshHead` is what makes that -safe — unlike `setPostings` it keeps the cursor on its posting and keeps a selection — -and the re-read has its own request lane (`liveRequestID`, `postingsRefreshedMsg`) so it -can never be confused with a read the user asked for, or show the spinner. +subscribes and relays typed `tui.MailWatchEvent` values; `internal/tui/live.go` defines +that `tui.MailWatcher` contract, so the TUI never sees cable or auth, and a test hands it +a plain channel. Box events are doorbells: `mailView.refreshBox` reads that box's top page +again, which is where a change shows up. Connection events carry disconnect/reconnect +state. A reconnect asks for `tui.AnyBoxChanged`, standing for broadcasts sent during the +gap, and the model re-reads the box on screen. + +`internal/tui/live.go` owns both refresh and reconnect timing. `liveRefreshDelay` collects +one delivery's changes into a single re-read — a thread rings the doorbell once per +posting. `liveRetryDelay` is how long a re-read waits when a form or a picker is open over +the list, or a write hasn't landed: the change is held rather than dropped, because a +re-read replaces what the reader is looking at. `mailWatchRetryDelay` backs a watch that +could not start or stopped for good from two seconds up to thirty; authentication failures +stand without retrying. `contentList.refreshHead` is what makes a re-read safe — unlike +`setPostings` it keeps the cursor on its posting and keeps a selection — and the re-read +has its own request lane (`liveRequestID`, `postingsRefreshedMsg`) so it can never be +confused with a read the user asked for, or show the spinner. The watch outlives the view context, which a mail account switch throws away; that is what -`model.watchCtx` is for. When the stream closes for good the mail list says so and stays -saying so until ctrl+r reads the box again. +`model.watchCtx` is for. The model holds connection state above the active section, so an +offline or reconnecting notice remains visible in Mail, Contacts, Calendar and Journal. +A temporary drop is followed by Action Cable's own reconnect; an initial network failure +or a stream that closes for good gets a fresh bounded dial from the model. The initial dial +itself is capped so it can report state instead of waiting inside Action Cable forever. +A successful connection removes the status row and catches the current mail box up. The Screener is told over a different stream, because haystack has no channel for it: `Clearance::Broadcasting` re-renders the Screener's own button over a Turbo stream, and @@ -614,7 +620,10 @@ ctrl+r, closing The Screener, or the doorbell itself will all reopen a watch the hung up on. Opening one gives up the one before it — the subscription belongs to the watch's context, and cancelling is what unsubscribes it, so a stream nobody is following does not go on ringing. A mail account switch gives it up for the same reason: the signed -name is the account's, and the new account's sources read serves its own. +name is the account's, and the new account's sources read serves its own. The subscription +uses that short-lived context, while `tuiCableClient` is owned by the TUI-wide watch +context; replacing a Screener name therefore unsubscribes it without closing Mail's shared +connection. The doorbell always re-reads the count, wherever the user is, because the mail list is where The Screener announces itself. When The Screener is what's on screen the queue is @@ -623,9 +632,12 @@ mail list — and held while a decision is in flight or the clear-everything que On the history tab nothing is read; the pending pane is just marked unloaded, which is what `switchTab` already looks at. Both watches share one websocket (`tuiCableClient` in `internal/cmd/tui_watch.go`): two subscriptions, one authorization. -A client that stopped itself — the server said don't come back — answers every `Subscribe` -with `ErrClosed` and never dials again, so `tuiSubscribe` drops it and dials a fresh one -rather than handing a reopened watch a dead connection. +A client that stopped itself never dials again: depending on how it stopped, `Subscribe` +answers `ErrClosed` or preserves the server's non-reconnecting `DisconnectError`. +`tuiSubscribe` recognizes both, drops that client and dials a fresh one rather than handing +a reopened watch a dead connection. Mail doorbells are coalesced when their bounded queue +is full, while a connection transition drains those stale doorbells and takes priority; +the reconnect catch-up reads the current state once. ### API documentation diff --git a/README.md b/README.md index b1872622..2fe88b23 100644 --- a/README.md +++ b/README.md @@ -225,9 +225,10 @@ Navigate between Mail, Contacts, Calendar, and Journal. The context-sensitive sh The mail list follows the server. HEY tells the TUI when a box changed over the same Action Cable connection `hey watch` uses, and the box on screen is read again a moment later, keeping your place in the list and anything you had selected. A change that arrives -while a form or a picker is open waits for it to close. Press Ctrl+R to read the box again -yourself; if the connection goes away for good, the list says so and Ctrl+R is how you -catch up. +while a form or a picker is open waits for it to close. A standing status below the header +appears in every section while the network is offline or live updates are reconnecting. +The TUI retries the connection, clears the status when it returns, and catches up the box +on screen; Ctrl+R remains available whenever you want to read it yourself. The Screener keeps up too. When a first-time sender writes, the count above the threads changes on its own, and if you have The Screener open the new sender appears in the queue diff --git a/internal/cable/cable.go b/internal/cable/cable.go index 6ba55de9..a8f4a2e8 100644 --- a/internal/cable/cable.go +++ b/internal/cable/cable.go @@ -42,6 +42,10 @@ func Dial(ctx context.Context, baseURL string, authMgr *auth.Manager, options .. client := actioncable.New(cableURL, settings...) if err := client.Connect(ctx); err != nil { + // Connect's context bounds the caller's wait rather than the client's lifetime. + // A dial that did not complete has no owner to close it, so stop its retry loop + // before returning the error. + _ = client.Close() return nil, err } diff --git a/internal/cable/cable_test.go b/internal/cable/cable_test.go index 31342ff2..727f3ece 100644 --- a/internal/cable/cable_test.go +++ b/internal/cable/cable_test.go @@ -120,6 +120,14 @@ func TestEveryDialCarriesCurrentCredentials(t *testing.T) { if got := headers[1+redialed].Get("Origin"); got != "https://app.hey.com" { t.Errorf("redial Origin = %q, want the client's own headers kept", got) } + + // Connect's deadline only bounds its wait; Dial owns and stops a client that never + // connected so no retry goroutine is left behind after the error. + dialsAtReturn := len(headers) + time.Sleep(10 * time.Millisecond) + if got := len(recorded.recorded()); got != dialsAtReturn { + t.Errorf("dials after return = %d, want the %d attempts already made", got, dialsAtReturn) + } } func TestDialWithoutCredentialsFailsBeforeConnecting(t *testing.T) { diff --git a/internal/cmd/tui_watch.go b/internal/cmd/tui_watch.go index a38571cb..8341c1db 100644 --- a/internal/cmd/tui_watch.go +++ b/internal/cmd/tui_watch.go @@ -18,18 +18,53 @@ const turboStreamsChannel = "Turbo::StreamsChannel" // instead of blocking the relay. A full backlog means a re-read is already coming. const mailChangeBacklog = 16 -// reconnectBacklog is one, because a reconnect always says the same thing. It is a -// channel of its own so that the relay goroutine is the only writer to the channel it -// closes: the cable client drains callbacks queued before it was told to stop, and a +// reconnectBacklog is one, because a Screener reconnect always says the same thing. It +// is a channel of its own so that the relay goroutine is the only writer to the channel +// it closes: the cable client drains callbacks queued before it was told to stop, and a // send on a closed channel panics whatever the select around it says — off a goroutine // Bubble Tea knows nothing about, which takes the terminal down in raw mode. Nothing // closes this one, so a callback arriving after the relay is gone rings into the buffer // and is collected with it. const reconnectBacklog = 1 -// unsubscribeTimeout bounds the goodbye sent for a watch that is over. Nothing waits on -// it, and a connection that has gone away is reason to stop trying rather than to hang. -const unsubscribeTimeout = 5 * time.Second +type mailConnectionNotifier struct { + sync.Mutex + event tui.MailWatchEvent + version uint64 + wake chan struct{} +} + +func newMailConnectionNotifier() *mailConnectionNotifier { + return &mailConnectionNotifier{wake: make(chan struct{}, 1)} +} + +// note keeps the newest connection state and wakes the relay without blocking Action +// Cable's callback dispatcher. If a disconnect and reconnect arrive before the relay can +// draw either, the reconnect is the state that matters and still asks for a catch-up. +func (n *mailConnectionNotifier) note(connection tui.MailConnection, willReconnect bool) { + n.Lock() + n.event = tui.MailWatchEvent{Connection: connection, WillReconnect: willReconnect} + n.version++ + n.Unlock() + ring(n.wake, struct{}{}) +} + +func (n *mailConnectionNotifier) after(version uint64) (tui.MailWatchEvent, uint64, bool) { + n.Lock() + defer n.Unlock() + return n.event, n.version, n.version != version +} + +const ( + // unsubscribeTimeout bounds the goodbye sent for a watch that is over. Nothing waits + // on it, and a connection that has gone away is reason to stop trying rather than hang. + unsubscribeTimeout = 5 * time.Second + + // tuiCableDialTimeout turns an unreachable cable server into app state instead of + // leaving the startup command inside Action Cable's retry loop forever. The model + // owns the retries after this first bounded attempt. + tuiCableDialTimeout = 5 * time.Second +) // tuiWatchers are the streams `hey tui` follows to stay live. func tuiWatchers() tui.Watchers { @@ -43,40 +78,45 @@ func tuiWatchers() tui.Watchers { // // The stream closes when ctx is done, or when the connection is gone for good, which is // how the TUI hears that its list has stopped being live. -func watchMailChanges(ctx context.Context) (<-chan int64, error) { - // A reconnect stands for every box: the changes broadcast while the connection was - // down were missed, and the box on screen has to be read again to find them. It - // arrives on a channel of its own so that the relay is the only thing writing to - // the one the TUI reads — see reconnectBacklog. - reconnects := make(chan struct{}, reconnectBacklog) - subscription, err := tuiSubscribe(ctx, actioncable.Identifier{Channel: changesChannel}, +func watchMailChanges(ctx context.Context) (<-chan tui.MailWatchEvent, error) { + connection := newMailConnectionNotifier() + subscription, err := tuiSubscribe(ctx, ctx, actioncable.Identifier{Channel: changesChannel}, actioncable.OnConnected(func(reconnected bool) { if reconnected { - ring(reconnects, struct{}{}) + connection.note(tui.MailConnectionReconnected, false) } + }), + actioncable.OnDisconnected(func(willReconnect bool) { + connection.note(tui.MailConnectionDisconnected, willReconnect) })) if err != nil { return nil, err } - changes := make(chan int64, mailChangeBacklog) + events := make(chan tui.MailWatchEvent, mailChangeBacklog) go func() { defer unsubscribe(ctx, subscription) - relayMailChanges(ctx, subscription.Messages(), reconnects, changes) + relayMailChanges(ctx, subscription.Messages(), connection, events) }() - return changes, nil + return events, nil } -func relayMailChanges(ctx context.Context, messages <-chan actioncable.Message, reconnects <-chan struct{}, changes chan<- int64) { - defer close(changes) +func relayMailChanges(ctx context.Context, messages <-chan actioncable.Message, connection *mailConnectionNotifier, events chan tui.MailWatchEvent) { + defer close(events) + var connectionVersion uint64 for { select { case <-ctx.Done(): return - case <-reconnects: - ring(changes, tui.AnyBoxChanged) + case <-connection.wake: + event, version, changed := connection.after(connectionVersion) + if !changed { + continue + } + connectionVersion = version + ringMailWatchEvent(events, event) case message, open := <-messages: if !open { return @@ -87,11 +127,7 @@ func relayMailChanges(ctx context.Context, messages <-chan actioncable.Message, if err := message.Unmarshal(¬ification); err != nil { continue } - select { - case changes <- notification.BoxID: - case <-ctx.Done(): - return - } + ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: notification.BoxID}) } } } @@ -101,9 +137,9 @@ func relayMailChanges(ctx context.Context, messages <-chan actioncable.Message, // and serves the signed name of that stream with the pending count. What it broadcasts is // markup for the web app, so nothing is read out of it: the arrival is the whole message, // and the TUI reads the count again behind it. -func watchScreenerChanges(ctx context.Context, signedStreamName string) (<-chan struct{}, error) { +func watchScreenerChanges(ctx, connectionCtx context.Context, signedStreamName string) (<-chan struct{}, error) { reconnects := make(chan struct{}, reconnectBacklog) - subscription, err := tuiSubscribe(ctx, actioncable.Identifier{ + subscription, err := tuiSubscribe(ctx, connectionCtx, actioncable.Identifier{ Channel: turboStreamsChannel, Params: actioncable.Params{"signed_stream_name": signedStreamName}, }, actioncable.OnConnected(func(reconnected bool) { @@ -153,6 +189,35 @@ func unsubscribe(ctx context.Context, subscription *actioncable.Subscription) { _ = subscription.Unsubscribe(goodbye) } +// ringMailWatchEvent keeps connection state ahead of stale box doorbells. Box events can +// be coalesced because one re-read catches up every posting in that box; a connection +// transition empties that backlog so the global status changes promptly, and reconnecting +// catches the visible box up without relying on an older doorbell. +func ringMailWatchEvent(events chan tui.MailWatchEvent, event tui.MailWatchEvent) { + if event.Connection == tui.MailConnectionUnchanged { + select { + case events <- event: + default: + } + return + } + + for { + select { + case <-events: + continue + default: + } + select { + case events <- event: + return + default: + // The reader raced us between draining and sending. Try the now-current + // queue again rather than blocking Action Cable's relay. + } + } +} + // ring drops the notification when one is already waiting: they all say the same thing, // and a reader that has fallen behind must not hold up the goroutine doing the ringing. func ring[T any](notifications chan<- T, notification T) { @@ -167,16 +232,16 @@ func ring[T any](notifications chan<- T, notification T) { // every Subscribe with ErrClosed and never dials again on its own, so a Screener stream // reopened after that would find a dead connection and stay dead. Each dial carries // current credentials, which is what makes redialling worth doing. -func tuiSubscribe(ctx context.Context, identifier actioncable.Identifier, options ...actioncable.SubscriptionOption) (*actioncable.Subscription, error) { - client, err := tuiCableClient(ctx) +func tuiSubscribe(ctx, connectionCtx context.Context, identifier actioncable.Identifier, options ...actioncable.SubscriptionOption) (*actioncable.Subscription, error) { + client, err := tuiCableClient(connectionCtx) if err != nil { return nil, err } subscription, err := client.Subscribe(ctx, identifier, options...) - if errors.Is(err, actioncable.ErrClosed) { + if stoppedTuiCableError(err) { forgetTuiCable(client) - if client, err = tuiCableClient(ctx); err != nil { + if client, err = tuiCableClient(connectionCtx); err != nil { return nil, err } subscription, err = client.Subscribe(ctx, identifier, options...) @@ -184,6 +249,14 @@ func tuiSubscribe(ctx context.Context, identifier actioncable.Identifier, option return subscription, err } +func stoppedTuiCableError(err error) bool { + if errors.Is(err, actioncable.ErrClosed) { + return true + } + var disconnected *actioncable.DisconnectError + return errors.As(err, &disconnected) && !disconnected.Reconnect +} + // tuiCable is the one connection the TUI's watches share — two subscriptions over one // websocket, authorized once. It is opened by whichever watch needs it first and closed // when that watch's context is done, which is the TUI's own lifetime. @@ -200,7 +273,9 @@ func tuiCableClient(ctx context.Context) (*actioncable.Client, error) { return tuiCable.client, nil } - client, err := cable.Dial(ctx, cfg.BaseURL, authMgr) + dialing, stopDialing := context.WithTimeout(ctx, tuiCableDialTimeout) + defer stopDialing() + client, err := cable.Dial(dialing, cfg.BaseURL, authMgr) if err != nil { return nil, watchDialError(err) } diff --git a/internal/cmd/tui_watch_test.go b/internal/cmd/tui_watch_test.go index 87f0b76e..6a57a18b 100644 --- a/internal/cmd/tui_watch_test.go +++ b/internal/cmd/tui_watch_test.go @@ -3,7 +3,9 @@ package cmd import ( "context" "errors" + "io" "net/http" + "sync" "testing" "time" @@ -14,28 +16,46 @@ import ( "github.com/basecamp/hey-cli/internal/tui" ) -func TestRelayMailChangesNamesTheChangedBox(t *testing.T) { +func TestRelayMailChangesNamesTheChangedBoxAndConnectionState(t *testing.T) { messages := make(chan actioncable.Message, 1) - reconnects := make(chan struct{}, reconnectBacklog) - changes := make(chan int64, mailChangeBacklog) + connection := newMailConnectionNotifier() + events := make(chan tui.MailWatchEvent, mailChangeBacklog) - go relayMailChanges(t.Context(), messages, reconnects, changes) + go relayMailChanges(t.Context(), messages, connection, events) messages <- actioncable.Message(`{"change":"upsert","box_id":24088}`) - if got := <-changes; got != 24088 { - t.Errorf("change = %d, want the box the notification named", got) + if got := <-events; got.BoxID != 24088 || got.Connection != tui.MailConnectionUnchanged { + t.Errorf("event = %+v, want the box the notification named", got) } - ring(reconnects, struct{}{}) - if got := <-changes; got != tui.AnyBoxChanged { - t.Errorf("change = %d, want a reconnect to stand for every box", got) + connection.note(tui.MailConnectionDisconnected, true) + if got := <-events; got.Connection != tui.MailConnectionDisconnected || !got.WillReconnect { + t.Errorf("event = %+v, want the temporary disconnect", got) + } + connection.note(tui.MailConnectionReconnected, false) + if got := <-events; got.Connection != tui.MailConnectionReconnected { + t.Errorf("event = %+v, want the reconnect", got) } // A notification that can't be read leaves the stream alone. messages <- actioncable.Message(`not json`) messages <- actioncable.Message(`{"box_id":31145}`) - if got := <-changes; got != 31145 { - t.Errorf("change = %d, want the stream to carry on past what it can't read", got) + if got := <-events; got.BoxID != 31145 { + t.Errorf("event = %+v, want the stream to carry on past what it can't read", got) + } +} + +func TestMailConnectionNotifierKeepsTheNewestRapidTransition(t *testing.T) { + connection := newMailConnectionNotifier() + connection.note(tui.MailConnectionDisconnected, true) + connection.note(tui.MailConnectionReconnected, false) + + event, version, changed := connection.after(0) + if !changed || version != 2 || event.Connection != tui.MailConnectionReconnected { + t.Errorf("event = %+v, version = %d, changed = %v; want the latest reconnect", event, version, changed) + } + if _, _, changed := connection.after(version); changed { + t.Error("the same transition should only be read once") } } @@ -62,11 +82,12 @@ func TestARelayIsTheOnlyWriterToTheStreamItCloses(t *testing.T) { mailMessages := make(chan actioncable.Message) screenerMessages := make(chan actioncable.Message) + connection := newMailConnectionNotifier() reconnects := make(chan struct{}, reconnectBacklog) - mail := make(chan int64, mailChangeBacklog) + mail := make(chan tui.MailWatchEvent, mailChangeBacklog) screener := make(chan struct{}, 1) - go relayMailChanges(relaying, mailMessages, reconnects, mail) + go relayMailChanges(relaying, mailMessages, connection, mail) go relayScreenerChanges(relaying, screenerMessages, reconnects, screener) stop() @@ -78,10 +99,92 @@ func TestARelayIsTheOnlyWriterToTheStreamItCloses(t *testing.T) { } for range reconnectBacklog + 2 { + connection.note(tui.MailConnectionReconnected, false) ring(reconnects, struct{}{}) } } +func TestMailConnectionStateTakesPriorityOverAFullBoxBacklog(t *testing.T) { + events := make(chan tui.MailWatchEvent, 2) + ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 1}) + ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 2}) + ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 3}) + + ringMailWatchEvent(events, tui.MailWatchEvent{Connection: tui.MailConnectionDisconnected, WillReconnect: true}) + if len(events) != 1 { + t.Fatalf("events = %d, want stale box doorbells coalesced behind connection state", len(events)) + } + if got := <-events; got.Connection != tui.MailConnectionDisconnected || !got.WillReconnect { + t.Errorf("event = %+v, want the disconnect", got) + } +} + +type scriptedCableTransport struct{ conn *scriptedCableConn } + +func (t scriptedCableTransport) Dial(context.Context, string, actioncable.DialOptions) (actioncable.Conn, error) { + return t.conn, nil +} + +type scriptedCableConn struct { + reads chan []byte + done chan struct{} + once sync.Once +} + +func newScriptedCableConn() *scriptedCableConn { + return &scriptedCableConn{reads: make(chan []byte, 2), done: make(chan struct{})} +} + +func (c *scriptedCableConn) Subprotocol() string { return "actioncable-v1-json" } + +func (c *scriptedCableConn) Read(ctx context.Context) ([]byte, error) { + select { + case payload := <-c.reads: + return payload, nil + case <-c.done: + return nil, io.EOF + case <-ctx.Done(): + return nil, ctx.Err() + } +} + +func (c *scriptedCableConn) Write(context.Context, []byte) error { return nil } + +func (c *scriptedCableConn) Close() error { + c.once.Do(func() { close(c.done) }) + return nil +} + +func TestStoppedTuiCableErrorsIncludeTerminalServerDisconnects(t *testing.T) { + if !stoppedTuiCableError(actioncable.ErrClosed) { + t.Error("an explicitly closed client should be replaced") + } + if !stoppedTuiCableError(&actioncable.DisconnectError{Reason: actioncable.ReasonUnauthorized, Reconnect: false}) { + t.Error("a client the server stopped for good should be replaced") + } + if stoppedTuiCableError(&actioncable.DisconnectError{Reason: "server restart", Reconnect: true}) { + t.Error("a client already reconnecting should stay shared") + } +} + +func TestServerStoppedActionCableClientIsReplaceable(t *testing.T) { + conn := newScriptedCableConn() + conn.reads <- []byte(`{"type":"welcome"}`) + client := actioncable.New("ws://cable.example.test/cable", actioncable.WithTransport(scriptedCableTransport{conn: conn})) + if err := client.Connect(t.Context()); err != nil { + t.Fatalf("connect: %v", err) + } + defer client.Close() + + conn.reads <- []byte(`{"type":"disconnect","reason":"unauthorized","reconnect":false}`) + subscribing, stop := context.WithTimeout(t.Context(), time.Second) + defer stop() + _, err := client.Subscribe(subscribing, actioncable.Identifier{Channel: changesChannel}) + if !stoppedTuiCableError(err) { + t.Fatalf("subscribe error = %T %v, want a terminal server disconnect that opens a new client", err, err) + } +} + func TestTuiSubscribeReplacesAClientThatStoppedItself(t *testing.T) { // A client the server hung up on for good answers every Subscribe with ErrClosed and // never dials again on its own, so handing it to a reopened watch means the TUI is @@ -110,7 +213,7 @@ func TestTuiSubscribeReplacesAClientThatStoppedItself(t *testing.T) { dialing, giveUp := context.WithTimeout(t.Context(), 200*time.Millisecond) defer giveUp() - _, err := tuiSubscribe(dialing, actioncable.Identifier{Channel: changesChannel}) + _, err := tuiSubscribe(dialing, dialing, actioncable.Identifier{Channel: changesChannel}) if err == nil { t.Fatal("a dial against a dead address should fail") } diff --git a/internal/tui/errors.go b/internal/tui/errors.go index a41b2fe3..1fbe9f63 100644 --- a/internal/tui/errors.go +++ b/internal/tui/errors.go @@ -1,6 +1,8 @@ package tui import ( + "errors" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/terminal" ) @@ -11,7 +13,10 @@ import ( // a shell prompt what to run, and this is read inside a full-screen app. The // message may be the server's, so the line is sanitized before it is shown. func errorNotice(what string, err error) string { - e := apierr.AsError(apierr.FromSDK(err)) + var e *apierr.Error + if !errors.As(err, &e) { + e = apierr.AsError(apierr.FromSDK(err)) + } if e.Hint != "" && e.Code != apierr.CodeAuth { return terminal.SanitizeLine(what + ": " + e.Message + " — " + e.Hint) } diff --git a/internal/tui/live.go b/internal/tui/live.go index 3ead0da4..6a80346b 100644 --- a/internal/tui/live.go +++ b/internal/tui/live.go @@ -2,9 +2,12 @@ package tui import ( "context" + "errors" "time" tea "charm.land/bubbletea/v2" + + "github.com/basecamp/hey-cli/internal/apierr" ) // Watchers are the streams the TUI follows to stay live. Both are optional: without them @@ -14,20 +17,48 @@ type Watchers struct { Screener ScreenerWatcher } -// MailWatcher opens the stream of boxes that changed, so the TUI is told when to re-read -// a box instead of polling for it. The stream closes when ctx is done, or when whatever -// is behind it has given up for good — which is what tells the TUI it isn't live any more. -type MailWatcher func(ctx context.Context) (<-chan int64, error) +// MailWatcher opens the stream of mail and connection events that keeps the TUI live. +// Box events ask it to re-read changed mail, while connection events let it show when +// live updates are reconnecting. The stream closes when ctx is done, or when whatever +// is behind it has given up for good. +type MailWatcher func(ctx context.Context) (<-chan MailWatchEvent, error) + +// MailConnection is a transition reported by the connection behind a mail watch. +type MailConnection uint8 + +const ( + MailConnectionUnchanged MailConnection = iota + MailConnectionDisconnected + MailConnectionReconnected +) -// ScreenerWatcher opens the stream that says The Screener changed. HEY signs the stream -// name and serves it alongside the pending count, so a watcher can only be opened once -// that name has been read — there is nothing to subscribe to before then. -type ScreenerWatcher func(ctx context.Context, signedStreamName string) (<-chan struct{}, error) +// MailWatchEvent reports either a changed box or a connection transition. A disconnected +// event says whether the connection is already retrying; a reconnect asks the TUI to +// catch up the box on screen because broadcasts sent during the gap were missed. +type MailWatchEvent struct { + BoxID int64 + Connection MailConnection + WillReconnect bool +} + +// ScreenerWatcher opens the stream that says The Screener changed. ctx owns this signed +// subscription; connectionCtx owns the shared TUI connection that remains live when a +// signed stream is replaced. HEY serves the signed name alongside the pending count, so +// a watcher opens after that name has been read. +type ScreenerWatcher func(ctx, connectionCtx context.Context, signedStreamName string) (<-chan struct{}, error) // AnyBoxChanged stands for "something changed, we don't know what" — a watcher sends it // after a reconnect, where the changes broadcast while it was away were missed. const AnyBoxChanged int64 = 0 +type mailWatchStatus uint8 + +const ( + mailWatchLive mailWatchStatus = iota + mailWatchReconnecting + mailWatchUnavailable +) + // liveRefreshDelay collects one delivery's changes into a single re-read: a thread lands // as several postings, and each one rings the doorbell separately. // @@ -35,47 +66,137 @@ const AnyBoxChanged int64 = 0 // or a picker is open over the list. Nothing is read until it closes, but the change is // held onto rather than dropped. const ( - liveRefreshDelay = 500 * time.Millisecond - liveRetryDelay = 2 * time.Second + liveRefreshDelay = 500 * time.Millisecond + liveRetryDelay = 2 * time.Second + mailWatchFirstRetry = 2 * time.Second + mailWatchMaximumRetry = 30 * time.Second ) // mailWatchStartedMsg carries the stream a watcher opened, or the reason there isn't one. type mailWatchStartedMsg struct { - changes <-chan int64 + attempt uint64 + events <-chan MailWatchEvent err error } -// mailChangedMsg reports one changed box, or a stream that has closed. -type mailChangedMsg struct { - boxID int64 +// mailWatchEventMsg reports one mail-watch event, or a stream that has closed. +type mailWatchEventMsg struct { + event MailWatchEvent closed bool } +// mailWatchRetryMsg asks the model to open a new watch after a failed start or a stream +// that stopped for good. The attempt identifies the state that scheduled it, so a timer +// left behind by a successful connection cannot replace that connection. +type mailWatchRetryMsg struct{ attempt uint64 } + // mailRefreshDueMsg is the re-read a change asked for, once its delay has passed. type mailRefreshDueMsg struct{ boxID int64 } -func startMailWatchCmd(ctx context.Context, watch MailWatcher) tea.Cmd { +func startMailWatchCmd(ctx context.Context, watch MailWatcher, attempt uint64) tea.Cmd { if watch == nil { return nil } return func() tea.Msg { - changes, err := watch(ctx) - return mailWatchStartedMsg{changes: changes, err: err} + events, err := watch(ctx) + return mailWatchStartedMsg{attempt: attempt, events: events, err: err} } } -// waitForMailChangeCmd blocks until the next box changes, then reports it once. The -// handler re-arms it, the way watchThemeCmd is re-armed. -func waitForMailChangeCmd(changes <-chan int64) tea.Cmd { - if changes == nil { +// waitForMailWatchEventCmd blocks until the next box or connection event, then reports it +// once. The handler re-arms it, the way watchThemeCmd is re-armed. +func waitForMailWatchEventCmd(events <-chan MailWatchEvent) tea.Cmd { + if events == nil { return nil } return func() tea.Msg { - boxID, open := <-changes + event, open := <-events if !open { - return mailChangedMsg{closed: true} + return mailWatchEventMsg{closed: true} + } + return mailWatchEventMsg{event: event} + } +} + +func retryMailWatchLaterCmd(attempt uint64, delay time.Duration) tea.Cmd { + return tea.Tick(delay, func(time.Time) tea.Msg { return mailWatchRetryMsg{attempt: attempt} }) +} + +func mailWatchRetryDelay(failures int) time.Duration { + delay := mailWatchFirstRetry + for range max(failures-1, 0) { + if delay >= mailWatchMaximumRetry/2 { + return mailWatchMaximumRetry + } + delay *= 2 + } + return min(delay, mailWatchMaximumRetry) +} + +func retryableMailWatchError(err error) bool { + var known *apierr.Error + if errors.As(err, &known) { + return known.Code == apierr.CodeNetwork + } + return apierr.AsError(apierr.FromSDK(err)).Code == apierr.CodeNetwork +} + +func (m *model) mailWatchFailed(err error) tea.Cmd { + m.mailWatchEvents = nil + if retryableMailWatchError(err) { + m.mailWatchStatus = mailWatchReconnecting + m.mailWatchReason = "Offline — reconnecting to HEY" + return m.retryMailWatch() + } + + m.mailWatchStatus = mailWatchUnavailable + m.mailWatchReason = errorNotice("Live updates unavailable", err) + return nil +} + +func (m *model) retryMailWatch() tea.Cmd { + if m.watchMail == nil { + return nil + } + m.mailWatchStatus = mailWatchReconnecting + m.mailWatchFailures++ + return retryMailWatchLaterCmd(m.mailWatchAttempt, mailWatchRetryDelay(m.mailWatchFailures)) +} + +func (m *model) mailWatchStopped() tea.Cmd { + m.mailWatchReason = "Live updates disconnected — reconnecting to HEY" + return m.retryMailWatch() +} + +func (m *model) mailWatchDisconnected(willReconnect bool) { + m.mailWatchStatus = mailWatchReconnecting + m.mailWatchReason = "Live updates disconnected — reconnecting to HEY" + if !willReconnect { + m.mailWatchStatus = mailWatchUnavailable + m.mailWatchReason = "Live updates disconnected" + } +} + +func (m *model) mailWatchConnected() { + m.mailWatchStatus = mailWatchLive + m.mailWatchReason = "" + m.mailWatchFailures = 0 +} + +func (m model) mailWatchNotice() string { + switch m.mailWatchStatus { + case mailWatchReconnecting: + if m.mailWatchReason != "" { + return m.mailWatchReason + } + return "Live updates disconnected — reconnecting to HEY" + case mailWatchUnavailable: + if m.mailWatchReason != "" { + return m.mailWatchReason } - return mailChangedMsg{boxID: boxID} + return "Live updates unavailable" + default: + return "" } } @@ -107,12 +228,12 @@ type screenerChangedMsg struct { // screenerRefreshDueMsg is the re-read a Screener change asked for, once its delay has passed. type screenerRefreshDueMsg struct{} -func startScreenerWatchCmd(ctx context.Context, watch ScreenerWatcher, signedStreamName string) tea.Cmd { +func startScreenerWatchCmd(ctx, connectionCtx context.Context, watch ScreenerWatcher, signedStreamName string) tea.Cmd { if watch == nil || signedStreamName == "" { return nil } return func() tea.Msg { - changes, err := watch(ctx, signedStreamName) + changes, err := watch(ctx, connectionCtx, signedStreamName) return screenerWatchStartedMsg{stream: signedStreamName, changes: changes, err: err} } } diff --git a/internal/tui/live_test.go b/internal/tui/live_test.go index 24686d3c..c1437d70 100644 --- a/internal/tui/live_test.go +++ b/internal/tui/live_test.go @@ -11,54 +11,55 @@ import ( hey "github.com/basecamp/hey-sdk/go/pkg/hey" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/mail" ) // --- The changes stream --- -func TestWaitForMailChangeReportsTheChangedBox(t *testing.T) { - changes := make(chan int64, 1) - changes <- 7 +func TestWaitForMailWatchEventReportsTheChangedBox(t *testing.T) { + events := make(chan MailWatchEvent, 1) + events <- MailWatchEvent{BoxID: 7} - msg := waitForMailChangeCmd(changes)() - changed, ok := msg.(mailChangedMsg) + msg := waitForMailWatchEventCmd(events)() + changed, ok := msg.(mailWatchEventMsg) if !ok { - t.Fatalf("msg = %#v, want mailChangedMsg", msg) + t.Fatalf("msg = %#v, want mailWatchEventMsg", msg) } - if changed.boxID != 7 || changed.closed { + if changed.event.BoxID != 7 || changed.closed { t.Errorf("changed = %+v, want box 7 and an open stream", changed) } } -func TestWaitForMailChangeReportsAClosedStream(t *testing.T) { - changes := make(chan int64) - close(changes) +func TestWaitForMailWatchEventReportsAClosedStream(t *testing.T) { + events := make(chan MailWatchEvent) + close(events) - if changed := waitForMailChangeCmd(changes)().(mailChangedMsg); !changed.closed { + if changed := waitForMailWatchEventCmd(events)().(mailWatchEventMsg); !changed.closed { t.Errorf("changed = %+v, want a closed stream", changed) } - if cmd := waitForMailChangeCmd(nil); cmd != nil { + if cmd := waitForMailWatchEventCmd(nil); cmd != nil { t.Error("nothing to wait on should be no command") } } -func TestStartMailWatchCarriesTheStreamOrTheReason(t *testing.T) { - changes := make(chan int64) - opened := startMailWatchCmd(context.Background(), func(context.Context) (<-chan int64, error) { - return changes, nil - })().(mailWatchStartedMsg) - if opened.changes == nil || opened.err != nil { - t.Errorf("opened = %+v, want the stream", opened) +func TestStartMailWatchCarriesTheStreamAttemptOrReason(t *testing.T) { + events := make(chan MailWatchEvent) + opened := startMailWatchCmd(context.Background(), func(context.Context) (<-chan MailWatchEvent, error) { + return events, nil + }, 4)().(mailWatchStartedMsg) + if opened.events == nil || opened.err != nil || opened.attempt != 4 { + t.Errorf("opened = %+v, want attempt 4 and its stream", opened) } - refused := startMailWatchCmd(context.Background(), func(context.Context) (<-chan int64, error) { + refused := startMailWatchCmd(context.Background(), func(context.Context) (<-chan MailWatchEvent, error) { return nil, errors.New("cable server said no") - })().(mailWatchStartedMsg) - if refused.err == nil { - t.Error("a watcher that fails should say why") + }, 5)().(mailWatchStartedMsg) + if refused.err == nil || refused.attempt != 5 { + t.Errorf("refused = %+v, want attempt 5 and the reason it failed", refused) } - if cmd := startMailWatchCmd(context.Background(), nil); cmd != nil { + if cmd := startMailWatchCmd(context.Background(), nil, 1); cmd != nil { t.Error("no watcher should be no command") } } @@ -68,59 +69,131 @@ func TestStartMailWatchCarriesTheStreamOrTheReason(t *testing.T) { func TestModelListensForMailChanges(t *testing.T) { m := newModel() m.mailView.boxes = orderBoxes(testBoxes()) - changes := make(chan int64, 1) + events := make(chan MailWatchEvent, 1) - updated, cmd := m.Update(mailWatchStartedMsg{changes: changes}) + updated, cmd := m.Update(mailWatchStartedMsg{attempt: m.mailWatchAttempt, events: events}) m = updated.(model) - if m.mailChanges == nil { + if m.mailWatchEvents == nil { t.Fatal("the model should hold on to the stream") } if cmd == nil { - t.Fatal("the model should wait for the first change") + t.Fatal("the model should wait for the first event") } - updated, _ = m.Update(mailChangedMsg{boxID: m.mailView.currentBoxID()}) + updated, _ = m.Update(mailWatchEventMsg{event: MailWatchEvent{BoxID: m.mailView.currentBoxID()}}) m = updated.(model) if !m.mailView.liveRefreshDue { t.Error("a change to the box on screen should arm a re-read") } } -func TestModelSaysWhenLiveUpdatesStopped(t *testing.T) { +func TestModelShowsAStandingOfflineNoticeAndRetriesAClosedWatch(t *testing.T) { m := newModel() - m.mailView.boxes = orderBoxes(testBoxes()) - m.mailView.Update(currentPostingsLoaded(m.mailView, testPostings())) - m.mailChanges = make(chan int64) + m.width, m.height = 80, 30 + m.vc.width = 80 + m.watchMail = func(context.Context) (<-chan MailWatchEvent, error) { return nil, nil } + m.mailWatchEvents = make(chan MailWatchEvent) - updated, cmd := m.Update(mailChangedMsg{closed: true}) + updated, cmd := m.Update(mailWatchEventMsg{closed: true}) m = updated.(model) - if cmd != nil { - t.Error("a closed stream should not be waited on again") + if cmd == nil { + t.Fatal("a closed stream should schedule a new connection") } - if m.mailChanges != nil { + if m.mailWatchEvents != nil { t.Error("the closed stream should be let go of") } - if !strings.Contains(m.mailView.View(), "ctrl+r") { - t.Errorf("the list should say it stopped following the server:\n%s", m.mailView.View()) + if !strings.Contains(m.View().Content, "reconnecting to HEY") { + t.Errorf("the whole app should say live updates are reconnecting:\n%s", m.View().Content) } +} - if cmd := m.mailView.reloadPostings(); cmd == nil { - t.Fatal("ctrl+r should read the box again") +func TestModelShowsAndClearsATemporaryDisconnectAcrossSections(t *testing.T) { + m := newModel() + m.width, m.height = 80, 30 + m.vc.width, m.vc.height = 80, 20 + m.help.setHidden(true) + m.activeView = m.calendarView + m.mailWatchEvents = make(chan MailWatchEvent) + fullHeight := m.contentHeight() + + updated, cmd := m.Update(mailWatchEventMsg{event: MailWatchEvent{Connection: MailConnectionDisconnected, WillReconnect: true}}) + m = updated.(model) + if cmd == nil { + t.Fatal("the model should keep waiting while Action Cable reconnects") + } + if !strings.Contains(m.View().Content, "Live updates disconnected") { + t.Errorf("the Calendar should carry the global connection notice:\n%s", m.View().Content) } - if strings.Contains(m.mailView.View(), "Not live") { - t.Error("a reload should take the standing word back down") + if m.contentHeight() != fullHeight-1 { + t.Errorf("content height = %d, want one row held for status from %d", m.contentHeight(), fullHeight) + } + + updated, cmd = m.Update(mailWatchEventMsg{event: MailWatchEvent{Connection: MailConnectionReconnected}}) + m = updated.(model) + if cmd == nil { + t.Fatal("a reconnect should wait for more events and catch mail up") + } + if strings.Contains(m.View().Content, "Live updates disconnected") { + t.Errorf("the reconnect should clear the notice:\n%s", m.View().Content) + } + if m.contentHeight() != fullHeight { + t.Errorf("content height = %d, want the status row returned", m.contentHeight()) } } func TestModelReportsAWatcherThatNeverStarted(t *testing.T) { m := newModel() + m.width, m.height = 80, 30 m.vc.width = 80 m.mailView.boxes = orderBoxes(testBoxes()) - updated, _ := m.Update(mailWatchStartedMsg{err: errors.New("cable server said no")}) + updated, cmd := m.Update(mailWatchStartedMsg{attempt: m.mailWatchAttempt, err: errors.New("\x1b[31mcable server said no")}) + m = updated.(model) + if cmd != nil { + t.Error("an unclassified refusal should not enter a reconnect loop") + } + view := m.View().Content + if !strings.Contains(view, "cable server said no") { + t.Errorf("view = %q, want the reason there are no live updates", view) + } + if strings.Contains(m.mailWatchNotice(), "\x1b") { + t.Errorf("notice = %q, want the cable error sanitized", m.mailWatchNotice()) + } +} + +func TestModelRetriesANetworkFailureButNotAStaleTimer(t *testing.T) { + m := newModel() + m.watchMail = func(context.Context) (<-chan MailWatchEvent, error) { return nil, nil } + network := apierr.ErrNetwork(errors.New("no route to host")) + + updated, cmd := m.Update(mailWatchStartedMsg{attempt: m.mailWatchAttempt, err: network}) m = updated.(model) - if !strings.Contains(m.mailView.notice, "cable server said no") { - t.Errorf("notice = %q, want the reason there are no live updates", m.mailView.notice) + if cmd == nil || m.mailWatchFailures != 1 { + t.Fatal("a network failure should schedule its first retry") + } + if notice := m.mailWatchNotice(); !strings.Contains(notice, "Offline") { + t.Errorf("notice = %q, want the network failure identified as offline", notice) + } + if delay := mailWatchRetryDelay(10); delay != mailWatchMaximumRetry { + t.Errorf("retry delay = %s, want it capped at %s", delay, mailWatchMaximumRetry) + } + + m.mailWatchEvents = make(chan MailWatchEvent) + _, cmd = m.Update(mailWatchRetryMsg{attempt: m.mailWatchAttempt}) + if cmd != nil { + t.Error("a timer left behind after connecting must not replace the live stream") + } + + m.mailWatchEvents = nil + attempt := m.mailWatchAttempt + updated, cmd = m.Update(mailWatchRetryMsg{attempt: attempt}) + m = updated.(model) + if cmd == nil || m.mailWatchAttempt != attempt+1 { + t.Fatal("the current retry timer should open the next watch attempt") + } + started := cmd().(mailWatchStartedMsg) + if started.attempt != attempt+1 { + t.Errorf("started attempt = %d, want %d", started.attempt, attempt+1) } } @@ -347,7 +420,7 @@ func TestWaitForScreenerChangeReportsTheRingAndTheClose(t *testing.T) { func TestModelOpensTheScreenerStreamOnlyWhenItsNameChanges(t *testing.T) { m := newModel() - m.watchScreener = func(context.Context, string) (<-chan struct{}, error) { + m.watchScreener = func(context.Context, context.Context, string) (<-chan struct{}, error) { return make(chan struct{}), nil } @@ -367,9 +440,10 @@ func TestModelOpensTheScreenerStreamOnlyWhenItsNameChanges(t *testing.T) { func TestModelGivesUpTheOldScreenerStreamWhenItOpensANewOne(t *testing.T) { m := newModel() - var opened []context.Context - m.watchScreener = func(ctx context.Context, _ string) (<-chan struct{}, error) { + var opened, owners []context.Context + m.watchScreener = func(ctx, owner context.Context, _ string) (<-chan struct{}, error) { opened = append(opened, ctx) + owners = append(owners, owner) return make(chan struct{}), nil } @@ -385,12 +459,15 @@ func TestModelGivesUpTheOldScreenerStreamWhenItOpensANewOne(t *testing.T) { if opened[1].Err() != nil { t.Error("the stream just opened should still be open") } + if len(owners) != 2 || owners[0] != m.watchCtx || owners[1] != m.watchCtx || m.watchCtx.Err() != nil { + t.Error("replacing a Screener stream must leave the TUI-wide connection context alive") + } } func TestModelReopensTheScreenerStreamFromACountRead(t *testing.T) { m := newModel() opened := make(chan string, 2) - m.watchScreener = func(_ context.Context, signedStreamName string) (<-chan struct{}, error) { + m.watchScreener = func(_, _ context.Context, signedStreamName string) (<-chan struct{}, error) { opened <- signedStreamName return make(chan struct{}), nil } @@ -428,7 +505,7 @@ func TestMailViewReadsTheScreenerCountWithItsStreamName(t *testing.T) { func TestModelGivesUpTheScreenerStreamOnAMailAccountSwitch(t *testing.T) { m := newModel() var opened []context.Context - m.watchScreener = func(ctx context.Context, _ string) (<-chan struct{}, error) { + m.watchScreener = func(ctx, _ context.Context, _ string) (<-chan struct{}, error) { opened = append(opened, ctx) return make(chan struct{}), nil } @@ -465,7 +542,7 @@ func TestModelSaysWhenTheScreenerStreamCloses(t *testing.T) { func TestModelIgnoresTheCloseOfAScreenerStreamItGaveUp(t *testing.T) { m := newModel() - m.watchScreener = func(context.Context, string) (<-chan struct{}, error) { + m.watchScreener = func(context.Context, context.Context, string) (<-chan struct{}, error) { return make(chan struct{}), nil } runCmd(m.startScreenerWatch("stream-one")) diff --git a/internal/tui/mail.go b/internal/tui/mail.go index 7a36fd0f..639166f0 100644 --- a/internal/tui/mail.go +++ b/internal/tui/mail.go @@ -229,11 +229,10 @@ type mailView struct { folderDiscoveryErr string collectionDiscoveryErr string - liveRequestID uint64 // identifies the only live re-read allowed to update the list - liveRefreshDue bool // a re-read is already on its way - liveUpdatesOver bool // the changes stream closed, so the list is a snapshot again - moreRequestID uint64 // identifies the only page-below read allowed to grow the list - searchMoreID uint64 // the same, for the search results + liveRequestID uint64 // identifies the only live re-read allowed to update the list + liveRefreshDue bool // a re-read is already on its way + moreRequestID uint64 // identifies the only page-below read allowed to grow the list + searchMoreID uint64 // the same, for the search results } func newMailView(vc *viewContext) *mailView { @@ -694,16 +693,14 @@ func (v *mailView) openModal(open modal) { open.resize(v.vc.width, v.vc.height) } -// listHeader carries the one-shot notice, the standing word that the list has stopped -// following the server, and the Screener's standing invitation above the posting list. +// listHeader carries the one-shot notice and The Screener's standing invitation above +// the posting list. Connection status belongs to the app header, where every section +// can see it. func (v *mailView) listHeader() string { var lines []string if v.notice != "" { lines = append(lines, v.vc.styles.title.Render(v.notice)) } - if v.liveUpdatesOver && !isOrganizedMailSource(v.currentSourceKind()) { - lines = append(lines, v.vc.styles.title.Render("Not live any more — press ctrl+r to reload")) - } if hint := v.screenerHint(); hint != "" { lines = append(lines, centerText(v.vc.styles.pill.Render(hint), v.vc.width), "") } @@ -1314,14 +1311,12 @@ func (v *mailView) loadMorePostings() tea.Cmd { return v.fetchMorePostings(v.vc.ctx, v.moreRequestID, *source, v.postingPaging.nextPage) } -// reloadPostings reads the box on screen again, on the user's say-so. It is how a list -// that stopped being live is caught up, and how anything else is put right. +// reloadPostings reads the box on screen again, on the user's say-so. func (v *mailView) reloadPostings() tea.Cmd { source := v.currentSource() if source == nil { return nil } - v.liveUpdatesOver = false return v.requestPostings(*source) } @@ -1363,19 +1358,6 @@ func (v *mailView) showsBox(boxID int64) bool { return boxID == AnyBoxChanged || boxID == source.ID } -// liveUpdatesStopped stands above the list until the box is read again: what was live is -// a snapshot now, and a reader has no other way of telling. -func (v *mailView) liveUpdatesStopped() { - v.liveUpdatesOver = true - v.liveRefreshDue = false -} - -// liveUpdatesUnavailable is the other way it goes wrong: there was never a stream to -// begin with, which is worth saying once rather than standing over the list forever. -func (v *mailView) liveUpdatesUnavailable(err error) { - v.noteFailure("Live updates unavailable", err) -} - // screenerUpdatesUnavailable is said in the mail list because that is where The Screener // announces itself — the count above the threads is what stops keeping up. func (v *mailView) screenerUpdatesUnavailable(err error) { diff --git a/internal/tui/screener_test.go b/internal/tui/screener_test.go index 3bc2729c..8f8ef560 100644 --- a/internal/tui/screener_test.go +++ b/internal/tui/screener_test.go @@ -268,6 +268,11 @@ func TestScreenerNoticesSayWhatTheCLISays(t *testing.T) { err: &hey.Error{Code: hey.CodeAuth, Message: "not authenticated", HTTPStatus: 401}, want: "Could not screen Jane Doe: not authenticated", }, + { + name: "a classified error keeps the CLI's message", + err: apierr.ErrAuth("the cable server rejected these credentials"), + want: "Could not screen Jane Doe: the cable server rejected these credentials", + }, { name: "an error that never went near the API keeps its own text", err: errors.New("no route to host"), diff --git a/internal/tui/tui.go b/internal/tui/tui.go index e94e1252..588c7408 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -58,10 +58,14 @@ type model struct { // Live mail updates: the watcher, the stream it opened, and the context that keeps // the stream open. It outlives the view context, which a mail account switch throws // away — the changes stream is the same one whichever account is being read. - watchMail MailWatcher - mailChanges <-chan int64 - watchCtx context.Context - stopWatching context.CancelFunc + watchMail MailWatcher + mailWatchEvents <-chan MailWatchEvent + mailWatchAttempt uint64 + mailWatchFailures int + mailWatchStatus mailWatchStatus + mailWatchReason string + watchCtx context.Context + stopWatching context.CancelFunc // The Screener's stream, which can only be opened once HEY has served its name, and // the context that holds it open. Cancelling that context is how the stream behind an @@ -134,6 +138,7 @@ func newModelWithMailAccounts(rootSDK, sdk *hey.Client, selected string, watcher screenerView: sv, watchMail: watchers.Mail, watchScreener: watchers.Screener, + mailWatchAttempt: 1, watchCtx: watchCtx, stopWatching: stopWatching, mailAccounts: []mailAccountChoice{{label: "All Accounts"}}, @@ -170,7 +175,7 @@ func (m model) Init() tea.Cmd { spinnerTick(), tea.RequestBackgroundColor, watchThemeCmd(omarchyWatchDir(userHomeDir())), - startMailWatchCmd(m.watchCtx, m.watchMail), + startMailWatchCmd(m.watchCtx, m.watchMail, m.mailWatchAttempt), ) } @@ -291,23 +296,55 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.closeScreener() case mailWatchStartedMsg: - if msg.err != nil { - m.mailView.liveUpdatesUnavailable(msg.err) + if msg.attempt != m.mailWatchAttempt { return m, nil } - m.mailChanges = msg.changes - return m, waitForMailChangeCmd(m.mailChanges) + if msg.err != nil { + cmd := m.mailWatchFailed(msg.err) + m.updateHelpBindings() + return m, cmd + } + catchUp := m.mailWatchStatus != mailWatchLive || m.mailWatchFailures > 0 + m.mailWatchEvents = msg.events + m.mailWatchConnected() + m.updateHelpBindings() + wait := waitForMailWatchEventCmd(m.mailWatchEvents) + if catchUp { + return m, tea.Batch(m.stampViewCmd(m.mailView.boxChanged(AnyBoxChanged)), wait) + } + return m, wait - case mailChangedMsg: + case mailWatchEventMsg: if msg.closed { - m.mailChanges = nil - m.mailView.liveUpdatesStopped() - return m, nil + m.mailWatchEvents = nil + cmd := m.mailWatchStopped() + m.updateHelpBindings() + return m, cmd + } + wait := waitForMailWatchEventCmd(m.mailWatchEvents) + switch msg.event.Connection { + case MailConnectionUnchanged: + // The box event below is the ordinary live-update path. + case MailConnectionDisconnected: + m.mailWatchDisconnected(msg.event.WillReconnect) + m.updateHelpBindings() + return m, wait + case MailConnectionReconnected: + m.mailWatchConnected() + m.updateHelpBindings() + return m, tea.Batch(m.stampViewCmd(m.mailView.boxChanged(AnyBoxChanged)), wait) } // The stream is listened to for as long as the TUI runs, whichever section is // on screen: mail that arrived while the user was in the calendar is there when // they come back. - return m, tea.Batch(m.stampViewCmd(m.mailView.boxChanged(msg.boxID)), waitForMailChangeCmd(m.mailChanges)) + return m, tea.Batch(m.stampViewCmd(m.mailView.boxChanged(msg.event.BoxID)), wait) + + case mailWatchRetryMsg: + if msg.attempt != m.mailWatchAttempt || m.mailWatchEvents != nil { + return m, nil + } + m.mailWatchAttempt++ + return m, startMailWatchCmd(m.watchCtx, m.watchMail, m.mailWatchAttempt) case mailRefreshDueMsg, postingsRefreshedMsg: cmd, _ := m.mailView.Update(msg) @@ -506,6 +543,10 @@ func (m model) View() tea.View { b.WriteString(renderHeader(&m)) b.WriteString("\n") + if notice := m.mailWatchNotice(); notice != "" { + b.WriteString(m.styles.title.Render(truncateStr(notice, max(m.width, 1)))) + b.WriteString("\n") + } content := m.contentView() if m.mailAccountPicker { @@ -620,7 +661,11 @@ func (m model) contentHeight() int { if helpHeight := m.help.height(); helpHeight > 0 { footerHeight = helpHeight + 3 } - return max(m.height-headerHeight-footerHeight, 1) + statusHeight := 0 + if m.mailWatchNotice() != "" { + statusHeight = 1 + } + return max(m.height-headerHeight-footerHeight-statusHeight, 1) } // --- Key handling --- @@ -870,7 +915,7 @@ func (m *model) startScreenerWatch(signedStreamName string) tea.Cmd { watchCtx, stop := context.WithCancel(m.watchCtx) //nolint:gosec // G118: cancel stored, called on the next watch, an account switch or ctrl+c m.screenerStream = signedStreamName m.stopScreenerWatch = stop - return startScreenerWatchCmd(watchCtx, m.watchScreener, signedStreamName) + return startScreenerWatchCmd(watchCtx, m.watchCtx, m.watchScreener, signedStreamName) } // dropScreenerWatch gives up the stream the TUI was following. The subscription behind it From cb8fea86f9653322efc00318dedf316e16c05bba Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Sat, 22 Aug 2026 06:49:48 -0400 Subject: [PATCH 2/3] Coalesce busy mail watches --- internal/cmd/tui_watch.go | 23 +++++++++++++++++++++++ internal/cmd/tui_watch_test.go | 8 ++++++++ 2 files changed, 31 insertions(+) diff --git a/internal/cmd/tui_watch.go b/internal/cmd/tui_watch.go index 8341c1db..f3cea220 100644 --- a/internal/cmd/tui_watch.go +++ b/internal/cmd/tui_watch.go @@ -197,8 +197,31 @@ func ringMailWatchEvent(events chan tui.MailWatchEvent, event tui.MailWatchEvent if event.Connection == tui.MailConnectionUnchanged { select { case events <- event: + return default: } + + // A full queue may contain changes for boxes other than the one on screen. + // Replace its box-specific doorbells with one catch-all rather than dropping + // this box and assuming another event will happen to refresh it. Keep a queued + // connection transition ahead of that catch-up. + var connection *tui.MailWatchEvent + boxBacklog: + for { + select { + case queued := <-events: + if queued.Connection != tui.MailConnectionUnchanged { + latest := queued + connection = &latest + } + default: + break boxBacklog + } + } + if connection != nil { + ring(events, *connection) + } + ring(events, tui.MailWatchEvent{BoxID: tui.AnyBoxChanged}) return } diff --git a/internal/cmd/tui_watch_test.go b/internal/cmd/tui_watch_test.go index 6a57a18b..4b03b9ce 100644 --- a/internal/cmd/tui_watch_test.go +++ b/internal/cmd/tui_watch_test.go @@ -109,7 +109,15 @@ func TestMailConnectionStateTakesPriorityOverAFullBoxBacklog(t *testing.T) { ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 1}) ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 2}) ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 3}) + if len(events) != 1 { + t.Fatalf("events = %d, want a full box backlog coalesced", len(events)) + } + if got := <-events; got.BoxID != tui.AnyBoxChanged || got.Connection != tui.MailConnectionUnchanged { + t.Errorf("event = %+v, want a catch-all so the box on screen cannot stay stale", got) + } + ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 1}) + ringMailWatchEvent(events, tui.MailWatchEvent{BoxID: 2}) ringMailWatchEvent(events, tui.MailWatchEvent{Connection: tui.MailConnectionDisconnected, WillReconnect: true}) if len(events) != 1 { t.Fatalf("events = %d, want stale box doorbells coalesced behind connection state", len(events)) From a6c6c45d637409b3cc1b0426e854e0f0dbe1ee78 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Sat, 22 Aug 2026 16:50:20 -0400 Subject: [PATCH 3/3] Recover every stopped TUI cable client --- AGENTS.md | 9 +++--- internal/cmd/tui_watch.go | 43 ++++++++++++++++---------- internal/cmd/tui_watch_test.go | 56 +++++++++++++++++++++++++--------- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 88ee01d5..b248c53c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -632,10 +632,11 @@ mail list — and held while a decision is in flight or the clear-everything que On the history tab nothing is read; the pending pane is just marked unloaded, which is what `switchTab` already looks at. Both watches share one websocket (`tuiCableClient` in `internal/cmd/tui_watch.go`): two subscriptions, one authorization. -A client that stopped itself never dials again: depending on how it stopped, `Subscribe` -answers `ErrClosed` or preserves the server's non-reconnecting `DisconnectError`. -`tuiSubscribe` recognizes both, drops that client and dials a fresh one rather than handing -a reopened watch a dead connection. Mail doorbells are coalesced when their bounded queue +A client that stopped itself preserves its terminal failure and never dials again. +`tuiSubscribe` detects that state after any failed subscription, drops the stopped client, +and dials a fresh one; a replacement that also stops is dropped and its failure is +classified as authentication or network so the TUI can respond consistently. Mail +doorbells are coalesced when their bounded queue is full, while a connection transition drains those stale doorbells and takes priority; the reconnect catch-up reads the current state once. diff --git a/internal/cmd/tui_watch.go b/internal/cmd/tui_watch.go index f3cea220..4bbdf0a8 100644 --- a/internal/cmd/tui_watch.go +++ b/internal/cmd/tui_watch.go @@ -251,33 +251,44 @@ func ring[T any](notifications chan<- T, notification T) { } // tuiSubscribe subscribes over the connection the TUI's watches share, dialling a new one -// when the one on hand has stopped itself. A client the server hung up on for good answers -// every Subscribe with ErrClosed and never dials again on its own, so a Screener stream -// reopened after that would find a dead connection and stay dead. Each dial carries -// current credentials, which is what makes redialling worth doing. +// when the one on hand has stopped itself. A stopped client preserves its terminal failure +// and never dials again on its own, so a reopened stream replaces it with a connection that +// carries current credentials. func tuiSubscribe(ctx, connectionCtx context.Context, identifier actioncable.Identifier, options ...actioncable.SubscriptionOption) (*actioncable.Subscription, error) { client, err := tuiCableClient(connectionCtx) if err != nil { return nil, err } - subscription, err := client.Subscribe(ctx, identifier, options...) - if stoppedTuiCableError(err) { - forgetTuiCable(client) - if client, err = tuiCableClient(connectionCtx); err != nil { - return nil, err - } - subscription, err = client.Subscribe(ctx, identifier, options...) + subscription, stopped, err := subscribeTuiCable(ctx, client, identifier, options...) + if !stopped { + return subscription, err } + + client, err = tuiCableClient(connectionCtx) + if err != nil { + return nil, err + } + subscription, _, err = subscribeTuiCable(ctx, client, identifier, options...) return subscription, err } -func stoppedTuiCableError(err error) bool { - if errors.Is(err, actioncable.ErrClosed) { - return true +// subscribeTuiCable returns stopped when the shared client needs replacing. Every shared +// client has connected before it is cached, so Connect reports ErrAlreadyConnected while +// it is live or reconnecting and preserves the terminal failure after it stops. +func subscribeTuiCable(ctx context.Context, client *actioncable.Client, identifier actioncable.Identifier, options ...actioncable.SubscriptionOption) (*actioncable.Subscription, bool, error) { + subscription, err := client.Subscribe(ctx, identifier, options...) + if err == nil { + return subscription, false, nil } - var disconnected *actioncable.DisconnectError - return errors.As(err, &disconnected) && !disconnected.Reconnect + + stoppedBecause := client.Connect(ctx) + if errors.Is(stoppedBecause, actioncable.ErrAlreadyConnected) { + return nil, false, err + } + + forgetTuiCable(client) + return nil, true, watchDialError(stoppedBecause) } // tuiCable is the one connection the TUI's watches share — two subscriptions over one diff --git a/internal/cmd/tui_watch_test.go b/internal/cmd/tui_watch_test.go index 4b03b9ce..c83d0662 100644 --- a/internal/cmd/tui_watch_test.go +++ b/internal/cmd/tui_watch_test.go @@ -11,6 +11,7 @@ import ( actioncable "github.com/basecamp/actioncable-go" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" "github.com/basecamp/hey-cli/internal/config" "github.com/basecamp/hey-cli/internal/tui" @@ -134,16 +135,21 @@ func (t scriptedCableTransport) Dial(context.Context, string, actioncable.DialOp } type scriptedCableConn struct { - reads chan []byte - done chan struct{} - once sync.Once + reads chan []byte + done chan struct{} + once sync.Once + subprotocol string } func newScriptedCableConn() *scriptedCableConn { - return &scriptedCableConn{reads: make(chan []byte, 2), done: make(chan struct{})} + return &scriptedCableConn{ + reads: make(chan []byte, 2), + done: make(chan struct{}), + subprotocol: actioncable.SubprotocolV1JSON, + } } -func (c *scriptedCableConn) Subprotocol() string { return "actioncable-v1-json" } +func (c *scriptedCableConn) Subprotocol() string { return c.subprotocol } func (c *scriptedCableConn) Read(ctx context.Context) ([]byte, error) { select { @@ -163,15 +169,26 @@ func (c *scriptedCableConn) Close() error { return nil } -func TestStoppedTuiCableErrorsIncludeTerminalServerDisconnects(t *testing.T) { - if !stoppedTuiCableError(actioncable.ErrClosed) { - t.Error("an explicitly closed client should be replaced") +func TestSubscribeTuiCableRecognizesAnUnenumeratedTerminalFailure(t *testing.T) { + conn := newScriptedCableConn() + conn.subprotocol = "actioncable-v9-telepathy" + client := actioncable.New("ws://cable.example.test/cable", actioncable.WithTransport(scriptedCableTransport{conn: conn})) + if err := client.Connect(t.Context()); !errors.Is(err, actioncable.ErrUnsupportedSubprotocol) { + t.Fatalf("connect error = %v, want the unsupported protocol to stop the client", err) } - if !stoppedTuiCableError(&actioncable.DisconnectError{Reason: actioncable.ReasonUnauthorized, Reconnect: false}) { - t.Error("a client the server stopped for good should be replaced") + tuiCable.client = client + t.Cleanup(func() { tuiCable.client = nil }) + + _, stopped, err := subscribeTuiCable(t.Context(), client, actioncable.Identifier{Channel: changesChannel}) + if !stopped { + t.Fatal("a client stopped by an unenumerated terminal failure should be replaced") } - if stoppedTuiCableError(&actioncable.DisconnectError{Reason: "server restart", Reconnect: true}) { - t.Error("a client already reconnecting should stay shared") + var known *apierr.Error + if !errors.As(err, &known) || known.Code != apierr.CodeNetwork { + t.Errorf("error = %T %v, want a retryable network error", err, err) + } + if tuiCable.client != nil { + t.Error("a stopped client should not remain cached") } } @@ -183,13 +200,22 @@ func TestServerStoppedActionCableClientIsReplaceable(t *testing.T) { t.Fatalf("connect: %v", err) } defer client.Close() + tuiCable.client = client + t.Cleanup(func() { tuiCable.client = nil }) conn.reads <- []byte(`{"type":"disconnect","reason":"unauthorized","reconnect":false}`) subscribing, stop := context.WithTimeout(t.Context(), time.Second) defer stop() - _, err := client.Subscribe(subscribing, actioncable.Identifier{Channel: changesChannel}) - if !stoppedTuiCableError(err) { - t.Fatalf("subscribe error = %T %v, want a terminal server disconnect that opens a new client", err, err) + _, stopped, err := subscribeTuiCable(subscribing, client, actioncable.Identifier{Channel: changesChannel}) + if !stopped { + t.Fatalf("subscribe error = %T %v, want the terminal server disconnect to replace the client", err, err) + } + var known *apierr.Error + if !errors.As(err, &known) || known.Code != apierr.CodeAuth { + t.Errorf("error = %T %v, want an authentication error", err, err) + } + if tuiCable.client != nil { + t.Error("a client stopped by the server should not remain cached") } }