From a3711c652c7c9102fb3208ad580c870bbebd4364 Mon Sep 17 00:00:00 2001 From: Tyler Koenig Date: Thu, 11 Jun 2026 11:39:15 -0400 Subject: [PATCH] fix(tui): move all store writes out of Update into tea.Cmds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deletes, pause toggles, maintenance end, theme/collapse prefs, and all four form submits wrote to the store synchronously on the UI goroutine; with busy_timeout=5000 a contended DB froze input for up to 5s. Writes now run through a writeCmd helper returning writeDoneMsg. The in-memory engine/model mutations stay in Update so rows react instantly; the reply logs failures and reloads tab data, so the UI converges on what was actually written. Closures capture snapshotted values only — never the model. --- internal/tui/data.go | 14 ++++++- internal/tui/messages.go | 8 ++++ internal/tui/tab_alerts.go | 22 +++++----- internal/tui/tab_maint.go | 9 +++-- internal/tui/tab_sites.go | 21 +++++----- internal/tui/tab_users.go | 22 +++++----- internal/tui/update.go | 81 +++++++++++++++++++++++-------------- internal/tui/update_test.go | 65 ++++++++++++++++++++++++++--- 8 files changed, 168 insertions(+), 74 deletions(-) diff --git a/internal/tui/data.go b/internal/tui/data.go index ad8038e..46ba093 100644 --- a/internal/tui/data.go +++ b/internal/tui/data.go @@ -27,7 +27,9 @@ func loadCollapsed(s store.Store) map[int]bool { return m } -func saveCollapsed(s store.Store, collapsed map[int]bool) { +// collapsedJSON snapshots the collapsed-group set for persistence. Marshaling +// happens on the UI goroutine so the write Cmd never reads the live map. +func collapsedJSON(collapsed map[int]bool) string { var ids []int for id, v := range collapsed { if v { @@ -35,7 +37,15 @@ func saveCollapsed(s store.Store, collapsed map[int]bool) { } } data, _ := json.Marshal(ids) - _ = s.SetPreference("collapsed_groups", string(data)) + return string(data) +} + +// writeCmd runs a store mutation off the UI goroutine. The closure must only +// capture values snapshotted in Update — never the model itself. +func writeCmd(op string, fn func() error) tea.Cmd { + return func() tea.Msg { + return writeDoneMsg{op: op, err: fn()} + } } func sortSitesForDisplay(allSites []models.Site, collapsed map[int]bool) []models.Site { diff --git a/internal/tui/messages.go b/internal/tui/messages.go index 59db852..fbf5653 100644 --- a/internal/tui/messages.go +++ b/internal/tui/messages.go @@ -54,3 +54,11 @@ type slaDataMsg struct { periodIdx int changes []models.StateChange } + +// writeDoneMsg reports a store mutation that ran off the UI goroutine. op +// names the action for the error log; the handler reloads tab data so the UI +// converges on what was actually written. +type writeDoneMsg struct { + op string + err error +} diff --git a/internal/tui/tab_alerts.go b/internal/tui/tab_alerts.go index 7fd7ef8..2f050dc 100644 --- a/internal/tui/tab_alerts.go +++ b/internal/tui/tab_alerts.go @@ -445,7 +445,7 @@ func (m *Model) initAlertHuhForm() tea.Cmd { return m.huhForm.Init() } -func (m *Model) submitAlertForm() { +func (m *Model) submitAlertForm() tea.Cmd { d := m.alertFormData settings := make(map[string]string) @@ -486,14 +486,16 @@ func (m *Model) submitAlertForm() { settings["url"] = d.WebhookURL } - if m.editID > 0 { - if err := m.store.UpdateAlert(m.editID, d.Name, d.AlertType, settings); err != nil { - m.engine.AddLog("Update alert failed: " + err.Error()) - } - } else { - if err := m.store.AddAlert(d.Name, d.AlertType, settings); err != nil { - m.engine.AddLog("Add alert failed: " + err.Error()) - } - } + st := m.store + id := m.editID + name, aType := d.Name, d.AlertType m.state = stateDashboard + if id > 0 { + return writeCmd("Update alert", func() error { + return st.UpdateAlert(id, name, aType, settings) + }) + } + return writeCmd("Add alert", func() error { + return st.AddAlert(name, aType, settings) + }) } diff --git a/internal/tui/tab_maint.go b/internal/tui/tab_maint.go index 4ba4747..132cad1 100644 --- a/internal/tui/tab_maint.go +++ b/internal/tui/tab_maint.go @@ -206,7 +206,7 @@ func (m *Model) initMaintHuhForm() tea.Cmd { return m.huhForm.Init() } -func (m *Model) submitMaintForm() { +func (m *Model) submitMaintForm() tea.Cmd { d := m.maintFormData monitorID, _ := strconv.Atoi(d.MonitorID) @@ -237,8 +237,9 @@ func (m *Model) submitMaintForm() { } } - if err := m.store.AddMaintenanceWindow(mw); err != nil { - m.engine.AddLog("Add maintenance window failed: " + err.Error()) - } + st := m.store m.state = stateDashboard + return writeCmd("Add maintenance window", func() error { + return st.AddMaintenanceWindow(mw) + }) } diff --git a/internal/tui/tab_sites.go b/internal/tui/tab_sites.go index 5cb5d89..4086512 100644 --- a/internal/tui/tab_sites.go +++ b/internal/tui/tab_sites.go @@ -518,7 +518,7 @@ func (m *Model) initSiteHuhForm() tea.Cmd { return m.huhForm.Init() } -func (m *Model) submitSiteForm() { +func (m *Model) submitSiteForm() tea.Cmd { d := m.siteFormData interval, _ := strconv.Atoi(d.Interval) alertID, _ := strconv.Atoi(d.AlertID) @@ -555,15 +555,14 @@ func (m *Model) submitSiteForm() { Regions: d.Regions, } - if m.editID > 0 { - if err := m.store.UpdateSite(site); err != nil { - m.engine.AddLog("Update site failed: " + err.Error()) - } - m.engine.UpdateSiteConfig(site) - } else { - if err := m.store.AddSite(site); err != nil { - m.engine.AddLog("Add site failed: " + err.Error()) - } - } + st := m.store m.state = stateDashboard + if m.editID > 0 { + // The engine's in-memory config updates immediately; the DB write + // follows in the Cmd. New sites enter the engine via its poll loop + // once the insert lands. + m.engine.UpdateSiteConfig(site) + return writeCmd("Update site", func() error { return st.UpdateSite(site) }) + } + return writeCmd("Add site", func() error { return st.AddSite(site) }) } diff --git a/internal/tui/tab_users.go b/internal/tui/tab_users.go index eb8307b..16b45e3 100644 --- a/internal/tui/tab_users.go +++ b/internal/tui/tab_users.go @@ -110,16 +110,18 @@ func (m *Model) initUserHuhForm() tea.Cmd { return m.huhForm.Init() } -func (m *Model) submitUserForm() { +func (m *Model) submitUserForm() tea.Cmd { d := m.userFormData - if m.editID > 0 { - if err := m.store.UpdateUser(m.editID, d.Username, d.PublicKey, d.Role); err != nil { - m.engine.AddLog("Update user failed: " + err.Error()) - } - } else { - if err := m.store.AddUser(d.Username, d.PublicKey, d.Role); err != nil { - m.engine.AddLog("Add user failed: " + err.Error()) - } - } + st := m.store + id := m.editID + username, key, role := d.Username, d.PublicKey, d.Role m.state = stateUsers + if id > 0 { + return writeCmd("Update user", func() error { + return st.UpdateUser(id, username, key, role) + }) + } + return writeCmd("Add user", func() error { + return st.AddUser(username, key, role) + }) } diff --git a/internal/tui/update.go b/internal/tui/update.go index 8dd8277..605658c 100644 --- a/internal/tui/update.go +++ b/internal/tui/update.go @@ -38,6 +38,12 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil case slaDataMsg: return m.handleSLAData(msg) + case writeDoneMsg: + if msg.err != nil { + m.engine.AddLog(msg.op + " failed: " + msg.err.Error()) + } + m.refreshLive() + return m, m.loadTabDataCmd() } if m.state == stateConfirmDelete { @@ -63,27 +69,26 @@ func (m *Model) handleConfirmDelete(msg tea.Msg) (tea.Model, tea.Cmd) { } switch keyMsg.String() { case "y", "Y": + // The store delete runs in a Cmd; the in-memory engine/model updates + // stay here so the row vanishes immediately. If the delete fails, the + // writeDoneMsg reload converges the UI back to the DB state (and the + // engine poll loop re-adds a site that is still in the DB). + st := m.store + id := m.deleteID + var cmd tea.Cmd switch m.deleteTab { case 0: - if err := m.store.DeleteSite(m.deleteID); err != nil { - m.engine.AddLog("Delete site failed: " + err.Error()) - } - m.engine.RemoveSite(m.deleteID) + cmd = writeCmd("Delete site", func() error { return st.DeleteSite(id) }) + m.engine.RemoveSite(id) m.adjustCursor(len(m.sites) - 1) case 1: - if err := m.store.DeleteAlert(m.deleteID); err != nil { - m.engine.AddLog("Delete alert failed: " + err.Error()) - } + cmd = writeCmd("Delete alert", func() error { return st.DeleteAlert(id) }) m.adjustCursor(len(m.alerts) - 1) case 4: - if err := m.store.DeleteMaintenanceWindow(m.deleteID); err != nil { - m.engine.AddLog("Delete maintenance window failed: " + err.Error()) - } + cmd = writeCmd("Delete maintenance window", func() error { return st.DeleteMaintenanceWindow(id) }) m.adjustCursor(len(m.maintenanceWindows) - 1) case 5: - if err := m.store.DeleteUser(m.deleteID); err != nil { - m.engine.AddLog("Delete user failed: " + err.Error()) - } + cmd = writeCmd("Delete user", func() error { return st.DeleteUser(id) }) m.adjustCursor(len(m.users) - 1) } m.refreshLive() @@ -91,7 +96,7 @@ func (m *Model) handleConfirmDelete(msg tea.Msg) (tea.Model, tea.Cmd) { if m.deleteTab == 5 { m.state = stateUsers } - return m, m.loadTabDataCmd() + return m, cmd case "n", "N", "esc": m.state = stateDashboard if m.deleteTab == 5 { @@ -127,10 +132,12 @@ func (m *Model) handleFormMsg(msg tea.Msg) (tea.Model, tea.Cmd) { m.huhForm = f } if m.huhForm.State == huh.StateCompleted { - m.submitForm() + // The store write runs in the returned Cmd; its writeDoneMsg + // triggers the tab-data reload once the row actually exists. + cmd := m.submitForm() m.refreshLive() m.huhForm = nil - return m, m.loadTabDataCmd() + return m, cmd } return m, formCmd } @@ -555,16 +562,22 @@ func (m *Model) handleDashboardKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { if m.currentTab == 0 && len(m.sites) > 0 && m.sites[m.cursor].Type == "group" { gid := m.sites[m.cursor].ID m.collapsed[gid] = !m.collapsed[gid] - saveCollapsed(m.store, m.collapsed) + payload := collapsedJSON(m.collapsed) + st := m.store m.refreshLive() + return m, writeCmd("Save collapsed groups", func() error { + return st.SetPreference("collapsed_groups", payload) + }) } case "p": if m.currentTab == 0 && len(m.sites) > 0 { - site := m.sites[m.cursor] - m.engine.ToggleSitePause(site.ID) - site.Paused = !site.Paused - _ = m.store.UpdateSitePaused(site.ID, site.Paused) + id := m.sites[m.cursor].ID + paused := m.engine.ToggleSitePause(id) + st := m.store m.refreshLive() + return m, writeCmd("Update pause state", func() error { + return st.UpdateSitePaused(id, paused) + }) } case "i": if m.currentTab == 0 && len(m.sites) > 0 { @@ -579,18 +592,23 @@ func (m *Model) handleDashboardKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { now := time.Now() isActive := !mw.StartTime.After(now) && (mw.EndTime.IsZero() || mw.EndTime.After(now)) if isActive { - if err := m.store.EndMaintenanceWindow(mw.ID); err != nil { - m.engine.AddLog("End maintenance failed: " + err.Error()) - } + st := m.store + id := mw.ID m.refreshLive() - return m, m.loadTabDataCmd() + return m, writeCmd("End maintenance", func() error { + return st.EndMaintenanceWindow(id) + }) } } case "T": m.themeIndex = (m.themeIndex + 1) % len(themes) m.theme = themes[m.themeIndex] m.st = newStyles(m.theme) - _ = m.store.SetPreference("theme", m.theme.Name) + st := m.store + name := m.theme.Name + return m, writeCmd("Save theme", func() error { + return st.SetPreference("theme", name) + }) case "d", "backspace": return m.handleDeleteItem() } @@ -738,25 +756,26 @@ func (m *Model) adjustCursor(newLen int) { } } -func (m *Model) submitForm() { +func (m *Model) submitForm() tea.Cmd { switch m.state { case stateFormSite: if m.siteFormData != nil { - m.submitSiteForm() + return m.submitSiteForm() } case stateFormAlert: if m.alertFormData != nil { - m.submitAlertForm() + return m.submitAlertForm() } case stateFormUser: if m.userFormData != nil { - m.submitUserForm() + return m.submitUserForm() } case stateFormMaint: if m.maintFormData != nil { - m.submitMaintForm() + return m.submitMaintForm() } } + return nil } func (m Model) currentListLen() int { diff --git a/internal/tui/update_test.go b/internal/tui/update_test.go index 473efe2..ea15ba4 100644 --- a/internal/tui/update_test.go +++ b/internal/tui/update_test.go @@ -1,6 +1,7 @@ package tui import ( + "strings" "testing" "time" @@ -19,6 +20,7 @@ type tuiMockStore struct { maint []models.MaintenanceWindow stateChanges []models.StateChange stateChangeCalls int // counts GetStateChanges hits (to prove View does no IO) + deleteSiteCalls int // counts DeleteSite hits (to prove writes run in Cmds) } func (m *tuiMockStore) GetAllAlerts() ([]models.AlertConfig, error) { return m.alerts, nil } @@ -32,12 +34,15 @@ func (m *tuiMockStore) GetAllMaintenanceWindows(int) ([]models.MaintenanceWindow return m.maint, nil } -func (m *tuiMockStore) Init() error { return nil } -func (m *tuiMockStore) GetSites() ([]models.Site, error) { return nil, nil } -func (m *tuiMockStore) AddSite(models.Site) error { return nil } -func (m *tuiMockStore) UpdateSite(models.Site) error { return nil } -func (m *tuiMockStore) UpdateSitePaused(int, bool) error { return nil } -func (m *tuiMockStore) DeleteSite(int) error { return nil } +func (m *tuiMockStore) Init() error { return nil } +func (m *tuiMockStore) GetSites() ([]models.Site, error) { return nil, nil } +func (m *tuiMockStore) AddSite(models.Site) error { return nil } +func (m *tuiMockStore) UpdateSite(models.Site) error { return nil } +func (m *tuiMockStore) UpdateSitePaused(int, bool) error { return nil } +func (m *tuiMockStore) DeleteSite(int) error { + m.deleteSiteCalls++ + return nil +} func (m *tuiMockStore) GetAlert(int) (models.AlertConfig, error) { return models.AlertConfig{}, nil } func (m *tuiMockStore) AddAlert(string, string, map[string]string) error { return nil } func (m *tuiMockStore) UpdateAlert(int, string, string, map[string]string) error { return nil } @@ -309,6 +314,54 @@ func TestSLAData_DropsStaleReply(t *testing.T) { } } +func TestConfirmDelete_WritesOffUIGoroutine(t *testing.T) { + ms := &tuiMockStore{} + m := newTestModel(ms) + m.sites = []models.Site{{ID: 4, Name: "s"}} + m.state = stateConfirmDelete + m.deleteTab = 0 + m.deleteID = 4 + + updated, cmd := (&m).handleConfirmDelete(keyMsg("y")) + if ms.deleteSiteCalls != 0 { + t.Fatal("delete hit the store synchronously in Update") + } + if cmd == nil { + t.Fatal("expected a write Cmd") + } + if got := updated.(*Model); got.state != stateDashboard { + t.Fatalf("expected return to dashboard, got state %v", got.state) + } + + wd, ok := cmd().(writeDoneMsg) + if !ok || wd.err != nil { + t.Fatalf("unexpected write result: %+v", wd) + } + if ms.deleteSiteCalls != 1 { + t.Fatalf("expected exactly 1 store delete from the Cmd, got %d", ms.deleteSiteCalls) + } +} + +func TestWriteDoneMsg_LogsErrorAndReloads(t *testing.T) { + m := newTestModel(&tuiMockStore{}) + + updated, cmd := m.Update(writeDoneMsg{op: "Delete site", err: errSentinel}) + if cmd == nil { + t.Error("writeDoneMsg did not trigger a tab-data reload") + } + + mm := updated.(Model) + found := false + for _, line := range mm.engine.GetLogs() { + if strings.Contains(line, "Delete site failed: boom") { + found = true + } + } + if !found { + t.Error("write error was not logged") + } +} + func TestDetailRefreshCmd_OnlyWhileDetailOpen(t *testing.T) { ms := &tuiMockStore{stateChanges: []models.StateChange{{FromStatus: "UP", ToStatus: "DOWN"}}} m := newTestModel(ms)