-
Notifications
You must be signed in to change notification settings - Fork 16
feat: add handler for unknown persona messages #2507
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
base: capts-log-stardate-somewhere-sometime
Are you sure you want to change the base?
feat: add handler for unknown persona messages #2507
Conversation
This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@sentry/node](https://redirect.github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://redirect.github.com/getsentry/sentry-javascript)) | [`^9.3.0` -> `^9.4.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/9.3.0/9.4.0) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | | [@sentry/profiling-node](https://redirect.github.com/getsentry/sentry-javascript/tree/master/packages/profiling-node) ([source](https://redirect.github.com/getsentry/sentry-javascript)) | [`9.3.0` -> `9.4.0`](https://renovatebot.com/diffs/npm/@sentry%2fprofiling-node/9.3.0/9.4.0) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@​sentry/node)</summary> ### [`v9.4.0`](https://redirect.github.com/getsentry/sentry-javascript/releases/tag/9.4.0) [Compare Source](https://redirect.github.com/getsentry/sentry-javascript/compare/9.3.0...9.4.0) - feat(core): Add types for logs protocol and envelope ([#​15530](https://redirect.github.com/getsentry/sentry-javascript/pull/15530)) - feat(deps): Bump `@sentry/cli` from 2.41.1 to 2.42.2 ([#​15510](https://redirect.github.com/getsentry/sentry-javascript/pull/15510)) - feat(deps): Bump `@sentry/webpack-plugin` from 3.1.2 to 3.2.1 ([#​15512](https://redirect.github.com/getsentry/sentry-javascript/pull/15512)) - feat(feedback) Allowing annotation via highlighting & masking ([#​15484](https://redirect.github.com/getsentry/sentry-javascript/pull/15484)) - feat(nextjs): Add `use client` directive to client SDK entrypoints ([#​15575](https://redirect.github.com/getsentry/sentry-javascript/pull/15575)) - feat(nextjs): Allow silencing of instrumentation warning ([#​15555](https://redirect.github.com/getsentry/sentry-javascript/pull/15555)) - feat(sveltekit): Ensure `AsyncLocalStorage` async context strategy is used in Cloudflare Pages ([#​15557](https://redirect.github.com/getsentry/sentry-javascript/pull/15557)) - fix(cloudflare): Make `@cloudflare/workers-types` an optional peer dependency ([#​15554](https://redirect.github.com/getsentry/sentry-javascript/pull/15554)) - fix(core): Don't reverse values in event filters ([#​15584](https://redirect.github.com/getsentry/sentry-javascript/pull/15584)) - fix(core): Handle normalization of null prototypes correctly ([#​15556](https://redirect.github.com/getsentry/sentry-javascript/pull/15556)) - fix(nextjs): Only warn on missing `onRequestError` in version 15 ([#​15553](https://redirect.github.com/getsentry/sentry-javascript/pull/15553)) - fix(node): Allow for `undefined` transport to be passed in ([#​15560](https://redirect.github.com/getsentry/sentry-javascript/pull/15560)) - fix(wasm): Fix wasm integration stacktrace parsing for filename ([#​15572](https://redirect.github.com/getsentry/sentry-javascript/pull/15572)) - perf(node): Store normalized request for processing ([#​15570](https://redirect.github.com/getsentry/sentry-javascript/pull/15570)) #### Bundle size 📦 | Path | Size | | ---------------------------------------------------------------- | ----------------- | | [@​sentry/browser](https://redirect.github.com/sentry/browser) | 23.13 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) - with treeshaking flags | 22.92 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. Tracing) | 36.2 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. Tracing, Replay) | 73.38 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. Tracing, Replay) - with treeshaking flags | 66.79 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. Tracing, Replay with Canvas) | 77.99 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. Tracing, Replay, Feedback) | 89.91 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. Feedback) | 39.66 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. sendFeedback) | 27.76 KB | | [@​sentry/browser](https://redirect.github.com/sentry/browser) (incl. FeedbackAsync) | 32.54 KB | | [@​sentry/react](https://redirect.github.com/sentry/react) | 24.95 KB | | [@​sentry/react](https://redirect.github.com/sentry/react) (incl. Tracing) | 38.08 KB | | [@​sentry/vue](https://redirect.github.com/sentry/vue) | 27.38 KB | | [@​sentry/vue](https://redirect.github.com/sentry/vue) (incl. Tracing) | 37.89 KB | | [@​sentry/svelte](https://redirect.github.com/sentry/svelte) | 23.17 KB | | CDN Bundle | 24.34 KB | | CDN Bundle (incl. Tracing) | 36.24 KB | | CDN Bundle (incl. Tracing, Replay) | 71.25 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) | 76.41 KB | | CDN Bundle - uncompressed | 71.12 KB | | CDN Bundle (incl. Tracing) - uncompressed | 107.51 KB | | CDN Bundle (incl. Tracing, Replay) - uncompressed | 218.78 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 231.31 KB | | [@​sentry/nextjs](https://redirect.github.com/sentry/nextjs) (client) | 39.25 KB | | [@​sentry/sveltekit](https://redirect.github.com/sentry/sveltekit) (client) | 36.61 KB | | [@​sentry/node](https://redirect.github.com/sentry/node) | 142.14 KB | | [@​sentry/node](https://redirect.github.com/sentry/node) - without tracing | 95.57 KB | | [@​sentry/aws-serverless](https://redirect.github.com/sentry/aws-serverless) | 119.92 KB | </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/rustymotors/server). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xODUuNCIsInVwZGF0ZWRJblZlciI6IjM5LjE4NS40IiwidGFyZ2V0QnJhbmNoIjoiZGV2IiwibGFiZWxzIjpbXX0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThe changes update dependency versions in multiple package configuration files by upgrading Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Handler
participant Logger
Server->>Handler: Receive unknown persona message (opCode 18245)
Handler->>Logger: Log operation code and message details
Logger-->>Handler: Logging complete
Handler-->>Server: Return { connectionId, messages: [] }
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
Here's the code health analysis summary for commits Analysis Summary
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
❌ Your project check has failed because the head coverage (46.37%) is below the adjusted base coverage (46.39%). You can increase the head coverage or adjust the Removed Code Behavior. Additional details and impacted files@@ Coverage Diff @@
## capts-log-stardate-somewhere-sometime #2507 +/- ##
=========================================================================
- Coverage 46.39% 46.37% -0.02%
=========================================================================
Files 205 206 +1
Lines 11452 11472 +20
Branches 722 722
=========================================================================
+ Hits 5313 5320 +7
- Misses 6138 6151 +13
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/persona/src/handlers/handleUnknownPersonaMessage.ts (1)
44-45: Consider adding metrics collection for unknown messages.While logging unknown messages is helpful for debugging, consider adding metrics collection to track the frequency of unknown messages. This would help identify commonly occurring unknown messages that might need dedicated handlers.
- log.debug(`Received unknown persona message: opCode=${message._header.id}`); - log.debug(`Message data: ${message._doSerialize().toString("hex")}`); + log.debug(`Received unknown persona message: opCode=${message._header.id}`); + log.debug(`Message data: ${message._doSerialize().toString("hex")}`); + + // If using Sentry, you could track this as a breadcrumb or custom event + try { + const Sentry = await import("@sentry/node"); + Sentry.addBreadcrumb({ + category: 'persona', + message: `Unknown persona message received with opCode=${message._header.id}`, + level: 'info' + }); + } catch (e) { + // Silently continue if Sentry import fails + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-45: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L43-L45
Added lines #L43 - L45 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/nps/package.json(1 hunks)packages/persona/package.json(1 hunks)packages/persona/src/handlers/handleUnknownPersonaMessage.ts(1 hunks)packages/persona/src/internal.ts(2 hunks)packages/shard/package.json(1 hunks)packages/shared-packets/package.json(1 hunks)packages/transactions/package.json(1 hunks)src/chat/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/shard/package.json
- packages/cli/package.json
- packages/shared-packets/package.json
- packages/transactions/package.json
- src/chat/package.json
- packages/persona/package.json
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/persona/src/handlers/handleUnknownPersonaMessage.ts
[warning] 32-36: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L32-L36
Added lines #L32 - L36 were not covered by tests
[warning] 43-45: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L43-L45
Added lines #L43 - L45 were not covered by tests
[warning] 48-52: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L48-L52
Added lines #L48 - L52 were not covered by tests
🔇 Additional comments (5)
packages/nps/package.json (1)
21-21: Dependency version update to @sentry/profiling-node 9.4.0 looks good.The package has been updated from the previous version (9.3.0) to 9.4.0, which is a minor version increment that likely includes bug fixes and performance improvements.
package.json (1)
41-42: Sentry dependencies update looks good.Both Sentry packages have been updated to version 9.4.0, which keeps dependencies in sync across the project. This is a minor version update that likely includes bug fixes and improvements to error tracking and performance monitoring.
packages/persona/src/handlers/handleUnknownPersonaMessage.ts (1)
32-52:❓ Verification inconclusive
Good implementation of fallback handler for unknown messages.
This handler provides a graceful way to deal with unknown message codes by logging them for debugging purposes and allowing the server to continue processing. This prevents 'UNSUPPORTED_MESSAGECODE' errors as intended.
However, this implementation has no test coverage as indicated by the static analysis tool.
Could we add unit tests to verify this handler works as expected? A simple test would verify:
- The handler logs the message data
- It returns an empty messages array with the connection ID
🏁 Script executed:
#!/bin/bash # Check if there are any test files for this handler fd "handleUnknownPersonaMessage.test" --type f --exec cat {}Length of output: 62
Action: Add Unit Tests for Fallback Handler
This implementation gracefully handles unknown messages by logging details and returning an empty messages array. However, our verification shows there's currently no test coverage for this handler.
Could you add unit tests to cover the following?
- Verify that the handler logs the received message data.
- Confirm that the returned object contains the correct connection ID and an empty
messagesarray.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-36: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L32-L36
Added lines #L32 - L36 were not covered by tests
[warning] 43-45: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L43-L45
Added lines #L43 - L45 were not covered by tests
[warning] 48-52: packages/persona/src/handlers/handleUnknownPersonaMessage.ts#L48-L52
Added lines #L48 - L52 were not covered by testspackages/persona/src/internal.ts (2)
14-14: Import for new handler looks good.The import statement is correctly added to bring in the new handler function.
74-78:✅ Verification successful
Good integration of unknown message handler.
The new handler is correctly added to the messageHandlers array with the appropriate operation code (18245). The hexadecimal form (0x4745) is included in comments which helps with cross-referencing to client or protocol documentation.
Let's verify if there are any other occurrences of this op code in the codebase that might provide more context:
🏁 Script executed:
#!/bin/bash # Search for the opcode in decimal or hex form echo "Searching for opcode 18245 (0x4745) in the codebase..." rg -i "18245|0x4745" --type ts --type jsLength of output: 221
Verified opcode usage in the repository.
The new handler for the unknown persona message is correctly integrated inpackages/persona/src/internal.ts(lines 74-78) with the op code 18245 (noted as 0x4745 in the comment). A codebase search confirmed that this op code appears only in this location, indicating that the implementation is both isolated and correct.



👋 Hi there! This PR was automatically generated by Autofix 🤖
This fix was triggered by Joe Becher
Fixes SERVER-CD
This change introduces a handler for unknown persona messages to prevent 'UNSUPPORTED_MESSAGECODE' errors. The handler logs the message and returns an empty response, allowing the server to continue processing.
If you have any questions or feedback for the Sentry team about this fix, please email [email protected] with the Run ID: 8233.
Summary by CodeRabbit
Chores
New Features