feat: add pull to refresh to retry start node

[ben-kaufman]

This PR adds pul to refresh on app status and LN node status pages, which will try to start the node in case it was stopped or failed with an error

[claude]

Code Review for PR #253: Pull to Refresh to Retry Node Start

Summary

This PR adds pull-to-refresh functionality to both NodeStateView and AppStatusView, allowing users to retry starting the Lightning node when it's in an error or stopped state. Overall, the implementation is good and follows the project's SwiftUI patterns, but there are a few concerns around concurrency, code duplication, and potential race conditions.

---

🟢 What's Good

1. Proper SwiftUI Patterns: Uses .refreshable modifier correctly, which is the modern SwiftUI approach
2. User Experience: Addresses a real UX pain point - users can now manually retry failed node starts
3. Error Handling: Properly catches errors and displays them via toast notifications
4. MainActor Usage: Correctly uses await MainActor.run for toast calls
5. Follows Architecture: Keeps business logic in ViewModels and uses proper @EnvironmentObject injection

---

🟡 Concerns & Recommendations

1. Missing StateLocker Protection ⚠️

Location: NodeStateView.swift:62-77, AppStatusView.swift:48-64

According to the project's CLAUDE.md documentation:

All Lightning operations go through LightningService.shared
Lock the Lightning state with StateLocker.lock(.lightning) for critical operations

The wallet.start() calls should be protected with StateLocker to prevent concurrent Lightning operations, especially since:

• The app uses StateLocker to prevent concurrent Lightning operations (see LightningService.swift:27)
• Multiple users could trigger pull-to-refresh simultaneously
• Background processes might be accessing the node

Recommendation:

private func refreshNodeState() async {
    wallet.syncState()
    
    if case .errorStarting = wallet.nodeLifecycleState {
        do {
            try StateLocker.lock(.lightning, wait: 5)
            defer { try? StateLocker.unlock(.lightning) }
            try await wallet.start()
        } catch {
            await MainActor.run {
                app.toast(error)
            }
        }
    } else if wallet.nodeLifecycleState == .stopped {
        // Same pattern here
    }
}

2. Code Duplication 📋

Location: Both files have nearly identical refreshNodeState/refreshAppStatus logic

The error-handling logic for retrying node start is duplicated in both views. Consider extracting this to a shared method in WalletViewModel:

// In WalletViewModel
func retryStartIfNeeded() async throws {
    syncState()
    
    guard case .errorStarting = nodeLifecycleState || nodeLifecycleState == .stopped else {
        return
    }
    
    try StateLocker.lock(.lightning, wait: 5)
    defer { try? StateLocker.unlock(.lightning) }
    try await start()
}

This would simplify both views and ensure consistent behavior.

3. Potential Race Condition 🏃

Location: NodeStateView.swift:61-77, AppStatusView.swift:48-64

The pattern of checking state then calling start() creates a time-of-check to time-of-use (TOCTOU) issue:

// State could change between check and action
if case .errorStarting = wallet.nodeLifecycleState {
    try await wallet.start()  // What if state changed?
}

While wallet.start() does set state to .starting at line 105, there's no guard against already-starting nodes. If a user rapidly pulls to refresh, or if background sync triggers start, you could have concurrent start attempts.

Recommendation:
Add a state check at the beginning of WalletViewModel.start():

func start(walletIndex: Int = 0) async throws {
    guard nodeLifecycleState != .starting && nodeLifecycleState != .running else {
        Logger.warn("Node already starting or running")
        return
    }
    // ... existing code
}

4. Missing Sync When Node is Already Running

Location: NodeStateView.swift:57-78

In AppStatusView, when the node is running, it calls wallet.sync() (lines 38-46). However, NodeStateView.refreshNodeState() doesn't have this logic. This creates an inconsistency - refreshing on the app status page syncs a running node, but refreshing on the node detail page doesn't.

Recommendation: Add the same sync logic to NodeStateView:

private func refreshNodeState() async {
    wallet.syncState()
    
    // If node is running, just sync
    if wallet.nodeLifecycleState == .running {
        do {
            try await wallet.sync()
        } catch {
            await MainActor.run {
                app.toast(error)
            }
        }
        return
    }
    
    // ... rest of retry logic
}

5. Minor: Layout Change in AppStatusView

Location: AppStatusView.swift:10-25

Wrapping the VStack in a ScrollView changes the layout behavior - the original design had Spacer() at the bottom (line 28), which suggests the status rows should stick to the top. Now they'll be vertically centered when there's extra space. This might be intentional for pull-to-refresh, but worth confirming it matches the design intent.

---

🔴 Potential Bugs

Missing State Transition Handling

Location: Both refresh functions

The code doesn't handle the .starting or .stopping states. If a user pulls to refresh while the node is already starting (from another trigger), nothing happens. Consider adding user feedback:

if wallet.nodeLifecycleState == .starting {
    await MainActor.run {
        app.toast(type: .info, title: "Node Starting", description: "Please wait...")
    }
    return
}

---

📝 Testing Recommendations

1. Test concurrent refresh: Pull to refresh on both screens simultaneously
2. Test during state transitions: Pull to refresh while node is already starting
3. Test with slow network: Ensure proper loading indicators and timeout behavior
4. Test error states: Verify different error types are handled correctly
5. Test StateLocker conflicts: Verify behavior when extension or background process holds the lock

---

🎯 Summary of Action Items

High Priority:

1. Add StateLocker protection to wallet.start() calls to prevent concurrent operations
2. Add guard in WalletViewModel.start() to prevent concurrent start attempts

Medium Priority:
3. Extract common retry logic to WalletViewModel to reduce duplication
4. Add sync functionality to NodeStateView.refreshNodeState() for consistency
5. Handle .starting and .stopping states with user feedback

Low Priority:
6. Verify layout behavior change in AppStatusView matches design intent

---

✅ Conclusion

This is a valuable feature that improves the user experience when the node fails to start. The core implementation is solid and follows SwiftUI best practices. The main concerns are around concurrency safety and code consistency. With the addition of StateLocker protection and some refactoring to reduce duplication, this will be production-ready.

Great work! The pull-to-refresh UX is exactly what users need when dealing with node errors.

[ovitrif]

tAck

tested:

• pull-to-refresh on node info 🟢
  • when node in error state
  • when node running
• pull-to-refresh on apps status - when node running 🟢