Conversation
ee3b578 to
9de80cf
Compare
|
9de80cf to
165c525
Compare
|
@pwltr conflicts. |
165c525 to
b418053
Compare
b418053 to
f04ec51
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Two low-severity observations inline, nothing blocking.
Checked and clean:
- Concurrent pulls:
beginRefreshing()is main-actor and synchronous,endRefreshingruns indefer, and the fade-out completion checksisRefreshingso a late animation cannot stop a newer spinner. CurrencyViewModel.refresh()setsisRefreshingbefore the first await, so a pull cannot race the polling timer into publishing an older response.- Node stopped or wiped mid-pull:
syncthrows and toasts, no hang. - Pull trigger only fires on page 0 with at least 80pt overscroll; the pan target detaches and reattaches with the window.
- Journey log paths and UTC timestamps match the existing hardware-wallet journeys;
HomeScrollViewmatches the Android testTag.
Follow-up outside this diff: the Android journey at https://github.com/synonymdev/bitkit-android/blob/master/journeys/home/pull-to-refresh-rates.xml still says "iOS does not refresh rates on pull". It needs updating once this merges.
There was a problem hiding this comment.
QA reviewed on f04ec51.
QA review
Reviewed the full PR diff against its merge base, at f04ec51.
No new actionable code findings.
Unresolved: Home pull keeps the indicator for the full rates retry budget. Still present at this revision. A Home pull that starts a rates fetch leaves the custom indicator and the 60-point wallet spacing up until currency.refresh() returns, and beginRefreshing() ignores later pulls for that whole time. fetchLatestRates tries three times through URLSession.shared, with 1 second and then 2 seconds between failures. The shared session's request timeout is 60 seconds by URLSessionConfiguration default, so a host that never answers would hold this indicator for about three minutes. A rates failure surfaces as a toast only through the existing stale-data path, after 10 minutes without a successful refresh. Source analysis at f04ec51; the hung-host wait was not executed in this review. Home should release the indicator and accept another pull without waiting out that retry budget. The rates request can continue in the background.
Additional test cases
- iOS simulator, onboarded wallet, Home wallet page (
HomeScrollView), at least 20 seconds after the lastCurrency rates refreshed successfullylog: pull down to refresh and, within about a second, take a screenshot. A spinner should be visible under the header, with the wallet content shifted down. The indicator is hidden fromsnapshot-ui. After that success line, the spinner and the extra top space should be gone. This is the visual check for #519. The new journey only asserts the log line. Discussion. - Same screen, with the rates host not answering before the request timeout: one pull. The indicator should clear and a second pull should be accepted while the first rates attempt is still retrying.
Rates refresh on Home pull matches the in-flight dedupe and success log from bitkit-android#1281. Widget refresh is outside the scope stated in this PR. The Android journey still says iOS does not refresh rates on pull; that file is not in this diff.
Device testing: not performed in this review. Unit, integration, and e2e checks on this revision succeeded. This review inspected those results and did not re-run them. Those checks do not drive this pull gesture.
129e58c to
341c8ff
Compare
piotr-iohk
left a comment
There was a problem hiding this comment.
QA reviewed on 341c8ff.
QA review
1 actionable finding — resolve or provide an evidence-backed rebuttal.
QA review
Re-reviewed changes since f04ec51, including affected refresh paths and prior findings, at 341c8ff. This is a follow-up to the previous QA review. Base c906d5a and merge-base 018d904 are unchanged. This pass read the new feedback timeout, its unit tests, and the journey screenshot steps, and inventoried the rest of the PR diff (Home gesture and indicator, rates success log, changelog).
1 actionable finding
- LOW: Cleared-spinner journey check runs when the rates log appears, before wallet sync finishes
Home pull keeps the indicator for the full rates retry budget does not match this revision: HomePullRefreshFeedback.wait returns after 10 seconds and leaves currency.refresh() running. The spinner journey check now has screenshot steps; the cleared-spinner step is the finding above.
Home pull still dedupes an in-flight rates fetch the same way as bitkit-android#1281, and still logs Currency rates refreshed successfully. Widget refresh is outside the scope stated in this PR. The Android journey still says iOS does not refresh rates on pull.
Additional test cases
- iOS simulator, onboarded wallet, Home wallet page (
HomeScrollView), rates host not answering: pull once. About 10 seconds later the spinner and the extra space above the wallet should be gone, with no success line yet. Pull again. The indicator should show and wallet sync should run, without a second rates request while the firstcurrency.refresh()is still in flight, and without a "Rates currently unavailable" toast from that attempt. - Same wallet with Widgets turned off, so Home is a single page: pull down. A spinner should appear under the header and the log should gain a
Currency rates refreshed successfullyline. This checks that removing.refreshablestill leaves a pull gesture when the page does not scroll. - Same wallet, normal network, node running, with wallet sync slower than the rates request: when the success line appears, the spinner and extra top space should still be visible, and both should clear only after wallet sync and activity sync finish.
Device testing: not performed in this review. Run Tests and Run Integration Tests succeeded on this revision. This review inspected those conclusions and did not re-run the tests. Those jobs do not drive this pull gesture. e2e-tests-local was still queued and does not cover this journey. No new Appium spec: this pull belongs in the Home journey. Short-content bounce with widgets off was not executed here.
Findings
- [LOW] Cleared-spinner journey check runs when the rates log appears — inline at
journeys/home/pull-to-refresh-rates.xml:19.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA reviewed on bea4740.
QA review
No new actionable code findings.
QA review
Re-reviewed changes since 341c8ff, including the Home refresh path and prior findings, at bea47403. This is a follow-up to the previous QA review. Base c906d5a and merge-base 018d904 are unchanged. The new commit only adds the journey wait step. This pass re-read refresh(), the indicator, the rates fetch, and journeys/home/pull-to-refresh-rates.xml, and inventoried the rest of the PR diff.
No new actionable code findings in the follow-up.
Cleared-spinner journey check runs when the rates log appears does not match this revision. The journey now waits for wallet and activity synchronization and the spinner fade-out before the cleared-state screenshot. refresh() still hides the indicator only after wallet.sync(), activity.syncLdkNodePayments(), and HomePullRefreshFeedback.wait finish. That wait ends when currency.refresh() returns, or after 10 seconds, and the rates request keeps running.
Home pull keeps the indicator for the full rates retry budget stays fixed by that same 10-second feedback wait.
Home pull still dedupes an in-flight rates fetch the same way as bitkit-android#1281, and still logs Currency rates refreshed successfully. Widget refresh is outside the scope stated in this PR. The Android journey still says iOS does not refresh rates on pull.
Additional test cases
- iOS simulator, onboarded wallet on the Home wallet page (
HomeScrollView), withbitkit.stag0.blocktank.tonot answering: pull once. About 10 seconds later the spinner and the extra space above the wallet should be gone, with noCurrency rates refreshed successfullyline yet. Pull again. The indicator should show, the log should not gain a secondRefreshing ratesline while the first fetch is still retrying, and no "Rates currently unavailable" toast should appear from that attempt. - Same wallet, Settings → General → Widgets (
WidgetsSettings), turn off Show widgets (ShowWidgets), then return to Home so the pager is a single page: pull down. A spinner should appear under the header and the log should gain aCurrency rates refreshed successfullyline. This checks that removing.refreshablestill leaves a pull when the page does not scroll.
Device testing: not performed in this review. Run Tests succeeded on this revision. This review inspected that conclusion and did not re-run the tests. Those tests cover HomePullRefreshFeedback.wait only; they do not drive the pull gesture. Run Integration Tests was still in progress. e2e-tests-local was still pending and does not cover this journey. No new Appium spec: this pull belongs in the Home journey.
Ready for device testing.
|
Device result: passed (iOS only, iPhone 17 /
Notes: a single screenshot right after swipe often missed the spinner (refresh finished in ~2s); burst capture was needed. Hung-host / 10s-cap path not induced. Android not tested. Earlier code-review threads on an older head are unchanged by this run — this is not a review clear of those threads by itself. |
piotr-iohk
left a comment
There was a problem hiding this comment.
Approving after iOS device pass on bea4740 (Home pull-to-refresh rates + spinner journey and second-pull exploration). Android not tested in this session.
jvsena42
left a comment
There was a problem hiding this comment.
341c8ff and bea4740: both of my threads are fixed. HomePullRefreshFeedback.wait caps the feedback at 10 s while the deduplicated currency request keeps running, and the timeout task is cancelled on normal completion. The journey now checks the spinner by screenshot and waits for wallet/activity sync before the cleared-state check. No new findings.
Fixes #519
Closes #344
Description
Out of Scope
Design
N/A — no design available.
Preview
Empty:
Simulator.Screen.Recording.-.iPhone.17.-.2026-09-24.at.21.55.15.mov
Populated:
Simulator.Screen.Recording.-.iPhone.17.-.2026-09-24.at.21.54.43.mov
QA Notes
Journeys
pull-to-refresh-rates.xml— Home pull visibly refreshes exchange rates, records a successful update, and verifies the indicator appears and clearsManual Tests
N/A
Automated Checks
HomePullRefreshFeedbackTests— 2 passed; normal completion and capped feedback with a continuing background refresh