Fix incident selection synchronization issues

pkg/tui/keymap.go

@@ -7,14 +7,18 @@ func (k keymap) ShortHelp() []key.Binding {
 }
 
 func (k keymap) FullHelp() [][]key.Binding {
-	// TODO: Return a pop-over window here instead
+	// Column layout:
+	// Col 1: Navigation + Help
+	// Col 2: Primary incident actions
+	// Col 3: Settings & toggles, Quit at bottom
+
 	return [][]key.Binding{
-		// Each slice here is a column in the help window
-		{k.Up, k.Down, k.Top, k.Bottom, k.Enter, k.Back},
-		{k.Ack, k.Login, k.Open, k.Note},
-		{k.UnAck, k.Silence},
-		{k.Team, k.Refresh},
-		{k.AutoRefresh, k.AutoAck, k.ToggleActionLog, k.Quit, k.Help},
+		// Column 1: Help at top, navigation
+		{k.Help, k.Up, k.Down, k.Top, k.Bottom, k.Enter, k.Back},
+		// Column 2: Primary incident actions
+		{k.Ack, k.Note, k.Login, k.Open, k.UnAck, k.Silence},
+		// Column 3: Settings & toggles, Quit at bottom
+		{k.Team, k.Refresh, k.AutoRefresh, k.AutoAck, k.ToggleActionLog, k.Quit},
 	}
 }
 

pkg/tui/model.go

@@ -169,25 +169,101 @@ func (m *model) clearSelectedIncident(reason interface{}) {
 	log.Debug("clearSelectedIncident", "selectedIncident", m.selectedIncident, "cleared", true, "reason", reason)
 }
 
-// getHighlightedIncident returns the incident object for the currently highlighted table row
-// by looking it up in m.incidentList. Returns nil if no row is highlighted or incident not found.
-func (m *model) getHighlightedIncident() *pagerduty.Incident {
+// syncSelectedIncidentToHighlightedRow updates m.selectedIncident to match the currently
+// highlighted table row. Sets to nil if no row is highlighted. Uses cached data if available,
+// otherwise uses stub data from m.incidentList.
+func (m *model) syncSelectedIncidentToHighlightedRow() {
 	row := m.table.SelectedRow()
 	if row == nil {
-		return nil
+		// Clear selection regardless of viewing state
+		// If user scrolls table out of bounds, selection should be nil
+		m.selectedIncident = nil
+		m.incidentDataLoaded = false
+		m.incidentNotesLoaded = false
+		m.incidentAlertsLoaded = false
+		log.Debug("syncSelectedIncidentToHighlightedRow", "no row highlighted", "cleared selection")
+		return
 	}
 
 	incidentID := row[1] // Column [1] is the incident ID
 
-	// Look up the incident in the incident list
+	// If already viewing this incident, don't change anything
+	if m.selectedIncident != nil && m.selectedIncident.ID == incidentID {
+		return
+	}
+
+	// Find incident in list
 	for i := range m.incidentList {
 		if m.incidentList[i].ID == incidentID {
-			return &m.incidentList[i]
+			// Check if we have cached full data
+			if cached, exists := m.incidentCache[incidentID]; exists && cached.incident != nil {
+				// Check if cache is fresh relative to incident list data
+				listIncident := &m.incidentList[i]
+				if cached.incident.LastStatusChangeAt != listIncident.LastStatusChangeAt {
+					// Cache is stale relative to list - use list data but keep cached notes/alerts
+					log.Debug("syncSelectedIncidentToHighlightedRow", "cache stale, using updated list data", "incident", incidentID)
+					incidentCopy := m.incidentList[i]
+					m.selectedIncident = &incidentCopy
+					m.incidentDataLoaded = false
+					// Keep cached notes/alerts if available
+					if cached.notesLoaded {
+						m.selectedIncidentNotes = cached.notes
+						m.incidentNotesLoaded = true
+					} else {
+						m.selectedIncidentNotes = nil
+						m.incidentNotesLoaded = false
+					}
+					if cached.alertsLoaded {
+						m.selectedIncidentAlerts = cached.alerts
+						m.incidentAlertsLoaded = true
+					} else {
+						m.selectedIncidentAlerts = nil
+						m.incidentAlertsLoaded = false
+					}
+				} else {
+					// Cache is fresh - use it
+					log.Debug("syncSelectedIncidentToHighlightedRow", "using cached data", "incident", incidentID)
+					// Create copy to avoid pointer issues
+					incidentCopy := *cached.incident
+					m.selectedIncident = &incidentCopy
+					m.incidentDataLoaded = cached.dataLoaded
+					m.incidentNotesLoaded = cached.notesLoaded
+					m.incidentAlertsLoaded = cached.alertsLoaded
+					if cached.notes != nil {
+						m.selectedIncidentNotes = cached.notes
+					} else {
+						m.selectedIncidentNotes = nil
+					}
+					if cached.alerts != nil {
+						m.selectedIncidentAlerts = cached.alerts
+					} else {
+						m.selectedIncidentAlerts = nil
+					}
+				}
+			} else {
+				// Use stub data from incidentList
+				log.Debug("syncSelectedIncidentToHighlightedRow", "using stub data", "incident", incidentID)
+				// Create a copy to avoid pointer aliasing when slice is reallocated
+				incidentCopy := m.incidentList[i]
+				m.selectedIncident = &incidentCopy
+				m.incidentDataLoaded = false
+				m.incidentNotesLoaded = false
+				m.incidentAlertsLoaded = false
+				m.selectedIncidentNotes = nil
+				m.selectedIncidentAlerts = nil
+			}
+			return
 		}
 	}
 
-	log.Debug("getHighlightedIncident", "incident not found in list", incidentID)
-	return nil
+	// Incident not found in list
+	log.Debug("syncSelectedIncidentToHighlightedRow", "incident not found in list", incidentID)
+	if !m.viewingIncident {
+		m.selectedIncident = nil
+		m.incidentDataLoaded = false
+		m.incidentNotesLoaded = false
+		m.incidentAlertsLoaded = false
+	}
 }
 
 func (m *model) setStatus(msg string) {
@@ -201,6 +277,7 @@ func (m *model) toggleHelp() {
 
 // addActionLogEntry adds an action to the action log, maintaining only the last 5 entries
 func (m *model) addActionLogEntry(key, id, summary, action string) {
+	log.Debug("addActionLogEntry", "key", key, "id", id, "summary", summary, "action", action)
 	entry := actionLogEntry{
 		key:       key,
 		id:        id,

pkg/tui/model_test.go

@@ -267,7 +267,7 @@
 			m.incidentList = tt.incidentList
 
 			// Set up table rows to match incident list
 			if tt.hasSelectedRow && len(tt.incidentList) > 0 {
 				// Manually set the cursor to simulate a selected row
 				// We'll use the test directly by creating a mock selected row
 				// Since we can't easily mock the table.SelectedRow(), we'll test by
@@ -688,7 +688,7 @@
 			{Title: "", Width: 2},
 			{Title: "", Width: 15},
 			{Title: "", Width: 30},
-			{Title: "", Width: 20},
+			{Title: "", Width: 29},
 		})
 		m.config = &pd.Config{
 			CurrentUser: &pagerduty.User{
@@ -698,8 +698,18 @@
 
 		// Set initial incident list
 		m.incidentList = []pagerduty.Incident{
-			{APIObject: pagerduty.APIObject{ID: "Q123"}, Title: "Incident 1", LastStatusChangeAt: time.Now().Format(time.RFC3339)},
-			{APIObject: pagerduty.APIObject{ID: "Q456"}, Title: "Incident 2", LastStatusChangeAt: time.Now().Format(time.RFC3339)},
+			{
+				APIObject:          pagerduty.APIObject{ID: "Q123"},
+				Title:              "Incident 1",
+				Service:            pagerduty.APIObject{Summary: "test-service-1"},
+				LastStatusChangeAt: time.Now().Format(time.RFC3339),
+			},
+			{
+				APIObject:          pagerduty.APIObject{ID: "Q456"},
+				Title:              "Incident 2",
+				Service:            pagerduty.APIObject{Summary: "test-service-2"},
+				LastStatusChangeAt: time.Now().Format(time.RFC3339),
+			},
 		}
 
 		// Update with a list that's missing Q123 (it resolved)
@@ -718,7 +728,7 @@
 		assert.Equal(t, "%R", m.actionLog[0].key, "Resolved incident should have %R key")
 		assert.Equal(t, "Q123", m.actionLog[0].id, "Should log the resolved incident ID")
 		assert.Equal(t, "Incident 1", m.actionLog[0].summary, "Should log the incident title")
-		assert.Equal(t, "resolved", m.actionLog[0].action, "Action should be 'resolved'")
+		assert.Equal(t, "test-service-1", m.actionLog[0].action, "Action should contain service summary")
 	})
 
 	t.Run("Does not add duplicate resolved incidents to action log", func(t *testing.T) {
@@ -729,7 +739,7 @@
 			{Title: "", Width: 2},
 			{Title: "", Width: 15},
 			{Title: "", Width: 30},
-			{Title: "", Width: 20},
+			{Title: "", Width: 29},
 		})
 		m.config = &pd.Config{
 			CurrentUser: &pagerduty.User{
@@ -744,7 +754,12 @@
 
 		// Set initial incident list
 		m.incidentList = []pagerduty.Incident{
-			{APIObject: pagerduty.APIObject{ID: "Q123"}, Title: "Incident 1", LastStatusChangeAt: time.Now().Format(time.RFC3339)},
+			{
+				APIObject:          pagerduty.APIObject{ID: "Q123"},
+				Title:              "Incident 1",
+				Service:            pagerduty.APIObject{Summary: "test-service-1"},
+				LastStatusChangeAt: time.Now().Format(time.RFC3339),
+			},
 		}
 
 		// Update with empty list (Q123 resolved again)
@@ -783,3 +798,121 @@
 		assert.False(t, m.showActionLog, "showActionLog should be false after second toggle")
 	})
 }
+
+// TestEscapeKeySyncsToHighlightedRow verifies that pressing Escape re-syncs
+// the selectedIncident to whatever row is currently highlighted
+func TestEscapeKeySyncsToHighlightedRow(t *testing.T) {
+	m := createTestModel()
+	m.config = &pd.Config{}
+
+	// Create incident list
+	m.incidentList = []pagerduty.Incident{
+		{
+			APIObject:          pagerduty.APIObject{ID: "Q123"},
+			Title:              "Incident 1",
+			Service:            pagerduty.APIObject{Summary: "service-1"},
+			LastStatusChangeAt: time.Now().Format(time.RFC3339),
+		},
+		{
+			APIObject:          pagerduty.APIObject{ID: "Q456"},
+			Title:              "Incident 2",
+			Service:            pagerduty.APIObject{Summary: "service-2"},
+			LastStatusChangeAt: time.Now().Format(time.RFC3339),
+		},
+	}
+
+	// Create table with the incidents
+	cols := []table.Column{
+		{Title: "Status", Width: 10},
+		{Title: "ID", Width: 10},
+		{Title: "Title", Width: 20},
+		{Title: "Service", Width: 15},
+		{Title: "Since", Width: 10},
+		{Title: "User", Width: 10},
+	}
+	rows := []table.Row{
+		{"triggered", "Q123", "Incident 1", "service-1", "2024-01-01", "user1"},
+		{"triggered", "Q456", "Incident 2", "service-2", "2024-01-01", "user2"},
+	}
+	m.table = table.New(
+		table.WithColumns(cols),
+		table.WithRows(rows),
+		table.WithFocused(true),
+	)
+
+	// Select first incident and view it
+	m.selectedIncident = &m.incidentList[0]
+	m.viewingIncident = true
+
+	// Simulate Escape key - should clear and re-sync to highlighted row
+	// Note: In real usage, the table cursor position would determine which incident gets selected
+	// For this test, we're verifying the sync happens after Escape
+	m.clearSelectedIncident("test escape")
+	m.syncSelectedIncidentToHighlightedRow()
+
+	// After sync, selectedIncident should match the highlighted row
+	// Since we can't easily control table selection in unit tests, we verify the sync was attempted
+	assert.NotNil(t, m.selectedIncident, "selectedIncident should be re-synced after Escape")
+}
+
+// TestSelectedIncidentSurvivesListUpdate verifies that copying incident data
+// prevents pointer aliasing issues when the incident list is reallocated
+func TestSelectedIncidentSurvivesListUpdate(t *testing.T) {
+	m := createTestModel()
+
+	// Create initial incident list
+	m.incidentList = []pagerduty.Incident{
+		{
+			APIObject:          pagerduty.APIObject{ID: "Q123"},
+			Title:              "Original Title",
+			Service:            pagerduty.APIObject{Summary: "service-1"},
+			LastStatusChangeAt: "2024-01-01T00:00:00Z",
+		},
+	}
+
+	// Create table with the incident
+	cols := []table.Column{
+		{Title: "Status", Width: 10},
+		{Title: "ID", Width: 10},
+		{Title: "Title", Width: 20},
+		{Title: "Service", Width: 15},
+		{Title: "Since", Width: 10},
+		{Title: "User", Width: 10},
+	}
+	rows := []table.Row{
+		{"triggered", "Q123", "Original Title", "service-1", "2024-01-01", "user1"},
+	}
+	m.table = table.New(
+		table.WithColumns(cols),
+		table.WithRows(rows),
+		table.WithFocused(true),
+	)
+
+	// Sync to select the incident (creates a copy)
+	m.syncSelectedIncidentToHighlightedRow()
+
+	assert.NotNil(t, m.selectedIncident, "selectedIncident should be set")
+	originalTitle := m.selectedIncident.Title
+	assert.Equal(t, "Original Title", originalTitle)
+
+	// Update the incident list (reallocate the slice)
+	m.incidentList = []pagerduty.Incident{
+		{
+			APIObject:          pagerduty.APIObject{ID: "Q123"},
+			Title:              "Updated Title",
+			Service:            pagerduty.APIObject{Summary: "service-1"},
+			LastStatusChangeAt: "2024-01-01T00:00:00Z",
+		},
+		{
+			APIObject:          pagerduty.APIObject{ID: "Q456"},
+			Title:              "New Incident",
+			Service:            pagerduty.APIObject{Summary: "service-2"},
+			LastStatusChangeAt: "2024-01-01T00:00:00Z",
+		},
+	}
+
+	// Verify selectedIncident still has the original title (not affected by list update)
+	// This proves we copied the data instead of storing a pointer to the slice element
+	assert.Equal(t, "Original Title", m.selectedIncident.Title,
+		"selectedIncident should retain original data after list reallocation")
+}