[ECO-5054] Extract common code for message processing in RealtimeChannel to a separate function #1911
Conversation
WalkthroughThe changes in this pull request focus on enhancing the message processing logic within the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
VeskeR
force-pushed
the
1907/extract-common-message-decoding
branch
from
c1681b2 to
ca0d08d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1911/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1911/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1911/features
VeskeR
force-pushed
the
1907/extract-common-message-decoding
branch
from
ca0d08d to
7b9cfa9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1911/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1911/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1911/features
VeskeR
changed the title
VeskeR
force-pushed
the
1907/extract-common-message-decoding
branch
from
7b9cfa9 to
d17f97a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/common/lib/client/realtimechannel.ts (2)
696-727: Consider decoding messages in parallel for improved performanceIn the
_decodeAndPrepareMessagesmethod, messages are decoded sequentially within aforloop usingawait decodeFn(msg). If the decoding of each message is independent, consider decoding all messages in parallel usingPromise.all, which can enhance performance when processing a large array of messages.Here is how you might refactor the code:
- for (let i = 0; i < messages.length; i++) { - const msg = messages[i]; - try { - await decodeFn(msg); - } catch (e) { - Logger.logAction( - this.logger, - Logger.LOG_ERROR, - 'RealtimeChannel.decodeAndPrepareMessages()', - (e as Error).toString(), - ); - if (decodeErrorRecoveryHandler) { - const { unrecoverableError } = decodeErrorRecoveryHandler(e as Error); - if (unrecoverableError) { - // break out of for loop by returning - return { unrecoverableError: true }; - } - } - } - if (!msg.connectionId) msg.connectionId = connectionId; - if (!msg.timestamp) msg.timestamp = timestamp; - if (!msg.id) msg.id = id + ':' + i; - } + const decodePromises = messages.map(async (msg, index) => { + try { + await decodeFn(msg); + } catch (e) { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'RealtimeChannel._decodeAndPrepareMessages()', + `Error decoding message at index ${index}: ${(e as Error).toString()}`, + ); + if (decodeErrorRecoveryHandler) { + const { unrecoverableError } = decodeErrorRecoveryHandler(e as Error); + if (unrecoverableError) { + // Throw to exit early + throw e; + } + } + } + if (!msg.connectionId) msg.connectionId = connectionId; + if (!msg.timestamp) msg.timestamp = timestamp; + if (!msg.id) msg.id = id + ':' + index; + }); + try { + await Promise.all(decodePromises); + return { unrecoverableError: false }; + } catch (e) { + return { unrecoverableError: true }; + }
705-710: Enhance error logging with additional contextWhen logging errors in
_decodeAndPrepareMessages, consider including more context, such as the message index or contents, to facilitate debugging and error tracing.Apply this diff to improve the log statement:
Logger.logAction( this.logger, Logger.LOG_ERROR, 'RealtimeChannel.decodeAndPrepareMessages()', - (e as Error).toString(), + `Error decoding message at index ${i}: ${(e as Error).toString()}`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scripts/moduleReport.ts(1 hunks)src/common/lib/client/realtimechannel.ts(4 hunks)
🔇 Additional comments (2)
scripts/moduleReport.ts (1)
9-9: LGTM! Consider adding a comment explaining the threshold increase.
The increase in the raw bundle size threshold from 98 to 99 KiB aligns with the expected 0.12 KiB increase mentioned in the PR objectives. This temporary increase is justified as it's expected to lead to a decrease when new message processing is added (PR #1910).
Consider adding a comment explaining why this threshold was increased to help future maintainers understand the context.
Add a comment above the constant:
// The maximum size we allow for a minimal useful Realtime bundle (i.e. one that can subscribe to a channel)
+// Note: Threshold increased to 99 KiB to accommodate the temporary size increase from extracting common message processing code.
+// This is expected to decrease when new message processing is added (see PR #1910).
const minimalUsefulRealtimeBundleSizeThresholdsKiB = { raw: 99, gzip: 30 };Let's verify that the new threshold accommodates the actual bundle size:
src/common/lib/client/realtimechannel.ts (1)
684-727: LGTM: Well-structured implementation of _decodeAndPrepareMessages
The new _decodeAndPrepareMessages method is well-implemented with clear logic and proper error handling. This refactoring improves code reuse and readability in the message processing flow.
…parate function Also fixes the issue with processing Presence messages, where a decoding error would prevent us from setting `connectionId`, `timestamp` and `id` fields for a message. Now this behavior is aligned with how it worked with regular messages. Resolves #1907
VeskeR
force-pushed
the
1907/extract-common-message-decoding
branch
from
d17f97a to
282b188
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/common/lib/client/realtimechannel.ts (2)
684-727: Good extraction of common logic, with room for minor improvements.The new method effectively consolidates common message processing logic. Consider these improvements:
- Make the error logging more specific by including the message type and index:
- 'RealtimeChannel.decodeAndPrepareMessages()', + 'RealtimeChannel._decodeAndPrepareMessages()', - (e as Error).toString(), + `Failed to decode ${msg.constructor.name} at index ${i}: ${(e as Error).toString()}`,
- Consider splitting the method into smaller, focused functions:
- Message decoding with error handling
- Message field population (connectionId, timestamp, id)
This would improve maintainability and make the code easier to test.
Line range hint
572-727: Well-structured extraction of common message processing logic.The refactoring successfully:
- Reduces code duplication between presence and regular message processing
- Provides a consistent approach to message decoding and error handling
- Makes the code more maintainable for future message types (like STATE and STATE_SYNC mentioned in PR objectives)
The slight increase in bundle size is a reasonable trade-off for the improved code structure and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/common/lib/client/realtimechannel.ts(4 hunks)
🔇 Additional comments (2)
src/common/lib/client/realtimechannel.ts (2)
572-583: LGTM! Clean refactoring of presence message processing.
The variable rename from 'presence' to 'presenceMessages' improves clarity, and the extraction of common decoding logic reduces code duplication while maintaining the same functionality.
624-652: LGTM! Improved error handling and code organization.
The changes effectively:
- Centralize error handling logic in the new method
- Add clear early return pattern for unrecoverable errors
- Make error handling consistent between presence and regular messages
Resolves #1907
It slightly increases the bundle size for a default bundle on main branch (0.12KiB for All exports modular plugin, 0.04KiB if gzipped), but leads to a decrease as we add new message processing (see #1910 with STATE and STATE_SYNC messages).
This also fixes the issue with processing Presence messages, where an error thrown from a decoding call would prevent us from setting
connectionId,timestampandidfields for a message. Now this behavior is aligned with how it worked with regular messages.Summary by CodeRabbit
New Features
Bug Fixes