8d2cc4b3 - Follow the account merge as a ticket when it runs long - #1216
8d2cc4b3 - Follow the account merge as a ticket when it runs long#1216TaprootFreak wants to merge 3 commits into
Conversation
The merge endpoint answers with a ticket once the work no longer finishes inside the request - measured, it needs 16.4 s at the 95th percentile today. The screen now tracks that ticket to its result instead of holding a bare spinner, and it shows the ticket number: that number is what support needs to find one specific merge instead of searching a time window. The immediate-result path is unchanged, as are the 400 and 409 cases.
Two blocking defects from the review. From the second poll on, the client asked for code=null. The screen reads otp from the URL on every render, the mount effect removes it from the URL, and that re-render produced a new fetch closure without the code — which then overwrote the good one, because the ref is reassigned on every render. The request came back 400 and was treated as final, so the user was told 'Invalid link' while the merge was running and would very likely succeed. With p95 at 16.4 s against a 900 ms threshold, that hit the majority of the cases this exists for. The code is now captured once and every poll uses it. The ten-minute cap was only evaluated before starting a fetch and when scheduling the next one, so a request that never settles kept the tracking alive forever — the hook promised a bound it did not hold, and the underlying client sets no timeout of its own. A deadline timer now runs independently of any in-flight request, and a late result is discarded once it has fired.
🤖 PR Review Bot❌ ESLint: 2 errors, 0 warningsThis is an automated review. Please address the issues above. |
The factories cannot reference outer imports because jest hoists them, so they pulled react in with require() — which the lint rule forbids. jest.requireActual is the API meant for exactly this and is not a require statement.
|
Review complete: two lenses (conformance against the repo's conventions, and logic in context), one round, nine findings. Two were merge-blocking and are fixed here. From the second poll onwards the client asked for The ten-minute cap was only evaluated before starting a fetch and when scheduling the next one, so a request that never settles kept the tracking alive indefinitely — the hook promised a bound it did not hold, and the underlying client sets no timeout of its own. A deadline timer now runs independently of any in-flight request, and a late result is discarded once it has fired. Both are covered by tests: the second poll is asserted to use the same code as the first, and a never-settling fetch is asserted to end the tracking and to have no effect when it resolves afterwards. The remaining seven are not merge-blocking and are tracked in #1224 — untranslated terminal and timeout messages, backoff steps without their own coverage, a missing generation guard for repeated One connection worth recording, which neither lens stated on its own: while the On the bot comment above: it is from the run at 23:22, before the ESLint fix. The current run started at 23:25 and is green; the bot posts only when counters are non-zero and therefore never removed the old comment. Verified in the job log at the current head, not from the comment list. CI is green on the latest push: PR CI, PR Review Bot, CodeQL and CodeQL Advanced. Tests were additionally run on a dedicated build host: 610 passing across 61 suites. |
1 participant
Why
GET auth/mail/confirm— the account merge — takes 16.4 s at the 95th percentile in production. That is far past the point where holding a request open is the right shape, so the endpoint now answers with a ticket once the work does not finish inside the request. This is the client side of that.What
hooks/job.hook.ts—useJobTrackerfollows a ticket to its result by re-calling the same endpoint, with a 1 s / 2 s / 5 s backoff and a ten-minute cap.components/job/job-progress.tsx— reusable progress view: spinner, message, and the ticket number.screens/account-merge.screen.tsx— uses the tracker instead of a single call and a bare spinner.What does not change
The immediate-result path behaves exactly as before, including
setAuthTokenand thekycHashview, and so do the 400 (Invalid link) and 409 (Merge is already completed) cases. A client talking to an API that still answers synchronously sees no difference.Design decisions worth reviewing
The ticket number is on screen. It is the key support needs to find one specific merge instead of searching a time window, and it is also what the timeout message carries.
A transient failure keeps the tracking alive; only 400 and 409 are treated as final, since those are statements about the link rather than about the network.
No new dependency and no new package version — the existing
useApi().callalready treats a 202 as a success, so the tracker rides on what is there.isOverduecomes from the server'sexpectedSeconds, not from a threshold hardcoded in the frontend, so the "taking longer than usual" hint follows the configured limit instead of drifting away from it.