fix: address comments

safe-global · Sep 14, 2023 · 12116b7 · 12116b7
1 parent a115c14
commit 12116b7
Show file tree

Hide file tree

Showing 13 changed files with 70 additions and 72 deletions.
diff --git a/package.json b/package.json
@@ -53,7 +53,7 @@
     "@safe-global/safe-core-sdk-utils": "^1.7.4",
     "@safe-global/safe-deployments": "1.25.0",
     "@safe-global/safe-ethers-lib": "^1.9.4",
-    "@safe-global/safe-gateway-typescript-sdk": "^3.11.0",
+    "@safe-global/safe-gateway-typescript-sdk": "^3.12.0",
     "@safe-global/safe-modules-deployments": "^1.0.0",
     "@safe-global/safe-react-components": "^2.0.6",
     "@sentry/react": "^7.28.1",

diff --git a/src/components/settings/PushNotifications/GlobalPushNotifications.tsx b/src/components/settings/PushNotifications/GlobalPushNotifications.tsx
@@ -261,13 +261,15 @@ export const GlobalPushNotifications = (): ReactElement | null => {
       return
     }
 
+    // Although the (un-)registration functions will request permission,
+    // we manually change beforehand prevent multiple promises from throwing
     const isGranted = await requestNotificationPermission()
 
     if (!isGranted) {
       return
     }
 
-    const registrationPromises: Array<Promise<void>> = []
+    const registrationPromises: Array<Promise<unknown>> = []
 
     const safesToRegister = getSafesToRegister(selectedSafes, currentNotifiedSafes)
     if (safesToRegister) {

diff --git a/src/components/settings/PushNotifications/PushNotificationsBanner/index.tsx b/src/components/settings/PushNotifications/PushNotificationsBanner/index.tsx
@@ -41,11 +41,11 @@ export const PushNotificationsBanner = ({ children }: { children: ReactElement }
   const dismissBanner = useCallback(() => {
     trackEvent(PUSH_NOTIFICATION_EVENTS.DISMISS_BANNER)
 
-    setDismissedBannerPerChain({
-      ...dismissedBannerPerChain,
+    setDismissedBannerPerChain((prev) => ({
+      ...prev,
       [safe.chainId]: true,
-    })
-  }, [dismissedBannerPerChain, safe.chainId, setDismissedBannerPerChain])
+    }))
+  }, [safe.chainId, setDismissedBannerPerChain])
 
   // Click outside to dismiss banner
   useEffect(() => {

diff --git a/src/components/settings/PushNotifications/PushNotificationsBanner/styles.module.css b/src/components/settings/PushNotifications/PushNotificationsBanner/styles.module.css
@@ -33,7 +33,6 @@
   font-size: 12px;
   width: var(--space-5);
   height: 24px;
-  z-index: 9999999;
   position: relative;
 }
 

diff --git a/src/components/settings/PushNotifications/hooks/__tests__/useNotificationPreferences.test.ts b/src/components/settings/PushNotifications/hooks/__tests__/useNotificationPreferences.test.ts
@@ -216,7 +216,7 @@ describe('useNotificationPreferences', () => {
       })
     })
 
-    it('should clearPreferences preferences, then hydrate the preferences state', async () => {
+    it('should delete all preferences, then hydrate the preferences state', async () => {
       const chainId1 = '1'
       const safeAddress1 = hexZeroPad('0x1', 20)
       const safeAddress2 = hexZeroPad('0x1', 20)

diff --git a/...omponents/settings/PushNotifications/hooks/__tests__/useNotificationRegistrations.test.ts b/...omponents/settings/PushNotifications/hooks/__tests__/useNotificationRegistrations.test.ts
@@ -302,13 +302,13 @@ describe('useNotificationRegistrations', () => {
       unregisterDeviceSpy.mockImplementation(() => Promise.resolve('Unregistration could not be completed.'))
 
       const uuid = self.crypto.randomUUID()
-      const clearPreferencesMock = jest.fn()
+      const deleteAllPreferencesMock = jest.fn()
 
       ;(preferences.useNotificationPreferences as jest.Mock).mockImplementation(
         () =>
           ({
             uuid,
-            _clearPreferences: clearPreferencesMock,
+            _deleteAllPreferences: deleteAllPreferencesMock,
           } as unknown as ReturnType<typeof preferences.useNotificationPreferences>),
       )
 
@@ -318,20 +318,20 @@ describe('useNotificationRegistrations', () => {
 
       expect(unregisterDeviceSpy).toHaveBeenCalledWith('1', uuid)
 
-      expect(clearPreferencesMock).not.toHaveBeenCalled()
+      expect(deleteAllPreferencesMock).not.toHaveBeenCalled()
     })
 
     it('does not clear preferences if unregistration throws', async () => {
       unregisterDeviceSpy.mockImplementation(() => Promise.reject())
 
       const uuid = self.crypto.randomUUID()
-      const clearPreferencesMock = jest.fn()
+      const deleteAllPreferencesMock = jest.fn()
 
       ;(preferences.useNotificationPreferences as jest.Mock).mockImplementation(
         () =>
           ({
             uuid,
-            _clearPreferences: clearPreferencesMock,
+            _deleteAllPreferences: deleteAllPreferencesMock,
           } as unknown as ReturnType<typeof preferences.useNotificationPreferences>),
       )
 
@@ -341,20 +341,20 @@ describe('useNotificationRegistrations', () => {
 
       expect(unregisterDeviceSpy).toHaveBeenCalledWith('1', uuid)
 
-      expect(clearPreferencesMock).not.toHaveBeenCalledWith()
+      expect(deleteAllPreferencesMock).not.toHaveBeenCalledWith()
     })
 
     it('clears preferences if unregistration succeeds', async () => {
       unregisterDeviceSpy.mockImplementation(() => Promise.resolve())
 
       const uuid = self.crypto.randomUUID()
-      const clearPreferencesMock = jest.fn()
+      const deleteAllPreferencesMock = jest.fn()
 
       ;(preferences.useNotificationPreferences as jest.Mock).mockImplementation(
         () =>
           ({
             uuid,
-            _clearPreferences: clearPreferencesMock,
+            _deleteAllPreferences: deleteAllPreferencesMock,
           } as unknown as ReturnType<typeof preferences.useNotificationPreferences>),
       )
 
@@ -364,7 +364,7 @@ describe('useNotificationRegistrations', () => {
 
       expect(unregisterDeviceSpy).toHaveBeenCalledWith('1', uuid)
 
-      expect(clearPreferencesMock).toHaveBeenCalled()
+      expect(deleteAllPreferencesMock).toHaveBeenCalled()
     })
   })
 })
diff --git a/src/components/settings/PushNotifications/hooks/useNotificationPreferences.ts b/src/components/settings/PushNotifications/hooks/useNotificationPreferences.ts
@@ -50,7 +50,7 @@ export const useNotificationPreferences = (): {
   ) => void
   _createPreferences: (safesToRegister: NotifiableSafes) => void
   _deletePreferences: (safesToUnregister: NotifiableSafes) => void
-  _clearPreferences: () => void
+  _deleteAllPreferences: () => void
 } => {
   // State
   const uuid = useUuid()
@@ -195,7 +195,7 @@ export const useNotificationPreferences = (): {
   }
 
   // Delete all preferences store entries
-  const clearPreferences = () => {
+  const deleteAllPreferences = () => {
     if (!preferencesStore) {
       return
     }
@@ -212,6 +212,6 @@ export const useNotificationPreferences = (): {
     updatePreferences,
     _createPreferences: createPreferences,
     _deletePreferences: deletePreferences,
-    _clearPreferences: clearPreferences,
+    _deleteAllPreferences: deleteAllPreferences,
   }
 }
diff --git a/src/components/settings/PushNotifications/hooks/useNotificationRegistrations.ts b/src/components/settings/PushNotifications/hooks/useNotificationRegistrations.ts
@@ -11,7 +11,7 @@ import { logError } from '@/services/exceptions'
 import ErrorCodes from '@/services/exceptions/ErrorCodes'
 import type { NotifiableSafes } from '../logic'
 
-const registrationFlow = async (registrationFn: Promise<void>, callback: () => void) => {
+const registrationFlow = async (registrationFn: Promise<void>, callback: () => void): Promise<boolean> => {
   let success = false
 
   try {
@@ -27,16 +27,19 @@ const registrationFlow = async (registrationFn: Promise<void>, callback: () => v
   if (success) {
     callback()
   }
+
+  return success
 }
+
 export const useNotificationRegistrations = (): {
-  registerNotifications: (safesToRegister: NotifiableSafes, withSignature?: boolean) => Promise<void>
-  unregisterSafeNotifications: (chainId: string, safeAddress: string) => Promise<void>
-  unregisterChainNotifications: (chainId: string) => Promise<void>
+  registerNotifications: (safesToRegister: NotifiableSafes, withSignature?: boolean) => Promise<boolean | undefined>
+  unregisterSafeNotifications: (chainId: string, safeAddress: string) => Promise<boolean | undefined>
+  unregisterChainNotifications: (chainId: string) => Promise<boolean | undefined>
 } => {
   const dispatch = useAppDispatch()
   const web3 = useWeb3()
 
-  const { uuid, _createPreferences, _deletePreferences, _clearPreferences } = useNotificationPreferences()
+  const { uuid, _createPreferences, _deletePreferences, _deleteAllPreferences } = useNotificationPreferences()
 
   const registerNotifications = async (safesToRegister: NotifiableSafes) => {
     if (!uuid || !web3) {
@@ -53,7 +56,7 @@ export const useNotificationRegistrations = (): {
       return registerDevice(payload)
     }
 
-    await registrationFlow(register(), () => {
+    return registrationFlow(register(), () => {
       _createPreferences(safesToRegister)
 
       const totalRegistered = Object.values(safesToRegister).reduce(
@@ -80,7 +83,7 @@ export const useNotificationRegistrations = (): {
 
   const unregisterSafeNotifications = async (chainId: string, safeAddress: string) => {
     if (uuid) {
-      await registrationFlow(unregisterSafe(chainId, safeAddress, uuid), () => {
+      return registrationFlow(unregisterSafe(chainId, safeAddress, uuid), () => {
         _deletePreferences({ [chainId]: [safeAddress] })
         trackEvent(PUSH_NOTIFICATION_EVENTS.UNREGISTER_SAFE)
       })
@@ -89,8 +92,8 @@ export const useNotificationRegistrations = (): {
 
   const unregisterChainNotifications = async (chainId: string) => {
     if (uuid) {
-      await registrationFlow(unregisterDevice(chainId, uuid), () => {
-        _clearPreferences()
+      return registrationFlow(unregisterDevice(chainId, uuid), () => {
+        _deleteAllPreferences()
         trackEvent(PUSH_NOTIFICATION_EVENTS.UNREGISTER_DEVICE)
       })
     }

diff --git a/src/components/settings/PushNotifications/index.tsx b/src/components/settings/PushNotifications/index.tsx
@@ -22,18 +22,19 @@ import { GlobalPushNotifications } from './GlobalPushNotifications'
 import useIsSafeOwner from '@/hooks/useIsSafeOwner'
 import { IS_DEV } from '@/config/constants'
 import { useAppDispatch } from '@/store'
-import { showNotification } from '@/store/notificationsSlice'
 import { trackEvent } from '@/services/analytics'
 import { PUSH_NOTIFICATION_EVENTS } from '@/services/analytics/events/push-notifications'
 import { AppRoutes } from '@/config/routes'
 import CheckWallet from '@/components/common/CheckWallet'
+import { useIsMac } from '@/hooks/useIsMac'
 
 import css from './styles.module.css'
 
 export const PushNotifications = (): ReactElement => {
   const dispatch = useAppDispatch()
   const { safe, safeLoaded } = useSafeInfo()
   const isOwner = useIsSafeOwner()
+  const isMac = useIsMac()
 
   const { updatePreferences, getPreferences, getAllPreferences } = useNotificationPreferences()
   const { unregisterSafeNotifications, unregisterChainNotifications, registerNotifications } =
@@ -45,7 +46,6 @@ export const PushNotifications = (): ReactElement => {
     updatePreferences(safe.chainId, safe.address.value, newPreferences)
   }
 
-  const isMac = typeof navigator !== 'undefined' && navigator.userAgent.includes('Mac')
   const shouldShowMacHelper = isMac || IS_DEV
 
   const handleOnChange = async () => {
@@ -216,28 +216,29 @@ export const PushNotifications = (): ReactElement => {
                   control={
                     <Checkbox
                       checked={preferences[WebhookType.CONFIRMATION_REQUEST]}
-                      onChange={async (_, checked) => {
-                        registerNotifications({
-                          [safe.chainId]: [safe.address.value],
-                        })
-                          .then(() => {
-                            setPreferences({
-                              ...preferences,
-                              [WebhookType.CONFIRMATION_REQUEST]: checked,
-                            })
+                      onChange={(_, checked) => {
+                        const updateConfirmationRequestPreferences = () => {
+                          setPreferences({
+                            ...preferences,
+                            [WebhookType.CONFIRMATION_REQUEST]: checked,
+                          })
 
-                            trackEvent({ ...PUSH_NOTIFICATION_EVENTS.TOGGLE_CONFIRMATION_REQUEST, label: checked })
+                          trackEvent({ ...PUSH_NOTIFICATION_EVENTS.TOGGLE_CONFIRMATION_REQUEST, label: checked })
+                        }
 
-                            dispatch(
-                              showNotification({
-                                message:
-                                  'You will now receive notifications about confirmation requests for this Safe Account in your browser.',
-                                variant: 'success',
-                                groupKey: 'notifications',
-                              }),
-                            )
+                        if (checked) {
+                          registerNotifications({
+                            [safe.chainId]: [safe.address.value],
                           })
-                          .catch(() => null)
+                            .then((registered) => {
+                              if (registered) {
+                                updateConfirmationRequestPreferences()
+                              }
+                            })
+                            .catch(() => null)
+                        } else {
+                          updateConfirmationRequestPreferences()
+                        }
                       }}
                     />
                   }

diff --git a/src/components/settings/PushNotifications/logic.ts b/src/components/settings/PushNotifications/logic.ts
@@ -5,8 +5,6 @@ import type { RegisterNotificationsRequest } from '@safe-global/safe-gateway-typ
 import type { Web3Provider } from '@ethersproject/providers'
 
 import { FIREBASE_VAPID_KEY, initializeFirebaseApp } from '@/services/push-notifications/firebase'
-import { trackEvent } from '@/services/analytics'
-import { PUSH_NOTIFICATION_EVENTS } from '@/services/analytics/events/push-notifications'
 import packageJson from '../../../../package.json'
 import { logError } from '@/services/exceptions'
 import ErrorCodes from '@/services/exceptions/ErrorCodes'
@@ -29,15 +27,7 @@ export const requestNotificationPermission = async (): Promise<boolean> => {
     logError(ErrorCodes._400, e)
   }
 
-  const isGranted = permission === 'granted'
-
-  trackEvent(isGranted ? PUSH_NOTIFICATION_EVENTS.GRANT_PERMISSION : PUSH_NOTIFICATION_EVENTS.REJECT_PERMISSION)
-
-  if (!isGranted) {
-    alert('You must allow notifications to register your device.')
-  }
-
-  return isGranted
+  return permission === 'granted'
 }
 
 const getSafeRegistrationSignature = ({

diff --git a/src/hooks/useDecodeTx.ts b/src/hooks/useDecodeTx.ts
@@ -14,7 +14,7 @@ const useDecodeTx = (tx?: SafeTransaction): AsyncResult<DecodedDataResponse> =>
   const nativeTransfer = isEmptyData && !isRejection ? getNativeTransferData(tx?.data) : undefined
 
   const [data = nativeTransfer, error, loading] = useAsync<DecodedDataResponse>(() => {
-    if (!encodedData || isEmptyData || !tx.data.to) return
+    if (!encodedData || isEmptyData) return
     return getDecodedData(chainId, encodedData, tx.data.to)
   }, [chainId, encodedData, isEmptyData, tx?.data.to])
 

diff --git a/src/hooks/useIsMac.ts b/src/hooks/useIsMac.ts
@@ -0,0 +1,13 @@
+import { useState, useEffect } from 'react'
+
+export const useIsMac = (): boolean => {
+  const [isMac, setIsMac] = useState(false)
+
+  useEffect(() => {
+    if (typeof navigator !== 'undefined') {
+      setIsMac(navigator.userAgent.includes('Mac'))
+    }
+  }, [])
+
+  return isMac
+}
diff --git a/src/services/analytics/events/push-notifications.ts b/src/services/analytics/events/push-notifications.ts
@@ -11,16 +11,6 @@ export const PUSH_NOTIFICATION_EVENTS = {
     action: 'Click notification',
     category,
   },
-  // User granted notification permissions
-  GRANT_PERMISSION: {
-    action: 'Allow notifications',
-    category,
-  },
-  // User refused notification permissions
-  REJECT_PERMISSION: {
-    action: 'Reject notifications',
-    category,
-  },
   // User registered Safe(s) for notifications
   REGISTER_SAFES: {
     action: 'Register Safe(s) notifications',