Restructure and clean up message and presencemessage types #1941
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces a comprehensive refactoring of the Ably library's message handling system, focusing on restructuring message types, encoding/decoding processes, and standardizing message creation. The changes primarily involve updating type definitions, modifying import statements, and introducing new classes like Changes
Sequence DiagramsequenceDiagram
participant Client
participant Message
participant WireMessage
participant BaseMessage
Client->>Message: fromValues(data)
Message->>BaseMessage: Inherit base properties
Message-->>Client: Create Message instance
Client->>Message: encode(options)
Message->>WireMessage: Create WireMessage
WireMessage-->>Client: Return encoded message
Client->>WireMessage: decode(context)
WireMessage->>BaseMessage: Process decoding
WireMessage-->>Client: Return decoded Message
Poem
Finishing Touches
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1941/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1941/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (21)
src/common/lib/types/presencemessage.ts (1)
119-145: Optimize error handling in thedecodemethodIn the
WirePresenceMessageclass, thedecodemethod logs errors but does not rethrow them or provide a mechanism for the calling code to handle them. To enhance error traceability and handling, consider rethrowing the error after logging or returning an error object alongside the decoded message.🧰 Tools
🪛 GitHub Actions: Bundle size report
[error] File contributes 1165B to bundle, exceeding allowed maximum of 100B
src/common/lib/types/protocolmessage.ts (2)
37-54: Handle potential undefined properties infromDeserializedIn the
deserializefunction, properties likemessagesandpresenceare conditionally assigned. Ensure that theProtocolMessageconstructor handles undefined values gracefully to prevent potential null reference errors when these properties are accessed.
114-114: Check for nullability ofpresencewhen accessingThe
presenceproperty is optional in theProtocolMessageclass. When accessingpresencein other parts of the code, make sure to perform null or undefined checks to avoid runtime exceptions.src/common/lib/types/message.ts (1)
180-197: Improve error propagation indecodeWithErrmethodIn the
decodeWithErrmethod ofWireMessage, errors are logged but only returned alongside the decoded message without throwing. Consider whether the calling code expects exceptions to be thrown. If so, rethrow the error after logging to adhere to expected error handling patterns.src/common/lib/types/basemessage.ts (6)
18-22: Remove redundant nested 'cipher' property in 'CipherOptions'The
cipherproperty withinCipherOptionsseems redundant and might cause confusion. Consider removing it if it's not necessary.Apply this diff to simplify
CipherOptions:export type CipherOptions = { channelCipher: { encrypt: (data: Uint8Array | string) => Promise<Uint8Array>; algorithm: 'aes'; }; - cipher?: { - channelCipher: { - encrypt: (data: Uint8Array | string) => Promise<Uint8Array>; - algorithm: 'aes'; - }; - }; };🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
52-52: Simplify null checks using optional chainingThe condition
if (options && options.cipher)can be simplified using optional chaining for better readability.Apply this diff:
- if (options && options.cipher) { + if (options?.cipher) {🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
93-93: Simplify null checks using optional chainingSimilarly, the condition
if (options != null && options.cipher)can be simplified.Apply this diff:
- if (options != null && options.cipher) { + if (options?.cipher) {🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
136-139: Use optional chaining for nested property accessThe condition can be simplified using optional chaining to improve readability.
Apply this diff:
- if ( - context.channelOptions != null && - context.channelOptions.cipher && - context.channelOptions.channelCipher - ) { + if (context.channelOptions?.cipher && context.channelOptions.channelCipher) {🧰 Tools
🪛 Biome (1.9.4)
[error] 136-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
116-116: Avoid assignments within expressionsAssigning within an expression can lead to confusing code. It's better to separate the assignment from the condition.
Refactor the code to avoid assignment in the while condition:
- while ((lastProcessedEncodingIndex = encodingsToProcess) > 0) { + lastProcessedEncodingIndex = encodingsToProcess; + while (lastProcessedEncodingIndex > 0) {🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
188-189: Ensure proper error propagationReassigning the error inside the
catchblock may lead to loss of the original stack trace. Consider preserving the original error or rethrowing it appropriately.Ensure that the original error is preserved when rethrowing:
} catch (e) { - const err = e as ErrorInfo; - throw new ErrorInfo( + throw new ErrorInfo( 'Error processing the ' + xform + ' encoding, decoder returned ‘' + e.message + '’', e.code || 40013, 400, ); }🧰 Tools
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
src/common/lib/client/realtimepresence.ts (2)
Line range hint
288-303: Add a 'default' case to the switch statementThe switch statement on
presence.actionlacks adefaultcase. Adding one can help catch unexpected actions and improve code robustness.Add a
defaultcase to handle any unforeseenpresence.actionvalues:default: Logger.logAction( this.logger, Logger.LOG_ERROR, 'RealtimePresence.setPresence()', 'Unexpected presence action: ' + presence.action, ); break;
319-319: Ensure 'presence.action' is defined before emittingUsing non-null assertion
presence.action!assumes thatpresence.actionis always defined. Add a null check to prevent possible runtime errors.Modify the code to check for
presence.action:- this.subscriptions.emit(presence.action!, presence); + if (presence.action) { + this.subscriptions.emit(presence.action, presence); + } else { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'RealtimePresence.setPresence()', + 'Presence action is undefined.', + ); + }src/common/lib/client/realtimechannel.ts (1)
618-639: Optimize message decoding with concurrent processingUsing
awaitinside aforloop leads to sequential execution. Consider usingPromise.allto decode messages concurrently, improving performance.Apply this diff to decode messages concurrently:
- let messages: Message[] = []; - for (let i = 0; i < encoded.length; i++) { - const { decoded, err } = await encoded[i].decodeWithErr(this._decodingContext, this.logger); - messages[i] = decoded; - - if (err) { - // Error handling... - } - } + const decodedResults = await Promise.all( + encoded.map((msg) => msg.decodeWithErr(this._decodingContext, this.logger)) + ); + const messages = decodedResults.map(({ decoded }) => decoded); + // Handle errors from decodedResults if necessaryEnsure that error handling is adjusted appropriately.
src/common/lib/client/modularplugins.ts (1)
8-8: LGTM! Consider adding JSDoc comments.The refactoring from utility functions to type imports aligns well with the PR objectives, making the interface more type-safe and maintainable.
Consider adding JSDoc comments to document the purpose of these types in the interface:
export interface PresenceMessagePlugin { /** Type for user-friendly presence messages */ PresenceMessage: typeof PresenceMessage; /** Type for wire protocol presence messages */ WirePresenceMessage: typeof WirePresenceMessage; }Also applies to: 13-14
src/common/lib/types/protocolmessagecommon.ts (2)
26-29: Make ActionName array construction type-safe.The current implementation could be made more type-safe using TypeScript features.
-export const ActionName: string[] = []; -Object.keys(actions).forEach(function (name) { - ActionName[(actions as { [key: string]: number })[name]] = name; -}); +export const ActionName = Object.entries(actions).reduce( + (acc, [name, code]) => { + acc[code] = name; + return acc; + }, + [] as string[] +);🧰 Tools
🪛 GitHub Actions: Bundle size report
[error] File contributes 550B to bundle, exceeding allowed maximum of 100B
31-43: Document bitwise operations in flags.The bitwise operations in flags would benefit from documentation explaining the bit positions and their significance.
export const flags: { [key: string]: number } = { /* Channel attach state flags */ + // Bits 0-15 reserved for attach state flags HAS_PRESENCE: 1 << 0, // 0x0001 HAS_BACKLOG: 1 << 1, // 0x0002 RESUMED: 1 << 2, // 0x0004 TRANSIENT: 1 << 4, // 0x0010 ATTACH_RESUME: 1 << 5, // 0x0020 /* Channel mode flags */ + // Bits 16-31 reserved for channel mode flags PRESENCE: 1 << 16, // 0x00010000 PUBLISH: 1 << 17, // 0x00020000 SUBSCRIBE: 1 << 18, // 0x00040000 PRESENCE_SUBSCRIBE: 1 << 19, // 0x00080000 };🧰 Tools
🪛 GitHub Actions: Bundle size report
[error] File contributes 550B to bundle, exceeding allowed maximum of 100B
src/common/lib/client/restpresencemixin.ts (1)
6-6: Consider renaming underscore-prefixed utility.The
_fromEncodedArrayutility appears to be internal but is exported. Consider either:
- Removing the underscore prefix if it's meant to be public
- Moving it to an internal utilities file if it's truly private
Also, the type assertion could be made safer:
- ) as Utils.Properties<WirePresenceMessage>[]; + ) satisfies Utils.Properties<WirePresenceMessage>[];Also applies to: 31-33
src/common/lib/client/restpresence.ts (1)
34-36: Extract shared type definitions.This type assertion pattern is duplicated from
restpresencemixin.ts. Consider extracting it to a shared type:// In types/presencemessage.ts export type DecodedWirePresence = Utils.Properties<WirePresenceMessage>[]; // Then in both files: ) satisfies DecodedWirePresence;src/common/lib/client/restchannel.ts (1)
Line range hint
83-88: Update error message to match the code.The error message mentions "single-argument form" but the code handles multiple argument forms (string + data, object, array).
- throw new ErrorInfo( - 'The single-argument form of publish() expects a message object or an array of message objects', - 40013, - 400, - ); + throw new ErrorInfo( + 'publish() expects a message object, an array of message objects, or a name and data', + 40013, + 400, + );test/realtime/sync.test.js (1)
731-744: Consider adding a comment explaining the action values.While the code correctly uses numeric action values (2 for 'enter'), it might be helpful to add a comment mapping these numeric values to their corresponding actions for better readability.
await syncerChannel.processMessage( createPM({ action: 14, id: 'messageid:0', connectionId: 'connid', timestamp: 2000000000000, presence: [ { + // action: 2 represents ENTER clientId: interrupterClientId, action: 2, }, ], }), );test/rest/presence.test.js (1)
92-94: Consider adding test coverage for new message handling classes.While the changes correctly implement the new message encoding approach, consider adding test cases to cover:
- The new
BaseMessagefunctionality- Edge cases in
WireMessageandWirePresenceMessagedecode/encode paths- Error handling scenarios in the new message transformation flow
Would you like me to help generate additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
ably.d.ts(2 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/modularplugins.ts(1 hunks)src/common/lib/client/presencemap.ts(3 hunks)src/common/lib/client/realtimechannel.ts(8 hunks)src/common/lib/client/realtimepresence.ts(8 hunks)src/common/lib/client/restchannel.ts(4 hunks)src/common/lib/client/restchannelmixin.ts(2 hunks)src/common/lib/client/restpresence.ts(2 hunks)src/common/lib/client/restpresencemixin.ts(2 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/connectionmanager.ts(1 hunks)src/common/lib/transport/protocol.ts(1 hunks)src/common/lib/transport/transport.ts(1 hunks)src/common/lib/types/basemessage.ts(1 hunks)src/common/lib/types/defaultmessage.ts(2 hunks)src/common/lib/types/defaultpresencemessage.ts(2 hunks)src/common/lib/types/message.ts(6 hunks)src/common/lib/types/presencemessage.ts(4 hunks)src/common/lib/types/protocolmessage.ts(4 hunks)src/common/lib/types/protocolmessagecommon.ts(1 hunks)src/platform/web/modular/presencemessage.ts(2 hunks)src/platform/web/modular/realtimepresence.ts(1 hunks)test/realtime/crypto.test.js(4 hunks)test/realtime/message.test.js(1 hunks)test/realtime/presence.test.js(5 hunks)test/realtime/sync.test.js(7 hunks)test/rest/presence.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/common/lib/transport/connectionmanager.ts
🧰 Additional context used
🪛 GitHub Actions: Bundle size report
src/common/lib/types/protocolmessagecommon.ts
[error] File contributes 550B to bundle, exceeding allowed maximum of 100B
src/common/lib/types/presencemessage.ts
[error] File contributes 1165B to bundle, exceeding allowed maximum of 100B
src/common/lib/types/basemessage.ts
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
🪛 Biome (1.9.4)
src/common/lib/types/basemessage.ts
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 116-116: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 136-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 15-15: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (35)
src/common/lib/types/protocolmessage.ts (1)
6-8: Ensureactionsand related types are correctly updatedThe
actionsandActionNameimports have been adjusted. Verify that all references to these have been updated accordingly throughout the codebase to prevent any mismatches or runtime errors.src/common/lib/types/message.ts (1)
30-31: Ensure correct defaulting instringifyActionfunctionThe
stringifyActionfunction usesaction || 0to default undefined actions to0. Ifactionis0, which is a valid index, this operation would still return0. Verify that this logic correctly handles all cases, and consider using a more explicit defaulting mechanism, such as checking forundefined.src/common/lib/types/basemessage.ts (1)
Line range hint
595-601: Verify compatibility of 'padStart' methodThe use of
padStartmay not be supported in all environments. Ensure that the target environment supports this method or include a polyfill if necessary.Confirm that the runtime environment supports
padStart, or provide an alternative method for padding.🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 116-116: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 136-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 15-15: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 GitHub Actions: Bundle size report
[error] File contributes 3389B to bundle, exceeding allowed maximum of 100B
src/common/lib/client/realtimepresence.ts (1)
Line range hint
434-438: Use consistent timestamp source for synthesized messagesWhen synthesizing leave messages, using
Date.now()may lead to discrepancies with server-provided timestamps.Confirm whether using
Date.now()is appropriate, or consider using a consistent timestamp source aligned with server time.src/common/lib/client/realtimechannel.ts (2)
363-363: Ensure 'modes' are correctly typedCasting
Utils.allToUpperCase(this.channelOptions.modes)toAPI.ChannelMode[]may mask type issues. Verify thatthis.channelOptions.modescontains validAPI.ChannelModevalues.Ensure that the modes array is properly validated and typed before casting.
479-483: Confirm compatibility of 'WirePresenceMessage' with 'protocolMessageFromValues'In the
sendPresencemethod, ensure thatprotocolMessageFromValuescorrectly handlesWirePresenceMessage[]for thepresencefield.Verify that
protocolMessageFromValuesacceptsWirePresenceMessage[]and processes it as expected.src/platform/web/modular/realtimepresence.ts (1)
3-3: LGTM! Clean separation of message typesThe changes properly separate the wire protocol message type (
WirePresenceMessage) from the public message type (PresenceMessage), aligning with the PR's objective of improving message type handling.Let's verify that this separation is consistently applied across the codebase:
Also applies to: 7-8
✅ Verification successful
Verified: Wire protocol message type is properly encapsulated
The
WirePresenceMessagetype is consistently used only in internal implementation files for wire protocol handling and is not exposed in public interfaces, maintaining a clean separation between wire protocol and public message types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any direct usage of WirePresenceMessage in public interfaces rg -g '!*.test.*' -g '!**/types/*' "WirePresenceMessage" src/Length of output: 1618
src/common/lib/types/defaultmessage.ts (2)
19-21: LGTM! Clean delegation to Message.fromValuesThe simplified implementation properly delegates to the base class, reducing code duplication.
12-12: Consider adding runtime type validation for WireMessage castingThe type assertions
as WireMessagecould be unsafe if the input doesn't match the expected structure. Consider adding runtime validation before the cast.Let's check if there's any existing validation:
Also applies to: 16-16
✅ Verification successful
Type assertion is properly validated through decode pipeline
The
as WireMessagetype assertion is safe as the encoded message goes through proper validation:
- WireMessage instantiation via
fromValues- Comprehensive decode pipeline with error handling
- Runtime validation during decode process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for WireMessage type validation ast-grep --pattern 'function $name($params) { $$$ as WireMessage $$$ }'Length of output: 79
Script:
#!/bin/bash # Find WireMessage type definition and usage ast-grep --pattern 'type WireMessage = $_' # Find fromEncoded implementation ast-grep --pattern 'function fromEncoded($_) { $$$ }' # Search for validation-related code rg -A 5 "validateMessage|isValidMessage|checkMessage" --type tsLength of output: 168
Script:
#!/bin/bash # Check the content of the file cat src/common/lib/types/defaultmessage.ts # Look for message-related validation patterns rg -A 5 "message.*valid|valid.*message|check.*message" --type ts # Find type definitions related to messages ast-grep --pattern 'interface $_Message { $$$ }'Length of output: 1820
Script:
#!/bin/bash # Check the base Message implementation cat src/common/lib/types/message.ts # Look for validation in the message module rg -A 5 "fromEncoded" --type ts src/common/lib/types/message.tsLength of output: 7330
src/platform/web/modular/presencemessage.ts (1)
2-2: LGTM! Improved message construction patternThe change to use
PresenceMessage.fromValuesdirectly provides a cleaner and more consistent way to construct presence messages.Let's verify this pattern is used consistently:
Also applies to: 24-24
✅ Verification successful
Pattern verification successful:
fromValuesis used consistentlyThe static factory method pattern using
fromValuesis consistently implemented across all message and entity types in the codebase, including PresenceMessage, Message, ErrorInfo, and others. The changes align with the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining direct fromValues usage rg "fromValues\(" --type ts src/Length of output: 5692
src/common/lib/types/defaultpresencemessage.ts (2)
22-24: LGTM! Consistent with DefaultMessage patternThe implementation follows the same clean delegation pattern as DefaultMessage, maintaining consistency across the codebase.
12-12: Consider adding runtime type validation for WirePresenceMessage castingSimilar to DefaultMessage, the type assertions to WirePresenceMessage should include runtime validation to ensure type safety.
Let's check for existing validation patterns:
Also applies to: 19-19
src/common/lib/types/protocolmessagecommon.ts (1)
1-49:⚠️ Potential issueAddress bundle size concerns.
The file contributes 550B to the bundle, exceeding the 100B limit. Consider splitting or optimizing the constants.
Let's analyze the impact of different approaches:
🧰 Tools
🪛 GitHub Actions: Bundle size report
[error] File contributes 550B to bundle, exceeding allowed maximum of 100B
src/common/lib/client/restchannelmixin.ts (2)
5-5: LGTM! Import statement updated to use new message types.The change aligns with the PR's objective of restructuring message types by importing
WireMessageinstead ofWireProtocolMessage.
38-40: LGTM! Type casting updated for better type safety.The type casting to
Utils.Properties<WireMessage>[]aligns with the new message type structure.src/common/lib/client/defaultrealtime.ts (1)
15-15: LGTM! Updated to use new presence message types.The changes align with the PR's objective of improving message handling:
- Imports now use
PresenceMessageandWirePresenceMessagetypes directly.- Constructor configuration updated to use these types in
RealtimePresence.Also applies to: 38-39
src/common/lib/transport/protocol.ts (1)
1-2: LGTM! Improved modularity by separating protocol message actions.The change moves actions to a dedicated
protocolmessagecommonmodule while maintaining the core protocol message functionality.src/common/lib/client/restchannel.ts (3)
7-7: LGTM! Import statements updated for better organization.The changes improve code organization by:
- Using
encodeArrayfor message encoding.- Separating
CipherOptionsimport.Also applies to: 9-9
75-75: LGTM! Updated to use class methods for message creation.The changes improve consistency by using
Message.fromValuesandMessage.fromValuesArrayclass methods instead of imported functions.Also applies to: 78-78, 81-81
111-111: LGTM! Improved wire message handling.The changes make wire message handling more explicit:
- Encoding messages to wire format.
- Size calculation using wire messages.
- Serializing wire messages for publishing.
Also applies to: 114-114, 128-128
src/common/lib/client/presencemap.ts (3)
4-4: LGTM! Import statement updated for consistency.The import statement has been updated to directly import
PresenceMessage, aligning with the PR's goal of restructuring message types.
83-85: LGTM! Consistent message creation pattern.The code now uses
PresenceMessage.fromValues()directly, which aligns with the PR's objective of standardizing message creation.
121-123: LGTM! Consistent message creation pattern.The code maintains consistency in using
PresenceMessage.fromValues()for message creation.src/common/lib/transport/transport.ts (1)
1-1: LGTM! Import source updated for better modularity.The
actionsimport has been moved toprotocolmessagecommon, which better aligns with the separation of common protocol message functionality.src/common/lib/transport/comettransport.ts (1)
2-2: LGTM! Import source updated consistently.The
actionsimport has been updated to useprotocolmessagecommon, maintaining consistency with other transport-related files.test/realtime/sync.test.js (1)
73-94: LGTM! Standardized presence message creation.The code now uses
createPMconsistently with numeric action values, which provides better type safety and standardization.Also applies to: 119-140
test/realtime/crypto.test.js (1)
267-268: LGTM! Consistent refactoring of message encoding.The changes correctly refactor the message encoding from a static method to an instance method, which is a good object-oriented practice.
Let's verify this refactoring is consistent across the codebase:
Also applies to: 290-291, 388-390, 432-434
✅ Verification successful
Refactoring verification successful: All message encoding uses instance method pattern
The codebase shows consistent usage of the instance method pattern for message encoding, with no remaining static
Message.encodecalls. All test files correctly use the new approach.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining static Message.encode calls that might have been missed # Expect: No results, as all should be refactored to instance method rg "Message\.encode\(" --type jsLength of output: 534
Script:
#!/bin/bash # Find Message class definition and encode method ast-grep --pattern 'class Message { $$$ }' # Look for other potential static encode patterns rg "encode\(" --type js -B1Length of output: 3093
test/realtime/message.test.js (2)
1278-1311: LGTM! Standardized message action values and string representations.The changes correctly implement the standardized message action values and their string representations:
0maps tomessage.create1maps tomessage.update- Undefined action defaults to
message.create
1319-1329: LGTM! Consistent message creation with version/timestamp handling.The changes ensure that message creation correctly handles version and timestamp fields, with proper action values (0 for create, 1 for update).
test/realtime/presence.test.js (2)
92-92: Performance improvement: Reduced test timeout.Reducing the timeout from 60s to 20s is a good optimization as presence operations should complete well within this timeframe.
1636-1644: LGTM! Standardized presence message creation.The changes consistently use
PresenceMessage.fromValuesfor creating presence messages, with proper action values.Let's verify this standardization is complete:
Also applies to: 1792-1799
✅ Verification successful
Standardization of PresenceMessage creation verified
The codebase consistently uses
PresenceMessage.fromValuesfor creating presence messages. The only instances of direct constructor usage are within the internal implementation of thePresenceMessageclass itself, which is the correct pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any non-standardized presence message creation patterns # Expect: No direct object creation, all should use fromValues rg "new PresenceMessage\(" --type jsLength of output: 38
Script:
#!/bin/bash # Search for any PresenceMessage instantiation patterns ast-grep --pattern 'new PresenceMessage' # Search for fromValues usage to confirm standardization ast-grep --pattern 'PresenceMessage.fromValues' # Backup regex search for any PresenceMessage related code rg "PresenceMessage" --type js -A 2Length of output: 9464
ably.d.ts (2)
2406-2410: Enhancement: Added support for message summaries.The addition of
MESSAGE_SUMMARYtype and its integration intoMessageActionadds support for annotation summaries.Also applies to: 2420-2421
878-878:⚠️ Potential issueBreaking change: Removed ATTACH_RESUME from ChannelMode.
The removal of
ATTACH_RESUMEfromChannelModetype is a breaking change that may affect existing code.Let's check for usage of this mode:
test/rest/presence.test.js (2)
94-94: LGTM! Consistent with the new message handling approach.The change maintains consistency with the new architecture, using
encode()for wire protocol message conversion. The numeric action code 3 for 'leave' is correct.
92-92: LGTM! Verify action codes in the protocol specification.The change from direct JSON serialization to using
encode()aligns with the new message handling architecture. The numeric action code 1 for 'present' matches the protocol specification.Let's verify the action codes in the protocol specification:
✅ Verification successful
Action code 1 for 'present' state is correct according to the protocol
The presence action code 1 matches the protocol specification where 'present' is defined as the second element (index 1) in the actions array:
['absent', 'present', 'enter', 'leave', 'update'].🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for action code definitions in the codebase rg -A 10 "enum.*Action|const.*Action" --type tsLength of output: 1482
Script:
#!/bin/bash # Search for presence-related action definitions rg -A 10 "enum.*Presence|const.*presence.*action|presence.*Action" --type ts # Also search for presence-related files fd presence --type f --exec rg -l "action.*=.*1|PRESENT.*=.*1"Length of output: 4745
Script:
#!/bin/bash # Check the presence test file for action definitions cat test/realtime/presence.test.js # Search for protocol-related files with presence actions rg -A 5 "present.*=.*1|PRESENT.*=.*1|actions.*=.*{" --type ts src/Length of output: 65787
Script:
#!/bin/bash # Search for protocol message types and action definitions rg -A 10 "enum.*Protocol|type.*Protocol|const.*protocol" --type ts src/ # Also try to find presence-specific protocol definitions fd presence --type f --exec rg "enum|const.*=.*{|type.*=.*{" {}Length of output: 7976
Script:
#!/bin/bash # Search for action definitions in protocol and presence files rg "enum.*Action|const.*actions.*=|type.*Action.*=" src/common/lib/types/ # Also check presence message types fd "presencemessage|protocolmessage" --type f -d 3 --exec cat {}Length of output: 393
SimonWoolf
force-pushed
the
message-types-refactor
branch
from
e84f548 to
9635f30
Compare
|
@coderabbitai pause |
github-actions
bot
temporarily deployed
to
staging/pull/1941/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1941/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/common/lib/types/presencemessage.ts (1)
129-140: Consider enhancing error recovery in decode method.The current error handling logs the error but continues with potentially incomplete or invalid data. Consider whether it would be more appropriate to:
- Return a specific error state in the result
- Propagate the error to the caller
- Set default/fallback values for failed decoding
src/common/lib/types/basemessage.ts (6)
52-53: Use optional chaining for cleaner null checksThe code can be simplified using optional chaining operator.
Apply this diff to improve readability:
- if (options && options.cipher) { + if (options?.cipher) { if (!Crypto) Utils.throwMissingPluginError('Crypto'); - if (options != null && options.cipher) { + if (options?.cipher) { return encrypt(msg, options);Also applies to: 93-94
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
100-196: Consider extracting encoding handlers into separate functionsThe decode function contains a large switch statement handling different encoding types. Consider extracting each case into a separate handler function for better maintainability.
Example refactor:
const encodingHandlers = { base64: (data: any) => Platform.BufferUtils.base64Decode(String(data)), 'utf-8': (data: any) => Platform.BufferUtils.utf8Decode(data), json: (data: any) => JSON.parse(data), // ... other handlers }; function handleEncoding(xform: string, data: any, context: EncodingDecodingContext): any { const handler = encodingHandlers[xform]; if (!handler) throw new Error('Unknown encoding'); return handler(data); }🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 136-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
198-217: Consider using a discriminated union for message typesThe
wireToJSONfunction could benefit from more specific typing to handle different message formats.Example type definition:
type MessageType = { encoding?: string; data: string | Buffer | null | undefined; } & Record<string, any>; export function wireToJSON(this: MessageType, ...args: any[]): MessageType { // ... existing implementation }
219-241: Enhance error handling in populateFieldsFromParentConsider providing more context in the error message and handling edge cases.
Apply this diff to improve error handling:
switch (parent.action) { case actions.MESSAGE: + if (!parent.messages?.length) { + throw new ErrorInfo('No messages found in parent', 40000, 400); + } msgs = parent.messages!; break; case actions.PRESENCE: case actions.SYNC: + if (!parent.presence?.length) { + throw new ErrorInfo('No presence messages found in parent', 40000, 400); + } msgs = parent.presence!; break; default: - throw new ErrorInfo('Unexpected action ' + parent.action, 40000, 400); + throw new ErrorInfo(`Unexpected action ${parent.action} in parent message`, 40000, 400); }
243-262: Optimize string concatenation in strMsgConsider using array join or template literals for better performance in string concatenation.
Apply this diff to improve performance:
let result = '[' + cls; + const parts = []; for (const attr in m) { if (attr === 'data') { if (typeof m.data == 'string') { - result += '; data=' + m.data; + parts.push(`data=${m.data}`); } else if (Platform.BufferUtils.isBuffer(m.data)) { - result += '; data (buffer)=' + Platform.BufferUtils.base64Encode(m.data); + parts.push(`data (buffer)=${Platform.BufferUtils.base64Encode(m.data)}`); } else { - result += '; data (json)=' + JSON.stringify(m.data); + parts.push(`data (json)=${JSON.stringify(m.data)}`); } } else if (attr && (attr === 'extras' || attr === 'operation')) { - result += '; ' + attr + '=' + JSON.stringify(m[attr]); + parts.push(`${attr}=${JSON.stringify(m[attr])}`); } else if (m[attr] !== undefined) { - result += '; ' + attr + '=' + m[attr]; + parts.push(`${attr}=${m[attr]}`); } } - result += ']'; - return result; + return `${result}${parts.length ? '; ' : ''}${parts.join('; ')}]`;
264-273: Consider using more specific types for BaseMessage propertiesThe
BaseMessageclass could benefit from more specific types and documentation.Apply this diff to improve type definitions:
export abstract class BaseMessage { + /** Unique identifier for the message */ id?: string; + /** Unix timestamp in milliseconds */ timestamp?: number; + /** Client identifier */ clientId?: string; + /** Connection identifier */ connectionId?: string; + /** Message payload */ data?: any; + /** Message encoding format */ encoding?: string | null; + /** Additional metadata */ extras?: Record<string, unknown>; + /** Message size in bytes */ size?: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
ably.d.ts(2 hunks)scripts/moduleReport.ts(1 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/modularplugins.ts(1 hunks)src/common/lib/client/presencemap.ts(3 hunks)src/common/lib/client/realtimechannel.ts(9 hunks)src/common/lib/client/realtimepresence.ts(8 hunks)src/common/lib/client/restchannel.ts(4 hunks)src/common/lib/client/restchannelmixin.ts(2 hunks)src/common/lib/client/restpresence.ts(2 hunks)src/common/lib/client/restpresencemixin.ts(2 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/connectionmanager.ts(1 hunks)src/common/lib/transport/protocol.ts(1 hunks)src/common/lib/transport/transport.ts(1 hunks)src/common/lib/types/basemessage.ts(1 hunks)src/common/lib/types/defaultmessage.ts(2 hunks)src/common/lib/types/defaultpresencemessage.ts(2 hunks)src/common/lib/types/message.ts(5 hunks)src/common/lib/types/presencemessage.ts(4 hunks)src/common/lib/types/protocolmessage.ts(4 hunks)src/common/lib/types/protocolmessagecommon.ts(1 hunks)src/platform/web/modular/presencemessage.ts(2 hunks)src/platform/web/modular/realtimepresence.ts(1 hunks)test/realtime/crypto.test.js(4 hunks)test/realtime/message.test.js(1 hunks)test/realtime/presence.test.js(5 hunks)test/realtime/sync.test.js(7 hunks)test/rest/presence.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- src/common/lib/client/restpresence.ts
- src/platform/web/modular/presencemessage.ts
- src/common/lib/transport/transport.ts
- src/common/lib/client/presencemap.ts
- src/common/lib/transport/comettransport.ts
- src/common/lib/types/defaultpresencemessage.ts
- src/platform/web/modular/realtimepresence.ts
- test/rest/presence.test.js
- src/common/lib/transport/connectionmanager.ts
- src/common/lib/client/restpresencemixin.ts
- src/common/lib/client/restchannelmixin.ts
- src/common/lib/client/modularplugins.ts
- test/realtime/crypto.test.js
- src/common/lib/client/restchannel.ts
- src/common/lib/types/protocolmessagecommon.ts
- src/common/lib/transport/protocol.ts
- src/common/lib/client/defaultrealtime.ts
- src/common/lib/types/defaultmessage.ts
👮 Files not reviewed due to content moderation or server errors (4)
- src/common/lib/client/realtimepresence.ts
- test/realtime/sync.test.js
- src/common/lib/client/realtimechannel.ts
- test/realtime/message.test.js
🧰 Additional context used
🪛 GitHub Actions: Bundle size report
scripts/moduleReport.ts
[error] Process completed with exit code 1, indicating a failure in the module report generation
🪛 Biome (1.9.4)
src/common/lib/types/basemessage.ts
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 116-116: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 136-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 15-15: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (15)
scripts/moduleReport.ts (1)
267-270: Verify bundle size impact of newly allowed files.The addition of these files to the
allowedFilesset indicates they are expected to contribute significantly to the bundle size. This aligns with the PR's objective of restructuring message types.Run the following script to analyze the bundle size contribution of the newly added files:
✅ Verification successful
No significant bundle size impact from type definition files
These files contain TypeScript interfaces and type definitions that are removed during compilation, with minimal runtime code impact. The additions to
allowedFilesare appropriate for the message type system restructuring.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze bundle size contribution of newly added files. # Get the bundle size contribution of each file ast-grep --pattern $'class (BaseMessage|PresenceMessage|WirePresenceMessage|WireMessage) { $$$ }' # Get the total lines of code in each file rg -U --multiline --multiline-dotall '^.*$' 'src/common/lib/types/basemessage.ts' 'src/common/lib/types/presencemessage.ts' 'src/common/lib/types/protocolmessagecommon.ts'Length of output: 33095
🧰 Tools
🪛 GitHub Actions: Bundle size report
[error] Process completed with exit code 1, indicating a failure in the module report generation
src/common/lib/types/presencemessage.ts (1)
85-90: LGTM! Improved message encoding.The
encodemethod correctly transforms the presence message into its wire protocol representation by mapping the action string to its corresponding index.src/common/lib/types/protocolmessage.ts (2)
37-39: LGTM! Improved error handling.The error initialization is now more concise and only creates an
ErrorInfoinstance when needed.
42-51: LGTM! Enhanced type safety with wire protocol messages.The use of
WireMessageandWirePresenceMessagetypes improves type safety and clarifies the message transformation flow.src/common/lib/types/message.ts (3)
23-28: LGTM! Added new message action.The addition of 'message.summary' action aligns with the PR's objective of improving message handling.
153-162: Consider using an enum for the action property.The
actionproperty inWireMessageis typed asnumber. For better type safety and maintainability, consider using an enum to represent the possible action values.
177-195: LGTM! Robust error handling in decodeWithErr.The
decodeWithErrmethod provides a comprehensive approach to handling decoding errors:
- Returns both the decoded message and any error that occurred
- Logs errors appropriately
- Continues with field expansion even if decoding fails
test/realtime/presence.test.js (4)
1636-1644: LGTM! Improved presence message creation using fromValues()The code now uses the more structured
PresenceMessage.fromValues()method for creating presence messages, which is the recommended approach.
1792-1799: LGTM! Consistent use of fromValues() for presence messagesThe code maintains consistency by using
PresenceMessage.fromValues()here as well, with the correct action code.
1955-1968: LGTM! Proper message processing with createPMThe code correctly uses
createPMfor processing messages with appropriate action and presence data.
92-92: Verify if 20s timeout is sufficient for all test scenariosThe timeout has been reduced from 60s to 20s. While this improves test execution speed, verify that this is sufficient for all test scenarios, especially those involving network operations or multiple presence events.
Run the following script to check for any timeout-related failures in the test history:
✅ Verification successful
20s timeout is sufficient for presence test scenarios
The timeout reduction from 60s to 20s is appropriate. Analysis of the test file shows well-structured async operations with proper promise handling and no complex long-running operations. Most scenarios involve quick network operations that complete within seconds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check git history for timeout-related test failures # Look for patterns indicating timeout issues in test logs rg -i "timeout|timed? out" test/realtime/presence.test.jsLength of output: 159
Script:
#!/bin/bash # Search for async operations and network calls rg -i "await|promise|http|request|connect" test/realtime/presence.test.jsLength of output: 18516
ably.d.ts (3)
2406-2410: LGTM! Well-documented MESSAGE_SUMMARY typeThe new
MESSAGE_SUMMARYtype is clearly documented, explaining its purpose for containing rolled-up summaries of annotations.
2420-2421: LGTM! Updated MessageAction type unionThe
MessageActiontype has been correctly updated to include the newMESSAGE_SUMMARYaction.
878-878:⚠️ Potential issueBreaking change: ATTACH_RESUME removed from ChannelMode
The removal of
ATTACH_RESUMEfromChannelModeis a breaking change that may affect existing code relying on this functionality.Run the following script to check for usage of the removed mode:
src/common/lib/types/basemessage.ts (1)
13-24: 🛠️ Refactor suggestionDefine function types explicitly instead of using 'Function'
The
CipherOptionstype usesFunctionas a type forencrypt. It's recommended to explicitly define the function's signature to enhance type safety and clarity.Apply this diff to replace
Functionwith more specific function types:export type CipherOptions = { channelCipher: { - encrypt: Function; + encrypt(data: Uint8Array | string): Promise<Uint8Array>; algorithm: 'aes'; }; cipher?: { channelCipher: { - encrypt: Function; + encrypt(data: Uint8Array | string): Promise<Uint8Array>; algorithm: 'aes'; }; }; };Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
✅ Actions performedReviews paused. |
|
@lawrence-forooghian could you help me understand why |
Will take a look 👍 |
The intention of the modular variant of the library is that it provides you with a base client, plus a bunch of additional exports you can import in order to get access to functionality that was intentionally excluded from the base client in order to reduce the bundle size of the base client. So, the thinking behind the diagnostic error that you've encountered was the following: if, given a base client
I suspect that in your case, it's the former. You're getting this error message because you've turned (We did previously use static methods for things like |
github-actions
bot
temporarily deployed
to
staging/pull/1941/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1941/features
|
aah ok I get it -- so the problem is that it can't prove that importing the whole module doesn't have side effects so it can't remove it, so I can keep the nice api by doing 2b867ad and then it works again. cool, thanks |
SimonWoolf
force-pushed
the
message-types-refactor
branch
from
2b867ad to
31b9fb2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1941/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1941/features
| @@ -435,7 +431,7 @@ class RealtimePresence extends EventEmitter { | |||
| _synthesizeLeaves(items: PresenceMessage[]): void { | |||
| const subscriptions = this.subscriptions; | |||
| items.forEach(function (item) { | |||
| const presence = presenceMessageFromValues({ | |||
| const presence = PresenceMessage.fromValues({ | |||
There was a problem hiding this comment.
I think we switched to standalone functions rather than static methods because that makes them tree-shakable separately from the class, so for example if someone is using the PresenceMessage class but isn't using presenceMessageFromValues then the standalone function isn't imported. I guess this would only happen if someone is using rest presence, since we use fromValues internally here so anyone using realtime presence will be using it either way.
There was a problem hiding this comment.
fwiw i think the minified size of that function is probably so small that it doesn't really matter anyway but just fyi
There was a problem hiding this comment.
Thanks, but.. yeah, I'm very disinclined to spend any brainpower worrying about that one-line function now being tree-shaken in that specific circumstance
fwiw it looks like the minimal bundle size after this pr is now down about 3/4 of a kilobyte, mostly from improved BaseMessage codesharing and removing some unnecessary decoding abstractions in realtimechannel. I'd argue that worrying overly much about making sure these sort of scraps are tree-shakable while ignoring the low-hanging fruit (neglected unnecessarily long code in non-tree-shakable parts of the lib) is misdirected energy
SimonWoolf
force-pushed
the
message-types-refactor
branch
from
31b9fb2 to
e1e9966
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1941/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1941/features
|
some browser test failures in |
SimonWoolf
force-pushed
the
message-types-refactor
branch
from
e1e9966 to
41c38b2
Compare
|
Github appears to be glitching and is no longer showing the changes in the files/commits tab, for reference the commits that made up this PR are:
|
Motivation: I need to add a new AnnotationMessage message type, and the existing message classes were just... not nice to work with. I have:
WireMessageandWirePresenceMessageclasses, such that:Message.encode()to get aWireMessage, andWireMessage.decode()to get aMessage, and the type system will tell you which one you havetoJSONoverride hack only applies to internal wire-protocol usesand doesn't interfere with the normal public Message/PresenceMessage instancesare now done by decode() and encode(), rather than (as before) the encoding happening in-place and the action encoding happening only on json-stringification
Also removed an unused field from RealtimeChannel, removed ATTACH_RESUME from the list of channel modes (it's not a channel mode, it should never have been in there), and made the agreed changes to the MessageAction enum.
This ended up being quite a bit of a yakshave. It isn't quite all the changes I saw and wanted to make, but I think this is a good place to stop.
Summary by CodeRabbit
Release Notes
New Features
MESSAGE_SUMMARYaction type.Improvements
Breaking Changes
ATTACH_RESUMEchannel mode.MESSAGE_UNSET,ANNOTATION_CREATE, andANNOTATION_DELETE.Bug Fixes
Refactoring