From 634c3ee03c94e89d965391e17aeb15e15162b299 Mon Sep 17 00:00:00 2001 From: Tyler Koenig Date: Thu, 11 Jun 2026 11:35:03 -0400 Subject: [PATCH] fix(tui): finish moving keypress DB reads into tea.Cmds The #101 refactor stopped at the tick path; 'h' history and the SLA view still queried state changes synchronously in Update, freezing the UI for up to busy_timeout on a contended DB. Both now load through Cmds with loading placeholders. Also closes the remaining staleness holes in the async data flow: - tabDataMsg carries a sequence number; out-of-order replies from slower earlier loads are dropped instead of overwriting newer data - history/SLA replies are dropped when the user has navigated to a different site or period - the open detail panel refreshes on the tab-data cadence instead of loading once on entry and going stale - initSiteHuhForm reads the m.alerts cache instead of hitting the store --- internal/tui/data.go | 45 ++++++++++++--- internal/tui/messages.go | 25 +++++++- internal/tui/tab_sites.go | 13 ++--- internal/tui/tui.go | 2 + internal/tui/update.go | 83 ++++++++++++++++++++------ internal/tui/update_test.go | 112 ++++++++++++++++++++++++++++++++++++ 6 files changed, 245 insertions(+), 35 deletions(-) diff --git a/internal/tui/data.go b/internal/tui/data.go index aa0b68c..ad8038e 100644 --- a/internal/tui/data.go +++ b/internal/tui/data.go @@ -4,6 +4,7 @@ import ( "encoding/json" "sort" "strings" + "time" "gitea.lerkolabs.com/lerkolabs/uptop/internal/models" "gitea.lerkolabs.com/lerkolabs/uptop/internal/store" @@ -108,32 +109,36 @@ func (m *Model) clampCursor() { } // loadTabDataCmd returns a tea.Cmd that loads the DB-backed tab tables off the -// UI goroutine. The closure reads only stable fields (store, isAdmin) and never -// mutates the model; results come back as a tabDataMsg. On the first store -// error it returns an error-only msg so the model keeps its previous data. +// UI goroutine. Each call bumps tabSeq and stamps the reply with it, so +// handleTabData can drop out-of-order results from slower earlier loads. The +// closure reads only stable fields (store, isAdmin) and never mutates the +// model; results come back as a tabDataMsg. On the first store error it +// returns an error-only msg so the model keeps its previous data. func (m *Model) loadTabDataCmd() tea.Cmd { + m.tabSeq++ + seq := m.tabSeq st := m.store isAdmin := m.isAdmin return func() tea.Msg { alerts, err := st.GetAllAlerts() if err != nil { - return tabDataMsg{err: err} + return tabDataMsg{seq: seq, err: err} } var users []models.User if isAdmin { if users, err = st.GetAllUsers(); err != nil { - return tabDataMsg{err: err} + return tabDataMsg{seq: seq, err: err} } } nodes, err := st.GetAllNodes() if err != nil { - return tabDataMsg{err: err} + return tabDataMsg{seq: seq, err: err} } maint, err := st.GetAllMaintenanceWindows(100) if err != nil { - return tabDataMsg{err: err} + return tabDataMsg{seq: seq, err: err} } - return tabDataMsg{alerts: alerts, users: users, nodes: nodes, maint: maint} + return tabDataMsg{seq: seq, alerts: alerts, users: users, nodes: nodes, maint: maint} } } @@ -145,3 +150,27 @@ func (m *Model) loadDetailCmd(siteID int) tea.Cmd { return detailDataMsg{siteID: siteID, changes: eng.GetStateChanges(siteID, 5)} } } + +// loadHistoryCmd loads the full state-change history for the history view off +// the UI goroutine. +func (m *Model) loadHistoryCmd(siteID int) tea.Cmd { + eng := m.engine + return func() tea.Msg { + return historyDataMsg{siteID: siteID, changes: eng.GetStateChanges(siteID, 100)} + } +} + +// loadSLACmd loads the state changes backing the SLA view off the UI +// goroutine. The reply carries the request's site and period so a stale reply +// can be recognized and dropped. +func (m *Model) loadSLACmd(siteID, periodIdx int) tea.Cmd { + eng := m.engine + since := time.Now().Add(-slaPeriods[periodIdx].duration) + return func() tea.Msg { + return slaDataMsg{ + siteID: siteID, + periodIdx: periodIdx, + changes: eng.GetStateChangesSince(siteID, since), + } + } +} diff --git a/internal/tui/messages.go b/internal/tui/messages.go index 6ba2eff..59db852 100644 --- a/internal/tui/messages.go +++ b/internal/tui/messages.go @@ -18,8 +18,11 @@ type tickMsg time.Time // tabDataMsg carries the result of an async load of the DB-backed tab tables. // On err, the model keeps its previous data and logs — never wiping the view on -// a transient store error. +// a transient store error. seq orders in-flight loads: replies whose seq is +// older than the model's current tabSeq are dropped, so a slow load can never +// overwrite the result of a newer one. type tabDataMsg struct { + seq int alerts []models.AlertConfig users []models.User nodes []models.ProbeNode @@ -28,8 +31,26 @@ type tabDataMsg struct { } // detailDataMsg carries the state-change history for the detail panel, loaded -// when the panel is opened so View never touches the database. +// on entry and refreshed on the tab-data cadence so View never touches the +// database. type detailDataMsg struct { siteID int changes []models.StateChange } + +// historyDataMsg carries the full state-change history for the history view. +// siteID guards against a slow reply landing after the user opened a +// different site's history. +type historyDataMsg struct { + siteID int + changes []models.StateChange +} + +// slaDataMsg carries the state changes backing the SLA view for one +// site+period request. siteID and periodIdx guard stale replies the same way +// historyDataMsg does. +type slaDataMsg struct { + siteID int + periodIdx int + changes []models.StateChange +} diff --git a/internal/tui/tab_sites.go b/internal/tui/tab_sites.go index af9838e..5cb5d89 100644 --- a/internal/tui/tab_sites.go +++ b/internal/tui/tab_sites.go @@ -325,14 +325,13 @@ func (m *Model) initSiteHuhForm() tea.Cmd { } } + // m.alerts is the tab-data cache (≤5s stale) — no store IO in Update. alertOpts := []huh.Option[string]{huh.NewOption("None", "0")} - if alerts, err := m.store.GetAllAlerts(); err == nil { - for _, a := range alerts { - alertOpts = append(alertOpts, huh.NewOption( - fmt.Sprintf("%s (%s)", a.Name, a.Type), - strconv.Itoa(a.ID), - )) - } + for _, a := range m.alerts { + alertOpts = append(alertOpts, huh.NewOption( + fmt.Sprintf("%s (%s)", a.Name, a.Type), + strconv.Itoa(a.ID), + )) } groupOpts := []huh.Option[string]{huh.NewOption("None", "0")} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 8b5d4f3..4644bc0 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -121,6 +121,7 @@ type Model struct { historyViewport viewport.Model historyChanges []models.StateChange historySiteName string + historySiteID int slaViewport viewport.Model slaReport monitor.SLAReport @@ -155,6 +156,7 @@ type Model struct { nodes []models.ProbeNode maintenanceWindows []models.MaintenanceWindow lastTabLoad time.Time // last dispatch of loadTabDataCmd (throttle) + tabSeq int // seq of the newest issued tab-data load // detail-panel state-change history, loaded on enter so View does no DB IO detailChanges []models.StateChange diff --git a/internal/tui/update.go b/internal/tui/update.go index 089971a..8dd8277 100644 --- a/internal/tui/update.go +++ b/internal/tui/update.go @@ -20,9 +20,24 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case tabDataMsg: return m.handleTabData(msg) case detailDataMsg: + // Drop replies for a site the user has already navigated away from, + // so a slow load can't clobber the panel currently on screen. + if m.state == stateDetail && m.cursor < len(m.sites) && m.sites[m.cursor].ID != msg.siteID { + return m, nil + } m.detailChanges = msg.changes m.detailChangesSiteID = msg.siteID return m, nil + case historyDataMsg: + if msg.siteID != m.historySiteID { + return m, nil // stale reply for a previously opened history + } + m.historyChanges = msg.changes + m.historyViewport.SetContent(m.buildHistoryContent()) + m.historyViewport.GotoTop() + return m, nil + case slaDataMsg: + return m.handleSLAData(msg) } if m.state == stateConfirmDelete { @@ -152,14 +167,31 @@ func (m *Model) handleTick(t time.Time) (tea.Model, tea.Cmd) { if t.Sub(m.lastTabLoad) > tabRefreshTTL { m.lastTabLoad = t cmds = append(cmds, m.loadTabDataCmd()) + if dc := m.detailRefreshCmd(); dc != nil { + cmds = append(cmds, dc) + } } return m, tea.Batch(cmds...) } -// handleTabData folds an async tab-data load into the model. On error the -// previous data is kept and the failure logged, so a transient store error -// never blanks the view. +// detailRefreshCmd reloads the open detail panel's state-change list on the +// tab-data cadence, so a flap that happens while the panel is on screen shows +// up without leaving and re-entering. Nil when no detail panel is open. +func (m *Model) detailRefreshCmd() tea.Cmd { + if m.state != stateDetail || m.cursor >= len(m.sites) { + return nil + } + return m.loadDetailCmd(m.sites[m.cursor].ID) +} + +// handleTabData folds an async tab-data load into the model. Replies older +// than the newest issued load are dropped so out-of-order completions can't +// overwrite fresher data. On error the previous data is kept and the failure +// logged, so a transient store error never blanks the view. func (m *Model) handleTabData(msg tabDataMsg) (tea.Model, tea.Cmd) { + if msg.seq != m.tabSeq { + return m, nil + } if msg.err != nil { m.engine.AddLog("Tab data refresh failed: " + msg.err.Error()) return m, nil @@ -324,18 +356,19 @@ func (m *Model) handleDetailKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { if m.cursor < len(m.sites) { site := m.sites[m.cursor] m.historySiteName = site.Name - m.historyChanges = m.engine.GetStateChanges(site.ID, 100) + m.historySiteID = site.ID + m.historyChanges = nil m.historyViewport = viewport.New( m.termWidth-chromePadH, m.termHeight-10, ) - m.historyViewport.SetContent(m.buildHistoryContent()) - m.historyViewport.GotoTop() + m.historyViewport.SetContent("\n Loading state history...") m.state = stateHistory + return m, m.loadHistoryCmd(site.ID) } case "s": if m.cursor < len(m.sites) { - m.openSLAView(m.sites[m.cursor]) + return m, m.openSLAView(m.sites[m.cursor]) } case "q": return m, tea.Quit @@ -375,7 +408,7 @@ func (m *Model) handleSLAKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { idx := int(msg.String()[0]-'0') - 1 if idx >= 0 && idx < len(slaPeriods) { m.slaPeriodIdx = idx - m.recomputeSLA() + return m, m.loadSLACmd(m.slaSiteID, idx) } case "up", "k": m.slaViewport.ScrollUp(1) @@ -391,26 +424,39 @@ func (m *Model) handleSLAKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } -func (m *Model) openSLAView(site models.Site) { +func (m *Model) openSLAView(site models.Site) tea.Cmd { m.slaSiteName = site.Name m.slaSiteID = site.ID m.slaPeriodIdx = 2 // default 30d - m.recomputeSLA() + m.slaViewport = viewport.New( + m.termWidth-chromePadH, + m.termHeight-16, + ) + m.slaViewport.SetContent("\n Loading SLA report...") m.state = stateSLA + return m.loadSLACmd(site.ID, m.slaPeriodIdx) } -func (m *Model) recomputeSLA() { - period := slaPeriods[m.slaPeriodIdx] - since := time.Now().Add(-period.duration) - changes := m.engine.GetStateChangesSince(m.slaSiteID, since) +// handleSLAData folds an async SLA load into the model. The SLA math itself is +// pure CPU and cheap, so it runs here; only the state-change read happens in +// the Cmd. Replies for a different site or period than currently selected are +// stale and dropped. +func (m *Model) handleSLAData(msg slaDataMsg) (tea.Model, tea.Cmd) { + if msg.siteID != m.slaSiteID || msg.periodIdx != m.slaPeriodIdx { + return m, nil + } + period := slaPeriods[msg.periodIdx] var currentStatus string - if m.cursor < len(m.sites) { - currentStatus = m.sites[m.cursor].Status + for _, s := range m.sites { + if s.ID == msg.siteID { + currentStatus = s.Status + break + } } - m.slaReport = monitor.ComputeSLA(changes, currentStatus, period.duration) - m.slaDailyBreakdown = monitor.ComputeDailyBreakdown(changes, currentStatus, period.days, time.Now()) + m.slaReport = monitor.ComputeSLA(msg.changes, currentStatus, period.duration) + m.slaDailyBreakdown = monitor.ComputeDailyBreakdown(msg.changes, currentStatus, period.days, time.Now()) m.slaViewport = viewport.New( m.termWidth-chromePadH, @@ -418,6 +464,7 @@ func (m *Model) recomputeSLA() { ) m.slaViewport.SetContent(m.buildSLADailyContent()) m.slaViewport.GotoTop() + return m, nil } func (m *Model) handleHistoryKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { diff --git a/internal/tui/update_test.go b/internal/tui/update_test.go index 5ee60b6..473efe2 100644 --- a/internal/tui/update_test.go +++ b/internal/tui/update_test.go @@ -6,6 +6,7 @@ import ( "gitea.lerkolabs.com/lerkolabs/uptop/internal/models" "gitea.lerkolabs.com/lerkolabs/uptop/internal/monitor" + tea "github.com/charmbracelet/bubbletea" zone "github.com/lrstanley/bubblezone" ) @@ -222,3 +223,114 @@ func TestHandleTick_ThrottlesTabLoad(t *testing.T) { t.Errorf("tick past TTL should re-dispatch; lastTabLoad=%v want %v", mp.lastTabLoad, t2) } } + +// keyMsg builds a plain-rune key message ("h", "s", ...). +func keyMsg(s string) tea.KeyMsg { + return tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune(s)} +} + +func TestHandleTabData_DropsStaleSeq(t *testing.T) { + m := newTestModel(&tuiMockStore{}) + mp := &m + _ = mp.loadTabDataCmd() // seq 1 (superseded) + _ = mp.loadTabDataCmd() // seq 2 (newest) + + updated, _ := mp.handleTabData(tabDataMsg{seq: 1, alerts: []models.AlertConfig{{ID: 1}}}) + if got := updated.(*Model); len(got.alerts) != 0 { + t.Error("stale tab-data reply was applied over a newer in-flight load") + } + + updated, _ = mp.handleTabData(tabDataMsg{seq: 2, alerts: []models.AlertConfig{{ID: 2}}}) + if got := updated.(*Model); len(got.alerts) != 1 || got.alerts[0].ID != 2 { + t.Error("fresh tab-data reply was not applied") + } +} + +func TestHistoryKey_LoadsOffUIGoroutine(t *testing.T) { + ms := &tuiMockStore{stateChanges: []models.StateChange{{FromStatus: "UP", ToStatus: "DOWN"}}} + m := newTestModel(ms) + m.sites = []models.Site{{ID: 7, Name: "site"}} + m.state = stateDetail + m.termWidth, m.termHeight = 120, 40 + + updated, cmd := (&m).handleDetailKey(keyMsg("h")) + if ms.stateChangeCalls != 0 { + t.Fatal("history keypress hit the store synchronously in Update") + } + got := updated.(*Model) + if got.state != stateHistory || got.historySiteID != 7 { + t.Fatalf("history view not opened: state=%v siteID=%d", got.state, got.historySiteID) + } + if cmd == nil { + t.Fatal("expected a history load Cmd") + } + + msg := cmd() + hd, ok := msg.(historyDataMsg) + if !ok || hd.siteID != 7 || len(hd.changes) != 1 { + t.Fatalf("unexpected historyDataMsg: %+v", msg) + } + + folded, _ := got.Update(hd) + m2 := folded.(Model) + if len(m2.historyChanges) != 1 { + t.Fatal("history reply not folded into the model") + } + + // A reply for a previously opened site must not clobber the current one. + m2.historySiteID = 9 + stale, _ := m2.Update(historyDataMsg{siteID: 7, changes: nil}) + if m3 := stale.(Model); len(m3.historyChanges) != 1 { + t.Error("stale history reply overwrote the current view") + } +} + +func TestSLAData_DropsStaleReply(t *testing.T) { + m := newTestModel(&tuiMockStore{}) + m.termWidth, m.termHeight = 120, 40 + m.sites = []models.Site{{ID: 3, Status: "UP"}} + + if cmd := (&m).openSLAView(m.sites[0]); cmd == nil { + t.Fatal("openSLAView should return a load Cmd") + } + + // Reply for a different period than currently selected → dropped. + // (slaDataMsg routes through a pointer-receiver handler, so Update + // returns *Model on this path.) + updated, _ := m.Update(slaDataMsg{siteID: 3, periodIdx: 0}) + if mm := updated.(*Model); mm.slaDailyBreakdown != nil { + t.Error("stale SLA reply (old period) was applied") + } + + // Matching reply → report computed. + updated, _ = updated.(*Model).Update(slaDataMsg{siteID: 3, periodIdx: m.slaPeriodIdx}) + if mm := updated.(*Model); mm.slaDailyBreakdown == nil { + t.Error("matching SLA reply was not applied") + } +} + +func TestDetailRefreshCmd_OnlyWhileDetailOpen(t *testing.T) { + ms := &tuiMockStore{stateChanges: []models.StateChange{{FromStatus: "UP", ToStatus: "DOWN"}}} + m := newTestModel(ms) + m.sites = []models.Site{{ID: 5, Name: "site"}} + + m.state = stateDashboard + if (&m).detailRefreshCmd() != nil { + t.Error("refresh Cmd issued outside the detail view") + } + + m.state = stateDetail + cmd := (&m).detailRefreshCmd() + if cmd == nil { + t.Fatal("open detail panel should refresh on the tab-data cadence") + } + dd, ok := cmd().(detailDataMsg) + if !ok || dd.siteID != 5 || len(dd.changes) != 1 { + t.Fatalf("unexpected detail refresh reply: %+v", dd) + } + + m.cursor = 7 // cursor out of range → no refresh, no panic + if (&m).detailRefreshCmd() != nil { + t.Error("refresh Cmd issued for an out-of-range cursor") + } +}