fix(ui): auto-recover from ChunkLoadError after deployment#15944
fix(ui): auto-recover from ChunkLoadError after deployment#15944nancysangani wants to merge 1 commit intoargoproj:mainfrom
Conversation
nancysangani
force-pushed
the
fix/chunk-loading-errors-15640
branch
from
7689ebb to
23c2d7a
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce end-user impact of post-deployment “Loading chunk X failed” errors by adding (1) a retryable lazy-import helper and (2) a chunk-load-specific error boundary that reloads the page when a chunk fails to load.
Changes:
- Added a
lazyImporthelper with retry/backoff behavior for failed dynamic imports. - Added
ChunkLoadErrorBoundaryintended to auto-reload the app on chunk-load failures, and wired it intoApp. - Added Jest/RTL tests for the new utility and boundary.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/shared/utils/lazy-import.ts | New retry wrapper for dynamic imports (currently not used by any production code paths). |
| ui/src/shared/utils/lazy-import.test.ts | Unit tests for retry behavior (timer/microtask ordering concerns). |
| ui/src/app/chunk-load-error-boundary.tsx | New error boundary that reloads on chunk-load-like errors (needs safeguards and better composition). |
| ui/src/app/chunk-load-error-boundary.test.tsx | Tests for boundary behavior (JSDOM window.location mocking + timer handling issues). |
| ui/src/app.tsx | Wraps the app in the new boundary (but existing inner ErrorBoundary likely intercepts first). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import {ComponentType} from 'react'; | ||
|
|
||
| /** | ||
| * Lazy import wrapper that retries loading chunks on failure. | ||
| * Fixes "Loading chunk X failed" errors by attempting to reload the chunk. | ||
| * See: https://github.com/argoproj/argo-workflows/issues/15640 | ||
| */ | ||
| export function lazyImport<T extends ComponentType<any>>( | ||
| importFunc: () => Promise<{default: T}>, | ||
| retries: number = 3, | ||
| delayMs: number = 1000 | ||
| ): Promise<{default: T}> { | ||
| let attemptCount = 0; | ||
|
|
||
| const attempt = (): Promise<{default: T}> => { | ||
| attemptCount++; | ||
| return importFunc().catch(error => { | ||
| if (attemptCount >= retries + 1) { | ||
| throw error; | ||
| } | ||
| return new Promise(resolve => setTimeout(() => resolve(attempt()), delayMs * attemptCount)); | ||
| }); | ||
| }; | ||
|
|
||
| return attempt(); | ||
| } |
There was a problem hiding this comment.
lazyImport is currently unused in the UI codebase (only referenced by its own test). As-is, this PR adds retry logic but doesn’t apply it to any existing React.lazy(() => import(...)) call sites, so it won’t affect runtime chunk loading. Consider wiring this into the existing lazy imports or removing it until it’s actually used.
| import {ComponentType} from 'react'; | |
| /** | |
| * Lazy import wrapper that retries loading chunks on failure. | |
| * Fixes "Loading chunk X failed" errors by attempting to reload the chunk. | |
| * See: https://github.com/argoproj/argo-workflows/issues/15640 | |
| */ | |
| export function lazyImport<T extends ComponentType<any>>( | |
| importFunc: () => Promise<{default: T}>, | |
| retries: number = 3, | |
| delayMs: number = 1000 | |
| ): Promise<{default: T}> { | |
| let attemptCount = 0; | |
| const attempt = (): Promise<{default: T}> => { | |
| attemptCount++; | |
| return importFunc().catch(error => { | |
| if (attemptCount >= retries + 1) { | |
| throw error; | |
| } | |
| return new Promise(resolve => setTimeout(() => resolve(attempt()), delayMs * attemptCount)); | |
| }); | |
| }; | |
| return attempt(); | |
| } | |
| export {}; |
| <ChunkLoadErrorBoundary> | ||
| <Provider value={providerContext}> | ||
| <AppRouter history={history} notificationsManager={notificationsManager} popupManager={popupManager} /> | ||
| </Provider> | ||
| </ChunkLoadErrorBoundary> |
There was a problem hiding this comment.
Wrapping the app in ChunkLoadErrorBoundary here likely won’t catch chunk-load failures in practice, because AppRouter already wraps the route Switch with the existing ErrorBoundary (which will intercept render errors first). As a result, chunk-load errors will still render the ErrorPanel instead of triggering the auto-reload. Consider moving ChunkLoadErrorBoundary inside AppRouter (around the Switch) or integrating chunk-load handling into the existing ErrorBoundary so it actually sees these errors.
| return importFunc().catch(error => { | ||
| if (attemptCount >= retries + 1) { | ||
| throw error; | ||
| } | ||
| return new Promise(resolve => setTimeout(() => resolve(attempt()), delayMs * attemptCount)); |
There was a problem hiding this comment.
The retry loop retries on any import failure. That will delay surfacing real bugs (e.g., syntax/runtime errors in the imported module) and can cause repeated side effects if importFunc isn’t a pure dynamic import. Consider only retrying when the error matches a chunk-load/network transient (or accept a predicate) and immediately rethrow for other errors.
| // Only handle chunk load errors; re-throw others | ||
| if (ChunkLoadErrorBoundary.isChunkLoadError(error)) { | ||
| return {error, isReloading: true}; | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
Throwing from getDerivedStateFromError for non-chunk errors is not a supported way to “bubble” errors to a parent boundary and can behave inconsistently across React versions. Prefer composing boundaries (e.g., nesting this inside the existing ErrorBoundary) or handling non-chunk errors with a fallback instead of throwing here.
| const reloadSpy = jest.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| Object.defineProperty(window, 'location', { | ||
| writable: true, | ||
| value: {reload: reloadSpy} | ||
| }); | ||
| reloadSpy.mockClear(); |
There was a problem hiding this comment.
Redefining window.location like this is likely to throw in JSDOM (the location property is typically non-configurable). Prefer spying/mocking window.location.reload directly (without replacing window.location).
| const reloadSpy = jest.fn(); | |
| beforeEach(() => { | |
| Object.defineProperty(window, 'location', { | |
| writable: true, | |
| value: {reload: reloadSpy} | |
| }); | |
| reloadSpy.mockClear(); | |
| let reloadSpy: jest.SpyInstance; | |
| beforeEach(() => { | |
| reloadSpy = jest.spyOn(window.location, 'reload').mockImplementation(() => {}); |
| expect(reloadSpy).toHaveBeenCalledTimes(1); | ||
| expect(screen.getByText(/Reloading/i)).toBeTruthy(); |
There was a problem hiding this comment.
The implementation calls reload() inside setTimeout(..., 100), so asserting reloadSpy was called immediately after render(...) will fail unless timers are faked/advanced (and updates flushed). Advance timers before this assertion (wrap in act if needed).
nancysangani
force-pushed
the
fix/chunk-loading-errors-15640
branch
4 times, most recently
from
1b3c171 to
e47bd42
Compare
|
/cc @Joibel |
nancysangani
force-pushed
the
fix/chunk-loading-errors-15640
branch
from
e47bd42 to
58d0f65
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant App as App Component
participant Boundary as ChunkLoadErrorBoundary
participant Module as Dynamic Module
participant Reload as window.location
User->>App: Load application
App->>Boundary: Render with children
Boundary->>Module: Attempt to load chunk
Module-->>Boundary: Loading chunk 775 failed (error)
Boundary->>Boundary: isChunkLoadError() checks error
Boundary->>Boundary: getDerivedStateFromError() sets isReloading=true
Boundary->>Boundary: componentDidCatch() logs error
Boundary->>Boundary: Schedule reload after 100ms
Boundary->>User: Render "Reloading..." UI
Boundary->>Reload: window.location.reload()
Reload->>User: Page reloads
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
ui/src/shared/utils/lazy-import.test.ts (1)
7-13: Cover the retry and final-failure paths.Line 8 only verifies the success path, but the PR’s main behavior is retrying failed dynamic imports. Add fake-timer tests that assert
importFuncis retried after failures and rejects after the configured retry budget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/shared/utils/lazy-import.test.ts` around lines 7 - 13, Add tests for lazyImport's retry and final-failure paths: write two fake-timer tests using jest.useFakeTimers() that mock the dynamic import function (importFunc) to first reject a number of times then resolve, and assert lazyImport retries by checking mock call count after advancing timers (use jest.advanceTimersByTime or runAllTimers) and that the returned value is eventually the module when within the retry budget; also add a test where importFunc always rejects and assert lazyImport rejects after the configured retry budget (check thrown error and final call count matches maxRetries), referencing lazyImport and the import function mock to locate the logic and ensuring timers are advanced between retry intervals to trigger retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/app.tsx`:
- Around line 9-10: The import order in app.tsx is wrong: move the
ChunkLoadErrorBoundary import so it comes after the AppRouter import to match
project ordering rules; specifically update the import sequence so AppRouter is
imported before ChunkLoadErrorBoundary (references: AppRouter and
ChunkLoadErrorBoundary in app.tsx) and save formatting so CI passes.
- Around line 25-30: Remove the outer ChunkLoadErrorBoundary that currently
wraps Provider and AppRouter in app.tsx and instead wrap only the route Switch
inside the existing ErrorBoundary in app-router.tsx: update app.tsx to render
Provider -> AppRouter (no ChunkLoadErrorBoundary) and update app-router.tsx to
place ChunkLoadErrorBoundary directly around the Switch (inside the existing
ErrorBoundary) so that ChunkLoadErrorBoundary catches lazy chunk load failures
first while the outer ErrorBoundary continues to handle non-chunk errors.
In `@ui/src/app/chunk-load-error-boundary.test.tsx`:
- Around line 19-28: The test triggers ChunkLoadErrorBoundary's error handler
which schedules window.location.reload via setTimeout; to avoid side effects use
jest.useFakeTimers() at the start of the test, mock or spy on
window.location.reload (e.g., jest.spyOn(window, 'location', 'get') or replace
reload with a jest.fn()), render the ThrowError component inside
ChunkLoadErrorBoundary, advance timers with jest.advanceTimersByTime(100) (or
jest.runAllTimers()) and then assert the mocked reload was called, and finally
restore timers and the mock to avoid affecting other tests.
In `@ui/src/app/chunk-load-error-boundary.tsx`:
- Around line 16-43: The chunk-load detection is too broad and the unconditional
reload in componentDidCatch causes potential 100ms refresh loops; narrow
isChunkLoadError to only detect chunk/dynamic-import errors (e.g., check
error.name === 'ChunkLoadError' and message patterns like 'Loading chunk' or
'dynamic import' but remove generic 'Failed to fetch'/'NetworkError'), and
change retry logic to be one-shot using state/flag: update
getDerivedStateFromError to set a retry flag (e.g., isReloading or hasRetried)
only when a chunk error is detected, and in componentDidCatch only call
window.location.reload() if the retry flag is false then set hasRetried=true
(persisted in component state or sessionStorage) so subsequent errors render a
fallback UI instead of reloading repeatedly; ensure getDerivedStateFromError
re-throws non-chunk errors as before.
In `@ui/src/shared/utils/lazy-import.ts`:
- Around line 8-12: The function signature for lazyImport is not formatted to
the project's formatter output; update the declaration of lazyImport so its
entire signature is on a single line (including generic, parameters importFunc,
retries, delayMs and return type Promise<{default: T}>), matching the
CI-reported formatter shape; locate the lazyImport function and replace the
current multi-line signature with the formatter-aligned one-line signature and
commit the change.
- Around line 3-6: Import the lazyImport helper from
ui/src/shared/utils/lazy-import and use it to wrap the dynamic import factories
used with React.lazy at each lazy-loading call site: replace React.lazy(() =>
import('...')) with React.lazy(lazyImport(() => import('...'))) in the lazy
React imports inside full-height-logs-viewer (FullHeightLogsViewer),
suspense-monaco-editor (SuspenseMonacoEditor), suspense-react-markdown-gfm
(SuspenseReactMarkdownGfm) and report-container (ReportContainer) so the retry
logic in lazyImport is actually invoked when chunks fail to load.
---
Nitpick comments:
In `@ui/src/shared/utils/lazy-import.test.ts`:
- Around line 7-13: Add tests for lazyImport's retry and final-failure paths:
write two fake-timer tests using jest.useFakeTimers() that mock the dynamic
import function (importFunc) to first reject a number of times then resolve, and
assert lazyImport retries by checking mock call count after advancing timers
(use jest.advanceTimersByTime or runAllTimers) and that the returned value is
eventually the module when within the retry budget; also add a test where
importFunc always rejects and assert lazyImport rejects after the configured
retry budget (check thrown error and final call count matches maxRetries),
referencing lazyImport and the import function mock to locate the logic and
ensuring timers are advanced between retry intervals to trigger retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85865b81-8223-48d2-a173-97a31c6d0f63
📒 Files selected for processing (5)
ui/src/app.tsxui/src/app/chunk-load-error-boundary.test.tsxui/src/app/chunk-load-error-boundary.tsxui/src/shared/utils/lazy-import.test.tsui/src/shared/utils/lazy-import.ts
Signed-off-by: Nancy <9d.24.nancy.sangani@gmail.com>
nancysangani
force-pushed
the
fix/chunk-loading-errors-15640
branch
from
58d0f65 to
e9eb250
Compare
Summary
Fixes "Loading chunk X failed" errors by adding retry logic and error boundary for chunk loading failures.
Changes
lazyImportutility with retry mechanismChunkLoadErrorBoundarythat auto-reloads on chunk errorsFixes
Closes #15640
Summary by CodeRabbit
New Features
Bug Fixes