Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
4c7995a to
ee20db0
Compare
| 1. [Sign up](https://ably.com/signup) for an Ably account. | ||
| 2. Create a [new app](https://ably.com/accounts/any/apps/new), and create your first API key in the **API Keys** tab of the dashboard. | ||
| 3. Your API key will need the `publish` and `subscribe` capabilities. For sending push notifications from your app, you'll also need the `push-admin` capability. | ||
| 4. For channel-based push, add a rule for the channel with **Push notifications enabled** checked. In the dashboard: go to **Pub/Sub** → **Channels** → **Configure** → **Rules** → **Add** or **Edit** a rule, |
There was a problem hiding this comment.
In the new dashboard, this isn't correct - you select your app then go to the Rules tab in the leftnav
| 5. A modern browser that supports the [Push API](https://developer.mozilla.org/en-US/docs/Web/API/Push_API) (Chrome, Firefox, or Edge recommended). Safari has limited Web Push support. | ||
| 6. A local web server to serve your files over `http://localhost` (service workers require a secure context or localhost). | ||
|
|
||
| ### (Optional) Install Ably CLI <a id="install-cli"/> |
There was a problem hiding this comment.
There's a CLI PR to add push commands - should we orient the guide more towards using the CLI in that regard? Or at least making it clear you can easily test push sending without any code that way?
There was a problem hiding this comment.
should we orient the guide more towards using the CLI
I guess so, but when that PR is going to be merged though?
Also re this particular comment, was it triggered by presence of the "(Optional)" prefix in the title? I'm not against removing it - I feel it a bit distracting.
And samples of using CLI here for sending push to device/client are using commands that are not yet released. I'm not sure what to do about that, remove them or wait that PR is merged?
| // Initialize Ably connection | ||
| async function getStarted() { | ||
| realtimeClient = new Ably.Realtime({ | ||
| key: '{{API_KEY}}', |
There was a problem hiding this comment.
We should call out in a comment here that in production you would never use a key on the client side.
ee20db0 to
91c271f
Compare
GregHolmes
left a comment
There was a problem hiding this comment.
A good guide @maratal I've left a few comments.
My only other comment is that your IDE seems to wrap text into a new line, where we don't usually do that. It would just be for easier review/developer reading to keep them on the same line for a paragraph.
| 5. You'll need a real iOS device to test push notifications (the simulator doesn't support APNs). | ||
| 6. Set up Apple Push Notification service (APNs) certificates through the [Apple Developer Portal](https://developer.apple.com/). | ||
| 4. For channel-based push, add a rule for the channel with **Push notifications enabled** checked. In the dashboard left sidebar: **Configuration** → **Rules** → **Add** or **Edit** a rule, | ||
| then enable the Push notifications option. See [channel rules](https://ably.com/docs/channels#rules) for details. |
There was a problem hiding this comment.
| then enable the Push notifications option. See [channel rules](https://ably.com/docs/channels#rules) for details. | |
| then enable the Push notifications option. See [channel rules](/docs/channels#rules) for details. |
| 2. Create a [new app](https://ably.com/accounts/any/apps/new), and create your first API key in the **API Keys** tab of the dashboard. | ||
| 3. Your API key will need the `publish` and `subscribe` capabilities. For sending push notifications from your app, you'll also need the `push-admin` capability. | ||
| 4. For channel-based push, add a rule for the channel with **Push notifications enabled** checked. In the dashboard left sidebar: **Configuration** → **Rules** → **Add** or **Edit** a rule, | ||
| then enable the Push notifications option. See [channel rules](https://ably.com/docs/channels#rules) for details. |
There was a problem hiding this comment.
| then enable the Push notifications option. See [channel rules](https://ably.com/docs/channels#rules) for details. | |
| then enable the Push notifications option. See [channel rules](/docs/channels#rules) for details. |
|
|
||
| Add all the new buttons code to your `HTML` and `<script>` sections: | ||
|
|
||
| <Code> |
There was a problem hiding this comment.
Do we need these 3 to be separate?
Or at least the 2 Javascript ones?
If you merge the 2 JS ones, you would then just need a brief intro to the code in between the html and the one JS.
|
|
||
| The `extras.push` object has two parts: | ||
|
|
||
| - **`notification`**: Contains `title` and `body` that are displayed in the browser notification. |
There was a problem hiding this comment.
This is quite an AI smell, can we remove the bold from these bullet points? I think if you ask Claude to refer to CLAUDE.md and review this page it'd pick that up.
|
|
||
| Now you can send push notifications to your device, client, or channel using the buttons in the UI: | ||
|
|
||
|  |
There was a problem hiding this comment.
There are 2 images with the alt: Screenshot of the Swift push tutorial application in this guide. We should adapt this, but we shouldn't really have the same alt for two different images?
| Sending push notifications using `deviceId` or `clientId` requires the `push-admin` capability for your API key. | ||
| Use this method for testing purposes. In a production environment, you would typically send push notifications | ||
| from your backend server (by posting messages with `push` `extras` field to a channel). |
There was a problem hiding this comment.
| Sending push notifications using `deviceId` or `clientId` requires the `push-admin` capability for your API key. | |
| Use this method for testing purposes. In a production environment, you would typically send push notifications | |
| from your backend server (by posting messages with `push` `extras` field to a channel). | |
| Sending push notifications using `deviceId` or `clientId` requires the `push-admin` capability for your API key. Use this method for testing purposes. In a production environment, you would typically send push notifications from your backend server (by posting messages with `push` `extras` field to a channel). |
| from your backend server (by posting messages with `push` `extras` field to a channel). | ||
|
|
||
| To test push notifications in your app, you can use [Ably dashboard](https://ably.com/dashboard), | ||
| [Apple developer dashboard](https://icloud.developer.apple.com/dashboard/) or Ably CLI. |
There was a problem hiding this comment.
Is this part relevant regarding Apple?
|
|
||
| <Code> | ||
| ```javascript | ||
| /// Send push notification to a specific device ID |
There was a problem hiding this comment.
I think the commenting you're using here is Swift not JS, can we please make sure we follow JS styles?
|
|
||
| ### Handle notification clicks <a id="step-3-notification-clicks"/> | ||
|
|
||
| Add a `notificationclick` listener in the same `service-worker.js` to handle what happens when the user taps the notification: |
There was a problem hiding this comment.
We're using tap here which is usually a mobile term, It'd be better to use click. tap is used twice in this doc.
|
|
||
| There are also some helper functions to log messages to the output div, show status messages and clear the output log. | ||
|
|
||
| ## Step 2: Set up push notifications <a id="step-2"/> |
There was a problem hiding this comment.
Step 4 (UI) is the UI implementation, if a developer runs the code after step 2, they're going to get an error. It may be worth adding an Aside note just stating that the UI part is later in the guide.
|
Hey @GregHolmes, all your comments have been addressed — ready for another look! |
7205395 to
94ce859
Compare
- Use relative URLs for internal channel rules links - Add aside note in Step 2 clarifying UI is built in Step 4 - Remove Apple Developer Dashboard reference from web push guide - Merge paired JS code blocks in Steps 4 and 5 into single blocks - Replace triple-slash comments with standard JS double-slash - Remove bold prefixes from bullet list items - Differentiate duplicate image alt text and remove Swift reference - Change 'tap' to 'click' throughout web push guide - Join wrapped paragraph lines for reviewer readability Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix sendPushToClientIdBtn typo → sendPushToClientBtn (was a runtime error) - Add const declarations to button variables in Step 5 - Replace **Note:** with <Aside> component in browser compatibility section Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
94ce859 to
632708e
Compare
Description
Web push notifications tutorial.
There is also few small fixes for APNs guide as well, let me know if you would like to see it in a different PR.
Checklist