From 819cc045ab7cf68c103a19244e7b61cb61221814 Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Mon, 17 Nov 2025 08:32:41 -1000 Subject: [PATCH 1/4] Fix nil pointer dereference panic when acknowledging incidents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves panic that occurred when trying to acknowledge incidents by removing unsafe dereferences of m.selectedIncident in message handler closures. The closures were capturing the incident pointer at creation time, which could be nil, rather than at execution time when the incident data is guaranteed to be available. Changes: - Modified acknowledge/unacknowledge message handlers to create empty message structs instead of dereferencing m.selectedIncident in closures - Updated Update() function handlers to safely retrieve incident from model state when processing messages - Added nil check in silence incidents handler to prevent panic when appending selected incident The fix ensures that incident data is accessed only after it has been fetched from PagerDuty and stored in the model, preventing nil pointer dereferences. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/tui/msgHandlers.go | 8 ++++---- pkg/tui/tui.go | 33 ++++++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/pkg/tui/msgHandlers.go b/pkg/tui/msgHandlers.go index 90a49fc..44c7d24 100644 --- a/pkg/tui/msgHandlers.go +++ b/pkg/tui/msgHandlers.go @@ -269,7 +269,7 @@ func switchTableFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return waitForSelectedIncidentThenDoMsg{ msg: "acknowledge", action: func() tea.Msg { - return acknowledgeIncidentsMsg{incidents: []pagerduty.Incident{*m.selectedIncident}} + return acknowledgeIncidentsMsg{} }, } }, @@ -282,7 +282,7 @@ func switchTableFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return waitForSelectedIncidentThenDoMsg{ msg: "un-acknowledge", action: func() tea.Msg { - return unAcknowledgeIncidentsMsg{incidents: []pagerduty.Incident{*m.selectedIncident}} + return unAcknowledgeIncidentsMsg{} }, } }, @@ -360,10 +360,10 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return m, func() tea.Msg { return getIncidentMsg(m.selectedIncident.ID) } case key.Matches(msg, defaultKeyMap.Ack): - return m, func() tea.Msg { return acknowledgeIncidentsMsg{incidents: []pagerduty.Incident{*m.selectedIncident}} } + return m, func() tea.Msg { return acknowledgeIncidentsMsg{} } case key.Matches(msg, defaultKeyMap.UnAck): - return m, func() tea.Msg { return unAcknowledgeIncidentsMsg{incidents: []pagerduty.Incident{*m.selectedIncident}} } + return m, func() tea.Msg { return unAcknowledgeIncidentsMsg{} } case key.Matches(msg, defaultKeyMap.Silence): return m, func() tea.Msg { return silenceSelectedIncidentMsg{} } diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index b49460a..857e284 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -509,24 +509,36 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.viewingIncident = true case acknowledgeIncidentsMsg: - if msg.incidents == nil { - m.setStatus("failed acknowledging incidents - no incidents provided") - return m, nil + // If incidents are provided in the message, use those + // Otherwise, use the selected incident from the model + incidents := msg.incidents + if incidents == nil { + if m.selectedIncident == nil { + m.setStatus("failed acknowledging incidents - no incidents provided and no incident selected") + return m, nil + } + incidents = []pagerduty.Incident{*m.selectedIncident} } return m, tea.Sequence( - acknowledgeIncidents(m.config, msg.incidents, false), + acknowledgeIncidents(m.config, incidents, false), func() tea.Msg { return clearSelectedIncidentsMsg("sender: acknowledgeIncidentsMsg") }, ) case unAcknowledgeIncidentsMsg: - if msg.incidents == nil { - m.setStatus("failed re-escalating incidents - no incidents provided") - return m, nil + // If incidents are provided in the message, use those + // Otherwise, use the selected incident from the model + incidents := msg.incidents + if incidents == nil { + if m.selectedIncident == nil { + m.setStatus("failed re-escalating incidents - no incidents provided and no incident selected") + return m, nil + } + incidents = []pagerduty.Incident{*m.selectedIncident} } return m, tea.Sequence( - acknowledgeIncidents(m.config, msg.incidents, true), + acknowledgeIncidents(m.config, incidents, true), func() tea.Msg { return clearSelectedIncidentsMsg("sender: unAcknowledgeIncidentsMsg") }, ) @@ -630,7 +642,10 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } - incidents := append(msg.incidents, *m.selectedIncident) + incidents := msg.incidents + if m.selectedIncident != nil { + incidents = append(msg.incidents, *m.selectedIncident) + } return m, tea.Sequence( silenceIncidents(incidents, m.config.EscalationPolicies["silent_default"], silentDefaultPolicyLevel), func() tea.Msg { return clearSelectedIncidentsMsg("sender: silenceIncidentsMsg") }, From 20f9e9a2984f5bc91157d1db4248deb2c242754f Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Mon, 17 Nov 2025 14:17:11 -1000 Subject: [PATCH 2/4] Fix performance issues and re-escalation bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multiple performance and functionality improvements: - Fix re-escalation "Assignee cannot be empty" error by skipping unnecessary un-acknowledge step and going directly to re-escalation with proper user email authentication - Add comprehensive terminal escape sequence filtering to prevent fake keypresses from clogging the message queue (OSC, CSI, CPR responses) - Implement priority handling for all user keypresses to ensure responsive UI even when async messages are queued - Optimize auto-acknowledge feature with early return when disabled and cached on-call check to reduce API calls from N to 1 per cycle - Remove triple template rendering - now renders once on explicit request - Fix viewport consuming ESC and navigation keys with handledKey flag - Add debug logging to track re-escalation flow - Clean up unused code: remove unAcknowledgedIncidentsMsg and reEscalate parameter from acknowledgeIncidents() Performance impact: - ESC/Enter keys now respond immediately instead of waiting for queued messages - Auto-acknowledge overhead reduced to near-zero when feature is disabled - Eliminated redundant template renders (3x → 1x per incident view) Fixes: https://github.com/clcollins/srepd/issues/XXX 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/pd/pd.go | 18 +++-- pkg/tui/commands.go | 27 +++----- pkg/tui/commands_test.go | 36 +++------- pkg/tui/msgHandlers.go | 22 +++++- pkg/tui/tui.go | 141 ++++++++++++++++++++++++++------------- 5 files changed, 144 insertions(+), 100 deletions(-) diff --git a/pkg/pd/pd.go b/pkg/pd/pd.go index af9e265..6a117c6 100644 --- a/pkg/pd/pd.go +++ b/pkg/pd/pd.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/PagerDuty/go-pagerduty" + "github.com/charmbracelet/log" ) const ( @@ -115,21 +116,26 @@ func NewListIncidentOptsFromDefaults() pagerduty.ListIncidentsOptions { } -func AcknowledgeIncident(client PagerDutyClient, incidents []pagerduty.Incident, user *pagerduty.User) ([]pagerduty.Incident, error) { +func AcknowledgeIncident(client PagerDutyClient, incidents []pagerduty.Incident, user *pagerduty.User, currentUser *pagerduty.User) ([]pagerduty.Incident, error) { var ctx = context.Background() var email string opts := []pagerduty.ManageIncidentsOptions{} + // Use currentUser's email for API call authentication + if currentUser != nil { + email = currentUser.Email + } + for _, incident := range incidents { if user == nil { - email = "" + // Un-acknowledge: set escalation level without assignment opts = append(opts, pagerduty.ManageIncidentsOptions{ ID: incident.ID, EscalationLevel: 1, }) } else { - email = user.Email + // Acknowledge: assign to specific user opts = append(opts, pagerduty.ManageIncidentsOptions{ ID: incident.ID, Status: "acknowledged", @@ -302,8 +308,10 @@ func GetUserOnCalls(client PagerDutyClient, id string, opts pagerduty.ListOnCall func loopManageIncidents(client PagerDutyClient, ctx context.Context, email string, opts []pagerduty.ManageIncidentsOptions) (incidentList []pagerduty.Incident, err error) { for { + log.Debug("pd.loopManageIncidents", "email", email, "opts_count", len(opts)) response, err := client.ManageIncidentsWithContext(ctx, email, opts) if err != nil { + log.Error("pd.loopManageIncidents", "error", err, "email", email) return incidentList, err } @@ -347,7 +355,7 @@ func ReassignIncidents(client PagerDutyClient, incidents []pagerduty.Incident, u } // ReEscalateIncidents re-escalates a list of incidents to an escalation policy at a specific level -func ReEscalateIncidents(client PagerDutyClient, incidents []pagerduty.Incident, policy *pagerduty.EscalationPolicy, level uint) ([]pagerduty.Incident, error) { +func ReEscalateIncidents(client PagerDutyClient, incidents []pagerduty.Incident, user *pagerduty.User, policy *pagerduty.EscalationPolicy, level uint) ([]pagerduty.Incident, error) { var ctx = context.Background() opts := []pagerduty.ManageIncidentsOptions{} @@ -364,7 +372,7 @@ func ReEscalateIncidents(client PagerDutyClient, incidents []pagerduty.Incident, }) } - return loopManageIncidents(client, ctx, "", opts) + return loopManageIncidents(client, ctx, user.Email, opts) } func PostNote(client PagerDutyClient, id string, user *pagerduty.User, content string) (*pagerduty.IncidentNote, error) { diff --git a/pkg/tui/commands.go b/pkg/tui/commands.go index fa00f88..b4b0df4 100644 --- a/pkg/tui/commands.go +++ b/pkg/tui/commands.go @@ -251,7 +251,13 @@ func ShouldBeAcknowledged(p *pd.Config, i pagerduty.Incident, id string, autoAck "userIsOnCall", userIsOnCall, "doIt", doIt, ) - return AssignedToUser(i, id) && !AcknowledgedByUser(i, id) && autoAcknowledge + return doIt +} + +// ShouldBeAcknowledgedCached checks if an incident should be auto-acknowledged using a cached userIsOnCall result. +// This avoids repeated calls to UserIsOnCall() when checking multiple incidents in the same update cycle. +func ShouldBeAcknowledgedCached(i pagerduty.Incident, id string, userIsOnCall bool) bool { + return AssignedToUser(i, id) && !AcknowledgedByUser(i, id) && userIsOnCall } // AssignedToUser returns true if the incident is assigned to the given user @@ -523,22 +529,9 @@ type acknowledgedIncidentsMsg struct { incidents []pagerduty.Incident err error } -type unAcknowledgedIncidentsMsg struct { - incidents []pagerduty.Incident - err error -} - -func acknowledgeIncidents(p *pd.Config, incidents []pagerduty.Incident, reEscalate bool) tea.Cmd { +func acknowledgeIncidents(p *pd.Config, incidents []pagerduty.Incident) tea.Cmd { return func() tea.Msg { - var err error - - if reEscalate { - a, err := pd.AcknowledgeIncident(p.Client, incidents, &pagerduty.User{}) - return unAcknowledgedIncidentsMsg{a, err} - } - - a, err := pd.AcknowledgeIncident(p.Client, incidents, p.CurrentUser) - + a, err := pd.AcknowledgeIncident(p.Client, incidents, p.CurrentUser, p.CurrentUser) return acknowledgedIncidentsMsg{a, err} } } @@ -573,7 +566,7 @@ type reEscalatedIncidentsMsg []pagerduty.Incident func reEscalateIncidents(p *pd.Config, i []pagerduty.Incident, e *pagerduty.EscalationPolicy, l uint) tea.Cmd { return func() tea.Msg { - r, err := pd.ReEscalateIncidents(p.Client, i, e, l) + r, err := pd.ReEscalateIncidents(p.Client, i, p.CurrentUser, e, l) if err != nil { return errMsg{err} } diff --git a/pkg/tui/commands_test.go b/pkg/tui/commands_test.go index 5acdff2..178b069 100644 --- a/pkg/tui/commands_test.go +++ b/pkg/tui/commands_test.go @@ -30,41 +30,21 @@ func TestAcknowledgeIncident(t *testing.T) { } tests := []struct { - name string - incidents []pagerduty.Incident - reEscalate bool - expected tea.Msg + name string + incidents []pagerduty.Incident + expected tea.Msg }{ { - name: "return unAcknowledgedIncidentMsg with non-nil error if error occurs while re-escalating", - incidents: errIncidents, - reEscalate: true, - expected: unAcknowledgedIncidentsMsg{ - incidents: []pagerduty.Incident(nil), - err: pd.ErrMockError, - }, - }, - { - name: "return unAcknowledgedIncidentMsg with an incident list if no error occurs while re-escalating", - incidents: incidents, - reEscalate: true, - expected: unAcknowledgedIncidentsMsg{ - incidents: incidents, - }, - }, - { - name: "return acknowledgedIncidentMsg with non-nil error if error occurs while acknowledging", - incidents: errIncidents, - reEscalate: false, + name: "return acknowledgedIncidentMsg with non-nil error if error occurs while acknowledging", + incidents: errIncidents, expected: acknowledgedIncidentsMsg{ incidents: []pagerduty.Incident(nil), err: pd.ErrMockError, }, }, { - name: "return acknowledgedIncidentMsg with an incident list if no error occurs while acknowledging", - incidents: incidents, - reEscalate: false, + name: "return acknowledgedIncidentMsg with an incident list if no error occurs while acknowledging", + incidents: incidents, expected: acknowledgedIncidentsMsg{ incidents: incidents, }, @@ -73,7 +53,7 @@ func TestAcknowledgeIncident(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - cmd := acknowledgeIncidents(mockConfig, test.incidents, test.reEscalate) + cmd := acknowledgeIncidents(mockConfig, test.incidents) actual := cmd() assert.Equal(t, test.expected, actual) }) diff --git a/pkg/tui/msgHandlers.go b/pkg/tui/msgHandlers.go index 44c7d24..3f5269a 100644 --- a/pkg/tui/msgHandlers.go +++ b/pkg/tui/msgHandlers.go @@ -346,29 +346,41 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { var cmd tea.Cmd var cmds []tea.Cmd + // Track if we handled the key ourselves + handledKey := false + switch msg := msg.(type) { case tea.KeyMsg: switch { case key.Matches(msg, defaultKeyMap.Help): m.toggleHelp() + handledKey = true // This un-sets the selected incident and returns to the table view case key.Matches(msg, defaultKeyMap.Back): m.clearSelectedIncident(msg.String() + " (back)") + m.table.Focus() // Ensure table regains focus immediately + // Return immediately - no need to process anything else or update viewport + return m, nil case key.Matches(msg, defaultKeyMap.Refresh): + handledKey = true return m, func() tea.Msg { return getIncidentMsg(m.selectedIncident.ID) } case key.Matches(msg, defaultKeyMap.Ack): + handledKey = true return m, func() tea.Msg { return acknowledgeIncidentsMsg{} } case key.Matches(msg, defaultKeyMap.UnAck): + handledKey = true return m, func() tea.Msg { return unAcknowledgeIncidentsMsg{} } case key.Matches(msg, defaultKeyMap.Silence): + handledKey = true return m, func() tea.Msg { return silenceSelectedIncidentMsg{} } case key.Matches(msg, defaultKeyMap.Note): + handledKey = true // Note template requires full incident data (HTMLURL, Title, Service.Summary) if !m.incidentDataLoaded { m.setStatus("Loading incident details, please wait...") @@ -377,6 +389,7 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return m, func() tea.Msg { return parseTemplateForNoteMsg("add note") } case key.Matches(msg, defaultKeyMap.Login): + handledKey = true // Login requires alerts to extract cluster_id if !m.incidentAlertsLoaded { m.setStatus("Loading incident alerts, please wait...") @@ -385,6 +398,7 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return m, func() tea.Msg { return loginMsg("login") } case key.Matches(msg, defaultKeyMap.Open): + handledKey = true // Browser open requires HTMLURL from full incident data if !m.incidentDataLoaded { m.setStatus("Loading incident details, please wait...") @@ -395,8 +409,12 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { } } - m.incidentViewer, cmd = m.incidentViewer.Update(msg) - cmds = append(cmds, cmd) + // Only pass the message to the viewport if we didn't handle it as a key command + // This prevents the viewport from consuming ESC and other navigation keys + if !handledKey { + m.incidentViewer, cmd = m.incidentViewer.Update(msg) + cmds = append(cmds, cmd) + } return m, tea.Batch(cmds...) } diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index 857e284..002aad3 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -118,6 +118,45 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { log.Debug("Update", msgType, filterMsgContent(msg)) } + // PRIORITY HANDLING: Process user input keys immediately, before any queued messages + // This ensures navigation and interaction keys are always responsive + // even when the message queue is backed up with async responses + if keyMsg, ok := msg.(tea.KeyMsg); ok { + // Filter out terminal escape sequences that aren't real keypresses + // These come from terminal queries (colors, cursor position, etc.) + keyStr := keyMsg.String() + + // Drop terminal response sequences (OSC, CSI, etc.) + if strings.Contains(keyStr, "rgb:") || // Color queries: ]11;rgb:1d1d/1d1d/2020 + strings.Contains(keyStr, ":1d1d/") || // Partial color responses + strings.Contains(keyStr, "gb:1d1d/") || // Truncated color responses + strings.Contains(keyStr, "alt+]") || // OSC start sequence + strings.Contains(keyStr, "alt+\\") || // OSC/DCS end sequence + strings.Contains(keyStr, "CSI") || // Control Sequence Introducer + keyStr == "OP" || // SS3 sequence (function keys) + keyStr == "[A" || keyStr == "[B" || // Broken arrow key sequences + keyStr == "[C" || keyStr == "[D" || // (should be handled by bubbletea) + (strings.HasPrefix(keyStr, "[") && strings.HasSuffix(keyStr, "R")) || // CPR: [row;colR + (strings.HasPrefix(keyStr, "]11;") || strings.HasPrefix(keyStr, "11;")) { // OSC 11 fragments + // Drop these fake key messages - they're terminal responses, not user input + log.Debug("Update", "filtered terminal escape sequence", keyStr) + return m, nil + } + + // All real user keypresses get priority handling + // This ensures the UI is always responsive even when async messages are queued + log.Debug("Update", "priority key handling", keyMsg.String()) + return m.keyMsgHandler(keyMsg) + } + + // Filter out unknown CSI sequences (cursor position reports, etc.) + // These are private bubble tea types, so we check the string representation + msgStr := fmt.Sprintf("%T", msg) + if strings.Contains(msgStr, "unknownCSISequenceMsg") { + log.Debug("Update", "filtered unknown CSI sequence", msg) + return m, nil + } + var cmds []tea.Cmd switch msg := msg.(type) { @@ -226,9 +265,8 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.selectedIncidentNotes = msg.notes m.incidentNotesLoaded = true - if m.viewingIncident { - cmds = append(cmds, renderIncident(&m)) - } + // Don't auto-render here - wait for explicit render request + // This prevents redundant template renders when alerts/notes arrive separately } case gotIncidentAlertsMsg: @@ -262,9 +300,8 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.selectedIncidentAlerts = msg.alerts m.incidentAlertsLoaded = true - if m.viewingIncident { - cmds = append(cmds, renderIncident(&m)) - } + // Don't auto-render here - wait for explicit render request + // This prevents redundant template renders when alerts/notes arrive separately } case updateIncidentListMsg: @@ -328,14 +365,19 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Check if any incidents should be auto-acknowledged; // This must be done before adding the stale incidents - for _, i := range m.incidentList { - if ShouldBeAcknowledged(m.config, i, m.config.CurrentUser.ID, m.autoAcknowledge) { - acknowledgeIncidentsList = append(acknowledgeIncidentsList, i) + if m.autoAcknowledge { + // Cache the on-call check - it's the same for all incidents in this update + userIsOnCall := UserIsOnCall(m.config, m.config.CurrentUser.ID) + + for _, i := range m.incidentList { + if ShouldBeAcknowledgedCached(i, m.config.CurrentUser.ID, userIsOnCall) { + acknowledgeIncidentsList = append(acknowledgeIncidentsList, i) + } } - } - if len(acknowledgeIncidentsList) > 0 { - cmds = append(cmds, func() tea.Msg { return acknowledgeIncidentsMsg{incidents: acknowledgeIncidentsList} }) + if len(acknowledgeIncidentsList) > 0 { + cmds = append(cmds, func() tea.Msg { return acknowledgeIncidentsMsg{incidents: acknowledgeIncidentsList} }) + } } // Add stale incidents to the list @@ -481,6 +523,14 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } + // If the user has closed the incident view (via ESC), abort the action + // instead of waiting forever for an incident that will never be set + if m.selectedIncident == nil && !m.viewingIncident { + log.Debug("Update", "waitForSelectedIncidentThenDoMsg", "aborting action - incident view closed", "msg", msg.msg) + m.setStatus("action cancelled - incident view closed") + return m, nil + } + // Re-queue the message if the selected incident is not yet available if m.selectedIncident == nil { m.setStatus("waiting for incident info...") @@ -505,8 +555,16 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, func() tea.Msg { return errMsg{msg.err} } } log.Debug("renderedIncidentMsg") - m.incidentViewer.SetContent(msg.content) - m.viewingIncident = true + + // Only set viewing state if we still have a selected incident + // This prevents late-arriving render messages from reopening the incident view + // after the user has already closed it with ESC + if m.selectedIncident != nil { + m.incidentViewer.SetContent(msg.content) + m.viewingIncident = true + } else { + log.Debug("renderedIncidentMsg", "action", "discarding render - incident was closed") + } case acknowledgeIncidentsMsg: // If incidents are provided in the message, use those @@ -521,7 +579,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, tea.Sequence( - acknowledgeIncidents(m.config, incidents, false), + acknowledgeIncidents(m.config, incidents), func() tea.Msg { return clearSelectedIncidentsMsg("sender: acknowledgeIncidentsMsg") }, ) @@ -537,55 +595,42 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { incidents = []pagerduty.Incident{*m.selectedIncident} } - return m, tea.Sequence( - acknowledgeIncidents(m.config, incidents, true), - func() tea.Msg { return clearSelectedIncidentsMsg("sender: unAcknowledgeIncidentsMsg") }, - ) - - case acknowledgedIncidentsMsg: - if msg.err != nil { - return m, func() tea.Msg { return errMsg{msg.err} } - } - incidentIDs := strings.Join(getIDsFromIncidents(msg.incidents), " ") - m.setStatus(fmt.Sprintf("acknowledged incidents: %s", incidentIDs)) - - return m, func() tea.Msg { return updateIncidentListMsg("sender: acknowledgedIncidentsMsg") } - - case unAcknowledgedIncidentsMsg: - if msg.err != nil { - return m, func() tea.Msg { return errMsg{msg.err} } - } - incidentIDs := strings.Join(getIDsFromIncidents(msg.incidents), " ") - m.setStatus(fmt.Sprintf("un-acknowledged incidents: %s", incidentIDs)) - - // After un-acknowledging, re-escalate each incident to its escalation policy to page current on-call + // Skip un-acknowledge step - go directly to re-escalation + // Re-escalation will reassign to the current on-call at the escalation level // Group incidents by escalation policy policyGroups := make(map[string][]pagerduty.Incident) - for _, incident := range msg.incidents { + for _, incident := range incidents { policyKey := getEscalationPolicyKey(incident.Service.ID, m.config.EscalationPolicies) policyGroups[policyKey] = append(policyGroups[policyKey], incident) } - // Create re-escalate messages for each policy group + // Create re-escalate commands for each policy group var cmds []tea.Cmd for policyKey, incidents := range policyGroups { policy := m.config.EscalationPolicies[policyKey] if policy != nil && policy.ID != "" { - cmds = append(cmds, func() tea.Msg { - return reEscalateIncidentsMsg{ - incidents: incidents, - policy: policy, - level: reEscalateDefaultPolicyLevel, - } - }) + cmds = append(cmds, reEscalateIncidents(m.config, incidents, policy, reEscalateDefaultPolicyLevel)) } } + // Add clear selected incidents after re-escalation + cmds = append(cmds, func() tea.Msg { return clearSelectedIncidentsMsg("sender: unAcknowledgeIncidentsMsg") }) + if len(cmds) > 0 { - return m, tea.Batch(cmds...) + return m, tea.Sequence(cmds...) } - return m, func() tea.Msg { return updateIncidentListMsg("sender: unAcknowledgedIncidentsMsg") } + return m, func() tea.Msg { return updateIncidentListMsg("sender: unAcknowledgeIncidentsMsg") } + + case acknowledgedIncidentsMsg: + if msg.err != nil { + return m, func() tea.Msg { return errMsg{msg.err} } + } + incidentIDs := strings.Join(getIDsFromIncidents(msg.incidents), " ") + m.setStatus(fmt.Sprintf("acknowledged incidents: %s", incidentIDs)) + + return m, func() tea.Msg { return updateIncidentListMsg("sender: acknowledgedIncidentsMsg") } + case reassignIncidentsMsg: if msg.incidents == nil { From db1679805a35fb8a9748d4a579205107c85947be Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Tue, 18 Nov 2025 07:32:39 -1000 Subject: [PATCH 3/4] Add unit tests for views.go helper functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds comprehensive unit tests for pure display/formatting functions in pkg/tui/views.go that were previously untested: - assigneeArea(): Tests formatting of "You", "Team", and custom assignee displays - statusArea(): Tests formatting of status messages with various content - refreshArea(): Tests all combinations of auto-refresh and auto-acknowledge flags - summarizeNotes(): Tests conversion of PagerDuty notes to display format - summarizeIncident(): Tests incident conversion including priority, assignments, and duplicate acknowledgement deduplication - addNoteTemplate(): Tests note template generation with incident context All 6 functions now have test coverage with 16 test cases total, covering: - Edge cases (empty inputs, nil values) - Normal operation (single/multiple items) - Business logic (deduplication, conditional formatting) Test coverage: pkg/tui/views_test.go Tests pass: go test ./pkg/tui -v 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/tui/views_test.go | 331 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 331 insertions(+) create mode 100644 pkg/tui/views_test.go diff --git a/pkg/tui/views_test.go b/pkg/tui/views_test.go new file mode 100644 index 0000000..a944d9b --- /dev/null +++ b/pkg/tui/views_test.go @@ -0,0 +1,331 @@ +package tui + +import ( + "testing" + + "github.com/PagerDuty/go-pagerduty" + "github.com/stretchr/testify/assert" +) + +func TestAssigneeArea(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "formats 'You' assignee", + input: "You", + expected: "Showing assigned to You", + }, + { + name: "formats 'Team' assignee", + input: "Team", + expected: "Showing assigned to Team", + }, + { + name: "formats custom assignee", + input: "John Doe", + expected: "Showing assigned to John Doe", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := assigneeArea(test.input) + assert.Equal(t, test.expected, result) + }) + } +} + +func TestStatusArea(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "formats simple status", + input: "Loading...", + expected: "> Loading...", + }, + { + name: "formats status with numbers", + input: "showing 2/5 incidents", + expected: "> showing 2/5 incidents", + }, + { + name: "formats empty status", + input: "", + expected: "> ", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := statusArea(test.input) + assert.Equal(t, test.expected, result) + }) + } +} + +func TestRefreshArea(t *testing.T) { + tests := []struct { + name string + autoRefresh bool + autoAck bool + expected string + }{ + { + name: "both enabled", + autoRefresh: true, + autoAck: true, + expected: "Watching for updates... [auto-acknowledge]", + }, + { + name: "auto-refresh enabled, auto-ack disabled", + autoRefresh: true, + autoAck: false, + expected: "Watching for updates... ", + }, + { + name: "auto-refresh disabled", + autoRefresh: false, + autoAck: false, + expected: "Watching for updates... [PAUSED]", + }, + { + name: "auto-refresh disabled, auto-ack enabled (paused takes precedence)", + autoRefresh: false, + autoAck: true, + expected: "Watching for updates... [PAUSED]", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := refreshArea(test.autoRefresh, test.autoAck) + assert.Equal(t, test.expected, result) + }) + } +} + +func TestSummarizeNotes(t *testing.T) { + tests := []struct { + name string + input []pagerduty.IncidentNote + expected []noteSummary + }{ + { + name: "empty notes list", + input: []pagerduty.IncidentNote{}, + expected: nil, + }, + { + name: "single note", + input: []pagerduty.IncidentNote{ + { + ID: "NOTE123", + User: pagerduty.APIObject{Summary: "John Doe"}, + Content: "This is a test note", + CreatedAt: "2025-01-01T00:00:00Z", + }, + }, + expected: []noteSummary{ + { + ID: "NOTE123", + User: "John Doe", + Content: "This is a test note", + Created: "2025-01-01T00:00:00Z", + }, + }, + }, + { + name: "multiple notes", + input: []pagerduty.IncidentNote{ + { + ID: "NOTE1", + User: pagerduty.APIObject{Summary: "User 1"}, + Content: "First note", + CreatedAt: "2025-01-01T00:00:00Z", + }, + { + ID: "NOTE2", + User: pagerduty.APIObject{Summary: "User 2"}, + Content: "Second note", + CreatedAt: "2025-01-02T00:00:00Z", + }, + }, + expected: []noteSummary{ + { + ID: "NOTE1", + User: "User 1", + Content: "First note", + Created: "2025-01-01T00:00:00Z", + }, + { + ID: "NOTE2", + User: "User 2", + Content: "Second note", + Created: "2025-01-02T00:00:00Z", + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := summarizeNotes(test.input) + assert.Equal(t, test.expected, result) + }) + } +} + +func TestSummarizeIncident(t *testing.T) { + tests := []struct { + name string + input *pagerduty.Incident + expected incidentSummary + }{ + { + name: "basic incident without priority", + input: &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ + ID: "INC123", + HTMLURL: "https://example.pagerduty.com/incidents/INC123", + }, + Title: "Test incident", + Service: pagerduty.APIObject{Summary: "Test Service"}, + EscalationPolicy: pagerduty.APIObject{Summary: "Test Policy"}, + CreatedAt: "2025-01-01T00:00:00Z", + Urgency: "high", + Status: "triggered", + }, + expected: incidentSummary{ + ID: "INC123", + Title: "Test incident", + HTMLURL: "https://example.pagerduty.com/incidents/INC123", + Service: "Test Service", + EscalationPolicy: "Test Policy", + Created: "2025-01-01T00:00:00Z", + Urgency: "high", + Priority: "", + Status: "triggered", + Teams: nil, + Assigned: nil, + Acknowledged: nil, + }, + }, + { + name: "incident with priority and assignments", + input: &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ + ID: "INC456", + HTMLURL: "https://example.pagerduty.com/incidents/INC456", + }, + Title: "High priority incident", + Service: pagerduty.APIObject{Summary: "Critical Service"}, + EscalationPolicy: pagerduty.APIObject{Summary: "Critical Policy"}, + CreatedAt: "2025-01-01T00:00:00Z", + Urgency: "high", + Status: "acknowledged", + Priority: &pagerduty.Priority{APIObject: pagerduty.APIObject{Summary: "P1"}}, + Assignments: []pagerduty.Assignment{ + {Assignee: pagerduty.APIObject{Summary: "John Doe"}}, + {Assignee: pagerduty.APIObject{Summary: "Jane Smith"}}, + }, + Acknowledgements: []pagerduty.Acknowledgement{ + {Acknowledger: pagerduty.APIObject{Summary: "John Doe"}}, + }, + }, + expected: incidentSummary{ + ID: "INC456", + Title: "High priority incident", + HTMLURL: "https://example.pagerduty.com/incidents/INC456", + Service: "Critical Service", + EscalationPolicy: "Critical Policy", + Created: "2025-01-01T00:00:00Z", + Urgency: "high", + Priority: "P1", + Status: "acknowledged", + Teams: nil, + Assigned: []string{"John Doe", "Jane Smith"}, + Acknowledged: []string{"John Doe"}, + }, + }, + { + name: "incident with duplicate acknowledgements", + input: &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ + ID: "INC789", + HTMLURL: "https://example.com", + }, + Title: "Test", + Service: pagerduty.APIObject{Summary: "Service"}, + EscalationPolicy: pagerduty.APIObject{Summary: "Policy"}, + CreatedAt: "2025-01-01T00:00:00Z", + Urgency: "low", + Status: "acknowledged", + Acknowledgements: []pagerduty.Acknowledgement{ + {Acknowledger: pagerduty.APIObject{Summary: "John Doe"}}, + {Acknowledger: pagerduty.APIObject{Summary: "John Doe"}}, + {Acknowledger: pagerduty.APIObject{Summary: "Jane Smith"}}, + }, + }, + expected: incidentSummary{ + ID: "INC789", + Title: "Test", + HTMLURL: "https://example.com", + Service: "Service", + EscalationPolicy: "Policy", + Created: "2025-01-01T00:00:00Z", + Urgency: "low", + Priority: "", + Status: "acknowledged", + Teams: nil, + Assigned: nil, + Acknowledged: []string{"John Doe", "Jane Smith"}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := summarizeIncident(test.input) + assert.Equal(t, test.expected, result) + }) + } +} + +func TestAddNoteTemplate(t *testing.T) { + tests := []struct { + name string + id string + title string + service string + expectedContains []string + }{ + { + name: "generates note template", + id: "INC123", + title: "Test Incident", + service: "Test Service", + expectedContains: []string{ + "Incident: INC123", + "Summary: Test Incident", + "Service: Test Service", + "Please enter the note message content above", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result, err := addNoteTemplate(test.id, test.title, test.service) + assert.NoError(t, err) + for _, expected := range test.expectedContains { + assert.Contains(t, result, expected) + } + }) + } +} From 034493c2638717a0d9cdfe55d8b5944759664e61 Mon Sep 17 00:00:00 2001 From: Chris Collins Date: Tue, 18 Nov 2025 12:40:56 -1000 Subject: [PATCH 4/4] Fix golangci-lint errors and add lint to test target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes ineffectual assignment warnings from golangci-lint and ensures linting runs before tests to catch issues early. Changes: - pkg/tui/msgHandlers.go: - Remove ineffectual assignments to handledKey in switchIncidentFocusMode() - All removed assignments were in cases that return immediately, making the assignment unreachable and unnecessary - Affected cases: Refresh, Ack, UnAck, Silence, Note, Login, Open - handledKey is only checked at the end of the function to determine whether to pass the message to the viewport - Makefile: - Add lint as a dependency to the test target - Ensures golangci-lint runs before tests to catch linting issues early - Updated help text to "Run tests (after linting)" Testing: - All golangci-lint checks pass (0 issues) - All unit tests pass - `make test` now runs linting automatically before tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- Makefile | 2 +- pkg/tui/msgHandlers.go | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 26f61bd..5877218 100644 --- a/Makefile +++ b/Makefile @@ -45,7 +45,7 @@ tidy: ## Tidy up go modules go mod tidy .PHONY: test -test: ## Run tests +test: lint ## Run tests (after linting) @echo "Running tests..." go test ./... -v $(TESTOPTS) diff --git a/pkg/tui/msgHandlers.go b/pkg/tui/msgHandlers.go index 3f5269a..0405d2a 100644 --- a/pkg/tui/msgHandlers.go +++ b/pkg/tui/msgHandlers.go @@ -364,23 +364,18 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil case key.Matches(msg, defaultKeyMap.Refresh): - handledKey = true return m, func() tea.Msg { return getIncidentMsg(m.selectedIncident.ID) } case key.Matches(msg, defaultKeyMap.Ack): - handledKey = true return m, func() tea.Msg { return acknowledgeIncidentsMsg{} } case key.Matches(msg, defaultKeyMap.UnAck): - handledKey = true return m, func() tea.Msg { return unAcknowledgeIncidentsMsg{} } case key.Matches(msg, defaultKeyMap.Silence): - handledKey = true return m, func() tea.Msg { return silenceSelectedIncidentMsg{} } case key.Matches(msg, defaultKeyMap.Note): - handledKey = true // Note template requires full incident data (HTMLURL, Title, Service.Summary) if !m.incidentDataLoaded { m.setStatus("Loading incident details, please wait...") @@ -389,7 +384,6 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return m, func() tea.Msg { return parseTemplateForNoteMsg("add note") } case key.Matches(msg, defaultKeyMap.Login): - handledKey = true // Login requires alerts to extract cluster_id if !m.incidentAlertsLoaded { m.setStatus("Loading incident alerts, please wait...") @@ -398,7 +392,6 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { return m, func() tea.Msg { return loginMsg("login") } case key.Matches(msg, defaultKeyMap.Open): - handledKey = true // Browser open requires HTMLURL from full incident data if !m.incidentDataLoaded { m.setStatus("Loading incident details, please wait...")