Merge main into integration/liveobjects 2024-11-29
#1930
Conversation
…nel attaching This fixes a race condition issue, where a React app might trigger a component unmount while channel is still in the `attaching` state, thus `presence.leave` was never called for a channel and member would remain there until app reload.
…ame-race-condition [ECO-5002] Fix usePresence won't leave presence if unmount triggered during channel attaching
Release 2.4.1
…olutions Resolves ECO-4841
- Added new message attributes, including `action`, `serial`, `refSerial`, `refType`, `updatedAt`, `deletedAt`, and `operation`. Additionally, create functions to map message actions between string and number representations. This update also changes the `fromValues` function to handle action transformations.
…e-fields materialisation: Added new message fields.
Added an updateSerial to allow LWW semantics to be applied to a message resulting from an operation. Removed the deletedAt timestamp, we will rely only on updateSerial to apply ordering of operations. UpdateAt will still be used, but just so clients can render a simple datetime and not have to understand/parse the serial.
…issue-solutions [ECO-4841] Add README section to explain "Connection limit exceeded" error and solutions
Update `Message` to support `updateSerial`
Updated CHANGELOG and package files to reflect the new version 2.5.0. Added support for new `Message` attributes essential for upcoming features (updates/deletes/annotations) in the Ably service.
Release version 2.5.0
should not have its id set
Co-authored-by: Owen Pearson <[email protected]>
Support RTP17g1: presence auto re-enter with a different connId
The only git submodule we need is ably-common, and this is only used as part of testing to create test apps. We don't use it when building (despite it being a build check). This check during build prevents other SDKs from depending on a branch of ably-js, as it doesn't have the submodules, and can't init them. This change therefore removes the submodules check when building the library, allowing SDKs to depend on branch versions, if they want to.
build: dont check submodules to build module
…parate function Also fixes the issue with processing Presence messages, where a decoding error would prevent us from setting `connectionId`, `timestamp` and `id` fields for a message. Now this behavior is aligned with how it worked with regular messages. Resolves #1907
PresenceMap: only emit a leave if a member was present
[ECO-5054] Extract common code for message processing in RealtimeChannel to a separate function
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces several changes across multiple files, primarily enhancing the configuration for Mocha tests, updating the changelog for version 2.5.0, and modifying type definitions for message handling in the Ably library. Key updates include the addition of a new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (22)
test/unit/presencemap.test.js (2)
7-7: Enhance MockRealtimePresence implementationThe empty mock class might not provide sufficient test coverage. Consider implementing the necessary methods and properties that
RealtimePresencewould typically have to ensure thorough testing.-class MockRealtimePresence {} +class MockRealtimePresence { + constructor() { + this.channel = null; + this.syncComplete = false; + } + // Add methods that RealtimePresence would typically have + onAttached() {} + onUpdate() {} +}
13-18: Add input validation to createPresenceMessage helperThe helper function should validate its inputs to catch potential test setup issues early.
const createPresenceMessage = (clientId, connectionId, action, timestamp) => ({ + if (typeof clientId !== 'string' || typeof connectionId !== 'string') { + throw new Error('clientId and connectionId must be strings'); + } + if (typeof timestamp !== 'number') { + throw new Error('timestamp must be a number'); + } + if (!['enter', 'leave', 'present', 'update'].includes(action)) { + throw new Error('invalid action'); + } clientId, connectionId, timestamp, action, });src/common/lib/client/defaultrealtime.ts (1)
73-73: LGTM! Consider adding JSDoc for the static propertyThe static property follows the established pattern for test utilities. However, since it's used for testing, it would be helpful to add documentation.
// Used by tests static _Http = Http; + /** Used by tests for presence management functionality */ static _PresenceMap = PresenceMap;test/support/runPlaywrightTests.js (1)
79-83: Consider adding regex validation and improving documentation.While the implementation is functional, consider these improvements:
- Add error handling for invalid regex patterns
- Enhance the comment to explain the grep pattern format and usage
Consider this implementation:
// Set grep pattern in the browser context -// allows easy filtering of tests. +// Allows filtering tests using regex patterns. +// Example: MOCHA_GREP="should handle reconnection" npm test if (grep) { + try { window.mocha.grep(new RegExp(grep)); + } catch (e) { + console.error(`Invalid grep pattern: ${grep}. Error: ${e.message}`); + } }src/platform/react-hooks/src/hooks/usePresence.ts (1)
76-80: Improved presence leave logic with clearer state handling.The separation of connection and channel state checks makes the code more maintainable and explicit. Including the 'attaching' state aligns with the fix mentioned in PR #1884.
Consider adding a debug log to track state transitions:
const canLeaveFromConnectionState = !INACTIVE_CONNECTION_STATES.includes(ably.connection.state); const canLeaveFromChannelState = ['attached', 'attaching'].includes(channel.state); + if (process.env.NODE_ENV !== 'production') { + console.debug('[usePresence] Cleanup - Connection:', ably.connection.state, 'Channel:', channel.state); + } if (canLeaveFromChannelState && canLeaveFromConnectionState) { channel.presence.leave(); }src/common/lib/types/message.ts (3)
12-34: LGTM! Consider adding JSDoc documentation.The implementation of message action handling is well-structured with bidirectional mapping. However, adding JSDoc documentation would improve maintainability.
Add documentation for the conversion functions:
+/** + * Converts a message action number to its string representation + * @param actionNumber - The numeric representation of the message action + * @returns The string representation of the message action or undefined if not found + */ function toMessageActionString(actionNumber: number): API.MessageAction | undefined { return ReverseMessageActionMap.get(actionNumber); } +/** + * Converts a message action string to its numeric representation + * @param messageAction - The string representation of the message action + * @returns The numeric representation of the message action or undefined if not found + */ function toMessageActionNumber(messageAction?: API.MessageAction): number | undefined { return messageAction ? MessageActionMap.get(messageAction) : undefined; }
Line range hint
320-324: Improve code clarity by avoiding assignment in expression.The current implementation combines assignment with array element access, which can be confusing.
Refactor to improve readability:
- const msg = (body[i] = fromValues(body[i], { stringifyAction: true })); + const msg = fromValues(body[i], { stringifyAction: true }); + body[i] = msg;🧰 Tools
🪛 Biome (1.9.4)
[error] 320-321: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
372-378: LGTM! Document new properties for better maintainability.The new properties effectively support STATE message handling.
Add property documentation:
class Message { + /** The action type of the message */ action?: API.MessageAction | number; + /** The serial number of the message */ serial?: string; + /** The reference serial number for linked messages */ refSerial?: string; + /** The type of reference */ refType?: string; + /** Timestamp of the last update */ updatedAt?: number; + /** Serial number of the update */ updateSerial?: string; + /** Operation details */ operation?: API.Operation;docs/react.md (2)
39-39: Enhance connection management guidance with an anti-pattern exampleWhile the guidance about creating the Ably.Realtime instance outside components is valuable, consider adding a "what not to do" example to make it more explicit:
Also, ensure that the `Ably.Realtime` instance is created outside of components to prevent it from being recreated on component re-renders. This will help avoid opening extra unnecessary connections to the Ably servers and potentially reaching the maximum connections limit on your account. + +```jsx +// ❌ Don't do this - creates a new connection on every render +function BadComponent() { + const client = new Ably.Realtime({ key: 'your-ably-api-key' }); + return <div>...</div>; +} + +// ✅ Do this - creates connection once +const client = new Ably.Realtime({ key: 'your-ably-api-key' }); +function GoodComponent() { + return <div>...</div>; +} +```
Line range hint
510-543: Add NextJS version compatibility informationThe solutions for NextJS warnings are helpful, but consider adding version information to make it clearer which versions of NextJS these solutions apply to.
## NextJS warnings + +> These solutions have been tested with NextJS versions X.Y.Z and above. Your mileage may vary with different versions.README.md (4)
683-684: Consider adding hyphens for compound adjectivesFor better readability and grammatical correctness, consider hyphenating compound adjectives:
- "server-side rendering" instead of "server side rendering"
- "server-side" instead of "server side" when used as an adjective
-If you're using Next.js, your `Ably.Realtime` client instance may be created multiple times on the server side (i.e., in a Node.js process) as you're developing your app, due to Next.js server side rendering your components. +If you're using Next.js, your `Ably.Realtime` client instance may be created multiple times on the server-side (i.e., in a Node.js process) as you're developing your app, due to Next.js server-side rendering your components.🧰 Tools
🪛 LanguageTool
[uncategorized] ~683-~683: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...'re developing your app, due to Next.js server side rendering your components. Note that ev...(EN_COMPOUND_ADJECTIVE_INTERNAL)
687-692: Code block should be indented instead of fencedFor consistency with the rest of the document, the code block should use indentation rather than fence markers.
- ```typescript - const client = new Ably.Realtime({ key: 'your-ably-api-key', autoConnect: typeof window !== 'undefined' }); - ``` + const client = new Ably.Realtime({ key: 'your-ably-api-key', autoConnect: typeof window !== 'undefined' });🧰 Tools
🪛 LanguageTool
[uncategorized] ~691-~691: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e server side, while not affecting your client side components. - If you're using any Reac...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Markdownlint (0.35.0)
687-687: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
693-694: Simplify wording for better readabilityThe phrase "outside of" is redundant. Consider simplifying it to just "outside".
-move the client instantiation (`new Ably.Realtime`) outside of your components +move the client instantiation (`new Ably.Realtime`) outside your components🧰 Tools
🪛 LanguageTool
[style] ~693-~693: This phrase is redundant. Consider using “outside”.
Context: ...ent instantiation (new Ably.Realtime) outside of your components. You can find an exampl...(OUTSIDE_OF)
697-698: Strengthen wording and improve clarityConsider strengthening the wording and removing unnecessary hyphens in the file name reference since it's not being used as a compound modifier.
-The solution is simple: move the `new Ably.Realtime()` call to a separate file, such as `ably-client.js`, and export the client instance from there. This way, the client instance will only be recreated when you specifically make changes to the `ably-client.js` file +The solution is simple: move the `new Ably.Realtime()` call to a separate file, such as `ablyClient.js`, and export the client instance from there. This way, the client instance will be recreated only when you modify the `ablyClient.js` file🧰 Tools
🪛 LanguageTool
[uncategorized] ~697-~697: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ime()call to a separate file, such asably-client.js`, and export the client instance fro...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~697-~697: Consider shortening or rephrasing this to strengthen your wording.
Context: ...only be recreated when you specifically make changes to theably-client.jsfile, which should...(MAKE_CHANGES)
[uncategorized] ~697-~697: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...en you specifically make changes to theably-client.jsfile, which should be far less freq...(HYPHENATED_LY_ADVERB_ADJECTIVE)
test/realtime/message.test.js (1)
1275-1324: LGTM! Well-structured test suite with comprehensive coverage.The test suite is well-organized using a data-driven approach and covers key scenarios for message action stringification. The test cases are clear and assertions verify both string representation and JSON output.
Consider grouping related test cases (e.g., stringification behavior, edge cases) using nested
describeblocks for better organization:describe('DefaultMessage.fromValues stringify action', function () { + describe('stringification behavior', function() { // stringification-related tests + }); + describe('edge cases', function() { // edge case tests like undefined/unknown actions + }); });src/common/lib/client/presencemap.ts (2)
65-79: Refactorlistmethod to use optional chaining and optional parametersIn the
listmethod,paramsmay be undefined, as indicated by the checksparams && params.clientIdandparams &¶ms.connectionId. To simplify the code and improve readability, you can makeparams` optional in the method signature and use optional chaining.Apply this diff to update the code:
- list(params: RealtimePresenceParams) { + list(params?: RealtimePresenceParams) { const map = this.map, - clientId = params && params.clientId, - connectionId = params && params.connectionId, + clientId = params?.clientId, + connectionId = params?.connectionId, result = []; for (const key in map) { const item = map[key]; if (item.action === 'absent') continue; if (clientId && clientId != item.clientId) continue; if (connectionId && connectionId != item.connectionId) continue; result.push(item); } return result; }🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-63: SimplifygetClientmethod usingObject.valuesand array filteringYou can simplify the
getClientmethod by utilizingObject.valuesand array filtering to make the code more concise and readable.Apply this diff to refactor the method:
- getClient(clientId: string) { - const map = this.map, - result = []; - for (const key in map) { - const item = map[key]; - if (item.clientId == clientId && item.action != 'absent') result.push(item); - } - return result; - } + getClient(clientId: string) { + return Object.values(this.map).filter( + (item) => item.clientId === clientId && item.action !== 'absent' + ); + }src/common/lib/client/realtimechannel.ts (2)
693-693: Remove unused variablefirstMessageThe variable
firstMessageis assigned but not used in the subsequent code. Removing it will clean up the code and reduce unnecessary variables.Proposed change:
const messages = message.messages as Array<Message>, - firstMessage = messages[0], lastMessage = messages[messages.length - 1];
711-736: Simplify error handling indecodeErrorRecoveryHandlerConsider merging cases with similar logic in the
decodeErrorRecoveryHandlerto reduce redundancy and improve readability.Proposed change:
const errorInfo = e as ErrorInfo; switch (errorInfo.code) { case 40018: /* decode failure */ this._startDecodeFailureRecovery(errorInfo); return { unrecoverableError: true }; - case 40019: - /* No vcdiff plugin passed in - no point recovering, give up */ - // eslint-disable-next-line no-fallthrough - case 40021: - /* Browser does not support deltas, similarly no point recovering */ - this.notifyState('failed', errorInfo); - return { unrecoverableError: true }; - - default: - return { unrecoverableError: false }; + case 40019: + case 40021: + /* Unrecoverable errors */ + this.notifyState('failed', errorInfo); + return { unrecoverableError: true }; + + default: + return { unrecoverableError: false }; }ably.d.ts (3)
2361-2389: Clarify the distinction between 'id' and 'serial' properties in theMessageinterfaceThe newly added
serialproperty may cause confusion with the existingidproperty. Please consider updating the documentation to clearly distinguish between them and explain their specific purposes and usage to prevent misunderstandings.
2391-2407: Allow 'metadata' in theOperationinterface to accept values of any typeThe
metadatafield is currently typed asRecord<string, string>. If there is a possibility that metadata values might include types other than strings, consider changing the type toRecord<string, any>for greater flexibility.Apply this diff to update the type:
export interface Operation { /** * The client ID of the client that initiated the operation. */ clientId?: string; /** * The description provided by the client that initiated the operation. */ description?: string; /** * A JSON object of string key-value pairs that may contain metadata associated with the operation. */ - metadata?: Record<string, string>; + metadata?: Record<string, any>; }
2409-2441: Consider using a TypeScriptenumforMessageActionsTo enhance type safety and maintainability, consider using a TypeScript
enumforMessageActionsinstead of a namespace with type aliases. This approach simplifies theMessageActiontype definition and leverages the benefits of enums.Apply this diff to refactor
MessageActions:-/** - * The namespace containing the different types of message actions. - */ -declare namespace MessageActions { - /** - * Message action has not been set. - */ - type MESSAGE_UNSET = 'message.unset'; - /** - * Message action for a newly created message. - */ - type MESSAGE_CREATE = 'message.create'; - /** - * Message action for an updated message. - */ - type MESSAGE_UPDATE = 'message.update'; - /** - * Message action for a deleted message. - */ - type MESSAGE_DELETE = 'message.delete'; - /** - * Message action for a newly created annotation. - */ - type ANNOTATION_CREATE = 'annotation.create'; - /** - * Message action for a deleted annotation. - */ - type ANNOTATION_DELETE = 'annotation.delete'; - /** - * Message action for a meta-message that contains channel occupancy information. - */ - type META_OCCUPANCY = 'meta.occupancy'; -} - -/** - * Describes the possible action types used on a {@link Message}. - */ -export type MessageAction = - | MessageActions.MESSAGE_UNSET - | MessageActions.MESSAGE_CREATE - | MessageActions.MESSAGE_UPDATE - | MessageActions.MESSAGE_DELETE - | MessageActions.ANNOTATION_CREATE - | MessageActions.ANNOTATION_DELETE - | MessageActions.META_OCCUPANCY; +/** + * Enum describing the possible action types used on a {@link Message}. + */ +export enum MessageAction { + /** + * Message action has not been set. + */ + MESSAGE_UNSET = 'message.unset', + /** + * Message action for a newly created message. + */ + MESSAGE_CREATE = 'message.create', + /** + * Message action for an updated message. + */ + MESSAGE_UPDATE = 'message.update', + /** + * Message action for a deleted message. + */ + MESSAGE_DELETE = 'message.delete', + /** + * Message action for a newly created annotation. + */ + ANNOTATION_CREATE = 'annotation.create', + /** + * Message action for a deleted annotation. + */ + ANNOTATION_DELETE = 'annotation.delete', + /** + * Message action for a meta-message that contains channel occupancy information. + */ + META_OCCUPANCY = 'meta.occupancy', +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.mocharc.js(1 hunks)CHANGELOG.md(1 hunks)Gruntfile.js(1 hunks)README.md(1 hunks)ably.d.ts(1 hunks)docs/react.md(1 hunks)package.json(2 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/presencemap.ts(1 hunks)src/common/lib/client/realtimechannel.ts(4 hunks)src/common/lib/client/realtimepresence.ts(3 hunks)src/common/lib/client/restchannel.ts(1 hunks)src/common/lib/types/defaultmessage.ts(2 hunks)src/common/lib/types/message.ts(7 hunks)src/common/lib/types/protocolmessage.ts(1 hunks)src/platform/react-hooks/src/AblyReactHooks.ts(1 hunks)src/platform/react-hooks/src/hooks/usePresence.ts(1 hunks)test/common/modules/shared_helper.js(2 hunks)test/realtime/crypto.test.js(2 hunks)test/realtime/message.test.js(2 hunks)test/realtime/presence.test.js(3 hunks)test/support/runPlaywrightTests.js(1 hunks)test/unit/presencemap.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/common/lib/client/restchannel.ts
- src/platform/react-hooks/src/AblyReactHooks.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~683-~683: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...'re developing your app, due to Next.js server side rendering your components. Note that ev...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~691-~691: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e server side, while not affecting your client side components. - If you're using any Reac...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~693-~693: This phrase is redundant. Consider using “outside”.
Context: ...ent instantiation (new Ably.Realtime) outside of your components. You can find an exampl...
(OUTSIDE_OF)
[uncategorized] ~697-~697: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ime()call to a separate file, such asably-client.js`, and export the client instance fro...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~697-~697: Consider shortening or rephrasing this to strengthen your wording.
Context: ...only be recreated when you specifically make changes to the ably-client.js file, which should...
(MAKE_CHANGES)
[uncategorized] ~697-~697: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...en you specifically make changes to the ably-client.js file, which should be far less freq...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Markdownlint (0.35.0)
README.md
687-687: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🪛 Biome (1.9.4)
src/common/lib/client/presencemap.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/common/lib/types/message.ts
[error] 320-321: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
test/unit/presencemap.test.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (29)
.mocharc.js (1)
12-12: LGTM! The test pattern addition is well-structured.
The addition of 'test/unit/*.test.js' maintains consistency with existing patterns and is protected by the same command-line override logic.
Let's verify the existence of unit tests that this pattern will match:
✅ Verification successful
Test pattern correctly matches existing unit test file
The pattern test/unit/*.test.js matches the existing file test/unit/presencemap.test.js, confirming that the configuration change is valid and functional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of test files in the unit directory
# Expected: Find at least one .test.js file in test/unit directory
# Check for test files in the unit directory
fd -p 'test/unit/.*\.test\.js$'
Length of output: 63
src/common/lib/types/defaultmessage.ts (2)
4-4: LGTM: Import changes are well-structured
The imports are properly organized and align with the class's requirements. The uncommented imports are now actively used in the implementation.
Also applies to: 6-8
29-30: Verify test coverage for the updated fromValues signature
The implementation looks good and follows best practices by delegating to the imported function. Since this is marked as "Used by tests", we should verify that all test cases are updated to handle the new signature.
✅ Verification successful
Let's gather more context about the test implementation to verify if it's properly handling the new signature.
Based on the test file content, I can now generate the final response since I have found the relevant test case that verifies the updated fromValues signature.
Test coverage for the updated fromValues signature is verified
The test file includes a comprehensive test suite DefaultMessage.fromValues stringify action that specifically verifies the new signature with the options parameter. The test cases cover:
- Stringifying numeric actions
- Not stringifying actions when disabled
- Handling pre-stringified actions
- Handling undefined actions
- Handling unknown actions
The implementation is well-tested and no further verification is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all test files using DefaultMessage.fromValues to ensure they're updated
# for the new signature with options parameter
# Search for test files using DefaultMessage.fromValues
rg -l "DefaultMessage.fromValues" test/
# Show the actual usage context
rg -B 2 -A 2 "DefaultMessage.fromValues" test/
Length of output: 401
Script:
#!/bin/bash
# Get the full test implementation to verify the usage of DefaultMessage.fromValues
cat test/realtime/message.test.js
Length of output: 52783
test/unit/presencemap.test.js (1)
1-50: Verify alignment with PR objectives
The test suite should explicitly cover the processing of STATE and STATE_SYNC messages as mentioned in PR #1911.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
src/common/lib/client/defaultrealtime.ts (1)
14-14: Verify the usage of PresenceMap across the codebase
Let's ensure the imported PresenceMap is consistently used across the codebase, particularly in presence-related message processing.
✅ Verification successful
The imported PresenceMap is correctly used in the codebase
The verification shows that PresenceMap is properly integrated:
- Defined as a base class in
presencemap.ts - Used in
realtimepresence.tsfor managing presence members - Correctly imported and exposed as a static property in
defaultrealtime.tsfor testing purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of PresenceMap across the codebase
# Expected: Find references in presence-related files and tests
# Check for PresenceMap usage
rg "PresenceMap" --type ts
# Check specific presence-related files for implementation
ast-grep --pattern 'class $_ extends PresenceMap {
$$$
}'
Length of output: 1284
test/support/runPlaywrightTests.js (2)
71-72: LGTM: Clean implementation of grep parameter.
The addition of the grep parameter and its extraction from the environment variable is well-implemented and consistent with the existing codebase patterns.
84-84: LGTM: Proper parameter passing.
The grep parameter is correctly passed to the evaluate function, maintaining the proper scope between Node.js and browser contexts.
src/platform/react-hooks/src/hooks/usePresence.ts (1)
78-80: Verify behavior during rapid state transitions.
The separate state checks could potentially lead to race conditions during rapid state transitions.
Let's verify the presence leave behavior in other files:
✅ Verification successful
The previous results show basic presence leave handling, but we need to investigate how state transitions and potential race conditions are handled in tests and error scenarios. These new commands will help us understand the test coverage and error handling around presence state transitions.
The previous results show some connection state handling but we need to understand how the hook specifically handles state transitions and validates states. These new commands will help us examine the implementation details and test coverage of the usePresence hook.
Based on the gathered information, I can now provide a final response:
The state transition checks are properly implemented and safe from race conditions.
The implementation shows careful handling of state transitions with well-defined inactive states and proper validation:
INACTIVE_CONNECTION_STATESis explicitly defined as['suspended', 'closing', 'closed', 'failed']- Channel states are properly validated against
['attached', 'attaching'] - The state checks are performed synchronously and atomically before the
presence.leave()call - The checks are conservative, only allowing presence leave when both connection and channel states are definitively in valid states
The separate state checks don't introduce race conditions because:
- Both checks are performed synchronously in the same execution context
- The implementation uses a defensive approach, requiring both states to be valid
- The state validation happens immediately before the leave operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for presence leave handling in tests and related files
rg -t ts "presence.leave" --glob "!{node_modules,dist}/**" -A 5
# Look for any existing race condition handling in presence code
ast-grep --pattern 'presence.leave()' -l ts
# Check for related test cases
rg -t ts "test.*presence.*state" --glob "!{node_modules,dist}/**"
Length of output: 3190
Script:
#!/bin/bash
# Let's check for presence state transition handling in tests
rg -t ts "presence.*state.*transition" --glob "!{node_modules,dist}/**" -A 5
# Look for presence state handling in test files
rg -t ts "describe.*presence" --glob "!{node_modules,dist}/**" -A 10
# Check for any error handling around presence state
rg -t ts "presence.*error" --glob "!{node_modules,dist}/**" -A 5
# Look at the connection state handling implementation
ast-grep --pattern 'connection.state'
Length of output: 6606
Script:
#!/bin/bash
# Let's examine the INACTIVE_CONNECTION_STATES definition
rg -t ts "INACTIVE_CONNECTION_STATES" -A 3
# Check for presence error handling in usePresence hook
rg -t ts "usePresence.*error" --glob "!{node_modules,dist}/**" -A 5
# Look for channel state handling in the hook
ast-grep --pattern 'channel.state'
# Check for any presence-related tests specifically for the hook
rg -t ts "test.*usePresence" --glob "!{node_modules,dist}/**" -A 10
Length of output: 4459
package.json (2)
149-149: LGTM: Helpful addition for targeted testing
The new test:grep script is a valuable addition that will help developers run specific tests more efficiently. The implementation correctly includes all necessary build steps (node, push, and liveobjects) before running the filtered tests.
4-4: Verify version bump alignment
Since this PR is merging main into integration/liveobjects, please verify that this version bump (2.4.0 -> 2.5.0) was already made in the main branch first.
✅ Verification successful
Version bump is correctly aligned with main branch
The version "2.5.0" is already present in the main branch, confirming that this version bump was properly made in main first before being merged into the integration/liveobjects branch.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version in main branch
git fetch origin main
git show origin/main:package.json | grep '"version":'
Length of output: 184
Gruntfile.js (1)
76-76: Verify the impact of removing the submodule check from the build task.
While removing checkGitSubmodules from the build task simplifies the build process, we should ensure:
- That no build steps depend on files from the git submodules
- That the change is documented in the build documentation
- That developers are aware they need to initialize submodules manually if needed
Consider one of these alternatives:
- Add a warning message during build if submodules are missing but required
- Move the submodule check to a separate npm script that can be run independently
- Document the submodule requirements clearly in the README
src/common/lib/types/protocolmessage.ts (2)
106-108: LGTM: Improved error handling structure
The addition of braces around the error handling block improves code safety and maintainability.
111-115: Verify the impact of stringifying message actions
The addition of { stringifyAction: true } changes how message actions are processed. Let's verify this change:
✅ Verification successful
Based on the shell script results, I can now generate a final response as there's sufficient evidence to verify the impact of the stringifyAction change.
The stringifyAction change is safe and well-tested
The change to add { stringifyAction: true } is a supported feature that converts numeric action codes to their string representations for better readability. This is verified by:
- Existing test coverage in
test/realtime/message.test.jswith multiple test cases covering different scenarios - Proper implementation in
src/common/lib/types/message.tswherestringifyActionis handled - Usage in both Message and PresenceMessage types
- The change aligns with the handling of STATE (19) and STATE_SYNC (20) messages in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage and impact of stringifyAction option
# Find message action handling in the codebase
echo "=== Checking message action handling ==="
rg -A 5 "stringifyAction"
# Find STATE and STATE_SYNC message processing
echo "=== Checking STATE/STATE_SYNC message handling ==="
rg -A 5 "action.*STATE(_SYNC)?"
# Find tests related to message actions
echo "=== Checking related tests ==="
fd -e test.ts -e spec.ts | xargs rg -l "message.*action"
Length of output: 8786
src/common/lib/types/message.ts (2)
409-415: LGTM! Serialization properly handles new properties.
The toJSON() and toString() methods have been correctly updated to handle the new properties, with proper action conversion in toJSON().
Also applies to: 438-444
372-378: Verify usage of new message properties.
Let's ensure the new message properties are properly utilized across the codebase.
✅ Verification successful
Let me check the actual usage of these properties in the relevant files to ensure they are properly utilized.
Let me check the test files to verify the proper usage and validation of these properties.
Based on the comprehensive evidence gathered, I can now provide the final response:
New message properties are properly implemented and tested
The new message properties (action, serial, refSerial, refType, updatedAt, updateSerial, operation) are:
- Correctly utilized in state message handling (src/plugins/liveobjects/statemessage.ts)
- Properly integrated with transport and protocol layers
- Extensively tested, particularly in STATE and STATE_SYNC scenarios
- Well-covered in live objects implementation with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of new message properties
echo "Searching for usage of new message properties..."
rg -l "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)"
# Search for STATE message handling
echo "Searching for STATE message handling..."
rg -l "STATE(_SYNC)?"
Length of output: 932
Script:
#!/bin/bash
# Check implementation details in key files
echo "Checking statemessage.ts implementation..."
rg -A 5 "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)" src/plugins/liveobjects/statemessage.ts
echo "\nChecking realtimechannel.ts implementation..."
rg -A 5 "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)" src/common/lib/client/realtimechannel.ts
echo "\nChecking transport.ts implementation..."
rg -A 5 "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)" src/common/lib/transport/transport.ts
echo "\nChecking protocol.ts implementation..."
rg -A 5 "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)" src/common/lib/transport/protocol.ts
Length of output: 2432
Script:
#!/bin/bash
# Check test implementations for property usage
echo "Checking message.test.js..."
rg -A 5 "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)" test/realtime/message.test.js
echo "\nChecking sync.test.js..."
rg -A 5 "message\.(action|serial|refSerial|refType|updatedAt|updateSerial|operation)" test/realtime/sync.test.js
# Check for STATE message type handling in tests
echo "\nChecking STATE message handling in tests..."
rg -A 5 "STATE(_SYNC)?" test/realtime/live_objects.test.js
Length of output: 13347
test/common/modules/shared_helper.js (1)
268-268: LGTM: Return value properly propagated
The change correctly propagates the promise return value from _becomeSuspended, enabling both promise-based and callback-based usage patterns.
docs/react.md (1)
Line range hint 391-508: LGTM! Comprehensive documentation of the skip parameter
The documentation for the skip parameter is thorough, well-structured, and includes practical examples with TypeScript support. The real-world use cases effectively demonstrate when and how to use this feature.
README.md (1)
679-698: LGTM! The new section provides valuable guidance
The added section effectively addresses a common issue developers might encounter during development. It provides clear explanations and practical solutions for handling connection limit errors in different scenarios.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~683-~683: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...'re developing your app, due to Next.js server side rendering your components. Note that ev...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~691-~691: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e server side, while not affecting your client side components. - If you're using any Reac...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~693-~693: This phrase is redundant. Consider using “outside”.
Context: ...ent instantiation (new Ably.Realtime) outside of your components. You can find an exampl...
(OUTSIDE_OF)
[uncategorized] ~697-~697: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ime()call to a separate file, such asably-client.js`, and export the client instance fro...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~697-~697: Consider shortening or rephrasing this to strengthen your wording.
Context: ...only be recreated when you specifically make changes to the ably-client.js file, which should...
(MAKE_CHANGES)
[uncategorized] ~697-~697: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...en you specifically make changes to the ably-client.js file, which should be far less freq...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Markdownlint (0.35.0)
687-687: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
CHANGELOG.md (1)
5-10: LGTM! The changelog entry is well-structured.
The new version entry for 2.5.0 follows the standard format and clearly describes the addition of new Message attributes to support upcoming features. The changes are properly linked to PR #1888.
src/common/lib/client/presencemap.ts (1)
1-205: Overall, the implementation of PresenceMap is solid
The PresenceMap class provides robust methods for managing presence messages, following best practices. The code is clear, maintainable, and effectively utilizes TypeScript features.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/common/lib/client/realtimepresence.ts (4)
16-16: Correct import of PresenceMap and RealtimePresenceParams
The import statement correctly brings in PresenceMap and RealtimePresenceParams from './presencemap', reflecting the refactoring of the PresenceMap class into its own module.
406-407: Initialization of 'myMembers' and 'connId' variables
The variables myMembers and connId are appropriately initialized, ensuring that the subsequent logic references the correct instances.
420-421: Conditional assignment of 'id' based on connection ID
The logic to suppress the id when the connectionId has changed is correctly implemented. This ensures compliance with RTP17g1 by only including the id if the entry.connectionId matches the current connId.
421-431: Proper error handling in presence auto re-enter
The catch block appropriately wraps the error with a new ErrorInfo instance and logs the error using Logger.logAction. Emitting the 'update' event with a new ChannelStateChange effectively notifies listeners of the error state.
test/realtime/crypto.test.js (2)
398-398: Correct usage of { stringifyAction: true } in Message.fromValues
Including { stringifyAction: true } ensures that the action property is properly converted to a string during message deserialization, which is necessary for accurate message handling in the tests.
443-443: Consistent application of { stringifyAction: true } option
Adding { stringifyAction: true } here aligns with its usage in other test cases, ensuring uniform handling of the action field across all encryption tests.
src/common/lib/client/realtimechannel.ts (1)
771-815: The _decodeAndPrepareMessages method is well-implemented
The new _decodeAndPrepareMessages method effectively decodes messages, handles errors appropriately, and ensures that necessary fields like connectionId, timestamp, and id are set. The use of a generic type and an optional error recovery handler adds flexibility and robustness to message processing.
test/realtime/presence.test.js (2)
4-4: Simplify Chai imports using destructuring
The import statement has been updated to destructure expect and assert from chai, improving code readability and conciseness.
571-584: Refactor test to use async/await for better readability
The presenceEnterAndLeave test has been refactored to use async/await syntax instead of callbacks, enhancing the readability and maintainability of the code.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
VeskeR
changed the title
main into `integration/liveobjects' 2024-11-29main into integration/liveobjects 2024-11-29
Merge latest
maininto liveobjects branch to grab #1911 changes -- will allow us to use that common code forSTATEandSTATE_SYNCmessage processingSummary by CodeRabbit
Release Notes
New Features
Messageattributes to enhance metadata handling.PresenceMapclass for improved management of client presence in real-time applications.Bug Fixes
usePresencehook to refine presence leave conditions.Documentation
CHANGELOG.mdwith version updates and new feature highlights.Tests
async/awaitfor improved readability and error handling.