code improvements and test coverage#1411
Conversation
Phase 1 (immediate fixes): - Fix Table useEffect missing dependency array (was re-running on every render) Phase 2 (code modernization): - Replace custom makeAsyncPage/useAsyncComponent with React.lazy + Suspense in routes.tsx - Add ErrorBoundary component wrapping the app root for unhandled render errors - Improve Modal accessibility with role="dialog", aria-modal, aria-labelledby, useId() Phase 3 (test coverage): - Add 14 new test files bringing total to 19 test files, 181 tests - Helpers: clamp, codeblock_to_blob, highlight_substring, localStorage, tag_colors, trim_url, use_form, use_modal, use_wired_data - Components: form, modal, paging, table - Services: request_helper
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const AsyncLogin = lazy(() => import('src/pages/login')) | ||
| const AsyncOperationList = lazy(() => import('src/pages/operation_list')) | ||
| const AsyncOperationEdit = lazy(() => import('src/pages/operation_edit')) | ||
| const AsyncFindingShow = lazy(() => import('src/pages/operation_show/finding_show')) | ||
| const AsyncEvidenceList = lazy(() => import('src/pages/operation_show/evidence_list')) | ||
| const AsyncFindingList = lazy(() => import('src/pages/operation_show/finding_list')) | ||
| const AsyncAdminSettings = lazy(() => import('src/pages/admin')) | ||
| const AsyncAccountSettings = lazy(() => import('src/pages/account_settings')) | ||
| const AsyncNotFound = lazy(() => import('src/pages/not_found')) |
There was a problem hiding this comment.
🟡 Lazy load failures crash entire app UI due to missing page-level error boundary
The migration from useAsyncComponent to React.lazy lost per-page error handling for chunk load failures. The old useAsyncComponent (frontend/src/helpers/use_async_component.tsx:54-56) caught import errors and rendered an inline <ErrorDisplay> within the page layout, preserving navigation. With React.lazy, a failed dynamic import (e.g., stale chunk after deployment, network error) throws an error that propagates up to the nearest ErrorBoundary, which is at the app root (frontend/src/index.tsx:34). This replaces the entire app—including the <Layout>, nav, and sidebar—with a bare <ErrorDisplay>, and since ErrorBoundary has no reset mechanism, the user is permanently stuck with no way to navigate away without a full browser refresh.
Component hierarchy showing the gap
ErrorBoundary (root — catches error here, replaces everything)
→ RootComponent
→ AuthContext.Provider
→ BrowserRouter
→ Layout (lost on error)
→ Suspense (only handles loading, not errors)
→ Routes → lazy components (error thrown here)
Prompt for agents
Add an ErrorBoundary inside the Layout component (in frontend/src/index.tsx) wrapping the Suspense/Routes so that lazy-load failures are contained to the content area and the user retains the ability to navigate. Specifically, in frontend/src/index.tsx around lines 19-22, wrap the Suspense in an ErrorBoundary:
<Layout>
<ErrorBoundary>
<Suspense fallback={<LoadingSpinner />}>
<Routes />
</Suspense>
</ErrorBoundary>
</Layout>
Also consider adding a reset mechanism to the ErrorBoundary component (frontend/src/components/error_boundary/index.tsx) — for example, a button that calls setState({ error: null }) or resetting when the location changes — so users can recover without a full page reload.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export default class ErrorBoundary extends Component<Props, State> { | ||
| state: State = { error: null } | ||
|
|
||
| static getDerivedStateFromError(error: Error): State { | ||
| return { error } | ||
| } | ||
|
|
||
| componentDidCatch(_error: Error, _info: ErrorInfo) { | ||
| // Errors are surfaced via the render method | ||
| } | ||
|
|
||
| render() { | ||
| if (this.state.error) { | ||
| return <ErrorDisplay err={this.state.error} /> | ||
| } | ||
| return this.props.children | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 ErrorBoundary has no recovery mechanism
The ErrorBoundary component sets state.error when an error is caught via getDerivedStateFromError, but provides no way to reset the state back to null. Once triggered, the user is permanently stuck on the <ErrorDisplay> with no navigation or retry button. A common pattern is to reset on location change (via componentDidUpdate checking a key prop or location from the router) or to provide a "Try Again" button that calls this.setState({ error: null }). Since this boundary wraps the entire app at the root, recovery is especially important.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <Suspense fallback={<LoadingSpinner />}> | ||
| <Routes /> | ||
| </Suspense> |
There was a problem hiding this comment.
📝 Info: Duplicate Suspense boundaries (index.tsx and routes.tsx) are redundant but harmless
The <Suspense> at frontend/src/index.tsx:20 wraps <Routes />, and the <Suspense> components inside routes.tsx (lines 45, 64) also wrap the <Routes> content. Since AppRoutes is not itself a lazy component, the outer Suspense in index.tsx will never actually trigger — the inner Suspense boundaries in routes.tsx are the ones that catch the lazy component suspensions. The outer one is effectively dead code. Consider removing it from index.tsx or removing the inner ones from routes.tsx to avoid confusion.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Phase 1 (immediate fixes):
Phase 2 (code modernization):
Phase 3 (test coverage):
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.