feat: make foreground service opt-in#1053
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a98728be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ovitrif
left a comment
There was a problem hiding this comment.
Reviewed locally. I left two comments: one lifecycle issue around the background transition, and one changelog cleanup. Leaving this as a neutral review.
Pull request was converted to draft
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e571137fb2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pull request was converted to draft
…he LN node would keep running after app ON_STOP
|
Tested again on device (Samsung S22, Android 16). Observations Testing "Get paid when Bitkit is closed" ON:
Attaching session logs. Implementation looks correct to me — approve. Non-blocking: the text under "Get paid when Bitkit is closed" (
Worth a QA note too: case 5 with battery saver off; Samsung + battery saver on = known limitation for the default (no persistent notification) path. |
|
Disabled auto-merge so there’s room to update |
I worth a specific investigation. It is possible to display some warning when the app is in battery save mode and also enable specific permissions to don't limit the app on battery safe mode, but first we need to improve the node consumption. |
|
I left comment for future iterations https://www.figma.com/design/ltqvnKiejWj0JQiqtDf2JJ?node-id=40364-135743&m=dev#1827426454 |
Fixes #1007
This PR:
Description
Preview
disable-fg-switch.webm
disable-notifications.webm
running-from-master.webm
wake-to-pay.webm
enable-fg-service.webm
QA Notes
FIGMA
Manual Tests
regression:Keep active off, app closed → receive a payment: push notification arrives and shows the amount.Automated Checks
SettingsViewModelTest.kt.ReceivedNotificationContentTest.ktandNotifyChannelReadyHandlerTest.ktbecause the show-notification-details option no longer exists;SettingsDatastubs updated inNotifyPaymentReceivedHandlerTest.kt.just compile, the affected unit tests, andjust lint.