ui: add root and tab error boundaries with retry fallback#173
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Review Summary by QodoAdd root and tab-level error boundaries with retry fallback
WalkthroughsDescription• Add reusable ErrorBoundary component for renderer crash containment • Wrap root app and all tab panels with error boundaries and retry actions • Add localized fallback strings for EN/ES/FR/DE languages • Add comprehensive unit test coverage for ErrorBoundary behavior File Changes1. src/renderer/components/ErrorBoundary.tsx
|
Summary of ChangesHello @Mehdi-Bl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's resilience by introducing robust error handling mechanisms. It integrates a new, reusable Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable ErrorBoundary component to improve the application's robustness by containing renderer crashes. The implementation correctly wraps the root application and individual tabs, providing a fallback UI and a retry mechanism. The new component is well-designed and comes with good unit test coverage. The addition of localized strings for error messages is also a nice touch. My review includes one suggestion to improve the reliability of the retry mechanism for one of the tabs.
| fallbackTitle={tabFallbackTitle} | ||
| fallbackMessage={tabFallbackMessage} | ||
| resetLabel={retryLabel} | ||
| resetKeys={[activeTab, rootPath, selectedFiles.size, selectedFolders.size]} |
There was a problem hiding this comment.
Using selectedFiles.size and selectedFolders.size as reset keys for the error boundary is not robust. If a user replaces one file with another in the selection, the size might remain the same, but the content of the selection has changed. If the previous selection caused a crash, the error boundary would not reset on this new selection, preventing the component from recovering and re-rendering with the valid selection.
To fix this, the reset key should depend on the content of the selection sets, not just their size. A simple way to create a stable key from the sets is to convert them to a sorted string.
resetKeys={[activeTab, rootPath, [...selectedFiles].sort().join(','), [...selectedFolders].sort().join(',')]}
|
Code Review by Qodo
1. activeTab in resetKeys auto-retries crashed tabs
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| fallbackTitle={tabFallbackTitle} | ||
| fallbackMessage={tabFallbackMessage} | ||
| resetLabel={retryLabel} | ||
| resetKeys={[activeTab, configContent]} |
There was a problem hiding this comment.
1. Activetab in resetkeys auto-retries crashed tabs 🐞 Bug ✓ Correctness
All three tab error boundaries include activeTab as the first resetKey. Since all tabs stay permanently mounted (hidden via CSS), every tab switch changes activeTab for all three boundaries simultaneously. Any boundary in error state will auto-reset and re-render its still-broken child on every tab switch, causing repeated throws, noisy console output, and fallback UI flickering.
Agent Prompt
## Issue description
All three tab `ErrorBoundary` instances include `activeTab` as the first element of their `resetKeys`. Because all tabs remain permanently mounted (hidden via CSS), every tab switch changes `activeTab` for all three boundaries simultaneously. Any boundary currently in error state will auto-reset and immediately re-render its still-broken child, causing repeated throws, noisy `console.error` output, and fallback UI flickering on every tab switch.
## Issue Context
The tabs are kept mounted via a CSS `hidden` class (not unmounted), so all three `ErrorBoundary` instances are always live and receive prop updates. `haveResetKeysChanged` uses `Object.is` comparison — a tab switch from `'source'` to `'config'` is detected as a changed key for all three boundaries, triggering `setState({ hasError: false })` on any that are in error state.
## Fix Focus Areas
Remove `activeTab` from all three `resetKeys` arrays, keeping only the content-specific keys:
- `src/renderer/components/App.tsx[145-145]` → change to `resetKeys={[configContent]}`
- `src/renderer/components/App.tsx[161-161]` → change to `resetKeys={[rootPath, selectedFiles.size, selectedFolders.size]}`
- `src/renderer/components/App.tsx[190-190]` → change to `resetKeys={[processedResult?.content]}`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| componentDidUpdate(previousProps: ErrorBoundaryProps) { | ||
| if (!this.state.hasError) { | ||
| return; | ||
| } | ||
|
|
||
| if (haveResetKeysChanged(previousProps.resetKeys, this.props.resetKeys)) { | ||
| this.setState({ hasError: false }); | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Componentdidupdate missing previousstate guard 🐞 Bug ✓ Correctness
The componentDidUpdate override only accepts previousProps, omitting previousState. After getDerivedStateFromError transitions hasError to true, React commits the fallback UI and then calls componentDidUpdate with previousState.hasError === false. Without checking previousState.hasError, if any resetKey also changed in the same render cycle, the just-caught error is immediately cleared, the broken child re-renders, throws again, and the cycle repeats.
Agent Prompt
## Issue description
`componentDidUpdate` only receives `previousProps`, so there is no way to distinguish between "we just entered error state this cycle" and "we were already in error state from a previous cycle". The existing guard `if (!this.state.hasError) return` checks the *current* state (which is already `true` after `getDerivedStateFromError` ran), not the *previous* state. This means if any resetKey changed in the same render cycle the error was caught, the error is immediately cleared, the broken child re-renders, throws again, and the cycle repeats.
## Issue Context
React's lifecycle sequence when a child throws:
1. `getDerivedStateFromError` → sets `hasError: true` (previousState.hasError was `false`)
2. React renders fallback UI and commits to DOM
3. `componentDidUpdate(previousProps, previousState)` is called — `previousState.hasError === false`, `this.state.hasError === true`
The guard must check `previousState.hasError` to detect this transition and skip the reset logic during the initial error-catch cycle.
## Fix Focus Areas
- `src/renderer/components/ErrorBoundary.tsx[46-54]`
Change the method to:
```typescript
componentDidUpdate(previousProps: ErrorBoundaryProps, previousState: ErrorBoundaryState) {
// Only auto-reset if we were ALREADY in error state, not when we just entered it.
if (!previousState.hasError) {
return;
}
if (haveResetKeysChanged(previousProps.resetKeys, this.props.resetKeys)) {
this.setState({ hasError: false });
}
}
```
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
ErrorBoundarycomponent for renderer crash containmentconfig,source,processed) with tab-level boundaries and retry actionValidation
Backlog Item