Support annotations #1953
Support annotations #1953
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
ably.d.ts (1)
Line range hint
858-888: Add missing documentation for newChannelModeconstants.The constants
ANNOTATION_PUBLISHandANNOTATION_SUBSCRIBEin theChannelModesnamespace are missing documentation comments. This is causing warnings in the pipeline failures. Please add appropriate documentation to these constants.🧰 Tools
🪛 GitHub Actions: API Reference
[warning] RealtimeAnnotations class is missing documentation
[warning] RealtimeAnnotations.unsubscribe.refType is missing documentation
[warning] RestAnnotations class is missing documentation
[warning] Annotation class is missing documentation
[warning] AnnotationActions class is missing documentation
[warning] AnnotationAction class is missing documentation
[warning] Failed to resolve link to 'string' in RealtimeAnnotations.unsubscribe documentation
[error] Documentation generation failed with exit code 4
🧹 Nitpick comments (9)
src/common/lib/types/annotation.ts (1)
13-13: Ensure All Annotation Actions are Accounted ForThe
actionsarray currently includes only'annotation.create'and'annotation.delete'. If there are other annotation actions that need to be supported in the future, consider defining them here to maintain consistency.src/common/lib/types/protocolmessage.ts (1)
31-31: Missing Parameter Documentation forformatThe
deserializefunction now accepts aformatparameter, but its purpose isn't documented. Consider adding documentation or comments to explain the use of this parameter.src/common/lib/types/protocolmessagecommon.ts (2)
24-26: Add documentation for new action constants.The new action constants
STATE,STATE_SYNC, andANNOTATIONhave been added to theactionsobject without accompanying documentation. Please provide documentation comments for these constants to enhance code readability and maintainability.
60-67: Add documentation for new channel modes.The
channelModesarray now includesANNOTATION_PUBLISHandANNOTATION_SUBSCRIBEwithout accompanying documentation. Please document these new modes to maintain consistency and clarity.src/common/lib/client/realtimeannotations.ts (1)
73-78: Consider cleanup in unsubscribe method.The event emitter subscriptions might need cleanup when the channel is detached or closed.
Consider adding a cleanup method:
+ _cleanup(): void { + this.subscriptions.off(); + }src/common/lib/transport/comettransport.ts (1)
356-360: Consider improving the indentation for better readability.While the implementation is correct and consistent with the WebSocket transport, the function call could be formatted better.
- protocolMessageFromDeserialized( - items[i], - this.connectionManager.realtime._RealtimePresence, - this.connectionManager.realtime._Annotations, - ), + protocolMessageFromDeserialized(items[i], + this.connectionManager.realtime._RealtimePresence, + this.connectionManager.realtime._Annotations),src/common/lib/client/realtimepresence.ts (3)
179-197: Improved error handling with more specific state checks.The error handling is now more granular with explicit handling of different channel states. However, the error code 90001 is reused for different error scenarios (incompatible state vs invalid state), which could make error handling harder for API consumers.
Consider using distinct error codes for different error scenarios to help API consumers handle errors more precisely:
- throw new PartialErrorInfo('Unable to leave presence channel (incompatible state)', 90001); + throw new PartialErrorInfo('Unable to leave presence channel (incompatible state)', 90002);
354-357: Well-structured promise chain for presence message handling.The promise chain effectively handles both success and error cases for all queued presence messages. However, consider adding timeout handling for the presence message sending operation.
Consider adding a timeout to prevent indefinite waiting:
this.channel .sendPresence(presenceArray) + .timeout(this.channel.options.timeouts.presence) .then(() => multicaster()) .catch((err: ErrorInfo) => multicaster(err));
Line range hint
1-197: Strong API design and modularization for testing scenarios.The promise-based API design and error handling are well-suited for server-side end-to-end testing:
- Clean promise chains enable easy async testing
- Comprehensive error handling allows testing error scenarios
- Clear separation of concerns facilitates test isolation
Consider adding JSDoc comments to document the testing-specific behaviors and error scenarios for better test maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ably.d.ts(8 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/defaultrest.ts(2 hunks)src/common/lib/client/modularplugins.ts(2 hunks)src/common/lib/client/realtimeannotations.ts(1 hunks)src/common/lib/client/realtimechannel.ts(11 hunks)src/common/lib/client/realtimepresence.ts(3 hunks)src/common/lib/client/restannotations.ts(1 hunks)src/common/lib/client/restchannel.ts(3 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/websockettransport.ts(1 hunks)src/common/lib/types/annotation.ts(1 hunks)src/common/lib/types/protocolmessage.ts(5 hunks)src/common/lib/types/protocolmessagecommon.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: API Reference
ably.d.ts
[warning] RealtimeAnnotations class is missing documentation
[warning] RealtimeAnnotations.unsubscribe.refType is missing documentation
[warning] RestAnnotations class is missing documentation
[warning] Annotation class is missing documentation
[warning] AnnotationActions class is missing documentation
[warning] AnnotationAction class is missing documentation
[warning] Failed to resolve link to 'string' in RealtimeAnnotations.unsubscribe documentation
[error] Documentation generation failed with exit code 4
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (20)
src/common/lib/types/annotation.ts (2)
68-68: Use Default Action in Case of Undefinedthis.actionIn the
encodemethod of theAnnotationclass, ifthis.actionisundefined, it defaults toactions.indexOf('annotation.create'), which returns0. Ensure that this default behavior aligns with the intended functionality.
121-121: ExportAnnotationClass ConsistentlyEnsure that the default export at the end of the file exports the
Annotationclass correctly. Verify thatexport default Annotation;is appropriate and consistent with other exports in the codebase.src/common/lib/types/protocolmessage.ts (1)
146-147: Ensure Flags are Properly Defined ingetModeMethodThe
getModemethod usesflags.MODE_ALL. Confirm thatMODE_ALLis correctly defined in theflagsobject to avoid unexpected results whenthis.flagsisundefined.src/common/lib/client/realtimechannel.ts (3)
655-668: Ensure All Action Types are Handled inprocessMessageIn the
processMessagemethod, a new case foractions.ANNOTATIONhas been added. Verify that all possible action types are accounted for, and consider adding a default case to handle unexpected actions.
263-273: 🛠️ Refactor suggestionAdd Error Handling for Message Encoding in
publishMethodIn the
publishmethod, errors during message encoding are not caught, which could lead to unhandled promise rejections. Wrap the encoding logic in a try-catch block to handle potential errors.Apply this diff to add error handling:
const maxMessageSize = this.client.options.maxMessageSize; -const wireMessages = await encodeMessagesArray(messages, this.channelOptions as CipherOptions); +let wireMessages; +try { + wireMessages = await encodeMessagesArray(messages, this.channelOptions as CipherOptions); +} catch (err) { + throw new ErrorInfo('Failed to encode messages: ' + err.message, 50000, 500); +}Likely invalid or redundant comment.
68-73: 🛠️ Refactor suggestionHandle Nullability of
_annotationsin GetterIn the
annotationsgetter, an error is thrown ifthis._annotationsisnull. Consider initializing_annotationsin the constructor or adjusting the getter to handle thenullcase more gracefully.Initialize
_annotationsin the constructor if the plugin is missing:constructor(client: BaseRealtime, name: string, options?: API.ChannelOptions) { super(client.logger); // Existing initialization... - this._annotations = null; + this._annotations = client._Annotations ? new client._Annotations.RealtimeAnnotations(this) : null; + if (!this._annotations) { + this._annotations = new RealtimeAnnotations(this); + } }Likely invalid or redundant comment.
src/common/lib/types/protocolmessagecommon.ts (2)
46-47: Verify correctness of new flag bit shifts.The new flags
ANNOTATION_PUBLISH(1 << 21) andANNOTATION_SUBSCRIBE(1 << 22) have been added. Ensure that these bit shifts do not conflict with existing flags and that they are correctly integrated throughout the codebase.
52-58: UpdateMODE_ALLflag to include new annotation flags.The
MODE_ALLflag has been updated to includeANNOTATION_PUBLISHandANNOTATION_SUBSCRIBE. Ensure that all necessary flags are included and that this change does not introduce unintended side effects.src/common/lib/client/modularplugins.ts (2)
24-29: Well-structured plugin type definition!The
AnnotationsPlugintype follows the established plugin pattern and maintains consistency with the existing architecture.
36-36: Clean interface extension!The addition of the optional
Annotationsproperty maintains backward compatibility while extending the plugin system.src/common/lib/client/defaultrest.ts (1)
31-36: Consider encryption implications for annotations.The annotations plugin is added correctly, but since there's encryption support in the codebase (referenced by the Crypto plugin), consider whether annotations need encryption support as well.
Run this script to check encryption usage patterns:
src/common/lib/client/defaultrealtime.ts (1)
44-49: Excellent consistency with REST implementation!The annotation plugin integration mirrors the implementation in
defaultrest.ts, maintaining a consistent approach across both REST and Realtime clients.src/common/lib/client/realtimeannotations.ts (2)
62-68: Good channel mode validation!The explicit check for
ANNOTATION_SUBSCRIBEmode with a clear error message helps prevent confusion and improves developer experience.
21-48: Review error handling in publish method.The publish method should handle potential errors from:
annotation.encode()channel.sendMessage()Consider adding try-catch blocks. Let's verify error handling patterns:
src/common/lib/client/restchannel.ts (2)
36-42: LGTM! Clean implementation of the annotations getter.The implementation follows good practices:
- Private member with public getter pattern
- Proper null check with descriptive error message
- Type-safe implementation
53-55: LGTM! Proper plugin initialization.The initialization is well-placed in the constructor and follows the same pattern as other plugins (e.g., Push).
src/common/lib/client/baseclient.ts (1)
50-50: LGTM! Consistent plugin integration.The implementation follows the established plugin pattern:
- Readonly property ensures immutability
- Null-safe initialization with nullish coalescing
- Consistent with other plugin implementations (_MsgPack, _Crypto)
Also applies to: 103-103
src/common/lib/transport/websockettransport.ts (1)
143-143: LGTM! Proper integration with protocol message deserialization.The addition of _Annotations parameter is consistent with the existing pattern and necessary for annotation support.
src/common/lib/transport/comettransport.ts (1)
Line range hint
1-1: Overall feedback on API design and modularizationThe implementation demonstrates a well-thought-out approach:
- Consistent use of plugin architecture across all components
- Clean separation of concerns with proper encapsulation
- Type-safe implementations with proper null checking
- Follows existing patterns in the codebase
The modularization approach looks correct. Consider adding some basic unit tests even in the draft PR to validate the plugin initialization and error handling paths, which don't depend on the server-side implementation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 353-353: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/common/lib/client/realtimepresence.ts (1)
130-130: Great simplification of promise handling!Directly returning the promise from
sendPresenceeliminates unnecessary promise wrapping while maintaining the same error handling behavior.
SimonWoolf
force-pushed
the
support-annotations
branch
from
fe6c7ac to
eeaf7ff
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ably.d.ts (3)
1985-2038: 🛠️ Refactor suggestionAdd missing documentation for RealtimeAnnotations interface.
The interface needs TSDoc comments for:
- The interface itself
- The
subscribemethod's parameters and return value- The
unsubscribemethod's parameters- The
publishmethod's parameters and return valueThis will resolve the documentation warnings in the pipeline failures.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
2134-2147: 🛠️ Refactor suggestionAdd missing documentation for RestAnnotations interface.
The interface needs TSDoc comments for:
- The interface itself
- The
publishmethod's parameters and return valueThis will resolve the documentation warnings in the pipeline failures.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
2459-2492: 🛠️ Refactor suggestionAdd missing documentation for Annotation interface.
The interface needs TSDoc comments for:
- The interface itself
- Each property's purpose and type
This will resolve the documentation warnings in the pipeline failures.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
🧹 Nitpick comments (1)
src/common/lib/types/protocolmessage.ts (1)
132-132: Initialize new properties in ProtocolMessage class.The new properties
annotationsandparamsshould be initialized in the constructor to prevent potential undefined errors.Apply this diff:
class ProtocolMessage { + constructor() { + this.annotations = []; + this.params = {}; + } // ... rest of the class }Also applies to: 135-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ably.d.ts(8 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/defaultrest.ts(2 hunks)src/common/lib/client/modularplugins.ts(2 hunks)src/common/lib/client/realtimeannotations.ts(1 hunks)src/common/lib/client/realtimechannel.ts(11 hunks)src/common/lib/client/restannotations.ts(1 hunks)src/common/lib/client/restchannel.ts(3 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/websockettransport.ts(1 hunks)src/common/lib/types/annotation.ts(1 hunks)src/common/lib/types/protocolmessage.ts(5 hunks)src/common/lib/types/protocolmessagecommon.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/common/lib/transport/comettransport.ts
- src/common/lib/client/modularplugins.ts
- src/common/lib/client/restchannel.ts
- src/common/lib/client/defaultrealtime.ts
- src/common/lib/client/restannotations.ts
- src/common/lib/transport/websockettransport.ts
🧰 Additional context used
🪛 GitHub Actions: Lint
ably.d.ts
[warning] Code style issues found. Needs to be formatted using Prettier.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-npm-package
- GitHub Check: test-node (16.x)
🔇 Additional comments (12)
src/common/lib/types/protocolmessagecommon.ts (1)
24-26: LGTM! The new actions and flags are well-defined.The implementation correctly:
- Adds new action constants with unique values
- Defines annotation flags using non-overlapping bit shifts
- Updates MODE_ALL to include all channel modes
- Maintains consistency between flags and channelModes array
Also applies to: 46-47, 52-58, 60-67
src/common/lib/client/defaultrest.ts (1)
9-11: LGTM! Annotation support properly integrated.The changes correctly integrate annotation support by:
- Importing necessary annotation types
- Adding a well-structured Annotations object to the constructor options
Also applies to: 31-36
src/common/lib/client/realtimeannotations.ts (3)
80-84: 🛠️ Refactor suggestionAdd error handling for _processIncoming.
The method should handle potential errors during event emission to prevent crashes.
Apply this diff:
_processIncoming(annotations: Annotation[]): void { for (const annotation of annotations) { - this.subscriptions.emit(annotation.refType || '', annotation); + try { + this.subscriptions.emit(annotation.refType || '', annotation); + } catch (error) { + this.logger.error('Error processing annotation:', error); + } } }Likely invalid or redundant comment.
21-48: 🛠️ Refactor suggestionEnhance error handling in publish method.
Consider adding input validation and error handling:
- Validate refSerial and refType parameters
- Handle potential errors during annotation encoding
Apply this diff:
async publish(refSerial: string, refType: string, data: any): Promise<void> { + if (!refSerial || !refType) { + throw new ErrorInfo('refSerial and refType are required', 40000, 400); + } + const channelName = this.channel.name; const annotation = Annotation.fromValues({ action: 'annotation.create', refSerial, refType, data, }); // TODO get rid of CipherOptions type assertion, indicates channeloptions types are broken - const wireAnnotation = await annotation.encode(this.channel.channelOptions as CipherOptions); + let wireAnnotation; + try { + wireAnnotation = await annotation.encode(this.channel.channelOptions as CipherOptions); + } catch (e) { + throw new ErrorInfo('Failed to encode annotation: ' + e.message, 40001, 400); + }Likely invalid or redundant comment.
30-31: Address TODO about CipherOptions type assertion.The type assertion indicates a misalignment between ChannelOptions and CipherOptions types that should be resolved.
Run this script to analyze the type definitions:
✅ Verification successful
Type assertion is an architectural bridge, requires broader refactoring
The type assertion from
ChannelOptionstoCipherOptionsis consistently used across the codebase as a bridge between Ably's type system and internal implementation. While the TODO is valid, resolving it would require significant refactoring of the type system. Consider creating a separate ticket for this technical debt.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find channel options type definitions ast-grep --pattern 'interface ChannelOptions { $$$ }' # Find CipherOptions type and encode method ast-grep --pattern 'type CipherOptions = $_' ast-grep --pattern 'interface CipherOptions { $$$ }'Length of output: 1789
Script:
#!/bin/bash # Find encode method implementation and CipherParam types ast-grep --pattern 'encode($_) { $$$ }' # Find CipherParam related types ast-grep --pattern 'interface CipherParamOptions { $$$ }' ast-grep --pattern 'interface CipherParams { $$$ }' # Find where CipherOptions is used with encode rg "encode.*CipherOptions" -A 2Length of output: 4783
src/common/lib/types/annotation.ts (1)
103-114: 🛠️ Refactor suggestionImprove error handling in decode method.
The decode method catches errors but doesn't propagate them, which could hide issues.
Apply this diff:
async decode(channelOptions: ChannelOptions, logger: Logger): Promise<Annotation> { const res = Object.assign(new Annotation(), { ...this, action: actions[this.action!], }); try { await decode(res, channelOptions); } catch (e) { Logger.logAction(logger, Logger.LOG_ERROR, 'WireAnnotation.decode()', Utils.inspectError(e)); + throw new ErrorInfo('Failed to decode annotation: ' + e.message, 40002, 400); } return res; }Likely invalid or redundant comment.
src/common/lib/types/protocolmessage.ts (1)
145-147: LGTM! Improved null safety in getMode method.The method now safely handles undefined flags by providing a default value of 0.
src/common/lib/client/baseclient.ts (1)
15-15: LGTM! Clean plugin integration.The changes follow the established plugin pattern in the codebase, with proper initialization in the constructor.
Also applies to: 50-50, 103-103
src/common/lib/client/realtimechannel.ts (3)
61-61: LGTM! Well-structured annotation support.The annotation property and getter follow the established pattern used for presence, with proper error handling for missing plugins.
Also applies to: 68-73
107-109: LGTM! Clean plugin initialization.The annotation plugin initialization in the constructor is consistent with other plugins.
655-668: LGTM! Robust annotation message handling.The annotation message processing is well integrated into the existing message handling flow, with proper decoding and error handling.
ably.d.ts (1)
869-876: LGTM! Clear channel mode definitions.The new annotation channel modes are well-defined and follow the existing pattern.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
SimonWoolf
force-pushed
the
support-annotations
branch
from
eeaf7ff to
14c9963
Compare
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis pull request introduces a comprehensive annotation system to the Ably client library. The changes span multiple files and add new functionality for publishing, subscribing to, and managing message annotations. The implementation includes new interfaces, classes, and methods that allow clients to attach metadata or reactions to messages, with support for both real-time and REST channels. The changes modify type definitions, transport layers, and core client components to fully integrate annotation capabilities. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Channel
participant AnnotationsPlugin
participant Transport
Client->>Channel: publish annotation
Channel->>AnnotationsPlugin: process annotation
AnnotationsPlugin->>Transport: encode and send annotation
Transport-->>Channel: confirm transmission
Channel-->>Client: annotation published
Client->>Channel: subscribe to annotations
Channel->>AnnotationsPlugin: register listener
AnnotationsPlugin->>Transport: listen for incoming annotations
Transport->>AnnotationsPlugin: receive annotation
AnnotationsPlugin->>Client: trigger annotation event
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
ably.d.ts (1)
Line range hint
858-888: Format the code using Prettier to resolve style issuesThe pipeline failures indicate that the file has code style issues that need to be fixed using Prettier. Please format the code to adhere to the project's style guidelines.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. File needs to be formatted using Prettier.
♻️ Duplicate comments (2)
src/common/lib/types/annotation.ts (1)
104-114: 🛠️ Refactor suggestionPropagate Errors in
WireAnnotation.decodeMethodThe
decodemethod catches errors during decoding but does not propagate them. This could make it difficult for callers to handle decoding failures appropriately.Consider rethrowing the error after logging it to ensure that calling functions are aware of the failure and can handle it as needed.
src/common/lib/types/protocolmessage.ts (1)
132-135: 🛠️ Refactor suggestionInitialize
annotationsandparamsProperties inProtocolMessageThe
annotationsandparamsproperties are currently optional and may beundefined, which can lead to runtime errors if accessed without checking forundefined.Initialize these properties to default values to ensure they are always defined:
class ProtocolMessage { // Existing properties... - annotations?: WireAnnotation[]; + annotations: WireAnnotation[] = []; - params?: Record<string, string>; + params: Record<string, string> = {};
🧹 Nitpick comments (10)
src/common/lib/client/realtimepresence.ts (2)
191-193: Consider improving the error message for better user experience.The error message "Unable to leave presence channel (incompatible state)" could be more helpful by explicitly mentioning why the state is incompatible and what the user should do next.
- throw new PartialErrorInfo('Unable to leave presence channel (incompatible state)', 90001); + throw new PartialErrorInfo( + 'Unable to leave presence channel as it is not attached to the server. The channel must be attached before presence updates can be sent.', + 90001 + );
354-357: Consider explicit success case handling in promise chain.While the error handling is good, the success case could be more explicit. Currently, multicaster is called with no arguments on success, which works but could be clearer.
this.channel .sendPresence(presenceArray) - .then(() => multicaster()) + .then(() => multicaster(null)) // Explicitly pass null to indicate success .catch((err: ErrorInfo) => multicaster(err));src/common/lib/types/annotation.ts (1)
67-70: Ensure ValidactionValue inencodeMethodIn the
encodemethod,this.actionis used to find the index in theactionsarray. Ifthis.actionis not a valid action string within theactionsarray,actions.indexOfwill return-1, which may lead to incorrect behavior.Consider adding validation to ensure that
this.actionis a valid action before encoding.src/common/lib/client/realtimechannel.ts (1)
68-73: Add documentation for theannotationsgetterThe
get annotations()method lacks documentation comments. Providing documentation will help users understand how to use the annotations feature and its importance within theRealtimeChannelclass.ably.d.ts (1)
2093-2096: Document theannotationsproperty inChannelinterfaceThe
annotationsproperty in theChannelinterface lacks documentation. Adding descriptive comments will improve clarity and assist users in understanding how to utilize this property.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. File needs to be formatted using Prettier.
src/common/lib/types/protocolmessagecommon.ts (1)
60-67: Update documentation to reflect new channel modesNew channel modes
ANNOTATION_PUBLISHandANNOTATION_SUBSCRIBEhave been added to thechannelModesarray. Ensure that all relevant documentation and code comments are updated to include these new modes.src/common/lib/client/modularplugins.ts (1)
24-29: Consider adding JSDoc documentation for theAnnotationsPlugintype.The type definition is well-structured but would benefit from documentation explaining its purpose and usage.
Add documentation:
+/** + * Plugin interface for annotation functionality. + * Provides types and classes for managing message annotations + * in both realtime and REST contexts. + */ export type AnnotationsPlugin = { Annotation: typeof Annotation; WireAnnotation: typeof WireAnnotation; RealtimeAnnotations: typeof RealtimeAnnotations; RestAnnotations: typeof RestAnnotations; };src/common/lib/client/restchannel.ts (1)
36-42: LGTM! Consider enhancing the error message.The implementation of the annotations property follows good practices with proper encapsulation. However, the error message could be more descriptive.
Consider enhancing the error message to guide users:
- Utils.throwMissingPluginError('Annotations'); + Utils.throwMissingPluginError('Annotations', 'To use annotations, you need to include the Annotations plugin in your client options.');src/common/lib/transport/websockettransport.ts (1)
143-143: Consider enhancing error handling for malformed annotations.While the addition of
_Annotationsto deserializeProtocolMessage is correct, consider adding specific error handling for malformed annotations.The current try-catch block will catch all errors, but specific handling of annotation deserialization errors would improve debugging:
try { this.onProtocolMessage( deserializeProtocolMessage( data, this.connectionManager.realtime._MsgPack, this.connectionManager.realtime._RealtimePresence, this.connectionManager.realtime._Annotations, this.format, ), ); } catch (e) { + if ((e as Error).message.includes('annotation')) { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'WebSocketTransport.onWsData()', + 'Failed to deserialize annotation: ' + (e as Error).stack, + ); + return; + } Logger.logAction( this.logger, Logger.LOG_ERROR, 'WebSocketTransport.onWsData()', 'Unexpected exception handing channel message: ' + (e as Error).stack, ); }src/common/lib/transport/comettransport.ts (1)
356-360: Consider architectural implications of coupling transport with annotations.The transport layer now depends on the realtime client's annotation system. This could violate separation of concerns as the transport layer should ideally be agnostic to higher-level message features.
Consider alternatives:
- Inject the annotations handler through the constructor
- Use a message transformer pattern to handle annotations separately
- Move annotation handling to a higher layer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ably.d.ts(8 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/defaultrest.ts(2 hunks)src/common/lib/client/modularplugins.ts(2 hunks)src/common/lib/client/realtimeannotations.ts(1 hunks)src/common/lib/client/realtimechannel.ts(11 hunks)src/common/lib/client/realtimepresence.ts(3 hunks)src/common/lib/client/restannotations.ts(1 hunks)src/common/lib/client/restchannel.ts(3 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/protocol.ts(1 hunks)src/common/lib/transport/websockettransport.ts(1 hunks)src/common/lib/types/annotation.ts(1 hunks)src/common/lib/types/protocolmessage.ts(5 hunks)src/common/lib/types/protocolmessagecommon.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
ably.d.ts
[warning] Code style issues found. File needs to be formatted using Prettier.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (18)
src/common/lib/client/realtimepresence.ts (2)
130-130: Good simplification of promise handling!The direct return of
channel.sendPresenceremoves unnecessary promise wrapping while maintaining the same error handling behavior.
Line range hint
1-1: Consider adding tests for promise-based flows.Given the significant refactoring of promise-based operations in this file, it would be beneficial to add tests that specifically cover:
- Promise resolution/rejection flows in presence operations
- Error handling in different channel states
- Edge cases in the multicaster usage
I notice from the PR objectives that tests are pending due to server-side development. Please ensure these scenarios are covered when implementing the tests.
🧰 Tools
🪛 Biome (1.9.4)
[error] 132-133: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
src/common/lib/types/protocolmessage.ts (1)
145-147: Verify Impact of Changing Return Type ingetModeMethodThe
getModemethod's return type has been changed to always return anumber, defaulting to0ifthis.flagsisundefined. This may affect any code that relies on the previous behavior wheregetModecould returnundefined.Verify that all callers of
getModehandle the new return type correctly and that this change does not introduce unintended side effects.src/common/lib/client/realtimechannel.ts (1)
480-489: Update allsendMessagecallers to handle PromisesWith
sendMessagenow returning a Promise, ensure that all callers of this method are correctly handling the Promise, either withawait,then/catch, or appropriate error handling to prevent unhandled Promise rejections.Run the following script to identify all usages of
sendMessage:✅ Verification successful
All
sendMessagecallers properly handle PromisesAll usages of
sendMessagedemonstrate appropriate Promise handling:
- Promise propagation through direct returns
- Explicit error handling with
.catch(noop)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'sendMessage' in the codebase to verify proper Promise handling. rg 'sendMessage\(' -A 2Length of output: 1197
ably.d.ts (4)
1992-2045: Add missing documentation forRealtimeAnnotationsinterface and methodsThe
RealtimeAnnotationsinterface and its methods, such assubscribeandunsubscribe, are missing documentation comments. This omission is contributing to warnings in the pipeline failures. Please add appropriate documentation to resolve these warnings.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. File needs to be formatted using Prettier.
2141-2155: Add missing documentation forRestAnnotationsinterfaceThe
RestAnnotationsinterface is missing documentation. This absence is generating warnings in the pipeline failures. Please include comprehensive documentation to resolve these warnings.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. File needs to be formatted using Prettier.
2463-2500: Provide documentation for theAnnotationinterfaceThe
Annotationinterface lacks documentation comments. Adding the necessary documentation will address the warnings in the pipeline failures and enhance code readability.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. File needs to be formatted using Prettier.
2556-2573: Add missing documentation forAnnotationActionsandAnnotationActionThe
AnnotationActionsnamespace and theAnnotationActiontype need TSDoc comments to resolve the documentation warnings indicated in the pipeline failures.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. File needs to be formatted using Prettier.
src/common/lib/types/protocolmessagecommon.ts (2)
24-26: Verify uniqueness and sequencing of new action constantsThe action constants
STATE,STATE_SYNC, andANNOTATIONhave been added with values19,20, and21, respectively. Please ensure that these values are unique and do not conflict with existing action constants.
52-58: Confirm correct inclusion of new flags inMODE_ALLThe flags
ANNOTATION_PUBLISHandANNOTATION_SUBSCRIBEhave been added toflags.MODE_ALL. Verify that all intended flags are included correctly and thatMODE_ALLrepresents the combination of all mode flags without unintended side effects.src/common/lib/client/restannotations.ts (1)
8-14: Add JSDoc documentation for theRestAnnotationsclass.The class lacks documentation describing its purpose and usage.
src/common/lib/client/modularplugins.ts (1)
36-36: LGTM! Clean integration of annotations into ModularPlugins.The optional annotation support is well-integrated into the existing plugin system.
src/common/lib/client/defaultrest.ts (1)
31-36: LGTM! Clean integration of the Annotations plugin.The Annotations plugin is properly integrated into the DefaultRest constructor.
src/common/lib/client/defaultrealtime.ts (1)
44-49: LGTM! Consistent implementation with DefaultRest.The Annotations plugin integration mirrors the approach used in DefaultRest, maintaining consistency across the codebase.
src/common/lib/transport/protocol.ts (1)
24-24: LGTM! Verify annotation action handling across transport layers.The addition of
actions.ANNOTATIONtoackRequiredensures reliable delivery of annotations. This aligns with how MESSAGE and PRESENCE actions are handled.Let's verify that annotation actions are handled consistently across transport layers:
✅ Verification successful
Annotation handling verified across transport layers ✓
The implementation ensures reliable delivery of annotations through consistent handling in WebSocket, Comet, and XHR transport layers, following the same acknowledgment pattern as messages and presence updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for annotation action handling in transport-related files rg -l "actions.ANNOTATION" "src/common/lib/transport/"Length of output: 89
Script:
#!/bin/bash # Find actions definition and broader annotation handling echo "=== Searching for actions definition ===" rg -l "actions\s*=|export.*actions" src/ echo -e "\n=== Searching for annotation patterns in transport ===" rg -i "annotation" "src/common/lib/transport/" -A 3 -B 3 echo -e "\n=== Looking for transport layer implementations ===" fd "transport" src/ --type fLength of output: 2557
src/common/lib/client/restchannel.ts (1)
53-55: LGTM! Plugin initialization looks good.The conditional initialization of annotations in the constructor is clean and follows the same pattern as other plugins.
src/common/lib/client/baseclient.ts (1)
15-15: LGTM! Plugin integration is well-structured.The implementation follows the established pattern for plugin integration, with proper TypeScript modifiers and null handling.
Also applies to: 50-50, 103-103
src/common/lib/transport/comettransport.ts (1)
356-360: Consider adding type safety for the annotations parameter.The
_Annotationsproperty lacks type information. Consider adding type assertions or interface definitions to ensure type safety.Run this script to check for type definitions:
| // TODO get rid of CipherOptions type assertion, indicates channeloptions types are broken | ||
| const wireAnnotation = await annotation.encode(this.channel.channelOptions as CipherOptions); |
There was a problem hiding this comment.
💡 Codebase verification
Type assertion as CipherOptions is incorrect and needs to be fixed
The type assertion is forcing channelOptions (type: ChannelOptions) into CipherOptions, but these types have incompatible structures:
ChannelOptionshas an optionalcipher?: CipherParamOptions | CipherParamsCipherOptionsexpects a different structure withchannelCipherproperty
This mismatch could lead to runtime errors. The type definitions need to be aligned or the code needs to be updated to handle the correct types.
🔗 Analysis chain
Address the TODO comment regarding type assertion.
The type assertion as CipherOptions indicates a potential type safety issue that needs to be resolved.
Let's verify the type definitions with this script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CipherOptions usage and RestChannel type definitions
ast-grep --pattern 'interface RestChannel {
$$$
channelOptions: $_;
$$$
}'
ast-grep --pattern 'type CipherOptions = {
$$$
}'
Length of output: 801
Script:
#!/bin/bash
# Search for RestChannel and channelOptions definitions
ast-grep --pattern 'class RestChannel {
$$$
}'
ast-grep --pattern 'interface ChannelOptions {
$$$
}'
# Search for files importing CipherOptions
rg "import.*CipherOptions" -l
Length of output: 10913
Script:
#!/bin/bash
# Search for normaliseChannelOptions implementation
ast-grep --pattern 'function normaliseChannelOptions($$$)'
# Search for CipherParamOptions and CipherParams types
ast-grep --pattern 'type CipherParamOptions = {
$$$
}'
ast-grep --pattern 'interface CipherParams {
$$$
}'
Length of output: 1791
| protocolMessageFromDeserialized( | ||
| items[i], | ||
| this.connectionManager.realtime._RealtimePresence, | ||
| this.connectionManager.realtime._Annotations, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks for the annotations property.
The code assumes this.connectionManager.realtime._Annotations exists. Add null checks to handle cases where the annotations system isn't initialized.
this.onProtocolMessage(
protocolMessageFromDeserialized(
items[i],
this.connectionManager.realtime._RealtimePresence,
- this.connectionManager.realtime._Annotations,
+ this.connectionManager.realtime._Annotations ?? null,
),
);Committable suggestion skipped: line range outside the PR's diff.
SimonWoolf
force-pushed
the
support-annotations
branch
from
14c9963 to
e608ab3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/common/lib/types/annotation.ts (1)
109-112: 🛠️ Refactor suggestionPropagate the error after logging in
decodemethodThe
decodemethod inWireAnnotationcatches errors during decoding but doesn't propagate them. To inform the caller about the failure, consider rethrowing the error after logging it.Apply this diff to propagate the error:
await decode(res, channelOptions); } catch (e) { Logger.logAction(logger, Logger.LOG_ERROR, 'WireAnnotation.decode()', Utils.inspectError(e)); + throw e; } return res;src/common/lib/client/restannotations.ts (2)
15-43: 🛠️ Refactor suggestionAdd error handling in
publishmethodThe
publishmethod should handle potential errors that could occur during annotation encoding and network requests. Consider adding proper error handling to inform callers of any failures.Consider wrapping the code in a try-catch block and adding input validation. Here's a proposed change:
async publish(refSerial: string, refType: string, data: any): Promise<void> { + if (!refSerial || !refType) { + throw new Error('refSerial and refType are required parameters'); + } + const annotation = Annotation.fromValues({ action: 'annotation.create', refSerial, refType, data, }); + try { const wireAnnotation = await annotation.encode(this.channel.channelOptions as CipherOptions); const client = this.channel.client, options = client.options, format = options.useBinaryProtocol ? Utils.Format.msgpack : Utils.Format.json, headers = Defaults.defaultPostHeaders(client.options, { format }), params = {}; const requestBody = Utils.encodeBody([wireAnnotation], client._MsgPack, format); await Resource.post( client, client.rest.channelMixin.basePath(this.channel) + '/annotations', requestBody, headers, params, null, true, ); + } catch (error) { + throw new Error(`Failed to publish annotation: ${error.message}`); + } }
23-24:⚠️ Potential issueResolve the type assertion issue with
CipherOptionsThe type assertion
as CipherOptionssuggests a potential type mismatch. ThechannelOptionsmay not be compatible withCipherOptions, and forcing the type could lead to runtime errors. Please address the TODO comment by aligning the type definitions or updating the code to handle the correct types.ably.d.ts (4)
1992-1996: 🛠️ Refactor suggestionAdd TSDoc comment for the
RealtimeAnnotationsinterface.The interface is missing documentation. Please add a TSDoc comment to resolve the pipeline warning.
+/** + * Enables realtime annotation functionality for a channel, allowing clients to + * subscribe to and publish annotations such as emoji reactions. + */ export declare interface RealtimeAnnotations {🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
2141-2145: 🛠️ Refactor suggestionAdd TSDoc comment for the
RestAnnotationsinterface.The interface is missing documentation. Please add a TSDoc comment to resolve the pipeline warning.
+/** + * Enables REST annotation functionality for a channel, allowing clients to + * publish annotations such as emoji reactions. + */ export declare interface RestAnnotations {🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
2463-2466: 🛠️ Refactor suggestionAdd TSDoc comment for the
Annotationinterface.The interface is missing documentation. Please add a TSDoc comment to resolve the pipeline warning.
+/** + * Contains an individual annotation that is sent to, or received from, Ably. + * Annotations are used to attach metadata like emoji reactions to messages. + */ export interface Annotation {🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
2556-2573: 🛠️ Refactor suggestionAdd TSDoc comments for annotation actions.
The namespace and type need TSDoc comments. Please add documentation to resolve the pipeline warnings.
+/** + * The namespace containing the different types of annotation actions. + */ declare namespace AnnotationActions { /** * Annotation action for a created annotation. */ type ANNOTATION_CREATE = 'annotation.create'; /** * Annotation action for a deleted annotation. */ type ANNOTATION_DELETE = 'annotation.delete'; } +/** + * Describes the possible action types used on an {@link Annotation}. + */ export type AnnotationAction = AnnotationActions.ANNOTATION_CREATE | AnnotationActions.ANNOTATION_DELETE;🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
🧹 Nitpick comments (5)
src/common/lib/client/realtimeannotations.ts (3)
15-19: Add parameter validation in constructor.Consider adding null check for the
channelparameter to prevent potential runtime errors.constructor(channel: RealtimeChannel) { + if (!channel) { + throw new Error('Channel parameter is required'); + } this.channel = channel; this.logger = channel.logger; this.subscriptions = new EventEmitter(this.logger); }
50-71: Enhance error handling and add JSDoc documentation.Consider adding JSDoc documentation and improving error handling for better maintainability.
+/** + * Subscribes to annotation events on the channel. + * @param {...unknown[]} _args - Arguments array containing [refType] and listener + * @throws {ErrorInfo} If channel is in failed state or annotation_subscribe mode is not set + * @returns {Promise<void>} + */ async subscribe(..._args: unknown[] /* [refType], listener */): Promise<void> { const args = RealtimeChannel.processListenerArgs(_args); const event = args[0]; const listener = args[1]; + if (typeof listener !== 'function') { + throw new ErrorInfo('Listener must be a function', 40012, 400); + } // ... rest of the method }
73-78: Add error handling for unsubscribe operation.Consider adding error handling to gracefully handle invalid arguments.
unsubscribe(..._args: unknown[] /* [event], listener */): void { + try { const args = RealtimeChannel.processListenerArgs(_args); const event = args[0]; const listener = args[1]; this.subscriptions.off(event, listener); + } catch (error) { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'RealtimeAnnotations.unsubscribe()', + 'Error unsubscribing: ' + error.message + ); + throw error; + } }src/common/lib/client/restchannel.ts (1)
Line range hint
37-55: Enhance error message in annotations getter.Consider providing a more descriptive error message that guides users on how to enable the Annotations plugin.
get annotations(): RestAnnotations { if (!this._annotations) { - Utils.throwMissingPluginError('Annotations'); + Utils.throwMissingPluginError( + 'Annotations', + 'To use annotations, ensure the Annotations plugin is included in the client options.' + ); } return this._annotations; }src/common/lib/types/protocolmessage.ts (1)
145-147: Add JSDoc documentation for getMode method.Consider adding documentation to explain the return value and its significance.
+/** + * Gets the channel mode flags from the protocol message. + * @returns {number} The channel mode flags, or 0 if no flags are set. + */ getMode(): number { return (this.flags || 0) & flags.MODE_ALL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ably.d.ts(8 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/defaultrest.ts(2 hunks)src/common/lib/client/modularplugins.ts(2 hunks)src/common/lib/client/realtimeannotations.ts(1 hunks)src/common/lib/client/realtimechannel.ts(11 hunks)src/common/lib/client/restannotations.ts(1 hunks)src/common/lib/client/restchannel.ts(3 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/protocol.ts(1 hunks)src/common/lib/transport/websockettransport.ts(1 hunks)src/common/lib/types/annotation.ts(1 hunks)src/common/lib/types/basemessage.ts(1 hunks)src/common/lib/types/protocolmessage.ts(5 hunks)src/common/lib/types/protocolmessagecommon.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/common/lib/transport/protocol.ts
- src/common/lib/client/defaultrest.ts
- src/common/lib/transport/comettransport.ts
- src/common/lib/client/defaultrealtime.ts
- src/common/lib/client/modularplugins.ts
- src/common/lib/transport/websockettransport.ts
- src/common/lib/types/protocolmessagecommon.ts
🧰 Additional context used
🪛 GitHub Actions: Lint
ably.d.ts
[warning] Code style issues found. Needs to be formatted using Prettier.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (15)
src/common/lib/client/restannotations.ts (1)
8-14: Add missing documentation forRestAnnotationsclass andpublishmethodThe
RestAnnotationsclass and itspublishmethod lack documentation comments. Please add comprehensive documentation to describe the purpose, parameters, and usage of the class and method.src/common/lib/types/basemessage.ts (1)
239-241: Integration ofactions.ANNOTATIONlooks goodThe addition of the
actions.ANNOTATIONcase inpopulateFieldsFromParentcorrectly integrates annotations into the message handling process.src/common/lib/client/realtimeannotations.ts (3)
80-84: 🛠️ Refactor suggestionAdd error handling and type safety to _processIncoming.
The method should handle potential errors during event emission and ensure type safety.
_processIncoming(annotations: Annotation[]): void { + if (!Array.isArray(annotations)) { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'RealtimeAnnotations._processIncoming()', + 'Expected array of annotations' + ); + return; + } for (const annotation of annotations) { - this.subscriptions.emit(annotation.refType || '', annotation); + try { + const refType = annotation.refType || ''; + this.subscriptions.emit(refType, annotation); + } catch (error) { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'RealtimeAnnotations._processIncoming()', + 'Error processing annotation: ' + error.message + ); + } } }Likely invalid or redundant comment.
21-48: 🛠️ Refactor suggestionAdd input validation and error handling.
The method should validate input parameters and handle potential errors from
sendMessage.async publish(refSerial: string, refType: string, data: any): Promise<void> { + if (!refSerial || typeof refSerial !== 'string') { + throw new ErrorInfo('refSerial must be a non-empty string', 40012, 400); + } + if (!refType || typeof refType !== 'string') { + throw new ErrorInfo('refType must be a non-empty string', 40012, 400); + } const channelName = this.channel.name; // ... rest of the method ... - return this.channel.sendMessage(pm); + try { + await this.channel.sendMessage(pm); + } catch (error) { + Logger.logAction( + this.logger, + Logger.LOG_ERROR, + 'RealtimeAnnotations.publish()', + 'Error publishing annotation: ' + error.message + ); + throw error; + } }Likely invalid or redundant comment.
30-31: Resolve type assertion in encode method.The TODO comment indicates a type system issue that should be addressed. Consider defining proper type interfaces for channel options to eliminate the need for type assertion.
Let's analyze the type definitions:
✅ Verification successful
Type assertion is necessary for encryption functionality
The type assertion from
ChannelOptionstoCipherOptionsis required because the encode method needs access to cipher properties for encryption operations. While the TODO comment suggests improving the type definitions, the current type assertion is a valid workaround.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CipherOptions and ChannelOptions type definitions rg --type typescript "type.*CipherOptions|interface.*CipherOptions" -A 5 rg --type typescript "type.*ChannelOptions|interface.*ChannelOptions" -A 5Length of output: 221
Script:
#!/bin/bash # Search for type definitions and imports rg "CipherOptions|ChannelOptions" --type-add 'ts:*.{ts,tsx,d.ts}' --type ts -A 3 # Search for specific usage patterns ast-grep --pattern 'interface $_ extends CipherOptions' ast-grep --pattern 'type $_ = CipherOptions'Length of output: 32488
src/common/lib/client/restchannel.ts (1)
20-20: LGTM!The import and property declaration are properly typed and follow the codebase conventions.
Also applies to: 36-36
src/common/lib/types/protocolmessage.ts (1)
4-11: LGTM!The imports are well-organized and the type declarations are appropriate.
ably.d.ts (1)
869-876: LGTM! Channel modes for annotations are well-defined.The new channel modes
ANNOTATION_PUBLISHandANNOTATION_SUBSCRIBEare properly documented and correctly integrated into the type system.Also applies to: 886-888
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code style issues found. Needs to be formatted using Prettier.
src/common/lib/client/baseclient.ts (1)
15-15: LGTM! Plugin integration follows established patterns.The changes follow the consistent pattern used for other plugins in the codebase:
- Import the plugin type
- Add a readonly property for the plugin instance
- Initialize from options in the constructor
Also applies to: 50-50, 103-103
src/common/lib/client/realtimechannel.ts (6)
61-73: LGTM! Property and getter follow established patterns.The implementation of
_annotationsproperty and its getter follows the same pattern as other plugin-based features, with proper null checks and error handling.
107-109: LGTM! Constructor initialization follows plugin pattern.The initialization of annotations in the constructor follows the established pattern for plugin features.
480-489: LGTM! Promise-based implementation improves usability.The Promise-based implementation of
sendMessageprovides a more modern and easier-to-use API while maintaining backward compatibility through the callback inconnectionManager.send.
276-281: LGTM! Good error handling separation.Extracting the state validation into
_throwIfUnpublishableStateimproves code organization and reusability.
660-673: LGTM! Annotation handling follows presence message pattern.The implementation of annotation message handling follows the established pattern used for presence messages:
- Populates fields from parent
- Decodes messages using channel options
- Processes only if plugin is available
Line range hint
1-1: Consider adding tests when server-side functionality is ready.As mentioned in the PR description, tests are pending due to server-side functionality being under development. Please ensure to add comprehensive tests covering:
- Annotation plugin initialization
- Message sending and receiving
- Error handling scenarios
- Edge cases (e.g., plugin not available)
SimonWoolf
force-pushed
the
support-annotations
branch
from
e608ab3 to
586fe0b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/common/lib/client/restannotations.ts (1)
8-14: Add JSDoc documentation for the RestAnnotations class.The class lacks documentation describing its purpose, responsibilities, and usage examples.
Add documentation:
+/** + * RestAnnotations provides functionality for publishing annotations to a REST channel. + * It allows clients to attach metadata or reactions to existing messages. + */ class RestAnnotations { private channel: RestChannel; constructor(channel: RestChannel) { this.channel = channel; }src/common/lib/types/protocolmessage.ts (1)
135-138: Initialize new properties in ProtocolMessage class.The new properties should be properly initialized to prevent potential undefined errors.
Apply this diff:
class ProtocolMessage { // ... existing properties - annotations?: WireAnnotation[]; - params?: Record<string, string>; + annotations: WireAnnotation[] = []; + params: Record<string, string> = {};🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
ably.d.ts(8 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/defaultrest.ts(2 hunks)src/common/lib/client/modularplugins.ts(2 hunks)src/common/lib/client/realtimeannotations.ts(1 hunks)src/common/lib/client/realtimechannel.ts(11 hunks)src/common/lib/client/restannotations.ts(1 hunks)src/common/lib/client/restchannel.ts(3 hunks)src/common/lib/transport/comettransport.ts(1 hunks)src/common/lib/transport/connectionmanager.ts(1 hunks)src/common/lib/transport/protocol.ts(2 hunks)src/common/lib/transport/transport.ts(1 hunks)src/common/lib/transport/websockettransport.ts(1 hunks)src/common/lib/types/annotation.ts(1 hunks)src/common/lib/types/basemessage.ts(1 hunks)src/common/lib/types/protocolmessage.ts(6 hunks)src/common/lib/types/protocolmessagecommon.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/common/lib/transport/transport.ts
- src/common/lib/transport/connectionmanager.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/common/lib/transport/protocol.ts
- src/common/lib/transport/websockettransport.ts
- src/common/lib/types/basemessage.ts
- src/common/lib/transport/comettransport.ts
- src/common/lib/client/defaultrealtime.ts
- src/common/lib/client/restchannel.ts
- src/common/lib/client/baseclient.ts
- src/common/lib/client/modularplugins.ts
- src/common/lib/types/protocolmessagecommon.ts
- src/common/lib/client/defaultrest.ts
👮 Files not reviewed due to content moderation or server errors (2)
- src/common/lib/client/realtimechannel.ts
- ably.d.ts
🧰 Additional context used
🪛 GitHub Actions: Lint
ably.d.ts
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
src/common/lib/types/protocolmessage.ts
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (7)
src/common/lib/client/restannotations.ts (2)
15-43: 🛠️ Refactor suggestionAdd input validation and error handling to the publish method.
The method should validate input parameters and handle potential errors during annotation publishing.
Apply this diff:
+ /** + * Publishes an annotation for a message. + * @param refSerial - The serial number of the message being annotated + * @param refType - The type of annotation (e.g., 'reaction', 'edit') + * @param data - The annotation data + * @throws {Error} If required parameters are missing or if publishing fails + */ async publish(refSerial: string, refType: string, data: any): Promise<void> { + if (!refSerial || !refType) { + throw new Error('refSerial and refType are required parameters'); + } + const annotation = Annotation.fromValues({ action: 'annotation.create', refSerial, refType, data, }); // TODO get rid of CipherOptions type assertion, indicates channeloptions types are broken const wireAnnotation = await annotation.encode(this.channel.channelOptions as CipherOptions); const client = this.channel.client, options = client.options, format = options.useBinaryProtocol ? Utils.Format.msgpack : Utils.Format.json, headers = Defaults.defaultPostHeaders(client.options, { format }), params = {}; const requestBody = Utils.encodeBody([wireAnnotation], client._MsgPack, format); + try { await Resource.post( client, client.rest.channelMixin.basePath(this.channel) + '/annotations', requestBody, headers, params, null, true, ); + } catch (error) { + throw new Error(`Failed to publish annotation: ${error.message}`); + } }Likely invalid or redundant comment.
23-24: Resolve the TODO regarding CipherOptions type assertion.The type assertion indicates a mismatch between
ChannelOptionsandCipherOptions.Let's verify the type definitions:
✅ Verification successful
Update the TODO comment to explain the type assertion instead of removing it
The type assertion is actually correct and necessary as the
encodemethod requiresCipherOptionsfor encryption. Instead of removing it, update the TODO comment to explain that the channel options are being used for encryption parameters:// Channel options are used as cipher options for message encryption const wireAnnotation = await annotation.encode(this.channel.channelOptions as CipherOptions);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CipherOptions and ChannelOptions type definitions ast-grep --pattern 'type CipherOptions = { $$$ }' ast-grep --pattern 'interface ChannelOptions { $$$ }'Length of output: 2098
Script:
#!/bin/bash # Search for CipherParamOptions and CipherParams definitions ast-grep --pattern 'type CipherParamOptions = { $$$ }' ast-grep --pattern 'interface CipherParams { $$$ }' # Search for encode method definition ast-grep --pattern 'encode($$$) { $$$ }'Length of output: 1775
Script:
#!/bin/bash # Search for annotation class with encode method ast-grep --pattern 'class $_ { $$$ encode($$$) { $$$ } $$$ }' # Also search for any type definitions related to the encode method ast-grep --pattern 'encode($param: $_)'Length of output: 117
Script:
#!/bin/bash # Search in the specific file cat src/common/lib/client/restannotations.ts # Search for any class/interface with Annotation in the name ast-grep --pattern 'class $_Annotation { $$$ }' # Search for async encode specifically ast-grep --pattern 'async encode($$$)'Length of output: 1504
Script:
#!/bin/bash # Look for Annotation class definition cat src/common/lib/types/annotation.ts # Also search for any related interfaces ast-grep --pattern 'interface $_Annotation { $$$ }'Length of output: 3818
src/common/lib/client/realtimeannotations.ts (2)
80-84: 🛠️ Refactor suggestionAdd error handling to _processIncoming method.
The method should handle potential errors during event emission to prevent crashes.
Apply this diff:
_processIncoming(annotations: Annotation[]): void { for (const annotation of annotations) { - this.subscriptions.emit(annotation.refType || '', annotation); + try { + this.subscriptions.emit(annotation.refType || '', annotation); + } catch (error) { + this.logger.error('Error processing annotation:', error); + } } }Likely invalid or redundant comment.
21-48: 🛠️ Refactor suggestionAdd input validation to publish method.
The method should validate input parameters before processing.
Apply this diff:
async publish(refSerial: string, refType: string, data: any): Promise<void> { + if (!refSerial || !refType) { + throw new Error('refSerial and refType are required parameters'); + } + const channelName = this.channel.name; const annotation = Annotation.fromValues({ action: 'annotation.create', refSerial, refType, data, });Likely invalid or redundant comment.
src/common/lib/types/annotation.ts (1)
105-116: 🛠️ Refactor suggestionImprove error handling in decode method.
The method catches errors but doesn't propagate them, which could hide issues.
Apply this diff:
async decode(channelOptions: ChannelOptions, logger: Logger): Promise<Annotation> { const res = Object.assign(new Annotation(), { ...this, action: actions[this.action!], }); try { await decode(res, channelOptions); } catch (e) { Logger.logAction(logger, Logger.LOG_ERROR, 'WireAnnotation.decode()', Utils.inspectError(e)); + throw e; } return res; }Likely invalid or redundant comment.
src/common/lib/types/protocolmessage.ts (2)
60-65: 🛠️ Refactor suggestionAdd error handling for annotation deserialization.
The code should handle potential errors during annotation deserialization.
Apply this diff:
let annotations: WireAnnotation[] | undefined; if (annotationsPlugin && deserialized.annotations) { + try { annotations = annotationsPlugin.WireAnnotation.fromValuesArray( deserialized.annotations as Array<Properties<WireAnnotation>>, ); + } catch (error) { + Logger.logAction( + Logger.LOG_ERROR, + 'ProtocolMessage.fromDeserialized', + 'Error deserializing annotations: ' + error.message + ); + } }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
Line range hint
1-164: Fix code formatting issues.The pipeline failures indicate formatting issues that need to be addressed.
Run Prettier to fix the formatting:
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
SimonWoolf
force-pushed
the
support-annotations
branch
from
586fe0b to
cee74c7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
SimonWoolf
force-pushed
the
support-annotations
branch
from
6052fb0 to
99c7cca
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
SimonWoolf
force-pushed
the
support-annotations
branch
from
99c7cca to
efa5805
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
SimonWoolf
force-pushed
the
support-annotations
branch
from
efa5805 to
609cc1a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
SimonWoolf
force-pushed
the
support-annotations
branch
from
609cc1a to
c4c893a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1953/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1953/features
github-actions
bot
temporarily deployed
to
staging/pull/1953/typedoc
SimonWoolf
force-pushed
the
support-annotations
branch
from
c4c893a to
5df9aef
Compare
|
@lawrence-forooghian Now that annotation support is present in sandbox I've added tests: https://github.com/ably/ably-js/pull/1953/files#diff-fc7d8e7f44ab3e6adf257469c8103149faab63c5baf235b21db77a1ad1eaab68 |
Draft PR -- feature-complete, but no tests yet as haven't finished the server side of the functionality (doing the clientside alongside that for serverside e2e tests). Putting up now to get feedback on API design and whether I've done the modularisation correctly.
(...having said it's a draft pr I apparently failed to press the "draft PR" button)
Summary by CodeRabbit
Release Notes: Annotations Support
New Features
Improvements
Technical Enhancements