fix(buy/sell-converter): drop stale in-flight responses via seq guard#597
Merged
Conversation
The debounce-cancel pattern in `BuyConverterCubit` / `SellConverterCubit` only cancels pending Timers — once a Timer has fired and its body has started the API call, neither `cancel()` nor a subsequent user keystroke stops the in-flight request. Multiple overlapping requests can resolve in any order, and the LAST resolution wins via last-write-wins on `state.sharesText` / `state.fiatText`. Reproduced in v1.0.67: user types `4600` fast enough that the timer for intermediate `460` fires before the last `0`. API call for `460` (returns 321 shares at 1.43 CHF/share) is in flight when the API call for `4600` (returns 3216 shares) goes out. If `460`'s response lands last, the converter displays "3216 → 321 REALU for 4600 CHF" — the user sees the wrong (≈10× smaller) token count and has no way to know which request won. Fix: a single monotonic `_seq` counter incremented synchronously by each user-triggered method (`onFiatChanged`, `onSharesChanged`, `onCurrencyChanged`). The async body captures `mySeq` in its closure and bails before every `emit` if `_seq` has moved on. Stale in-flight responses are silently dropped instead of overwriting newer state. `onCurrencyChanged` additionally flips `state.currency` eagerly before the await so the picker reflects the user choice even if the in-flight conversion gets superseded — the existing test `'onCurrencyChanged still flips currency even on service error'` continues to pass. Tests: 5 new race tests (3 buy, 2 sell) that complete in-flight responses out of order and pin that the seq guard drops the stale ones. Existing 28 cubit tests + broader 180 buy/sell tests stay green. Not in scope (intentional, follow-ups): - D1 stale `sharesText` on error path → separate PR if needed - D2 comma normalization in brokerbot service - Loading indicator + error surface in UI (Welle 2) - Eager controller init / focus-aware sync (separate UX PR)
TaprootFreak
changed the base branch from
develop
to
integration/realunit-registration
TaprootFreak
approved these changes
May 28, 2026
Contributor
TaprootFreak
left a comment
TaprootFreak
left a comment
There was a problem hiding this comment.
Approve. Minimaler symmetrischer Fix Buy/Sell, race-sicher dank Dart-single-threaded + synchronem ++_seq vor jedem await. Guard konsistent vor jedem emit inkl. catch. Optional/nice-to-have: Race-Tests auf fakeAsync umstellen (CONTRIBUTING Z.152) — kein Blocker.
TaprootFreak
merged commit 720b698
into
RealUnitCH:integration/realunit-registration
10 of 11 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom (v1.0.67, reproduced live)
Open Buy page → quickly delete the prefilled `300` → type `4600` →
Result-Feld zeigt 321 statt der korrekten 3216 REALU. Erneutes
Löschen + Tippen von `4600` zeigt 3216.
`floor(460 / 1.43) = 321` — der Smoking Gun: die App hat das Resultat
einer API-Anfrage für `amount=460` angezeigt, obwohl der Controller
`4600` enthält.
Root cause
`_fiatDebounce?.cancel()` storniert nur noch nicht gefeuerte
Timer. Sobald ein Timer-Body läuft, schickt er seine API-Anfrage raus
und emittet das Resultat bei Rückkehr — unabhängig davon, was der User
inzwischen tippt.
Konkrete Sequenz für das Symptom:
```
T=0 '4' → emit fiatText:'4', Timer→100ms
T=20 '6' → cancel, Timer→120ms
T=40 '0' → cancel, Timer→140ms ← fiatText='460' jetzt
T=140 Timer für '460' feuert: emit loading:true, API_460 startet ─┐
T=160 '0' → cancel (zu spät!), Timer→260ms │ in flight
T=260 Timer für '4600' feuert: API_4600 startet ────┐ │
T=380 API_4600 antwortet → shares:3216 → emit ✓ │ │
T=420 API_460 antwortet später → shares:321 → emit ← ÜBERSCHREIBT
```
Zwei in-flight Requests, last-write-wins, der ältere gewinnt
nicht-deterministisch je nach Latenz.
Fix
Monotoner Counter `_seq` im Cubit. Jede User-Methode
(`onFiatChanged`, `onSharesChanged`, `onCurrencyChanged`) inkrementiert
ihn synchron und übergibt den Wert via Closure-Capture an den
async-Body. Der Body prüft vor jedem `emit` (vor und nach dem
`await`), ob `_seq` weitergewandert ist, und droppt sich stillschweigend
falls ja.
`onCurrencyChanged` flippt `state.currency` zusätzlich vor dem
`await` — der Picker reagiert sofort, auch wenn die nachfolgende
Konversion vom Seq-Guard verworfen wird. Existing Test
`'onCurrencyChanged still flips currency even on service error'`
bleibt grün.
Tests
5 neue Race-Tests (3 buy, 2 sell), die in-flight Responses out of
order completen und pinnen dass der Seq-Guard die alten droppt — also
direkt das Symptom aus v1.0.67 pinnen.
Bestehende Tests (28 cubit, 180 buy/sell breit) bleiben grün ohne
Anpassungen.
Verifiziert lokal
Was bewusst NICHT in dieser PR ist
Diese sind reale Defekte aber nicht Ursache des reproduzierten
Symptoms. Sie verschmutzen den Race-Fix nur und werden separat
adressiert.