fix: LightningNodeService startup

[jvsena42]

Closes #134

Description

This PR improves the LightningNodeService startup. Now it is only called if the wallet exists, notifications are enabled and the app is not in recovery mode

Preview

recovery_mode.webm

create_allow_notifications.webm

node_working_without_service.webm

create_dont_allow.webm

QA Notes

Note: Notification permission is only required for API 33+

• Device with and without google services
  • Create / recover wallet -> Allow notifications -> should start service
  • Create / recover wallet -> DON'T Allow notifications -> should NOT start service
  • Wallet not created -> DON'T start service
• Recovery mode -> DON'T start service
• Node should work even with service disabled

[jvsena42]

on draft to check behavior of when disable notifications after previously enabled

[claude]

PR Review: Fix LightningNodeService startup

Summary

This PR improves the LightningNodeService startup logic by ensuring the service only starts when appropriate conditions are met (wallet exists, notifications enabled, and not in recovery mode). The implementation is solid and addresses issue #134 effectively.

✅ Strengths

1. Clear Intent: The PR has a well-defined purpose and solves a specific problem
2. Good State Management: Uses reactive flows to monitor conditions (walletExists, notificationsGranted, isRecoveryMode)
3. Comprehensive Testing: QA notes show thorough manual testing across different scenarios
4. Error Handling: Proper use of runCatching blocks to handle service lifecycle errors

🐛 Potential Issues

1. Race Condition in MainActivity:91-108

Location: MainActivity.kt:100-107

LaunchedEffect(
    walletExists,
    isRecoveryMode,
    notificationsGranted
) {
    if (walletExists && !isRecoveryMode && notificationsGranted) {
        tryStartForegroundService()
    }
}

Issue: The walletExists state uses initialValue = false in the flow collection, which may cause a race condition during app startup. If the wallet actually exists but the flow hasn't emitted yet, the service won't start.

Recommendation: Consider initializing with the actual wallet state:

val walletExists by walletViewModel.walletState
    .map { it.walletExists }
    .collectAsStateWithLifecycle(initialValue = walletViewModel.walletExists)

2. Missing Service Stop Logic in ContentView

Location: ContentView.kt:216-222

The ON_STOP lifecycle event stops the node when notifications aren't granted, but there's no corresponding logic to stop the service. This creates an inconsistency:

• Node is stopped via walletViewModel.stop()
• Service may still be running in the background

Recommendation: Consider stopping the service explicitly or document why this is intentional.

3. Redundant Try-Catch in onDestroy

Location: MainActivity.kt:192-199

override fun onDestroy() {
    super.onDestroy()
    if (!settingsViewModel.notificationsGranted.value) {
        runCatching {
            stopService(Intent(this, LightningNodeService::class.java))
        }
    }
}

Issue: The runCatching silently swallows any errors. If the service fails to stop, you won't know why.

Recommendation: At minimum log the error:

runCatching {
    stopService(Intent(this, LightningNodeService::class.java))
}.onFailure { error ->
    Logger.error("Failed to stop LightningNodeService", error, context = "MainActivity")
}

4. Removed Error Handling in ContentView:202-204

Location: ContentView.kt:202-204

Before:

try {
    walletViewModel.start()
} catch (e: Throwable) {
    Logger.error("Failed to start wallet", e)
}

After:

if (walletExists && !isRecoveryMode) {
    walletViewModel.start()
}

Issue: The try-catch block was removed, but errors can still occur in walletViewModel.start(). While the error handling exists inside the ViewModel, removing the outer try-catch means uncaught exceptions could crash the app at the call site.

Recommendation: Keep defensive error handling at the UI layer or document why it's safe to remove.

🔒 Security Considerations

✅ No security concerns identified. The service start conditions appropriately check permissions and wallet state.

⚡ Performance Considerations

1. Multiple LaunchedEffects: The PR introduces reactive monitoring of three states. This is appropriate but ensure these flows don't emit too frequently.
2. Service Start Overhead: startForegroundService is called in a LaunchedEffect which could trigger multiple times during configuration changes. Consider adding a flag to prevent duplicate service starts.

🧪 Test Coverage

⚠️ No automated tests were added for this functionality. While manual testing was thorough, consider adding:

• Unit tests for the service start conditions
• Integration tests for the lifecycle interactions
• Tests for edge cases (rapid configuration changes, permission changes during runtime)

📝 Code Quality & Style

Minor Issues:

1. Inconsistent Property Access (ContentView.kt:189-195):

   val walletUiState by walletViewModel.walletState.collectAsStateWithLifecycle()
   val lightningState by walletViewModel.lightningState.collectAsStateWithLifecycle()
   // ... later:
   val walletExists = walletUiState.walletExists

   These states are collected at the top but walletExists is extracted inline. Consider consistency.

2. TODO Comments Removed: The PR removed TODO comments without addressing them:

   • Line 186: "TODO ADAPT THIS LOGIC TO WORK WITH LightningNodeService" - removed
   • Line 213: "TODO ADAPT THIS LOGIC TO WORK WITH LightningNodeService" - removed

   These should either be addressed or tracked in a follow-up issue.

3. Deprecated Property Usage (ContentView.kt:360):

   val walletUiState by walletViewModel.uiState.collectAsState()

   This uses the deprecated uiState property. Consider migrating to the new state properties.

🎯 Recommendations

High Priority:

1. Fix the race condition with initialValue in MainActivity
2. Add error logging to the onDestroy runCatching block
3. Consider service stop logic consistency in ON_STOP event

Medium Priority:
4. Add automated tests for the service lifecycle logic
5. Address or track the removed TODO comments
6. Add duplicate service start prevention

Low Priority:
7. Migrate away from deprecated uiState property
8. Consider extracting the service start conditions into a helper function for testability

✨ Overall Assessment

This is a solid improvement that addresses a real issue with clear benefits. The main concerns are around edge cases and error handling rather than fundamental design issues. With the recommended fixes, this would be production-ready.

Recommendation: Approve with minor changes requested.

---

Review generated by Claude Code

[claude]

Code Review - PR #478: Fix LightningNodeService startup

Summary

This PR improves the LightningNodeService startup logic by ensuring it only starts when appropriate conditions are met (wallet exists, notifications enabled, not in recovery mode). The implementation looks solid overall, but I've identified several areas for improvement.

---

✅ Strengths

1. Clear Intent: The PR addresses a real issue with service lifecycle management and adds proper conditional logic
2. Good State Management: Uses proper Kotlin Flow patterns with collectAsStateWithLifecycle()
3. Lifecycle Awareness: Properly handles app foreground/background transitions
4. Comprehensive Testing: QA notes show thorough manual testing across different scenarios

---

🔴 Critical Issues

1. Removed Error Handling in ContentView.kt:202-203

Location: ContentView.kt:202-203

The original try-catch block around walletViewModel.start() was removed:

// Before
try {
    walletViewModel.start()
} catch (e: Throwable) {
    Logger.error("Failed to start wallet", e)
}

// After
if (walletExists && !isRecoveryMode) {
    walletViewModel.start()
}

Issue: While walletViewModel.start() handles errors internally via onFailure, removing the try-catch could expose crashes if the coroutine context fails or if there are unexpected exceptions before the coroutine is launched.

Recommendation: Wrap the call in a runCatching or add back the try-catch for defensive programming:

if (walletExists && !isRecoveryMode) {
    runCatching {
        walletViewModel.start()
    }.onFailure { error ->
        Logger.error("Failed to start wallet in ON_START", error)
    }
}

---

⚠️ High Priority Issues

2. Race Condition in MainActivity.kt:100-108

Location: MainActivity.kt:100-108

LaunchedEffect(
    walletExists,
    isRecoveryMode,
    notificationsGranted
) {
    if (walletExists && !isRecoveryMode && notificationsGranted) {
        tryStartForegroundService()
    }
}

Issues:

• The walletExists initial value uses walletViewModel.walletExists (line 97), which might not be synchronized with the flow's actual state
• Multiple rapid state changes could trigger the service multiple times
• No cleanup/stop logic if conditions become false after the service has started

Recommendations:

1. Consider debouncing or adding a flag to prevent multiple start attempts
2. Add logic to stop the service when conditions are no longer met
3. Use consistent state sources (all from flows, not mixing property access)

3. Service Lifecycle Management in MainActivity.onDestroy:192-198

Location: MainActivity.onDestroy:192-198

override fun onDestroy() {
    super.onDestroy()
    if (!settingsViewModel.notificationsGranted.value) {
        runCatching {
            stopService(Intent(this, LightningNodeService::class.java))
        }
    }
}

Issues:

• Accessing .value directly in onDestroy() is risky—the ViewModel might already be cleared
• The service might be stopping asynchronously via onDestroy() while this is also calling stopService()
• No guarantee that settingsViewModel is still valid

Recommendations:

1. Add null check or use a safer state access pattern
2. Consider using a local flag to track if the service was started by this activity
3. Document why stopping the service here is necessary vs. letting it handle its own lifecycle

4. Missing Error Context in WalletViewModel.stop:156-166

Location: WalletViewModel.kt:156-166

fun stop() {
    if (!walletExists) return

    viewModelScope.launch(bgDispatcher) {
        lightningRepo.stop()
            .onFailure { error ->
                Logger.error("Node stop error", error)
                ToastEventBus.send(error)
            }
    }
}

Issue: Unlike start(), the stop() method shows toast errors to the user. This might be confusing when the app is backgrounding—users shouldn't see errors for expected behavior.

Recommendation: Add a parameter to control whether errors should be surfaced to the user:

fun stop(showErrors: Boolean = false) {
    if (!walletExists) return

    viewModelScope.launch(bgDispatcher) {
        lightningRepo.stop()
            .onFailure { error ->
                Logger.error("Node stop error", error)
                if (showErrors) {
                    ToastEventBus.send(error)
                }
            }
    }
}

---

💡 Medium Priority Issues

5. Inconsistent State Source in ContentView.kt

Location: ContentView.kt:189-195

The code now references both walletViewModel.walletState and walletViewModel.lightningState separately, then extracts nodeLifecycleState from lightningState. However, line 258 shows that the old code used walletUiState.nodeLifecycleState.

Issue: This suggests a refactoring that split uiState into separate states, but the deprecation notice on line 64 of WalletViewModel indicates uiState should be avoided. The code is inconsistent about which state source to use.

Recommendation: Ensure consistent usage throughout and update all references. Document the preferred state access pattern.

6. TODO Comments Removed Without Resolution

Location: ContentView.kt:188, 265

Two TODO comments were removed:

• // TODO ADAPT THIS LOGIC TO WORK WITH LightningNodeService

Issue: While the logic was adapted, it's unclear if the adaptation is complete or if there are still edge cases to handle.

Recommendation: Verify with the team that all service-related logic is properly integrated and no further work is needed.

---

📋 Low Priority / Style Issues

7. Typo in PR Title

The PR title has a typo: "LightningNodeServic" should be "LightningNodeService"

8. Magic Strings in Logging

Location: MainActivity.kt:199, 203

Logger.debug("Attempting to start LightningNodeService", context = "MainActivity")
Logger.error("Failed to start LightningNodeService", error, context = "MainActivity")

Recommendation: Consider using a constant for the context string or use TAG like in LightningNodeService.

9. Comment Clarity

Location: ContentView.kt:218-221

if (walletExists && !isRecoveryMode && !notificationsGranted) {
    // App backgrounded without notification permission - stop node
    walletViewModel.stop()
}
// If notificationsGranted=true, service keeps node running

Recommendation: The comment is helpful, but consider documenting what happens in other edge cases (e.g., when wallet doesn't exist).

---

🧪 Test Coverage

Missing Test Coverage:

• No unit tests for the new stop() method in WalletViewModel
• No tests for the MainActivity service lifecycle logic
• No tests for the ContentView ON_STOP behavior

Recommendations:

1. Add unit tests for WalletViewModel.stop() (similar to existing tests in WalletViewModelTest.kt)
2. Consider integration tests for service startup conditions
3. Test edge cases: rapid permission changes, app backgrounding during wallet initialization, etc.

---

🔒 Security Considerations

No significant security concerns identified. The changes primarily affect service lifecycle management and don't introduce new security vulnerabilities.

---

⚡ Performance Considerations

1. Multiple Service Start Attempts: The LaunchedEffect could trigger multiple times if state changes rapidly. Consider adding debouncing.
2. Flow Collection: Multiple flows are collected in ContentView—ensure this doesn't cause excessive recomposition.

---

📝 Overall Assessment

Quality: Good (7/10)

• The core logic is sound and addresses the issue
• Good use of Kotlin coroutines and Flow
• Manual testing was thorough

Recommendations for Merge:

• ✅ Can merge after addressing Critical Issue Android - Project setup #1 (error handling)
• ✅ Should address High Priority Issues POC wake2pay from notification #2-4 before or shortly after merge
• ✅ Consider the test coverage gaps for future work

Great work on improving the service lifecycle management! The conditional logic makes the app behavior much more predictable. 🚀

[claude]

Code Review for PR #478

Overview

This PR improves the LightningNodeService startup logic by adding proper conditions to ensure the service only starts when appropriate (wallet exists, notifications enabled, not in recovery mode). Overall, this is a solid improvement to the service lifecycle management.

✅ Strengths

1. Good problem identification: Properly addresses the issue where the service was starting unconditionally
2. Clear conditional logic: The conditions (walletExists && !isRecoveryMode && notificationsGranted) are well-defined and understandable
3. Proper error handling: Uses runCatching for service lifecycle operations
4. Good documentation: The tryStartForegroundService() method includes helpful KDoc

🐛 Potential Issues & Concerns

1. Critical: Race Condition in MainActivity.kt:91-107

val walletExists by walletViewModel.walletState
    .map { it.walletExists }
    .collectAsStateWithLifecycle(initialValue = walletViewModel.walletExists)

LaunchedEffect(
    walletExists,
    isRecoveryMode,
    notificationsGranted
) {
    if (walletExists && !isRecoveryMode && notificationsGranted) {
        tryStartForegroundService()
    }
}

Issue: The LaunchedEffect will trigger every time any of the three dependencies change. This means if a user toggles notification permissions multiple times, the service will be started repeatedly.

Recommendation: Add logic to track whether the service is already running or use a more idempotent approach.

2. Service Lifecycle Issue in ContentView.kt:216-222

Lifecycle.Event.ON_STOP -> {
    if (walletExists && !isRecoveryMode && !notificationsGranted) {
        walletViewModel.stop()
    }
}

Issue: This stops the node when backgrounded without notifications, but the service itself continues running. In MainActivity.onDestroy() (line 193), you only stop the service if !notificationsGranted, but this won't match states where the service was already stopped by walletViewModel.stop().

Recommendation: Consider synchronizing the service state with the node state more consistently.

3. Removed Try-Catch in ContentView.kt:202-204

// Old code:
try {
    walletViewModel.start()
} catch (e: Throwable) {
    Logger.error("Failed to start wallet", e)
}

// New code:
if (walletExists && !isRecoveryMode) {
    walletViewModel.start()
}

Issue: The explicit error handling was removed. While WalletViewModel.start() already has internal error handling, surface-level errors during lifecycle events might be silently lost.

Recommendation: Consider keeping the try-catch or ensure error handling in WalletViewModel.start() is comprehensive.

4. Typo in PR Title

The PR title says "LightningNodeServic startup" - missing the "e" at the end.

🔒 Security Considerations

• ✅ No security vulnerabilities identified
• ✅ Proper permission checking for notifications
• ✅ Service lifecycle is properly managed to prevent unauthorized background execution

⚡ Performance Considerations

• ✅ Good: Service only starts when needed, reducing battery drain
• ⚠️ Minor concern: Multiple collectAsStateWithLifecycle calls could be optimized by collecting once and destructuring

🧪 Test Coverage

Missing tests for:

• Service startup conditions (walletExists, !isRecoveryMode, notificationsGranted)
• Service lifecycle during state transitions
• Edge cases:
  • What happens when notification permission is revoked while service is running?
  • What happens during recovery mode transitions?
  • What happens when wallet is deleted while service is running?

Recommendation: Add integration tests for LightningNodeService lifecycle, particularly around the new conditional logic.

📝 Minor Code Quality Suggestions

1. ContentView.kt:189-195 - Consider extracting the state collection logic:

// Could be more concise:
data class NodeServiceState(
    val walletExists: Boolean,
    val isRecoveryMode: Boolean,
    val notificationsGranted: Boolean
)

2. MainActivity.kt:193 - Using .value on a StateFlow in lifecycle methods:

if (!settingsViewModel.notificationsGranted.value) {

This is synchronous access to a Flow. Consider using a collected value instead to avoid potential timing issues.

3. WalletViewModel.kt:157 - The new stop() method duplicates the early return pattern:

fun stop() {
    if (!walletExists) return

This is consistent with start(), which is good for symmetry.

🎯 Recommendations Summary

Must Fix:

1. Handle potential race condition from repeated LaunchedEffect triggers
2. Fix PR title typo

Should Fix:

1. Add comprehensive tests for service lifecycle
2. Synchronize service and node state more explicitly
3. Consider re-adding error handling in ContentView lifecycle observer

Nice to Have:

1. Refactor repeated state collection patterns
2. Extract service condition logic to a single source of truth

Overall Assessment

This is a good improvement that addresses a real problem. The code is generally well-structured and improves battery life by preventing unnecessary service execution. However, the race condition issue should be addressed before merging, and test coverage should be added to prevent regressions.

Recommendation: Request changes for the race condition fix, then approve.

---

🤖 Generated with Claude Code

[claude]

PR Review: Fix LightningNodeService Startup

Summary

This PR successfully addresses issue #134 by improving the LightningNodeService startup logic to only run when appropriate conditions are met (wallet exists, notifications enabled, not in recovery mode). The implementation is well-structured and addresses the core issue effectively.

Code Quality & Best Practices ✅

Strengths:

• Clean separation of concerns with reactive state management using Compose flows
• Proper use of LaunchedEffect with appropriate dependencies in MainActivity.kt:99-107
• Good error handling with runCatching blocks
• Consistent Kotlin idioms and coding style
• Appropriate use of lifecycle observers in ContentView.kt:198-232

Minor Suggestions:

1. ContentView.kt:189-195 - Multiple state collections at the top level could be consolidated or documented for clarity:

// These states control Lightning service lifecycle
val walletUiState by walletViewModel.walletState.collectAsStateWithLifecycle()
val lightningState by walletViewModel.lightningState.collectAsStateWithLifecycle()

2. MainActivity.kt:203-210 - Consider extracting the logging context string to a companion object constant for consistency with LightningNodeService.TAG pattern.

Potential Issues ⚠️

1. Race Condition Risk (Minor)

Location: MainActivity.kt:99-107

The LaunchedEffect observes three separate flows. There's a theoretical race condition where:

• walletExists becomes true
• isRecoveryMode becomes false
• Service starts
• notificationsGranted becomes true (late)
• Service tries to start again (idempotent but inefficient)

Impact: Low - startForegroundService is idempotent, but may cause unnecessary service restarts.

Recommendation: Consider adding a local state flag or using combine() operator:

val shouldStartService by remember {
    combine(
        walletViewModel.walletState.map { it.walletExists },
        walletViewModel.isRecoveryMode,
        settingsViewModel.notificationsGranted
    ) { wallet, recovery, notif -> wallet && !recovery && notif }
}.collectAsStateWithLifecycle(initialValue = false)

2. Service Stop Logic Inconsistency (Medium)

Location: ContentView.kt:216-222 vs MainActivity.kt:191-198

The service stop logic appears in two places with different conditions:

• ContentView: Stops node when app backgrounded WITHOUT notifications
• MainActivity.onDestroy: Stops service when activity destroyed WITHOUT notifications

Issue: If notifications are granted but the activity is destroyed (e.g., system kills app), the service should continue running but node lifecycle might be inconsistent.

Recommendation: Ensure alignment between node lifecycle (managed by service) and the conditional stop logic. Consider:

// ContentView.kt - Only stop node if service won't keep it running
Lifecycle.Event.ON_STOP -> {
    if (walletExists && !isRecoveryMode && !notificationsGranted) {
        // Service not running, we manage node lifecycle
        walletViewModel.stop()
    }
}

3. Missing Early Return Check

Location: WalletViewModel.kt:156-166

The new stop() function checks walletExists but the existing start() function at line 137 already has this check. Good consistency!

Performance Considerations ⚡

Positive:

• Lazy service initialization prevents unnecessary background processes
• Proper use of background dispatchers in WalletViewModel (line 159)
• Flow-based reactive updates avoid polling

Considerations:

1. Multiple State Collections: ContentView collects 4 separate flows (lines 189-195). Consider if these could be combined into a single UI state for better performance, though current approach is acceptable for this use case.

2. Service Restart Overhead: The LaunchedEffect will trigger service start on any of the 3 dependency changes. In practice, this should be rare after initial setup.

Security Concerns 🔒

Overall: Good ✅

1. Notification Permission Handling: Proper API level check mentioned in QA notes (API 33+)
2. Recovery Mode Protection: Service correctly avoids starting in recovery mode, preventing potential issues with incomplete wallet state
3. No Sensitive Data Exposure: State flows don't expose sensitive wallet data unnecessarily

Recommendations:

• Ensure notification permission is properly requested at runtime before setting notificationsGranted to true
• Consider documenting why recovery mode prevents service startup (security/stability)

Test Coverage 📋

Concerns:

• No new test files added for the new conditional logic
• Existing test: WalletViewModelTest.kt likely needs updates for the new stop() method

Recommendations:

1. Add unit tests for WalletViewModel.stop() function
2. Add integration tests for the service startup conditions:
   • Wallet exists + notifications granted + not recovery → service starts
   • Any condition false → service doesn't start
3. Test the lifecycle scenarios in ContentView (ON_START/ON_STOP behavior)

Example test structure:

@Test
fun `stop should not execute when wallet does not exist`() {
    // Given: walletExists = false
    // When: stop() called
    // Then: lightningRepo.stop() not called
}

Additional Observations

Documentation

• The PR has excellent QA notes with comprehensive test scenarios
• Video demonstrations provided show the feature working correctly
• TODO comments removed appropriately (lines 284 in ContentView.kt)

Breaking Changes

None - This is a backward-compatible improvement.

Edge Cases to Verify

1. What happens if notification permission is revoked while app is in background?
2. Does the service handle rapid on/off transitions gracefully (activity recreation)?
3. What if user enters recovery mode while service is running?

Recommendation

Approve with minor suggestions ✅

The PR successfully implements the requested feature with good code quality. The issues identified are minor and the code is ready to merge. The suggestions above would further improve robustness but are not blockers.

Priority fixes before merge:

• None (all issues are minor)

Nice-to-have improvements:

• Add unit tests for the new stop() function
• Consider the race condition mitigation using combine()
• Document the service lifecycle behavior

Great work @jvsena42! The implementation is clean and the comprehensive QA testing gives confidence in the changes.

[ovitrif]

tAck