[DX-725] Add extras in the message for channel push notifications#134
[DX-725] Add extras in the message for channel push notifications#134
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe changes add support for including extras.push in the Ably channel publish command payload. Documentation is updated to reflect this capability, implementation extracts extras from messageData and assigns them to the message's extras field, and tests verify the extras.push content passes through correctly to the channel publish call. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
| '$ ably channels publish --transport realtime my-channel "Using realtime transport"', | ||
| '$ ably channels publish my-channel "Hello World" --json', | ||
| '$ ably channels publish my-channel "Hello World" --pretty-json', | ||
| '$ ably channels publish my-channel \'{"data":"Push notification","extras":{"push":{"notification":{"title":"Hello","body":"World"}}}}\'', |
There was a problem hiding this comment.
Should add this as an --extras flag to the command? Right now what comes after my-channel is assumed to be the entire data field so implicitly taking something out of it might be confusing?
There was a problem hiding this comment.
I thought that when you type it like publish my-channel "Hello World" - it's a data for the message, but if you specify the whole message like this publish my-channel \'{"data":"Push notification","extras":{"push..., then whatever can be inside the message.
There was a problem hiding this comment.
Oops yes I misread that, that now makes sense!
There was a problem hiding this comment.
@AndyTWF should we go with this or changes still needed?
maratal
force-pushed
the
channel-push-support
branch
from
6a7494b to
207544c
Compare
maratal
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for including push notification metadata via the extras.push field in channel messages published through the CLI. The implementation extracts the extras object from JSON message input and includes it as a top-level message property, following the same pattern used for the name field.
Changes:
- Added
extrasextraction logic toprepareMessagemethod in the publish command - Added unit test to verify extras.push is correctly forwarded in publish calls
- Updated documentation to reflect the new extras.push capability
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/commands/channels/publish.ts | Extracts extras from message JSON and assigns it to message object, removes it from messageData to avoid duplication. Adds example showing extras.push usage. |
| test/unit/commands/channels/publish.test.ts | Adds test case verifying that extras.push is correctly included when provided in message data |
| docs/Product-Requirements.md | Updates command description to mention support for extras.push in message payload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add extras if provided in the message data (before processing data) | ||
| if ("extras" in messageData) { | ||
| message.extras = messageData.extras; | ||
| // Remove extras from messageData to avoid duplication in data | ||
| delete messageData.extras; | ||
| } |
There was a problem hiding this comment.
When a message JSON contains extras (and potentially name) but no explicit data field, the code will set message.data to an empty object after extracting extras and name. This happens because the extraction of extras happens before the check for whether data exists in messageData.
For example, if a user provides {"extras":{"push":{...}}}, after extracting extras, messageData becomes {}, and then line 221-222 assigns this empty object as message.data.
Consider checking if messageData still has any properties after extracting extras and name, and only then deciding whether to use it as data. Alternatively, extract extras after the data processing logic.
There was a problem hiding this comment.
@coderabbitai wdyt above above comment? Give me detailed explaination for the same
There was a problem hiding this comment.
@sacOO7 Great question! Let me walk you through Copilot's concern in detail.
The Issue
Copilot is pointing out a potential edge case in the current extraction logic. Here's what happens step by step:
Current Code Flow
// 1. Extract name (if present)
if (messageData.name) {
message.name = messageData.name;
delete messageData.name; // mutates messageData
}
// 2. Extract extras (if present) - NEW CODE
if ("extras" in messageData) {
message.extras = messageData.extras;
delete messageData.extras; // mutates messageData
}
// 3. Assign data
if ("data" in messageData) {
message.data = messageData.data;
} else {
message.data = messageData; // Use whatever is left
}The Problem Scenario
Input: {"extras":{"push":{"notification":{"title":"Hello"}}}}
Step-by-step execution:
- No
namefield → messageData stays as{"extras":{...}} - Extract
extras→ messageData becomes{}(empty object) - Check for
datafield → not found - Fall into else branch →
message.data = {}❌
Result:
{
data: {}, // Empty object - probably not intended!
extras: { push: {...} }
}Why This Matters
The question is: Should a message with only extras (no explicit data) have:
data: {}(empty object) ← current behaviordata: undefined(no data field) ← possibly more correct
An empty object might be treated differently by the Ably SDK compared to an undefined/missing data field.
Possible Solutions
Option 1: Check if messageData has content after extractions
if ("extras" in messageData) {
message.extras = messageData.extras;
delete messageData.extras;
}
if ("data" in messageData) {
message.data = messageData.data;
} else if (Object.keys(messageData).length > 0) {
// Only use messageData as data if there's something left
message.data = messageData;
}Option 2: Don't mutate messageData for extras
if ("data" in messageData) {
message.data = messageData.data;
} else {
const { name, extras, ...rest } = messageData;
if (Object.keys(rest).length > 0) {
message.data = rest;
}
}
if ("extras" in messageData) {
message.extras = messageData.extras;
}Questions for You
- Is this edge case valid? Should users be able to send messages with only
extras(nodata)? - What's the expected behavior? Should such messages have an empty
data: {}or no data field at all? - Looking at your PR example, you always include
data, but edge cases might occur in production.
What are your thoughts on this? Should we add a guard to handle the empty messageData case?
There was a problem hiding this comment.
@maratal please check what can go wrong here
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| expect(publishArgs.extras.push).toEqual({ | ||
| notification: { title: "Test", body: "Push notification" }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test only covers the happy path where both data and extras are provided. Consider adding test cases for edge cases:
- When only
extrasis provided withoutdata(should this be valid?) - When
extrasis provided withnamebut withoutdata - When
extrascontains fields other thanpushto ensure the entire extras object is preserved - When extras is an empty object or has invalid structure
These edge cases will help ensure the implementation handles all scenarios correctly.
| }); | |
| }); | |
| it("should publish a message when only extras is provided without data", async function () { | |
| const restMock = getMockAblyRest(); | |
| const channel = restMock.channels._getChannel("test-channel"); | |
| await runCommand( | |
| [ | |
| "channels:publish", | |
| "test-channel", | |
| '{"extras":{"push":{"notification":{"title":"Extras only","body":"No data field"}}}}', | |
| "--transport", | |
| "rest", | |
| ], | |
| import.meta.url, | |
| ); | |
| expect(channel.publish).toHaveBeenCalledOnce(); | |
| const publishArgs = channel.publish.mock.calls[0][0]; | |
| expect(publishArgs).toHaveProperty("extras"); | |
| expect(publishArgs.extras).toHaveProperty("push"); | |
| expect(publishArgs.extras.push).toEqual({ | |
| notification: { title: "Extras only", body: "No data field" }, | |
| }); | |
| expect(publishArgs).not.toHaveProperty("data"); | |
| }); | |
| it("should preserve name when extras is provided without data", async function () { | |
| const restMock = getMockAblyRest(); | |
| const channel = restMock.channels._getChannel("test-channel"); | |
| await runCommand( | |
| [ | |
| "channels:publish", | |
| "test-channel", | |
| '{"name":"eventName","extras":{"push":{"notification":{"title":"With name","body":"No data field"}}}}', | |
| "--transport", | |
| "rest", | |
| ], | |
| import.meta.url, | |
| ); | |
| expect(channel.publish).toHaveBeenCalledOnce(); | |
| const publishArgs = channel.publish.mock.calls[0][0]; | |
| expect(publishArgs).toHaveProperty("name", "eventName"); | |
| expect(publishArgs).toHaveProperty("extras"); | |
| expect(publishArgs.extras).toHaveProperty("push"); | |
| expect(publishArgs.extras.push).toEqual({ | |
| notification: { title: "With name", body: "No data field" }, | |
| }); | |
| expect(publishArgs).not.toHaveProperty("data"); | |
| }); | |
| it("should preserve all fields within extras, not just push", async function () { | |
| const restMock = getMockAblyRest(); | |
| const channel = restMock.channels._getChannel("test-channel"); | |
| await runCommand( | |
| [ | |
| "channels:publish", | |
| "test-channel", | |
| '{"data":"hello","extras":{"push":{"notification":{"title":"Test","body":"Push notification"}},"foo":"bar","nested":{"key":"value"}}}', | |
| "--transport", | |
| "rest", | |
| ], | |
| import.meta.url, | |
| ); | |
| expect(channel.publish).toHaveBeenCalledOnce(); | |
| const publishArgs = channel.publish.mock.calls[0][0]; | |
| expect(publishArgs).toHaveProperty("data", "hello"); | |
| expect(publishArgs).toHaveProperty("extras"); | |
| expect(publishArgs.extras).toHaveProperty("push"); | |
| expect(publishArgs.extras.push).toEqual({ | |
| notification: { title: "Test", body: "Push notification" }, | |
| }); | |
| expect(publishArgs.extras).toHaveProperty("foo", "bar"); | |
| expect(publishArgs.extras).toHaveProperty("nested"); | |
| expect(publishArgs.extras.nested).toEqual({ key: "value" }); | |
| }); | |
| it("should handle an empty extras object", async function () { | |
| const restMock = getMockAblyRest(); | |
| const channel = restMock.channels._getChannel("test-channel"); | |
| await runCommand( | |
| [ | |
| "channels:publish", | |
| "test-channel", | |
| '{"data":"hello","extras":{}}', | |
| "--transport", | |
| "rest", | |
| ], | |
| import.meta.url, | |
| ); | |
| expect(channel.publish).toHaveBeenCalledOnce(); | |
| const publishArgs = channel.publish.mock.calls[0][0]; | |
| expect(publishArgs).toHaveProperty("data", "hello"); | |
| expect(publishArgs).toHaveProperty("extras"); | |
| expect(publishArgs.extras).toEqual({}); | |
| }); |
It is not support of push notifications with deviceId or clientId. Just have added
extras.pushsupport for channel pushes. Tried locally with./bin/run.js channels publish exampleChannel1 '{"data":"Test","extras":{"push":{"notification":{"title":"Hello","body":"World!"}}}}' --api-key "***"and it works.
Note
Low Risk
Small, additive change to message construction plus a unit test and docs update; minimal impact outside publish payload shape.
Overview
ably channels publishnow extracts anextrasobject from JSON message input and sends it asmessage.extras(removing it from thedatapayload to avoid duplication), enablingextras.pushpush-notification metadata to be included with published messages.Documentation and examples were updated to mention
extras.push, and a unit test was added to assertextras.pushis forwarded correctly in the publish call.Written by Cursor Bugbot for commit 207544c. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests