Multiple changes to fix errors and responsiveness, sanitize log messages

cmd/root.go

@@ -194,35 +194,63 @@ func initConfig() {
 	}
 }
 
-func configureLogging() {
-	log.SetPrefix("srepd")
+// LogDestination represents where logs should be written
+type LogDestination int
 
-	switch runtime.GOOS {
+const (
+	LogToJournal LogDestination = iota
+	LogToFile
+	LogToStderr
+)
+
+// determineLogDestination returns the appropriate log destination based on OS and config
+func determineLogDestination(goos string, logToJournal bool, journalEnabled bool) (LogDestination, string) {
+	switch goos {
 	case "linux":
-		// Check if running under systemd
-		if journal.Enabled() {
-			log.SetOutput(journalWriter{})
-			log.Info("Logging to systemd journal")
-			return
+		if logToJournal {
+			if journalEnabled {
+				return LogToJournal, ""
+			}
+			return LogToFile, "/var/log/srepd.log"
 		}
-
-		// Fallback to /var/log/srepd.log for non-systemd Linux
-		logFile := "/var/log/srepd.log"
-		setupFileLogging(logFile)
-		log.Info("Logging to /var/log/srepd.log")
+		// User explicitly wants file logging
+		return LogToFile, "~/.config/srepd/debug.log"
 
 	case "darwin":
-		// macOS: Log to ~/Library/Logs/srepd.log
-		home, err := os.UserHomeDir()
-		if err != nil {
-			log.Fatal("Failed to get user home directory:", err)
-		}
-		logFile := home + "/Library/Logs/srepd.log"
-		setupFileLogging(logFile)
-		log.Info("Logging to ~/Library/Logs/srepd.log")
+		return LogToFile, "~/Library/Logs/srepd.log"
 
 	default:
-		// Default fallback for other OSes
+		return LogToStderr, ""
+	}
+}
+
+func configureLogging() {
+	log.SetPrefix("srepd")
+
+	// Check if user wants to log to journal (default: true)
+	viper.SetDefault("log_to_journal", true)
+	logToJournal := viper.GetBool("log_to_journal")
+
+	dest, logPath := determineLogDestination(runtime.GOOS, logToJournal, journal.Enabled())
+
+	switch dest {
+	case LogToJournal:
+		log.SetOutput(journalWriter{})
+		log.Info("Logging to systemd journal")
+
+	case LogToFile:
+		// Expand home directory if needed
+		if strings.HasPrefix(logPath, "~/") {
+			home, err := os.UserHomeDir()
+			if err != nil {
+				log.Fatal("Failed to get user home directory:", err)
+			}
+			logPath = home + logPath[1:]
+		}
+		setupFileLogging(logPath)
+		log.Info("Logging to " + logPath)
+
+	case LogToStderr:
 		log.SetOutput(os.Stderr)
 		log.Warn("Unsupported OS: logging to stderr")
 	}

cmd/root_test.go

@@ -0,0 +1,91 @@
+package cmd
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDetermineLogDestination(t *testing.T) {
+	tests := []struct {
+		name           string
+		goos           string
+		logToJournal   bool
+		journalEnabled bool
+		expectedDest   LogDestination
+		expectedPath   string
+	}{
+		{
+			name:           "Linux with journal enabled and log_to_journal=true",
+			goos:           "linux",
+			logToJournal:   true,
+			journalEnabled: true,
+			expectedDest:   LogToJournal,
+			expectedPath:   "",
+		},
+		{
+			name:           "Linux with journal disabled and log_to_journal=true",
+			goos:           "linux",
+			logToJournal:   true,
+			journalEnabled: false,
+			expectedDest:   LogToFile,
+			expectedPath:   "/var/log/srepd.log",
+		},
+		{
+			name:           "Linux with log_to_journal=false (user wants file logging)",
+			goos:           "linux",
+			logToJournal:   false,
+			journalEnabled: true, // Journal enabled but user wants file
+			expectedDest:   LogToFile,
+			expectedPath:   "~/.config/srepd/debug.log",
+		},
+		{
+			name:           "Linux with log_to_journal=false and journal disabled",
+			goos:           "linux",
+			logToJournal:   false,
+			journalEnabled: false,
+			expectedDest:   LogToFile,
+			expectedPath:   "~/.config/srepd/debug.log",
+		},
+		{
+			name:           "macOS always logs to file",
+			goos:           "darwin",
+			logToJournal:   true, // Ignored on macOS
+			journalEnabled: false,
+			expectedDest:   LogToFile,
+			expectedPath:   "~/Library/Logs/srepd.log",
+		},
+		{
+			name:           "macOS with log_to_journal=false",
+			goos:           "darwin",
+			logToJournal:   false,
+			journalEnabled: false,
+			expectedDest:   LogToFile,
+			expectedPath:   "~/Library/Logs/srepd.log",
+		},
+		{
+			name:           "Unsupported OS logs to stderr",
+			goos:           "windows",
+			logToJournal:   true,
+			journalEnabled: false,
+			expectedDest:   LogToStderr,
+			expectedPath:   "",
+		},
+		{
+			name:           "Unknown OS logs to stderr",
+			goos:           "freebsd",
+			logToJournal:   false,
+			journalEnabled: false,
+			expectedDest:   LogToStderr,
+			expectedPath:   "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dest, path := determineLogDestination(tt.goos, tt.logToJournal, tt.journalEnabled)
+			assert.Equal(t, tt.expectedDest, dest, "Log destination mismatch")
+			assert.Equal(t, tt.expectedPath, path, "Log path mismatch")
+		})
+	}
+}

pkg/tui/commands.go

@@ -79,8 +79,9 @@ func getIncident(p *pd.Config, id string) tea.Cmd {
 
 // gotIncidentAlertsMsg is a message that contains the fetched incident alerts
 type gotIncidentAlertsMsg struct {
-	alerts []pagerduty.IncidentAlert
-	err    error
+	incidentID string
+	alerts     []pagerduty.IncidentAlert
+	err        error
 }
 
 // getIncidentAlerts returns a command that fetches the alerts for the given incident
@@ -90,14 +91,15 @@ func getIncidentAlerts(p *pd.Config, id string) tea.Cmd {
 			return setStatusMsg{nilIncidentMsg}
 		}
 		a, err := pd.GetAlerts(p.Client, id, pagerduty.ListIncidentAlertsOptions{})
-		return gotIncidentAlertsMsg{a, err}
+		return gotIncidentAlertsMsg{incidentID: id, alerts: a, err: err}
 	}
 }
 
 // gotIncidentNotesMsg is a message that contains the fetched incident notes
 type gotIncidentNotesMsg struct {
-	notes []pagerduty.IncidentNote
-	err   error
+	incidentID string
+	notes      []pagerduty.IncidentNote
+	err        error
 }
 
 // getIncidentNotes returns a command that fetches the notes for the given incident
@@ -107,7 +109,7 @@ func getIncidentNotes(p *pd.Config, id string) tea.Cmd {
 			return setStatusMsg{nilIncidentMsg}
 		}
 		n, err := pd.GetNotes(p.Client, id)
-		return gotIncidentNotesMsg{n, err}
+		return gotIncidentNotesMsg{incidentID: id, notes: n, err: err}
 	}
 }
 
@@ -327,13 +329,6 @@ func openBrowserCmd(browser []string, url string) tea.Cmd {
 
 	c := exec.Command(browser[0], args...)
 	log.Debug(fmt.Sprintf("tui.openBrowserCmd(): %v", c.String()))
-	stderr, pipeErr := c.StderrPipe()
-	if pipeErr != nil {
-		log.Debug(fmt.Sprintf("tui.openBrowserCmd(): %v", pipeErr.Error()))
-		return func() tea.Msg {
-			return browserFinishedMsg{err: pipeErr}
-		}
-	}
 
 	err := c.Start()
 	if err != nil {
@@ -343,20 +338,8 @@ func openBrowserCmd(browser []string, url string) tea.Cmd {
 		}
 	}
 
-	out, err := io.ReadAll(stderr)
-	if err != nil {
-		log.Debug(fmt.Sprintf("tui.openBrowserCmd(): %v", err.Error()))
-		return func() tea.Msg {
-			return browserFinishedMsg{err}
-		}
-	}
-
-	if len(out) > 0 {
-		log.Debug(fmt.Sprintf("tui.openBrowserCmd(): error: %s", out))
-		return func() tea.Msg {
-			return browserFinishedMsg{fmt.Errorf("%s", out)}
-		}
-	}
+	// Browser opened successfully - don't wait for it or check stderr
+	// Browsers write warnings/info to stderr that aren't actual errors
 
 	return func() tea.Msg {
 		return browserFinishedMsg{}
@@ -607,12 +590,32 @@ var errSilenceIncidentInvalidArgs = errors.New("silenceIncidents: invalid argume
 
 func silenceIncidents(incidents []pagerduty.Incident, policy *pagerduty.EscalationPolicy, level uint) tea.Cmd {
 	// SilenceIncidents doesn't have it's own "silencedIncidentsMessage" because it's really just a re-escalation
-	log.Printf("silence requested for incident(s) %v; reassigning to %s(%s), level %d", incidents, policy.Name, policy.ID, level)
-	return func() tea.Msg {
-		if len(incidents) == 0 || policy.Name == "" || level == 0 {
+	if policy == nil {
+		log.Debug("silenceIncidents: nil escalation policy provided")
+		return func() tea.Msg {
+			return errMsg{errSilenceIncidentInvalidArgs}
+		}
+	}
+
+	if len(incidents) == 0 {
+		log.Debug("silenceIncidents: no incidents provided")
+		return func() tea.Msg {
+			return errMsg{errSilenceIncidentInvalidArgs}
+		}
+	}
+
+	if policy.Name == "" || policy.ID == "" || level == 0 {
+		log.Debug("silenceIncidents: invalid arguments", "incident(s) count: %d; policy: %s(%s), level: %d", len(incidents), policy.Name, policy.ID, level)
+		return func() tea.Msg {
 			return errMsg{errSilenceIncidentInvalidArgs}
 		}
-		//
+	}
+
+	incidentIDs := getIDsFromIncidents(incidents)
+
+	log.Printf("silence requested for incident(s) %v; reassigning to %s(%s), level %d", incidentIDs, policy.Name, policy.ID, level)
+
+	return func() tea.Msg {
 		return reEscalateIncidentsMsg{incidents, policy, level}
 	}
 }
@@ -734,3 +737,11 @@ func runScheduledJobs(m *model) []tea.Cmd {
 	}
 	return cmds
 }
+
+func getIDsFromIncidents(incidents []pagerduty.Incident) []string {
+	var ids []string
+	for _, i := range incidents {
+		ids = append(ids, i.ID)
+	}
+	return ids
+}