Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head b0207069f. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Traced every exit of SavingsProgressScreen at the PR head: OK on SUCCESS, SETTLING and INTERRUPTED, the trusted-only onUnavailable branch of runChannelClose, SavingsSwapResult.Failure, and the give-up onTransferUnavailable callback that startCoopCloseRetries keeps in viewModelScope for GIVE_UP_MS. All go through navigateOnSavingsTransferExit (ContentView.kt:945-946, 2086-2090), whose TransferRoot guard reads the nested-graph entry that NavController keeps on currentBackStack, so on-screen exits reach navigateToHome and the late callback is a no-op once the flow is gone; the toast it also fires is unchanged from master. Home is always beneath the transfer flow: the only in-app entry is Home -> Spending -> TransferRoot (HomeScreen.kt:392 uses the root controller), the dev-only screen deep links go through handleDeepLink with CLEAR_TASK which rebuilds the stack from the root start destination, and the two clearStack Navigate effects target RecoveryMode and CriticalUpdate only, so popBackStack(inclusive = false) in navigateToHome never hits the push-on-top fallback. Back and cancel paths are untouched: SavingsAvailability Cancel already used navigateToHome, system back on the progress screen still pops to SavingsConfirm as on master, and drawer navigation off the Interrupted screen leaves TransferRoot on the stack so a late callback there navigates Home where master popped the same entries to Spending. The Robolectric test builds a Home -> Spending -> TransferRoot stack with a real NavHostController and would fail on master's popBackStack (which lands on Spending). Note only: currentBackStack is @get:RestrictTo(LIBRARY_GROUP) in navigation 2.9.6; it compiles and CI runs detekt not Android lint, so no functional impact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # journeys/README.md
# Conflicts: # journeys/README.md
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 75a7bdc99. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — Late give-up callback still exits a sibling transfer flow started afterwards; LOW — 'no Spending screen title' assertion matches text that Home also shows
Checked: Round 4 (d745972 refactor + 096bf76 journey): Round 4 delta is d745972 (guard rewritten on public API) and 096bf76 (journey). The guard at ContentView.kt:2099-2102 now walks currentDestination.hierarchy for the TransferRoot graph built by navigationWithDefaultTransitions -> navigation<Routes.TransferRoot> (Transitions.kt:76-95), so hasRoute on the parent NavGraph entry is what makes it true; ContentViewTest's completion case proves that on a Home -> Spending -> TransferRoot/SavingsProgress stack and CI's testDevDebugUnitTest passed at head. Traced every caller: OK on SUCCESS/SETTLING/INTERRUPTED (SavingsProgressScreen.kt:121 -> ContentView.kt:957), the trusted-only immediate onUnavailable (SavingsProgressScreen.kt:150), SavingsSwapResult.Failure (SavingsProgressScreen.kt:106-112) and the give-up onTransferUnavailable that startCoopCloseRetries keeps in viewModelScope (TransferViewModel.kt:1699-1727, activity-scoped via MainActivity.kt:92) all fire on SavingsProgress when prompt, where the hierarchy contains TransferRoot and navigateToHome pops to Home exactly as at the reviewed sha. Late give-up after the user left: from Home, Settings, Savings, Spending or any screen pushed over the flow via the DrawerNavIcon, the current destination's hierarchy has no TransferRoot, so the exit is a no-op and the user is not moved; this fully answers greptile 4030447639 and is stricter than the old currentBackStack check for the pushed-on-top case, which the author already documented as an accepted trade in thread 4046976234. Process recreation destroys the TransferViewModel and its retry job, so no stale callback survives it, and the restored SavingsProgress entry re-wires a fresh lambda. Journey: every testTag named (ActivitySpending, ActivitySavings, TransferToSavings, SavingsIntro-button, AvailabilityContinue, GRAB, TransferSuccess, TransferSuccess-button, NavigationBack) exists at head, the intro step matches ContentView.kt:1229-1233, swap mode is off by default (SettingsStore.kt:175) so the CLOSE path and 'closes that channel' precondition hold, and DeriveBalanceStateUseCase.kt:57-59 subtracts the coop-close transfer so the Spending card reads 0 after OK; README suite and cross-platform rows are present and no bitkit-ios twin PR exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 0a2320e22. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up on 0a2320e: the earlier give-up-callback LOW is fixed. Commit 0a2320e fixes the earlier LOW: navigateOnSavingsTransferExit (ContentView.kt:2098-2102) now navigates home only when currentDestination is Routes.SavingsProgress, so a coop-close give-up/unavailable callback firing 30 min later inside a transfer-to-spending flow (same TransferRoot graph) is a no-op; the new test 'savings transfer exit leaves a later transfer to spending alone' pins this and the three earlier tests still hold. Every prompt exit still reaches Home: OK on SUCCESS/SETTLING/INTERRUPTED (Content onContinueClick, SavingsProgressScreen.kt:121), trusted-only unavailable from runChannelClose (:149-150), swap Failure (:106-112) and post-retry trusted-only give-up (TransferViewModel.kt:1727) all run while the user is on SavingsProgress; the screen has no back button and no BackHandler. Nothing legitimately on top of it changes currentDestination: sheets (Sheet.ForceTransfer, toasts) render through SheetHost (ContentView.kt:504), the PIN lock is a MainActivity overlay (MainActivity.kt:176), and Routes.AuthCheck is only pushed from Settings. hasRoute on the leaf destination works for the nested-graph data object route as in existing code. Journey change is correct: testTag TransferToSavings exists on SpendingWalletScreen.kt:191, so asserting its absence on Home is a valid check that Spending is not the visible screen.
|
@jvsena42 conflicts & ci e2e red |
|
Both are in hand. Conflicts: same one-line collision in e2e: I will report on the red shard here once the merge lands and CI re-runs on the new head, rather than reading a result from the pre-merge commit. |
…vings-home # Conflicts: # journeys/README.md
…vings-home # Conflicts: # journeys/README.md
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Audited - no findings.
QA: emu-1 redroid container on Android 15, dev build 2.4.1 (188)
-
✅ passed: Transfer Successful appeared after moving the funded Spending balance to Savings.
-
✅ passed: Cancel returned Home; separately, OK returned Home with Spending 0 and no transfer or back controls.
-
✅ passed: System back from Home opened the launcher without exposing Spending or transfer screens.
Tip
Test 1 worth a journey:
- Verify the Spending balance is positive
- Open Spending
- Tap Transfer To Savings
- Tap Continue on Funds Availability
- Swipe fully to transfer
- Verify Transfer Successful appears
Test 2 worth a journey:
- Open Spending and tap Transfer To Savings
- Tap Cancel on Funds Availability
- Verify Home appears with Savings and Spending cards
- Complete a separate transfer to the Transfer Successful screen
- Tap OK
- Verify Home appears with Spending 0 and no transfer or back control
Test 3 worth a journey:
- Start on Home after tapping OK
- Verify Savings and Spending cards are visible
- Press system back once
- Verify the launcher appears and no Bitkit transfer screen remains
Coverage:
QA: 3 of 3 manual tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)




Fixes #809
Related to #1275
This PR sends the user to Home after a transfer from Spending to Savings, instead of back to the Spending wallet screen.
Description
currentDestination?.hasRoute<Routes.SavingsProgress>(), public nav API), so the coop-close retry job cannot navigate to Home up to 30 minutes later from an unrelated screen, or from a transfer to spending the user started since. On the progress screen itself nothing changes.Companion e2e PR: synonymdev/bitkit-e2e-tests#225 removes the Android-only back/close taps after
TransferSuccess-button, which fail once this lands. Merge both together.Out of Scope
journeys/transfer: the journey is committed atjourneys/transfer/transfer-to-savings-returns-home.xml; each run closes the channel, so reopen one afterwards before any journey that needs a Spending balance.SavingsProgressViewalready resets navigation to Home.Design
Preview
Full flow recorded on the emulator (Pixel_9, dev build): Spending -> Transfer To Savings -> Funds availability -> Please confirm -> swipe -> Transfer Successful -> OK -> Home -> system back to the launcher.
809-transfer-to-savings-home.mp4
QA Notes
Manual Tests
regression:Savings Availability → Cancel: still lands on Home.A journey covering steps 1-1c was written and run by hand on the emulator, and is committed at
journeys/transfer/transfer-to-savings-returns-home.xml(suite and cross-platform rows added tojourneys/README.md).journeys/transfer/transfer-to-savings-returns-home.xml
Automated Checks
ContentViewTest.ktbuilds a Home → Spending → TransferRoot nav graph with Robolectric and asserts the savings transfer exit lands on Home with nothing left above it; it fails if the exit is reverted to popping the transfer routes. Three more cases assert a late exit call leaves the current screen alone: after navigating away from the flow, with a screen pushed on top of it, and with a transfer to spending started afterwards inside the sameTransferRootgraph.transfer.e2e.tsandactions.tsfor the new Android behaviour.just compile,just test,just lint— all green (detekt reports baseline findings only, none in the changed files).