Skip to content

feat: make foreground service opt-in#1053

Open
jvsena42 wants to merge 4 commits into
masterfrom
feat/fg-service-optional
Open

feat: make foreground service opt-in#1053
jvsena42 wants to merge 4 commits into
masterfrom
feat/fg-service-optional

Conversation

@jvsena42

@jvsena42 jvsena42 commented Jun 29, 2026

Copy link
Copy Markdown
Member

Fixes #1007

This PR:

  1. Adds a "Keep Bitkit active in background" toggle to the Background Payments screen that makes the persistent foreground service opt-in and disabled by default. The Lightning node still runs while the app is open, and background payments still arrive via push notifications; enabling the toggle keeps Bitkit active in the background for more reliable payments, at the cost of a persistent notification.
  2. Updates the Background Payments screen and the notification preview to match the latest design.
  3. Always shows the amount in payment notifications and removes the "Include amount in notifications" option.
  4. Fixes a Lightning event-handler leak so toggling the service on and off no longer accumulates stale handlers.

Description

  • New persisted setting, off by default. The foreground service start gate now also requires it, and turning it off stops a running service immediately so the persistent notification disappears at once.
  • Stopping the foreground service no longer stops the in-app node when an activity is active; in the foreground the node lifecycle is owned by the wallet view model, so toggling the service off leaves the node running. When the app is backgrounded, the node still stops as before.
  • The Background Payments screen is reordered to match the design: "Get paid when Bitkit is closed", "Keep Bitkit active in background", a Notifications section with the Customize button, and a Preview of the payment notification. The "Keep Bitkit active in background" switch is disabled while notifications are denied.
  • The notification preview is redesigned to the Android notification card style, with the app icon, title, time, amount, and an expand chevron.
  • Removed the show-notification-details setting and its privacy toggle. Payment notifications now always include the amount across the foreground service, in-app handler, and the background push path.
  • Added a remove-event-handler API and de-register the foreground service handler when the service is destroyed, preventing stale handlers from accumulating across service restarts.

Preview

disable-fg-switch.webm
running-from-master.webm
disable-notifications.webm
enable-fg-service.webm

QA Notes

FIGMA

Manual Tests

  • 1. Settings → Background Payments: "Keep Bitkit active in background" is off by default and no persistent Bitkit notification is shown in the status bar.
  • 2. Background Payments → toggle Keep active on: persistent Bitkit notification appears → background the app: node stays alive.
  • 3. Background Payments → toggle Keep active off: persistent notification disappears immediately → with the app still open, receive a payment: still works (node keeps running).
  • 4. Revoke notification permission in Android settings → Background Payments: "Get paid when Bitkit is closed" reads off and "Keep Bitkit active in background" is disabled.
  • 5. regression: Keep active off, app closed → receive a payment: push notification arrives and shows the amount.
  • 6. Background Payments: layout matches the design (two switches with explanations, Notifications/Customize, Preview card).

Automated Checks

  • Unit tests added: default value and setter for the new toggle in SettingsViewModelTest.kt.
  • Test coverage removed: hidden-amount assertions deleted from ReceivedNotificationContentTest.kt and NotifyChannelReadyHandlerTest.kt because the show-notification-details option no longer exists; SettingsData stubs updated in NotifyPaymentReceivedHandlerTest.kt.
  • Ran locally: just compile, the affected unit tests, and just lint.
  • CI: standard compile, unit test, and detekt checks run by the PR bot.

@jvsena42 jvsena42 self-assigned this Jun 29, 2026
@jvsena42 jvsena42 marked this pull request as ready for review June 29, 2026 20:35
@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown

Greptile Summary

This 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.

  • New keepBitkitActiveInBackground setting (persisted, default false): gates foreground service start in MainActivity and stops a running service immediately when toggled off; LightningNodeService.onDestroy now skips stopping the node when an activity is still active so WalletViewModel can continue owning the node lifecycle in the foreground.
  • Handler-leak fix: nodeEventHandler is promoted to a stable property-level reference and explicitly removed from LightningRepo._eventHandlers (a ConcurrentHashMap set) in onDestroy, preventing accumulation across restarts.
  • Notification simplification: showNotificationDetails removed; ReceivedNotificationContent always formats the amount.

Confidence Score: 3/5

The 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 onDestroy is not applied in onTimeout, meaning an Android 14+ foreground-service timeout while the app is in the foreground will stop the node under an active session. Separately, the LaunchedEffect only calls stopForegroundService() when keepActive is toggled off, so revoking notification permission while the service is running leaves it alive until the activity is closed.

LightningNodeService.kt (the onTimeout override) and MainActivity.kt (the LaunchedEffect stop condition) need the most attention.

Important Files Changed

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]
Loading
%%{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]
Loading

Comments Outside Diff (1)

  1. app/src/main/java/to/bitkit/androidServices/LightningNodeService.kt, line 257-263 (link)

    P1 onTimeout stops node unconditionally, bypassing the new foreground guard

    onTimeout explicitly calls lightningRepo.stop() before super.onTimeout() triggers onDestroy, so the "skip node stop when an activity is active" guard added to onDestroy is never reached for this path. If the system fires onTimeout (Android 14+ foreground service runtime budget exhausted) while the user is actively using the app, the node stops underneath WalletViewModel, which now "owns" the node lifecycle in the foreground. The onDestroy check for App.currentActivity?.value cannot 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

Comment on lines +135 to 141
val canStartService = walletExists && notificationsGranted && keepActive && restoreState.isIdle()
if (canStartService && !isRecoveryMode) {
tryStartForegroundService()
} else if (!keepActive) {
stopForegroundService()
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant run {} wrapper

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.

Suggested change
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!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +138 to +139
} else if (!keepActive) {
stopForegroundService()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +135 to +139
val canStartService = walletExists && notificationsGranted && keepActive && restoreState.isIdle()
if (canStartService && !isRecoveryMode) {
tryStartForegroundService()
} else if (!keepActive) {
stopForegroundService()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Polish Notifications Permissions Request

1 participant