Implement message updates and deletes #2088
Conversation
WalkthroughAdds message lifecycle APIs (getMessage, updateMessage, deleteMessage, getMessageVersions) to Channel/RealtimeChannel with REST mixin implementations and tests; refactors Defaults header helpers and call sites to stop passing per-call format (token requests still request JSON); adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant App as Client App
participant Channel as Channel (Realtime/Rest)
participant Mixin as RestChannelMixin
participant HTTP as HTTP layer
participant Service as Ably Service
Dev->>App: call getMessage/updateMessage/deleteMessage/getMessageVersions
App->>Channel: invoke channel method
Channel->>Mixin: delegate with channel context
rect rgba(200,220,255,0.12)
note right of Mixin: validate serial, serialize/encode, build headers via Defaults
end
alt get / versions
Mixin->>HTTP: GET /channels/{name}/messages/{serial} or /versions
else update
Mixin->>HTTP: PATCH /channels/{name}/messages
else delete
Mixin->>HTTP: POST /channels/{name}/messages/delete
end
HTTP->>Service: request
Service-->>HTTP: response (encoded)
HTTP-->>Mixin: payload
Mixin-->>Channel: decoded Message or PaginatedResult
Channel-->>App: Promise resolves with result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/2088/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: 6
🧹 Nitpick comments (2)
.gitignore (1)
14-14: Ignore all.claudeartifactsUse a glob to ignore variants like
.claude.json.-.claude +.claude*src/common/lib/client/restchannelmixin.ts (1)
118-131: Optional: DRY encoding/patch body constructionupdateMessage/deleteMessage duplicate header/body building and encode logic; consider a small helper to build headers and patch body.
Also applies to: 175-188
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)ably.d.ts(3 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/restchannel.ts(1 hunks)src/common/lib/client/restchannelmixin.ts(2 hunks)test/rest/updates-deletes.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/rest/updates-deletes.test.js (3)
src/common/lib/client/realtimechannel.ts (1)
updateMessage(1026-1034)src/common/lib/client/restchannel.ts (1)
updateMessage(159-166)src/common/lib/client/restchannelmixin.ts (1)
updateMessage(91-146)
src/common/lib/client/restchannel.ts (2)
ably.d.ts (2)
Message(3256-3314)MessageOperation(3439-3452)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/restchannelmixin.ts (5)
src/common/lib/types/message.ts (3)
WireMessage(158-210)_fromEncoded(72-75)_fromEncodedArray(77-83)src/common/lib/util/utils.ts (1)
Properties(479-479)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/realtimechannel.ts (2)
ably.d.ts (2)
Message(3256-3314)MessageOperation(3439-3452)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
ably.d.ts (2)
test/rest/updates-deletes.test.js (2)
operation(115-119)operation(168-172)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (4)
src/common/lib/client/realtimechannel.ts (1)
1020-1053: LGTM: Realtime message ops delegate cleanly to RestChannelMixinLogging, typings, and delegation look consistent with existing patterns (e.g., history/status).
src/common/lib/client/restchannel.ts (1)
154-183: LGTM: REST message ops mirror mixin, consistent logging and signaturesMethods align with typings and delegate correctly.
ably.d.ts (1)
3436-3452: LGTM: MessageOperation typeSimple, useful metadata container; matches implementation.
src/common/lib/client/restchannelmixin.ts (1)
97-103: Bug: ErrorInfo arguments reversed (statusCode set to 40003)Same issue for updateMessage.
- throw new ErrorInfo( - 'This message lacks a serial and cannot be updated. Make sure you have enabled "Message annotations, updates, and deletes" in channel settings on your dashboard.', - 400, - 40003, - ); + throw new ErrorInfo( + 'This message lacks a serial and cannot be updated. Make sure you have enabled "Message annotations, updates, and deletes" in channel settings on your dashboard.', + 40003, + 400, + );Likely an incorrect or invalid review comment.
SimonWoolf
force-pushed
the
updaet-delete
branch
from
fd67c45 to
e228930
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/typedoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/common/lib/client/rest.ts (1)
195-196: Use GET headers for a GET requestbatchPresence uses Resource.get but builds POST-style headers. Prefer defaultGetHeaders to avoid sending an unnecessary Content-Type.
Apply:
- headers = Defaults.defaultPostHeaders(this.client.options); + headers = Defaults.defaultGetHeaders(this.client.options);src/common/lib/client/push.ts (1)
236-237: Remove explicit format override for consistencyThese two removeWhere methods still pass { format } to defaultGetHeaders. Align with the new pattern across this PR.
Apply:
- headers = Defaults.defaultGetHeaders(client.options, { format }); + headers = Defaults.defaultGetHeaders(client.options);Also applies to: 306-307
test/rest/updates-deletes.test.js (1)
110-113: Avoid variable name shadowing for clarityUsing a local const named updateMessage alongside channel.updateMessage is confusing. Rename the payload variables.
- const updateMessage = { + const updatePayload = { serial: originalMessage.serial, data: { value: 'updated with metadata' }, }; @@ - const updatedMessage = await channel.updateMessage(updateMessage, operation); + const updatedMessage = await channel.updateMessage(updatePayload, operation);- const updateMessage = { + const updatePayload = { serial: originalMessage.serial, data: { value: 'version-2' }, }; - await channel.updateMessage(updateMessage); + await channel.updateMessage(updatePayload);Also applies to: 121-121, 223-227
src/common/lib/client/restchannelmixin.ts (1)
147-193: Consider deduplication between updateMessage and deleteMessage.The
deleteMessagemethod has nearly identical implementation toupdateMessageexcept for the endpoint path. Consider extracting shared logic to reduce duplication.Consider refactoring to reduce duplication:
private static async _updateOrDeleteMessage( channel: RestChannel | RealtimeChannel, message: Message, operation?: API.MessageOperation, params?: Record<string, any>, isDelete: boolean = false ): Promise<Message> { if (!message.serial) { const action = isDelete ? 'deleted' : 'updated'; throw new ErrorInfo( `This message lacks a serial and cannot be ${action}. Make sure you have enabled "Message annotations, updates, and deletes" in channel settings on your dashboard.`, 40003, 400, ); } const client = channel.client; const format = client.options.useBinaryProtocol ? Utils.Format.msgpack : Utils.Format.json; const headers = Defaults.defaultPostHeaders(client.options); Utils.mixin(headers, client.options.headers); let encoded: WireMessage | null = null; if (message.data !== undefined) { encoded = await Message.fromValues(message).encode(channel.channelOptions as CipherOptions); } const body: UpdateDeleteRequest = { serial: message.serial, operation: operation, name: message.name, data: encoded && encoded.data, encoding: encoded && encoded.encoding, extras: message.extras, }; const requestBody: RequestBody = serializeMessage(body, client._MsgPack, format); const path = this.basePath(channel) + '/messages/' + encodeURIComponent(message.serial) + (isDelete ? '/delete' : ''); const response = await (isDelete ? Resource.post<WireMessage>(client, path, requestBody, headers, params || {}, null, true) : Resource.patch<WireMessage>(client, path, requestBody, headers, params || {}, null, true) ); return _fromEncoded(response.body as Utils.Properties<WireMessage>, channel); } static async updateMessage( channel: RestChannel | RealtimeChannel, message: Message, operation?: API.MessageOperation, params?: Record<string, any>, ): Promise<Message> { return this._updateOrDeleteMessage(channel, message, operation, params, false); } static async deleteMessage( channel: RestChannel | RealtimeChannel, message: Message, operation?: API.MessageOperation, params?: Record<string, any>, ): Promise<Message> { return this._updateOrDeleteMessage(channel, message, operation, params, true); }
📜 Review details
Configuration used: CodeRabbit UI
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 (14)
.gitignore(1 hunks)ably.d.ts(3 hunks)src/common/lib/client/push.ts(8 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/rest.ts(3 hunks)src/common/lib/client/restannotations.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(5 hunks)src/common/lib/client/restpresence.ts(1 hunks)src/common/lib/client/restpresencemixin.ts(1 hunks)src/common/lib/util/defaults.ts(2 hunks)src/plugins/push/pushactivation.ts(2 hunks)src/plugins/push/pushchannel.ts(4 hunks)test/rest/updates-deletes.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/lib/client/realtimechannel.ts
- src/common/lib/client/restchannel.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/common/lib/client/restchannelmixin.ts (5)
ably.d.ts (2)
MessageOperation(3439-3452)Message(3256-3314)src/common/lib/types/message.ts (3)
WireMessage(158-210)_fromEncoded(72-75)_fromEncodedArray(77-83)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
test/rest/updates-deletes.test.js (3)
src/common/lib/client/restchannel.ts (1)
updateMessage(159-166)src/common/lib/client/restchannelmixin.ts (1)
updateMessage(99-145)src/common/lib/client/realtimechannel.ts (1)
updateMessage(1026-1034)
ably.d.ts (2)
test/rest/updates-deletes.test.js (2)
operation(115-119)operation(168-172)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/util/defaults.ts (1)
test/realtime/objects.test.js (1)
Utils(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (21)
src/plugins/push/pushactivation.ts (2)
79-79: Headers: removal of explicit format is correctAligns with Defaults inferring Accept/Content‑Type from options. No behavior change.
304-304: Delete flow: header change looks goodUsing defaultPostHeaders here is consistent with other DELETE paths in this codebase.
src/common/lib/client/restpresence.ts (1)
24-24: Presence.get: header construction LGTMSwapping to Defaults.defaultGetHeaders(options) matches the new defaults API; envelope handling still driven by format.
src/common/lib/client/restpresencemixin.ts (1)
21-21: Presence.history: header construction LGTMConsistent with updated Defaults helpers. No functional change.
src/plugins/push/pushchannel.ts (1)
19-20: PushChannel: header updates LGTMAll four call sites correctly rely on Defaults inferring format from options.
Also applies to: 33-34, 58-58, 74-74
src/common/lib/client/rest.ts (1)
173-174: Batch publish/revoke tokens: header updates LGTMdefaultPostHeaders(options) is appropriate for POST requests.
Also applies to: 227-227
src/common/lib/client/push.ts (1)
104-104: Push admin/device/channel subs: header updates LGTMConsistently using Defaults without explicit format matches the refactor; decode still governed by computed format.
Also applies to: 127-128, 155-156, 189-189, 207-207, 258-258, 287-287, 323-323
src/common/lib/client/restannotations.ts (1)
79-79: Annotations: header updates LGTMdefaultPostHeaders/defaultGetHeaders now infer format; request/response encoding remains correct.
Also applies to: 110-110
test/rest/updates-deletes.test.js (1)
268-274: Fix assertions: check error code 40003 (not message text)Tests should assert on the ErrorInfo code thrown by the implementation.
- } catch (err) { - expect(err.message).to.include('serial is required'); + } catch (err) { + expect(err).to.have.property('code', 40003); }- } catch (err) { - expect(err.message).to.include('serial is required'); + } catch (err) { + expect(err).to.have.property('code', 40003); }Also applies to: 284-290
src/common/lib/util/defaults.ts (2)
413-415: LGTM!The updated function signature properly handles optional parameters with appropriate defaults.
423-428: Consistent parameter handling across header methods.The changes correctly implement fallback logic for the
formatparameter using the nullish coalescing operator, ensuring binary vs. JSON format selection based on protocol preferences when no format is explicitly provided.src/common/lib/client/restchannelmixin.ts (4)
74-97: LGTM!The
getMessageimplementation correctly validates the serial, constructs appropriate headers, and performs the GET request to retrieve a message by its serial.
99-145: LGTM!The
updateMessageimplementation correctly:
- Validates the message serial
- Conditionally encodes message data only when defined
- Constructs the PATCH request body with proper fields
- Returns the updated message
195-229: LGTM!The
getMessageVersionsimplementation correctly validates the serial, creates a PaginatedResource with proper envelope handling, and returns paginated message versions.
74-83: Verify missing-serial error handling across message APIsSandbox search failed; confirm all message operations (getMessage, updateMessage, deleteMessage, etc.) throw ErrorInfo(code: 40003, status: 400) when serial is missing and document the dashboard requirement "Message annotations, updates, and deletes". Location: src/common/lib/client/restchannelmixin.ts:74-83
ably.d.ts (6)
2836-2843: Documentation: Clarify patch semantics for null vs undefined.
3084-3091: Documentation: Clarify patch semantics for null vs undefined.
2845-2852: Documentation: Use correct MessageAction value.
3093-3100: Documentation: Use correct MessageAction value.
3438-3452: LGTM!The
MessageOperationinterface is well-structured with optional fields for tracking update/delete operation metadata.
2828-2863: 40003 is consistent for missing-serial errors in channel methods. restchannelmixin.ts throws ErrorInfo(code=40003) for getMessage/updateMessage/deleteMessage/getMessageVersions; realtime/rest channel methods delegate to that mixin and tests assert 40003.
SimonWoolf
force-pushed
the
updaet-delete
branch
from
e228930 to
c0c9ae6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/2088/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/lib/client/push.ts (1)
236-236: Inconsistent header construction in removeWhere methods.These two methods still pass
{ format }todefaultGetHeaders, which is inconsistent with the refactoring pattern applied throughout the rest of the codebase.Apply this diff to make the header construction consistent:
- headers = Defaults.defaultGetHeaders(client.options, { format }); + headers = Defaults.defaultGetHeaders(client.options);This should be applied to both
DeviceRegistrations.removeWhere(line 236) andChannelSubscriptions.removeWhere(line 306).Also applies to: 306-306
🧹 Nitpick comments (2)
ably.d.ts (2)
2854-2863: Optional: consider a named params type for getMessageVersions.Using
Record<string, any>is flexible but opaque. A named params interface (even if index-signature-based) improves discoverability in editors.
3102-3111: Optional: consider a named params type for getMessageVersions.Same suggestion as for the REST Channel variant to improve editor hints.
📜 Review details
Configuration used: CodeRabbit UI
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 (14)
.gitignore(1 hunks)ably.d.ts(3 hunks)src/common/lib/client/push.ts(8 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/rest.ts(3 hunks)src/common/lib/client/restannotations.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(5 hunks)src/common/lib/client/restpresence.ts(1 hunks)src/common/lib/client/restpresencemixin.ts(1 hunks)src/common/lib/util/defaults.ts(2 hunks)src/plugins/push/pushactivation.ts(2 hunks)src/plugins/push/pushchannel.ts(4 hunks)test/rest/updates-deletes.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plugins/push/pushactivation.ts
- src/common/lib/client/realtimechannel.ts
- src/common/lib/client/rest.ts
- src/common/lib/client/restpresence.ts
- src/common/lib/client/restannotations.ts
🧰 Additional context used
🧬 Code graph analysis (5)
ably.d.ts (2)
test/rest/updates-deletes.test.js (2)
operation(115-119)operation(168-172)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/util/defaults.ts (1)
test/realtime/objects.test.js (1)
Utils(12-12)
src/common/lib/client/restchannelmixin.ts (5)
ably.d.ts (2)
MessageOperation(3439-3452)Message(3256-3314)src/common/lib/types/message.ts (3)
WireMessage(158-210)_fromEncoded(72-75)_fromEncodedArray(77-83)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/restchannel.ts (2)
ably.d.ts (2)
Message(3256-3314)MessageOperation(3439-3452)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
test/rest/updates-deletes.test.js (2)
src/common/lib/client/restchannel.ts (1)
updateMessage(159-166)src/common/lib/client/realtimechannel.ts (1)
updateMessage(1026-1034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
🔇 Additional comments (23)
.gitignore (1)
14-14: LGTM!Adding
.claudeto the gitignore is appropriate for excluding IDE or AI assistant configuration files.src/common/lib/util/defaults.ts (2)
413-413: LGTM! Header construction improvements.The refactored header building logic properly centralizes format defaulting within the function parameters using destructuring with defaults. This eliminates the need to pass explicit
{ format }options throughout the codebase and relies on the internal logic to determine content-type based onuseBinaryProtocol.Also applies to: 425-425
415-415: Clean nullish coalescing for content-type determination.The use of nullish coalescing (
format ?? (options.useBinaryProtocol ? Utils.Format.msgpack : Utils.Format.json)) provides a clean fallback mechanism for determining the appropriate content-type when format isn't explicitly provided.Also applies to: 427-428
src/common/lib/client/restpresencemixin.ts (1)
21-21: Consistent header construction pattern.The removal of the explicit
{ format }parameter aligns with the broader refactoring across the codebase to centralize format determination within the defaults functions.src/plugins/push/pushchannel.ts (1)
19-19: Consistent header construction across push operations.All push channel operations now use the standardized header construction pattern without explicit format overrides. The surrounding logic for header assembly and request construction remains intact.
Also applies to: 33-33, 57-57, 73-73
src/common/lib/client/push.ts (1)
103-103: Consistent header construction pattern across push admin operations.The removal of explicit
{ format }parameters from header construction calls aligns with the broader refactoring. All push admin operations now consistently rely on the defaults logic to determine appropriate headers based on client options.Also applies to: 127-127, 155-155, 188-188, 207-207, 257-257, 286-286, 322-322
test/rest/updates-deletes.test.js (2)
1-308: Comprehensive test coverage for new message operations.This test suite provides excellent coverage for the new REST message operations including getMessage, updateMessage, deleteMessage, and getMessageVersions. The tests properly validate:
- Message retrieval by serial and Message object
- Update operations with metadata (version tracking, timestamps, clientId, description, metadata)
- Delete operations with proper action tracking
- Version history retrieval
- Appropriate error handling for missing serials
The test structure is well-organized with proper async/await usage and thorough assertions on message properties, version metadata, and actions.
256-256: Verify error assertions match the implementation.The previous review comment mentioned that these assertions should check for error code 40003 instead of string matching, and it appears they've been updated correctly to use
expect(err).to.have.property('code', 40003).Also applies to: 272-272, 288-288, 304-304
src/common/lib/client/restchannel.ts (2)
111-111: Consistent header construction pattern.The removal of explicit
{ format }parameter aligns with the refactoring pattern applied across the codebase.
154-183: New message lifecycle API methods correctly implemented.The new message operations (getMessage, updateMessage, deleteMessage, getMessageVersions) are properly implemented:
- Each method includes appropriate logging at the micro level
- Methods correctly delegate to the RestChannelMixin for actual implementation
- Type signatures match the public API expectations (string | Message for serialOrMessage parameters)
- updateMessage and deleteMessage properly differentiate via the
isDeleteflag in the delegationThe implementation follows the existing patterns in the codebase and provides a clean separation between the public API and the implementation details.
src/common/lib/client/restchannelmixin.ts (6)
3-3: New imports properly support message operations.The additional imports provide the necessary functionality for the new message operations:
- ErrorInfo for proper error handling
- Message encoding/decoding utilities from the message module
- CipherOptions for encryption support
- RequestBody type for HTTP request payloads
Also applies to: 6-7, 11-11
20-27: Well-designed UpdateDeleteRequest type.The UpdateDeleteRequest type properly models the payload structure for update/delete operations, including:
- Required serial for message identification
- Optional data, name, encoding, and extras fields for message content
- Optional operation metadata for tracking changes
The type design aligns with the REST API requirements and supports both update and delete operations.
41-41: Consistent header construction in existing methods.The header construction has been updated to remove explicit format parameters, maintaining consistency with the broader refactoring.
Also applies to: 60-60
74-100: getMessage implementation with proper validation and error handling.The getMessage method is well-implemented:
- Properly extracts serial from string or Message object
- Provides informative error messages for missing serials with correct error codes
- Uses appropriate REST endpoint pattern with URL encoding for the serial
- Correctly handles both packed and unpacked responses
- Properly decodes the response using the channel's configuration
The error message helpfully guides users to enable the feature in their dashboard settings.
102-152: updateDeleteMessage implementation handles both operations correctly.The unified method for update and delete operations is well-designed:
- Validates serial presence with appropriate error messages
- Conditionally encodes message data only when provided (undefined check)
- Builds proper UpdateDeleteRequest payload structure
- Correctly selects HTTP method (PATCH for updates, POST for deletes) and path suffixes
- Handles both packed and unpacked responses appropriately
- Uses proper URL encoding for the serial parameter
The implementation efficiently handles both update and delete operations through a single method with configuration.
154-188: getMessageVersions provides proper pagination support.The getMessageVersions method is correctly implemented:
- Validates serial parameter with consistent error handling
- Uses PaginatedResource for proper pagination support
- Constructs appropriate REST endpoint for versions
- Handles envelope logic for link header support
- Properly decodes arrays of WireMessage objects to Message arrays
- Supports optional query parameters
The method provides the expected paginated interface consistent with other history methods in the SDK.
ably.d.ts (7)
2828-2834: LGTM: getMessage API and docsAPI shape, overloading, and return type look good and consistent with Channel history semantics.
2836-2843: Docs: patch semantics should use “undefined means unchanged”, not “null”.Implementation treats undefined as “leave unchanged”; null is sent and overwrites. Please correct the docstring.
Apply this diff to fix the wording:
- * Publishes an update to an existing message with patch semantics. Non-null `name`, `data`, and `extras` fields in the provided message will replace the corresponding fields in the existing message, while null fields will be left unchanged. + * Publishes an update to an existing message with patch semantics. Provided (non-`undefined`) `name`, `data`, and `extras` fields replace the corresponding fields in the existing message; omitted (`undefined`) fields are left unchanged. To clear a field, set it explicitly (for example, `data: null`).
2845-2852: Docs: fix patch semantics and remove stray trailing ‘|’.Same null/undefined issue as above, plus a dangling pipe at the end of the paragraph.
- * Marks a message as deleted by publishing an update with an action of `MESSAGE_DELETE`. This does not remove the message from the server, and the full message history remains accessible. Uses patch semantics: non-null `name`, `data`, and `extras` fields in the provided message will replace the corresponding fields in the existing message, while null fields will be left unchanged (meaning that if you for example want the `MESSAGE_DELETE` to have an empty data, you should explicitly set the `data` to an empty object). | + * Marks a message as deleted by publishing an update with an action of `MESSAGE_DELETE`. This does not remove the message from the server; full message history remains accessible. Uses patch semantics: provided (non-`undefined`) `name`, `data`, and `extras` fields replace the corresponding fields; omitted (`undefined`) fields are left unchanged (for example, to have empty `data`, explicitly set `data` to an empty object).
3076-3082: LGTM: Realtime getMessage API mirrors REST variantSignature and docs are consistent with the REST Channel.
3084-3091: Docs: patch semantics should use “undefined means unchanged”, not “null”.Mirror the fix applied to the REST Channel method.
- * Publishes an update to an existing message with patch semantics. Non-null `name`, `data`, and `extras` fields in the provided message will replace the corresponding fields in the existing message, while null fields will be left unchanged. + * Publishes an update to an existing message with patch semantics. Provided (non-`undefined`) `name`, `data`, and `extras` fields replace the corresponding fields in the existing message; omitted (`undefined`) fields are left unchanged. To clear a field, set it explicitly (for example, `data: null`).
3093-3100: Docs: fix patch semantics and remove trailing ‘|’.Same corrections as the REST Channel delete method.
- * Marks a message as deleted by publishing an update with an action of `MESSAGE_DELETE`. This does not remove the message from the server, and the full message history remains accessible. Uses patch semantics: non-null `name`, `data`, and `extras` fields in the provided message will replace the corresponding fields in the existing message, while null fields will be left unchanged (meaning that if you for example want the `MESSAGE_DELETE` to have an empty data, you should explicitly set the `data` to an empty object). | + * Marks a message as deleted by publishing an update with an action of `MESSAGE_DELETE`. This does not remove the message from the server; full message history remains accessible. Uses patch semantics: provided (non-`undefined`) `name`, `data`, and `extras` fields replace the corresponding fields; omitted (`undefined`) fields are left unchanged (for example, to have empty `data`, explicitly set `data` to an empty object).
3436-3452: LGTM: MessageOperation type is minimal and fits testsFields align with usage in tests (
clientId,description,metadata). Export placement is sensible.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/lib/client/push.ts (1)
233-237: Force JSON when hitting push device endpoints.These endpoints reject msgpack bodies today. With the new defaults path, any client running with
useBinaryProtocol = truewill emitapplication/x-msgpack, triggering 415s. Keep passing{ format: Utils.Format.json }(or inline guard) forremoveWhere(and the other admin calls) so we always negotiate JSON here.
🧹 Nitpick comments (1)
src/common/lib/client/restchannelmixin.ts (1)
111-111: Fix error message consistency.The error message says "cannot be updated" but this function handles both updates and deletes. Consider making it more generic.
Apply this diff to make the error message more generic:
- 'This message lacks a serial and cannot be updated. Make sure you have enabled "Message annotations, updates, and deletes" in channel settings on your dashboard.', + 'This message lacks a serial and cannot be updated or deleted. Make sure you have enabled "Message annotations, updates, and deletes" in channel settings on your dashboard.',
📜 Review details
Configuration used: CodeRabbit UI
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 (14)
.gitignore(1 hunks)ably.d.ts(3 hunks)src/common/lib/client/push.ts(8 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/rest.ts(3 hunks)src/common/lib/client/restannotations.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(5 hunks)src/common/lib/client/restpresence.ts(1 hunks)src/common/lib/client/restpresencemixin.ts(1 hunks)src/common/lib/util/defaults.ts(2 hunks)src/plugins/push/pushactivation.ts(2 hunks)src/plugins/push/pushchannel.ts(4 hunks)test/rest/updates-deletes.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plugins/push/pushchannel.ts
- src/common/lib/client/restpresence.ts
- src/common/lib/client/rest.ts
- src/common/lib/client/restannotations.ts
- test/rest/updates-deletes.test.js
🧰 Additional context used
🧬 Code graph analysis (5)
ably.d.ts (2)
test/rest/updates-deletes.test.js (2)
operation(115-119)operation(168-172)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/restchannel.ts (2)
ably.d.ts (2)
Message(3256-3314)MessageOperation(3439-3452)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/util/defaults.ts (1)
test/realtime/objects.test.js (1)
Utils(12-12)
src/common/lib/client/restchannelmixin.ts (5)
ably.d.ts (2)
MessageOperation(3439-3452)Message(3256-3314)src/common/lib/types/message.ts (3)
WireMessage(158-210)_fromEncoded(72-75)_fromEncodedArray(77-83)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/realtimechannel.ts (2)
ably.d.ts (2)
Message(3256-3314)MessageOperation(3439-3452)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
🔇 Additional comments (19)
src/common/lib/client/restchannelmixin.ts (14)
3-3: LGTM!The addition of the ErrorInfo import is necessary for the new error handling in the message operations.
6-6: LGTM!The updated import adds necessary utilities for the new message operations including serialization and decoding functions.
7-7: LGTM!The CipherOptions import is needed for encoding message data in the updateDeleteMessage function.
11-11: LGTM!The RequestBody type import is needed for the serialized message request body in updateDeleteMessage.
20-27: LGTM!The UpdateDeleteRequest type definition is well-structured and captures all the necessary fields for message update and delete operations.
41-41: LGTM!The change to use Defaults.defaultGetHeaders(client.options) is consistent with the header handling modernization across the codebase.
60-60: LGTM!The change to use Defaults.defaultPostHeaders(client.options) is consistent with the header handling modernization.
74-82: LGTM!The ErrorInfo constructor arguments are now correctly ordered (code=40003, statusCode=400). The error message is clear and helpful for developers.
74-100: LGTM!The getMessage implementation is well-structured:
- Correctly extracts serial from string or Message parameter
- Uses appropriate format detection and header construction
- Handles both packed and unpacked response formats
- Returns a properly decoded Message object
109-115: LGTM!The ErrorInfo constructor arguments are correctly ordered and the error message is appropriate for the update operation context.
102-152: LGTM!The updateDeleteMessage implementation is robust:
- Validates message has a serial before proceeding
- Conditionally encodes message data only when present
- Correctly selects HTTP method and path based on operation type
- Uses proper serialization and response decoding
- Returns a properly decoded Message object
160-166: LGTM!The ErrorInfo constructor arguments are correctly ordered for the getMessageVersions operation.
154-188: LGTM!The getMessageVersions implementation is well-designed:
- Validates serial presence before proceeding
- Uses PaginatedResource for proper pagination handling
- Correctly decodes array responses using _fromEncodedArray
- Returns a properly typed PaginatedResult
123-125: Verify encoding behavior for null vs undefined message.dataCode encodes only when message.data !== undefined — decide whether data === null should be treated as an explicit payload (encoded) or as absence (skip). Message.fromValues/encode implementation isn't in the repo (only ably.d.ts); manually verify runtime behavior and add unit tests for data = undefined and data = null.
src/common/lib/client/restpresencemixin.ts (1)
21-21: Header defaults change looks good.Switching to the new default header inference keeps behaviour identical while reducing per-call duplication. 👍
src/plugins/push/pushactivation.ts (1)
257-268: EnsureAuthorizationheader keeps required format.By dropping the explicit
{ format }argument we rely on the defaults utility to infer msgpack vs json. For push registration we still need JSON when transport can’t handle msgpack (e.g. Safari service workers). Please double-check the downstream behaviour; ifclient.options.useBinaryProtocolis ever true here we’ll send msgpack to endpoints that only accept JSON. Consider forcingformat: Utils.Format.jsonfor these push admin endpoints.ably.d.ts (1)
3438-3451: Great addition.
MessageOperationcleanly captures the metadata we need to expose—thanks for threading this through the public API.src/common/lib/client/restchannel.ts (1)
154-183: Nice channel surface.The REST channel delegators are clean and match the mixin helpers—looks good to me.
src/common/lib/client/realtimechannel.ts (1)
1020-1053: Realtime wrappers look solid.Thanks for wiring the realtime surface straight through the mixin; logging and delegation match our other methods.
SimonWoolf
force-pushed
the
updaet-delete
branch
from
c0c9ae6 to
296bf47
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2088/typedoc
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/common/lib/client/push.ts (1)
236-237: Remove obsolete{ format }arg from Defaults.defaultGetHeaders and defaultPostHeadersSeveral calls still pass a second argument. Update to match the new single-arg signature:
- headers = Defaults.defaultGetHeaders(client.options, { format }); + headers = Defaults.defaultGetHeaders(client.options);- headers = Defaults.defaultPostHeaders(client.options, { format }); + headers = Defaults.defaultPostHeaders(client.options);Occurrences to update:
- src/common/lib/client/push.ts:236, 306
- src/common/lib/client/rest.ts:123-124
- src/plugins/push/getW3CDeviceDetails.ts:50
- src/plugins/push/pushactivation.ts:267, 575
Also applies to ChannelSubscriptions.removeWhere. After these changes, ensure any now-unused
format(andprotocolVersion, if present) variables aren’t flagged by lints.#!/bin/bash # Find remaining header-helper calls with a format object rg -nP "Defaults\.default(Get|Post)Headers\([^)]*,\s*\{" -g "src/**"src/common/lib/client/restchannelmixin.ts (1)
58-72: Use GET headers instatusmethod
Inrestchannelmixin.ts’sstatus(lines 58–72), you’re callingResource.getbut usingDefaults.defaultPostHeaders; all other GET calls usedefaultGetHeaders. Replace with:- const headers = Defaults.defaultPostHeaders(channel.client.options); + const headers = Defaults.defaultGetHeaders(channel.client.options); + Utils.mixin(headers, channel.client.options.headers);
♻️ Duplicate comments (1)
src/common/lib/client/restchannel.ts (1)
111-112: Headers regression mirrors known defaults concernSame as previously noted in defaults.ts: ensure header construction safely maps format and doesn’t silently drop unknowns. Referencing prior comment thread.
🧹 Nitpick comments (1)
src/common/lib/client/restchannelmixin.ts (1)
102-152: LGTM with optional refinement suggestion.The
updateDeleteMessageimplementation correctly validates, encodes, serializes, and routes the request based on operation type.Consider only including
nameandextrasin the request when they are defined:const req: UpdateDeleteRequest = { serial: message.serial, operation: operation, - name: message.name, data: encoded && encoded.data, encoding: encoded && encoded.encoding, - extras: message.extras, }; + +if (message.name !== undefined) { + req.name = message.name; +} +if (message.extras !== undefined) { + req.extras = message.extras; +}This reduces payload size and makes the API contract clearer.
📜 Review details
Configuration used: CodeRabbit UI
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 (14)
.gitignore(1 hunks)ably.d.ts(3 hunks)src/common/lib/client/push.ts(8 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/rest.ts(3 hunks)src/common/lib/client/restannotations.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(5 hunks)src/common/lib/client/restpresence.ts(1 hunks)src/common/lib/client/restpresencemixin.ts(1 hunks)src/common/lib/util/defaults.ts(2 hunks)src/plugins/push/pushactivation.ts(2 hunks)src/plugins/push/pushchannel.ts(4 hunks)test/rest/updates-deletes.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (7)
- src/plugins/push/pushchannel.ts
- src/common/lib/client/rest.ts
- src/common/lib/client/restpresence.ts
- test/rest/updates-deletes.test.js
- src/common/lib/client/restpresencemixin.ts
- src/common/lib/util/defaults.ts
- src/plugins/push/pushactivation.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/common/lib/client/realtimechannel.ts (2)
ably.d.ts (2)
Message(3264-3322)MessageOperation(3447-3460)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
ably.d.ts (2)
test/rest/updates-deletes.test.js (2)
operation(115-119)operation(168-172)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/restchannelmixin.ts (5)
ably.d.ts (2)
MessageOperation(3447-3460)Message(3264-3322)src/common/lib/types/message.ts (3)
WireMessage(176-228)_fromEncoded(72-75)_fromEncodedArray(77-83)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/restchannel.ts (2)
ably.d.ts (2)
Message(3264-3322)MessageOperation(3447-3460)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (10)
src/common/lib/client/restannotations.ts (2)
78-81: Header helper call updated — LGTMSwitching to Defaults.defaultPostHeaders(client.options) is consistent with the repo-wide change.
109-111: GET headers change — LGTMUsing Defaults.defaultGetHeaders(client.options) here aligns with envelope/format handling below.
src/common/lib/client/push.ts (1)
103-104: Header helper refactor — looks goodCalls now rely on Defaults.defaultPostHeaders/defaultGetHeaders(client.options) without per-call format. Consistent across Admin, DeviceRegistrations, and ChannelSubscriptions.
Also applies to: 127-128, 155-156, 188-189, 207-208, 257-258, 286-287, 322-323
src/common/lib/client/restchannel.ts (1)
154-183: New message lifecycle methods — LGTMgetMessage/updateMessage/deleteMessage/getMessageVersions delegate cleanly to channelMixin with micro-logging.
src/common/lib/client/realtimechannel.ts (1)
1020-1053: Realtime message lifecycle methods — LGTMStraightforward delegation to rest channelMixin; consistent with RestChannel.
If useful, add brief docs/comments noting these use REST under the hood (no attach requirement).
ably.d.ts (1)
3444-3461: MessageOperation interface — LGTMFields and placement look appropriate and match tests’ usage.
src/common/lib/client/restchannelmixin.ts (4)
20-27: LGTM!The
UpdateDeleteRequesttype definition is well-structured and aligns with the requirements for update/delete operations.
34-56: LGTM!The refactoring to use
Defaults.defaultGetHeaderscentralizes header generation and improves consistency across the codebase.
74-100: LGTM!The
getMessageimplementation correctly validates the serial, uses appropriate headers, and properly decodes the response. Error handling is clear and actionable.
154-188: LGTM!The
getMessageVersionsimplementation correctly validates the serial, sets up pagination, and decodes the response array. The structure mirrors thehistorymethod appropriately.
SimonWoolf
force-pushed
the
updaet-delete
branch
from
296bf47 to
7a311c5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
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 (11)
src/common/lib/client/auth.ts(1 hunks)src/common/lib/client/push.ts(8 hunks)src/common/lib/client/rest.ts(4 hunks)src/common/lib/client/restannotations.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(5 hunks)src/common/lib/client/restpresence.ts(1 hunks)src/common/lib/client/restpresencemixin.ts(1 hunks)src/common/lib/util/defaults.ts(2 hunks)src/plugins/push/pushactivation.ts(2 hunks)src/plugins/push/pushchannel.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/common/lib/client/restpresence.ts
- src/common/lib/client/push.ts
- src/common/lib/client/restpresencemixin.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/common/lib/client/restchannel.ts (2)
ably.d.ts (2)
Message(3264-3322)MessageOperation(3447-3460)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
src/common/lib/client/restchannelmixin.ts (4)
ably.d.ts (2)
MessageOperation(3447-3460)Message(3264-3322)src/common/lib/types/message.ts (2)
WireMessage(176-228)_fromEncoded(72-75)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)
src/common/lib/util/defaults.ts (1)
test/realtime/objects.test.js (1)
Utils(12-12)
🪛 GitHub Actions: API Reference
src/common/lib/client/auth.ts
[error] 591-591: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Bundle size report
src/common/lib/client/auth.ts
[error] 591-591: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Lint
src/common/lib/client/auth.ts
[error] 591-591: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Spec coverage report
src/common/lib/client/auth.ts
[error] 591-591: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Test (react hooks)
src/common/lib/client/auth.ts
[error] 591-591: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Test browser
src/common/lib/client/auth.ts
[error] 591-591: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Test NodeJS
src/common/lib/client/auth.ts
[error] 591-86: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-76: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
🪛 GitHub Actions: Test NPM package
src/common/lib/client/auth.ts
[error] 591-591: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
src/common/lib/client/rest.ts
[error] 73-73: TS2322: Type '"json"' is not assignable to type 'Format | "xml" | "html" | "text" | undefined'.
SimonWoolf
force-pushed
the
updaet-delete
branch
from
7a311c5 to
64a5cfc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
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 (11)
src/common/lib/client/auth.ts(1 hunks)src/common/lib/client/push.ts(8 hunks)src/common/lib/client/rest.ts(4 hunks)src/common/lib/client/restannotations.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(5 hunks)src/common/lib/client/restpresence.ts(1 hunks)src/common/lib/client/restpresencemixin.ts(1 hunks)src/common/lib/util/defaults.ts(2 hunks)src/plugins/push/pushactivation.ts(2 hunks)src/plugins/push/pushchannel.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/common/lib/client/restpresence.ts
- src/common/lib/client/rest.ts
- src/plugins/push/pushactivation.ts
- src/common/lib/client/auth.ts
- src/plugins/push/pushchannel.ts
- src/common/lib/client/push.ts
- src/common/lib/client/restchannel.ts
- src/common/lib/client/restpresencemixin.ts
- src/common/lib/util/defaults.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/lib/client/restchannelmixin.ts (5)
ably.d.ts (2)
MessageOperation(3447-3460)Message(3264-3322)src/common/lib/types/message.ts (3)
WireMessage(176-228)_fromEncoded(72-75)_fromEncodedArray(77-83)src/common/lib/types/basemessage.ts (1)
CipherOptions(13-24)src/common/types/http.ts (1)
RequestBody(26-29)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (9)
src/common/lib/client/restannotations.ts (2)
101-126: LGTM: Header helper refactored to determine format internally.
Defaults.defaultGetHeadersnow defaultsformatviaoptions.useBinaryProtocolwhen none is passed, matching the local computation (line 107) and decoding logic (line 120).
71-94: LGTM:defaultPostHeadersanddefaultGetHeadersnow derive format internally fromoptions.useBinaryProtocol, so the explicitformatparameter is no longer needed.src/common/lib/client/restchannelmixin.ts (7)
3-11: LGTM! Clean import additions.All new imports are properly used in the implementation: ErrorInfo for validation errors, Message utilities for encoding/decoding, CipherOptions for encryption context, and RequestBody for type safety.
20-27: LGTM! Well-defined request structure.The UpdateDeleteRequest type correctly models the wire format for update/delete operations with appropriate optionality and types matching the WireMessage and MessageOperation interfaces.
34-56: LGTM! Header helper refactoring applied.The change to use
Defaults.defaultGetHeaders(client.options)without passing format directly aligns with the broader refactoring described in the AI summary while maintaining the same functionality.
58-72: LGTM! Consistent header refactoring.The status method follows the same header helper refactoring pattern as history, using
defaultPostHeadersconsistently.
74-100: LGTM! Well-structured message retrieval.The implementation correctly:
- Validates serial presence with appropriate ErrorInfo (arguments now in correct order per past reviews)
- Follows established patterns for format detection and header construction
- Uses
envelope=nullfor single-resource retrieval (appropriate since this isn't a collection)- Handles both packed and unpacked responses with proper decoding
117-152: LGTM! Solid update/delete implementation.The method correctly:
- Conditionally encodes message data only when present
- Builds a well-structured UpdateDeleteRequest matching the type definition
- Selects the appropriate HTTP method (PATCH for update, POST for delete) and path suffix
- Handles serialization, request execution, and response decoding consistently with getMessage
154-188: LGTM! Proper paginated resource implementation.The method correctly:
- Validates serial presence with appropriate error handling
- Uses envelope configuration consistent with the history method (undefined or format based on supportsLinkHeaders)
- Creates a PaginatedResource with proper path construction
- Handles both packed and unpacked array responses with
_fromEncodedArray
SimonWoolf
force-pushed
the
updaet-delete
branch
from
64a5cfc to
1e56fdc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
rather than calculate it the same way every time in every call site
SimonWoolf
force-pushed
the
updaet-delete
branch
from
1e56fdc to
786ef5e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/bundle-report
some language servers use this to determine proj sources
SimonWoolf
force-pushed
the
updaet-delete
branch
from
786ef5e to
10ae1c7
Compare
Spec at ably/specification#380
API DR at https://ably.atlassian.net/wiki/spaces/ENG/pages/3830349850/ADR-127+Pubsub+api+for+mutable+messages+annotations
Summary by CodeRabbit
New Features
Tests
Chores