Skip to content

Commit

Permalink
fix: Ensure ReactPanelErrorBoundary handles undefined children (#1089) (
Browse files Browse the repository at this point in the history
#1092)

- ReactPanelErrorBoundary would throw an error if children was undefined
- Just render `null` if `children` is `undefined`
- Tested using the steps in DH-18461. Unable to figure out steps to
reproduce in just DHC.
- There may still be a fix higher up in Enterprise, but this at least
ensures ReactPanelErrorBoundary is more robust and we don't totally
euchre our layout
  • Loading branch information
mofojed authored Jan 21, 2025
1 parent 8d7b60c commit 2c31622
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 2 deletions.
2 changes: 1 addition & 1 deletion plugins/ui/src/js/src/layout/ReactPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ function ReactPanel({
* Don't render the children if there's an error with the widget. If there's an error with the widget, we can assume the children won't render properly,
* but we still want the panels to appear so things don't disappear/jump around.
*/}
{renderedChildren}
{renderedChildren ?? null}
</ReactPanelErrorBoundary>
</Flex>
</View>
Expand Down
132 changes: 132 additions & 0 deletions plugins/ui/src/js/src/layout/ReactPanelErrorBoundary.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
import React from 'react';
import { TestUtils } from '@deephaven/test-utils';
import { render, screen } from '@testing-library/react';
import { ReactPanelErrorBoundary } from './ReactPanelErrorBoundary';

// Mock the WidgetErrorView component
jest.mock('../widget/WidgetErrorView', () => ({
__esModule: true,
default: function MockWidgetErrorView({ error }: { error: Error }) {
return <div data-testid="mock-error-view">{error.message}</div>;
},
}));

describe('ReactPanelErrorBoundary', () => {
// Suppress console.error for our intentional errors
beforeAll(() => {
TestUtils.disableConsoleOutput();
});

afterAll(() => {
jest.restoreAllMocks();
});

it('renders children when there is no error', () => {
render(
<ReactPanelErrorBoundary>
<div data-testid="test-child">Test Content</div>
</ReactPanelErrorBoundary>
);

expect(screen.getByTestId('test-child')).toBeInTheDocument();
expect(screen.getByText('Test Content')).toBeInTheDocument();
expect(screen.queryByTestId('mock-error-view')).not.toBeInTheDocument();
});

it('renders error view when child throws error', () => {
const ErrorComponent = () => {
throw new Error('Test error message');
};

render(
<ReactPanelErrorBoundary>
<ErrorComponent />
</ReactPanelErrorBoundary>
);

expect(screen.getByTestId('mock-error-view')).toBeInTheDocument();
expect(screen.getByText('Test error message')).toBeInTheDocument();
});

it('recovers when children are updated after error', () => {
const ErrorComponent = () => {
throw new Error('Test error message');
};

const { rerender } = render(
<ReactPanelErrorBoundary>
<ErrorComponent />
</ReactPanelErrorBoundary>
);

// Verify error state
expect(screen.getByTestId('mock-error-view')).toBeInTheDocument();
expect(screen.getByText('Test error message')).toBeInTheDocument();

// Update with working component
rerender(
<ReactPanelErrorBoundary>
<div data-testid="working-component">Working Content</div>
</ReactPanelErrorBoundary>
);

// Verify recovery
expect(screen.getByTestId('working-component')).toBeInTheDocument();
expect(screen.getByText('Working Content')).toBeInTheDocument();
expect(screen.queryByTestId('mock-error-view')).not.toBeInTheDocument();
});

it('maintains error state when props update does not include children change', () => {
const ErrorComponent = () => {
throw new Error('Test error message');
};

const { rerender } = render(
<ReactPanelErrorBoundary>
<ErrorComponent />
</ReactPanelErrorBoundary>
);

// Verify initial error state
expect(screen.getByTestId('mock-error-view')).toBeInTheDocument();

// Rerender with same children
rerender(
<ReactPanelErrorBoundary>
<ErrorComponent />
</ReactPanelErrorBoundary>
);

// Error view should still be present
expect(screen.getByTestId('mock-error-view')).toBeInTheDocument();
expect(screen.getByText('Test error message')).toBeInTheDocument();
});

it('calls componentDidCatch when error occurs', () => {
const errorSpy = jest.spyOn(
ReactPanelErrorBoundary.prototype,
'componentDidCatch'
);
const ErrorComponent = () => {
throw new Error('Test error message');
};

render(
<ReactPanelErrorBoundary>
<ErrorComponent />
</ReactPanelErrorBoundary>
);

expect(errorSpy).toHaveBeenCalled();
expect(errorSpy.mock.calls[0][0]).toBeInstanceOf(Error);
expect(errorSpy.mock.calls[0][0].message).toBe('Test error message');

errorSpy.mockRestore();
});

it('does not throw an error when children are undefined', () => {
expect(() =>
render(<ReactPanelErrorBoundary>{undefined}</ReactPanelErrorBoundary>)
).not.toThrow();
});
});
4 changes: 3 additions & 1 deletion plugins/ui/src/js/src/layout/ReactPanelErrorBoundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ export class ReactPanelErrorBoundary extends Component<
render(): ReactNode {
const { children } = this.props;
const { error } = this.state;
return error != null ? <WidgetErrorView error={error} /> : children;
// We need to check for undefined children because React will throw an error if we return undefined from a render method
// Note this behaviour was changed in React 18: https://github.com/reactwg/react-18/discussions/75
return error != null ? <WidgetErrorView error={error} /> : children ?? null;
}
}

Expand Down

0 comments on commit 2c31622

Please sign in to comment.