Refactor: Updated message API to match PV4 changes in pubsub. #620
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMigrates chat model and REST API to Protocol V4: replaces message.createdAt with message.timestamp, replaces primitive/Operation-based versioning with a structured Version object (serial, timestamp, clientId, description, metadata), updates REST types/parsing/mapping and chat API endpoints to v4, adds annotations support, and updates demo, tests, and test helpers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Demo
participant Hooks as Hooks / Messages module
participant API as Chat REST (v4)
participant Mapper as messageFromRest
participant Model as DefaultMessage
rect rgba(230,245,255,0.9)
note over UI,API: Send / Update / Delete (Protocol V4)
UI->>Hooks: send/update/delete(payload)
Hooks->>API: REST v4 request (uses endpoint)
API-->>Hooks: RestMessage { version:{serial,timestamp,clientId,...}, timestamp, ... }
Hooks->>Mapper: messageFromRest(RestMessage)
Mapper->>Model: new DefaultMessage({ version: Version, timestamp: Date, ... })
Model-->>Hooks: Message (version: Version, timestamp: Date)
Hooks-->>UI: Message
end
note right of Model: version.serial used for ordering\nannotations read from message.annotations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
splindsay-92
force-pushed
the
chat/move-to-protocol-v4
branch
from
ff5799a to
a45f435
Compare
|
Something to consider. I know PR is still a a draft but I'll forget. The
(Also: can be deferred to later, since we still have the |
splindsay-92
force-pushed
the
chat/move-to-protocol-v4
branch
2 times, most recently
from
a074ea2 to
b65c4c9
Compare
splindsay-92
force-pushed
the
chat/move-to-protocol-v4
branch
from
b65c4c9 to
7c18632
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/typedoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/core/chat-api.ts (1)
119-119: Fix protocol/version mismatch (tests 404).Endpoints use /chat/v4 but _apiProtocolVersion is 3. Set to 4 to align and fix 404s.
Apply:
- private readonly _apiProtocolVersion: number = 3; + private readonly _apiProtocolVersion: number = 4;test/core/message-parser.test.ts (3)
287-305: Align “created” fixture to PV4 shape; remove createdAt, use version object.Created events should not include createdAt; use timestamp for creation and set version to the same time.
Apply:
extras: { headers: { headerKey: 'headerValue' }, }, - createdAt: 1728402074206, - timestamp: 1728402074207, - version: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', + timestamp: 1728402074206, + version: { + serial: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', + timestamp: 1728402074206, + }, action: ChatMessageAction.MessageCreate, serial: '01728402074206-000@cbfkKvEYgBhDaZ38195418:0',And expectations:
- expect(result.timestamp).toEqual(new Date(1728402074206)); + expect(result.timestamp).toEqual(new Date(1728402074206)); ... - expect(result.version.serial).toBe('01728402074207-000@cbfkKvEYgBhDaZ38195418:0'); - expect(result.version.timestamp).toEqual(new Date(1728402074207)); + expect(result.version.serial).toBe('01728402074207-000@cbfkKvEYgBhDaZ38195418:0'); + expect(result.version.timestamp).toEqual(new Date(1728402074206));
327-350: Align “updated” fixture: createdAt removed; top-level timestamp = creation; version carries update details.Update to v4 by moving operation into version and fixing timestamps.
Apply:
action: ChatMessageAction.MessageUpdate, serial: '01728402074206-000@cbfkKvEYgBhDaZ38195418:0', - createdAt: 1728402074206, - timestamp: 1728402074207, - version: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', - operation: { clientId: 'client2', description: 'update message', metadata: { 'custom-update': 'some flag' } }, + timestamp: 1728402074206, + version: { + serial: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', + timestamp: 1728402074207, + clientId: 'client2', + description: 'update message', + metadata: { 'custom-update': 'some flag' }, + },And assertions:
- expect(result.timestamp).toEqual(new Date(1728402074206)); + expect(result.timestamp).toEqual(new Date(1728402074206)); expect(result.updatedAt).toEqual(new Date(1728402074207));
366-392: Align “deleted” fixture to v4: remove createdAt; move operation into version.Top-level timestamp = creation; version holds delete timestamp and actor/details.
Apply:
- timestamp: 1728402074207, - createdAt: 1728402074206, - version: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', - operation: { - clientId: 'client2', - description: 'delete message', - metadata: { 'custom-warning': 'this is a warning' }, - }, + timestamp: 1728402074206, + version: { + serial: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', + timestamp: 1728402074207, + clientId: 'client2', + description: 'delete message', + metadata: { 'custom-warning': 'this is a warning' }, + },And assertions remain consistent (timestamp = creation; deletedAt = version.timestamp).
test/core/message.test.ts (1)
579-581: Wrong event type for DELETE scenarioApplying a DELETE should emit ChatMessageEventType.Deleted, not Updated. This makes the test assert the wrong contract.
Apply this diff:
- const event: ChatMessageEvent = { - type: ChatMessageEventType.Updated, - message: eventMessage, - }; + const event: ChatMessageEvent = { + type: ChatMessageEventType.Deleted, + message: eventMessage, + };test/core/messages.test.ts (1)
1109-1112: Remove legacy fields (createdAt, operation) — PV4 no longer sends themProtocol V4 removed createdAt and operation. Keeping them in inbound fixtures risks masking parser issues and diverges from the new contract.
Apply this diff in both places:
- timestamp: Date.now(), - createdAt: Date.now(), - operation: { clientId: 'yoda' }, + timestamp: Date.now(),Also applies to: 1143-1145
♻️ Duplicate comments (2)
test/core/message-parser.test.ts (2)
405-432: Do the same v4 adjustments for this delete test.Apply the same changes as above: remove createdAt, set top-level timestamp to creation, and move operation → version.
446-473: Do the same v4 adjustments for this delete-with-empty-data test.Apply the same changes as above: remove createdAt, set top-level timestamp to creation, and move operation → version.
🧹 Nitpick comments (14)
src/core/message.ts (4)
33-58: Widen Version docs; annotate spec.Version also represents the initial creation state, not only updates/deletes. Add a brief note and annotate the spec point.
Apply:
-export interface Version { - /** - * A unique identifier for the latest version of this message. - */ +// @CHA-1101 +export interface Version { + /** + * A unique identifier for the latest version of this message (including creation). + */
450-464: Consider addressing TODO: newer summaries vs newer versions.if (this.version.serial >= message.version.serial) return this; ignores the case where reactions summary is newer than the current instance. Optional: compare a summary watermark if available to avoid dropping newer summaries.
420-454: Use ErrorCodes enum instead of magic numbers.Replace 40000 with ErrorCodes.* per project guidelines and document why this code applies.
Apply:
+import { ErrorCodes } from './errors.js'; ... -throw new Ably.ErrorInfo('cannot apply a created event to a message', 40000, 400); +throw new Ably.ErrorInfo('cannot apply a created event to a message', ErrorCodes.InvalidArgument, 400); ... -throw new Ably.ErrorInfo('cannot apply event for a different message', 40000, 400); +throw new Ably.ErrorInfo('cannot apply event for a different message', ErrorCodes.InvalidArgument, 400);If ErrorCodes names differ, I can adjust once you confirm the enum entries.
300-303: Annotate spec points in changed fields.Add @CHA-1101 near version/timestamp usage and cloning to make the PV4 rationale discoverable.
Apply:
export interface DefaultMessageParams { serial: string; clientId: string; text: string; metadata: MessageMetadata; headers: MessageHeaders; action: ChatMessageAction; + // @CHA-1101 version: Version; - timestamp: Date; + // @CHA-1101 + timestamp: Date; reactions: MessageReactions; } ... - this.version = version; + // @CHA-1101 + this.version = version; ... - version: replace?.version ?? cloneDeep(source.version), + // @CHA-1101 + version: replace?.version ?? cloneDeep(source.version),Also applies to: 317-319, 475-478
src/core/chat-api.ts (1)
116-125: Add JSDoc for public API methods.Per guidelines, document params/returns/throws on public methods (history/getMessage/send/update/delete/react). Also add trace-level logs with context at method entry.
Example:
export class ChatApi { @@ - async history(roomName: string, params: HistoryQueryParams): Promise<PaginatedResult<Message>> { + /** + * Fetch room message history. + * @param roomName Room name (will be URL-encoded). + * @param params History query filters. + * @returns PaginatedResult of mapped {@link Message}. + * @throws {@link Ably.ErrorInfo} on transport or server error. + */ + async history(roomName: string, params: HistoryQueryParams): Promise<PaginatedResult<Message>> { + this._logger.trace('ChatApi.history()', { roomName, hasFromSerial: Boolean(params.fromSerial) });test/core/message-parser.test.ts (2)
1-7: Mock ably in tests per guidelines.Add a module mock to avoid accidental runtime coupling.
Apply:
import * as Ably from 'ably'; import { describe, expect, it } from 'vitest'; +vi.mock('ably');
8-14: Annotate tests with spec point.Add // @CHA-1101 on the describe for PV4 message shape.
Apply:
-describe('parseMessage', () => { +describe('parseMessage', () => { + // @CHA-1101test/core/message.test.ts (3)
689-696: Test name doesn’t match assertionsThe description says “return original message if newer message is older”, but assertions expect replacement with the newer message. Rename for clarity.
Apply this diff:
- it('should return original message if newer message is older', () => { + it('should replace original when incoming message has a newer version', () => {
18-24: De-duplicate version object constructionYou repeat the same version literal (with description/metadata undefined) many times. Use a small helper to reduce noise and keep shapes consistent.
Example:
+// helper to keep Version construction consistent across tests +const makeVersion = (serial: string, tsMs: number, clientId?: string) => ({ + serial, + timestamp: new Date(tsMs), + clientId, + description: undefined, + metadata: undefined, +});And then:
- version: { - serial: firstSerial, - timestamp: new Date(1672531200000), - clientId: 'clientId', - description: undefined, - metadata: undefined, - }, + version: makeVersion(firstSerial, 1672531200000, 'clientId'),Repeat similarly for other occurrences in this file.
Also applies to: 36-42, 61-67, 79-85
265-287: Add summary-action coverage per DR commentPer PR discussion, summary updates shouldn’t mark a message deleted and should mark updated when serial != version.serial. Add tests for ChatMessageAction.MessageSummary to lock this behavior.
I can add two tests: one asserting isDeleted === false for summary actions; another asserting isUpdated based on serial/version.serial mismatch. Want me to push a patch?
Also applies to: 289-310, 312-358, 360-445
test/core/messages.test.ts (4)
388-404: Incoming version should follow PV4 object shape (not string)Subscription fixtures still use version as a string. If PV4 emits an object
{ serial, timestamp, clientId, ... }, update the fixtures to reflect that; otherwise you’re testing stale behavior.I can update all emits to:
- version: '01672531200000-123@abcdefghij', + version: { serial: '01672531200000-123@abcdefghij', timestamp: publishTimestamp, clientId: 'yoda' },Confirm the PV4 inbound schema and I’ll push a scoped patch.
Also applies to: 416-423, 461-466, 481-497, 520-526, 540-556
369-378: Avoid flakiness: replace real timers with fake timers/waitersUsing a 300ms real timeout can be flaky on CI. Prefer vi.useFakeTimers + vi.runAllTimers or vi.waitFor.
Sketch:
vi.useFakeTimers(); const unsub = context.room.messages.subscribe(() => { /* ... */ }); /* emit 3 events */ vi.runAllTimers(); unsub();
1-2: Align Ably imports with project guidelineGuideline: “When importing the package ably, use import * as Ably from 'ably'.” Consider importing RealtimeChannel as a type to avoid dual runtime imports.
Apply:
-import * as Ably from 'ably'; -import { RealtimeChannel } from 'ably'; +import * as Ably from 'ably'; +import type { RealtimeChannel } from 'ably';
124-151: Add spec annotations for PV4 changesPer coding guidelines, annotate tests that validate PV4 payload changes.
Example:
- it<TestContext>('should be able to delete a message and get it back from response', async (context) => { + it<TestContext>('should be able to delete a message and get it back from response', async (context) => { + // @CHA-1101: PV4 delete returns message with Version object and top-level timestampAlso applies to: 261-306
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
demo/src/components/MessageComponent/MessageComponent.tsx(1 hunks)src/core/chat-api.ts(8 hunks)src/core/index.ts(1 hunks)src/core/message-parser.ts(2 hunks)src/core/message.ts(9 hunks)src/core/messages.ts(3 hunks)src/core/rest-types.ts(3 hunks)test/core/chat-api.test.ts(1 hunks)test/core/message-parser.test.ts(11 hunks)test/core/message.test.ts(37 hunks)test/core/messages.integration.test.ts(12 hunks)test/core/messages.test.ts(15 hunks)test/core/rest-types.test.ts(11 hunks)test/core/serial.test.ts(1 hunks)test/react/hooks/use-messages.integration.test.tsx(1 hunks)test/react/hooks/use-messages.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/core/index.ts
- demo/src/components/MessageComponent/MessageComponent.tsx
- test/core/chat-api.test.ts
- test/core/serial.test.ts
- test/core/messages.integration.test.ts
- src/core/messages.ts
- src/core/rest-types.ts
- test/core/rest-types.test.ts
- test/react/hooks/use-messages.integration.test.tsx
- test/react/hooks/use-messages.test.tsx
- src/core/message-parser.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: When importing in this project, use relative imports.
When importing the package ably, doimport * as Ably from 'ably'.
Use named exports over default exports.
Group related exports together.
Use PascalCase for interface names.
Use descriptive names that reflect the purpose for interfaces.
Prefix interfaces with 'I' is not required.
Use JSDoc style comments for interfaces.
Include a clear description of the interface's purpose in JSDoc.
Document all properties inline, alongside the property itself in interfaces.
Document all errors using@throwsin JSDoc.
Document the return type using@returnin JSDoc.
Link using {@link} to types mentioned in JSDoc.
Use PascalCase for enum names.
Use PascalCase for enum members.
Include JSDoc documentation for both enum and members.
Use PascalCase for class names.
Implement interfaces explicitly in classes.
Avoid extension wherever possible in classes.
Document class purpose and implementation details.
Use underscore prefix for private members in classes.
Group class members by visibility (private, protected, public).
Document all public methods and properties in classes.
Use JSDoc for all public methods.
Make use of@param,@returnsand@throwsin method documentation.
Use descriptive parameter names in methods.
Group related parameters into parameter objects in methods.
Use optional parameters with default values when appropriate in methods.
The error type used isErrorInfofrom the package 'ably'.
Use consistent logging levels (trace, debug, error).
Include relevant context in log messages, the second argument is a free-formed object for any relevant context.
Never log instances of Ably channels.
Structure log messages for easy parsing.
Minimize use of type assertions.
Use type guards when possible.
Document when and why type assertions are necessary.
Never useany. If we really can't avoid it, useunknown, but strong typing always preferred.
Use async/await over raw promises.
Handle promise re...
Files:
test/core/message.test.tstest/core/messages.test.tssrc/core/chat-api.tstest/core/message-parser.test.tssrc/core/message.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Test files should end with.test.ts.
Name test files to reflect the module being tested.
Use clear, descriptive names that explain the test's purpose in test descriptions.
Group related tests usingdescribeblocks.
Use nested describes for sub-features in tests.
Arrange-Act-Assert structure for test cases.
Test both positive and negative cases, including edge cases and boundary conditions.
Use clear, specific assertions in tests.
Create meaningful test data that clearly demonstrates the test case.
Use random room IDs to avoid conflicts in tests.
Use .each to write table/data driven tests.
Use custom matchers like toBeErrorInfo, toThrowErrorInfo, toBeErrorInfoWithCode, toThrowErrorInfoWithCode, toBeErrorInfoWithCauseCode for error testing.
Include an annotation of the specification point in the relevant test methods, e.g// CHA-M10a.
**/*.test.{ts,tsx}: Use the custom matchertoBeErrorInfoto validate that an error matches expected ErrorInfo properties (code, statusCode, message, cause) when testing error scenarios.
Use the custom matchertoThrowErrorInfoto check if a function throws an error matching expected ErrorInfo properties in tests.
Use the custom matchertoBeErrorInfoWithCodeto validate that an error has a specific error code in tests.
Use the custom matchertoThrowErrorInfoWithCodeto check if a function throws an error with a specific code in tests.
Use the custom matchertoBeErrorInfoWithCauseCodeto validate that an error has a cause with a specific error code in tests.
UsewaitForEventualHookValueToBeDefinedutility to wait for a React hook value to become defined in asynchronous hook tests.
UsewaitForEventualHookValueutility to wait for a React hook to return an expected value in asynchronous hook tests.
Usevi.waitForto wait for asynchronous events to happen in tests.
**/*.test.{ts,tsx}: Mock theablylibrary as a module usingvi.mock('ably')in test files that use Ably.
Each test file should ...
Files:
test/core/message.test.tstest/core/messages.test.tstest/core/message-parser.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
test/core/message.test.tstest/core/messages.test.tstest/core/message-parser.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/unit-testing.mdc)
Test files should end with
.test.tsand the name should reflect the module being tested (e.g.,message.test.tsformessage.ts).
Files:
test/core/message.test.tstest/core/messages.test.tstest/core/message-parser.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
src/core/chat-api.tssrc/core/message.ts
⚙️ CodeRabbit configuration file
src/**/*.{ts,tsx}: If there are any changes to the public-facing API, add a review comment: "Public API changes detected. Please update the documentation and the Ably CLI."
Files:
src/core/chat-api.tssrc/core/message.ts
🧠 Learnings (4)
📚 Learning: 2024-12-04T14:08:24.299Z
Learnt from: vladvelici
PR: ably/ably-chat-js#427
File: src/core/message.ts:251-275
Timestamp: 2024-12-04T14:08:24.299Z
Learning: In `src/core/message.ts` (TypeScript), the `version` property of the `Message` class can be compared directly using `<`, `>`, and `===` operators, as the `version` strings are safely comparable in this context.
Applied to files:
test/core/message.test.tstest/core/messages.test.tstest/core/message-parser.test.tssrc/core/message.ts
📚 Learning: 2024-11-07T21:42:51.681Z
Learnt from: vladvelici
PR: ably/ably-chat-js#378
File: test/core/messages.test.ts:0-0
Timestamp: 2024-11-07T21:42:51.681Z
Learning: In `test/core/messages.test.ts`, the mocked response for `chatApi.deleteMessage` in the test case `'should be able to delete a message and get it back from response'` should only include `serial` and `deletedAt`, as the real `deleteMessage` API response does not provide additional properties like `text`, `clientId`, `createdAt`, or `roomId`.
Applied to files:
test/core/message.test.tstest/core/messages.test.tssrc/core/chat-api.tstest/core/message-parser.test.ts
📚 Learning: 2024-12-04T14:12:11.101Z
Learnt from: vladvelici
PR: ably/ably-chat-js#427
File: src/core/messages.ts:501-502
Timestamp: 2024-12-04T14:12:11.101Z
Learning: In `src/core/messages.ts`, when constructing a `DefaultMessage` in the `send` method, passing the same `createdAt` date twice is intentional. One date is for `createdAt`, and the other is for `timestamp`. They should point to the same value, represented by `createdAt` after a message is posted.
Applied to files:
test/core/message.test.tstest/core/messages.test.tstest/core/message-parser.test.tssrc/core/message.ts
📚 Learning: 2024-10-22T15:54:58.070Z
Learnt from: splindsay-92
PR: ably/ably-chat-js#365
File: test/core/messages.integration.test.ts:95-107
Timestamp: 2024-10-22T15:54:58.070Z
Learning: In tests handling `messageEvent.type`, we should throw an error for unexpected message event types to ensure changes are caught.
Applied to files:
test/core/message.test.ts
🧬 Code graph analysis (4)
test/core/message.test.ts (1)
src/core/message.ts (1)
emptyMessageReactions(490-494)
test/core/messages.test.ts (2)
src/core/message.ts (1)
emptyMessageReactions(490-494)src/core/chat-api.ts (2)
deleteMessage(185-198)updateMessage(216-224)
src/core/chat-api.ts (2)
src/core/rest-types.ts (1)
RestMessage(34-44)src/core/messages.ts (1)
UpdateMessageParams(113-128)
src/core/message.ts (1)
src/core/operation-metadata.ts (1)
OperationMetadata(10-10)
🪛 GitHub Actions: Build and test
src/core/chat-api.ts
[error] 262-262: ChatApi._makeAuthorizedRequest() failed with 404 Not Found (not found) during tests. REST endpoint '/chat/v4/rooms/{room}/messages' not found.
🔇 Additional comments (16)
src/core/message.ts (5)
82-82: Public API change: createdAt → timestamp. Update docs and CLI.This is a public API rename. Please update TypeDoc, website docs, and Ably CLI.
118-122: Public API change: version now an object. Update docs and CLI.Export shape changed from string to structured Version. Ensure TypeDoc and CLI are updated.
342-342: Freezing version is good.Runtime immutability for nested version is a solid guard.
358-372: Confirm summary-action semantics for isUpdated/isDeleted.Current getters rely solely on action. Per DR note, summary updates should not flip deleted state and should not be treated as full updates. Add tests to verify these.
Would you like me to add focused tests in test/core/message.test.ts for summary events?
379-396: Version ordering via serial string comparisons is OK.Using <, >, === on version.serial matches prior guidance; keep as is.
src/core/chat-api.ts (4)
185-198: v4 endpoints OK; error handling OK.Paths and error propagation look correct pending the version fix.
Also applies to: 216-224, 232-241
1-286: Public API changes detected. Update docs and Ably CLI.Creating/updating/deleting message response types and endpoints changed. Please update TypeDoc, website docs, and CLI.
149-153: Resolved: no stale v3 paths
Grep shows no/chat/v3/endpoints in code or tests (only in UPGRADING.md examples), so mocks/backends have been updated.
32-37: Confirm CreateMessageResponse contract for v4: the backend should return an envelope ({ message: { serial, timestamp } }) rather than top-levelserial/timestamp. After verifying the actual API response, updateCreateMessageResponseinsrc/core/chat-api.ts(lines 32–37 and 200–214) to either reuseRestMessage(export type CreateMessageResponse = { message: RestMessage }) or introduce a dedicatedRestCreateMessageResponseand map it viamessageFromRest.test/core/message-parser.test.ts (1)
124-132: Defaults for missing version look good.Using top-level timestamp for default Version.timestamp is consistent.
test/core/messages.test.ts (6)
251-257: Good negative-path coverage for missing serialThese assertions use toBeErrorInfo correctly and read well.
Also applies to: 355-361
732-769: History attachment/serial resolution tests look solidNice coverage for attach vs channel serial selection, resume behavior, and state-change updates. No changes requested.
Also applies to: 771-864, 950-1063
617-635: Default-value handling test is aligned with Version objectAsserting version.serial === '' ensures parser constructs a Version even when missing. LGTM.
205-206: Assert Version clientId on mutation responsesGood to assert version.clientId presence; ensures provenance is wired through.
Also applies to: 248-249, 305-306, 352-353
149-151: Mocking reactions with emptyMessageReactions is consistent with public shapeThis keeps type shape realistic. LGTM.
Also applies to: 191-193, 234-235, 287-288, 334-335
69-71: No changes needed:sendMessagereturns top-levelserial/timestamp
ConfirmedCreateMessageResponseis defined as:export interface CreateMessageResponse { serial: string; timestamp: string; }Mocks of
{ serial, timestamp }at the top level are correct—no update required.
| action: ChatMessageAction.MessageDelete, | ||
| version: { | ||
| serial: '01672531200001-123@abcdefghij:0', | ||
| timestamp: deleteTimestamp, | ||
| reactions: emptyMessageReactions(), | ||
| operation: { | ||
| clientId: 'clientId', | ||
| }, | ||
| clientId: 'clientId', | ||
| description: undefined, | ||
| metadata: undefined, | ||
| }, | ||
| timestamp: sendTimestamp, |
There was a problem hiding this comment.
Update mocks: action should be MessageUpdate, not MessageDelete
Both update tests set action to MessageDelete in the mocked response. That contradicts the scenario and weakens the contract being tested.
Apply this diff in both update tests:
- action: ChatMessageAction.MessageDelete,
+ action: ChatMessageAction.MessageUpdate,Also applies to: 325-335
🤖 Prompt for AI Agents
In test/core/messages.test.ts around lines 278-286 (and also update the similar
block at lines 325-335), the mocked message action is incorrectly set to
ChatMessageAction.MessageDelete for the update tests; change the action field to
ChatMessageAction.MessageUpdate in both mocked responses so the mocks match the
tested update scenario, and update any related variable names or expectations if
necessary to reflect the update action.
| * Represents the detail of a message deletion or update. | ||
| */ | ||
| export interface Operation { | ||
| export interface Version { |
There was a problem hiding this comment.
We've named it in MessageVersion in the spec, so should probably do the same here
src/core/message.ts
Outdated
| /** | ||
| * A unique identifier for the latest version of this message. | ||
| */ | ||
| serial: string; |
src/core/message.ts
Outdated
| /** | ||
| * The timestamp at which this version was updated, deleted, or created. | ||
| */ | ||
| timestamp: Date; |
github-actions
bot
temporarily deployed
to
staging/pull/620/typedoc
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)
test/core/message-parser.test.ts (1)
1-6: Mock Ably and fix the import extension to TypeScript.Tests that import ably should mock it per guidelines; also use .ts for local TS imports for consistency.
-import * as Ably from 'ably'; -import { describe, expect, it } from 'vitest'; +import * as Ably from 'ably'; +import { describe, expect, it, vi } from 'vitest'; import { ChatMessageAction } from '../../src/core/events.ts'; import { DefaultMessage } from '../../src/core/message.ts'; -import { parseMessage } from '../../src/core/message-parser.js'; +import { parseMessage } from '../../src/core/message-parser.ts'; + +vi.mock('ably');
🧹 Nitpick comments (11)
src/core/messages-reactions.ts (1)
161-169: Use allowed log levels (trace, debug, error); avoid info.
Project guidelines don’t list “info”. Use “debug” here.- this._logger.info('MessagesReactions._processAnnotationEvent(); ignoring unknown reaction type', { event }); + this._logger.debug('MessagesReactions._processAnnotationEvent(); ignoring unknown reaction type', { event }); @@ - this._logger.info('MessagesReactions._processAnnotationEvent(); ignoring unknown reaction event type', { event }); + this._logger.debug('MessagesReactions._processAnnotationEvent(); ignoring unknown reaction event type', { event });src/core/message.ts (2)
33-58: Add spec annotation for PV4 Version object introduction.
Documenting linkage helps future readers.-export interface Version { +// @CHA-1101: Protocol V4 – Operation removed; structured Version introduced (serial/timestamp/clientId/description/metadata) +export interface Version {
457-474: Tidy comparison helper: fix comment typo and use consistent checks.
No behavior change, improves clarity.- // If our serials aren't the same, not same message and cannot compare1 + // If our serials aren't the same, not same message and cannot compare if (!first.equal(second)) { return false; } - // If one our version serials isn't set, cannot compare - if (first.version.serial == undefined || second.version.serial === undefined) { + // If one of the version serials isn't set, cannot compare + if (first.version.serial === undefined || second.version.serial === undefined) { return false; } // Use the comparator to determine the comparison return comparator(first.version as VersionWithSerial, second.version as VersionWithSerial);test/helper/environment.ts (1)
3-15: Defaulting to undefined endpoint changes previous sandbox default.
If CI/dev rely on sandbox by default, consider keeping sandbox as fallback or document the change.Option A — keep sandbox fallback:
export const testEndpoint = () => { switch (process.env.VITE_ABLY_ENV) { case 'local': { return 'local-rest.ably.io'; } case 'sandbox': { return 'nonprod:sandbox'; } - default: { - return; - } + default: { + return 'nonprod:sandbox'; + } } };If you keep undefined, ensure CI sets VITE_ABLY_ENV explicitly.
src/core/message-parser.ts (1)
36-46: Avoid broad cast; build a typed Version and annotate spec.
Removes the need foras Versionand clarifies timestamp handling.- // Create the version - only converting the timestamp if it's actually defined. - const versionTimestamp = message.version.timestamp === undefined ? undefined : new Date(message.version.timestamp); - const version = ( - versionTimestamp - ? { - ...message.version, - timestamp: versionTimestamp, - } - : message.version - ) as Version; + // @CHA-1101: PV4 version object; convert numeric timestamp to Date only when present + const version: Version = { + serial: message.version?.serial, + timestamp: + message.version?.timestamp === undefined ? undefined : new Date(message.version.timestamp), + clientId: message.version?.clientId, + description: message.version?.description, + metadata: message.version?.metadata, + };test/core/message.test.ts (4)
611-611: Use idiomatic identity assertions.Prefer Jest/Vitest identity matchers for clarity.
Apply:
- expect(newMessage !== message).toBe(true); + expect(newMessage).not.toBe(message); - expect(newMessage === message).toBe(true); + expect(newMessage).toBe(message);Also applies to: 640-640, 669-669, 691-691, 732-732, 774-774, 816-816
736-736: Rename misleading test title.The test replaces the original when the incoming message is newer; the title says the opposite.
-it('should return original message if newer message is older', () => { +it('should return updated message when incoming message is newer', () => {
265-287: Add coverage for summary-action semantics on isUpdated/isDeleted.Per PR discussion, summary updates shouldn’t mark messages as deleted and may affect isUpdated. Consider adding tests for summary annotations.
Example to add:
describe('summary actions', () => { it('does not mark as deleted for summary', () => { const msg = new DefaultMessage({ serial: 's', clientId: 'c', text: 't', metadata: {}, headers: {}, action: ChatMessageAction.MessageUpdate, // summary implied by annotations version: { serial: 'v:1', timestamp: new Date(1), clientId: 'm' }, timestamp: new Date(0), reactions: emptyMessageReactions(), }); // Simulate a summary-only update event if your model exposes it expect(msg.isDeleted).toBe(false); expect(msg.isUpdated).toBe(true); }); });Also applies to: 289-310
11-27: Minor: extract shared fixtures to constants/helpers.Dates and version fragments repeat heavily; factoring them reduces noise and maintenance.
Example:
const T0 = new Date(1672531200000); const mkVersion = (serial: string, ts = T0, clientId?: string) => ({ serial, timestamp: ts, clientId, description: undefined, metadata: undefined } as const);Also applies to: 29-45, 54-71, 72-89, 97-114, 115-132, 140-157, 158-175, 183-200, 201-218, 226-243, 244-261
test/core/message-parser.test.ts (2)
312-328: Freeze time to assert the default timestamp deterministically.Makes the “timestamp undefined” test robust and precise.
it('should use current time as default when timestamp is undefined', () => { - const message = { + vi.useFakeTimers(); + vi.setSystemTime(new Date(1234567890000)); + const message = { data: { text: 'hello' }, clientId: 'client1', extras: {}, serial: '01728402074206-000@cbfkKvEYgBhDaZ38195418:0', version: { serial: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', timestamp: 1728402074207, }, action: ChatMessageAction.MessageCreate, }; const result = parseMessage(message as Ably.InboundMessage); expect(result).toBeInstanceOf(DefaultMessage); - expect(result.timestamp).toBeInstanceOf(Date); + expect(result.timestamp).toEqual(new Date(1234567890000)); + vi.useRealTimers(); });
387-390: Clarify intentional omission of annotations in tests
TheparseMessagefunction andDefaultMessageclass don’t surfaceannotations(includingannotations.summary), so the incomingannotationsare deliberately ignored. In the test around lines 387–390, add a brief comment stating thatannotations.summaryis intentionally dropped (or assert that the resultingMessagehas no annotations property) to document this behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
cspell.json(1 hunks)package.json(1 hunks)src/core/message-parser.ts(3 hunks)src/core/message.ts(10 hunks)src/core/messages-reactions.ts(1 hunks)test/core/message-parser.test.ts(27 hunks)test/core/message.test.ts(38 hunks)test/core/messages-reactions.test.ts(7 hunks)test/core/messages.test.ts(27 hunks)test/helper/environment.ts(1 hunks)test/helper/realtime-client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/core/messages.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: When importing in this project, use relative imports.
When importing the package ably, doimport * as Ably from 'ably'.
Use named exports over default exports.
Group related exports together.
Use PascalCase for interface names.
Use descriptive names that reflect the purpose for interfaces.
Prefix interfaces with 'I' is not required.
Use JSDoc style comments for interfaces.
Include a clear description of the interface's purpose in JSDoc.
Document all properties inline, alongside the property itself in interfaces.
Document all errors using@throwsin JSDoc.
Document the return type using@returnin JSDoc.
Link using {@link} to types mentioned in JSDoc.
Use PascalCase for enum names.
Use PascalCase for enum members.
Include JSDoc documentation for both enum and members.
Use PascalCase for class names.
Implement interfaces explicitly in classes.
Avoid extension wherever possible in classes.
Document class purpose and implementation details.
Use underscore prefix for private members in classes.
Group class members by visibility (private, protected, public).
Document all public methods and properties in classes.
Use JSDoc for all public methods.
Make use of@param,@returnsand@throwsin method documentation.
Use descriptive parameter names in methods.
Group related parameters into parameter objects in methods.
Use optional parameters with default values when appropriate in methods.
The error type used isErrorInfofrom the package 'ably'.
Use consistent logging levels (trace, debug, error).
Include relevant context in log messages, the second argument is a free-formed object for any relevant context.
Never log instances of Ably channels.
Structure log messages for easy parsing.
Minimize use of type assertions.
Use type guards when possible.
Document when and why type assertions are necessary.
Never useany. If we really can't avoid it, useunknown, but strong typing always preferred.
Use async/await over raw promises.
Handle promise re...
Files:
test/helper/environment.tssrc/core/messages-reactions.tstest/helper/realtime-client.tstest/core/messages-reactions.test.tstest/core/message.test.tstest/core/message-parser.test.tssrc/core/message-parser.tssrc/core/message.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
test/helper/environment.tstest/helper/realtime-client.tstest/core/messages-reactions.test.tstest/core/message.test.tstest/core/message-parser.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
src/core/messages-reactions.tssrc/core/message-parser.tssrc/core/message.ts
⚙️ CodeRabbit configuration file
src/**/*.{ts,tsx}: If there are any changes to the public-facing API, add a review comment: "Public API changes detected. Please update the documentation and the Ably CLI."
Files:
src/core/messages-reactions.tssrc/core/message-parser.tssrc/core/message.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Test files should end with.test.ts.
Name test files to reflect the module being tested.
Use clear, descriptive names that explain the test's purpose in test descriptions.
Group related tests usingdescribeblocks.
Use nested describes for sub-features in tests.
Arrange-Act-Assert structure for test cases.
Test both positive and negative cases, including edge cases and boundary conditions.
Use clear, specific assertions in tests.
Create meaningful test data that clearly demonstrates the test case.
Use random room IDs to avoid conflicts in tests.
Use .each to write table/data driven tests.
Use custom matchers like toBeErrorInfo, toThrowErrorInfo, toBeErrorInfoWithCode, toThrowErrorInfoWithCode, toBeErrorInfoWithCauseCode for error testing.
Include an annotation of the specification point in the relevant test methods, e.g// CHA-M10a.
**/*.test.{ts,tsx}: Use the custom matchertoBeErrorInfoto validate that an error matches expected ErrorInfo properties (code, statusCode, message, cause) when testing error scenarios.
Use the custom matchertoThrowErrorInfoto check if a function throws an error matching expected ErrorInfo properties in tests.
Use the custom matchertoBeErrorInfoWithCodeto validate that an error has a specific error code in tests.
Use the custom matchertoThrowErrorInfoWithCodeto check if a function throws an error with a specific code in tests.
Use the custom matchertoBeErrorInfoWithCauseCodeto validate that an error has a cause with a specific error code in tests.
UsewaitForEventualHookValueToBeDefinedutility to wait for a React hook value to become defined in asynchronous hook tests.
UsewaitForEventualHookValueutility to wait for a React hook to return an expected value in asynchronous hook tests.
Usevi.waitForto wait for asynchronous events to happen in tests.
**/*.test.{ts,tsx}: Mock theablylibrary as a module usingvi.mock('ably')in test files that use Ably.
Each test file should ...
Files:
test/core/messages-reactions.test.tstest/core/message.test.tstest/core/message-parser.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/unit-testing.mdc)
Test files should end with
.test.tsand the name should reflect the module being tested (e.g.,message.test.tsformessage.ts).
Files:
test/core/messages-reactions.test.tstest/core/message.test.tstest/core/message-parser.test.ts
🧠 Learnings (12)
📚 Learning: 2025-07-23T10:06:04.393Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/typescript-conventions.mdc:0-0
Timestamp: 2025-07-23T10:06:04.393Z
Learning: Applies to **/*.{ts,tsx} : When importing the package ably, use `import * as Ably from 'ably'`.
Applied to files:
package.jsonsrc/core/message-parser.ts
📚 Learning: 2025-07-22T15:04:27.777Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-22T15:04:27.777Z
Learning: Applies to **/*.{ts,tsx} : When importing the package ably, do `import * as Ably from 'ably'`.
Applied to files:
package.jsonsrc/core/message-parser.ts
📚 Learning: 2025-08-08T16:24:19.988Z
Learnt from: splindsay-92
PR: ably/ably-chat-js#601
File: test/core/realtime-subscriptions.test.ts:3-4
Timestamp: 2025-08-08T16:24:19.988Z
Learning: Applies to test/**/*.{ts,tsx} in ably/ably-chat-js: Keep explicit .ts/.tsx extensions in import paths within test files; do not switch to .js extensions. This aligns with the project’s Vitest + TS setup and existing test code conventions.
Applied to files:
test/helper/environment.tstest/helper/realtime-client.ts
📚 Learning: 2025-06-30T12:57:08.726Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-06-30T12:57:08.726Z
Learning: Applies to test-setup.ts : Automatically create a new app in the sandbox environment before tests run, handle API key setup, and set `process.env.sandboxApiKey` for test authentication in test setup.
Applied to files:
test/helper/environment.ts
📚 Learning: 2025-06-30T12:57:08.726Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-06-30T12:57:08.726Z
Learning: Applies to test-setup.ts : Skip test environment setup when using the local realtime cluster.
Applied to files:
test/helper/environment.tstest/helper/realtime-client.ts
📚 Learning: 2025-07-22T15:05:08.887Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-07-22T15:05:08.887Z
Learning: Applies to *.{integration.test.ts,integration.test.tsx} : Do not mock modules in integration tests; use full Ably.Realtime clients to talk to a real Ably service.
Applied to files:
test/helper/realtime-client.ts
📚 Learning: 2025-07-22T15:04:27.777Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-22T15:04:27.777Z
Learning: Applies to **/*.integration.test.{ts,tsx} : Do not mock modules in integration tests; use real Ably.Realtime clients.
Applied to files:
test/helper/realtime-client.ts
📚 Learning: 2024-10-30T19:57:12.859Z
Learnt from: AndyTWF
PR: ably/ably-chat-js#382
File: test/core/chat.integration.test.ts:92-98
Timestamp: 2024-10-30T19:57:12.859Z
Learning: In `test/core/chat.integration.test.ts`, the `ablyRealtimeClient` function returns an instance of `Ably.Realtime`, so the TypeScript compiler recognizes methods like `close()` without needing type assertions.
Applied to files:
test/helper/realtime-client.ts
📚 Learning: 2024-11-07T21:42:51.681Z
Learnt from: vladvelici
PR: ably/ably-chat-js#378
File: test/core/messages.test.ts:0-0
Timestamp: 2024-11-07T21:42:51.681Z
Learning: In `test/core/messages.test.ts`, the mocked response for `chatApi.deleteMessage` in the test case `'should be able to delete a message and get it back from response'` should only include `serial` and `deletedAt`, as the real `deleteMessage` API response does not provide additional properties like `text`, `clientId`, `createdAt`, or `roomId`.
Applied to files:
test/core/messages-reactions.test.tstest/core/message.test.tstest/core/message-parser.test.ts
📚 Learning: 2024-12-04T14:08:24.299Z
Learnt from: vladvelici
PR: ably/ably-chat-js#427
File: src/core/message.ts:251-275
Timestamp: 2024-12-04T14:08:24.299Z
Learning: In `src/core/message.ts` (TypeScript), the `version` property of the `Message` class can be compared directly using `<`, `>`, and `===` operators, as the `version` strings are safely comparable in this context.
Applied to files:
test/core/message.test.tstest/core/message-parser.test.tssrc/core/message-parser.tssrc/core/message.ts
📚 Learning: 2024-12-04T14:12:11.101Z
Learnt from: vladvelici
PR: ably/ably-chat-js#427
File: src/core/messages.ts:501-502
Timestamp: 2024-12-04T14:12:11.101Z
Learning: In `src/core/messages.ts`, when constructing a `DefaultMessage` in the `send` method, passing the same `createdAt` date twice is intentional. One date is for `createdAt`, and the other is for `timestamp`. They should point to the same value, represented by `createdAt` after a message is posted.
Applied to files:
test/core/message.test.tstest/core/message-parser.test.tssrc/core/message.ts
📚 Learning: 2024-10-22T15:54:58.070Z
Learnt from: splindsay-92
PR: ably/ably-chat-js#365
File: test/core/messages.integration.test.ts:95-107
Timestamp: 2024-10-22T15:54:58.070Z
Learning: In tests handling `messageEvent.type`, we should throw an error for unexpected message event types to ensure changes are caught.
Applied to files:
test/core/message.test.ts
🧬 Code graph analysis (6)
test/helper/realtime-client.ts (1)
test/helper/environment.ts (1)
testEndpoint(3-15)
test/core/messages-reactions.test.ts (1)
src/core/index.ts (2)
ChatMessageAction(22-22)MessageReactionType(25-25)
test/core/message.test.ts (2)
src/core/message.ts (2)
emptyMessageReactions(522-526)Message(68-253)src/core/index.ts (1)
Message(36-36)
test/core/message-parser.test.ts (1)
src/core/index.ts (1)
ChatMessageAction(22-22)
src/core/message-parser.ts (2)
src/core/message.ts (1)
Version(33-58)src/core/index.ts (1)
Version(42-42)
src/core/message.ts (2)
src/core/index.ts (4)
MessageOperationMetadata(40-40)OperationMetadata(65-65)Version(42-42)Message(36-36)src/core/operation-metadata.ts (1)
OperationMetadata(10-10)
⏰ 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). (4)
- GitHub Check: test node 18
- GitHub Check: test node 24
- GitHub Check: test node 20
- GitHub Check: test node 22
🔇 Additional comments (9)
cspell.json (1)
21-23: LGTM: added dictionary terms align with new endpoint naming.
No issues; “nonprod” supports testEndpoint('nonprod:sandbox').src/core/message.ts (1)
124-127: Public API changes detected. Please update the documentation and the Ably CLI.
Version object replaces Operation; createdAt → timestamp.test/helper/realtime-client.ts (1)
10-10: SupportedClientOptions.endpoint
Ably JS PV4 deprecatesenvironmentin favor ofendpoint, which accepts only a single routing-policy name (e.g."main"or"example-eu"). It does not support compound values like"nonprod:sandbox". To target a non-production environment via DNS, use the SDK’s nonprod flag or configure your nonprod routing-policy; to point at an arbitrary host (e.g.local-rest.ably.io), setrestHost/realtimeHostinstead.src/core/message-parser.ts (1)
19-21: Confirm version is always present on inbound PV4 messages.
Parser assumesmessage.versionexists; if not guaranteed, add a guard or default.test/core/message.test.ts (4)
18-24: PV4 version object usage is consistent and correct.All constructors now pass a structured version { serial, timestamp, clientId?, description?, metadata? }. Looks good.
Also applies to: 36-42, 61-67, 79-85, 104-110, 122-128, 147-153, 165-171, 190-196, 208-214, 233-239, 251-257, 275-281, 298-304, 326-332, 344-350, 472-477, 490-495, 512-518, 533-538, 564-569, 595-600, 624-629, 653-658, 703-708, 721-726, 745-750, 763-768, 787-792, 805-810
25-25: Top-level timestamp migration is correct.Using message.timestamp (Date) aligns with PV4 and tests assert it properly.
Also applies to: 43-43, 68-68, 86-86, 111-111, 129-129, 154-154, 172-172, 197-197, 215-215, 240-240, 258-258, 282-282, 305-305, 333-333, 351-351, 478-478, 496-496, 519-519, 539-539, 570-570, 601-601, 630-630, 659-659, 681-681, 709-709, 727-727, 751-751, 769-769, 793-793, 811-811
512-518: with() event application scenarios look sound.Update, delete, equal-version, and older-version behaviors are exercised well.
Also applies to: 533-538, 564-569, 595-600, 624-629, 653-658, 703-708, 721-726, 745-750, 763-768, 787-792, 805-810
267-269: Serial is opaque; version suffix is part of valid format. Tests using:0suffix are correct—no change needed.Likely an incorrect or invalid review comment.
test/core/message-parser.test.ts (1)
19-22: PV4 message shape assertions are accurate.Inbound → DefaultMessage mapping (timestamp to Date, nested version.*, action normalization) is validated correctly across cases.
Also applies to: 36-39, 55-57, 73-75, 84-84, 95-98, 114-116, 130-132, 137-156, 179-182, 196-198, 212-214, 228-230, 244-246, 260-262, 276-278, 292-294, 318-321, 339-341, 381-386, 387-390, 426-432, 433-436, 469-474, 475-478, 513-518, 519-522, 352-357, 447-454, 491-497, 535-541
| // As Chat uses mutable messages, we know that `serial` will be defined, so this cast is ok | ||
| const serial = event.serial as unknown as string; | ||
|
|
||
| // Set the reaction types from the summary | ||
| const summary = event.annotations.summary; | ||
|
|
||
| const unique = (event.summary[ReactionAnnotationType.Unique] ?? {}) as unknown as Ably.SummaryUniqueValues; | ||
| const distinct = (event.summary[ReactionAnnotationType.Distinct] ?? {}) as unknown as Ably.SummaryDistinctValues; | ||
| const multiple = (event.summary[ReactionAnnotationType.Multiple] ?? {}) as Ably.SummaryMultipleValues; | ||
| const unique = (summary[ReactionAnnotationType.Unique] ?? {}) as unknown as Ably.SummaryUniqueValues; | ||
| const distinct = (summary[ReactionAnnotationType.Distinct] ?? {}) as unknown as Ably.SummaryDistinctValues; | ||
| const multiple = (summary[ReactionAnnotationType.Multiple] ?? {}) as Ably.SummaryMultipleValues; | ||
|
|
There was a problem hiding this comment.
Guard against missing serial/annotations and drop unsafe cast.
Avoid double assertion; add runtime checks for event.serial and event.annotations?.summary to prevent undefined access.
Apply:
- // As Chat uses mutable messages, we know that `serial` will be defined, so this cast is ok
- const serial = event.serial as unknown as string;
-
- // Set the reaction types from the summary
- const summary = event.annotations.summary;
+ // @CHA-1101: summary events carry message serial and annotations.summary in PV4
+ if (typeof event.serial !== 'string' || !event.annotations?.summary) {
+ this._logger.debug('MessagesReactions._processMessageEvent(); ignoring summary with missing serial/summary', { event });
+ return;
+ }
+ const serial = event.serial;
+
+ // Set the reaction types from the summary
+ const summary = event.annotations.summary;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // As Chat uses mutable messages, we know that `serial` will be defined, so this cast is ok | |
| const serial = event.serial as unknown as string; | |
| // Set the reaction types from the summary | |
| const summary = event.annotations.summary; | |
| const unique = (event.summary[ReactionAnnotationType.Unique] ?? {}) as unknown as Ably.SummaryUniqueValues; | |
| const distinct = (event.summary[ReactionAnnotationType.Distinct] ?? {}) as unknown as Ably.SummaryDistinctValues; | |
| const multiple = (event.summary[ReactionAnnotationType.Multiple] ?? {}) as Ably.SummaryMultipleValues; | |
| const unique = (summary[ReactionAnnotationType.Unique] ?? {}) as unknown as Ably.SummaryUniqueValues; | |
| const distinct = (summary[ReactionAnnotationType.Distinct] ?? {}) as unknown as Ably.SummaryDistinctValues; | |
| const multiple = (summary[ReactionAnnotationType.Multiple] ?? {}) as Ably.SummaryMultipleValues; | |
| // @CHA-1101: summary events carry message serial and annotations.summary in PV4 | |
| if (typeof event.serial !== 'string' || !event.annotations?.summary) { | |
| this._logger.debug('MessagesReactions._processMessageEvent(); ignoring summary with missing serial/summary', { event }); | |
| return; | |
| } | |
| const serial = event.serial; | |
| // Set the reaction types from the summary | |
| const summary = event.annotations.summary; | |
| const unique = (summary[ReactionAnnotationType.Unique] ?? {}) as unknown as Ably.SummaryUniqueValues; | |
| const distinct = (summary[ReactionAnnotationType.Distinct] ?? {}) as unknown as Ably.SummaryDistinctValues; | |
| const multiple = (summary[ReactionAnnotationType.Multiple] ?? {}) as unknown as Ably.SummaryMultipleValues; |
🤖 Prompt for AI Agents
In src/core/messages-reactions.ts around lines 201 to 210, remove the unsafe
double-cast and add runtime guards: check that event.serial is present and is a
string (or convert/return early) and that event.annotations?.summary exists
before accessing it; then narrow types using proper TypeScript checks (e.g.,
typeof/optional chaining) and assign unique/distinct/multiple from the guarded
summary with correct typing rather than using "as unknown as". If any required
value is missing, handle it gracefully (early return or log and skip) so no
undefined access occurs.
| summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } }, | ||
| }, |
There was a problem hiding this comment.
Wrong enum used for summary keys; should be ReactionAnnotationType.
Using MessageReactionType here produces mismatched keys and fragile tests.
- summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },
+ summary: { [ReactionAnnotationType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },
@@
- summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },
+ summary: { [ReactionAnnotationType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },Also applies to: 333-334
🤖 Prompt for AI Agents
In test/core/messages-reactions.test.ts around lines 315-316 (and also lines
333-334), the test is using MessageReactionType as keys in the summary object
but the code expects ReactionAnnotationType; update the summary entries to use
ReactionAnnotationType instead of MessageReactionType, adjust any test imports
to include ReactionAnnotationType if not already imported, and ensure the
summary shapes/types still match the expected structure after replacing the
enum.
AndyTWF
force-pushed
the
chat/move-to-protocol-v4
branch
from
c7e3371 to
69a5362
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/typedoc
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/core/message.ts (1)
198-216: @throws docs don’t match implementation for before()/after().JSDoc claims these throw ErrorInfo for invalid serials, but code never throws. Either implement validation or remove @throws.
Minimal fix (docs-only):
- * @throws {@link ErrorInfo} if serials of either message is invalid.(Apply to both before() and after() blocks.)
src/core/messages.ts (2)
562-570: Public API changes detected. Please update the documentation and the Ably CLI.The delete method now returns messages with the Protocol V4 structure, which is a breaking change in the public API.
575-591: Public API changes detected. Please update the documentation and the Ably CLI.The update method now returns messages with the Protocol V4 structure, which is a breaking change in the public API.
test/core/message-parser.test.ts (1)
1-6: Mock Ably, use type-only import, and switch parser import to .tsFile: test/core/message-parser.test.ts — top of file.
- Add vi.mock('ably') and import vi from vitest to match other tests.
- Make the Ably import type-only to avoid a runtime load.
- Use the .ts extension for the parser import to follow test import conventions.
-import * as Ably from 'ably'; -import { describe, expect, it } from 'vitest'; +import type * as Ably from 'ably'; +import { describe, expect, it, vi } from 'vitest'; + +vi.mock('ably'); import { ChatMessageAction } from '../../src/core/events.ts'; import { DefaultMessage } from '../../src/core/message.ts'; -import { parseMessage } from '../../src/core/message-parser.js'; +import { parseMessage } from '../../src/core/message-parser.ts';
♻️ Duplicate comments (3)
test/core/messages-reactions.test.ts (2)
315-316: Wrong enum used for summary keys; should be ReactionAnnotationType.Using MessageReactionType here produces mismatched keys and fragile tests.
- summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } }, + summary: { [ReactionAnnotationType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },
333-334: Wrong enum used for summary keys; should be ReactionAnnotationType.Using MessageReactionType here produces mismatched keys and fragile tests.
- summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } }, + summary: { [ReactionAnnotationType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },test/core/messages.test.ts (1)
278-279: Fix incorrect action in update message mocks.The mocked response for update operations incorrectly uses
ChatMessageAction.MessageDeleteinstead ofChatMessageAction.MessageUpdate. This contradicts the update scenario being tested.Apply this diff to fix the action in both update tests:
- action: ChatMessageAction.MessageDelete, + action: ChatMessageAction.MessageUpdate,Also applies to: 325-326
Also applies to: 325-326
🧹 Nitpick comments (12)
test/core/rest-types.test.ts (2)
54-57: Avoid realistic auth header values in tests.Use a neutral placeholder to avoid normalizing sensitive-looking tokens in fixtures.
Apply:
- authorization: 'bearer-token', + authorization: 'redacted',
373-397: Rename test to reflect current mapping (no spread).The title mentions “spread” but implementation maps fields explicitly. Rename for clarity.
-it('should preserve all other RestMessage fields in the spread', () => { +it('should map all expected fields from RestMessage to Message', () => {src/core/message.ts (3)
457-474: Tidy comparator: strict checks and typo fix; document assertion need.
- Use strict undefined checks.
- Fix “cannot compare1” typo.
- Add a short note justifying the type assertion as per guidelines.
private _compareVersions( first: Message, second: Message, comparator: (first: VersionWithSerial, second: VersionWithSerial) => boolean, ): boolean { - // If our serials aren't the same, not same message and cannot compare1 + // If serials differ, they're different messages; cannot compare versions if (!first.equal(second)) { return false; } - // If one our version serials isn't set, cannot compare - if (first.version.serial == undefined || second.version.serial === undefined) { + // If either version serial is missing, cannot compare + if (first.version.serial === undefined || second.version.serial === undefined) { return false; } - // Use the comparator to determine the comparison - return comparator(first.version as VersionWithSerial, second.version as VersionWithSerial); + // Type assertion rationale: guarded above, so serial is defined on both versions. // @CHA-1101 + return comparator(first.version as VersionWithSerial, second.version as VersionWithSerial); }
347-353: Optional: freeze metadata and headers for full immutability.You freeze version and reactions; consider also freezing metadata and headers to prevent accidental mutation of nested structures. This would require minor test updates.
435-436: Replace magic 40000 error codes with ErrorCode.BadRequestsrc/core/errors.ts exports enum ErrorCode (BadRequest = 40000) — replace literal 40000 usages with ErrorCode.BadRequest. Example locations: src/core/message.ts (around lines 434–436, 439–442, 483–487). Run a repo-wide search for 40000 (src/** and test/**) and update occurrences accordingly.
test/core/message.test.ts (3)
1-5: Mock ably to isolate tests.Since the SUT constructs Ably.ErrorInfo, mock the module in this file.
import { describe, expect, it } from 'vitest'; import { ChatMessageAction, ChatMessageEvent, ChatMessageEventType } from '../../src/core/events.ts'; import { DefaultMessage, emptyMessageReactions, Message } from '../../src/core/message.ts'; +vi.mock('ably');
6-6: Annotate spec reference.Tie this suite to CHA-1101 for future readers.
-describe('ChatMessage', () => { +// @CHA-1101: PV4 Version object + timestamp semantics +describe('ChatMessage', () => {
736-736: Rename misleading test title.The test asserts that a newer message replaces the original, but the name says “newer message is older”.
-it('should return original message if newer message is older', () => { +it('should replace with the incoming message when it is a newer version', () => {test/core/message-parser.test.ts (4)
4-4: Prefer importing enums from the public surface to reduce test couplingIf
ChatMessageActionis re-exported fromsrc/core/index.ts, importing from there keeps tests decoupled from internal file paths. If not re-exported, ignore.-import { ChatMessageAction } from '../../src/core/events.ts'; +import { ChatMessageAction } from '../../src/core/index.ts';
170-310: Solid non-object coercion coverageNice table-driven tests covering
dataandextraswith invalid primitive types. Consider adding one more case:extrasis an object butextras.headersis an invalid type (e.g., string), to verify deep validation.Happy to add that targeted case if you want it in this PR.
312-328: Strengthen the “default to current time” assertionAs written, the test passes even if
parseMessageusesversion.timestampas a fallback, because it only checks for a Date instance. Freeze time and assert exact equality to the mocked “now”; also avoid providingversion.timestampto remove ambiguity.- it('should use current time as default when timestamp is undefined', () => { - const message = { - data: { text: 'hello' }, - clientId: 'client1', - extras: {}, - serial: '01728402074206-000@cbfkKvEYgBhDaZ38195418:0', - version: { - serial: '01728402074207-000@cbfkKvEYgBhDaZ38195418:0', - timestamp: 1728402074207, - }, - action: ChatMessageAction.MessageCreate, - }; - - const result = parseMessage(message as Ably.InboundMessage); - expect(result).toBeInstanceOf(DefaultMessage); - expect(result.timestamp).toBeInstanceOf(Date); - }); + it('should use current time as default when timestamp is undefined', () => { + vi.useFakeTimers(); + const NOW = new Date('2025-01-01T00:00:00.000Z'); + vi.setSystemTime(NOW); + + const message = { + data: { text: 'hello' }, + clientId: 'client1', + extras: {}, + serial: '01728402074206-000@cbfkKvEYgBhDaZ38195418:0', + action: ChatMessageAction.MessageCreate, + }; + + const result = parseMessage(message as Ably.InboundMessage); + expect(result).toBeInstanceOf(DefaultMessage); + expect(result.timestamp).toEqual(NOW); + + vi.useRealTimers(); + });
369-413: Updated-message case looks correct; consider exercising summary semantics via gettersGreat check for
updatedAt/updatedByderiving fromversion. IfDefaultMessageexposesisUpdated/isDeletedgetters, add expectations (especially withannotations.summary) to lock in the intended behavior from the PR comments.I can add a small follow-up test asserting
result.isUpdated === trueandresult.isDeleted === falsefor summary updates—let me know.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.env.test.example(1 hunks)cspell.json(1 hunks)demo/src/components/MessageComponent/MessageComponent.tsx(1 hunks)package.json(1 hunks)src/core/chat-api.ts(8 hunks)src/core/index.ts(1 hunks)src/core/message-parser.ts(3 hunks)src/core/message.ts(10 hunks)src/core/messages-reactions.ts(1 hunks)src/core/messages.ts(3 hunks)src/core/rest-types.ts(3 hunks)test/core/chat-api.test.ts(1 hunks)test/core/message-parser.test.ts(27 hunks)test/core/message.test.ts(38 hunks)test/core/messages-reactions.test.ts(7 hunks)test/core/messages.integration.test.ts(12 hunks)test/core/messages.test.ts(27 hunks)test/core/rest-types.test.ts(11 hunks)test/core/serial.test.ts(1 hunks)test/helper/environment.ts(1 hunks)test/helper/realtime-client.ts(1 hunks)test/react/hooks/use-messages.integration.test.tsx(1 hunks)test/react/hooks/use-messages.test.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/rest-types.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- test/helper/realtime-client.ts
- test/react/hooks/use-messages.integration.test.tsx
- src/core/index.ts
- cspell.json
- package.json
- demo/src/components/MessageComponent/MessageComponent.tsx
- test/helper/environment.ts
- test/react/hooks/use-messages.test.tsx
- src/core/messages-reactions.ts
- test/core/chat-api.test.ts
- src/core/message-parser.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: When importing in this project, use relative imports.
When importing the package ably, doimport * as Ably from 'ably'.
Use named exports over default exports.
Group related exports together.
Use PascalCase for interface names.
Use descriptive names that reflect the purpose for interfaces.
Prefix interfaces with 'I' is not required.
Use JSDoc style comments for interfaces.
Include a clear description of the interface's purpose in JSDoc.
Document all properties inline, alongside the property itself in interfaces.
Document all errors using@throwsin JSDoc.
Document the return type using@returnin JSDoc.
Link using {@link} to types mentioned in JSDoc.
Use PascalCase for enum names.
Use PascalCase for enum members.
Include JSDoc documentation for both enum and members.
Use PascalCase for class names.
Implement interfaces explicitly in classes.
Avoid extension wherever possible in classes.
Document class purpose and implementation details.
Use underscore prefix for private members in classes.
Group class members by visibility (private, protected, public).
Document all public methods and properties in classes.
Use JSDoc for all public methods.
Make use of@param,@returnsand@throwsin method documentation.
Use descriptive parameter names in methods.
Group related parameters into parameter objects in methods.
Use optional parameters with default values when appropriate in methods.
The error type used isErrorInfofrom the package 'ably'.
Use consistent logging levels (trace, debug, error).
Include relevant context in log messages, the second argument is a free-formed object for any relevant context.
Never log instances of Ably channels.
Structure log messages for easy parsing.
Minimize use of type assertions.
Use type guards when possible.
Document when and why type assertions are necessary.
Never useany. If we really can't avoid it, useunknown, but strong typing always preferred.
Use async/await over raw promises.
Handle promise re...
Files:
src/core/messages.tstest/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tstest/core/messages-reactions.test.tssrc/core/chat-api.tssrc/core/message.tstest/core/messages.test.tstest/core/messages.integration.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
src/core/messages.tssrc/core/chat-api.tssrc/core/message.ts
⚙️ CodeRabbit configuration file
src/**/*.{ts,tsx}: If there are any changes to the public-facing API, add a review comment: "Public API changes detected. Please update the documentation and the Ably CLI."
Files:
src/core/messages.tssrc/core/chat-api.tssrc/core/message.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Test files should end with.test.ts.
Name test files to reflect the module being tested.
Use clear, descriptive names that explain the test's purpose in test descriptions.
Group related tests usingdescribeblocks.
Use nested describes for sub-features in tests.
Arrange-Act-Assert structure for test cases.
Test both positive and negative cases, including edge cases and boundary conditions.
Use clear, specific assertions in tests.
Create meaningful test data that clearly demonstrates the test case.
Use random room IDs to avoid conflicts in tests.
Use .each to write table/data driven tests.
Use custom matchers like toBeErrorInfo, toThrowErrorInfo, toBeErrorInfoWithCode, toThrowErrorInfoWithCode, toBeErrorInfoWithCauseCode for error testing.
Include an annotation of the specification point in the relevant test methods, e.g// CHA-M10a.
**/*.test.{ts,tsx}: Use the custom matchertoBeErrorInfoto validate that an error matches expected ErrorInfo properties (code, statusCode, message, cause) when testing error scenarios.
Use the custom matchertoThrowErrorInfoto check if a function throws an error matching expected ErrorInfo properties in tests.
Use the custom matchertoBeErrorInfoWithCodeto validate that an error has a specific error code in tests.
Use the custom matchertoThrowErrorInfoWithCodeto check if a function throws an error with a specific code in tests.
Use the custom matchertoBeErrorInfoWithCauseCodeto validate that an error has a cause with a specific error code in tests.
UsewaitForEventualHookValueToBeDefinedutility to wait for a React hook value to become defined in asynchronous hook tests.
UsewaitForEventualHookValueutility to wait for a React hook to return an expected value in asynchronous hook tests.
Usevi.waitForto wait for asynchronous events to happen in tests.
**/*.test.{ts,tsx}: Mock theablylibrary as a module usingvi.mock('ably')in test files that use Ably.
Each test file should ...
Files:
test/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tstest/core/messages-reactions.test.tstest/core/messages.test.tstest/core/messages.integration.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
test/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tstest/core/messages-reactions.test.tstest/core/messages.test.tstest/core/messages.integration.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/unit-testing.mdc)
Test files should end with
.test.tsand the name should reflect the module being tested (e.g.,message.test.tsformessage.ts).
Files:
test/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tstest/core/messages-reactions.test.tstest/core/messages.test.tstest/core/messages.integration.test.ts
**/*.integration.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.integration.test.{ts,tsx}: Integration tests should use the vitest framework and follow the described structure.
Do not mock modules in integration tests; use real Ably.Realtime clients.
For async operations in integration tests, always use async/await and proper timeout handling.
Use vi.waitFor to wait for async events in integration tests.
Files:
test/core/messages.integration.test.ts
🧠 Learnings (5)
📚 Learning: 2024-11-07T21:42:51.681Z
Learnt from: vladvelici
PR: ably/ably-chat-js#378
File: test/core/messages.test.ts:0-0
Timestamp: 2024-11-07T21:42:51.681Z
Learning: In `test/core/messages.test.ts`, the mocked response for `chatApi.deleteMessage` in the test case `'should be able to delete a message and get it back from response'` should only include `serial` and `deletedAt`, as the real `deleteMessage` API response does not provide additional properties like `text`, `clientId`, `createdAt`, or `roomId`.
Applied to files:
src/core/messages.tstest/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tstest/core/messages-reactions.test.tssrc/core/chat-api.tstest/core/messages.test.tstest/core/messages.integration.test.ts
📚 Learning: 2024-12-04T14:12:11.101Z
Learnt from: vladvelici
PR: ably/ably-chat-js#427
File: src/core/messages.ts:501-502
Timestamp: 2024-12-04T14:12:11.101Z
Learning: In `src/core/messages.ts`, when constructing a `DefaultMessage` in the `send` method, passing the same `createdAt` date twice is intentional. One date is for `createdAt`, and the other is for `timestamp`. They should point to the same value, represented by `createdAt` after a message is posted.
Applied to files:
src/core/messages.tstest/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tssrc/core/message.tstest/core/messages.test.ts
📚 Learning: 2024-12-04T14:08:24.299Z
Learnt from: vladvelici
PR: ably/ably-chat-js#427
File: src/core/message.ts:251-275
Timestamp: 2024-12-04T14:08:24.299Z
Learning: In `src/core/message.ts` (TypeScript), the `version` property of the `Message` class can be compared directly using `<`, `>`, and `===` operators, as the `version` strings are safely comparable in this context.
Applied to files:
src/core/messages.tstest/core/serial.test.tstest/core/rest-types.test.tstest/core/message-parser.test.tstest/core/message.test.tssrc/core/message.tstest/core/messages.integration.test.ts
📚 Learning: 2024-10-22T15:54:58.070Z
Learnt from: splindsay-92
PR: ably/ably-chat-js#365
File: test/core/messages.integration.test.ts:95-107
Timestamp: 2024-10-22T15:54:58.070Z
Learning: In tests handling `messageEvent.type`, we should throw an error for unexpected message event types to ensure changes are caught.
Applied to files:
test/core/rest-types.test.tstest/core/message.test.ts
📚 Learning: 2025-08-08T16:24:19.988Z
Learnt from: splindsay-92
PR: ably/ably-chat-js#601
File: test/core/realtime-subscriptions.test.ts:3-4
Timestamp: 2025-08-08T16:24:19.988Z
Learning: Applies to test/**/*.{ts,tsx} in ably/ably-chat-js: Keep explicit .ts/.tsx extensions in import paths within test files; do not switch to .js extensions. This aligns with the project’s Vitest + TS setup and existing test code conventions.
Applied to files:
test/core/messages.test.ts
🧬 Code graph analysis (9)
src/core/messages.ts (2)
src/core/message.ts (1)
DefaultMessage(315-516)src/core/rest-types.ts (1)
messageFromRest(51-82)
test/core/rest-types.test.ts (2)
src/core/rest-types.ts (2)
messageFromRest(51-82)RestMessage(34-44)src/core/message.ts (2)
DefaultMessage(315-516)emptyMessageReactions(522-526)
test/core/message-parser.test.ts (1)
src/core/index.ts (1)
ChatMessageAction(22-22)
test/core/message.test.ts (1)
src/core/message.ts (2)
emptyMessageReactions(522-526)Message(68-253)
test/core/messages-reactions.test.ts (1)
src/core/index.ts (2)
ChatMessageAction(22-22)MessageReactionType(25-25)
src/core/chat-api.ts (4)
src/core/rest-types.ts (1)
RestMessage(34-44)src/core/messages.ts (1)
UpdateMessageParams(113-128)src/core/messages-reactions.ts (1)
DeleteMessageReactionParams(64-76)src/core/occupancy-parser.ts (1)
OccupancyData(6-16)
src/core/message.ts (1)
src/core/operation-metadata.ts (1)
OperationMetadata(10-10)
test/core/messages.test.ts (2)
src/core/message.ts (1)
emptyMessageReactions(522-526)src/core/chat-api.ts (2)
deleteMessage(185-198)updateMessage(216-224)
test/core/messages.integration.test.ts (2)
src/core/chat-api.ts (1)
history(126-154)src/core/messages.ts (1)
history(519-522)
⏰ 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
- GitHub Check: test node 24
- GitHub Check: test node 20
- GitHub Check: test node 22
- GitHub Check: demo-app
- GitHub Check: build
🔇 Additional comments (25)
.env.test.example (1)
4-4: Approve — sandbox mapping & v4 routes verifiedenvironment.ts maps 'sandbox' → 'nonprod:sandbox'; realtime-client.ts defaults endpoint via testEndpoint(); tests reference /chat/v4/ (see environment.ts, realtime-client.ts, test/core/chat-api.test.ts). Update/verify CI and developer docs (and CI .env) so missing .env.test cannot cause tests to target production.
test/core/rest-types.test.ts (1)
11-14: Good migration to Version object and Date conversions.The tests correctly assert the new RestVersion shape and the Date coercions for both top-level timestamp and version.timestamp. Also validates action fallback to MessageCreate. Looks solid.
Also applies to: 27-27, 33-35
src/core/message.ts (2)
33-58: Public API changes detected. Please update the documentation and the Ably CLI.Version introduced (object), Message.timestamp semantics, and Message.version type change are public. Ensure TypeDoc, website docs, and CLI are updated per PV4 (CHA-1101).
Also applies to: 84-88, 123-127
355-361: Clarify semantics for summary actions in getters.Confirm that summary events cannot set action to MessageUpdate/Delete; otherwise isUpdated/isDeleted would misclassify rolled-up summaries. Consider a note in JSDoc or a guard if needed.
Also applies to: 371-377
test/core/message.test.ts (1)
615-639: Confirm event type used for deletes.This test applies a delete using ChatMessageEventType.Updated. Validate against the spec whether a dedicated Deleted event type exists or if Updated is correct for both update and delete.
test/core/serial.test.ts (1)
30-30: LGTM! Version object structure correctly updated for Protocol V4.The test now properly uses the nested version object with
serialandtimestampfields, aligning with the Protocol V4 message structure changes.test/core/messages-reactions.test.ts (2)
194-201: LGTM! Annotations structure properly migrated to Protocol V4 format.The test correctly moves the summary data to the nested
annotations.summarystructure, which aligns with Protocol V4's approach to message annotations.
781-788: LGTM! Reaction events correctly use Protocol V4 version and annotations structure.The emulated backend events properly include the version object and nested annotations structure, ensuring tests accurately validate the Protocol V4 message format.
Also applies to: 818-823
src/core/messages.ts (3)
541-556: LGTM! Message creation correctly implements Protocol V4 version structure.The implementation properly constructs the version object with both
serialandtimestamp, and correctly uses the timestamp for both the version and top-level timestamp fields, as expected for newly created messages.
590-590: Resolved — updateMessage returns a RestMessage; current change is correct.Unit tests assert updateMessage returns the message object (serial, text, clientId, timestamp), so passing response to messageFromRest(response) is appropriate.
569-569: Resolved — chatApi.deleteMessage returns a RestMessage; no change needed.
ChatApi.deleteMessage is typed as DeleteMessageResponse = RestMessage (src/core/chat-api.ts), RestMessage is defined in src/core/rest-types.ts, and tests mock deleteMessage with a RestMessage-shaped object (test/core/messages.test.ts). Using messageFromRest(response) is correct.src/core/chat-api.ts (4)
32-37: LGTM! Documentation improvements enhance clarity.The addition of JSDoc comments for
serialandtimestampfields inCreateMessageResponseimproves API documentation and makes the interface contract clearer.
45-47: LGTM! Type aliases correctly reflect the new v4 API structure.The changes to
UpdateMessageResponseandDeleteMessageResponseto aliasRestMessagealign with the Protocol V4 message structure where these responses now return the full message data.
150-151: LGTM! All endpoints correctly migrated to v4.All Chat API endpoints have been successfully migrated from
/chat/v3/to/chat/v4/:
- history
- getMessage
- deleteMessage
- sendMessage
- updateMessage
- sendMessageReaction
- deleteMessageReaction
- getOccupancy
This migration aligns with the Protocol V4 changes for chat message payloads.
Also applies to: 179-180, 193-194, 213-213, 220-221, 229-229, 236-237, 245-245
119-119: Verify/update _apiProtocolVersion to v4_apiProtocolVersion is still 3 (src/core/chat-api.ts:119) and is passed to this._realtime.request (lines ~254 and ~273); a search for "chat/v3" returned no matches — confirm whether the HTTP endpoints were migrated to v4 and, if so, change the constant to 4; if this value intentionally represents the Ably realtime protocol version, leave it as-is.
test/core/messages.integration.test.ts (4)
172-177: LGTM! Version object assertions correctly validate the new structure.The test correctly validates that:
deletedAtequalsversion.timestampdeletedByequalsversion.clientId- Version metadata fields (
description,metadata) are properly preservedThis aligns with the new Protocol V4 structure where version information is now an object containing metadata previously held in the Operation field.
391-396: LGTM! Update message assertions properly validate version metadata.The test correctly validates the version object fields for updated messages, ensuring the description and metadata from the update operation are preserved in the version object.
646-653: LGTM! History tests correctly validate the version object structure.The test properly validates the version object in history results, ensuring all fields (serial, timestamp, clientId, description, metadata) are correctly populated from the deleted message's version data.
694-699: LGTM! Update history test correctly validates version fields.The test properly validates that updated messages in history contain the correct version object with all expected fields populated.
test/core/messages.test.ts (3)
134-150: LGTM! Delete message mocks correctly implement v4 structure.The mocked responses for delete operations correctly implement the new Protocol V4 structure with:
- Top-level message fields (serial, clientId, text, metadata, headers, action, timestamp)
- Nested version object containing versioning metadata
- Proper use of
emptyMessageReactions()helperAlso applies to: 176-192, 219-235
205-206: LGTM! Version assertions correctly validate clientId field.The tests properly validate that the version object contains the expected clientId, ensuring the version metadata is correctly populated.
Also applies to: 248-249, 305-306, 352-353
624-643: LGTM! Default values test correctly handles empty version object.The test properly validates that messages with missing fields use appropriate defaults, including an empty version object and properly structured reactions.
test/core/message-parser.test.ts (3)
137-156: Good coverage for missing version fieldsValidates that
versionsubfields default toundefinedwhen absent without leaking top-level values intoversion. This guards against accidental fallback coupling.
330-367: LGTM: new-message shape and expectations match PV4Asserts top-level
timestampmapping and version object presence without conflating their semantics. Good negative checks for update/delete fields.
415-545: Deletion scenarios well coveredVerifies empty text/metadata/headers and derives
deletedAt/deletedByfromversion. Good separation from update fields.
AndyTWF
force-pushed
the
chat/move-to-protocol-v4
branch
from
69a5362 to
65957d3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/typedoc
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demo/src/main.tsx (1)
1-12: Replace deprecated Ably host options (restHost/realtimeHost → endpoint).
- demo/api/ably-token-request/index.ts:26–27 — restHost and realtimeHost are still set to process.env.VITE_ABLY_HOST; update to use the Ably SDK's current 'endpoint' option (or remove these fields).
- demo/src/main.tsx:30,54 and vitest.config.ts:28,38 — the identifier "environment" appears (app/test config); confirm no Ably ClientOptions.environment remains.
🧹 Nitpick comments (5)
demo/package.json (1)
17-17: Pin Ably dependency to a commit SHA for reproducible builds.Branch heads can move and break the demo unexpectedly. Pin to a specific commit (or Git tag) on protocol-v4.
- "ably": "github:ably/ably-js#protocol-v4", + "ably": "github:ably/ably-js#<commit-sha>",demo/src/main.tsx (2)
48-51: Avoid setting endpoint: undefined in production.Only set endpoint when a host is provided; otherwise omit the property.
- realtimeOptions.endpoint = import.meta.env.VITE_ABLY_HOST ?? undefined; + const host = import.meta.env.VITE_ABLY_HOST; + if (host) { + realtimeOptions.endpoint = host; + }
36-56: Minor: structure logs with context.Optional: include structured context to aid log parsing.
- console.log('Using local Ably environment'); + console.log('Using local Ably environment', { env: environment }); - console.log('Using sandbox Ably environment'); + console.log('Using sandbox Ably environment', { env: environment }); - console.log('Using production Ably environment'); + console.log('Using production Ably environment', { env: environment, host: import.meta.env.VITE_ABLY_HOST ?? null });test/helper/environment.ts (2)
3-15: Type the return and be explicit about undefined; add a brief spec note.Improves readability and avoids implicit undefined; ties to CHA-1101.
-export const testEndpoint = () => { - switch (process.env.VITE_ABLY_ENV) { - case 'local': { - return 'local-rest.ably.io'; - } - case 'production': { - return; - } - default: { - return 'nonprod:sandbox'; - } - } -}; +/** + * Returns the Ably endpoint for tests based on VITE_ABLY_ENV. + * - 'local' -> 'local-rest.ably.io' + * - 'production' -> undefined (use Ably production defaults) + * - otherwise -> 'nonprod:sandbox' + * @see CHA-1101 + */ +export const testEndpoint = (): string | undefined => { + switch (process.env.VITE_ABLY_ENV) { + case 'local': + return 'local-rest.ably.io'; + case 'production': + return undefined; + default: + return 'nonprod:sandbox'; + } +};
1-1: Ensure boolean return from isNonSandboxEnvironment.Current expression can return undefined when the env var is unset.
-export const isNonSandboxEnvironment = () => process.env.VITE_ABLY_ENV && process.env.VITE_ABLY_ENV !== 'sandbox'; +export const isNonSandboxEnvironment = () => + Boolean(process.env.VITE_ABLY_ENV) && process.env.VITE_ABLY_ENV !== 'sandbox';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
demo/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.env.test.example(1 hunks)cspell.json(1 hunks)demo/package.json(1 hunks)demo/src/main.tsx(1 hunks)test/helper/environment.ts(1 hunks)test/helper/realtime-client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cspell.json
- test/helper/realtime-client.ts
- .env.test.example
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: When importing in this project, use relative imports.
When importing the package ably, doimport * as Ably from 'ably'.
Use named exports over default exports.
Group related exports together.
Use PascalCase for interface names.
Use descriptive names that reflect the purpose for interfaces.
Prefix interfaces with 'I' is not required.
Use JSDoc style comments for interfaces.
Include a clear description of the interface's purpose in JSDoc.
Document all properties inline, alongside the property itself in interfaces.
Document all errors using@throwsin JSDoc.
Document the return type using@returnin JSDoc.
Link using {@link} to types mentioned in JSDoc.
Use PascalCase for enum names.
Use PascalCase for enum members.
Include JSDoc documentation for both enum and members.
Use PascalCase for class names.
Implement interfaces explicitly in classes.
Avoid extension wherever possible in classes.
Document class purpose and implementation details.
Use underscore prefix for private members in classes.
Group class members by visibility (private, protected, public).
Document all public methods and properties in classes.
Use JSDoc for all public methods.
Make use of@param,@returnsand@throwsin method documentation.
Use descriptive parameter names in methods.
Group related parameters into parameter objects in methods.
Use optional parameters with default values when appropriate in methods.
The error type used isErrorInfofrom the package 'ably'.
Use consistent logging levels (trace, debug, error).
Include relevant context in log messages, the second argument is a free-formed object for any relevant context.
Never log instances of Ably channels.
Structure log messages for easy parsing.
Minimize use of type assertions.
Use type guards when possible.
Document when and why type assertions are necessary.
Never useany. If we really can't avoid it, useunknown, but strong typing always preferred.
Use async/await over raw promises.
Handle promise re...
Files:
demo/src/main.tsxtest/helper/environment.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
test/helper/environment.ts
🧠 Learnings (5)
📚 Learning: 2025-07-23T10:06:04.393Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/typescript-conventions.mdc:0-0
Timestamp: 2025-07-23T10:06:04.393Z
Learning: Applies to **/*.{ts,tsx} : When importing the package ably, use `import * as Ably from 'ably'`.
Applied to files:
demo/package.json
📚 Learning: 2025-07-22T15:04:27.777Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-22T15:04:27.777Z
Learning: Applies to **/*.{ts,tsx} : When importing the package ably, do `import * as Ably from 'ably'`.
Applied to files:
demo/package.json
📚 Learning: 2025-06-30T12:57:08.726Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-06-30T12:57:08.726Z
Learning: Applies to test-setup.ts : Skip test environment setup when using the local realtime cluster.
Applied to files:
demo/src/main.tsx
📚 Learning: 2025-08-08T16:24:19.988Z
Learnt from: splindsay-92
PR: ably/ably-chat-js#601
File: test/core/realtime-subscriptions.test.ts:3-4
Timestamp: 2025-08-08T16:24:19.988Z
Learning: Applies to test/**/*.{ts,tsx} in ably/ably-chat-js: Keep explicit .ts/.tsx extensions in import paths within test files; do not switch to .js extensions. This aligns with the project’s Vitest + TS setup and existing test code conventions.
Applied to files:
test/helper/environment.ts
📚 Learning: 2025-06-30T12:57:08.726Z
Learnt from: CR
PR: ably/ably-chat-js#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-06-30T12:57:08.726Z
Learning: Applies to test-setup.ts : Automatically create a new app in the sandbox environment before tests run, handle API key setup, and set `process.env.sandboxApiKey` for test authentication in test setup.
Applied to files:
test/helper/environment.ts
⏰ 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: check-all-package-lock-updated
- GitHub Check: test node 18
- GitHub Check: test node 22
- GitHub Check: demo-app
- GitHub Check: test node 20
- GitHub Check: test node 24
🔇 Additional comments (1)
demo/src/main.tsx (1)
36-41: Local endpoint mapping looks good.Setting endpoint + port 8081 + tls: false matches the local REST setup.
splindsay-92
left a comment
splindsay-92
left a comment
There was a problem hiding this comment.
Looks good to me, but I can't approve this..
- Version field is now an object, it contains the version string, timestamp of this version, and the clientId, description, and metadata fields previously held in the Operation field. - CreatedAt has been updated to timestamp. - Operation has been removed, its contents are now part of the version object. - For the operation endpoints, redundant fields have been removed, so it only returns the message. - For the `send` endpoint, createdAt is renamed to timestamp, and both the serial and timestamp filed are now embedded within a message field to mirror the delete/update endpoints.
…ude serial and timestamp.
AndyTWF
force-pushed
the
chat/move-to-protocol-v4
branch
from
65957d3 to
8e04a86
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/typedoc
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
temporarily deployed
to
staging/pull/620/typedoc
This change updates ably-js to a protocol-v4 capable version and fixes various tests. We will need to pin it to a released version upon release.
environment is deprecated so generating warnings.
AndyTWF
force-pushed
the
chat/move-to-protocol-v4
branch
from
a436e85 to
a9bfaf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
src/core/message.ts (2)
33-58: Public API changes detected. Please update the documentation and the Ably CLI.The
Versioninterface represents a significant API change where version information is now structured as an object instead of a primitive string. This version-centric approach aligns with Protocol V4 requirements.
87-87: Public API changes detected. Please update the documentation and the Ably CLI.The
Messageinterface now usestimestampinstead ofcreatedAtandversionis now aVersionobject instead of a string. These changes align with Protocol V4 specifications.Also applies to: 124-126
test/core/messages-reactions.test.ts (1)
337-337: Inconsistent enum usage in test data.Line 337 uses
MessageReactionType.Uniquewhile the rest of the file consistently usesReactionAnnotationType.Unique. This should be corrected for consistency.Apply this fix for consistency:
- summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } }, + summary: { [ReactionAnnotationType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },Also on line 355:
- summary: { [MessageReactionType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } }, + summary: { [ReactionAnnotationType.Unique]: { '🥦': { total: 1, clientIds: ['user1'] } } },Also applies to: 355-355
src/core/rest-types.ts (2)
17-24: Public API changes detected. Please update the documentation and the Ably CLI.The new
RestVersioninterface replaces the removedRestOperationinterface, consolidating version information with operation metadata. This is a significant API change that requires documentation updates.
33-43: Public API changes detected. Please update the documentation and the Ably CLI.The
RestMessageinterface has been updated to:
- Use
RestVersionfor theversionfield (instead of string)- Remove
createdAtandoperationfields- Keep
timestampat the top levelThis aligns with Protocol V4 requirements.
src/core/chat-api.ts (3)
32-37: Public API changes detected. Please update the documentation and the Ably CLI.The
CreateMessageResponseinterface has been updated:
- Removed
createdAt: number- Added
timestamp: number- Added documentation comments for both fields
This change aligns with Protocol V4.
45-47: Public API changes detected. Please update the documentation and the Ably CLI.The response types have been simplified:
MessageOperationResponseinterface removedUpdateMessageResponseandDeleteMessageResponsenow aliasRestMessageThis consolidation simplifies the API surface and aligns with Protocol V4.
150-150: LGTM! All API endpoints correctly migrated to v4.All Chat API endpoints have been consistently updated from
/chat/v3/to/chat/v4/, including:
- history, getMessage, deleteMessage, sendMessage, updateMessage
- sendMessageReaction, deleteMessageReaction, getClientReactions, getOccupancy
This ensures consistent use of Protocol V4 across all operations.
Also applies to: 179-179, 193-193, 213-213, 220-220, 229-229, 236-236, 248-248, 257-257
🧹 Nitpick comments (1)
test/core/message-parser.test.ts (1)
84-84: Consider adding more comprehensive test for empty version objectWhile testing with an empty version object is valuable, this test case could be more comprehensive by including other required fields to better validate the default handling behavior.
description: 'message.data.metadata is not an object', message: { data: { text: 'hello', metadata: 'not an object' }, + clientId: 'client1', + timestamp: 1728402074206, + serial: '01728402074206-000@cbfkKvEYgBhDaZ38195418:0', + action: ChatMessageAction.MessageCreate, version: {}, }, expectedDefaults: { metadata: {} },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
demo/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.env.test.example(1 hunks)cspell.json(1 hunks)demo/package.json(1 hunks)demo/src/components/MessageComponent/MessageComponent.tsx(1 hunks)demo/src/main.tsx(1 hunks)package.json(1 hunks)src/core/chat-api.ts(9 hunks)src/core/index.ts(1 hunks)src/core/message-parser.ts(3 hunks)src/core/message.ts(10 hunks)src/core/messages-reactions.ts(1 hunks)src/core/messages.ts(3 hunks)src/core/rest-types.ts(3 hunks)test/core/chat-api.test.ts(3 hunks)test/core/message-parser.test.ts(27 hunks)test/core/message.test.ts(38 hunks)test/core/messages-reactions.test.ts(7 hunks)test/core/messages.integration.test.ts(12 hunks)test/core/messages.test.ts(27 hunks)test/core/rest-types.test.ts(11 hunks)test/core/serial.test.ts(1 hunks)test/helper/environment.ts(1 hunks)test/helper/realtime-client.ts(1 hunks)test/react/hooks/use-messages.integration.test.tsx(1 hunks)test/react/hooks/use-messages.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- test/core/chat-api.test.ts
- cspell.json
- src/core/index.ts
- test/react/hooks/use-messages.integration.test.tsx
- src/core/messages-reactions.ts
- demo/src/main.tsx
- demo/package.json
- test/core/serial.test.ts
- .env.test.example
- package.json
- test/core/message.test.ts
- test/helper/environment.ts
- src/core/messages.ts
- test/core/messages.test.ts
- test/core/messages.integration.test.ts
- demo/src/components/MessageComponent/MessageComponent.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: When importing in this project, use relative imports.
When importing the package ably, doimport * as Ably from 'ably'.
Use named exports over default exports.
Group related exports together.
Use PascalCase for interface names.
Use descriptive names that reflect the purpose for interfaces.
Prefix interfaces with 'I' is not required.
Use JSDoc style comments for interfaces.
Include a clear description of the interface's purpose in JSDoc.
Document all properties inline, alongside the property itself in interfaces.
Document all errors using@throwsin JSDoc.
Document the return type using@returnin JSDoc.
Link using {@link} to types mentioned in JSDoc.
Use PascalCase for enum names.
Use PascalCase for enum members.
Include JSDoc documentation for both enum and members.
Use PascalCase for class names.
Implement interfaces explicitly in classes.
Avoid extension wherever possible in classes.
Document class purpose and implementation details.
Use underscore prefix for private members in classes.
Group class members by visibility (private, protected, public).
Document all public methods and properties in classes.
Use JSDoc for all public methods.
Make use of@param,@returnsand@throwsin method documentation.
Use descriptive parameter names in methods.
Group related parameters into parameter objects in methods.
Use optional parameters with default values when appropriate in methods.
The error type used isErrorInfofrom the package 'ably'.
Use consistent logging levels (trace, debug, error).
Include relevant context in log messages, the second argument is a free-formed object for any relevant context.
Never log instances of Ably channels.
Structure log messages for easy parsing.
Minimize use of type assertions.
Use type guards when possible.
Document when and why type assertions are necessary.
Never useany. If we really can't avoid it, useunknown, but strong typing always preferred.
Use async/await over raw promises.
Handle promise re...
Files:
test/helper/realtime-client.tstest/react/hooks/use-messages.test.tsxtest/core/messages-reactions.test.tssrc/core/chat-api.tssrc/core/message-parser.tstest/core/message-parser.test.tstest/core/rest-types.test.tssrc/core/message.tssrc/core/rest-types.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
test/helper/realtime-client.tstest/react/hooks/use-messages.test.tsxtest/core/messages-reactions.test.tstest/core/message-parser.test.tstest/core/rest-types.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Test files should end with.test.ts.
Name test files to reflect the module being tested.
Use clear, descriptive names that explain the test's purpose in test descriptions.
Group related tests usingdescribeblocks.
Use nested describes for sub-features in tests.
Arrange-Act-Assert structure for test cases.
Test both positive and negative cases, including edge cases and boundary conditions.
Use clear, specific assertions in tests.
Create meaningful test data that clearly demonstrates the test case.
Use random room IDs to avoid conflicts in tests.
Use .each to write table/data driven tests.
Use custom matchers like toBeErrorInfo, toThrowErrorInfo, toBeErrorInfoWithCode, toThrowErrorInfoWithCode, toBeErrorInfoWithCauseCode for error testing.
Include an annotation of the specification point in the relevant test methods, e.g// CHA-M10a.
**/*.test.{ts,tsx}: Use the custom matchertoBeErrorInfoto validate that an error matches expected ErrorInfo properties (code, statusCode, message, cause) when testing error scenarios.
Use the custom matchertoThrowErrorInfoto check if a function throws an error matching expected ErrorInfo properties in tests.
Use the custom matchertoBeErrorInfoWithCodeto validate that an error has a specific error code in tests.
Use the custom matchertoThrowErrorInfoWithCodeto check if a function throws an error with a specific code in tests.
Use the custom matchertoBeErrorInfoWithCauseCodeto validate that an error has a cause with a specific error code in tests.
UsewaitForEventualHookValueToBeDefinedutility to wait for a React hook value to become defined in asynchronous hook tests.
UsewaitForEventualHookValueutility to wait for a React hook to return an expected value in asynchronous hook tests.
Usevi.waitForto wait for asynchronous events to happen in tests.
**/*.test.{ts,tsx}: Mock theablylibrary as a module usingvi.mock('ably')in test files that use Ably.
Each test file should ...
Files:
test/react/hooks/use-messages.test.tsxtest/core/messages-reactions.test.tstest/core/message-parser.test.tstest/core/rest-types.test.ts
**/*.test.tsx
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use waitForEventualHookValueToBeDefined and waitForEventualHookValue for testing async React hooks.
Files:
test/react/hooks/use-messages.test.tsx
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/unit-testing.mdc)
Test files should end with
.test.tsand the name should reflect the module being tested (e.g.,message.test.tsformessage.ts).
Files:
test/core/messages-reactions.test.tstest/core/message-parser.test.tstest/core/rest-types.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/file-conventions.mdc)
Create all Typescript and TSX files using kebab-case.
Files:
src/core/chat-api.tssrc/core/message-parser.tssrc/core/message.tssrc/core/rest-types.ts
⚙️ CodeRabbit configuration file
src/**/*.{ts,tsx}: If there are any changes to the public-facing API, add a review comment: "Public API changes detected. Please update the documentation and the Ably CLI."
Files:
src/core/chat-api.tssrc/core/message-parser.tssrc/core/message.tssrc/core/rest-types.ts
🧬 Code graph analysis (7)
test/helper/realtime-client.ts (1)
test/helper/environment.ts (1)
testEndpoint(3-15)
test/core/messages-reactions.test.ts (1)
src/core/index.ts (2)
ChatMessageAction(22-22)MessageReactionType(25-25)
src/core/chat-api.ts (3)
src/core/rest-types.ts (1)
RestMessage(34-44)src/core/messages-reactions.ts (1)
DeleteMessageReactionParams(65-77)src/core/occupancy-parser.ts (1)
OccupancyData(6-16)
src/core/message-parser.ts (2)
src/core/message.ts (1)
Version(33-58)src/core/index.ts (1)
Version(42-42)
test/core/rest-types.test.ts (2)
src/core/rest-types.ts (2)
messageFromRest(51-82)RestMessage(34-44)src/core/message.ts (2)
DefaultMessage(315-516)emptyMessageReactions(522-526)
src/core/message.ts (1)
src/core/operation-metadata.ts (1)
OperationMetadata(10-10)
src/core/rest-types.ts (1)
src/core/message.ts (1)
DefaultMessage(315-516)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (27)
src/core/message.ts (7)
60-64: LGTM! Well-structured helper type for version comparisons.The
VersionWithSerialhelper type effectively enforces that version comparisons can only occur when both messages have defined version serials, preventing runtime errors in comparison operations.
305-305: LGTM! Parameter and property types correctly updated.The
DefaultMessageParamsandDefaultMessageclass have been properly updated to use the newVersiontype instead of strings, maintaining type safety throughout the codebase.Also applies to: 322-322
347-347: Good practice: Freezing version object for immutability.Adding
Object.freeze(this.version)ensures the version object remains immutable after construction, which is consistent with the readonly contract of the Message interface.
364-377: LGTM! Getters correctly updated to use version properties.The getter methods now properly extract
clientIdandtimestampfrom theversionobject instead of legacy operation properties, maintaining backward compatibility for consumers.
379-409: Well-implemented version comparison logic with proper safety checks.The version comparison methods now use the new
_compareVersionshelper which ensures both messages are the same message and have defined version serials before comparison. This prevents runtime errors and provides more reliable version ordering.
457-474: Robust version comparison helper with appropriate safety measures.The
_compareVersionsmethod correctly validates that messages are the same (viaequal()) and have defined version serials before applying the comparator function. This prevents invalid comparisons between different messages or messages without version information.
507-507: LGTM! Deep cloning ensures version object isolation.Using
cloneDeep(source.version)prevents unintended mutations between message copies, which is important since version objects may contain nested metadata.test/helper/realtime-client.ts (1)
4-4: LGTM! Configuration updated for new environment setup.The changes correctly update the import to use
testEndpointinstead oftestEnvironmentand setoptions.endpointinstead ofoptions.environment. This aligns with the broader endpoint-based configuration approach.Also applies to: 10-10
test/core/messages-reactions.test.ts (1)
216-224: Test data correctly updated for version-centric structure.The test payloads have been properly updated to use:
versionas an object with{ serial, timestamp }structureannotations.summaryfor reaction summary data- Consistent use of
ReactionAnnotationTypeenumsThis correctly reflects the Protocol V4 message structure.
Also applies to: 229-237, 242-252, 257-275
test/react/hooks/use-messages.test.tsx (1)
124-128: Test expectations correctly updated for version object structure.The test data has been properly updated to use the new
versionobject structure with{ serial, timestamp }instead of a simple string, and theDefaultMessageconstruction correctly reflects the new API withoutcreatedAt.Also applies to: 204-204
test/core/rest-types.test.ts (2)
11-14: Test cases correctly updated for RestVersion structure.All test cases have been properly updated to use the new
RestVersionobject structure:
versionfield now contains{ serial, timestamp }object- Operation-related data (clientId, description, metadata) now properly nested within version
- Tests correctly verify the new structure including
result.version.serialand version timestamp conversionThese changes accurately reflect the Protocol V4 REST API changes.
Also applies to: 27-27, 40-43, 86-89, 106-114, 125-131, 136-139, 178-182, 214-217, 257-260, 320-323, 356-359, 376-379, 402-405, 423-426, 477-480, 498-501
392-392: Verify version timestamp handling consistency.The tests correctly access
result.version.serialand checkresult.version.timestampconversion to Date objects. This properly validates the RestVersion structure and timestamp handling.Also applies to: 492-492
src/core/rest-types.ts (2)
62-69: LGTM! Version construction properly handles optional fields.The version object construction correctly maps all RestVersion fields including optional
clientId,description, andmetadata, with proper timestamp conversion from number to Date.
71-81: LGTM! DefaultMessage construction updated for new API.The
DefaultMessageconstructor call has been properly updated to use the new version object structure and maintains all required fields for the Protocol V4 message format.src/core/message-parser.ts (4)
4-4: Import correctly updated for new Version type.The import statement has been properly updated to include
Versionfrom the message module and removed the unusedOperationimport, reflecting the API changes.
19-21: Payload structure updated for Protocol V4.The
MessagePayloadinterface has been correctly updated to:
- Use
Ably.MessageVersionfor the version field- Add required
annotations: Ably.MessageAnnotationsThis aligns with Protocol V4 message structure requirements.
36-45: Robust version object construction with optional timestamp handling.The version construction logic properly handles cases where
message.version.timestampmay be undefined, only converting to Date when the timestamp is actually present. The conditional spread operator ensures the version object maintains the correct structure.
56-66: DefaultMessage construction correctly updated for new structure.The
DefaultMessageconstructor call has been properly updated to use the constructedversionobject instead of a string, and all other fields are correctly mapped including the preservedtimestampfield.test/core/message-parser.test.ts (9)
19-22: LGTM - Version structure correctly updated to Protocol V4The test fixture correctly uses the new nested version object structure with
serialandtimestampfields, aligning with Protocol V4 changes.
137-156: Verify the version defaults test implementationThe test description says "message.version.timestamp is undefined" but the version object is completely empty. The test correctly validates that all version subfields default to undefined when the version object lacks those properties.
352-352: LGTM - Timestamp field correctly updatedThe assertion correctly validates the new
timestampfield structure, replacing the oldcreatedAtfield as per Protocol V4 changes.
355-356: LGTM - Version object structure correctly validatedThe assertions properly validate the nested version object structure with
serialandtimestampfields, aligning with Protocol V4 requirements.
398-398: LGTM - Consistent timestamp usage across test casesAll test assertions correctly use the
timestampfield and convert version timestamps to Date objects, maintaining consistency with the Protocol V4 changes throughout all message types (create, update, delete).Also applies to: 401-401, 444-444, 447-447, 487-487, 490-490, 531-531, 534-534
404-408: LGTM - Comprehensive version object validationThe test properly validates all version object fields including
serial,timestamp,clientId,description, andmetadata, ensuring complete Protocol V4 compliance.
433-435: Verify annotations.summary requirement for deleted messagesDeleted-message tests include annotations.summary (test/core/message-parser.test.ts: 433–435, 475–477, 519–521). src/core/messages-reactions.ts reads event.annotations.summary — confirm whether annotations.summary must always be present for deleted messages; if required, enforce/validate it, otherwise make tests and runtime tolerant of its absence.
387-389: Verify annotations field usage in message parsingConfirm parseMessage validates and preserves annotations.summary as used in test/core/message-parser.test.ts (lines 387–389):
annotations: { summary: {}, },Repo search with the supplied script returned no matches for parseMessage — run a targeted search for
parseMessageor provide the message-parser implementation so this can be verified.
312-328: Adjust test to omitversion.timestamp
The test description targets the “timestamp undefined” branch, but themessageobject still includesversion.timestamp. Remove or explicitly setversion.timestamptoundefinedso thatparseMessagefalls back to the current‐time default.
|
I've reviewed the last few commits and they look good, lets merge 👍 |
AndyTWF
left a comment
AndyTWF
left a comment
There was a problem hiding this comment.
@splindsay-92 has approved the last few commits, so we'll merge this
Context
Pubsub will be moving to Pv4 soon, where appropriate, we should modify the chat message structure to match.
This will be a breaking change and require new version REST endpoints.
CHA-1101
Description
sendendpoint, createdAt is renamed to timestamp, and both the serial and timestamp filed are now embedded within a message field to mirror the delete/update endpoints.Checklist
Testing Instructions (Optional)
Summary by CodeRabbit
New Features
Refactor
Chores
Tests