-
Notifications
You must be signed in to change notification settings - Fork 155
fixes #1993 [Web Connector] refactor SSE, fix connection error #1995
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
Conversation
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 fixes issue #1993 where SSE connections fail with "Response head already sent" when users have missed messages in MongoDB. The fix is achieved by refactoring the SSE functionality into a dedicated connector-web-sse module and ensuring proper operation ordering.
Changes:
- Created new
connector-web-ssemodule to isolate SSE-related functionality for better maintainability - Fixed the SSE connection bug by ensuring
setupSSE()is called beforesendMissedEvents(), preventing writes before headers are configured - Migrated from deprecated
applicationIdtoconnectorIdand changedrecipientIdfromPlayerIdtoStringfor consistency
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bot/pom.xml | Added connector-web-sse module to parent POM |
| bot/connector-web/pom.xml | Added dependency on connector-web-sse module |
| bot/connector-web/src/main/kotlin/WebConnector.kt | Refactored SSE endpoint configuration to use new SseEndpoint class, fixed deprecated applicationId usage |
| bot/connector-web-sse/pom.xml | New module POM with dependencies for SSE functionality |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/SseEndpoint.kt | New endpoint handler that fixes the bug by calling setupSSE before sendMissedEvents |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/Ioc.kt | IoC configuration moved to new package |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/channel/SseChannels.kt | Refactored channels management with lazy listener initialization |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/channel/SseChannel.kt | Renamed from Channel to SseChannel |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/channel/ChannelCallback.kt | Package moved, updated to use WebConnectorResponseContract |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/channel/ChannelDAO.kt | Package moved |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/channel/ChannelEvent.kt | Package moved, updated to use WebConnectorResponseContract |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/channel/ChannelMongoDAO.kt | Package moved |
| bot/connector-web-sse/src/main/resources/META-INF/services/ai.tock.shared.service.BotAdditionalModulesService | Updated service provider class name |
| bot/connector-web-sse/src/test/kotlin/ai/tock/bot/connector/web/sse/channel/SseChannelsTest.kt | Refactored tests with updated package and test data structures |
| shared/src/main/kotlin/security/auth/spi/WebSecurityHandler.kt | Added documentation clarifying TOCK_USER_ID usage |
bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/SseEndpoint.kt
Outdated
Show resolved
Hide resolved
bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/SseEndpoint.kt
Outdated
Show resolved
Hide resolved
|
@copilot please review the changes since your last review |
|
@vsct-jburet I've opened a new pull request, #1996, to work on those changes. Once the pull request is ready, I'll request review from you. |
This PR fixes #1993 by applying a similar fix as #1994, but also includes a refactor of the SSE code into its own module, for better long-term maintainability.