perf(web): implement route-level code splitting via lazy routes (LUM-1900)#32079
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 072b4455fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
072b445 to
ca89b34
Compare
There was a problem hiding this comment.
✦ REQUEST_CHANGES
Value: Every user navigating away from chat (settings, logs, account) will see faster initial loads and smaller JS payloads — the 49% bundle reduction (4.1 MB → 2.1 MB) is real and measurable.
What this does: Converts all non-critical routes from static imports to React Router v7's object-based lazy property, creating per-route Vite chunks. Extracts ConversationRedirect and HomePageRoute into their own files so routes.tsx itself doesn't drag their deps into the critical bundle. Adds chunk-load-error resilience in main.tsx.
Blocker — Infinite reload loop in main.tsx
Devin caught this and I agree it's a genuine bug. The onError handler calls window.location.reload() unconditionally when a chunk load error is detected:
if (isChunkLoadError(error)) {
window.location.reload(); // no exit condition
return;
}If the chunk is persistently unavailable (network outage, CDN misconfiguration, deployment gap where old HTML references chunks that haven't propagated), this creates an infinite reload loop. The user cannot recover — the page reloads before any UI renders. The standard mitigation is a sessionStorage counter:
if (isChunkLoadError(error)) {
const key = "vellum:chunk-reload-count";
const count = Number(sessionStorage.getItem(key) || 0);
if (count < 2) {
sessionStorage.setItem(key, String(count + 1));
window.location.reload();
return;
}
// Exhausted retries — fall through to console.error so Sentry captures it
sessionStorage.removeItem(key);
}
console.error("[RouterProvider]", error);The counter clears itself on success (next page load after a good deploy clears session state naturally). Cap of 2 is standard; 1-3 are all reasonable.
P2 — Null/primitive safety in isChunkLoadError
Codex flagged this correctly. RouterProvider.onError accepts unknown, so a route component can throw null, a string, or a number. The current code:
const name = (error as { name?: string }).name;...will not throw on null in most JS engines (property access on null throws), but the TypeScript cast is misleading and the behavior is unsafe. Add an object guard:
const name = error != null && typeof error === "object"
? (error as { name?: string }).name
: undefined;This is a one-liner and makes the function side-effect-free for all thrown values.
What's solid
- Eager/lazy split is correct.
RootLayout,ChatLayout,ChatPage,DocumentViewerPage,ConversationRedirect,ActiveAssistantGate— all stay in the critical bundle. Everything else is lazy. No over-eagering, no mis-classification. - Middleware stays safe.
authMiddlewareis on the eager/assistantparent route. Middleware is inUnsupportedLazyRouteObjectKey— it can't accidentally end up in a lazy object. - Outlet context propagates correctly.
RootLayoutis eager and provides outlet context. Lazy children receive it normally via React Router's outlet mechanism. - Capacitor/iOS is unaffected. WKWebView loads from a local file URL via Capacitor — dynamic
import()splits work identically in that context. - Extracted component files are necessary.
ConversationRedirectandHomePageRouteneeded to move out of routes.tsx to actually be tree-shaken; they had deps (Zustand stores, navigation hooks) that would've stayed in the critical bundle otherwise. Correct call. - CI is green — Lint, Type Check, Build, Test all pass.
- Docs are thorough and accurate. The CONVENTIONS.md addition and AGENTS.md bullet give future contributors a clear target to hit.
Merge gate
Two fixes needed, both in main.tsx, same function:
- Add sessionStorage reload counter (Devin's finding — blocker)
- Add null/object guard in
isChunkLoadError(Codex P2 — easy one-liner)
Vellum Constitution — Trust-seeking: the chunk reload guard makes the app's failure recovery bounded and predictable rather than a silent infinite spin that users can't escape.
|
Both issues addressed in
|
ca89b34 to
fd20d43
Compare
Local Test Results — Lazy Route Code SplittingTested locally with full Production build verification: Main bundle 4,131 KB → 2,110 KB (49% reduction), 49 separate lazy route chunks produced. 5/5 tests passed
Environment & methodology
|
There was a problem hiding this comment.
✦ APPROVE (re-review — prior REQUEST_CHANGES resolved)
Value: Users navigating to settings, logs, account, or any non-chat route get faster initial boot — 60 routes' worth of component code is now deferred until actually needed.
Both prior issues verified fixed at HEAD (fd20d43c1b):
Fix #1 — infinite reload loop: The sessionStorage counter is in place and working correctly:
const key = "vellum:chunk-reload-count";
const count = Number(sessionStorage.getItem(key) || 0);
if (count < 2) {
sessionStorage.setItem(key, String(count + 1));
window.location.reload();
return;
}
sessionStorage.removeItem(key);Caps at 2 retries, clears the counter on exhaustion so a user who refreshes manually after a persistent failure gets a fresh budget. ✅
Fix #2 — isChunkLoadError null guard: Guard is present and in the right order:
if (typeof error !== "object" || error == null) return false;
const name = (error as { name?: string }).name;Non-object/null thrown values bail early before the property access. ✅
Codex P2 at HEAD — false positive
Codex flagged "guard unknown router errors before reading .name" at fd20d43c1b line 21. The guard is already there in the same commit. Devin confirmed in their inline reply. Not blocking.
Other observations (non-blocking)
- Object-based
lazy: { Component: () => import(...).then(m => m.Foo) }is the correct React Router v7 data-mode granular form — distinct from the function formlazy: async () => ({ Component: ... }). Both are valid; the object form is slightly more precise. ✅ Component: RootLayout(no JSX instantiation) in place ofelement: <RootLayout />for eager routes is the correct migration — they're mutually exclusive andComponentavoids needless JSX in the route tree definition. ✅- Auth middleware sits on the
/assistantparent route. Lazy-loading its children doesn't affect middleware application — the middleware guard fires before any child renders. ✅ useViewerStore.getState()anduseConversationStore.getState()inHomePageRoute'sonSuggestionSelectedcallback is the correct pattern for Zustand inside event handlers (imperative access, no subscription). ✅- AGENTS.md + CONVENTIONS.md docs are clear and actionable.
Merge gate: CI all green ✅. PR is currently dirty (merge conflict) — needs a rebase before merge.
Vellum Constitution — Trust-seeking: deferred chunk loading means the app's cold-start time doesn't grow linearly with feature count — users keep experiencing a fast, responsive first paint.
…1900) Apply React Router v7's object-based lazy property to all non-critical route groups. Vite creates separate chunks per dynamic import(), so each lazy route loads only when first navigated to. Main bundle reduced from 4,131 KB to 2,090 KB (49% smaller). Eager (critical path): RootLayout, ChatLayout, ChatPage, DocumentViewerPage, ConversationRedirect, ActiveAssistantGate, NotFound. Lazy (deferred): settings (18 pages), logs (4 pages), account/auth (8 pages), onboarding (3 screens), intelligence pages, library, inspector, home, connect. Add RouterProvider.onError handler in main.tsx to catch chunk load failures (stale deploys, network errors) and trigger a page reload. Update CONVENTIONS.md and AGENTS.md with code splitting conventions. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Add sessionStorage counter capping reload attempts at 2. After exhausting retries, fall through to console.error so the app remains usable during persistent chunk failures (network outage, CDN issue). Also guard isChunkLoadError against non-object thrown values — onError receives unknown, so null/undefined must not reach the property access. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
dc8c944
fd20d43 to
dc8c944
Compare
…orBoundary Remove isChunkLoadError heuristic, sessionStorage reload counter, and control-flow logic from RouterProvider.onError. These are replaced by React Router v7's first-class ErrorBoundary route property — the documented, recommended way to handle route errors in data mode. Changes: - Add RootErrorBoundary component using useRouteError() / isRouteErrorResponse() - Add ErrorBoundary: RootErrorBoundary to all root route objects (/account, /assistant, /logout, top-level catch-all) - Simplify onError to logging-only (its intended purpose per the docs) References: - https://reactrouter.com/how-to/error-boundary - https://reactrouter.com/api/data-routers/RouterProvider Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Local Test Results (post-Sentry integration)Ran
Changes since last test run:
CI: 7/7 green |
Prompt / plan
Part 2 of LUM-1900. Applies route-level code splitting using React Router v7's object-based
lazyproperty. Depends on #32075 landing first (which convertselement→Component).Closes https://linear.app/vellum/issue/LUM-1900
What changed
Route-level code splitting via
lazy(routes.tsx)All non-critical route groups now use the object-based
lazyproperty. When a route haslazy: { Component: () => import(...) }, Vite treats theimport()as a dynamic chunk boundary and creates a separate JS file for each route module. The router resolves each lazy property before transitioning, so the previous route stays visible while the new chunk downloads — no flash of loading state.Before: 60 static imports at the top of
routes.tsx→ entire app in a single 4,131 KBindex-*.jschunk.After: 10 eager imports (critical path) + ~50 lazy routes → main bundle is 2,090 KB (49% reduction). Each lazy route gets its own chunk (e.g.,
billing-page-*.js,settings-layout-*.js,inspect-page-*.js).RootLayout,ChatLayout,ChatPage,DocumentViewerPageConversationRedirect,ActiveAssistantGate,NotFoundRoute-level error boundaries (
routes.tsx+root-error-boundary.tsx)Added
ErrorBoundary: RootErrorBoundaryto all root route objects (/account,/assistant,/logout, top-level catch-all). This is React Router v7's first-class error handling mechanism for data mode — theErrorBoundaryroute property renders a component when any error occurs during that route's lifecycle, including lazy chunk load failures.The
RootErrorBoundarycomponent usesuseRouteError()andisRouteErrorResponse()to distinguish between HTTP error responses (404s) and unexpected errors (chunk failures, render crashes). It renders a user-friendly recovery UI with a "Reload" button.RouterProvider.onErroris kept as a reporting-only callback — its documented purpose is error reporting, not error recovery. It reports errors to Sentry viaSentry.captureException(), integrating with the existing Sentry setup (consent-gated, initialized insentry-init.ts). The previous implementation misusedonErrorfor control flow (auto-reloading on chunk errors with asessionStoragecounter), which is not what the API is designed for.What was removed and why:
isChunkLoadError()— custom heuristic that pattern-matched on browser-specific error messages. Fragile and unnecessary; theErrorBoundarycatches all route errors including chunk failures without needing to identify them.sessionStoragereload counter — invisible auto-reload loop that confuses users. Replaced by an explicit "Reload" button the user controls.Documentation updates
docs/CONVENTIONS.md: Added "Route-level code splitting" section with eager/lazy classification, code examples, and references to React Router docs.AGENTS.md: Added code splitting bullet to the Routing section linking to the conventions doc.routes.tsxheader comment: Added error boundary reference link.Both docs are open-source friendly — no internal references, link-heavy with authoritative sources.
Why this approach
React Router v7 (7.15.0, which we're on) supports two
lazysyntaxes:lazy: async () => ({ Component, loader, ... })— loads everything in one calllazy: { Component: () => import(...).then(m => m.X) }— each property loaded independentlyWe use the object-based syntax because:
LazyRouteDefinitiontype in 7.15.0 accepts both syntaxes (LazyRouteObject | LazyRouteFunction)For error handling, the
ErrorBoundaryroute property is React Router v7's recommended approach — "All applications should at a minimum export a root error boundary." This replaces the previous hackyonError-based auto-reload with the framework's built-in error recovery mechanism.Why this is safe
RootLayoutstays eager and provides outlet context. Lazy children receive context normally — the router propagates outlet context regardless of lazy loading.authMiddlewarelives on the eager/assistantparent route.middlewareis explicitly excluded fromLazyRouteObjectkeys (UnsupportedLazyRouteObjectKey) — it always runs eagerly./assistant/settings/general), the router fetches the chunk before rendering. Brief delay is acceptable since most users land on chat (eager).RootLayoutrenders immediately.ErrorBoundaryand render a recoverable UI. No silent auto-reload loops.onErrorreports to Sentry for observability.Alternatives not taken
React.lazy()wrapperslazy. Requires manualSuspenseboundaries at each split point. Doesn't integrate with the router's navigation lifecycle (old route stays visible while loading). Routerlazyis strictly better.codeSplitting(Rolldown)minSizehas no effect withoutgroups.lazyonErrorfor chunk error recoveryRouterProvider.onErroris designed for logging/reporting, not control flow. Using it to auto-reload is an undocumented hack. Route-levelErrorBoundaryis the correct mechanism.isChunkLoadErrorheuristicErrorBoundarycatches all route errors without needing to identify specific error types.console.errorinonErrorcaptureExceptionprovides structured error reporting, deduplication, and alerting.References
lazyComponentRouterProviderProps.onErrorTest plan
bunx tsc --noEmit— passesbun run lint— passesbun run build— produces separate chunks per route group; main bundle 2,090 KB (down from 4,131 KB)Link to Devin session: https://app.devin.ai/sessions/28ef481509de49b0bee7d7ed2768d370
Requested by: @ashleeradka