presence: remove event filter #675
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughPresence.subscribe API reduced to a single signature Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Presence
participant DefaultPresence as Impl
Note over Client,Presence: New unified subscription API
Client->>Presence: subscribe(listener)
Presence->>Impl: registerWrappedListener(listener)
rect rgba(220,240,255,0.35)
Note right of Impl: All presence events forwarded to the same listener\n(no per-event filtering)
Impl-->>Client: onPresenceEvent(event)
Impl-->>Client: onPresenceEvent(event)
end
Client->>Presence: unsubscribe()
Presence->>Impl: removeWrappedListener(listener)
Impl-->>Client: stopped receiving events
sequenceDiagram
autonumber
participant Client
participant Presence
Note over Client,Presence: Previous (removed) event-specific subscription
Client-x Presence: subscribe(eventType | [eventTypes], listener?)
Note over Presence: Overloads and per-event subscribe/unsubscribe logic removed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
If ably/ably-chat-js#675 is approved we will no longer allow subscription to specific events.
AndyTWF
requested review from
splindsay-92 and
vladvelici
and removed request for
vladvelici
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/core/presence.ts(2 hunks)test/core/presence.integration.test.ts(4 hunks)test/core/presence.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- test/core/presence.test.ts
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
src/**/*.{ts,tsx}: Import Ably as: import * as Ably from 'ably'
Include spec point comments in code using // @CHA- (e.g., // @CHA-M10a)
Files:
src/core/presence.ts
⚙️ CodeRabbit configuration file
src/**/*.{ts,tsx}: If there are any changes to the public-facing API, add a review comment: "Public API changes detected. Please update the documentation and the Ably CLI."
Files:
src/core/presence.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use relative imports within the project; import Ably asimport * as Ably from 'ably'
Prefer named exports over default exports and group related exports
Use PascalCase for interface names; do not prefix with 'I'; document interfaces and properties with JSDoc, including @throws and @return
Use PascalCase for enums and enum members; include JSDoc for enums and members
Use PascalCase for class names; implement interfaces explicitly; avoid inheritance when possible; document classes
Prefix private members with underscore; group members by visibility; document all public members
Document all public methods with JSDoc using @param, @returns, @throws
Use descriptive parameter names; prefer parameter objects for related params; use optional params with defaults when appropriate
Use Ably.ErrorInfo for errors with message, code, and HTTP-derived status code; use ErrorCodes from src/core/errors.ts
Use consistent logging levels (trace, debug, error); include context object; never log Ably channel instances; structure logs clearly
Minimize type assertions; prefer type guards; never use any (use unknown if unavoidable) and document assertions
Prefer async/await over raw promises; handle rejections; document async behavior
For each @[Testable]@ spec point, include the spec annotation (e.g., // @CHA-M10a) in the relevant production code
**/*.{ts,tsx}: Use relative imports within the project
Import Ably asimport * as Ably from 'ably'
Export interfaces and types that are part of the public API
Prefer named exports over default exports
Group related exports together
Use PascalCase for interface names
Use descriptive interface names that reflect purpose
Do not prefix interfaces with 'I'
Use JSDoc comments for interfaces with a clear description
Document all interface properties inline with JSDoc
Document errors with @throws on interfaces when relevant
Document return types with @return in interface-related docs when relevant
Link referenced types in docs using ...
Files:
src/core/presence.tstest/core/presence.integration.test.ts
{src,test}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create all TypeScript and TSX files using kebab-case file names
Files:
src/core/presence.tstest/core/presence.integration.test.ts
{src,test,react,demo}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{src,test,react,demo}/**/*.{ts,tsx}: Use relative imports within the project
Use PascalCase for classes, interfaces, and enums
Prefix private members with an underscore (e.g., _roomId, _channel)
Avoid any; use unknown if necessary, and prefer strong typing
Use async/await over raw promises
Files:
src/core/presence.tstest/core/presence.integration.test.ts
src/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/core/**/*.{ts,tsx}: Error handling must use Ably.ErrorInfo with ErrorCodes enum; construct as new Ably.ErrorInfo(message, code, statusCode)
Log key operations with _logger.trace/_logger.debug/_logger.error using context objects; never log Ably channel instances
Files:
src/core/presence.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
test/**/*.{ts,tsx}: Include spec point comments in tests using // CHA- (e.g., // CHA-M10a)
Use Vitest (describe, it, expect) for tests
Use custom matchers from test/helper/expectations.ts for error assertions (e.g., toBeErrorInfo, toThrowErrorInfo)
Follow Arrange-Act-Assert structure in tests
Use data-driven tests with .each() when appropriate
Files:
test/core/presence.integration.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
**/*.test.{ts,tsx}: Use the custom matchertoBeErrorInfoto validate that an error matches expected ErrorInfo properties (code, statusCode, message, cause) when testing error scenarios.
Use the custom matchertoThrowErrorInfoto check if a function throws an error matching expected ErrorInfo properties in tests.
Use the custom matchertoBeErrorInfoWithCodeto validate that an error has a specific error code in tests.
Use the custom matchertoThrowErrorInfoWithCodeto check if a function throws an error with a specific code in tests.
Use the custom matchertoBeErrorInfoWithCauseCodeto validate that an error has a cause with a specific error code in tests.
UsewaitForEventualHookValueToBeDefinedutility to wait for a React hook value to become defined in asynchronous hook tests.
UsewaitForEventualHookValueutility to wait for a React hook to return an expected value in asynchronous hook tests.
Usevi.waitForto wait for asynchronous events to happen in tests.
**/*.test.{ts,tsx}: Mock theablylibrary as a module usingvi.mock('ably')in test files that use Ably.
Each test file should use Vitest and follow the structure: import { describe, expect, it } from 'vitest'; group tests withdescribeand define test cases withit.
Use clear, descriptive names for test descriptions, following the format:it('should [expected behavior]').
Group related tests usingdescribeblocks and use nested describes for sub-features.
Structure each test case with Arrange, Act, and Assert sections.
Test both positive and negative cases, including edge cases and boundary conditions.
Use clear, specific assertions withexpect, and chain assertions when testing multiple aspects.
Use common assertion methods such astoBe(),toEqual(),toBeTruthy(),toBeFalsy(), andtoThrowErrorInfo()in tests.
Create meaningful test data that clearly demonstrates the test case, and use constants for repeated values.
Test error cases explicitly using `toThrowErrorInf...
Files:
test/core/presence.integration.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/unit-testing.mdc)
Test files should end with
.test.tsand the name should reflect the module being tested (e.g.,message.test.tsformessage.ts).Unit test files should end with .test.ts and have clear, behavior-focused descriptions
Files:
test/core/presence.integration.test.ts
**/*.integration.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.integration.test.{ts,tsx}: Integration tests must use Vitest structure with describe/it/beforeEach and optional typed TestContext
Use vi.waitFor for async events and async/await with appropriate timeouts in integration tests
Do not mock modules in integration tests; use real Ably.Realtime clients
Files:
test/core/presence.integration.test.ts
test/**/*.integration.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.integration.test.{ts,tsx}: Integration test files must end with .integration.test.ts or .integration.test.tsx
Integration tests connect to a real Ably sandbox (no mocking)
Use newChatClient() helper to create fully connected clients in integration tests
Use vi.waitFor() for async event waiting in integration tests
Use random room IDs in integration tests to avoid conflicts
Files:
test/core/presence.integration.test.ts
test/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests for core SDK under test/core/ mirroring src/core/
Files:
test/core/presence.integration.test.ts
🧬 Code graph analysis (2)
src/core/presence.ts (2)
src/core/subscription.ts (1)
Subscription(12-19)src/core/utils/event-emitter.ts (1)
wrap(86-89)
test/core/presence.integration.test.ts (2)
src/core/presence.ts (1)
PresenceEvent(85-95)src/core/index.ts (1)
PresenceEvent(67-67)
🔇 Additional comments (3)
test/core/presence.integration.test.ts (1)
228-306: LGTM! Tests correctly updated for simplified API.The integration tests have been properly refactored to use the generic
subscribe(listener)API:
- All subscribe calls now pass a single listener that receives all presence events
- Comments accurately reflect listening to all presence events
- Event-specific validation is handled via filtering in the test assertions
- Unsubscribe behavior is correctly verified
The changes align with the PR objective of removing event filtering from the SDK.
src/core/presence.ts (2)
310-327: Implementation correctly simplified to single subscription path.Assuming EventEmitter supports the wildcard listener pattern, the implementation correctly:
- Validates that presence events are enabled (lines 314-317)
- Wraps the listener for identity preservation during unsubscribe (line 319)
- Registers the listener to receive all presence events (line 320)
- Returns a subscription with proper cleanup (lines 321-326)
- Maintains consistent logging (lines 311, 323)
The simplified implementation reduces complexity and aligns with the PR objective of removing event filtering.
310-327: No action needed:EventEmitter.on()supports all-events subscriptions. TheInterfaceEventEmitterdefines an overloadon(callback: Callback<EventsMap>)for subscribing to all events.
If ably/ably-chat-js#675 is approved we will no longer allow subscription to specific events.
In the rest of the SDK, we've not provided event filters on subscription events - this is a fairly trivial thing for users to write (if/switch). Also, when dealing with multiple events in the list, the if/switch is required anyway, so the real benefit is when you're listening to a single event (but still marginal). Furthermore, the overloading is somewhat cumbersome to maintain. This change removes the event filtering for presence.
AndyTWF
force-pushed
the
presence-filter-removal
branch
from
4bca40c to
4b452d7
Compare
Description
In the rest of the SDK, we've not provided event filters on subscription events - this is a fairly trivial thing for users to write (if/switch). Also, when dealing with multiple events in the list, the if/switch is required anyway and in many languages must be exhaustive, so the real benefit is when you're listening to a single event (but still marginal).
Furthermore, the overloading is somewhat cumbersome to maintain.
This change removes the event filtering for presence.
Spec change: ably/specification#390
Checklist
Testing Instructions (Optional)
Summary by CodeRabbit