Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PresenceMap: only emit a leave if a member was present #1918

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

SimonWoolf
Copy link
Member

@SimonWoolf SimonWoolf commented Nov 13, 2024

per ably/specification#222

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a PresenceMap class for managing client presence in real-time applications.
    • Enhanced DefaultRealtime class with access to PresenceMap.
  • Bug Fixes

    • Improved error handling in presence management, ensuring better feedback for missing client IDs.
  • Tests

    • Added a test suite for the PresenceMap class, validating the functionality of the remove() method.
    • Refactored presence tests to utilize async/await for improved readability and reliability.

Copy link

coderabbitai bot commented Nov 13, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The changes in this pull request involve modifications to several files to enhance the management of client presence in a real-time application. The .mocharc.js file now includes an additional test specification path for unit tests. A new PresenceMap class is introduced in presencemap.ts to manage client presence data, which is then integrated into the RealtimePresence class in realtimepresence.ts. Additionally, a test suite for the PresenceMap class is added in presencemap.test.js to validate its functionality.

Changes

File Path Change Summary
.mocharc.js Updated config.spec to include 'test/unit/*.test.js' for unit tests.
src/common/lib/client/defaultrealtime.ts Added static property _PresenceMap to DefaultRealtime class, importing PresenceMap.
src/common/lib/client/presencemap.ts Introduced PresenceMap class managing client presence with methods for adding, retrieving, and removing messages, along with synchronization methods. Added interface RealtimePresenceParams.
src/common/lib/client/realtimepresence.ts Integrated PresenceMap into RealtimePresence, removed newerThan function, and updated presence management logic.
test/unit/presencemap.test.js Added a test suite for PresenceMap, validating the remove() method with various test cases to ensure correct behavior.
test/realtime/presence.test.js Refactored presenceEnterAndLeave test case to use async/await for better readability and reliability, updating variable declarations accordingly.

Possibly related PRs

Suggested reviewers

  • ttypic
  • lawrence-forooghian

Poem

🐰 In the realm of code where bunnies play,
A new PresenceMap has come to stay.
With tests to ensure it hops just right,
Managing presence, a joyous sight!
So let’s celebrate this change today,
With fluffy code, we’ll leap and sway! 🐇


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot temporarily deployed to staging/pull/1918/features November 13, 2024 20:19 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/bundle-report November 13, 2024 20:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/typedoc November 13, 2024 20:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/features November 13, 2024 20:22 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/bundle-report November 13, 2024 20:22 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/typedoc November 13, 2024 20:22 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (8)
.mocharc.js (1)

12-12: LGTM! Consider updating the comment to document the test directories.

The addition of the unit test pattern is correct and aligns with the new PresenceMap implementation. Consider updating the comment above to document all test directories for better maintainability.

 // mocha has a ridiculous issue (https://github.com/mochajs/mocha/issues/4100) that command line
 // specs don't override config specs; they are merged instead, so you can't run a single test file
-// if you've defined specs in your config. therefore we work around it by only adding specs to the
-// config if none are passed as arguments
+// if you've defined specs in your config. Therefore we work around it by only adding specs to the
+// config if none are passed as arguments. The specs include:
+// - test/realtime/*.test.js: Realtime-related tests
+// - test/rest/*.test.js: REST-related tests
+// - test/unit/*.test.js: Unit tests for individual components
test/unit/presencemap.test.js (3)

1-6: Remove redundant 'use strict' directive

The 'use strict' directive is redundant in JavaScript modules as they are automatically in strict mode.

-'use strict';

define(['chai', 'ably'], function (chai, Ably) {
🧰 Tools
🪛 Biome

[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)


10-16: Add input validation and documentation to helper function

The helper function could benefit from input validation and JSDoc documentation for better maintainability.

-    // Helper function to create a presence message
-    const createPresenceMessage = (clientId, connectionId, action, timestamp) => ({
+    /**
+     * Creates a presence message object for testing
+     * @param {string} clientId - The client identifier
+     * @param {string} connectionId - The connection identifier
+     * @param {string} action - The presence action (present/leave)
+     * @param {number} timestamp - The message timestamp
+     * @returns {Object} The presence message object
+     */
+    const createPresenceMessage = (clientId, connectionId, action, timestamp) => {
+      if (!clientId || !connectionId || !action || timestamp == null) {
+        throw new Error('All parameters are required');
+      }
+      return {
       clientId,
       connectionId,
       timestamp,
       action,
-    });
+      };
+    };

27-46: Consider adding more test cases for comprehensive coverage

While the current tests cover the basic scenarios, consider adding tests for:

  1. Multiple presence entries for the same client
  2. Edge cases with identical timestamps
  3. Different presence actions (enter, update)
  4. Boundary conditions for timestamps

Example test case:

it('should handle multiple presence entries for same client', () => {
  const present1 = createPresenceMessage('client1', 'conn1', 'present', 100);
  const present2 = createPresenceMessage('client1', 'conn2', 'present', 150);
  presenceMap.put(present1);
  presenceMap.put(present2);
  
  const leave1 = createPresenceMessage('client1', 'conn1', 'leave', 200);
  assert.isTrue(presenceMap.remove(leave1));
  assert.isTrue(presenceMap.has('client1:conn2'));
});
src/common/lib/client/presencemap.ts (4)

67-68: Use Optional Chaining for Safer Property Access

You can simplify property access and prevent potential null reference errors by using optional chaining when accessing params.clientId and params.connectionId.

Apply this diff to improve the code:

 const map = this.map,
-      clientId = params && params.clientId,
-      connectionId = params && params.connectionId,
+      clientId = params?.clientId,
+      connectionId = params?.connectionId,
       result = [];
🧰 Tools
🪛 Biome

[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)


60-60: Use Strict Equality Operators to Avoid Type Coercion

Using strict equality (=== and !==) instead of abstract equality (== and !=) prevents unexpected type coercion, leading to safer comparisons.

Apply this diff to enhance code safety:

/* Line 60 */
- if (item.clientId == clientId && item.action != 'absent') result.push(item);
+ if (item.clientId === clientId && item.action !== 'absent') result.push(item);

/* Lines 74-75 */
- if (clientId && clientId != item.clientId) continue;
- if (connectionId && connectionId != item.connectionId) continue;
+ if (clientId && clientId !== item.clientId) continue;
+ if (connectionId && connectionId !== item.connectionId) continue;

/* Line 105 */
- if (item.action != 'absent') result.push(item);
+ if (item.action !== 'absent') result.push(item);

Also applies to: 74-75, 105-105


44-44: Consider Using Map for Enhanced Performance

Using a Map object instead of a plain object for this.map can improve performance, especially when dealing with a large number of members. Map provides better optimization for frequent additions and deletions.

Apply this diff to update the implementation:

- this.map = Object.create(null);
+ this.map = new Map<string, PresenceMessage>();

You'll also need to update the methods to accommodate the Map API, for example, using this.map.get(key) instead of this.map[key].


115-117: Avoid Returning false When No Action is Needed

In the remove method, returning false when no action is taken might be misleading. Consider returning void or a more descriptive result to indicate that no modification was necessary.

Adjust the return statement:

- return false;
+ return;

And update the method signature if needed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 18a2559 and c28f836.

📒 Files selected for processing (5)
  • .mocharc.js (1 hunks)
  • src/common/lib/client/defaultrealtime.ts (2 hunks)
  • src/common/lib/client/presencemap.ts (1 hunks)
  • src/common/lib/client/realtimepresence.ts (1 hunks)
  • test/unit/presencemap.test.js (1 hunks)
🧰 Additional context used
🪛 Biome
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)

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 (4)
test/unit/presencemap.test.js (1)

18-25: LGTM! Well-structured test initialization

The test setup is well-designed with:

  • Proper beforeEach hook
  • Correct member key function combining clientId and connectionId
  • Appropriate timestamp-based comparison function
src/common/lib/client/defaultrealtime.ts (2)

14-14: LGTM! Clean import statement

The import is properly grouped with other presence-related imports and follows the established conventions.


73-73: Verify test usage of _PresenceMap

The static property follows the established pattern for test-related properties. Let's verify its usage in tests.

✅ Verification successful

Static _PresenceMap property is used in tests as expected

The verification confirms that:

  • The _PresenceMap static property is used in tests via Ably.Realtime._PresenceMap
  • It follows the established pattern for test access as seen in test/unit/presencemap.test.js
  • The property provides test access to the internal PresenceMap class which is imported in defaultrealtime.ts
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of _PresenceMap in test files
# Expected: Find references to DefaultRealtime._PresenceMap in test files

# Search for test files using _PresenceMap
rg "DefaultRealtime\._PresenceMap" -t js -t ts

Length of output: 48


Script:

#!/bin/bash
# Let's try a broader search to understand the usage pattern
# First, find all test files that mention DefaultRealtime
rg "DefaultRealtime" --type ts --type js -l | grep -i "test"

# Also search for any usage of _PresenceMap in test files
rg "_PresenceMap" --type ts --type js | grep -i "test"

# Let's also check the actual implementation to understand the context
rg "class DefaultRealtime" -A 5 -B 5

Length of output: 1473


Script:

#!/bin/bash
# Let's check the presencemap.test.js file content to understand the test usage
cat test/unit/presencemap.test.js

# Also check the PresenceMap import in DefaultRealtime
rg "import.*PresenceMap" src/common/lib/client/defaultrealtime.ts -A 2 -B 2

Length of output: 1995

src/common/lib/client/realtimepresence.ts (1)

16-16: Integration of PresenceMap and RealtimePresenceParams

The addition of this import statement correctly integrates the PresenceMap and RealtimePresenceParams classes from './presencemap', which aligns with the proposed enhancements to the presence management system.

src/common/lib/client/presencemap.ts Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging/pull/1918/bundle-report November 13, 2024 20:52 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/features November 13, 2024 20:52 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1918/typedoc November 13, 2024 20:52 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
test/unit/presencemap.test.js (2)

1-1: Remove redundant 'use strict' directive

The 'use strict' directive is redundant in modules as they are automatically in strict mode.

-'use strict';
🧰 Tools
🪛 Biome

[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)


29-48: Enhance test coverage for leave events

While the current tests verify basic removal behavior, consider adding these test cases to better align with the PR objective of "only emit leave if member was present":

  1. Multiple members scenario:

    • Add multiple members
    • Remove one member
    • Verify others remain unchanged
  2. Double leave scenario:

    • Add a member
    • Remove the member
    • Attempt to remove again
    • Verify second remove returns false
  3. Timestamp edge cases:

    • Test removal with equal timestamps

Would you like me to help generate these additional test cases?

test/realtime/presence.test.js (1)

571-584: Great refactoring to async/await pattern!

The refactoring improves code readability and maintainability by:

  1. Using modern async/await syntax instead of callbacks
  2. Using Promise.all for concurrent operations
  3. Properly handling cleanup by closing the client

Consider adding an assertion to verify the presence state after leave:

 await Promise.all([clientChannel.presence.leave(), clientChannel.presence.subscriptions.once('leave')]);
+const members = await clientChannel.presence.get();
+expect(members.length).to.equal(0, 'Presence set should be empty after leave');
 clientRealtime.close();
src/common/lib/client/presencemap.ts (1)

67-68: Simplify property access using optional chaining

You can enhance code readability by using optional chaining when accessing clientId and connectionId from params.

Apply this diff to simplify the code:

-    clientId = params && params.clientId,
-    connectionId = params && params.connectionId,
+    clientId = params?.clientId,
+    connectionId = params?.connectionId,
🧰 Tools
🪛 Biome

[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)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c28f836 and 90ef5bc.

📒 Files selected for processing (4)
  • .mocharc.js (1 hunks)
  • src/common/lib/client/presencemap.ts (1 hunks)
  • test/realtime/presence.test.js (1 hunks)
  • test/unit/presencemap.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .mocharc.js
🧰 Additional context used
🪛 Biome
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)

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 (2)
test/unit/presencemap.test.js (2)

13-27: LGTM! Well-structured test setup

The test setup is comprehensive with:

  • Clear helper function for creating presence messages
  • Proper memberKey function combining clientId and connectionId
  • Correct timestamp-based comparison function

5-5: Verify the use of internal _PresenceMap API

The code is accessing Ably.Realtime._PresenceMap which appears to be an internal API (prefixed with underscore). This could be risky if the internal API changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants