Conversation
WhisperPacketHandler split the raw wire message without trimming the leading space the "/" header leaves before the target name, so the receiver lookup always failed (IsNotPlaying) and the leaked name stayed in the delivered text. PubSubHub broadcast with Clients.Others, which excludes the sender's own WorldServer connection, so same-instance delivery had no recipient even when the parsing was fixed. WorldServer never registered ChannelCommunicationRunner as a hosted service, so nothing was listening for interchannel messages at all - this is why neither of the above mattered until all three were fixed together. Verified live with two connected clients plus an unrelated third: whisper delivers once to the target with the correct sender and stripped text, the unrelated client receives nothing, the sender gets no duplicate, and normal map chat is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NosCoreIO/NosCore/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe PR updates pub/sub broadcasts, trims leading whitespace during whisper parsing, and registers ChangesCommunication Updates
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WhisperPacketHandler.cs: the raw wire message from the/header carries a leading space before the target name, but the handler split it without trimming - the receiver lookup always saw an empty name (IsNotPlayingon every whisper) and the strip loop left the target name embedded in the delivered text.PubSubHub.cs:SendMessageAsync/SendMessagesAsyncbroadcast with SignalR'sClients.Others, which excludes the sending WorldServer's own connection - so a whisper between two players on the same WorldServer had zero recipients even with the parsing bug fixed. Changed toClients.All; verified safe against duplicate delivery since the receiving handler (PostedPacketMessageChannelCommunicationMessageHandler) already filters to a locally-connected session and no-ops otherwise.WorldServerBootstrap.cs:ChannelCommunicationRunner, the hosted service that subscribes to incoming interchannel messages and dispatches them to handlers, was never registered anywhere in the WorldServer host - so nothing was listening for these messages at all, independent of the other two issues.All three needed to be fixed together for whisper delivery to work end-to-end.
Test plan
dotnet build NosCore.sln- 0 warnings, 0 errorsIsNotPlayingGenerated with Claude Code (https://claude.com/claude-code)
Summary by CodeRabbit
New Features
Bug Fixes