Skip to content

Add error boundary logic#210

Merged
markmiro merged 2 commits into
runtimed:mainfrom
markmiro:error-boundaries
Jul 11, 2025
Merged

Add error boundary logic#210
markmiro merged 2 commits into
runtimed:mainfrom
markmiro:error-boundaries

Conversation

@markmiro

@markmiro markmiro commented Jul 11, 2025

Copy link
Copy Markdown
Contributor
Screenshot 2025-07-11 at 11 29 47 AM

const renderOutputs = useCallback(() => {
// Note: this approach is not ideal, but it ensures that if this component throws, we can put an error boundary that works
// Otherwise, just calling `<ErrorBoundary FallbackComponent={OutputsErrorBoundary}>renderOutputs()</ErrorBoundary>` will not work as expected
const MaybeOutputs = useCallback(() => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If a component sometimes returns empty, I like to prefix them with Maybe . Normally, if we want to render a cell component, conditionally, we wouldn't do it inside the cell, and instead do something like this:

<div>{showCell && <Cell />}<div>

But if the Cell component owns its own state, and sometimes returns null, we should name it MaybeCell .

That way, if you render it, you're not surprised if it's sometimes not showing up:

<div><MaybeCell /></div>

This could also apply to nullable variables or types. Normally, you don't want null or undefined as part of a type, but when you do, it would be prefixed with "maybe".

{hasOutputs && cell.outputVisible && renderOutputs()}
<ErrorBoundary FallbackComponent={OutputsErrorBoundary}>
{cell.outputVisible && <MaybeOutputs />}
</ErrorBoundary>

@rgbkrk rgbkrk Jul 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might as well do this per output honestly. Sometimes the custom output types in notebooks have strangeness.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll probably do it in another PR

@markmiro
markmiro merged commit e967891 into runtimed:main Jul 11, 2025
1 check passed
@markmiro

Copy link
Copy Markdown
Contributor Author

Oops, merged individual commits too

@rgbkrk

rgbkrk commented Jul 11, 2025

Copy link
Copy Markdown
Member

It all squashes anyways, no prob

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