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 |
maratal
force-pushed
the
DX-286-kotlin-push-guide
branch
from
65ab558 to
e106ea1
Compare
maratal
force-pushed
the
DX-286-kotlin-push-guide
branch
from
e106ea1 to
a20acb1
Compare
maratal
force-pushed
the
DX-286-kotlin-push-guide
branch
from
a20acb1 to
885f61b
Compare
maratal
force-pushed
the
DX-286-kotlin-push-guide
branch
from
885f61b to
65fb475
Compare
maratal
force-pushed
the
DX-287-web-push-guide
branch
from
7205395 to
94ce859
Compare
GregHolmes
left a comment
GregHolmes
left a comment
There was a problem hiding this comment.
Looks good! Thank you @maratal I've made a few comments, let me know what you think?
| when (intent.action) { | ||
| "io.ably.broadcast.PUSH_ACTIVATE" -> { | ||
| val error = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| intent.getParcelableExtra("error", ErrorInfo::class.java) |
There was a problem hiding this comment.
I may be wrong here (@ttypic would correct if so), but does the Java SDK actually have ErrorInfo implementing Parcelable?
I think the SDK uses IntentUtils to serialize errors as individual extras.
// ...
val error = IntentUtils.getErrorInfo(intent)```
If I'm right here, wouldn't need `TIRAMISU` version check, or the `@Suppress("DEPRECATION")` block.
There was a problem hiding this comment.
Done, switched to IntentUtils.getErrorInfo(intent).
| val channel = realtime.channels.get(channelName) | ||
| channel.subscribe { message -> | ||
| runOnUiThread { | ||
| appendToLog("Received message: ${message.name} - ${message.data}") |
There was a problem hiding this comment.
If the user is running the app while adding this code, it will fail to compile.
We should either make step 2 self contained or note that it won't compile until step 4.
So either, create an appendToLog() here, remove it until step 4, or note.
There was a problem hiding this comment.
Fixed — updated the Step 3 snippet to use Log.d instead, since appendToLog isn't defined until Step 4.
| Log.e(TAG, "Push activation failed: ${error.message}") | ||
| updateStatus("Push activation failed: ${error.message}") | ||
| } else { | ||
| val deviceId = realtime.device().id |
There was a problem hiding this comment.
device() can throw errors I think, should we wrap this in a try/catch?
There was a problem hiding this comment.
Fixed, wrapped in a try/catch.
| import io.ably.lib.types.AblyException | ||
| import io.ably.lib.types.ErrorInfo | ||
| import io.ably.lib.realtime.CompletionListener | ||
| import com.google.gson.JsonObject |
There was a problem hiding this comment.
In this step we add a handful of imports that aren't used until step 5. This is fine, however in step 4 we miss some imports.
ScrollView, Button, Bundle, TextView.
I'd recommend we add them here too.
But we add a note to say something like The following imports cover all steps in this guide What do you think?
There was a problem hiding this comment.
Added all missing imports with a note that they cover all steps in this guide.
| return instance ?: synchronized(this) { | ||
| instance ?: run { | ||
| val options = ClientOptions().apply { | ||
| key = "{{API_KEY}}" |
There was a problem hiding this comment.
We really should have a comment somewhere to say
// Use token authentication in production
| </Code> | ||
|
|
||
| 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.
We may need to resolve some inconsistencies here.
Throughout the guide we use push and push notifications.
Should we pick with one and stick with it?
For example:
Line 395: "Sending push notifications using deviceId..."
Line 398: "send a push to your client ID"
Line 660: "test pushes via channel"
Line 676: "you don't need the admin capabilities"
There was a problem hiding this comment.
Fixed, standardised on 'push notifications' throughout.
| ``` | ||
| </Code> | ||
|
|
||
| Now add push activation and deactivation to your `MainActivity.kt`. The Ably Android SDK sends activation results as local broadcasts, so register a `BroadcastReceiver` to handle them: |
There was a problem hiding this comment.
Should we explain here why this pattern is being used? something like: the SDK activates asynchronously and the result arrives via Android's broadcast system)?
There was a problem hiding this comment.
Done, added an explanation.
|
|
||
| 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. |
There was a problem hiding this comment.
| 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. | |
| 3. Your API key needs the `publish` and `subscribe` capabilities. For sending push notifications from your app, you'll also need the `push-admin` capability. |
| 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. | ||
| 5. Install [Android Studio](https://developer.android.com/studio). | ||
| 6. A real Android device or an emulator with Google Play Services installed (required for FCM). |
There was a problem hiding this comment.
All other prerequisites start with verbs. This one is a noun. I think we should align it with the others. It's only a small nitpick though.
| 6. A real Android device or an emulator with Google Play Services installed (required for FCM). | |
| 6. Use a real Android device or an emulator with Google Play Services installed (required for FCM). |
| 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 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.
| 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. | |
| 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](/docs/channels#rules) for details. |
maratal
force-pushed
the
DX-287-web-push-guide
branch
from
94ce859 to
632708e
Compare
maratal
force-pushed
the
DX-286-kotlin-push-guide
branch
from
65fb475 to
789d8eb
Compare
3 participants
Description
Kotlin push guide
Checklist