Skip to content

fix(push publish): parse --message as a full Ably message shape#352

Merged
umair-ably merged 3 commits intomainfrom
fix/push-publish-message-shape
Apr 20, 2026
Merged

fix(push publish): parse --message as a full Ably message shape#352
umair-ably merged 3 commits intomainfrom
fix/push-publish-message-shape

Conversation

@umair-ably
Copy link
Copy Markdown
Collaborator

@umair-ably umair-ably commented Apr 17, 2026

Summary

  • ably push publish --message now parses JSON input as a full Ably message (name, data, extras) instead of always stuffing the parsed value into data. This matches the semantics of ably channels publish and makes it possible to set the realtime message's name.
  • Reuses the existing prepareMessageFromInput() helper in src/utils/message.ts — no new utility.
  • The CLI-built push payload is still attached as extras.push on the published message; any user-supplied extras keys are merged alongside it.
  • If --message already contains extras.push, the command fails with a clear error (the CLI owns that field via --title / --body / --payload / etc.).
  • JSON output (--json) now includes messageName and messageData when present, making the result self-describing (replaces the previous messageData: true boolean sentinel).

Follow-up to #310 (addresses the reviewer comment that --message couldn't set the message name).

Behaviour change ⚠️

Moving to prepareMessageFromInput means any JSON object that already contains top-level name, data, or extras keys is now treated as a full Ably message shape. This is the point of the change, but two cases are worth calling out explicitly:

--message input Before After
'hello' (plain string) data: "hello" data: "hello" — unchanged
'{"key":"val"}' (object, no reserved keys) data: { key: "val" } data: { key: "val" } — unchanged
'{"name":"alert","data":"hi"}' data: { name: "alert", data: "hi" } name: "alert", data: "hi" — new capability
'{"data":"hi"}' data: { data: "hi" } data: "hi" — inner value unwrapped
'{"extras":{"headers":{...}}}' data: { extras: {...} } extras: { headers: {...}, push: ... } — merged
'{"extras":{"push":{...}}}' data: { extras: { push: {...} } } Error — CLI owns extras.push

Callers who relied on the previous (arguably incorrect) behaviour where {"data":"x"} was stored as data: { data: "x" } will see the inner value unwrapped instead. The new shape matches channels publish and how a plain Ably realtime subscriber expects messages.

Test plan

  • pnpm prepare (build + manifest) succeeds
  • pnpm exec eslint src/commands/push/publish.ts test/unit/commands/push/publish.test.ts — 0 errors
  • pnpm test test/unit/commands/push/publish.test.ts — 32/32 pass, including:
    • full message object sets name and data
    • bare object without reserved keys stays wrapped as data
    • top-level data key is unwrapped (behaviour-change assertion)
    • user extras merged with push
    • extras.push collision fails with clear error, publish is not called
    • JSON output surfaces messageName + messageData
  • pnpm test:unit — full suite green
  • Manual smoke against a real Ably app (recommended before merge):
    • ably push publish --channel demo --title Hi --message '{"name":"alert","data":"hi"}' --force — verify a realtime subscriber sees name === "alert" and data === "hi".
    • ably push publish --channel demo --title Hi --message '{"extras":{"push":{"notification":{"title":"x"}}}}' --force — verify the collision error and non-zero exit.

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 17, 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 9:28am

Request Review

@claude-code-ably-assistant
Copy link
Copy Markdown

Walkthrough

This PR fixes ably push publish --message to correctly parse JSON input as a full Ably message shape (name, data, extras) instead of always stuffing the entire parsed value into data. It reuses the existing prepareMessageFromInput() helper and adds a guard that rejects --message values containing extras.push (a field owned by the CLI's push flags). The fix aligns push publish semantics with channels publish.

Changes

Area Files Summary
Commands src/commands/push/publish.ts Parse --message as full message shape; merge user extras with push payload; add extras.push collision guard; surface messageName in JSON output
Tests test/unit/commands/push/publish.test.ts 5 new cases: full message object, extras merge, messageName/messageData in JSON output, extras.push collision error + publish not called

Review Notes

  • Behavioral change: --message '{"name":"alert","data":"hi"}' previously published { data: { name: "alert", data: "hi" } }; it now publishes { name: "alert", data: "hi" }. This is intentional and documented but is a breaking change for any automation relying on the old (incorrect) behavior.
  • Extras merge order: User-supplied extras keys are spread before push ({ ...userExtras, push: payload }), meaning extras.push from the user is silently overwritten by the collision guard before the spread — the guard fires first and exits, so the spread is safe. Worth a second read to confirm the control flow is clear.
  • No new dependencies: reuses prepareMessageFromInput() from src/utils/message.ts.
  • Manual smoke test pending: The PR author flagged one manual step (verify a realtime subscriber sees name === "alert") as recommended before merge — not yet confirmed.

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/publish.ts 1 issue Undocumented behavior change
test/unit/commands/push/publish.test.ts 1 issue Missing test for changed edge case

The core feature is correctly implemented — error handling, base class, output helpers, JSON structure, and flag architecture all look good. One behavioral change is not covered by tests and is inconsistent with the PR description's claim.


src/commands/push/publish.ts + test/unit/commands/push/publish.test.ts

Undocumented behavior change for bare objects containing a data key

prepareMessageFromInput treats a plain-object JSON value as an Ably message shape, which means it unwraps the data field when present:

--message '{"data":"hello"}'

Old behavior → publishMessage.data = { data: "hello" } (whole object stored as data)
New behavior → publishMessage.data = "hello" (inner value extracted)

The PR description says "Plain string and bare-object --message values keep existing behaviour" and lists '{"key":"val"}' as an example. That example is accurate — when the object has no name, data, or extras keys, the whole object becomes data via prepareMessageFromInput's else if branch, matching the old result. But '{"data":"something"}' falls into the "data" in messageData branch, which extracts just the inner value.

The same gap exists in the test suite. The existing "should parse JSON message data when publishing via channel" test uses '{"key":"val"}' (no special keys, unchanged behavior), not '{"data":"val"}' (unwrapped, changed behavior). None of the new tests cover it either.

Why it matters: a user who had --message '{"data":{"key":"val"}}' expecting the subscriber to receive data === { data: { key: "val" } } will now receive data === { key: "val" }. This may be the correct semantic (matching channels publish), but it is a breaking change that is not documented or tested.

Suggested fix: Add a test case that asserts the new behavior explicitly, and update the PR description to accurately describe this change. For example:

it("should extract data field when --message object has a data key", async () => {
  const mock = getMockAblyRest();
  const channel = mock.channels._getChannel("my-channel");

  await runCommand(
    ["push:publish", "--channel", "my-channel", "--title", "Hi",
     "--message", '{"data":"extracted"}', "--force"],
    import.meta.url,
  );

  // data is extracted from the wrapper, not stored as { data: "extracted" }
  expect(channel.publish).toHaveBeenCalledWith(
    expect.objectContaining({ data: "extracted" }),
  );
});

Everything else looks correct: this.fail() usage, component casing ("pushPublish"), the extras.push collision guard, the extras merge order ({ ...userExtras, push: payload } keeps CLI-owned push last), JSON domain nesting under notification, and logSuccessMessage placement outside the JSON path.

Copy link
Copy Markdown
Contributor

@maratal maratal left a comment

Choose a reason for hiding this comment

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

A suggestion.

Comment thread src/commands/push/publish.ts Outdated
Reuses prepareMessageFromInput (already used by channels publish) so
JSON input can set name, data, and extras — not just data. The CLI
push payload is still attached as extras.push; user-supplied extras
are merged, and extras.push in --message is rejected with a clear
error since the CLI owns that field via its push flags.

JSON output includes messageName and messageData when present.
Explicitly assert that --message '{"data":"x"}' now unwraps to
data: "x" rather than data: { data: "x" } — a behaviour change
from the previous JSON.parse-into-data implementation.
Copy link
Copy Markdown
Contributor

@maratal maratal left a comment

Choose a reason for hiding this comment

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

LGTM

@umair-ably umair-ably merged commit 4d5370b into main Apr 20, 2026
11 checks passed
@maratal maratal linked an issue Apr 20, 2026 that may be closed by this pull request
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.

Push publish message flag

2 participants