Skip to content
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.

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

refactor: separate all the context into a individual file #1676

Merged
merged 2 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 0 additions & 2 deletions packages/extension-polkagate/tsconfig.build.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
{ "path": "../extension-inject/tsconfig.build.json"
},
{ "path": "../extension-mocks/tsconfig.build.json"
},
{ "path": "../extension-ui/tsconfig.build.json"
}
]
}
51 changes: 51 additions & 0 deletions packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { SavedAssets } from '@polkadot/extension-polkagate/hooks/useAssetsBalances';

import React, { useContext, useEffect, useState } from 'react';

import { AccountContext, AccountsAssetsContext, AlertContext, GenesisHashOptionsContext, UserAddedChainContext, WorkerContext } from '@polkadot/extension-polkagate/src/components/contexts';
import { setStorage } from '@polkadot/extension-polkagate/src/components/Loading';
import useAssetsBalances, { ASSETS_NAME_IN_STORAGE } from '@polkadot/extension-polkagate/src/hooks/useAssetsBalances';
import useNFT from '@polkadot/extension-polkagate/src/hooks/useNFT';

export default function AccountAssetProvider ({ children }: { children: React.ReactNode }) {
const { accounts } = useContext(AccountContext);
const genesisHashOptions = useContext(GenesisHashOptionsContext);
const { setAlerts } = useContext(AlertContext);
const userAddedChainCtx = useContext(UserAddedChainContext);
const worker = useContext(WorkerContext);

const [accountsAssets, setAccountsAssets] = useState<SavedAssets | null | undefined>();

const assetsOnChains = useAssetsBalances(accounts, setAlerts, genesisHashOptions, userAddedChainCtx, worker);

useNFT(accounts);

useEffect(() => {
assetsOnChains && setAccountsAssets({ ...assetsOnChains });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [assetsOnChains?.timeStamp]);
Comment on lines +26 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the search results, I can now generate the final response since I have found the relevant information about the assetsOnChains structure and its usage:

Consider using all relevant properties in the dependency array

The assetsOnChains object is of type SavedAssets which contains balances and timeStamp. The current dependency on just timeStamp is insufficient since the effect spreads the entire assetsOnChains object but only watches for timestamp changes. This could miss updates to the balances property.

  • Update the dependency array to include assetsOnChains instead of just assetsOnChains?.timeStamp
🔗 Analysis chain

Review useEffect dependencies

The effect only depends on assetsOnChains.timeStamp. Consider if other properties of assetsOnChains should trigger the effect.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check SavedAssets type definition for available properties
ast-grep --pattern 'type SavedAssets = {
  $$$
}'

Length of output: 51


Script:

#!/bin/bash
# Let's try to find the type definition using ripgrep
rg "type SavedAssets" -A 5

# Also search for the assetsOnChains variable declaration/usage
rg "assetsOnChains" -A 2

Length of output: 2050


Script:

#!/bin/bash
# Let's check the useAssetsBalances hook to understand the structure of assetsOnChains
rg "useAssetsBalances" -A 10

# Also check for any interfaces or types related to assets
rg "interface.*Asset|type.*Asset" -A 5

Length of output: 47547


useEffect(() => {
/** remove forgotten accounts from assetChains if any */
if (accounts && assetsOnChains?.balances) {
Object.keys(assetsOnChains.balances).forEach((_address) => {
const found = accounts.find(({ address }) => address === _address);

if (!found) {
delete assetsOnChains.balances[_address];
setStorage(ASSETS_NAME_IN_STORAGE, assetsOnChains, true).catch(console.error);
}
});
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [accounts?.length, assetsOnChains?.timeStamp]);

return (
<AccountsAssetsContext.Provider value={{ accountsAssets, setAccountsAssets }}>
{children}
</AccountsAssetsContext.Provider>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { IconTheme } from '@polkadot/react-identicon/types';

import React, { useEffect, useState } from 'react';

import { AccountIconThemeContext } from '@polkadot/extension-polkagate/src/components/contexts';
import { getStorage, watchStorage } from '@polkadot/extension-polkagate/src/components/Loading';
import { DEFAULT_ACCOUNT_ICON_THEME } from '@polkadot/extension-polkagate/src/util/constants';

export default function AccountIconThemeProvider ({ children }: { children: React.ReactNode }) {
const [accountIconTheme, setAccountIconTheme] = useState<IconTheme>(DEFAULT_ACCOUNT_ICON_THEME);

useEffect(() => {
getStorage('iconTheme')
.then((maybeTheme) => setAccountIconTheme((maybeTheme as IconTheme | undefined) || DEFAULT_ACCOUNT_ICON_THEME))
.catch(console.error);

const unsubscribe = watchStorage('iconTheme', setAccountIconTheme);

return () => {
unsubscribe();
};
}, []);

return (
<AccountIconThemeContext.Provider value={{ accountIconTheme, setAccountIconTheme }}>
{children}
</AccountIconThemeContext.Provider>
);
}
75 changes: 75 additions & 0 deletions packages/extension-ui/src/Popup/contexts/AccountProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { AccountJson, AccountsContext } from '@polkadot/extension-base/background/types';

import React, { useEffect, useState } from 'react';

import { canDerive } from '@polkadot/extension-base/utils';
import { AccountContext } from '@polkadot/extension-polkagate/src/components/contexts';
import { getStorage, type LoginInfo, updateStorage } from '@polkadot/extension-polkagate/src/components/Loading';
import { subscribeAccounts } from '@polkadot/extension-polkagate/src/messaging';
import { buildHierarchy } from '@polkadot/extension-polkagate/src/util/buildHierarchy';

function initAccountContext (accounts: AccountJson[]): AccountsContext {
const hierarchy = buildHierarchy(accounts);
const master = hierarchy.find(({ isExternal, type }) => !isExternal && canDerive(type));

return {
accounts,
hierarchy,
master
};
}

export default function AccountProvider ({ children }: { children: React.ReactNode }) {
const [accounts, setAccounts] = useState<null | AccountJson[]>(null);
const [accountCtx, setAccountCtx] = useState<AccountsContext>({ accounts: [], hierarchy: [] });
const [loginInfo, setLoginInfo] = useState<LoginInfo>();

useEffect(() => {
subscribeAccounts(setAccounts).catch(console.log);
}, []);
AMIRKHANEF marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
const fetchLoginInfo = async () => {
chrome.storage.onChanged.addListener(function (changes, areaName) {
if (areaName === 'local' && 'loginInfo' in changes) {
const newValue = changes['loginInfo'].newValue as LoginInfo;

setLoginInfo(newValue);
}
});
const info = await getStorage('loginInfo') as LoginInfo;

setLoginInfo(info);
};

fetchLoginInfo().catch(console.error);
}, []);
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (!loginInfo) {
return;
}

if (loginInfo.status !== 'forgot') {
setAccountCtx(initAccountContext(accounts || []));
} else if (loginInfo.status === 'forgot') {
setAccountCtx(initAccountContext([]));
const addresses = accounts?.map((account) => account.address);

updateStorage('loginInfo', { addressesToForget: addresses }).catch(console.error);
}
}, [accounts, loginInfo]);
AMIRKHANEF marked this conversation as resolved.
Show resolved Hide resolved

if (!accounts) {
return null;
}

return (
<AccountContext.Provider value={accountCtx}>
{children}
</AccountContext.Provider>
);
}
20 changes: 20 additions & 0 deletions packages/extension-ui/src/Popup/contexts/ActionProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import React, { useCallback } from 'react';

import { ActionContext } from '@polkadot/extension-polkagate/src/components/contexts';

export default function ActionProvider ({ children }: { children: React.ReactNode }) {
const onAction = useCallback((to?: string): void => {
if (to) {
window.location.hash = to;
}
}, []);

return (
<ActionContext.Provider value={onAction}>
{children}
</ActionContext.Provider>
);
}
18 changes: 18 additions & 0 deletions packages/extension-ui/src/Popup/contexts/AlertProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { AlertType } from '@polkadot/extension-polkagate/util/types';

import React, { useState } from 'react';

import { AlertContext } from '@polkadot/extension-polkagate/src/components/contexts';

export default function AlertProvider ({ children }: { children: React.ReactNode }) {
const [alerts, setAlerts] = useState<AlertType[]>([]);

return (
<AlertContext.Provider value={{ alerts, setAlerts }}>
{children}
</AlertContext.Provider>
);
}
22 changes: 22 additions & 0 deletions packages/extension-ui/src/Popup/contexts/ApiProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { APIs } from '@polkadot/extension-polkagate/util/types';

import React, { useState } from 'react';

import { APIContext } from '@polkadot/extension-polkagate/src/components/contexts';

export default function ApiProvider ({ children }: { children: React.ReactNode }) {
const [apis, setApis] = useState<APIs>({});

const setIt = (change: APIs) => {
setApis(change);
};
AMIRKHANEF marked this conversation as resolved.
Show resolved Hide resolved

return (
<APIContext.Provider value={{ apis, setIt }}>
{children}
</APIContext.Provider>
);
}
61 changes: 61 additions & 0 deletions packages/extension-ui/src/Popup/contexts/CurrencyProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { CurrencyItemType } from '@polkadot/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency';
import type { Prices, PricesInCurrencies } from '@polkadot/extension-polkagate/src/util/types';

import React, { useEffect, useState } from 'react';

import { CurrencyContext } from '@polkadot/extension-polkagate/src/components/contexts';
import { getStorage, setStorage } from '@polkadot/extension-polkagate/src/components/Loading';
import usePriceIds from '@polkadot/extension-polkagate/src/hooks/usePriceIds';
import { isPriceUpToDate } from '@polkadot/extension-polkagate/src/hooks/usePrices';
import { getPrices } from '@polkadot/extension-polkagate/src/util/api';

interface CurrencyProviderProps {
children: React.ReactNode;
}

export default function CurrencyProvider ({ children }: CurrencyProviderProps) {
const priceIds = usePriceIds();

const [currency, setCurrency] = useState<CurrencyItemType>();
const isFetchingPricesRef = React.useRef(false);

useEffect(() => {
if (priceIds && currency?.code && !isFetchingPricesRef.current) {
isFetchingPricesRef.current = true;

getStorage('pricesInCurrencies')
.then((res) => {
const savedPricesInCurrencies = (res || {}) as PricesInCurrencies;
const maybeSavedPriceInCurrentCurrencyCode = savedPricesInCurrencies[currency.code];

if (maybeSavedPriceInCurrentCurrencyCode && isPriceUpToDate(maybeSavedPriceInCurrentCurrencyCode.date)) {
/** price in the selected currency is already updated hence no need to fetch again */
// TODO: FixMe: what if users change selected chainS during price validity period?
return;
}

getPrices(priceIds, currency.code.toLowerCase())
.then((newPrices) => {
delete (newPrices as Prices).currencyCode;
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved
savedPricesInCurrencies[currency.code] = newPrices;
setStorage('pricesInCurrencies', savedPricesInCurrencies)
.catch(console.error);
})
.catch(console.error);
})
.catch(console.error)
.finally(() => {
isFetchingPricesRef.current = false;
});
}
}, [currency?.code, priceIds]);

return (
<CurrencyContext.Provider value={{ currency, setCurrency }}>
{children}
</CurrencyContext.Provider>
);
}
22 changes: 22 additions & 0 deletions packages/extension-ui/src/Popup/contexts/FetchingProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import type { Fetching } from '@polkadot/extension-polkagate/util/types';

import React, { useCallback, useState } from 'react';

import { FetchingContext } from '@polkadot/extension-polkagate/src/components/contexts';

export default function FetchingProvider ({ children }: { children: React.ReactNode }) {
const [fetching, setFetching] = useState<Fetching>({});

const set = useCallback((change: Fetching) => {
setFetching(change);
}, []);

return (
<FetchingContext.Provider value={{ fetching, set }}>
{children}
</FetchingContext.Provider>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import React from 'react';

import { GenesisHashOptionsContext } from '@polkadot/extension-polkagate/src/components/contexts';
import useGenesisHashOptions from '@polkadot/extension-polkagate/src/hooks/useGenesisHashOptions';
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved

export default function GenesisHashOptionsProvider ({ children }: { children: React.ReactNode }) {
const genesisHashOptionsCtx = useGenesisHashOptions();

return (
<GenesisHashOptionsContext.Provider value={genesisHashOptionsCtx}>
{children}
</GenesisHashOptionsContext.Provider>
);
}
49 changes: 49 additions & 0 deletions packages/extension-ui/src/Popup/contexts/MediaProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0

import React, { useContext, useEffect, useState } from 'react';

import { MediaContext, SettingsContext } from '@polkadot/extension-polkagate/src/components/contexts';
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid circular dependencies between packages

The import from @polkadot/extension-polkagate in an extension-ui component creates a circular dependency between packages. Consider moving shared contexts to a common location or defining them within the extension-ui package.

-import { MediaContext, SettingsContext } from '@polkadot/extension-polkagate/src/components/contexts';
+import { MediaContext, SettingsContext } from '../../contexts';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { MediaContext, SettingsContext } from '@polkadot/extension-polkagate/src/components/contexts';
import { MediaContext, SettingsContext } from '../../contexts';


interface MediaProviderProps {
children: React.ReactNode;
}

// Request permission for video, based on access we can hide/show import
async function requestMediaAccess (cameraOn: boolean): Promise<boolean> {
if (!cameraOn) {
return false;
}

try {
await navigator.mediaDevices.getUserMedia({ video: true });

return true;
} catch (error) {
console.error('Permission for video declined', (error as Error).message);
}

return false;
}
Comment on lines +13 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance media access handling and cleanup

The current implementation has several areas for improvement:

  1. Error handling only logs to console
  2. No cleanup of media streams
  3. Limited reusability with hardcoded video constraint

Consider this improved implementation:

-async function requestMediaAccess (cameraOn: boolean): Promise<boolean> {
+async function requestMediaAccess (cameraOn: boolean, constraints: MediaStreamConstraints = { video: true }): Promise<boolean> {
   if (!cameraOn) {
     return false;
   }
 
+  let stream: MediaStream | null = null;
   try {
-    await navigator.mediaDevices.getUserMedia({ video: true });
+    stream = await navigator.mediaDevices.getUserMedia(constraints);
     return true;
   } catch (error) {
-    console.error('Permission for video declined', (error as Error).message);
+    console.warn('Media access error:', (error as Error).message);
+    return false;
+  } finally {
+    if (stream) {
+      stream.getTracks().forEach(track => track.stop());
+    }
   }
-
-  return false;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function requestMediaAccess (cameraOn: boolean): Promise<boolean> {
if (!cameraOn) {
return false;
}
try {
await navigator.mediaDevices.getUserMedia({ video: true });
return true;
} catch (error) {
console.error('Permission for video declined', (error as Error).message);
}
return false;
}
async function requestMediaAccess (cameraOn: boolean, constraints: MediaStreamConstraints = { video: true }): Promise<boolean> {
if (!cameraOn) {
return false;
}
let stream: MediaStream | null = null;
try {
stream = await navigator.mediaDevices.getUserMedia(constraints);
return true;
} catch (error) {
console.warn('Media access error:', (error as Error).message);
return false;
} finally {
if (stream) {
stream.getTracks().forEach(track => track.stop());
}
}
}


export default function MediaProvider ({ children }: MediaProviderProps) {
const settings = useContext(SettingsContext);
const [cameraOn, setCameraOn] = useState(settings.camera === 'on');
const [mediaAllowed, setMediaAllowed] = useState(false);

useEffect(() => {
setCameraOn(settings.camera === 'on');
}, [settings.camera]);

useEffect(() => {
requestMediaAccess(cameraOn)
.then(setMediaAllowed)
.catch(console.error);
}, [cameraOn]);

return (
<MediaContext.Provider value={cameraOn && mediaAllowed}>
{children}
</MediaContext.Provider>
);
}
Comment on lines +29 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance provider with loading and error states

The current implementation could be improved by:

  1. Adding loading state during media access request
  2. Exposing error state to consumers
  3. Combining related effects
  4. Adding proper cleanup

Consider this enhanced implementation:

+interface MediaContextState {
+  isEnabled: boolean;
+  isLoading: boolean;
+  error?: Error;
+}
+
 export default function MediaProvider ({ children }: MediaProviderProps) {
   const settings = useContext(SettingsContext);
-  const [cameraOn, setCameraOn] = useState(settings.camera === 'on');
-  const [mediaAllowed, setMediaAllowed] = useState(false);
+  const [state, setState] = useState<MediaContextState>({
+    isEnabled: false,
+    isLoading: false
+  });
 
   useEffect(() => {
-    setCameraOn(settings.camera === 'on');
-  }, [settings.camera]);
-
-  useEffect(() => {
-    requestMediaAccess(cameraOn)
-      .then(setMediaAllowed)
-      .catch(console.error);
-  }, [cameraOn]);
+    const isOn = settings.camera === 'on';
+    
+    if (!isOn) {
+      setState({ isEnabled: false, isLoading: false });
+      return;
+    }
+    
+    setState(prev => ({ ...prev, isLoading: true }));
+    
+    requestMediaAccess(isOn)
+      .then(allowed => {
+        setState({
+          isEnabled: allowed,
+          isLoading: false
+        });
+      })
+      .catch(error => {
+        setState({
+          isEnabled: false,
+          isLoading: false,
+          error
+        });
+      });
+  }, [settings.camera]);
 
   return (
-    <MediaContext.Provider value={cameraOn && mediaAllowed}>
+    <MediaContext.Provider value={state}>
       {children}
     </MediaContext.Provider>
   );
 }

Committable suggestion skipped: line range outside the PR's diff.

Loading
Loading