Skip to content

feat(push): support realtime message data in batch-publish channel items#355

Closed
maratal wants to merge 1 commit intofix/push-publish-message-shapefrom
fix/345-batch-publish-to-channel
Closed

feat(push): support realtime message data in batch-publish channel items#355
maratal wants to merge 1 commit intofix/push-publish-message-shapefrom
fix/345-batch-publish-to-channel

Conversation

@maratal
Copy link
Copy Markdown
Contributor

@maratal maratal commented Apr 20, 2026

Summary

Depends on #352.

  • Channel-based batch items may include an optional "message" field whose value is published as the realtime message alongside the push notification in extras.push
  • Uses the existing prepareMessageFromInput from message.ts (via JSON.stringify) — no new utility needed
  • The message field follows the same shape as --message in push publish (PR fix(push publish): parse --message as a full Ably message shape #352): name and data are extracted; if only name is present, remaining fields become data

Example

[
  {
    "channels": ["my-channel"],
    "payload": {"notification": {"title": "Hello", "body": "World"}},
    "message": {"name": "alert", "data": "Server down"}
  }
]

Test plan

  • pnpm test:unit passes (21 tests in batch-publish, covering string message, object with name+data, object with name-only, no message)
  • pnpm exec eslint . clean

🤖 Generated with Claude Code

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cli-web-cli Ready Ready Preview, Comment Apr 20, 2026 3:50pm

Request Review

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for including an optional realtime message payload when using push:batch-publish with channel-based batch items (routing via /messages with extras.push), aligning the message parsing semantics with prepareMessageFromInput() used elsewhere in the CLI.

Changes:

  • Extend push:batch-publish channel items to accept an optional message field and publish it alongside the push payload via extras.push.
  • Reuse prepareMessageFromInput() to derive realtime name, data, and optional extras from the provided message.
  • Add unit tests covering string message data, message objects with name+data, name-only objects, and absence of message.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/commands/push/batch-publish.ts Adds parsing/merging logic for optional message in channel-routed batch items and updates CLI examples/arg description.
test/unit/commands/push/batch-publish.test.ts Adds unit tests validating the new message behavior for channel batch items.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/commands/push/batch-publish.ts Outdated
Comment thread test/unit/commands/push/batch-publish.test.ts Outdated
@umair-ably umair-ably force-pushed the fix/push-publish-message-shape branch from 47411f5 to 295a823 Compare April 20, 2026 09:28
fix(push publish): parse --message as a full Ably message shape
An error occurred while trying to automatically change base from fix/push-publish-message-shape to main April 20, 2026 11:47
@maratal maratal force-pushed the fix/345-batch-publish-to-channel branch from c0ddf56 to 2761a92 Compare April 20, 2026 12:02
@maratal maratal requested a review from Copilot April 20, 2026 12:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/commands/push/batch-publish.ts Outdated
Comment thread src/commands/push/batch-publish.ts Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/commands/push/batch-publish.ts Outdated
Comment thread test/unit/commands/push/batch-publish.test.ts Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@maratal maratal force-pushed the fix/345-batch-publish-to-channel branch from ab90c88 to dc2e45a Compare April 20, 2026 14:58
@maratal maratal marked this pull request as ready for review April 20, 2026 14:59
@maratal maratal requested review from sacOO7 and umair-ably April 20, 2026 14:59
@claude-code-ably-assistant
Copy link
Copy Markdown

Walkthrough

This PR extends push batch-publish to support an optional message field on channel-based batch items, allowing a realtime Ably message to be published alongside the push notification. The message field is parsed using the existing prepareMessageFromInput utility (same shape as --message in push publish), and the resulting name/data/extras are merged into the outgoing /messages payload with extras.push preserved from the item's payload field. Recipient-based items silently drop the message field with a warning, as it is not applicable there.

Changes

Area Files Summary
Commands src/commands/push/batch-publish.ts Added message field handling for channel items: parses via prepareMessageFromInput, merges name/data/extras into the message spec, guards against extras.push conflicts, strips and warns on recipient items; updated arg description and added example
Tests test/unit/commands/push/batch-publish.test.ts Added 8 new unit tests covering string message, object with name+data, name-only (remainder → data), non-push extras preservation, extras.push conflict error, plain object as data, recipient-item stripping with warning, and no-message baseline

Review Notes

  • Behavioral change: Channel batch items now accept a new optional message key. Payloads that previously included an unrecognised message field would have passed it through silently; they now have it parsed and applied. This is unlikely to affect existing callers but is worth noting.
  • Error path for extras.push conflict: If a message contains extras.push, this.fail() is called mid-map — the remaining items in the array are never processed and the entire batch request is aborted. Reviewers should confirm this is the intended UX.
  • prepareMessageFromInput receives JSON.stringify(entry.message): The field can be a string or an object; JSON.stringify normalises both before passing to the parser. Worth verifying edge cases like message: null or message: 42 don't cause unexpected behaviour (not currently tested).
  • No new dependencies — reuses prepareMessageFromInput from src/utils/message.ts.

Copy link
Copy Markdown

@claude-code-ably-assistant claude-code-ably-assistant Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

File Status Issues
src/commands/push/batch-publish.ts OK
test/unit/commands/push/batch-publish.test.ts OK

Implementation correctness

The this.fail() call inside the .map() callback (line 330) is the right approach. Because fail() always throws (returns never), the exception propagates out of .map() to the outer try/catch, which calls this.fail() again on the caught error. The base class handles oclif re-throws automatically — no double JSON error envelope, no double output. The test at line 166 verifies the API is never called when the guard fires, confirming the abort-before-request behavior is correct.

The JSON.stringify(entry.message) round-trip into prepareMessageFromInput is the right technique for reusing the existing utility. Since entry.message comes from JSON.parse(jsonString) (user input), circular references are impossible, and the serialization handles all types correctly: string → {data: string}, number/array/null → {data: value}, plain object → structured message.

The asymmetry between recipient items (warn + strip) and channel items (actively process) is intentional and clearly communicated to users.

Error handling

  • this.fail() used everywhere, no this.error() direct calls
  • Component string "pushBatchPublish" is camelCase
  • No new Error(...) wrapping in this.fail()
  • No manual oclif re-throw guard in the catch block
  • this.logWarning() used correctly for the non-fatal recipient warning

Test coverage

8 new tests cover all documented cases: string message, object with name+data, name-only (remaining fields become data), non-push extras preserved, extras.push conflict rejection, plain object as data, recipient strip+warn, and no-message baseline. All 5 required describe blocks are present (3 explicit + 2 via standard helpers). No auth flags, no --duration in unit test args.

No issues found. The feature is implemented correctly and the test suite is thorough.

@maratal
Copy link
Copy Markdown
Contributor Author

maratal commented Apr 20, 2026

Closing in favour of #358 which is based on main instead of the now-merged fix/push-publish-message-shape branch.

@maratal maratal closed this Apr 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants