Skip to content

ZulipMobile: Add and use RootErrorBoundary.#4585

Merged
gnprice merged 1 commit intozulip:masterfrom
chrisbobbe:pr-root-error-boundary
Apr 2, 2021
Merged

ZulipMobile: Add and use RootErrorBoundary.#4585
gnprice merged 1 commit intozulip:masterfrom
chrisbobbe:pr-root-error-boundary

Conversation

@chrisbobbe
Copy link
Copy Markdown
Contributor

Fixes: #4584

@chrisbobbe
Copy link
Copy Markdown
Contributor Author

See screenshots/discussion at #4584 (comment).

@chrisbobbe chrisbobbe force-pushed the pr-root-error-boundary branch from 627a902 to a844200 Compare April 1, 2021 21:53
@chrisbobbe
Copy link
Copy Markdown
Contributor Author

chrisbobbe commented Apr 1, 2021

Hmm, potentially interesting: Sentry provides a ready-to-use component Sentry.ErrorBoundary. It looks like it was designed for use on the web, so in particular its built-in user feedback widget hasn't been working for me.

But other than that, I think it'd work, if we wanted to use it. One nice thing, if I pass a function for the fallback prop, I get the error info, as I'd expect (we'd use that to show the stack trace, like we do in this revision)— but I also get the Sentry event ID, which could be useful debugging data.

@chrisbobbe chrisbobbe force-pushed the pr-root-error-boundary branch from a844200 to de70e18 Compare April 1, 2021 22:04
@chrisbobbe
Copy link
Copy Markdown
Contributor Author

It looks like it was designed for use on the web, so in particular its built-in user feedback widget hasn't been working for me.

Though it looks like it might not be too hard to set something like this up manually: getsentry/sentry-react-native#500

@chrisbobbe chrisbobbe force-pushed the pr-root-error-boundary branch from de70e18 to 9124ca1 Compare April 1, 2021 22:24
@gnprice
Copy link
Copy Markdown
Member

gnprice commented Apr 2, 2021

Looks good, thanks! Merging.

I have one bit of UI feedback that I think is worth doing (naturally we don't want to spend a lot of time on this UI): instead of having a lot of text in the button:
image

a more idiomatic thing to do would be to have some text to say most of that, and then have the button very concisely say what the button itself will do. Like maybe "Copy details". See e.g.:
https://material.io/components/buttons#text-button

But given that I'd like to ship a beta today with #4588, and it'd be good to include this in that too, that is not something worth blocking on and instead can be a small followup 🙂

@gnprice gnprice force-pushed the pr-root-error-boundary branch from 9124ca1 to 2cc385a Compare April 2, 2021 22:58
@gnprice gnprice merged commit 2cc385a into zulip:master Apr 2, 2021
@gnprice
Copy link
Copy Markdown
Member

gnprice commented Apr 2, 2021

(Also I made a tiny formatting tweak to the output:

 Component Stack:
-
 ${

because it seemed like empirically there were one too many blank lines in there. My screenshot just above reflects that change.)

@chrisbobbe
Copy link
Copy Markdown
Contributor Author

Thanks for the review!

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 3, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 5, 2021
@chrisbobbe chrisbobbe deleted the pr-root-error-boundary branch November 4, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a React "error boundary" near the root of the component tree.

2 participants