Add generic PushProvider abstraction for notification delivery#6619
Draft
sk7n4k3d wants to merge 5 commits intohome-assistant:mainfrom
Draft
Add generic PushProvider abstraction for notification delivery#6619sk7n4k3d wants to merge 5 commits intohome-assistant:mainfrom
sk7n4k3d wants to merge 5 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 👍 |
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new push-notification provider abstraction layer to the Android companion app, aiming to enable multiple push delivery mechanisms (FCM, WebSocket, and future providers) behind a common interface and expose a user-selectable provider setting.
Changes:
- Introduces
PushProvider+PushProviderManagerand adds unit tests for the abstraction layer - Extends device registration payload/storage to include
pushUrlandpushEncrypt - Adds a new “Push provider” ListPreference plus WebSocket/FCM provider implementations and Dagger multibindings
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| common/src/main/kotlin/io/homeassistant/companion/android/common/push/PushProvider.kt | New push provider interface + registration result model |
| common/src/main/kotlin/io/homeassistant/companion/android/common/push/PushProviderManager.kt | Provider selection + server registration update helper |
| common/src/main/kotlin/io/homeassistant/companion/android/common/data/integration/DeviceRegistration.kt | Adds pushUrl and pushEncrypt fields to registration model |
| common/src/main/kotlin/io/homeassistant/companion/android/common/data/integration/impl/IntegrationRepositoryImpl.kt | Persists pushUrl and sends push_url/push_encrypt during registration updates |
| common/src/main/res/values/strings.xml | Adds strings for the new push provider setting |
| common/src/test/kotlin/io/homeassistant/companion/android/common/push/PushProviderTest.kt | Unit tests for default interface behaviors and PushRegistrationResult |
| common/src/test/kotlin/io/homeassistant/companion/android/common/push/PushProviderManagerTest.kt | Unit tests for provider manager selection and server update behavior |
| app/src/main/res/xml/preferences.xml | Adds “Push provider” ListPreference under Notifications |
| app/src/main/kotlin/io/homeassistant/companion/android/settings/SettingsPresenter.kt | Adds presenter APIs for push provider list/value/change |
| app/src/main/kotlin/io/homeassistant/companion/android/settings/SettingsPresenterImpl.kt | Implements push provider list/value/change (currently in-memory) |
| app/src/main/kotlin/io/homeassistant/companion/android/settings/SettingsFragment.kt | Populates ListPreference entries dynamically and restarts WebSocket worker when selected |
| app/src/main/kotlin/io/homeassistant/companion/android/websocket/WebsocketManager.kt | Adds restart() helper to re-enqueue periodic WebSocket worker |
| app/src/main/kotlin/io/homeassistant/companion/android/push/WebSocketPushProvider.kt | WebSocket-backed PushProvider implementation |
| app/src/full/kotlin/io/homeassistant/companion/android/push/FcmPushProvider.kt | Full-flavor FCM-backed PushProvider implementation |
| app/src/full/kotlin/io/homeassistant/companion/android/push/PushProviderModule.kt | Hilt multibinding module for full flavor providers |
| app/src/minimal/kotlin/io/homeassistant/companion/android/push/PushProviderModule.kt | Hilt multibinding module for minimal flavor providers |
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!
sk7n4k3d
force-pushed
the
push-provider-abstraction
branch
from
d84a596 to
d05d1c9
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Member
|
Check the CI and also the comments from Copilot and then put the PR ready for review again once ready. |
Author
|
All Copilot review comments addressed and ktlint fixed:
CI should pass now. Ready for review. |
Member
|
CI is failing on building the wear module |
5 tasks
7 tasks
Author
|
@TimoPtr Fixed the wear build failure. The issue was positional arguments in Switched to named parameters in both wear files so it's resilient to parameter order changes. CI should be green now. |
Introduce a PushProvider interface that abstracts push notification delivery, allowing the user to choose between available providers (FCM, WebSocket) via a new "Push provider" setting under Notifications. This lays the groundwork for additional providers (e.g. UnifiedPush) without coupling the abstraction to any specific implementation. - Add PushProvider interface with name, isAvailable, isActive, register, unregister, and requiresPersistentConnection - Add PushProviderManager for provider selection and server registration - Add FcmPushProvider (full flavor) and WebSocketPushProvider - Add Dagger multibinding modules for both flavors - Add "Push provider" ListPreference in Notifications settings - Add WebsocketManager.restart() for instant provider switching - Extend DeviceRegistration with pushUrl and pushEncrypt fields - Add 28 unit tests for PushProvider and PushProviderManager Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove suspend from WebsocketManager.restart() (no suspending calls) - Remove unused context parameter from WebSocketPushProvider - Clear server-side push config when switching to WebSocket - Rethrow CancellationException in PushProviderManager and FcmPushProvider - Derive initial push provider from available providers instead of hardcoded default - Stabilize getAllProviders() ordering with sortedBy name - Fix PushProvider KDoc to reflect user-selection model - Wire handlePushProviderChange to actually call selectAndRegister - Remove dead getString/putString cases for notification_push_provider - Move push provider label resolution to Fragment using string resources Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Persist selectedPushProvider via PrefsRepository instead of in-memory variable to survive process death - Trigger server-side registration update when push provider changes by calling updateServerRegistration after selectAndRegister - Add explanatory comment for preferenceDataStore = null in SettingsFragment push provider preference - Replace hardcoded "WebSocket" fallback with WebSocketPushProvider.NAME constant reference Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ment - Fix import ordering in SettingsFragment.kt and SettingsPresenterImpl.kt - Fix argument wrapping for Toast.makeText call in SettingsFragment.kt - Fix constructor parameter formatting in FcmPushProvider.kt - Clarify PushProvider KDoc: explicitly note user-configurable selection with no automatic "best provider" logic
The positional args broke when pushUrl/pushEncrypt were added to DeviceRegistration. Boolean was being passed where String? was expected. Named parameters make this resilient to parameter order.
sk7n4k3d
force-pushed
the
push-provider-abstraction
branch
from
bd375c8 to
54b225f
Compare
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.
Summary
PushProviderinterface that abstracts push notification delivery, allowing users to choose their preferred providerPushProviderManagerfor provider selection and server registrationFcmPushProvider(full flavor) andWebSocketPushProvideras built-in providersDeviceRegistrationwithpushUrlandpushEncryptfields for future provider supportWebsocketManager.restart()for instant WebSocket push channel resubscription on provider switchThis is the first of two PRs splitting the UnifiedPush work as requested in #6599. This PR contains only the generic abstraction layer — no UnifiedPush-specific code. The second PR (#TBD) will add the UnifiedPush implementation on top.
Testing
Type of change