Skip to content

Fix: MWA Select but No Connect Bug #1049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Fix: MWA Select but No Connect Bug #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6181e1d
init fix + fix tests
Funkatronics Feb 6, 2025
6fc480c
fix mwa disconnect + tests
Funkatronics Feb 6, 2025
ba7b3c4
add changeset
Funkatronics Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silly-bananas-repeat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@solana/wallet-adapter-react': patch
---

Update the default MWA selection behavior
9 changes: 2 additions & 7 deletions packages/core/react/src/WalletProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,7 @@ export function WalletProvider({
}
return [mobileWalletAdapter, ...adaptersWithStandardAdapters];
}, [adaptersWithStandardAdapters, mobileWalletAdapter]);
const [walletName, setWalletName] = useLocalStorage<WalletName | null>(
localStorageKey,
getIsMobile(adaptersWithStandardAdapters) ? SolanaMobileWalletAdapterWalletName : null
);
const [walletName, setWalletName] = useLocalStorage<WalletName | null>(localStorageKey, null);
const adapter = useMemo(
() => adaptersWithMobileWalletAdapter.find((a) => a.name === walletName) ?? null,
[adaptersWithMobileWalletAdapter, walletName]
Comment on lines 80 to 82
Copy link
Member

@mcintyre94 mcintyre94 Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different to #960 and #1013 which change this to:

    const adapter = useMemo(
        () =>
            adaptersWithMobileWalletAdapter.find((a) => a.name === walletName) ??
            (adaptersWithMobileWalletAdapter.length === 1 && adaptersWithMobileWalletAdapter[0] === mobileWalletAdapter
                ? mobileWalletAdapter
                : null),
        [adaptersWithMobileWalletAdapter, walletName, mobileWalletAdapter]
    );

Do we not need that change any more?

It looks like that change was motivated by this comment: #960 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we do not want this change. If it was added I would propose a new PR to remove it as it would bring back the bug we are trying to fix here 😅

This change was added originally to try to preserve the existing behavior with MWA in the (rare) case that MWA is the one and only adapter available on Mobile. But we are aiming to remove the existing behavior, since it causes the bug with react state leading to MWA being non-functional for mobile users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its an edge case that will likely never happen anyways, MWA being the one and only adapter available. So between that and the fact that it can reintroduce the bug we are trying to fix, we think its better to remove this special treatment for MWA.

If at some point we decide to somehow change the react state handling in the UI layer to alleviate this issue caused by default selection of the adapter, we could then reintroduce special handling on MWA.

See this comment from, Jordan on the original pr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks! Glad we're finally getting this fixed!

Expand All @@ -105,8 +102,6 @@ export function WalletProvider({
if (!adapter) return;
function handleDisconnect() {
if (isUnloadingRef.current) return;
// Leave the adapter selected in the event of a disconnection.
if (walletName === SolanaMobileWalletAdapterWalletName && getIsMobile(adaptersWithStandardAdapters)) return;
setWalletName(null);
}
adapter.on('disconnect', handleDisconnect);
Expand Down Expand Up @@ -150,7 +145,7 @@ export function WalletProvider({
};
}, [adaptersWithStandardAdapters, walletName]);
const handleConnectError = useCallback(() => {
if (adapter && adapter.name !== SolanaMobileWalletAdapterWalletName) {
if (adapter) {
// If any error happens while connecting, unset the adapter.
changeWallet(null);
}
Expand Down
57 changes: 13 additions & 44 deletions packages/core/react/src/__tests__/WalletProviderMobile-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
} from '@solana-mobile/wallet-adapter-mobile';
import {
type Adapter,
BaseWalletAdapter,
WalletError,
type WalletName,
WalletReadyState,
Expand All @@ -25,6 +24,7 @@ import { act } from 'react-dom/test-utils';
import { useConnection } from '../useConnection.js';
import { useWallet, type WalletContextState } from '../useWallet.js';
import { WalletProvider, type WalletProviderProps } from '../WalletProvider.js';
import { MockWalletAdapter } from '../__mocks__/MockWalletAdapter.js';

jest.mock('../getEnvironment.js', () => ({
...jest.requireActual('../getEnvironment.js'),
Expand Down Expand Up @@ -90,36 +90,6 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
});
}

abstract class MockWalletAdapter extends BaseWalletAdapter {
connectedValue = false;
get connected() {
return this.connectedValue;
}
readyStateValue: WalletReadyState = WalletReadyState.Installed;
get readyState() {
return this.readyStateValue;
}
connecting = false;
connect = jest.fn(async () => {
this.connecting = true;
this.connecting = false;
this.connectedValue = true;
act(() => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this.emit('connect', this.publicKey!);
});
});
disconnect = jest.fn(async () => {
this.connecting = false;
this.connectedValue = false;
act(() => {
this.emit('disconnect');
});
});
sendTransaction = jest.fn();
supportedTransactionVersions = null;
autoConnect = jest.fn();
}
class FooWalletAdapter extends MockWalletAdapter {
name = 'FooWallet' as WalletName<'FooWallet'>;
url = 'https://foowallet.com';
Expand Down Expand Up @@ -201,7 +171,6 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
expect(jest.mocked(SolanaMobileWalletAdapter).mock.calls[0][0].cluster).toBe('fake-cluster-for-test');
});
});

describe('when a custom mobile wallet adapter is supplied in the adapters array', () => {
let customAdapter: Adapter;
const CUSTOM_APP_IDENTITY = {
Expand All @@ -219,9 +188,9 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
adapters.push(customAdapter);
jest.clearAllMocks();
});
it('loads the custom mobile wallet adapter into state as the default', () => {
it('does not load the custom mobile wallet adapter into state as the default', () => {
renderTest({});
expect(ref.current?.getWalletContextState().wallet?.adapter).toBe(customAdapter);
expect(ref.current?.getWalletContextState().wallet?.adapter).not.toBe(customAdapter);
});
it('does not construct any further mobile wallet adapters', () => {
renderTest({});
Expand All @@ -230,11 +199,11 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
});
describe('when there exists no stored wallet name', () => {
beforeEach(() => {
localStorage.removeItem(WALLET_NAME_CACHE_KEY);
(localStorage.getItem as jest.Mock).mockReturnValue(null);
});
it('loads the mobile wallet adapter into state as the default', () => {
it('loads no wallet into state', () => {
renderTest({});
expect(ref.current?.getWalletContextState().wallet?.adapter.name).toBe(SolanaMobileWalletAdapterWalletName);
expect(ref.current?.getWalletContextState().wallet).toBeNull();
});
it('loads no public key into state', () => {
renderTest({});
Expand All @@ -243,7 +212,7 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
});
describe('when there exists a stored wallet name', () => {
beforeEach(() => {
localStorage.setItem(WALLET_NAME_CACHE_KEY, JSON.stringify('FooWallet'));
(localStorage.getItem as jest.Mock).mockReturnValue(JSON.stringify('FooWallet'));
});
it('loads the corresponding adapter into state', () => {
renderTest({});
Expand Down Expand Up @@ -357,8 +326,8 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
adapter.emit('error', errorThrown);
});
});
it('should fire the `onError` callback', () => {
expect(onError).toHaveBeenCalledWith(errorThrown, adapter);
it('should not fire the `onError` callback', () => {
expect(onError).not.toHaveBeenCalled();
});
});
});
Expand Down Expand Up @@ -452,8 +421,8 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
mobileWalletAdapter.disconnect();
});
});
it('should not clear the stored wallet name', () => {
expect(localStorage.removeItem).not.toHaveBeenCalled();
it('should clear the stored wallet name', () => {
expect(localStorage.removeItem).toHaveBeenCalledWith(WALLET_NAME_CACHE_KEY);
});
});
describe('when window beforeunload event fires', () => {
Expand All @@ -470,8 +439,8 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
mobileWalletAdapter.disconnect();
});
});
it('should not clear the stored wallet name', () => {
expect(localStorage.removeItem).not.toHaveBeenCalled();
it('should clear the stored wallet name', () => {
expect(localStorage.removeItem).toHaveBeenCalledWith(WALLET_NAME_CACHE_KEY);
});
it('should clear out the state', () => {
expect(ref.current?.getWalletContextState()).toMatchObject({
Expand Down