Skip to content

CatchError in Connect: Provide theme when rendering caught error#37006

Merged
ravicious merged 2 commits intomasterfrom
r7s/catch-error-theme
Jan 22, 2024
Merged

CatchError in Connect: Provide theme when rendering caught error#37006
ravicious merged 2 commits intomasterfrom
r7s/catch-error-theme

Conversation

@ravicious
Copy link
Copy Markdown
Member

Fixes part of #36936.

CatchError acts as an error boundary. When it catches an error, it renders Failed from design/CardError.

However, since about v13, Failed assumes that a theme is provided through ThemeProvider. CatchError is rendered at a place in the hierarchy where ThemeProvider is not available. This resulted in CatchError crashing whenever it attempted to render a caught error.

export const App: React.FC<{ ctx: AppContext }> = ({ ctx }) => {
return (
<StyledApp>
<CatchError>
<DndProvider backend={HTML5Backend}>
<AppContextProvider value={ctx}>
<ThemeProvider>
<AppInitializer />
</ThemeProvider>

We already had a problem like this with FailedApp which is rendered when an error is caught during the boot procedure of the app. We fixed that problem by using StaticThemeProvider within FailedApp (#29046). That provider always returns the dark theme.

This PR moves FailedApp to a separate file so that it can be used in both CatchError and boot.tsx. It then reuses FailedApp in CatchError so that the theme is provided when rendering a caught error. It also adds regression tests to make sure that CatchError and FailedApp are rendered properly when ThemeProvider is not available.

@ravicious ravicious added backport/branch/v13 no-changelog Indicates that a PR does not require a changelog entry labels Jan 22, 2024
@ravicious ravicious requested a review from gzdunek January 22, 2024 11:04
render() {
if (this.state.error) {
return (
<Failed alignSelf={'baseline'} message={this.state.error.message} />
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FailedApp is equivalent to Failed, but it also adds StyledApp and StaticThemeProvider.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, actually that's bad because in case of an error we render StyledApp twice (one time in App and one time in CatchError). I'm just going to move CatchError to the top of the hierarchy in App then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also convert this file to TS while we are there?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd rather spend time elsewhere since this is used only in a single place. I tried to quickly do mv web/packages/teleterm/src/ui/components/CatchError/CatchError.{j,t}sx, but there are issues with how children are typed in Props.

@ravicious ravicious added this pull request to the merge queue Jan 22, 2024
Merged via the queue into master with commit 746b573 Jan 22, 2024
@ravicious ravicious deleted the r7s/catch-error-theme branch January 22, 2024 17:16
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/md ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants