Release: develop -> main - #1487
Merged
Merged
Conversation
* Adopt A38 so fork CI no longer waits for a maintainer Draft pull requests run PR CI. After an A38 enforce pass, dfx pr guard approves held fork workflow runs. Ready does not start CI. * Stop Ready from starting CI and run CodeQL on drafts Ready is a no-op. Draft CodeQL and review-bot jobs run so a fork approval is not a skip. * Drop leftover Ready-dispatch wording from CI workflows Comments, input descriptions, and the e2e-stack job skip still described a Ready kick that no longer exists. * Run handbook checks on drafts and drop leftover skip comments Handbook CI followed Ready; that contradicted A38. The e2e-stack comments still named a job-level draft skip that is gone. * Align test-architecture and handbook docs with A38 draft CI Those pages still said drafts skip and Ready starts handbook checks. * Format the test-architecture table with Prettier CI format:md:check failed on the widened Runs-in-CI column. * Measure only lint locally on this public repository GitHub-hosted runners run Jest, builds, widget, and handbook. A38 locally covers npm run lint and format:md:check. --------- Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
September 12, 2026 11:17
* Move the info banner into the app content Render the CMS notice inside the padded content column below the navigation header instead of as a full-bleed strip above the app. * Add visual baselines for the in-app info banner Capture Support landing and the ticket list with a mocked CMS notice so the PR shows the banner below the header, inside the content column. * Declare the info-banner layout visual spec fake A green run of the new Playwright spec proves the banner renders below the header on mocked Support routes. It does not prove login, token verification, or that the API returns that copy. * Drop non-null assertions in the info-banner layout spec Narrow the bounding boxes with an explicit guard so the y-order check does not use !. --------- Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
Write collaborators no longer hold Ready through merge conflicts. The pin matches the current public agent stand. Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
…language or currency (#1490) * 01a09fba - Hide Delete account behind a collapsible Danger Zone Keep the red delete button collapsed until Danger Zone is opened, and state the Swiss 10-year retention duty next to the button and in the confirmation overlay. * 01a09fba - Add Danger Zone handbook screenshots Collapsed, expanded, and overlay variants for the Settings Danger Zone disclosure, generated on macOS Chromium. * 01a09fba - Declare the Danger Zone visual fake and German handbook copy Add the settings-danger-zone reality declaration and match handbook metadata to the German enrichment convention. * Show a Saved flash when language or currency changes Settings persisted language and currency with no visible confirmation. Flash the existing Cointracking Saved! overlay on the changed field for two seconds, and cover settings.screen.tsx at 100% in Jest. * Open Danger Zone before delete-account coverage cases Settings now keeps Delete account behind a collapsed disclosure. Expand it in the language/currency coverage suite and mock DfxIcon. * Refresh verification-call Settings screenshots for collapsed Danger Zone Full-page /settings baselines still showed Delete account. Capture them again so they match the collapsed Danger Zone disclosure. * 01a09fba - Refresh the settings-main golden for collapsed Danger Zone The user-flows Settings screenshot still showed Delete account. The viewport baseline now matches the collapsed disclosure. --------- Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
A ticket the AutoResponder replied to last sat under Awaiting reply with a waiting badge, although the customer already got an answer. It now sits under Answered and only returns to Awaiting reply once the customer writes again. Waiting hours and overview escalation tiles follow the same rule. Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
…eaks Web3 (#1489) * fix: keep the app rendering when the injected wallet provider breaks Web3 Some browser wallet extensions wrap window.ethereum in a Proxy that violates the Proxy invariant for the 'on' property. new Web3(givenProvider) then throws synchronously during the root render, because useMetaMask() runs inside the app-wide WalletContextProvider. - useMetaMask: fall back to a provider-less Web3 when construction throws - useWeb3.toChainHex: use the static Web3.utils.toHex instead of instantiating Web3 just to reach a utility - add a regression test for the throwing constructor * test: cover useWeb3 fully and drop its unreachable default branch Every blockchain in chainIds has a case in toChainObject, and one without a chain id returns undefined before the switch, so the default branch could never run. The new test mocks the Web3 constructor to throw, with only the real static utils available. It covers every supported chain, unsupported chains and ids, the stable interface, and that toChainHex never constructs Web3. * refactor: drop the unused catch binding in useMetaMask The rest of the file writes an ignored error as catch {}. * fix: narrow useWeb3 chain id return types and check signEip7702Authorization toChainHex and toChainId only ever return a string or undefined, so their Web3Interface signatures now say so instead of also allowing a number. The interface test in the MetaMask hook suite now also checks signEip7702Authorization. * test: arm the throwing Web3 mock before each useWeb3 test The Jest config resets mock implementations before every test, so the throwing constructor set in the jest.mock factory never reached a test body. Set it in beforeEach and assert that constructing Web3 throws. * test: assert the complete useWeb3 chain object for every chain The chain object test only compared the chain id exactly, so swapped names, currencies or URLs between chains would still have passed. Each chain case now carries its full expected chain object. --------- Co-authored-by: Daniel-DFX <320632643+Daniel-DFX@users.noreply.github.com>
* fix(connect): only auto-connect wallets that are available ConnectBase started connect() on mount whenever autoConnect was set, even when isSupported() had just returned false and the install hint was shown. For a missing wallet this produced a connect error (web3's "Provider not set or invalid" for MetaMask, for example) that rendered behind the install hint and was reported as a HandledError client error. Gate the auto-connect on the supported result. Because that makes isSupported() decide whether a connect happens, align four wallet hooks with what their adapter's connect can actually reach: - Phantom: adapter ready state Installed or Loadable (on iOS Safari connect opens the page in the Phantom app). The old check threw when window.phantom had no solana provider and ignored window.solana. - Trust (Solana): adapter ready state Installed (window.trustwallet.solana) instead of window.ethereum.isTrustWallet, which is false whenever another extension owns window.ethereum. - Trust (Tron): supportTrust(), or a mobile browser outside the Trust app, where connect deep-links into the app. - TronLink: supportTronLink(), or a mobile browser outside the TronLink app, where connect deep-links into the app. * fix(connect): wait for late wallets and handle wallet app redirects Follow-up to gating the auto-connect on isSupported(): - Trust (Tron) and TronLink count their own in-app browser as available: there the adapter's connect waits for a wallet that is injected after the page loaded. - Phantom on iOS Safari, and Trust (Tron) / TronLink on a mobile browser outside their app, open the page in the wallet app instead of connecting. The hooks surfaced this as "No public key found" / "The wallet is not found.", which was shown and reported as a client error. They now wait 5 s and throw AbortError, as ConnectAlby does after its own redirect. - ConnectAlby uses the new useAlby().isAvailable(), which waits for a late injected window.webln the same way enable() does. - useAlby memoized its result with an empty dependency list, so isEnabled and the sendPayment closure stayed false; the memo now depends on isEnabled. * fix(connect): wait for late injected Tron wallets, ignore results after unmount Follow-up to gating the auto-connect on isSupported(): - Trust (Tron) and TronLink get isAvailable(), which retries the detection every 100 ms, and the components use it as isSupported. Both adapters poll for a wallet that is injected after the page loaded, and the gate would otherwise decide once at mount and strand a wallet that is actually there. The wait is capped at 2 s instead of the adapters' own checkTimeout (2 s for Trust, 30 s for TronLink), because it runs before the install hint is shown. - ConnectBase keeps a mounted ref and ignores the continuations of init() and of connect()'s catch once it is unmounted. With the 5 s windows of the wallet app redirects, a stale instance would otherwise call onCancel and throw the user out of the wallet just selected. A completed login is still reported. * fix(connect): drop a connect attempt after unmount, close two Tron gaps Review follow-up: - ConnectBase guarded only the catch of connect(). The success chain ran on, and doLogin performs global side effects: a wallet prompt approved after the user had moved on could log out a newer session and log the abandoned wallet in. Once unmounted, ConnectBase now performs no further work for that attempt - no doLogin, no onLogin, no sign hint. - The Tron availability poll checked at 0.0 s to 1.9 s and then returned without a last look, so a wallet injected in the final window was missed although the adapter still finds it. It now checks once more after the last wait. - Trust (Tron) and TronLink decided in the catch whether the adapter had opened the wallet app, by re-evaluating a condition that can change while the request is pending. The decision is now taken before the adapter is called, so a failure with another cause is not reclassified as a redirect. * fix(connect): close the last gaps in the availability checks Review follow-up, no change to the connect gate itself: - alby.hook.ts: waitForWebln checked ten times and then gave up without looking once more after its last wait, the same off-by-one the Tron hooks had. It now checks again before throwing, which also helps enable(). isAvailable() decides the install hint, so the miss was more than a lost retry. - trust-trx.hook.ts: the Trust adapter polls for readiness from its constructor and caches the verdict, with the same 2 s window as our own poll but started earlier. Its stale "not found" could answer a connect we had just approved, which opens trustwallet.com and throws. The adapter now gets checkTimeout 3000, so its window always outlives ours; TronLink keeps its 30 s default. - connect-base tests: the unmount tests asserted DOM absence after unmount and a React state-update warning that React 18 no longer emits, so they passed with or without the guards. They now resolve the values that would trigger the guarded work and assert it does not happen; removing a guard makes them fail. The two signature tests now state plainly that the wallet's own request continues. * fix(connect): treat a failing availability check as unavailable Review follow-up: - init() awaited isSupported() without a catch, from an effect that does not handle a rejection. A hook that rejected would have left the spinner up for good and raised an unhandled rejection. It now reads as not available, which shows the install hint. No hook rejects today; the gate depends on that call, so the case is closed rather than left open. - The test that claimed to prove the wallet signature request keeps running after unmount asserted a call that had already happened before the unmount, and otherwise only that nothing is rendered after unmounting, which is true for any component. It now resolves the signature after the unmount and asserts the flow completes while onLogin stays untouched. - The wallet switch test carries a note that the dropped account is a declared, pre-existing defect described in the pull request, so the assertion is not read as intended behaviour. * test(connect): cover a synchronously throwing availability check isSupported may return a boolean, return a promise, or throw, and only the rejecting promise was covered. The missing case is the one that separates the try/catch in init() from the shorter `await isSupported().catch(...)`, which breaks for every synchronous implementation - most wallets and most tests use one. --------- Co-authored-by: Daniel-DFX <320632643+Daniel-DFX@users.noreply.github.com>
…1485) Plasma boots an Ethers wallet at Nest startup, so the shared test key and address have to be present or the stack never becomes healthy. Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
#1480) * Report an expired Taro LNURL login as "LNURL login expired" When the LNURL challenge is no longer known to the API (404 after its 5-minute TTL), the Taro login poller now rejects with a dedicated "LNURL login expired" message instead of "Authentication failed". Every other poll failure (network errors, 5xx, unexpected errors) still reports "Authentication failed". * Cover ConnectTaro fully in its component test Rename the poller test to connect-taro.test.tsx and extend it to the whole component so the touched file reaches 100 % statement, branch, function and line coverage: reconnect with a session address, the Taro link and Open app button, polling until the challenge completes, cleanup on unmount, and the connect error with its retry button. * Ignore stale Taro status polls once polling has stopped The status poller fires every second without waiting for the previous request. A request still in flight when the login had already failed could reject after the user retried and reset the new challenge, which stopped the new poller and left the retry hanging. Responses that arrive after polling stopped (settled, restarted or unmounted) are now ignored. * Poll Taro login status serially and date the expiry by challenge age The API deletes an LNURL challenge on the first completed status read, so an overlapping status request could fail with 404 and drop a login that had just succeeded. The poller now sends the next request only after the previous one has settled. A 404 also follows a wallet callback the API refused (for example a blocked IP country), which can happen long before the challenge expires. 'LNURL login expired' is therefore reported only when the challenge is at least 5 minutes old, the API's challenge lifetime; an earlier 404 stays 'Authentication failed'. The test types now use Blockchain and LnurlAuthStatus instead of plain string and object. * Describe the API's LNURL challenge cleanup accurately The API removes a challenge in a cleanup job once it is older than 5 minutes, not exactly 5 minutes after creating it. --------- Co-authored-by: Daniel-DFX <320632643+Daniel-DFX@users.noreply.github.com>
…on /link and /2fa (#1496) * Redirect merged accounts instead of showing the raw error on /link and /2fa The /link and /2fa screens caught the backend's 401 merged-account error but never called the existing handleMergedError hook, so a user whose account got merged saw the raw "User is merged" text instead of being redirected to the master account's KYC code, the same way edit-mail already handles it. Adds two hermetic Playwright specs that reproduce both screens' bug and assert the redirect. * Add reality declaration entries for the two new merged-redirect specs docs/test-architecture.md requires a declaration entry whenever a spec fakes the backend; the two new e2e specs from the previous commit were missing theirs. * Note the 501 catch-all in the new reality declaration entries Matches the detail level of the neighboring entries in the same section. * Spell out the bootstrap GET paths in the new reality declaration entries Matches the peer entries' format instead of abbreviating the path list. * Add screenshot baselines and fix a fixture-masked crash in the new specs Two fixes from the codex review round: - The mocked KycInfo/User payloads for the post-redirect /kyc screen were missing required fields (tradingLimit, kycSteps, etc.), which crashed that screen's render without failing either test — neither spec was listening for page errors. Fixed the fixtures and added a pageerror assertion. - e2e/ is the visual-regression layer per docs/test-architecture.md and CONTRIBUTING.md's Handbook coverage rule; both specs were missing their required screenshot baseline and scripts/handbook/metadata.json entry. Added one representative screenshot per spec (the destination /kyc screen after the redirect) plus the metadata entries. Also fixes a pre-existing Prettier violation in tfa.screen.tsx (an unwrapped import line) that slipped through review since it's outside the lint glob, and expands e2e coverage to a second catch site per screen (continueKyc for /link, verify2fa for /2fa). * Extend reality declarations for the second test case per spec, note tfa's no-token asymmetry The two new specs each grew a second test case (continueKyc / verify2fa coverage) whose additional mocked endpoints weren't reflected in their reality declaration entries. Also notes that the tfa spec doesn't seed an auth token, since that path is reached without a login. * Note why the two new baseline screenshots are pixel-identical Both specs redirect to the same /kyc?code=MASTER_KYC_CODE destination with the same fixture, so an identical baseline is expected rather than a copy-paste artifact. * Close an open-redirect gap, guard against stale merge-error responses, add unit coverage Three fixes from a codex review round: - merged-account.hook.ts's navigate() to /kyc merged the current page's full query string in, including kyc-redirect. Combined with /2fa's unauthenticated URL-code entry point and the existing (accepted, pre-existing) open-redirect sink on /kyc for completed accounts, this widened that sink's reachable surface. Now clears kyc-redirect explicitly on the merged-account redirect. - link.screen.tsx and tfa.screen.tsx called handleMergedError from mount-time effects with no guard against a stale response arriving after unmount, unlike the edit-mail.screen.tsx reference pattern which only calls it from user-submit handlers. Added a cancellation ref checked before acting on the mount-triggered catches. - The e2e specs asserted functional behavior (redirect) in a layer whose own architecture doc scopes it to visual regression. Added Jest unit tests (src/__tests__/link.screen.test.tsx, tfa.screen.test.tsx) following the existing edit-mail-return.test.tsx pattern to prove handleMergedError is tried first in every catch site at the appropriate layer, plus a render-stability wait in the e2e specs' second test case so a pageerror can't slip past the assertions. * Simplify e2e specs to the visual-regression layer, cover merged-account.hook.ts directly From a follow-up review round: - The e2e/ layer is scoped to visual regression per docs/test-architecture.md and cannot prove function; two of the four e2e test cases had no screenshot at all and were now redundant with the Jest tests added in the previous commit (which already cover continueKyc and verify2fa). Dropped those two cases, kept one screenshot-backed case per screen, reworded the reality declaration and test titles from "proves... redirects" to "proves... renders" to match this repo's other visual specs, and pointed the declaration at the Jest tests for the behavioral claim. - src/hooks/merged-account.hook.ts (the open-redirect fix) was only ever mocked in existing tests, so it had 0% coverage of its own — the actual fix had no direct regression protection. Added src/__tests__/merged-account.hook.test.tsx testing the real implementation. - The cancelledRef guard added in the previous commit was a single ref shared across effect re-runs: a re-run (kycCode/isSessionMode changing while a prior request is still in flight) would reset it and un-cancel the stale run's response. Replaced it with a local per-effect-run flag, matching the existing idiom already used elsewhere in this repo (compliance-kyc-step.screen.tsx), and added a regression test per screen that rejects the request after unmount and asserts handleMergedError is never called. * Add the missing re-run race regression test, fix reality-declaration attribution The unmount regression tests added earlier only covered a plain unmount-with-no-rerun, which the old shared-ref implementation also passed — they didn't exercise the actual bug the local-flag refactor fixed (a second effect run, triggered by kycCode changing while a prior request is still in flight, un-cancelling that stale run). Added a test per screen that does trigger a re-run mid-flight; verified it fails against the old shared-ref code while the existing unmount tests still pass against it. Also corrects docs/test-architecture.md, which credited both the screen test and the hook test with pinning "redirect and logout behaviour" — only merged-account.hook.test.tsx does that; the screen tests pin that handleMergedError is tried first, nothing more. * Guard the success path too, not just the catch path, against a stale re-run The cancelled guard added in an earlier commit only checked its flag in the .catch() handlers. A stale run that resolves instead of rejecting after being superseded by a re-run would still fire its .then() handler with outdated data — same race class, just on the other outcome. Extended the guard to the .then() and .finally() callbacks in both screens' mount-triggered effects, and added a regression test per screen that lets a stale run resolve after a re-run and asserts its data never reaches the UI. Verified both new tests fail against the un-guarded then-path. * Exercise the real kyc-redirect stripping in the e2e specs, not just a mock merged-account.hook.test.tsx only proved handleMergedError calls navigate() with the right clearParams argument against a fully mocked navigate — it never ran navigation.hook.ts's real merge logic. Neither e2e spec started with a kyc-redirect param either, so both would have passed identically whether the stripping worked or not. Both specs now enter with a kyc-redirect=https://evil.example param and assert it's absent from the post-redirect /kyc URL. Verified both fail when the clearParams fix is reverted. * Attribute the real clearParams proof to the e2e specs, not the hook unit test The reality declaration still credited merged-account.hook.test.tsx with pinning the actual redirect/logout/clearParams behaviour. Since the previous commit, that's no longer accurate: the hook test only proves the call arguments against a mocked useNavigation, while the e2e specs now seed a real kyc-redirect param and assert it's stripped by the actual navigation.hook.ts logic. Updated both entries to say so and to declare the new kyc-redirect param as part of what each spec seeds. * Address Codex PR-gate findings: branch coverage, assertion strength, e2e dedup, doc gap - Cover the two tfa.screen.tsx catch branches (2FA-already-set-up message, 403 statusCode) that were untested when handleMergedError returns false - Strengthen the two stale-rerun regression tests to also assert error-hint never appeared, not just that handleMergedError wasn't called - Assert navigate-then-logout call order in merged-account.hook.test.tsx - Extract the duplicated KycInfo fixture and fulfillJson helper from both merged-redirect e2e specs into e2e/helpers/merged-redirect-fixtures.ts - Note in test-architecture.md that the link spec also proves the synthetic auth token is cleared from localStorage post-redirect
…onfirm (#1495) * Treat a 409 already-confirmed response as success on buy confirm The backend returns 409 when a buy payment request was already confirmed by an earlier call whose response the client missed (dropped connection, timeout). The confirm handler treated that the same as any other error and showed the error hint instead of the completion screen, even though the payment had actually gone through. * Add a full-stack test for the already-confirmed buy replay Drives a real second confirm through the actual API after the request was already confirmed once, proving the frontend renders the completion screen on the backend's genuine 409 rather than the mocked unit-test scenario alone. * Fix a race and a missing assertion in the already-confirmed e2e test The test could capture the provisional (pre-final-price) transaction request id instead of the one the UI itself later confirms, since every quote fetch creates a new request server-side; waiting for the confirm button to enable first guarantees the final id. It also now asserts the UI's own confirm call gets a real 409, not just that the completion text appears. * Make the already-confirmed e2e test's id capture deterministic capture.get() from the shared helper had no causal link to the confirm button's enabled state, so it could in principle still return the provisional quote's id rather than the exact-price one the UI itself confirms. A dedicated response counter now waits for both quote responses and keeps the latest body, removing the dependency on that timing. * Identify the exact-price quote response by its own payload Counting successful quote responses assumed the two page.on('response') handler invocations would resolve res.json() in the same order the underlying requests were dispatched, which Playwright does not guarantee. Matching the exact-price response directly by its request body (exactPrice: true) removes that assumption entirely. * Pin the exact-price response match to the test's own amount openQuoteCapableBuy returns as soon as the spend heading is visible, before the default 300 quote it triggers is guaranteed to have settled. That default quote's own exact-price response could satisfy the confirm test's predicate instead of the amount actually under test; matching on the request's amount field as well rules that out.
…1504) The API also returns 409 when a quote is deactivated. Showing the completion screen for that status was a false success. Co-authored-by: David May (AI) <323118616+davidleomayAI@users.noreply.github.com>
davidleomay
approved these changes
Sep 17, 2026
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
4 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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist