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.

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

fix(web): appUrl query was getting lost while switching network in safeApp #5081

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

fix(web): appUrl query was getting lost while switching network in safeApp #5081

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
33480b4
fix(web): appUrl query was getting lost while switching network in ap…
chiranjeev13 Feb 22, 2025
8211879
fix(web): if chainId is invalid redirect to safeApp dashboard (#4980)
chiranjeev13 Feb 23, 2025
c288de4
fix(web): displaying name instead of chainId and using useEffect for …
chiranjeev13 Feb 23, 2025
16232ee
Merge branch 'safe-global:dev' into dev
chiranjeev13 Feb 24, 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 apps/web/src/components/common/NetworkSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export const getNetworkLink = (router: NextRouter, safeAddress: string, networkS
safe?: string
chain?: string
safeViewRedirectURL?: string
appUrl?: string
}

const route = {
Expand All @@ -92,6 +93,10 @@ export const getNetworkLink = (router: NextRouter, safeAddress: string, networkS
route.query.safeViewRedirectURL = router.query?.safeViewRedirectURL.toString()
}

if (router.query?.appUrl) {
route.query.appUrl = router.query.appUrl.toString()
Copy link
Member

Choose a reason for hiding this comment

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

This preserves the query indeed but ignores the fact that a Safe App might be not enabled on all chains.

From the original issue requirements:

Providing the Safe App is present on the destination chain, the Safe App should remain open, otherwise the dashboard navigated to, for example.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I was thinking the same. However, in the case of other EOAs, when we switch to a different network within an app, it's the app that indicates it as an unsupported network.

Since we're displaying the app within Safe's dashboard, I guess it's a better user experience for Safe to indicate an unsupported network. Working on this fix !!

Copy link
Author

Choose a reason for hiding this comment

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

Updated the unsupported chain handling logic in apps/web/src/pages/apps/open.tsx:

In this commit 8211879

Key Modifications:

  • Refined the condition to check when safeApp.chainIds is empty
  • Maintained 3-second redirect timeout
  • ensured the remoteSafeAppsLoading is false before we check for chainIds

Video Preview

Screen.Recording.2025-02-23.at.10.11.45.PM.mov

Let me know if any thing else needs to be updated @katspaugh

Copy link
Member

Choose a reason for hiding this comment

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

Good idea!

}

return route
}

Expand Down
28 changes: 27 additions & 1 deletion apps/web/src/pages/apps/open.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { NextPage } from 'next'
import { useRouter } from 'next/router'
import { useCallback } from 'react'
import { Box, CircularProgress } from '@mui/material'
import { Box, CircularProgress, Typography } from '@mui/material'

import { useSafeAppUrl } from '@/hooks/safe-apps/useSafeAppUrl'
import { useSafeApps } from '@/hooks/safe-apps/useSafeApps'
Expand Down Expand Up @@ -88,6 +88,32 @@ const SafeApps: NextPage = () => {
)
}

if (!remoteSafeAppsLoading && !isLoading && safeApp.chainIds.length === 0) {
setTimeout(() => {
router.push({
pathname: AppRoutes.apps.index,
query: { safe: router.query.safe },
})
}, 3000)
Copy link
Member

Choose a reason for hiding this comment

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

This should be done in a useEffect.

return (
<Box
display="flex"
flexDirection="column"
alignItems="center"
justifyContent="center"
height="100%"
textAlign="center"
p={2}
>
<Typography variant="body1" gutterBottom>
Chain {chainId} is not supported in this app. <br />
Copy link
Member

Choose a reason for hiding this comment

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

Can we display chainName here? It can be taken from useCurrentChain().

Copy link
Author

Choose a reason for hiding this comment

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

Sure let me just do that instead!

Copy link
Author

Choose a reason for hiding this comment

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

Updated fix:

  • Using useEffect for routing back to safeApp Dashboard
  • Displaying chain name instead of chainId using useCurrentChain()

check it out here: c288de4

Redirecting to home page
</Typography>
<CircularProgress />
</Box>
)
}

return (
<SafeAppsErrorBoundary render={() => <SafeAppsLoadError onBackToApps={() => router.back()} />}>
<AppFrame appUrl={appUrl} allowedFeaturesList={getAllowedFeaturesList(origin)} safeAppFromManifest={safeApp} />
Expand Down