feat(account-merge): handle the 202 job response from the confirmation link - #1322
Conversation
…n link The account-merge confirmation link answers HTTP 202 with a JobDto once the merge outruns the endpoint's wait window (DFXswiss/backend#4496). `call()` resolves 202 through `response.ok` and never exposes the status code, so the screen took the ticket for a MergeResponseDto, read an undefined kycHash and left the user on the spinner indefinitely. Discriminate on the body instead: a ticket is polled via GET /job/:uid until it is terminal, then the merge endpoint is asked again for the result — the access token is issued in the HTTP context and deliberately not stored in the job. The polling budget is the job's own expectedSeconds, so it follows the API's group config rather than a second constant that would drift from it. Retry is not treated as terminal, a ticket that already carries Failed or DeadLetter is reported without polling, and polling stops when the screen unmounts. Refs #1304. Blocked on DFXswiss/backend#4496.
The effect cleanup sets isCancelled so an unmounted screen stops polling and discards its result. The flag was never cleared again, so a second invocation on the same component instance — which is what StrictMode does — would run the merge and then throw the result away, leaving the spinner up forever: exactly the failure this screen was changed to stop producing. The app does not enable StrictMode today, so this is latent rather than live. Pinned by a test that renders the screen inside StrictMode; removing the re-arm turns it red.
…result The isCancelled guard in the success handler was unverified: removing it left the whole suite green. It is not a cosmetic guard — setAuthToken writes the global auth context rather than component state, so a merge landing after the user navigated away would sign them in from a screen that no longer exists. Resolves the merge from a deferred promise after unmounting and asserts the token is never set; removing the guard now turns the test red.
CONTRIBUTING now requires 100 % statement, branch, function and line coverage for every file a pull request touches. Measured after the rebase, the screen sat at 95.55 % statements / 79.16 % branches and the helper at 92.3 % / 80 %. Covers what was missing rather than what was convenient: the poll helper's option defaults and the break that fires when cancellation lands during the wait, and on the screen the 409 mapping, the pass-through for an unmapped error, the budget-exhausted message, the account button, the catch-side cancellation guard, and a result call that answers with another job ticket. Both files now measure 100 % on all four metrics.
bc12dab to
9f5aab6
Compare
|
Four review passes (correctness + conformance) before the last came back clean. Findings per round were 1, 1, 0, 8 — the jump at the end is the rebase: Worth stating plainly, because it shapes what the numbers above mean: the review lenses were mostly unproductive here. Across the first three rounds they produced one finding (a missing Playwright baseline, which I refuted — no account-merge baseline existed and The cancellation flag was never re-armed. The effect cleanup sets The cancellation guard in the success handler was never verified. Deleting A branch in the screen was dead logic. Coverage was below the new bar. Measured after the rebase, Two contract details in the issue were wrong, and they changed the design. Read off api#4496 at Verification is mutation-checked in both directions rather than asserted. Ten mutations, each applied to the real source and restored: Deliberately not fixed, all declared in the description under CONTRIBUTING § Deviating: the handbook baseline and spec for the account-merge screen, the raw One pre-existing bug reported rather than fixed, per § Report every bug you find: the effect strips One judged non-fix: a single rejected Rebased onto |
Found by running the change against the real stack: api#4496 plus this frontend, with the endpoint's wait window shortened so every merge takes the 202 path. The API log read GET /v1/auth/mail/confirm 202 GET /v1/job/J372B31EAFABE44B5 404 and the screen sent the user to /error while the job was sitting in Postgres, Complete. The job is enqueued against the merge's master account, but whoever follows the confirmation link is still signed in as the slave. `useApi().call` attaches that session token, and the API's ownership guard — `jwt.account !== job.userData.id` — then 404s its own caller. Polling with `token: false` sends no Authorization header, which is the trust model the endpoint documents for this case: the uid is a random value known only to whoever triggered the job, the same level as the link that created it. Confirmed against the running API — the identical uid answers 404 with the session token and 200 without it. The merge call itself keeps its token; that is what the fresh access token is issued from. Both halves are pinned by a test. Re-ran the full-stack spec afterwards: 202 -> poll -> re-call -> 200, UI shows the success state, Postgres shows the merge completed.
Closes #1304. Prerequisite for DFXswiss/api#4496 — this has to merge first.
The dependency runs frontend-first, and the issue records it the wrong way round. Merged before api#4496 this change is inert: the API never sends a 202, so
isJobResponse()never matches and the 200/400/409 behaviour is unchanged, which the tests pin. Merged after it, api#4496 would have shipped the #1304 bug in the meantime — it returns aJobDtofor any merge slower than its 900 ms wait window (auth.controller.ts:228), and this repo's currentdevelopreads that as aMergeResponseDto, gets an undefinedkycHashand holds the spinner forever. api#4496's ownjob-group.config.ts:15puts the sync merge p95 at ~16.4 s — 18× that window — so this is the common case, not an edge.Problem
GET /v1/auth/mail/confirmanswers HTTP 202 with aJobDtoonce the merge outruns the endpoint's wait window.useApi().callresolves every 2xx throughresponse.okand never exposes the status code, so the screen took the ticket for aMergeResponseDto, read an undefinedkycHash, and left the user on the spinner indefinitely — the symptom recorded in the issue asauth.spec.ts:346timing out onAccount merged successfully!.Change
src/util/job.ts— the async-job contract:JobStatus,JobResponse,isJobResponse,isJobTerminal,pollJobUntilTerminal.account-merge.screen.tsx— discriminates on the body shape (uid/statusvskycHash), since the status code is unavailable. A ticket is polled onGET /job/:uiduntil terminal, then the merge endpoint is asked again for the result.screens/errorstrings, translated in de/fr/it.The re-call is required, not belt-and-braces:
ACCOUNT_MERGEis configuredexposeResult: false(job-group.config.ts), so the job never carries the result, and the access token is issued in the HTTP context by design.Contract details worth flagging
Read off api#4496 at
281bf4f, and two things differ from the issue text:waitForJobResultusespollDeadline = Date.now() + 900.maxWaitSeconds: 5is the group's queue budget, not the endpoint's. So 202 is the common path, not a rare long tail.ACCEPTEDfor "still running or endedFailed/DeadLetter".pollJobUntilTerminalreturns such a ticket unpolled.Retryis not terminal, so polling continues through it. The polling budget is the ticket's ownexpectedSeconds(65 s here) rather than a client constant that would drift from the API config.Coverage (CONTRIBUTING § Coverage)
Measured with
npm run test -- --coverage --collectCoverageFrom=…:src/util/job.tssrc/screens/account-merge.screen.tsxThe other five files in the diff are translation JSON and test files, which carry no coverage.
Reality declaration (CONTRIBUTING § Reality declaration)
No declaration entry is required. This PR introduces no fake in the declared sense — no faked provider, disabled cron, SQL-written state, schema shortcut, suppressed side effect or seed correction. It adds Jest module mocks in two unit tests, which are the unit layer's normal mechanism rather than a change to what a full-stack run does or does not prove;
e2e-stack/is untouched. Flagging the reasoning rather than staying silent, since the rule is new.Declared deviations (CONTRIBUTING § Deviating from these guidelines)
Three, each the reviewer's call:
metadata.jsonentry is added for the account-merge screen. Reasons: the diff changes no rendered markup (same components, same strings, same two states — the only behavioural difference is which state is reached and when), and the one genuinely new visual situation, the prolonged waiting state behind a 202, cannot be produced at all until api#4496 merges. Generating a baseline also needs the local full stack, which is out of scope for this change per the issue and the task framing. Happy to add a spec + baseline as a condition of merge once api#4496 is in.GET /job/:uidis fired with a rawuseApi().call. The SDK encapsulates neither this endpoint nor the siblingauth/mail/confirm(which this file already called raw before this change). The rule's remedy — add it toDFXswiss/packages, release, then consume — would mean releasing an SDK hook for an endpoint that exists in no released API. Both calls belong in the SDK together once api#4496 ships.auth.spec.tsalready claims/account-mergeand covers the fast path, so the route gate is satisfied, but no test covers the 202 waiting state. The issue scopes that out explicitly ("Zwei Punkte für die Harness getrennt davon"), and it is unreachable whilee2e-stack/env/api.envsetsDISABLED_PROCESSES=*— which also disablesJOB_ACCOUNT_MERGE, a user-endpoint execution path rather than a background job.Pre-existing bug found in touched code (CONTRIBUTING § Report every bug you find)
A page reload during the merge strands the user on
/kyc. The effect deletesotpfrom the query string immediately (account-merge.screen.tsx:40-41, unchanged by this PR), so a reload — plausible now that the wait can last tens of seconds — finds nootpand redirects to/kycinstead of resuming. The confirmation link remains valid and idempotent for 15 minutes, so the user's actual link still works; only the reloaded tab is lost. Not fixed here: keeping the otp in the URL is a deliberate hygiene choice that predates this change, and reversing it deserves its own decision. Flagging for the reviewer to rule on.Test plan
npm run test— 83 suites / 1004 tests pass, 35 of them acrossjob.test.tsandaccount-merge.screen.test.tsx.npm run lintclean (post-rebase it globs*.{ts,tsx}, so both new.tsxfiles are covered).Mutation-verified — each of these turns the suite red:
isJobResponsealways true (7) and always false (6),Retrytreated as terminal (2), theexpectedSecondsbudget zeroed (7), polling removed entirely (7), the terminal check inverted (4), thejob.errorfallback dropped (1), the post-Completere-call skipped (2), the cancellation re-arm removed (1), the.thencancellation guard deleted (1). The last two were added because the mutation initially stayed green.Verified end to end against a real stack. Ran
e2e-stackwith the API image built from api#4496 (merged onto current APIdevelop, which the harness'ssafe-log.jsguard requires) andJOB_ACCOUNT_MERGEenabled — the harness'sDISABLED_PROCESSES=*otherwise disables the very path under test. Both branches exercised:GET /v1/auth/mail/confirm 200 273.198 ms. The merge finished inside the 900 ms window,auth.spec.ts"merges accounts" passed, and thejobrow readAccountMerge | Complete. No regression.GET /v1/auth/mail/confirm 202→GET /v1/job/J6754284BF8AF42F5 200→GET /v1/auth/mail/confirm 200, the UI showed "Account merged successfully!", and Postgres confirmed the merge. That is the implemented sequence, observed rather than asserted.That run is also what surfaced the
token: falsefix below; before it, the same spec failed with the screen on/error.Rebased onto
developafter #1288/#1305/#1308 landed; the translation conflicts were resolved as a union with the keys those PRs added.