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

Prevent crash when network error occur while fetching account names #1578

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented Oct 18, 2024

Make Explorer usable (gracefully handle network errors) when account names feature is not working.

For some reason I cannot access raw.githubusercontent.com today (works when I switch to another Internet provider).
Turns out that we are not handling axios errors correctly when fetching account names. We're not handling potential issues like network issues, server unreachability, or certificate errors. When such error occurs Explorer is unusable.

Screenshot from 2024-10-18 10-37-55
can be repro with
https://explorer.oasis.io/mainnet/sapphire -> dev tools -> mainnet_consensus.json -> block request domain

Copy link

github-actions bot commented Oct 18, 2024

Deployed to Cloudflare Pages

Latest commit: 7eec7dd36f7e7e02b0d32a3b8f96e3992aebdcb4
Status:✅ Deploy successful!
Preview URL: https://4f02be56.oasis-explorer.pages.dev
Alias: https://pr-1578.oasis-explorer.pages.dev

Comment on lines 64 to 65
} catch (error) {
if (axios.isAxiosError(error)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think ignoring errors should be handled per use - and I thought it already was:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is weird, but Ok. our pattern is "disable suspense's default behavior of throwing errors to the error boundary" and we do it per use like here https://github.com/oasisprotocol/explorer/blob/master/src/app/data/oasis-account-names.ts#L104.

Copy link
Member

@lukaw3d lukaw3d Oct 21, 2024

Choose a reason for hiding this comment

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

even worse:

  • react-query's default behavior is: don't throw error (useErrorBoundary in v5 is renamed to throwOnError)
  • we change defaults to throw on 5xx, and on disconnect (because no status code, because no CORS, because cloudflare)

    explorer/src/index.tsx

    Lines 17 to 27 in 2955d9d

    useErrorBoundary: (error: any) => {
    // Automatically throw on 5xx errors. Components that want to handle
    // errors should set `useErrorBoundary: false` in their queries.
    if (error.response?.status >= 500) return true
    // https://nexus.oasis.io/v1/sapphire/events?offset=0&limit=10&type=evm.log&evm_log_signature=ddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef&rel=oasis1qpdgv5nv2dhxp4q897cgag6kgnm9qs0dccwnckuu
    // threw 524 error after 100 seconds but didn't return status code to javascript. It's because Nexus didn't
    // respond quickly enough, so Cloudflare canceled with timeout, but Cloudflare doesn't add Nexus' CORS headers.
    if (!error.response && error.code === 'ERR_NETWORK') return true
    return false
  • and override the default when result isn't important (e.g. coingecko price)

I'd prefer if we ignored errors inside the displaying component. E.g. transfer value UI component knows that it isn't criticial if coingecko fails. But I took a shortcut in bc66fc4 and ignored errors inside the hooks, because it would be too much work to refactor.

Current

<SearchResultsPage
  const tokenPrices =
    useAllTokenPrices() ->
      useGetTokenPricesFromGecko() ->
        // Ignore errors. hopefully every use in every parent hook and all its sub-component isn't essential
        useQuery(useErrorBoundary: false)
  <SearchResultsView tokenPrices={tokenPrices}
    <GlobalSearchResultsView tokenPrices={tokenPrices}
      <SearchResultsList tokenPrices={tokenPrices}
        <RuntimeTransactionDetailView tokenPrices={tokenPrices}
          <dd><CurrentFiatValue amount={transaction.amount} ...tokenPrices /></dd>

Prefer

<SearchResultsPage
  <SearchResultsView
    <GlobalSearchResultsView
      <SearchResultsList
        <RuntimeTransactionDetailView
          const tokenPrices = 
            // converted tx value display isn't essential, ignore errors
            useAllTokenPrices(useErrorBoundary: false) ->
              useGetTokenPricesFromGecko(useErrorBoundary) ->
                useQuery(useErrorBoundary)
          <dd><CurrentFiatValue amount={transaction.amount} ...tokenPrices /></dd>

But this conflicts with my other preference: most UI components should be pure, so lets move fetching towards Page components.

🤷

@buberdds buberdds force-pushed the mz/error branch 2 times, most recently from 31c3d05 to 6a508b8 Compare October 21, 2024 13:24
@buberdds buberdds merged commit 487f6c3 into master Oct 22, 2024
8 checks passed
@buberdds buberdds deleted the mz/error branch October 22, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants