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

Add Sentry config and ErrorBoundary Component#1528

Merged
dasanra merged 36 commits intorelease/v2.14.0from
issue-1289
Oct 30, 2020
Merged

Add Sentry config and ErrorBoundary Component#1528
dasanra merged 36 commits intorelease/v2.14.0from
issue-1289

Conversation

@nicosampler
Copy link
Contributor

@nicosampler nicosampler commented Oct 26, 2020

Closes #1289.

  • Add basic config for Sentry. (react-router and components optimization are not included
  • Add ErrorBoundary config
  • Design for ErrorBoundary page. (@alongoni Let me know if you need help in order to force an error)
  • It needs some environment configuration @dasanra will work on that.
  • it needs some configuration so Travis only uploads source-code when a release is done

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Oct 26, 2020

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 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Oct 27, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Oct 27, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Oct 27, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Oct 27, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/app

@nicosampler nicosampler mentioned this pull request Oct 27, 2020
@ghost
Copy link

ghost commented Oct 28, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Oct 28, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

Comment on lines +31 to +39
<Button
size="md"
color="primary"
onClick={() => {
window.location.href = '/app/'
}}
>
Go to Landing
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a link

Copy link
Contributor Author

@nicosampler nicosampler Oct 28, 2020

Choose a reason for hiding this comment

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

right, we should add a Link component in Components. And make it possible to look like a button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikheevm do you know if we already have a link with the styles of a button in Gnosis? otherwise, I would leave it as it is and create a ticket in Components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikheevm I just started the ticket to add the Link component, but, should we use links as buttons? Technically we could, we only need to "copy" all the material-ui styles for the button and apply them to the anchor element. But does it make sense? shouldn't we use the links with a different style than buttons? they are used for different actions so it's reasonable they should use different styles.

What I'm suggesting is that we should change the button and use the link style, like in here:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@nicosampler nicosampler Oct 29, 2020

Choose a reason for hiding this comment

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

TLTR, I think we should separate the actions from navigation, meaning, if we want to move inside the page or to another page, we should use links with link styles. If we want to trigger an action, we should use buttons with button styles.

Giving a quick review of the app the only inconsistency I found was in the nav of each section:
image
image

It should have the button style.

@ghost
Copy link

ghost commented Oct 29, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Oct 29, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Oct 29, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Oct 29, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Oct 29, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@fernandomg
Copy link
Contributor

fernandomg commented Oct 29, 2020

@nicosampler, I think that the case for when we know the address and redirect to the /balances page is not working. See #1289 (comment)

This invalid URL redirects to https://pr1528--safereact.review.gnosisdev.com/rinkeby/app/, given that the safe address is valid

@nicosampler
Copy link
Contributor Author

@fernandomg pushed the fix, can you check it now, please?

@ghost
Copy link

ghost commented Oct 30, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@fernandomg
Copy link
Contributor

pushed the fix, can you check it now, please?

@nicosampler It works fine.


Something I want to understand, as I interpreted the requirements a bit differently.

I thought that the error page will be shown, and when clicking 'go home' it would redirect to /balances or /welcome page.

By "known safe" I understood something around querying the tx-service (safes/{safeAddress}/) for the Safe's info.

If the service does not recognize the Safe, we redirect to /welcome, else /balances.

That way, if I enter an EOA or any address, it will prevent loading the Safe App if it's not a known Safe Address for the current environment.

@nicosampler
Copy link
Contributor Author

pushed the fix, can you check it now, please?

@nicosampler It works fine.

Something I want to understand, as I interpreted the requirements a bit differently.

I thought that the error page will be shown, and when clicking 'go home' it would redirect to /balances or /welcome page.

By "known safe" I understood something around querying the tx-service (safes/{safeAddress}/) for the Safe's info.

If the service does not recognize the Safe, we redirect to /welcome, else /balances.

That way, if I enter an EOA or any address, it will prevent loading the Safe App if it's not a known Safe Address for the current environment.

@fernandomg I think we never talked about that flow but seems an interesting idea.

The idea of this ticket was:

  • if something goes wrong, we show a page error.
  • if the user enters a not known page, let's say http://localhost:3000/app#/something, we redirect to http://localhost:3000/app#/welcome
  • if the user enters a not known subpath for a safe or just a safe, we redirect to /balances. (that's the issue you reported yesterday)

I think we should create a ticket for the flow you are proposing

Copy link
Contributor

@Agupane Agupane left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@dasanra dasanra changed the base branch from development to release/v2.14.0 October 30, 2020 19:31
@ghost
Copy link

ghost commented Oct 30, 2020

Travis automatic deployment:
https://pr1528--safereact.review.gnosisdev.com/rinkeby/app

@dasanra dasanra merged commit df42b36 into release/v2.14.0 Oct 30, 2020
@dasanra dasanra deleted the issue-1289 branch October 30, 2020 19:55
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2020
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.

Crash logging

6 participants