chore: address more comments from #361 #389
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses code review feedback from issue #361 by improving function naming and simplifying error handling. The changes focus on making the code more maintainable and consistent.
- Renamed
handleRequesttohandleSessionRequestfor better clarity about the function's purpose - Simplified error handling in the main function by removing redundant logging and cleanup logic
- Updated all references to use the new function name consistently
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/transports/streamableHttp.ts | Renamed handleRequest function to handleSessionRequest and updated all call sites |
| src/index.ts | Simplified error handling by removing duplicate cleanup logic and redundant logging |
src/index.ts
Outdated
| } finally { | ||
| process.exit(1); | ||
| } | ||
| await transportRunner.close(); |
There was a problem hiding this comment.
The transportRunner.close() call in the catch block may fail and throw an error, which would mask the original error. Consider wrapping this in a try-catch block to ensure the original error is preserved and re-thrown.
| await transportRunner.close(); | |
| try { | |
| await transportRunner.close(); | |
| } catch (closeError: unknown) { | |
| logger.error(LogId.serverCloseFailure, "server", `Error closing server during shutdown: ${closeError as string}`); | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
hmm.. this is a good point, maybe we can log the error beforehand? I guess we can also preserve the old structure
Pull Request Test Coverage Report for Build 16449914667Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
I guess we can omit the |
Proposed changes
address more comments from #361
Checklist