Mutable message field changes and Message/PresenceMessage.fromValues refactoring #1923
Conversation
SimonWoolf
requested review from
owenpearson,
lawrence-forooghian and
splindsay-92
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces significant modifications to the Ably library's type definitions and message handling. Key changes include renaming properties in the Changes
Possibly related PRs
Suggested reviewers
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/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
![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
🧹 Outside diff range and nitpick comments (13)
src/platform/web/modular/presencemessage.ts (1)
9-9: LGTM! Consider updating documentation.The addition of
Platform.Cryptoparameter aligns with the PR objectives for improved cryptography support. The type assertion maintains API compatibility.Consider adding a JSDoc comment to document the crypto parameter's purpose and its relationship to encrypted channel support.
src/common/lib/types/defaultpresencemessage.ts (1)
15-20: Consider improving array type definitionSimilar to the previous method, consider declaring the parameter type explicitly instead of using type assertion.
- static async fromEncodedArray( - encodedArray: Array<unknown>, - options?: API.ChannelOptions, - ): Promise<PresenceMessage[]> { - return fromEncodedArray(Logger.defaultLogger, Platform.Crypto, encodedArray as WireProtocolPresenceMessage[], options); + static async fromEncodedArray( + encodedArray: WireProtocolPresenceMessage[], + options?: API.ChannelOptions, + ): Promise<PresenceMessage[]> { + return fromEncodedArray(Logger.defaultLogger, Platform.Crypto, encodedArray, options);src/common/lib/types/defaultmessage.ts (2)
23-24: Consider adding runtime type validation before type assertionWhile the type assertion to
WireProtocolMessageimproves type safety at compile time, consider adding runtime validation to ensure theencodedparameter matches the expected wire protocol format before the assertion.static async fromEncoded(encoded: unknown, inputOptions?: API.ChannelOptions): Promise<Message> { + if (!encoded || typeof encoded !== 'object') { + throw new Error('Invalid wire protocol message format'); + } return fromEncoded(Logger.defaultLogger, Platform.Crypto, encoded as WireProtocolMessage, inputOptions); }
27-28: Consider adding runtime type validation for array elementsSimilar to
fromEncoded, consider adding runtime validation to ensure each element in the array matches the expected wire protocol format.static async fromEncodedArray(encodedArray: Array<unknown>, options?: API.ChannelOptions): Promise<Message[]> { + if (!Array.isArray(encodedArray)) { + throw new Error('Expected an array of wire protocol messages'); + } + encodedArray.forEach((encoded, index) => { + if (!encoded || typeof encoded !== 'object') { + throw new Error(`Invalid wire protocol message format at index ${index}`); + } + }); return fromEncodedArray(Logger.defaultLogger, Platform.Crypto, encodedArray as WireProtocolMessage[], options); }src/common/lib/types/protocolmessage.ts (1)
94-103: Consider strengthening type assertionsWhile the wire protocol handling is good, we can improve type safety further.
Consider this safer approach:
- const dm = (deserialized.messages as WireProtocolMessage[]); + const dm = Array.isArray(deserialized.messages) + ? (deserialized.messages as WireProtocolMessage[]) + : []; - const dp = deserialized.presence as WireProtocolPresenceMessage[]; + const dp = Array.isArray(deserialized.presence) + ? (deserialized.presence as WireProtocolPresenceMessage[]) + : [];This change adds runtime type checking before the type assertions, making the code more robust against malformed input.
src/common/lib/types/message.ts (2)
Line range hint
109-121: Consider enhancing error handling in fromEncodedThe current error handling catches all errors and only logs them. Consider:
- Adding error context to help with debugging
- Distinguishing between different types of errors
try { await decode(msg, options); } catch (e) { - Logger.logAction(logger, Logger.LOG_ERROR, 'Message.fromEncoded()', (e as Error).toString()); + const error = e as Error; + Logger.logAction( + logger, + Logger.LOG_ERROR, + 'Message.fromEncoded()', + `Failed to decode message: ${error.toString()}. Message ID: ${msg.id}, Encoding: ${msg.encoding}` + ); }
368-379: Add JSDoc comments for expandFields methodThe
expandFieldsmethod handles important backward compatibility logic but lacks documentation explaining its purpose and the TM2k/TM2o references.+ /** + * Expands fields for backward compatibility with older message formats. + * - TM2k: Copies version to serial if serial is missing + * - TM2o: Copies timestamp to createdAt if createdAt is missing + */ expandFields() { if (this.action === 'message.create') { // TM2kably.d.ts (1)
Based on the gathered information, I can now provide a final response about the implementation of PR objectives:
PR Objectives Implementation Status
- Wire protocol type implementation is present:
WireProtocolMessageandWireProtocolPresenceMessagetypes are defined- Both properly handle action field conversion between number (wire) and string (API) formats
- stringifyAction option:
- Still present in type definitions:
fromValues(values: PresenceMessage | Record<string, unknown>, stringifyAction?: boolean): PresenceMessage;- This appears to be inconsistent with PR objective to remove this option
- CipherParams interface:
- Properly defined with required fields
- No problematic assertions found in the implementation
The implementation is incomplete as the
stringifyActionparameter is still present in the type definitions, which contradicts the PR objective to remove this option. This parameter appears in the PresenceMessage interface'sfromValuesmethod signature.Locations needing attention:
ably.d.ts: RemovestringifyActionparameter fromfromValuesmethod signature- Update related documentation that references this parameter
🔗 Analysis chain
Line range hint
1-2368: Verify implementation of all PR objectivesWhile the Message interface changes are well implemented, let's verify the complete implementation of the PR objectives:
- Wire protocol type implementation
- Removal of stringifyAction option
- Fix for incorrect cipher options assertions
Let me gather more information about the wire protocol implementation and stringifyAction removal.
Let me try a different approach to check the wire protocol implementation and other changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for wire protocol related types and stringifyAction references echo "Checking for wire protocol types..." rg -l "WireProtocol|stringifyAction|CipherOptions" # Search for fromWireProtocol method echo "Checking for fromWireProtocol implementation..." ast-grep --pattern 'fromWireProtocol'Length of output: 1941
Script:
#!/bin/bash # Check wire protocol type definitions echo "Checking wire protocol type definitions..." rg -A 10 "WireProtocol.*Message.*=.*{" # Check for stringifyAction references echo "Checking stringifyAction references..." rg "stringifyAction" # Check for cipher options assertions echo "Checking cipher options assertions..." rg -A 5 "CipherOptions.*assert"Length of output: 704
Script:
#!/bin/bash # Check wire protocol type definitions with fixed regex echo "Checking wire protocol type definitions..." rg -A 10 "type WireProtocol" # Check Message interface changes echo "Checking Message interface changes..." rg -A 15 "interface Message" # Check PresenceMessage fromValues method echo "Checking PresenceMessage fromValues..." rg -A 5 "fromValues.*PresenceMessage" # Check CipherParams interface echo "Checking CipherParams interface..." rg -A 10 "interface CipherParams"Length of output: 7363
src/common/lib/client/restpresencemixin.ts (1)
33-38: Add Error Handling to Asynchronous FunctionThe asynchronous function lacks error handling for potential exceptions during decoding or processing of the response body. Consider adding a
try-catchblock to handle any errors that may occur, enhancing the robustness of the method.Apply this diff to include error handling:
async (body, headers, unpacked) => { + try { const decoded: WireProtocolPresenceMessage[] = unpacked ? body as WireProtocolPresenceMessage[] : Utils.decodeBody(body, client._MsgPack, format); return fromEncodedArray(presence.logger, Platform.Crypto, decoded, options); + } catch (error) { + presence.logger.error('Error processing presence history:', error); + throw error; + } }src/common/lib/client/restpresence.ts (1)
37-41: Validate the response body before casting toWireProtocolPresenceMessage[]When
unpackedistrue, the code castsbodydirectly toWireProtocolPresenceMessage[]. To enhance robustness and prevent potential runtime errors from unexpected response formats, consider validating thatbodyindeed conforms to the expected structure before casting.src/common/lib/types/presencemessage.ts (2)
Line range hint
98-98: Correct the substring parameters in 'isSynthesized' method.The parameters in
substring(this.connectionId.length, 0)are reversed. Thesubstringmethod expects the start index to be less than the end index. To check ifiddoes not start withconnectionId, the indices should be swapped.Apply this diff to fix the logic:
-return this.id.substring(this.connectionId.length, 0) !== this.connectionId; +return this.id.substring(0, this.connectionId.length) !== this.connectionId;🧰 Tools
🪛 GitHub Check: lint
[failure] 4-4:
'Utils' is defined but never used. Allowed unused vars must match /^_/u
[failure] 6-6:
'MsgPack' is defined but never used. Allowed unused vars must match /^_/u
[failure] 9-9:
'ChannelOptions' is defined but never used. Allowed unused vars must match /^_/u
Line range hint
134-143: Improve data encoding in 'toJSON' method.In the
toJSONmethod, consider handling the encoding more robustly to ensure that data is correctly encoded and that theencodingproperty reflects all transformations applied.Apply this diff to improve encoding handling:
if (data && Platform.BufferUtils.isBuffer(data)) { if (arguments.length > 0) { /* stringify call */ - encoding = encoding ? encoding + '/base64' : 'base64'; + encoding = encoding ? `${encoding}/base64` : 'base64'; data = Platform.BufferUtils.base64Encode(data); } else { /* Called by msgpack. toBuffer returns a datatype understandable by * that platform's msgpack implementation (Buffer in node, Uint8Array * in browsers) */ data = Platform.BufferUtils.toBuffer(data); } }This ensures consistency in the encoding string and improves readability.
🧰 Tools
🪛 GitHub Check: lint
[failure] 4-4:
'Utils' is defined but never used. Allowed unused vars must match /^_/u
[failure] 6-6:
'MsgPack' is defined but never used. Allowed unused vars must match /^_/u
[failure] 9-9:
'ChannelOptions' is defined but never used. Allowed unused vars must match /^_/usrc/common/lib/client/realtimechannel.ts (1)
678-678: Review necessity of callingmsg.expandFields().The comment suggests that
expandFields()has already been called infromWireProtocol, but due to field copying fromprotocolMessage, it might be necessary to call it again. Verify if this call is essential to ensure all message fields are correctly expanded, and remove it if redundant to optimize performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
ably.d.ts(1 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/modularplugins.ts(1 hunks)src/common/lib/client/realtimechannel.ts(2 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/types/defaultmessage.ts(1 hunks)src/common/lib/types/defaultpresencemessage.ts(1 hunks)src/common/lib/types/message.ts(9 hunks)src/common/lib/types/presencemessage.ts(3 hunks)src/common/lib/types/protocolmessage.ts(2 hunks)src/common/lib/util/utils.ts(1 hunks)src/platform/web/modular/presencemessage.ts(1 hunks)src/platform/web/modular/realtimepresence.ts(1 hunks)test/realtime/crypto.test.js(2 hunks)test/realtime/message.test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/common/lib/client/restpresencemixin.ts
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/common/lib/util/utils.ts
[error] 471-471: 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 Check: lint
src/common/lib/types/presencemessage.ts
[failure] 4-4:
'Utils' is defined but never used. Allowed unused vars must match /^_/u
[failure] 6-6:
'MsgPack' is defined but never used. Allowed unused vars must match /^_/u
[failure] 9-9:
'ChannelOptions' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (33)
src/platform/web/modular/realtimepresence.ts (1)
6-6: LGTM! The wire protocol handling enhancement looks good.
The addition of presenceMessageFromWireProtocol aligns well with the PR's objective to introduce explicit wire protocol message handling. The changes maintain consistency with existing presence message utilities while enhancing the plugin interface.
Let's verify that this change is properly integrated with other components:
Also applies to: 13-13
✅ Verification successful
Wire protocol integration is properly implemented and used consistently
The verification confirms that presenceMessageFromWireProtocol is well-integrated across the codebase:
- Correctly imported and exported in
realtimepresence.ts - Properly defined in the modular plugin interface (
modularplugins.ts) - Used consistently in protocol message processing (
protocolmessage.ts) - Integrated with the default realtime implementation (
defaultrealtime.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of presenceMessageFromWireProtocol across the codebase
# Expected: Should find references in RealtimeChannel, RestPresenceMixin, and DefaultRealtime
# Search for direct usage of presenceMessageFromWireProtocol
echo "Checking direct usage of presenceMessageFromWireProtocol:"
rg "presenceMessageFromWireProtocol" -A 2
# Search for wire protocol related presence message processing
echo "Checking for wire protocol presence message processing:"
ast-grep --pattern 'fromWireProtocol($_)'
Length of output: 2682
src/platform/web/modular/presencemessage.ts (2)
3-3: Verify the non-relative import path.
The import uses a non-relative path. Let's verify that the module resolution is configured correctly.
✅ Verification successful
Import path is correctly configured and widely used across the codebase
The import path 'common/platform' is valid as:
- The platform module exists at
./src/common/platform.ts - This import pattern is consistently used across 27 different files in the codebase
- The relative path from
src/platform/web/modular/presencemessage.tscorrectly resolves tosrc/common/platform.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the module resolution for the Platform import
# Expected: Find the actual location of the platform module and any other usages
# Find the actual location of the platform module
fd -t f "platform.ts" -x echo "Platform module location: {}"
# Find other usages of this import pattern
rg "from 'common/platform'" -A 1
Length of output: 4444
13-13: LGTM! Verify crypto parameter usage.
The change maintains consistency with decodePresenceMessage and properly supports encrypted channels.
✅ Verification successful
Crypto parameter usage is consistent across the codebase
The verification shows that Platform.Crypto is consistently passed as the second parameter to both fromEncoded and fromEncodedArray across all presence message decoding implementations:
src/platform/web/modular/presencemessage.tssrc/common/lib/types/defaultpresencemessage.tssrc/common/lib/client/restpresence.tssrc/common/lib/client/restchannelmixin.tssrc/common/lib/client/restpresencemixin.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of Platform.Crypto in message decoding
# Expected: Find all message decoding calls to ensure consistent crypto parameter usage
# Search for fromEncoded/fromEncodedArray usage
rg "fromEncoded(.*Platform\.Crypto" -A 1
# Search for potential places where crypto might be needed but missing
ast-grep --pattern 'fromEncoded($_)'
Length of output: 174
Script:
#!/bin/bash
# Search for fromEncoded and fromEncodedArray usage with proper escaping
rg "fromEncoded.*\(.*Crypto" -A 2
# Search for all fromEncoded/fromEncodedArray calls to ensure we're not missing any
rg "fromEncoded(Array)?\(" -A 2
# Look for presence message decoding implementations
ast-grep --pattern 'function $name($_) {
$$$
fromEncoded($$$)
$$$
}'
Length of output: 11285
src/common/lib/types/defaultpresencemessage.ts (2)
3-5: LGTM! Import changes align with PR objectives
The addition of WireProtocolPresenceMessage type and Platform imports supports the transition to explicit wire protocol handling, improving type safety.
11-12: Consider moving type assertion to parameter type
Instead of using type assertion in the method call, consider declaring the parameter type explicitly for better type safety.
- static async fromEncoded(encoded: unknown, inputOptions?: API.ChannelOptions): Promise<PresenceMessage> {
- return fromEncoded(Logger.defaultLogger, Platform.Crypto, encoded as WireProtocolPresenceMessage, inputOptions);
+ static async fromEncoded(encoded: WireProtocolPresenceMessage, inputOptions?: API.ChannelOptions): Promise<PresenceMessage> {
+ return fromEncoded(Logger.defaultLogger, Platform.Crypto, encoded, inputOptions);Verify crypto parameter usage
The addition of Platform.Crypto parameter needs verification across all call sites.
✅ Verification successful
Let me analyze the results from the rg command to verify the crypto parameter usage.
The search results show several implementations and usages of fromEncoded:
-
The core implementation in
src/common/lib/types/presencemessage.tsandsrc/common/lib/types/message.tsboth acceptCryptoas their second parameter. -
All call sites properly pass the crypto parameter:
src/platform/web/modular/message.tsuses bothnullandCryptosrc/platform/web/modular/presencemessage.tsusesPlatform.Cryptosrc/common/lib/types/defaultmessage.tsusesPlatform.Cryptosrc/common/lib/types/defaultpresencemessage.ts(our file) usesPlatform.Crypto
Type assertion and crypto parameter usage are both correct
The type assertion in the code is actually appropriate here as it's used in a static factory method that needs to handle unknown input. The current implementation provides runtime type safety through the implementation while maintaining flexibility at the API level. The Platform.Crypto parameter is also being used consistently across all relevant call sites.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all fromEncoded calls to verify proper crypto parameter usage
rg -A 2 "fromEncoded\(" --type ts
Length of output: 3794
src/common/lib/client/modularplugins.ts (2)
11-11: LGTM! Clear and consistent import naming
The addition of fromWireProtocol import aligns with the PR's objective to support wire protocol message handling, and follows the established naming pattern.
19-19: Verify implementations of PresenceMessagePlugin interface
The interface addition looks good and aligns with the wire protocol support. However, we should verify that all implementations of this interface have been updated accordingly.
✅ Verification successful
Let me gather more information to verify if there are any implementations of the interface that might have been missed.
Let me try one final search to find where this plugin is actually instantiated and used.
Interface implementation is handled through direct imports
Based on the evidence gathered, the PresenceMessagePlugin interface is implemented through direct imports of the wire protocol functions rather than through class or object implementations. The interface serves as a type definition for bundling related functions, and the actual implementation is provided by importing and using the same functions that define the interface's type signatures. This is confirmed by:
- No explicit class/object implementations of the interface found
- Direct usage of imported functions in
protocolmessage.ts - The interface methods are typed using
typeofof the imported functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for classes/objects implementing PresenceMessagePlugin
# to ensure they implement the new method
# Look for potential implementations
ast-grep --pattern 'class $_ implements PresenceMessagePlugin {
$$$
}'
# Also check for object literal implementations
rg -l 'PresenceMessagePlugin' | xargs rg 'presenceMessageFromWireProtocol'
Length of output: 758
Script:
#!/bin/bash
# Search for the interface definition and any potential implementations
rg -A 5 "interface PresenceMessagePlugin"
# Look for object literal assignments that might implement the interface
ast-grep --pattern "const $_ : PresenceMessagePlugin = {
$$$
}"
# Also search for type assertions to PresenceMessagePlugin
ast-grep --pattern "$_ as PresenceMessagePlugin"
Length of output: 765
Script:
#!/bin/bash
# Search for imports and usage of the plugin
rg -A 3 "import.*PresenceMessagePlugin"
# Look for where the plugin methods are actually used
rg "const.*presenceMessage.*Plugin"
# Search for any exports of presence message plugin implementations
rg "export.*presenceMessage.*Plugin"
Length of output: 578
src/common/lib/types/defaultmessage.ts (3)
2-2: LGTM! Import changes align with wire protocol refactoring
The addition of WireProtocolMessage type and related imports supports the PR's objective of introducing explicit wire protocol handling.
Also applies to: 10-10, 17-17
36-38: LGTM! New method enhances wire protocol handling
The addition of fromWireProtocol provides a clearer API for handling wire protocol messages, which aligns with the PR objectives.
✅ Verification successful
Method is well covered by tests
The fromWireProtocol method has comprehensive test coverage across multiple test files:
- Dedicated test suite in
test/realtime/message.test.js - Integration tests in crypto scenarios in
test/realtime/crypto.test.js - Tests cover both basic message creation and update scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify test coverage for new fromWireProtocol method
rg "fromWireProtocol" test/
Length of output: 579
31-34: Verify test coverage for removed options parameter
The removal of the options parameter aligns with the PR objectives. Since this method is used by tests, we should verify that all test cases are updated accordingly.
✅ Verification successful
No impact from options parameter removal
Based on the codebase analysis, the test files only use fromValues with a single parameter containing message properties. No test cases were found using the removed options parameter, indicating that its removal won't affect test coverage.
- The only test usage found is in
test/realtime/presence.test.jswhich usesPresenceMessage.fromValues()with just the properties parameter - All other occurrences of
fromValuesin the codebase also show single-parameter usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using fromValues with options parameter
rg -l "fromValues.*options" test/
Length of output: 35
Script:
#!/bin/bash
# Search for test files using fromValues method with context
rg -A 3 "fromValues" test/
Length of output: 1181
Script:
#!/bin/bash
# Search for test files using Message.fromValues with context
rg -A 3 "Message\.fromValues" test/
Length of output: 1192
Script:
#!/bin/bash
# Search for test files that might be using the method
fd -e test.ts -e spec.ts -e test.js -e spec.js --exec rg -l "fromValues"
Length of output: 105
Script:
#!/bin/bash
# Let's check the actual Message.fromValues usage in the codebase
ast-grep --pattern 'Message.fromValues($$$)'
Length of output: 46
Script:
#!/bin/bash
# Let's also check for any direct fromValues usage
ast-grep --pattern 'fromValues($$$)'
Length of output: 630
Script:
#!/bin/bash
# Let's check the test files content more thoroughly
rg -C 5 "fromValues" test/
Length of output: 2396
src/common/lib/client/restchannelmixin.ts (3)
5-6: LGTM: Import changes align with wire protocol refactoring
The new imports support the transition to explicit wire protocol handling and improved message processing.
44-44: Verify error handling for message decoding
The fromEncodedArray call should include proper error handling for cases where decoding fails or crypto operations fail.
Let's check the implementation of fromEncodedArray:
✅ Verification successful
Let me search for the implementation of fromEncodedArray to verify error handling:
Let me check the implementation of fromEncodedArray in message types:
Error handling is properly implemented
The fromEncodedArray implementation and its dependencies have comprehensive error handling:
- The
fromEncodedArrayfunction usesPromise.allto process each message, where each message is handled byfromEncoded - The
fromEncodedfunction has a try-catch block that catches and logs any decoding errors - The decoding process itself has multiple layers of error handling for:
- Encryption/decryption failures
- Vcdiff delta decoding errors
- Unknown encoding errors
- General processing errors with specific error codes
All errors are properly caught, logged, and propagated with appropriate error codes and messages, making the implementation robust and production-ready.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for fromEncodedArray implementation
ast-grep --pattern 'function fromEncodedArray($$$) {
$$$
}'
# Look for error handling patterns in related files
rg 'catch|throw' --type ts
Length of output: 27388
Script:
#!/bin/bash
# Search for fromEncodedArray implementation
ast-grep --pattern 'fromEncodedArray($$$) {
$$$
}'
# Also search for any direct references to this function
rg 'fromEncodedArray' --type ts
Length of output: 3630
Script:
#!/bin/bash
# Search for the implementation in message.ts
rg -A 20 'export async function fromEncodedArray' src/common/lib/types/message.ts
# Also check for error handling in the decoding process
rg -A 10 'catch' src/common/lib/types/message.ts
Length of output: 1919
40-42: Verify type safety of message body handling
The type assertion body as WireProtocolMessage[] could be unsafe if the message structure doesn't match expectations. Consider adding runtime validation or type guards.
Let's verify the wire protocol message structure across the codebase:
✅ Verification successful
Type assertion is safe in this context
The type assertion is safe because:
WireProtocolMessageis a well-defined type that extends the base Message type with a numeric action field- The
decodeBodyutility function handles format-specific decoding (msgpack/json) - The
fromEncodedArrayfunction provides additional validation and processing of the decoded messages - There are proper error boundaries in the Resource class that catch and handle decoding failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WireProtocolMessage type definition and usage
ast-grep --pattern 'interface WireProtocolMessage {
$$$
}'
# Search for other type assertions to WireProtocolMessage
rg 'as WireProtocolMessage'
Length of output: 582
Script:
#!/bin/bash
# Let's check the WireProtocolMessage interface definition and related type guards
rg -A 10 "interface WireProtocolMessage"
# Check Utils.decodeBody implementation for validation
ast-grep --pattern 'decodeBody($$$) {
$$$
}'
# Check fromEncoded and fromEncodedArray implementations
ast-grep --pattern 'fromEncoded($$$) {
$$$
}'
ast-grep --pattern 'fromEncodedArray($$$) {
$$$
}'
Length of output: 196
Script:
#!/bin/bash
# Try different approaches to find the interface and implementations
# Look for WireProtocolMessage in typescript files
rg -t typescript "WireProtocolMessage" -B 2 -A 5
# Look for Utils.decodeBody usage and definition
rg -t typescript "decodeBody" -B 2 -A 5
# Look for type validation or guards
rg -t typescript "instanceof.*WireProtocolMessage|isWireProtocolMessage" -B 2 -A 5
Length of output: 278
Script:
#!/bin/bash
# Search without typescript filter
rg "WireProtocolMessage" -B 2 -A 5
# Look for Utils.decodeBody usage and definition
rg "decodeBody" -B 2 -A 5
# Find files that might contain the interface
fd -e ts -x cat {} | rg -A 10 "interface WireProtocolMessage"
Length of output: 13838
src/common/lib/client/defaultrealtime.ts (2)
17-17: LGTM! Import addition aligns with wire protocol refactoring
The new import for fromWireProtocol is correctly grouped with related presence message imports and follows the established naming pattern.
43-43: LGTM! Plugin configuration enables wire protocol handling
The addition of presenceMessageFromWireProtocol to the RealtimePresence plugin options is consistent with the refactoring goals and maintains the established structure.
Let's verify the wire protocol handling implementation:
✅ Verification successful
Wire protocol handling is properly integrated and consistent
The verification confirms that presenceMessageFromWireProtocol is correctly imported and used across the codebase, particularly in the RealtimePresence plugin implementation. The function is properly referenced in the modular plugins system and the default realtime client, maintaining consistency with the wire protocol handling requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the wire protocol implementation across the codebase
# Expected: Find usage of fromWireProtocol in message processing
# Check for wire protocol message processing implementations
ast-grep --pattern 'fromWireProtocol($_)'
# Check for any remaining stringifyAction usage that should have been removed
rg 'stringifyAction'
Length of output: 528
Script:
#!/bin/bash
# Let's verify the presence message wire protocol implementation
ast-grep --pattern 'presenceMessageFromWireProtocol'
# Check the implementation of fromWireProtocol in presence context
rg -A 5 'fromWireProtocol.*presence'
Length of output: 2497
src/common/lib/types/protocolmessage.ts (3)
6-15: LGTM: Import changes align with wire protocol refactoring
The introduction of wire protocol types and methods through imports is well-structured and follows TypeScript best practices. This change supports the PR's objective of introducing explicit wire protocol handling.
106-106: LGTM: Clean object construction
The return statement properly combines the deserialized data with explicitly typed message arrays using object spread.
113-113: Verify test coverage for wire protocol changes
The test helper has been updated, but we should verify that all wire protocol scenarios are covered in tests.
✅ Verification successful
Let's examine the test files to see the actual test coverage.
Wire protocol test coverage is comprehensive
The test files show thorough coverage of wire protocol message handling:
message.test.jsincludes dedicated test suite forfromWireProtocolwith multiple test cases covering different actions (create, update)crypto.test.jsverifies wire protocol handling with encoded messages- Tests validate message properties, timestamps, versioning, and serialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relevant test cases
rg -l "fromDeserializedIncludingDependencies|fromWireProtocol" --type ts --type js test/
Length of output: 149
Script:
#!/bin/bash
# Check test coverage in the found files
rg "fromDeserializedIncludingDependencies|fromWireProtocol" -B 2 -A 2 test/realtime/message.test.js test/realtime/crypto.test.js
Length of output: 2645
src/common/lib/types/message.ts (4)
10-11: Well-structured type definitions for wire protocol messages!
The introduction of explicit type imports and the WireProtocolMessage type definition effectively enforces the wire protocol format while maintaining type safety. This aligns well with the PR objective of introducing wire-protocol types to eliminate unnecessary type assertions.
Also applies to: 60-61
282-286: Clean implementation of wire protocol message handling!
The fromWireProtocol function provides a clear and explicit method for handling wire protocol messages, replacing the previous stringifyAction boolean property. The automatic field expansion through expandFields() is a nice touch for maintaining backward compatibility.
319-324: Well-structured property updates for wire protocol support!
The property changes align well with the wire protocol format while maintaining clear type definitions. The addition of optional properties provides flexibility for different message types.
360-361: Verify the impact of property renames
The renaming of properties from updatedAt to createdAt and updateSerial to version might affect dependent code.
src/common/lib/util/utils.ts (1)
471-472: Verify the usage of Properties type in message handling
Let's ensure the new Properties type is being used consistently across message-related code.
✅ Verification successful
Properties type is correctly used in message handling
Based on the verification results:
- The
Properties<A>type is correctly used infromValuesimplementations for bothMessageandPresenceMessageclasses - The type correctly captures all non-function properties of the respective classes as seen in their implementations
- The usage is consistent across message-related code, with proper implementations in both default message classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of Properties type in message-related code
# Expected: Find usage in Message and PresenceMessage implementations
# Search for Properties type usage
echo "=== Properties type usage ==="
rg "Properties<" -A 3
# Search for message-related files that might need this type
echo -e "\n=== Message-related files ==="
fd -e ts -e js "message" "presence"
Length of output: 1200
Script:
#!/bin/bash
# Let's verify the implementation details of Message and PresenceMessage classes
# to ensure Properties type is used correctly
# Check Message class implementation
echo "=== Message class implementation ==="
ast-grep --pattern 'class Message {
$$$
}'
# Check PresenceMessage class implementation
echo -e "\n=== PresenceMessage class implementation ==="
ast-grep --pattern 'class PresenceMessage {
$$$
}'
# Check for any other fromValues implementations
echo -e "\n=== Other fromValues implementations ==="
ast-grep --pattern 'fromValues($$$) {
$$$
}'
Length of output: 14857
🧰 Tools
🪛 Biome (1.9.4)
[error] 471-471: 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)
test/realtime/crypto.test.js (3)
396-397: LGTM: Message construction refactored correctly
The change from Message.fromValues to Message.fromWireProtocol aligns with the PR objectives to improve message handling and type safety. The new method name better reflects its purpose of handling wire protocol messages.
440-440: LGTM: Consistent refactoring applied
The same refactoring has been correctly applied to the 256-bit test case, maintaining consistency across the test suite.
396-397: Verify test coverage for new wire protocol handling
While the changes look good, let's ensure we have comprehensive test coverage for the new wire protocol message handling.
Also applies to: 440-440
✅ Verification successful
Wire protocol message handling is well tested
The codebase shows comprehensive test coverage for Message.fromWireProtocol:
- Dedicated test suite in
test/realtime/message.test.jswith multiple test cases covering different message actions - Tests for timestamp/version handling
- Tests for message creation and updates
- Integration tests in crypto context showing proper handling with msgpack decoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for wire protocol message handling
# Look for other tests that might need similar updates
rg -l "Message\.fromValues.*msgpack\.decode" --type js
# Look for any remaining uses of the old method
rg "Message\.fromValues" --type js
# Check for tests specifically covering wire protocol handling
rg -B 2 -A 2 "fromWireProtocol" --type test
Length of output: 603
Script:
#!/bin/bash
# Let's try a more focused search for wire protocol tests
# Search for fromWireProtocol in all js files
rg -B 2 -A 2 "fromWireProtocol" --type js
# Look for Message-related test files
fd "message.*test.*js$"
# Check for any Message class definitions and their methods
ast-grep --pattern 'class Message {
$$$
static fromWireProtocol($_) {
$$$
}
$$$
}'
Length of output: 2735
test/realtime/message.test.js (2)
1278-1313: LGTM! Well-structured test suite for wire protocol message handling.
The test suite effectively validates the new fromWireProtocol functionality with comprehensive test cases covering:
- Numeric and string action values
- Missing/undefined actions
- Unknown action values
- JSON and string representation consistency
The test structure using a test case array with clear descriptions makes the test suite maintainable and easy to extend.
1314-1331: LGTM! Thorough validation of create message field population.
The test case effectively verifies that:
serialandcreatedAtfields are correctly populated fromversionandtimestampfor create actions- These fields remain undefined for update actions, maintaining the expected behavior distinction
This aligns well with the PR's objective of enhancing the handling of serial/version information.
ably.d.ts (1)
2335-2368: LGTM! Message interface changes improve clarity and versioning support
The changes to the Message interface are well-documented and align with the PR objectives. The renaming of fields (updatedAt → createdAt, updateSerial → version) and addition of new fields (operation) provide better clarity and support for message versioning.
src/common/lib/client/restpresencemixin.ts (1)
6-7: Imports Updated Appropriately
The import statement now includes WireProtocolPresenceMessage and fromEncodedArray, which are necessary for the new message handling logic. This aligns with the refactoring of message processing.
src/common/lib/client/restpresence.ts (1)
4-4: Imported necessary modules for updated presence message processing
The addition of WireProtocolPresenceMessage and fromEncodedArray imports aligns with the new approach for handling presence messages.
src/common/lib/client/realtimechannel.ts (2)
625-625: Confirm message.channelSerial is defined before assignment.
When assigning channelSerial = message.channelSerial, ensure that message.channelSerial is not undefined or null to prevent potential issues with downstream operations that depend on channelSerial.
681-682: Ensure _lastPayload properties are properly initialized.
When assigning this._lastPayload.messageId and this._lastPayload.protocolMessageChannelSerial, confirm that _lastPayload has been initialized and these properties exist. This prevents potential undefined property access errors.
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
b2a0fff to
de2171e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
de2171e to
6e1dbc2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (15)
src/common/lib/client/restpresencemixin.ts (1)
33-36: Consider adding error handling for message decoding.The current implementation could benefit from explicit error handling for decoding failures and validation of the wire protocol message format.
Consider wrapping the decoding in a try-catch and adding validation:
async (body, headers, unpacked) => { + try { const decoded: WireProtocolPresenceMessage[] = unpacked ? body as WireProtocolPresenceMessage[] : Utils.decodeBody(body, client._MsgPack, format); + + // Validate decoded messages + if (!Array.isArray(decoded)) { + throw new Error('Invalid wire protocol message format'); + } + return fromEncodedArray(presence.logger, Platform.Crypto, decoded, options); + } catch (error) { + presence.logger.error('Error decoding presence messages:', error); + throw error; + } }src/common/lib/types/defaultmessage.ts (2)
27-28: Consider adding runtime validation for array elementsSimilar to
fromEncoded, consider adding runtime validation to ensure each element inencodedArraymatches the wire protocol format before casting.Consider adding validation:
static async fromEncodedArray(encodedArray: Array<unknown>, options?: API.ChannelOptions): Promise<Message[]> { + if (!Array.isArray(encodedArray)) { + throw new Error('Expected an array of wire protocol messages'); + } + for (const encoded of encodedArray) { + if (!encoded || typeof encoded !== 'object' || !('action' in encoded)) { + throw new Error('Invalid wire protocol message format in array'); + } + } return fromEncodedArray(Logger.defaultLogger, Platform.Crypto, encodedArray as WireProtocolMessage[], options); }
36-38: Enhance documentation for fromWireProtocolWhile the method implementation looks good, consider adding JSDoc comments to document:
- The expected wire protocol message format
- When to use this method vs fromValues
- Why it's currently marked as "Used by tests"
Consider adding documentation:
- // Used by tests + /** + * Creates a Message instance from a wire protocol message. + * @param values - Wire protocol message conforming to the Ably protocol specification + * @returns A new Message instance + * @internal Currently used in tests, but may be exposed for public use in future versions + */ static fromWireProtocol(values: WireProtocolMessage): Message {src/common/lib/client/restpresence.ts (1)
37-41: LGTM: Improved message processing with proper crypto handlingThe new implementation correctly:
- Handles both packed and unpacked message formats
- Passes crypto options through to
fromEncodedArray- Aligns with the PR objective of improving encrypted channel support
Consider adding debug logging for message decoding failures to aid troubleshooting in production.
src/common/lib/client/restchannelmixin.ts (1)
40-42: Consider adding error handling for message decodingWhile the wire protocol message handling is well-implemented, consider adding explicit error handling for the decoding operation to gracefully handle malformed messages.
const decoded: WireProtocolMessage[] = unpacked ? body as WireProtocolMessage[] - : Utils.decodeBody(body, client._MsgPack, format); + : Utils.decodeBody(body, client._MsgPack, format) || [];src/common/lib/types/presencemessage.ts (2)
Line range hint
18-31: LGTM: Enhanced cryptography supportThe refactored
fromEncodedfunction properly handles cryptography support and maintains good error handling. However, consider adding a type assertion for the options parameter.Apply this diff to add type assertion:
- const options = normalizeCipherOptions(Crypto, logger, inputOptions ?? null); + const options = normalizeCipherOptions(Crypto, logger, inputOptions ?? null) as API.ChannelOptions | null;
Line range hint
144-167: Consider caching the string representationThe
toStringmethod performs multiple string concatenations which can be inefficient for large messages. Consider using an array join operation instead.Apply this diff to optimize string concatenation:
toString(): string { - let result = '[PresenceMessage'; - result += '; action=' + this.action; - if (this.id) result += '; id=' + this.id; - if (this.timestamp) result += '; timestamp=' + this.timestamp; - if (this.clientId) result += '; clientId=' + this.clientId; - if (this.connectionId) result += '; connectionId=' + this.connectionId; - if (this.encoding) result += '; encoding=' + this.encoding; + const parts = ['[PresenceMessage']; + parts.push('action=' + this.action); + if (this.id) parts.push('id=' + this.id); + if (this.timestamp) parts.push('timestamp=' + this.timestamp); + if (this.clientId) parts.push('clientId=' + this.clientId); + if (this.connectionId) parts.push('connectionId=' + this.connectionId); + if (this.encoding) parts.push('encoding=' + this.encoding); if (this.data) { if (typeof this.data == 'string') { - result += '; data=' + this.data; + parts.push('data=' + this.data); } else if (Platform.BufferUtils.isBuffer(this.data)) { - result += '; data (buffer)=' + Platform.BufferUtils.base64Encode(this.data); + parts.push('data (buffer)=' + Platform.BufferUtils.base64Encode(this.data)); } else { - result += '; data (json)=' + JSON.stringify(this.data); + parts.push('data (json)=' + JSON.stringify(this.data)); } } if (this.extras) { - result += '; extras=' + JSON.stringify(this.extras); + parts.push('extras=' + JSON.stringify(this.extras)); } - result += ']'; - return result; + return parts.join('; ') + ']'; }src/common/lib/types/protocolmessage.ts (2)
94-103: Consider using more descriptive variable namesThe deserialization logic is well-structured and type-safe, but the variable names 'dm' and 'dp' could be more descriptive.
Consider this improvement:
- const dm = (deserialized.messages as WireProtocolMessage[]); - messages = dm.map(m => messageFromWireProtocol(m)); + const wireMessages = (deserialized.messages as WireProtocolMessage[]); + messages = wireMessages.map(m => messageFromWireProtocol(m)); - const dp = deserialized.presence as WireProtocolPresenceMessage[]; - presence = dp.map(pm => presenceMessagePlugin.presenceMessageFromWireProtocol(pm)); + const wirePresenceMessages = deserialized.presence as WireProtocolPresenceMessage[]; + presence = wirePresenceMessages.map(pm => presenceMessagePlugin.presenceMessageFromWireProtocol(pm));
113-113: Consider enhancing the test utility documentationWhile the comment indicates this is used by tests, it would be helpful to document:
- Why this separate function is needed for tests
- How it differs from the main
fromDeserializedfunction- What specific test scenarios it enables
src/common/lib/types/message.ts (3)
Line range hint
74-87: Consider improving error message in fromEncodedWhile the error handling is good, the error message could be more specific about what aspect of decoding failed.
Consider updating the error logging to include more context:
- Logger.logAction(logger, Logger.LOG_ERROR, 'Message.fromEncoded()', (e as Error).toString()); + Logger.logAction( + logger, + Logger.LOG_ERROR, + 'Message.fromEncoded()', + `Failed to decode message: ${(e as Error).toString()}` + );Also applies to: 110-113
366-377: Add JSDoc documentation for expandFields methodThe
expandFieldsmethod handles important field derivation logic but lacks documentation explaining the TM2k and TM2o cases.Consider adding documentation:
+ /** + * Expands derived fields based on wire protocol values. + * - TM2k: Derives serial from version if not present + * - TM2o: Derives createdAt from timestamp if not present + */ expandFields() {
Line range hint
379-405: Consider refactoring toString for better maintainabilityThe
toStringmethod could be made more maintainable by using an array of field definitions to reduce repetition and ensure consistent ordering with class fields.Consider refactoring like this:
toString(): string { - let result = '[Message'; - if (this.name) result += '; name=' + this.name; - // ... more fields - result += ']'; - return result; + const fields = [ + { name: 'name', value: this.name }, + { name: 'id', value: this.id }, + { name: 'timestamp', value: this.timestamp }, + // ... more fields in same order as class declaration + ]; + + return '[Message' + + fields + .filter(f => f.value !== undefined) + .map(f => `; ${f.name}=${ + typeof f.value === 'string' ? f.value : + Platform.BufferUtils.isBuffer(f.value) ? 'buffer=' + Platform.BufferUtils.base64Encode(f.value) : + JSON.stringify(f.value) + }`) + .join('') + + ']';src/common/lib/client/realtimechannel.ts (1)
676-678: Document the necessity of duplicate expandFields callWhile the comment explains why expandFields is called again, it would be helpful to document this more explicitly, perhaps with a code comment that includes an example scenario where the duplicate call is necessary.
-// already done in fromWireProtocol -- but for realtime messages the source -// fields might be copied from the protocolmessage, so need to do it again +/* This expandFields call is necessary even though it's already done in fromWireProtocol. + * For realtime messages, source fields might be copied from the protocol message. + * Example: when a message is received with fields {data: "foo", extras: {key: "value"}} + * these fields need to be expanded again after copying from the protocol message. + */ msg.expandFields()test/realtime/message.test.js (1)
1314-1331: Consider improving test readability with constants.The test effectively verifies the serial/createdAt behavior, but could be more readable with named constants.
Consider this refactor:
it('create message should fill out serial and createdAt from version/timestamp', function () { - const values = { action: 1, timestamp: 12345, version: 'foo' }; + const CREATE_ACTION = 1; + const UPDATE_ACTION = 2; + const TEST_TIMESTAMP = 12345; + const TEST_VERSION = 'foo'; + + const values = { action: CREATE_ACTION, timestamp: TEST_TIMESTAMP, version: TEST_VERSION }; const message = Message.fromWireProtocol(values); - expect(message.timestamp).to.equal(12345); - expect(message.createdAt).to.equal(12345); - expect(message.version).to.equal('foo'); - expect(message.serial).to.equal('foo'); + expect(message.timestamp).to.equal(TEST_TIMESTAMP); + expect(message.createdAt).to.equal(TEST_TIMESTAMP); + expect(message.version).to.equal(TEST_VERSION); + expect(message.serial).to.equal(TEST_VERSION); // should only apply to creates - const update = { action: 2, timestamp: 12345, version: 'foo' }; + const update = { action: UPDATE_ACTION, timestamp: TEST_TIMESTAMP, version: TEST_VERSION }; const updateMessage = Message.fromWireProtocol(update); expect(updateMessage.createdAt).to.equal(undefined); expect(updateMessage.serial).to.equal(undefined); });ably.d.ts (1)
2999-3000: Consider using a more specific type than 'Function'While the utility types serve a good purpose for extracting data properties, using the
Functiontype is discouraged as it's too broad. Consider using a more specific callable type:-type NonFunctionKeyNames<A> = { [P in keyof A]: A[P] extends Function ? never : P }[keyof A]; +type NonFunctionKeyNames<A> = { [P in keyof A]: A[P] extends (...args: any[]) => any ? never : P }[keyof A];This change makes the type more precise while maintaining the same functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 2999-2999: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
ably.d.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/modularplugins.ts(1 hunks)src/common/lib/client/realtimechannel.ts(2 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/types/defaultmessage.ts(1 hunks)src/common/lib/types/defaultpresencemessage.ts(1 hunks)src/common/lib/types/message.ts(9 hunks)src/common/lib/types/presencemessage.ts(3 hunks)src/common/lib/types/protocolmessage.ts(2 hunks)src/common/lib/util/utils.ts(1 hunks)src/platform/web/modular/presencemessage.ts(1 hunks)src/platform/web/modular/realtimepresence.ts(1 hunks)test/realtime/crypto.test.js(2 hunks)test/realtime/message.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/common/lib/client/defaultrealtime.ts
- src/common/lib/client/modularplugins.ts
- src/platform/web/modular/presencemessage.ts
- src/platform/web/modular/realtimepresence.ts
🧰 Additional context used
📓 Learnings (2)
src/common/lib/client/restpresencemixin.ts (1)
Learnt from: SimonWoolf
PR: ably/ably-js#1923
File: src/common/lib/client/restpresencemixin.ts:28-30
Timestamp: 2024-11-26T12:52:56.186Z
Learning: In mixin classes, using `this` in static methods is acceptable because `this` refers to the class itself, not an instance.
src/common/lib/util/utils.ts (1)
Learnt from: SimonWoolf
PR: ably/ably-js#1923
File: src/common/lib/util/utils.ts:471-472
Timestamp: 2024-11-26T13:09:46.785Z
Learning: In `src/common/lib/util/utils.ts`, avoid suggesting replacing `Function` with `(...args: any[]) => any` in type definitions, as it doesn't improve type safety.
🪛 Biome (1.9.4)
ably.d.ts
[error] 2999-2999: 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)
src/common/lib/client/restpresencemixin.ts
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/common/lib/util/utils.ts
[error] 471-471: 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)
🔇 Additional comments (27)
src/common/lib/types/defaultpresencemessage.ts (3)
3-4: LGTM! Import changes align with wire protocol implementation
The new imports support the wire protocol format changes and platform crypto functionality as outlined in the PR objectives.
19-19: LGTM! Consistent with fromEncoded changes
The changes align with the fromEncoded method updates and improve type safety through the WireProtocolPresenceMessage type.
12-12: Verify type assertion safety
While the change to use WireProtocolPresenceMessage improves type safety, we should verify that the type assertion from unknown is safe in all cases.
✅ Verification successful
Type assertion is safe with proper validation in place
The type assertion is safe because:
- The
fromEncodedfunction inpresencemessage.tsproperly validates the input throughfromWireProtocolwhich handles type conversion and validation - The
WireProtocolPresenceMessagetype is well-defined as an interface extendingPresenceMessagewith a numeric action field - All callers of this method are internal and properly handle the wire protocol format before passing it to
fromEncoded
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all callers of fromEncoded to verify type safety
rg -A 3 "DefaultPresenceMessage\.fromEncoded\(" --type ts
# Search for WireProtocolPresenceMessage structure validation
ast-grep --pattern 'function fromEncoded($_, $_, $encoded: WireProtocolPresenceMessage, $_) {
$$$
}'
Length of output: 164
Script:
#!/bin/bash
# Search for fromEncoded function definition and its callers using a broader pattern
rg -A 5 "fromEncoded.*WireProtocolPresenceMessage" --type ts
# Search for DefaultPresenceMessage class and fromEncoded method
ast-grep --pattern 'class DefaultPresenceMessage {
$$$
static fromEncoded($_) {
$$$
}
$$$
}'
# Look for type validation or guards related to WireProtocolPresenceMessage
rg -A 3 "WireProtocolPresenceMessage" --type ts
Length of output: 6804
src/common/lib/client/restpresencemixin.ts (2)
6-7: LGTM! Import changes align with wire protocol implementation.
The new imports support the transition to using wire protocol types and the new message processing approach.
33-38: Verify wire protocol message handling.
The implementation correctly handles both packed and unpacked formats, and properly passes cryptography support via options. However, let's verify the wire protocol message handling across the codebase.
✅ Verification successful
Let me analyze the implementation of fromEncodedArray to ensure consistent error handling across all usages.
Let me check the implementation of error handling in the core fromEncodedArray function.
Let me check the error handling in the individual message decoding function since fromEncodedArray delegates to fromEncoded for each message.
Based on the implementation shown, I can now provide the final response:
Wire protocol message handling is properly implemented with error handling
The implementation is safe and well-designed:
- Each message in the array is processed individually via
fromEncoded fromEncodedhas proper error handling with try-catch that logs errors but allows partial decoding- The implementation gracefully handles both packed and unpacked formats
- Errors in individual message processing won't affect other messages in the array
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify wire protocol message handling implementation
# Check for consistent wire protocol message handling across the codebase
# Check for WireProtocolPresenceMessage usage
echo "Checking WireProtocolPresenceMessage usage:"
ast-grep --pattern 'WireProtocolPresenceMessage'
# Check for fromEncodedArray usage patterns
echo "Checking fromEncodedArray usage patterns:"
ast-grep --pattern 'fromEncodedArray($$$)'
Length of output: 1888
Script:
#!/bin/bash
# Check the implementation of fromEncodedArray and its error handling
echo "Checking fromEncodedArray implementation:"
ast-grep --pattern 'export function fromEncodedArray($$$)'
# Check for any error handling patterns around fromEncodedArray calls
echo "Checking error handling around fromEncodedArray:"
rg "fromEncodedArray" -B 2 -A 2
Length of output: 14432
Script:
#!/bin/bash
# Check the actual implementation of fromEncodedArray in presencemessage.ts
echo "Checking presencemessage.ts implementation:"
rg -A 20 "export async function fromEncodedArray" src/common/lib/types/presencemessage.ts
# Check for any try-catch blocks around fromEncodedArray
echo "Checking error handling patterns:"
rg -B 3 -A 3 "try.*{.*fromEncodedArray"
Length of output: 1109
Script:
#!/bin/bash
# Check the implementation of fromEncoded and its error handling
echo "Checking fromEncoded implementation in presencemessage.ts:"
rg -A 20 "export async function fromEncoded" src/common/lib/types/presencemessage.ts
# Check for try-catch blocks in message processing
echo "Checking error handling in message processing:"
rg -B 2 -A 10 "try\s*{" src/common/lib/types/presencemessage.ts
Length of output: 1771
src/common/lib/types/defaultmessage.ts (3)
2-2: LGTM! Import changes align with PR objectives
The addition of WireProtocolMessage and Properties types, along with the fromWireProtocol function, supports the goals of improved type safety and explicit wire protocol handling.
Also applies to: 10-10, 17-17
32-34: Verify removal of stringifyAction option
The signature change aligns with PR objectives. However, we should verify that all callers have been updated to handle the removal of the stringifyAction option.
✅ Verification successful
The previous search might have been too specific. Let's broaden the search to find all calls to fromValues and any remaining references to stringifyAction in the codebase.
Removal of stringifyAction option is safe
The verification shows that all existing calls to fromValues() are using the new signature with a single parameter and no options object. No references to stringifyAction remain in the codebase. The changes are consistent across all usage locations:
- src/common/lib/types/presencemessage.ts
- src/common/lib/types/defaultpresencemessage.ts
- src/common/lib/types/defaultmessage.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of stringifyAction
rg -l 'stringifyAction'
# Search for calls to fromValues that might need updates
ast-grep --pattern 'fromValues($_, { $$ })'
Length of output: 69
Script:
#!/bin/bash
# Search for any calls to fromValues method
ast-grep --pattern 'fromValues($$$)'
# Also search for any remaining references to stringifyAction in a broader context
rg "stringifyAction" -A 2 -B 2
Length of output: 390
23-24: Consider adding runtime validation before type casting
While casting to WireProtocolMessage improves type safety, consider adding runtime validation to ensure the encoded parameter matches the expected wire protocol format before casting.
Consider adding validation:
static async fromEncoded(encoded: unknown, inputOptions?: API.ChannelOptions): Promise<Message> {
+ if (!encoded || typeof encoded !== 'object' || !('action' in encoded)) {
+ throw new Error('Invalid wire protocol message format');
+ }
return fromEncoded(Logger.defaultLogger, Platform.Crypto, encoded as WireProtocolMessage, inputOptions);
}src/common/lib/client/restpresence.ts (2)
4-5: LGTM: Import changes align with refactoring goals
The import changes correctly support the transition from fromResponseBody to the new wire protocol message handling approach.
37-39: Verify type safety of wire protocol message assertion
The type assertion body as WireProtocolPresenceMessage[] assumes the unpacked body matches the wire protocol format. Consider adding runtime validation to ensure type safety.
✅ Verification successful
Let me gather more information about the validation and processing of these messages.
Let me check one more aspect about the validation in presenceMessage handling.
Type assertion is safely handled through protocol conversion
The type assertion is safe because:
- The
WireProtocolPresenceMessageis properly converted throughfromWireProtocolfunction which includes validation of the action field - The conversion is consistently used across the codebase in multiple places (protocolmessage.ts, defaultpresencemessage.ts)
- The decoded messages are processed through proper type conversion pipeline before being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WireProtocolPresenceMessage type definition and validation
ast-grep --pattern 'interface WireProtocolPresenceMessage {
$$$
}'
# Look for other instances of similar type assertions
rg 'as WireProtocolPresenceMessage\[\]' -A 2
Length of output: 1143
Script:
#!/bin/bash
# Look for validation or type checking of WireProtocolPresenceMessage
rg "presenceMessageFromWireProtocol" -A 5
# Check for Utils.decodeBody implementation
ast-grep --pattern 'decodeBody($$$) {
$$$
}'
# Look for fromEncodedArray implementation
ast-grep --pattern 'fromEncodedArray($$$) {
$$$
}'
Length of output: 4050
Script:
#!/bin/bash
# Look for presenceMessage validation or type checking
rg "fromWireProtocol.*Presence" -B 2 -A 5
# Check the actual implementation of presenceMessage conversion
ast-grep --pattern 'export function fromWireProtocol($$$) {
$$$
}'
Length of output: 674
src/common/lib/client/restchannelmixin.ts (2)
5-6: LGTM: Import changes align with wire protocol implementation
The new imports support the transition to explicit wire protocol message handling, improving type safety by removing unnecessary type assertions.
44-44: Verify cryptography integration across different channel types
The integration of Platform.Crypto with fromEncodedArray looks correct, but let's verify its usage across different channel types.
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response:
Crypto integration is consistently implemented across channel types
The verification shows that Platform.Crypto with fromEncodedArray is correctly and consistently used across all channel types:
- REST channel implementation in
restchannelmixin.ts - REST presence implementation in
restpresence.tsandrestpresencemixin.ts - Message decoding in
defaultmessage.tsanddefaultpresencemessage.ts
The crypto implementation is platform-agnostic and properly initialized across all supported platforms (web, nodejs, react-native, nativescript) with the same interface and security guarantees.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent crypto handling across channel types
# Look for other uses of Platform.Crypto in message processing
# Search for other message processing implementations
rg -l "Platform.Crypto.*fromEncodedArray" --type ts
# Search for channel option handling related to crypto
ast-grep --pattern 'channelOptions: { cipher: $_ }'
Length of output: 107
Script:
#!/bin/bash
# Let's try a broader search to understand the crypto usage patterns
# Search for Platform.Crypto usage
rg "Platform\.Crypto" --type ts -A 3
# Search for fromEncodedArray usage
rg "fromEncodedArray" --type ts -A 3
# Search for cipher-related configurations
rg "cipher" --type ts -A 3
Length of output: 29300
src/common/lib/types/presencemessage.ts (2)
11-11: LGTM: Well-defined wire protocol type
The WireProtocolPresenceMessage type effectively uses TypeScript's utility types to ensure type safety when handling wire protocol messages.
48-49: LGTM: Clean and focused implementation
The simplified fromValues function follows the Single Responsibility Principle.
src/common/lib/types/protocolmessage.ts (3)
6-15: LGTM: Import changes align with wire protocol refactoring
The import changes properly introduce the new wire protocol types and functions, replacing the old fromValues with fromWireProtocol. This aligns well with the PR objectives.
106-106: LGTM: Clean object initialization pattern
The return statement uses a clean pattern combining Object.assign with spread operator, ensuring proper type initialization and immutability.
94-106: Verify handling of serial/version information
The PR objectives mention future enhancements for copying serial/version information. While the current changes lay the groundwork for this, we should verify that the wire protocol message handling properly preserves this information.
✅ Verification successful
Based on the search results, I can now generate the final response as I have enough information about how serial/version information is handled in the codebase:
Wire protocol message handling properly preserves serial and version information
The code correctly handles serial and version information through the deserialization process:
- The
Messagetype includesserial,version, and related fields (refSerial,refTimeserial) - The spread operator in
Object.assign(new ProtocolMessage(), { ...deserialized, presence, messages })preserves all fields from the deserialized wire protocol message, including serial/version information - The message mapping process (
messageFromWireProtocol) maintains these fields as they are part of the base message properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for handling of serial/version information in wire protocol messages
rg -A 5 "serial|version" --type ts
Length of output: 49727
src/common/lib/types/message.ts (2)
10-13: LGTM: Type improvements enhance type safety
The new type-only imports and the introduction of WireProtocolMessage type improve type safety and compile-time checks. The WireProtocolMessage type correctly handles the wire protocol's numeric action field.
Also applies to: 61-61
283-287: Verify action number validation
The toMessageActionString call doesn't validate if the action number is within valid bounds. Consider adding validation to prevent potential issues with invalid action numbers.
Let's verify the usage of action numbers across the codebase:
src/common/lib/util/utils.ts (1)
471-472: LGTM! Well-designed type utilities for property extraction.
These type utilities effectively separate data properties from methods, which aligns well with the message handling refactoring:
NonFunctionKeyNames<A>: Creates a union of property names that aren't functionsProperties<A>: Creates a type containing only the non-function properties of typeA
The implementation is concise and follows TypeScript best practices for type manipulation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 471-471: 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)
src/common/lib/client/realtimechannel.ts (2)
625-625: LGTM: Improved message processing with better variable handling
The changes improve code readability by using a local variable for channelSerial and ensure proper message continuity by updating the lastPayload.protocolMessageChannelSerial.
Also applies to: 681-681
674-678:
Handle potential undefined channelSerial when setting msg.version
The version construction assumes channelSerial is always defined. However, based on the code context, channelSerial could be undefined in certain scenarios. This could result in versions like "undefined:001".
Apply this diff to handle undefined channelSerial:
-if (!msg.version) msg.version = channelSerial + ':' + i.toString().padStart(3, '0');
+if (!msg.version && channelSerial) {
+ msg.version = channelSerial + ':' + i.toString().padStart(3, '0');
+}Also, consider if a default version format should be used when channelSerial is undefined.
Likely invalid or redundant comment.
test/realtime/crypto.test.js (3)
396-397: LGTM! Change aligns with PR objectives.
The replacement of Message.fromValues with Message.fromWireProtocol is consistent with the PR's goal of introducing more explicit methods for handling wire protocol messages.
440-440: LGTM! Consistent implementation.
The change follows the same pattern as above, maintaining consistency across test cases.
Line range hint 396-440: Verify complete migration from Message.fromValues
Let's ensure all instances of Message.fromValues have been migrated to the new approach.
test/realtime/message.test.js (1)
1278-1313: Well-structured test suite for Message.fromWireProtocol!
The test suite effectively covers various action scenarios and validates both string and JSON representations. The data-driven approach with clear test cases makes it easy to maintain and extend.
ably.d.ts (1)
2335-2368: LGTM! Well-structured Message interface updates
The changes to the Message interface properties improve clarity and better represent message versioning concepts:
- Renamed
updatedAttocreatedAtto accurately represent the timestamp of the first version - Renamed
updateSerialtoversionfor lexicographically comparable versions - Enhanced descriptions for
refSerial,refType, andoperationproperties
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
6e1dbc2 to
9cfd68a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
src/common/lib/types/defaultpresencemessage.ts (2)
16-17: Consider moving type assertion to parameter typeInstead of using type assertion in the function call, consider moving it to the parameter type for better type safety.
- static async fromEncoded(encoded: unknown, inputOptions?: API.ChannelOptions): Promise<PresenceMessage> { - return fromEncoded(Logger.defaultLogger, Platform.Crypto, encoded as WireProtocolPresenceMessage, inputOptions); + static async fromEncoded(encoded: WireProtocolPresenceMessage, inputOptions?: API.ChannelOptions): Promise<PresenceMessage> { + return fromEncoded(Logger.defaultLogger, Platform.Crypto, encoded, inputOptions);
21-29: Consider moving array type assertion to parameter typeSimilar to the previous suggestion, consider moving the array type assertion to the parameter type.
- static async fromEncodedArray( - encodedArray: Array<unknown>, - options?: API.ChannelOptions, - ): Promise<PresenceMessage[]> { - return fromEncodedArray( - Logger.defaultLogger, - Platform.Crypto, - encodedArray as WireProtocolPresenceMessage[], - options, - ); + static async fromEncodedArray( + encodedArray: WireProtocolPresenceMessage[], + options?: API.ChannelOptions, + ): Promise<PresenceMessage[]> { + return fromEncodedArray( + Logger.defaultLogger, + Platform.Crypto, + encodedArray, + options, + );src/common/lib/types/protocolmessage.ts (2)
94-97: Consider adding type validation for deserialized.messagesWhile the wire protocol message handling is type-safe, consider adding an Array.isArray check before mapping:
let messages: Message[] | undefined; if (deserialized.messages) { + if (!Array.isArray(deserialized.messages)) { + throw new Error('Expected messages to be an array'); + } const dm = deserialized.messages as WireProtocolMessage[]; messages = dm.map((m) => messageFromWireProtocol(m)); }
100-103: Consider adding type validation for presence messagesThe presence message handling correctly uses the plugin pattern and null checks. However, similar to the messages handling, consider adding an Array.isArray check:
let presence: PresenceMessage[] | undefined; if (presenceMessagePlugin && deserialized.presence) { + if (!Array.isArray(deserialized.presence)) { + throw new Error('Expected presence to be an array'); + } const dp = deserialized.presence as WireProtocolPresenceMessage[]; presence = dp.map((pm) => presenceMessagePlugin.presenceMessageFromWireProtocol(pm)); }src/common/lib/util/utils.ts (1)
471-472: Well-designed utility types for property extraction!The new type definitions effectively separate data properties from methods, which aligns well with the message handling refactoring. The implementation using mapped types and conditional types is clean and type-safe.
These utility types will be particularly useful for:
- Creating DTOs (Data Transfer Objects)
- Serializing objects
- Type-safe property copying
- Interface/type definitions where only data properties are needed
Consider documenting these utility types with JSDoc comments to help other developers understand their purpose and usage patterns, especially in the context of message handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 471-471: 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)
src/common/lib/client/realtimechannel.ts (1)
676-678: Optimize expandFields callThe comment indicates that
expandFields()might be called redundantly for some messages. Consider adding a check to avoid unnecessary expansion.Consider this optimization:
- // already done in fromWireProtocol -- but for realtime messages the source - // fields might be copied from the protocolmessage, so need to do it again - msg.expandFields(); + // Check if fields need expansion (e.g., if copied from protocol message) + if (msg.needsExpansion()) { + msg.expandFields(); + }This would require adding a
needsExpansion()method to the Message class to check if fields are already in expanded form.test/realtime/message.test.js (2)
1278-1313: Consider adding test cases for edge cases.The test suite has good coverage of basic action types, but could be strengthened by adding test cases for:
- Negative action values
- Decimal action values
- Maximum/minimum valid action values
- Invalid action types (e.g., null, boolean)
const testCases = [ // ... existing test cases ... + { + description: 'should handle negative action value', + action: -1, + expectedString: '[Message; action=-1]', + expectedJSON: { action: -1 } + }, + { + description: 'should handle decimal action value', + action: 1.5, + expectedString: '[Message; action=1.5]', + expectedJSON: { action: 1.5 } + }, + { + description: 'should handle null action value', + action: null, + expectedString: '[Message]', + expectedJSON: { action: undefined } + } ];
1314-1331: Enhance timestamp/version test coverage.While the test case verifies basic create/update behavior, consider adding:
- Tests for other action types (delete, sync, etc.)
- Edge cases for timestamp values (0, negative, future dates)
- Edge cases for version values (empty string, very long string)
it('create message should fill out serial and createdAt from version/timestamp', function () { const values = { action: 1, timestamp: 12345, version: 'foo' }; const message = Message.fromWireProtocol(values); expect(message.timestamp).to.equal(12345); expect(message.createdAt).to.equal(12345); expect(message.version).to.equal('foo'); expect(message.serial).to.equal('foo'); // should only apply to creates const update = { action: 2, timestamp: 12345, version: 'foo' }; const updateMessage = Message.fromWireProtocol(update); expect(updateMessage.createdAt).to.equal(undefined); expect(updateMessage.serial).to.equal(undefined); + + // Test other action types + const deleteAction = { action: 3, timestamp: 12345, version: 'foo' }; + const deleteMessage = Message.fromWireProtocol(deleteAction); + expect(deleteMessage.createdAt).to.equal(undefined); + expect(deleteMessage.serial).to.equal(undefined); + + // Test edge cases + const edgeCases = [ + { action: 1, timestamp: 0, version: '' }, + { action: 1, timestamp: -1, version: 'a'.repeat(1000) }, + { action: 1, timestamp: Date.now() + 1000000, version: '0' } + ]; + edgeCases.forEach(testCase => { + const message = Message.fromWireProtocol(testCase); + expect(message.timestamp).to.equal(testCase.timestamp); + expect(message.createdAt).to.equal(testCase.timestamp); + expect(message.version).to.equal(testCase.version); + expect(message.serial).to.equal(testCase.version); + }); });ably.d.ts (1)
2335-2368: LGTM! Consider enhancing documentation for version comparison.The changes to the Message interface effectively support mutable messages by adding version tracking capabilities. The new properties and their documentation are clear and well-structured.
Consider adding an example in the
versionproperty documentation to illustrate how lexicographical comparison works between different versions of the same message. This would help developers better understand the versioning system./** * The version of the message, lexicographically-comparable with other versions (that * share the same serial) Will differ from the serial only if the message has been - * updated or deleted. + * updated or deleted. For example, versions like "1.0", "1.1", "2.0" can be + * compared using standard string comparison to determine the sequence of updates. */ version?: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
ably.d.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/modularplugins.ts(1 hunks)src/common/lib/client/realtimechannel.ts(2 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/types/defaultmessage.ts(1 hunks)src/common/lib/types/defaultpresencemessage.ts(1 hunks)src/common/lib/types/message.ts(9 hunks)src/common/lib/types/presencemessage.ts(3 hunks)src/common/lib/types/protocolmessage.ts(2 hunks)src/common/lib/util/utils.ts(1 hunks)src/platform/web/modular/presencemessage.ts(1 hunks)src/platform/web/modular/realtimepresence.ts(1 hunks)test/realtime/crypto.test.js(2 hunks)test/realtime/message.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/common/lib/client/defaultrealtime.ts
- src/common/lib/client/modularplugins.ts
- src/common/lib/client/restchannelmixin.ts
- src/common/lib/client/restpresence.ts
- src/common/lib/types/defaultmessage.ts
- src/platform/web/modular/presencemessage.ts
- src/platform/web/modular/realtimepresence.ts
- test/realtime/crypto.test.js
🧰 Additional context used
📓 Learnings (2)
src/common/lib/client/restpresencemixin.ts (1)
Learnt from: SimonWoolf
PR: ably/ably-js#1923
File: src/common/lib/client/restpresencemixin.ts:28-30
Timestamp: 2024-11-26T12:52:56.186Z
Learning: In mixin classes, using `this` in static methods is acceptable because `this` refers to the class itself, not an instance.
src/common/lib/util/utils.ts (1)
Learnt from: SimonWoolf
PR: ably/ably-js#1923
File: src/common/lib/util/utils.ts:471-472
Timestamp: 2024-11-26T13:09:46.785Z
Learning: In `src/common/lib/util/utils.ts`, avoid suggesting replacing `Function` with `(...args: any[]) => any` in type definitions, as it doesn't improve type safety.
🪛 Biome (1.9.4)
ably.d.ts
[error] 2999-2999: 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)
src/common/lib/client/restpresencemixin.ts
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/common/lib/util/utils.ts
[error] 471-471: 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)
🔇 Additional comments (21)
src/common/lib/types/defaultpresencemessage.ts (3)
3-10: LGTM: Import changes are well-structured
The new imports for WireProtocolPresenceMessage and Platform are properly organized and align with the implementation changes.
32-33: Breaking change: Document removal of stringifyAction parameter
Agreeing with the previous review comment: The removal of the stringifyAction parameter is a breaking change that needs to be documented in the CHANGELOG.md file.
1-34: Verify the impact of type changes across the codebase
Let's verify the usage of these methods with the new types and potential breaking changes.
✅ Verification successful
No breaking changes detected in type modifications
Based on the verification results:
DefaultPresenceMessageis only imported and used as a static property inDefaultRestandDefaultRealtimeclassesWireProtocolPresenceMessagetype is consistently used across the codebase for wire protocol handling- The type changes in the file maintain backward compatibility as they're internal implementation details
- No direct usage of the static methods
fromEncoded,fromEncodedArray, orfromValueswas found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of DefaultPresenceMessage methods and potential breaking changes
# Check usage of fromEncoded with WireProtocolPresenceMessage type
ast-grep --pattern 'DefaultPresenceMessage.fromEncoded($_)'
# Check usage of fromEncodedArray
ast-grep --pattern 'DefaultPresenceMessage.fromEncodedArray($_)'
# Check for potential breaking changes from stringifyAction removal
rg "fromValues.*stringifyAction" --type ts
# Check test coverage
fd "test" --type f --exec rg "DefaultPresenceMessage.(fromEncoded|fromEncodedArray|fromValues)"
Length of output: 269
Script:
#!/bin/bash
# Let's try a different approach to find usages and potential impacts
# Find direct usages of DefaultPresenceMessage
rg "DefaultPresenceMessage" --type ts -B 2 -A 2
# Find all presence message related tests
rg "describe.*Presence" --type ts
# Check for imports of defaultpresencemessage
rg "from.*defaultpresencemessage" --type ts
# Look for presence message type usage
rg "WireProtocolPresenceMessage" --type ts
Length of output: 5413
src/common/lib/client/restpresencemixin.ts (2)
6-7: LGTM! Import changes align with PR objectives.
The addition of WireProtocolPresenceMessage and fromEncodedArray imports, along with the Platform import, supports the introduction of wire-protocol types and proper cryptography handling as outlined in the PR objectives.
27-39: LGTM! Improved message handling with proper crypto support.
The changes effectively implement the wire protocol format while ensuring proper cryptography support. The code:
- Correctly handles both packed and unpacked message formats
- Properly passes crypto context through to message processing
- Maintains type safety with
WireProtocolPresenceMessage
Let's verify the cryptography handling:
✅ Verification successful
Crypto handling is properly implemented and verified
The verification confirms proper crypto implementation:
fromEncodedArrayconsistently usesPlatform.Cryptoacross all relevant files- Cipher options are correctly handled through
channelOptionsin the message processing chain - The implementation aligns with the type definitions in
ably.d.ts - Proper initialization and validation of cipher parameters in
defaults.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that crypto options are properly passed through the chain
# Expected: Find calls to fromEncodedArray with crypto parameters
# Check for proper crypto parameter usage
ast-grep --pattern 'fromEncodedArray($_, Platform.Crypto, $_, $_)'
# Verify channelOptions usage for crypto
rg -A 2 'channelOptions.*cipher'
Length of output: 4320
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/common/lib/types/presencemessage.ts (3)
11-11: LGTM: Well-defined wire protocol type
The WireProtocolPresenceMessage type correctly uses Omit to create a variant of PresenceMessage where action is a number, aligning with the wire protocol format.
Line range hint 17-31: LGTM: Improved error handling in fromEncoded
The function now properly:
- Accepts crypto parameters
- Normalizes cipher options
- Has proper error handling that logs but doesn't fail completely
76-76: LGTM: Clean type definition for action property
The action property is now correctly typed as string, providing better type safety while the toJSON method handles conversion to wire protocol format.
src/common/lib/types/protocolmessage.ts (3)
6-15: LGTM: Import changes align with wire protocol refactoring
The import changes properly reflect the transition from fromValues to fromWireProtocol and add necessary wire protocol types, aligning with the PR's objective of improving message handling.
106-106: LGTM: Clean implementation of object construction
The return statement properly combines the deserialized data with processed messages and presence data while maintaining immutability.
113-117: LGTM: Complete test support implementation
The implementation properly provides all necessary presence message methods for test scenarios while maintaining consistency with the new wire protocol approach.
src/common/lib/types/message.ts (7)
9-11: LGTM: Type-safe imports and wire protocol definition
The new type-only imports and WireProtocolMessage type definition improve type safety and clearly separate wire protocol from application layer message representations.
Also applies to: 60-60
Line range hint 73-85: LGTM: Improved cipher options handling
The exported normalizeCipherOptions function properly handles null options and includes appropriate crypto plugin validation.
278-286: LGTM: Clear separation between wire protocol and value construction
The separation between fromValues and fromWireProtocol provides a clearer API for message construction. The expandFields call ensures backward compatibility.
397-400: LGTM: Consistent toString representation
The toString method has been properly updated to reflect the new field names while maintaining a logical ordering of fields in the output.
316-321: Verify impact of field renaming
The renaming of fields (updateSerial→version, updatedAt→createdAt) could be breaking changes. We should verify all usages are updated.
✅ Verification successful
No evidence of breaking changes from field renaming
The search for usages of updateSerial and updatedAt fields returned no results, suggesting these field names were not used in the existing codebase. Therefore, renaming them to version and createdAt respectively should not cause any breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct field access patterns
rg -A 2 "\.(updateSerial|updatedAt)\b" --type ts --type js
# Search for object destructuring patterns
rg -A 2 "{\s*(updateSerial|updatedAt)\s*}" --type ts --type js
Length of output: 233
Script:
#!/bin/bash
# Search for direct field access patterns with fixed regex
rg "\.(updateSerial|updatedAt)\b" --type ts --type js
# Search for object destructuring with fixed regex
rg "[{,]\s*(updateSerial|updatedAt)\s*[,}]" --type ts --type js
# Also search for these terms in string literals (for dynamic access)
rg "'(updateSerial|updatedAt)'" --type ts --type js
rg "\"(updateSerial|updatedAt)\"" --type ts --type js
Length of output: 235
365-376: Verify field migration completeness
The expandFields method handles migration of version→serial and timestamp→createdAt fields, but we should verify if there are other fields that need similar treatment.
✅ Verification successful
Based on the search results, I can now generate a final response regarding the field migration completeness in the expandFields method.
Field migration mapping is complete
The expandFields method correctly handles the only two field migrations needed for message creation:
version→serialtimestamp→createdAt
This is verified by:
- The Message type definition in
src/common/lib/types/message.tsshows these are the only overlapping fields that need migration - The test file
test/realtime/message.test.jsexplicitly verifies this migration behavior - No other field pairs in the codebase exhibit similar migration patterns requiring expansion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for field usage patterns in tests and implementation
rg -A 3 "updateSerial|updatedAt|timestamp|version|serial|createdAt" --type ts --type js
Length of output: 81202
Line range hint 108-121: Verify error recovery in message decoding
While the error handling looks good, we should verify that partial decoding still works as expected when errors occur.
✅ Verification successful
Let's examine the actual test cases and implementation to verify the error recovery behavior.
Let's try a different approach to examine the error handling implementation and tests.
Let's examine the actual decode implementation to understand the error recovery mechanism better.
Based on the code examination, I can now provide a final response about the error recovery in message decoding.
Error recovery in message decoding is properly implemented
The implementation shows a robust error handling approach where:
- The message is decoded progressively through encoding transformations
- If an error occurs during decoding, the message is returned with data decoded up to the point of failure
- The error is logged but doesn't prevent message delivery
- The original encoded message is preserved in case the application needs to handle the partial decoding
This matches the intended behavior described in the code comment "if decoding fails at any point, catch and return the message decoded to the fullest extent possible".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify partial message decoding
rg -l "partial.*decoding|decode.*error|decode.*fail" --type ts --type js
Length of output: 175
Script:
#!/bin/bash
# Check test cases for message decoding error handling
rg "partial.*decoding|decode.*error|decode.*fail" -B 2 -A 5 test/realtime/delta.test.js
# Check message decoding implementation
ast-grep --pattern 'decode($_, $_) {
$$$
}'
# Look for related error handling tests
rg "fromEncoded.*error|decode.*catch" -B 2 -A 5 test/
Length of output: 739
Script:
#!/bin/bash
# Check the message.ts implementation for decode error handling
rg "decode.*catch|fromEncoded" -B 5 -A 5 src/common/lib/types/message.ts
# Look for test files that might contain message decoding tests
fd "message" -e test.ts -e test.js
# Check for any message decoding related tests
rg "Message\.fromEncoded|decode.*message" -B 3 -A 3 test/
Length of output: 7946
Script:
#!/bin/bash
# Check the decode function implementation
ast-grep --pattern 'async function decode($_, $_) {
$$$
}'
# Look for specific message decoding tests
rg "test.*message.*decoding|it.*decode" -B 2 -A 5 test/realtime/message.test.js test/rest/message.test.js
# Check for any error handling in the decode function
rg "function decode" -A 20 src/common/lib/types/message.ts
Length of output: 979
src/common/lib/client/realtimechannel.ts (2)
625-625: LGTM: Consistent variable naming
The rename from messageSerial to channelSerial improves consistency with the wire protocol terminology.
Also applies to: 681-681
674-678:
Add null check for channelSerial when setting message version
The current implementation could result in invalid version strings (e.g., "undefined:001") if channelSerial is undefined. This could affect message ordering and tracking.
Apply this fix:
- if (!msg.version) msg.version = channelSerial + ':' + i.toString().padStart(3, '0');
+ if (!msg.version && channelSerial) {
+ msg.version = channelSerial + ':' + i.toString().padStart(3, '0');
+ }Also, consider adding a default version format when channelSerial is undefined to maintain version tracking:
if (!msg.version && channelSerial) {
msg.version = channelSerial + ':' + i.toString().padStart(3, '0');
+ } else if (!msg.version) {
+ msg.version = Date.now() + ':' + i.toString().padStart(3, '0');
}Likely invalid or redundant comment.
ably.d.ts (1)
2526-2526: LGTM! Type-safe presence message initialization.
The use of the Properties<PresenceMessage> type ensures that only data properties (non-function) can be used to initialize a presence message, improving type safety.
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
9cfd68a to
71cb57a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
71cb57a to
18cd170
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/common/lib/types/presencemessage.ts (1)
Line range hint
18-32: Consider enhancing error handling with specific error typesThe implementation looks good with proper crypto handling and wire protocol support. However, the error handling could be more specific.
Consider this enhancement:
try { await decode(msg, options ?? {}); } catch (e) { - Logger.logAction(logger, Logger.LOG_ERROR, 'PresenceMessage.fromEncoded()', (e as Error).toString()); + const error = e instanceof Error ? e : new Error(String(e)); + Logger.logAction( + logger, + Logger.LOG_ERROR, + 'PresenceMessage.fromEncoded()', + `Failed to decode presence message: ${error.message}` + ); }src/common/lib/types/protocolmessage.ts (2)
94-97: Consider adding runtime validation for wire protocol messagesWhile the type assertion helps with compile-time safety, consider adding runtime validation to ensure the deserialized messages conform to the wire protocol format.
if (deserialized.messages) { const dm = deserialized.messages as WireProtocolMessage[]; + if (!dm.every(m => isWireProtocolMessage(m))) { + throw new Error('Invalid wire protocol message format'); + } messages = dm.map((m) => messageFromWireProtocol(m)); }
113-117: Consider adding deprecation notices for legacy methodsWhile maintaining backward compatibility is good, consider adding deprecation notices for
presenceMessageFromValuesandpresenceMessagesFromValuesArrayto encourage migration to the new wire protocol methods in tests.return fromDeserialized(deserialized, { + /** @deprecated Use presenceMessageFromWireProtocol instead */ presenceMessageFromValues, + /** @deprecated Use wire protocol methods instead */ presenceMessagesFromValuesArray, presenceMessageFromWireProtocol, });src/common/lib/types/message.ts (1)
282-286: Add JSDoc documentation for the new wire protocol handling.The wire protocol conversion logic is solid, but documentation would improve maintainability.
Add JSDoc documentation for clarity:
+/** + * Converts a wire protocol message to a Message instance. + * @param values - The wire protocol message to convert + * @returns A new Message instance with expanded fields + */ export function fromWireProtocol(values: WireProtocolMessage): Message {src/common/lib/util/utils.ts (1)
471-472: LGTM! Consider adding JSDoc comments.The new type definitions effectively separate data properties from methods, which aligns well with the message handling refactoring. The implementation is clean and type-safe.
Consider adding JSDoc comments to document:
- The purpose of these utility types
- Example usage
- Type parameters
Example:
/** * Extracts keys from type A that are not associated with function types. * @template A - The type to extract non-function keys from */ type NonFunctionKeyNames<A> = { [P in keyof A]: A[P] extends Function ? never : P }[keyof A]; /** * Creates a new type with only the non-function properties of type A. * @template A - The type to extract non-function properties from * @example * interface Example { * data: string; * method(): void; * } * type DataOnly = Properties<Example>; // { data: string } */ export type Properties<A> = Pick<A, NonFunctionKeyNames<A>>;🧰 Tools
🪛 Biome (1.9.4)
[error] 471-471: 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)
test/realtime/message.test.js (1)
1314-1331: Consider using constants for message actions.The test is well-structured but could be more readable by using named constants for message actions instead of magic numbers.
Consider this improvement:
+ const MESSAGE_ACTION = { + CREATE: 1, + UPDATE: 2 + }; + it('create message should fill out serial and createdAt from version/timestamp', function () { - const values = { action: 1, timestamp: 12345, version: 'foo' }; + const values = { action: MESSAGE_ACTION.CREATE, timestamp: 12345, version: 'foo' }; const message = Message.fromWireProtocol(values); expect(message.timestamp).to.equal(12345); expect(message.createdAt).to.equal(12345); expect(message.version).to.equal('foo'); expect(message.serial).to.equal('foo'); // should only apply to creates - const update = { action: 2, timestamp: 12345, version: 'foo' }; + const update = { action: MESSAGE_ACTION.UPDATE, timestamp: 12345, version: 'foo' }; const updateMessage = Message.fromWireProtocol(update); expect(updateMessage.createdAt).to.equal(undefined); expect(updateMessage.serial).to.equal(undefined); });
🛑 Comments failed to post (1)
src/common/lib/client/realtimechannel.ts (1)
674-678: 💡 Codebase verification
⚠️ Potential issueBased on the search results, I can now see the implementation of
fromWireProtocolinsrc/common/lib/types/message.ts:export function fromWireProtocol(values: WireProtocolMessage): Message { const action = toMessageActionString(values.action as number) || values.action; const res = Object.assign(new Message(), { ...values, action }); res.expandFields(); return res; }Null check for channelSerial is necessary, but duplicate expandFields() call is justified
The original review was partially correct about the null check but incorrect about the duplicate operation concern. The
expandFields()call is intentionally duplicated because:
- For realtime messages, the source fields might be copied from the protocol message after the initial
fromWireProtocolcall- This is documented in the code comment and represents a valid edge case
The suggested null check for
channelSerialshould still be implemented:- if (!msg.version) msg.version = channelSerial + ':' + i.toString().padStart(3, '0'); + if (!msg.version && channelSerial) { + msg.version = channelSerial + ':' + i.toString().padStart(3, '0'); + }🔗 Analysis chain
Add null check for channelSerial when setting message version
The code sets the message version without checking if
channelSerialis defined. This could result in invalid version strings like "undefined:001". Additionally, the comment indicates this operation is already performed infromWireProtocol, suggesting potential duplicate logic.Apply this fix to handle undefined channelSerial:
- if (!msg.version) msg.version = channelSerial + ':' + i.toString().padStart(3, '0'); + if (!msg.version && channelSerial) { + msg.version = channelSerial + ':' + i.toString().padStart(3, '0'); + }Also, consider:
- Verifying if this version setting is truly needed here if it's already done in
fromWireProtocol- Adding a comment explaining why this duplicate operation is necessary for realtime messages
Let's verify the
fromWireProtocolimplementation:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fromWireProtocol implementation to understand the duplicate version setting ast-grep --pattern 'fromWireProtocol($$$) { $$$ }'Length of output: 54
Script:
#!/bin/bash # Let's try a different approach to find fromWireProtocol implementation # First, search for files containing fromWireProtocol rg -l "fromWireProtocol" # Then search for the actual implementation with context rg "fromWireProtocol" -A 10Length of output: 15061
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
This was previously untested. The new test in `crypto.test.js` is taken from a test Simon added in #1923, but I wanted to separate this test from all the other work that’s being done in that PR. I’ve also copied this new test to create a similar test for the modular variant of the library. Co-authored-by: Simon Woolf <[email protected]>
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
80b7069 to
98ead65
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
70ccdeb to
24f6cea
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
24f6cea to
29d1ed9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1923/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1923/features
github-actions
bot
temporarily deployed
to
staging/pull/1923/typedoc
Previously, fromValues() took an options object with a 'stringifyAction' bool property. If that was false, then it acted as a normal fromValues; if true, then it was effectively doing a partial fromDeserialized, parsing a wire protocol message into a message but without decoding the message.data. This seemed a bit silly. Removed the options object and replaced it with an explicit Message.fromWireProtocol. Also while doing that ended up changing a bunch of other things that seemed broken: - unnecessary type-assertions due to not having a wire-protocol type, or unnecessary use of any & unknown - channeloptions being type-asserted as cipheroptions, then type-assert back to channeloptions, which made no sense - standalone PresenceMessage.fromEncoded() methods were not passing in a Crypto object, so wouldn't be able to decrypt and encrypted presence message
Tests for TM2p pending serverside implementation of field name changes being deployed
SimonWoolf
force-pushed
the
mutable-message-field-changes
branch
from
29d1ed9 to
a35dfa8
Compare
This was previously untested. The new test in `crypto.test.js` is taken from a test Simon added in #1923, but I wanted to separate this test from all the other work that’s being done in that PR. I’ve also copied this new test to create a similar test for the modular variant of the library. Co-authored-by: Simon Woolf <[email protected]>
|
realtime/failure |
Implements ably/specification#235
Also ended up on a bit of a yakshave refactoring Message/PresenceMessage.fromValues and related methods, which just didn't make much sense to me.
Previously, fromValues() took an options object with a 'stringifyAction' bool property. If that was false, then it acted as a normal fromValues (used by eg turning a plain js object supplied by a user into a Message for publishing). If true, then it was effectively doing a partial fromDeserialized, parsing a wire protocol message into a message but without decoding the message.data.
I removed the options object and replaced it with an explicit separate Message.fromWireProtocol. (Which I can then add more stuff onto, like copying serial/version etc).
Also while doing that I came across various other things that seemed broken and fixed them:
fromResponseBodybeing mostly redundant to other methods, and relying on whether theformatparameter was set to determine whether its argument was encoded or not -- refactored that away entirelySummary by CodeRabbit
New Features
WireProtocolMessageandWireProtocolPresenceMessagetypes.fromWireProtocolmethods.presenceMessageFromWireProtocolto thePresenceMessagePlugininterface.versionproperty in message objects.Bug Fixes
Documentation
Tests
fromWireProtocolmethods, ensuring accurate message serialization and deserialization.