Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces a comprehensive push notification management system to the CLI, including device registration commands, channel subscription management, APNs and FCM configuration, and push publishing capabilities. The deprecated set-apns-p12 command is hidden with a migration warning. Service layer updates extend the control API App interface with push-related fields, and extensive unit and end-to-end test suites provide coverage for all new commands. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
Contributor
Author
|
@coderabbitai review |
There was a problem hiding this comment.
Pull request overview
Adds new CLI capabilities for Ably Push Notifications (publish, batch publish, device registrations, channel subscriptions, and push config via Control API) and extends Chat room message management with update/delete commands, with accompanying unit/E2E tests and documentation updates.
Changes:
- Introduces a new
ably push ...command group (publish, batch-publish, devices, channels, config show/set/clear). - Adds
ably rooms messages updateandably rooms messages deleteChat commands. - Updates Control API app model/update method, test mocks/fixtures, and CLI docs; deprecates/hides
apps set-apns-p12.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/rooms/messages/update.test.ts | Unit tests for rooms:messages:update (flags, JSON output, errors). |
| test/unit/commands/rooms/messages/delete.test.ts | Unit tests for rooms:messages:delete (details, JSON output, errors). |
| test/unit/commands/push/publish.test.ts | Unit tests for push:publish. |
| test/unit/commands/push/devices/save.test.ts | Unit tests for push:devices:save. |
| test/unit/commands/push/devices/remove.test.ts | Unit tests for push:devices:remove. |
| test/unit/commands/push/devices/remove-where.test.ts | Unit tests for push:devices:remove-where. |
| test/unit/commands/push/devices/list.test.ts | Unit tests for push:devices:list. |
| test/unit/commands/push/devices/get.test.ts | Unit tests for push:devices:get (token redaction). |
| test/unit/commands/push/config/show.test.ts | Unit tests for push:config:show Control API rendering/JSON. |
| test/unit/commands/push/config/set-fcm.test.ts | Unit tests for push:config:set-fcm (file validation). |
| test/unit/commands/push/config/set-apns.test.ts | Unit tests for push:config:set-apns (p8/p12 flows). |
| test/unit/commands/push/config/clear-fcm.test.ts | Unit tests for push:config:clear-fcm. |
| test/unit/commands/push/config/clear-apns.test.ts | Unit tests for push:config:clear-apns. |
| test/unit/commands/push/channels/save.test.ts | Unit tests for push:channels:save. |
| test/unit/commands/push/channels/remove.test.ts | Unit tests for push:channels:remove. |
| test/unit/commands/push/channels/remove-where.test.ts | Unit tests for push:channels:remove-where. |
| test/unit/commands/push/channels/list.test.ts | Unit tests for push:channels:list. |
| test/unit/commands/push/channels/list-channels.test.ts | Unit tests for push:channels:list-channels. |
| test/unit/commands/push/batch-publish.test.ts | Unit tests for push:batch-publish. |
| test/helpers/mock-ably-rest.ts | Extends REST mock to support new push admin APIs. |
| test/helpers/mock-ably-chat.ts | Extends Chat mock to support message update/delete. |
| test/fixtures/push/test-fcm-service-account.json | Test fixture for FCM service account validation. |
| test/fixtures/push/test-apns-key.p8 | Test fixture for APNs P8 key flows. |
| test/e2e/push/push-config-e2e.test.ts | E2E coverage for push config show/set/clear flows. |
| src/utils/output.ts | Adds formatDeviceState helper for push device state display. |
| src/services/control-api.ts | Expands App model fields; broadens updateApp to Partial<App>. |
| src/commands/rooms/typing/subscribe.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/typing/keystroke.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/reactions/subscribe.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/reactions/send.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/occupancy/subscribe.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/occupancy/get.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/messages/update.ts | Implements message update command (metadata/headers/details/JSON output). |
| src/commands/rooms/messages/send.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/messages/reactions/subscribe.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/messages/reactions/send.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/messages/reactions/remove.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/messages/index.ts | Adds examples for new update/delete subcommands. |
| src/commands/rooms/messages/history.ts | Ensures early-return on fail() for missing Chat client. |
| src/commands/rooms/messages/delete.ts | Implements message delete command (details/JSON output). |
| src/commands/push/publish.ts | Implements single-recipient push publish command. |
| src/commands/push/index.ts | Adds push top-level topic command. |
| src/commands/push/devices/save.ts | Implements device registration save/update. |
| src/commands/push/devices/remove.ts | Implements device registration removal with confirmation. |
| src/commands/push/devices/remove-where.ts | Implements filtered bulk device removal with confirmation. |
| src/commands/push/devices/list.ts | Implements device registration listing with state formatting. |
| src/commands/push/devices/index.ts | Adds push devices topic command. |
| src/commands/push/devices/get.ts | Implements device registration retrieval with token redaction. |
| src/commands/push/config/show.ts | Implements push config display via Control API. |
| src/commands/push/config/set-fcm.ts | Implements FCM configuration via service account file. |
| src/commands/push/config/set-apns.ts | Implements APNs configuration via p12 upload or p8 key update. |
| src/commands/push/config/index.ts | Adds push config topic command. |
| src/commands/push/config/clear-fcm.ts | Implements clearing FCM config with confirmation and no-op behavior. |
| src/commands/push/config/clear-apns.ts | Implements clearing APNs config with confirmation and no-op behavior. |
| src/commands/push/channels/save.ts | Implements push channel subscription save. |
| src/commands/push/channels/remove.ts | Implements push channel subscription removal with confirmation. |
| src/commands/push/channels/remove-where.ts | Implements filtered bulk channel unsubscription with confirmation. |
| src/commands/push/channels/list.ts | Implements listing channel subscriptions with filters/limit. |
| src/commands/push/channels/list-channels.ts | Implements listing channels that have push subscriptions. |
| src/commands/push/channels/index.ts | Adds push channels topic command. |
| src/commands/push/batch-publish.ts | Implements batch publish with JSON/file/stdin input and validation. |
| src/commands/apps/set-apns-p12.ts | Marks legacy APNs P12 command hidden and warns it’s deprecated. |
| src/base-command.ts | Restricts anonymous web CLI usage for push* commands. |
| README.md | Adds docs for new push commands and chat message update/delete; removes legacy APNs P12 docs. |
| docs/Project-Structure.md | Documents new push command/test structure and fixtures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
test/unit/commands/push/devices/get.test.ts (1)
57-66: UsecaptureJsonLogs()instead of parsing raw stdout directly.It’s more consistent with the unit-test JSON assertion pattern used across this repo.
♻️ Suggested test adjustment
import { standardHelpTests, standardArgValidationTests, standardFlagTests, } from "../../../../helpers/standard-tests.js"; +import { captureJsonLogs } from "../../../../helpers/ndjson.js"; @@ - const result = JSON.parse(stdout); + const [result] = captureJsonLogs(stdout); expect(result).toHaveProperty("type", "result"); expect(result).toHaveProperty("success", true); expect(result).toHaveProperty("device");Based on learnings: In unit tests for the ably/ably-cli repo, for JSON output assertions, use the
captureJsonLogs()helper fromtest/helpers/ndjson.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/push/devices/get.test.ts` around lines 57 - 66, The test currently parses raw stdout from runCommand(["push:devices:get", "device-123", "--json"], import.meta.url) and asserts on the parsed object; replace this with the repository's JSON log helper by importing and using captureJsonLogs from test/helpers/ndjson.ts to capture and parse the command output (wrap the runCommand invocation with captureJsonLogs or call captureJsonLogs to execute the command as per existing tests), then run the same assertions against the returned parsed object (keep references to the runCommand invocation and the "push:devices:get" args so the intent remains clear).test/unit/commands/push/channels/list.test.ts (1)
64-73: PrefercaptureJsonLogs()over directJSON.parse(stdout)in unit JSON assertions.This keeps assertions resilient to envelope/log formatting differences.
♻️ Suggested test adjustment
import { standardHelpTests, standardArgValidationTests, standardFlagTests, } from "../../../../helpers/standard-tests.js"; +import { captureJsonLogs } from "../../../../helpers/ndjson.js"; @@ - const result = JSON.parse(stdout); + const [result] = captureJsonLogs(stdout); expect(result).toHaveProperty("type", "result"); expect(result).toHaveProperty("success", true); expect(result).toHaveProperty("subscriptions");Based on learnings: In unit tests for the ably/ably-cli repo, for JSON output assertions, use the
captureJsonLogs()helper fromtest/helpers/ndjson.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/push/channels/list.test.ts` around lines 64 - 73, The test currently uses JSON.parse(stdout) to assert JSON output; replace that with the captureJsonLogs() helper from test/helpers/ndjson.ts to make assertions resilient to envelope/log formatting. Call captureJsonLogs(stdout) (or import and use captureJsonLogs around the runCommand call) to obtain the parsed JSON object, then assert on properties like type, success, and subscriptions against the returned object instead of using JSON.parse; update the assertions in the test/unit/commands/push/channels/list.test.ts block where result is derived to use captureJsonLogs.src/services/control-api.ts (1)
11-30: Consider narrowingupdateApppayload type for better type safety.The use of
Partial<App>is overly permissive becauseAppincludes an index signature ([key: string]: unknown). While current usage correctly passes only writable fields, explicitly defining which fields are updatable prevents accidental mutation of readonly fields in future changes.♻️ Proposed type-safe patch shape
+export type AppUpdateData = Partial< + Pick< + App, + | "name" + | "tlsOnly" + | "apnsAuthType" + | "apnsCertificate" + | "apnsIssuerKey" + | "apnsPrivateKey" + | "apnsSigningKey" + | "apnsSigningKeyId" + | "apnsTopicHeader" + | "apnsUseSandboxEndpoint" + | "fcmServiceAccount" + > +>; + // Update an app -async updateApp(appId: string, appData: Partial<App>): Promise<App> { +async updateApp(appId: string, appData: AppUpdateData): Promise<App> { return this.request<App>(`/apps/${appId}`, "PATCH", appData); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/control-api.ts` around lines 11 - 30, The current update payload uses Partial<App>, but because App has an index signature ([key: string]: unknown) that makes Partial permissive, define a concrete update shape (e.g., type UpdateApp = { name?: string; status?: string; tlsOnly?: boolean; apnsUseSandboxEndpoint?: boolean | null; apnsUsesSandboxCert?: boolean; apnsAuthType?: string | null; apnsCertificate?: string | null; apnsIssuerKey?: string | null; apnsPrivateKey?: string | null; apnsSigningKey?: string | null; apnsSigningKeyId?: string | null; apnsTopicHeader?: string | null; fcmServiceAccount?: string | null } or use Pick<App, 'name'|'status'|...>) and replace all uses of Partial<App> in updateApp (and any related functions/handlers) with this UpdateApp type to ensure only intended fields are writable while preserving the original App interface.test/unit/commands/push/config/show.test.ts (1)
159-170: UsecaptureJsonLogs()for--jsonassertions in this unit test.Parsing raw
stdoutis more brittle than using the shared NDJSON capture helper used in this repo’s unit-test pattern.Based on learnings: “For JSON output assertions, use the captureJsonLogs() helper from test/helpers/ndjson.ts.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/push/config/show.test.ts` around lines 159 - 170, The test currently parses raw stdout from runCommand for the --json output which is brittle; replace that parsing by using the shared NDJSON helper captureJsonLogs() to collect JSON results from runCommand (keep using runCommand to execute ["push:config:show", "--json"]), then assert on the parsed object returned by captureJsonLogs() (check properties type, success, appId, apns, fcm). Update the test to call captureJsonLogs() after/around runCommand invocation and use its returned JSON object for assertions instead of JSON.parse(stdout).test/unit/commands/push/publish.test.ts (1)
101-111: PrefercaptureJsonLogs()overJSON.parse(stdout)in unit tests.For JSON-mode assertions, use the shared NDJSON helper so tests remain stable even if non-JSON lines are emitted.
Based on learnings: “For JSON output assertions, use the captureJsonLogs() helper from test/helpers/ndjson.ts.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/push/publish.test.ts` around lines 101 - 111, Replace the direct JSON.parse(stdout) usage with the NDJSON helper: call captureJsonLogs() to capture and parse JSON-mode output from runCommand (instead of parsing stdout directly), then assert on the returned JSON object (checking "type", "success", "published"); update the test around runCommand and the variable result to use captureJsonLogs() to retrieve the parsed JSON log entry for assertions (referencing captureJsonLogs and runCommand to locate the change).src/commands/push/batch-publish.ts (2)
166-178: Consider adding a timeout for stdin reading.The
readStdin()method will block indefinitely if stdin never ends (e.g., user runs command without piping input). This is a minor UX concern rather than a bug.♻️ Optional: Add timeout to prevent indefinite blocking
private async readStdin(): Promise<string> { - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("Timeout waiting for stdin input")); + }, 30000); // 30 second timeout + let data = ""; process.stdin.setEncoding("utf8"); process.stdin.on("data", (chunk) => { data += chunk; }); process.stdin.on("end", () => { + clearTimeout(timeout); resolve(data); }); - process.stdin.on("error", reject); + process.stdin.on("error", (err) => { + clearTimeout(timeout); + reject(err); + }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/push/batch-publish.ts` around lines 166 - 178, The readStdin() Promise can hang if stdin never ends; update the readStdin() method to include a configurable timeout (e.g., default 5s) that rejects (or resolves with empty string) when elapsed, and ensure you clean up listeners and the timer to avoid leaks. Inside readStdin(), create a timer with setTimeout that triggers a reject/resolve with a clear error message, and in each of the stdin event handlers (data, end, error) clear the timer and remove their listeners; expose the timeout as an optional parameter or constant so callers can adjust it if needed.
59-76: Potential uninitialized variable afterthis.fail()calls.TypeScript's control flow analysis may not recognize that
this.fail()always throws, potentially flaggingbatchPayloadas possibly uninitialized at line 70. Consider initializing with a dummy value or restructuring to make the control flow clearer.♻️ Suggested restructure to clarify control flow
- let batchPayload: unknown[]; - try { - batchPayload = JSON.parse(jsonString) as unknown[]; - } catch { - this.fail( - "Payload must be a valid JSON array", - flags as BaseFlags, - "pushBatchPublish", - ); - } - - if (!Array.isArray(batchPayload)) { + let batchPayload: unknown; + try { + batchPayload = JSON.parse(jsonString); + } catch { + return this.fail( + "Payload must be a valid JSON array", + flags as BaseFlags, + "pushBatchPublish", + ); + } + + if (!Array.isArray(batchPayload)) { + return this.fail( + "Payload must be a JSON array", + flags as BaseFlags, + "pushBatchPublish", + ); + }Adding
returnbeforethis.fail()calls makes it explicit to TypeScript that these are terminal branches, even thoughfail()throws internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/push/batch-publish.ts` around lines 59 - 76, TypeScript may think batchPayload is uninitialized after the error branches; update the error branches in the JSON parsing/validation logic (the calls to this.fail(...) inside the try/catch and the Array.isArray check in the pushBatchPublish command) to explicitly return after calling this.fail (or alternatively initialize batchPayload to a safe default like [] as unknown[]) so the compiler understands those branches are terminal and batchPayload is always assigned before use.src/commands/push/channels/save.ts (1)
67-67: Consider using proper typing instead ofas nevercast.The
as nevercast suppresses type checking entirely. If the SDK expects a specific type for the subscription parameter, consider defining or importing that type for better type safety.♻️ Suggested improvement
- await rest.push.admin.channelSubscriptions.save(subscription as never); + await rest.push.admin.channelSubscriptions.save( + subscription as { channel: string; deviceId?: string; clientId?: string }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/push/channels/save.ts` at line 67, The call to rest.push.admin.channelSubscriptions.save currently uses a blanket cast "as never" which disables type checking; replace this with the correct SDK type for the subscription parameter by importing or declaring the expected type (e.g., the type the SDK exposes for channel subscription payload) and change the call to rest.push.admin.channelSubscriptions.save(subscription) typed as that type (or cast to that specific SDK type) so TypeScript can validate the payload; locate the call site using the save method and the subscription variable to implement the typed change.src/commands/push/devices/get.ts (1)
47-50: Consider redactingdeviceTokenin JSON output as well.The deviceToken is redacted in human-readable output (lines 70-77) but not in JSON output. If this is intentional for automation use cases, consider documenting this behavior. Otherwise, sensitive tokens should be consistently redacted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/push/devices/get.ts` around lines 47 - 50, The JSON branch currently calls logJsonResult({ device }, flags) without redacting device.deviceToken; update the logic in the shouldOutputJson path to redact the token the same way as the human-readable branch (apply the same redaction logic used around the human output code to device.deviceToken) before calling logJsonResult so JSON output does not expose the raw token (or document intentional exception if you want to keep raw tokens for automation). Reference symbols: shouldOutputJson, logJsonResult, and device.deviceToken to locate where to mutate the object prior to logging.src/commands/push/config/show.ts (1)
80-129: Use the shared section-output helpers here.These raw section headers and the final guidance line won't match the rest of the CLI's human-readable output. Use
formatHeading()/formatWarning()instead of plain strings.As per coding guidelines "Always use formatProgress(), formatSuccess(), formatWarning(), formatListening(), formatResource(), formatTimestamp(), formatClientId(), formatEventType(), formatHeading(), formatIndex(), formatCountLabel(), and formatLimitWarning() from src/utils/output.ts instead of direct chalk calls".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/push/config/show.ts` around lines 80 - 129, The output uses raw strings for section headers and the final guidance line; replace those with the shared helpers to match CLI style: use formatHeading() for the section titles ("APNs Configuration:", "FCM Configuration:", "Web Push:") instead of this.log("..."), keep the existing formatSuccess/formatResource usage, and use formatWarning() for the final guidance line ("Configure APNs or FCM to enable web push notifications."); ensure all calls still go through this.log(...) and preserve the conditional blocks around apnsConfigured/fcm/projectId as-is so only the presentation changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 2665-3251: The README push docs contain many unannotated fenced
code blocks (e.g., the blocks under the USAGE/DESCRIPTION/EXAMPLES sections for
"ably push", "ably push batch-publish", "ably push channels", etc.) which
trigger markdownlint MD040; update each triple-backtick fence in that push docs
section to include a language identifier (for example use "text" or
"sh-session") so fences read ```text or ```sh-session consistently across the
blocks for commands like "ably push", "ably push batch-publish", "ably push
channels", "ably push config", and "ably push devices".
In `@src/commands/apps/set-apns-p12.ts`:
- Around line 52-54: Replace the unconditional this.warn call in the
set-apns-p12 command with a guarded, stderr-formatted warning: wrap the warning
in if (!this.shouldOutputJson(flags)) and inside that block call
this.logToStderr(this.formatWarning('This command is deprecated. Use "ably push
config set-apns" instead.')); reference the existing this.warn usage and the
flags variable in the command to locate where to change this behavior.
In `@src/commands/push/channels/list.ts`:
- Around line 73-75: Replace direct human-readable outputs in the channel list
command with the shared output formatters: where the code currently does
this.log("No subscriptions found.") use formatWarning(...) to render the
empty-state message, and where it prints raw clientId use formatClientId(...) to
render client identifiers; update the usages in the method handling
subscriptions (references: subscriptions array check and the later clientId
output) to call formatWarning and formatClientId from src/utils/output.ts and
pass the same message/string values to preserve behavior.
In `@src/commands/push/channels/save.ts`:
- Around line 41-47: The call to this.fail(...) in the push channel save logic
can return control instead of throwing in some environments; after the
validation block that checks flags (the snippet using flags, BaseFlags and the
"pushChannelSave" tag), add an immediate return right after this.fail(...) to
defensively stop execution when neither --device-id nor --client-id is provided
so subsequent code in the surrounding method does not run with invalid state.
In `@src/commands/push/config/set-apns.ts`:
- Around line 69-77: The code path that checks flags.certificate calls
this.fail(...) when the certificate file doesn't exist but doesn't return/stop
execution, so subsequent code (e.g., using certPath) can run; update the
pushConfigSetApns flow to immediately exit after this.fail by adding a return
(or throw) right after the this.fail(...) call so the function does not continue
when the cert file is missing — reference the flags.certificate check, certPath,
and this.fail() when making the change.
- Around line 56-62: The validation in the pushConfigSetApns flow calls
this.fail(...) but does not stop execution; add an explicit return immediately
after the this.fail(...) call in the code path inside pushConfigSetApns (the
block that checks flags.certificate and flags["key-file"]) so execution does not
continue when the validation fails; reference the flags variable used in that
check and ensure the function/method exits after calling this.fail().
- Around line 108-137: The validation branch in pushConfigSetApns calling
this.fail() for missing flags ("key-id", "team-id", "topic") and missing key
file (keyPath / fs.existsSync) does not stop execution; add an immediate
control-flow stop after each this.fail() call (e.g., return or throw) so the
function exits and does not continue into the P8 flow when validation fails —
update the checks around flags["key-id"], flags["team-id"], flags.topic and the
keyPath existence check to return immediately after calling this.fail() in the
pushConfigSetApns logic.
In `@src/commands/push/devices/save.ts`:
- Around line 210-237: parseDataFlag currently returns any JSON value
(arrays/primitives) but callers like run() expect a device object (e.g.,
accessing deviceData.id and passing to deviceRegistrations.save); update
parseDataFlag to validate the parsed JSON is a non-null plain object (typeof
result === "object" && result !== null && !Array.isArray(result)) and if not
call this.fail with the existing error message/flags/"pushDeviceSave" so only
object payloads are returned.
In `@src/commands/push/publish.ts`:
- Around line 95-179: The parsed JSON for structured flags in pushPublish must
be validated to ensure it's a non-null plain object; update each JSON.parse
block for flags.recipient, flags.payload (and the fallback-built payload),
flags.data (when parsed), flags.apns, flags.fcm, and flags.web to check typeof
parsed === "object" && parsed !== null && !Array.isArray(parsed) and call
this.fail("--<flag> must be a JSON object", flags as BaseFlags, "pushPublish")
when the check fails; ensure payload remains a Record<string, unknown> and that
the payload = JSON.parse(...) assignments are replaced with the validated object
before use.
---
Nitpick comments:
In `@src/commands/push/batch-publish.ts`:
- Around line 166-178: The readStdin() Promise can hang if stdin never ends;
update the readStdin() method to include a configurable timeout (e.g., default
5s) that rejects (or resolves with empty string) when elapsed, and ensure you
clean up listeners and the timer to avoid leaks. Inside readStdin(), create a
timer with setTimeout that triggers a reject/resolve with a clear error message,
and in each of the stdin event handlers (data, end, error) clear the timer and
remove their listeners; expose the timeout as an optional parameter or constant
so callers can adjust it if needed.
- Around line 59-76: TypeScript may think batchPayload is uninitialized after
the error branches; update the error branches in the JSON parsing/validation
logic (the calls to this.fail(...) inside the try/catch and the Array.isArray
check in the pushBatchPublish command) to explicitly return after calling
this.fail (or alternatively initialize batchPayload to a safe default like [] as
unknown[]) so the compiler understands those branches are terminal and
batchPayload is always assigned before use.
In `@src/commands/push/channels/save.ts`:
- Line 67: The call to rest.push.admin.channelSubscriptions.save currently uses
a blanket cast "as never" which disables type checking; replace this with the
correct SDK type for the subscription parameter by importing or declaring the
expected type (e.g., the type the SDK exposes for channel subscription payload)
and change the call to rest.push.admin.channelSubscriptions.save(subscription)
typed as that type (or cast to that specific SDK type) so TypeScript can
validate the payload; locate the call site using the save method and the
subscription variable to implement the typed change.
In `@src/commands/push/config/show.ts`:
- Around line 80-129: The output uses raw strings for section headers and the
final guidance line; replace those with the shared helpers to match CLI style:
use formatHeading() for the section titles ("APNs Configuration:", "FCM
Configuration:", "Web Push:") instead of this.log("..."), keep the existing
formatSuccess/formatResource usage, and use formatWarning() for the final
guidance line ("Configure APNs or FCM to enable web push notifications.");
ensure all calls still go through this.log(...) and preserve the conditional
blocks around apnsConfigured/fcm/projectId as-is so only the presentation
changes.
In `@src/commands/push/devices/get.ts`:
- Around line 47-50: The JSON branch currently calls logJsonResult({ device },
flags) without redacting device.deviceToken; update the logic in the
shouldOutputJson path to redact the token the same way as the human-readable
branch (apply the same redaction logic used around the human output code to
device.deviceToken) before calling logJsonResult so JSON output does not expose
the raw token (or document intentional exception if you want to keep raw tokens
for automation). Reference symbols: shouldOutputJson, logJsonResult, and
device.deviceToken to locate where to mutate the object prior to logging.
In `@src/services/control-api.ts`:
- Around line 11-30: The current update payload uses Partial<App>, but because
App has an index signature ([key: string]: unknown) that makes Partial
permissive, define a concrete update shape (e.g., type UpdateApp = { name?:
string; status?: string; tlsOnly?: boolean; apnsUseSandboxEndpoint?: boolean |
null; apnsUsesSandboxCert?: boolean; apnsAuthType?: string | null;
apnsCertificate?: string | null; apnsIssuerKey?: string | null; apnsPrivateKey?:
string | null; apnsSigningKey?: string | null; apnsSigningKeyId?: string | null;
apnsTopicHeader?: string | null; fcmServiceAccount?: string | null } or use
Pick<App, 'name'|'status'|...>) and replace all uses of Partial<App> in
updateApp (and any related functions/handlers) with this UpdateApp type to
ensure only intended fields are writable while preserving the original App
interface.
In `@test/unit/commands/push/channels/list.test.ts`:
- Around line 64-73: The test currently uses JSON.parse(stdout) to assert JSON
output; replace that with the captureJsonLogs() helper from
test/helpers/ndjson.ts to make assertions resilient to envelope/log formatting.
Call captureJsonLogs(stdout) (or import and use captureJsonLogs around the
runCommand call) to obtain the parsed JSON object, then assert on properties
like type, success, and subscriptions against the returned object instead of
using JSON.parse; update the assertions in the
test/unit/commands/push/channels/list.test.ts block where result is derived to
use captureJsonLogs.
In `@test/unit/commands/push/config/show.test.ts`:
- Around line 159-170: The test currently parses raw stdout from runCommand for
the --json output which is brittle; replace that parsing by using the shared
NDJSON helper captureJsonLogs() to collect JSON results from runCommand (keep
using runCommand to execute ["push:config:show", "--json"]), then assert on the
parsed object returned by captureJsonLogs() (check properties type, success,
appId, apns, fcm). Update the test to call captureJsonLogs() after/around
runCommand invocation and use its returned JSON object for assertions instead of
JSON.parse(stdout).
In `@test/unit/commands/push/devices/get.test.ts`:
- Around line 57-66: The test currently parses raw stdout from
runCommand(["push:devices:get", "device-123", "--json"], import.meta.url) and
asserts on the parsed object; replace this with the repository's JSON log helper
by importing and using captureJsonLogs from test/helpers/ndjson.ts to capture
and parse the command output (wrap the runCommand invocation with
captureJsonLogs or call captureJsonLogs to execute the command as per existing
tests), then run the same assertions against the returned parsed object (keep
references to the runCommand invocation and the "push:devices:get" args so the
intent remains clear).
In `@test/unit/commands/push/publish.test.ts`:
- Around line 101-111: Replace the direct JSON.parse(stdout) usage with the
NDJSON helper: call captureJsonLogs() to capture and parse JSON-mode output from
runCommand (instead of parsing stdout directly), then assert on the returned
JSON object (checking "type", "success", "published"); update the test around
runCommand and the variable result to use captureJsonLogs() to retrieve the
parsed JSON log entry for assertions (referencing captureJsonLogs and runCommand
to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d07b42f2-c91a-46c2-8b5b-f92c374645a6
📒 Files selected for processing (48)
README.mddocs/Project-Structure.mdsrc/base-command.tssrc/commands/apps/set-apns-p12.tssrc/commands/push/batch-publish.tssrc/commands/push/channels/index.tssrc/commands/push/channels/list-channels.tssrc/commands/push/channels/list.tssrc/commands/push/channels/remove-where.tssrc/commands/push/channels/remove.tssrc/commands/push/channels/save.tssrc/commands/push/config/clear-apns.tssrc/commands/push/config/clear-fcm.tssrc/commands/push/config/index.tssrc/commands/push/config/set-apns.tssrc/commands/push/config/set-fcm.tssrc/commands/push/config/show.tssrc/commands/push/devices/get.tssrc/commands/push/devices/index.tssrc/commands/push/devices/list.tssrc/commands/push/devices/remove-where.tssrc/commands/push/devices/remove.tssrc/commands/push/devices/save.tssrc/commands/push/index.tssrc/commands/push/publish.tssrc/services/control-api.tssrc/utils/output.tstest/e2e/push/push-config-e2e.test.tstest/fixtures/push/test-apns-key.p8test/fixtures/push/test-fcm-service-account.jsontest/helpers/mock-ably-rest.tstest/unit/commands/push/batch-publish.test.tstest/unit/commands/push/channels/list-channels.test.tstest/unit/commands/push/channels/list.test.tstest/unit/commands/push/channels/remove-where.test.tstest/unit/commands/push/channels/remove.test.tstest/unit/commands/push/channels/save.test.tstest/unit/commands/push/config/clear-apns.test.tstest/unit/commands/push/config/clear-fcm.test.tstest/unit/commands/push/config/set-apns.test.tstest/unit/commands/push/config/set-fcm.test.tstest/unit/commands/push/config/show.test.tstest/unit/commands/push/devices/get.test.tstest/unit/commands/push/devices/list.test.tstest/unit/commands/push/devices/remove-where.test.tstest/unit/commands/push/devices/remove.test.tstest/unit/commands/push/devices/save.test.tstest/unit/commands/push/publish.test.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ptions, publishing, and configuration
- Guard deprecation warning in set-apns-p12 for JSON mode - Use formatWarning() and formatClientId() in push channels list - Validate --data flag parses to a JSON object in push devices save - Validate all JSON flags (--recipient, --payload, --data, --apns, --fcm, --web) parse to objects in push publish - Let E2E test setup failures propagate instead of silently skipping
Three bugs found during manual testing of push publish commands:
1. `push publish --payload <file>` failed with "--payload must be a valid
JSON object" because the command only accepted inline JSON strings.
Added file path support: both `@filepath` (curl-style) and bare paths
starting with `./`, `../`, or `/` are now resolved and read from disk.
2. `push batch-publish --payload <file>` required the undocumented `@`
prefix (e.g. `@./file.json`). Bare path support added here too,
consistent with the fix above.
3. `push batch-publish` validator checked `entry.notification` and
`entry.data` at the top level of each batch item, but the Ably batch
publish API uses `{recipient, payload: {notification, data}}` — the
notification content is nested under `payload`. This caused all
batch-publish calls with correctly-formatted payloads to be rejected
with "Item at index 0 must have a 'notification' or 'data' field".
Validator updated to check `entry.payload.notification` /
`entry.payload.data`. The inline example in the command was also wrong
(missing `payload` wrapper) and has been corrected.
Tests updated to use the correct `{recipient, payload: {notification}}`
batch item format.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
@umair-ably Manual testing results — 3 bugs found and fixed in commit 14ce8a3. Test commands (✅ passed, ❌ failed)Bug detailsBug 1 — Fix: added Bug 2 — All correctly-formatted batch payloads were rejected with:
Fix: validator now checks Bug 3 — Fix: bare path detection added (same as Bug 1 fix in |
The uploadApnsP12 method was calling POST /apps/{appId}/push/certificate
which returns 404. The correct path per the Control API OpenAPI spec is
POST /apps/{appId}/pkcs12.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
@umair-ably Follow-up: manual testing of Commands tested
Bug 1 —
|
The uploadApnsP12 method had three bugs found during manual testing:
1. Wrong content type: the method was sending the certificate as
base64-encoded JSON, but POST /apps/{id}/pkcs12 requires
multipart/form-data with fields p12File (binary) and p12Pass
(string), as defined in the Control API OpenAPI spec.
2. Wrong field names: the original code sent { p12Certificate, password,
useForSandbox } but the API schema defines { p12File, p12Pass } only.
useForSandbox is not part of the pkcs12 endpoint schema at all.
3. Sandbox flag handling: since useForSandbox cannot be sent to the
pkcs12 endpoint, uploadApnsP12 now handles it as a follow-up
PATCH /apps/{id} with { apnsUseSandboxEndpoint } when the flag is set.
This is the correct endpoint for that field per the app_patch schema.
Updated callers (push config set-apns, apps set-apns-p12) to pass a
Buffer instead of a base64 string.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dbox flag
The condition `useForSandbox !== undefined` was always true because both
`--use-for-sandbox` (apps:set-apns-p12) and `--sandbox` (push:config:set-apns)
default to `false` rather than `undefined`. This caused every P12 upload to
make an extra PATCH /apps/{id} request even when sandbox mode wasn't requested,
breaking nock intercepts in tests and sending unnecessary API calls in production.
Fix: change condition to truthy check (`if (options.useForSandbox)`) so the
sandbox PATCH is only made when explicitly set to true.
Also add missing nock interceptor for the PATCH in the sandbox test case of
apps:set-apns-p12, and fix nock path from /push/certificate to /pkcs12 in
both test files.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ormat
Ably batch push API requires {recipient, payload: {notification}} not
the flat {recipient, notification} format used in these tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Control API stores fcmProjectId as a separate field from fcmServiceAccount. Not sending it meant the dashboard kept showing the old project ID after updating the service account. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Used #126 as a reference PR but heavily updated to follow new standards in the codebase
Summary by CodeRabbit
New Features
Deprecations
set-apns-p12command; useably push config set-apnsinstead.Documentation
Summary
Adds comprehensive push notification management to the Ably CLI, covering the full Push Admin API surface: device registrations, channel subscriptions, publishing (single + batch), and provider configuration (APNs/FCM) via the Control API. Also deprecates the legacy
apps set-apns-p12command in favour ofpush config set-apns.51 files changed, ~6,700 lines added
What's new
ably push publish— Send a push notification to a single device or clientably push batch-publish— Send push notifications to multiple recipients (JSON, file, or stdin)ably push devices {get,list,save,remove,remove-where}— Manage device registrationsably push channels {list,list-channels,save,remove,remove-where}— Manage channel subscriptionsably push config {show,set-apns,set-fcm,clear-apns,clear-fcm}— Configure APNs/FCM via Control APIably apps set-apns-p12— Now hidden + deprecated with a warning pointing topush config set-apnsOther changes
base-command.ts: Restrictspush*commands for anonymous web CLI userscontrol-api.ts: ExpandsAppmodel with push-related fields; broadensupdateAppto acceptPartial<App>output.ts: AddsformatDeviceState()helperReview strategy
This PR is large but highly repetitive — most push commands follow identical patterns. Reviewing in the order below lets you establish the patterns early and skim the rest.
src/base-command.ts,src/services/control-api.ts,src/utils/output.tssrc/commands/push/publish.ts+test/unit/commands/push/publish.test.tsfail()usage). All other product API commands follow this template.src/commands/push/config/show.ts+test/unit/commands/push/config/show.test.tsControlBaseCommand,runControlCommand,requireAppId). All other config commands follow this template.src/commands/push/devices/save.ts+ testsrc/commands/push/batch-publish.ts+ testsrc/commands/push/config/set-apns.ts,set-fcm.ts+ testssrc/commands/push/devices/*+ testspublish.tspattern from step 2. Spot-check oneremoveand onelist.src/commands/push/channels/*+ testssrc/commands/push/config/clear-*+ testssrc/commands/apps/set-apns-p12.tshidden = true+ deprecation warning.test/helpers/mock-ably-rest.ts,test/fixtures/push/*,test/e2e/push/*README.md,docs/Project-Structure.md, topicindex.tsfilesTest plan
pnpm test:unit— all 14 new push unit test files passpnpm test:e2e— 4 push E2E test files pass against real Ablypnpm exec eslint .— 0 errorsably push --helpshows all subcommand groupsably apps set-apns-p12 --helpshows deprecation warning--jsonoutput produces correct NDJSON envelope for push commands