server/sse.go (2)

127-138: Good implementation of option functions

The implementation of WithKeepAliveInterval and WithKeepAlive follows the existing pattern in the codebase. I like how WithKeepAliveInterval automatically enables keep-alive when setting the interval.

Consider adding documentation comments for these exported functions to explain their purpose and behavior.

+// WithKeepAliveInterval sets the interval for keep-alive pings and enables the keep-alive feature
 func WithKeepAliveInterval(keepAliveInterval time.Duration) SSEOption {
     return func(s *SSEServer) {
         s.keepAlive = true
         s.keepAliveInterval = keepAliveInterval
     }
 }

+// WithKeepAlive enables or disables the keep-alive ping mechanism
 func WithKeepAlive(keepAlive bool) SSEOption {
     return func(s *SSEServer) {
         s.keepAlive = keepAlive
     }
 }

---

266-282: Well-implemented keep-alive mechanism

The keep-alive implementation correctly:

1. Only runs when the feature is enabled
2. Uses a ticker with the configured interval
3. Properly cleans up resources with defer ticker.Stop()
4. Handles termination conditions through multiple select cases
5. Formats ping events according to SSE standards

Consider adding validation for very short keep-alive intervals that could impact performance, and consider handling the case where the event queue might be full (similar to the check in SendEventToSession).

 // Start keep alive : ping
 if s.keepAlive {
+    // Ensure minimum interval to prevent performance issues
+    if s.keepAliveInterval < 100 * time.Millisecond {
+        s.keepAliveInterval = 100 * time.Millisecond
+    }
     go func() {
         ticker := time.NewTicker(s.keepAliveInterval)
         defer ticker.Stop()
         for {
             select {
             case <-ticker.C:
-                session.eventQueue <- fmt.Sprintf("event: ping\ndata: %s\n\n", time.Now().Format(time.RFC3339))
+                select {
+                    case session.eventQueue <- fmt.Sprintf("event: ping\ndata: %s\n\n", time.Now().Format(time.RFC3339)):
+                        // Event queued successfully
+                    case <-session.done:
+                        return
+                    default:
+                        // Queue is full, could log this
+                }
             case <-session.done:
                 return
             case <-r.Context().Done():
                 return
             }
         }
     }()
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/sse.go (2)

137-142: Consider making behavior more explicit

WithKeepAliveInterval implicitly enables keep-alive by setting s.keepAlive = true. This implicit behavior might not be immediately obvious to users.

Consider either:

1. Renaming to WithEnabledKeepAliveInterval to make it clear that this also enables the feature, or
2. Adding a comment that documents this behavior

 func WithKeepAliveInterval(keepAliveInterval time.Duration) SSEOption {
 	return func(s *SSEServer) {
+		// Setting an interval implicitly enables keep-alive
 		s.keepAlive = true
 		s.keepAliveInterval = keepAliveInterval
 	}
 }

---

286-286: Consider making the ping format customizable

The current implementation uses a fixed format for ping messages (:ping - <timestamp>). In some scenarios, it might be beneficial to allow customization of this format.

While not necessary for the current requirements, a future enhancement could allow for customizable ping formats:

// Add to SSEServer struct
+	pingFormat string

// Add new option
+func WithPingFormat(format string) SSEOption {
+	return func(s *SSEServer) {
+		s.pingFormat = format
+	}
+}

// Initialize in NewSSEServer
+		pingFormat:        ":ping - %s\n\n",

// Use in ping message
-					session.eventQueue <- fmt.Sprintf(":ping - %s\n\n", time.Now().Format(time.RFC3339))
+					session.eventQueue <- fmt.Sprintf(s.pingFormat, time.Now().Format(time.RFC3339))

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/sse.go (1)

286-291: Good implementation of non-blocking ping with error logging.

The implementation now properly handles the case where the event queue is full by using a non-blocking channel send with the select statement and logging warnings for dropped pings.

server/sse.go (3)

137-142: Consider separating configuration concerns.

The WithKeepAliveInterval function automatically enables keep-alive by setting s.keepAlive = true. This could cause unexpected behavior if a user only wants to configure the interval without enabling the feature yet.

Consider separating these concerns:

func WithKeepAliveInterval(keepAliveInterval time.Duration) SSEOption {
	return func(s *SSEServer) {
-		s.keepAlive = true
		s.keepAliveInterval = keepAliveInterval
	}
}

---

278-299: Consider adding validation for keepAliveInterval.

The implementation doesn't validate the keepAliveInterval value. Very short intervals could cause excessive network traffic or overload the client, while very long intervals might not be effective at keeping connections alive with some load balancers.

Consider adding validation in the WithKeepAliveInterval function:

func WithKeepAliveInterval(keepAliveInterval time.Duration) SSEOption {
	return func(s *SSEServer) {
+		// Ensure reasonable limits for the interval
+		if keepAliveInterval < time.Second {
+			keepAliveInterval = time.Second // Minimum 1 second
+		} else if keepAliveInterval > 2 * time.Minute {
+			keepAliveInterval = 2 * time.Minute // Maximum 2 minutes
+		}
		s.keepAlive = true
		s.keepAliveInterval = keepAliveInterval
	}
}

---

301-302: Unused variable.

The messageEndpoint variable is computed but not used in the code that follows. This appears to be leftover code from a refactoring.

Remove the unused variable:

-	messageEndpoint := fmt.Sprintf("%s?sessionId=%s", s.CompleteMessageEndpoint(), sessionID)
-

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/sse.go (1)

3-17: 🛠️ Refactor suggestion

Add log package to imports.

The recommended implementation for the keep-alive functionality uses log.Printf() for error handling, but the "log" package is not imported.

import (
	"context"
	"encoding/json"
	"fmt"
+	"log"
	"net/http"
	"net/http/httptest"
	"net/url"
	"strings"
	"sync"
	"sync/atomic"
	"time"

	"github.com/google/uuid"
	"github.com/mark3labs/mcp-go/mcp"
)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/sse.go (2)

137-142: Inconsistent behavior in WithKeepAliveInterval method.

The method automatically sets keepAlive to true even if the user has explicitly disabled it with WithKeepAlive(false). This creates an unintuitive API behavior where the order of option application affects the outcome.

func WithKeepAliveInterval(keepAliveInterval time.Duration) SSEOption {
 	return func(s *SSEServer) {
-		s.keepAlive = true
 		s.keepAliveInterval = keepAliveInterval
 	}
}

---

277-294: ⚠️ Potential issue

Add error handling for dropped keep-alive pings.

The current implementation doesn't handle the case when the event queue is full, unlike how notification events are handled elsewhere in the code. Additionally, there's a comment that appears to be debug/example output that should be removed.

if s.keepAlive {
	go func() {
		ticker := time.NewTicker(s.keepAliveInterval)
		defer ticker.Stop()
		for {
			select {
			case <-ticker.C:
-				//: ping - 2025-03-27 07:44:38.682659+00:00
-				session.eventQueue <- fmt.Sprintf(":ping - %s\n\n", time.Now().Format(time.RFC3339))
+				select {
+				case session.eventQueue <- fmt.Sprintf(":ping - %s\n\n", time.Now().Format(time.RFC3339)):
+					// Ping sent successfully
+				default:
+					// Queue is full, could log this or take appropriate action
+				}
			case <-session.done:
				return
			case <-r.Context().Done():
				return
			}
		}
	}()
}

server/sse.go (1)

296-296: Remove extra blank line.

This extra blank line doesn't serve any purpose and should be removed for consistency.

	}

-

	// Send the initial endpoint event

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro