Repository navigation
fix: handle late usdt introductions during home checks - #300
Conversation
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Makes the Home waits (balance polling, HeaderMenu taps, openSettings, doTriggerTimedSheet, the onchain_2 total read) resolve a USDT introduction that arrives late, once and outside the retried waitUntil predicate, then keep requiring the original outcome on the same deadline. Reviewed 407d4a0, full tier, static.
What I checked, and 5 candidates I ruled out
Read in full: test/helpers/actions.ts (balance helpers, tap, waitForWalletHome, waitForWalletHomeCondition, handleUsdtIntro, restoreWallet, triggerTimedSheetUnlessPresent, dismissInterveningUsdtIntro), test/helpers/navigation.ts, the onchain_2 hunk, both unit fixtures.
Call sites traced: every handleUsdtIntro caller (onboarding, restoreWallet, the timed-sheet path) keeps its old contract: no timeout means no deadline, and the decline path still returns false when nothing shows within 5s.
Build skipped: talos-pr-run could not check the author's permission on this repo and fails closed, so I did not run the unit suite or tsc/eslint myself. This repo reports no checks of its own.
Paired CI: bitkit-android run 42580213958, attempt 2, is success on app head b5ff811 across all seven local shards. I did not re-read the shard logs to confirm which E2E sha they checked out.
Ruled out
- A USDT opt-in journey gets declined by a later Home wait:
waitForWalletHomeConditiondefaults toenableUsdt: false, but no spec callscompleteOnboarding({ enableUsdt: true })today, so no current path reaches it. - Polling cost of the extra
UsdtIntroDeclineprobe:isDisplayeddoes not wait on a missing element, and the balance helpers poll every 2s. - iOS
elementByIdWithinblocking inside the onchain_2 predicate: the intro probe runs first and short-circuits, and a rejected predicate is retried bywaitUntil. - Restarted wait budget after resolving the intro: one deadline is computed up front and every inner wait takes
remaining(). - Stale
lastValuein the balance timeout message: the message is built once at call time, same as before this diff.
Merge confidence: 4/5, no findings, but I could not run the unit suite here and there is no iOS runtime evidence for the new waits.
pwltr
left a comment
There was a problem hiding this comment.
Reviewed commit 407d4a0. No findings.
Home waits now resolve a late USDT introduction once, outside the retried polling predicate, and keep requiring the original outcome on the same deadline; an intro that reappears after its decision or vanishes before it fails clearly. On Android the decision waits for the button to settle and confirms the sheet closed.
Review details
General review
Model: Claude Opus 5.5
Reasoning effort: high
talosmachina
left a comment
There was a problem hiding this comment.
Re-review of fba9a6e after 407d4a0. 0 earlier findings, 0 new. The only change since my last review is the merge of main at a62926c (Move to Spark recovery); 3135e8f and 407d4a0 are unchanged (git range-diff). Full tier, static.
What I checked, and 4 candidates I ruled out
Merge resolutions read: tap (test/helpers/actions.ts:394), openSettings and doTriggerTimedSheet (test/helpers/navigation.ts), the unit fixture. Each keeps main's withMoveToSparkIntroRecovery and puts the PR's waitForWalletHomeCondition inside it, so both sides' behaviour survives. doTriggerTimedSheet keeps its 45s timeout. openSettings keeps main's default timeout.
Auto-merged side: main replaced dismissInterveningUsdtIntro with dismissInterveningSheets. The PR never changed that function, and the new one keeps the same USDT branch (handleUsdtIntro() with no deadline, then doTriggerTimedSheet), so the timed-sheet path still behaves as before.
New tests: the Spark-then-USDT case for menu tap, Settings and the timed-sheet trigger, and the ineffective Spark dismissal, on both platform models. The fixture hides the USDT intro and Home while Spark is up, which is how the recovery's isDisplayed probe sees the stack.
Build skipped: talos-pr-run still cannot check the author's permission on this repo and fails closed, so I did not run the unit suite, tsc or eslint. The repo reports no checks of its own.
Paired CI: bitkit-android run 42580213958, attempt 3, the one on the merged source, is still in progress (7 local shards running).
Ruled out
- Spark recovery gets a shorter budget because the USDT wait already spent it: the retry calls
waitForWalletHomeConditionagain, which computes a new deadline. That matches main, where the retry also restartswaitForDisplayedwith the full timeout. - USDT "reappeared" error masked by the Spark retry:
withMoveToSparkIntroRecoveryrethrows unless the Spark intro is actually showing, so a USDT reappearance with no Spark sheet still fails. - A Spark sheet stalls a USDT decision mid-wait: while Spark covers it,
UsdtIntroDeclineis not displayed, so no choice is attempted until Spark is dismissed. - Double recovery in
tap('HeaderMenu'): both waits are wrapped, but each wraps its own wait and dismisses only a sheet that is actually showing, so a single Spark intro produces oneLatertap (the new menu-tap test asserts exactly that).
Merge confidence: 4/5, no findings, but I could not run the unit suite here and the paired Android run on the merged source has not finished yet.
|
Conflict resolution is pushed at Paired Android workflow attempt 3 finished red across all seven shards at that exact E2E head. Eleven Settings cases passed; failing setup hooks stopped much of the 39-case cohort. I inspected all seven failed-shard artifacts, all 13 failure screenshots, and sampled frames from all 13 recordings, plus matching app/device/job logs. Three recordings have no decoded 75% sample; other sampled frames and screenshots were inspected. Hook-only failures have no per-case media. The receipt failures show uncovered Home with zero balance. Boost stays on its submission spinner, backup uploads stay running/failed, Settings connection updates stall, and most subsequent onboarding failures stay at 100% setup. Later reinstalled app processes stop during Merging remains blocked pending a successful current-head runtime result and refreshed human approval. The earlier 39/39 first-attempt result belongs to |
|
Current conflict-resolution head The existing 78 repository tests plus 4 boundary probes, typecheck/lint, independent full reassessment and Talos re-review apply to this exact unchanged source. Historical failed attempts remain in the description with their actual evidence. Fresh human review has been requested from pwltr after the conflict-resolution push; the existing ben-kaufman request is retained. Once current-head approval and live merge policy clear, this companion can land before synonymdev/bitkit-android#1808. |
talosmachina
left a comment
There was a problem hiding this comment.
Re-review of 7f61681 after fba9a6e. 0 earlier findings, 0 new. The only change since my last review is the merge of main at 0e83ea6; the PR's own diff against its base is byte-identical to the one at fba9a6e (only the blob index line differs). Full tier, static.
What I checked
Merge content: main brought #301 (triggerTimedSheetUnlessPresent now waits up to 3s for sheetId or MoveToSparkIntroLater before dismissMoveToSparkIntroIfShown()), the Blocktank test removal, the 3.1.0 migration baselines and the rustls bump. None of it overlaps a hunk of this PR, and the merge has no hand-resolved conflicts (git show --cc 7f61681 is empty).
Interaction with #301: the PR's doTriggerTimedSheet (USDT-aware waitForWalletHomeCondition inside withMoveToSparkIntroRecovery, 45s) runs before #301's new wait, so a late USDT intro is still resolved before the trigger, and #301's post-trigger wait is untouched.
Removed helpers: transferSpendingToSavings, tryDismissBackgroundPaymentsIfVisible, transferHardwareWalletToSpending, waitForHardwareTransferProgress, dismissSpendingBalanceToastIfShown have no remaining references at 7f61681, including in the PR's unit tests (unchanged since fba9a6e).
Build skipped: talos-pr-run still cannot check the author's permission on this repo and fails closed, so I did not run the unit suite, tsc or eslint. The repo reports no checks of its own.
Paired CI: bitkit-android run 42580213958, attempt 6, is in progress (build-local running).
Merge confidence: 4/5, no findings, but I could not run the unit suite here and the paired Android run on the merged source has not finished.
|
Current integration head Current-head local validation is 78 repository tests plus 4 boundary probes, typecheck, scoped lint and diff checks. Independent implementation reassessment and Talos review are clear. Main |
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 7f61681. The static General review found no supported introduced defects. Selected unit validation passed: 55 tests, 0 failures, 0 skipped.
The selected files were late-usdt-intro.test.ts (26 passed), wallet-home.test.ts (24 passed) and profile-copy.test.ts (5 passed), using pinned tsx 4.20.3 on Node 26.13.0. Source, package.json and the lockfile remained unchanged. Lifecycle scripts were disabled during locked dependency setup.
Coverage is limited to the General assessment and these mocked unit tests. No real-device, simulator, visual, backend or full-E2E checks ran. Security review was explicitly skipped and remains unreviewed. Formatting, lint and builds were outside the selected validation scope.
Review details
General review
Model: GPT-6.1 Sol
Reasoning effort: high
Validation
Model: GPT-6.1 Sol
Reasoning effort: high
Companion: synonymdev/bitkit-android#1808
An optional USDT introduction can appear after onboarding has returned or after a receipt returns the app to Home. The default Android suite then waits for balance, menu or backup controls covered by that sheet. This change resolves the introduction during the relevant Home wait, then continues requiring the original outcome.
Changes
mainat0e83ea6and preserve its Move-to-Spark recovery around the complete Home navigation waits. Cover Spark followed by USDT and ineffective Spark dismissal on both platform models.Validation
Current paired Android CI passes: workflow 42580213958, attempt 6, exact E2E
7f61681bb8d4b0d31dc836a1c03a8488170baf66paired with unchanged appb5ff8114be9a1392adc2caef371d5b367868c7dd. All seven checkouts are verified: 38/39 first-attempt cases, 39/39 eventual passes, one built-in retry and 13 retry steps skipped; no legacy toast fallback ran. The fresh app build checked out merge2353ee4c30a71c7c09c709cc3403f3231080f94b, whose tree matchesb5ff8114b.The retried
multi_address_3case timed out waiting forReceivedTransaction. Its failure screenshot and sampled recording show uncovered Home, and the device log records an incremental Electrum on-chain sync timeout during that wait. The same case passed on its built-in retry with unchanged sources. The artifact-backed disposition is flake, with the underlying sync-timeout cause unisolated; this is not proof of staging VSS causality. The exported app log starts after the next reinstall. Three recording samples and the failure screenshot were inspected; a fourth requested frame did not decode.Current main integration head
7f61681includes main0e83ea6and its timed-sheet polling changes. 78 repository unit tests plus 4 boundary probes, typecheck, scoped lint and diff checks pass. Four independent implementation reviews have no remaining findings; the current-head Talos review is clear. Current-head human approvals: ben-kaufman and pwltr; GitHub reports APPROVED/CLEAN.Historical
fba9a6epaired Android CI pass: workflow 42580213958, attempt 5 completed 39/39 cases on their first attempt at exact E2Efba9a6efde8fdd9066a0408e3f5f1fe1b5ec23ac; every shard checkout is confirmed, all 14 retry steps skipped, and no legacy toast fallback ran. It reused the successful app build of merge2353ee4c30a71c7c09c709cc3403f3231080f94b(same source tree as app headb5ff8114b).Node 22.23.4: 78/78 repository unit tests plus 4/4 independent boundary probes pass at current main-integration head
7f61681bb8d4b0d31dc836a1c03a8488170baf66.TypeScript
--noEmit, ESLint for all changed files andgit diff --checkpass.Four independent implementation review rounds completed with no remaining findings, including a full reassessment against the new base and 59 independently executed focused/boundary checks. All four authored commits, including both merges from main, are signed locally and GitHub Verified.
Four focused regressions failed against the old fixture before the correction. An initial broad run at
3135e8ffound five Profile Copy mocks missing the newly used visibility method; the one-line mock correction at407d4a0resolves them without changing their original assertions.Historical pre-conflict paired Android CI passed: 39/39 cases on their first case attempt, across all seven shards. Workflow 42580213958, attempt 2, workflow attempt 2; all 14 retry steps skipped. Every shard checked out exact E2E
407d4a05fdcf6796960df0afbda552f575c93e97.Historical post-conflict validation: workflow attempt 3 finished failure in all seven shards. All seven logs confirm E2E
fba9a6e; the app build checked out the original2353ee4cmerge. Eleven Settings cases passed, but setup hooks aborted much of the 39-case cohort; failure counts include hooks, so this is not a completed 39-case assertion run. This failed attempt remains separate from the later successful unchanged-source attempt 5. The previous pass is retained at its actual407d4a0source.CI evidence and setup
The failed Android workflow attempt 1 used app
b5ff8114be9a1392adc2caef371d5b367868c7ddand E2Emainat849806e70784da0960f44da6206e2346b1e0ba3e: 27/39 first-attempt and 35/39 eventual passes. All 22 failure screenshots, sampled frames from all 22 recordings, and corresponding logs show the undismissed USDT intro. Diagnosis.Use the standard Android E2E workflow at that app head, Node 22, Pixel 6/API33 and the local Docker backend. This E2E branch matches the app branch, so normal branch selection picks it up. Attempt 2 executed that same seven-shard cohort with the existing 30s UI waits. Standard PR checkout built merge ref
2353ee4c30a71c7c09c709cc3403f3231080f94b; its source tree813c7ff6e9aac702bd0be30422812c0dfb528936exactly matches app headb5ff8114b. The passing LNURL/Transfer device log records two USDT-intro presentations followed by one decline click for each new wallet, establishing that the runtime exercised the correction.Settled investigation: all seven failed-shard artifacts, all 13 failure screenshots, and sampled frames from all 13 recordings were inspected with app/device logs. Hook failures often have no per-case media. Three onboarding recordings lack a decoded 75% sample; their other sampled frames and failure screenshots were inspected. Three failed-case screenshots show uncovered Home with zero balance; Settings failures show Electrum/RGS connection screens. Subsequent reinstall processes stall in native VSS-backed node construction and time out waiting for the node to run. Read-only workstation GET probes to staging
/vss_rs_auth/and/vss_rs_auth/get_objectalso time out while/lnurl_auth/authresponds. This is consistent with external dependency trouble; it does not establish the exact cause of every initial receipt failure or prove a global service outage. No introduced fixture defect has been demonstrated. The triage remains insufficient evidence for one established whole-run cause, rather than labeling the different-tree historical pass as a flake. The suite subsequently passed in attempt 5 at unchangedfba9a6e; current7f61681execution is recorded separately above. Fresh approvals at7f61681are recorded above.Limits and scope
Android master has additionally advanced to
36a2c4d, adding USDT first-sync receipt celebration/restore-window handling and loaded-account on-chain address search. These do not alter this PR's Lightning capacity refresh or #300's Home-introduction controls. Existing CI remains pinned to the actual app merge2353ee4c/ headb5ff8114b; the combination with the newer master changes has not been rerun.Previously assessed Android master
3f2009e: its delta from the originally tested base consists of release 3.1.0 version/changelog bookkeeping and #1817 instrumentation-test/local-regtest workflow changes. No production app source, dependencies or standard local E2E workflow changed. Those new instrumentation fixtures are outside this Appium cohort. The rerun remains evidence for the original app merge snapshot and unchangedb5ff811behavior, not execution of current-master version metadata or migration behavior.This is a standalone change from
main; experimental E2E #298 remains stacked on #296. Its historical iOS cohort and separate RBF, Receive-transition and staging-funding investigations remain outside this fix. No new iOS runtime execution is claimed. One successful Android cohort does not establish a durable CI reliability rate.The main-based Send2 case retains its historical extra-key toast fallback; attempt 6 logged zero uses of that fallback. Its pass must be distinguished from the separate pinned Send2 run, which preserves the stronger one-key warning and retained-field assertion and passed 90/90 outcomes against app
b5ff8114b/ E2Ee560ece. This PR adds no such fallback and preserves existing financial, fee, invoice and history assertions.Models used
gpt-6.1-sol(reasoning:high)gpt-6.1-sol(reasoning:high)gpt-6.1-sol(reasoning:high)