-
Notifications
You must be signed in to change notification settings - Fork 97
mcp: add stream type #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mcp: add stream type #171
Conversation
Consolidate several maps into a single struct. Simplifies the code, for the most part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR consolidates the streamable server transport implementation by introducing a single stream
struct to replace multiple separate maps. The main purpose is to simplify the code structure and improve maintainability by grouping related stream data together.
- Replaces three separate maps (
outgoing
,signals
,streamRequests
) with a singlestreams
map containingstream
structs - Introduces a new
stream
type that encapsulates all stream-related state and behavior - Updates all stream-related operations to work with the consolidated structure
t.mu.Unlock() | ||
|
||
for _, data := range outgoing { | ||
if err := t.opts.EventStore.Append(req.Context(), t.id, id, data); err != nil { | ||
if err := t.opts.EventStore.Append(req.Context(), t.SessionID(), stream.id, data); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent method call: using t.SessionID()
here but t.id
was used in the original code. This may cause incorrect session identification in the event store.
if err := t.opts.EventStore.Append(req.Context(), t.SessionID(), stream.id, data); err != nil { | |
if err := t.opts.EventStore.Append(req.Context(), t.SessionID(), t.SessionID(), data); err != nil { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice LGTM!
Consolidate several maps into a single struct.
Simplifies the code, for the most part.