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
2026-06-11 11:35:03 -04:00
parent 274f0081e2
commit 634c3ee03c
6 changed files with 245 additions and 35 deletions
@@ -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),
+		}
+	}
+}
@@ -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
+}
@@ -325,15 +325,14 @@ 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 {
+	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")}
 	for _, s := range m.sites {
@@ -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
@@ -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) {
@@ -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")
+	}
+}