feat: make foreground service opt-in#1053
Conversation
Greptile SummaryThis PR makes the Lightning foreground service opt-in via a new "Keep Bitkit active in background" toggle (off by default), redesigns the Background Payments settings screen and notification preview to match the latest design, always includes the payment amount in notifications (removing the privacy toggle), and fixes a handler-leak bug that caused stale LDK event handlers to accumulate across service restarts.
Confidence Score: 3/5The core toggle, handler-leak fix, and notification simplification are well-structured, but two gaps in the service-lifecycle logic need attention before merge. The new skip-node-stop-when-activity-active logic in
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/androidServices/LightningNodeService.kt | Extracts the event handler to a stable property so it can be removed on destroy (fixing the handler-leak bug), and adds a guard to skip stopping the node when an activity is still active. However, onTimeout still calls lightningRepo.stop() unconditionally, bypassing this new guard. |
| app/src/main/java/to/bitkit/ui/MainActivity.kt | Adds keepBitkitActiveInBackground as a new gate for starting the foreground service and calls stopForegroundService() when the toggle is off. The stop-on-permission-revoke path while the activity is foregrounded is only handled in onDestroy, not in the LaunchedEffect. |
| app/src/main/java/to/bitkit/repositories/LightningRepo.kt | Adds removeEventHandler to the singleton repo, backed by the existing ConcurrentHashMap.newKeySet(). Thread-safe and correct; contains a minor redundant run {} wrapper. |
| app/src/main/java/to/bitkit/data/SettingsStore.kt | Renames showNotificationDetails (default true) to keepBitkitActiveInBackground (default false). Existing users will transparently default to false on upgrade, which is the intended opt-in behaviour. |
| app/src/main/java/to/bitkit/viewmodels/SettingsViewModel.kt | Replaces toggleNotificationDetails() with explicit-value setKeepBitkitActiveInBackground(value), making the setter idempotent and easier to test. |
| app/src/main/java/to/bitkit/domain/commands/ReceivedNotificationContent.kt | Removes the showNotificationDetails branch; notifications now always include the amount. |
| app/src/main/java/to/bitkit/ui/settings/backgroundPayments/BackgroundPaymentsSettings.kt | Reorders the Background Payments screen to add the Keep Bitkit active switch and notification preview. Straightforward UI restructuring. |
| app/src/test/java/to/bitkit/viewmodels/SettingsViewModelTest.kt | Adds two unit tests covering the default value and round-trip persistence of keepBitkitActiveInBackground. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MainActivity LaunchedEffect] --> B{canStartService?}
B -- yes --> C[tryStartForegroundService]
B -- no && keepActive=false --> D[stopForegroundService]
B -- no && keepActive=true --> E[no-op: permission revoked]
C --> F[LightningNodeService.onDestroy]
F --> G{activity active?}
G -- no --> H[lightningRepo.stop]
G -- yes --> I[Skip stop: WalletViewModel owns lifecycle]
F --> J[removeEventHandler]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[MainActivity LaunchedEffect] --> B{canStartService?}
B -- yes --> C[tryStartForegroundService]
B -- no && keepActive=false --> D[stopForegroundService]
B -- no && keepActive=true --> E[no-op: permission revoked]
C --> F[LightningNodeService.onDestroy]
F --> G{activity active?}
G -- no --> H[lightningRepo.stop]
G -- yes --> I[Skip stop: WalletViewModel owns lifecycle]
F --> J[removeEventHandler]
Comments Outside Diff (1)
-
app/src/main/java/to/bitkit/androidServices/LightningNodeService.kt, line 257-263 (link)onTimeoutstops node unconditionally, bypassing the new foreground guardonTimeoutexplicitly callslightningRepo.stop()beforesuper.onTimeout()triggersonDestroy, so the "skip node stop when an activity is active" guard added toonDestroyis never reached for this path. If the system firesonTimeout(Android 14+ foreground service runtime budget exhausted) while the user is actively using the app, the node stops underneathWalletViewModel, which now "owns" the node lifecycle in the foreground. TheonDestroycheck forApp.currentActivity?.valuecannot help because the stop has already been issued by this point.
Reviews (1): Last reviewed commit: "refactor: always show amount in payment ..." | Re-trigger Greptile
| val canStartService = walletExists && notificationsGranted && keepActive && restoreState.isIdle() | ||
| if (canStartService && !isRecoveryMode) { | ||
| tryStartForegroundService() | ||
| } else if (!keepActive) { | ||
| stopForegroundService() | ||
| } | ||
| } |
There was a problem hiding this comment.
Service not stopped inline when notification permission is revoked while
keepActive is true
The else if (!keepActive) branch only fires when keepActive is explicitly toggled off, so revoking notification permission while the service is running (keepActive = true, notificationsGranted transitions to false) leaves the service running until onDestroy. This means the persistent foreground service notification may remain visible in the status bar even after the user has revoked notification permission in Android Settings, until the activity is closed. The onDestroy guard only handles this on app close, not while the app is in the foreground.
| result | ||
| } | ||
|
|
||
| fun removeEventHandler(handler: NodeEventHandler) = run { _eventHandlers.remove(handler) } |
There was a problem hiding this comment.
The run { ... } block here is an identity no-op — it just executes the lambda and returns its result. The function can be written directly as = _eventHandlers.remove(handler) without the intermediate scope.
| fun removeEventHandler(handler: NodeEventHandler) = run { _eventHandlers.remove(handler) } | |
| fun removeEventHandler(handler: NodeEventHandler) = _eventHandlers.remove(handler) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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".
| } else if (!keepActive) { | ||
| stopForegroundService() |
There was a problem hiding this comment.
Stop the node after opting out of the service
When Keep Bitkit active is off, this branch stops only LightningNodeService while the activity is still active. LightningNodeService.onDestroy() now intentionally skips lightningRepo.stop() when App.currentActivity is non-null, and ContentView's ON_STOP gate still only stops the wallet-owned node when notifications are denied, so users with notifications granted and the new default/off setting continue running the LDK node after backgrounding the app without a foreground-service notification. The background lifecycle needs to also stop walletViewModel when keepActive is false.
Useful? React with 👍 / 👎.
| val canStartService = walletExists && notificationsGranted && keepActive && restoreState.isIdle() | ||
| if (canStartService && !isRecoveryMode) { | ||
| tryStartForegroundService() | ||
| } else if (!keepActive) { | ||
| stopForegroundService() |
There was a problem hiding this comment.
Stop the service when notifications are revoked
If a user has keepActive=true and then revokes notification permission in Android settings, canStartService becomes false but this else if (!keepActive) branch does not run because the stored keep-active toggle remains true. The existing foreground service therefore keeps running even though the screen reports background payments disabled and the start gate no longer permits it; stop the service whenever notifications are no longer granted as well.
Useful? React with 👍 / 👎.
Fixes #1007
This PR:
Description
Preview
disable-fg-switch.webm
running-from-master.webm
disable-notifications.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.