Add UnifiedPush support with PushProvider abstraction#6599
Add UnifiedPush support with PushProvider abstraction#6599sk7n4k3d wants to merge 26 commits intohome-assistant:mainfrom
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive UnifiedPush support with a generic PushProvider abstraction layer, allowing users to receive push notifications without relying on Google's FCM infrastructure. This is especially important for the minimal/F-Droid flavor and privacy-conscious users.
Changes:
- Introduces a generic
PushProviderinterface with priority-based selection and Dagger multibinding coordination - Implements three concrete providers:
FcmPushProvider(full flavor only),UnifiedPushProvider(both flavors), andWebSocketPushProvider(fallback) - Adds
PushProviderManagerto orchestrate provider selection, registration, and server updates - Integrates push provider selection into app startup and settings flows
- Includes 28 unit tests covering provider logic, message parsing, and interface contracts
- Adds user-facing settings to select a UnifiedPush distributor
- Updates push message handling in WebSocket and UnifiedPush receivers
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
common/src/main/kotlin/io/homeassistant/companion/android/common/push/ |
New PushProvider interface and PushProviderManager for coordinating providers |
app/src/main/kotlin/io/homeassistant/companion/android/push/ |
Implementations of FcmPushProvider, UnifiedPushProvider, and WebSocketPushProvider |
app/src/main/kotlin/io/homeassistant/companion/android/unifiedpush/ |
UnifiedPush-specific manager, receiver, and worker classes |
app/src/main/kotlin/io/homeassistant/companion/android/launch/LaunchActivity.kt |
Integration of push provider selection into onboarding flow |
app/src/main/kotlin/io/homeassistant/companion/android/settings/ |
Settings UI for UnifiedPush distributor selection |
common/src/main/kotlin/io/homeassistant/companion/android/common/data/integration/ |
Extended DeviceRegistration to support pushUrl and pushEncrypt fields |
common/src/test/kotlin/io/homeassistant/companion/android/common/push/ |
Comprehensive unit tests for push providers |
gradle/libs.versions.toml |
Added UnifiedPush connector library dependency |
| appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})", | ||
| deviceName = null, | ||
| pushToken = result?.pushToken, | ||
| pushUrl = result?.pushUrl ?: result?.pushToken?.let { "" }, |
There was a problem hiding this comment.
The pushUrl fallback logic incorrectly converts WebSocket provider's null pushUrl to an empty string. When result?.pushToken is empty (as with WebSocket), the elvis operator and let should not produce an empty string. This causes WebSocket registrations to be incorrectly converted to the default push URL instead of remaining empty.
| pushUrl = result?.pushUrl ?: result?.pushToken?.let { "" }, | |
| pushUrl = result?.pushUrl | |
| ?: result?.pushToken | |
| ?.takeIf { it.isNotEmpty() } | |
| ?.let { "" }, |
| DeviceRegistration( | ||
| appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})", | ||
| pushToken = result?.pushToken, | ||
| pushUrl = result?.pushUrl ?: result?.pushToken?.let { "" }, |
There was a problem hiding this comment.
The pushUrl fallback logic incorrectly converts WebSocket provider's null pushUrl to an empty string. When result?.pushToken is empty (as with WebSocket), the elvis operator and let should not produce an empty string. This causes WebSocket registrations to be incorrectly converted to the default push URL instead of remaining empty.
| pushUrl = result?.pushUrl ?: result?.pushToken?.let { "" }, | |
| pushUrl = result?.pushUrl ?: result?.pushToken?.takeIf { it.isNotBlank() }?.let { "" }, |
| appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})", | ||
| deviceName = deviceName, | ||
| pushToken = pushResult?.pushToken, | ||
| pushUrl = pushResult?.pushUrl ?: pushResult?.pushToken?.let { "" }, |
There was a problem hiding this comment.
The pushUrl fallback logic incorrectly converts WebSocket provider's null pushUrl to an empty string. When result?.pushToken is empty (as with WebSocket), the elvis operator and let should not produce an empty string. This causes WebSocket registrations to be incorrectly converted to the default push URL instead of remaining empty.
| pushUrl = pushResult?.pushUrl ?: pushResult?.pushToken?.let { "" }, | |
| pushUrl = pushResult?.pushUrl, |
| appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})", | ||
| deviceName = deviceName, | ||
| pushToken = pushResult?.pushToken, | ||
| pushUrl = pushResult?.pushUrl ?: pushResult?.pushToken?.let { "" }, |
There was a problem hiding this comment.
The pushUrl fallback logic incorrectly converts WebSocket provider's null pushUrl to an empty string. When result?.pushToken is empty (as with WebSocket), the elvis operator and let should not produce an empty string. This causes WebSocket registrations to be incorrectly converted to the default push URL instead of remaining empty.
| pushUrl = pushResult?.pushUrl ?: pushResult?.pushToken?.let { "" }, | |
| pushUrl = pushResult?.pushUrl | |
| ?: pushResult?.pushToken?.takeIf { it.isNotEmpty() }?.let { "" }, |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
ktlint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The UnifiedPush connector depends on Tink which pulls in protobuf-java, conflicting with protobuf-javalite used by Firebase/gRPC in the full flavor. This exclusion was already applied to the minimal flavor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adapt to upstream refactoring: MessagingTokenProvider, servers(), kotlinx.serialization, and new SettingsPresenter interface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| android:value="true" /> | ||
| </service> | ||
|
|
||
| <receiver |
Check failure
Code scanning / Android Lint
Receiver does not require permission
|
|
||
| private fun updateNotificationUnifiedPushPrefs() { | ||
| val notificationsEnabled = | ||
| Build.VERSION.SDK_INT < Build.VERSION_CODES.O || |
Check failure
Code scanning / Android Lint
Obsolete SDK_INT Version Check
Add ExportedReceiver baseline entry for the UnifiedPush broadcast receiver (must be exported for distributors to send messages) and ObsoleteSdkInt for the automotive flavor SDK check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The Unit Tests check failed with 1 failure out of 500 tests: This is a pre-existing flaky test unrelated to the UnifiedPush changes -- it tests network connectivity status monitoring, not push notifications. All 28 UnifiedPush-specific tests pass. |
|
Thanks for the attempt but please don't merge everything into one giant PR. The abstraction can and should still be a different PR. You're also presuming that notifications have a priority, and that websocket is exclusively an alternative, while it technically exists side-by-side and I would argue that the priority should be up to the user. Something that can be discussed in more details if you do abstraction separately. There's so much AI generated code here I'm a bit skeptical. Did you test this yourself? Can you provide screenshots of your implementation as requested in the PR template? |
Address reviewer feedback: remove priority-based auto-selection in favor of a user-facing "Push provider" setting under Notifications. The user can now switch between FCM, UnifiedPush distributors, and WebSocket at any time without restarting the app. - Remove `priority` field from PushProvider interface - Replace hidden UnifiedPush-only preference with always-visible "Push provider" ListPreference showing all available providers - Add IgnoreViolationRule for UnifiedPush connector StrictMode violations - Fix UnifiedPush message parsing for nested JSON objects - Auto-restart WebSocket push channel when switching providers - Add fallback to empty token when FCM is unavailable - Fix toast shown incorrectly when disabling UnifiedPush - Update tests to reflect priority removal Tested on Pixel 9 Pro Fold (Android 16, GrapheneOS): - Switch WebSocket <-> UnifiedPush in both directions without restart - Notifications received on both providers in foreground and background - App killed: UnifiedPush still delivers via ntfy - Auto-fallback to WebSocket when ntfy topic is deleted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Thanks for the feedback @jpelgrom, you raised valid points. I've reworked the approach based on your review: Priority removed — user choosesThe hardcoded priority system is gone. The WebSocket is no longer treated as a "fallback". It's a first-class option at the same level as the others. Switching between providers is instant and doesn't require an app restart. Splitting the PRI'll split this into two PRs as suggested:
Tested on deviceTested on a Pixel 9 Pro Fold running Android 16 (GrapheneOS):
I'll provide screenshots in the split PRs. |
|
@sk7n4k3d what do we do about this PR? if you did split it please close it. |
|
@TimoPtr Splitting it now. I'll close this PR and open two separate ones:
Will have them up shortly. |
|
Closing in favor of two split PRs as requested:
|
Summary
Adds support for UnifiedPush as an alternative push notification provider, allowing users to receive notifications without relying on Google's FCM infrastructure. This is especially useful for the minimal/F-Droid flavor and privacy-conscious users.
Building on the work started by @lone-faerie in #5261 and #5301, this PR:
PushProviderinterface with priority-based selection via Dagger multibindingFcmPushProvider(full flavor),UnifiedPushProvider, andWebSocketPushProvider(fallback)PushProviderManagerto coordinate provider selection, registration, and server updatesLaunchActivity,LaunchPresenter, andSettingsPresenterBug fixes on top of the original implementation
Architecture
Closes #3174
Related: #5261, #5301, #3206, #1480
Checklist
Test plan
Any other notes
This builds on the excellent foundational work by @lone-faerie in #5261 and incorporates the PushProvider interface approach suggested by reviewers @TimoPtr and @jpelgrom in #5301.