28835ea3 - Only auto-connect wallets that are available - #1483
TaprootFreak merged 7 commits into
Conversation
|
EN: DE: DetailsReview gates at the final head (
|
| Round | Head | Findings | Outcome |
|---|---|---|---|
| 1 | 138671c2 |
1 conformity, 3 logic | Late-injection handling for the Tron wallets and Alby, wallet-app redirects, handbook reasoning |
| 2 | adc8de62 |
3 logic (second stage) | Unmount guard, Tron poll boundary, redirect classification |
| 3 | 13a58839 |
2 conformity, 4 logic | Guard completed for the success path, poll and predicate corrected, description corrected |
| 4 | 88688828 |
1 conformity, 3 logic | Alby wait, adapter readiness window, test quality |
| 5 | bacbe61b |
1 conformity, 3 logic | Availability check hardened, one declared defect, test quality |
| 6 | 1306ece6 |
0 | Coverage gap named; regression test added |
| 7 | f0eb80a9 |
0 after a description correction | Final |
Every reported point was either fixed, or verified and rejected with evidence (two were: a reality-declaration claim that contradicts this repository's own test-architecture document and 128 existing unit test files, and a state-update warning that the current React version no longer emits).
Verification at f0eb80a9
- Full unit suite: 157 suites, 2439 tests, green.
npm run lint,npm run build:dev,npm run widget:dev, Prettier: green.- Coverage, measured per file: 100 % statements, branches, functions and lines for all nine changed source files.
- Guard tests were checked against their own absence: removing a guard makes exactly the intended tests fail, so they are not vacuous.
- Mergeable against
develop, no open review threads or comments.
Open for the reviewer to decide
Three items are declared in the description and need a written decision; declaring is not granting:
- The handbook deviation — no Playwright baseline is added, because the states this change turns on depend on a browser wallet extension the harness cannot reproduce.
- A login that has already started still completes after the connect screen is closed.
- The wallet-switch branch discards the account it retrieves after switching.
Both defects are identical on develop; fixing either one reaches beyond this change into shared login and wallet-switch behaviour.
CI
The three workflow runs for this head are held at "action required", and the head therefore has no check runs. A maintainer needs to approve the runs, or apply the ci label, which also starts CI on a draft. The pull request stays a draft until that has happened and CI is green.
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.
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.
…er 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.
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.
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.
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.
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.
f0eb80a to
6b9394b
Compare
|
dfx pr guard EN: Thanks for your contribution! This repository follows A38. A38 pass: author local-CI report accepted for this head. DE: Danke für deinen Beitrag! In diesem Repository gilt A38. A38 pass: Autor-Local-CI-Report für diesen Head akzeptiert. Details
|
|
EN: DE: Details
Durations rounded up to whole seconds / Laufzeiten auf ganze Sekunden aufgerundet. Original report / Originalbericht{
"head": "6b9394b9533f8e64aef32c1e15c93cc5d2bdfa55",
"private": false,
"recorded_at": "2026-09-12T16:57:49Z",
"repo": "DFXswiss/app",
"required": [
"lint"
],
"runs": [
{
"command": "agent a38 job commands --config '{\"env\":{\"CI\":\"true\"},\"npm\":{\"canaries\":[\"react-app-rewired/package.json\",\"react-scripts/package.json\",\"typescript/lib/typescript.js\",\"prettier/package.json\"],\"node_major\":20},\"steps\":[[\"npm\",\"run\",\"lint\"],[\"npm\",\"run\",\"format:md:check\"]]}'",
"duration_s": 68.73764337506145,
"exit_code": 0,
"id": "lint",
"name": "Lint and Markdown format",
"result": "pass",
"timeout_s": 600.0
}
],
"schema": "dfx-local-ci/v1"
} |
|
EN: DE: Details{
"base": "33aae361881965299fa555618988a65dd87531b3",
"head": "6b9394b9533f8e64aef32c1e15c93cc5d2bdfa55",
"pr": 1483,
"repo": "DFXswiss/app",
"runs": [
{
"run_id": 34690781010,
"workflow": ".github/workflows/codeql.yml"
},
{
"run_id": 34690781043,
"workflow": ".github/workflows/pr-review-bot.yml"
},
{
"run_id": 34690781197,
"workflow": ".github/workflows/pr.yml"
}
]
} |
|
EN: DE: Details{
"base": "33aae361881965299fa555618988a65dd87531b3",
"head": "6b9394b9533f8e64aef32c1e15c93cc5d2bdfa55",
"phase": "applied",
"pr": 1483,
"reasons": [],
"repo": "DFXswiss/app",
"state": "ready"
} |
TaprootFreakAI
left a comment
TaprootFreakAI
left a comment
There was a problem hiding this comment.
EN:
Without a wallet in the browser, auto-connect no longer runs, so the false connect errors behind the install hint go away; real connection failures stay ERROR.
DE:
Ohne Wallet im Browser wird nicht mehr automatisch verbunden, deshalb verschwinden die falschen Connect-Fehler hinter dem Install-Hinweis; echte Verbindungsfehler bleiben ERROR.
3 participants
EN:
Wallet connect screens no longer start a connect attempt for a wallet that is not available in the browser; that attempt failed behind the install hint and was reported as a client error. Because the availability check now decides whether a connect happens, each hook detects its wallet the way its own connect path does: through the adapter's state for Phantom and Trust on Solana, through a short wait for a wallet injected after the page loaded for Alby and the two Tron wallets, and through the mobile redirects into the wallet apps wherever the adapter performs them. When the page is sent to a wallet app and the app does not take over, the screen returns to the wallet selection instead of showing and reporting an error, and an unmounted ConnectBase starts no login and reports nothing for that attempt, so a wallet approved after the user moved on can no longer log that wallet in. This PR declares a deviation from the handbook rule, explained in the details.
DE:
Die Wallet-Connect-Screens starten keinen Verbindungsversuch mehr für eine Wallet, die im Browser nicht verfügbar ist; dieser Versuch schlug hinter dem Installationshinweis fehl und wurde als Client-Error gemeldet. Weil die Verfügbarkeitsprüfung jetzt über den Connect entscheidet, erkennt jeder Hook seine Wallet so wie sein eigener Connect-Pfad: über den Adapter-Zustand bei Phantom und Trust auf Solana, über ein kurzes Warten auf eine nach dem Laden injizierte Wallet bei Alby und den beiden Tron-Wallets und über die mobilen Weiterleitungen in die Wallet-Apps, wo der Adapter sie ausführt. Wird die Seite an eine Wallet-App übergeben und die App übernimmt nicht, kehrt der Screen zur Wallet-Auswahl zurück, statt einen Fehler anzuzeigen und zu melden, und ein unmountetes ConnectBase startet keinen Login mehr und meldet für diesen Versuch nichts, sodass eine nach dem Weggehen freigegebene Wallet nicht mehr eingeloggt werden kann. Dieser PR deklariert eine Abweichung von der Handbook-Regel, begründet in den Details.
Details
Problem
ConnectBase.init()computedsupported = await isSupported(), showed the install hint when it was false, and then calledconnect()anyway wheneverautoConnectwas set:For MetaMask without an extension,
new Web3(Web3.givenProvider)has no provider,web3.eth.requestAccounts()throws web3'sProvider not set or invalid,connect()stores it as the connect error, andConnectErrorreports it throughuseReportDisplayedErroras aHandledError, although the user only sees the install hint (the content with the error is rendered hidden behind it). The same applies to every other wallet component whose availability check can come back false whileautoConnectis set, each with its own SDK's error text. Taro and WalletConnect setautoConnectunconditionally as well, but theirisSupportedis always true, so the pattern never arose there.Change
connect-base.tsxThe
autoConnectprops of the wallet components stay unconditional on purpose: gating them onactiveWallet === props.wallet(like BitBox, CLI, Ledger, Trezor) would also stop auto-connecting on a wallet switch, which is a different behavior.Wallet detection reviewed
Before this change, a wallet that
isSupported()missed could still connect, because the hiddenconnect()ran anyway — including the waits the wallet SDKs perform internally. With the gate that path is gone, so eachisSupported()has to answer "can this wallet's connect path succeed from here", not just "is some global set at this instant".isSupportedbefore() => true() => truewindow.ethereum?.isTrustWalletwindow.trustwallet.solana; false whenever another extension (MetaMask, Rabby) ownswindow.ethereum. Now: adapterreadyStateisInstalled. The adapter'sconnect()does not wait for a late wallet, so a check at mount matches it.window.phantom?.solana.isPhantomTypeErrorwhenwindow.phantomhas nosolanaprovider (the promise ininit()then rejects and the spinner never ends), ignoreswindow.solana, and is false on iOS Safari, where the adapter opens the page in the Phantom app. Now: adapterreadyStateisInstalledorLoadable.Boolean(window.webln)enable()waits up to ~100 ms (waitForWebln) for a late injectedwindow.webln, but that wait was only reachable throughconnect(). NowisSupportedis the newuseAlby().isAvailable(), which uses the same wait.window.ethereum?.isTrustWalletwindow.trustwallet.tronLink, waits for a late wallet, and on a mobile browser outside the Trust app opens the page in the app. Now:isInstalled()issupportTrust() || isTrustApp() || (isInMobileBrowser() && !isTrustApp()), andisSupportedis the newisAvailable(), which retries it.Boolean(window.tronLink)window.tron(TIP-1193) andwindow.tronWeb, waits for a late wallet, and on a mobile browser outside the app opens the page in the app. Now:isInstalled()issupportTronLink() || isInTronLinkApp() || (!supportTronLink() && isInMobileBrowser() && !isInTronLinkApp()), andisSupportedis the newisAvailable().WalletReadyStateandisInMobileBrowsercome from the adapters' base packages, which are installed as their dependencies;src/hooks/tron.hook.tsalready imports from@tronweb3/tronwallet-abstract-adapterthe same way.Waiting for a wallet that is injected late
Both Tron adapters tolerate a wallet that appears after the page loaded: their constructor starts
_checkWallet()andconnect()awaits it, polling every 100 ms. Since the gate decides once at mount, the hooks now do the same throughisAvailable(), which retriesisInstalled()every 100 ms — the delay-retry shapealby.hook.tsalready uses forwindow.webln— and checks once more after the last wait, so the closing window is not missed.The wait is deliberately capped at 2 s for both, instead of following the adapters' own
checkTimeout(2 s for Trust, 30 s for TronLink): the check runs before anything is shown, so the cap is what a user without the extension waits before seeing the install hint. Two seconds matches the Trust adapter's own default and covers a slow content script; a 30 s spinner would not be acceptable for the far more common case of a wallet that is simply not installed.The cap is therefore a deliberate trade-off, not full parity with the adapter: a TronLink wallet that only appears between 2 s and 30 s after the screen opened is shown as unavailable, although the adapter alone would still have found it. Reopening the wallet re-runs the check, and the install hint is the honest answer for everything slower than the cap.
The Trust adapter needed one more thing: it runs its own readiness poll from its constructor and caches the verdict, with a default window of 2 s — the same length as ours, but started earlier, during render rather than in the effect. Its cached "not found" could therefore be older than our own decision, and
connect()would answer from that cache: open trustwallet.com in a new tab and throw. It is now constructed withcheckTimeout: 3000, so its window always outlives ours. TronLink needs no such change; its default is 30 s.Redirects into a wallet app
On three paths the adapter does not connect but navigates the page into the wallet app: Phantom on iOS Safari (
connect()setslocation.hrefand returns without a public key), and Trust (Tron) / TronLink on a mobile browser outside their app (connect()setslocation.hrefand throwsWalletNotFoundError). The hooks surfaced these asNo public key found/The wallet is not found., whichConnectErrorshowed and reported. The hooks now wait 5 s and throwAbortError, the patternConnectAlbyalready uses after its own redirect;ConnectBasemaps it toonCancel, which only resets the wallet selection state. For the two Tron wallets the redirect decision is taken before the adapter is called, the same moment the adapter itself decides, so a failure that happens for any other reason is never reclassified as a redirect.Results arriving after unmount
ConnectBaseraninit()from auseEffectwithout cleanup, and both continuations ofconnect()ran whenever they eventually settled. Two ways that hurts: with the 5 s redirect windows above, a user who leaves the wallet in the meantime was thrown back to the wallet selection by the stale instance's.catch; and a wallet prompt approved after the user had moved on resolved the stalegetAccount, whose success chain callsdoLogin—logout(),login(),setSession(),switchBlockchain()— and could therefore log out a newer session and log the abandoned wallet in.init()also treats a failing availability check as "not available" —await isSupported()sits in atry, and thecatchleavessupportedfalse — instead of leaving the spinner up forever with an unhandled rejection. Thetryalso covers an implementation that throws synchronously, which the prop's() => boolean | Promise<boolean>type allows. No wallet hook rejects today, but the gate now depends on that call, so the failure mode is closed rather than documented.ConnectBasenow keeps a mounted ref and checks it at every point where a settled promise would act on the component or the app: it does not startdoLogin, does not callonLogin,onCanceloronSwitch, and does not touch the sign hint once it is gone. The attempt is dropped rather than reported.Two things deliberately still finish. A
doLoginthat already started runs to its end — it begins withlogout(), so stopping in the middle would leave the user logged out and nothing else. And the wallet's ownsignMessagecall is not cancelled; only the hint around it is suppressed, since the signature request lives in the wallet, not in this component.Found while touching
alby.hook.tsTwo defects in code this pull request touches, both fixed here:
useAlbymemoized its return value with an empty dependency list, soisEnabledand thesendPaymentclosure stayed at the first render'sfalse. The dependency list is now[isEnabled].waitForWebln()checked ten times and then threwTimeoutwithout looking once more after its last wait — the same off-by-one the two Tron hooks had. It matters more now, becauseisAvailable()wraps it and decides whether the install hint is shown. It now checks once more before giving up, whichenable()benefits from as well.Behavior changes
window.ethereumbelongs to another extension no longer see "Please install Trust!".Tests
src/__tests__/connect-base.test.tsx(new): auto-connect gate, fallback switch, chain selection, abort / wallet switch / error handling, the login paths, three cases where the availability check itself fails (a rejected promise, with and withoutautoConnect, and an implementation that throws synchronously), and six unmount cases: no auto-connect, no fallback switch, no login, noonCancel— each of which fails without its guard — plus the two that document what deliberately continues (the wallet's signature request).src/__tests__/connect-alby-redirect.test.tsx(extended): the remainingConnectAlbylogin branches.src/__tests__/connect-tron-wallets.test.tsx(new):ConnectTrustTrxandConnectTronLinkTrx— install hint, mobile fallback, auto-connect, reconnect, connect error and signature forwarding.src/hooks/wallets/__tests__/alby.hook.test.ts(new): detection, the availability wait, enable / sign / payment including theisEnabledfix.src/hooks/wallets/__tests__/solana-wallets.hook.test.ts(new): Phantom and Trust (Solana) detection per adapter ready state, connect including the Phantom redirect, sign, transactions.src/hooks/wallets/__tests__/tron-wallets.hook.test.ts(new): Trust (Tron) and TronLink detection for injected wallet / in-app / mobile browser / desktop / no browser, theisAvailable()retries, connect including the app redirect, sign, transactions.Coverage per touched file (
npm run test -- --coverage --collectCoverageFrom=<file>):src/components/home/connect-base.tsxsrc/components/home/wallet/connect-alby.tsxsrc/components/home/wallet/connect-tronlink-trx.tsxsrc/components/home/wallet/connect-trust-trx.tsxsrc/hooks/wallets/alby.hook.tssrc/hooks/wallets/phantom.hook.tssrc/hooks/wallets/tronlink-trx.hook.tssrc/hooks/wallets/trust-sol.hook.tssrc/hooks/wallets/trust-trx.hook.tsLocally on Node 20:
npm run lint, Prettier on all touched files,npm run build:dev,npm run widget:dev, and the full Jest suite (157 suites, 2439 tests) are green.Declared deviation: handbook coverage
Guideline: CONTRIBUTING.md › Handbook, "Every screen or flow a pull request changes has to be represented there" (a committed Playwright baseline per visual variant plus a
scripts/handbook/metadata.jsonentry).This PR changes the wallet connect flow: which of the existing states (install hint, connecting, connect error, back to the wallet selection) a user reaches depends now on the corrected wallet detection. It adds no new visual variant and does not change how any of these states look. None of these states has a baseline or a spec today; the existing login baselines (
login-process,user-flows,responsive) authenticate with a session token and never renderConnectBase.Reason for not adding baselines in this PR: every state this PR changes depends on the presence and timing of a browser wallet extension or a wallet in-app browser (injected, injected late, iOS Safari, inside the Trust or TronLink app), which the Playwright harness cannot reproduce; only the install hint without any extension would be capturable, and its appearance is unchanged. The state matrix is covered by the unit tests above instead. Whether this deviation is accepted is the reviewer's decision.
Reported, not fixed here: the account after a wallet switch
When
getAccountrejects with aWalletSwitchError,connect()switches the wallet and asks the new one for an account — but that call sits in the terminal.catch(), so its result goes nowhere: it never reachesdoLogin, and a rejection has no handler. A BitBox user who picks the Ethereum entry on a Bitcoin-only device is switched to the Bitcoin wallet, selects an address, and is simply not logged in.The code is unchanged by this pull request (identical on
develop), but this pull request adds the first test that covers the path, and that test records the current behaviour. Repairing it means restructuringconnect()so the retry re-enters the same login pipeline, which changes how every wallet switch behaves — a different change from the availability gate this pull request is about. Reported here for the reviewer to route, per CONTRIBUTING.md's rule on pre-existing defects.Reported, not fixed here: a login that has already started
Once
doLoginhas begun, it runs to completion even ifConnectBaseunmounts in the meantime:useWalletContext().loginfetches the sign message, asks the wallet to sign, callscreateSession, sets the active wallet and blockchain, and reloads the user. A user who closes the connect screen while the wallet prompt is still open and then signs anyway therefore ends up logged in with that wallet, although the screen reported nothing.This is not introduced by this pull request — before it, no continuation was guarded at all — and it is not this repository's wallet detection: stopping it properly means carrying a cancellation signal into
login()insrc/contexts/wallet.context.tsx, which every wallet shares, and deciding what a half-finished login should leave behind (login()starts afterlogout(), so an abort in the wrong place logs the user out and nothing else). Fixing that here would widen this pull request well beyond the connect gate it is about, so it is reported and left for the reviewer to route, per CONTRIBUTING.md's rule on pre-existing defects.Not part of this change
e2e-stack/specs/auth.spec.tschecks the wallet grid), and this change does not add the first one. No fakes were added or changed, so the reality declaration is unchanged.