bug: Change error handling to prevent UI crash #1588
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphView
participant Server
participant GraphViewErrorAlert
User->>GraphView: Render component
GraphView->>Server: Fetch graph data
Server-->>GraphView: Error response (e.g., 500)
GraphView->>GraphViewErrorAlert: Render error alert
GraphViewErrorAlert-->>User: Display error message
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
LucasParraF
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/GraphViewErrorAlert.tsx (1)
25-25: Consider making the error message more informative.The generic error message doesn't provide users with context about what went wrong. Consider making the component accept props to display more specific error information when available.
-const GraphViewErrorAlert = () => { +interface GraphViewErrorAlertProps { + message?: string; +} + +const GraphViewErrorAlert = ({ message }: GraphViewErrorAlertProps) => { const theme = useTheme(); return ( <Box display={'flex'} justifyContent={'center'} mt={theme.spacing(8)} mx={theme.spacing(4)}> <Alert severity={'error'}> <AlertTitle>Error</AlertTitle> - <p>An unexpected error has occurred. Please refresh the page and try again.</p> + <p>{message || 'An unexpected error has occurred. Please refresh the page and try again.'}</p> </Alert> </Box> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/ui/src/views/Explore/GraphView.test.tsx(1 hunks)cmd/ui/src/views/Explore/GraphView.tsx(3 hunks)packages/go/graphschema/ad/ad.go(0 hunks)packages/go/graphschema/azure/azure.go(0 hunks)packages/go/graphschema/common/common.go(0 hunks)packages/go/graphschema/graph.go(0 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/GraphViewErrorAlert.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/index.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/go/graphschema/azure/azure.go
- packages/go/graphschema/graph.go
- packages/go/graphschema/common/common.go
- packages/go/graphschema/ad/ad.go
🔇 Additional comments (4)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/GraphViewErrorAlert.tsx (1)
19-29: Well-implemented error alert component.The component correctly uses Material-UI components and theming for consistent styling. The centered layout and error severity are appropriate for this use case.
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/index.ts (1)
18-18: Export follows established patterns.The export is correctly placed and follows the existing naming conventions in the index file.
cmd/ui/src/views/Explore/GraphView.tsx (2)
23-23: Import correctly added for new error alert component.The import follows the existing pattern and is properly included with other bh-shared-ui imports.
120-120: Excellent fix for preventing UI crashes.Replacing the error throw with component rendering is the correct approach. This prevents unhandled exceptions from crashing the UI while providing users with a clear error message and recovery instructions.
| const server = setupServer( | ||
| rest.get('/api/v2/graphs/cypher', (req, res, ctx) => { | ||
| return res(ctx.status(500)); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Add server lifecycle management.
The mock server is defined but not properly managed. You need to start and stop the server in your test lifecycle.
const server = setupServer(
rest.get('/api/v2/graphs/cypher', (req, res, ctx) => {
return res(ctx.status(500));
})
);
+
+beforeAll(() => server.listen());
+afterEach(() => server.resetHandlers());
+afterAll(() => server.close());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const server = setupServer( | |
| rest.get('/api/v2/graphs/cypher', (req, res, ctx) => { | |
| return res(ctx.status(500)); | |
| }) | |
| ); | |
| const server = setupServer( | |
| rest.get('/api/v2/graphs/cypher', (req, res, ctx) => { | |
| return res(ctx.status(500)); | |
| }) | |
| ); | |
| beforeAll(() => server.listen()); | |
| afterEach(() => server.resetHandlers()); | |
| afterAll(() => server.close()); |
🤖 Prompt for AI Agents
In cmd/ui/src/views/Explore/GraphView.test.tsx around lines 22 to 26, the mock
server is set up but lacks lifecycle management. Add server start and stop calls
by invoking server.listen() before tests run, server.resetHandlers() after each
test, and server.close() after all tests complete to properly manage the server
lifecycle and avoid side effects.
| it('displays an error message', async () => { | ||
| server.use; | ||
| render(<GraphView />); | ||
| const errorAlert = await waitFor(() => | ||
| screen.findByText('An unexpected error has occurred. Please refresh the page and try again.') | ||
| ); | ||
|
|
||
| expect(errorAlert).toBeInTheDocument; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test structure and assertions.
The test logic could be clearer and the server usage needs correction.
it('displays an error message', async () => {
- server.use;
render(<GraphView />);
- const errorAlert = await waitFor(() =>
- screen.findByText('An unexpected error has occurred. Please refresh the page and try again.')
- );
+
+ const errorAlert = await screen.findByText(
+ 'An unexpected error has occurred. Please refresh the page and try again.'
+ );
- expect(errorAlert).toBeInTheDocument;
+ expect(errorAlert).toBeInTheDocument();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('displays an error message', async () => { | |
| server.use; | |
| render(<GraphView />); | |
| const errorAlert = await waitFor(() => | |
| screen.findByText('An unexpected error has occurred. Please refresh the page and try again.') | |
| ); | |
| expect(errorAlert).toBeInTheDocument; | |
| }); | |
| it('displays an error message', async () => { | |
| render(<GraphView />); | |
| const errorAlert = await screen.findByText( | |
| 'An unexpected error has occurred. Please refresh the page and try again.' | |
| ); | |
| expect(errorAlert).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
In cmd/ui/src/views/Explore/GraphView.test.tsx around lines 33 to 41, the test
incorrectly calls server.use without invoking it and uses toBeInTheDocument
without parentheses. Fix this by properly calling server.use with the required
handler and change expect(errorAlert).toBeInTheDocument to
expect(errorAlert).toBeInTheDocument() to correctly assert the presence of the
error message.
| beforeEach(() => server.listen()); | ||
| afterAll(() => server.close()); |
There was a problem hiding this comment.
These typically go outside of the describe block. We should remove these lines and add the ones that coderabbit posted in the comment right above and outside of the describe block like this:
const server = setupServer(
rest.get('/api/v2/graphs/cypher', (req, res, ctx) => {
return res(ctx.status(500));
})
);
beforeAll(() => server.listen());
afterEach(() => server.resetHandlers());
afterAll(() => server.close());
describe('GraphView', () => {
...
Description
BHE and BHCE were crashing when an error on the GraphView component appeared. For BHCE, Explore view used this component and was affected by this bug. In order to fix this, I refactored the error handle on the component so it would render a new component with an error alert to keep consistency on how were managing other errors
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-5948
How Has This Been Tested?
Created test for GraphView component
Screenshots (optional):
BHCE-Explore.mov
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests