Test/lightning

[piotr-iohk]

Description

Adjustments to enable @ligntning tests.

Linked Issues/Tasks

• Test/lightning bitkit-e2e-tests#60

Screenshot / Video

Simulator.Screen.Recording.-.iPhone.17.-.2025-12-03.at.15.51.06-1.mov

[claude]

Pull Request Review: Test/Lightning

Thanks for the PR! This adds test identifiers to enable @lightning tests and improves the manual channel connection flow. Overall, the changes look good, but I have identified several areas for improvement:

✅ Positive Aspects

1. Good test infrastructure: Adding accessibility identifiers systematically across views will greatly improve UI test reliability
2. Improved UX: Pre-connecting to peer in FundManualAmountView matches Android flow and improves user experience
3. Proper error handling: The new connectPeer functions include appropriate error handling with logging and user feedback
4. Follows project patterns: Uses ServiceQueue.background(.ldk) correctly for Lightning operations

🐛 Potential Issues

1. Race Condition Risk in FundManualAmountView (High Priority)

The didAttemptPeerConnection flag prevents reconnection attempts, but if the user navigates away and back to this view, the state will reset. Consider:

• Moving this logic to WalletViewModel or tracking connected peers there
• Or checking if peer is already connected before attempting connection

Location: Bitkit/Views/Transfer/FundManualAmountView.swift:91-107

2. Missing StateLocker for Lightning Operations (High Priority)

The new connectPeer method in LightningService does not use StateLocker.lock(.lightning). According to CLAUDE.md, the app uses StateLocker to prevent concurrent Lightning operations.

Location: Bitkit/Services/LightningService.swift:219-233

The method should wrap the connection logic with StateLocker.lock(.lightning) to prevent concurrent operations.

3. Inconsistent Test ID Placement (Medium Priority)

In WalletBalanceView.swift, the test identifier was moved from the container to individual text elements. This changes the accessibility hierarchy and might break existing tests that rely on the container identifier.

Location: Bitkit/Components/WalletBalanceView.swift:21-52

4. Keyboard Type Addition Should Be Separate (Low Priority)

The port field gained .keyboardType(.numberPad) - this is a good UX improvement but is technically unrelated to test IDs and should be in a separate commit for clarity.

Location: Bitkit/Views/Transfer/FundManualSetupView.swift:82

🎨 Code Quality & Best Practices

1. Missing Error Recovery: When peer connection fails in FundManualAmountView, the user sees a toast but cannot retry without backing out and re-entering the view. Consider adding a retry button or letting users proceed anyway.

2. Typo in PR Description: The description says "Adjustments to enable @ligntning tests" - should be @lightning.

🔒 Security Considerations

No security concerns identified. The changes do not introduce new attack vectors.

⚡ Performance Considerations

• Peer connection in .task is appropriate - it will auto-cancel if view disappears
• No performance concerns identified

✅ Test Coverage

This PR adds test identifiers but does not include the actual tests. Consider adding the UI tests that use these identifiers in the same PR, or linking to the test PR/issue in the description.

📝 Recommendations

Must fix before merge:

1. Add StateLocker.lock(.lightning) to connectPeer method
2. Address the race condition in FundManualAmountView

Should fix:
3. Verify test ID hierarchy changes will not break existing tests
4. Add retry mechanism for failed peer connections

Nice to have:
5. Add the actual UI tests that consume these identifiers
6. Split keyboard type change into separate commit

📚 Testing Checklist

• Verify manual channel connection flow works end-to-end
• Test peer connection failure handling
• Verify all new test IDs are accessible in UI tests
• Test navigation away/back to FundManualAmountView (peer connection behavior)
• Verify close connection flow with new test IDs

Let me know if you would like me to clarify any of these points or if you have questions about the suggested fixes!

[Copilot]

Pull request overview

This PR adds accessibility identifiers to enable UI testing for Lightning-related flows. The changes primarily focus on adding test identifiers to various views and components, along with introducing peer connection logic to the manual channel funding flow.

Key Changes

• Added parameterized test identifiers to MoneyStack and WalletBalanceView components for flexible testing
• Introduced automatic peer connection in FundManualAmountView when the view appears
• Added new connectPeer methods to LightningService and WalletViewModel to support peer connection functionality

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
Bitkit/Components/MoneyStack.swift	Added testIdPrefix parameter to allow customization of accessibility identifiers
Bitkit/Components/WalletBalanceView.swift	Added optional amountTestIdentifier parameter and applied it to amount text views
Bitkit/Views/Wallets/SpendingWalletView.swift	Added "TotalBalance" test ID prefix to MoneyStack
Bitkit/Views/Wallets/SavingsWalletView.swift	Added "TotalBalance" test ID prefix to MoneyStack
Bitkit/Views/Wallets/HomeView.swift	Added "ActivitySavings" and "ActivitySpending" test identifiers to wallet balances
Bitkit/Views/Wallets/Sheets/ReceivedTx.swift	Replaced direct accessibilityIdentifier with testIdPrefix parameter for MoneyStack
Bitkit/Views/Wallets/Send/SendConfirmationView.swift	Added "ReviewAmount" test ID prefix to MoneyStack instances
Bitkit/Views/Transfer/FundManualSetupView.swift	Added accessibility identifiers to input fields and continue button, plus numberPad keyboard type
Bitkit/Views/Transfer/FundManualAmountView.swift	Added automatic peer connection on view appearance with error handling
Bitkit/Views/Sheets/QuickpaySheet.swift	Changed test ID from "QuickpaySheet" to "QuickpayIntro"
Bitkit/Views/Sheets/NotificationsSheet.swift	Changed test ID from "NotificationsSheet" to "BackgroundPayments"
Bitkit/Views/Settings/Advanced/LightningConnectionsView.swift	Added "NavigationAction" and "Channel" accessibility identifiers
Bitkit/Views/Settings/Advanced/LightningConnectionDetailView.swift	Added optional test identifiers to detail row helper functions and applied them to specific rows
Bitkit/Views/Settings/Advanced/CloseConnectionConfirmation.swift	Added "CloseConnectionButton" accessibility identifier
Bitkit/ViewModels/WalletViewModel.swift	Added connectPeer method that delegates to LightningService
Bitkit/Services/LightningService.swift	Added connectPeer method with proper error handling and logging

[pwltr]

tACK

This flow needs some work, but that is outside of this PR

[piotr-iohk]

tACK

This flow needs some work, but that is outside of this PR

Whoops, pushed one more commit 29fc26d, if you think it's too much, I can revert :)

[pwltr]

tACK
This flow needs some work, but that is outside of this PR

Whoops, pushed one more commit 29fc26d, if you think it's too much, I can revert :)

Yes please :) that navigation logic is too hacky and I think we can simplify by trying to connect on "Continue" press and showing error if it fails. That is good enough UX imo, the user understands that they can retry.

[piotr-iohk]

tACK
This flow needs some work, but that is outside of this PR

Whoops, pushed one more commit 29fc26d, if you think it's too much, I can revert :)

Yes please :) that navigation logic is too hacky and I think we can simplify by trying to connect on "Continue" press and showing error if it fails. That is good enough UX imo, the user understands that they can retry.

Fair enough. Reverted 👍

[pwltr]

reACK