Skip to content

Conversation

@liady
Copy link
Collaborator

@liady liady commented May 12, 2025

No description provided.

Copilot AI review requested due to automatic review settings May 12, 2025 19:45
Copy link
Contributor

Copilot AI left a 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 introduces HTTP streaming support by refactoring the request handling logic and replacing the legacy mount-based handler with dedicated serveSSE and serve endpoints. Key changes include:

  • Removal of the mcpHandler variable and switching to direct calls to MyMCP.serveSSE()/serve.
  • Renaming the streaming check variable from isSse to isStreamMethod for clarity.
  • Bumping dependency versions in package.json.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
src/index.ts Refactored stream handling logic and variable renaming for HTTP streaming support.
package.json Upgraded @modelcontextprotocol/sdk and agents dependencies to newer versions.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)

src/index.ts:161

  • [nitpick] Consider renaming this variable to something like 'isGetRequest' to avoid confusion with the previously renamed 'isStreamMethod', which indicates the overall streaming condition.
const isSse = request.method === "GET";

@idosal idosal requested a review from Copilot May 12, 2025 21:13
Copy link
Contributor

Copilot AI left a 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 adds support for HTTP streaming by refactoring the request handling logic and updating associated dependencies. Key changes include:

  • Refactoring of the request parsing in MyMCP to use a new property (requestUrl).
  • Adjusting the logic for streaming requests by renaming and re-routing requests to the proper streaming handler.
  • Updating dependency versions in package.json and changing the DefaultRepoHandler parameter schema from undefined to an empty object.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
src/index.ts Refactored request handling and streaming routing logic in the MyMCP class.
src/api/tools/repoHandlers/DefaultRepoHandler.ts Revised parameter schema assignment to improve type consistency.
package.json Updated dependency versions to align with current releases.
Comments suppressed due to low confidence (2)

src/index.ts:140

  • [nitpick] The variable 'isStreamMethod' is introduced while a similarly purposed variable 'isSse' is defined later. For clarity, consider using a consistent naming convention (e.g., renaming 'isSse' to 'isGetMethod').
const isStreamMethod = request.headers.get("accept")?.includes("text/event-stream") &&

src/api/tools/repoHandlers/DefaultRepoHandler.ts:33

  • Changing 'paramsSchema' from undefined to an empty object assumes that the callback (fetchDocumentation) correctly handles a defined schema. Please confirm that the behavior remains consistent with expected schema validation or usage.
paramsSchema: {}

@idosal idosal requested a review from Copilot May 13, 2025 19:04
Copy link
Contributor

Copilot AI left a 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 adds support for HTTP streaming, enhancing the handling of server‐sent events and related request routing while updating dependency versions and tool schemas.

  • Introduces streaming support and updates SSE handling logic in src/index.ts.
  • Refactors tool parameter schema handling in DefaultRepoHandler.ts.
  • Updates dependency versions in package.json.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
src/index.ts Adjusts request URL parsing, updates streaming and SSE routing, and renames variables to clarify processing method.
src/api/tools/repoHandlers/DefaultRepoHandler.ts Updates the tool’s parameter schema from undefined to a union type to explicitly allow an empty object or null.
package.json Bumps version numbers for @modelcontextprotocol/sdk, agents, @cloudflare/workers-types, and wrangler to support new features.
Comments suppressed due to low confidence (1)

src/index.ts:156

  • [nitpick] The variable 'isSse' here solely checks if the request method is GET, which may cause confusion given its earlier use in determining SSE support via headers. Consider renaming it to 'isGetMethod' or a similar name for clearer intent.
const isSse = request.method === "GET";

@idosal idosal merged commit 228b32c into main May 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants