fix(kyc): keep financial-data answers on submit failure (retryable)#620
Conversation
Why this PR shows a red ✗ (it is not this change)The functional checks for this PR pass: Analyze & Test ✅, Coverage Floor Gate ✅, BitBox quirks audit ✅. The red is the Visual Regression job, and it is a pre-existing baseline drift, not caused by this PR. It fails on exactly one golden: Evidence that it is unrelated to this change:
Resolution: the drifted baseline needs to be regenerated via the |
|
Per global repo convention (
Please drop the issue refs before merge:
The state class docstring also re-explains implementation details ( Fix itself looks correct — the |
|
Re API as Decision Authority (
Fix lands in the correct layer. |
|
@TaprootFreak done — dropped the |
Status — ready for review
Open gate: formal maintainer approval (currently |
## Summary Scopes a `diffThreshold: 0.005` only on the `home_page_loaded` golden via `AlchemistConfig.runWithConfig`. Every other golden stays at the default `0.0` (exact-match). ## Why The home_page_loaded golden renders a stroked price-chart path. Sub-pixel anti-aliasing along that path produces coverage values that jitter across runs even on the locked dfx01 renderer — hardware-lock is not enough for stroked-path AA. Evidence: - `044852b` (28.05.26, bot regen) refreshed `home_page_loaded` *and* `buy_registration_required` - `d25c61d` (01.06.26, #620) refreshed `home_page_loaded` again — only 4 days later, no UI change in scope - Pixel diff between the two `home_page_loaded` versions: **0.24% of pixels** changed, clustered in the chart-curve region only (rows 129–244, cols 252–385). Both regens were the *correct* fix in the moment (a real pixel drift had to be reconciled), but they're paying recurring cost for a structural property of the chart widget. ## Why scoped, not global A global `diffThreshold` would weaken the gate for **every** screen — including 93 static-content goldens where 0.0 is exactly right. A small icon removal (~256 px on 329 160-pixel surface) is well under 0.5% and would be silently swallowed. Local scope is the surgical fix: the chart-drift screen tolerates ~0.5%, everything else stays strict. ## Why 0.005 (not lower or higher) - Current chart drift: 0.238% of pixels → 0.005 absorbs it with ~2× headroom for next time Skia jitters slightly more. - Sanity ceiling: any meaningful UI regression in a 390×844 viewport (icon, button, label) covers >2% — well above the threshold. The gate still catches real changes. ## Alternative considered — not chosen Hardening the chart with `StrokeJoin.bevel` + `StrokeCap.butt` in the production code would remove drift at the source, but at the cost of making the chart visually less smooth for end users to satisfy a test concern. Wrong direction. ## Verification - `flutter analyze test/goldens/screens/home/home_golden_test.dart` → No issues found. - Threshold is read at goldenTest registration time via `Zone.current` (see `alchemist/src/golden_test.dart:163`), so the zone-based override correctly reaches the comparator's `variantConfig.diffThreshold` (`golden_test.dart:195`). - The wrapper `goldenTest` from `test/helper/golden_test_with_assets.dart` is preserved (alchemist import uses `show AlchemistConfig, PlatformGoldensConfig` to avoid ambiguous import on `goldenTest`).
A failed financial-data submit replaced KycFinancialDataLoadedSuccess (which holds the user's answers) with a terminal KycFinancialDataFailure rendered by an AppBar-only failure page — no retry, all answers lost. Introduce KycFinancialDataSubmitFailure (a LoadedSuccess subtype) that retains the responses so the questions page keeps rendering and the user can retry; surface the error as a transient snackbar instead of a dead-end. The load-failure path keeps its terminal failure page. Updates the prior test that asserted the dead-end behavior and adds coverage for answer-retention + a successful retry. Refs RealUnitCH#613 (K2)
TaprootFreak
force-pushed
the
joshua/fix-financial-data-submit-dataloss
branch
from
d25c61d to
2ac9aad
Compare
2 participants
Summary
A failed financial-data submit (
_submitAllcatch) replacedKycFinancialDataLoadedSuccess— which holds the user'sresponses— with a terminalKycFinancialDataFailure, rendered by an AppBar-only failure page with no retry. The collected answers were lost and the user stranded.Fix:
KycFinancialDataSubmitFailurestate that subclassesKycFinancialDataLoadedSuccess(retainsresponses/url/index). The page'sBlocBuilderkeeps rendering the questions page (it matches theLoadedSuccessarm) and the cubit'sis! KycFinancialDataLoadedSuccessguards still allow a retry._submitAllemits it instead ofKycFinancialDataFailure.Test
LoadedSuccess(not aFailure); a retry submits the same answers and reachesSubmitSuccess.Verification (local, CI-equivalent, Flutter 3.41.6)
flutter analyzeon the 4 changed files: No issues found.Fixes the K2 item of #613.