Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Feature: support redirect param on creation page, redirect to the Safe app page if Safe was created from the landing #3790

Merged
merged 5 commits into from
Apr 26, 2022

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Apr 13, 2022

What it solves

Resolves safe-global/safe-react-apps#417

How this PR fixes it

Adds new redirect param for the safe creation route. If the URL string has a template for Safe Route, it populates it with the address.

How to test it

  1. Create a safe from https://pr3790--safereact.review-safe.gnosisdev.com/app/share/safe-app?appUrl=https%3A%2F%2Fapps.gnosis-safe.io%2Ftx-builder&chainId=4
  2. After the creation, it should take you to the tx builder app

@mmv08 mmv08 changed the base branch from dev to safe-app-landing-page April 13, 2022 14:31
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Apr 13, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@mmv08 mmv08 force-pushed the feat/create-safe-apps-redirect branch from b7aaab4 to 2414d14 Compare April 13, 2022 14:38
@github-actions
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

@mmv08 mmv08 requested review from DaniSomoza, katspaugh, iamacook, dasanra and yagopv and removed request for DaniSomoza April 13, 2022 14:41
@mmv08 mmv08 force-pushed the feat/create-safe-apps-redirect branch from 2414d14 to e9670f9 Compare April 13, 2022 14:59
@mmv08 mmv08 force-pushed the feat/create-safe-apps-redirect branch from e9670f9 to 93f5ff2 Compare April 13, 2022 15:11
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -75,6 +75,10 @@ const SafeAppLandingPage = (): ReactElement => {

const showLoader = isLoading || !safeAppDetails

if (!safeAppUrl) {
return <Redirect to={WELCOME_ROUTE} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This edge case is already addressed in the first useEffect right?

What is your opinion about to remove this redundant redirection and keep showing the Loader? (and restore this 2 lines in the unit tests?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think addressing it in useEffect is the way to go. We don't need to render anything if there's no safeAppUrl. This is also better for types stuff, because query.get returns string | undefined and this narrows it to string, making it easer for subsequent access

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe makes sense update both useEffect ?. Something like this:

const SafeAppLandingPage = (): ReactElement => {
  const query = useQuery()
  const safeAppChainId = query.get('chainId')
  const safeAppUrl = query.get('appUrl')

  useEffect(() => {
    if (isValidChainId(safeAppChainId)) {
      setChainId(safeAppChainId)
    }
  }, [safeAppChainId])

  [...] // other stuff

  // the second useEffect is unnecessary, and instead, do:

  const isSafeAppMissing = !isLoading && !safeAppDetails && isManifestError

  if (!safeAppUrl || !safeAppChainId || isSafeAppMissing) {
    return <Redirect to={WELCOME_ROUTE} />
  }

  const availableChains = safeAppDetails?.chainIds || []

  const showLoader = isLoading || !safeAppDetails

  return (
    <Container>
      <StyledCard>
            [...]

Copy link
Member Author

Choose a reason for hiding this comment

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

done senor

@mmv08 mmv08 force-pushed the feat/create-safe-apps-redirect branch from 18ea042 to 49fbce8 Compare April 18, 2022 16:33
@github-actions
Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@DaniSomoza DaniSomoza merged commit 4a3bbe4 into safe-app-landing-page Apr 26, 2022
@DaniSomoza DaniSomoza deleted the feat/create-safe-apps-redirect branch April 26, 2022 16:00
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants